fix: refactor CoordinateDependencies, FetchReasons#1293
Conversation
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughConsolidates per-fetch metadata into a new FetchInfo model, adds optional fetch-reason generation/propagation (BuildFetchReasons), renames KeyConditionCoordinate → FieldCoordinate, centralizes fetch-reason indices in DataSourceMetadata, updates visitor/loader/postprocess to use FetchInfo, enriches federation test metadata, and simplifies Makefile golangci-lint invocations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (12)
v2/pkg/engine/plan/configuration.go (1)
33-41: Enforce config invariants for BuildFetchReasons.Docs state BuildFetchReasons requires DisableIncludeInfo=false and DisableIncludeFieldDependencies=false, but this isn’t enforced. Add a validation in planner construction (or here via a Validate method) to fail fast or auto-correct when misconfigured.
Would you like me to draft a small Validate() helper for Configuration and wire it in NewPlanner?
v2/pkg/engine/datasourcetesting/datasourcetesting.go (1)
36-36: Nice option wiring; invariants held.WithFetchReasons enabling withFieldInfo/withFieldDependencies and setting BuildFetchReasons keeps the planner/loader in sync. Consider adding a short doc comment to WithFetchReasons() to clarify it implies those toggles.
Also applies to: 74-75, 79-85, 160-163
execution/engine/execution_engine_test.go (1)
231-236: Good coverage for fetch-reasons propagation.The helper and expectations exercise planner wiring and loader encoding. Consider adding a GET-based datasource case to assert we either (a) switch to POST or (b) encode fetch_reasons appropriately for GET (query param), to avoid regressions.
Also applies to: 268-274, 834-883, 4544-4545
v2/pkg/engine/plan/visitor.go (1)
1336-1352: Tighten gating and remove redundant IncludeInfo check.CoordinateDependencies/FetchReasons rely on dependency tracking; compute them only when field dependencies are enabled. Also the inner DisableIncludeInfo check duplicates the outer one.
- if !v.Config.DisableIncludeInfo { - singleFetch.Info.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) - - if v.Config.BuildFetchReasons { - singleFetch.Info.FetchReasons = v.buildFetchReasons(internal.fetchID) - if singleFetch.Info.FetchReasons != nil { - dsConfig := v.planners[internal.fetchID].DataSourceConfiguration() - indices := make([]int, 0, len(singleFetch.Info.FetchReasons)) - for i, fr := range singleFetch.Info.FetchReasons { - if dsConfig.RequiresFetchReason(fr.TypeName, fr.FieldName) { - indices = append(indices, i) - } - } - singleFetch.Info.RequireFetchReason = indices - } - } - } + if !v.Config.DisableIncludeFieldDependencies { + singleFetch.Info.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) + if v.Config.BuildFetchReasons { + singleFetch.Info.FetchReasons = v.buildFetchReasons(internal.fetchID) + if len(singleFetch.Info.FetchReasons) > 0 { + dsConfig := v.planners[internal.fetchID].DataSourceConfiguration() + indices := make([]int, 0, len(singleFetch.Info.FetchReasons)) + for i, fr := range singleFetch.Info.FetchReasons { + if dsConfig.RequiresFetchReason(fr.TypeName, fr.FieldName) { + indices = append(indices, i) + } + } + singleFetch.Info.RequireFetchReason = indices + } + } + }v2/pkg/engine/resolve/fetch.go (6)
103-105: Rename method to match interface and avoid “Get” prefixAlign with the proposed interface change.
-func (s *SingleFetch) GetInfo() *FetchInfo { - return s.Info -} +func (s *SingleFetch) Info() *FetchInfo { + return s.Info +}
180-182: Rename to Info for consistency across implementations-func (b *BatchEntityFetch) GetInfo() *FetchInfo { - return b.Info -} +func (b *BatchEntityFetch) Info() *FetchInfo { + return b.Info +}
227-229: Rename to Info for consistency across implementations-func (e *EntityFetch) GetInfo() *FetchInfo { - return e.Info -} +func (e *EntityFetch) Info() *FetchInfo { + return e.Info +}
262-264: Rename to Info for consistency across implementations-func (p *ParallelListItemFetch) GetInfo() *FetchInfo { - return p.Fetch.Info -} +func (p *ParallelListItemFetch) Info() *FetchInfo { + return p.Fetch.Info +}
396-418: Clarify immutability and add safe helpers for propagated reasonsThese fields look planner-derived and read-only at execution time. Please document immutability and provide helpers to avoid out-of-bounds on
RequireFetchReasonand to centralize propagation logic.
- Add doc: “FetchInfo is immutable after planning; all fields may be nil/empty depending on flags.”
- Provide helpers (outside this hunk) to validate and materialize propagated reasons:
// Suggested additions near FetchInfo: // PropagatedReasons returns the subset of FetchReasons referenced by RequireFetchReason. func (fi *FetchInfo) PropagatedReasons() []FetchReason { if fi == nil || len(fi.RequireFetchReason) == 0 || len(fi.FetchReasons) == 0 { return nil } out := make([]FetchReason, 0, len(fi.RequireFetchReason)) for _, idx := range fi.RequireFetchReason { if idx >= 0 && idx < len(fi.FetchReasons) { out = append(out, fi.FetchReasons[idx]) } } return out } // Validate checks internal invariants; call once after building in the planner. func (fi *FetchInfo) Validate() error { if fi == nil { return nil } for i, idx := range fi.RequireFetchReason { if idx < 0 || idx >= len(fi.FetchReasons) { return fmt.Errorf("RequireFetchReason[%d]=%d out of range [0,%d)", i, idx, len(fi.FetchReasons)) } } return nil }Also ensure
DataSourceInfo()methods tolerate a nilInfo(or assert early during planning). Example guard:func (s *SingleFetch) DataSourceInfo() DataSourceInfo { if s.Info == nil { return DataSourceInfo{} } return DataSourceInfo{ID: s.Info.DataSourceID, Name: s.Info.DataSourceName} }
24-25: Rename Fetch.GetInfo() to Info() and enforce non-nil contract- GetInfo() *FetchInfo + Info() *FetchInfo // must be non-nil after planningUpdate all callsites of
GetInfo()(e.g. loader.go, fetchtree.go, resolve_test.go, postprocess/deduplicate_single_fetches.go, resolvable.go) to useInfo()and ensure the non-nil guarantee is documented and upheld.v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (2)
3114-3153: Add a guard to validate RequireFetchReason indices.Small safety net for future edits: assert that each index in RequireFetchReason is within bounds of FetchReasons to avoid brittle expectations.
You can add a helper and call it right after constructing the plan (example):
@@ return &plan.SynchronousResponsePlan{ Response: &resolve.GraphQLResponse{ @@ }, } } +// test helper (place near other helpers in this file or a common test util) +func assertRequireIndicesValid(t *testing.T, info *resolve.FetchInfo) { + if info == nil { + return + } + for _, i := range info.RequireFetchReason { + if i < 0 || i >= len(info.FetchReasons) { + t.Fatalf("RequireFetchReason index %d out of bounds, reasons=%d", i, len(info.FetchReasons)) + } + } +}And invoke it wherever Info is set:
@@ - Info: &resolve.FetchInfo{ + Info: &resolve.FetchInfo{ DataSourceID: "user.service", @@ - }, + }, + // sanity check for indices + // assertRequireIndicesValid(t, <THIS_FETCH>.Info) // reference current fetch variable if available
3192-3263: CoordinateDependencies and FetchReasons for fetch(1) look right; consider being explicit with RequireFetchReason.
- Dependencies from Address.country/city → Address.id (fetch 0) are modeled correctly.
- Optional: add RequireFetchReason: []int{1,2} to make it obvious that only the requires (“city”, “country”) must propagate. Purely readability; behavior is unchanged.
Info: &resolve.FetchInfo{ DataSourceID: "address-enricher.service", DataSourceName: "address-enricher.service", @@ - OperationType: ast.OperationTypeQuery, + OperationType: ast.OperationTypeQuery, + RequireFetchReason: []int{1, 2},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(12 hunks)v2/pkg/engine/datasourcetesting/datasourcetesting.go(3 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/visitor.go(1 hunks)v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go(0 hunks)v2/pkg/engine/postprocess/deduplicate_single_fetches.go(1 hunks)v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go(6 hunks)v2/pkg/engine/resolve/fetch.go(6 hunks)v2/pkg/engine/resolve/fetchtree.go(3 hunks)v2/pkg/engine/resolve/loader.go(1 hunks)
💤 Files with no reviewable changes (1)
- v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go
🔇 Additional comments (8)
v2/pkg/engine/resolve/fetchtree.go (1)
219-220: Confirm Info is always populated when building query plans.These paths dereference f.Info.* unconditionally. If a caller builds a query plan while DisableIncludeInfo is true, this will panic. If we rely on includeQueryPlan ⇒ includeInfo, please assert that invariant at entry, or guard against nil.
Also applies to: 234-235, 249-250
v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go (1)
100-113: Tests correctly model CoordinateDependencies on FetchInfo.These additions validate ID remapping through Info.CoordinateDependencies after deduplication. Once the write-back bug is fixed in the processor, these should pass deterministically.
Also applies to: 128-139, 154-168, 183-197, 230-241, 256-270
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (6)
3089-3153: Good addition: Response.Info and FetchInfo on the first fetch are coherent.
- GraphQLResponseInfo.OperationType and FetchInfo’s DataSourceID/Name, RootFields, and OperationType are consistent with the query.
- RequireFetchReason points to indices 1,2,3 which correctly map to Address.id/line1/line2 in FetchReasons.
3316-3385: Solid: zip/line3 dependency fan-in is modeled correctly.
- zip depends on country/city from fetch(1) and id from fetch(0); flags IsRequires/IsKey are consistent.
3431-3465: Variables completeness for final Address entity fetch looks good.
- Including line1, line2, line3, zip, id aligns with account.service requirements and avoids follow-up hops.
3472-3550: FetchInfo for fullAddress is consistent and user-reasoned.
- CoordinateDependencies reflect the mixed origins (user.service and address.service) and mark IsUserRequested on the target coordinate.
3556-3633: Per-field provenance (FieldInfo/SourceName) is precise and valuable.
- ExactParentTypeName, ParentTypeNames, and Source IDs/Names match the fetches; this should greatly aid debugging/authorization.
3679-3681: Nice toggle: WithFetchReasons only for tests that assert reasons.
- Keeps other tests lean. Ensure planner config indeed disables building reasons when this option is absent to avoid perf regressions.
Would you like me to scan the repo for planner instantiations and verify BuildFetchReasons is default-off outside these tests?
ysmolski
changed the title
Move them into the FetchInfo struct. Add the BuildFetchReasons option to the planner, that allows other parts to enable building the FetchReason list.
ysmolski
force-pushed
the
yury/eng-7769-refactor-data-hierarchy
branch
from
c61cfa6 to
fe0d374
Compare
ysmolski
changed the title
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 (2)
execution/engine/execution_engine_test.go (1)
845-851: Update comment in fetch.go to use “fetch_reasons”
The only singular occurrence is in the comment at v2/pkg/engine/resolve/fetch.go (line 390) – code and tests already use the plural key.v2/pkg/engine/plan/datasource_configuration.go (1)
70-78: Avoid exposing mutable internal maps via the interface. Return a read-only copy and add a boolean helper.Returning the backing map invites accidental mutation and data races across planner/engine stages. Prefer a copy and a predicate for fast checks.
Apply:
type NodesInfo interface { HasRootNode(typeName, fieldName string) bool HasExternalRootNode(typeName, fieldName string) bool HasRootNodeWithTypename(typeName string) bool HasChildNode(typeName, fieldName string) bool HasExternalChildNode(typeName, fieldName string) bool HasChildNodeWithTypename(typeName string) bool - RequireFetchReasons() map[FieldCoordinate]struct{} + RequireFetchReasons() map[FieldCoordinate]struct{} + // RequiresFetchReason reports whether a coordinate is marked. + RequiresFetchReason(typeName, fieldName string) bool }And implement the helper (see comment at Lines 211-213 for the copy-return).
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/loader.go (1)
1602-1617: Resolves prior nil/index safety issue; propagation is now guardedThe switch to Info.PropagatedFetchReasons with nil/length checks addresses the earlier panic risk.
v2/pkg/engine/postprocess/deduplicate_single_fetches.go (1)
54-61: Fix validated: no more mutations on range copies; nil-safe access to FetchInfoThe switch to indexed loops and the nil-guard addresses the previous persistence and NPE issues.
🧹 Nitpick comments (19)
v2/pkg/engine/plan/configuration.go (1)
34-36: Add a unit test for BuildFetchReasons preconditions
Runtime guards in visitor.go already return early when DisableIncludeInfo or DisableIncludeFieldDependencies is true, so no additional gating is needed. Add a unit test that sets BuildFetchReasons=true with either disable flag enabled and asserts that FetchReasons is not generated.v2/pkg/engine/postprocess/postprocess.go (1)
183-191: Nil-safe switch to FetchInfo; consider cheaper de-dup for large fetch setsThe nil guard is good. If responses can have many fetches, slices.Contains(list, dsInfo) is O(n²). A tiny map set would make this O(n).
- var list = make([]resolve.DataSourceInfo, 0, len(fetches)) + var ( + list = make([]resolve.DataSourceInfo, 0, len(fetches)) + seen = make(map[resolve.DataSourceInfo]struct{}, len(fetches)) + ) for _, fetch := range fetches { info := fetch.Fetch.FetchInfo() if info != nil { dsInfo := resolve.DataSourceInfo{ID: info.DataSourceID, Name: info.DataSourceName} - if !slices.Contains(list, dsInfo) { + if _, ok := seen[dsInfo]; !ok { list = append(list, dsInfo) + seen[dsInfo] = struct{}{} } } }v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go (2)
78-79: Avoid slice aliasing when copying DependsOnFetchIDs into new fetch structsShallow-copying FetchDependencies carries the same backing array for DependsOnFetchIDs; later mutations can unintentionally affect other instances. Deep-copy the slice before assigning.
- return &resolve.BatchEntityFetch{ - FetchDependencies: fetch.FetchDependencies, - Info: fetch.Info, + deps := fetch.FetchDependencies + if len(deps.DependsOnFetchIDs) > 0 { + deps.DependsOnFetchIDs = append([]int(nil), deps.DependsOnFetchIDs...) + } + return &resolve.BatchEntityFetch{ + FetchDependencies: deps, + Info: fetch.Info,
123-124: Repeat: deep-copy DependsOnFetchIDs to prevent shared backing array between SingleFetch and EntityFetchSame concern as above. Copy the slice before assignment.
- return &resolve.EntityFetch{ - FetchDependencies: fetch.FetchDependencies, - Info: fetch.Info, + deps := fetch.FetchDependencies + if len(deps.DependsOnFetchIDs) > 0 { + deps.DependsOnFetchIDs = append([]int(nil), deps.DependsOnFetchIDs...) + } + return &resolve.EntityFetch{ + FetchDependencies: deps, + Info: fetch.Info,v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go (1)
102-114: Optional: Populate Coordinate for better introspection/debuggability.You only set DependsOn origins; setting the top-level Coordinate (type/field) for each dependency will make diffs and tooling clearer without affecting behavior.
execution/engine/execution_engine_test.go (1)
837-886: Reduce brittleness of body comparisons including extensions.Comparing raw JSON strings is fragile as field order may change. Consider normalizing in the round-tripper (parse to map and deep-compare) or switch these to JSONEq-style assertions for request bodies.
- expectedBody: `{"query":"{hero {name}}","extensions":{"fetch_reasons":[...]}}`, + expectedBody: mustJSON(`{"query":"{hero {name}}","extensions":{"fetch_reasons":[...]}}`),And in the test helper, decode and compare structurally.
v2/pkg/engine/plan/visitor.go (1)
1344-1359: Avoid emitting empty PropagatedFetchReasons slices.Setting an empty (non-nil) slice can lead to sending empty extensions downstream if the loader checks only for nil. Set it only when non-empty.
- singleFetch.Info.PropagatedFetchReasons = propagated + if len(propagated) > 0 { + singleFetch.Info.PropagatedFetchReasons = propagated + }v2/pkg/engine/plan/datasource_configuration.go (4)
100-109: Pre-size requireFetchReasons to reduce allocations.You already know counts per node; sum them and size the map up-front.
func (d *DataSourceMetadata) InitNodesIndex() { @@ - d.rootNodesIndex = make(map[string]fieldsIndex, len(d.RootNodes)) - d.childNodesIndex = make(map[string]fieldsIndex, len(d.ChildNodes)) - d.requireFetchReasons = make(map[FieldCoordinate]struct{}) + d.rootNodesIndex = make(map[string]fieldsIndex, len(d.RootNodes)) + d.childNodesIndex = make(map[string]fieldsIndex, len(d.ChildNodes)) + // Pre-size requireFetchReasons + frCount := 0 + for i := range d.RootNodes { + frCount += len(d.RootNodes[i].FetchReasonFields) + } + for i := range d.ChildNodes { + frCount += len(d.ChildNodes[i].FetchReasonFields) + } + d.requireFetchReasons = make(map[FieldCoordinate]struct{}, frCount)
124-124: Prefer named struct literals for resilience.Avoid positional literals in case FieldCoordinate changes order.
- d.requireFetchReasons[FieldCoordinate{typeName, name}] = struct{}{} + d.requireFetchReasons[FieldCoordinate{TypeName: typeName, FieldName: name}] = struct{}{}
129-134: Also pre-size child indices for parity with root indices.Minor perf/readability consistency.
- d.childNodesIndex[typeName] = fieldsIndex{ - fields: make(map[string]struct{}), - externalFields: make(map[string]struct{}), - } + d.childNodesIndex[typeName] = fieldsIndex{ + fields: make(map[string]struct{}, len(d.ChildNodes[i].FieldNames)), + externalFields: make(map[string]struct{}, len(d.ChildNodes[i].ExternalFieldNames)), + }
143-143: Same named-literal nit here.- d.requireFetchReasons[FieldCoordinate{typeName, name}] = struct{}{} + d.requireFetchReasons[FieldCoordinate{TypeName: typeName, FieldName: name}] = struct{}{}v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (6)
3113-3176: FetchReasons/PropagatedFetchReasons assertions risk flakiness due to ordering.If the planner ever changes traversal order, these slices may reorder. Consider stabilizing ordering (e.g., sort by TypeName+FieldName+flags) before comparing, or sort when building FetchInfo. Do you want a small helper to sort in the test harness?
3355-3407: Consider deterministic ordering of CoordinateDependencies.Recommend sorting DependsOn by (FetchID, Subgraph, Coordinate.TypeName, Coordinate.FieldName) to keep tests stable.
3409-3441: Propagated vs. full FetchReasons separation is clear.Nice separation; just apply the same stabilization advice as above.
3593-3668: FieldInfo assertions are thorough; one nit.Source.IDs/Names may contain duplicates when multiple datasources merge. If that’s possible, dedupe before asserting to avoid future planner changes breaking tests.
16263-16279: FieldCoordinate usage in conditional key conditions looks right.Rename from KeyCoordinate → FieldCoordinate is reflected consistently; paths map to the provided FieldPath. Consider adding one negative test (unknown field) to catch regressions in validation.
17204-17219: Second conditional-key variant also consistent.Same note on a small negative test for invalid coordinates to harden validation.
v2/pkg/engine/resolve/fetch.go (2)
102-104: Clarify FetchInfo nilability at call sites (doc on methods helps)To reduce ambiguity for readers, add a short doc comment to each implementation noting it may return nil depending on planner flags (e.g., BuildFetchReasons).
+// FetchInfo returns derived metadata for this fetch. +// May be nil if planner flags didn't build it (e.g., BuildFetchReasons=false). func (s *SingleFetch) FetchInfo() *FetchInfo { return s.Info } +// FetchInfo returns derived metadata for this batched entity fetch. +// May be nil if planner flags didn't build it. func (b *BatchEntityFetch) FetchInfo() *FetchInfo { return b.Info } +// FetchInfo returns derived metadata for this entity fetch. +// May be nil if planner flags didn't build it. func (e *EntityFetch) FetchInfo() *FetchInfo { return e.Info } +// FetchInfo returns derived metadata for the underlying single fetch. +// May be nil if planner flags didn't build it. func (p *ParallelListItemFetch) FetchInfo() *FetchInfo { return p.Fetch.Info }Also applies to: 172-174, 212-214, 240-242
370-392: Tighten FetchInfo doc; consider single source of truth for QueryPlanSmall wording nit in the comment and a potential duplication:
QueryPlanexists on bothFetchConfigurationandFetchInfo. If they can diverge, decide the authoritative location or ensure they reference the same instance.-// FetchInfo contains additional (derived) information about the fetch. Some fields of this -// structure may be not generated depending on the planner flags. +// FetchInfo contains additional (derived) information about the fetch. +// Some fields of this structure may not be populated depending on planner flags. +// Note: QueryPlan also exists on FetchConfiguration; ensure a single source of truth. type FetchInfo struct {Action items:
- Confirm whether
FetchConfiguration.QueryPlanis still needed now thatFetchInfo.QueryPlanexists.- If both remain, document which one is authoritative and keep them in sync where they’re set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.workis excluded by!**/*.work
📒 Files selected for processing (17)
execution/Makefile(1 hunks)execution/engine/execution_engine_test.go(5 hunks)v2/Makefile(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(14 hunks)v2/pkg/engine/datasourcetesting/datasourcetesting.go(3 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/datasource_configuration.go(6 hunks)v2/pkg/engine/plan/federation_metadata.go(1 hunks)v2/pkg/engine/plan/node_selection_visitor.go(1 hunks)v2/pkg/engine/plan/visitor.go(3 hunks)v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go(2 hunks)v2/pkg/engine/postprocess/deduplicate_single_fetches.go(1 hunks)v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go(6 hunks)v2/pkg/engine/postprocess/postprocess.go(1 hunks)v2/pkg/engine/resolve/fetch.go(6 hunks)v2/pkg/engine/resolve/fetchtree.go(3 hunks)v2/pkg/engine/resolve/loader.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-24)FetchInfo(372-392)
v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go (1)
v2/pkg/engine/resolve/fetch.go (1)
FetchDependencies(108-111)
v2/pkg/engine/postprocess/postprocess.go (2)
v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-24)FetchInfo(372-392)v2/pkg/engine/resolve/loader.go (1)
DataSourceInfo(46-49)
v2/pkg/engine/postprocess/deduplicate_single_fetches.go (1)
v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-24)FetchInfo(372-392)
v2/pkg/engine/plan/datasource_configuration.go (2)
v2/pkg/engine/plan/federation_metadata.go (1)
FieldCoordinate(89-92)pkg/introspection/introspection.go (1)
TypeName(42-44)
v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go (1)
v2/pkg/engine/resolve/fetch.go (1)
FetchInfo(372-392)
v2/pkg/engine/resolve/fetch.go (3)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(273-286)v2/pkg/engine/resolve/datasource.go (1)
DataSource(12-15)pkg/graphql/request.go (1)
OperationType(21-21)
v2/pkg/engine/plan/visitor.go (2)
v2/pkg/engine/resolve/fetch.go (3)
FetchInfo(372-392)QueryPlan(248-251)FetchReason(361-368)v2/pkg/engine/plan/federation_metadata.go (1)
FieldCoordinate(89-92)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (8)
v2/pkg/engine/resolve/response.go (1)
GraphQLResponseInfo(44-46)v2/pkg/engine/resolve/fetch.go (7)
FetchInfo(372-392)GraphCoordinate(394-398)FetchReason(361-368)SingleFetch(88-96)SingleFetch(150-152)FetchDependencies(108-111)FetchConfiguration(267-299)v2/pkg/engine/resolve/variables.go (3)
Variables(27-27)Variable(21-25)ResolvableObjectVariable(160-162)v2/pkg/engine/resolve/variables_renderer.go (1)
NewGraphQLVariableResolveRenderer(335-340)v2/pkg/engine/resolve/node_object.go (5)
Object(8-16)Object(30-32)Field(89-98)FieldInfo(130-152)TypeFieldSource(168-171)v2/pkg/engine/resolve/fetchtree.go (2)
ObjectPath(40-45)SingleWithPath(69-82)v2/pkg/engine/datasourcetesting/datasourcetesting.go (1)
WithFetchReasons(79-85)v2/pkg/engine/plan/federation_metadata.go (1)
FieldCoordinate(89-92)
🔇 Additional comments (25)
v2/Makefile (1)
18-18: Drop manual config and fmt availability checks: golangci-lint auto-discovers.golangci.ymlfromv2/upward (golangci-lint.run), and thefmtsubcommand was introduced in golangci-lint v2 (CI uses v2.4.0) (ldez.github.io, golangci-lint.run).v2/pkg/engine/plan/node_selection_visitor.go (1)
48-48: LGTM: reuse of rewrittenFieldRefs slice is efficientResetting via slicing in EnterDocument avoids reallocations.
v2/pkg/engine/resolve/fetchtree.go (1)
219-220: Approve Info initialization on EntityFetch and BatchEntityFetch
Info is always set in the post-processing constructors (createEntityFetch and createEntityBatchFetch initializeInfofromfetch.Info), so no additional nil checks are required.v2/pkg/engine/datasourcetesting/datasourcetesting.go (3)
36-36: Test option flag looks goodwithFetchReasons added to options struct is straightforward.
74-76: Nice: WithFieldDependencies implies WithFieldInfoAvoids misconfiguring tests that rely on field info presence.
79-85: WithFetchReasons bundles prerequisites correctlySetting field info + dependencies together prevents inconsistent planner config in tests.
v2/pkg/engine/postprocess/deduplicate_single_fetches.go (1)
50-57: Always refresh CoordinateDependencies: drop theif !replaced { continue }guard so coords can’t fall out-of-sync with DependsOnFetchIDs- if !replaced { - continue - } - - info := root.ChildNodes[i].Item.Fetch.FetchInfo() - if info == nil { - continue - } + info := root.ChildNodes[i].Item.Fetch.FetchInfo() + if info == nil { + continue + }If you need a fast-path, track a separate “coordsNeedUpdate” flag during the scan and only skip when both dependency sets are empty.
v2/pkg/engine/plan/federation_metadata.go (1)
85-92: Ignore duplicate-type warning: FieldCoordinate is only defined in federation_metadata.go; other files reference this single definition.Likely an incorrect or invalid review comment.
v2/pkg/engine/postprocess/deduplicate_single_fetches_test.go (1)
102-114: Keep dependency origin ordering deterministic after dedup.These assertions rely on slice order in Info.CoordinateDependencies[x].DependsOn. If the dedup step rewrites IDs without stable sorting, tests can become flaky. Please ensure the post-dedup pass sorts origins (e.g., by FetchID) or sort in the test before asserting.
Also applies to: 129-140, 155-169, 185-198, 231-242, 257-271
execution/engine/execution_engine_test.go (3)
234-239: Nice toggle helper.withFetchReasons() cleanly scopes the new behavior to specific tests.
271-277: Good: planner and resolver are gated by the same option.Setting plannerConfig.BuildFetchReasons and ResolverOptions.PropagateFetchReasons together avoids config drift.
4361-4364: Fetch-reasons payload looks correct and ordered.Order matches type/field sort (Admin.some, then User.id) and includes by_subgraphs/is_key as expected.
v2/pkg/engine/plan/visitor.go (2)
1328-1343: Early returns keep the hot path lean.Bailing out when DisableIncludeInfo/DisableIncludeFieldDependencies is set avoids unnecessary allocations/work.
1426-1509: Solid dedup/indexing of fetch reasons.Using FieldCoordinate as the map key with final sort/compact gives deterministic output and avoids duplicates; good choice.
v2/pkg/engine/plan/datasource_configuration.go (4)
57-58: Good centralization of fetch-reason lookup.Unexported map keyed by FieldCoordinate is a clear, O(1) membership store.
81-83: fieldsIndex shape looks good.Removing fetchReasonFields from this struct simplifies the indices.
113-115: Nice: capacity hints for root indices.Pre-sizing maps to FieldNames/ExternalFieldNames reduces churn.
70-78: Verify no legacyRequiresFetchReasoncalls and no map mutations onRequireFetchReasons()
• Search for any remaining uses of the old API:rg -nP '\bRequiresFetchReason\s*\(' --glob '*.go' --glob '!vendor/**'• Inspect all call sites of
RequireFetchReasons()to ensure nobody writes to or deletes from the returned map:rg -nP -C2 'RequireFetchReasons\(\)' --glob '*.go' --glob '!vendor/**' \ | rg -nP '\[[^]]+\]\s*=|delete\s*\('Also apply these checks around lines 211–213.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (6)
3089-3091: Good addition of GraphQLResponseInfo.Explicitly asserting OperationType improves plan introspection in tests.
3186-3215: Entity variable shape looks correct.The _entities payload includes __typename and key (id) for Address. LGTM.
3231-3266: CoordinateDependencies are precise and readable.Nice use of FetchID/Subgraph/Coordinate to explain “country/city” dependency on Address.id.
3319-3331: Verify preconditions for propagated variables.country and city are used as inputs to the next fetch. Please ensure they are always present (non-null) in the prior fetch result to avoid spurious null-skips in SetTemplateOutputToNullOnVariableNull paths.
3339-3354: Info block completeness.Including RootFields and OperationType per fetch is helpful for tracing; matches the new FetchInfo contract.
3716-3716: Good toggle coverage via WithFetchReasons().Ensures FetchReasons/CoordinateDependencies are only built when requested.
v2/pkg/engine/resolve/fetch.go (1)
160-166: Carrying Info on Batch/Entity fetches is consistent with SingleFetchThis makes downstream consumption uniform. Good change.
Also applies to: 200-206
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/pkg/engine/resolve/fetch.go (1)
203-209: Same Info invariants as BatchEntityFetch
Duplicate of the earlier suggestion: document that Info is read-only post-planning and may be nil.- Info *FetchInfo + // Info is read-only after planning and may be nil depending on planner flags. + Info *FetchInfo
🧹 Nitpick comments (5)
v2/pkg/engine/resolve/fetch.go (5)
23-26: FetchInfo() addition: solid; consider compile-time assertions + immutability noteDoc about possibly returning nil looks good. Consider:
- Add compile-time interface assertions to catch regressions.
- Clarify that returned FetchInfo must be treated as read-only after planning to avoid accidental mutations/data races across engine stages.
Add near the bottom of this file (anywhere at package scope):
var ( _ Fetch = (*SingleFetch)(nil) _ Fetch = (*BatchEntityFetch)(nil) _ Fetch = (*EntityFetch)(nil) _ Fetch = (*ParallelListItemFetch)(nil) )
163-169: Document Info field invariants (read-only, may be nil)
To prevent accidental mutation by downstream stages or external packages, document invariants on Info right where it’s declared.- Info *FetchInfo + // Info is read-only after planning and may be nil depending on planner flags. + Info *FetchInfoIf tests rely on mutating Info, consider keeping it exported but provide helper constructors to populate it rather than modifying in place.
243-245: Guard against nil p.Fetch to avoid panic
ParallelListItemFetch.FetchInfo will panic if p.Fetch is nil (defensive guard is cheap).func (p *ParallelListItemFetch) FetchInfo() *FetchInfo { - return p.Fetch.Info + if p.Fetch == nil { + return nil + } + return p.Fetch.Info }
373-375: Tiny doc grammar fix + summarize invariants-// FetchInfo contains additional (derived) information about the fetch. Some fields of this -// structure may be not generated depending on the planner flags. +// FetchInfo contains additional (derived) information about the fetch. +// Some fields may not be generated depending on planner flags. +// Callers must treat FetchInfo as read-only after planning; it may be nil when disabled by planner options.
382-395: Document PropagatedFetchReasons invariants and clarify JSON tag naming
- Add inline doc on PropagatedFetchReasons noting it’s a deduplicated, stable-ordered subset of FetchReasons (filtered via DataSourceConfiguration.RequireFetchReasons) to lock in consumer assumptions.
- Annotate the JSON-tag discrepancy (GraphCoordinate uses “typeName”/“fieldName” vs FetchReason’s “typename”/“field”) as intentional to match the “fetch_reason” extension schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/fetch.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/fetch.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(273-286)pkg/graphql/request.go (1)
OperationType(21-21)
⏰ 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.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (3)
v2/pkg/engine/resolve/fetch.go (3)
105-107: SingleFetch.FetchInfo: LGTM
175-177: BatchEntityFetch.FetchInfo: LGTM
215-217: EntityFetch.FetchInfo: LGTM
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v2/pkg/engine/resolve/fetch.go (2)
365-371: Fix FetchReason JSON tags and update test expectations
ChangeFetchReasonto usejson:"typeName"/json:"fieldName"and update the two literals in execution/engine/execution_engine_test.go (around lines 848 and 4363) to expect"typeName"/"fieldName"in thefetch_reasonsextension payload.
304-333: Include OperationName in FetchConfiguration.Equals
Add a check forOperationNamebefore the finalreturn truein v2/pkg/engine/resolve/fetch.go, e.g.:… if fc.SetTemplateOutputToNullOnVariableNull != other.SetTemplateOutputToNullOnVariableNull { return false } +if fc.OperationName != other.OperationName { + return false +} return trueThis prevents two fetches with the same Input/Variables but different operation names from being deduped incorrectly.
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/fetch.go (1)
144-146: Remove stale comment referencing a non-existent “Response template”This line predates the current struct fields and can mislead readers.
Apply this diff:
- // Response template is unused in the current codebase - so we can ignore it -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/fetch.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/fetch.go (3)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(273-286)v2/pkg/engine/resolve/datasource.go (1)
DataSource(12-15)pkg/graphql/request.go (1)
OperationType(21-21)
⏰ 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). (3)
- 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 / ubuntu-latest)
🔇 Additional comments (4)
v2/pkg/engine/resolve/fetch.go (4)
24-27: FetchInfo accessor documented and added to interface — LGTMNil semantics are clear; read-only post-planning is explicit.
105-107: Consistent FetchInfo() implementations across fetch types — LGTMEach fetch kind cleanly forwards to its Info pointer.
Also applies to: 175-177, 215-217, 243-245
501-507: Interface conformance checks — LGTMGood guardrail to prevent regressions.
392-394: Update comment to reference "fetch_reasons" extension
File: v2/pkg/engine/resolve/fetch.go lines 392–393- // PropagatedFetchReasons holds those FetchReasons that will be propagated - // with the request to the subgraph as part of the "fetch_reason" extension. + // PropagatedFetchReasons holds the FetchReasons propagated + // with the request to the subgraph as part of the "fetch_reasons" extension.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/Makefile (1)
16-16: Config discovery may not work from subdirs; verify or keep explicit --config.Relying on auto-discovery from v2/ to ../.golangci.yml can be flaky across golangci-lint versions; please verify. If unsure, keep an explicit --config using the repo root.
Example robust setup (add at top of the Makefile outside this hunk):
ROOT_DIR := $(shell git rev-parse --show-toplevel)Then use
--config $(ROOT_DIR)/.golangci.ymlin the commands below.
🧹 Nitpick comments (4)
v2/Makefile (2)
23-23: Make lint invocation explicit and consistent; drop trailing space.For determinism across shells and CI working dirs, pin the config and (optionally) the path.
- golangci-lint run + golangci-lint run --config $(ROOT_DIR)/.golangci.yml .
19-19: Use explicit config and remove trailing space
- Replace
withgolangci-lint fmt(remove trailing whitespace) (golangci-lint.run)golangci-lint fmt --config $(ROOT_DIR)/.golangci.yml- Add at the top of
v2/Makefile:ROOT_DIR := $(shell git rev-parse --show-toplevel)- If your goal includes auto-fixing lint issues (beyond formatting), use:
golangci-lint run --fix --config $(ROOT_DIR)/.golangci.ymlv2/pkg/engine/plan/visitor.go (2)
1344-1351: Prefer len-check over nil-check for FetchReasonsbuildFetchReasons may return an empty slice; treat both nil and empty uniformly.
- if singleFetch.Info.FetchReasons == nil { + if len(singleFetch.Info.FetchReasons) == 0 { return singleFetch }
1352-1362: Decouple from planner index; tighten capacity pre-allocationUse internal.planner instead of indexing v.planners by fetchID to avoid relying on the “planner index == fetchID” invariant. Also pre-size the slice by the max elements we can actually append.
- dsConfig := v.planners[internal.fetchID].DataSourceConfiguration() - lookup := dsConfig.RequireFetchReasons() - propagated := make([]resolve.FetchReason, 0, len(lookup)) + dsConfig := internal.planner.DataSourceConfiguration() + if dsConfig == nil { + return singleFetch + } + lookup := dsConfig.RequireFetchReasons() + propagated := make([]resolve.FetchReason, 0, len(singleFetch.Info.FetchReasons))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
execution/Makefile(1 hunks)v2/Makefile(1 hunks)v2/pkg/engine/plan/visitor.go(3 hunks)v2/pkg/engine/resolve/fetch.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/Makefile
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/engine/plan/visitor.go (3)
v2/pkg/engine/resolve/fetch.go (3)
FetchInfo(375-396)QueryPlan(251-254)FetchReason(364-371)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceConfiguration(268-271)v2/pkg/engine/plan/federation_metadata.go (1)
FieldCoordinate(89-92)
v2/pkg/engine/resolve/fetch.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(273-286)pkg/graphql/request.go (1)
OperationType(21-21)
⏰ 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 / windows-latest)
- 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)
🔇 Additional comments (12)
v2/pkg/engine/plan/visitor.go (5)
1328-1337: Early return + FetchInfo initialization look solidNice guard rails and minimal allocation when info is disabled. LGTM.
1339-1343: Good: dependencies attached only when enabledPopulating CoordinateDependencies behind the feature flag is correct. LGTM.
1427-1428: Good choice of dedup keySwitching to FieldCoordinate as the dedup/index key is clean and avoids ad-hoc tuple types. LGTM.
1441-1469: Reason construction logic is soundCollecting dependant subgraphs and flags (IsKey/IsRequires) per fieldRef is correct, with later dedup. LGTM.
1473-1499: Merging and stable orderingMerging user/subgraph reasons with a stable sort + compact is tidy. LGTM.
v2/pkg/engine/resolve/fetch.go (7)
105-107: SingleFetch.FetchInfo passthrough LGTM
163-169: BatchEntityFetch: Info field + accessor LGTMAlso applies to: 175-177
203-209: EntityFetch: Info field + accessor LGTMAlso applies to: 215-217
243-245: ParallelListItemFetch: delegate to inner fetchReturning p.Fetch.Info is the right call. LGTM.
373-396: FetchInfo doc and fieldsDocs clearly signal optional generation. Consider keeping this struct read-only post-plan; callers should avoid mutation (already implied by comments). LGTM.
502-509: Compile-time interface assertionsGreat safety net for future refactors. LGTM.
23-27: All FetchInfo() call sites guard against nil – no action needed
Verified that each.FetchInfo()invocation is preceded by aninfo != nilcheck before any field access.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.225](v2.0.0-rc.224...v2.0.0-rc.225) (2025-09-09) ### Features * add support of field selection reasons extensions ([#1282](#1282)) ([37c9582](37c9582)) * upgrade all components to go 1.25 ([#1289](#1289)) ([6bd2713](6bd2713)) ### Bug Fixes * refactor CoordinateDependencies, FetchReasons ([#1293](#1293)) ([cfebc16](cfebc16)) --- 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 * **New Features** * Added support for field selection reasons extensions. * Upgraded all components to Go 1.25 for improved compatibility and performance. * **Chores** * Bumped release version to 2.0.0-rc.225. * **Documentation** * Updated changelog with a new 2.0.0-rc.225 entry detailing recent features. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Move them into the "FetchInfo" struct. That struct is ideal for storing derived data
for a Fetch and passing it to the later stages of the engine.
Add the BuildFetchReasons option to the planner, that allows other parts to enable building the FetchReason list.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
Checklist