-
Notifications
You must be signed in to change notification settings - Fork 155
feat: implement static validation for defer/stream #1322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/defer
Are you sure you want to change the base?
feat: implement static validation for defer/stream #1322
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds 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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this 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 importDeferStreamComplexRule 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
📒 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 goodComment 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 clarificationComment 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 appropriateLABEL 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 intentHelpful context and scope; no behavioral changes.
39-44
: Store fieldRef for non-scalar requirementsNecessary to fetch left-side directives for compatibility checks. LGTM.
Also applies to: 165-171
176-186
: Directive equality semantics are correct
WithcheckDirectivesEquality=false
,FieldsAreEqualFlat
invokesDirectiveSetsHasCompatibleStreamDirective
, enforcing only stream‐directive compatibility and ignoring other directives.v2/pkg/operationreport/externalerror.go (2)
161-164
: New stream directive conflict errorConstructor and message look good.
423-463
: New defer/stream error constructorsMessages and locations are consistent and useful. LGTM.
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
Improvements
Chores