Open
Conversation
This PR enables the use of dot-path slicing arguments in a listSize
directives. If an operation uses deeply nested input objects, then user
can specify the deeply nested int value to be used as an estimation of
the returned list size:
input PaginationInput {
first: Int
after: String
}
input SearchInput {
pagination: PaginationInput
query: String
}
type Query {
search(input: SearchInput!): [Book]
@listsize(slicingArguments: ["input.pagination.first"])
}
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 and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📝 WalkthroughWalkthroughAdds support for dot-path slicing arguments (e.g., "pagination.limit.first") so the cost calculator can extract integer slice sizes from nested input-object variables. Implements a helper to traverse variable JSON, updates multiplier and validation logic, and expands tests with dot-path literal/variable and null/missing cases. ChangesDot-Path Slicing Argument Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
execution/engine/execution_engine_cost_test.go (1)
1477-1565: ⚡ Quick winAdd a regression for omitted
$inputvariable (empty variables object).Current required dot-path invalid tests cover missing intermediate and null leaf, but not the root variable being absent (
{}), which is a distinct path worth locking down.🧪 Suggested test addition
+ t.Run("required dot-path slicing argument with omitted root variable is invalid", runWithAndCompareError( + ExecutionEngineTestCase{ + schema: schemaSlicing, + operation: func(t *testing.T) graphql.Request { + return graphql.Request{ + Query: `query NestedInput($input: SInput) { + search(input: $input) { id } + }`, + Variables: []byte(`{}`), + } + }, + dataSources: []plan.DataSource{ + // same datasource setup as other required-dot-path invalid cases + }, + fields: fieldConfig, + }, + "external: field 'Query.search' requires exactly one slicing argument, but none was provided, locations: [], path: [search]", + computeCosts(), + ))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/engine/execution_engine_cost_test.go` around lines 1477 - 1565, Add a regression test that mirrors the two existing "required dot-path slicing argument missing" cases but uses an empty variables object (Variables: []byte(`{}`)) so the root $input variable is omitted; update the suite by adding a new t.Run using runWithAndCompareError with ExecutionEngineTestCase referencing schemaSlicing, the same mustGraphqlDataSourceConfiguration and DataSourceCostConfig (SlicingArguments: []string{"input.pagination.first"}, RequireOneSlicingArgument: true) and assert the same error string "external: field 'Query.search' requires exactly one slicing argument, but none was provided, locations: [], path: [search]" while keeping computeCosts() as the executor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@execution/engine/execution_engine_cost_test.go`:
- Around line 1477-1565: Add a regression test that mirrors the two existing
"required dot-path slicing argument missing" cases but uses an empty variables
object (Variables: []byte(`{}`)) so the root $input variable is omitted; update
the suite by adding a new t.Run using runWithAndCompareError with
ExecutionEngineTestCase referencing schemaSlicing, the same
mustGraphqlDataSourceConfiguration and DataSourceCostConfig (SlicingArguments:
[]string{"input.pagination.first"}, RequireOneSlicingArgument: true) and assert
the same error string "external: field 'Query.search' requires exactly one
slicing argument, but none was provided, locations: [], path: [search]" while
keeping computeCosts() as the executor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a41d237b-36e1-4494-bb19-7192b052eb64
📒 Files selected for processing (2)
execution/engine/execution_engine_cost_test.gov2/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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@execution/engine/execution_engine_cost_test.go`:
- Around line 1431-1609: The failing test "required dot-path slicing argument
missing leaf is invalid" currently sends Variables with
`"pagination":{"first":null}` which doesn't exercise a truly absent leaf; update
the operation function for that test (the graphql.Request Variables payload in
the test case inside t.Run "required dot-path slicing argument missing leaf is
invalid") to use an omitted leaf such as `"pagination":{}` (e.g.
`{"input":{"pagination":{},"query":"abc"}}`) so the dot-path walker sees the key
missing rather than null; keep the rest of the test (dataSources, CostConfig,
expected error) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08057027-7aaf-4581-a204-78b5d7a39297
📒 Files selected for processing (1)
execution/engine/execution_engine_cost_test.go
… yury/eng-9338-router-support-dot-path-in-slicingarguments
… yury/eng-9338-router-support-dot-path-in-slicingarguments
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.
This PR enables the use of dot-path slicing arguments in a listSize directives. If an operation uses deeply nested input objects, then user can specify the deeply nested int value to be used as an estimation of the returned list size: