Conversation
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughThis change removes the dedicated field selection merging logic and its tests, integrates field merging into the existing inline fragment merging visitor (renamed and expanded to handle both fields and inline fragments), updates or adds comments for clarity, and consolidates and broadens test coverage to reflect the new merging approach. Changes
Estimated code review effort4 (60–120 minutes) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ 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)
🔇 Additional comments (4)
✨ 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 (
|
ysmolski
force-pushed
the
yury/eng-7133-fix-merging-fields-and-fragments-in-normalization
branch
from
2489f15 to
e9d0f5b
Compare
![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
🧹 Nitpick comments (1)
v2/pkg/astnormalization/inline_fragment_field_selection_merging_test.go (1)
1-335: Excellent test suite consolidation and coverage.This comprehensive test file successfully replaces the previous separate fragment and field merging tests with a unified, well-structured test suite. The tests thoroughly validate the new
mergeInlineFragmentFieldSelectionsfunction across a wide range of scenarios from simple cases to complex nested structures.Consider adding brief comments for the more complex test cases to document the expected merging behavior, particularly around duplicate field preservation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/astnormalization/inline_fragment_field_selection_merging_test.go(1 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 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (5)
v2/pkg/astnormalization/inline_fragment_field_selection_merging_test.go (5)
1-3: LGTM! Clean package structure.The package declaration and imports are minimal and appropriate for this test file.
5-6: LGTM! Well-structured test organization.The test function is properly organized with clear categorization using
t.Runfor different merging scenarios.
6-125: LGTM! Comprehensive fragment merging tests.The fragment tests thoroughly cover the merging scenarios including edge cases with directives and type conditions. The expected behavior of preserving duplicate fields in merged fragments appears intentional for the normalization process.
126-219: LGTM! Thorough field merging test coverage.The field tests appropriately cover various merging scenarios including depth variations, aliasing, and directive handling. The test cases correctly demonstrate when fields should and shouldn't merge.
220-335: LGTM! Excellent comprehensive test coverage.The mixed fragment and field tests provide thorough coverage of complex real-world scenarios. The "mixed heavily fields with deep inline fragments" test is particularly valuable as it validates the merging behavior across multiple levels of nesting and different fragment types.
ysmolski
force-pushed
the
yury/eng-7133-fix-merging-fields-and-fragments-in-normalization
branch
2 times, most recently
from
5c73a0d to
4dc12a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/astnormalization/inline_fragment_selection_merging.go (2)
10-18: Consider renaming the function to match the expanded functionality.The function name
mergeInlineFragmentSelectionsdoesn't reflect that it now also merges field selections. Consider renaming it tomergeInlineFragmentFieldSelectionsfor consistency with the visitor name and actual functionality.-// mergeInlineFragmentSelections registers a visitor that -// merges inline fragment and field selections. -func mergeInlineFragmentSelections(walker *astvisitor.Walker) { +// mergeInlineFragmentFieldSelections registers a visitor that +// merges inline fragment and field selections. +func mergeInlineFragmentFieldSelections(walker *astvisitor.Walker) {
107-107: Use conventional loop variable naming.Consider using
jinstead ofkfor the inner loop variable, as this is the standard convention for nested loops.- for k := i + 1; k < len(selectionRefs); k++ { + for j := i + 1; j < len(selectionRefs); j++ {Also update all references to
kwithin the loop toj.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
v2/pkg/ast/ast_selection.go(0 hunks)v2/pkg/astnormalization/abstract_field_normalizer.go(0 hunks)v2/pkg/astnormalization/astnormalization.go(4 hunks)v2/pkg/astnormalization/field_deduplication.go(1 hunks)v2/pkg/astnormalization/field_selection_merging.go(0 hunks)v2/pkg/astnormalization/field_selection_merging_test.go(0 hunks)v2/pkg/astnormalization/fragment_definition_removal.go(1 hunks)v2/pkg/astnormalization/inline_fragment_selection_merging.go(3 hunks)v2/pkg/astnormalization/inline_fragment_selection_merging_test.go(6 hunks)v2/pkg/astnormalization/remove_self_aliasing.go(1 hunks)
💤 Files with no reviewable changes (4)
- v2/pkg/astnormalization/abstract_field_normalizer.go
- v2/pkg/ast/ast_selection.go
- v2/pkg/astnormalization/field_selection_merging_test.go
- v2/pkg/astnormalization/field_selection_merging.go
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/astnormalization/field_deduplication.go
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/pkg/astnormalization/remove_self_aliasing.go
- v2/pkg/astnormalization/fragment_definition_removal.go
- v2/pkg/astnormalization/astnormalization.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
v2/pkg/astnormalization/inline_fragment_selection_merging.go (1)
v2/pkg/ast/ast_selection.go (3)
SelectionKind(11-11)SelectionKindInlineFragment(17-17)SelectionKindField(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). (2)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (4)
v2/pkg/astnormalization/inline_fragment_selection_merging_test.go (1)
5-334: Comprehensive test coverage for the expanded functionality.The test restructuring and expansion effectively covers the new field merging capabilities alongside the existing inline fragment merging. The organization into "just fragments", "just fields", and "fragments and fields" sections provides clear separation of concerns and makes the tests easy to understand and maintain.
v2/pkg/astnormalization/inline_fragment_selection_merging.go (3)
59-93: Well-implemented field merging logic.The new methods follow the same pattern as the existing fragment merging logic, ensuring consistency. The checks for field name, alias, and directive equality are comprehensive and correct for determining when fields can be safely merged.
94-143: Correct implementation with early return optimization.The unified handling of inline fragments and fields is well-structured. The early return after each merge is a good optimization that prevents index-related issues and ensures the selection set is re-evaluated after modifications.
Note that the algorithm has O(n²) complexity for n selections, which could be a concern for deeply nested queries with many selections. However, this is likely acceptable given typical GraphQL query structures.
129-140: RemoveFromSelectionSet slicing is correct; no index-shift issues
RemoveFromSelectionSet simply removes the element at the given index via a slice operation, and every call is followed byRevisitNode()and an immediate return—so you never continue iterating on a shifted slice. No changes needed.
ysmolski
force-pushed
the
yury/eng-7133-fix-merging-fields-and-fragments-in-normalization
branch
5 times, most recently
from
799b16e to
d56e3d0
Compare
jensneuse
left a comment
jensneuse
left a comment
There was a problem hiding this comment.
Looks ok but also check with @devsergiy
One comment you could look into or ignore.
|
we won't merge this PR immediately, we need to run it through regression test suite |
This change fixes normalization for cases like the following:
query fragmentsWithFields {
dog {
extras {
... on DogExtra {
string
}
}
extras {
... on DogExtra {
string
string1
}
}
}
}
I have marged two different visitors and their tests into a single
visitor "mergeInlineFragmentSelections".
EnterSelectionSets from two stages were merged into one,
but with the following differences:
* Internal loop was optimized,
* Early return happens only when something is merged.
ysmolski
force-pushed
the
yury/eng-7133-fix-merging-fields-and-fragments-in-normalization
branch
from
d56e3d0 to
a6a620b
Compare
ysmolski
changed the title
ysmolski
deleted the
yury/eng-7133-fix-merging-fields-and-fragments-in-normalization
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.209](v2.0.0-rc.208...v2.0.0-rc.209) (2025-07-22) ### Bug Fixes * merge inline fragment and field selections together ([#1240](#1240)) ([99f2b32](99f2b32)) --- 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 * **Bug Fixes** * Resolved an issue where inline fragment and field selections were not merged correctly. * **Documentation** * Added a changelog entry for version 2.0.0-rc.209 detailing the latest bug fix. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
This change fixes normalization for cases like the following:
I have marged two different visitors and their tests into a single visitor. EnterSelectionSets from two stages were merged into one, but with the following differences:
Summary by CodeRabbit
Refactor
Tests
Checklist