diff --git a/execution/engine/execution_engine.go b/execution/engine/execution_engine.go index 8b57b9c11d..e920e5e00f 100644 --- a/execution/engine/execution_engine.go +++ b/execution/engine/execution_engine.go @@ -17,6 +17,7 @@ import ( "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" "github.com/wundergraph/graphql-go-tools/v2/pkg/astnormalization" "github.com/wundergraph/graphql-go-tools/v2/pkg/astprinter" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astvalidation" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/datasource/introspection_datasource" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/plan" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/postprocess" @@ -62,6 +63,7 @@ type ExecutionEngine struct { resolver *resolve.Resolver executionPlanCache *lru.Cache apolloCompatibilityFlags apollocompatibility.Flags + validationOptions []astvalidation.Option } type WebsocketBeforeStartHook interface { @@ -130,6 +132,11 @@ func NewExecutionEngine(ctx context.Context, logger abstractlogger.Logger, engin dsIDs[ds.Id()] = struct{}{} } + var validationOpts []astvalidation.Option + if engineConfig.plannerConfig.RelaxSubgraphOperationFieldSelectionMergingNullability { + validationOpts = append(validationOpts, astvalidation.WithRelaxFieldSelectionMergingNullability()) + } + return &ExecutionEngine{ logger: logger, config: engineConfig, @@ -138,6 +145,7 @@ func NewExecutionEngine(ctx context.Context, logger abstractlogger.Logger, engin apolloCompatibilityFlags: apollocompatibility.Flags{ ReplaceInvalidVarError: resolverOptions.ResolvableOptions.ApolloCompatibilityReplaceInvalidVarError, }, + validationOptions: validationOpts, }, nil } @@ -159,7 +167,7 @@ func (e *ExecutionEngine) Execute(ctx context.Context, operation *graphql.Reques } // Validate the operation against the schema. - if result, err := operation.ValidateForSchema(e.config.schema); err != nil { + if result, err := operation.ValidateForSchema(e.config.schema, e.validationOptions...); err != nil { return err } else if !result.Valid { return result.Errors diff --git a/execution/engine/execution_engine_test.go b/execution/engine/execution_engine_test.go index 9ac77cd131..3b15c6dce4 100644 --- a/execution/engine/execution_engine_test.go +++ b/execution/engine/execution_engine_test.go @@ -223,6 +223,7 @@ type _executionTestOptions struct { propagateFetchReasons bool validateRequiredExternalFields bool computeStaticCost bool + relaxFieldSelectionMergingNullability bool } type executionTestOptions func(*_executionTestOptions) @@ -253,6 +254,12 @@ func computeStaticCost() executionTestOptions { } } +func relaxFieldSelectionMergingNullability() executionTestOptions { + return func(options *_executionTestOptions) { + options.relaxFieldSelectionMergingNullability = true + } +} + func TestExecutionEngine_Execute(t *testing.T) { run := func(testCase ExecutionEngineTestCase, withError bool, expectedErrorMessage string, options ...executionTestOptions) func(t *testing.T) { t.Helper() @@ -289,6 +296,7 @@ func TestExecutionEngine_Execute(t *testing.T) { engineConf.plannerConfig.ValidateRequiredExternalFields = opts.validateRequiredExternalFields engineConf.plannerConfig.ComputeStaticCost = opts.computeStaticCost engineConf.plannerConfig.StaticCostDefaultListSize = 10 + engineConf.plannerConfig.RelaxSubgraphOperationFieldSelectionMergingNullability = opts.relaxFieldSelectionMergingNullability resolveOpts := resolve.ResolverOptions{ MaxConcurrency: 1024, ResolvableOptions: opts.resolvableOptions, @@ -321,7 +329,7 @@ func TestExecutionEngine_Execute(t *testing.T) { if withError { require.Error(t, err) if expectedErrorMessage != "" { - assert.Contains(t, err.Error(), expectedErrorMessage) + assert.Equal(t, expectedErrorMessage, err.Error()) } } else { require.NoError(t, err) @@ -6812,6 +6820,134 @@ func TestExecutionEngine_Execute(t *testing.T) { }) }) + + t.Run("field merging with different nullability on non-overlapping union types", func(t *testing.T) { + unionSchema := ` + union Entity = User | Organization + type Query { entity: Entity } + type User { id: ID!, email: String! } + type Organization { id: ID!, email: String } + ` + schema, err := graphql.NewSchemaFromString(unionSchema) + require.NoError(t, err) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"entity"}}, + {TypeName: "User", FieldNames: []string{"id", "email"}}, + {TypeName: "Organization", FieldNames: []string{"id", "email"}}, + } + + customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "POST", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, unionSchema), + }) + + fieldConfig := []plan.FieldConfiguration{ + { + TypeName: "Query", + FieldName: "entity", + Path: []string{"entity"}, + }, + } + + t.Run("without relaxation flag, validation rejects differing nullability", runWithAndCompareError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + OperationName: "O", + Query: `query O { entity { ... on User { email } ... on Organization { email } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "ds-id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", + expectedPath: "/", + expectedBody: "", + sendResponseBody: `{"data":{"entity":{"__typename":"User","email":"user@test.com"}}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + }, + customConfig, + ), + }, + fields: fieldConfig, + }, + `fields 'email' conflict because they return conflicting types 'String!' and 'String', locations: [], path: [query,entity,$1Organization]`, + )) + + t.Run("non-null email from User type", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + OperationName: "O", + Query: `query O { entity { ... on User { email } ... on Organization { email } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "ds-id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", + expectedPath: "/", + expectedBody: "", + sendResponseBody: `{"data":{"entity":{"__typename":"User","email":"user@test.com"}}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + }, + customConfig, + ), + }, + fields: fieldConfig, + expectedResponse: `{"data":{"entity":{"email":"user@test.com"}}}`, + }, + relaxFieldSelectionMergingNullability(), + )) + + t.Run("null email from Organization type", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + OperationName: "O", + Query: `query O { entity { ... on User { email } ... on Organization { email } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "ds-id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", + expectedPath: "/", + expectedBody: "", + sendResponseBody: `{"data":{"entity":{"__typename":"Organization","email":null}}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + }, + customConfig, + ), + }, + fields: fieldConfig, + expectedResponse: `{"data":{"entity":{"email":null}}}`, + }, + relaxFieldSelectionMergingNullability(), + )) + }) } func testNetHttpClient(t *testing.T, testCase roundTripperTestCase) *http.Client { diff --git a/execution/graphql/validation.go b/execution/graphql/validation.go index c681c12c71..4789408f00 100644 --- a/execution/graphql/validation.go +++ b/execution/graphql/validation.go @@ -11,19 +11,22 @@ type ValidationResult struct { Errors graphqlerrors.Errors } -func (r *Request) ValidateForSchema(schema *Schema) (result ValidationResult, err error) { +func (r *Request) ValidateForSchema(schema *Schema, options ...astvalidation.Option) (result ValidationResult, err error) { if schema == nil { return ValidationResult{Valid: false, Errors: nil}, ErrNilSchema } schemaHash := schema.Hash() + useCache := len(options) == 0 if r.validForSchema == nil { r.validForSchema = map[uint64]ValidationResult{} } - if result, ok := r.validForSchema[schemaHash]; ok { - return result, nil + if useCache { + if result, ok := r.validForSchema[schemaHash]; ok { + return result, nil + } } report := r.parseQueryOnce() @@ -31,13 +34,15 @@ func (r *Request) ValidateForSchema(schema *Schema) (result ValidationResult, er return operationValidationResultFromReport(report) } - validator := astvalidation.DefaultOperationValidator() + validator := astvalidation.DefaultOperationValidator(options...) validator.Validate(&r.document, &schema.document, &report) result, err = operationValidationResultFromReport(report) if err != nil { return result, err } - r.validForSchema[schemaHash] = result + if useCache { + r.validForSchema[schemaHash] = result + } return result, err } diff --git a/v2/pkg/ast/ast_type.go b/v2/pkg/ast/ast_type.go index 92aff65a7b..0164b6fa5e 100644 --- a/v2/pkg/ast/ast_type.go +++ b/v2/pkg/ast/ast_type.go @@ -204,10 +204,37 @@ func (d *Document) TypeNumberOfListWraps(ref int) int { } func (d *Document) TypesAreCompatibleDeep(left int, right int) bool { + return d.typesAreCompatible(left, right, false) +} + +// TypesAreCompatibleIgnoringNullability is a relaxed variant of TypesAreCompatibleDeep +// that strips NonNull wrappers at every nesting level but still requires the same list +// structure and the same base named type. +// +// Use this only when the two fields' enclosing types cannot overlap at runtime +// (i.e. potentiallySameObject returns false), for example two distinct concrete +// object types inside inline fragments on the same union/interface field. In that +// case the spec's SameResponseShape algorithm permits differing nullability because +// only one branch can ever execute for a given runtime object. +// +// See https://spec.graphql.org/October2021/#SameResponseShape() +func (d *Document) TypesAreCompatibleIgnoringNullability(left int, right int) bool { + return d.typesAreCompatible(left, right, true) +} + +func (d *Document) typesAreCompatible(left int, right int, ignoreNullability bool) bool { for { if left == -1 || right == -1 { return false } + if ignoreNullability { + if d.Types[left].TypeKind == TypeKindNonNull { + left = d.Types[left].OfType + } + if d.Types[right].TypeKind == TypeKindNonNull { + right = d.Types[right].OfType + } + } if d.Types[left].TypeKind != d.Types[right].TypeKind { return false } diff --git a/v2/pkg/ast/ast_type_test.go b/v2/pkg/ast/ast_type_test.go index bd41296310..c341b545fe 100644 --- a/v2/pkg/ast/ast_type_test.go +++ b/v2/pkg/ast/ast_type_test.go @@ -1 +1,170 @@ package ast + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDocument_TypesAreCompatibleIgnoringNullability(t *testing.T) { + // Build a document with various type refs to test against. + // We create types programmatically using the Add* methods. + doc := NewSmallDocument() + + // Named types + stringRef := doc.AddNamedType([]byte("String")) // String + intRef := doc.AddNamedType([]byte("Int")) // Int + stringRef2 := doc.AddNamedType([]byte("String")) // String (second ref) + profileRef := doc.AddNamedType([]byte("Profile")) // Profile + profileRef2 := doc.AddNamedType([]byte("Profile")) // Profile (second ref) + + // NonNull named types + stringNonNullRef := doc.AddNonNullNamedType([]byte("String")) // String! + intNonNullRef := doc.AddNonNullNamedType([]byte("Int")) // Int! + profileNonNullRef := doc.AddNonNullNamedType([]byte("Profile")) // Profile! + + // List types + listStringRef := doc.AddListType(doc.AddNamedType([]byte("String"))) // [String] + listStringNonNullRef := doc.AddListType(doc.AddNonNullNamedType([]byte("String"))) // [String!] + + // NonNull list types + nonNullListStringRef := doc.AddNonNullType(doc.AddListType(doc.AddNamedType([]byte("String")))) // [String]! + nonNullListStringNonNullRef := doc.AddNonNullType(doc.AddListType(doc.AddNonNullNamedType([]byte("String")))) // [String!]! + + tests := []struct { + name string + left int + right int + deep bool // expected result from TypesAreCompatibleDeep + relaxed bool // expected result from TypesAreCompatibleIgnoringNullability + }{ + { + name: "String vs String - identical", + left: stringRef, + right: stringRef2, + deep: true, + relaxed: true, + }, + { + name: "String! vs String - nullability differs", + left: stringNonNullRef, + right: stringRef, + deep: false, + relaxed: true, + }, + { + name: "String vs String! - nullability differs (reversed)", + left: stringRef, + right: stringNonNullRef, + deep: false, + relaxed: true, + }, + { + name: "String! vs Int! - different base types", + left: stringNonNullRef, + right: intNonNullRef, + deep: false, + relaxed: false, + }, + { + name: "String vs Int - different base types", + left: stringRef, + right: intRef, + deep: false, + relaxed: false, + }, + { + name: "String! vs Int - different base types, different nullability", + left: stringNonNullRef, + right: intRef, + deep: false, + relaxed: false, + }, + { + name: "Profile! vs Profile - non-scalar nullability differs", + left: profileNonNullRef, + right: profileRef, + deep: false, + relaxed: true, + }, + { + name: "Profile vs Profile - identical", + left: profileRef, + right: profileRef2, + deep: true, + relaxed: true, + }, + { + name: "[String] vs [String] - identical lists", + left: listStringRef, + right: listStringRef, + deep: true, + relaxed: true, + }, + { + name: "[String!] vs [String] - inner nullability differs", + left: listStringNonNullRef, + right: listStringRef, + deep: false, + relaxed: true, + }, + { + name: "[String]! vs [String] - outer nullability differs", + left: nonNullListStringRef, + right: listStringRef, + deep: false, + relaxed: true, + }, + { + name: "[String!]! vs [String] - both levels differ", + left: nonNullListStringNonNullRef, + right: listStringRef, + deep: false, + relaxed: true, + }, + { + name: "[String!]! vs [String!] - only outer differs", + left: nonNullListStringNonNullRef, + right: listStringNonNullRef, + deep: false, + relaxed: true, + }, + { + name: "[String] vs String - list vs non-list", + left: listStringRef, + right: stringRef, + deep: false, + relaxed: false, + }, + { + name: "[String]! vs String! - list vs non-list with NonNull", + left: nonNullListStringRef, + right: stringNonNullRef, + deep: false, + relaxed: false, + }, + { + name: "invalid ref left", + left: -1, + right: stringRef, + deep: false, + relaxed: false, + }, + { + name: "invalid ref right", + left: stringRef, + right: -1, + deep: false, + relaxed: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.deep, doc.TypesAreCompatibleDeep(tt.left, tt.right), + "TypesAreCompatibleDeep") + assert.Equal(t, tt.relaxed, doc.TypesAreCompatibleIgnoringNullability(tt.left, tt.right), + "TypesAreCompatibleIgnoringNullability") + }) + } +} diff --git a/v2/pkg/astvalidation/nullability_relaxation_justification.md b/v2/pkg/astvalidation/nullability_relaxation_justification.md new file mode 100644 index 0000000000..8233b60856 --- /dev/null +++ b/v2/pkg/astvalidation/nullability_relaxation_justification.md @@ -0,0 +1,151 @@ +# Justification: Relaxed Nullability Validation on Non-Overlapping Concrete Types + +## Problem Statement + +The Router rejects valid-in-practice queries where inline fragments on different +concrete union/interface member types select the same field with different +nullability: + +```graphql +# Schema: +union Entity = User | Organization +type User { email: String! } +type Organization { email: String } + +# Query (rejected): +{ + entity { + ... on User { email } + ... on Organization { email } + } +} +# Error: "fields 'email' conflict because they return conflicting types 'String!' and 'String'" +``` + +## What the Spec Says + +The GraphQL spec ([sec 5.3.2 "Field Selection Merging"](https://spec.graphql.org/October2021/#sec-Field-Selection-Merging)) +defines two algorithms: + +1. **FieldsInSetCanMerge**: For each pair of fields with the same response name, + calls `SameResponseShape`. If parent types are the same (or either is not an + Object Type), also checks that field names and arguments match. + +2. **SameResponseShape**: Checks structural type compatibility. Step 2 states: + "If typeA or typeB is Non-Null: if the other is nullable, return false." + This check runs **unconditionally** — it has no concept of parent type + mutual exclusivity. + +The `FieldsInSetCanMerge` algorithm *does* have a concept of mutual exclusivity +(step 3: "if parentType1 is not equal to parentType2, and both are Object Types"), +but this flag **only** relaxes the field-name and argument-equality checks — not +the `SameResponseShape` check. + +**Per the literal spec text, our query is invalid.** + +## What the Reference Implementation Does + +The [graphql-js reference implementation](https://github.com/graphql/graphql-js/blob/main/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts) +mirrors the spec exactly. The `doTypesConflict` function runs **outside** the +`!areMutuallyExclusive` guard: + +```typescript +// In findConflict(): +if (!areMutuallyExclusive) { + // Only check field name + argument equality when types could overlap +} + +// TYPE CHECK — ALWAYS RUNS, regardless of areMutuallyExclusive +if (type1 && type2 && doTypesConflict(type1, type2)) { + return [/* conflict */]; +} +``` + +**graphql-js also rejects the query.** + +## Known Issues About This Behavior + +This has been reported multiple times across the ecosystem: + +| Issue | Status | +|-------|--------| +| [graphql-js #1361](https://github.com/graphql/graphql-js/issues/1361): "Fragment safe divergence does not consider field nullability" | Closed as "working as designed". Maintainer suggested filing a spec change proposal. | +| [graphql-js #1065](https://github.com/graphql/graphql-js/issues/1065): "GraphQL union and conflicting types" | Closed with same explanation | +| [Netflix DGS #1583](https://github.com/Netflix/dgs-framework/issues/1583) | Closed as "working as designed per spec" | + +No spec change proposal has been filed or merged. + +## Why the Relaxation Is Safe + +Despite deviating from the spec, our relaxation is safe for the following reasons: + +### 1. Non-overlapping types can never co-resolve + +`User` and `Organization` are distinct concrete object types. At runtime, the +`entity` field resolves to exactly one of them. The `... on User` branch and +the `... on Organization` branch can **never** both apply to the same response +object. Therefore, a client will only ever see one nullability variant per object. + +### 2. The base types are still required to match + +Our relaxation **only** strips `NonNull` wrappers. It still requires: +- Same list structure (`[String]` vs `String` is still rejected) +- Same base named type (`String` vs `Int` is still rejected) +- Same abstract/concrete compatibility + +A nullable `String` is always a valid superset of a non-null `String!`. There is +no type safety issue. + +### 3. Conservative for interfaces + +When either enclosing type is an **interface**, we conservatively enforce the +strict check. This is because a concrete type could implement both interfaces, +making overlap possible. The relaxation only applies when both enclosing types +are **different concrete Object types**. + +### 4. The original spec rationale doesn't apply here + +The [original commit](https://github.com/graphql/graphql-js/commit/c034de91acce10d5c06d03bd332c6ebd45e2213c) +that introduced strict type checking for mutually exclusive types gave this +motivating example: + +```graphql +... on Person { foo: birthday { bar: year } } # bar: Int +... on Business { foo: location { bar: street } } # bar: String +``` + +Here `data.foo.bar` could be `Int` or `String` — genuinely ambiguous. But that +is a **different base type** conflict (`Int` vs `String`), which our fix still +rejects. Nullability differences (`String!` vs `String`) do not create this kind +of ambiguity — a client can always use the nullable type (`String`) as the field +type. + +## What We Changed + +In `operation_rule_field_selection_merging.go`, when `TypesAreCompatibleDeep` +fails (e.g. `String!` vs `String`), we now check: + +1. **Can the enclosing types overlap?** (`potentiallySameObject`) + - Interface + anything: YES (conservative, strict check) + - Same object type: YES (strict check) + - Different object types: NO (relaxed check) + +2. **If they can't overlap**: use `TypesAreCompatibleIgnoringNullability` which + strips `NonNull` wrappers at every nesting level but still requires the same + list structure and base named type. + +Existing test cases are unaffected: + +| Existing Test | Types Involved | Result | +|---------------|---------------|--------| +| `NonNullStringBox1` (interface) + `StringBox` (object) | Interface overlap possible | **Still Invalid** | +| `IntBox.scalar: Int` + `StringBox.scalar: String` | Different base types | **Still Invalid** | +| All "112" tests with `String` vs `Int` on Dog/Cat | Different base types | **Still Invalid** | + +## Conclusion + +This is a **deliberate, targeted deviation** from the GraphQL specification. It +addresses a real-world pain point that has been independently reported across +multiple GraphQL implementations. The relaxation is narrowly scoped (only +different concrete object types, only nullability differences) and preserves all +existing rejection behavior for genuinely conflicting types. diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index 9e762aa32c..04dad67ecc 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -10,9 +10,10 @@ import ( ) // FieldSelectionMerging validates if field selections can be merged -func FieldSelectionMerging() Rule { +func FieldSelectionMerging(relaxNullabilityCheck ...bool) Rule { + relax := len(relaxNullabilityCheck) > 0 && relaxNullabilityCheck[0] return func(walker *astvisitor.Walker) { - visitor := fieldSelectionMergingVisitor{Walker: walker} + visitor := fieldSelectionMergingVisitor{Walker: walker, relaxNullabilityCheck: relax} walker.RegisterEnterDocumentVisitor(&visitor) walker.RegisterEnterFieldVisitor(&visitor) walker.RegisterEnterOperationVisitor(&visitor) @@ -27,12 +28,14 @@ type fieldSelectionMergingVisitor struct { scalarRequirements scalarRequirements nonScalarRequirements nonScalarRequirements refs []int + relaxNullabilityCheck bool } type nonScalarRequirement struct { path ast.Path objectName ast.ByteSlice fieldTypeRef int fieldTypeDefinitionNode ast.Node + enclosingTypeDefinition ast.Node } type nonScalarRequirements []nonScalarRequirement @@ -116,18 +119,26 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { 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) + // Deliberate deviation from SameResponseShape (spec sec 5.3.2): when enclosing + // types cannot overlap at runtime (two distinct concrete object types), + // we allow nullability differences because only one branch will ever + // contribute to the response. This is gated behind relaxNullabilityCheck. + if !f.relaxNullabilityCheck || + f.potentiallySameObject(f.nonScalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) || + !f.definition.TypesAreCompatibleIgnoringNullability(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 } - right, err := f.definition.PrintTypeBytes(fieldType, nil) - if err != nil { - f.StopWithInternalErr(err) - return - } - f.StopWithExternalErr(operationreport.ErrTypesForFieldMismatch(objectName, left, right)) - return } if fieldDefinitionTypeNode.Kind != f.nonScalarRequirements[i].fieldTypeDefinitionNode.Kind { @@ -144,6 +155,7 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { objectName: objectName, fieldTypeRef: fieldType, fieldTypeDefinitionNode: fieldDefinitionTypeNode, + enclosingTypeDefinition: f.EnclosingTypeDefinition, }) return } @@ -160,18 +172,26 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { } } if !f.definition.TypesAreCompatibleDeep(f.scalarRequirements[i].fieldType, fieldType) { - left, err := f.definition.PrintTypeBytes(f.scalarRequirements[i].fieldType, nil) - if err != nil { - f.StopWithInternalErr(err) - return - } - right, err := f.definition.PrintTypeBytes(fieldType, nil) - if err != nil { - f.StopWithInternalErr(err) + // Per SameResponseShape (spec sec 5.3.2), when enclosing types cannot overlap + // at runtime (two distinct concrete object types), nullability differences are + // acceptable because only one branch will ever contribute to the response. + // This relaxation is gated behind the relaxNullabilityCheck flag. + if !f.relaxNullabilityCheck || + f.potentiallySameObject(f.scalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) || + !f.definition.TypesAreCompatibleIgnoringNullability(f.scalarRequirements[i].fieldType, fieldType) { + left, err := f.definition.PrintTypeBytes(f.scalarRequirements[i].fieldType, 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.ErrFieldsConflict(objectName, left, right)) return } - f.StopWithExternalErr(operationreport.ErrFieldsConflict(objectName, left, right)) - return } if fieldDefinitionTypeNode.Kind != f.scalarRequirements[i].fieldTypeDefinitionNode.Kind { @@ -193,6 +213,14 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { }) } +// potentiallySameObject reports whether two enclosing type definitions could apply +// to the same runtime object. This determines whether field merging must enforce +// strict type equality (including nullability) or may relax it. +// +// - If either type is an interface, returns true (conservative: any concrete +// type might implement that interface). +// - Two object types overlap only when they share the same name. +// - All other combinations return false. func (f *fieldSelectionMergingVisitor) potentiallySameObject(left, right ast.Node) bool { switch { case left.Kind == ast.NodeKindInterfaceTypeDefinition || right.Kind == ast.NodeKindInterfaceTypeDefinition: diff --git a/v2/pkg/astvalidation/operation_validation.go b/v2/pkg/astvalidation/operation_validation.go index ec3f345fc2..e24a72a5be 100644 --- a/v2/pkg/astvalidation/operation_validation.go +++ b/v2/pkg/astvalidation/operation_validation.go @@ -12,7 +12,8 @@ import ( ) type OperationValidatorOptions struct { - ApolloCompatibilityFlags apollocompatibility.Flags + ApolloCompatibilityFlags apollocompatibility.Flags + RelaxFieldSelectionMergingNullabilityCheck bool } func WithApolloCompatibilityFlags(flags apollocompatibility.Flags) Option { @@ -21,6 +22,16 @@ func WithApolloCompatibilityFlags(flags apollocompatibility.Flags) Option { } } +// WithRelaxFieldSelectionMergingNullability enables a deliberate spec deviation that allows +// differing nullability (e.g. String! vs String) on fields in non-overlapping concrete +// object types within inline fragments. Without this option, the validator enforces the +// strict GraphQL spec behavior where nullability must always match. +func WithRelaxFieldSelectionMergingNullability() Option { + return func(options *OperationValidatorOptions) { + options.RelaxFieldSelectionMergingNullabilityCheck = true + } +} + type Option func(options *OperationValidatorOptions) // DefaultOperationValidator returns a fully initialized OperationValidator with all default rules registered @@ -47,7 +58,7 @@ func DefaultOperationValidator(options ...Option) *OperationValidator { validator.RegisterRule(LoneAnonymousOperation()) validator.RegisterRule(SubscriptionSingleRootField()) validator.RegisterRule(FieldSelections()) - validator.RegisterRule(FieldSelectionMerging()) + validator.RegisterRule(FieldSelectionMerging(opts.RelaxFieldSelectionMergingNullabilityCheck)) validator.RegisterRule(KnownArguments()) validator.RegisterRule(Values()) validator.RegisterRule(ArgumentUniqueness()) diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index ae8ea3580e..74783bae26 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -1077,6 +1077,58 @@ func TestExecutionValidation(t *testing.T) { } }`, FieldSelectionMerging(), Valid) }) + t.Run("allows differing scalar nullability on non-overlapping object types", func(t *testing.T) { + runWithDefinition(t, entityDefinition, ` + { + entity { + ... on User { email } + ... on Organization { email } + } + }`, FieldSelectionMerging(true), Valid) + }) + t.Run("rejects differing scalar nullability on non-overlapping object types without relaxation flag", func(t *testing.T) { + runWithDefinition(t, entityDefinition, ` + { + entity { + ... on User { email } + ... on Organization { email } + } + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`fields 'email' conflict because they return conflicting types 'String!' and 'String'`)) + }) + t.Run("allows differing non-scalar nullability on non-overlapping object types", func(t *testing.T) { + runWithDefinition(t, entityDefinition, ` + { + entity { + ... on User { profile { name } } + ... on Organization { profile { name } } + } + }`, FieldSelectionMerging(true), Valid) + }) + t.Run("rejects differing non-scalar nullability on non-overlapping object types without relaxation flag", func(t *testing.T) { + runWithDefinition(t, entityDefinition, ` + { + entity { + ... on User { profile { name } } + ... on Organization { profile { name } } + } + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`differing types 'Profile!' and 'Profile' for objectName 'profile'`)) + }) + t.Run("disallows differing return type nullability when interface could overlap", func(t *testing.T) { + runWithDefinition(t, boxDefinition, ` + { + someBox { + ... on NonNullStringBox1 { + scalar + } + ... on StringBox { + scalar + } + } + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String!' and 'String'`)) + }) t.Run("same wrapped scalar return types", func(t *testing.T) { runWithDefinition(t, boxDefinition, ` { @@ -5646,6 +5698,42 @@ schema { query: Query }` +const entityDefinition = ` +scalar String +scalar Int +scalar ID + +interface Node { + id: ID! +} + +type User implements Node { + id: ID! + email: String! + profile: Profile! +} + +type Organization implements Node { + id: ID! + email: String + profile: Profile +} + +type Profile { + name: String +} + +union Entity = User | Organization + +type Query { + entity: Entity + node: Node +} + +schema { + query: Query +}` + const countriesDefinition = `directive @cacheControl(maxAge: Int, scope: CacheControlScope) on FIELD_DEFINITION | OBJECT | INTERFACE scalar String diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go index 8d46cfb6e1..f4268d1f6a 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -92,6 +92,8 @@ type Planner[T Configuration] struct { // gRPC grpcClient grpc.ClientConnInterface + + printKitPool *sync.Pool } func (p *Planner[T]) EnableSubgraphRequestMinifier() { @@ -1685,10 +1687,10 @@ type printKit struct { report *operationreport.Report } -var ( - printKitPool = &sync.Pool{ +func newPrintKitPool(validationOptions ...astvalidation.Option) *sync.Pool { + return &sync.Pool{ New: func() any { - validator := astvalidation.DefaultOperationValidator() + validator := astvalidation.DefaultOperationValidator(validationOptions...) // as we are creating operation programmatically in the graphql datasource planner, // we need to catch incorrect behavior of the planner // as graphql datasource planner should visit only selection sets which has fields, @@ -1710,14 +1712,28 @@ var ( } }, } +} + +var ( + defaultPrintKitPool = newPrintKitPool() + relaxedPrintKitPool *sync.Pool + relaxedPrintKitPoolOnce sync.Once ) +func getRelaxedPrintKitPool() *sync.Pool { + relaxedPrintKitPoolOnce.Do(func() { + relaxedPrintKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability()) + }) + return relaxedPrintKitPool +} + type Factory[T Configuration] struct { executionContext context.Context httpClient *http.Client grpcClient grpc.ClientConnInterface grpcClientProvider func() grpc.ClientConnInterface subscriptionClient GraphQLSubscriptionClient + printKitPool *sync.Pool } // NewFactory (HTTP) creates a new factory for the GraphQL datasource planner @@ -1780,13 +1796,36 @@ func NewFactoryGRPCClientProvider(executionContext context.Context, clientProvid } func (p *Planner[T]) getKit() *printKit { - return printKitPool.Get().(*printKit) + pool := p.printKitPool + if pool == nil { + pool = defaultPrintKitPool + } + return pool.Get().(*printKit) } func (p *Planner[T]) releaseKit(kit *printKit) { kit.buf.Reset() kit.report.Reset() - printKitPool.Put(kit) + pool := p.printKitPool + if pool == nil { + pool = defaultPrintKitPool + } + pool.Put(kit) +} + +// EnableSubgraphFieldSelectionMergingNullabilityRelaxation implements +// plan.SubgraphFieldSelectionMergingNullabilityRelaxer. It configures the +// factory to use a shared pool whose validator allows differing nullability +// on fields in non-overlapping concrete types. +func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() { + f.printKitPool = getRelaxedPrintKitPool() +} + +func (f *Factory[T]) getPrintKitPool() *sync.Pool { + if f.printKitPool != nil { + return f.printKitPool + } + return defaultPrintKitPool } func (f *Factory[T]) Planner(logger abstractlogger.Logger) plan.DataSourcePlanner[T] { @@ -1799,6 +1838,7 @@ func (f *Factory[T]) Planner(logger abstractlogger.Logger) plan.DataSourcePlanne fetchClient: f.httpClient, grpcClient: grpcClient, subscriptionClient: f.subscriptionClient, + printKitPool: f.getPrintKitPool(), } } diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go index e0e6d29b38..05b07df9e1 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go @@ -21,6 +21,7 @@ import ( "github.com/stretchr/testify/require" "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" + "github.com/wundergraph/graphql-go-tools/v2/pkg/astvalidation" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/datasource/httpclient" . "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/datasourcetesting" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/plan" @@ -8231,6 +8232,138 @@ func TestGraphQLDataSource(t *testing.T) { }, DisableResolveFieldPositions: true, }, WithDefaultPostProcessor())) + + t.Run("inline fragments on non-overlapping types with different field nullability", func(t *testing.T) { + definition := ` + scalar String + scalar Int + scalar ID + + interface Node { + id: ID! + } + + type User implements Node { + id: ID! + email: String! + profile: Profile! + } + + type Organization implements Node { + id: ID! + email: String + profile: Profile + } + + type Profile { + name: String + } + + union Entity = User | Organization + + type Query { + entity: Entity + } + + schema { + query: Query + } + ` + + t.Run("run", RunTest(definition, ` + query MyQuery { + entity { + ... on User { email } + ... on Organization { email } + } + }`, + "MyQuery", + &plan.SynchronousResponsePlan{ + Response: &resolve.GraphQLResponse{ + Fetches: resolve.Sequence( + resolve.Single(&resolve.SingleFetch{ + FetchConfiguration: resolve.FetchConfiguration{ + DataSource: &Source{}, + Input: `{"method":"POST","url":"https://example.com/graphql","body":{"query":"{entity {__typename ... on User {email} ... on Organization {email}}}"}}`, + PostProcessing: DefaultPostProcessingConfiguration, + }, + DataSourceIdentifier: []byte("graphql_datasource.Source"), + })), + Data: &resolve.Object{ + Fields: []*resolve.Field{ + { + Name: []byte("entity"), + Value: &resolve.Object{ + Path: []string{"entity"}, + Nullable: true, + PossibleTypes: map[string]struct{}{ + "User": {}, + "Organization": {}, + }, + TypeName: "Entity", + Fields: []*resolve.Field{ + { + Name: []byte("email"), + Value: &resolve.String{ + Path: []string{"email"}, + }, + OnTypeNames: [][]byte{[]byte("User")}, + }, + { + Name: []byte("email"), + Value: &resolve.String{ + Path: []string{"email"}, + Nullable: true, + }, + OnTypeNames: [][]byte{[]byte("Organization")}, + }, + }, + }, + }, + }, + }, + }, + }, plan.Configuration{ + DataSources: []plan.DataSource{ + mustDataSourceConfiguration( + t, + "ds-id", + &plan.DataSourceMetadata{ + RootNodes: []plan.TypeField{ + { + TypeName: "Query", + FieldNames: []string{"entity"}, + }, + }, + ChildNodes: []plan.TypeField{ + { + TypeName: "User", + FieldNames: []string{"id", "email", "profile"}, + }, + { + TypeName: "Organization", + FieldNames: []string{"id", "email", "profile"}, + }, + { + TypeName: "Profile", + FieldNames: []string{"name"}, + }, + }, + }, + mustCustomConfiguration(t, ConfigurationInput{ + Fetch: &FetchConfiguration{ + URL: "https://example.com/graphql", + }, + SchemaConfiguration: mustSchema(t, nil, definition), + }), + ), + }, + DisableResolveFieldPositions: true, + RelaxSubgraphOperationFieldSelectionMergingNullability: true, + }, WithDefaultPostProcessor(), + WithValidationOptions(astvalidation.WithRelaxFieldSelectionMergingNullability()), + )) + }) } var errSubscriptionClientFail = errors.New("subscription client fail error") diff --git a/v2/pkg/engine/datasourcetesting/datasourcetesting.go b/v2/pkg/engine/datasourcetesting/datasourcetesting.go index 5987666447..ec9c8907f3 100644 --- a/v2/pkg/engine/datasourcetesting/datasourcetesting.go +++ b/v2/pkg/engine/datasourcetesting/datasourcetesting.go @@ -34,6 +34,7 @@ type testOptions struct { withPrintPlan bool withFieldDependencies bool withFetchReasons bool + validationOptions []astvalidation.Option } func WithPostProcessors(postProcessors ...*postprocess.Processor) func(*testOptions) { @@ -84,6 +85,12 @@ func WithFetchReasons() func(*testOptions) { } } +func WithValidationOptions(options ...astvalidation.Option) func(*testOptions) { + return func(o *testOptions) { + o.validationOptions = options + } +} + func RunWithPermutations(t *testing.T, definition, operation, operationName string, expectedPlan plan.Plan, config plan.Configuration, options ...func(*testOptions)) { t.Helper() @@ -181,7 +188,7 @@ func RunTestWithVariables(definition, operation, operationName, variables string normalized := unsafeprinter.PrettyPrint(&op) _ = normalized - valid := astvalidation.DefaultOperationValidator() + valid := astvalidation.DefaultOperationValidator(opts.validationOptions...) valid.Validate(&op, &def, &report) p, err := plan.NewPlanner(config) diff --git a/v2/pkg/engine/plan/configuration.go b/v2/pkg/engine/plan/configuration.go index cff8109235..5194c8e976 100644 --- a/v2/pkg/engine/plan/configuration.go +++ b/v2/pkg/engine/plan/configuration.go @@ -52,6 +52,13 @@ type Configuration struct { // When the list size is unknown from directives, this value is used as a default for static cost. StaticCostDefaultListSize int + + // RelaxSubgraphOperationFieldSelectionMergingNullability relaxes the nullability validation + // for field selection merging in upstream (subgraph) operations when enclosing types are + // non-overlapping concrete object types. This is a deliberate spec deviation. + // When true, factories that support it will allow differing nullability + // (e.g. String! vs String) on fields in non-overlapping inline fragments. + RelaxSubgraphOperationFieldSelectionMergingNullability bool } type DebugConfiguration struct { diff --git a/v2/pkg/engine/plan/datasource_configuration.go b/v2/pkg/engine/plan/datasource_configuration.go index afc922be8d..0601e3abfb 100644 --- a/v2/pkg/engine/plan/datasource_configuration.go +++ b/v2/pkg/engine/plan/datasource_configuration.go @@ -300,6 +300,12 @@ func (d *dataSourceConfiguration[T]) CustomConfiguration() T { } func (d *dataSourceConfiguration[T]) CreatePlannerConfiguration(logger abstractlogger.Logger, fetchConfig *objectFetchConfiguration, pathConfig *plannerPathsConfiguration, configuration *Configuration) PlannerConfiguration { + if configuration.RelaxSubgraphOperationFieldSelectionMergingNullability { + if relaxer, ok := d.factory.(SubgraphFieldSelectionMergingNullabilityRelaxer); ok { + relaxer.EnableSubgraphFieldSelectionMergingNullabilityRelaxation() + } + } + planner := d.factory.Planner(logger) fetchConfig.planner = planner @@ -475,6 +481,14 @@ type Identifyable interface { astvisitor.VisitorIdentifier } +// SubgraphFieldSelectionMergingNullabilityRelaxer is an optional interface that a PlannerFactory +// can implement to support relaxed nullability checks when validating upstream operations. +// When called, the factory should configure its internal validator to allow differing nullability +// on fields in non-overlapping concrete types. +type SubgraphFieldSelectionMergingNullabilityRelaxer interface { + EnableSubgraphFieldSelectionMergingNullabilityRelaxation() +} + type DataSourcePlanner[T any] interface { DataSourceFetchPlanner DataSourceBehavior diff --git a/v2/pkg/engine/resolve/resolve_test.go b/v2/pkg/engine/resolve/resolve_test.go index 268c7ce565..fda6f60806 100644 --- a/v2/pkg/engine/resolve/resolve_test.go +++ b/v2/pkg/engine/resolve/resolve_test.go @@ -892,6 +892,91 @@ func TestResolver_ResolveNode(t *testing.T) { }, *NewContext(context.Background()), `{"data":{"pets":[{"name":"Woofie"},{}]}}` })) + t.Run("resolve fields with differing nullability on non-overlapping types", testFn(false, func(t *testing.T, ctrl *gomock.Controller) (response *GraphQLResponse, ctx Context, expectedOutput string) { + // This test verifies that the resolver correctly handles fields where the same field name + // has different nullability on non-overlapping concrete types (e.g. User.email: String! vs Organization.email: String). + // A User object should resolve email as non-null, and an Organization object should resolve email as nullable. + return &GraphQLResponse{ + Fetches: Single(&SingleFetch{ + FetchConfiguration: FetchConfiguration{DataSource: FakeDataSource(`{"entity":{"__typename":"User","email":"user@example.com"}}`)}, + }), + Data: &Object{ + Fields: []*Field{ + { + Name: []byte("entity"), + Value: &Object{ + Path: []string{"entity"}, + PossibleTypes: map[string]struct{}{ + "User": {}, + "Organization": {}, + }, + TypeName: "Entity", + Fields: []*Field{ + { + OnTypeNames: [][]byte{[]byte("User")}, + Name: []byte("email"), + Value: &String{ + Path: []string{"email"}, + }, + }, + { + OnTypeNames: [][]byte{[]byte("Organization")}, + Name: []byte("email"), + Value: &String{ + Path: []string{"email"}, + Nullable: true, + }, + }, + }, + }, + }, + }, + }, + }, *NewContext(context.Background()), + `{"data":{"entity":{"email":"user@example.com"}}}` + })) + t.Run("resolve fields with differing nullability on non-overlapping types - nullable variant", testFn(false, func(t *testing.T, ctrl *gomock.Controller) (response *GraphQLResponse, ctx Context, expectedOutput string) { + // Same scenario but the runtime object is Organization (nullable email) with a null email value + return &GraphQLResponse{ + Fetches: Single(&SingleFetch{ + FetchConfiguration: FetchConfiguration{DataSource: FakeDataSource(`{"entity":{"__typename":"Organization","email":null}}`)}, + }), + Data: &Object{ + Fields: []*Field{ + { + Name: []byte("entity"), + Value: &Object{ + Path: []string{"entity"}, + PossibleTypes: map[string]struct{}{ + "User": {}, + "Organization": {}, + }, + TypeName: "Entity", + Fields: []*Field{ + { + OnTypeNames: [][]byte{[]byte("User")}, + Name: []byte("email"), + Value: &String{ + Path: []string{"email"}, + }, + }, + { + OnTypeNames: [][]byte{[]byte("Organization")}, + Name: []byte("email"), + Value: &String{ + Path: []string{"email"}, + Nullable: true, + }, + }, + }, + }, + }, + }, + }, + }, *NewContext(context.Background()), + `{"data":{"entity":{"email":null}}}` + })) + t.Run("with unescape json enabled", func(t *testing.T) { t.Run("json object within a string", testFn(false, func(t *testing.T, ctrl *gomock.Controller) (response *GraphQLResponse, ctx Context, expectedOutput string) { return &GraphQLResponse{