Conversation
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCost computation was refactored so CostCalculator is constructed with and caches Configuration; public cost APIs no longer accept Configuration. Input-object argument costs are computed recursively. Call sites, planner wiring, request APIs, and tests were updated; tests now use nullable expected-cost pointers. Changes
Sequence Diagram(s)mermaid 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![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/cost_visitor.go (1)
166-220:⚠️ Potential issue | 🟠 MajorInline input-object literals still bypass this recursion.
Line 179 only handles
ast.ValueKindVariable, so Lines 210-212 and 223-264 are only reached after variable extraction.execution/engine.ExecutionEngine.Executedoes that normalization first, butplan.NewPlanner(...).Plan(...)does not. Unless the caller pre-normalizes the operation, a direct planner call likefield(arg: { nested: { ... } })will miss input-object field costs and can underestimate the result. Either normalize literals before the cost walk whenComputeCostsis enabled, or add object/list-literal handling here.Based on learnings: In wundergraph/graphql-go-tools, the execution engine converts all inline literal argument values into variables before cost validation runs.
Also applies to: 223-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/plan/cost_visitor.go` around lines 166 - 220, The extractor in CostVisitor.extractFieldArguments currently only handles ast.ValueKindVariable and therefore ignores inline object/list literals, causing input-object field costs to be missed; update extractFieldArguments (inside the switch on argValue.Kind) to also handle ast.ValueKindObject and ast.ValueKindList by detecting input-object literal shapes, marking argInfo.isInputObject (and isSimple where appropriate), resolving the literal's underlying type like you do for variables (use v.Operation.ResolveUnderlyingType / v.Operation.TypeNameString and lookup with v.Definition.NodeByNameStr), and call v.buildInputObjectFieldTypes to populate argInfo.inputObjectFieldTypes for nested fields; alternatively, if you prefer normalization, ensure callers of plan.NewPlanner(...).Plan(...) normalize inline argument literals into variables the same way execution/engine.ExecutionEngine.Execute does before cost computation so extractFieldArguments can rely on variables alone.
🧹 Nitpick comments (2)
v2/pkg/engine/plan/cost.go (1)
231-236: Honor the documentedisListprecedence here.
inputObjectFieldsaysisListtakes priority, butinputObjectCostchecksisInputObjectfirst. If a list-of-input-objects ever sets both flags, the walker will callGetObject()on the array and skip its elements.Small alignment fix
- if typeInfo.isInputObject { - valueObj := value.GetObject() - if valueObj != nil { - cost += inputObjectCost(typeInfo.unwrappedTypeName, valueObj, weights, types) - } - } else if typeInfo.isList { + if typeInfo.isList { valueArray := value.GetArray() - if valueArray != nil { - for _, item := range valueArray { - if item.Type() == astjson.TypeObject { - cost += inputObjectCost(typeInfo.unwrappedTypeName, item.GetObject(), weights, types) - } - } + for _, item := range valueArray { + if item.Type() == astjson.TypeObject { + cost += inputObjectCost(typeInfo.unwrappedTypeName, item.GetObject(), weights, types) + } } + } else if typeInfo.isInputObject { + valueObj := value.GetObject() + if valueObj != nil { + cost += inputObjectCost(typeInfo.unwrappedTypeName, valueObj, weights, types) + } }Also applies to: 577-603
🤖 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 231 - 236, The inputObjectField struct documents that isList takes precedence over isInputObject, but inputObjectCost currently checks isInputObject first and thus treats list-of-input-objects as an input object (calling GetObject() on the array); update inputObjectCost (and the similar logic around the other block noted ~577-603) to check isList first and, when true, iterate over list elements (e.g., process each element or call GetArray/GetAt) rather than treating the whole value as an input object; reference inputObjectField, inputObjectCost, and any walker/GetObject/GetArray calls to locate and adjust the branching order so lists win over input object handling.execution/engine/execution_engine_cost_test.go (1)
4044-4127: Add one case where a nested negative subtotal goes below zero.These tests cover whole-node clamping and an exact-zero nested subtotal, but they do not distinguish that from “only clamp at the outer field.” A positive sibling plus an over-discounted child would lock that behavior down.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/execution_engine_cost_test.go` around lines 4044 - 4127, Add a new test case in execution_engine_cost_test.go that exercises a nested subtotal that would go below zero but is combined with a positive sibling to ensure clamping happens at the nested subtotal (not just the outer field). Copy the pattern used by the surrounding t.Run cases: create an ExecutionEngineTestCase with schema, operation returning a graphql.Request whose input causes the nested child subtotal to be negative (e.g., an inner reduction larger than its base so inner subtotal < 0) while the outer object includes a positive field; use mustGraphqlDataSourceConfiguration/mustFactory/testNetHttpClient and the same CostConfig/fieldConfig, call computeCosts(), and set expectedEstimatedCost/expectedActualCost to the sum where the negative nested subtotal is floored to zero and only the positive sibling contributes. Ensure the test name clearly indicates "nested negative subtotal clamped" and reference the same data source metadata (RootNodes/CostConfig) as other cases.
🤖 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/graphql/request.go`:
- Around line 213-220: In ComputeActualCost on Request, restore the nil-reset
path: if actualListSizes == nil then set r.actualCost = 0 and return (do not
call calc.ActualCost); otherwise if calc == nil set r.actualCost = 0 as before,
else call calc.ActualCost(variables, actualListSizes). Update the
Request.ComputeActualCost function to check actualListSizes for nil before
invoking plan.CostCalculator.ActualCost so a reused Request won't retain a stale
non-zero actualCost.
---
Outside diff comments:
In `@v2/pkg/engine/plan/cost_visitor.go`:
- Around line 166-220: The extractor in CostVisitor.extractFieldArguments
currently only handles ast.ValueKindVariable and therefore ignores inline
object/list literals, causing input-object field costs to be missed; update
extractFieldArguments (inside the switch on argValue.Kind) to also handle
ast.ValueKindObject and ast.ValueKindList by detecting input-object literal
shapes, marking argInfo.isInputObject (and isSimple where appropriate),
resolving the literal's underlying type like you do for variables (use
v.Operation.ResolveUnderlyingType / v.Operation.TypeNameString and lookup with
v.Definition.NodeByNameStr), and call v.buildInputObjectFieldTypes to populate
argInfo.inputObjectFieldTypes for nested fields; alternatively, if you prefer
normalization, ensure callers of plan.NewPlanner(...).Plan(...) normalize inline
argument literals into variables the same way
execution/engine.ExecutionEngine.Execute does before cost computation so
extractFieldArguments can rely on variables alone.
---
Nitpick comments:
In `@execution/engine/execution_engine_cost_test.go`:
- Around line 4044-4127: Add a new test case in execution_engine_cost_test.go
that exercises a nested subtotal that would go below zero but is combined with a
positive sibling to ensure clamping happens at the nested subtotal (not just the
outer field). Copy the pattern used by the surrounding t.Run cases: create an
ExecutionEngineTestCase with schema, operation returning a graphql.Request whose
input causes the nested child subtotal to be negative (e.g., an inner reduction
larger than its base so inner subtotal < 0) while the outer object includes a
positive field; use
mustGraphqlDataSourceConfiguration/mustFactory/testNetHttpClient and the same
CostConfig/fieldConfig, call computeCosts(), and set
expectedEstimatedCost/expectedActualCost to the sum where the negative nested
subtotal is floored to zero and only the positive sibling contributes. Ensure
the test name clearly indicates "nested negative subtotal clamped" and reference
the same data source metadata (RootNodes/CostConfig) as other cases.
In `@v2/pkg/engine/plan/cost.go`:
- Around line 231-236: The inputObjectField struct documents that isList takes
precedence over isInputObject, but inputObjectCost currently checks
isInputObject first and thus treats list-of-input-objects as an input object
(calling GetObject() on the array); update inputObjectCost (and the similar
logic around the other block noted ~577-603) to check isList first and, when
true, iterate over list elements (e.g., process each element or call
GetArray/GetAt) rather than treating the whole value as an input object;
reference inputObjectField, inputObjectCost, and any walker/GetObject/GetArray
calls to locate and adjust the branching order so lists win over input object
handling.
🪄 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: 694a1c9a-fda3-4b65-a854-a60aa67fbde7
📒 Files selected for processing (8)
execution/engine/execution_engine.goexecution/engine/execution_engine_cost_test.goexecution/engine/execution_engine_test.goexecution/graphql/request.gov2/pkg/ast/ast_type.gov2/pkg/engine/plan/cost.gov2/pkg/engine/plan/cost_visitor.gov2/pkg/engine/plan/planner.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
v2/pkg/engine/plan/cost_visitor.go (1)
239-244: Minor: Redundant type name resolution.
ResolveTypeNameString(fieldTypeRef)is called twice - once to setunwrappedTypeNameand again insideNodeByNameStr. Consider reusing the already-computed value.♻️ Suggested simplification
fieldTypeRef := v.Definition.InputValueDefinitionType(inputFieldRef) unwrappedTypeName := v.Definition.ResolveTypeNameString(fieldTypeRef) -fieldNode, exists := v.Definition.NodeByNameStr(v.Definition.ResolveTypeNameString(fieldTypeRef)) +fieldNode, exists := v.Definition.NodeByNameStr(unwrappedTypeName) if !exists { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/plan/cost_visitor.go` around lines 239 - 244, The code redundantly calls ResolveTypeNameString(fieldTypeRef) twice; compute it once into unwrappedTypeName and reuse that when fetching the node instead of calling ResolveTypeNameString a second time. Update the NodeByNameStr call to use unwrappedTypeName (e.g., fieldNode, exists := v.Definition.NodeByNameStr(unwrappedTypeName)) so only one resolution occurs, leaving v.Definition, ResolveTypeNameString, unwrappedTypeName, NodeByNameStr, and fieldTypeRef as the referenced symbols.v2/pkg/engine/plan/cost.go (3)
592-598: Edge case: Nested arrays ([[InputObject]]) not fully handled.When
isList=trueand the JSON contains nested arrays (e.g.,[[{...}]]), the inner arrays are skipped sinceitem.Type() == astjson.TypeObjectis false for array items. If nested list support is needed, this would require additional handling.This is likely acceptable for the current scope since nested list arguments are uncommon in GraphQL schemas. Consider documenting this limitation or adding support if use cases emerge.
🤖 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 592 - 598, The list-handling branch in typeInfo.isList uses value.GetArray() and only processes items where item.Type() == astjson.TypeObject, skipping nested arrays like [[InputObject]]; update the logic in the isList branch (around typeInfo.isList, value.GetArray(), and inputObjectCost / typeInfo.unwrappedTypeName) to detect astjson.TypeArray items and either recursively iterate into the inner arrays until you reach objects (calling inputObjectCost for objects) or call a small helper that walks nested arrays and invokes inputObjectCost for any encountered objects; alternatively, if nested lists are intentionally unsupported, add a clear comment documenting the limitation.
630-634: Redundant nil check after constructor change.
NewCostCalculatornow always stores a non-nilDataSourceCostConfig(creating an empty one ifGetCostConfig()returns nil). The checkdsCostConfig == nilat line 407 will never be true for configs set by this constructor.♻️ Option 1: Remove the nil assignment to skip empty configs entirely
for _, ds := range config.DataSources { dsCostConfig := ds.GetCostConfig() - if dsCostConfig == nil { - dsCostConfig = &DataSourceCostConfig{} + if dsCostConfig == nil { + continue // Skip data sources without cost configuration } c.costConfigs[ds.Hash()] = dsCostConfig }This would avoid storing empty configs and make the
nilcheck at line 407 meaningful again, aligning with the PR summary which mentions "skipping data sources that have no cost configuration".🤖 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 630 - 634, The nil-check is redundant because NewCostCalculator currently creates and stores an empty DataSourceCostConfig, so change the constructor to only store a config when the source actually provides one: call ds.GetCostConfig(), and if it returns non-nil assign it to c.costConfigs[ds.Hash()]; do not replace a nil with &DataSourceCostConfig{} — this removes storing empty configs and makes later nil checks (e.g., in cost calculation code) meaningful; update the code path around NewCostCalculator and the use of DataSourceCostConfig/GetCostConfig so only real configs are inserted.
456-476: Code correctly implements additive argument weights per IBM GraphQL Cost Specification.Per the IBM GraphQL Cost Specification, explicit argument weights are intentionally additive with input object field-level costs. The
@costdirective applies to both ARGUMENT_DEFINITION and INPUT_FIELD_DEFINITION, and cost calculation recursively sums the argument weight plus all input field costs. The current logic at lines 461 and 469 is spec-compliant. Consider adding a comment referencing the IBM spec to clarify this behavior for future maintainers.🤖 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 456 - 476, The argument-weight logic in the node.arguments loop (using argumentWeightFound, arg.isInputObject, arg.inputFieldsCost, dsCostConfig.EnumScalarTypeWeight, and dsCostConfig.ObjectTypeWeight) is correct per the IBM GraphQL Cost Specification but is unclear to future readers; add a concise inline comment above the block explaining that explicit argument weights are intentionally additive with input object field-level costs per the IBM spec (`@cost` applies to ARGUMENT_DEFINITION and INPUT_FIELD_DEFINITION and should be summed recursively), and include a link or reference to the IBM spec for maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v2/pkg/engine/plan/cost_visitor.go`:
- Around line 239-244: The code redundantly calls
ResolveTypeNameString(fieldTypeRef) twice; compute it once into
unwrappedTypeName and reuse that when fetching the node instead of calling
ResolveTypeNameString a second time. Update the NodeByNameStr call to use
unwrappedTypeName (e.g., fieldNode, exists :=
v.Definition.NodeByNameStr(unwrappedTypeName)) so only one resolution occurs,
leaving v.Definition, ResolveTypeNameString, unwrappedTypeName, NodeByNameStr,
and fieldTypeRef as the referenced symbols.
In `@v2/pkg/engine/plan/cost.go`:
- Around line 592-598: The list-handling branch in typeInfo.isList uses
value.GetArray() and only processes items where item.Type() ==
astjson.TypeObject, skipping nested arrays like [[InputObject]]; update the
logic in the isList branch (around typeInfo.isList, value.GetArray(), and
inputObjectCost / typeInfo.unwrappedTypeName) to detect astjson.TypeArray items
and either recursively iterate into the inner arrays until you reach objects
(calling inputObjectCost for objects) or call a small helper that walks nested
arrays and invokes inputObjectCost for any encountered objects; alternatively,
if nested lists are intentionally unsupported, add a clear comment documenting
the limitation.
- Around line 630-634: The nil-check is redundant because NewCostCalculator
currently creates and stores an empty DataSourceCostConfig, so change the
constructor to only store a config when the source actually provides one: call
ds.GetCostConfig(), and if it returns non-nil assign it to
c.costConfigs[ds.Hash()]; do not replace a nil with &DataSourceCostConfig{} —
this removes storing empty configs and makes later nil checks (e.g., in cost
calculation code) meaningful; update the code path around NewCostCalculator and
the use of DataSourceCostConfig/GetCostConfig so only real configs are inserted.
- Around line 456-476: The argument-weight logic in the node.arguments loop
(using argumentWeightFound, arg.isInputObject, arg.inputFieldsCost,
dsCostConfig.EnumScalarTypeWeight, and dsCostConfig.ObjectTypeWeight) is correct
per the IBM GraphQL Cost Specification but is unclear to future readers; add a
concise inline comment above the block explaining that explicit argument weights
are intentionally additive with input object field-level costs per the IBM spec
(`@cost` applies to ARGUMENT_DEFINITION and INPUT_FIELD_DEFINITION and should be
summed recursively), and include a link or reference to the IBM spec for
maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f16ab907-603c-4d82-94f4-27688e812c87
📒 Files selected for processing (2)
v2/pkg/engine/plan/cost.gov2/pkg/engine/plan/cost_visitor.go
ysmolski
added a commit
to wundergraph/cosmo
that referenced
this pull request
Mar 31, 2026
This PR adds support of input object fields passed as arguments. It handles nested input objects, recursive types, list of input objects and list-typed arguments. Negative weights on input fields reduce the cost, but clamped to zero for each field. Engine PR: wundergraph/graphql-go-tools#1461
devsergiy
approved these changes
Apr 7, 2026
… yury/eng-8731-cost-handle-recursion-for-arguments-containing-input-objects
ysmolski
deleted the
yury/eng-8731-cost-handle-recursion-for-arguments-containing-input-objects
branch
ysmolski
pushed a commit
that referenced
this pull request
Apr 8, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.269](v2.0.0-rc.268...v2.0.0-rc.269) (2026-04-08) ### Features * 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).
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 was referenced Apr 8, 2026
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.
This PR adds support of input object fields passed as arguments.
It handles nested input objects, recursive types, list of input objects
and list-typed arguments.
Negative weights on input fields reduce the cost, but clamped to zero for each field.
I have simplified code by moving costConfigs and defaultListSize into the CostCalculator.