feat: implement resolving fetch dependencies#1216
feat: implement resolving fetch dependencies#1216jensneuse merged 4 commits intowundergraph:masterfrom
Conversation
jensneuse
requested review from
a team,
Noroth,
StarpTech and
devsergiy
as code owners
WalkthroughThis change introduces detailed tracking and reporting of field-level dependencies in the GraphQL federation query planning and execution system. It adds new data structures and configuration options to capture, propagate, and expose how fields and fetches depend on each other, including the kind of dependency (key or requires). New tests and helper methods are provided to validate this behavior, including handling of recursive fragment queries. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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). (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
v2/pkg/engine/resolve/fetch.go (1)
287-320: Critical: Update the Equals method to compare Dependencies fieldThe
Equalsmethod needs to be updated to include comparison of the newDependenciesfield. Without this, twoFetchConfigurationinstances with different dependencies would incorrectly be considered equal.Add this comparison to the
Equalsmethod after line 319:if fc.SetTemplateOutputToNullOnVariableNull != other.SetTemplateOutputToNullOnVariableNull { return false } + + if !slices.EqualFunc(fc.Dependencies, other.Dependencies, func(a, b FetchDependency) bool { + if a.Coordinate != b.Coordinate { + return false + } + if a.IsUserRequested != b.IsUserRequested { + return false + } + return slices.EqualFunc(a.DependsOn, b.DependsOn, func(x, y FetchDependencyOrigin) bool { + return x.FetchID == y.FetchID && + x.Subgraph == y.Subgraph && + x.Coordinate == y.Coordinate && + x.IsKey == y.IsKey && + x.IsRequires == y.IsRequires + }) + }) { + return false + } return true
🧹 Nitpick comments (1)
execution/engine/graphql_client_test.go (1)
76-86: Consider using or removing the unusedexpectedStatusCodeparameter.The method accepts an
expectedStatusCodeparameter but doesn't use it for validation. This could be misleading to callers who might expect status code assertion. Consider either:
- Adding status code validation:
assert.Equal(t, expectedStatusCode, resp.StatusCode)- Removing the unused parameter if raw response access is the only requirement
Option 1: Add status code validation
func (g *GraphqlClient) QueryStatusCode(ctx context.Context, addr, queryFilePath string, variables queryVariables, expectedStatusCode int, t *testing.T) []byte { reqBody := loadQuery(t, queryFilePath, variables) req, err := http.NewRequest(http.MethodPost, addr, bytes.NewBuffer(reqBody)) require.NoError(t, err) req = req.WithContext(ctx) resp, err := g.httpClient.Do(req) require.NoError(t, err) responseBodyBytes, err := io.ReadAll(resp.Body) require.NoError(t, err) + assert.Equal(t, expectedStatusCode, resp.StatusCode) return responseBodyBytes }Option 2: Remove unused parameter
-func (g *GraphqlClient) QueryStatusCode(ctx context.Context, addr, queryFilePath string, variables queryVariables, expectedStatusCode int, t *testing.T) []byte { +func (g *GraphqlClient) QueryRaw(ctx context.Context, addr, queryFilePath string, variables queryVariables, t *testing.T) []byte {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
execution/engine/federation_integration_test.go(1 hunks)execution/engine/graphql_client_test.go(1 hunks)execution/federationtesting/testdata/queries/recursive_fragment.graphql(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(4 hunks)v2/pkg/engine/datasourcetesting/datasourcetesting.go(4 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/node_selection_builder.go(2 hunks)v2/pkg/engine/plan/node_selection_visitor.go(6 hunks)v2/pkg/engine/plan/planner.go(1 hunks)v2/pkg/engine/plan/visitor.go(5 hunks)v2/pkg/engine/resolve/fetch.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
execution/engine/federation_integration_test.go (1)
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
🧬 Code Graph Analysis (3)
execution/engine/federation_integration_test.go (2)
execution/federationtesting/util.go (1)
NewFederationSetup(52-70)execution/engine/graphql_client_test.go (1)
NewGraphqlClient(51-55)
v2/pkg/engine/datasourcetesting/datasourcetesting.go (1)
v2/pkg/engine/postprocess/postprocess.go (1)
Processor(21-28)
v2/pkg/engine/plan/node_selection_builder.go (3)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(252-262)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (1)
NodeSuggestions(91-96)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfiguration(73-82)
⏰ 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: Linters
- 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)
🔇 Additional comments (29)
v2/pkg/engine/plan/planner.go (1)
152-153: LGTM! Clean integration of field dependency tracking.The assignment of field dependency information from
selectionsConfigto the planning visitor follows the established pattern and integrates well with the existing configuration flow.v2/pkg/engine/plan/configuration.go (1)
31-31: LGTM! Consistent configuration field addition.The new
DisableIncludeFieldDependenciesfield follows the established naming pattern and is appropriately positioned alongside the similarDisableIncludeInfofield.execution/engine/federation_integration_test.go (1)
357-365: LGTM! Well-structured test for recursive fragment error handling.The test case follows the established pattern and appropriately verifies that recursive fragment queries result in an HTTP 500 error with an empty response body. The test setup and cleanup are handled correctly.
execution/federationtesting/testdata/queries/recursive_fragment.graphql (1)
1-12: LGTM! Effective test case for recursive fragment detection.The query creates a clear recursive cycle between
FragmentAandFragmentB, which is exactly what's needed to test the system's handling of recursive fragment references. The structure is minimal but sufficient for the test scenario.v2/pkg/engine/datasourcetesting/datasourcetesting.go (5)
31-31: Flag inconsistency with AI summary.The
postProcessorsfield addition is not mentioned in the AI summary, which focuses specifically on field dependency tracking. This suggests the change might be unrelated to the main feature or the summary is incomplete.Likely an incorrect or invalid review comment.
35-35: LGTM: Field dependency option follows existing pattern.The new
withFieldDependenciesfield is consistent with the existingwithFieldInfopattern and properly integrates with the test configuration system.
71-75: LGTM: Option function follows established convention.The
WithFieldDependencies()function correctly follows the existing pattern for test options, providing a clean API for enabling field dependency tracking in tests.
135-135: LGTM: Sensible default configuration.Disabling field dependencies by default is appropriate for keeping tests focused and less verbose, consistent with the approach taken for field info.
146-148: LGTM: Configuration logic is correct.The conditional logic properly overrides the default when the field dependencies option is enabled, following the same pattern as the existing
withFieldInfoimplementation.v2/pkg/engine/plan/node_selection_builder.go (3)
22-27: LGTM: Well-defined dependency kind enumeration.The
fieldDependencyKindtype and constants provide clear semantics for distinguishing between key-based and requires-based field dependencies, which aligns with GraphQL federation concepts.
35-36: LGTM: Field additions support dependency tracking.The new fields in
NodeSelectionResultproperly extend the structure to include field dependency information:
fieldRefDependsOnmaps field references to their dependenciesfieldDependencyKindcategorizes the type of dependency relationshipThe naming and types are consistent with the existing codebase patterns.
182-183: LGTM: Proper field population from visitor.The new fields are correctly populated from the corresponding visitor fields when constructing the
NodeSelectionResult, ensuring the dependency information is properly propagated through the planning system.v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (4)
3102-3137: LGTM! Well-structured federation dependency tracking.The dependency declarations correctly model how the Address fields (country, city) depend on the key field (id) from the user.service subgraph. The structure is consistent and the IsKey flag properly identifies the key field dependency.
3177-3230: Excellent modeling of complex multi-fetch dependencies.The dependency structure correctly captures how the zip field depends on both required fields (country, city) from the address-enricher.service and the key field (id) from user.service. This demonstrates proper handling of complex federation scenarios where fields span multiple fetches.
3281-3336: Comprehensive dependency modeling for user-requested fields.The dependency structure excellently captures how the fullAddress field depends on multiple fields across different subgraphs. The IsUserRequested flag correctly identifies this as a directly requested field, and the dependency chain properly models the complex relationships with appropriate IsKey and IsRequires flags.
3477-3477: Proper enablement of dependency tracking feature.The addition of
WithFieldDependencies()correctly enables the dependency tracking feature for the test, ensuring that the comprehensive dependency data added to the FetchConfiguration instances will be processed and validated.v2/pkg/engine/plan/node_selection_visitor.go (5)
41-42: LGTM!The new fields are well-named and appropriately typed for tracking field dependency information.
52-54: LGTM!The struct is well-designed for use as a composite map key.
151-151: LGTM!Proper initialization following the established pattern.
512-514: LGTM!Correctly records @requires field dependencies.
603-605: LGTM!Properly tracks @key field dependencies for both chain items and requesting fields.
Also applies to: 626-628
v2/pkg/engine/resolve/fetch.go (2)
324-333: LGTM!The
FetchDependencystruct is well-designed with clear documentation and appropriate fields.
335-349: LGTM!The
FetchDependencyOriginstruct effectively captures all necessary metadata about dependency origins.v2/pkg/engine/plan/visitor.go (6)
46-47: LGTM!The new fields are well-documented and appropriately structured for tracking field dependencies and planner associations.
Also applies to: 56-63
205-221: LGTM!Efficiently tracks bidirectional field-planner associations with proper conditional checks.
340-342: LGTM!Good use of
strings.Cloneto ensure proper string copying and avoid potential memory issues.
1061-1063: LGTM!Proper initialization of the new tracking maps.
1329-1331: LGTM!Clean integration of dependency resolution into the fetch configuration.
1336-1391: LGTM!Comprehensive implementation that correctly:
- Distinguishes between user-requested and dependency fields
- Maps dependency kinds (key vs requires)
- Uses
strings.Clonefor proper string handling- Handles all edge cases appropriately
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.200](v2.0.0-rc.199...v2.0.0-rc.200) (2025-07-08) ### Features * implement resolving fetch dependencies ([#1216](#1216)) ([ca9ebaa](ca9ebaa)) ### Bug Fixes * use existing files in BenchmarkMinify ([#1214](#1214)) ([0083b78](0083b78)) * use int in netpoll's BenchmarkSocketFdReflect ([#1213](#1213)) ([35f3175](35f3175)) --- 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 resolving fetch dependencies. * **Bug Fixes** * Fixed an issue to ensure existing files are used in the BenchmarkMinify test. * Corrected the type used in netpoll's BenchmarkSocketFdReflect benchmark to int. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests