Skip to content

fix: incorrect type detection for abstract types - #1613

Merged
Noroth merged 3 commits into
masterfrom
ludwig/router-604-graphql-router-typename-not-detected-from-grpc-oneof
Jul 31, 2026
Merged

fix: incorrect type detection for abstract types#1613
Noroth merged 3 commits into
masterfrom
ludwig/router-604-graphql-router-typename-not-detected-from-grpc-oneof

Conversation

@Noroth

@Noroth Noroth commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Abstract types for required fields were missing some type configuration in the planner step. This PR adds the missing config and provides test cases

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.

Open Source AI Manifesto

This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.

Summary by CodeRabbit

  • New Features
    • Added federation @requires support when the required field returns abstract GraphQL types (interfaces/unions), including nested and list/nullable scenarios.
    • Added new Storage fields for recommended items and latest operations (including optional latest operation), with corresponding gRPC-backed responses.
  • Bug Fixes
    • Improved execution of composite selections for @requires, ensuring only selected abstract members contribute to results.
  • Tests
    • Added end-to-end and planner-level test coverage for abstract return types, nested inline fragments, and composite @requires behavior.

@Noroth
Noroth requested a review from a team as a code owner July 30, 2026 09:29

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7a84e8c8-e5ec-4d28-97b7-10f4420cad74

📥 Commits

Reviewing files that changed from the base of the PR and between 61f2d2e and 8b8e91d.

📒 Files selected for processing (1)
  • execution/engine/execution_engine_grpc_requires_test.go

📝 Walkthrough

Walkthrough

Adds support for federated @requires fields returning interfaces and unions, including nested and nullable cases. It updates composite selection planning, required-field RPC contracts, mappings, mock responses, federation metadata, and execution-plan and datasource tests.

Changes

Abstract @requires return types

Layer / File(s) Summary
Schema and RPC contracts
v2/pkg/grpctest/testdata/products.graphqls, v2/pkg/grpctest/product.proto, v2/pkg/grpctest/productv1/productv1connect/*
Adds four abstract-return Storage fields and the protobuf and Connect RPC definitions required to resolve them.
Required-field mappings and mock responses
v2/pkg/grpctest/mapping/mapping.go, v2/pkg/grpctest/schema.go, v2/pkg/grpctest/mockservice_requires.go, v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go
Maps the new fields to RPC lookups, exposes their required selections, and returns concrete interface or union members from the mock service.
Composite selection planning
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go, v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor*.go
Builds composite selection metadata as a value and includes one-of and fragment information when constructing federation required-field messages.
Execution-plan and datasource coverage
v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go, v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go, execution/engine/execution_engine_grpc_requires_test.go
Tests interface, union, nested, list, nullable, and concrete-member selection behavior in generated plans, _entities responses, and full gRPC subgraph execution.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GraphQLQuery
  participant NewDataSource
  participant RPCPlanVisitor
  participant ProductService
  participant MockService
  GraphQLQuery->>NewDataSource: load _entities query
  NewDataSource->>RPCPlanVisitor: build `@requires` execution plan
  RPCPlanVisitor->>ProductService: call abstract-return required-field RPC
  ProductService->>MockService: resolve Storage field by id
  MockService-->>ProductService: return interface or union value
  ProductService-->>NewDataSource: return protobuf response
  NewDataSource-->>GraphQLQuery: return concrete __typename and selected fields
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the core fix around abstract-type handling, even though the PR also adds related gRPC schema and test coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ludwig/router-604-graphql-router-typename-not-detected-from-grpc-oneof

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go (1)

3592-3663: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider hoisting the shared expectation helpers out of the test functions.

storageEntityLookupCall and storageKeyMessage are byte-for-byte copies of the closures in TestExecutionPlan_FederationRequires_AbstractTypes (lines 2664-2734) in this same file. Extracting them to package-level helpers keeps the two suites from drifting when the lookup call shape changes.

🤖 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/datasource/grpc_datasource/execution_plan_requires_test.go`
around lines 3592 - 3663, Extract the duplicated storage lookup builders from
the current test scope into package-level helpers, such as shared functions for
the RPCCall and RPCMessage, and update both
TestExecutionPlan_FederationRequires_AbstractTypes and the current test to use
them. Remove the local storageEntityLookupCall and storageKeyMessage closures
while preserving their existing returned structures.
🤖 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.

Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go`:
- Around line 3592-3663: Extract the duplicated storage lookup builders from the
current test scope into package-level helpers, such as shared functions for the
RPCCall and RPCMessage, and update both
TestExecutionPlan_FederationRequires_AbstractTypes and the current test to use
them. Remove the local storageEntityLookupCall and storageKeyMessage closures
while preserving their existing returned structures.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 33a28d12-bcaa-49d0-9614-cd091a565d5f

📥 Commits

Reviewing files that changed from the base of the PR and between 826c838 and 61f2d2e.

⛔ Files ignored due to path filters (2)
  • v2/pkg/grpctest/productv1/product.pb.go is excluded by !**/*.pb.go
  • v2/pkg/grpctest/productv1/product_grpc.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (12)
  • v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
  • v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go
  • v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
  • v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
  • v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go
  • v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go
  • v2/pkg/grpctest/mapping/mapping.go
  • v2/pkg/grpctest/mockservice_requires.go
  • v2/pkg/grpctest/product.proto
  • v2/pkg/grpctest/productv1/productv1connect/product.connect.go
  • v2/pkg/grpctest/schema.go
  • v2/pkg/grpctest/testdata/products.graphqls

@Noroth
Noroth merged commit e249e88 into master Jul 31, 2026
11 checks passed
@Noroth
Noroth deleted the ludwig/router-604-graphql-router-typename-not-detected-from-grpc-oneof branch July 31, 2026 11:18
Noroth pushed a commit that referenced this pull request Jul 31, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.14.2](v2.14.1...v2.14.2)
(2026-07-31)


### Bug Fixes

* incorrect type detection for abstract types
([#1613](#1613))
([e249e88](e249e88))

---
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>
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.

3 participants