feat: support user-defined extension forwarding from subgraphs#2836
Open
feat: support user-defined extension forwarding from subgraphs#2836
Conversation
Noroth
requested review from
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
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:
WalkthroughThis PR implements subgraph extension forwarding for the GraphQL router. It adds configuration types, schema validation, executor wiring to propagate subgraph response extensions to clients, comprehensive integration tests covering forwarding behavior and conflict resolution algorithms, documentation, dependency updates, and removes an unrelated Kafka websocket test. ChangesSubgraph Extension Propagation Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2836 +/- ##
===========================================
+ Coverage 46.23% 65.72% +19.49%
===========================================
Files 1080 254 -826
Lines 144704 26539 -118165
Branches 9247 0 -9247
===========================================
- Hits 66897 17442 -49455
+ Misses 76077 7683 -68394
+ Partials 1730 1414 -316
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
router-tests/protocol/extension_forwarding_test.go (2)
38-55: ⚡ Quick winExtract repeated extension-injection middleware into a shared helper.
The same recorder/unmarshal/marshal/write block is duplicated across many subtests. A file-level helper will reduce noise and make future test adjustments safer.
♻️ Refactor sketch
+func injectExtensions(t *testing.T, raw string) func(http.Handler) http.Handler { + t.Helper() + return func(handler http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, r) + + var response graphqlResponseWithExtensions + require.NoError(t, json.Unmarshal(recorder.Body.Bytes(), &response)) + response.Extensions = json.RawMessage(raw) + + responseBytes, err := json.Marshal(response) + require.NoError(t, err) + w.WriteHeader(recorder.Code) + _, _ = w.Write(responseBytes) + }) + } +}Then use:
- Middleware: func(handler http.Handler) http.Handler { ... }, + Middleware: injectExtensions(t, `{"myExtension":"myValue"}`),Also applies to: 86-103, 106-123, 154-171, 174-191, 222-239, 260-279, 353-370, 401-418, 456-477
🤖 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-tests/protocol/extension_forwarding_test.go` around lines 38 - 55, The test repeats identical middleware logic (creating an httptest.NewRecorder, calling handler.ServeHTTP, unmarshalling into graphqlResponseWithExtensions, injecting a fixed Extensions JSON, re-marshalling and writing the response) across many subtests; extract that into a file-level helper function, e.g. createInjectExtensionsMiddleware() (or injectExtensionsMiddleware) that returns an http.Handler middleware performing the recorder/unmarshal/modify/marshal/write sequence, and replace each inline Middleware: func(...) block with a call to this helper so all tests reuse the same implementation.
64-69: ⚡ Quick winAssert GraphQL errors explicitly in response checks.
These assertions can pass with partial data while
errorsis present. Addrequire.Empty(t, resp.Errors)(or a small helper) after decode in each case.✅ Minimal improvement
var resp testenv.GraphQLResponse require.NoError(t, json.NewDecoder(strings.NewReader(res.Body)).Decode(&resp)) +require.Empty(t, resp.Errors) require.Equal(t, `{"employee":{"id":1}}`, string(resp.Data))Also applies to: 133-137, 200-205, 248-253, 309-335, 379-384, 427-432, 492-497
🤖 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-tests/protocol/extension_forwarding_test.go` around lines 64 - 69, The response checks currently only assert Data and Extensions but may ignore GraphQL errors; after decoding into resp (type testenv.GraphQLResponse) and after the existing require.NoError(t, json.NewDecoder(...).Decode(&resp)) add an explicit assertion require.Empty(t, resp.Errors) (or call the shared helper) before asserting resp.Data/resp.Extensions; do this in the shown block (resp variable decode) and mirror the same change in the other test blocks referenced (around the resp checks at the other listed ranges).
🤖 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-tests/go.mod`:
- Around line 33-35: Update the pinned OpenTelemetry SDK versions in this module
by changing any require or replace entries that reference
go.opentelemetry.io/otel/sdk (and its submodule
go.opentelemetry.io/otel/sdk/metric) from v1.39.0 or older (including the
replace at v1.28.0) to v1.40.0 or later; locate the require line that lists
"go.opentelemetry.io/otel/sdk" and the replace directives referencing
"go.opentelemetry.io/otel/sdk" (and "go.opentelemetry.io/otel/sdk/metric") and
bump their versions to at least v1.40.0 so both the direct dependency and any
replacements are aligned to a patched release.
In `@router/go.mod`:
- Line 34: Update all go.opentelemetry.io/otel/sdk pins in the router module to
v1.40.0 or later: change the require lines for "go.opentelemetry.io/otel/sdk"
and "go.opentelemetry.io/otel/sdk/metric" from v1.39.0 to v1.40.0+ and update
any replace directives that currently pin "go.opentelemetry.io/otel/sdk" (and
its metric submodule) from v1.28.0 to v1.40.0+ so the module meets the security
floor; after editing the go.mod entries run go mod tidy to update go.sum.
---
Nitpick comments:
In `@router-tests/protocol/extension_forwarding_test.go`:
- Around line 38-55: The test repeats identical middleware logic (creating an
httptest.NewRecorder, calling handler.ServeHTTP, unmarshalling into
graphqlResponseWithExtensions, injecting a fixed Extensions JSON, re-marshalling
and writing the response) across many subtests; extract that into a file-level
helper function, e.g. createInjectExtensionsMiddleware() (or
injectExtensionsMiddleware) that returns an http.Handler middleware performing
the recorder/unmarshal/modify/marshal/write sequence, and replace each inline
Middleware: func(...) block with a call to this helper so all tests reuse the
same implementation.
- Around line 64-69: The response checks currently only assert Data and
Extensions but may ignore GraphQL errors; after decoding into resp (type
testenv.GraphQLResponse) and after the existing require.NoError(t,
json.NewDecoder(...).Decode(&resp)) add an explicit assertion require.Empty(t,
resp.Errors) (or call the shared helper) before asserting
resp.Data/resp.Extensions; do this in the shown block (resp variable decode) and
mirror the same change in the other test blocks referenced (around the resp
checks at the other listed ranges).
🪄 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: d69064fa-9739-44a7-8d25-125043b743f5
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
router-tests/go.modrouter-tests/protocol/extension_forwarding_test.gorouter-tests/testenv/testenv.gorouter/core/executor.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs-website/router/configuration.mdx`:
- Line 1972: The line starting "Forwards `extensions` returned by subgraphs to
the client response..." is a long multi-clause sentence; split it into two
short, declarative sentences: one stating that the router forwards subgraph
`extensions` to the client response, and a second stating that an allow list
restricts which root extension fields are propagated and that an algorithm
selects a winner when multiple subgraphs return the same root field. Ensure
punctuation and tone match reference docs style.
🪄 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: 6b1ff110-b472-4a92-bec7-815626c56e39
📒 Files selected for processing (2)
docs-website/router/configuration.mdxrouter-tests/events/kafka_events_test.go
💤 Files with no reviewable changes (1)
- router-tests/events/kafka_events_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router-tests/testenv/testenv.go (1)
1416-1418: ⚡ Quick winDefault the extension propagation algorithm in testenv to match runtime defaults.
cfg.SubgraphExtensionPropagationstarts as a zero-value inconfigureRouter, so partial callback mutations can leaveAlgorithmempty and produce behavior that differs from config-loaded runtime defaults (first_write).♻️ Proposed fix
if testConfig.ModifySubgraphExtensionPropagation != nil { testConfig.ModifySubgraphExtensionPropagation(&cfg.SubgraphExtensionPropagation) } + if cfg.SubgraphExtensionPropagation.Algorithm == "" { + cfg.SubgraphExtensionPropagation.Algorithm = config.SubgraphExtensionPropagationAlgorithmFirstWrite + }Also applies to: 1499-1499
🤖 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-tests/testenv/testenv.go` around lines 1416 - 1418, The testenv currently leaves cfg.SubgraphExtensionPropagation as a zero-value so callbacks via testConfig.ModifySubgraphExtensionPropagation can produce an empty Algorithm that differs from runtime defaults; update the setup in configureRouter/testenv so cfg.SubgraphExtensionPropagation.Algorithm is defaulted to "first_write" (the runtime default) before invoking testConfig.ModifySubgraphExtensionPropagation (or ensure the callback only mutates non-zero fields), referencing the symbols cfg.SubgraphExtensionPropagation and testConfig.ModifySubgraphExtensionPropagation to locate where to set the default.
🤖 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 3807-3813: The current schema makes an empty or missing
allowed_extension_fields behave as "allow all", which is unsafe; change the
schema so empty/missing allowed_extension_fields defaults to forwarding none and
add a new boolean allow_all_extension_fields flag (default false) that must be
explicitly set true to enable full passthrough; update validation/consumer logic
that reads allowed_extension_fields and any enabled setting (e.g., the code that
checks enabled) to treat an absent/empty allowed_extension_fields as deny-all
unless allow_all_extension_fields === true, and ensure schema descriptions for
allowed_extension_fields and the new allow_all_extension_fields clearly document
the safer default and the explicit opt-in requirement.
---
Nitpick comments:
In `@router-tests/testenv/testenv.go`:
- Around line 1416-1418: The testenv currently leaves
cfg.SubgraphExtensionPropagation as a zero-value so callbacks via
testConfig.ModifySubgraphExtensionPropagation can produce an empty Algorithm
that differs from runtime defaults; update the setup in configureRouter/testenv
so cfg.SubgraphExtensionPropagation.Algorithm is defaulted to "first_write" (the
runtime default) before invoking testConfig.ModifySubgraphExtensionPropagation
(or ensure the callback only mutates non-zero fields), referencing the symbols
cfg.SubgraphExtensionPropagation and
testConfig.ModifySubgraphExtensionPropagation to locate where to set the
default.
🪄 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: 0c7c9b72-d5ef-40cc-b659-473dca5ff26c
📒 Files selected for processing (18)
docs-website/docs.jsondocs-website/router/configuration.mdxdocs-website/router/subgraph-data-propagation/subgraph-error-propagation.mdxdocs-website/router/subgraph-data-propagation/subgraph-extension-propagation.mdxdocs-website/router/subscriptions-migration.mdxrouter-tests/protocol/extension_forwarding_test.gorouter-tests/testenv/testenv.gorouter/core/executor.gorouter/core/factoryresolver.gorouter/core/graph_server.gorouter/core/router.gorouter/core/router_config.gorouter/core/supervisor_instance.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
✅ Files skipped from review due to trivial changes (3)
- router/core/router_config.go
- docs-website/router/subscriptions-migration.mdx
- docs-website/router/subgraph-data-propagation/subgraph-extension-propagation.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/config/fixtures/full.yaml
- docs-website/docs.json
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
1 participant
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.
Summary by CodeRabbit
New Features
first_write/last_write) to resolve conflicts when multiple subgraphs return the same extension field.Documentation
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.