Conversation
WalkthroughA new GraphQL validation rule for empty selection sets is introduced and integrated into the operation validation pipeline for dynamically generated operations. The data source filter visitor logic is refactored to adjust node selection stages, remove certain leaf node selections, and modify parent chain handling, with corresponding updates to selection reason constants and test case reason strings. Changes
Estimated code review effort3 (~40 minutes) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 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
force-pushed
the
sergiy/eng-7397-fix-planner-parent-entity-jump
branch
from
cd67466 to
528a716
Compare
devsergiy
force-pushed
the
sergiy/eng-7397-fix-planner-parent-entity-jump
branch
from
528a716 to
00e61d2
Compare
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: 2
🧹 Nitpick comments (5)
v2/pkg/astvalidation/operation_rule_validate_empty_selection_set.go (3)
10-12: Clarify the usage context in the comment.The comment mentions "should be used only when the operation is created on the fly" but doesn't explain why this restriction exists or what constitutes an operation "created on the fly".
Consider expanding the comment to clarify:
-// ValidateEmptySelectionSets validates if selection sets are not empty -// should be used only when the operation is created on the fly +// ValidateEmptySelectionSets validates that selection sets are not empty. +// This rule is specifically designed for dynamically generated operations +// where empty selection sets indicate a planner error, as opposed to +// user-provided queries where empty selection sets are caught by standard validation.
33-35: Enhance error message for better debugging.The error message could be more descriptive to aid in debugging.
- r.Walker.StopWithInternalErr(fmt.Errorf("astvalidation selection set on path %s is empty", r.Walker.Path.DotDelimitedString())) + r.Walker.StopWithInternalErr(fmt.Errorf("empty selection set detected at path: %s - this indicates a planner error in dynamically generated operations", r.Walker.Path.DotDelimitedString()))
1-36: Add unit tests for the validation rule.This new validation rule should have corresponding unit tests to ensure it works correctly.
Would you like me to generate unit tests for the
ValidateEmptySelectionSetsrule to ensure proper coverage?v2/pkg/engine/plan/datasource_filter_visitor.go (2)
523-527: Update stage numbering in comments to match implementation.The comment mentions "stages 2,3,4" but the implementation shows stages 2-5.
- // stages 2,3,4 - are stages when choices are equal, and we should select first available node + // stages 2-5 are stages when choices are equal, and we should select first available node
597-627: Clarify the TODO comment about parent entity chain selection.The TODO comment suggests replacing the logic with "smallest parent entity chain" selection, but the current implementation already considers jump counts.
Consider clarifying or implementing the TODO:
- // TODO: replace with all nodes check - select smallest parent entity chain + // TODO: Consider all possible parent chains and select the one with the minimum jumps + // Current implementation only checks the first valid parent chain found
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/astvalidation/operation_rule_validate_empty_selection_set.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(1 hunks)v2/pkg/engine/plan/datasource_filter_visitor.go(6 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(8 hunks)
🧠 Learnings (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
Learnt from: SkArchon
PR: #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.
🧰 Additional context used
🧠 Learnings (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
Learnt from: SkArchon
PR: #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.
⏰ 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)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (3)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
1687-1698: LGTM! Excellent defensive programming approach.This validation rule integration effectively catches incorrect planner behavior by ensuring that only selection sets containing fields are processed. The clear comments explain the rationale well, and placing this validation in the pool initialization ensures consistent application across all operations.
This change aligns perfectly with the PR objectives around planner fixes for parent entity jumps and unique node selections.
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
294-294: LGTM! Test updates align with simplified selection logic.The test updates correctly reflect the changes in the main code where the selection reason strings have been simplified to
"stage1: unique". This maintains test coverage while aligning with the refactored selection logic.Also applies to: 563-564, 823-823, 832-832, 941-941, 959-959, 995-995, 998-998, 1008-1008, 1011-1011
v2/pkg/engine/plan/datasource_filter_visitor.go (1)
240-280: Verify impact of new root-node requirement in selectUniqNodeParentsUpToRootNodeIt looks like no existing tests exercise this method or the
ReasonStage1SameSourceParentpath, so we can’t confirm that the change won’t break query planning. Please manually review and/or add tests to cover:
- Calling
selectUniqNodeParentsUpToRootNodewhen:
- The parent chain never reaches a root node → expect no selections.
- A root node is found but has
DisabledEntityResolver == true→ expect no selections.- A root node is found and resolver is enabled → expect all intermediate parents to be selected.
- Behaviors when encountering an external parent with
IsProvided == false.- Edge cases around
IsRootNodeon the initial item.File to review:
• v2/pkg/engine/plan/datasource_filter_visitor.go (lines ~240–280)
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.210](v2.0.0-rc.209...v2.0.0-rc.210) (2025-07-22) ### Bug Fixes * planner fixes for parent entity jumps and unique nodes selections ([#1230](#1230)) ([1a7ed16](1a7ed16)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
1 participant
fixes ENG-7397
fixes ENG-7355
fixes ENG-7228
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist