add regression test for missing field when merging concrete type root field and interface fragment #1346
Conversation
… field and interface fragment
WalkthroughThis change adds test infrastructure for federation scenarios involving concrete type merging within root fields and interface fragments. New schema types (Base interface, Category, Owner, ProductA, ProductB), a corresponding resolver, generated models, a GraphQL query, and an integration test case are introduced to exercise this federation behavior. Changes
Sequence DiagramsequenceDiagram
participant Test as Integration Test
participant Gateway
participant QueryResolver
participant Categories as Category/Owner Data
Test->>Gateway: Execute GraphQL query<br/>productA { category { id owner { name } } }
Gateway->>QueryResolver: Resolve productA (root field)
QueryResolver->>Categories: Return ProductA with Category
Gateway->>Categories: Resolve Base interface fragment<br/>on concrete ProductA type
Gateway->>Categories: Extract category.id and owner.name<br/>from concrete type
Gateway-->>Test: Return merged concrete type<br/>within interface context
Note over Gateway: Merges concrete type selection<br/>with interface fragment selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
execution/engine/federation_integration_test.go (1)
573-583: New regression subtest is consistent and correctly wiredThe subtest setup, query path, and expected JSON all align with the new schema, resolver, and query (ProductA → Category c111 → Owner name "owner" under alias
displayOwner). It matches the structure of existing federation integration tests and should reliably capture the regression once the engine is fixed.If you want to make the intent clearer for future readers, consider adding a short comment above this subtest referencing cosmo issue 2346 and noting that it currently fails as a regression guard.
execution/federationtesting/accounts/graph/schema.graphqls (1)
14-15: Schema additions line up with resolvers, models, and query usageThe new
productAfield,Baseinterface, andCategory/Owner/ProductA/ProductBtypes are coherent:
ProductA implements Basewithcategory: Categorysupports the... on Baseand nested concrete fragments used in the new query.- Nullability and field names match the Go models and the
ProductAresolver.- Including
ProductBas anotherBaseimplementation is a good way to mirror the multi‑concrete‑type scenario behind the regression.No schema-level issues from a federation/testing perspective.
If you expect
Category.ownerto always be present in this test context, you could tighten it toOwner!in a follow‑up, but it’s not required for this regression.Also applies to: 185-205
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
execution/federationtesting/accounts/graph/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (5)
execution/engine/federation_integration_test.go(1 hunks)execution/federationtesting/accounts/graph/model/models_gen.go(3 hunks)execution/federationtesting/accounts/graph/schema.graphqls(2 hunks)execution/federationtesting/accounts/graph/schema.resolvers.go(1 hunks)execution/federationtesting/testdata/queries/merge_concrete_type_in_root_field_and_interface_fragment.graphql(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
execution/federationtesting/accounts/graph/schema.resolvers.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
execution/engine/federation_integration_test.go
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
execution/engine/federation_integration_test.go
🧬 Code graph analysis (1)
execution/federationtesting/accounts/graph/schema.resolvers.go (1)
execution/federationtesting/accounts/graph/model/models_gen.go (5)
ProductA(166-169)ProductA(171-171)Category(128-131)Owner(156-158)Name(49-52)
🔇 Additional comments (3)
execution/federationtesting/testdata/queries/merge_concrete_type_in_root_field_and_interface_fragment.graphql (1)
1-27: Query shape correctly matches schema and expected JSONThe query and
ProductDetailfragment correctly model the scenario:productAas a concrete root field, refined via an interface fragment (Base) and nested concrete type fragments (ProductA/ProductB). The aliasdisplayOwner: ownerlines up with the expected responsecategory.displayOwner.namein the integration test, so this is well‑aligned with the regression you want to capture.execution/federationtesting/accounts/graph/schema.resolvers.go (1)
214-225: ProductA resolver is consistent with schema and testsThe
ProductAresolver correctly constructs the nestedProductA → Category → Ownergraph and matches the IDs/names asserted in the federation integration test. It follows the existing pattern of static data resolvers in this file.execution/federationtesting/accounts/graph/model/models_gen.go (1)
21-24: Generated model changes correctly reflect the new schemaThe added
Baseinterface,Category/Ownerstructs, andProductA/ProductBtypes (with theirIsBaseandGetCategoryimplementations) are consistent with the GraphQL schema and the new resolver. This is standard gqlgen output and looks correct; no manual changes are needed here.Also applies to: 128-132, 156-158, 166-181
1 participant
Summary by CodeRabbit
Tests
New Features
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist
This adds a federation integration test that reproduces a missing field in the response when:
querying a concrete type in the root field, and
using an interface fragment with concrete type fragments inside.
The new test merge_concrete_type_in_root_field_and_interface_fragment expects displayOwner to be present under productA.category, but currently the field is missing in the response.
This is related to cosmo issue wundergraph/cosmo#2346.
Additional findings
I further investigated this on the router side and confirmed:
displayOwnerfield.displayOwnerfield with the expected value.displayOwner.This strongly suggests that:
resolveexecution), where the subgraph JSON is mapped into the final GraphQL response.Response plan structure (resolve tree)
I instrumented
ExecutionEngine.Executeto dumpp.Response.Data(theresolve.Objecttree) right before callingResolveGraphQLResponse.For the failing test (
merge_concrete_type_in_root_field_and_interface_fragment), the tree forproductA.categorylooks like this (base64-decodedNamefields shown for clarity):{ "Data": { "productA": { "category": { "id": { ... }, "displayOwner": { "name": { ... } } } } } }More precisely, the
Fieldstructure undercategoryis:Name = "id", Value.Path = ["id"]Name = "displayOwner", Value.Path = ["displayOwner"]child field: Name = "name", Value.Path = ["name"]So at the plan/postprocess stage, the resolve tree correctly contains a
displayOwnerfield with a nestednamefield.Type condition metadata on
displayOwnerLooking at the raw JSON dump of
p.Response.Data, the relevant part of theFieldmetadata is:For
category.displayOwner:{ "Name": "displayOwner", "Value": { "Path": ["displayOwner"], "Fields": [ { "Name": "name", "Value": { "Path": ["name"], "Nullable": false }, "ParentOnTypeNames": [ { "Depth": 2, "Names": ["ProductB", "ProductA"] } ], "Info": { "Name": "name", "ExactParentTypeName": "Owner", "ParentTypeNames": ["Owner"], "NamedType": "String" } } ] }, "ParentOnTypeNames": [ { "Depth": 1, "Names": ["ProductB"] } ], "Info": { "Name": "owner", "ExactParentTypeName": "Category", "ParentTypeNames": ["Category"], "NamedType": "Owner" } }namefield hasParentOnTypeNames.Names = ["ProductB", "ProductA"]atDepth = 2, which matches the fragment:... on ProductB { category { displayOwner: owner { name } } }... on ProductA { category { displayOwner: owner { name } } }displayOwnerfield hasParentOnTypeNames.Names = ["ProductB"]atDepth = 1.displayOwneras if it were only valid onProductB,nameis marked as valid on bothProductAandProductB.Since the concrete runtime type for
productAisProductA, this likely causes the router to skip resolvingdisplayOwnerforProductAaltogether due to the type condition metadata, even though the subgraph response contains the expectedownervalue.Suspected area
Given the above:
displayOwneris there).displayOwneras expected.displayOwner.It seems likely that the bug is in the type-condition handling for abstract types in the v2 engine, rather than in planning or subgraph I/O:
Either when building
OnTypeNames/ParentOnTypeNamesfor fields under fragments like:... on Base { ... on ProductA { ... } ... on ProductB { ... } }Or when evaluating those type conditions at resolve time to decide whether a field should be resolved for a particular concrete type.
In this specific case, it looks like the type-condition metadata for
displayOwnerwas narrowed down toProductBonly, even though the query selects it for bothProductAandProductB. This causesdisplayOwnerto be dropped forProductAat execution time.