Skip to content

Conversation

ysmolski
Copy link
Contributor

@ysmolski ysmolski commented Oct 15, 2025

This includes only static validation.
Stream validation for "if" is not implemented yet. Right now
stream directives should match fully to be mergeable.

Runtime validation (for variables) will be implemented separately.

Summary by CodeRabbit

  • New Features

    • Added GraphQL validation for @defer/@stream: allowed per operation (queries allowed, mutations only on nested fields, subscriptions disallowed), unique static labels required, @stream only on list fields, non-negative initialCount, and detection of conflicting stream directives during field merging.
  • Improvements

    • More precise, user-friendly validation errors with accurate locations for directive misuse.
  • Chores

    • Added ignore rule for /.cursorrules.

Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds ignore for /.cursorrules. Expands comments in AST merge code. Introduces three new validation rules for @defer/@stream usage, uniqueness of labels, and list-only enforcement. Refactors field selection merging to include directive compatibility and improved type checks. Adds LABEL and INITIAL_COUNT literals. Adds new external error constructors and helper.

Changes

Cohort / File(s) Summary of changes
Repo config
/.gitignore
Add ignore pattern for /.cursorrules.
AST comments
v2/pkg/ast/ast_directive.go, v2/pkg/astnormalization/inline_fragment_selection_merging.go
Add clarifying comments; no functional changes.
New validation rules: defer/stream
v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go,
v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go,
v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go
Add rules enforcing operation-level placement, unique labels, label static checks, list-only application, and initialCount non-negative.
Field selection merging updates
v2/pkg/astvalidation/operation_rule_field_selection_merging.go
Rework non-scalar handling, add stream directive compatibility checks, defer deep type compatibility until after directive checks, track fieldRef, avoid duplicate requirements, remove empty EnterSelectionSet.
Tests for defer/stream
v2/pkg/astvalidation/operation_validation_test.go
Add rule scaffold and comprehensive tests for defer/stream scenarios.
Lexer literals
v2/pkg/lexer/literal/literal.go
Add exported literals LABEL and INITIAL_COUNT.
External errors for defer/stream
v2/pkg/operationreport/externalerror.go
Add new error constructors and helper; adjust unique-per-location logic to use ordered location aggregation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.32% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly identifies the main feature added by the pull request—static validation for defer and stream directives—and directly reflects the core changes described in the summary without extraneous detail. It is concise, clear, and informative for anyone scanning the project history.

Comment @coderabbitai help to get the list of available commands and usage tips.

@ysmolski
Copy link
Contributor Author

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
v2/pkg/astvalidation/operation_validation_test.go (1)

15-15: Remove unused placeholder rule to avoid future conflicts and dead import

DeferStreamComplexRule is unused and may collide with a real implementation later. It also forces the astvisitor import solely for the stub.

Apply:

@@
-    "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor"
@@
-// Placeholder rule functions for defer/stream validation - these need to be implemented
-func DeferStreamComplexRule() Rule {
-    // TODO: Implement complex validation
-    return func(walker *astvisitor.Walker) {
-        // Implementation needed
-    }
-}

Also applies to: 5614-5621

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a142a5 and aded752.

📒 Files selected for processing (10)
  • .gitignore (1 hunks)
  • v2/pkg/ast/ast_directive.go (1 hunks)
  • v2/pkg/astnormalization/inline_fragment_selection_merging.go (1 hunks)
  • v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go (1 hunks)
  • v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go (1 hunks)
  • v2/pkg/astvalidation/operation_rule_field_selection_merging.go (4 hunks)
  • v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go (1 hunks)
  • v2/pkg/astvalidation/operation_validation_test.go (7 hunks)
  • v2/pkg/lexer/literal/literal.go (1 hunks)
  • v2/pkg/operationreport/externalerror.go (2 hunks)
🔇 Additional comments (9)
.gitignore (1)

8-9: Looks good—nice addition.

Root-scoping .cursorrules keeps editor-specific config out of the repo without affecting nested paths.

v2/pkg/ast/ast_directive.go (1)

138-150: Clarified stream-compatibility semantics look good

Comment matches current behavior: both sides must have identical @stream or neither; mixed presence is incompatible. No functional change.

v2/pkg/astnormalization/inline_fragment_selection_merging.go (1)

73-75: Good clarification

Comment correctly reflects that for fields with selections, directive sets must be equal (covers @skip/@include/@defer/@stream).

v2/pkg/lexer/literal/literal.go (1)

70-72: New literals are appropriate

LABEL and INITIAL_COUNT align with defer/stream args; consistent with existing literal style.

v2/pkg/astvalidation/operation_rule_field_selection_merging.go (3)

12-27: Doc expansion improves intent

Helpful context and scope; no behavioral changes.


39-44: Store fieldRef for non-scalar requirements

Necessary to fetch left-side directives for compatibility checks. LGTM.

Also applies to: 165-171


176-186: Directive equality semantics are correct
With checkDirectivesEquality=false, FieldsAreEqualFlat invokes DirectiveSetsHasCompatibleStreamDirective, enforcing only stream‐directive compatibility and ignoring other directives.

v2/pkg/operationreport/externalerror.go (2)

161-164: New stream directive conflict error

Constructor and message look good.


423-463: New defer/stream error constructors

Messages and locations are consistent and useful. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant