Conversation
endigma
requested review from
Noroth,
SkArchon,
StarpTech,
devsergiy and
jensneuse
as code owners
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds two query-hash metric fields for cache-warmup tests, expands safelist tests to exercise access-log sha256 handling, and updates graph-server/prehandler logic to prefer client-provided persisted hashes when no query body, conditionally compute operation SHA‑256, and materialize persisted-query hashes for safelist/logUnknown paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
endigma
force-pushed
the
jesse/eng-6851-fix-compute-sha256-safelist-independent-checks
branch
from
594fa6f to
9252484
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2633 +/- ##
==========================================
- Coverage 64.58% 62.91% -1.68%
==========================================
Files 301 244 -57
Lines 42876 25816 -17060
Branches 4600 0 -4600
==========================================
- Hits 27690 16241 -11449
+ Misses 15165 8202 -6963
- Partials 21 1373 +1352
🚀 New features to boost your workflow:
|
Noroth
approved these changes
Mar 11, 2026
…ibutes Convert the mutually exclusive if/else-if chain in graph_server.go:671-689 to independent if blocks so safelist/logUnknown checks always evaluate. Guard the persisted query hash overwrite in graphql_prehandler.go:558-563 to preserve client-provided hashes for "run by ID" requests. Use client-provided hash for telemetry when a request has a hash but no query body. Add tests for safelist + access log SHA256 attributes (the configuration that exposed the bug). Update cache_warmup_test.go cache metrics expectations.
endigma
force-pushed
the
jesse/eng-6851-fix-compute-sha256-safelist-independent-checks
branch
from
9252484 to
de3b807
Compare
…elemetry Tests previously expected sha256 of empty string for operation_sha256 when a request provided a persisted hash but no query body. Now correctly expects the client-provided persisted hash.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router-tests/structured_logging_test.go (1)
2032-2065:⚠️ Potential issue | 🟡 MinorRename this subtest or add a query body.
After the assertion change, this case is clearly exercising the no query body path, but the subtest name still says “with persisted hash and body”. That makes the suite advertise coverage it does not actually provide.
✏️ Suggested tweak
- t.Run("validate request.operation.sha256Hash expression with persisted hash and body", func(t *testing.T) { + t.Run("validate request.operation.sha256Hash expression with persisted hash and no query body", func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/structured_logging_test.go` around lines 2032 - 2065, The subtest started with t.Run("validate request.operation.sha256Hash expression with persisted hash and body", ...) no longer includes a query body, so either rename that test title to reflect the "no body" path (e.g., change the t.Run string to mention "no body" or "persisted hash only") or add a GraphQL request body to the GraphQLRequest used in xEnv.MakeGraphQLRequest (populate the Body field alongside OperationName and Extensions) so the test truly exercises the "with persisted hash and body" scenario; update the t.Run string or the GraphQLRequest Body accordingly and keep assertions against request.operation.sha256Hash unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@router-tests/structured_logging_test.go`:
- Around line 2032-2065: The subtest started with t.Run("validate
request.operation.sha256Hash expression with persisted hash and body", ...) no
longer includes a query body, so either rename that test title to reflect the
"no body" path (e.g., change the t.Run string to mention "no body" or "persisted
hash only") or add a GraphQL request body to the GraphQLRequest used in
xEnv.MakeGraphQLRequest (populate the Body field alongside OperationName and
Extensions) so the test truly exercises the "with persisted hash and body"
scenario; update the t.Run string or the GraphQLRequest Body accordingly and
keep assertions against request.operation.sha256Hash unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49cc0862-cfac-42fb-badc-f1e4708d124d
📒 Files selected for processing (1)
router-tests/structured_logging_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/structured_logging_test.go (1)
2032-2065: Rename this subtest to match the scenario.The request here no longer includes a query body, so
"with persisted hash and body"now points at a different code path than the one being asserted.♻️ Suggested rename
- t.Run("validate request.operation.sha256Hash expression with persisted hash and body", func(t *testing.T) { + t.Run("validate request.operation.sha256Hash expression with persisted hash and no query body", func(t *testing.T) {Based on learnings: Hash validation for persisted queries with query bodies is performed in
router/core/graphql_prehandler.goinhandleOperation, not in the APQ processing logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/structured_logging_test.go` around lines 2032 - 2065, The subtest name is misleading because the request has no query body and exercises the APQ/persisted-hash path; rename the t.Run description from "validate request.operation.sha256Hash expression with persisted hash and body" to reflect the actual scenario (e.g. "validate request.operation.sha256Hash expression with persisted hash (no body)" or similar) so the test name matches the asserted behavior in this test and distinguishes it from the body-validation path handled by handleOperation in graphql_prehandler.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/structured_logging_test.go`:
- Around line 2032-2065: The subtest name is misleading because the request has
no query body and exercises the APQ/persisted-hash path; rename the t.Run
description from "validate request.operation.sha256Hash expression with
persisted hash and body" to reflect the actual scenario (e.g. "validate
request.operation.sha256Hash expression with persisted hash (no body)" or
similar) so the test name matches the asserted behavior in this test and
distinguishes it from the body-validation path handled by handleOperation in
graphql_prehandler.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0726ac63-0e0e-4e1d-b1ae-14d2dc88c630
📒 Files selected for processing (1)
router-tests/structured_logging_test.go
endigma
deleted the
jesse/eng-6851-fix-compute-sha256-safelist-independent-checks
branch
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Convert the mutually exclusive if/else-if chain to independent if blocks so safelist/logUnknown checks always evaluate. Guard the persisted query hash overwrite to preserve client-provided hashes for "run by ID" requests. Use client-provided hash for telemetry when a request has a hash but no query body.
Add tests for safelist + access log SHA256 attributes and update cache metrics expectations.
Summary by CodeRabbit
Tests
Improvements