feat: rewrite abstract fragments for grpc#1268
Conversation
WalkthroughMoves per-datasource planning behavior from Planner to Factory; renames and extends DataSourcePlanningBehavior (IncludeTypeNameFields → AllowPlanningTypeName, adds AlwaysFlattenFragments); wires Factory.PlanningBehavior() through DataSourceConfiguration into rewriters, visitors, datasources, and tests; updates constructors and error identifiers accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ysmolski
force-pushed
the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
from
6793deb to
5032dbc
Compare
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
ysmolski
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: 2
🧹 Nitpick comments (2)
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
58-62: WithBehavior ordering: consider guarding mis-ordering in testsThe note says WithBehavior should be called before Schema. To make failures explicit in tests, you could add a lightweight guard (e.g., panic or t.Helper() assert) if Schema has already been invoked.
func (b *dsBuilder) WithBehavior(behavior DataSourcePlanningBehavior) *dsBuilder { + if b.ds.factory != nil { + panic("WithBehavior must be called before Schema() in test setup") + } b.behavior = &behavior return b }v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
102-124: Potential goroutine capture pitfall in range loopThe closure uses index and invocation from the range loop. If not building with Go 1.22+ semantics, capture locals to avoid races:
- for index, invocation := range invocations { - errGrp.Go(func() error { + for idx, inv := range invocations { + index, invocation := idx, inv + errGrp.Go(func() error { // ... - methodName := fmt.Sprintf("/%s/%s", invocation.ServiceName, invocation.MethodName) + methodName := fmt.Sprintf("/%s/%s", invocation.ServiceName, invocation.MethodName) // ... responses[index] = response return nil }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/factory.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/planner.go(0 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go(1 hunks)v2/pkg/engine/datasource/staticdatasource/static_datasource.go(1 hunks)v2/pkg/engine/plan/abstract_selection_rewriter.go(6 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_test.go(102 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(2 hunks)v2/pkg/engine/plan/node_selection_visitor.go(2 hunks)v2/pkg/engine/plan/path_builder_visitor.go(2 hunks)v2/pkg/engine/plan/planner_configuration.go(0 hunks)v2/pkg/engine/plan/planner_test.go(1 hunks)v2/pkg/engine/plan/visitor.go(1 hunks)
💤 Files with no reviewable changes (2)
- v2/pkg/engine/datasource/introspection_datasource/planner.go
- v2/pkg/engine/plan/planner_configuration.go
🧰 Additional context used
🧬 Code Graph Analysis (10)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceConfiguration(249-252)
v2/pkg/engine/datasource/introspection_datasource/factory.go (1)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceConfiguration(249-252)DataSourcePlanningBehavior(376-405)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(376-405)
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceMetadata(34-54)DataSourcePlanningBehavior(376-405)
v2/pkg/engine/plan/planner_test.go (1)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceConfiguration(249-252)DataSourcePlanningBehavior(376-405)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceConfiguration(249-252)DataSourcePlanningBehavior(376-405)
v2/pkg/engine/plan/abstract_selection_rewriter.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(254-267)
v2/pkg/engine/plan/node_selection_visitor.go (3)
v2/pkg/engine/plan/configuration.go (1)
DebugConfiguration(34-47)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(254-267)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (1)
NodeSuggestions(91-96)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(376-405)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
v2/pkg/engine/plan/datasource_configuration.go (3)
DataSourcePlanner(442-447)DataSourceConfiguration(249-252)DataSourcePlanningBehavior(376-405)
🔇 Additional comments (23)
v2/pkg/engine/plan/node_selection_visitor.go (2)
12-17: Documentation improvements look good.The updated comments provide better clarity on the visitor's behavior and fix minor grammatical issues.
Also applies to: 25-29
652-652: Constructor pattern change aligns with the PR objective.The change from a 2-argument constructor with setter methods to a 4-argument constructor passing
upstreamSchemaanddsdirectly is consistent with moving planning behavior configuration from Planner to Factory level.v2/pkg/engine/plan/abstract_selection_rewriter.go (2)
67-68: Well-structured enforcement mechanism for fragment rewriting.The addition of
enforceRewritingflag and its initialization fromdsConfiguration.PlanningBehavior().OperationEnforceRewritingFragmentSelectionscleanly implements the configurable rewriting behavior. This aligns perfectly with the PR's goal of moving planning behavior to the Factory level.Also applies to: 78-88
178-184: Consistent enforcement pattern across all rewrite scenarios.The enforcement checks are uniformly applied across union, object, and interface field selections. When
enforceRewritingis true and inline fragments are present, the rewriter immediately returns true to trigger rewriting. This is particularly useful for datasources like gRPC that require flattened fragments.Also applies to: 362-368, 435-442
v2/pkg/engine/plan/planner_test.go (2)
999-1008: Test factory correctly implements the new PlanningBehavior API.The implementation includes proper lazy initialization and follows Go conventions for unused parameters. This ensures tests can configure and verify planning behavior consistently with the production code.
1010-1017: FakePlanner initialization properly propagates factory state.The planner now correctly receives both
upstreamSchemaandbehaviorfrom the factory, ensuring test consistency with the new architecture where planning behavior is owned by the factory.v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)
18-19: Test structure properly updated for the new constructor pattern.The test harness correctly builds the DataSource using
dsBuilderwith the upstream definition and passes all required arguments tonewFieldSelectionRewriter. The requirement check forfieldRefbefore DS creation is a good defensive practice.Also applies to: 49-59
209-294: Excellent test coverage for gRPC fragment flattening behavior.The new test cases specifically validate the
OperationEnforceRewritingFragmentSelectionsflag behavior for both interfaces and unions, which is critical for gRPC datasource support. The test expectations correctly show fragments being flattened to concrete types.v2/pkg/engine/plan/datasource_configuration.go (3)
17-32: Clean architectural improvement moving planning behavior to Factory level.The addition of
PlanningBehavior()to thePlannerFactoryinterface and the improved documentation clearly establish the factory as the owner of planning behavior configuration. This is a good separation of concerns.
259-260: DataSource interface properly exposes planning behavior.The addition of
UpstreamSchema()andPlanningBehavior()methods to the DataSource interface, with the implementation delegating to the factory, maintains clean abstraction boundaries.Also applies to: 295-297
375-405: Comprehensive DataSourcePlanningBehavior configuration structure.The struct provides well-documented flags for controlling various planning aspects:
- Alias handling for dynamic vs static datasources
- Response path management
- Type name field inclusion
- Fragment rewriting enforcement (critical for gRPC support)
The documentation includes clear examples and use cases.
v2/pkg/engine/plan/visitor.go (1)
1024-1026: Behavior sourcing via DataSourceConfiguration is correctSwitching to plannerConfig.DataSourceConfiguration().PlanningBehavior() keeps alias override logic intact and aligns with the new API surface.
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
23-26: Builder initialization refactor is fineConstructing dsBuilder with an explicit DataSourceMetadata allocation improves clarity and keeps semantics unchanged.
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (2)
307-309: UpstreamSchema: underscore parameter is appropriateUnderscoring the unused parameter is a good cleanup and doesn’t change behavior.
311-317: PlanningBehavior defaults changed; verify intent for Pub/SubThis sets:
- MergeAliasedRootNodes: true
- OverrideFieldPathFromAlias: true
- IncludeTypeNameFields: true
Previously (on the Planner) MergeAliasedRootNodes was false and OverrideFieldPathFromAlias was false. For Pub/Sub, merging aliased roots or aliasing response paths may change runtime behavior (e.g., multiple root events collapsing into one planning path, or path aliasing expectations).
Please confirm this change is intentional and covered by tests. If not, consider staying closer to earlier semantics:
- return plan.DataSourcePlanningBehavior{ - MergeAliasedRootNodes: true, - OverrideFieldPathFromAlias: true, - IncludeTypeNameFields: true, - } + return plan.DataSourcePlanningBehavior{ + MergeAliasedRootNodes: false, + OverrideFieldPathFromAlias: false, + IncludeTypeNameFields: true, + }Would you like me to add/adjust Pub/Sub planner tests to validate alias handling and aliased root merging behavior?
v2/pkg/engine/datasource/introspection_datasource/factory.go (2)
29-32: UpstreamSchema: underscore parameter is appropriateMinor cleanup; behavior unchanged.
33-39: PlanningBehavior defaults look correct for introspection
- MergeAliasedRootNodes: false
- OverrideFieldPathFromAlias: true
- IncludeTypeNameFields: true
These settings match typical needs for introspection.
v2/pkg/engine/plan/path_builder_visitor.go (2)
724-724: Behavior retrieval moved to dsConfigurationUsing dsConfiguration.PlanningBehavior() is consistent with the PR-wide API shift and keeps subsequent decisions (typename, alias merging) correct.
1240-1243: Doc improves contract for typename path additionThe clarification that the planner must already own parentPath avoids accidental path fan-out across planners.
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (4)
17-19: Planner signature cleanup is fineUnderscoring the unused logger param is appropriate in this simple planner.
25-27: UpstreamSchema: underscore parameter is appropriateMinor cleanup; behavior unchanged.
29-31: PlanningBehavior zero-value is sensible for a static sourceReturning the zero-value behavior (all false) matches the intended defaults for static/REST-like datasources.
33-36: Configuration struct addition is fineNo concerns.
ysmolski
force-pushed
the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
2 times, most recently
from
e3be9d0 to
e124697
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/factory.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/planner.go(0 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go(1 hunks)v2/pkg/engine/datasource/staticdatasource/static_datasource.go(1 hunks)v2/pkg/engine/plan/abstract_selection_rewriter.go(6 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_test.go(102 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(2 hunks)v2/pkg/engine/plan/node_selection_visitor.go(2 hunks)v2/pkg/engine/plan/path_builder_visitor.go(2 hunks)v2/pkg/engine/plan/planner_configuration.go(0 hunks)v2/pkg/engine/plan/planner_test.go(1 hunks)v2/pkg/engine/plan/visitor.go(1 hunks)
💤 Files with no reviewable changes (2)
- v2/pkg/engine/plan/planner_configuration.go
- v2/pkg/engine/datasource/introspection_datasource/planner.go
🚧 Files skipped from review as they are similar to previous changes (9)
- v2/pkg/engine/datasource/introspection_datasource/factory.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
- v2/pkg/engine/plan/visitor.go
- v2/pkg/engine/plan/datasource_filter_visitor_test.go
- v2/pkg/engine/plan/abstract_selection_rewriter.go
- v2/pkg/engine/plan/node_selection_visitor.go
- v2/pkg/engine/plan/planner_test.go
- v2/pkg/engine/datasource/staticdatasource/static_datasource.go
- v2/pkg/engine/plan/datasource_configuration.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceConfiguration(249-252)DataSourcePlanningBehavior(376-405)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(376-405)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)
🔇 Additional comments (8)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
307-309: LGTM: UpstreamSchema intentionally unusedReturning (nil, false) is appropriate for this datasource; the underscore-parameter makes intent explicit.
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (5)
18-19: LGTM!The addition of
upstreamDefinitionanddsBuilderfields properly supports the new Factory-based approach for DataSource creation. The comment clearly explains the purpose of theupstreamDefinitionfield.
49-55: LGTM!The field reference validation ensures test robustness, and the DS creation using
dsBuilder.Schema().DS()pattern aligns with the new architecture. The comment about schema parsing behavior provides valuable context.
59-59: LGTM!The constructor call update to the 4-argument form (
&op, &def, &upstreamDef, ds) properly aligns with the new DataSource wiring approach.
186-294: LGTM!Excellent test coverage for the new grpc fragment flattening functionality:
definitionBprovides a clean schema with interfaces and unions for testing- Both test cases properly use
OperationEnforceRewritingFragmentSelections: trueto enable the new behavior- Expected operations correctly demonstrate fragment flattening from nested interface/union fragments to concrete type fragments
- Test scenarios cover both interface and union fragment flattening paths
These tests directly validate the core PR objective of rewriting abstract fragments for grpc.
319-333: LGTM!The consistent migration from
dsConfiguration: dsb()...DS()todsBuilder: dsb()...improves test structure by:
- Centralizing DS creation logic in the test harness
- Removing inline DS construction from test case definitions
- Creating cleaner separation between test data and test execution
This pattern aligns well with the broader architectural changes in the PR.
Also applies to: 401-415, 484-498, 571-585, 658-677, 731-745, 773-787, 810-824, 868-882, 933-947, 999-1013, 1059-1073, 1123-1137, 1169-1183, 1217-1231, 1250-1272, 1303-1317, 1342-1346, 1403-1417, 1453-1461, 1536-1551, 1630-1645, 1702-1722, 1775-1803, 1865-1885, 1941-1955, 2009-2024, 2081-2095, 2159-2174, 2231-2245, 2303-2317, 2395-2409, 2481-2494, 2562-2576, 2648-2663, 2742-2757, 2815-2823, 2879-2888, 2948-2957, 2983-2983, 3016-3016, 3046-3046, 3075-3075, 3104-3104, 3132-3132, 3158-3158, 3184-3184, 3222-3222, 3254-3254, 3286-3286, 3321-3321, 3385-3398, 3463-3476, 3563-3576, 3671-3675, 3767-3776, 3845-3855
v2/pkg/engine/plan/path_builder_visitor.go (2)
723-723: LGTM!The change to derive
planningBehaviourfromdsConfiguration.PlanningBehavior()properly implements the architectural shift of moving planning behavior APIs from Planner to Factory, as described in the PR objectives.
1240-1270: LGTM!Excellent improvements to
addPlannerPathForTypename:
- Enhanced documentation clearly warns about correct usage to prevent incorrect selection set visiting
- Proper gating by
planningBehaviour.IncludeTypeNameFieldsensures__typenamepaths are only added when the planning behavior permits it- Existing path check prevents duplicate path additions
- Clean implementation with logical flow: validate field → check behavior → verify path → add configuration
This properly integrates planning behavior for
__typenamefield handling and prevents misuse that could affect root-aliased node merging and field planning.
ysmolski
force-pushed
the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
from
e124697 to
91c52dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
v2/pkg/engine/plan/datasource_configuration.go (2)
295-297: Delegation for PlanningBehavior()Simple, correct delegation. If PlanningBehavior is expensive to compute for some factories, consider caching the result on dataSourceConfiguration during construction.
375-405: New flag: OperationEnforceRewritingFragmentSelectionsClear and useful for gRPC flattening. Consider expanding the comment to specify interaction with IncludeTypeNameFields and when enforcement applies (e.g., only for abstract selections, and whether it affects union vs. interface equivalently).
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (3)
49-56: Precondition on fieldRef and DS constructionThe require.NotEqual check is a solid precondition. The comment “rewriter does not use def from the FakeFactory” reads stale now that upstreamDef and ds are both passed in. Consider clarifying or removing this comment.
2395-2410: Potential confusion: RootNode configured for "iface" in a returnsUnion testThis test exercises Query.returnsUnion, but the DS root node is configured for "iface". While it may not affect rewriter behavior here, aligning the configured root field with the tested field improves clarity.
ds := testCase.dsBuilder. - Schema(testCase.upstreamDefinition). - DS() + Schema(testCase.upstreamDefinition). + DS() ... -RootNode("Query", "iface"). +RootNode("Query", "returnsUnion").If intentional, please add a short comment clarifying why the mismatch is acceptable in this test.
999-1012: General: RootNode usage not always matching fieldName under testMultiple tests exercise a Query field different from “iface” but configure the DS root at “iface”. If this is by design for the rewriter-only unit scope, consider adding a brief comment once to explain why exact root field alignment is not required.
Also applies to: 1060-1073, 2395-2410
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/factory.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/planner.go(0 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go(1 hunks)v2/pkg/engine/datasource/staticdatasource/static_datasource.go(1 hunks)v2/pkg/engine/plan/abstract_selection_rewriter.go(6 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_test.go(102 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(2 hunks)v2/pkg/engine/plan/node_selection_visitor.go(2 hunks)v2/pkg/engine/plan/path_builder_visitor.go(2 hunks)v2/pkg/engine/plan/planner_configuration.go(0 hunks)v2/pkg/engine/plan/planner_test.go(1 hunks)v2/pkg/engine/plan/visitor.go(1 hunks)
💤 Files with no reviewable changes (2)
- v2/pkg/engine/plan/planner_configuration.go
- v2/pkg/engine/datasource/introspection_datasource/planner.go
🚧 Files skipped from review as they are similar to previous changes (10)
- v2/pkg/engine/plan/visitor.go
- v2/pkg/engine/plan/abstract_selection_rewriter.go
- v2/pkg/engine/plan/datasource_filter_visitor_test.go
- v2/pkg/engine/datasource/introspection_datasource/factory.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
- v2/pkg/engine/plan/node_selection_visitor.go
- v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go
- v2/pkg/engine/datasource/staticdatasource/static_datasource.go
- v2/pkg/engine/plan/path_builder_visitor.go
- v2/pkg/engine/plan/planner_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (6)
execution/graphql/schema.go (1)
Schema(30-36)pkg/ast/index.go (1)
Index(12-29)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(376-405)pkg/execution/execution.go (1)
RootNode(333-336)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)v2/pkg/ast/ast_selection.go (1)
SelectionSet(20-24)
v2/pkg/engine/plan/datasource_configuration.go (9)
v2/pkg/engine/datasource/introspection_datasource/planner.go (1)
Planner(16-25)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
Planner(37-40)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
Planner(54-63)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
Planner(55-96)v2/pkg/engine/plan/planner.go (1)
Planner(15-25)v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/plan/federation_metadata.go (1)
FederationMetaData(10-16)execution/graphql/schema.go (1)
TypeFields(19-22)v2/pkg/engine/plan/type_field.go (1)
TypeFields(13-13)
⏰ 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.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (8)
v2/pkg/engine/plan/datasource_configuration.go (3)
35-49: Documentation improvement on DataSourceMetadataThe clarifications around RootNodes/ChildNodes for federation are helpful. No functional concerns.
413-436: DownstreamResponseFieldAlias docs sync with PlanningBehavior.OverrideFieldPathFromAliasDoc alignment looks good and prevents misuse.
258-261: No custom DataSource implementations requiring updates
- Grepping the codebase shows only the built-in fakes (FakeFactory, FakePlanner) and
dataSourceConfigurationitself implement the new methods.- Both fakes in
planner_test.goandplan_closer_test.goalready defineUpstreamSchema()andPlanningBehavior().- No other structs were found needing these methods.
All existing mocks and implementations are up to date—no further changes required.
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (5)
18-20: Harness change: upstreamDefinition and dsBuilderGood shift: upstreamDefinition is now explicit and used to build the DS. This makes tests clearer about upstream vs. composed schema.
208-228: gRPC flattening: enabling OperationEnforceRewritingFragmentSelectionsGood targeted activation of the new behavior flag and relevant coverage.
253-271: gRPC union flattening testCoverage looks appropriate and complements the interface test. No concerns.
2980-3011: Broad test additions and refactorsThe refactors to use dsBuilder.Schema(...).DS() and the expanded matrix of scenarios (unions, interfaces, interface objects, requires) look comprehensive and valuable. Nice work.
Also applies to: 3013-3041, 3043-3070, 3072-3099, 3101-3126, 3128-3153, 3155-3179, 3181-3217, 3219-3316, 3318-3347, 3350-3420, 3422-3518, 3520-3624, 3626-3738, 3740-3797, 3799-3883
59-60: All deprecated API usages removedNo remaining calls to SetUpstreamDefinition or SetDatasourceConfiguration were found, and all calls to newFieldSelectionRewriter use the updated arity. LGTM.
ysmolski
force-pushed
the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
from
91c52dc to
5755b8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
17-32: Factory-centric PlanningBehavior/UpstreamSchema API shift looks good; verify all factories implement both methodsThe interface change is sound and centralizes planning flags/static schema on the Factory. Please verify all Factory types now satisfy the new interface (both methods present) to prevent runtime interface assertion failures.
Run from repo root:
#!/bin/bash set -euo pipefail echo "Factories missing PlanningBehavior():" # List all Factory types mapfile -t factories < <(rg -n --pcre2 'type\s+([A-Za-z0-9_]+Factory(\[[^\]]+\])?)\s+struct' -r '$1' | awk -F: '{print $2}' | sed 's/struct$//' | sed 's/[[:space:]]*$//' | sort -u) # For each factory, check for PlanningBehavior and UpstreamSchema missing_pb=0 missing_us=0 for f in "${factories[@]}"; do if ! rg -q --pcre2 "func\s*\(\s*\*?$f\s*\)\s*PlanningBehavior\(\)\s*plan\.DataSourcePlanningBehavior"; then echo " - $f" missing_pb=1 fi done echo echo "Factories missing UpstreamSchema(cfg):" for f in "${factories[@]}"; do if ! rg -q --pcre2 "func\s*\(\s*\*?$f\s*\)\s*UpstreamSchema\(\s*dataSourceConfig\s+plan\.DataSourceConfiguration\[.*\]\s*\)\s*\(\s*\*ast\.Document\s*,\s*bool\s*\)"; then echo " - $f" missing_us=1 fi done echo echo "Any legacy references to IncludeTypeNameFields?" rg -n 'IncludeTypeNameFields' echo echo "Any old newFieldSelectionRewriter(op, def) call-sites left?" rg -n --pcre2 'newFieldSelectionRewriter\(\s*[^,]+,\s*[^,)]+\)' -g '!**/*_test.go' exit $((missing_pb || missing_us))
🧹 Nitpick comments (5)
v2/pkg/engine/plan/datasource_configuration.go (2)
39-50: Doc wording polish: GraphQL casing, gRPC casing, and “cannot” phrasingTiny clarity/grammar improvements in comments; adjust GraphQL/gRPC casing and “couldn’t” → “cannot.”
Apply:
- // ChildNodes describes additional fields, which are requested along with fields that the datasource has. - // They're always required for the Graphql datasources because each field could have its own datasource. - // For a flat datasource (HTTP/REST or GRPC) we couldn't request fewer fields, as we always get a full response. + // ChildNodes describes additional fields which are requested along with fields that the datasource has. + // They're always required for GraphQL datasources because each field could have its own datasource. + // For a flat datasource (HTTP/REST or gRPC) we cannot request fewer fields, as we always get a full response.
377-387: Clarify MergeAliasedRootNodes guidance re: gRPCThe note “static data sources (REST, static, GRPC)” can be misread for GraphQL-over-gRPC, which remains “dynamic.” Suggest clarifying it refers to non-GraphQL/flat gRPC implementations.
Apply:
- // On dynamic data sources (GraphQL, SQL) this should be set to true, - // and for static data sources (REST, static, GRPC) it should be false. + // On dynamic data sources (GraphQL, SQL) this should be set to true, + // and for static data sources (REST, static, non-GraphQL gRPC) it should be false.v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
307-309: Nit: Add explanatory comment to UpstreamSchema()This datasource has no upstream GraphQL schema. A short comment would avoid confusion.
Apply:
-func (f *Factory[T]) UpstreamSchema(_ plan.DataSourceConfiguration[T]) (*ast.Document, bool) { - return nil, false -} +func (f *Factory[T]) UpstreamSchema(_ plan.DataSourceConfiguration[T]) (*ast.Document, bool) { + // PubSub datasource does not operate against an upstream GraphQL schema. + return nil, false +}v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)
205-247: New gRPC flattening tests are on pointThe grpc-like behavior via AlwaysFlattenFragments=true validates interface/union flattening well. Consider adding a negative case (same setup with AlwaysFlattenFragments=false) to assert no flattening by default.
I can add such a test variant if helpful.
Also applies to: 249-290
165-181: Test DSL usage is repetitive; consider helpers to DRY up dsBuilder setupMany cases repeat identical RootNode/ChildNode/KeysMetadata and (for gRPC) WithBehavior blocks. Extract small helpers (e.g., dsbGrpcWithCommonNodes(), dsbWithCommonNodes()) to reduce duplication and keep focus on per-test intent.
Also applies to: 315-329, 397-411, 480-494, 567-581, 727-741, 769-783, 807-821, 865-879, 929-943, 995-1009, 1055-1069, 1119-1133, 1165-1179, 1213-1227, 1247-1269, 1299-1313, 1339-1343, 1399-1413, 1449-1457, 1533-1547, 1626-1641, 1718-1736, 1771-1800, 1861-1881, 1937-1951, 2005-2019, 2077-2091, 2155-2171, 2227-2241, 2299-2313, 2391-2405, 2477-2491, 2559-2573, 2645-2660, 2739-2753, 2811-2820, 2876-2885, 2945-2953, 2979-2983, 3009-3013, 3039-3043, 3068-3072, 3097-3101, 3125-3129, 3154-3158, 3179-3183, 3217-3221, 3247-3251, 3279-3283, 3317-3321, 3381-3395, 3459-3473, 3559-3573, 3667-3672, 381-3852
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/factory.go(1 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go(1 hunks)v2/pkg/engine/plan/abstract_selection_rewriter.go(6 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_test.go(102 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(2 hunks)v2/pkg/engine/plan/node_selection_visitor.go(2 hunks)v2/pkg/engine/plan/path_builder_visitor.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/datasource/introspection_datasource/factory.go
- v2/pkg/engine/plan/datasource_filter_visitor_test.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(378-407)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceConfiguration(251-254)DataSourcePlanningBehavior(378-407)
v2/pkg/engine/plan/node_selection_visitor.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(256-269)
v2/pkg/engine/plan/abstract_selection_rewriter.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(256-269)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(378-407)
v2/pkg/engine/plan/path_builder_visitor.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(378-407)
v2/pkg/engine/plan/datasource_configuration.go (8)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
Planner(54-63)v2/pkg/engine/datasource/introspection_datasource/planner.go (1)
Planner(16-25)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
Planner(37-40)v2/pkg/engine/plan/planner.go (1)
Planner(15-25)v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/plan/federation_metadata.go (1)
FederationMetaData(10-16)execution/graphql/schema.go (1)
TypeFields(19-22)v2/pkg/engine/plan/type_field.go (1)
TypeFields(13-13)
🔇 Additional comments (15)
v2/pkg/engine/plan/datasource_configuration.go (2)
261-263: LGTM: DataSource now exposes PlanningBehavior and UpstreamSchemaThe interface surface (and the shift away from the old DataSourceUpstreamSchema) is consistent with the rest of the PR and simplifies downstream consumption.
293-299: LGTM: Delegation from DataSourceConfiguration to FactoryForwarding UpstreamSchema/PlanningBehavior to the Factory centralizes responsibilities and keeps configuration immutable/clean at runtime.
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
311-317: LGTM: PubSub planning flags aligned with static datasource behavior
- MergeAliasedRootNodes: false
- OverrideFieldPathFromAlias: false
- AllowToPlanTypeNameFields: true
Matches the intended static/flat nature of PubSub.
v2/pkg/engine/plan/node_selection_visitor.go (1)
12-18: Doc: Expanded responsibilities are clearThe added notes about rewriting abstract selections are accurate and help set expectations for this visitor’s role.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
1820-1828: LGTM: AlwaysFlattenFragments toggled by gRPC capability
- Dynamic defaults (merge aliases, alias path override, typename planning) preserved.
- AlwaysFlattenFragments bound to (grpcClient != nil || grpcClientProvider != nil) aligns with the PR’s goal to enforce fragment flattening for gRPC mode.
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (3)
18-20: Test harness extended to pass DS/upstream via builderAdding upstreamDefinition and dsBuilder to cases matches the new constructor API and improves test clarity.
47-56: LGTM: Guard against missing fieldRef and switch to new 3-arg rewriterThe require on fieldRef prevents misleading false positives, and the constructor update is correct.
182-203: Schema fix: User implements Named & Numbered correctlyUser now defines name, satisfying Named; this prevents invalid schema/test ambiguity.
v2/pkg/engine/plan/path_builder_visitor.go (3)
721-724: LGTM! Clean refactoring to usePlanningBehavior()The change correctly migrates from accessing
plannerConfig.DataSourcePlanningBehavior()todsConfiguration.PlanningBehavior(), aligning with the new API design where planning behavior is obtained directly from the datasource configuration.
764-766: Good adaptation to renamed fieldAllowToPlanTypeNameFieldsThe conditional logic correctly uses the new field name
AllowToPlanTypeNameFields(formerlyIncludeTypeNameFields) from the planning behavior, maintaining the same semantics for controlling whether__typenamefields should be planned.
1240-1270: Clean internal documentation updateThe comment update from line 1245-1246 improves clarity by explaining that
__typenamepaths should only be added when the planner has a parent path. The implementation correctly guards against adding__typenameinappropriately through theAllowToPlanTypeNameFieldscheck and idempotency handling.v2/pkg/engine/plan/abstract_selection_rewriter.go (4)
78-90: Robust constructor refactoring with proper error handlingThe constructor now correctly:
- Accepts a
DataSourceparameter instead of requiring separateupstreamDefinitionanddsConfiguration- Retrieves the upstream schema with proper existence checking
- Initializes the
alwaysRewriteflag from the planning behavior- Maintains clean separation of concerns
The panic on missing upstream schema is appropriate since this is a programming error that should be caught during development.
171-176: Clear and focused rewrite condition helperThe
mustRewritemethod provides a clean abstraction for determining when aggressive fragment rewriting should occur. The condition correctly checks for the presence of inline fragments on interfaces, unions, or objects whenalwaysRewriteis enabled.
179-181: Consistent integration ofmustRewriteacross all rewrite pathsThe
mustRewritecheck is properly integrated at the beginning of:
unionFieldSelectionNeedsRewrite(line 179-181)objectFieldSelectionNeedsRewrite(line 359-361)interfaceFieldSelectionNeedsRewrite(line 428-431)This ensures consistent behavior across all abstract type rewriting scenarios when
AlwaysFlattenFragmentsis enabled.Also applies to: 359-361, 428-431
456-463: Improved comment clarity for interface object handlingThe updated comment better explains the rationale for adding shared
__typenameselection when dealing with interface objects and concrete type fragments. This clarification helps future maintainers understand the flattening behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
v2/pkg/engine/plan/abstract_selection_rewriter_info.go (1)
158-176: Bug: union fragment current-DS type names overwrite federated listIn collectInlineFragmentInformation for union fragments, unionMemberTypeNames is first set from the federated schema (Line 158), but then overwritten with the current DS types (Lines 174-175) instead of populating unionMemberTypeNamesInCurrentDS. This loses the original federated list and mislabels the current-DS-specific set.
Fix by assigning the current DS list to unionMemberTypeNamesInCurrentDS and keep the original unionMemberTypeNames intact. Also, sort the current DS list for deterministic behavior (mirrors the earlier sort for the federated list).
Apply:
- inlineFragmentSelectionOnUnion := inlineFragmentSelectionOnUnion{ + inlineFragmentSelectionOnUnion := inlineFragmentSelectionOnUnion{ inlineFragmentSelection: inlineFragmentSelection, unionMemberTypeNames: unionMemberTypeNames, } @@ - if upstreamNode.Kind == ast.NodeKindUnionTypeDefinition { - unionMemberTypeNamesFromCurrentDS, _ := r.upstreamDefinition.UnionTypeDefinitionMemberTypeNames(upstreamNode.Ref) - entityNames, _ := r.datasourceHasEntitiesWithName(unionMemberTypeNamesFromCurrentDS) - - inlineFragmentSelectionOnUnion.unionMemberTypeNames = unionMemberTypeNamesFromCurrentDS - inlineFragmentSelectionOnUnion.unionMemberEntityNames = entityNames - } + if upstreamNode.Kind == ast.NodeKindUnionTypeDefinition { + unionMemberTypeNamesFromCurrentDS, _ := r.upstreamDefinition.UnionTypeDefinitionMemberTypeNames(upstreamNode.Ref) + sort.Strings(unionMemberTypeNamesFromCurrentDS) + entityNames, _ := r.datasourceHasEntitiesWithName(unionMemberTypeNamesFromCurrentDS) + + inlineFragmentSelectionOnUnion.unionMemberTypeNamesInCurrentDS = unionMemberTypeNamesFromCurrentDS + inlineFragmentSelectionOnUnion.unionMemberEntityNames = entityNames + }
♻️ Duplicate comments (1)
v2/pkg/engine/plan/node_selection_visitor.go (1)
647-656: Avoid duplicated UpstreamSchema() checks; let the rewriter gateThis block re-checks ds.UpstreamSchema(), but newFieldSelectionRewriter also validates presence and returns ErrNoUpstreamSchema. Remove the pre-check and construct the rewriter unconditionally; this consolidates the logic in one place and avoids divergent behavior.
Apply:
- _, ok := ds.UpstreamSchema() - if !ok { - return - } - - rewriter, err := newFieldSelectionRewriter(c.operation, c.definition, ds) + rewriter, err := newFieldSelectionRewriter(c.operation, c.definition, ds) if err != nil { c.walker.StopWithInternalErr(err) return }
🧹 Nitpick comments (8)
v2/pkg/engine/plan/abstract_selection_rewriter_info.go (2)
101-106: Consistent error renames; minor grammar follow-up is elsewhereUsing ErrInlineFragmentDoesntHaveSelectionSet (Line 104) and ErrInlineFragmentTypeIsNotExists (Line 113) matches the new exported identifiers. The grammar of the error string is defined in abstract_selection_rewriter.go; I’ve suggested a nit fix there.
Also applies to: 109-115
143-146: Typo in comments: “definion” and grammar tweakTwo small comment nits:
- Line 143: “definion” → “definition”
- Line 167: “is not exists” → “does not exist”
Apply:
- // NOTE: We are getting type names implementing interface from the current SUBGRAPH definion + // NOTE: We are getting type names implementing interface from the current SUBGRAPH definition @@ - // NOTE: at this point we ignore case when upstreamNode is not exists in the upstream schema + // NOTE: at this point we ignore the case when upstreamNode does not exist in the upstream schemaAlso applies to: 166-168
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (1)
206-248: Add a negative counterpart for AlwaysFlattenFragments disabledThe two new GRPC-focused tests validate aggressive flattening when AlwaysFlattenFragments is true. Please add complementary tests with AlwaysFlattenFragments set to false for the same inputs to assert there is no rewrite. This guards the gating logic in mustRewrite across interface/union paths.
Do you want me to draft those two additional test cases mirroring these inputs with AlwaysFlattenFragments: false?
Also applies to: 250-291
v2/pkg/engine/plan/node_selection_visitor.go (2)
12-18: Doc comments improved; minor nit on phrasingThe multi-pass explanation reads clearer now. Tiny phrasing nit: “rewrite abstract selection if necessary” → “rewrite abstract selections if necessary.”
Apply:
-// This visitor might add required fields and rewrite abstract selection if necessary. +// This visitor might add required fields and rewrite abstract selections if necessary.
672-677: Typo in comment: “stoping” → “stopping”Apply:
- // skip walking into a rewritten field instead of stoping the whole visitor + // skip walking into a rewritten field instead of stopping the whole visitorv2/pkg/engine/plan/abstract_selection_rewriter.go (3)
15-21: Exported error names: adjust messages for grammarThe new exported errors are good. Minor grammar nit: “does not exists” → “does not exist”.
Apply:
- ErrInlineFragmentTypeIsNotExists = errors.New("unexpected error: inline fragment type condition does not exists") + ErrInlineFragmentTypeIsNotExists = errors.New("unexpected error: inline fragment type condition does not exist")
551-555: Typo in comment: “dissalow” → “disallow” (twice)Apply:
- // we also dissalow adding typename because for the interface object type we intentionally do not add root node for a typename + // we also disallow adding typename because for the interface object type we intentionally do not add a root node for a typename
360-381: Optional: Early gate to avoid some work when alwaysRewrite is onIn objectFieldSelectionNeedsRewrite, you already short-circuit via mustRewrite(s). Consider moving the mustRewrite check before computing selectionSetInfo in the caller where feasible to avoid some collection work when AlwaysFlattenFragments is true. Not critical, but could shave cycles in large operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/plan/abstract_selection_rewriter.go(8 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_info.go(3 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_test.go(102 hunks)v2/pkg/engine/plan/node_selection_visitor.go(2 hunks)
⏰ 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). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (6)
v2/pkg/engine/plan/abstract_selection_rewriter_info.go (1)
83-90: Error identifier rename usage looks consistentThe switch to ErrFieldDoesntHaveSelectionSet in collectFieldInformation (Line 86) aligns with the new exported error naming and is correct.
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (3)
16-25: Test harness refactor to dsBuilder/upstream wiring is solid
- Carrying upstreamDefinition via testCase and using dsBuilder.SchemaMergedWithBase(...) ensures rewriter construction has upstream schema context.
- The additional dsBuilder in the test case struct reads well.
47-57: Good defensive check for fieldRef validityrequire.NotEqual against ast.InvalidRef prevents downstream panics and makes failures explicit. The new constructor call newFieldSelectionRewriter(&op, &def, ds) with require.NoError is aligned with the new API.
49-52: Plumbing DS through dsBuilder and using new rewriter API is correctUsing dsBuilder.SchemaMergedWithBase(upstreamDefinition).DS() guarantees the DataSource provides UpstreamSchema() to the rewriter. The three-argument constructor and error handling are used correctly.
Also applies to: 55-57
v2/pkg/engine/plan/abstract_selection_rewriter.go (2)
80-92: Constructor: upstream schema derivation and behavior wiring LGTM
- Pulling upstreamDefinition from dsConfiguration.UpstreamSchema() centralizes configuration.
- alwaysRewrite sourced from PlanningBehavior().AlwaysFlattenFragments matches the PR’s direction.
173-178: Aggressive rewrite gate reads wellmustRewrite(s) cleanly centralizes the AlwaysFlattenFragments behavior and is used consistently across union/object/interface checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
v2/pkg/engine/plan/abstract_selection_rewriter_info.go (1)
143-146: Nit: Fix typos in comments ("definion" → "definition", grammar)Minor comment cleanups to improve readability.
- // NOTE: We are getting type names implementing interface from the current SUBGRAPH definion - // NOTE: at this point we ignore case when upstreamNode is not exists in the upstream schema + // NOTE: We are getting type names implementing the interface from the current SUBGRAPH definition + // NOTE: at this point we ignore the case when the upstreamNode does not exist in the upstream schema- // NOTE: We are getting type names of union members from the current SUBGRAPH definion - // NOTE: at this point we ignore case when upstreamNode is not exists in the upstream schema + // NOTE: We are getting union member type names from the current SUBGRAPH definition + // NOTE: at this point we ignore the case when the upstreamNode does not exist in the upstream schemaAlso applies to: 166-168
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (1)
222-288: Use proper casing “gRPC” in test namesNit: test names currently use “grpc”. Prefer “gRPC” for clarity and consistency.
- name: "should flatten interfaces for grpc", + name: "should flatten interfaces for gRPC",- name: "should flatten union for grpc", + name: "should flatten union for gRPC",- name: "should not flatten interfaces for non-grpc", + name: "should not flatten interfaces for non-gRPC",- name: "should not flatten union for non-grpc", + name: "should not flatten union for non-gRPC",Also applies to: 290-296, 324-329
v2/pkg/engine/plan/abstract_selection_rewriter.go (3)
15-21: Nit: Fix grammar in error message“does not exists” → “does not exist”
- ErrInlineFragmentHasNoCondition = errors.New("unexpected error: inline fragment type condition does not exists") + ErrInlineFragmentHasNoCondition = errors.New("unexpected error: inline fragment type condition does not exist")
80-92: Constructor: enrich ErrNoUpstreamSchema with DS identifiersReturning ErrNoUpstreamSchema is correct. Include DS id/name for faster diagnosis.
- upstreamDefinition, ok := dsConfiguration.UpstreamSchema() - if !ok { - return nil, ErrNoUpstreamSchema - } + upstreamDefinition, ok := dsConfiguration.UpstreamSchema() + if !ok { + return nil, fmt.Errorf("%w: id=%s name=%s", ErrNoUpstreamSchema, dsConfiguration.Id(), dsConfiguration.Name()) + }
641-643: Nit: typo in TODO“dissapear” → “disappear”
- // TODO: some path actually could dissapear due to rewrite + // TODO: some paths could actually disappear due to rewritev2/pkg/engine/plan/datasource_configuration.go (1)
377-407: Docs and flags LGTM; Nit: use “gRPC” casing in commentsThe behavior flags and their documentation are clear. Prefer “gRPC” casing.
- // and for static data sources (REST, static, GRPC) it should be false. + // and for static data sources (REST, static, gRPC) it should be false.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/plan/abstract_selection_rewriter.go(8 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_info.go(3 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_test.go(102 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
v2/pkg/engine/plan/abstract_selection_rewriter_info.go (1)
v2/pkg/engine/plan/abstract_selection_rewriter.go (3)
ErrFieldHasNoSelectionSet(16-16)ErrInlineFragmentHasNoSelectionSet(17-17)ErrInlineFragmentHasNoCondition(18-18)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)
v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(378-407)
v2/pkg/engine/plan/datasource_configuration.go (3)
v2/pkg/engine/plan/federation_metadata.go (1)
FederationMetaData(10-16)execution/graphql/schema.go (1)
TypeFields(19-22)v2/pkg/engine/plan/type_field.go (1)
TypeFields(13-13)
v2/pkg/engine/plan/abstract_selection_rewriter.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(256-269)
⏰ 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.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (14)
v2/pkg/engine/plan/abstract_selection_rewriter_info.go (3)
83-90: Error rename alignment with package-level declarations looks correctReturning ErrFieldHasNoSelectionSet here matches the exported errors defined in abstract_selection_rewriter.go and keeps error semantics unchanged.
101-106: Consistent inline fragment selection set errorReturning ErrInlineFragmentHasNoSelectionSet is consistent with the new exported error names and usage in the rewriter.
109-115: Consistent inline fragment condition errorReturning ErrInlineFragmentHasNoCondition here matches the new exported error. Behavior remains the same.
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (6)
18-25: Test harness updates: upstream wiring and builder usage are soundAdding upstreamDefinition and dsBuilder to the test case struct cleanly decouples DS provisioning from the rewriter. This aligns tests with the new constructor and upstream retrieval behavior.
47-57: Good: Validate fieldRef, construct DS via builder, and use the new constructor
- require.NotEqual on fieldRef prevents silent passes.
- Constructing DS via dsBuilder.SchemaMergedWithBase(...) ensures upstream is provided where needed.
- Asserting newFieldSelectionRewriter error keeps test assumptions explicit.
166-185: Nice: dsBuilderA ensures clean state per testRe-initializing the builder via a function avoids cross-test state leakage. KeysMetadata is set appropriately for A and B.
186-207: Schema B correctness looks good
- User implements Named & Numbered with the correct field names.
- Query fields and union U are consistent with the new gRPC flattening tests.
208-220: dsBuilderB setup is coherent for gRPC flattening scenariosRoot and child nodes and keys metadata are consistent with the tests’ expectations for flattening and id usage.
222-288: Behavioral coverage: AlwaysFlattenFragments → forced flattening looks correctBoth gRPC-focused tests validate that nested interface/union fragments are flattened to the concrete User type, and that shared field selections (id via keys and name) are merged correctly. The expectedOperation matches the new mustRewrite gate semantics.
v2/pkg/engine/plan/abstract_selection_rewriter.go (2)
173-179: Force-rewrite gate is minimal and correctmustRewrite checks AlwaysFlattenFragments and presence of any inline fragments. This is sufficient to trigger flattening in gRPC-like scenarios without affecting non-fragment selections.
360-381: Early exit with mustRewrite for object selections is appropriateThe object path now respects the AlwaysFlattenFragments gate. This maintains behavior parity while enabling the new “always flatten” mode consistently.
v2/pkg/engine/plan/datasource_configuration.go (3)
293-300: Delegation methods LGTMdataSourceConfiguration.UpstreamSchema() and .PlanningBehavior() correctly delegate to the factory, matching the new DataSource interface.
17-32: All PlannerFactory implementations updated
I’ve verified that everyFactory[T]in the v2 engine now implementsContext(),UpstreamSchema(...), andPlanningBehavior(), including:
- v2/pkg/engine/datasource/introspection_datasource/factory.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
- v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go
- v2/pkg/engine/datasource/staticdatasource/static_datasource.go
- v2/pkg/engine/plan/planner_test.go (FakeFactory)
No further changes are required.
256-269: DataSource interface widening validated
- The concrete wrapper
dataSourceConfiguration[T]implements the new methods:
•UpstreamSchema()at lines 293–294
•PlanningBehavior()at lines 297–298- No other
*DataSourcestruct types were found in the repositoryAll downstream implementors compile without changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (1)
3829-3859: Test mis-targets a scalar field; it doesn’t exercise interface-object rewrite logicThe case “field is an interface object with concrete type fragments” sets fieldName to "name", which is a scalar under User. Rewriter operates on the field selection with an abstract return type (“user” here). As written, this test does not actually exercise the intended rewrite path and will trivially pass.
Fix by targeting the “user” field, expecting a rewrite (the rewriter will add __typename for interface objects with concrete fragments and no typename):
- fieldName: "name", + fieldName: "user", ... - expectedOperation: ` + expectedOperation: ` query { __typename user { + __typename ... on User { name } } }`, - shouldRewrite: false, + shouldRewrite: true,
♻️ Duplicate comments (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
17-32: PlannerFactory interface extended; verify all concrete factories implement PlanningBehavior() and UpstreamSchema(...)Great API move. Centralizing planning flags and upstream schema on the Factory is the right place. Please ensure every Factory type now satisfies the updated interface.
Run this to confirm there are no missing implementations and no leftover references to the old interface:
#!/bin/bash set -euo pipefail echo "1) Check for old upstream schema surface still referenced" rg -n "DataSourceUpstreamSchema|DataSourcePlanningBehavior\(\)" || true echo echo "2) List all Factory types" rg -n "type .*Factory\[" -A 2 echo echo "3) Verify PlanningBehavior() on Factories" rg -n $'func \\([^)]*Factory\\[[^)]*\\]\\)\\s*PlanningBehavior\\(\\)' -A 1 || true echo echo "4) Verify UpstreamSchema(DataSourceConfiguration[T]) on Factories" rg -n $'func \\([^)]*Factory\\[[^)]*\\]\\)\\s*UpstreamSchema\\(\\s*dataSourceConfig\\s+plan\\.DataSourceConfiguration\\[[^)]*\\]\\)' -A 1 || true
🧹 Nitpick comments (4)
v2/pkg/engine/plan/datasource_configuration.go (2)
377-407: DataSourcePlanningBehavior: flags and docs LGTM; new AlwaysFlattenFragments is clear
- Renaming IncludeTypeNameFields -> AllowToPlanTypeNameFields improves semantics.
- AlwaysFlattenFragments fits the new rewrite behavior well.
Minor nit: the name “AllowToPlanTypeNameFields” reads a bit awkwardly; consider “AllowPlanningTypeName” or “PlanTypeNameFields” in a future iteration, if API stability permits.
440-448: Typo: Identifyable → IdentifiableThe interface name has a common typo. Renaming would be breaking, but you can add an alias to improve API ergonomics while maintaining compatibility.
Apply this addition outside the shown hunk (near the interface definition):
// Deprecated: use Identifiable. type Identifiable = Identifyablev2/pkg/engine/plan/abstract_selection_rewriter.go (2)
80-92: Constructor: add nil DataSource guardFetching UpstreamSchema() on a nil DataSource will panic. Add a defensive nil check to fail fast with a clear error.
func newFieldSelectionRewriter(operation *ast.Document, definition *ast.Document, dsConfiguration DataSource) (*fieldSelectionRewriter, error) { + if dsConfiguration == nil { + return nil, errors.New("nil DataSource") + } upstreamDefinition, ok := dsConfiguration.UpstreamSchema() if !ok { return nil, ErrNoUpstreamSchema } return &fieldSelectionRewriter{ operation: operation, definition: definition, upstreamDefinition: upstreamDefinition, dsConfiguration: dsConfiguration, alwaysRewrite: dsConfiguration.PlanningBehavior().AlwaysFlattenFragments, }, nil }
639-646: TODO: disappearing paths after rewriteConsider tracking lost paths to aid downstream consumers of changedRefs or documenting that disappearance is expected in these scenarios. Not urgent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/plan/abstract_selection_rewriter.go(9 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_info.go(5 hunks)v2/pkg/engine/plan/abstract_selection_rewriter_test.go(102 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/plan/abstract_selection_rewriter_info.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (3)
v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)pkg/introspection/introspection.go (1)
TypeName(42-44)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourcePlanningBehavior(378-407)
v2/pkg/engine/plan/datasource_configuration.go (6)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
Planner(37-40)v2/pkg/engine/plan/planner.go (1)
Planner(15-25)v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/plan/federation_metadata.go (1)
FederationMetaData(10-16)execution/graphql/schema.go (1)
TypeFields(19-22)v2/pkg/engine/plan/type_field.go (1)
TypeFields(13-13)
v2/pkg/engine/plan/abstract_selection_rewriter.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(256-269)
⏰ 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.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (9)
v2/pkg/engine/plan/datasource_configuration.go (3)
35-37: Federation metadata docs improvedThe revised wording is clearer about entity interfaces and @key/@requires/@provides. Looks good.
261-299: DataSource now exposes UpstreamSchema() and PlanningBehavior() via configuration delegationThe delegation pattern through dataSourceConfiguration is clean and aligns the rest of the plan layer (rewriter/visitors) with a single source of truth.
415-438: DownstreamResponseFieldAlias explanation is thorough and actionableThe conditions and example make the contract unambiguous.
v2/pkg/engine/plan/abstract_selection_rewriter_test.go (2)
47-48: Good guard: assert fieldRef is foundEnsures the test targets the intended field before running the rewriter.
55-57: Constructor usage updated with error handlingUsing the new (rewriter, error) signature prevents silent failures.
v2/pkg/engine/plan/abstract_selection_rewriter.go (4)
173-184: Force-rewrite gate (AlwaysFlattenFragments) is correctly centralizedThe mustRewrite guard at the top of all “needsRewrite” paths ensures consistent behavior across union/object/interface. This matches the new PlanningBehavior flag.
Also applies to: 360-364, 429-433
276-284: Stricter error on missing selection set is appropriateReturning ErrFieldHasNoSelectionSet (and bubbling it with context) prevents silent misbehavior. Good change.
458-466: Interface Object handling comment clarifies __typename additionClear rationale for adding __typename before flattening interface-object selections.
15-21: No stale error references foundThe search for
FieldDoesntHaveSelectionSetErr,InlineFragmentDoesntHaveSelectionSetErr, andInlineFragmentTypeIsNotExistsErrreturned no matches. All old identifiers have been successfully replaced.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
1820-1828: PlanningBehavior: gRPC-driven AlwaysFlattenFragments is correct and includes provider caseGood fix. AlwaysFlattenFragments now turns on when either a concrete gRPC client or a provider is configured, aligning fragment rewriting enforcement with all gRPC-backed setups.
🧹 Nitpick comments (2)
v2/pkg/engine/plan/datasource_configuration.go (2)
35-51: Tighten docs; fix GraphQL/gRPC casing and wordingMinor doc polish for precision and consistency (GraphQL/gRPC casing, clearer phrasing).
Apply:
-// FederationMetaData has federation-specific configuration for entity interfaces and -// the @key, @requires, @provides directives. +// FederationMetaData has federation-specific configuration for entity interfaces and +// the @key, @requires, and @provides directives. -// RootNodes defines the nodes where the responsibility of the DataSource begins. -// RootNode is a node from which we could start a query or a subquery. +// RootNodes define the nodes where the responsibility of the DataSource begins. +// A RootNode is a node from which we can start a query or a subquery. -// ChildNodes describes additional fields, which are requested along with fields that the datasource has. -// They're always required for Graphql datasources because each field could have its own datasource. -// For a flat datasource (HTTP/REST or GRPC) we cannot request fewer fields, as we always get a full response. -// For a federation, ChildNodes contain non-entity type fields and interface type fields. -// Unions shouldn't be present in the child or root nodes. +// ChildNodes describe additional fields that are requested alongside fields owned by the DataSource. +// They're always required for GraphQL data sources because each field can have its own data source. +// For flat data sources (HTTP/REST or gRPC), we cannot request fewer fields, as we always get a full response. +// For federation, ChildNodes contain non-entity type fields and interface type fields. +// Union types shouldn't be present in ChildNodes or RootNodes.
377-408: Clarify PlanningBehavior docs; avoid ambiguity around “gRPC” data sourcesDocs are generally good. One nit: “static data sources (REST, static, gRPC)” can be read as implying all gRPC sources are static, which is misleading given GraphQL-over-gRPC is dynamic in this repo. Consider narrowing the examples.
Suggested wording:
-// On dynamic data sources (GraphQL, SQL) this should be set to true, -// and for static data sources (REST, static, gRPC) it should be false. +// On dynamic data sources (e.g., GraphQL, SQL) set this to true. +// On flat/opaque data sources (e.g., REST, static payloads) set this to false.The rest (AllowPlanningTypeName, AlwaysFlattenFragments) reads well and matches behavior elsewhere in the PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(1 hunks)v2/pkg/engine/datasource/introspection_datasource/factory.go(1 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go(1 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)v2/pkg/engine/plan/path_builder_visitor.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go
- v2/pkg/engine/datasource/introspection_datasource/factory.go
- v2/pkg/engine/plan/path_builder_visitor.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/pkg/engine/plan/datasource_configuration.go (9)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
Planner(54-63)v2/pkg/engine/datasource/introspection_datasource/planner.go (1)
Planner(16-25)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
Planner(37-40)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
Planner(55-96)v2/pkg/engine/plan/planner.go (1)
Planner(15-25)v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/plan/federation_metadata.go (1)
FederationMetaData(10-16)execution/graphql/schema.go (1)
TypeFields(19-22)v2/pkg/engine/plan/type_field.go (1)
TypeFields(13-13)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (3)
v2/pkg/engine/datasource/introspection_datasource/factory.go (1)
Factory(13-15)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource.go (1)
Factory(290-294)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
Factory(15-15)
⏰ 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.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (2)
v2/pkg/engine/plan/datasource_configuration.go (2)
261-263: DataSource interface: wiring UpstreamSchema and PlanningBehavior is soundExposing these via DataSource and delegating to the factory is consistent with the new architecture.
297-300: PlanningBehavior delegation: LGTMSimple and correct delegation to the factory. Keeps the DataSource facade thin.
ysmolski
deleted the
yury/eng-7487-enforce-rewriting-abstract-fragments-for-grpc-datasource
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.220](v2.0.0-rc.219...v2.0.0-rc.220) (2025-08-13) ### Features * always rewrite abstract fragments for gRPC ([#1268](#1268)) ([ebe1e53](ebe1e53)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
I have used DataSourcePlanningBehavior for the flag that enforces rewriting of fragments.
I had to refactor the code so that structure is being specified in the Factory's method instead of Planner.
Access to this data happens via DataSource.PlanningBehavior()
BEGIN_COMMIT_OVERRIDE
feat: always rewrite abstract fragments for grpc
END_COMMIT_OVERRIDE
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Checklist