Skip to content

feat(router): validate slicing arguments in Cost Control#2692

Merged
ysmolski merged 7 commits intomainfrom
yury/eng-8842-cost-validate-requireoneslicingargument-in-queries
Mar 26, 2026
Merged

feat(router): validate slicing arguments in Cost Control#2692
ysmolski merged 7 commits intomainfrom
yury/eng-8842-cost-validate-requireoneslicingargument-in-queries

Conversation

@ysmolski
Copy link
Copy Markdown
Contributor

@ysmolski ysmolski commented Mar 25, 2026

This PR validates if slicing arguments passed for a field conform to the value of listSize:requireOneSlicingArgument.

Summary by CodeRabbit

  • New Features
    • Added a new slicedThings query to request sliced lists via a single slicing argument (first or last).
  • Behavior Change
    • Requests now validate that exactly one slicing argument is provided; this validation can be disabled per datasource.
  • Tests
    • Added tests covering valid/invalid slicing argument combinations and the config-disabled scenario.
  • Chores
    • Updated module dependency references.

This PR validates if slicing arguments passed for a field
conform to the value of listSize:requireOneSlicingArgument.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 25, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

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: 9221d3b1-7378-40c9-a603-a15fc529c4f2

📥 Commits

Reviewing files that changed from the base of the PR and between d22536e and 83942f4.

⛔ 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
✅ Files skipped from review due to trivial changes (2)
  • router-tests/go.mod
  • router/go.mod

Walkthrough

Adds a new GraphQL query slicedThings(first: Int, last: Int): [Thing] annotated with @listSize, implements its resolver, integrates slice-argument validation into static cost validation, adds tests covering slicing-argument validation and a config override, and updates graphql-go-tools dependency versions in two go.mod files.

Changes

Cohort / File(s) Summary
Schema
demo/pkg/subgraphs/products/subgraph/schema.graphqls
Added slicedThings(first: Int, last: Int): [Thing] with @listSize(slicingArguments: ["first", "last"]).
Resolver Implementation
demo/pkg/subgraphs/products/subgraph/schema.resolvers.go
Added queriesResolver.SlicedThings(ctx context.Context, first *int, last *int) ([]*model.Thing, error) returning nil when both args are nil, otherwise selecting a size (prefers first), validating 0..1000, and returning a populated slice or error.
Operation Processing
router/core/operation_processor.go
OperationKit.ValidateStaticCost now calls costCalc.ValidateSliceArguments(...) and returns a reportError immediately if that report contains errors before estimating cost.
Tests & Introspection
router-tests/security/costs_test.go, router-tests/operations/testdata/introspection/base-graph-schema.json
Added tests under TestOperationCost for require-one-slicing-argument (zero, one, multiple, null) and a config-override disabling validation; updated introspection JSON to include slicedThings.
Dependency Manifests
router/go.mod, router-tests/go.mod
Bumped github.com/wundergraph/graphql-go-tools/v2 from v2.0.0-rc.265 to v2.0.0-rc.267.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 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 (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly describes the main change: adding validation for slicing arguments in Cost Control, which is the core objective of this changeset.

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


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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 25, 2026

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

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

@github-actions
Copy link
Copy Markdown

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-3a1e353ae0295cae0083a0c91f54d572ddaea810-nonroot

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

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 the current code and only fix it if needed.

Inline comments:
In `@demo/pkg/subgraphs/products/subgraph/schema.resolvers.go`:
- Around line 127-131: The grouped type block "type ( documentationResolver
struct{ *Resolver } mutationResolver struct{ *Resolver } queriesResolver struct{
*Resolver } )" causes gqlgen/CI formatting mismatch; replace it with separate
type declarations for each resolver (i.e., declare "type documentationResolver
struct{ *Resolver }", then "type mutationResolver struct{ *Resolver }", then
"type queriesResolver struct{ *Resolver }") so the file matches gqlgen's
expected format, or alternatively regenerate the file with `go generate` to
produce the correct layout for the Resolver-related types
(documentationResolver, mutationResolver, queriesResolver).
🪄 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: 11376f16-cf95-4380-a834-0a3faceb4826

📥 Commits

Reviewing files that changed from the base of the PR and between dcd4e0a and af34bff.

⛔ Files ignored due to path filters (3)
  • demo/pkg/subgraphs/products/subgraph/generated/generated.go is excluded by !**/generated/**
  • router-tests/go.sum is excluded by !**/*.sum
  • router/go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • demo/pkg/subgraphs/products/subgraph/schema.graphqls
  • demo/pkg/subgraphs/products/subgraph/schema.resolvers.go
  • router-tests/go.mod
  • router-tests/security/costs_test.go
  • router-tests/testenv/testdata/config.json
  • router/core/operation_processor.go
  • router/go.mod

Comment thread demo/pkg/subgraphs/products/subgraph/schema.resolvers.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.09%. Comparing base (dcd4e0a) to head (83942f4).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2692       +/-   ##
===========================================
+ Coverage   35.53%   63.09%   +27.55%     
===========================================
  Files         129      245      +116     
  Lines       11782    26273    +14491     
  Branches      467        0      -467     
===========================================
+ Hits         4187    16576    +12389     
- Misses       7593     8354      +761     
- Partials        2     1343     +1341     
Files with missing lines Coverage Δ
router/core/operation_processor.go 84.95% <100.00%> (ø)

... and 373 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.

Comment thread demo/pkg/subgraphs/products/subgraph/schema.resolvers.go Fixed
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

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 the current code and only fix it if needed.

Inline comments:
In `@demo/pkg/subgraphs/products/subgraph/schema.resolvers.go`:
- Around line 102-109: The SlicedThings resolver currently derives size directly
from user-controlled pointers (first/last) and uses it as the capacity for
things := make([]*model.Thing, 0, size) which can panic or OOM; replicate the
guard pattern used in SharedThings: validate that the chosen value (from first
or last) is non-nil, non-negative, and enforce an upper bound (e.g., a constant
MAX_PAGE_SIZE) before using it as capacity, returning a clear error for
out-of-range inputs; update the logic around determining size (the size variable
and the allocation in SlicedThings) to use the validated/ capped value.
🪄 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: eb95a3f4-cefc-49f5-8d5a-cdb22faed8bf

📥 Commits

Reviewing files that changed from the base of the PR and between af34bff and 7d32232.

📒 Files selected for processing (1)
  • demo/pkg/subgraphs/products/subgraph/schema.resolvers.go

Comment thread demo/pkg/subgraphs/products/subgraph/schema.resolvers.go
Copy link
Copy Markdown
Member

@endigma endigma left a comment

Choose a reason for hiding this comment

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

lgtm, ping when its release go tools version

Comment thread router/go.mod Outdated
@ysmolski ysmolski requested a review from endigma March 26, 2026 11:05
@ysmolski ysmolski merged commit 4416030 into main Mar 26, 2026
39 checks passed
@ysmolski ysmolski deleted the yury/eng-8842-cost-validate-requireoneslicingargument-in-queries branch March 26, 2026 11:44
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.

3 participants