fix: ensure subgraph operation name is unique#2094
Conversation
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughThis change updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
router-tests/go.mod (1)
178-179: Delete the stale commentedreplacedirectiveThe commented path override for
graphql-go-tools/v2is obsolete and may confuse future readers. Safely remove it:-// github.com/wundergraph/graphql-go-tools/v2 => ../../graphql-go-tools/v2router/pkg/config/config.schema.json (1)
2717-2721: Add an explicit example value and escape placeholders for better readabilityThe new description is clear, but adding an explicit example (using JSON-pointer style) makes it much easier for end-users and tooling to understand the resulting wire format, and wrapping the placeholders in back-ticks avoids accidental
$substitutions in some editors.- "description": "Enable appending the operation name to subgraph fetches. This will ensure that the operation name will be included in the corresponding subgraph requests using the following format: $OperationName__$SubgraphName__$FetchID." + "description": "Enable appending the operation name to subgraph fetches. When enabled the router rewrites sub-requests so the operation name follows the format:\n\n `\$OperationName__\$SubgraphName__\$FetchID`\n\nExample produced subgraph request name:\n\n```json\n{\n \"operationName\": \"GetProducts__products__1\"\n}\n```"router-tests/testdata/fixtures/query_plans/response_with_query_plan_operation_name_sanitized_no_data.json (1)
26-29: Confirm sanitisation of exotic sub-graph identifiersThe sanitised segments (
mo_o_d,av_ai_la_bi_lit_y) strip unsafe characters, but the rawsubgraphNamefields still contain symbols like$,%,&that are illegal in GraphQL names.
Please double-check that only the sanitised value is ever used as an operation name, and consider asserting this invariant in unit tests to guard against future regressions.Also applies to: 68-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
router-tests/go.mod(1 hunks)router-tests/integration_test.go(2 hunks)router-tests/testdata/fixtures/query_plans/response_with_query_plan_operation_name.json(5 hunks)router-tests/testdata/fixtures/query_plans/response_with_query_plan_operation_name_sanitized_no_data.json(5 hunks)router/go.mod(1 hunks)router/pkg/config/config.schema.json(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.
router-tests/testdata/fixtures/query_plans/response_with_query_plan_operation_name.json (1)
Learnt from: SkArchon
PR: #2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
router/pkg/config/config.schema.json (1)
Learnt from: SkArchon
PR: #2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.
router-tests/testdata/fixtures/query_plans/response_with_query_plan_operation_name_sanitized_no_data.json (1)
Learnt from: SkArchon
PR: #2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
router-tests/integration_test.go (1)
Learnt from: Noroth
PR: #2088
File: demo/pkg/subgraphs/projects/src/main_test.go:0-0
Timestamp: 2025-07-29T08:19:55.720Z
Learning: In Go testing, t.Fatal, t.FailNow, t.Skip* and similar methods cannot be called from goroutines other than the main test goroutine, as they will cause a runtime panic. Use channels, t.Error*, or other synchronization mechanisms to communicate errors from goroutines back to the main test thread.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
router-tests/go.mod (1)
29-29: Dependency bump approved – manual verification required
github.com/wundergraph/graphql-go-tools/v2is now pinned tov2.0.0-rc.214-6af5027c19d2. Nogo.workfile was detected, so please manually confirm that every module in the repo uses the identical commit:
- List all version pins:
grep -R 'github.com/wundergraph/graphql-go-tools/v2' -n .- For each module directory (e.g.,
router-tests/,router/, etc.):and ensure no mismatched versions appear.cd <module_dir> go mod tidyrouter-tests/testdata/fixtures/query_plans/response_with_query_plan_operation_name_sanitized_no_data.json (1)
15-16: Operation-name uniqueness suffix looks correct – no further action requiredAll updated
querystrings now append__<fetchId>to the operation name, guaranteeing uniqueness across subgraph fetches while still starting with a non-reserved character. This satisfies the new router requirement and remains GraphQL-spec compliant.Also applies to: 39-40, 81-82, 135-136, 194-195
router/go.mod (1)
34-36: Version bump is shadowed by the localreplace– verify intent
require github.com/wundergraph/graphql-go-tools/v2 v2.0.0-rc.214…was updated, but thereplacedirective at Line 165 still points to a local checkout (../../graphql-go-tools/v2).
In Go modules thereplacewins, so the build will ignore this new pseudo-version. If the intent is to test the upstream change, drop thereplaceor update the local checkout accordingly; otherwise the version bump is dead code.router-tests/testdata/fixtures/query_plans/response_with_query_plan_operation_name.json (1)
33-34: Unique-suffixed operation names in fixture are consistent – looks goodThe fixture now mirrors the router’s
$OperationName__$Subgraph__$FetchIDformat across all fetch nodes. Alignment with the sanitised fixture and the new integration test is correct.Also applies to: 57-58, 99-100, 153-154, 212-213
router-tests/integration_test.go (3)
17-17: LGTM! Clean import addition.The
slicespackage import is correctly added and will be used for slice operations in the new test function.
591-625: LGTM! Well-structured simple test case.The test correctly:
- Enables the operation name propagation feature via
EnableSubgraphFetchOperationName- Validates the expected operation name format
OperationName__SubgraphName__FetchID- Verifies variable remapping works as expected (
count→a)- Uses proper middleware interception to assert subgraph request content
The test implementation follows good practices and thoroughly validates the feature behavior.
627-711: LGTM! Comprehensive complex test case with proper concurrency handling.The test excellently demonstrates:
- Thread-safe tracking: Proper mutex usage around shared
expectEmployeeOpsslice operations- Multi-subgraph validation: Each subgraph middleware correctly validates expected operation names
- Flexible assertions: Uses
require.Containsfor Mood/Availability subgraphs, allowing for implementation flexibility- Complete verification: Ensures all expected operations are encountered via
require.Empty(t, expectEmployeeOps)- Proper request restoration: All middleware correctly restores request bodies with
io.NopCloserThe synchronization logic using
slices.Indexandslices.Deletewithin mutex protection is correctly implemented and thread-safe.
ysmolski
force-pushed
the
yury/eng-7691-ensure-subgraph-operation-name-is-unique
branch
3 times, most recently
from
955c846 to
8eee9f8
Compare
Integration tests for the engine changes. Tests that expect debug query plan were fixed to expect FetchID appended to the operation name.
ysmolski
force-pushed
the
yury/eng-7691-ensure-subgraph-operation-name-is-unique
branch
from
8eee9f8 to
e3a9601
Compare
ysmolski
deleted the
yury/eng-7691-ensure-subgraph-operation-name-is-unique
branch
3 participants
Integration tests for the engine changes.
Tests that expect debug query plan were fixed to expect FetchID appended to the operation name.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Checklist