Conversation
…s, when concrete types has all fields local
devsergiy
requested review from
Noroth,
StarpTech,
jensneuse and
ysmolski
as code owners
Contributor
📝 WalkthroughWalkthroughThe pull request enhances the GraphQL federation planner with refined interface member type handling (adding interface type name to return values), improved node selection logic in the datasource filter visitor, updated debug output in the path builder, and expanded federation test coverage for multi-subgraph federation scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
Comment |
devsergiy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
645-652: Fix CI:golangci-lintfailsgciformatting on this file.CI is red (gci). Please run the repo’s standard formatter (
gci/golangci-lint --fix/goimportsper project conventions) onv2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.goand commit the result.
🤖 Fix all issues with AI agents
In
`@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go`:
- Around line 20032-20034: Remove the placeholder skip marker used via
WithSkipReason("fix me") in the failing test chain; either fix the test's
expected plan so the scenario passes (update the expectation used alongside
WithDefaultPostProcessor() to the correct plan/assertion) or replace the
merge-time skip with an explicit runtime skip (call t.Skip("tracking:
<issue-number-or-link>") at the start of the test) and include a tracking issue
link; locate the usage of WithSkipReason in the
graphql_datasource_federation_test (the test that calls
WithDefaultPostProcessor() followed by WithSkipReason("fix me")) and apply one
of these two fixes.
- Around line 19419-19469: firstSubgraphSDL contains a duplicated type
definition for Title1 and omits Title2 while firstDatasourceConfiguration's
ChildNodes list references Title2; update firstSubgraphSDL to remove the
duplicate Title1 definition and add a proper Title2 type (matching the NodeTitle
interface shape) so the SDL matches the ChildNodes in
firstDatasourceConfiguration and the FederationMetaData expectations.
- Around line 19042-19102: The federation key in thirdDatasourceConfiguration
mismatches the SDL: thirdSubgraphSDL defines type Entity2 but the
plan.FederationMetaData Keys entry has TypeName: "Entity"; fix by either
updating the Keys entry to TypeName: "Entity2" (so the key matches
thirdSubgraphSDL) or remove the Keys entry entirely if this datasource is
intended to be unreachable; edit the Keys slice in thirdDatasourceConfiguration
accordingly.
In `@v2/pkg/engine/plan/abstract_selection_rewriter_test.go`:
- Around line 4124-4128: The dsBuilder in this test registers
RootNode("Account", "id", "title") but the test schema defines no Account type;
update the dsBuilder to register RootNode("Admin", "id", "title") (i.e., replace
"Account" with "Admin") so the registered root node matches the test's
definition/upstreamDefinition and the rest of the test (e.g., fieldName "user"
and ChildNode("Named", "id")) remains consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.gov2/pkg/engine/plan/abstract_selection_rewriter.gov2/pkg/engine/plan/abstract_selection_rewriter_helpers.gov2/pkg/engine/plan/abstract_selection_rewriter_test.gov2/pkg/engine/plan/datasource_filter_visitor.gov2/pkg/engine/plan/path_builder.gov2/pkg/engine/plan/path_builder_visitor.go
💤 Files with no reviewable changes (1)
- v2/pkg/engine/plan/path_builder_visitor.go
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
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-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/plan/abstract_selection_rewriter_test.gov2/pkg/engine/plan/abstract_selection_rewriter_helpers.gov2/pkg/engine/plan/abstract_selection_rewriter.gov2/pkg/engine/plan/datasource_filter_visitor.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_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:
v2/pkg/engine/plan/abstract_selection_rewriter_test.gov2/pkg/engine/plan/abstract_selection_rewriter.gov2/pkg/engine/plan/datasource_filter_visitor.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/plan/abstract_selection_rewriter_test.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/plan/datasource_filter_visitor.go
📚 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:
v2/pkg/engine/plan/datasource_filter_visitor.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-08-25T14:44:39.420Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1283
File: v2/pkg/grpctest/product.proto:14-15
Timestamp: 2025-08-25T14:44:39.420Z
Learning: In the gRPC test mapping system in v2/pkg/grpctest/mapping/mapping.go, entity lookup RPCs (like LookupWarehouseById) belong in EntityRPCs mapping but should NOT be added to QueryRPCs. Entity RPCs and Query RPCs serve different purposes in the mapping architecture.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
🧬 Code graph analysis (2)
v2/pkg/engine/plan/datasource_filter_visitor.go (1)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (1)
IsMutationOrQueryRootType(552-554)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (3)
v2/pkg/engine/plan/configuration.go (2)
DebugConfiguration(51-64)Configuration(9-49)v2/pkg/engine/datasource/graphql_datasource/configuration.go (3)
FetchConfiguration(132-136)FederationConfiguration(138-141)Configuration(26-33)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
DefaultPostProcessingConfiguration(40-43)
🪛 GitHub Actions: v2-ci
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
[error] 645-645: golangci-lint: File is not properly formatted (gci).
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
[failure] 645-645:
File is not properly formatted (gci)
🔇 Additional comments (19)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (5)
1312-1312: LGTM!Adding the
Nodeinterface as a child node ensures the datasource correctly advertises the interface fields, which aligns with the test's expectation that no rewriting should occur when all fields are local.
1361-1361: LGTM!Consistent with the previous change - registering the
Nodeinterface fields in the datasource configuration.
1448-1448: LGTM!This addition ensures the interface is properly registered, matching the test's expectation that rewriting should not occur.
1486-1486: LGTM!Consistent registration of the
Nodeinterface fields.
4081-4149: Good test coverage for the interface field rewrite edge case.This test validates that when an interface doesn't have all fields available in a datasource (upstream
Namedonly hasid, notname), the rewriter correctly transforms interface field selections into concrete type fragments. The test structure is clear and the expected output correctly shows the fragment spreading.v2/pkg/engine/plan/path_builder.go (4)
74-76: LGTM!Clearer debug message format with consistent naming convention.
92-94: LGTM!Consistent with the initial run message format.
137-150: LGTM!The enhanced debug output provides better visibility into the path builder's state:
- Simplified "Will revisit" indicator
- Unconditional missing paths output
- New detailed section for fields waiting on dependencies with field reference and datasource hash
This will help with debugging complex federation planning scenarios.
152-154: LGTM!Adding a header improves readability of the debug output.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
18527-18527: Test group rename looks good.v2/pkg/engine/plan/datasource_filter_visitor.go (6)
445-456: LGTM!The skip logic correctly ensures that root nodes which cannot provide child fields are not unnecessarily selected, improving the selection accuracy for complex federation scenarios.
460-467: LGTM!Correctly skips leaf nodes from child evaluation since they have no children to select.
469-481: LGTM!The sibling selection logic is well-structured:
- Non-leaf nodes that can't provide children are correctly skipped.
- The
__typenamefield is appropriately handled, allowing selection only on root query types where it makes sense.
483-538: LGTM!The enhanced logic for root node selection properly handles:
- Parent key verification for non-root-query nodes.
- Non-leaf nodes that serve as connection points for key jumping between datasources.
- Edge case where only
__typenameis selected as a child field.
648-673: Verify the external parent check uses the correct node index.On line 653, the condition checks
!f.nodes.items[i].IsProvidedwhereiis the original input node, notparentIdx. This appears intentional (checking if the target node is provided when encountering an external parent), but please verify this matches the intended behavior.If the intent is to check whether the parent itself is provided, this should use
parentIdx:- if f.nodes.items[parentIdx].IsExternal && !f.nodes.items[i].IsProvided { + if f.nodes.items[parentIdx].IsExternal && !f.nodes.items[parentIdx].IsProvided {
895-925: LGTM! Recursive evaluation improves accuracy.The restructured logic correctly:
- Identifies leaf nodes as directly selectable.
- Recursively evaluates non-leaf nodes to determine if they can eventually provide selectable child fields.
- Avoids false negatives where intermediate nodes have no local fields but their descendants do.
The tree structure of
childNodesOnSameSourceshould naturally prevent infinite recursion.v2/pkg/engine/plan/abstract_selection_rewriter.go (2)
409-421: LGTM!Correctly captures and propagates the new
interfaceTypeNamereturn value to enable downstream field locality checks.
447-482: Well-documented fix for interface field locality.The new pre-check correctly handles scenarios where:
- Fields are selected directly on an interface type.
- The interface definition in the current datasource doesn't include all selected fields (even if concrete types do).
The detailed example in the comments clearly explains the edge case being addressed. This is a solid fix for ENG-8697.
v2/pkg/engine/plan/abstract_selection_rewriter_helpers.go (1)
537-605: LGTM! Consistent propagation of interface type name.The new first return value
interfaceTypeNameis correctly set:
- Empty string for error cases and when dealing with concrete types or interface objects.
- The actual interface name when the field returns an interface type that needs field locality checking.
This enables callers to verify that selected fields exist on the interface definition itself, not just on implementing types.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Contributor
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/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
3-11: Fixgciimport formatting (CI failure).
Linters report the file isn’tgci-formatted; this is typically import grouping/order (notably the dot import). Please re-run the repo’sgci/format target or adjust imports accordingly.Proposed import ordering tweak (if you want a manual fix)
import ( "testing" "github.com/wundergraph/graphql-go-tools/v2/pkg/ast" - . "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/datasourcetesting" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/plan" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/postprocess" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/resolve" + + . "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/datasourcetesting" )
🤖 Fix all issues with AI agents
In
`@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go`:
- Around line 19419-19439: The SDL assigned to firstSubgraphSDL references
TitleValue via NodeTitle.title but never defines the TitleValue type, which will
cause schema parse errors; update the firstSubgraphSDL string to include a
definition for TitleValue (e.g., scalar or object/enum) so NodeTitle.title:
TitleValue! resolves, ensuring the subgraph schema parses correctly and tests
run.
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
20032-20034: RemoveWithSkipReason("fix me")before merge.
This is still a placeholder skip marker in the assertion chain.Suggested direction
- Either fix the expected plan so it passes, or
- Replace with an explicit
t.Skip("tracking: <issue-link>")at the start of the subtest.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
18885-19373: New “nested selections of shared nodes” scenarios look solid; consider making the “unreachable” subgraph future-proof.
The Entity2 alignment looks consistent now, and the test intent (don’t select shared nested nodes based on the wrong parent/subgraph) is clear. One optional hardening: ifEntity2ever becomes reachable in the future, this test might stop exercising the intended edge; consider explicitly making it non-resolvable (or otherwise unreachable by config) to keep the scenario stable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.gov2/pkg/engine/plan/abstract_selection_rewriter_test.gov2/pkg/engine/plan/datasource_filter_visitor_test.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/plan/abstract_selection_rewriter_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_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:
v2/pkg/engine/plan/abstract_selection_rewriter_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/plan/abstract_selection_rewriter_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-12-09T15:31:05.946Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:31:05.946Z
Learning: In the graphql-go-tools repository, Context instances in v2/pkg/engine/resolve must be created using NewContext() rather than Context{} literals. The codebase intentionally avoids defensive nil checks for Context.subgraphErrors to enforce this pattern and fail fast on improper usage. Do not suggest adding defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-08-25T14:44:39.420Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1283
File: v2/pkg/grpctest/product.proto:14-15
Timestamp: 2025-08-25T14:44:39.420Z
Learning: In the gRPC test mapping system in v2/pkg/grpctest/mapping/mapping.go, entity lookup RPCs (like LookupWarehouseById) belong in EntityRPCs mapping but should NOT be added to QueryRPCs. Entity RPCs and Query RPCs serve different purposes in the mapping architecture.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
🧬 Code graph analysis (2)
v2/pkg/engine/plan/datasource_filter_visitor_test.go (2)
v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(120-120)v2/pkg/ast/ast_selection.go (1)
SelectionSet(21-25)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (5)
v2/pkg/engine/plan/configuration.go (2)
DebugConfiguration(51-64)Configuration(9-49)v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceMetadata(34-59)DataSource(277-290)v2/pkg/engine/plan/federation_metadata.go (2)
FederationMetaData(10-18)FederationFieldConfigurations(120-120)v2/pkg/engine/datasource/graphql_datasource/configuration.go (5)
ConfigurationInput(17-24)FetchConfiguration(132-136)SchemaConfiguration(143-147)FederationConfiguration(138-141)Configuration(26-33)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (3)
DefaultPostProcessingConfiguration(40-43)Source(1831-1833)SingleEntityPostProcessingConfiguration(48-51)
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
[failure] 645-645:
File is not properly formatted (gci)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (6)
v2/pkg/engine/plan/datasource_filter_visitor_test.go (2)
1069-1069: LGTM: Test cleanup to prevent state leakage.Resetting
requiresKeytonilafter each iteration ensures that residual key requirements from one test case don't affect subsequent assertions.
1154-1159: LGTM: Federation key metadata now explicit for shareable data sources.The
KeysMetadataadditions forshareableDS1,shareableDS2, andshareableDS3align with the@key(fields: "id")directives in their schemas and ensure proper federation entity resolution in tests.Also applies to: 1182-1187, 1210-1215
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)
1312-1312: LGTM: Interface field registration ensures proper availability tracking.Adding
ChildNode("Node", "id", "name")to these test configurations correctly registers theNodeinterface fields, enabling the rewriter to determine field availability when deciding whether to expand interface selections into concrete type fragments.Also applies to: 1361-1361, 1448-1448, 1486-1486
4081-4149: LGTM: Good coverage for interface field rewriting with partial field availability.This test correctly validates the scenario where:
- The upstream interface (
Named) has fewer fields than the schema definition- Concrete types (
Admin,User) provide the missing field (name)- The rewriter must expand interface selections into concrete type inline fragments
The test structure is consistent with existing patterns in the file.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (2)
645-652: Debug config change is fine; just ensure debug flags remain off by default.
Keeping the debug toggles commented is OK for local troubleshooting; looks intentionally disabled for CI.
18527-18527: Test group rename looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
devsergiy
deleted the
sergiy/eng-8584-fix-planning-shared-nested-nodes
branch
devsergiy
pushed a commit
that referenced
this pull request
Jan 20, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.243](v2.0.0-rc.242...v2.0.0-rc.243) (2026-01-20) ### Bug Fixes * planner fixes for abstract selection rewriter, node selection rules ([#1360](#1360)) ([7baca8b](7baca8b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed planner issues affecting selection operations. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
endigma
pushed a commit
that referenced
this pull request
Jan 30, 2026
…les (#1360) fixes ENG-8583 fixes ENG-8584 fixes ENG-8697 <!-- Important: Before developing new features, please open an issue to discuss your ideas with the maintainers. This ensures project alignment and helps avoid unnecessary work for you. Thank you for your contribution! Please provide a detailed description below and ensure you've met all the requirements. Squashed commit messages must follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard to facilitate changelog generation. Please ensure your PR title follows the Conventional Commits specification, using the appropriate type (e.g., feat, fix, docs) and scope. Examples of good PR titles: - 💥feat!: change implementation in an non-backward compatible way - ✨feat(auth): add support for OAuth2 login - 🐞fix(router): add support for custom metrics - 📚docs(README): update installation instructions - 🧹chore(deps): bump dependencies to latest versions --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved interface field selection handling in federated schemas to correctly handle datasources with incomplete field sets. * Refined query planning logic for complex multi-subgraph federation scenarios. * **Tests** * Expanded federation test coverage for nested selections across multiple subgraphs and interface field handling. * **Chores** * Updated internal debug output for better diagnostic clarity during query planning. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- Please add any additional information or context regarding your changes here. -->
endigma
pushed a commit
that referenced
this pull request
Jan 30, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.243](v2.0.0-rc.242...v2.0.0-rc.243) (2026-01-20) ### Bug Fixes * planner fixes for abstract selection rewriter, node selection rules ([#1360](#1360)) ([7baca8b](7baca8b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed planner issues affecting selection operations. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
endigma
pushed a commit
that referenced
this pull request
Jan 30, 2026
…les (#1360) fixes ENG-8583 fixes ENG-8584 fixes ENG-8697 <!-- Important: Before developing new features, please open an issue to discuss your ideas with the maintainers. This ensures project alignment and helps avoid unnecessary work for you. Thank you for your contribution! Please provide a detailed description below and ensure you've met all the requirements. Squashed commit messages must follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard to facilitate changelog generation. Please ensure your PR title follows the Conventional Commits specification, using the appropriate type (e.g., feat, fix, docs) and scope. Examples of good PR titles: - 💥feat!: change implementation in an non-backward compatible way - ✨feat(auth): add support for OAuth2 login - 🐞fix(router): add support for custom metrics - 📚docs(README): update installation instructions - 🧹chore(deps): bump dependencies to latest versions --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved interface field selection handling in federated schemas to correctly handle datasources with incomplete field sets. * Refined query planning logic for complex multi-subgraph federation scenarios. * **Tests** * Expanded federation test coverage for nested selections across multiple subgraphs and interface field handling. * **Chores** * Updated internal debug output for better diagnostic clarity during query planning. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- Please add any additional information or context regarding your changes here. -->
endigma
pushed a commit
that referenced
this pull request
Jan 30, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.243](v2.0.0-rc.242...v2.0.0-rc.243) (2026-01-20) ### Bug Fixes * planner fixes for abstract selection rewriter, node selection rules ([#1360](#1360)) ([7baca8b](7baca8b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed planner issues affecting selection operations. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
endigma
pushed a commit
that referenced
this pull request
Jan 30, 2026
…les (#1360) fixes ENG-8583 fixes ENG-8584 fixes ENG-8697 <!-- Important: Before developing new features, please open an issue to discuss your ideas with the maintainers. This ensures project alignment and helps avoid unnecessary work for you. Thank you for your contribution! Please provide a detailed description below and ensure you've met all the requirements. Squashed commit messages must follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard to facilitate changelog generation. Please ensure your PR title follows the Conventional Commits specification, using the appropriate type (e.g., feat, fix, docs) and scope. Examples of good PR titles: - 💥feat!: change implementation in an non-backward compatible way - ✨feat(auth): add support for OAuth2 login - 🐞fix(router): add support for custom metrics - 📚docs(README): update installation instructions - 🧹chore(deps): bump dependencies to latest versions --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved interface field selection handling in federated schemas to correctly handle datasources with incomplete field sets. * Refined query planning logic for complex multi-subgraph federation scenarios. * **Tests** * Expanded federation test coverage for nested selections across multiple subgraphs and interface field handling. * **Chores** * Updated internal debug output for better diagnostic clarity during query planning. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- Please add any additional information or context regarding your changes here. -->
endigma
pushed a commit
that referenced
this pull request
Jan 30, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.243](v2.0.0-rc.242...v2.0.0-rc.243) (2026-01-20) ### Bug Fixes * planner fixes for abstract selection rewriter, node selection rules ([#1360](#1360)) ([7baca8b](7baca8b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed planner issues affecting selection operations. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 tasks
3 tasks
3 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes ENG-8653
fixes ENG-8584
fixes ENG-8697
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist