feat: improve abstract field validation - #1588
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe execution planner now requests ChangesAbstract type validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Planner
participant Subgraph
participant Resolvable
Client->>Planner: submit abstract field selection
Planner->>Subgraph: request fields with __typename
Subgraph-->>Resolvable: return abstract value
Resolvable->>Resolvable: validate runtime typename
Resolvable-->>Client: return data, nulls, or INVALID_GRAPHQL errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
endigma
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/plan/visitor.go (1)
811-848: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle abstract types with no accessible members. When every interface implementer or union member is
@inaccessible,PossibleTypesis empty, soisAbstract()returns false and the__typenamevalidation path is skipped entirely. Treat that zero-accessible-members case as abstract too, and add a regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/visitor.go` around lines 811 - 848, Update the abstract-type construction and isAbstract logic so interfaces or unions with only inaccessible implementers/members are still classified as abstract when PossibleTypes is empty but InaccessibleTypes is populated. Ensure __typename validation runs for this zero-accessible-members case, and add a regression test covering an interface or union whose every member is `@inaccessible`.
🧹 Nitpick comments (1)
execution/engine/execution_engine_cost_test.go (1)
295-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
expectedResponseassertion for the rejection behavior this test targets.This test verifies cost (
expectedActualCost: 0) but never asserts that the response actually contains theINVALID_GRAPHQLrejection/null for the missing__typename, unlike the analogous tests at Lines 782-794 and 7505-7513 in this same file. As written, a cost-side regression that still yields 0 for an unrelated reason (e.antml:cite the field simply carrying no weight) would pass this test undetected.♻️ Suggested addition
expectedEstimatedCost: intPtr(13), // Query.hero(13) // the abstract hero value is rejected and nulled, so nothing is billed expectedActualCost: intPtr(0), +expectedResponse: `{"errors":[{"message":"Subgraph 'id' returned an invalid value for __typename field.","path":["hero"],"extensions":{"code":"INVALID_GRAPHQL"}}],"data":{"hero":null}}`,(Adjust the exact message/path to match actual engine output.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/engine/execution_engine_cost_test.go` around lines 295 - 332, Add an expectedResponse assertion to the “single abstract field without __typename is rejected and bills nothing” test, verifying that hero is null and the response includes the INVALID_GRAPHQL rejection with the correct message/path for the missing __typename. Match the established response shape and exact engine output used by the analogous tests, while preserving the existing cost assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@execution/engine/abstract_type_validation_test.go`:
- Around line 13-14: Suppress the tparallel lint warning for the affected
subtests in TestAbstractTypeValidation, including the cases around the
additional reported range. Place a targeted tparallel nolint directive on the
relevant t.Run calls or closures, and do not add another t.Parallel() call
because runWithoutError/runExecutionTest already invokes it.
---
Outside diff comments:
In `@v2/pkg/engine/plan/visitor.go`:
- Around line 811-848: Update the abstract-type construction and isAbstract
logic so interfaces or unions with only inaccessible implementers/members are
still classified as abstract when PossibleTypes is empty but InaccessibleTypes
is populated. Ensure __typename validation runs for this zero-accessible-members
case, and add a regression test covering an interface or union whose every
member is `@inaccessible`.
---
Nitpick comments:
In `@execution/engine/execution_engine_cost_test.go`:
- Around line 295-332: Add an expectedResponse assertion to the “single abstract
field without __typename is rejected and bills nothing” test, verifying that
hero is null and the response includes the INVALID_GRAPHQL rejection with the
correct message/path for the missing __typename. Match the established response
shape and exact engine output used by the analogous tests, while preserving the
existing cost assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ccfe3b3-5d11-425f-a42f-31b8850be6e6
📒 Files selected for processing (12)
execution/engine/abstract_type_validation_test.goexecution/engine/execution_engine_cost_test.goexecution/engine/execution_engine_helpers_test.goexecution/engine/execution_engine_test.goexecution/engine/testdata/complex_nesting_query_with_art.jsonv2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_interface_provides_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/resolve/node_object.gov2/pkg/engine/resolve/resolvable.go
29e94e5 to
c8b4bce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@execution/engine/abstract_type_validation_test.go`:
- Around line 247-310: Move the test client construction involving
testNetHttpClient and roundTripperTestCase into the t.Run callback so it
receives the subtest’s t, ensuring request-body assertions target the correct
subtest. Update the runner or ExecutionEngineTestCase setup around mustFactory
and mustGraphqlDataSourceConfigurationWithName while preserving the existing
test configuration and expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84049401-2a11-4536-980c-7bed803fb820
📒 Files selected for processing (1)
execution/engine/abstract_type_validation_test.go
causes issues, refactor to fix is probably not worth it
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
76f190f to
9fd2746
Compare
|
LGTM |
🤖 I have created a release *beep* *boop* --- ## [2.13.0](v2.12.1...v2.13.0) (2026-07-17) ### Features * improve abstract field validation ([#1588](#1588)) ([d6cceac](d6cceac)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
2 participants
Also incorporates a fix to one of the execution test runners, we cannot run
t.Parallel()inside because the whole thing is slightly flawed where the wrongtis captured formust…(t, …)type invocations on fields of the TC struct.This is normally not an issue as the
must…helpers are usually just ceremony for dropping the error value, but when we might actually expect them to fail they will panic by callingrequire.XXX(t, …)with the parenttinstead of the subtestt.This can be resolved by in these cases evaluating it and all of its arguments within the subtest closure
func(t) { … }but then thetparallellinter complains about missing parallel, which cannot be fixed because the inner function also tried to call it.The proper way to fix this would be to have
dataSourcesetc fields on the TC be evaluated with subtesttvia likedataSources: func (t testing.T) []*DataSourceinstead of directly providing it, but this would be a very intrusive change.