Skip to content

fix: use remapped variables in cost calculation - #2882

Merged
ysmolski merged 8 commits into
mainfrom
yury/eng-9610-cost-calculation-ignores-remapvariables
May 22, 2026
Merged

fix: use remapped variables in cost calculation#2882
ysmolski merged 8 commits into
mainfrom
yury/eng-9610-cost-calculation-ignores-remapvariables

Conversation

@ysmolski

@ysmolski ysmolski commented May 21, 2026

Copy link
Copy Markdown
Contributor

Simple PR to verify that remapped variables are handled correctly in the fixed engine.

Summary by CodeRabbit

  • Chores

    • Bumped a core GraphQL tooling dependency to v2.4.0.
  • Tests

    • Expanded cost-control tests to cover variable remapping, input-object slicing, scalar multipliers, explicit null handling, and related validation edge cases.
  • Improvements

    • Cost estimation, validation, and response cost reporting now honor remapped GraphQL variables.

Review Change Stack

@ysmolski
ysmolski requested a review from a team as a code owner May 21, 2026 12:51

@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 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.

@github-actions

github-actions Bot commented May 21, 2026

Copy link
Copy Markdown

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-bdcf598295200db4ed65f2ce92d46cffeae02b2e-nonroot

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2962be12-a767-499e-9375-f8715ee0775d

📥 Commits

Reviewing files that changed from the base of the PR and between 0195245 and cbca4e6.

⛔ Files ignored due to path filters (2)
  • router-tests/go.sum is excluded by !**/*.sum
  • router/go.sum is excluded by !**/*.sum
📒 Files selected for processing (2)
  • router-tests/go.mod
  • router/go.mod

Walkthrough

Bump graphql-go-tools to v2.4.0, add operationContext.VariablesView(), update router cost validation and actual-cost callers to use the view, and add tests exercising slicing-argument behavior with remapped variables.

Changes

Variable Remapping Cost Calculation

Layer / File(s) Summary
Dependency bump and VariablesView method
router-tests/go.mod, router/go.mod, router/core/context.go
Bumps github.com/wundergraph/graphql-go-tools/v2 to v2.4.0 and adds operationContext.VariablesView() that constructs a resolve.VariablesView from stored variables and remap directives (plus a small import-format blank-line change).
Cost calculation callsites using VariablesView
router/core/graphql_handler.go, router/core/operation_processor.go
Replace direct uses of raw variables with VariablesView() when calling costCalc.ActualCost, and use the view for ValidateSliceArguments and EstimateCost in ValidateStaticCost.
Test coverage for variable remapping cost scenarios
router-tests/security/costs_test.go
Adds t.Run("variables remap") with subtests validating slicing-argument estimation follows remapped variables (input-object dot-path and multiple scalar remaps), preserves explicit null, validates multi-arg errors, and checks behavior when remapping is disabled; also renames a slicing-argument test variable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • wundergraph/cosmo#2880: Adjusts router cost calculation/validation of slicingArguments, interacting with remapped-variable resolution and default handling.
  • wundergraph/cosmo#2716: Overlaps at the same cost-calculation callsites in graphql_handler.go and operation_processor.go, modifying how cost calculation accesses variables for slice-argument validation and actual-cost computation.
  • wundergraph/cosmo#2801: Complements this PR by adding dot-path support for @listSize slicingArguments in composition, enabling the same slicing-argument resolution via remapped variables that this PR integrates into cost calculation.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: use remapped variables in cost calculation' clearly and concisely summarizes the main change across the pull request, which updates cost calculation logic to use remapped variables instead of raw variables.
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.

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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@router-tests/security/costs_test.go`:
- Around line 604-605: The test constructs a GraphQL request using Query:
`query($some: Int) { slicedThings(first: $some) { a } }` but the Variables JSON
uses the key `"$some"`, which is invalid; change the Variables payload to use
the variable name without the dollar sign (e.g. replace `{"$some": null}` with
`{"some": null}`) so the test sends a null variable value for `$some` rather
than a missing variable path.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 240e9913-075f-4dcb-bcf2-788baee1e23f

📥 Commits

Reviewing files that changed from the base of the PR and between fd5ceed and 98c3398.

⛔ Files ignored due to path filters (2)
  • router-tests/go.sum is excluded by !**/*.sum
  • router/go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • router-tests/go.mod
  • router-tests/security/costs_test.go
  • router/core/context.go
  • router/core/graphql_handler.go
  • router/core/operation_processor.go
  • router/go.mod

Comment thread router-tests/security/costs_test.go Outdated
@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.24%. Comparing base (8ad7d44) to head (cbca4e6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2882      +/-   ##
==========================================
+ Coverage   66.04%   66.24%   +0.20%     
==========================================
  Files         258      258              
  Lines       27053    27056       +3     
==========================================
+ Hits        17866    17923      +57     
+ Misses       7759     7720      -39     
+ Partials     1428     1413      -15     
Files with missing lines Coverage Δ
router/core/context.go 74.85% <100.00%> (+0.14%) ⬆️
router/core/graphql_handler.go 66.47% <100.00%> (ø)
router/core/operation_processor.go 85.10% <100.00%> (+0.01%) ⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ysmolski
ysmolski merged commit d211459 into main May 22, 2026
84 of 87 checks passed
@ysmolski
ysmolski deleted the yury/eng-9610-cost-calculation-ignores-remapvariables branch May 22, 2026 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants