Skip to content

fix(cost): actual cost inflation when sizedFields parent is a non-list wrapper field - #1492

Closed
budziam wants to merge 2 commits into
wundergraph:masterfrom
budziam:master
Closed

fix(cost): actual cost inflation when sizedFields parent is a non-list wrapper field#1492
budziam wants to merge 2 commits into
wundergraph:masterfrom
budziam:master

Conversation

@budziam

@budziam budziam commented May 11, 2026

Copy link
Copy Markdown

Problem

When a non-list wrapper field (e.g. items_page: ItemsPage!) is annotated
with @listSize(slicingArguments: ["limit"], sizedFields: ["items"]), its occurrences are never recorded
in actualListSizes because the resolver only tracks array fields there.

This caused the averaging denominator for the child list multiplier to default
to 1 (total items across all parents) instead of the number of parent field
occurrences, inflating the combined actual cost above what you'd get by summing
the same entities queried individually.

Example:

type Query {
  boards(limit: Int): [Board!]! @listSize(slicingArguments: ["limit"])
}
type Board {
  items_page(limit: Int!): ItemsPage! @listSize(slicingArguments: ["limit"], sizedFields: ["items"])
}
type ItemsPage {
  items: [Item!]!
}
{
  boards(limit: 4) {
    items_page(limit: 1) {
      items {
        id
      }
    }
  }
}

With 4 boards and 2 total items, the multiplier was computed as 2/1 = 2
instead of the correct 2/4 = 0.5.

Fix

Instead of looking up the immediate parent by chopping the JSON path string,
climb the CostTreeNode parent chain to find the nearest ancestor where
returnsListType == true, then use its actualListSizes count as the
denominator. This uses the type information already on each node rather than
inferring list-ness from map absence.

budziam added 2 commits May 11, 2026 19:09
…parent

When a non-list wrapper field (e.g. items_page: ItemsPage!) is used as a
sizedFields parent via @listsize, its occurrences are not recorded in
actualListSizes because it does not return a list type. This causes the
child list multiplier to use total items (2) as its denominator instead
of the number of parent occurrences (4), inflating the combined actual
cost above the sum of individually queried costs.

The test asserts the expected contract — combined cost must not exceed
the sum of separate costs — and will pass once the fix is in place.
When computing the actual cost multiplier for a list field, the averaging
logic looked only at the immediate parent path to find the parent occurrence
count. If the parent is a non-list wrapper field,
it is never recorded in actualListSizes because it does not return a list type.
This caused parentCount to default to 1, inflating the multiplier by the total
item count instead of the per-parent average.

Fix by climbing the CostTreeNode parent chain to find the nearest ancestor
where returnsListType is true, then look up its occurrence count. This uses
the type information already on each node rather than inferring it from map
absence via string path manipulation.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR refactors how parent list sizes are tracked during nested cost calculations. Instead of inferring parent list size by truncating jsonPath strings, the code now walks up the ancestor node chain to find the nearest list-returning parent and reads its actual list size. A new test validates the behavior when non-list wrapper fields separate list-returning fields.

Changes

Parent List Tracking Logic

Layer / File(s) Summary
Cost Calculation Logic
v2/pkg/engine/plan/cost.go
CostTreeNode.cost now determines parentCount by traversing node.parent to the nearest ancestor that returnsListType and reading actualListSizes[p.jsonPath], replacing the prior jsonPath string truncation method.
Test for Non-List Wrapper Parent
v2/pkg/engine/plan/cost_sized_fields_test.go
New test TestActualCost_SizedFieldsParentNotTracked validates cost calculation through a chain where a non-list wrapper field (items_page) sits between two list-returning fields (boards and items), ensuring combined costs do not exceed sum of per-entity costs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and specifically describes the main change: fixing a cost calculation bug when a non-list wrapper field is annotated with @listSize.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly explains the problem (non-list wrapper fields not being tracked in actualListSizes), provides a concrete GraphQL example, and details the fix (climbing the parent chain instead of path truncation).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

ysmolski added a commit that referenced this pull request May 14, 2026
…#1493)

Instead of just trying the parent of the fields specifed in the
sizedFields and failing on non-list wrapper, find nearest list-typed
ancestor to avoid double counting of nested lists.

Closes #1492
ysmolski pushed a commit that referenced this pull request May 15, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.3.0](v2.2.0...v2.3.0)
(2026-05-15)


### Features

* support dot-path in slicingArguments
([#1485](#1485))
([2cb8d5e](2cb8d5e))


### Bug Fixes

* find proper parent when sizedFields parent is a non-list wrapper
([#1493](#1493))
([6b96976](6b96976)),
closes
[#1492](#1492)

---
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 May 15, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.15.0](execution/v1.14.0...execution/v1.15.0)
(2026-05-15)


### Features

* support dot-path in slicingArguments
([#1485](#1485))
([2cb8d5e](2cb8d5e))


### Bug Fixes

* find proper parent when sizedFields parent is a non-list wrapper
([#1493](#1493))
([6b96976](6b96976)),
closes
[#1492](#1492)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant