Conversation
For an operation, the engine validates if arguments of the field satisfy the configured listSize:requireOneSlicingArgument option.
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy,
dkorittki and
jensneuse
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📝 WalkthroughWalkthroughAdds runtime validation of list-slicing arguments: after plan lookup, ExecutionEngine calls CostCalculator.ValidateSliceArguments(...) using planner config and request variables and returns immediately on reported validation errors; supporting cost-tree, metadata, debug-print, and tests were added/updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionEngine as EE
participant Planner
participant CostCalculator as CostCalc
participant Operation as Op
Client->>EE: send request
EE->>Planner: resolve plan (cached or fresh)
Planner-->>EE: return plan
EE->>CostCalc: ValidateSliceArguments(config, variables, report)
alt validation fails
CostCalc-->>EE: report with errors
EE-->>Client: return validation errors
else validation succeeds
EE->>Operation: ComputeEstimatedCost(plan, variables, report)
Operation-->>EE: estimated cost
EE-->>Client: continue execution / return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ysmolski
changed the title
ysmolski
changed the title
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e6071fa-bd55-4992-a2d5-a065f8f70f5b
📒 Files selected for processing (5)
execution/engine/execution_engine.goexecution/engine/execution_engine_cost_test.gov2/pkg/engine/plan/cost.gov2/pkg/engine/plan/cost_visitor.gov2/pkg/engine/plan/federation_metadata.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 657-672: The count logic in cost.go currently only increments for
variable-based slicing arguments; update the loop over listSize.SlicingArguments
(using node.arguments and the arg variable) to also count literal integer
slicing arguments: inside the loop, keep the existing branch that checks if
arg.hasVariable and consults variables.Get(arg.varName), but add an else-if that
checks arg.intValue > 0 (or the equivalent literal value field on arg) and
increments count for those literals as well so literal slicing args (e.g.,
users(first: 10)) are included in the validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b435e971-573f-4213-8af2-848448b16978
📒 Files selected for processing (1)
v2/pkg/engine/plan/cost.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 646-653: The current logic only consults
node.parent.requiringOneArgImplementingField when listSize is nil, so an
abstract field's listSize with RequireOneSlicingArgument=false can mask a
concrete implementation that requires one; change the logic so that when
node.isEnclosingTypeAbstract and node.parent != nil you always consult the
implementing-type info via
node.parent.requiringOneArgImplementingField(dsCostConfig,
node.fieldCoords.FieldName) and if that returns a non-nil listSize with
RequireOneSlicingArgument==true then treat the effective listSize as requiring
one slicing argument (either by replacing listSize or setting
RequireOneSlicingArgument=true), before the subsequent check that enforces
RequireOneSlicingArgument and SlicingArguments length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d12531ec-b49b-4ce1-a692-40d12016b18d
📒 Files selected for processing (1)
v2/pkg/engine/plan/cost.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v2/pkg/engine/plan/cost.go (1)
616-623:⚠️ Potential issue | 🟠 MajorDon't let the abstract field config mask a stricter implementation.
This only consults
requiringOneArgImplementingFieldwhen the abstract coordinate has nolistSize. If the interface field is configured withRequireOneSlicingArgument=falseand any concrete implementation sets it totrue, the abstract selection is incorrectly accepted.Based on learnings: In wundergraph/graphql-go-tools (v2/pkg/engine/plan/cost.go), when validating `RequireOneSlicingArgument` for abstract fields, the intended behavior is: if ANY implementing concrete type has `RequireOneSlicingArgument=true`, the operation must satisfy the slicing argument constraint.💡 Suggested fix
- listSize := dsCostConfig.ListSizes[node.fieldCoords] - if listSize == nil && node.isEnclosingTypeAbstract && node.parent != nil && node.parent.returnsAbstractType { - // We pick the first from the list of implementing types. Composition should verify that - // all implementations are aligned on the slicingArguments within the single subgraph. - // Otherwise, we would have inconsistent expectations between implementing types. - listSize = node.parent.requiringOneArgImplementingField(dsCostConfig, node.fieldCoords.FieldName) - } + listSize := dsCostConfig.ListSizes[node.fieldCoords] + if node.isEnclosingTypeAbstract && node.parent != nil && node.parent.returnsAbstractType { + // Any strict implementation must make the abstract selection strict as well. + if requiring := node.parent.requiringOneArgImplementingField(dsCostConfig, node.fieldCoords.FieldName); requiring != nil && + (listSize == nil || !listSize.RequireOneSlicingArgument) { + listSize = requiring + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/plan/cost.go` around lines 616 - 623, When handling abstract fields in calculate cost, also consult implementing-type configs even when the abstract `listSize` is present: call node.parent.requiringOneArgImplementingField(dsCostConfig, node.fieldCoords.FieldName) (when node.isEnclosingTypeAbstract && node.parent != nil) and compute an effective requirement like effectiveRequireOne := (listSize != nil && listSize.RequireOneSlicingArgument) || (implListSize != nil && implListSize.RequireOneSlicingArgument); then use effectiveRequireOne and the presence of slicing args from either listSize or implListSize to decide the error path (e.g., if effectiveRequireOne && (both SlicingArguments lists are empty or missing) then treat as missing slicing argument). Update the checks around listSize, RequireOneSlicingArgument and SlicingArguments accordingly so a concrete implementation requiring a slicing arg cannot be masked by the abstract field config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/engine/execution_engine_cost_test.go`:
- Around line 3415-3425: The test currently sets RequireOneSlicingArgument=true
for all implementations so it never exercises the mixed abstract-policy path;
update the ListSizes map in execution_engine_cost_test.go (the entries keyed by
plan.FieldCoordinate like {TypeName: "UserPaginated", FieldName: "items"} and
{TypeName: "PostPaginated", FieldName: "items"}) to create a mixed-policy case —
e.g., make one implementation have RequireOneSlicingArgument=false (or add a new
concrete implementation entry with false) while another stays true — so the
abstractCostConfig code path that treats abstract fields as requiring an
argument if ANY implementation requires it is exercised and the test assertions
reflect that behavior.
---
Duplicate comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 616-623: When handling abstract fields in calculate cost, also
consult implementing-type configs even when the abstract `listSize` is present:
call node.parent.requiringOneArgImplementingField(dsCostConfig,
node.fieldCoords.FieldName) (when node.isEnclosingTypeAbstract && node.parent !=
nil) and compute an effective requirement like effectiveRequireOne := (listSize
!= nil && listSize.RequireOneSlicingArgument) || (implListSize != nil &&
implListSize.RequireOneSlicingArgument); then use effectiveRequireOne and the
presence of slicing args from either listSize or implListSize to decide the
error path (e.g., if effectiveRequireOne && (both SlicingArguments lists are
empty or missing) then treat as missing slicing argument). Update the checks
around listSize, RequireOneSlicingArgument and SlicingArguments accordingly so a
concrete implementation requiring a slicing arg cannot be masked by the abstract
field config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 205f35aa-e7a9-46c5-8792-0d1d83463aa8
📒 Files selected for processing (4)
execution/engine/execution_engine.goexecution/engine/execution_engine_cost_test.goexecution/graphql/request.gov2/pkg/engine/plan/cost.go
✅ Files skipped from review due to trivial changes (1)
- execution/engine/execution_engine.go
devsergiy
approved these changes
Mar 25, 2026
ysmolski
changed the title
ysmolski
changed the title
…ingargument-in-queries
ysmolski
deleted the
yury/eng-8842-cost-validate-requireoneslicingargument-in-queries
branch
This was referenced Mar 26, 2026
ysmolski
pushed a commit
that referenced
this pull request
Mar 26, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.267](v2.0.0-rc.266...v2.0.0-rc.267) (2026-03-26) ### Features * check slicing Arguments passed when requireOneSlicingArgument ([#1456](#1456)) ([72c181f](72c181f))
ysmolski
pushed a commit
that referenced
this pull request
Apr 8, 2026
## [1.11.0](execution/v1.10.0...execution/v1.11.0) (2026-04-08) ### Features * check slicing Arguments passed when requireOneSlicingArgument ([#1456](#1456)) ([72c181f](72c181f)) * handle recursion for arguments containing input objects ([#1461](#1461)) ([ba21793](ba21793)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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
2 participants
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.
For an operation, the engine validates if slicing arguments of the field satisfy the configured listSize:requireOneSlicingArgument option.
Summary by CodeRabbit