Skip to content

feat: add field selection merging type mismatch relaxation#1412

Open
jensneuse wants to merge 8 commits intomasterfrom
jensneuse/field-merge-analysis
Open

feat: add field selection merging type mismatch relaxation#1412
jensneuse wants to merge 8 commits intomasterfrom
jensneuse/field-merge-analysis

Conversation

@jensneuse
Copy link
Member

@jensneuse jensneuse commented Mar 2, 2026

Summary

  • Add a RelaxTypeMismatchCheck flag to field selection merging validation that allows completely different types (e.g. IssueState vs PullRequestReviewState) on fields in non-overlapping concrete object types — addressing schemas like GitHub's where interfaces have implementing types with same-named fields returning different leaf types
  • Extract duplicated type mismatch/nullability relaxation logic into a checkTypeMismatch helper method with a clear three-outcome result type (typeMismatchAccept, typeMismatchSkip, typeMismatchReject)
  • Consolidate print kit pool management from separate variables and sync.Once into a sync.Map keyed by flag combination
  • Add comprehensive tests for the new relaxation flag (both standalone and combined with the existing nullability relaxation)

Test plan

  • Existing astvalidation tests pass — no regressions on strict-mode validation
  • New unit tests verify type mismatch relaxation accepts different types on non-overlapping concrete types while rejecting them on same/overlapping types
  • Execution engine integration tests pass with both individual and combined relaxation flags
  • GraphQL datasource tests pass

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced optional type mismatch relaxation for field selection merging in subgraph operations. When enabled, allows fields with different leaf types to be merged when their enclosing types are non-overlapping concrete object types.
  • Tests

    • Added comprehensive test coverage for type mismatch relaxation scenarios and validation of the new configuration option.

jensneuse and others added 2 commits February 26, 2026 23:13
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 2, 2026

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

This PR extends field selection merging validation with support for relaxing type mismatch checks on non-overlapping concrete object types in subgraph operations. It adds a FieldSelectionMergingConfig struct with a RelaxTypeMismatchCheck flag, wires this through the execution engine, operation validator, and datasource layers, and refactors pool management in the GraphQL datasource to enable runtime relaxation toggles. Comprehensive test coverage validates the new feature across configuration, validation, and datasource components.

Changes

Cohort / File(s) Summary
Configuration & Planning
v2/pkg/engine/plan/configuration.go, v2/pkg/engine/plan/datasource_configuration.go, execution/engine/execution_engine.go
Adds RelaxSubgraphOperationFieldSelectionMergingTypeMismatch configuration flag and SubgraphFieldSelectionMergingTypeMismatchRelaxer interface; wires relaxation flag into execution engine validator options via conditional validation option appending.
Validation Core Logic
v2/pkg/astvalidation/operation_rule_field_selection_merging.go, v2/pkg/astvalidation/operation_validation.go
Refactors field selection merging rule to accept FieldSelectionMergingConfig struct (replacing boolean parameter) with RelaxNullabilityCheck and RelaxTypeMismatchCheck flags; introduces checkTypeMismatch() decision logic with three outcomes (accept, skip, reject); adds RelaxFieldSelectionMergingTypeMismatchCheck option and SetFieldSelectionMergingConfig() method for runtime reconfiguration.
GraphQL Datasource
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
Refactors pool management from multiple relaxed/default pools to single per-config pool with sync.Once initialization; propagates relaxNullabilityCheck and relaxTypeMismatchCheck flags through Factory and Planner structs; changes EnableSubgraphFieldSelectionMerging*Relaxation methods from pool-swapping to flag-toggling; updates printOperation() to pass flags to validator via FieldSelectionMergingConfig.
Testing
execution/engine/execution_engine_test.go, v2/pkg/astvalidation/operation_validation_test.go, v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
Adds test option for type mismatch relaxation in execution engine tests; introduces "Type mismatch relaxation tests" cohort with unions/interfaces/disjoint types; adds TestSetFieldSelectionMergingConfig() validating runtime reconfiguration of a single validator instance; adds TestFactorySinglePrintKitPool() and TestFieldSelectionMergingRelaxationCombinations() for pool and relaxation behavior validation.
Documentation
v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md
Broadens justification scope from nullability-only to general field selection merging relaxation; adds "Type Mismatch Relaxation" section detailing safety constraints (non-overlapping concrete types only, never interfaces); clarifies interaction between type mismatch and nullability relaxations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • PR #1378: Adds nullability relaxation wiring through execution engine and planner configuration; this PR introduces parallel type mismatch relaxation wiring using the same plumbing patterns and configuration layer architecture.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add field selection merging type mismatch relaxation' is clear and directly describes the main feature added across multiple files and components in this pull request.

✏️ 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 jensneuse/field-merge-analysis

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
execution/engine/execution_engine_test.go (1)

7727-7825: Add a strict-mode control case for this new scenario.

Both new tests enable relaxation. Please add one sibling case without relaxFieldSelectionMergingTypeMismatch() and assert validation failure, so default strict behavior is guarded at engine-integration level.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@execution/engine/execution_engine_test.go` around lines 7727 - 7825, Add a
sibling test case under the same t.Run("field merging with different types on
non-overlapping union types") that exercises the identical operation and
dataSource setup but does NOT pass relaxFieldSelectionMergingTypeMismatch(); use
the corresponding failure helper (e.g., runWithError or the test harness helper
used elsewhere for expecting validation errors) instead of runWithoutError and
set the test's ExecutionEngineTestCase to expect a validation failure (assert an
error/validation message) for the type-mismatch on the union field; reference
the existing inner cases (the ones named "with relaxation flag, different enum
types work" and "with both relaxation flags, different enum types work"), the
ExecutionEngineTestCase struct, and the relaxFieldSelectionMergingTypeMismatch()
flag so reviewers can locate and mirror the setup but invert the relaxation flag
to confirm strict-mode validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@v2/pkg/astvalidation/operation_rule_field_selection_merging.go`:
- Around line 29-33: FieldSelectionMerging changed signature from variadic bool
to variadic FieldSelectionMergingConfig causing external callers passing a bare
bool to break; restore backward compatibility by adding a wrapper overload that
accepts the old signature (e.g., detect when first arg is a bool and convert it
into a FieldSelectionMergingConfig with the equivalent field), keep the new
FieldSelectionMerging(config ...FieldSelectionMergingConfig) implementation
intact, and ensure callers of FieldSelectionMerging(true) or
FieldSelectionMerging(false) are routed through the wrapper which constructs and
forwards a proper FieldSelectionMergingConfig.

In `@v2/pkg/astvalidation/operation_validation_test.go`:
- Around line 1133-1160: The third test ("allows differing enum types on
interface implementors with type mismatch flag via union") is using the `id`
field which is `ID!` on both `User` and `Organization`, so it doesn't exercise
type-mismatch relaxation; update the test to actually use the mismatched field
used elsewhere (e.g., `priority`) or rename the test to match its current
assertion. Concretely, in the `runWithDefinition(..., typeMismatchDefinition,
"...", FieldSelectionMerging(FieldSelectionMergingConfig{RelaxTypeMismatchCheck:
true}), Valid)` case, replace the selection of `id` with the mismatched field
`priority` (so it mirrors the earlier `priority` tests that assert `Int` vs
`String`) if you want to test RelaxTypeMismatchCheck, or change the test name to
indicate it validates identical `ID` types; reference `runWithDefinition`,
`typeMismatchDefinition`, `FieldSelectionMerging`, and
`FieldSelectionMergingConfig{RelaxTypeMismatchCheck: true}` when making the
change.

---

Nitpick comments:
In `@execution/engine/execution_engine_test.go`:
- Around line 7727-7825: Add a sibling test case under the same t.Run("field
merging with different types on non-overlapping union types") that exercises the
identical operation and dataSource setup but does NOT pass
relaxFieldSelectionMergingTypeMismatch(); use the corresponding failure helper
(e.g., runWithError or the test harness helper used elsewhere for expecting
validation errors) instead of runWithoutError and set the test's
ExecutionEngineTestCase to expect a validation failure (assert an
error/validation message) for the type-mismatch on the union field; reference
the existing inner cases (the ones named "with relaxation flag, different enum
types work" and "with both relaxation flags, different enum types work"), the
ExecutionEngineTestCase struct, and the relaxFieldSelectionMergingTypeMismatch()
flag so reviewers can locate and mirror the setup but invert the relaxation flag
to confirm strict-mode validation.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78ea8ff and def07c0.

📒 Files selected for processing (9)
  • execution/engine/execution_engine.go
  • execution/engine/execution_engine_test.go
  • v2/pkg/astvalidation/nullability_relaxation_justification.md
  • v2/pkg/astvalidation/operation_rule_field_selection_merging.go
  • v2/pkg/astvalidation/operation_validation.go
  • v2/pkg/astvalidation/operation_validation_test.go
  • v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
  • v2/pkg/engine/plan/configuration.go
  • v2/pkg/engine/plan/datasource_configuration.go

jensneuse and others added 2 commits March 2, 2026 08:24
…atch

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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
v2/pkg/astvalidation/operation_validation.go (1)

114-119: Clarify non-concurrent usage in the setter docs.

SetFieldSelectionMergingConfig mutates validator state used during validation. A short note that this should be called between validations (not concurrently) would make the contract explicit.

Proposed doc tweak
 // 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.
+// OperationValidator is not goroutine-safe; call this between Validate invocations
+// on the same instance.
 func (o *OperationValidator) SetFieldSelectionMergingConfig(config FieldSelectionMergingConfig) {
 	o.fieldSelectionMergingConfig = config
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astvalidation/operation_validation.go` around lines 114 - 119, Update
the comment for OperationValidator.SetFieldSelectionMergingConfig to explicitly
state that it mutates the validator's internal state
(fieldSelectionMergingConfig) and must not be called concurrently with Validate
or other validation operations; note it should be invoked between validations
when reusing a validator (e.g., from a sync.Pool) to change relaxation settings
per call.
v2/pkg/astvalidation/operation_validation_test.go (1)

12564-12566: Assert normalization success in validate(...) before calling Validate.

Right now, normalization errors are silently carried into validation. A direct check after Line 12565 makes failures easier to diagnose.

🧪 Suggested assertion
 		normalizer := astnormalization.NewWithOpts(astnormalization.WithInlineFragmentSpreads())
 		normalizer.NormalizeOperation(&operation, &definition, &validationReport)
+		require.False(t, validationReport.HasErrors(), validationReport.Error())
 		return validator.Validate(&operation, &definition, &validationReport)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astvalidation/operation_validation_test.go` around lines 12564 -
12566, In the validate(...) test helper, after calling
normalizer.NormalizeOperation(&operation, &definition, &validationReport) and
before calling validator.Validate(&operation, &definition, &validationReport),
add an explicit assertion that normalization produced no errors by checking the
validationReport for errors (e.g., call
validationReport.HasErrors()/IsEmpty()/Errors() as appropriate) and fail the
test immediately (using the existing test assertion helper) if any normalization
errors are present so validation failures are not masked by normalization
issues.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (1)

8497-8500: Split config-path vs option-path assertions to avoid masked regressions.

Each subtest currently enables relaxation through both plan.Configuration flags and WithValidationOptions(...) at the same time. That can let one path hide breakage in the other. Consider adding paired cases that exercise each path independently.

Also applies to: 8559-8562, 8636-8641

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go`
around lines 8497 - 8500, The tests currently enable relaxation via both
plan.Configuration (created by makePlanConfig) and
WithValidationOptions(astvalidation.WithRelaxFieldSelectionMergingNullability()),
which can mask regressions; split each affected subtest into paired cases: one
that sets the relaxation flag only on makePlanConfig (e.g., makePlanConfig(t,
true, ...)) with no WithValidationOptions, and a second that leaves the plan
config unrelaxed but passes
WithValidationOptions(astvalidation.WithRelaxFieldSelectionMergingNullability())
to the runner; apply this pattern where the existing tests call
makePlanConfig(...), WithDefaultPostProcessor(), WithValidationOptions(...) (and
similarly at the other occurrences you noted) so each code path is exercised
independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go`:
- Line 8436: There is a gci formatting failure around the Go test configuration
(see the DisableResolveFieldPositions setting in graphql_datasource_test.go);
run the repository formatter/linter (gci and gofmt/gofumpt as used by the repo)
to reformat imports and code, stage and commit the resulting changes so the
DisableResolveFieldPositions line and surrounding imports/formatting conform to
CI rules, then push the commit to clear the lint failure.

---

Nitpick comments:
In `@v2/pkg/astvalidation/operation_validation_test.go`:
- Around line 12564-12566: In the validate(...) test helper, after calling
normalizer.NormalizeOperation(&operation, &definition, &validationReport) and
before calling validator.Validate(&operation, &definition, &validationReport),
add an explicit assertion that normalization produced no errors by checking the
validationReport for errors (e.g., call
validationReport.HasErrors()/IsEmpty()/Errors() as appropriate) and fail the
test immediately (using the existing test assertion helper) if any normalization
errors are present so validation failures are not masked by normalization
issues.

In `@v2/pkg/astvalidation/operation_validation.go`:
- Around line 114-119: Update the comment for
OperationValidator.SetFieldSelectionMergingConfig to explicitly state that it
mutates the validator's internal state (fieldSelectionMergingConfig) and must
not be called concurrently with Validate or other validation operations; note it
should be invoked between validations when reusing a validator (e.g., from a
sync.Pool) to change relaxation settings per call.

In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go`:
- Around line 8497-8500: The tests currently enable relaxation via both
plan.Configuration (created by makePlanConfig) and
WithValidationOptions(astvalidation.WithRelaxFieldSelectionMergingNullability()),
which can mask regressions; split each affected subtest into paired cases: one
that sets the relaxation flag only on makePlanConfig (e.g., makePlanConfig(t,
true, ...)) with no WithValidationOptions, and a second that leaves the plan
config unrelaxed but passes
WithValidationOptions(astvalidation.WithRelaxFieldSelectionMergingNullability())
to the runner; apply this pattern where the existing tests call
makePlanConfig(...), WithDefaultPostProcessor(), WithValidationOptions(...) (and
similarly at the other occurrences you noted) so each code path is exercised
independently.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between def07c0 and 6ddec1a.

📒 Files selected for processing (6)
  • execution/engine/execution_engine_test.go
  • v2/pkg/astvalidation/operation_rule_field_selection_merging.go
  • v2/pkg/astvalidation/operation_validation.go
  • v2/pkg/astvalidation/operation_validation_test.go
  • v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
  • v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • execution/engine/execution_engine_test.go

jensneuse and others added 3 commits March 2, 2026 08:59
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
v2/pkg/astvalidation/operation_validation_test.go (1)

1133-1143: ⚠️ Potential issue | 🟡 Minor

“Enum types” test name does not match what is asserted.

At Line 1133, the test says “enum types,” but Line 1137-Line 1138 assert priority (Int vs String). This is misleading and overlaps with the base-type scenario.

🔧 Proposed fix (make this test truly enum-based)
 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 }
+				searchable {
+					... on Issue { state }
+					... on PullRequestReview { state }
 				}
 			}`, FieldSelectionMerging(), Invalid,
-		withValidationErrors(`fields 'priority' conflict because they return conflicting types 'Int' and 'String'`))
+		withValidationErrors(`fields 'state' conflict because they return conflicting types 'IssueState' and 'PullRequestReviewState'`))
 })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astvalidation/operation_validation_test.go` around lines 1133 - 1143,
Rename or update the test so its description matches the asserted types: either
change the test name string in the t.Run call (currently "rejects differing enum
types on union members without type mismatch flag") to reflect base-type
mismatch (e.g., "rejects differing base types on union members without type
mismatch flag") or change the asserted types to actual enum types; locate the
failing test via runWithDefinition, typeMismatchDefinition,
FieldSelectionMerging, Invalid and withValidationErrors and update the t.Run
description (or the test input/assertion) so the name and the asserted conflict
('priority' returning 'Int' and 'String') are consistent.
🧹 Nitpick comments (2)
execution/engine/execution_engine_test.go (1)

7794-7888: Consider adding a relaxed scalar-mismatch success case for parity.

You already assert strict scalar rejection (Lines 7794-7823) and relaxed enum success, but a scalar success-path with relaxFieldSelectionMergingTypeMismatch() would make this block fully symmetric and harder to regress.

Suggested test addition
+		t.Run("with relaxation flag, different scalar types work", runWithoutError(
+			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,
+				expectedResponse: `{"data":{"entity":{"priority":1}}}`,
+			},
+			relaxFieldSelectionMergingTypeMismatch(),
+		))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@execution/engine/execution_engine_test.go` around lines 7794 - 7888, Add a
symmetric success case for relaxed scalar mismatches by copying the existing
failing test that uses
scalarMismatchParsedSchema/scalarMismatchRootNodes/scalarMismatchConfig/scalarMismatchFieldConfig
but change the runner to runWithoutError, provide an expectedResponse matching
the scalar value (e.g., the JSON returned by the data source), and pass
relaxFieldSelectionMergingTypeMismatch() as the relaxation option; use the same
dataSource factory/mustGraphqlDataSourceConfiguration block as in the failing
case so the test mirrors the strict-reject case but asserts success under the
relaxation.
v2/pkg/astvalidation/operation_validation.go (1)

114-124: Document non-concurrent usage for runtime config updates.

Please explicitly state that SetFieldSelectionMergingConfig must not be called concurrently with Validate on the same validator instance.

Suggested doc-only patch
 // 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.
+// This method must not be called concurrently with Validate on the same validator instance.
 func (o *OperationValidator) SetFieldSelectionMergingConfig(config FieldSelectionMergingConfig) {
 	o.fieldSelectionMergingConfig = config
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astvalidation/operation_validation.go` around lines 114 - 124, Add
explicit documentation to SetFieldSelectionMergingConfig on OperationValidator
stating that it is not safe to call concurrently with Validate on the same
validator instance; update the comment above the SetFieldSelectionMergingConfig
method (and optionally OperationValidator type doc) to note that runtime config
updates are non-concurrent and callers must synchronize (e.g., via external
locking) when calling SetFieldSelectionMergingConfig while any goroutine may
call Validate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md`:
- Around line 145-180: Update the earlier assertion that "base named types must
match" to explicitly state it applies only to the RelaxNullabilityCheck rule;
clarify that RelaxTypeMismatchCheck is a broader relaxation that permits
differing base named types across non-overlapping concrete object types.
Reference the symbols RelaxNullabilityCheck and RelaxTypeMismatchCheck so
readers understand the scope difference and ensure the sentence near the initial
nullability discussion is qualified to avoid implying the restriction applies to
both checks.

---

Duplicate comments:
In `@v2/pkg/astvalidation/operation_validation_test.go`:
- Around line 1133-1143: Rename or update the test so its description matches
the asserted types: either change the test name string in the t.Run call
(currently "rejects differing enum types on union members without type mismatch
flag") to reflect base-type mismatch (e.g., "rejects differing base types on
union members without type mismatch flag") or change the asserted types to
actual enum types; locate the failing test via runWithDefinition,
typeMismatchDefinition, FieldSelectionMerging, Invalid and withValidationErrors
and update the t.Run description (or the test input/assertion) so the name and
the asserted conflict ('priority' returning 'Int' and 'String') are consistent.

---

Nitpick comments:
In `@execution/engine/execution_engine_test.go`:
- Around line 7794-7888: Add a symmetric success case for relaxed scalar
mismatches by copying the existing failing test that uses
scalarMismatchParsedSchema/scalarMismatchRootNodes/scalarMismatchConfig/scalarMismatchFieldConfig
but change the runner to runWithoutError, provide an expectedResponse matching
the scalar value (e.g., the JSON returned by the data source), and pass
relaxFieldSelectionMergingTypeMismatch() as the relaxation option; use the same
dataSource factory/mustGraphqlDataSourceConfiguration block as in the failing
case so the test mirrors the strict-reject case but asserts success under the
relaxation.

In `@v2/pkg/astvalidation/operation_validation.go`:
- Around line 114-124: Add explicit documentation to
SetFieldSelectionMergingConfig on OperationValidator stating that it is not safe
to call concurrently with Validate on the same validator instance; update the
comment above the SetFieldSelectionMergingConfig method (and optionally
OperationValidator type doc) to note that runtime config updates are
non-concurrent and callers must synchronize (e.g., via external locking) when
calling SetFieldSelectionMergingConfig while any goroutine may call Validate.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8fe751c and fb1be32.

📒 Files selected for processing (4)
  • execution/engine/execution_engine_test.go
  • v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md
  • v2/pkg/astvalidation/operation_validation.go
  • v2/pkg/astvalidation/operation_validation_test.go

- 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 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant