From 41a05e967769149a96dc529e0735f7c7847cc747 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Tue, 30 Sep 2025 19:19:09 +0300 Subject: [PATCH 01/11] add draft tests --- .../operation_validation_test.go | 516 +++++++++++++++++- 1 file changed, 515 insertions(+), 1 deletion(-) diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index 57e056f3e..5055b77f9 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -12,6 +12,7 @@ import ( "github.com/wundergraph/graphql-go-tools/v2/pkg/astnormalization" "github.com/wundergraph/graphql-go-tools/v2/pkg/astparser" "github.com/wundergraph/graphql-go-tools/v2/pkg/astprinter" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor" "github.com/wundergraph/graphql-go-tools/v2/pkg/errorcodes" "github.com/wundergraph/graphql-go-tools/v2/pkg/internal/unsafeparser" "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" @@ -4176,6 +4177,493 @@ type Query { }) }) }) + + t.Run("defer/stream directive", func(t *testing.T) { + + t.Run("labels", func(t *testing.T) { + t.Run("defer directive with unique labels", func(t *testing.T) { + run(t, ` + query { + dog { + ...dogFragment @defer(label: "dogLabel") + ...otherFragment @defer(label: "otherLabel") + } + } + fragment dogFragment on Dog { name } + fragment otherFragment on Dog { nickname } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + + t.Run("stream directive with unique labels", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream(label: "extrasStream") { string } + mustExtras @stream(label: "mustExtrasStream") { string } + } + } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + + t.Run("defer and stream with same label", func(t *testing.T) { + run(t, ` + query { + dog { + ...dogFragment @defer(label: "sameLabel") + extras @stream(label: "sameLabel") { string } + } + } + fragment dogFragment on Dog { name } + `, DeferStreamDirectiveLabelRule(), Invalid) + }) + t.Run("defer directives with same label", func(t *testing.T) { + run(t, ` + query { + dog { + ...dogFragment @defer(label: "duplicateLabel") + ...otherFragment @defer(label: "duplicateLabel") + } + } + fragment dogFragment on Dog { name } + fragment otherFragment on Dog { nickname } + `, DeferStreamDirectiveLabelRule(), Invalid) + }) + t.Run("multiple stream directives with same label", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream(label: "duplicateLabel") { string } + mustExtras @stream(label: "duplicateLabel") { string } + } + } + `, DeferStreamDirectiveLabelRule(), Invalid) + }) + t.Run("defer without label", func(t *testing.T) { + run(t, ` + query { + dog { + ...dogFragmentA @defer(label: "fragA") + ...dogFragmentB @defer + } + } + fragment dogFragmentA on Dog { name } + fragment dogFragmentB on Dog { nickname } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + t.Run("stream without label", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream { string } + } + } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + t.Run("defer directive with variable label", func(t *testing.T) { + run(t, ` + query($label: String) { + dog { + ...dogFragment @defer(label: $label) + } + } + fragment dogFragment on Dog { name } + `, DeferStreamDirectiveLabelRule(), Invalid) + }) + t.Run("stream directive with variable label", func(t *testing.T) { + run(t, ` + query($label: String) { + dog { + extras @stream(label: $label) { string } + } + } + `, DeferStreamDirectiveLabelRule(), Invalid) + }) + }) + + t.Run("on root field", func(t *testing.T) { + t.Run("defer on root query field", func(t *testing.T) { + run(t, ` + query { + ... @defer { + dog { name } + } + } + `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + }) + t.Run("stream on root query field", func(t *testing.T) { + run(t, ` + query { + extras @stream { string } + } + `, DeferStreamDirectiveOnRootFieldRule(), Valid) + }) + + t.Run("defer on root mutation field", func(t *testing.T) { + run(t, ` + mutation { + ... @defer { + mutateDog { name } + } + } + `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + }) + t.Run("disabled defer on root mutation field", func(t *testing.T) { + run(t, ` + mutation { + ... @defer (if: false) { + mutateDog { name } + } + } + `, DeferStreamDirectiveOnRootFieldRule(), Valid) + }) + t.Run("stream field on root mutation field", func(t *testing.T) { + run(t, ` + mutation { + mutateDogs @stream { name } + } + `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + }) + t.Run("disabled stream on root mutation field", func(t *testing.T) { + run(t, ` + mutation { + mutateDogs @stream (if: false) { name } + } + `, DeferStreamDirectiveOnRootFieldRule(), Valid) + }) + + t.Run("defer on root subscription field", func(t *testing.T) { + run(t, ` + subscription { + ... @defer { + subscribeDog { name } + } + } + `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + }) + t.Run("disabled defer on root subscription field", func(t *testing.T) { + run(t, ` + subscription { + ... @defer (if: false) { + subscribeDog { name } + } + } + `, DeferStreamDirectiveOnRootFieldRule(), Valid) + }) + t.Run("stream field on root subscription field", func(t *testing.T) { + run(t, ` + subscription { + subscribeDog @stream { name } + } + `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + }) + t.Run("disabled stream on root subscription field", func(t *testing.T) { + run(t, ` + subscription { + subscribeDog @stream (if: false) { name } + } + `, DeferStreamDirectiveOnRootFieldRule(), Valid) + }) + + t.Run("defer on nested field", func(t *testing.T) { + run(t, ` + query { + dog { + ... @defer { + extra { string } + } + } + } + `, DeferStreamDirectiveOnRootFieldRule(), Valid) + }) + t.Run("defer with if argument as variable", func(t *testing.T) { + run(t, ` + query($shouldDefer: Boolean!) { + ... @defer(if: $shouldDefer) { + dog { name } + } + } + `, DeferStreamDirectiveOnRootFieldRule(), Valid) + }) + }) + + t.Run("stream on lists", func(t *testing.T) { + t.Run("stream on list field", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream { string } + } + } + `, StreamDirectiveOnListFieldRule(), Valid) + }) + t.Run("stream on non-list field", func(t *testing.T) { + run(t, ` + query { + dog @stream { name } + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + t.Run("stream on scalar field", func(t *testing.T) { + run(t, ` + query { + dog { + name @stream + } + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + }) + + t.Run("defer location", func(t *testing.T) { + t.Run("defer on inline fragment", func(t *testing.T) { + run(t, ` + query { + pet { + ... on Dog @defer { name } + } + } + `, DirectivesAreInValidLocations(), Valid) + }) + t.Run("defer on fragment spread", func(t *testing.T) { + run(t, ` + query { + dog { + ...dogFragment @defer + } + } + fragment dogFragment on Dog { name } + `, DirectivesAreInValidLocations(), Valid) + }) + t.Run("defer on field", func(t *testing.T) { + run(t, ` + query { + dog @defer { name } + } + `, DirectivesAreInValidLocations(), Invalid, withValidationErrors("defer not allowed on node of kind: FIELD")) + }) + t.Run("stream on fragment spread", func(t *testing.T) { + run(t, ` + query { + ...extrasFrag @stream + } + fragment extrasFrag on Query { + extras { + ... on DogExtra { + string + } + ... on CatExtra { + string2 + } + } + } + `, DirectivesAreInValidLocations(), Invalid, withValidationErrors("stream not allowed on node of kind: INLINE_FRAGMENT")) + }) + }) + + t.Run("complex scenarios", func(t *testing.T) { + t.Run("nested defer directives", func(t *testing.T) { + run(t, ` + query { + dog { + ... @defer { + name + extra { + ... @defer { string } + } + } + } + } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + t.Run("defer and stream in same query", func(t *testing.T) { + run(t, ` + query { + dog { + ... @defer { name } + extras @stream { string } + } + } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + t.Run("stream on multiple fields with unique labels", func(t *testing.T) { + run(t, ` + query { + dog { + extra { + mustStrings @stream(label: "dogExtraStream") + } + extras @stream(label: "dogExtras") { + string + } + } + cat { + extra { + mustStrings @stream(label: "catExtraStrings") + } + } + extras @stream(label: "rootExtras") { + strings @stream(label: "extrasStrings") + } + } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + t.Run("duplicate labels with one disabled defer", func(t *testing.T) { + run(t, ` + query { + dog { + ...fragment1 @defer(label: "a", if: false) + ...fragment2 @defer(label: "a") + } + } + fragment fragment1 on Dog { + name + } + fragment fragment2 on Dog { + nickname + } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + t.Run("deeply nested defer with unique labels", func(t *testing.T) { + run(t, ` + query { + dog { + ... @defer(label: "level1") { + name + extras { + ... @defer(label: "level2") { + string + ... fragment1 @defer(label: "level3") + } + } + } + } + } + fragment fragment1 on Dog { + mustStrings + } + `, DeferStreamDirectiveLabelRule(), Valid) + }) + t.Run("deeply nested defer with duplicate labels", func(t *testing.T) { + run(t, ` + query { + dog { + ... @defer(label: "level1") { + name + extras { + ... @defer(label: "level2") { + string + ... fragment1 @defer(label: "level1") + } + } + } + } + } + fragment fragment1 on Dog { + mustStrings + } + `, DeferStreamDirectiveLabelRule(), Invalid) + }) + t.Run("defer on typed inline fragment", func(t *testing.T) { + run(t, ` + query { + extras { + ... on CatExtra @defer { + string1 + } + ... on DogExtra @defer { + string2 + } + } + } + `, DirectivesAreInValidLocations(), Valid) + }) + + }) + + t.Run("stream merging", func(t *testing.T) { + t.Run("stream with positive initialCount argument", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream(initialCount: 5) { string } + } + } + `, StreamDirectiveOnListFieldRule(), Valid) + }) + t.Run("stream with negative initialCount argument", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream(initialCount: -1) { string } + } + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + t.Run("same stream directives supported", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "same", initialCount: 1) + extras @stream(label: "same", initialCount: 1) + } + `, StreamDirectiveOnListFieldRule(), Valid) + }) + t.Run("different stream directive label", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "one", initialCount: 1) + extras @stream(label: "two", initialCount: 1) + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + t.Run("different stream directive initialCount", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "same", initialCount: 1) + extras @stream(label: "same", initialCount: 5) + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + t.Run("different stream directive: first missing args", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream + extras @stream(label: "two", initialCount: 5) + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + t.Run("different stream directive: second missing args", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "one", initialCount: 1) + extras @stream + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + t.Run("mix of stream and no stream", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras + extras @stream + } + `, StreamDirectiveOnListFieldRule(), Invalid) + }) + t.Run("different stream directive both missing args", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream + extras @stream + } + `, StreamDirectiveOnListFieldRule(), Valid) + }) + }) + }) } func TestValidationEdgeCases(t *testing.T) { @@ -4921,9 +5409,32 @@ func TestValidateFieldSelection(t *testing.T) { }) } +// Placeholder rule functions for defer/stream validation - these need to be implemented +func DeferStreamDirectiveLabelRule() Rule { + // TODO: Implement label uniqueness and variable validation + return func(walker *astvisitor.Walker) { + // Implementation needed + } +} + +func DeferStreamDirectiveOnRootFieldRule() Rule { + // TODO: Implement root field validation + return func(walker *astvisitor.Walker) { + // Implementation needed + } +} + +func StreamDirectiveOnListFieldRule() Rule { + // TODO: Implement list field validation + return func(walker *astvisitor.Walker) { + // Implementation needed + } +} + var testDefinition = ` directive @tag(name: String) on FIELD -directive @stream(label: String) on FIELD +directive @stream(label: String, initialCount: Int, if: Boolean = true) on FIELD +directive @defer(label: String, if: Boolean = true) on FRAGMENT_SPREAD | INLINE_FRAGMENT schema { query: Query @@ -4938,6 +5449,7 @@ type Message { type Subscription { subscribeDog: Dog + subscribeDogs: [Dog] newMessage: Message foo: String bar: String @@ -4946,6 +5458,7 @@ type Subscription { type Mutation { mutateDog: Dog + mutateDogs: [Dog] } input ComplexInput { name: String, owner: String, optionalListOfOptionalStrings: [String]} @@ -5011,6 +5524,7 @@ type Query { findDogNonOptional(complex: ComplexNonOptionalInput): Dog booleanList(booleanListArg: [Boolean!]): Boolean extra: Extra + extras: [Extra!]! nested(input: NestedInput): Boolean args: Arguments } From 7309a83b182f7ebc1f17f8b5c4a399d67e7ebe6b Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Wed, 1 Oct 2025 12:36:56 +0300 Subject: [PATCH 02/11] add a stream on lists rule --- ...ion_rule_stream_directive_on_list_field.go | 73 +++++++++++++++++++ .../operation_validation_test.go | 32 ++++---- v2/pkg/operationreport/externalerror.go | 5 ++ 3 files changed, 94 insertions(+), 16 deletions(-) create mode 100644 v2/pkg/astvalidation/operation_rule_stream_directive_on_list_field.go diff --git a/v2/pkg/astvalidation/operation_rule_stream_directive_on_list_field.go b/v2/pkg/astvalidation/operation_rule_stream_directive_on_list_field.go new file mode 100644 index 000000000..40dcd78a7 --- /dev/null +++ b/v2/pkg/astvalidation/operation_rule_stream_directive_on_list_field.go @@ -0,0 +1,73 @@ +package astvalidation + +import ( + "bytes" + + "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor" + "github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/literal" + "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" +) + +// StreamDirectiveOnListFieldRule validates that the stream directive is used on list fields +func StreamDirectiveOnListFieldRule() Rule { + return func(walker *astvisitor.Walker) { + visitor := streamDirectiveOnListFieldVisitor{ + Walker: walker, + } + walker.RegisterEnterDocumentVisitor(&visitor) + walker.RegisterEnterDirectiveVisitor(&visitor) + } +} + +type streamDirectiveOnListFieldVisitor struct { + *astvisitor.Walker + + operation, definition *ast.Document +} + +func (s *streamDirectiveOnListFieldVisitor) EnterDocument(operation, definition *ast.Document) { + s.operation = operation + s.definition = definition +} + +func (s *streamDirectiveOnListFieldVisitor) EnterDirective(ref int) { + directiveName := s.operation.DirectiveNameBytes(ref) + + // Only validate @stream directives + if !bytes.Equal(directiveName, literal.STREAM) { + return + } + + if len(s.Ancestors) == 0 { + return + } + ancestor := s.Ancestors[len(s.Ancestors)-1] + + // Get the field definition from the schema + // We need to walk up the type definitions to find the field + fieldName := s.operation.FieldNameBytes(ancestor.Ref) + + // Find the enclosing type by looking at TypeDefinitions in the walker. + // Start from the parent of the current typeDefinitions (the last item in the slice). + var fieldDefinition int + var exists bool + for i := len(s.TypeDefinitions) - 2; i >= 0; i-- { + fieldDefinition, exists = s.definition.NodeFieldDefinitionByName(s.TypeDefinitions[i], fieldName) + if exists { + break + } + } + + if !exists { + // If the field doesn't exist in the schema, that's a different validation error + // Skip this check + return + } + + fieldTypeRef := s.definition.FieldDefinitionType(fieldDefinition) + + if !s.definition.TypeIsList(fieldTypeRef) { + s.StopWithExternalErr(operationreport.ErrStreamDirectiveOnNonListField(directiveName, fieldName)) + } +} diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index 5055b77f9..3533d5cb8 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4396,6 +4396,13 @@ type Query { } `, StreamDirectiveOnListFieldRule(), Valid) }) + t.Run("stream on root list field", func(t *testing.T) { + run(t, ` + query { + extras @stream { name } + } + `, StreamDirectiveOnListFieldRule(), Valid) + }) t.Run("stream on non-list field", func(t *testing.T) { run(t, ` query { @@ -4588,7 +4595,7 @@ type Query { extras @stream(initialCount: 5) { string } } } - `, StreamDirectiveOnListFieldRule(), Valid) + `, FieldSelectionMerging(), Valid) }) t.Run("stream with negative initialCount argument", func(t *testing.T) { run(t, ` @@ -4597,7 +4604,7 @@ type Query { extras @stream(initialCount: -1) { string } } } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, FieldSelectionMerging(), Invalid) }) t.Run("same stream directives supported", func(t *testing.T) { run(t, ` @@ -4606,7 +4613,7 @@ type Query { extras @stream(label: "same", initialCount: 1) extras @stream(label: "same", initialCount: 1) } - `, StreamDirectiveOnListFieldRule(), Valid) + `, FieldSelectionMerging(), Valid) }) t.Run("different stream directive label", func(t *testing.T) { run(t, ` @@ -4615,7 +4622,7 @@ type Query { extras @stream(label: "one", initialCount: 1) extras @stream(label: "two", initialCount: 1) } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, FieldSelectionMerging(), Invalid) }) t.Run("different stream directive initialCount", func(t *testing.T) { run(t, ` @@ -4624,7 +4631,7 @@ type Query { extras @stream(label: "same", initialCount: 1) extras @stream(label: "same", initialCount: 5) } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, FieldSelectionMerging(), Invalid) }) t.Run("different stream directive: first missing args", func(t *testing.T) { run(t, ` @@ -4633,7 +4640,7 @@ type Query { extras @stream extras @stream(label: "two", initialCount: 5) } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, FieldSelectionMerging(), Invalid) }) t.Run("different stream directive: second missing args", func(t *testing.T) { run(t, ` @@ -4642,7 +4649,7 @@ type Query { extras @stream(label: "one", initialCount: 1) extras @stream } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, FieldSelectionMerging(), Invalid) }) t.Run("mix of stream and no stream", func(t *testing.T) { run(t, ` @@ -4651,7 +4658,7 @@ type Query { extras extras @stream } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, FieldSelectionMerging(), Invalid) }) t.Run("different stream directive both missing args", func(t *testing.T) { run(t, ` @@ -4660,7 +4667,7 @@ type Query { extras @stream extras @stream } - `, StreamDirectiveOnListFieldRule(), Valid) + `, FieldSelectionMerging(), Valid) }) }) }) @@ -5424,13 +5431,6 @@ func DeferStreamDirectiveOnRootFieldRule() Rule { } } -func StreamDirectiveOnListFieldRule() Rule { - // TODO: Implement list field validation - return func(walker *astvisitor.Walker) { - // Implementation needed - } -} - var testDefinition = ` directive @tag(name: String) on FIELD directive @stream(label: String, initialCount: Int, if: Boolean = true) on FIELD diff --git a/v2/pkg/operationreport/externalerror.go b/v2/pkg/operationreport/externalerror.go index 8de9805f5..d1da7e329 100644 --- a/v2/pkg/operationreport/externalerror.go +++ b/v2/pkg/operationreport/externalerror.go @@ -411,6 +411,11 @@ func ErrDirectiveMustBeUniquePerLocation(directiveName ast.ByteSlice, position, return err } +func ErrStreamDirectiveOnNonListField(directiveName, fieldName ast.ByteSlice) (err ExternalError) { + err.Message = fmt.Sprintf(`directive "@%s" can only be used on list fields, but field "%s" is not a list`, directiveName, fieldName) + return err +} + func ErrOnlyOneQueryTypeAllowed() (err ExternalError) { err.Message = "there can be only one query type in schema" return err From 0c595592d46224cbc485104fa3eb30d544c8b407 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Wed, 1 Oct 2025 15:53:34 +0300 Subject: [PATCH 03/11] add labels checker --- ...ation_rule_defer_stream_directive_label.go | 95 ++++++++++++ .../operation_validation_test.go | 140 +++++++++--------- v2/pkg/lexer/literal/literal.go | 1 + v2/pkg/operationreport/externalerror.go | 29 +++- 4 files changed, 193 insertions(+), 72 deletions(-) create mode 100644 v2/pkg/astvalidation/operation_rule_defer_stream_directive_label.go diff --git a/v2/pkg/astvalidation/operation_rule_defer_stream_directive_label.go b/v2/pkg/astvalidation/operation_rule_defer_stream_directive_label.go new file mode 100644 index 000000000..094a32cce --- /dev/null +++ b/v2/pkg/astvalidation/operation_rule_defer_stream_directive_label.go @@ -0,0 +1,95 @@ +package astvalidation + +import ( + "bytes" + + "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor" + "github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/literal" + "github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/position" + "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" +) + +// DeferStreamDirectiveLabelRule validates that defer and stream directive labels are: +// 1. Unique across all defer and stream directives within an operation +// 2. Not using variables (must be static string values) +func DeferStreamDirectiveLabelRule() Rule { + return func(walker *astvisitor.Walker) { + visitor := deferStreamDirectiveLabelVisitor{ + Walker: walker, + } + walker.RegisterEnterDocumentVisitor(&visitor) + walker.RegisterEnterOperationVisitor(&visitor) + walker.RegisterEnterDirectiveVisitor(&visitor) + } +} + +type labelPosition struct { + directiveRef int + position position.Position +} + +type deferStreamDirectiveLabelVisitor struct { + *astvisitor.Walker + + operation, definition *ast.Document + + // Track seen labels with their directive refs and positions for duplicate detection. + seenLabels map[string]labelPosition +} + +func (d *deferStreamDirectiveLabelVisitor) EnterDocument(operation, definition *ast.Document) { + d.operation = operation + d.definition = definition +} + +func (d *deferStreamDirectiveLabelVisitor) EnterOperationDefinition(ref int) { + d.seenLabels = make(map[string]labelPosition) +} + +func (d *deferStreamDirectiveLabelVisitor) EnterDirective(ref int) { + directiveName := d.operation.DirectiveNameBytes(ref) + + if !bytes.Equal(directiveName, literal.DEFER) && !bytes.Equal(directiveName, literal.STREAM) { + return + } + + labelValue, hasLabel := d.operation.DirectiveArgumentValueByName(ref, literal.LABEL) + if !hasLabel { + // No label is okay, directives can be used without labels + return + } + + directivePosition := d.operation.Directives[ref].At + + // Labels must be static strings, not variables + if labelValue.Kind == ast.ValueKindVariable { + d.StopWithExternalErr(operationreport.ErrDeferStreamDirectiveLabelMustBeStatic(directiveName, directivePosition)) + return + } + + if labelValue.Kind != ast.ValueKindString { + // This should be caught by other validation rules, but skip if not a string + return + } + + labelString := d.operation.StringValueContentString(labelValue.Ref) + + if previous, exists := d.seenLabels[labelString]; exists { + previousDirectiveName := d.operation.DirectiveNameBytes(previous.directiveRef) + d.StopWithExternalErr(operationreport.ErrDeferStreamDirectiveLabelMustBeUnique( + directiveName, + previousDirectiveName, + labelString, + previous.position, + directivePosition, + )) + return + } + + // Record this label with its position + d.seenLabels[labelString] = labelPosition{ + directiveRef: ref, + position: directivePosition, + } +} diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index 3533d5cb8..ad216205a 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4183,100 +4183,106 @@ type Query { t.Run("labels", func(t *testing.T) { t.Run("defer directive with unique labels", func(t *testing.T) { run(t, ` - query { - dog { - ...dogFragment @defer(label: "dogLabel") - ...otherFragment @defer(label: "otherLabel") + query { + dog { + ...dogFragment @defer(label: "dogLabel") + ...otherFragment @defer(label: "otherLabel") + } } - } - fragment dogFragment on Dog { name } - fragment otherFragment on Dog { nickname } - `, DeferStreamDirectiveLabelRule(), Valid) + fragment dogFragment on Dog { name } + fragment otherFragment on Dog { nickname } + `, DeferStreamDirectiveLabelRule(), Valid) }) t.Run("stream directive with unique labels", func(t *testing.T) { run(t, ` - query { - dog { - extras @stream(label: "extrasStream") { string } - mustExtras @stream(label: "mustExtrasStream") { string } + query { + dog { + extras @stream(label: "extrasStream") { string } + mustExtras @stream(label: "mustExtrasStream") { string } + } } - } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamDirectiveLabelRule(), Valid) }) t.Run("defer and stream with same label", func(t *testing.T) { run(t, ` - query { - dog { - ...dogFragment @defer(label: "sameLabel") - extras @stream(label: "sameLabel") { string } + query { + dog { + ...dogFragment @defer(label: "sameLabel") + extras @stream(label: "sameLabel") { string } + } } - } - fragment dogFragment on Dog { name } - `, DeferStreamDirectiveLabelRule(), Invalid) + fragment dogFragment on Dog { name } + `, DeferStreamDirectiveLabelRule(), Invalid, withValidationErrors( + `directive "@stream" label "sameLabel" must be unique, but was already used on "@defer" directive`, + )) }) t.Run("defer directives with same label", func(t *testing.T) { run(t, ` - query { - dog { - ...dogFragment @defer(label: "duplicateLabel") - ...otherFragment @defer(label: "duplicateLabel") + query { + dog { + ...dogFragment @defer(label: "duplicateLabel") + ...otherFragment @defer(label: "duplicateLabel") + } } - } - fragment dogFragment on Dog { name } - fragment otherFragment on Dog { nickname } - `, DeferStreamDirectiveLabelRule(), Invalid) + fragment dogFragment on Dog { name } + fragment otherFragment on Dog { nickname } + `, DeferStreamDirectiveLabelRule(), Invalid, withDisableNormalization(), + withValidationErrors(`directive "@defer" label "duplicateLabel" must be unique`)) }) t.Run("multiple stream directives with same label", func(t *testing.T) { run(t, ` - query { - dog { - extras @stream(label: "duplicateLabel") { string } - mustExtras @stream(label: "duplicateLabel") { string } + query { + dog { + extras @stream(label: "duplicateLabel") { string } + mustExtras @stream(label: "duplicateLabel") { string } + } } - } - `, DeferStreamDirectiveLabelRule(), Invalid) + `, DeferStreamDirectiveLabelRule(), Invalid, + withValidationErrors(`directive "@stream" label "duplicateLabel" must be unique`)) }) t.Run("defer without label", func(t *testing.T) { run(t, ` - query { - dog { - ...dogFragmentA @defer(label: "fragA") - ...dogFragmentB @defer + query { + dog { + ...dogFragmentA @defer(label: "fragA") + ...dogFragmentB @defer + } } - } - fragment dogFragmentA on Dog { name } - fragment dogFragmentB on Dog { nickname } - `, DeferStreamDirectiveLabelRule(), Valid) + fragment dogFragmentA on Dog { name } + fragment dogFragmentB on Dog { nickname } + `, DeferStreamDirectiveLabelRule(), Valid) }) t.Run("stream without label", func(t *testing.T) { run(t, ` - query { - dog { - extras @stream { string } + query { + dog { + extras @stream { string } + } } - } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamDirectiveLabelRule(), Valid) }) t.Run("defer directive with variable label", func(t *testing.T) { run(t, ` - query($label: String) { - dog { - ...dogFragment @defer(label: $label) + query($label: String) { + dog { + ...dogFragment @defer(label: $label) + } } - } - fragment dogFragment on Dog { name } - `, DeferStreamDirectiveLabelRule(), Invalid) + fragment dogFragment on Dog { name } + `, DeferStreamDirectiveLabelRule(), Invalid, + withValidationErrors(`directive "@defer" label argument must be a static string value, not a variable`)) }) t.Run("stream directive with variable label", func(t *testing.T) { run(t, ` - query($label: String) { - dog { - extras @stream(label: $label) { string } + query($label: String) { + dog { + extras @stream(label: $label) { string } + } } - } - `, DeferStreamDirectiveLabelRule(), Invalid) + `, DeferStreamDirectiveLabelRule(), Invalid, + withValidationErrors(`directive "@stream" label argument must be a static string value, not a variable`)) }) }) @@ -4480,7 +4486,7 @@ type Query { } } } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamComplexRule(), Valid) }) t.Run("defer and stream in same query", func(t *testing.T) { run(t, ` @@ -4490,7 +4496,7 @@ type Query { extras @stream { string } } } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamComplexRule(), Valid) }) t.Run("stream on multiple fields with unique labels", func(t *testing.T) { run(t, ` @@ -4512,7 +4518,7 @@ type Query { strings @stream(label: "extrasStrings") } } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamComplexRule(), Valid) }) t.Run("duplicate labels with one disabled defer", func(t *testing.T) { run(t, ` @@ -4528,7 +4534,7 @@ type Query { fragment fragment2 on Dog { nickname } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamComplexRule(), Valid) }) t.Run("deeply nested defer with unique labels", func(t *testing.T) { run(t, ` @@ -4548,7 +4554,7 @@ type Query { fragment fragment1 on Dog { mustStrings } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamComplexRule(), Valid) }) t.Run("deeply nested defer with duplicate labels", func(t *testing.T) { run(t, ` @@ -4568,7 +4574,7 @@ type Query { fragment fragment1 on Dog { mustStrings } - `, DeferStreamDirectiveLabelRule(), Invalid) + `, DeferStreamComplexRule(), Invalid) }) t.Run("defer on typed inline fragment", func(t *testing.T) { run(t, ` @@ -5417,8 +5423,8 @@ func TestValidateFieldSelection(t *testing.T) { } // Placeholder rule functions for defer/stream validation - these need to be implemented -func DeferStreamDirectiveLabelRule() Rule { - // TODO: Implement label uniqueness and variable validation +func DeferStreamComplexRule() Rule { + // TODO: Implement complex validation return func(walker *astvisitor.Walker) { // Implementation needed } diff --git a/v2/pkg/lexer/literal/literal.go b/v2/pkg/lexer/literal/literal.go index 8c57db74c..4f3f73e40 100644 --- a/v2/pkg/lexer/literal/literal.go +++ b/v2/pkg/lexer/literal/literal.go @@ -67,6 +67,7 @@ var ( SKIP = []byte("skip") DEFER = []byte("defer") STREAM = []byte("stream") + LABEL = []byte("label") SCHEMA = []byte("schema") EXTEND = []byte("extend") SCALAR = []byte("scalar") diff --git a/v2/pkg/operationreport/externalerror.go b/v2/pkg/operationreport/externalerror.go index d1da7e329..aaea076d4 100644 --- a/v2/pkg/operationreport/externalerror.go +++ b/v2/pkg/operationreport/externalerror.go @@ -402,17 +402,36 @@ func ErrDirectiveNotAllowedOnNode(directiveName, nodeKindName ast.ByteSlice) (er func ErrDirectiveMustBeUniquePerLocation(directiveName ast.ByteSlice, position, duplicatePosition position.Position) (err ExternalError) { err.Message = fmt.Sprintf(`The directive "@%s" can only be used once at this location.`, directiveName) - if duplicatePosition.LineStart < position.LineStart || duplicatePosition.CharStart < position.CharStart { - err.Locations = LocationsFromPosition(duplicatePosition, position) + err.Locations = orderedLocationsFromPositions(position, duplicatePosition) + + return err +} + +func orderedLocationsFromPositions(posA, posB position.Position) (locations []Location) { + if posA.LineStart < posB.LineStart || posA.CharStart < posB.CharStart { + return LocationsFromPosition(posA, posB) } else { - err.Locations = LocationsFromPosition(position, duplicatePosition) + return LocationsFromPosition(posB, posA) } +} +func ErrStreamDirectiveOnNonListField(directiveName, fieldName ast.ByteSlice) (err ExternalError) { + err.Message = fmt.Sprintf(`directive "@%s" can only be used on list fields, but field "%s" is not a list`, + directiveName, fieldName) return err } -func ErrStreamDirectiveOnNonListField(directiveName, fieldName ast.ByteSlice) (err ExternalError) { - err.Message = fmt.Sprintf(`directive "@%s" can only be used on list fields, but field "%s" is not a list`, directiveName, fieldName) +func ErrDeferStreamDirectiveLabelMustBeStatic(directiveName ast.ByteSlice, directivePosition position.Position) (err ExternalError) { + err.Message = fmt.Sprintf(`directive "@%s" label argument must be a static string value, not a variable`, + directiveName) + err.Locations = LocationsFromPosition(directivePosition) + return err +} + +func ErrDeferStreamDirectiveLabelMustBeUnique(directiveNameA, directiveNameB ast.ByteSlice, label string, posA, posB position.Position) (err ExternalError) { + err.Message = fmt.Sprintf(`directive "@%s" label "%s" must be unique, but was already used on "@%s" directive`, + directiveNameA, label, directiveNameB) + err.Locations = orderedLocationsFromPositions(posA, posB) return err } From ed26fd21bd0dcfc9b4e1b9097366970b8684a509 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Wed, 1 Oct 2025 15:59:45 +0300 Subject: [PATCH 04/11] rename rules --- ...ration_rule_defer_stream_unique_labels.go} | 14 +++++----- ...ration_rule_stream_on_list_fields_only.go} | 12 ++++----- .../operation_validation_test.go | 26 +++++++++---------- 3 files changed, 26 insertions(+), 26 deletions(-) rename v2/pkg/astvalidation/{operation_rule_defer_stream_directive_label.go => operation_rule_defer_stream_unique_labels.go} (83%) rename v2/pkg/astvalidation/{operation_rule_stream_directive_on_list_field.go => operation_rule_stream_on_list_fields_only.go} (81%) diff --git a/v2/pkg/astvalidation/operation_rule_defer_stream_directive_label.go b/v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go similarity index 83% rename from v2/pkg/astvalidation/operation_rule_defer_stream_directive_label.go rename to v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go index 094a32cce..9d1886863 100644 --- a/v2/pkg/astvalidation/operation_rule_defer_stream_directive_label.go +++ b/v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go @@ -10,12 +10,12 @@ import ( "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" ) -// DeferStreamDirectiveLabelRule validates that defer and stream directive labels are: +// DeferStreamHaveUniqueLabels validates that defer and stream directive labels are: // 1. Unique across all defer and stream directives within an operation // 2. Not using variables (must be static string values) -func DeferStreamDirectiveLabelRule() Rule { +func DeferStreamHaveUniqueLabels() Rule { return func(walker *astvisitor.Walker) { - visitor := deferStreamDirectiveLabelVisitor{ + visitor := deferStreamLabelsVisitor{ Walker: walker, } walker.RegisterEnterDocumentVisitor(&visitor) @@ -29,7 +29,7 @@ type labelPosition struct { position position.Position } -type deferStreamDirectiveLabelVisitor struct { +type deferStreamLabelsVisitor struct { *astvisitor.Walker operation, definition *ast.Document @@ -38,16 +38,16 @@ type deferStreamDirectiveLabelVisitor struct { seenLabels map[string]labelPosition } -func (d *deferStreamDirectiveLabelVisitor) EnterDocument(operation, definition *ast.Document) { +func (d *deferStreamLabelsVisitor) EnterDocument(operation, definition *ast.Document) { d.operation = operation d.definition = definition } -func (d *deferStreamDirectiveLabelVisitor) EnterOperationDefinition(ref int) { +func (d *deferStreamLabelsVisitor) EnterOperationDefinition(ref int) { d.seenLabels = make(map[string]labelPosition) } -func (d *deferStreamDirectiveLabelVisitor) EnterDirective(ref int) { +func (d *deferStreamLabelsVisitor) EnterDirective(ref int) { directiveName := d.operation.DirectiveNameBytes(ref) if !bytes.Equal(directiveName, literal.DEFER) && !bytes.Equal(directiveName, literal.STREAM) { diff --git a/v2/pkg/astvalidation/operation_rule_stream_directive_on_list_field.go b/v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go similarity index 81% rename from v2/pkg/astvalidation/operation_rule_stream_directive_on_list_field.go rename to v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go index 40dcd78a7..024b57b99 100644 --- a/v2/pkg/astvalidation/operation_rule_stream_directive_on_list_field.go +++ b/v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go @@ -9,10 +9,10 @@ import ( "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" ) -// StreamDirectiveOnListFieldRule validates that the stream directive is used on list fields -func StreamDirectiveOnListFieldRule() Rule { +// StreamAppliedToListFieldsOnly validates that the stream directive is used on list fields +func StreamAppliedToListFieldsOnly() Rule { return func(walker *astvisitor.Walker) { - visitor := streamDirectiveOnListFieldVisitor{ + visitor := streamAppliedToListFieldsVisitor{ Walker: walker, } walker.RegisterEnterDocumentVisitor(&visitor) @@ -20,18 +20,18 @@ func StreamDirectiveOnListFieldRule() Rule { } } -type streamDirectiveOnListFieldVisitor struct { +type streamAppliedToListFieldsVisitor struct { *astvisitor.Walker operation, definition *ast.Document } -func (s *streamDirectiveOnListFieldVisitor) EnterDocument(operation, definition *ast.Document) { +func (s *streamAppliedToListFieldsVisitor) EnterDocument(operation, definition *ast.Document) { s.operation = operation s.definition = definition } -func (s *streamDirectiveOnListFieldVisitor) EnterDirective(ref int) { +func (s *streamAppliedToListFieldsVisitor) EnterDirective(ref int) { directiveName := s.operation.DirectiveNameBytes(ref) // Only validate @stream directives diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index ad216205a..0d476a758 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4191,7 +4191,7 @@ type Query { } fragment dogFragment on Dog { name } fragment otherFragment on Dog { nickname } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("stream directive with unique labels", func(t *testing.T) { @@ -4202,7 +4202,7 @@ type Query { mustExtras @stream(label: "mustExtrasStream") { string } } } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("defer and stream with same label", func(t *testing.T) { @@ -4214,7 +4214,7 @@ type Query { } } fragment dogFragment on Dog { name } - `, DeferStreamDirectiveLabelRule(), Invalid, withValidationErrors( + `, DeferStreamHaveUniqueLabels(), Invalid, withValidationErrors( `directive "@stream" label "sameLabel" must be unique, but was already used on "@defer" directive`, )) }) @@ -4228,7 +4228,7 @@ type Query { } fragment dogFragment on Dog { name } fragment otherFragment on Dog { nickname } - `, DeferStreamDirectiveLabelRule(), Invalid, withDisableNormalization(), + `, DeferStreamHaveUniqueLabels(), Invalid, withDisableNormalization(), withValidationErrors(`directive "@defer" label "duplicateLabel" must be unique`)) }) t.Run("multiple stream directives with same label", func(t *testing.T) { @@ -4239,7 +4239,7 @@ type Query { mustExtras @stream(label: "duplicateLabel") { string } } } - `, DeferStreamDirectiveLabelRule(), Invalid, + `, DeferStreamHaveUniqueLabels(), Invalid, withValidationErrors(`directive "@stream" label "duplicateLabel" must be unique`)) }) t.Run("defer without label", func(t *testing.T) { @@ -4252,7 +4252,7 @@ type Query { } fragment dogFragmentA on Dog { name } fragment dogFragmentB on Dog { nickname } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("stream without label", func(t *testing.T) { run(t, ` @@ -4261,7 +4261,7 @@ type Query { extras @stream { string } } } - `, DeferStreamDirectiveLabelRule(), Valid) + `, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("defer directive with variable label", func(t *testing.T) { run(t, ` @@ -4271,7 +4271,7 @@ type Query { } } fragment dogFragment on Dog { name } - `, DeferStreamDirectiveLabelRule(), Invalid, + `, DeferStreamHaveUniqueLabels(), Invalid, withValidationErrors(`directive "@defer" label argument must be a static string value, not a variable`)) }) t.Run("stream directive with variable label", func(t *testing.T) { @@ -4281,7 +4281,7 @@ type Query { extras @stream(label: $label) { string } } } - `, DeferStreamDirectiveLabelRule(), Invalid, + `, DeferStreamHaveUniqueLabels(), Invalid, withValidationErrors(`directive "@stream" label argument must be a static string value, not a variable`)) }) }) @@ -4400,21 +4400,21 @@ type Query { extras @stream { string } } } - `, StreamDirectiveOnListFieldRule(), Valid) + `, StreamAppliedToListFieldsOnly(), Valid) }) t.Run("stream on root list field", func(t *testing.T) { run(t, ` query { extras @stream { name } } - `, StreamDirectiveOnListFieldRule(), Valid) + `, StreamAppliedToListFieldsOnly(), Valid) }) t.Run("stream on non-list field", func(t *testing.T) { run(t, ` query { dog @stream { name } } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, StreamAppliedToListFieldsOnly(), Invalid) }) t.Run("stream on scalar field", func(t *testing.T) { run(t, ` @@ -4423,7 +4423,7 @@ type Query { name @stream } } - `, StreamDirectiveOnListFieldRule(), Invalid) + `, StreamAppliedToListFieldsOnly(), Invalid) }) }) From 1dd3a7d1c5029a4682daabd9c6b375c925463fcb Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Thu, 2 Oct 2025 10:28:38 +0300 Subject: [PATCH 05/11] check initialCount --- ...eration_rule_stream_on_list_fields_only.go | 19 +++++++-- .../operation_validation_test.go | 40 +++++++++---------- v2/pkg/lexer/literal/literal.go | 1 + v2/pkg/operationreport/externalerror.go | 10 ++++- 4 files changed, 46 insertions(+), 24 deletions(-) diff --git a/v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go b/v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go index 024b57b99..79913fd45 100644 --- a/v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go +++ b/v2/pkg/astvalidation/operation_rule_stream_on_list_fields_only.go @@ -39,6 +39,19 @@ func (s *streamAppliedToListFieldsVisitor) EnterDirective(ref int) { return } + // Validate initialCount argument if present + initialCountValue, hasCount := s.operation.DirectiveArgumentValueByName(ref, literal.INITIAL_COUNT) + if hasCount { + if initialCountValue.Kind == ast.ValueKindInteger { + initialCount := s.operation.IntValueAsInt32(initialCountValue.Ref) + if initialCount < 0 { + directivePosition := s.operation.Directives[ref].At + s.StopWithExternalErr(operationreport.ErrStreamInitialCountMustBeNonNegative(directiveName, directivePosition)) + return + } + } + } + if len(s.Ancestors) == 0 { return } @@ -47,9 +60,8 @@ func (s *streamAppliedToListFieldsVisitor) EnterDirective(ref int) { // Get the field definition from the schema // We need to walk up the type definitions to find the field fieldName := s.operation.FieldNameBytes(ancestor.Ref) - // Find the enclosing type by looking at TypeDefinitions in the walker. - // Start from the parent of the current typeDefinitions (the last item in the slice). + // Start from the item before the last one of typeDefinitions. var fieldDefinition int var exists bool for i := len(s.TypeDefinitions) - 2; i >= 0; i-- { @@ -68,6 +80,7 @@ func (s *streamAppliedToListFieldsVisitor) EnterDirective(ref int) { fieldTypeRef := s.definition.FieldDefinitionType(fieldDefinition) if !s.definition.TypeIsList(fieldTypeRef) { - s.StopWithExternalErr(operationreport.ErrStreamDirectiveOnNonListField(directiveName, fieldName)) + directivePosition := s.operation.Directives[ref].At + s.StopWithExternalErr(operationreport.ErrStreamDirectiveOnNonListField(directiveName, fieldName, directivePosition)) } } diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index 0d476a758..54c973505 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4392,7 +4392,25 @@ type Query { }) }) - t.Run("stream on lists", func(t *testing.T) { + t.Run("stream location", func(t *testing.T) { + t.Run("stream with positive initialCount argument", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream(initialCount: 5) { string } + } + } + `, StreamAppliedToListFieldsOnly(), Valid) + }) + t.Run("stream with negative initialCount argument", func(t *testing.T) { + run(t, ` + query { + dog { + extras @stream(initialCount: -1) { string } + } + } + `, StreamAppliedToListFieldsOnly(), Invalid) + }) t.Run("stream on list field", func(t *testing.T) { run(t, ` query { @@ -4594,24 +4612,6 @@ type Query { }) t.Run("stream merging", func(t *testing.T) { - t.Run("stream with positive initialCount argument", func(t *testing.T) { - run(t, ` - query { - dog { - extras @stream(initialCount: 5) { string } - } - } - `, FieldSelectionMerging(), Valid) - }) - t.Run("stream with negative initialCount argument", func(t *testing.T) { - run(t, ` - query { - dog { - extras @stream(initialCount: -1) { string } - } - } - `, FieldSelectionMerging(), Invalid) - }) t.Run("same stream directives supported", func(t *testing.T) { run(t, ` query { dog { ...dogFragment } } @@ -4628,7 +4628,7 @@ type Query { extras @stream(label: "one", initialCount: 1) extras @stream(label: "two", initialCount: 1) } - `, FieldSelectionMerging(), Invalid) + `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive initialCount", func(t *testing.T) { run(t, ` diff --git a/v2/pkg/lexer/literal/literal.go b/v2/pkg/lexer/literal/literal.go index 4f3f73e40..e47b5ab79 100644 --- a/v2/pkg/lexer/literal/literal.go +++ b/v2/pkg/lexer/literal/literal.go @@ -68,6 +68,7 @@ var ( DEFER = []byte("defer") STREAM = []byte("stream") LABEL = []byte("label") + INITIAL_COUNT = []byte("initialCount") SCHEMA = []byte("schema") EXTEND = []byte("extend") SCALAR = []byte("scalar") diff --git a/v2/pkg/operationreport/externalerror.go b/v2/pkg/operationreport/externalerror.go index aaea076d4..969d6d56e 100644 --- a/v2/pkg/operationreport/externalerror.go +++ b/v2/pkg/operationreport/externalerror.go @@ -415,9 +415,10 @@ func orderedLocationsFromPositions(posA, posB position.Position) (locations []Lo } } -func ErrStreamDirectiveOnNonListField(directiveName, fieldName ast.ByteSlice) (err ExternalError) { +func ErrStreamDirectiveOnNonListField(directiveName, fieldName ast.ByteSlice, directivePosition position.Position) (err ExternalError) { err.Message = fmt.Sprintf(`directive "@%s" can only be used on list fields, but field "%s" is not a list`, directiveName, fieldName) + err.Locations = LocationsFromPosition(directivePosition) return err } @@ -435,6 +436,13 @@ func ErrDeferStreamDirectiveLabelMustBeUnique(directiveNameA, directiveNameB ast return err } +func ErrStreamInitialCountMustBeNonNegative(directiveName ast.ByteSlice, directivePosition position.Position) (err ExternalError) { + err.Message = fmt.Sprintf(`directive "@%s" has invalid initialCount argument: must be non-negative`, + directiveName) + err.Locations = LocationsFromPosition(directivePosition) + return err +} + func ErrOnlyOneQueryTypeAllowed() (err ExternalError) { err.Message = "there can be only one query type in schema" return err From 194aaa57e77b688568228121fbf30dce81714d65 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Fri, 3 Oct 2025 18:02:52 +0300 Subject: [PATCH 06/11] check relative to operation types --- ...ration_rule_defer_stream_on_root_fields.go | 132 ++++++++++++++++++ .../operation_validation_test.go | 123 +++++++++++----- v2/pkg/operationreport/externalerror.go | 7 + 3 files changed, 225 insertions(+), 37 deletions(-) create mode 100644 v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go diff --git a/v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go b/v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go new file mode 100644 index 000000000..4513c237e --- /dev/null +++ b/v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go @@ -0,0 +1,132 @@ +package astvalidation + +import ( + "bytes" + + "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor" + "github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/literal" + "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" +) + +// DeferStreamOnValidOperations validates that defer/stream directives are used on valid operations: +// - Query operations: @defer and @stream are allowed everywhere (root and nested fields) +// - Mutation operations: @defer and @stream are NOT allowed on root fields, but allowed on nested fields +// - Subscription operations: @defer and @stream are NOT allowed anywhere (root or nested fields) +// Directives with if: false are allowed (disabled directives). +// Directives with if: $variable are allowed (dynamic directives that can't be statically determined). +func DeferStreamOnValidOperations() Rule { + return func(walker *astvisitor.Walker) { + visitor := deferStreamOnValidOpsVisitor{ + Walker: walker, + } + walker.RegisterEnterDocumentVisitor(&visitor) + walker.RegisterEnterOperationVisitor(&visitor) + walker.RegisterEnterDirectiveVisitor(&visitor) + } +} + +type deferStreamOnValidOpsVisitor struct { + *astvisitor.Walker + + operation, definition *ast.Document + currentOperationType ast.OperationType +} + +func (d *deferStreamOnValidOpsVisitor) EnterDocument(operation, definition *ast.Document) { + d.operation = operation + d.definition = definition +} + +func (d *deferStreamOnValidOpsVisitor) EnterOperationDefinition(ref int) { + d.currentOperationType = d.operation.OperationDefinitions[ref].OperationType +} + +func (d *deferStreamOnValidOpsVisitor) EnterDirective(ref int) { + directiveName := d.operation.DirectiveNameBytes(ref) + + // Only validate @defer and @stream directives + if !bytes.Equal(directiveName, literal.DEFER) && !bytes.Equal(directiveName, literal.STREAM) { + return + } + + if ifValue, hasIf := d.operation.DirectiveArgumentValueByName(ref, literal.IF); hasIf { + switch ifValue.Kind { + case ast.ValueKindBoolean: + // If "if: false", the directive is disabled, so it's allowed + if !d.operation.BooleanValue(ifValue.Ref) { + return + } + case ast.ValueKindVariable: + // If if: $variable, we can't statically determine if it's enabled, + // so we allow it (it might be false at runtime) + return + } + } + + directivePosition := d.operation.Directives[ref].At + + // For subscriptions, @defer and @stream are not allowed anywhere (root or nested) + if d.currentOperationType == ast.OperationTypeSubscription { + operationTypeName := d.currentOperationType.Name() + d.StopWithExternalErr(operationreport.ErrDeferStreamDirectiveNotAllowedOnRootField( + directiveName, + operationTypeName, + directivePosition, + )) + return + } + + // For queries, @defer and @stream are allowed everywhere + if d.currentOperationType == ast.OperationTypeQuery { + return + } + + // For mutations, check if we're at the root level + if len(d.Ancestors) == 0 { + return + } + + // The directive's immediate parent (the node it's attached to) + ancestor := d.Ancestors[len(d.Ancestors)-1] + + // Determine if this is a root level directive + isRootLevel := false + + switch ancestor.Kind { + case ast.NodeKindInlineFragment: + // For inline fragments with @defer, check if it's directly in the operation's selection set + // At root level, ancestors should be: [OperationDefinition, SelectionSet, InlineFragment] + // For nested: [OperationDefinition, SelectionSet, Field, ..., SelectionSet, InlineFragment] + if len(d.Ancestors) == 3 { + // Check if pattern is [OperationDefinition, SelectionSet, InlineFragment] + if d.Ancestors[0].Kind == ast.NodeKindOperationDefinition && + d.Ancestors[1].Kind == ast.NodeKindSelectionSet && + d.Ancestors[2].Kind == ast.NodeKindInlineFragment { + isRootLevel = true + } + } + case ast.NodeKindField: + // For fields with @stream, check if we're directly in the operation's selection set + // Count how many SelectionSets we've traversed (depth of nesting) + // A root-level field has only one SelectionSet ancestor (the operation's selection set) + selectionSetCount := 0 + for _, a := range d.Ancestors { + if a.Kind == ast.NodeKindSelectionSet { + selectionSetCount++ + } + } + // If there's only one SelectionSet in the ancestor chain, we're at root level + isRootLevel = selectionSetCount == 1 + } + + // For mutations, @defer and @stream are not allowed on root fields + if isRootLevel { + operationTypeName := d.currentOperationType.Name() + d.StopWithExternalErr(operationreport.ErrDeferStreamDirectiveNotAllowedOnRootField( + directiveName, + operationTypeName, + directivePosition, + )) + } +} diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index 54c973505..f9b39a296 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4286,113 +4286,169 @@ type Query { }) }) - t.Run("on root field", func(t *testing.T) { - t.Run("defer on root query field", func(t *testing.T) { + t.Run("on valid operations", func(t *testing.T) { + // query + t.Run("defer inline fragment spread on root query field", func(t *testing.T) { run(t, ` query { ... @defer { dog { name } } } - `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + `, DeferStreamOnValidOperations(), Valid) }) + t.Run("defer inline fragment spread on nested query field", func(t *testing.T) { + run(t, ` + query { + dog { + ... @defer { + extra { string } + } + } + } + `, DeferStreamOnValidOperations(), Valid) + }) + t.Run("defer fragment spread on root query field", func(t *testing.T) { + run(t, ` + query { + ...rootFragment @defer + } + fragment rootFragment on Query { + extras { string } + } + `, DeferStreamOnValidOperations(), Valid) + }) + + // stream t.Run("stream on root query field", func(t *testing.T) { run(t, ` query { extras @stream { string } } - `, DeferStreamDirectiveOnRootFieldRule(), Valid) + `, DeferStreamOnValidOperations(), Valid) + }) + t.Run("stream field on fragment on root query field", func(t *testing.T) { + run(t, ` + query { + ...rootFragment + } + fragment rootFragment on Query { + extras @stream { string } + } + `, DeferStreamOnValidOperations(), Valid) }) - t.Run("defer on root mutation field", func(t *testing.T) { + // mutation + t.Run("defer inline fragment spread on root mutation field", func(t *testing.T) { run(t, ` mutation { ... @defer { mutateDog { name } } } - `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + `, DeferStreamOnValidOperations(), Invalid) }) - t.Run("disabled defer on root mutation field", func(t *testing.T) { + t.Run("defer fragment spread on nested mutation field", func(t *testing.T) { + run(t, ` + mutation { + mutateDog { + ... @defer { + extra { string } + } + } + } + `, DeferStreamOnValidOperations(), Valid) + }) + t.Run("non-defer inline fragment spread on root mutation field", func(t *testing.T) { run(t, ` mutation { ... @defer (if: false) { mutateDog { name } } } - `, DeferStreamDirectiveOnRootFieldRule(), Valid) + `, DeferStreamOnValidOperations(), Valid) }) t.Run("stream field on root mutation field", func(t *testing.T) { run(t, ` mutation { mutateDogs @stream { name } } - `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + `, DeferStreamOnValidOperations(), Invalid) }) t.Run("disabled stream on root mutation field", func(t *testing.T) { run(t, ` mutation { mutateDogs @stream (if: false) { name } } - `, DeferStreamDirectiveOnRootFieldRule(), Valid) + `, DeferStreamOnValidOperations(), Valid) }) - t.Run("defer on root subscription field", func(t *testing.T) { + // subscriptions + t.Run("defer inline fragment spread on root subscription field", func(t *testing.T) { run(t, ` subscription { ... @defer { subscribeDog { name } } } - `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + `, DeferStreamOnValidOperations(), Invalid) + }) + t.Run("defer inline fragment spread on nested subscription field", func(t *testing.T) { + run(t, ` + subscription { + newMessage { + ... @defer { + body + } + } + } + `, DeferStreamOnValidOperations(), Invalid) }) - t.Run("disabled defer on root subscription field", func(t *testing.T) { + t.Run("non-defer inline fragment spread on root subscription field", func(t *testing.T) { run(t, ` subscription { ... @defer (if: false) { subscribeDog { name } } } - `, DeferStreamDirectiveOnRootFieldRule(), Valid) + `, DeferStreamOnValidOperations(), Valid) }) t.Run("stream field on root subscription field", func(t *testing.T) { run(t, ` subscription { subscribeDog @stream { name } } - `, DeferStreamDirectiveOnRootFieldRule(), Invalid) + `, DeferStreamOnValidOperations(), Invalid) }) - t.Run("disabled stream on root subscription field", func(t *testing.T) { + t.Run("stream on nested subscription field", func(t *testing.T) { run(t, ` subscription { - subscribeDog @stream (if: false) { name } + subscribeDogs { + extras @stream { string } + } } - `, DeferStreamDirectiveOnRootFieldRule(), Valid) + `, DeferStreamOnValidOperations(), Invalid) }) - - t.Run("defer on nested field", func(t *testing.T) { + t.Run("disabled stream on root subscription field", func(t *testing.T) { run(t, ` - query { - dog { - ... @defer { - extra { string } - } - } + subscription { + subscribeDog @stream (if: false) { name } } - `, DeferStreamDirectiveOnRootFieldRule(), Valid) + `, DeferStreamOnValidOperations(), Valid) }) - t.Run("defer with if argument as variable", func(t *testing.T) { + + t.Run("defer with variable if argument", func(t *testing.T) { run(t, ` query($shouldDefer: Boolean!) { ... @defer(if: $shouldDefer) { dog { name } } } - `, DeferStreamDirectiveOnRootFieldRule(), Valid) + `, DeferStreamOnValidOperations(), Valid) }) }) - t.Run("stream location", func(t *testing.T) { + t.Run("stream with lists only", func(t *testing.T) { t.Run("stream with positive initialCount argument", func(t *testing.T) { run(t, ` query { @@ -5430,13 +5486,6 @@ func DeferStreamComplexRule() Rule { } } -func DeferStreamDirectiveOnRootFieldRule() Rule { - // TODO: Implement root field validation - return func(walker *astvisitor.Walker) { - // Implementation needed - } -} - var testDefinition = ` directive @tag(name: String) on FIELD directive @stream(label: String, initialCount: Int, if: Boolean = true) on FIELD diff --git a/v2/pkg/operationreport/externalerror.go b/v2/pkg/operationreport/externalerror.go index 969d6d56e..ff715bd44 100644 --- a/v2/pkg/operationreport/externalerror.go +++ b/v2/pkg/operationreport/externalerror.go @@ -443,6 +443,13 @@ func ErrStreamInitialCountMustBeNonNegative(directiveName ast.ByteSlice, directi return err } +func ErrDeferStreamDirectiveNotAllowedOnRootField(directiveName ast.ByteSlice, operationType string, directivePosition position.Position) (err ExternalError) { + err.Message = fmt.Sprintf(`directive "@%s" is not allowed on root fields of %s operations`, + directiveName, operationType) + err.Locations = LocationsFromPosition(directivePosition) + return err +} + func ErrOnlyOneQueryTypeAllowed() (err ExternalError) { err.Message = "there can be only one query type in schema" return err From 6cee126035f038132fad3d7b25e2d7c6f7a64458 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Wed, 8 Oct 2025 13:55:17 +0300 Subject: [PATCH 07/11] refine error messages and tests --- ...ration_rule_defer_stream_on_root_fields.go | 6 +- ...eration_rule_defer_stream_unique_labels.go | 14 ++ .../operation_validation_test.go | 138 +++++++++++++----- v2/pkg/operationreport/externalerror.go | 7 + 4 files changed, 120 insertions(+), 45 deletions(-) diff --git a/v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go b/v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go index 4513c237e..d75e91820 100644 --- a/v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go +++ b/v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go @@ -68,10 +68,8 @@ func (d *deferStreamOnValidOpsVisitor) EnterDirective(ref int) { // For subscriptions, @defer and @stream are not allowed anywhere (root or nested) if d.currentOperationType == ast.OperationTypeSubscription { - operationTypeName := d.currentOperationType.Name() - d.StopWithExternalErr(operationreport.ErrDeferStreamDirectiveNotAllowedOnRootField( + d.StopWithExternalErr(operationreport.ErrDeferStreamDirectiveNotAllowedOnSubs( directiveName, - operationTypeName, directivePosition, )) return @@ -82,11 +80,9 @@ func (d *deferStreamOnValidOpsVisitor) EnterDirective(ref int) { return } - // For mutations, check if we're at the root level if len(d.Ancestors) == 0 { return } - // The directive's immediate parent (the node it's attached to) ancestor := d.Ancestors[len(d.Ancestors)-1] diff --git a/v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go b/v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go index 9d1886863..a9f19d140 100644 --- a/v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go +++ b/v2/pkg/astvalidation/operation_rule_defer_stream_unique_labels.go @@ -73,6 +73,20 @@ func (d *deferStreamLabelsVisitor) EnterDirective(ref int) { return } + if ifValue, hasIf := d.operation.DirectiveArgumentValueByName(ref, literal.IF); hasIf { + switch ifValue.Kind { + case ast.ValueKindBoolean: + // If "if: false", ignore the directive + if !d.operation.BooleanValue(ifValue.Ref) { + return + } + case ast.ValueKindVariable: + // If if: $variable, we can't statically determine if it's enabled, + // so we ignore this until variable's value is provided. + return + } + } + labelString := d.operation.StringValueContentString(labelValue.Ref) if previous, exists := d.seenLabels[labelString]; exists { diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index f9b39a296..df3315221 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4179,6 +4179,12 @@ type Query { }) t.Run("defer/stream directive", func(t *testing.T) { + allRules := []Rule{ + DeferStreamOnValidOperations(), + DeferStreamHaveUniqueLabels(), + DirectivesAreInValidLocations(), + StreamAppliedToListFieldsOnly(), + } t.Run("labels", func(t *testing.T) { t.Run("defer directive with unique labels", func(t *testing.T) { @@ -4284,6 +4290,38 @@ type Query { `, DeferStreamHaveUniqueLabels(), Invalid, withValidationErrors(`directive "@stream" label argument must be a static string value, not a variable`)) }) + t.Run("duplicate labels with one disabled defer", func(t *testing.T) { + runManyRules(t, ` + query { + dog { + ...fragment1 @defer(label: "a", if: false) + ...fragment2 @defer(label: "a") + } + } + fragment fragment1 on Dog { + name + } + fragment fragment2 on Dog { + nickname + } + `, Valid, allRules...) + }) + t.Run("duplicate labels with one optional defer", func(t *testing.T) { + runManyRules(t, ` + query q($b: Boolean) { + dog { + ...fragment1 @defer(label: "a", if: $b) + ...fragment2 @defer(label: "a") + } + } + fragment fragment1 on Dog { + name + } + fragment fragment2 on Dog { + nickname + } + `, Valid, allRules...) + }) }) t.Run("on valid operations", func(t *testing.T) { @@ -4318,8 +4356,6 @@ type Query { } `, DeferStreamOnValidOperations(), Valid) }) - - // stream t.Run("stream on root query field", func(t *testing.T) { run(t, ` query { @@ -4395,14 +4431,36 @@ type Query { }) t.Run("defer inline fragment spread on nested subscription field", func(t *testing.T) { run(t, ` - subscription { - newMessage { - ... @defer { - body + subscription { + newMessage { + ... @defer { + body + } + } } - } - } - `, DeferStreamOnValidOperations(), Invalid) + `, DeferStreamOnValidOperations(), Invalid) + }) + t.Run("defer inline nested fragment spreads on subscription field", func(t *testing.T) { + run(t, ` + subscription { + newMessage { + ... frag1 + } + } + fragment frag1 on Message { + ...frag2 + } + fragment frag2 on Message { + ...frag3 + } + fragment frag3 on Message { + ... @defer { + body + } + }`, + DeferStreamOnValidOperations(), + Invalid, + withValidationErrors(`directive "@defer" is not allowed on subscription operations`)) }) t.Run("non-defer inline fragment spread on root subscription field", func(t *testing.T) { run(t, ` @@ -4437,7 +4495,7 @@ type Query { `, DeferStreamOnValidOperations(), Valid) }) - t.Run("defer with variable if argument", func(t *testing.T) { + t.Run("defer with variable if argument on query", func(t *testing.T) { run(t, ` query($shouldDefer: Boolean!) { ... @defer(if: $shouldDefer) { @@ -4446,6 +4504,15 @@ type Query { } `, DeferStreamOnValidOperations(), Valid) }) + t.Run("defer with variable if argument on subscription", func(t *testing.T) { + run(t, ` + subscription($shouldDefer: Boolean!) { + ... @defer(if: $shouldDefer) { + dog { name } + } + } + `, DeferStreamOnValidOperations(), Valid) + }) }) t.Run("stream with lists only", func(t *testing.T) { @@ -4549,7 +4616,7 @@ type Query { t.Run("complex scenarios", func(t *testing.T) { t.Run("nested defer directives", func(t *testing.T) { - run(t, ` + runManyRules(t, ` query { dog { ... @defer { @@ -4560,20 +4627,20 @@ type Query { } } } - `, DeferStreamComplexRule(), Valid) + `, Valid, allRules...) }) t.Run("defer and stream in same query", func(t *testing.T) { - run(t, ` + runManyRules(t, ` query { dog { ... @defer { name } extras @stream { string } } } - `, DeferStreamComplexRule(), Valid) + `, Valid, allRules...) }) t.Run("stream on multiple fields with unique labels", func(t *testing.T) { - run(t, ` + runManyRules(t, ` query { dog { extra { @@ -4592,26 +4659,11 @@ type Query { strings @stream(label: "extrasStrings") } } - `, DeferStreamComplexRule(), Valid) - }) - t.Run("duplicate labels with one disabled defer", func(t *testing.T) { - run(t, ` - query { - dog { - ...fragment1 @defer(label: "a", if: false) - ...fragment2 @defer(label: "a") - } - } - fragment fragment1 on Dog { - name - } - fragment fragment2 on Dog { - nickname - } - `, DeferStreamComplexRule(), Valid) + `, Valid, allRules...) }) + t.Run("deeply nested defer with unique labels", func(t *testing.T) { - run(t, ` + runManyRules(t, ` query { dog { ... @defer(label: "level1") { @@ -4626,9 +4678,12 @@ type Query { } } fragment fragment1 on Dog { - mustStrings + mustExtra + ... @defer(label: "level4") { + mustExtras + } } - `, DeferStreamComplexRule(), Valid) + `, Valid, allRules...) }) t.Run("deeply nested defer with duplicate labels", func(t *testing.T) { run(t, ` @@ -4639,19 +4694,22 @@ type Query { extras { ... @defer(label: "level2") { string - ... fragment1 @defer(label: "level1") + ... fragment1 } } } } } fragment fragment1 on Dog { - mustStrings + mustExtra + ... @defer(label: "level1") { + mustExtras + } } - `, DeferStreamComplexRule(), Invalid) + `, DeferStreamHaveUniqueLabels(), Invalid) }) t.Run("defer on typed inline fragment", func(t *testing.T) { - run(t, ` + runManyRules(t, ` query { extras { ... on CatExtra @defer { @@ -4662,7 +4720,7 @@ type Query { } } } - `, DirectivesAreInValidLocations(), Valid) + `, Valid, allRules...) }) }) diff --git a/v2/pkg/operationreport/externalerror.go b/v2/pkg/operationreport/externalerror.go index ff715bd44..349728ff1 100644 --- a/v2/pkg/operationreport/externalerror.go +++ b/v2/pkg/operationreport/externalerror.go @@ -443,6 +443,13 @@ func ErrStreamInitialCountMustBeNonNegative(directiveName ast.ByteSlice, directi return err } +func ErrDeferStreamDirectiveNotAllowedOnSubs(directiveName ast.ByteSlice, directivePosition position.Position) (err ExternalError) { + err.Message = fmt.Sprintf(`directive "@%s" is not allowed on subscription operations`, + directiveName) + err.Locations = LocationsFromPosition(directivePosition) + return err +} + func ErrDeferStreamDirectiveNotAllowedOnRootField(directiveName ast.ByteSlice, operationType string, directivePosition position.Position) (err ExternalError) { err.Message = fmt.Sprintf(`directive "@%s" is not allowed on root fields of %s operations`, directiveName, operationType) From 21978739bcb335d24b98c6ec57ff52652c791214 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Wed, 8 Oct 2025 17:18:19 +0300 Subject: [PATCH 08/11] reformat tests --- .../operation_validation_test.go | 525 +++++++++--------- 1 file changed, 249 insertions(+), 276 deletions(-) diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index df3315221..954bcf322 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4197,7 +4197,7 @@ type Query { } fragment dogFragment on Dog { name } fragment otherFragment on Dog { nickname } - `, DeferStreamHaveUniqueLabels(), Valid) + `, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("stream directive with unique labels", func(t *testing.T) { @@ -4208,7 +4208,7 @@ type Query { mustExtras @stream(label: "mustExtrasStream") { string } } } - `, DeferStreamHaveUniqueLabels(), Valid) + `, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("defer and stream with same label", func(t *testing.T) { @@ -4220,9 +4220,10 @@ type Query { } } fragment dogFragment on Dog { name } - `, DeferStreamHaveUniqueLabels(), Invalid, withValidationErrors( - `directive "@stream" label "sameLabel" must be unique, but was already used on "@defer" directive`, - )) + `, DeferStreamHaveUniqueLabels(), + Invalid, + withValidationErrors(`directive "@stream" label "sameLabel" must be unique, but was already used on "@defer" directive`), + ) }) t.Run("defer directives with same label", func(t *testing.T) { run(t, ` @@ -4234,7 +4235,9 @@ type Query { } fragment dogFragment on Dog { name } fragment otherFragment on Dog { nickname } - `, DeferStreamHaveUniqueLabels(), Invalid, withDisableNormalization(), + `, DeferStreamHaveUniqueLabels(), + Invalid, + withDisableNormalization(), withValidationErrors(`directive "@defer" label "duplicateLabel" must be unique`)) }) t.Run("multiple stream directives with same label", func(t *testing.T) { @@ -4244,8 +4247,8 @@ type Query { extras @stream(label: "duplicateLabel") { string } mustExtras @stream(label: "duplicateLabel") { string } } - } - `, DeferStreamHaveUniqueLabels(), Invalid, + }`, DeferStreamHaveUniqueLabels(), + Invalid, withValidationErrors(`directive "@stream" label "duplicateLabel" must be unique`)) }) t.Run("defer without label", func(t *testing.T) { @@ -4258,7 +4261,7 @@ type Query { } fragment dogFragmentA on Dog { name } fragment dogFragmentB on Dog { nickname } - `, DeferStreamHaveUniqueLabels(), Valid) + `, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("stream without label", func(t *testing.T) { run(t, ` @@ -4266,8 +4269,7 @@ type Query { dog { extras @stream { string } } - } - `, DeferStreamHaveUniqueLabels(), Valid) + }`, DeferStreamHaveUniqueLabels(), Valid) }) t.Run("defer directive with variable label", func(t *testing.T) { run(t, ` @@ -4277,7 +4279,8 @@ type Query { } } fragment dogFragment on Dog { name } - `, DeferStreamHaveUniqueLabels(), Invalid, + `, DeferStreamHaveUniqueLabels(), + Invalid, withValidationErrors(`directive "@defer" label argument must be a static string value, not a variable`)) }) t.Run("stream directive with variable label", func(t *testing.T) { @@ -4287,45 +4290,38 @@ type Query { extras @stream(label: $label) { string } } } - `, DeferStreamHaveUniqueLabels(), Invalid, + `, DeferStreamHaveUniqueLabels(), + Invalid, withValidationErrors(`directive "@stream" label argument must be a static string value, not a variable`)) }) t.Run("duplicate labels with one disabled defer", func(t *testing.T) { runManyRules(t, ` - query { - dog { - ...fragment1 @defer(label: "a", if: false) - ...fragment2 @defer(label: "a") + query { + dog { + ...fragment1 @defer(label: "a", if: false) + ...fragment2 @defer(label: "a") + } } - } - fragment fragment1 on Dog { - name - } - fragment fragment2 on Dog { - nickname - } - `, Valid, allRules...) + fragment fragment1 on Dog { name } + fragment fragment2 on Dog { nickname } + `, Valid, allRules...) }) t.Run("duplicate labels with one optional defer", func(t *testing.T) { runManyRules(t, ` - query q($b: Boolean) { - dog { - ...fragment1 @defer(label: "a", if: $b) - ...fragment2 @defer(label: "a") + query q($b: Boolean) { + dog { + ...fragment1 @defer(label: "a", if: $b) + ...fragment2 @defer(label: "a") + } } - } - fragment fragment1 on Dog { - name - } - fragment fragment2 on Dog { - nickname - } - `, Valid, allRules...) + fragment fragment1 on Dog { name } + fragment fragment2 on Dog { nickname } + `, Valid, allRules...) }) }) - t.Run("on valid operations", func(t *testing.T) { - // query + t.Run("on operations", func(t *testing.T) { + // on queries t.Run("defer inline fragment spread on root query field", func(t *testing.T) { run(t, ` query { @@ -4348,86 +4344,77 @@ type Query { }) t.Run("defer fragment spread on root query field", func(t *testing.T) { run(t, ` - query { - ...rootFragment @defer - } - fragment rootFragment on Query { - extras { string } - } - `, DeferStreamOnValidOperations(), Valid) + query { + ...rootFragment @defer + } + fragment rootFragment on Query { + extras { string } + }`, DeferStreamOnValidOperations(), Valid) }) t.Run("stream on root query field", func(t *testing.T) { run(t, ` - query { - extras @stream { string } - } - `, DeferStreamOnValidOperations(), Valid) + query { + extras @stream { string } + }`, DeferStreamOnValidOperations(), Valid) }) t.Run("stream field on fragment on root query field", func(t *testing.T) { run(t, ` - query { - ...rootFragment - } - fragment rootFragment on Query { - extras @stream { string } - } - `, DeferStreamOnValidOperations(), Valid) + query { + ...rootFragment + } + fragment rootFragment on Query { + extras @stream { string } + }`, DeferStreamOnValidOperations(), Valid) }) - // mutation + // on mutations t.Run("defer inline fragment spread on root mutation field", func(t *testing.T) { run(t, ` - mutation { - ... @defer { - mutateDog { name } - } - } - `, DeferStreamOnValidOperations(), Invalid) + mutation { + ... @defer { + mutateDog { name } + } + }`, DeferStreamOnValidOperations(), Invalid) }) t.Run("defer fragment spread on nested mutation field", func(t *testing.T) { run(t, ` - mutation { - mutateDog { - ... @defer { - extra { string } + mutation { + mutateDog { + ... @defer { + extra { string } + } } - } - } - `, DeferStreamOnValidOperations(), Valid) + }`, DeferStreamOnValidOperations(), Valid) }) t.Run("non-defer inline fragment spread on root mutation field", func(t *testing.T) { run(t, ` - mutation { - ... @defer (if: false) { - mutateDog { name } - } - } - `, DeferStreamOnValidOperations(), Valid) + mutation { + ... @defer (if: false) { + mutateDog { name } + } + }`, DeferStreamOnValidOperations(), Valid) }) t.Run("stream field on root mutation field", func(t *testing.T) { run(t, ` - mutation { - mutateDogs @stream { name } - } - `, DeferStreamOnValidOperations(), Invalid) + mutation { + mutateDogs @stream { name } + }`, DeferStreamOnValidOperations(), Invalid) }) t.Run("disabled stream on root mutation field", func(t *testing.T) { run(t, ` - mutation { - mutateDogs @stream (if: false) { name } - } - `, DeferStreamOnValidOperations(), Valid) + mutation { + mutateDogs @stream (if: false) { name } + }`, DeferStreamOnValidOperations(), Valid) }) - // subscriptions + // on subscriptions t.Run("defer inline fragment spread on root subscription field", func(t *testing.T) { run(t, ` - subscription { - ... @defer { - subscribeDog { name } - } - } - `, DeferStreamOnValidOperations(), Invalid) + subscription { + ... @defer { + subscribeDog { name } + } + }`, DeferStreamOnValidOperations(), Invalid) }) t.Run("defer inline fragment spread on nested subscription field", func(t *testing.T) { run(t, ` @@ -4437,8 +4424,7 @@ type Query { body } } - } - `, DeferStreamOnValidOperations(), Invalid) + }`, DeferStreamOnValidOperations(), Invalid) }) t.Run("defer inline nested fragment spreads on subscription field", func(t *testing.T) { run(t, ` @@ -4447,16 +4433,10 @@ type Query { ... frag1 } } - fragment frag1 on Message { - ...frag2 - } - fragment frag2 on Message { - ...frag3 - } + fragment frag1 on Message { ...frag2 } + fragment frag2 on Message { ...frag3 } fragment frag3 on Message { - ... @defer { - body - } + ... @defer { body } }`, DeferStreamOnValidOperations(), Invalid, @@ -4464,263 +4444,242 @@ type Query { }) t.Run("non-defer inline fragment spread on root subscription field", func(t *testing.T) { run(t, ` - subscription { - ... @defer (if: false) { - subscribeDog { name } - } - } - `, DeferStreamOnValidOperations(), Valid) + subscription { + ... @defer (if: false) { + subscribeDog { name } + } + }`, DeferStreamOnValidOperations(), Valid) }) t.Run("stream field on root subscription field", func(t *testing.T) { run(t, ` - subscription { - subscribeDog @stream { name } - } - `, DeferStreamOnValidOperations(), Invalid) + subscription { + subscribeDog @stream { name } + }`, DeferStreamOnValidOperations(), Invalid) }) t.Run("stream on nested subscription field", func(t *testing.T) { run(t, ` - subscription { - subscribeDogs { - extras @stream { string } - } - } - `, DeferStreamOnValidOperations(), Invalid) + subscription { + subscribeDogs { + extras @stream { string } + } + }`, DeferStreamOnValidOperations(), Invalid) }) t.Run("disabled stream on root subscription field", func(t *testing.T) { run(t, ` - subscription { - subscribeDog @stream (if: false) { name } - } - `, DeferStreamOnValidOperations(), Valid) + subscription { + subscribeDog @stream (if: false) { name } + }`, DeferStreamOnValidOperations(), Valid) }) t.Run("defer with variable if argument on query", func(t *testing.T) { run(t, ` - query($shouldDefer: Boolean!) { - ... @defer(if: $shouldDefer) { - dog { name } - } - } - `, DeferStreamOnValidOperations(), Valid) + query($shouldDefer: Boolean!) { + ... @defer(if: $shouldDefer) { + dog { name } + } + }`, DeferStreamOnValidOperations(), Valid) }) t.Run("defer with variable if argument on subscription", func(t *testing.T) { run(t, ` - subscription($shouldDefer: Boolean!) { - ... @defer(if: $shouldDefer) { - dog { name } - } - } - `, DeferStreamOnValidOperations(), Valid) + subscription($shouldDefer: Boolean!) { + ... @defer(if: $shouldDefer) { + dog { name } + } + }`, DeferStreamOnValidOperations(), Valid) }) }) t.Run("stream with lists only", func(t *testing.T) { t.Run("stream with positive initialCount argument", func(t *testing.T) { run(t, ` - query { - dog { - extras @stream(initialCount: 5) { string } - } - } - `, StreamAppliedToListFieldsOnly(), Valid) + query { + dog { + extras @stream(initialCount: 5) { string } + } + }`, StreamAppliedToListFieldsOnly(), Valid) }) t.Run("stream with negative initialCount argument", func(t *testing.T) { run(t, ` - query { - dog { - extras @stream(initialCount: -1) { string } - } - } - `, StreamAppliedToListFieldsOnly(), Invalid) + query { + dog { + extras @stream(initialCount: -1) { string } + } + }`, StreamAppliedToListFieldsOnly(), Invalid) }) t.Run("stream on list field", func(t *testing.T) { run(t, ` - query { - dog { - extras @stream { string } - } - } - `, StreamAppliedToListFieldsOnly(), Valid) + query { + dog { + extras @stream { string } + } + }`, StreamAppliedToListFieldsOnly(), Valid) }) t.Run("stream on root list field", func(t *testing.T) { run(t, ` - query { - extras @stream { name } - } - `, StreamAppliedToListFieldsOnly(), Valid) + query { + extras @stream { name } + }`, StreamAppliedToListFieldsOnly(), Valid) }) t.Run("stream on non-list field", func(t *testing.T) { run(t, ` - query { - dog @stream { name } - } - `, StreamAppliedToListFieldsOnly(), Invalid) + query { + dog @stream { name } + }`, StreamAppliedToListFieldsOnly(), Invalid) }) t.Run("stream on scalar field", func(t *testing.T) { run(t, ` - query { - dog { - name @stream - } - } - `, StreamAppliedToListFieldsOnly(), Invalid) + query { + dog { name @stream } + }`, StreamAppliedToListFieldsOnly(), Invalid) }) }) - t.Run("defer location", func(t *testing.T) { + t.Run("valid location", func(t *testing.T) { t.Run("defer on inline fragment", func(t *testing.T) { run(t, ` - query { - pet { - ... on Dog @defer { name } - } - } - `, DirectivesAreInValidLocations(), Valid) + query { + pet { + ... on Dog @defer { name } + } + }`, DirectivesAreInValidLocations(), Valid) }) t.Run("defer on fragment spread", func(t *testing.T) { run(t, ` - query { - dog { - ...dogFragment @defer + query { + dog { + ...dogFragment @defer + } } - } - fragment dogFragment on Dog { name } - `, DirectivesAreInValidLocations(), Valid) + fragment dogFragment on Dog { name }`, + DirectivesAreInValidLocations(), Valid) }) t.Run("defer on field", func(t *testing.T) { run(t, ` - query { - dog @defer { name } - } - `, DirectivesAreInValidLocations(), Invalid, withValidationErrors("defer not allowed on node of kind: FIELD")) + query { + dog @defer { name } + }`, + DirectivesAreInValidLocations(), + Invalid, + withValidationErrors("defer not allowed on node of kind: FIELD")) }) t.Run("stream on fragment spread", func(t *testing.T) { run(t, ` - query { - ...extrasFrag @stream - } - fragment extrasFrag on Query { - extras { - ... on DogExtra { - string - } - ... on CatExtra { - string2 - } + query { + ...extrasFrag @stream } - } - `, DirectivesAreInValidLocations(), Invalid, withValidationErrors("stream not allowed on node of kind: INLINE_FRAGMENT")) + fragment extrasFrag on Query { + extras { + ... on DogExtra { string } + ... on CatExtra { string2 } + } + }`, + DirectivesAreInValidLocations(), + Invalid, + withValidationErrors("stream not allowed on node of kind: INLINE_FRAGMENT")) }) }) - t.Run("complex scenarios", func(t *testing.T) { + t.Run("complex", func(t *testing.T) { t.Run("nested defer directives", func(t *testing.T) { runManyRules(t, ` - query { - dog { - ... @defer { - name - extra { - ... @defer { string } + query { + dog { + ... @defer { + name + extra { + ... @defer { string } + } } } - } - } - `, Valid, allRules...) + }`, Valid, allRules...) }) t.Run("defer and stream in same query", func(t *testing.T) { runManyRules(t, ` - query { - dog { - ... @defer { name } - extras @stream { string } - } - } - `, Valid, allRules...) + query { + dog { + ... @defer { name } + extras @stream { string } + } + }`, Valid, allRules...) }) t.Run("stream on multiple fields with unique labels", func(t *testing.T) { runManyRules(t, ` - query { - dog { - extra { - mustStrings @stream(label: "dogExtraStream") + query { + dog { + extra { + mustStrings @stream(label: "dogExtraStream") + } + extras @stream(label: "dogExtras") { + string + } } - extras @stream(label: "dogExtras") { - string + cat { + extra { + mustStrings @stream(label: "catExtraStrings") + } } - } - cat { - extra { - mustStrings @stream(label: "catExtraStrings") + extras @stream(label: "rootExtras") { + strings @stream(label: "extrasStrings") } - } - extras @stream(label: "rootExtras") { - strings @stream(label: "extrasStrings") - } - } - `, Valid, allRules...) + }`, Valid, allRules...) }) t.Run("deeply nested defer with unique labels", func(t *testing.T) { runManyRules(t, ` - query { - dog { - ... @defer(label: "level1") { - name - extras { - ... @defer(label: "level2") { - string - ... fragment1 @defer(label: "level3") + query { + dog { + ... @defer(label: "level1") { + name + extras { + ... @defer(label: "level2") { + string + ... fragment1 @defer(label: "level3") + } } } } } - } - fragment fragment1 on Dog { - mustExtra - ... @defer(label: "level4") { - mustExtras - } - } - `, Valid, allRules...) + fragment fragment1 on Dog { + mustExtra + ... @defer(label: "level4") { + mustExtras + } + }`, Valid, allRules...) }) t.Run("deeply nested defer with duplicate labels", func(t *testing.T) { run(t, ` - query { - dog { - ... @defer(label: "level1") { - name - extras { - ... @defer(label: "level2") { - string - ... fragment1 + query { + dog { + ... @defer(label: "level1") { + name + extras { + ... @defer(label: "level2") { + string + ... fragment1 + } } } } } - } - fragment fragment1 on Dog { - mustExtra - ... @defer(label: "level1") { - mustExtras - } - } - `, DeferStreamHaveUniqueLabels(), Invalid) + fragment fragment1 on Dog { + mustExtra + ... @defer(label: "level1") { + mustExtras + } + }`, DeferStreamHaveUniqueLabels(), Invalid) }) t.Run("defer on typed inline fragment", func(t *testing.T) { runManyRules(t, ` - query { - extras { - ... on CatExtra @defer { - string1 - } - ... on DogExtra @defer { - string2 + query { + extras { + ... on CatExtra @defer { + string1 + } + ... on DogExtra @defer { + string2 + } } - } - } - `, Valid, allRules...) + }`, Valid, allRules...) }) }) @@ -4744,6 +4703,20 @@ type Query { } `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) }) + t.Run("different stream directive label separate fragments", func(t *testing.T) { + run(t, ` + query { dog { + ...dogFragment1 + ...dogFragment2 + } } + fragment dogFragment1 on Dog { + extras @stream(label: "one", initialCount: 1) + } + fragment dogFragment2 on Dog { + extras @stream(label: "two", initialCount: 1) + } + `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) + }) t.Run("different stream directive initialCount", func(t *testing.T) { run(t, ` query { dog { ...dogFragment } } @@ -4751,7 +4724,7 @@ type Query { extras @stream(label: "same", initialCount: 1) extras @stream(label: "same", initialCount: 5) } - `, FieldSelectionMerging(), Invalid) + `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive: first missing args", func(t *testing.T) { run(t, ` @@ -4760,7 +4733,7 @@ type Query { extras @stream extras @stream(label: "two", initialCount: 5) } - `, FieldSelectionMerging(), Invalid) + `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive: second missing args", func(t *testing.T) { run(t, ` @@ -4769,7 +4742,7 @@ type Query { extras @stream(label: "one", initialCount: 1) extras @stream } - `, FieldSelectionMerging(), Invalid) + `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("mix of stream and no stream", func(t *testing.T) { run(t, ` @@ -4778,7 +4751,7 @@ type Query { extras extras @stream } - `, FieldSelectionMerging(), Invalid) + `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive both missing args", func(t *testing.T) { run(t, ` From 12c1e88bec74e2805d6c45d1be65e63e15daa42a Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Thu, 9 Oct 2025 15:01:53 +0300 Subject: [PATCH 09/11] implement merging for streams --- .gitignore | 3 +- v2/pkg/ast/ast_directive.go | 7 + .../inline_fragment_selection_merging.go | 2 + .../operation_rule_field_selection_merging.go | 60 +++-- .../operation_validation_test.go | 205 +++++++++++++----- v2/pkg/operationreport/externalerror.go | 5 + 6 files changed, 207 insertions(+), 75 deletions(-) diff --git a/.gitignore b/.gitignore index 960ca0c8d..ddccb5cd8 100644 --- a/.gitignore +++ b/.gitignore @@ -5,4 +5,5 @@ .DS_Store pkg/parser/testdata/lotto.graphql *node_modules* -*vendor* \ No newline at end of file +*vendor* +/.cursorrules diff --git a/v2/pkg/ast/ast_directive.go b/v2/pkg/ast/ast_directive.go index 9b70b521a..b4d4f1664 100644 --- a/v2/pkg/ast/ast_directive.go +++ b/v2/pkg/ast/ast_directive.go @@ -135,10 +135,17 @@ func (d *Document) DirectiveSetsHasCompatibleStreamDirective(left, right []int) leftRef, leftExists := d.DirectiveWithNameBytes(left, literal.STREAM) rightRef, rightExists := d.DirectiveWithNameBytes(right, literal.STREAM) + // Both have @stream: they must be equal if leftExists && rightExists { return d.DirectivesAreEqual(leftRef, rightRef) } + // One has @stream, the other doesn't: incompatible + if leftExists != rightExists { + return false + } + + // Neither has @stream: compatible return true } diff --git a/v2/pkg/astnormalization/inline_fragment_selection_merging.go b/v2/pkg/astnormalization/inline_fragment_selection_merging.go index 118c9eb53..8a54ca0fb 100644 --- a/v2/pkg/astnormalization/inline_fragment_selection_merging.go +++ b/v2/pkg/astnormalization/inline_fragment_selection_merging.go @@ -70,6 +70,8 @@ func (f *inlineFragmentSelectionMergeVisitor) fieldsCanMerge(left, right int) bo leftDirectives := f.operation.FieldDirectives(left) rightDirectives := f.operation.FieldDirectives(right) + // For fields with selections, check that all directives are equal + // This ensures @skip, @include, @defer and @stream all match return f.operation.DirectiveSetsAreEqual(leftDirectives, rightDirectives) } diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index 9e762aa32..3c0901c67 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -9,7 +9,14 @@ import ( "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" ) -// FieldSelectionMerging validates if field selections can be merged +// FieldSelectionMerging returns a validation rule that ensures field selections can be merged. +// +// This rule implements the validation described in the GraphQL specification section 5.3.2: +// "Field Selection Merging". It ensures that when multiple fields with the same response key +// (name or alias) are selected in overlapping selection sets, they can be unambiguously merged +// into a single field in the response. +// +// The rule is applied to each operation and fragment definition in the document. func FieldSelectionMerging() Rule { return func(walker *astvisitor.Walker) { visitor := fieldSelectionMergingVisitor{Walker: walker} @@ -31,6 +38,7 @@ type fieldSelectionMergingVisitor struct { type nonScalarRequirement struct { path ast.Path objectName ast.ByteSlice + fieldRef int fieldTypeRef int fieldTypeDefinitionNode ast.Node } @@ -107,41 +115,57 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { if fieldDefinitionTypeNode.Kind != ast.NodeKindScalarTypeDefinition { matchedRequirements := f.NonScalarRequirementsByPathField(path, objectName) - fieldDefinitionTypeKindPresentInRequirements := false + hasDifferentKindInRequirements := false for _, i := range matchedRequirements { if !f.potentiallySameObject(fieldDefinitionTypeNode, f.nonScalarRequirements[i].fieldTypeDefinitionNode) { + // This condition below can never be true because if objects aren't potentially the same, + // and we know objectNames are equal (from the filter), they cannot be not equal at the same time. + // Perhaps this should be an else case or restructured? if !objectName.Equals(f.nonScalarRequirements[i].objectName) { f.StopWithExternalErr(operationreport.ErrResponseOfDifferingTypesMustBeOfSameShape(objectName, f.nonScalarRequirements[i].objectName)) return } - } else if !f.definition.TypesAreCompatibleDeep(f.nonScalarRequirements[i].fieldTypeRef, fieldType) { - left, err := f.definition.PrintTypeBytes(f.nonScalarRequirements[i].fieldTypeRef, nil) - if err != nil { - f.StopWithInternalErr(err) + } else { + // Check stream directive compatibility for non-scalar fields + leftDirectives := f.operation.FieldDirectives(f.nonScalarRequirements[i].fieldRef) + rightDirectives := f.operation.FieldDirectives(ref) + if !f.operation.DirectiveSetsHasCompatibleStreamDirective(leftDirectives, rightDirectives) { + f.StopWithExternalErr(operationreport.ErrConflictingStreamDirectivesOnField(objectName)) return } - right, err := f.definition.PrintTypeBytes(fieldType, nil) - if err != nil { - f.StopWithInternalErr(err) + + if !f.definition.TypesAreCompatibleDeep(f.nonScalarRequirements[i].fieldTypeRef, fieldType) { + left, err := f.definition.PrintTypeBytes(f.nonScalarRequirements[i].fieldTypeRef, nil) + if err != nil { + f.StopWithInternalErr(err) + return + } + right, err := f.definition.PrintTypeBytes(fieldType, nil) + if err != nil { + f.StopWithInternalErr(err) + return + } + f.StopWithExternalErr(operationreport.ErrTypesForFieldMismatch(objectName, left, right)) return } - f.StopWithExternalErr(operationreport.ErrTypesForFieldMismatch(objectName, left, right)) - return } if fieldDefinitionTypeNode.Kind != f.nonScalarRequirements[i].fieldTypeDefinitionNode.Kind { - fieldDefinitionTypeKindPresentInRequirements = true + hasDifferentKindInRequirements = true } } - if len(matchedRequirements) != 0 && fieldDefinitionTypeKindPresentInRequirements { + if hasDifferentKindInRequirements { + // If we've already checked this field against a requirement with a different Kind, + // we don't need to add it again to requirements. return } f.nonScalarRequirements = append(f.nonScalarRequirements, nonScalarRequirement{ path: path, objectName: objectName, + fieldRef: ref, fieldTypeRef: fieldType, fieldTypeDefinitionNode: fieldDefinitionTypeNode, }) @@ -149,7 +173,7 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { } matchedRequirements := f.ScalarRequirementsByPathField(path, objectName) - fieldDefinitionTypeKindPresentInRequirements := false + hasDifferentKindInRequirements := false for _, i := range matchedRequirements { if f.potentiallySameObject(f.scalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) { @@ -175,11 +199,11 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { } if fieldDefinitionTypeNode.Kind != f.scalarRequirements[i].fieldTypeDefinitionNode.Kind { - fieldDefinitionTypeKindPresentInRequirements = true + hasDifferentKindInRequirements = true } } - if len(matchedRequirements) != 0 && fieldDefinitionTypeKindPresentInRequirements { + if hasDifferentKindInRequirements { return } @@ -203,7 +227,3 @@ func (f *fieldSelectionMergingVisitor) potentiallySameObject(left, right ast.Nod return false } } - -func (f *fieldSelectionMergingVisitor) EnterSelectionSet(_ int) { - -} diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index 954bcf322..c04c765fd 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -1552,7 +1552,9 @@ func TestExecutionValidation(t *testing.T) { doesKnowCommand(dogCommand: SIT) doesKnowCommand }`, - FieldSelectionMerging(), Invalid) + FieldSelectionMerging(), + Invalid, + withValidationErrors(`differing fields for objectName 'doesKnowCommand'`)) }) t.Run("111", func(t *testing.T) { run(t, ` @@ -4687,81 +4689,176 @@ type Query { t.Run("stream merging", func(t *testing.T) { t.Run("same stream directives supported", func(t *testing.T) { run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras @stream(label: "same", initialCount: 1) - extras @stream(label: "same", initialCount: 1) - } - `, FieldSelectionMerging(), Valid) + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "same", initialCount: 1) + extras @stream(label: "same", initialCount: 1) + }`, FieldSelectionMerging(), Valid) }) t.Run("different stream directive label", func(t *testing.T) { run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras @stream(label: "one", initialCount: 1) - extras @stream(label: "two", initialCount: 1) - } - `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "one", initialCount: 1) + extras @stream(label: "two", initialCount: 1) + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive label separate fragments", func(t *testing.T) { run(t, ` - query { dog { - ...dogFragment1 - ...dogFragment2 - } } - fragment dogFragment1 on Dog { - extras @stream(label: "one", initialCount: 1) - } - fragment dogFragment2 on Dog { - extras @stream(label: "two", initialCount: 1) - } - `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) + query { dog { + ...dogFragment1 + ...dogFragment2 + } } + fragment dogFragment1 on Dog { + extras @stream(label: "one", initialCount: 1) + } + fragment dogFragment2 on Dog { + extras @stream(label: "two", initialCount: 1) + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive initialCount", func(t *testing.T) { run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras @stream(label: "same", initialCount: 1) - extras @stream(label: "same", initialCount: 5) - } - `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "same", initialCount: 1) + extras @stream(label: "same", initialCount: 5) + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive: first missing args", func(t *testing.T) { run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras @stream - extras @stream(label: "two", initialCount: 5) - } - `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream + extras @stream(label: "two", initialCount: 5) + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive: second missing args", func(t *testing.T) { run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras @stream(label: "one", initialCount: 1) - extras @stream - } - `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "one", initialCount: 1) + extras @stream + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("mix of stream and no stream", func(t *testing.T) { run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras - extras @stream - } - `, FieldSelectionMerging(), Invalid, withValidationErrors(`found conflicting stream directives on the same field`)) + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras + extras @stream + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) }) t.Run("different stream directive both missing args", func(t *testing.T) { run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras @stream - extras @stream - } - `, FieldSelectionMerging(), Valid) + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream + extras @stream + }`, FieldSelectionMerging(), Valid) + }) + t.Run("on union type, different members, same field name", func(t *testing.T) { + run(t, ` + query { + extras { + ... on DogExtra { + strings @stream(label: "dog") + } + ... on CatExtra { + strings @stream(label: "cat") + } + } + }`, FieldSelectionMerging(), Valid) + }) + t.Run("on same field with different aliases", func(t *testing.T) { + run(t, ` + query { dog { + list1: extras @stream(label: "one") + list2: extras @stream(label: "two") + } }`, FieldSelectionMerging(), Valid) + }) + t.Run("on same field with same alias different streams", func(t *testing.T) { + run(t, ` + query { dog { + list: extras @stream(label: "one") + list: extras @stream(label: "two") + } }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) + }) + t.Run("on inline fragments of same type", func(t *testing.T) { + run(t, ` + query { dog { + ... on Dog { + extras @stream(label: "one") + } + ... on Dog { + extras @stream(label: "two") + } + } }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) }) + t.Run("nested stream directive conflicts", func(t *testing.T) { + run(t, ` + query { dog { + extra { + strings @stream(label: "one") + } + extra { + strings @stream(label: "two") + } + } }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`differing fields for objectName 'strings'`)) + }) + t.Run("multiple no-stream selections with one stream", func(t *testing.T) { + run(t, ` + query { dog { + extras + extras + extras @stream(label: "one") + } }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) + }) + + // conditionals in streams are not implemented yet, some tests are disabled for now. + t.Run("with different if conditions", func(t *testing.T) { + run(t, ` + query { dog { ...dogFragment } } + fragment dogFragment on Dog { + extras @stream(label: "same", initialCount: 1, if: true) + extras @stream(label: "same", initialCount: 1, if: false) + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) + }) + t.Run("with variable if conditions", func(t *testing.T) { + run(t, ` + query($cond1: Boolean!, $cond2: Boolean!) { + dog { + extras @stream(label: "same", initialCount: 1, if: $cond1) + extras @stream(label: "same", initialCount: 1, if: $cond2) + } + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`found conflicting stream directives on the same field`)) + }) + // t.Run("with if true vs no if argument", func(t *testing.T) { + // run(t, ` + // query { dog { + // extras @stream(label: "same", initialCount: 1, if: true) + // extras @stream(label: "same", initialCount: 1) + // } }`, FieldSelectionMerging(), Valid) + // }) + // t.Run("with if false vs no stream", func(t *testing.T) { + // run(t, ` + // query { dog { + // extras + // extras @stream(label: "one", if: false) + // } }`, FieldSelectionMerging(), Invalid, + // withValidationErrors(`found conflicting stream directives on the same field`)) + // }) }) }) } diff --git a/v2/pkg/operationreport/externalerror.go b/v2/pkg/operationreport/externalerror.go index 349728ff1..725c13f43 100644 --- a/v2/pkg/operationreport/externalerror.go +++ b/v2/pkg/operationreport/externalerror.go @@ -158,6 +158,11 @@ func ErrDifferingFieldsOnPotentiallySameType(objectName ast.ByteSlice) (err Exte return err } +func ErrConflictingStreamDirectivesOnField(fieldName ast.ByteSlice) (err ExternalError) { + err.Message = fmt.Sprintf("found conflicting stream directives on the same field '%s'", fieldName) + return err +} + func ErrFieldSelectionOnLeaf(enumTypeName ast.ByteSlice, typeName string, position position.Position) (err ExternalError) { err.Message = fmt.Sprintf(`Field "%s" must not have a selection since type "%s" has no subfields.`, enumTypeName, typeName) err.Locations = LocationsFromPosition(position) From aded752f84cef9b4569a3dd7709a34d8c2674375 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Tue, 14 Oct 2025 17:36:54 +0300 Subject: [PATCH 10/11] disable and comment tests for stream ifs --- .../operation_validation_test.go | 47 ++++++++++--------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index c04c765fd..fb7d78ff7 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4825,25 +4825,29 @@ type Query { }) // conditionals in streams are not implemented yet, some tests are disabled for now. - t.Run("with different if conditions", func(t *testing.T) { - run(t, ` - query { dog { ...dogFragment } } - fragment dogFragment on Dog { - extras @stream(label: "same", initialCount: 1, if: true) - extras @stream(label: "same", initialCount: 1, if: false) - }`, FieldSelectionMerging(), Invalid, - withValidationErrors(`found conflicting stream directives on the same field`)) - }) - t.Run("with variable if conditions", func(t *testing.T) { - run(t, ` - query($cond1: Boolean!, $cond2: Boolean!) { - dog { - extras @stream(label: "same", initialCount: 1, if: $cond1) - extras @stream(label: "same", initialCount: 1, if: $cond2) - } - }`, FieldSelectionMerging(), Invalid, - withValidationErrors(`found conflicting stream directives on the same field`)) - }) + // + // Streams with "if:false" should be just ignored. + // t.Run("with different if conditions", func(t *testing.T) { + // run(t, ` + // query { dog { ...dogFragment } } + // fragment dogFragment on Dog { + // extras @stream(label: "same", initialCount: 1, if: true) + // extras @stream(label: "same", initialCount: 1, if: false) + // }`, FieldSelectionMerging(), Valid) + // }) + // + // Streams with "if" set to variables, should have matching other fields. + // t.Run("with variable if conditions", func(t *testing.T) { + // run(t, ` + // query($cond1: Boolean!, $cond2: Boolean!) { + // dog { + // extras @stream(label: "same", initialCount: 1, if: $cond1) + // extras @stream(label: "same", initialCount: 1, if: $cond2) + // } + // }`, FieldSelectionMerging(), Valid) + // }) + // + // "if: true" in streams can be omitted and merged without it // t.Run("with if true vs no if argument", func(t *testing.T) { // run(t, ` // query { dog { @@ -4851,13 +4855,14 @@ type Query { // extras @stream(label: "same", initialCount: 1) // } }`, FieldSelectionMerging(), Valid) // }) + // + // streams with "if: false" should be ignored // t.Run("with if false vs no stream", func(t *testing.T) { // run(t, ` // query { dog { // extras // extras @stream(label: "one", if: false) - // } }`, FieldSelectionMerging(), Invalid, - // withValidationErrors(`found conflicting stream directives on the same field`)) + // } }`, FieldSelectionMerging(), Valid) // }) }) }) From 222c08462c830c203247b458c63d95f0a5eaf257 Mon Sep 17 00:00:00 2001 From: Yury Smolski <140245+ysmolski@users.noreply.github.com> Date: Wed, 15 Oct 2025 17:35:05 +0300 Subject: [PATCH 11/11] fix from comments --- .../operation_rule_field_selection_merging.go | 2 +- v2/pkg/astvalidation/operation_validation_test.go | 11 +++++++++++ v2/pkg/operationreport/externalerror.go | 3 ++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index 3c0901c67..e35e9b124 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -121,7 +121,7 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { if !f.potentiallySameObject(fieldDefinitionTypeNode, f.nonScalarRequirements[i].fieldTypeDefinitionNode) { // This condition below can never be true because if objects aren't potentially the same, // and we know objectNames are equal (from the filter), they cannot be not equal at the same time. - // Perhaps this should be an else case or restructured? + // Perhaps this should be remove altogether? if !objectName.Equals(f.nonScalarRequirements[i].objectName) { f.StopWithExternalErr(operationreport.ErrResponseOfDifferingTypesMustBeOfSameShape(objectName, f.nonScalarRequirements[i].objectName)) return diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index fb7d78ff7..10cb61332 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -4388,6 +4388,17 @@ type Query { } }`, DeferStreamOnValidOperations(), Valid) }) + t.Run("defer fragment spread on root mutation field", func(t *testing.T) { + run(t, ` + mutation { + ...rootFragment @defer + } + fragment rootFragment on Mutation { + extras { string } + }`, DeferStreamOnValidOperations(), Invalid, + withValidationErrors(`directive "@defer" is not allowed on root fields of mutation operations`)) + }) + t.Run("non-defer inline fragment spread on root mutation field", func(t *testing.T) { run(t, ` mutation { diff --git a/v2/pkg/operationreport/externalerror.go b/v2/pkg/operationreport/externalerror.go index 725c13f43..c16ec1baf 100644 --- a/v2/pkg/operationreport/externalerror.go +++ b/v2/pkg/operationreport/externalerror.go @@ -413,7 +413,8 @@ func ErrDirectiveMustBeUniquePerLocation(directiveName ast.ByteSlice, position, } func orderedLocationsFromPositions(posA, posB position.Position) (locations []Location) { - if posA.LineStart < posB.LineStart || posA.CharStart < posB.CharStart { + // Order by (line, column) non-descending. + if posA.LineStart < posB.LineStart || (posA.LineStart == posB.LineStart && posA.CharStart < posB.CharStart) { return LocationsFromPosition(posA, posB) } else { return LocationsFromPosition(posB, posA)