fix: do not charge actual cost for denied fields - #1582
Conversation
📝 WalkthroughWalkthroughAdds authorization-aware actual cost accounting: cost paths move to rendered ChangesAuth-aware cost accounting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/graphql/request.go`:
- Around line 211-218: Guard resolveCtx before using it in
Request.ComputeActualCost so nil callers don’t panic; keep the existing fallback
behavior of setting actualCost to 0 when cost cannot be computed. Update the
initial nil check in ComputeActualCost to include resolveCtx alongside calc and
resolveCtx.TypeNameStats, and only call calc.ActualCostWithDenials when
resolveCtx is non-nil and has the required stats.
In `@v2/pkg/engine/plan/cost.go`:
- Around line 968-972: The comment on ActualCostWithDenials currently describes
typeNameDenials as JSON paths, but the matching logic in isFieldDenied uses
exact resolver field paths including the leading operation type. Update the
documentation in ActualCostWithDenials and any nearby references to say these
are resolver field paths, not JSON paths, and clarify the required format with
the operation type prefix so callers use the same keys that isFieldDenied
expects.
🪄 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: dfa4049f-399c-4327-b4a3-a842df9dc21f
📒 Files selected for processing (8)
execution/engine/execution_engine.goexecution/engine/execution_engine_cost_auth_test.goexecution/graphql/request.gov2/pkg/engine/plan/cost.gov2/pkg/engine/plan/cost_visitor.gov2/pkg/engine/resolve/context.gov2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolve.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
execution/engine/execution_engine_cost_auth_test.go (1)
263-284: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRename the stale test titles
expectedActualCostis5in both cases, so onlyQuery.useris charged. Update the titles to match that behavior:
denied non-nullable field nulls the parent: earlier sibling is charged→... earlier sibling is also not chargedsubgraph null for non-nullable field nulls the parent: earlier sibling and the field are charged→... neither sibling nor the field is charged🤖 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_auth_test.go` around lines 263 - 284, The affected subtests in execution_engine_cost_auth_test.go have stale names that no longer match the asserted cost behavior. Update the `t.Run(...)` titles around the `expectedActualCost` assertions so they describe that only `Query.user` is charged, using the same test cases and identifiers (`denied non-nullable field nulls the parent` and `subgraph null for non-nullable field nulls the parent`) to rename them to the corrected “earlier sibling is also not charged” and “neither sibling nor the field is charged” wording.
🤖 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_auth_test.go`:
- Around line 263-284: The affected subtests in
execution_engine_cost_auth_test.go have stale names that no longer match the
asserted cost behavior. Update the `t.Run(...)` titles around the
`expectedActualCost` assertions so they describe that only `Query.user` is
charged, using the same test cases and identifiers (`denied non-nullable field
nulls the parent` and `subgraph null for non-nullable field nulls the parent`)
to rename them to the corrected “earlier sibling is also not charged” and
“neither sibling nor the field is charged” wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c844cd03-fbab-4cef-8e47-96d65598ed53
📒 Files selected for processing (5)
execution/engine/execution_engine_cost_auth_test.goexecution/graphql/request.gov2/pkg/engine/plan/cost.gov2/pkg/engine/plan/cost_visitor.gov2/pkg/engine/resolve/resolvable.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/plan/cost_visitor.go
423b398 to
dc832f5
Compare
🤖 I have created a release *beep* *boop* --- ## [2.10.0](v2.9.2...v2.10.0) (2026-07-09) ### Features * **resolve:** add opt-in pre-fetch field authorization mode ([#1561](#1561)) ([eb3b142](eb3b142)) ### Bug Fixes * do not charge actual cost for denied fields ([#1582](#1582)) ([00d9f66](00d9f66)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [1.17.1](execution/v1.17.0...execution/v1.17.1) (2026-07-09) ### Bug Fixes * do not charge actual cost for denied fields ([#1582](#1582)) ([00d9f66](00d9f66)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
Do not charge actual costs when some feld is denied or
nulled because some of its non-nullable children was null.
We collect stats on render pass in Resolvable too.
This gives easy way to handle stats for nulled items of arrays.