feat: allow rendering custom extensions in the final JSON response#1489
feat: allow rendering custom extensions in the final JSON response#1489
Conversation
📝 WalkthroughWalkthroughThis PR captures top-level GraphQL response ChangesSubgraph Extension Capture and Forwarding
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader
participant Resolvable
participant Printer
Client->>Loader: execute fetch/merge
Loader->>Resolvable: append parsed subgraph extensions
Resolvable->>Resolvable: filterAllowedSubgraphExtensions (apply allow-list & algorithm)
Resolvable->>Printer: printExtensions (include allowedExtensions)
Printer-->>Client: final JSON response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…forwarding-support
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.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/resolvable_test.go (1)
1433-1438: ⚡ Quick winUse
NewContext()in the helper.This helper introduces a bare
Context{}intov2/pkg/engine/resolve, but the package relies on constructor-initialized state. Switching toNewContext(context.Background())keeps the test aligned with production setup and avoids future nil-map failures when the helper starts touching response/subgraph error state.Based on learnings Ensure all Context instances in `v2/pkg/engine/resolve` are created with `NewContext()` instead of `Context{}` literals.💡 Suggested change
runResolve := func(t *testing.T, opts ResolvableOptions, extensions ...string) string { t.Helper() res := NewResolvable(nil, opts) - ctx := &Context{} + ctx := NewContext(context.Background()) err := res.Init(ctx, []byte(`{"hello":"world"}`), ast.OperationTypeQuery) assert.NoError(t, err)🤖 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 `@v2/pkg/engine/resolve/resolvable_test.go` around lines 1433 - 1438, The runResolve test helper creates a bare Context literal (Context{}) which bypasses constructor-initialized state used elsewhere; update the helper to call NewContext(context.Background()) instead of using Context{} so any maps/fields are properly initialized before calling res.Init in NewResolvable; ensure the test imports/uses context.Background() when constructing the Context via NewContext(context.Background()) and run the same assertions after Init.
🤖 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 `@v2/pkg/engine/resolve/loader.go`:
- Around line 573-579: The extensions object from a subgraph response is being
ignored when the loader early-returns on null data; capture and record the
top-level extensions before that early return so error-related metadata isn't
lost. In loader.go, read response.Get("extensions") into a local variable and,
if l.allowCustomExtensionProperties and astjson.ValueIsNonNull(...)/TypeObject,
append it to l.resolvable.subgraphExtensions (same logic as the existing block)
before the code path that returns on null data; ensure you only append once by
removing or guarding the later append branch if you move the logic.
In `@v2/pkg/engine/resolve/resolvable.go`:
- Around line 459-467: getDefaultReservedExtensions currently reserves keys for
authorization, rateLimit, queryPlan, trace, and valueCompletion unconditionally;
change it to only include a key when the corresponding router/response extension
is actually enabled. Update getDefaultReservedExtensions to accept booleans (or
a config struct) indicating which extensions are enabled and only add
string(literalAuthorization), string(literalRateLimit),
string(literalQueryPlan), string(literalTrace), and
string(literalValueCompletion) when their flags are true, and then update all
callers (the other spots referenced around the function) to pass the right
enablement flags from the router/resolvable config so legitimate subgraph
extensions can be forwarded.
- Around line 69-70: The Resolvable struct's request-scoped fields
subgraphExtensions and allowedExtensions are not cleared between requests;
update Resolvable.Reset() (and/or Init()) to reset subgraphExtensions to nil (or
an empty slice) and allowedExtensions to nil (or make a new empty map) so
previously forwarded extensions are not reused across resolves; locate the
Resolvable type and ensure both Reset() and Init() clear or reinitialize these
fields to their zero-values.
---
Nitpick comments:
In `@v2/pkg/engine/resolve/resolvable_test.go`:
- Around line 1433-1438: The runResolve test helper creates a bare Context
literal (Context{}) which bypasses constructor-initialized state used elsewhere;
update the helper to call NewContext(context.Background()) instead of using
Context{} so any maps/fields are properly initialized before calling res.Init in
NewResolvable; ensure the test imports/uses context.Background() when
constructing the Context via NewContext(context.Background()) and run the same
assertions after Init.
🪄 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: 6ba6c0b0-ec66-4510-89e5-a3c9a5a3b135
📒 Files selected for processing (5)
v2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolvable_test.gov2/pkg/engine/resolve/resolve.go
|
|
||
| type ExtensionForwardingAlgorithm string | ||
|
|
||
| const ( |
There was a problem hiding this comment.
From what I understood without going too deep, this is for extracting extensions from subgraphs. You mention "Currently only a small set of extensions are allowed to be returned.". How are conflicts from these extensions handled right now?
There was a problem hiding this comment.
I ignore extensions that are pre-defined for our engine
…forwarding-support
This PR adds options to add user defined extensions from subgraph responses to the final rendered JSON response. Currently only a small set of extensions are allowed to be returned. This PR allows users to configure root level extension fields that should be propagated to the client.
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.