feat: add flag for relaxed nullability checks on same shape

[jensneuse]

Summary by CodeRabbit

• New Features

  • Optional relaxation of nullability validation for field-selection merging, controllable by configuration and validator options; factories/planners can opt in.
  • Per-instance pooling for planner resources to improve isolation and predictability.

• Documentation

  • Added detailed rationale and guidance for the nullability-relaxation behavior.

• Tests

  • Expanded validation, engine, datasource, planner, and resolve tests covering non-overlapping-type nullability scenarios.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds an optional, conservative relaxation for nullability checks during GraphQL field selection merging and threads that option through AST validators, planner/factory/printKit initialization, request validation, execution engine wiring, and tests for non-overlapping-type scenarios. Strict checks remain for potentially overlapping types.

Changes

Cohort / File(s)	Summary
Type compatibility v2/pkg/ast/ast_type.go, v2/pkg/ast/ast_type_test.go	Add TypesAreCompatibleIgnoringNullability and internal typesAreCompatible(..., ignoreNullability) to compare types while stripping NonNull wrappers; update TypesAreCompatibleDeep to use helper; add unit tests.
Validation API & caching execution/graphql/validation.go	Request.ValidateForSchema now accepts variadic astvalidation.Option; DefaultOperationValidator(options...) used; per-schema validation cache bypassed when options are provided.
Operation validator options v2/pkg/astvalidation/operation_validation.go	Add RelaxFieldSelectionMergingNullabilityCheck to OperationValidatorOptions and WithRelaxFieldSelectionMergingNullability(); pass flag into FieldSelectionMerging(...).
Field selection merging rule & docs v2/pkg/astvalidation/operation_rule_field_selection_merging.go, v2/pkg/astvalidation/nullability_relaxation_justification.md	Make FieldSelectionMerging accept optional relaxNullabilityCheck; track enclosing type definitions; add potentiallySameObject() overlap check; conditionally use TypesAreCompatibleIgnoringNullability for non-overlapping concrete types; add justification doc.
Field selection tests v2/pkg/astvalidation/operation_validation_test.go	Add tests covering differing nullability across non-overlapping object types and enforcement when interfaces could overlap; add schema snippets exercising scenarios.
Planner/Factory printKit pools v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go, v2/pkg/engine/datasource/graphql_datasource_test.go	Replace global printKit pool with per-Factory/per-Planner printKitPool(validationOptions...); add newPrintKitPool(...), relaxed pool initializer, EnableSubgraphFieldSelectionMergingNullabilityRelaxation() and wiring; add tests for inline fragments with differing nullability.
Test harness validation wiring v2/pkg/engine/datasourcetesting/datasourcetesting.go, execution/engine/execution_engine_test.go	Add WithValidationOptions(...) test hook and validationOptions storage; expose test helper to enable relaxation in tests; tighten some error assertions.
Execution engine execution/engine/execution_engine.go	Wire validation options into ExecutionEngine (compute/passthrough when config flag RelaxSubgraphOperationFieldSelectionMergingNullability is enabled) so validation uses configured options.
Configuration & planner creation v2/pkg/engine/plan/configuration.go, v2/pkg/engine/plan/datasource_configuration.go	Add exported RelaxSubgraphOperationFieldSelectionMergingNullability flag to Configuration; introduce SubgraphFieldSelectionMergingNullabilityRelaxer interface and invoke factory hook when config flag is set during planner/datasource setup.
Resolve tests v2/pkg/engine/resolve/resolve_test.go	Add tests ensuring resolver behavior respects per-type nullability for polymorphic fields across non-overlapping types.
Execution validation wiring execution/engine/execution_engine.go, execution/engine/execution_engine_test.go	Compute and pass validation options into ValidateForSchema during execution when planner config or test flags enable nullability relaxation.

Sequence Diagram(s)

sequenceDiagram
  participant Config
  participant Factory
  participant Planner
  participant ExecutionEngine
  participant Request
  participant Validator

  rect rgba(200,200,255,0.5)
  Config->>Factory: set RelaxSubgraphOperationFieldSelectionMergingNullability
  Factory->>Factory: getPrintKitPool(validationOptions...)
  Factory->>Planner: create Planner(printKitPool)
  end

  rect rgba(200,255,200,0.5)
  ExecutionEngine->>Planner: obtain Planner (contains printKitPool)
  ExecutionEngine->>Request: ValidateForSchema(schema, validationOptions...)
  Request->>Validator: DefaultOperationValidator(options...)
  Validator->>Validator: FieldSelectionMerging(relaxFlag?)
  Validator-->>Request: validation result
  Request-->>ExecutionEngine: return result
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• fix: fix collecting representation for fetches scoped to concrete types #1200 — Related changes touching federation planning and scoping fetches by concrete __typename, which intersects with per-type handling introduced for nullability and selection merging.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main objective: adding a flag to enable relaxed nullability checks for field selection merging on non-overlapping types with the same response shape.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into master

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/allow-differing-nullability-non-overlapping-types

---

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

execution/graphql/validation.go (1)

14-41: ⚠️ Potential issue | 🟠 Major

Cache key ignores validation options.

With options now affecting validation, caching only by schema hash can return a relaxed result for a strict call (or vice‑versa) when the same request is validated with different options. Consider bypassing the cache when options are provided, or incorporate an options hash into the key.

💡 Suggested fix (bypass cache when options are supplied)

 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()
 	if report.HasErrors() {
 		return operationValidationResultFromReport(report)
 	}
 
 	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
 }

🧹 Nitpick comments (2)

v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)

1803-1808: Potential data race on lazy pool initialization.

getPrintKitPool() performs a non-atomic check-then-write on f.printKitPool. If Planner() is called concurrently when the pool is uninitialized, multiple pools could be created with one being lost.

If concurrent factory usage is expected, consider using sync.Once for initialization:

♻️ Thread-safe lazy initialization

 type Factory[T Configuration] struct {
 	executionContext   context.Context
 	httpClient         *http.Client
 	grpcClient         grpc.ClientConnInterface
 	grpcClientProvider func() grpc.ClientConnInterface
 	subscriptionClient GraphQLSubscriptionClient
 	printKitPool       *sync.Pool
+	printKitPoolOnce   sync.Once
+	relaxNullability   bool
 }

 func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() {
-	f.printKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability())
+	f.relaxNullability = true
 }

 func (f *Factory[T]) getPrintKitPool() *sync.Pool {
-	if f.printKitPool == nil {
-		f.printKitPool = newPrintKitPool()
-	}
+	f.printKitPoolOnce.Do(func() {
+		if f.relaxNullability {
+			f.printKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability())
+		} else {
+			f.printKitPool = newPrintKitPool()
+		}
+	})
 	return f.printKitPool
 }

v2/pkg/astvalidation/operation_validation_test.go (1)

1080-1131: Add a relaxed‑mode overlap guard test.
The new cases cover non‑overlapping concrete types, but they don’t assert that FieldSelectionMerging(true) still rejects potentially overlapping types (e.g., interface vs object). Adding a variant will lock that behavior in.

🧪 Suggested test addition

+					t.Run("still rejects differing return type nullability when overlap is possible (relaxed)", func(t *testing.T) {
+						runWithDefinition(t, boxDefinition, `
+								{
+									someBox {
+										... on NonNullStringBox1 {
+											scalar
+										}
+										... on StringBox {
+											scalar
+										}
+									}
+								}`, FieldSelectionMerging(true), Invalid,
+							withValidationErrors(`fields 'scalar' conflict because they return conflicting types 'String!' and 'String'`))
+					})

[coderabbitai]

🧹 Nitpick comments (1)

v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)

1723-1725: Avoid reconfiguring the pool after initialization.

EnableSubgraphFieldSelectionMergingNullabilityRelaxation() replaces the pool without synchronization; if called after any planner creation, you can end up with mixed validation behavior across planners and potential data races. Consider storing a flag and applying options only at first initialization.

♻️ Suggested refactor to make configuration one-time

 type Factory[T Configuration] struct {
 	executionContext   context.Context
 	httpClient         *http.Client
 	grpcClient         grpc.ClientConnInterface
 	grpcClientProvider func() grpc.ClientConnInterface
 	subscriptionClient GraphQLSubscriptionClient
 	printKitPool       *sync.Pool
 	printKitPoolOnce   sync.Once
+	relaxFieldSelectionMergingNullability bool
 }
 
 func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() {
-	f.printKitPool = newPrintKitPool(astvalidation.WithRelaxFieldSelectionMergingNullability())
+	f.relaxFieldSelectionMergingNullability = true
 }
 
 func (f *Factory[T]) getPrintKitPool() *sync.Pool {
 	f.printKitPoolOnce.Do(func() {
-		if f.printKitPool == nil {
-			f.printKitPool = newPrintKitPool()
-		}
+		opts := []astvalidation.Option{}
+		if f.relaxFieldSelectionMergingNullability {
+			opts = append(opts, astvalidation.WithRelaxFieldSelectionMergingNullability())
+		}
+		f.printKitPool = newPrintKitPool(opts...)
 	})
 	return f.printKitPool
 }

Also applies to: 1796-1811

[coderabbitai]

🧹 Nitpick comments (1)

v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)

1797-1814: Consider adding a safeguard against out-of-order calls.

The sync.Once pattern means that if getPrintKitPool() (or Planner()) is called before EnableSubgraphFieldSelectionMergingNullabilityRelaxation(), the relaxation flag will be ignored silently. The comment documents the requirement, but a defensive check could prevent subtle bugs.

💡 Optional: Add a check or panic for out-of-order calls

 func (f *Factory[T]) EnableSubgraphFieldSelectionMergingNullabilityRelaxation() {
+	// Ensure this is called before the pool is initialized
+	if f.printKitPool != nil {
+		panic("EnableSubgraphFieldSelectionMergingNullabilityRelaxation must be called before Planner() or getPrintKitPool()")
+	}
 	f.relaxFieldMergingNullability = true
 }

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go`:
- Around line 1717-1721: The gci formatting linter flagged import
grouping/ordering that causes a failure near the var block (variables
defaultPrintKitPool, relaxedPrintKitPool, relaxedPrintKitPoolOnce); fix by
running your project's formatter (e.g., gci write) to auto-reorder import groups
and then run gofmt/goimports (or your repo's format command) to apply
formatting; re-run linters to confirm the import grouping issue is resolved and
commit the resulting changes.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go`:
- Around line 1798-1806: The getKit and releaseKit methods on Planner[T] can
panic if p.printKitPool is nil; update getKit to use p.printKitPool if non-nil
otherwise fall back to defaultPrintKitPool when calling Get(), and update
releaseKit to reset and Put on p.printKitPool if non-nil otherwise use
defaultPrintKitPool; change the methods referenced are Planner.getKit,
Planner.releaseKit (kept used by printOperation and exposed via
DataSourcePlanner.ConfigureFetch/ConfigureSubscription) so the fallback to
defaultPrintKitPool prevents nil dereference when Planner is instantiated
directly.