feat: add field selection merging type mismatch relaxation#1412
feat: add field selection merging type mismatch relaxation#1412
Conversation
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>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis 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 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md
Show resolved
Hide resolved
There was a problem hiding this comment.
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
📒 Files selected for processing (9)
execution/engine/execution_engine.goexecution/engine/execution_engine_test.gov2/pkg/astvalidation/nullability_relaxation_justification.mdv2/pkg/astvalidation/operation_rule_field_selection_merging.gov2/pkg/astvalidation/operation_validation.gov2/pkg/astvalidation/operation_validation_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/plan/configuration.gov2/pkg/engine/plan/datasource_configuration.go
…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>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
v2/pkg/astvalidation/operation_validation.go (1)
114-119: Clarify non-concurrent usage in the setter docs.
SetFieldSelectionMergingConfigmutates 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 invalidate(...)before callingValidate.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.Configurationflags andWithValidationOptions(...)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
📒 Files selected for processing (6)
execution/engine/execution_engine_test.gov2/pkg/astvalidation/operation_rule_field_selection_merging.gov2/pkg/astvalidation/operation_validation.gov2/pkg/astvalidation/operation_validation_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/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
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
Outdated
Show resolved
Hide resolved
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>
There was a problem hiding this comment.
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(IntvsString). 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
SetFieldSelectionMergingConfigmust not be called concurrently withValidateon 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
📒 Files selected for processing (4)
execution/engine/execution_engine_test.gov2/pkg/astvalidation/field_selection_merging_relaxation_justification.mdv2/pkg/astvalidation/operation_validation.gov2/pkg/astvalidation/operation_validation_test.go
v2/pkg/astvalidation/field_selection_merging_relaxation_justification.md
Show resolved
Hide resolved
- 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>
Summary
RelaxTypeMismatchCheckflag to field selection merging validation that allows completely different types (e.g.IssueStatevsPullRequestReviewState) 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 typescheckTypeMismatchhelper method with a clear three-outcome result type (typeMismatchAccept,typeMismatchSkip,typeMismatchReject)sync.Onceinto async.Mapkeyed by flag combinationTest plan
astvalidationtests pass — no regressions on strict-mode validation🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests