fix: fix collecting representation for fetches scoped to concrete types #1200
Conversation
WalkthroughThe changes introduce enhanced handling and testing for nested fetches on GraphQL interfaces and unions in a federated engine. They add new test cases for nested fetches on specific types, refactor internal path tracking to include type names, extend fetch path elements with type metadata, improve deduplication logic, and unify fetch path resolution logic. 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 (1)
✨ 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 (
|
devsergiy
changed the title
devsergiy
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: 1
🧹 Nitpick comments (1)
v2/pkg/engine/postprocess/deduplicate_single_fetches.go (1)
20-20: Consider potential side effects of in-place modification.The
mergeFetchPathfunction modifies theleftslice in-place by updating theTypeNamesfield of its elements. While this may be intentional for performance reasons, it could be unexpected behavior for callers.Consider documenting this behavior or making a defensive copy if the in-place modification is not intended:
func (d *deduplicateSingleFetches) mergeFetchPath(left, right []resolve.FetchItemPathElement) []resolve.FetchItemPathElement { + // Note: modifies left slice in-place for i := range left { left[i].TypeNames = d.mergeTypeNames(left[i].TypeNames, right[i].TypeNames) } return left }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
execution/engine/execution_engine_test.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(5 hunks)v2/pkg/engine/plan/path_builder.go(2 hunks)v2/pkg/engine/plan/path_builder_visitor.go(7 hunks)v2/pkg/engine/postprocess/deduplicate_single_fetches.go(2 hunks)v2/pkg/engine/resolve/fetch.go(1 hunks)v2/pkg/engine/resolve/fetchtree.go(1 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)v2/pkg/engine/resolve/resolve_federation_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
undefined
<retrieved_learning>
Learnt from: SkArchon
PR: #1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
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.
</retrieved_learning>
🧬 Code Graph Analysis (3)
v2/pkg/engine/resolve/fetchtree.go (1)
v2/pkg/engine/resolve/fetch.go (1)
FetchItemPathElement(74-78)
v2/pkg/engine/postprocess/deduplicate_single_fetches.go (1)
v2/pkg/engine/resolve/fetch.go (1)
FetchItemPathElement(74-78)
v2/pkg/engine/plan/path_builder_visitor.go (2)
pkg/ast/ast_node_kind.go (3)
NodeKindObjectTypeDefinition(14-14)NodeKindInterfaceTypeDefinition(16-16)NodeKindUnionTypeDefinition(18-18)v2/pkg/engine/resolve/fetch.go (2)
FetchItemPathElement(74-78)FetchItemPathElementKindObject(83-83)
🔇 Additional comments (19)
v2/pkg/engine/plan/path_builder.go (1)
67-67: LGTM! Typo fixes are correct.The method name corrections from
populateMissingPahts()topopulateMissingPaths()fix the spelling error consistently across both call sites.Also applies to: 89-89
v2/pkg/engine/resolve/fetch.go (1)
77-77: LGTM! Clean struct extension.The addition of the
TypeNames []stringfield toFetchItemPathElementproperly extends the struct to carry type metadata, which aligns with the PR's objective of handling fetches scoped to concrete types.v2/pkg/engine/resolve/fetchtree.go (1)
47-50: LGTM! Clean helper function.The
PathElementWithTypeNamesfunction provides a convenient way to set theTypeNamesfield on aFetchItemPathElement. The implementation is straightforward and correct.v2/pkg/engine/resolve/resolve_federation_test.go (1)
2835-2990: Excellent test coverage for type-scoped nested fetches.This test effectively validates the core functionality described in the PR title. The implementation correctly demonstrates:
- Type-specific filtering: The
OnTypeNames: [][]byte{[]byte("User")}filtering in both the variable renderer (lines 2886, 2893) and data structure (lines 2955, 2981) ensures fetches only apply to User types- Path element type scoping: The use of
PathElementWithTypeNames(ObjectPath("some"), []string{"User"})on line 2922 properly routes the fetch to only User-typed "some" fields- Differential response handling: The expected output correctly shows Users receiving the additional "title" field while Admins retain their original structure
The test comprehensively covers the edge case where a polymorphic field (accounts containing both User and Admin) needs selective nested fetching based on concrete types.
execution/engine/execution_engine_test.go (1)
4228-4434: Well-structured federation test for nested interface fetches.This test effectively validates a complex federation scenario involving:
- Interface types with multiple concrete implementations (User/Admin)
- Nested fetches with type-specific field selections using inline fragments
- Cross-subgraph federation where external fields require additional entity fetches
- Selective federation fetching (only User entities need additional resolution)
The test setup correctly mirrors real-world federation patterns where different subgraphs own different aspects of the same types, and the mock expectations accurately reflect the expected federation query patterns.
v2/pkg/engine/resolve/loader.go (3)
325-337: Good simplification of the control flow.The removal of conditional branching based on element kind makes the code cleaner and easier to follow. The unified
selectItemsmethod now handles all cases consistently.
339-353: Well-implemented type filtering logic.The method properly handles all edge cases with defensive programming:
- Early return when no type filter is specified
- Graceful handling of non-object types
- Safe handling of missing
__typenamefieldThe use of
slices.Containsfor type checking is clean and efficient.
355-393: Excellent unification of selection logic.The unified
selectItemsmethod successfully combines the functionality of the three removed methods while adding type-aware filtering. The implementation:
- Maintains the original behavior for both single and multiple items
- Consistently applies type filtering before field selection
- Properly handles array unwrapping in both cases
- Reduces code duplication and improves maintainability
v2/pkg/engine/plan/path_builder_visitor.go (6)
39-42: Good encapsulation of selection set metadata.The new
selectionSetTypeInfostruct properly groups the selection set reference with its associated type names, making the code more maintainable and the intent clearer.Also applies to: 102-105
244-245: Thanks for fixing the typo!Good catch on correcting
populateMissingPahts→populateMissingPaths.
122-128: Good use of Go idioms for the method signature.The renamed method with the boolean return value follows Go best practices for methods that may not have a value to return. The handling of the empty case is correct.
370-391: Robust implementation of type name collection for inline fragments.The logic correctly:
- Validates that the type condition exists in the schema with proper error handling
- Handles different GraphQL type kinds appropriately:
- Object types: single type name
- Interfaces: all implementing object types
- Unions: all member types
- Stores the collected type names for later use in filtering
1019-1028: Correct propagation of type information to fetch paths.The code safely retrieves type names from the current selection set and propagates them to the fetch path element. This enables type-aware filtering during fetch resolution.
270-273: Initialization correctly updated for the new type.The slice initialization properly uses the new
selectionSetTypeInfotype with an appropriate initial capacity.v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (5)
10447-10447: Schema updates look consistent.The addition of
accounts: [Node!]!field to both Query type definitions is consistent and aligns with the test requirements.Also applies to: 10472-10472
10484-10484: Configuration properly updated for new field.Adding "accounts" to the FieldNames slice correctly registers the new field as a root node field in the data source configuration.
10938-11129: Well-structured test for nested fetches with type differentiation.This test comprehensively covers the scenario where the same path requires different fetch strategies based on concrete types. The test correctly:
- Sets up a query with inline fragments on User and Admin types
- Expects two separate nested fetches for different path depths ([email protected] and [email protected])
- Properly filters fetch targets by type names (User only for nested fetches)
- Includes correct variable construction with __typename and id fields
- Structures the expected response with proper type conditions
The test logic aligns with the PR's goal of fixing representation collection for concrete types.
11131-11264: Effective test for selective nested fetching.This test validates the scenario where nested fetches are only required for specific types. The test correctly:
- Queries both User and Admin types with different field requirements
- Expects only one nested fetch (for User types at [email protected])
- Properly excludes Admin from nested fetches since its fields are available locally
- Maintains correct type filtering in the fetch path configuration
This test effectively demonstrates the selective nature of nested fetch optimization based on concrete type requirements.
12084-12084: Type name filtering correctly applied to path element.The change from
resolve.ObjectPath("address")toresolve.PathElementWithTypeNames(resolve.ObjectPath("address"), []string{"Moderator"})properly adds explicit type filtering to the path element. This fix aligns with the PR's objective to correctly collect representations for fetches scoped to concrete types.
devsergiy
deleted the
sergiy/eng-7460-fix-unnecessary-fetch-from-a-subgraph
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.198](v2.0.0-rc.197...v2.0.0-rc.198) (2025-07-04) ### Features * add support for composite types ([#1197](#1197)) ([e9b9f19](e9b9f19)) ### Bug Fixes * fix collecting representation for fetches scoped to concrete types ([#1200](#1200)) ([bcf547d](bcf547d)) --- 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 composite types. * **Bug Fixes** * Corrected the collection of representations for fetches scoped to concrete types. * **Documentation** * Updated the changelog with details for version 2.0.0-rc.198. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
Checklist