feat(router): router force use of variables and not inline values - #3055
Conversation
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds configurable inline-argument disallowance for GraphQL operations, carries inline-argument findings through normalization and caching, propagates them into HTTP and WebSocket execution paths, and adds integration coverage. It also updates ChangesDisallow Inline Arguments feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
router/core/websocket.go (1)
970-997: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInline-argument reporting order diverges from the HTTP prehandler.
Here the
logInlineArguments/ReturnInResponseExtensionsblock runs beforeNormalizeVariables(), whereas ingraphql_prehandler.gothe equivalent block runs afterNormalizeVariables()succeeds (and only then). IfNormalizeVariables()fails on WS, the warning/extension has already been emitted even though the request is aborted — on HTTP the same failure prevents the log/extension from firing at all. This contradicts the explicit "matching the HTTP prehandler" intent in the comments here.🐛 Proposed fix to match HTTP ordering
opContext.normalizationCacheHit = operationKit.parsedOperation.NormalizationCacheHit - // Non-enforcing mode: warn about any inline argument values, matching the HTTP - // prehandler so subscriptions over WebSockets are not silently exempt. - logInlineArguments(h.logger, operationKit.parsedOperation) - - // When configured, also surface the inline arguments to the client under - // `extensions.inlineArguments`, matching the HTTP prehandler. - if h.operationProcessor.parseKitOptions.disallowInlineArguments.ReturnInResponseExtensions { - opContext.inlineArguments = inlineArgumentQualifiedNames(operationKit.parsedOperation) - } - // Validate the operation against the schema BEFORE variable extraction, which would // serialize inline literals into JSON variables and let invalid-type literals through. // The error is surfaced later, during validation, so normalization timing stays accurate. _, operationValidationErr := operationKit.ValidateOperation() cached, _, err := operationKit.NormalizeVariables() if err != nil { opContext.normalizationTime = time.Since(startNormalization) return nil, nil, err } opContext.variablesNormalizationCacheHit = cached + + // Non-enforcing mode: warn about any inline argument values, matching the HTTP + // prehandler so subscriptions over WebSockets are not silently exempt. + logInlineArguments(h.logger, operationKit.parsedOperation) + + // When configured, also surface the inline arguments to the client under + // `extensions.inlineArguments`, matching the HTTP prehandler. + if h.operationProcessor.parseKitOptions.disallowInlineArguments.ReturnInResponseExtensions { + opContext.inlineArguments = inlineArgumentQualifiedNames(operationKit.parsedOperation) + }🤖 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 `@router/core/websocket.go` around lines 970 - 997, Move the inline-argument reporting in the websocket operation flow so it matches the HTTP prehandler ordering: in `handleOperation` (or the surrounding websocket normalization path), keep `NormalizeOperation` first, then call `NormalizeVariables()`, and only after that succeeds run `logInlineArguments` and populate `opContext.inlineArguments` when `ReturnInResponseExtensions` is enabled. This ensures the warning and response extension are emitted only for requests that fully normalize, consistent with `graphql_prehandler.go`.
🧹 Nitpick comments (1)
router/core/graphql_prehandler.go (1)
889-898: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared "report inline arguments" helper.
The same three-line
ReturnInResponseExtensionsgate plus thelogInlineArgumentscall is duplicated here and inwebsocket.go(lines 978-987). As shown in thewebsocket.goreview, the two call sites have already drifted in execution order relative toNormalizeVariables. A single shared helper (e.g.reportInlineArguments(logger, parsedOperation, disallowInlineArguments) []string) called at the same normalization stage in both places would eliminate this duplication and prevent future divergence.Also applies to: 1428-1459
🤖 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 `@router/core/graphql_prehandler.go` around lines 889 - 898, The inline-arguments reporting logic is duplicated and has already drifted between GraphQL prehandler and websocket handling. Extract a shared helper around logInlineArguments and the ReturnInResponseExtensions gate, such as reportInlineArguments(logger, parsedOperation, disallowInlineArguments), and call it from both graphql_prehandler.go and websocket.go at the same NormalizeVariables stage so both paths stay aligned and return the inline argument names consistently.
🤖 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/pkg/config/config.schema.json`:
- Around line 3662-3666: The enforce_http_status_code schema in
config.schema.json currently allows any integer, which can let enforce-mode
rejections use non-error responses like 200. Tighten the schema for
enforce_http_status_code by adding a valid HTTP rejection range constraint (for
example, minimum 400 and maximum 599) while keeping the existing default and
description intact so misconfiguration is prevented.
---
Outside diff comments:
In `@router/core/websocket.go`:
- Around line 970-997: Move the inline-argument reporting in the websocket
operation flow so it matches the HTTP prehandler ordering: in `handleOperation`
(or the surrounding websocket normalization path), keep `NormalizeOperation`
first, then call `NormalizeVariables()`, and only after that succeeds run
`logInlineArguments` and populate `opContext.inlineArguments` when
`ReturnInResponseExtensions` is enabled. This ensures the warning and response
extension are emitted only for requests that fully normalize, consistent with
`graphql_prehandler.go`.
---
Nitpick comments:
In `@router/core/graphql_prehandler.go`:
- Around line 889-898: The inline-arguments reporting logic is duplicated and
has already drifted between GraphQL prehandler and websocket handling. Extract a
shared helper around logInlineArguments and the ReturnInResponseExtensions gate,
such as reportInlineArguments(logger, parsedOperation, disallowInlineArguments),
and call it from both graphql_prehandler.go and websocket.go at the same
NormalizeVariables stage so both paths stay aligned and return the inline
argument names consistently.
🪄 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: df4508ff-fc1a-492c-aab7-bf24c627b117
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
router-tests/go.modrouter-tests/operations/disallow_inline_arguments_test.gorouter/core/context.gorouter/core/graph_server.gorouter/core/graphql_handler.gorouter/core/graphql_prehandler.gorouter/core/operation_processor.gorouter/core/websocket.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
SkArchon
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3055 +/- ##
==========================================
- Coverage 71.04% 61.97% -9.07%
==========================================
Files 335 261 -74
Lines 49334 30714 -18620
Branches 6033 0 -6033
==========================================
- Hits 35047 19036 -16011
+ Misses 14261 10169 -4092
- Partials 26 1509 +1483
🚀 New features to boost your workflow:
|
…erengine-force-use-of-variables
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…erengine-force-use-of-variables
|
Caution Review failedAn error occurred during the review process. Please try again later. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds configurable inline-argument disallowance for GraphQL operations, carries inline-argument findings through normalization and caching, propagates them into HTTP and WebSocket execution paths, and adds integration coverage. It also updates ChangesDisallow Inline Arguments feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
2 participants
This PR adds the capability to block inline arguments. This is useful to increase normalization cache hit rates. The feature is broken down into three modes.
In addition to this we have two additional options which allow users to either exempt (default) or also validate persisted operations, as well as return extensions (two formats for "Enforcing" and "Non-Enforcing").
This PR depends on wundergraph/graphql-go-tools#1577
Summary by CodeRabbit
engine.disallow_inline_argumentspolicy (config/env) to detect inline GraphQL argument values with modes:off,enabled-non-enforcing,enabled-enforcing, including options for persisted operations and optionalextensions.inlineArguments.github.com/wundergraph/graphql-go-tools/v2version.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.