From a454400ae086dc426c4b71df592d879aa77ea95d Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Thu, 26 Feb 2026 23:13:07 +0100 Subject: [PATCH 1/8] feat: add field selection merging type mismatch relaxation Allow completely different types (e.g. IssueState vs PullRequestReviewState) on fields in non-overlapping concrete object types within inline fragments. This is a deliberate spec deviation gated behind a feature flag (default OFF) that enables schemas like GitHub's where interface implementations define same-named fields with different leaf types. Co-Authored-By: Claude Opus 4.6 --- execution/engine/execution_engine.go | 3 + execution/engine/execution_engine_test.go | 75 ++++++++++ .../operation_rule_field_selection_merging.go | 62 ++++++-- v2/pkg/astvalidation/operation_validation.go | 20 ++- .../operation_validation_test.go | 137 +++++++++++++++++- .../graphql_datasource/graphql_datasource.go | 51 ++++++- v2/pkg/engine/plan/configuration.go | 7 + .../engine/plan/datasource_configuration.go | 13 ++ 8 files changed, 346 insertions(+), 22 deletions(-) diff --git a/execution/engine/execution_engine.go b/execution/engine/execution_engine.go index 27c7a46df1..5e95d8d2fb 100644 --- a/execution/engine/execution_engine.go +++ b/execution/engine/execution_engine.go @@ -132,6 +132,9 @@ func NewExecutionEngine(ctx context.Context, logger abstractlogger.Logger, engin if engineConfig.plannerConfig.RelaxSubgraphOperationFieldSelectionMergingNullability { validationOpts = append(validationOpts, astvalidation.WithRelaxFieldSelectionMergingNullability()) } + if engineConfig.plannerConfig.RelaxSubgraphOperationFieldSelectionMergingTypeMismatch { + validationOpts = append(validationOpts, astvalidation.WithRelaxFieldSelectionMergingTypeMismatch()) + } return &ExecutionEngine{ logger: logger, diff --git a/execution/engine/execution_engine_test.go b/execution/engine/execution_engine_test.go index 9c3370efe0..cef6d49b73 100644 --- a/execution/engine/execution_engine_test.go +++ b/execution/engine/execution_engine_test.go @@ -225,6 +225,7 @@ type _executionTestOptions struct { validateRequiredExternalFields bool computeCosts bool relaxFieldSelectionMergingNullability bool + relaxFieldSelectionMergingTypeMismatch bool } type executionTestOptions func(*_executionTestOptions) @@ -261,6 +262,12 @@ func relaxFieldSelectionMergingNullability() executionTestOptions { } } +func relaxFieldSelectionMergingTypeMismatch() executionTestOptions { + return func(options *_executionTestOptions) { + options.relaxFieldSelectionMergingTypeMismatch = true + } +} + func TestExecutionEngine_Execute(t *testing.T) { run := func(testCase ExecutionEngineTestCase, withError bool, expectedErrorMessage string, options ...executionTestOptions) func(t *testing.T) { t.Helper() @@ -298,6 +305,7 @@ func TestExecutionEngine_Execute(t *testing.T) { engineConf.plannerConfig.ComputeCosts = opts.computeCosts engineConf.plannerConfig.StaticCostDefaultListSize = 10 engineConf.plannerConfig.RelaxSubgraphOperationFieldSelectionMergingNullability = opts.relaxFieldSelectionMergingNullability + engineConf.plannerConfig.RelaxSubgraphOperationFieldSelectionMergingTypeMismatch = opts.relaxFieldSelectionMergingTypeMismatch resolveOpts := resolve.ResolverOptions{ MaxConcurrency: 1024, ResolvableOptions: opts.resolvableOptions, @@ -7715,6 +7723,73 @@ func TestExecutionEngine_Execute(t *testing.T) { relaxFieldSelectionMergingNullability(), )) }) + + t.Run("field merging with different types on non-overlapping union types", func(t *testing.T) { + unionSchema := ` + enum IssueState { OPEN CLOSED } + enum PullRequestReviewState { PENDING APPROVED } + union Updatable = Issue | PullRequestReview + type Query { updatable: Updatable } + type Issue { id: ID!, state: IssueState } + type PullRequestReview { id: ID!, state: PullRequestReviewState } + ` + schema, err := graphql.NewSchemaFromString(unionSchema) + require.NoError(t, err) + + rootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"updatable"}}, + {TypeName: "Issue", FieldNames: []string{"id", "state"}}, + {TypeName: "PullRequestReview", FieldNames: []string{"id", "state"}}, + } + + 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: "updatable", + Path: []string{"updatable"}, + }, + } + + t.Run("with relaxation flag, different enum types work", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + OperationName: "O", + Query: `query O { updatable { ... on Issue { state } ... on PullRequestReview { state } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "ds-id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", + expectedPath: "/", + expectedBody: "", + sendResponseBody: `{"data":{"updatable":{"__typename":"Issue","state":"OPEN"}}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + }, + customConfig, + ), + }, + fields: fieldConfig, + expectedResponse: `{"data":{"updatable":{"state":"OPEN"}}}`, + }, + relaxFieldSelectionMergingTypeMismatch(), + )) + }) } func testNetHttpClient(t *testing.T, testCase roundTripperTestCase) *http.Client { diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index 04dad67ecc..b5bae45e14 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -9,11 +9,28 @@ import ( "github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport" ) +// FieldSelectionMergingConfig controls which spec-deviation relaxations are active. +type FieldSelectionMergingConfig struct { + // RelaxNullabilityCheck allows differing nullability (e.g. String! vs String) + // on fields in non-overlapping concrete object types. + RelaxNullabilityCheck bool + // RelaxTypeMismatchCheck allows completely different types (e.g. IssueState vs + // PullRequestReviewState) on fields in non-overlapping concrete object types. + RelaxTypeMismatchCheck bool +} + // FieldSelectionMerging validates if field selections can be merged -func FieldSelectionMerging(relaxNullabilityCheck ...bool) Rule { - relax := len(relaxNullabilityCheck) > 0 && relaxNullabilityCheck[0] +func FieldSelectionMerging(config ...FieldSelectionMergingConfig) Rule { + var cfg FieldSelectionMergingConfig + if len(config) > 0 { + cfg = config[0] + } return func(walker *astvisitor.Walker) { - visitor := fieldSelectionMergingVisitor{Walker: walker, relaxNullabilityCheck: relax} + visitor := fieldSelectionMergingVisitor{ + Walker: walker, + relaxNullabilityCheck: cfg.RelaxNullabilityCheck, + relaxTypeMismatchCheck: cfg.RelaxTypeMismatchCheck, + } walker.RegisterEnterDocumentVisitor(&visitor) walker.RegisterEnterFieldVisitor(&visitor) walker.RegisterEnterOperationVisitor(&visitor) @@ -24,11 +41,12 @@ func FieldSelectionMerging(relaxNullabilityCheck ...bool) Rule { type fieldSelectionMergingVisitor struct { *astvisitor.Walker - definition, operation *ast.Document - scalarRequirements scalarRequirements - nonScalarRequirements nonScalarRequirements - refs []int - relaxNullabilityCheck bool + definition, operation *ast.Document + scalarRequirements scalarRequirements + nonScalarRequirements nonScalarRequirements + refs []int + relaxNullabilityCheck bool + relaxTypeMismatchCheck bool } type nonScalarRequirement struct { path ast.Path @@ -119,10 +137,16 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { return } } else if !f.definition.TypesAreCompatibleDeep(f.nonScalarRequirements[i].fieldTypeRef, fieldType) { - // 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. + // Type mismatch relaxation (spec sec 5.3.2): when enclosing types are + // provably non-overlapping (two distinct concrete object types), any type + // difference is safe because only one branch contributes to the response. + if f.relaxTypeMismatchCheck && + !f.potentiallySameObject(f.nonScalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) { + continue + } + // Nullability relaxation (spec sec 5.3.2): when enclosing types cannot + // overlap at runtime, we allow nullability differences. Gated behind + // relaxNullabilityCheck. if !f.relaxNullabilityCheck || f.potentiallySameObject(f.nonScalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) || !f.definition.TypesAreCompatibleIgnoringNullability(f.nonScalarRequirements[i].fieldTypeRef, fieldType) { @@ -172,10 +196,16 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { } } if !f.definition.TypesAreCompatibleDeep(f.scalarRequirements[i].fieldType, fieldType) { - // 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. + // Type mismatch relaxation (spec sec 5.3.2): when enclosing types are + // provably non-overlapping (two distinct concrete object types), any type + // difference is safe because only one branch contributes to the response. + if f.relaxTypeMismatchCheck && + !f.potentiallySameObject(f.scalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) { + continue + } + // Nullability relaxation (spec sec 5.3.2): when enclosing types cannot + // overlap at runtime, we allow nullability differences. Gated behind + // relaxNullabilityCheck. if !f.relaxNullabilityCheck || f.potentiallySameObject(f.scalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) || !f.definition.TypesAreCompatibleIgnoringNullability(f.scalarRequirements[i].fieldType, fieldType) { diff --git a/v2/pkg/astvalidation/operation_validation.go b/v2/pkg/astvalidation/operation_validation.go index e24a72a5be..b3a0d53f3f 100644 --- a/v2/pkg/astvalidation/operation_validation.go +++ b/v2/pkg/astvalidation/operation_validation.go @@ -12,8 +12,9 @@ import ( ) type OperationValidatorOptions struct { - ApolloCompatibilityFlags apollocompatibility.Flags - RelaxFieldSelectionMergingNullabilityCheck bool + ApolloCompatibilityFlags apollocompatibility.Flags + RelaxFieldSelectionMergingNullabilityCheck bool + RelaxFieldSelectionMergingTypeMismatchCheck bool } func WithApolloCompatibilityFlags(flags apollocompatibility.Flags) Option { @@ -32,6 +33,16 @@ func WithRelaxFieldSelectionMergingNullability() Option { } } +// WithRelaxFieldSelectionMergingTypeMismatch enables a deliberate spec deviation that allows +// completely different types (e.g. IssueState vs PullRequestReviewState) on fields in +// non-overlapping concrete object types within inline fragments. Without this option, the +// validator enforces the strict GraphQL spec behavior where leaf types must be identical. +func WithRelaxFieldSelectionMergingTypeMismatch() Option { + return func(options *OperationValidatorOptions) { + options.RelaxFieldSelectionMergingTypeMismatchCheck = true + } +} + type Option func(options *OperationValidatorOptions) // DefaultOperationValidator returns a fully initialized OperationValidator with all default rules registered @@ -58,7 +69,10 @@ func DefaultOperationValidator(options ...Option) *OperationValidator { validator.RegisterRule(LoneAnonymousOperation()) validator.RegisterRule(SubscriptionSingleRootField()) validator.RegisterRule(FieldSelections()) - validator.RegisterRule(FieldSelectionMerging(opts.RelaxFieldSelectionMergingNullabilityCheck)) + validator.RegisterRule(FieldSelectionMerging(FieldSelectionMergingConfig{ + RelaxNullabilityCheck: opts.RelaxFieldSelectionMergingNullabilityCheck, + RelaxTypeMismatchCheck: opts.RelaxFieldSelectionMergingTypeMismatchCheck, + })) 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 74783bae26..ec61a0786d 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -1084,7 +1084,7 @@ func TestExecutionValidation(t *testing.T) { ... on User { email } ... on Organization { email } } - }`, FieldSelectionMerging(true), Valid) + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxNullabilityCheck: true}), Valid) }) t.Run("rejects differing scalar nullability on non-overlapping object types without relaxation flag", func(t *testing.T) { runWithDefinition(t, entityDefinition, ` @@ -1103,7 +1103,7 @@ func TestExecutionValidation(t *testing.T) { ... on User { profile { name } } ... on Organization { profile { name } } } - }`, FieldSelectionMerging(true), Valid) + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxNullabilityCheck: true}), Valid) }) t.Run("rejects differing non-scalar nullability on non-overlapping object types without relaxation flag", func(t *testing.T) { runWithDefinition(t, entityDefinition, ` @@ -1129,6 +1129,96 @@ func TestExecutionValidation(t *testing.T) { }`, FieldSelectionMerging(), Invalid, withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String!' and 'String'`)) }) + // Type mismatch relaxation tests + t.Run("rejects differing enum types on union members without type mismatch flag", func(t *testing.T) { + runWithDefinition(t, typeMismatchDefinition, ` + { + entity { + ... on User { priority } + ... on Organization { priority } + } + }`, FieldSelectionMerging(), Invalid, + withValidationErrors(`fields 'priority' conflict because they return conflicting types 'Int' and 'String'`)) + }) + t.Run("allows differing enum types on union members with type mismatch flag", func(t *testing.T) { + runWithDefinition(t, typeMismatchDefinition, ` + { + entity { + ... on User { priority } + ... on Organization { priority } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Valid) + }) + t.Run("allows differing enum types on interface implementors with type mismatch flag via union", func(t *testing.T) { + runWithDefinition(t, typeMismatchDefinition, ` + { + entity { + ... on User { id } + ... on Organization { id } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Valid) + }) + t.Run("rejects differing types when enclosing type is interface (could overlap)", func(t *testing.T) { + // NonNullStringBox1 is an interface (scalar: String!), IntBox is a + // concrete type (scalar: Int). Because one enclosing type is an + // interface, potentiallySameObject returns true (conservative) and + // the relaxation must NOT apply. + runWithDefinition(t, boxDefinition, ` + { + someBox { + ... on NonNullStringBox1 { + scalar + } + ... on IntBox { + scalar + } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Invalid, + withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String!' and 'Int'`)) + }) + t.Run("allows differing enum types on interface implementors with type mismatch flag", func(t *testing.T) { + // This is the exact GitHub use case: Updatable is an interface, + // Issue and PullRequestReview implement it with different enum types + // for the state field. The inline fragments narrow to concrete types. + runWithDefinition(t, typeMismatchDefinition, ` + { + updatable { + ... on Issue { state } + ... on PullRequestReview { state } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Valid) + }) + t.Run("type mismatch flag also covers nullability differences on disjoint types", func(t *testing.T) { + runWithDefinition(t, entityDefinition, ` + { + entity { + ... on User { email } + ... on Organization { email } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Valid) + }) + t.Run("only nullability flag does not cover type mismatches", func(t *testing.T) { + runWithDefinition(t, typeMismatchDefinition, ` + { + entity { + ... on User { priority } + ... on Organization { priority } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxNullabilityCheck: true}), Invalid, + withValidationErrors(`fields 'priority' conflict because they return conflicting types 'Int' and 'String'`)) + }) + t.Run("both flags enabled together", func(t *testing.T) { + runWithDefinition(t, typeMismatchDefinition, ` + { + entity { + ... on User { priority } + ... on Organization { priority } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{ + RelaxNullabilityCheck: true, + RelaxTypeMismatchCheck: true, + }), Valid) + }) t.Run("same wrapped scalar return types", func(t *testing.T) { runWithDefinition(t, boxDefinition, ` { @@ -5734,6 +5824,49 @@ schema { query: Query }` +const typeMismatchDefinition = ` +scalar String +scalar Int +scalar ID + +enum IssueState { OPEN CLOSED } +enum PullRequestReviewState { PENDING APPROVED } + +interface Updatable { + id: ID! +} + +type Issue implements Updatable { + id: ID! + state: IssueState +} + +type PullRequestReview implements Updatable { + id: ID! + state: PullRequestReviewState +} + +type User { + id: ID! + priority: Int +} + +type Organization { + id: ID! + priority: String +} + +union Entity = User | Organization + +type Query { + updatable: Updatable + entity: Entity +} + +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 f4268d1f6a..37e3dcf128 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -1718,6 +1718,12 @@ var ( defaultPrintKitPool = newPrintKitPool() relaxedPrintKitPool *sync.Pool relaxedPrintKitPoolOnce sync.Once + + typeMismatchPrintKitPool *sync.Pool + typeMismatchPrintKitPoolOnce sync.Once + + bothRelaxedPrintKitPool *sync.Pool + bothRelaxedPrintKitPoolOnce sync.Once ) func getRelaxedPrintKitPool() *sync.Pool { @@ -1727,6 +1733,23 @@ func getRelaxedPrintKitPool() *sync.Pool { return relaxedPrintKitPool } +func getTypeMismatchPrintKitPool() *sync.Pool { + typeMismatchPrintKitPoolOnce.Do(func() { + typeMismatchPrintKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingTypeMismatch()) + }) + return typeMismatchPrintKitPool +} + +func getBothRelaxedPrintKitPool() *sync.Pool { + bothRelaxedPrintKitPoolOnce.Do(func() { + bothRelaxedPrintKitPool = newPrintKitPool( + astvalidation.WithRelaxFieldSelectionMergingNullability(), + astvalidation.WithRelaxFieldSelectionMergingTypeMismatch(), + ) + }) + return bothRelaxedPrintKitPool +} + type Factory[T Configuration] struct { executionContext context.Context httpClient *http.Client @@ -1734,6 +1757,9 @@ type Factory[T Configuration] struct { grpcClientProvider func() grpc.ClientConnInterface subscriptionClient GraphQLSubscriptionClient printKitPool *sync.Pool + + relaxNullabilityCheck bool + relaxTypeMismatchCheck bool } // NewFactory (HTTP) creates a new factory for the GraphQL datasource planner @@ -1813,12 +1839,35 @@ func (p *Planner[T]) releaseKit(kit *printKit) { pool.Put(kit) } +func (f *Factory[T]) resolvePool() *sync.Pool { + switch { + case f.relaxNullabilityCheck && f.relaxTypeMismatchCheck: + return getBothRelaxedPrintKitPool() + case f.relaxNullabilityCheck: + return getRelaxedPrintKitPool() + case f.relaxTypeMismatchCheck: + return getTypeMismatchPrintKitPool() + default: + return defaultPrintKitPool + } +} + // 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() + f.relaxNullabilityCheck = true + f.printKitPool = f.resolvePool() +} + +// EnableSubgraphFieldSelectionMergingTypeMismatchRelaxation implements +// plan.SubgraphFieldSelectionMergingTypeMismatchRelaxer. It configures the +// factory to use a shared pool whose validator allows completely different types +// on fields in non-overlapping concrete types. +func (f *Factory[T]) EnableSubgraphFieldSelectionMergingTypeMismatchRelaxation() { + f.relaxTypeMismatchCheck = true + f.printKitPool = f.resolvePool() } func (f *Factory[T]) getPrintKitPool() *sync.Pool { diff --git a/v2/pkg/engine/plan/configuration.go b/v2/pkg/engine/plan/configuration.go index eebd9df352..aa8e63957b 100644 --- a/v2/pkg/engine/plan/configuration.go +++ b/v2/pkg/engine/plan/configuration.go @@ -59,6 +59,13 @@ type Configuration struct { // When true, factories that support it will allow differing nullability // (e.g. String! vs String) on fields in non-overlapping inline fragments. RelaxSubgraphOperationFieldSelectionMergingNullability bool + + // RelaxSubgraphOperationFieldSelectionMergingTypeMismatch relaxes the type mismatch + // 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 completely different types + // (e.g. IssueState vs PullRequestReviewState) on fields in non-overlapping inline fragments. + RelaxSubgraphOperationFieldSelectionMergingTypeMismatch bool } type DebugConfiguration struct { diff --git a/v2/pkg/engine/plan/datasource_configuration.go b/v2/pkg/engine/plan/datasource_configuration.go index 0601e3abfb..49ee30c67a 100644 --- a/v2/pkg/engine/plan/datasource_configuration.go +++ b/v2/pkg/engine/plan/datasource_configuration.go @@ -305,6 +305,11 @@ func (d *dataSourceConfiguration[T]) CreatePlannerConfiguration(logger abstractl relaxer.EnableSubgraphFieldSelectionMergingNullabilityRelaxation() } } + if configuration.RelaxSubgraphOperationFieldSelectionMergingTypeMismatch { + if relaxer, ok := d.factory.(SubgraphFieldSelectionMergingTypeMismatchRelaxer); ok { + relaxer.EnableSubgraphFieldSelectionMergingTypeMismatchRelaxation() + } + } planner := d.factory.Planner(logger) @@ -489,6 +494,14 @@ type SubgraphFieldSelectionMergingNullabilityRelaxer interface { EnableSubgraphFieldSelectionMergingNullabilityRelaxation() } +// SubgraphFieldSelectionMergingTypeMismatchRelaxer is an optional interface that a PlannerFactory +// can implement to support relaxed type mismatch checks when validating upstream operations. +// When called, the factory should configure its internal validator to allow completely different +// types on fields in non-overlapping concrete types. +type SubgraphFieldSelectionMergingTypeMismatchRelaxer interface { + EnableSubgraphFieldSelectionMergingTypeMismatchRelaxation() +} + type DataSourcePlanner[T any] interface { DataSourceFetchPlanner DataSourceBehavior From def07c0036efeae9069b2acd1369878a52f88646 Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Mon, 2 Mar 2026 07:57:46 +0100 Subject: [PATCH 2/8] refactor: extract checkTypeMismatch helper and consolidate print kit pools Extract duplicated type mismatch/nullability relaxation logic into a checkTypeMismatch method with a clear three-outcome result type. Replace separate pool variables and sync.Once with a sync.Map keyed by flag combination. Add test for combined relaxation flags. Co-Authored-By: Claude Opus 4.6 --- execution/engine/execution_engine_test.go | 33 +++++++++ .../nullability_relaxation_justification.md | 49 +++++++++++-- .../operation_rule_field_selection_merging.go | 73 +++++++++++++------ .../graphql_datasource/graphql_datasource.go | 72 +++++++++--------- 4 files changed, 158 insertions(+), 69 deletions(-) diff --git a/execution/engine/execution_engine_test.go b/execution/engine/execution_engine_test.go index cef6d49b73..d573902da1 100644 --- a/execution/engine/execution_engine_test.go +++ b/execution/engine/execution_engine_test.go @@ -7789,6 +7789,39 @@ func TestExecutionEngine_Execute(t *testing.T) { }, relaxFieldSelectionMergingTypeMismatch(), )) + + t.Run("with both relaxation flags, different enum types work", runWithoutError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + OperationName: "O", + Query: `query O { updatable { ... on Issue { state } ... on PullRequestReview { state } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "ds-id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", + expectedPath: "/", + expectedBody: "", + sendResponseBody: `{"data":{"updatable":{"__typename":"Issue","state":"OPEN"}}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + }, + customConfig, + ), + }, + fields: fieldConfig, + expectedResponse: `{"data":{"updatable":{"state":"OPEN"}}}`, + }, + relaxFieldSelectionMergingNullability(), + relaxFieldSelectionMergingTypeMismatch(), + )) }) } diff --git a/v2/pkg/astvalidation/nullability_relaxation_justification.md b/v2/pkg/astvalidation/nullability_relaxation_justification.md index 8233b60856..12614d4d41 100644 --- a/v2/pkg/astvalidation/nullability_relaxation_justification.md +++ b/v2/pkg/astvalidation/nullability_relaxation_justification.md @@ -1,4 +1,4 @@ -# Justification: Relaxed Nullability Validation on Non-Overlapping Concrete Types +# Justification: Relaxed Field Selection Merging on Non-Overlapping Concrete Types ## Problem Statement @@ -142,10 +142,47 @@ Existing test cases are unaffected: | `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** | +## Type Mismatch Relaxation (RelaxTypeMismatchCheck) + +In addition to the nullability relaxation above, a broader relaxation is +available via `RelaxTypeMismatchCheck`. This allows **completely different types** +(e.g. `IssueState` vs `PullRequestReviewState`) on fields in non-overlapping +concrete object types. + +### Problem + +Schemas like GitHub's define interfaces (e.g. `Updatable`) whose implementing +types have same-named fields returning different leaf types: + +```graphql +interface Updatable { id: ID! } +type Issue implements Updatable { id: ID!, state: IssueState } +type PullRequestReview implements Updatable { id: ID!, state: PullRequestReviewState } +``` + +The spec's `SameResponseShape` algorithm rejects `state` across inline fragments +even though only one branch executes at runtime. + +### Safety + +The same `potentiallySameObject` guard applies: the relaxation only fires when +both enclosing types are different concrete Object types (never for interfaces). +At runtime, the response object can only match one concrete type, so different +field types cannot both appear for the same object. + +### Relationship to nullability relaxation + +`RelaxTypeMismatchCheck` is strictly broader than `RelaxNullabilityCheck`. When +the type mismatch flag is enabled on non-overlapping types, **all** type +differences (including nullability) are skipped because the code `continue`s +before reaching the nullability check. Both flags exist because callers may want +the narrower nullability-only relaxation without the broader type mismatch +relaxation. + ## 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. +These are **deliberate, targeted deviations** from the GraphQL specification. +They address real-world pain points that have been independently reported across +multiple GraphQL implementations. Both relaxations are narrowly scoped (only +different concrete object types) and preserve all existing rejection behavior +for genuinely conflicting types on overlapping or same types. diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index b5bae45e14..4000b2e7b5 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -10,6 +10,12 @@ import ( ) // FieldSelectionMergingConfig controls which spec-deviation relaxations are active. +// +// Note: RelaxTypeMismatchCheck is strictly broader than RelaxNullabilityCheck — +// when RelaxTypeMismatchCheck is true on non-overlapping types, all type differences +// (including nullability) are allowed, making RelaxNullabilityCheck redundant for +// those cases. Both flags exist because callers may want the narrower nullability-only +// relaxation without the broader type mismatch relaxation. type FieldSelectionMergingConfig struct { // RelaxNullabilityCheck allows differing nullability (e.g. String! vs String) // on fields in non-overlapping concrete object types. @@ -137,19 +143,10 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { return } } else if !f.definition.TypesAreCompatibleDeep(f.nonScalarRequirements[i].fieldTypeRef, fieldType) { - // Type mismatch relaxation (spec sec 5.3.2): when enclosing types are - // provably non-overlapping (two distinct concrete object types), any type - // difference is safe because only one branch contributes to the response. - if f.relaxTypeMismatchCheck && - !f.potentiallySameObject(f.nonScalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) { + switch f.checkTypeMismatch(f.nonScalarRequirements[i].enclosingTypeDefinition, f.nonScalarRequirements[i].fieldTypeRef, fieldType) { + case typeMismatchSkip: continue - } - // Nullability relaxation (spec sec 5.3.2): when enclosing types cannot - // overlap at runtime, we allow nullability differences. Gated behind - // relaxNullabilityCheck. - if !f.relaxNullabilityCheck || - f.potentiallySameObject(f.nonScalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) || - !f.definition.TypesAreCompatibleIgnoringNullability(f.nonScalarRequirements[i].fieldTypeRef, fieldType) { + case typeMismatchReject: left, err := f.definition.PrintTypeBytes(f.nonScalarRequirements[i].fieldTypeRef, nil) if err != nil { f.StopWithInternalErr(err) @@ -196,19 +193,10 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { } } if !f.definition.TypesAreCompatibleDeep(f.scalarRequirements[i].fieldType, fieldType) { - // Type mismatch relaxation (spec sec 5.3.2): when enclosing types are - // provably non-overlapping (two distinct concrete object types), any type - // difference is safe because only one branch contributes to the response. - if f.relaxTypeMismatchCheck && - !f.potentiallySameObject(f.scalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) { + switch f.checkTypeMismatch(f.scalarRequirements[i].enclosingTypeDefinition, f.scalarRequirements[i].fieldType, fieldType) { + case typeMismatchSkip: continue - } - // Nullability relaxation (spec sec 5.3.2): when enclosing types cannot - // overlap at runtime, we allow nullability differences. Gated behind - // relaxNullabilityCheck. - if !f.relaxNullabilityCheck || - f.potentiallySameObject(f.scalarRequirements[i].enclosingTypeDefinition, f.EnclosingTypeDefinition) || - !f.definition.TypesAreCompatibleIgnoringNullability(f.scalarRequirements[i].fieldType, fieldType) { + case typeMismatchReject: left, err := f.definition.PrintTypeBytes(f.scalarRequirements[i].fieldType, nil) if err != nil { f.StopWithInternalErr(err) @@ -243,6 +231,38 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { }) } +type typeMismatchResult int + +const ( + // typeMismatchAccept means the types differ only in nullability and the + // relaxation flag allowed it — no error, but keep processing this requirement. + typeMismatchAccept typeMismatchResult = iota + // typeMismatchSkip means the enclosing types are provably disjoint and + // the relaxation flag allowed the full type mismatch — skip this requirement. + typeMismatchSkip + // typeMismatchReject means the type difference must be reported as an error. + typeMismatchReject +) + +// checkTypeMismatch decides how to handle a type incompatibility between an +// existing requirement's field type and the current field's type. It applies +// type mismatch relaxation first (broader), then nullability relaxation (narrower). +func (f *fieldSelectionMergingVisitor) checkTypeMismatch(existingEnclosing ast.Node, existingFieldType, currentFieldType int) typeMismatchResult { + sameObject := f.potentiallySameObject(existingEnclosing, f.EnclosingTypeDefinition) + // Type mismatch relaxation (spec sec 5.3.2, SameResponseShape): when enclosing + // types are provably non-overlapping, any type difference is safe. + if f.relaxTypeMismatchCheck && !sameObject { + return typeMismatchSkip + } + // Nullability relaxation (spec sec 5.3.2, SameResponseShape): when enclosing + // types cannot overlap at runtime, we allow nullability differences. + if f.relaxNullabilityCheck && !sameObject && + f.definition.TypesAreCompatibleIgnoringNullability(existingFieldType, currentFieldType) { + return typeMismatchAccept + } + return typeMismatchReject +} + // 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. @@ -251,6 +271,11 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { // type might implement that interface). // - Two object types overlap only when they share the same name. // - All other combinations return false. +// +// Union types are not handled explicitly because the AST walker resolves inline +// fragment type conditions to their concrete member types before visiting fields; +// the enclosing type definition seen here is always the concrete type from the +// inline fragment, never the union itself. func (f *fieldSelectionMergingVisitor) potentiallySameObject(left, right ast.Node) bool { switch { case left.Kind == ast.NodeKindInterfaceTypeDefinition || right.Kind == ast.NodeKindInterfaceTypeDefinition: diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go index 37e3dcf128..061ea1d181 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -1714,40 +1714,40 @@ func newPrintKitPool(validationOptions ...astvalidation.Option) *sync.Pool { } } +// Each combination of relaxation flags requires its own pool because the +// validator configuration is baked into the printKit at pool-creation time. +// Mixing printKits with different validator settings in the same pool would +// cause non-deterministic validation behavior. var ( - defaultPrintKitPool = newPrintKitPool() - relaxedPrintKitPool *sync.Pool - relaxedPrintKitPoolOnce sync.Once - - typeMismatchPrintKitPool *sync.Pool - typeMismatchPrintKitPoolOnce sync.Once - - bothRelaxedPrintKitPool *sync.Pool - bothRelaxedPrintKitPoolOnce sync.Once + defaultPrintKitPool = newPrintKitPool() + printKitPools sync.Map // printKitPoolKey -> *sync.Pool ) -func getRelaxedPrintKitPool() *sync.Pool { - relaxedPrintKitPoolOnce.Do(func() { - relaxedPrintKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability()) - }) - return relaxedPrintKitPool +// printKitPoolKey identifies a unique pool for a given set of relaxation flags. +type printKitPoolKey struct { + relaxNullability bool + relaxTypeMismatch bool } -func getTypeMismatchPrintKitPool() *sync.Pool { - typeMismatchPrintKitPoolOnce.Do(func() { - typeMismatchPrintKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingTypeMismatch()) - }) - return typeMismatchPrintKitPool -} - -func getBothRelaxedPrintKitPool() *sync.Pool { - bothRelaxedPrintKitPoolOnce.Do(func() { - bothRelaxedPrintKitPool = newPrintKitPool( - astvalidation.WithRelaxFieldSelectionMergingNullability(), - astvalidation.WithRelaxFieldSelectionMergingTypeMismatch(), - ) - }) - return bothRelaxedPrintKitPool +// getPrintKitPoolForConfig returns a lazily-initialized pool for the given +// relaxation flag combination. The zero-value key returns defaultPrintKitPool. +func getPrintKitPoolForConfig(key printKitPoolKey) *sync.Pool { + if !key.relaxNullability && !key.relaxTypeMismatch { + return defaultPrintKitPool + } + if v, ok := printKitPools.Load(key); ok { + return v.(*sync.Pool) + } + var opts []astvalidation.Option + if key.relaxNullability { + opts = append(opts, astvalidation.WithRelaxFieldSelectionMergingNullability()) + } + if key.relaxTypeMismatch { + opts = append(opts, astvalidation.WithRelaxFieldSelectionMergingTypeMismatch()) + } + pool := newPrintKitPool(opts...) + v, _ := printKitPools.LoadOrStore(key, pool) + return v.(*sync.Pool) } type Factory[T Configuration] struct { @@ -1840,16 +1840,10 @@ func (p *Planner[T]) releaseKit(kit *printKit) { } func (f *Factory[T]) resolvePool() *sync.Pool { - switch { - case f.relaxNullabilityCheck && f.relaxTypeMismatchCheck: - return getBothRelaxedPrintKitPool() - case f.relaxNullabilityCheck: - return getRelaxedPrintKitPool() - case f.relaxTypeMismatchCheck: - return getTypeMismatchPrintKitPool() - default: - return defaultPrintKitPool - } + return getPrintKitPoolForConfig(printKitPoolKey{ + relaxNullability: f.relaxNullabilityCheck, + relaxTypeMismatch: f.relaxTypeMismatchCheck, + }) } // EnableSubgraphFieldSelectionMergingNullabilityRelaxation implements From 247c90bf636c7388073f77da6c53b577c2c09dff Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Mon, 2 Mar 2026 08:24:45 +0100 Subject: [PATCH 3/8] fix: address PR review comments for field selection merging type mismatch Add strict-mode failure test in execution engine, fix non-assertive test that queried same-type field (id) instead of mismatched field (priority), add narrow-scope test proving interface overlap is still rejected with the flag enabled, and add examples to checkTypeMismatch doc comment. Co-Authored-By: Claude Opus 4.6 --- execution/engine/execution_engine_test.go | 64 +++++++++++++++++++ .../operation_rule_field_selection_merging.go | 8 +++ .../operation_validation_test.go | 20 +++++- 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/execution/engine/execution_engine_test.go b/execution/engine/execution_engine_test.go index d573902da1..d6c104fdda 100644 --- a/execution/engine/execution_engine_test.go +++ b/execution/engine/execution_engine_test.go @@ -7758,6 +7758,70 @@ func TestExecutionEngine_Execute(t *testing.T) { }, } + // Strict-mode failure test: use scalar type mismatch (Int vs String) to + // verify that validation rejects different types without the relaxation flag. + scalarMismatchSchema := ` + type User { id: ID!, priority: Int } + type Organization { id: ID!, priority: String } + union Entity = User | Organization + type Query { entity: Entity } + ` + scalarMismatchParsedSchema, err2 := graphql.NewSchemaFromString(scalarMismatchSchema) + require.NoError(t, err2) + + scalarMismatchRootNodes := []plan.TypeField{ + {TypeName: "Query", FieldNames: []string{"entity"}}, + {TypeName: "User", FieldNames: []string{"id", "priority"}}, + {TypeName: "Organization", FieldNames: []string{"id", "priority"}}, + } + + scalarMismatchConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "POST", + }, + SchemaConfiguration: mustSchemaConfig(t, nil, scalarMismatchSchema), + }) + + scalarMismatchFieldConfig := []plan.FieldConfiguration{ + { + TypeName: "Query", + FieldName: "entity", + Path: []string{"entity"}, + }, + } + + t.Run("without relaxation flag, validation rejects different scalar types", runWithAndCompareError( + ExecutionEngineTestCase{ + schema: scalarMismatchParsedSchema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + OperationName: "O", + Query: `query O { entity { ... on User { priority } ... on Organization { priority } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "ds-id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", + expectedPath: "/", + expectedBody: "", + sendResponseBody: `{"data":{"entity":{"__typename":"User","priority":1}}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: scalarMismatchRootNodes, + }, + scalarMismatchConfig, + ), + }, + fields: scalarMismatchFieldConfig, + }, + `fields 'priority' conflict because they return conflicting types 'Int' and 'String', locations: [], path: [query,entity,$1Organization]`, + )) + t.Run("with relaxation flag, different enum types work", runWithoutError( ExecutionEngineTestCase{ schema: schema, diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index 4000b2e7b5..9d7681bed2 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -247,6 +247,14 @@ const ( // checkTypeMismatch decides how to handle a type incompatibility between an // existing requirement's field type and the current field's type. It applies // type mismatch relaxation first (broader), then nullability relaxation (narrower). +// +// Examples: +// - typeMismatchAccept: User.email is String!, Organization.email is String +// → types differ only in nullability, RelaxNullabilityCheck allows it +// - typeMismatchSkip: Issue.state is IssueState, PullRequestReview.state is PullRequestReviewState +// → enclosing types are disjoint concrete objects, RelaxTypeMismatchCheck skips the check entirely +// - typeMismatchReject: NonNullStringBox1.scalar is String!, IntBox.scalar is Int +// → one enclosing type is an interface (could overlap), difference must be reported as error func (f *fieldSelectionMergingVisitor) checkTypeMismatch(existingEnclosing ast.Node, existingFieldType, currentFieldType int) typeMismatchResult { sameObject := f.potentiallySameObject(existingEnclosing, f.EnclosingTypeDefinition) // Type mismatch relaxation (spec sec 5.3.2, SameResponseShape): when enclosing diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index ec61a0786d..d321093324 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -1149,12 +1149,12 @@ func TestExecutionValidation(t *testing.T) { } }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Valid) }) - t.Run("allows differing enum types on interface implementors with type mismatch flag via union", func(t *testing.T) { + t.Run("allows differing base types on union members with type mismatch flag", func(t *testing.T) { runWithDefinition(t, typeMismatchDefinition, ` { entity { - ... on User { id } - ... on Organization { id } + ... on User { priority } + ... on Organization { priority } } }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Valid) }) @@ -1219,6 +1219,20 @@ func TestExecutionValidation(t *testing.T) { RelaxTypeMismatchCheck: true, }), Valid) }) + // Narrow-scope tests: verify that the type mismatch relaxation is limited to + // disjoint concrete object types. These checks must still fail with the flag enabled. + t.Run("with type mismatch flag, nullability on overlapping interface types still rejects", func(t *testing.T) { + // Even a pure nullability difference (String! vs String) is rejected + // when one enclosing type is an interface (could overlap at runtime). + runWithDefinition(t, boxDefinition, ` + { + someBox { + ... on NonNullStringBox1 { scalar } + ... on StringBox { scalar } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), 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, ` { From 6ddec1af8870b618e3744e46e6577c409412a248 Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Mon, 2 Mar 2026 08:54:02 +0100 Subject: [PATCH 4/8] refactor: simplify print kit pools to single per-factory pool Replace the multi-pool architecture (sync.Map of pools keyed by relaxation flag combinations) with a single pool per Factory. The FieldSelectionMerging rule now holds a pointer to its config, allowing the validator to be reconfigured between Validate calls via SetFieldSelectionMergingConfig. This eliminates the need for separate validator instances per flag combination. The pool is lazily initialized on the Factory via sync.Once and shared across all Planners created from it. Each Planner sets the appropriate relaxation config on the validator before each validation call in printOperation. Co-Authored-By: Claude Opus 4.6 --- .../operation_rule_field_selection_merging.go | 27 +- v2/pkg/astvalidation/operation_validation.go | 15 +- .../operation_validation_test.go | 81 +++++ .../graphql_datasource/graphql_datasource.go | 104 ++----- .../graphql_datasource_test.go | 277 ++++++++++++++++++ 5 files changed, 415 insertions(+), 89 deletions(-) diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index 9d7681bed2..1e3850434e 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -31,11 +31,17 @@ func FieldSelectionMerging(config ...FieldSelectionMergingConfig) Rule { if len(config) > 0 { cfg = config[0] } + return fieldSelectionMergingRule(&cfg) +} + +// fieldSelectionMergingRule creates the rule with a shared config pointer. +// The visitor reads from the pointer on each walk, so the caller can update +// the config between Validate calls without recreating the validator. +func fieldSelectionMergingRule(cfg *FieldSelectionMergingConfig) Rule { return func(walker *astvisitor.Walker) { visitor := fieldSelectionMergingVisitor{ - Walker: walker, - relaxNullabilityCheck: cfg.RelaxNullabilityCheck, - relaxTypeMismatchCheck: cfg.RelaxTypeMismatchCheck, + Walker: walker, + config: cfg, } walker.RegisterEnterDocumentVisitor(&visitor) walker.RegisterEnterFieldVisitor(&visitor) @@ -47,12 +53,11 @@ func FieldSelectionMerging(config ...FieldSelectionMergingConfig) Rule { type fieldSelectionMergingVisitor struct { *astvisitor.Walker - definition, operation *ast.Document - scalarRequirements scalarRequirements - nonScalarRequirements nonScalarRequirements - refs []int - relaxNullabilityCheck bool - relaxTypeMismatchCheck bool + definition, operation *ast.Document + scalarRequirements scalarRequirements + nonScalarRequirements nonScalarRequirements + refs []int + config *FieldSelectionMergingConfig } type nonScalarRequirement struct { path ast.Path @@ -259,12 +264,12 @@ func (f *fieldSelectionMergingVisitor) checkTypeMismatch(existingEnclosing ast.N sameObject := f.potentiallySameObject(existingEnclosing, f.EnclosingTypeDefinition) // Type mismatch relaxation (spec sec 5.3.2, SameResponseShape): when enclosing // types are provably non-overlapping, any type difference is safe. - if f.relaxTypeMismatchCheck && !sameObject { + if f.config.RelaxTypeMismatchCheck && !sameObject { return typeMismatchSkip } // Nullability relaxation (spec sec 5.3.2, SameResponseShape): when enclosing // types cannot overlap at runtime, we allow nullability differences. - if f.relaxNullabilityCheck && !sameObject && + if f.config.RelaxNullabilityCheck && !sameObject && f.definition.TypesAreCompatibleIgnoringNullability(existingFieldType, currentFieldType) { return typeMismatchAccept } diff --git a/v2/pkg/astvalidation/operation_validation.go b/v2/pkg/astvalidation/operation_validation.go index b3a0d53f3f..99b59744e3 100644 --- a/v2/pkg/astvalidation/operation_validation.go +++ b/v2/pkg/astvalidation/operation_validation.go @@ -69,10 +69,11 @@ func DefaultOperationValidator(options ...Option) *OperationValidator { validator.RegisterRule(LoneAnonymousOperation()) validator.RegisterRule(SubscriptionSingleRootField()) validator.RegisterRule(FieldSelections()) - validator.RegisterRule(FieldSelectionMerging(FieldSelectionMergingConfig{ + validator.fieldSelectionMergingConfig = FieldSelectionMergingConfig{ RelaxNullabilityCheck: opts.RelaxFieldSelectionMergingNullabilityCheck, RelaxTypeMismatchCheck: opts.RelaxFieldSelectionMergingTypeMismatchCheck, - })) + } + validator.RegisterRule(fieldSelectionMergingRule(&validator.fieldSelectionMergingConfig)) validator.RegisterRule(KnownArguments()) validator.RegisterRule(Values()) validator.RegisterRule(ArgumentUniqueness()) @@ -101,7 +102,8 @@ func NewOperationValidator(rules []Rule) *OperationValidator { // OperationValidator orchestrates the validation process of Operations type OperationValidator struct { - walker astvisitor.Walker + walker astvisitor.Walker + fieldSelectionMergingConfig FieldSelectionMergingConfig } // RegisterRule registers a rule to the OperationValidator @@ -109,6 +111,13 @@ func (o *OperationValidator) RegisterRule(rule Rule) { rule(&o.walker) } +// SetFieldSelectionMergingConfig updates the field selection merging relaxation flags. +// This allows reusing a single validator (e.g. from a sync.Pool) with different +// relaxation settings per call, instead of maintaining separate validator instances. +func (o *OperationValidator) SetFieldSelectionMergingConfig(config FieldSelectionMergingConfig) { + o.fieldSelectionMergingConfig = config +} + // Validate validates the operation against the definition using the registered ruleset. func (o *OperationValidator) Validate(operation, definition *ast.Document, report *operationreport.Report) ValidationState { diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index d321093324..40930ae0b5 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -12548,3 +12548,84 @@ input UpdateRegionGameInput { input UserPreferencesInput { notifications: PreNotificationsInput! }` + +// TestSetFieldSelectionMergingConfig proves that a single DefaultOperationValidator +// can be reconfigured between Validate calls. This is the foundation for using a +// single sync.Pool of validators instead of one pool per relaxation-flag combination. +func TestSetFieldSelectionMergingConfig(t *testing.T) { + // Helper: parse and normalize, then validate with the given validator. + validate := func(t *testing.T, validator *OperationValidator, definitionStr, operationStr string) ValidationState { + t.Helper() + definition, report := astparser.ParseGraphqlDocumentString(definitionStr) + require.False(t, report.HasErrors(), report.Error()) + operation, report2 := astparser.ParseGraphqlDocumentString(operationStr) + require.False(t, report2.HasErrors(), report2.Error()) + var validationReport operationreport.Report + normalizer := astnormalization.NewWithOpts(astnormalization.WithInlineFragmentSpreads()) + normalizer.NormalizeOperation(&operation, &definition, &validationReport) + return validator.Validate(&operation, &definition, &validationReport) + } + + // nullabilityOp: User.email is String!, Organization.email is String → differs in nullability + const nullabilityOp = `{ entity { ... on User { email } ... on Organization { email } } }` + // typeMismatchOp: User.priority is Int, Organization.priority is String → completely different types + const typeMismatchOp = `{ entity { ... on User { priority } ... on Organization { priority } } }` + + t.Run("nullability relaxation cycle", func(t *testing.T) { + validator := DefaultOperationValidator() + + // Strict (default) → reject + require.Equal(t, Invalid, validate(t, validator, entityDefinition, nullabilityOp)) + + // Enable nullability relaxation → accept + validator.SetFieldSelectionMergingConfig(FieldSelectionMergingConfig{RelaxNullabilityCheck: true}) + require.Equal(t, Valid, validate(t, validator, entityDefinition, nullabilityOp)) + + // Reset to strict → reject again + validator.SetFieldSelectionMergingConfig(FieldSelectionMergingConfig{}) + require.Equal(t, Invalid, validate(t, validator, entityDefinition, nullabilityOp)) + }) + + t.Run("type mismatch relaxation cycle", func(t *testing.T) { + validator := DefaultOperationValidator() + + // Strict → reject + require.Equal(t, Invalid, validate(t, validator, typeMismatchDefinition, typeMismatchOp)) + + // Enable type mismatch relaxation → accept + validator.SetFieldSelectionMergingConfig(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}) + require.Equal(t, Valid, validate(t, validator, typeMismatchDefinition, typeMismatchOp)) + + // Reset → reject + validator.SetFieldSelectionMergingConfig(FieldSelectionMergingConfig{}) + require.Equal(t, Invalid, validate(t, validator, typeMismatchDefinition, typeMismatchOp)) + }) + + t.Run("both flags together", func(t *testing.T) { + validator := DefaultOperationValidator() + + // Both operations fail under strict + require.Equal(t, Invalid, validate(t, validator, entityDefinition, nullabilityOp)) + require.Equal(t, Invalid, validate(t, validator, typeMismatchDefinition, typeMismatchOp)) + + // Enable both → both pass + validator.SetFieldSelectionMergingConfig(FieldSelectionMergingConfig{ + RelaxNullabilityCheck: true, + RelaxTypeMismatchCheck: true, + }) + require.Equal(t, Valid, validate(t, validator, entityDefinition, nullabilityOp)) + require.Equal(t, Valid, validate(t, validator, typeMismatchDefinition, typeMismatchOp)) + }) + + t.Run("nullability flag alone does not cover type mismatches", func(t *testing.T) { + validator := DefaultOperationValidator() + validator.SetFieldSelectionMergingConfig(FieldSelectionMergingConfig{RelaxNullabilityCheck: true}) + require.Equal(t, Invalid, validate(t, validator, typeMismatchDefinition, typeMismatchOp)) + }) + + t.Run("type mismatch flag covers nullability differences", func(t *testing.T) { + validator := DefaultOperationValidator() + validator.SetFieldSelectionMergingConfig(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}) + require.Equal(t, Valid, validate(t, validator, entityDefinition, nullabilityOp)) + }) +} diff --git a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go index 061ea1d181..7a93f42f4f 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go @@ -93,6 +93,9 @@ type Planner[T Configuration] struct { // gRPC grpcClient grpc.ClientConnInterface + relaxNullabilityCheck bool + relaxTypeMismatchCheck bool + printKitPool *sync.Pool } @@ -1468,6 +1471,10 @@ func (p *Planner[T]) printOperation() (operationBytes []byte, variablesBytes []b copy(variablesBytes, p.upstreamOperation.Input.Variables) } + kit.validator.SetFieldSelectionMergingConfig(astvalidation.FieldSelectionMergingConfig{ + RelaxNullabilityCheck: p.relaxNullabilityCheck, + RelaxTypeMismatchCheck: p.relaxTypeMismatchCheck, + }) kit.validator.Validate(p.upstreamOperation, definition, kit.report) if kit.report.HasErrors() { p.stopWithError(errors.WithStack(fmt.Errorf("printOperation planner id: %d: validation failed: %w", p.id, kit.report))) @@ -1687,10 +1694,10 @@ type printKit struct { report *operationreport.Report } -func newPrintKitPool(validationOptions ...astvalidation.Option) *sync.Pool { +func newPrintKitPool() *sync.Pool { return &sync.Pool{ New: func() any { - validator := astvalidation.DefaultOperationValidator(validationOptions...) + validator := astvalidation.DefaultOperationValidator() // 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, @@ -1714,52 +1721,25 @@ func newPrintKitPool(validationOptions ...astvalidation.Option) *sync.Pool { } } -// Each combination of relaxation flags requires its own pool because the -// validator configuration is baked into the printKit at pool-creation time. -// Mixing printKits with different validator settings in the same pool would -// cause non-deterministic validation behavior. -var ( - defaultPrintKitPool = newPrintKitPool() - printKitPools sync.Map // printKitPoolKey -> *sync.Pool -) - -// printKitPoolKey identifies a unique pool for a given set of relaxation flags. -type printKitPoolKey struct { - relaxNullability bool - relaxTypeMismatch bool -} - -// getPrintKitPoolForConfig returns a lazily-initialized pool for the given -// relaxation flag combination. The zero-value key returns defaultPrintKitPool. -func getPrintKitPoolForConfig(key printKitPoolKey) *sync.Pool { - if !key.relaxNullability && !key.relaxTypeMismatch { - return defaultPrintKitPool - } - if v, ok := printKitPools.Load(key); ok { - return v.(*sync.Pool) - } - var opts []astvalidation.Option - if key.relaxNullability { - opts = append(opts, astvalidation.WithRelaxFieldSelectionMergingNullability()) - } - if key.relaxTypeMismatch { - opts = append(opts, astvalidation.WithRelaxFieldSelectionMergingTypeMismatch()) - } - pool := newPrintKitPool(opts...) - v, _ := printKitPools.LoadOrStore(key, pool) - return v.(*sync.Pool) -} - type Factory[T Configuration] struct { executionContext context.Context httpClient *http.Client grpcClient grpc.ClientConnInterface grpcClientProvider func() grpc.ClientConnInterface subscriptionClient GraphQLSubscriptionClient - printKitPool *sync.Pool relaxNullabilityCheck bool relaxTypeMismatchCheck bool + + printKitPool *sync.Pool + poolOnce sync.Once +} + +func (f *Factory[T]) getPrintKitPool() *sync.Pool { + f.poolOnce.Do(func() { + f.printKitPool = newPrintKitPool() + }) + return f.printKitPool } // NewFactory (HTTP) creates a new factory for the GraphQL datasource planner @@ -1822,53 +1802,25 @@ func NewFactoryGRPCClientProvider(executionContext context.Context, clientProvid } func (p *Planner[T]) getKit() *printKit { - pool := p.printKitPool - if pool == nil { - pool = defaultPrintKitPool - } - return pool.Get().(*printKit) + return p.printKitPool.Get().(*printKit) } func (p *Planner[T]) releaseKit(kit *printKit) { kit.buf.Reset() kit.report.Reset() - pool := p.printKitPool - if pool == nil { - pool = defaultPrintKitPool - } - pool.Put(kit) -} - -func (f *Factory[T]) resolvePool() *sync.Pool { - return getPrintKitPoolForConfig(printKitPoolKey{ - relaxNullability: f.relaxNullabilityCheck, - relaxTypeMismatch: f.relaxTypeMismatchCheck, - }) + p.printKitPool.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. +// plan.SubgraphFieldSelectionMergingNullabilityRelaxer. func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() { f.relaxNullabilityCheck = true - f.printKitPool = f.resolvePool() } // EnableSubgraphFieldSelectionMergingTypeMismatchRelaxation implements -// plan.SubgraphFieldSelectionMergingTypeMismatchRelaxer. It configures the -// factory to use a shared pool whose validator allows completely different types -// on fields in non-overlapping concrete types. +// plan.SubgraphFieldSelectionMergingTypeMismatchRelaxer. func (f *Factory[T]) EnableSubgraphFieldSelectionMergingTypeMismatchRelaxation() { f.relaxTypeMismatchCheck = true - f.printKitPool = f.resolvePool() -} - -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] { @@ -1878,10 +1830,12 @@ func (f *Factory[T]) Planner(logger abstractlogger.Logger) plan.DataSourcePlanne } return &Planner[T]{ - fetchClient: f.httpClient, - grpcClient: grpcClient, - subscriptionClient: f.subscriptionClient, - printKitPool: f.getPrintKitPool(), + fetchClient: f.httpClient, + grpcClient: grpcClient, + subscriptionClient: f.subscriptionClient, + relaxNullabilityCheck: f.relaxNullabilityCheck, + relaxTypeMismatchCheck: f.relaxTypeMismatchCheck, + 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 05b07df9e1..2b23ded4d6 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go @@ -8366,6 +8366,283 @@ func TestGraphQLDataSource(t *testing.T) { }) } +func TestFactorySinglePrintKitPool(t *testing.T) { + factory := &Factory[Configuration]{} + + p1 := factory.Planner(nil).(*Planner[Configuration]) + p2 := factory.Planner(nil).(*Planner[Configuration]) + p3 := factory.Planner(nil).(*Planner[Configuration]) + + // All planners must share the exact same pool instance. + require.Same(t, p1.printKitPool, p2.printKitPool) + require.Same(t, p2.printKitPool, p3.printKitPool) + + // Get and return a kit to verify the pool works. + kit := p1.getKit() + require.NotNil(t, kit) + p2.releaseKit(kit) + + // Getting from another planner may return the same kit (same pool). + kit2 := p3.getKit() + require.NotNil(t, kit2) + p3.releaseKit(kit2) +} + +func TestFieldSelectionMergingRelaxationCombinations(t *testing.T) { + // Schema where User.priority is Int and Organization.priority is String (type mismatch) + // and User.email is String! vs Organization.email is String (nullability mismatch). + definition := ` + type User { + id: ID! + email: String! + priority: Int + } + + type Organization { + id: ID! + email: String + priority: String + } + + union Entity = User | Organization + + type Query { + entity: Entity + } + ` + + makePlanConfig := func(t *testing.T, relaxNullability, relaxTypeMismatch bool) plan.Configuration { + t.Helper() + return 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", "priority"}}, + {TypeName: "Organization", FieldNames: []string{"id", "email", "priority"}}, + }, + }, + mustCustomConfiguration(t, ConfigurationInput{ + Fetch: &FetchConfiguration{URL: "https://example.com/graphql"}, + SchemaConfiguration: mustSchema(t, nil, definition), + }), + ), + }, + DisableResolveFieldPositions: true, + RelaxSubgraphOperationFieldSelectionMergingNullability: relaxNullability, + RelaxSubgraphOperationFieldSelectionMergingTypeMismatch: relaxTypeMismatch, + } + } + + t.Run("nullability relaxation only", func(t *testing.T) { + 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")}, + }, + }, + }, + }, + }, + }, + }, + }, + makePlanConfig(t, true, false), + WithDefaultPostProcessor(), + WithValidationOptions(astvalidation.WithRelaxFieldSelectionMergingNullability()), + )) + }) + + t.Run("type mismatch relaxation only", func(t *testing.T) { + t.Run("run", RunTest(definition, ` + query MyQuery { + entity { + ... on User { priority } + ... on Organization { priority } + } + }`, + "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 {priority} ... on Organization {priority}}}"}}`, + 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("priority"), + Value: &resolve.Integer{ + Path: []string{"priority"}, + Nullable: true, + }, + OnTypeNames: [][]byte{[]byte("User")}, + }, + { + Name: []byte("priority"), + Value: &resolve.String{ + Path: []string{"priority"}, + Nullable: true, + }, + OnTypeNames: [][]byte{[]byte("Organization")}, + }, + }, + }, + }, + }, + }, + }, + }, + makePlanConfig(t, false, true), + WithDefaultPostProcessor(), + WithValidationOptions(astvalidation.WithRelaxFieldSelectionMergingTypeMismatch()), + )) + }) + + t.Run("both relaxations", func(t *testing.T) { + t.Run("run", RunTest(definition, ` + query MyQuery { + entity { + ... on User { email priority } + ... on Organization { email priority } + } + }`, + "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 priority} ... on Organization {email priority}}}"}}`, + 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("priority"), + Value: &resolve.Integer{ + Path: []string{"priority"}, + Nullable: true, + }, + OnTypeNames: [][]byte{[]byte("User")}, + }, + { + Name: []byte("email"), + Value: &resolve.String{ + Path: []string{"email"}, + Nullable: true, + }, + OnTypeNames: [][]byte{[]byte("Organization")}, + }, + { + Name: []byte("priority"), + Value: &resolve.String{ + Path: []string{"priority"}, + Nullable: true, + }, + OnTypeNames: [][]byte{[]byte("Organization")}, + }, + }, + }, + }, + }, + }, + }, + }, + makePlanConfig(t, true, true), + WithDefaultPostProcessor(), + WithValidationOptions( + astvalidation.WithRelaxFieldSelectionMergingNullability(), + astvalidation.WithRelaxFieldSelectionMergingTypeMismatch(), + ), + )) + }) +} + var errSubscriptionClientFail = errors.New("subscription client fail error") type FailingSubscriptionClient struct{} From f7f61e651274b82ae4310e83ced55c4c00ad910a Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Mon, 2 Mar 2026 08:59:47 +0100 Subject: [PATCH 5/8] fix: correct gofmt alignment in test struct literals Co-Authored-By: Claude Opus 4.6 --- .../graphql_datasource/graphql_datasource_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 2b23ded4d6..5ab19a37f7 100644 --- a/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go +++ b/v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go @@ -8433,9 +8433,9 @@ func TestFieldSelectionMergingRelaxationCombinations(t *testing.T) { }), ), }, - DisableResolveFieldPositions: true, - RelaxSubgraphOperationFieldSelectionMergingNullability: relaxNullability, - RelaxSubgraphOperationFieldSelectionMergingTypeMismatch: relaxTypeMismatch, + DisableResolveFieldPositions: true, + RelaxSubgraphOperationFieldSelectionMergingNullability: relaxNullability, + RelaxSubgraphOperationFieldSelectionMergingTypeMismatch: relaxTypeMismatch, } } From 8fe751c32c4e7fa12e06a7d06f7b5bd6255e003c Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Mon, 2 Mar 2026 09:21:58 +0100 Subject: [PATCH 6/8] refactor: add explicit typeMismatchAccept case to switch statements Make the three-way enum handling self-documenting by adding explicit case arms instead of relying on implicit fall-through. Co-Authored-By: Claude Opus 4.6 --- .../astvalidation/operation_rule_field_selection_merging.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go index 1e3850434e..e49885e820 100644 --- a/v2/pkg/astvalidation/operation_rule_field_selection_merging.go +++ b/v2/pkg/astvalidation/operation_rule_field_selection_merging.go @@ -149,6 +149,8 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { } } else if !f.definition.TypesAreCompatibleDeep(f.nonScalarRequirements[i].fieldTypeRef, fieldType) { switch f.checkTypeMismatch(f.nonScalarRequirements[i].enclosingTypeDefinition, f.nonScalarRequirements[i].fieldTypeRef, fieldType) { + case typeMismatchAccept: + // Types differ only in nullability; allowed by relaxation config. case typeMismatchSkip: continue case typeMismatchReject: @@ -199,6 +201,8 @@ func (f *fieldSelectionMergingVisitor) EnterField(ref int) { } if !f.definition.TypesAreCompatibleDeep(f.scalarRequirements[i].fieldType, fieldType) { switch f.checkTypeMismatch(f.scalarRequirements[i].enclosingTypeDefinition, f.scalarRequirements[i].fieldType, fieldType) { + case typeMismatchAccept: + // Types differ only in nullability; allowed by relaxation config. case typeMismatchSkip: continue case typeMismatchReject: From fb1be321cf56f2757321084d4715479c9deafc27 Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Mon, 2 Mar 2026 09:48:02 +0100 Subject: [PATCH 7/8] refactor: address review low-priority findings - Fix duplicate test: differentiate enum vs base type mismatch tests by querying enum types (IssueState/PullRequestReviewState) via union - Add godoc note that SetFieldSelectionMergingConfig only works with DefaultOperationValidator - Rename nullability_relaxation_justification.md to reflect broader scope - Make "both flags" execution engine test exercise both type mismatch and nullability differences simultaneously Co-Authored-By: Claude Opus 4.6 --- execution/engine/execution_engine_test.go | 16 ++++++++-------- ...election_merging_relaxation_justification.md} | 0 v2/pkg/astvalidation/operation_validation.go | 5 +++++ .../astvalidation/operation_validation_test.go | 9 ++++++--- 4 files changed, 19 insertions(+), 11 deletions(-) rename v2/pkg/astvalidation/{nullability_relaxation_justification.md => field_selection_merging_relaxation_justification.md} (100%) diff --git a/execution/engine/execution_engine_test.go b/execution/engine/execution_engine_test.go index d6c104fdda..312754264b 100644 --- a/execution/engine/execution_engine_test.go +++ b/execution/engine/execution_engine_test.go @@ -7730,16 +7730,16 @@ func TestExecutionEngine_Execute(t *testing.T) { enum PullRequestReviewState { PENDING APPROVED } union Updatable = Issue | PullRequestReview type Query { updatable: Updatable } - type Issue { id: ID!, state: IssueState } - type PullRequestReview { id: ID!, state: PullRequestReviewState } + type Issue { id: ID!, state: IssueState, title: String! } + type PullRequestReview { id: ID!, state: PullRequestReviewState, title: String } ` schema, err := graphql.NewSchemaFromString(unionSchema) require.NoError(t, err) rootNodes := []plan.TypeField{ {TypeName: "Query", FieldNames: []string{"updatable"}}, - {TypeName: "Issue", FieldNames: []string{"id", "state"}}, - {TypeName: "PullRequestReview", FieldNames: []string{"id", "state"}}, + {TypeName: "Issue", FieldNames: []string{"id", "state", "title"}}, + {TypeName: "PullRequestReview", FieldNames: []string{"id", "state", "title"}}, } customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ @@ -7854,13 +7854,13 @@ func TestExecutionEngine_Execute(t *testing.T) { relaxFieldSelectionMergingTypeMismatch(), )) - t.Run("with both relaxation flags, different enum types work", runWithoutError( + t.Run("with both relaxation flags, different enum types and nullability work", runWithoutError( ExecutionEngineTestCase{ schema: schema, operation: func(t *testing.T) graphql.Request { return graphql.Request{ OperationName: "O", - Query: `query O { updatable { ... on Issue { state } ... on PullRequestReview { state } } }`, + Query: `query O { updatable { ... on Issue { state title } ... on PullRequestReview { state title } } }`, } }, dataSources: []plan.DataSource{ @@ -7870,7 +7870,7 @@ func TestExecutionEngine_Execute(t *testing.T) { expectedHost: "example.com", expectedPath: "/", expectedBody: "", - sendResponseBody: `{"data":{"updatable":{"__typename":"Issue","state":"OPEN"}}}`, + sendResponseBody: `{"data":{"updatable":{"__typename":"Issue","state":"OPEN","title":"Bug report"}}}`, sendStatusCode: 200, }), ), @@ -7881,7 +7881,7 @@ func TestExecutionEngine_Execute(t *testing.T) { ), }, fields: fieldConfig, - expectedResponse: `{"data":{"updatable":{"state":"OPEN"}}}`, + expectedResponse: `{"data":{"updatable":{"state":"OPEN","title":"Bug report"}}}`, }, relaxFieldSelectionMergingNullability(), relaxFieldSelectionMergingTypeMismatch(), diff --git a/v2/pkg/astvalidation/nullability_relaxation_justification.md b/v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md similarity index 100% rename from v2/pkg/astvalidation/nullability_relaxation_justification.md rename to v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md diff --git a/v2/pkg/astvalidation/operation_validation.go b/v2/pkg/astvalidation/operation_validation.go index 99b59744e3..7dc14969da 100644 --- a/v2/pkg/astvalidation/operation_validation.go +++ b/v2/pkg/astvalidation/operation_validation.go @@ -114,6 +114,11 @@ func (o *OperationValidator) RegisterRule(rule Rule) { // SetFieldSelectionMergingConfig updates the field selection merging relaxation flags. // This allows reusing a single validator (e.g. from a sync.Pool) with different // relaxation settings per call, instead of maintaining separate validator instances. +// +// Note: This only works when the validator was created via DefaultOperationValidator, +// which binds the FieldSelectionMerging rule to the validator's config field. +// Validators created via NewOperationValidator with FieldSelectionMerging() will +// have an independent config copy and will not be affected by this method. func (o *OperationValidator) SetFieldSelectionMergingConfig(config FieldSelectionMergingConfig) { o.fieldSelectionMergingConfig = config } diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index 40930ae0b5..d14ca5a0a3 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -1143,9 +1143,9 @@ func TestExecutionValidation(t *testing.T) { t.Run("allows differing enum types on union members with type mismatch flag", func(t *testing.T) { runWithDefinition(t, typeMismatchDefinition, ` { - entity { - ... on User { priority } - ... on Organization { priority } + searchable { + ... on Issue { state } + ... on PullRequestReview { state } } }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Valid) }) @@ -5872,9 +5872,12 @@ type Organization { union Entity = User | Organization +union Searchable = Issue | PullRequestReview + type Query { updatable: Updatable entity: Entity + searchable: Searchable } schema { From 2c05f55dbd9c5285e6dab9a65af3beb6968ec4cf Mon Sep 17 00:00:00 2001 From: Jens Neuse Date: Mon, 2 Mar 2026 19:19:47 +0100 Subject: [PATCH 8/8] test: address PR review comments - Add negative test for scalar type mismatch without relaxation flag - Add narrow-scope tests proving relaxation doesn't apply when one enclosing type is an interface (even with both flags enabled) - Clarify in justification doc that "base types must match" applies only to RelaxNullabilityCheck, not RelaxTypeMismatchCheck Co-Authored-By: Claude Opus 4.6 --- execution/engine/execution_engine_test.go | 39 +++++++++++++++++-- ...ection_merging_relaxation_justification.md | 9 +++-- .../operation_validation_test.go | 36 +++++++++++++++++ 3 files changed, 77 insertions(+), 7 deletions(-) diff --git a/execution/engine/execution_engine_test.go b/execution/engine/execution_engine_test.go index 312754264b..be3df48ae2 100644 --- a/execution/engine/execution_engine_test.go +++ b/execution/engine/execution_engine_test.go @@ -7730,16 +7730,16 @@ func TestExecutionEngine_Execute(t *testing.T) { enum PullRequestReviewState { PENDING APPROVED } union Updatable = Issue | PullRequestReview type Query { updatable: Updatable } - type Issue { id: ID!, state: IssueState, title: String! } - type PullRequestReview { id: ID!, state: PullRequestReviewState, title: String } + type Issue { id: ID!, state: IssueState, title: String!, priority: Int } + type PullRequestReview { id: ID!, state: PullRequestReviewState, title: String, priority: String } ` schema, err := graphql.NewSchemaFromString(unionSchema) require.NoError(t, err) rootNodes := []plan.TypeField{ {TypeName: "Query", FieldNames: []string{"updatable"}}, - {TypeName: "Issue", FieldNames: []string{"id", "state", "title"}}, - {TypeName: "PullRequestReview", FieldNames: []string{"id", "state", "title"}}, + {TypeName: "Issue", FieldNames: []string{"id", "state", "title", "priority"}}, + {TypeName: "PullRequestReview", FieldNames: []string{"id", "state", "title", "priority"}}, } customConfig := mustConfiguration(t, graphql_datasource.ConfigurationInput{ @@ -7822,6 +7822,37 @@ func TestExecutionEngine_Execute(t *testing.T) { `fields 'priority' conflict because they return conflicting types 'Int' and 'String', locations: [], path: [query,entity,$1Organization]`, )) + t.Run("without relaxation flag, validation rejects different scalar types on union members", runWithAndCompareError( + ExecutionEngineTestCase{ + schema: schema, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + OperationName: "O", + Query: `query O { updatable { ... on Issue { priority } ... on PullRequestReview { priority } } }`, + } + }, + dataSources: []plan.DataSource{ + mustGraphqlDataSourceConfiguration(t, "ds-id", + mustFactory(t, + testNetHttpClient(t, roundTripperTestCase{ + expectedHost: "example.com", + expectedPath: "/", + expectedBody: "", + sendResponseBody: `{"data":{"updatable":{"__typename":"Issue","priority":1}}}`, + sendStatusCode: 200, + }), + ), + &plan.DataSourceMetadata{ + RootNodes: rootNodes, + }, + customConfig, + ), + }, + fields: fieldConfig, + }, + `fields 'priority' conflict because they return conflicting types 'Int' and 'String', locations: [], path: [query,updatable,$1PullRequestReview]`, + )) + t.Run("with relaxation flag, different enum types work", runWithoutError( ExecutionEngineTestCase{ schema: schema, diff --git a/v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md b/v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md index 12614d4d41..8c60aa16b3 100644 --- a/v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md +++ b/v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md @@ -130,9 +130,12 @@ fails (e.g. `String!` vs `String`), we now 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. +2. **If they can't overlap** (nullability relaxation only): use + `TypesAreCompatibleIgnoringNullability` which strips `NonNull` wrappers at + every nesting level but still requires the same list structure and base named + type. Note: this "base types must match" guarantee applies only to + `RelaxNullabilityCheck`. The broader `RelaxTypeMismatchCheck` permits + entirely different base named types on non-overlapping concrete types. Existing test cases are unaffected: diff --git a/v2/pkg/astvalidation/operation_validation_test.go b/v2/pkg/astvalidation/operation_validation_test.go index d14ca5a0a3..10e2480dc2 100644 --- a/v2/pkg/astvalidation/operation_validation_test.go +++ b/v2/pkg/astvalidation/operation_validation_test.go @@ -1233,6 +1233,42 @@ func TestExecutionValidation(t *testing.T) { }`, FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}), Invalid, withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String!' and 'String'`)) }) + t.Run("with both flags, field selected at interface level conflicts with concrete fragment field", func(t *testing.T) { + // SomeBox.scalar is String (interface), IntBox.scalar is Int (concrete fragment). + // The interface-level selection causes potentiallySameObject to return true, + // so relaxation does not apply even with both flags enabled. + runWithDefinition(t, boxDefinition, ` + { + someBox { + scalar + ... on IntBox { scalar } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{ + RelaxNullabilityCheck: true, + RelaxTypeMismatchCheck: true, + }), Invalid, + withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String' and 'Int'`)) + }) + t.Run("with both flags, type mismatch on same concrete type still rejects", func(t *testing.T) { + // IntBox.scalar is Int, StringBox.scalar is String. Both concrete types + // implement SomeBox, but the type condition in inline fragments narrows + // to concrete types. Without the inline fragments the scalar is accessed + // at the SomeBox interface level where it's String. + // Test that the field selected at the interface level (String) conflicts + // with the concrete fragment field (Int) even with both flags. + runWithDefinition(t, boxDefinition, ` + { + someBox { + scalar + ... on StringBox { scalar } + ... on IntBox { scalar } + } + }`, FieldSelectionMerging(FieldSelectionMergingConfig{ + RelaxNullabilityCheck: true, + RelaxTypeMismatchCheck: true, + }), Invalid, + withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String' and 'Int'`)) + }) t.Run("same wrapped scalar return types", func(t *testing.T) { runWithDefinition(t, boxDefinition, ` {