fix: fix conflict of rewriter and required fields#1338
Merged
Conversation
This reverts commit ec723fd.
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughRefactors AST debug printing, required-fields fragment construction and API, selection-rewriter options (forceRewrite), datasource skipping in path building, orphan-marking for node suggestions, and multiple visitor flows; adds tests and tightens planner-level error messages. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Visitor as Selection Visitor
participant ReqHandler as handleRequiresInSelectionSet
participant Pending as PendingRequirements
participant EnterField as EnterField
participant Rewriter as FieldSelectionRewriter
Note over Visitor,Rewriter: Early required-fields handling + force-rewrite
Visitor->>ReqHandler: EnterSelectionSet
activate ReqHandler
ReqHandler->>Pending: Pre-populate pending requirements
deactivate ReqHandler
Visitor->>EnterField: EnterField(ref)
activate EnterField
alt field previously rewritten
EnterField->>Rewriter: newFieldSelectionRewriter(..., withForceRewrite)
Rewriter-->>EnterField: rewritten selection
end
EnterField->>Pending: Add pending deps (skip self-ref)
deactivate EnterField
sequenceDiagram
participant PathPlanner as Path Builder
participant EnterField as EnterField
participant DSMatcher as Datasource Matcher
participant SkipDS as skipDS state
participant LeaveField as LeaveField
Note over PathPlanner,LeaveField: Per-datasource skip tracking (DSSkip)
PathPlanner->>EnterField: Visit field ref
activate EnterField
EnterField->>DSMatcher: Can this DS plan the field?
alt DS is in skipDS for this field
DSMatcher-->>EnterField: Skip datasource
else Cannot plan on this DS
EnterField->>SkipDS: append DSSkip{popOnFieldRef: ref, DSHash: ds}
else Success
EnterField->>PathPlanner: Process nested fields
end
deactivate EnterField
PathPlanner->>LeaveField: LeaveField(ref)
activate LeaveField
LeaveField->>SkipDS: Remove DSSkip entries for ref
LeaveField->>PathPlanner: Cleanup arrays/response-path
deactivate LeaveField
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
devsergiy
requested review from
Noroth,
StarpTech,
jensneuse and
ysmolski
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/plan/node_selection_visitor.go (1)
63-66: InitializenewFieldRefsbefore writing to it
addNewFieldRefswrites intoc.newFieldRefs, but that map is never initialized. The first time we append required fields we executeaddSkipFieldRefs → addNewFieldRefs, which will panic withassignment to entry in nil map. Please ensure the map is created before storing entries (e.g., guard insideaddNewFieldRefsor initialize/reset it inEnterDocument).func (c *nodeSelectionVisitor) addNewFieldRefs(fieldRefs ...int) { + if c.newFieldRefs == nil { + c.newFieldRefs = make(map[int]struct{}, len(fieldRefs)) + } for _, fieldRef := range fieldRefs { c.newFieldRefs[fieldRef] = struct{}{} } }
🧹 Nitpick comments (1)
v2/pkg/engine/plan/datasource_filter_node_suggestions.go (1)
752-777: Guard against reselecting orphaned suggestions.
checkNodesnow builds its candidate list without filteringIsOrphan. After we mark a subtree as orphaned (e.g., viaRemoveTreeNodeChilds), the tree node’s data slice still contains those indices, so the duplicate-selection stages can resurrect them. Please skip orphans here to keep the new invariant airtight.Apply this diff:
for _, i := range duplicates { - if f.nodes.items[i].IsExternal && !f.nodes.items[i].IsProvided { + if f.nodes.items[i].IsOrphan { + continue + } + + if f.nodes.items[i].IsExternal && !f.nodes.items[i].IsProvided { continue }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
v2/pkg/astprinter/astprinter.go(3 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(7 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_entity_interfaces_test.go(9 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(1 hunks)v2/pkg/engine/plan/abstract_selection_rewriter.go(1 hunks)v2/pkg/engine/plan/datasource_filter_node_suggestions.go(5 hunks)v2/pkg/engine/plan/datasource_filter_visitor.go(2 hunks)v2/pkg/engine/plan/federation_metadata.go(2 hunks)v2/pkg/engine/plan/node_selection_builder.go(6 hunks)v2/pkg/engine/plan/node_selection_visitor.go(9 hunks)v2/pkg/engine/plan/path_builder.go(1 hunks)v2/pkg/engine/plan/path_builder_visitor.go(3 hunks)v2/pkg/engine/plan/required_fields_visitor.go(1 hunks)v2/pkg/engine/plan/required_fields_visitor_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/plan/datasource_filter_visitor.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_entity_interfaces_test.gov2/pkg/engine/plan/path_builder_visitor.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-07-09T09:34:51.269Z
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1218
File: v2/pkg/engine/resolve/loader.go:58-60
Timestamp: 2025-07-09T09:34:51.269Z
Learning: In Go's bytes package, the String() method on *bytes.Buffer handles nil receivers gracefully by returning an empty string rather than panicking, making additional nil checks unnecessary when calling String() on potentially nil *bytes.Buffer instances.
Applied to files:
v2/pkg/engine/plan/federation_metadata.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/plan/path_builder_visitor.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
🧬 Code graph analysis (11)
v2/pkg/engine/plan/path_builder.go (1)
v2/pkg/ast/path.go (1)
FieldName(23-23)
v2/pkg/engine/plan/abstract_selection_rewriter.go (1)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(277-290)
v2/pkg/astprinter/astprinter.go (2)
v2/pkg/lexer/literal/literal.go (2)
RPAREN(36-36)LBRACE(39-39)v2/pkg/astvisitor/simplevisitor.go (1)
SimpleWalker(9-17)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_entity_interfaces_test.go (4)
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 (3)
Object(8-16)Object(30-32)Field(89-98)v2/pkg/engine/resolve/node_scalar.go (6)
StaticString(103-106)StaticString(108-110)String(48-54)String(91-93)Scalar(5-9)Scalar(11-13)
v2/pkg/engine/plan/path_builder_visitor.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DSHash(15-15)v2/pkg/astvisitor/visitor.go (1)
LeaveField(736-736)
v2/pkg/engine/plan/node_selection_builder.go (1)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (1)
TreeNodeID(536-541)
v2/pkg/engine/plan/node_selection_visitor.go (3)
v2/pkg/astvisitor/visitor.go (2)
LeaveSelectionSet(734-734)EnterField(735-735)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfiguration(76-85)v2/pkg/ast/ast_selection.go (1)
SelectionSet(20-24)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
v2/pkg/engine/plan/required_fields_visitor.go (1)
QueryPlanRequiredFieldsFragment(48-51)
v2/pkg/engine/plan/required_fields_visitor_test.go (1)
v2/pkg/engine/plan/required_fields_visitor.go (1)
QueryPlanRequiredFieldsFragment(48-51)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (13)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSourceMetadata(34-59)DataSource(277-290)v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)v2/pkg/engine/plan/federation_metadata.go (3)
FederationMetaData(10-18)EntityInterfaceConfiguration(71-74)FederationFieldConfigurations(120-120)v2/pkg/engine/resolve/fetch.go (5)
Fetch(20-27)FetchConfiguration(270-302)SingleFetch(91-99)SingleFetch(153-155)FetchDependencies(111-114)v2/pkg/engine/plan/configuration.go (2)
Configuration(9-49)DebugConfiguration(51-64)v2/pkg/engine/datasourcetesting/datasourcetesting.go (2)
RunWithPermutations(87-108)WithDefaultPostProcessor(51-53)v2/pkg/engine/resolve/fetchtree.go (6)
Sequence(26-31)Single(59-67)SingleWithPath(69-82)ObjectPath(40-45)ArrayPath(52-57)PathElementWithTypeNames(47-50)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (4)
DefaultPostProcessingConfiguration(40-43)Source(1830-1832)EntitiesPostProcessingConfiguration(44-47)SingleEntityPostProcessingConfiguration(48-51)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 (3)
Object(8-16)Object(30-32)Field(89-98)v2/pkg/engine/resolve/node_scalar.go (4)
String(48-54)String(91-93)Scalar(5-9)Scalar(11-13)v2/pkg/engine/resolve/node_array.go (2)
Array(9-15)Array(23-25)
v2/pkg/engine/plan/required_fields_visitor.go (3)
v2/pkg/ast/ast.go (1)
Document(10-56)v2/pkg/operationreport/operationreport.go (1)
Report(9-12)v2/pkg/astparser/parser.go (1)
ParseGraphqlDocumentString(37-45)
⏰ 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 (8)
v2/pkg/engine/plan/required_fields_visitor_test.go (1)
610-610: LGTM! Test correctly updated for API change.The parameter order has been updated to match the new signature of
QueryPlanRequiredFieldsFragment(typeName, fieldName, requiredFields).v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (2)
1375-1375: LGTM! API usage correctly updated.The function calls have been properly updated to use
QueryPlanRequiredFieldsFragmentwith the new signature(typeName, fieldName, requiredFields). TheincludeTypenameboolean logic (fieldName == "") is now handled internally by the function.Also applies to: 1410-1410
1449-1449: LGTM! Enhanced error context.Adding the planner ID to error messages improves observability and debugging, especially when multiple planners are in use.
Also applies to: 1460-1460, 1471-1471, 1481-1481, 1496-1496
v2/pkg/engine/plan/node_selection_builder.go (4)
61-61: Verify the visitor registration change.The change from
RegisterEnterDocumentVisitortoRegisterDocumentVisitormeans the walker will now call bothEnterDocumentandLeaveDocumenton the visitor. Please confirm thatnodeSelectionVisitorproperly implements or handlesLeaveDocument, as this could introduce unexpected behavior if the visitor doesn't support it.
128-180: LGTM! Improved handling of unresolved fields.The addition of
hasUnresolvedFieldstracking enables the loop to retry resolution when fields remain unresolved, while the 100-iteration guard prevents infinite loops. The logic correctly sets the flag on errors and resets it on success.
182-194: LGTM! Correct error handling for initial run.The first-run resolvability check now correctly returns immediately on error, as there's no point continuing if the operation cannot be resolved and no new fields were added.
221-243: LGTM! Enhanced debug output with defensive checks.The restructured printing logic adds defensive nil checks and includes helpful skip field markers in the debug output. The use of
slices.Contains(imported at line 6) is appropriate for checking skip field references.v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
13070-13336: Great coverage on rewritesAppreciate how this scenario locks down the tricky rewrite path while asserting the fetch dependency graph end-to-end. This should stop regressions around required-field rewrites from sneaking back in.
devsergiy
deleted the
sergiy/eng-8243-fix-conflict-of-rewriter-and-required-fields
branch
devsergiy
pushed a commit
that referenced
this pull request
Oct 31, 2025
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.236](v2.0.0-rc.235...v2.0.0-rc.236) (2025-10-31) ### Bug Fixes * fix conflict of rewriter and required fields ([#1338](#1338)) ([03189bc](03189bc)) --- 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 ## Release Notes * **Bug Fixes** * Resolved conflict between rewriter and required fields <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 tasks
3 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Checklist