fix: keep order of error extension fields after sanitization#1407
Merged
fix: keep order of error extension fields after sanitization#1407
Conversation
Noroth
requested review from
StarpTech,
devsergiy,
dkorittki,
jensneuse and
ysmolski
as code owners
Contributor
📝 WalkthroughWalkthroughGraphQLError.Extensions was changed from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/resolve/errors.go (1)
85-97:⚠️ Potential issue | 🟠 MajorUse
StringBytes()instead ofString()to extract unquoted error codes.The
astjson.Value.String()method returns the JSON representation of the value. For a JSON string like"UNAUTHORIZED", it returns the Go string"\"UNAUTHORIZED\""(16 chars with surrounding double-quotes included). TheCodes()method currently appends these quoted values to the slice, so downstream code comparing codes against plain strings will fail.Call
StringBytes()on the code value instead:if code := downstreamError.Extensions.Get("code"); code != nil { codeStr := string(code.StringBytes()) if !slices.Contains(codes, codeStr) { codes = append(codes, codeStr) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/resolve/errors.go` around lines 85 - 97, In SubgraphError.Codes(), the code value is being appended using astjson.Value.String() which yields a quoted JSON representation; update the extraction to use code.StringBytes(), convert to string (e.g., string(code.StringBytes())), and use that unquoted codeStr when checking slices.Contains and appending to the codes slice so comparisons against plain strings succeed (references: SubgraphError.Codes, DownstreamErrors, downstreamError.Extensions.Get("code"), slices.Contains).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/resolve/errors.go`:
- Around line 40-42: Replace the panic-prone astjson.MustParseBytes call used to
populate e.Extensions from aux.Extensions with the error-returning
astjson.ParseBytes: call astjson.ParseBytes(aux.Extensions), handle and return
any parse error, and if the parsed *astjson.Value is of JSON null set
e.Extensions = nil (to preserve prior nil-map semantics and omitempty behavior);
update the code around e.Extensions and aux.Extensions to propagate the parse
error instead of panicking.
- Around line 47-61: GraphQLError.MarshalJSON currently embeds *GraphQLError in
the anonymous aux struct which promotes the MarshalJSON method and causes
infinite recursion; change the aux to embed a non-method-carrying alias (e.g.
define type Alias GraphQLError and use *Alias in aux) so json.Marshal reflects
fields instead of calling back into GraphQLError.MarshalJSON, and copy e into
the alias before marshalling; preserve the existing Extensions handling (calling
e.Extensions.MarshalTo(nil) into aux.Extensions) but assign aux.GraphQLError =
(*Alias)(&e) to break the recursion.
In `@v2/pkg/engine/resolve/loader.go`:
- Line 685: Remove the stray debug print in loader.go that prints mergeErrors to
stdout; delete the fmt.Println("mergeErrors", string(value.String())) call and,
if logging is required, replace it with the project’s logger (e.g.,
processLogger or zap) using value.String() directly (do not wrap it in
string(...)). Ensure no raw fmt.Println remains and that any replacement uses
the existing logger API for appropriate log level and message context.
---
Outside diff comments:
In `@v2/pkg/engine/resolve/errors.go`:
- Around line 85-97: In SubgraphError.Codes(), the code value is being appended
using astjson.Value.String() which yields a quoted JSON representation; update
the extraction to use code.StringBytes(), convert to string (e.g.,
string(code.StringBytes())), and use that unquoted codeStr when checking
slices.Contains and appending to the codes slice so comparisons against plain
strings succeed (references: SubgraphError.Codes, DownstreamErrors,
downstreamError.Extensions.Get("code"), slices.Contains).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/resolve/errors.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_hooks_test.gov2/pkg/engine/resolve/resolve_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/errors.go (2)
57-72:MarshalJSONinfinite-recursion bug is correctly fixed.The
type Alias GraphQLErrordeclaration ensuresAliasdoes not carry theMarshalJSONmethod, sojson.Marshal(aux)reflects over fields rather than recursively callingMarshalJSON. This resolves the critical issue flagged in the previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/resolve/errors.go` around lines 57 - 72, The previous infinite-recursion in GraphQLError.MarshalJSON is fixed by defining "type Alias GraphQLError" and embedding "*Alias" in the aux struct so json.Marshal(aux) reflects fields rather than calling MarshalJSON again; keep the current implementation in MarshalJSON (use Alias: (*Alias)(&e), set aux.Extensions = e.Extensions.MarshalTo(nil) when e.Extensions != nil) and ensure the symbols GraphQLError, MarshalJSON, Alias, aux, Extensions, and MarshalTo remain as shown to prevent recursion.
43-50:⚠️ Potential issue | 🟡 MinorJSON
nullextensions still produces a non-nil*astjson.Value.
"extensions": nullpasses thelen(aux.Extensions) > 0guard (4 bytes) andastjson.ParseBytesreturns a non-nil TypeNull value, leavinge.Extensions != nil. This diverges from the previous nil-map behaviour: theomitemptytag on the struct field won't suppress it during marshalling, and callers that nil-checkExtensionsbefore calling.Get(...)will behave differently.The prior review proposed guarding against this:
🛡️ Proposed fix
if len(aux.Extensions) > 0 { extensions, err := astjson.ParseBytes(aux.Extensions) if err != nil { return err } - e.Extensions = extensions + if extensions.Type() != astjson.TypeNull { + e.Extensions = extensions + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/resolve/errors.go` around lines 43 - 50, The code currently parses aux.Extensions and assigns e.Extensions even when the JSON is literal null (astjson returns a non-nil TypeNull), causing e.Extensions to be non-nil; fix by, after calling astjson.ParseBytes(aux.Extensions), check the parsed value's type and only assign e.Extensions when the parsed value is not the JSON null sentinel (e.g., parsed.Type() != astjson.TypeNull or !parsed.IsNull()); leave e.Extensions nil for JSON null so omitempty and nil-checks behave as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/resolve/errors.go`:
- Around line 100-102: In Codes(), guard against a nil Extensions pointer on
downstreamError before calling Get: check that downstreamError.Extensions != nil
(or similar) prior to calling downstreamError.Extensions.Get("code"); only call
Get and append code.String() when Extensions is non-nil and the returned value
is non-nil to avoid the nil-pointer panic in Codes().
---
Duplicate comments:
In `@v2/pkg/engine/resolve/errors.go`:
- Around line 57-72: The previous infinite-recursion in GraphQLError.MarshalJSON
is fixed by defining "type Alias GraphQLError" and embedding "*Alias" in the aux
struct so json.Marshal(aux) reflects fields rather than calling MarshalJSON
again; keep the current implementation in MarshalJSON (use Alias: (*Alias)(&e),
set aux.Extensions = e.Extensions.MarshalTo(nil) when e.Extensions != nil) and
ensure the symbols GraphQLError, MarshalJSON, Alias, aux, Extensions, and
MarshalTo remain as shown to prevent recursion.
- Around line 43-50: The code currently parses aux.Extensions and assigns
e.Extensions even when the JSON is literal null (astjson returns a non-nil
TypeNull), causing e.Extensions to be non-nil; fix by, after calling
astjson.ParseBytes(aux.Extensions), check the parsed value's type and only
assign e.Extensions when the parsed value is not the JSON null sentinel (e.g.,
parsed.Type() != astjson.TypeNull or !parsed.IsNull()); leave e.Extensions nil
for JSON null so omitempty and nil-checks behave as before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/resolve/errors.gov2/pkg/engine/resolve/loader.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/loader.go
devsergiy
approved these changes
Feb 25, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v2/pkg/engine/resolve/errors.go (1)
43-50:⚠️ Potential issue | 🟡 MinorTreat
extensions: nullas absent to preserve previous behavior.On Line 43, non-empty
extensionsvalues are always parsed, so JSONnullbecomes a non-nil*astjson.Value. That changes prior nil-map semantics and can force"extensions": nullinstead of omitting the field.💡 Proposed fix
if len(aux.Extensions) > 0 { - extensions, err := astjson.ParseBytes(aux.Extensions) - if err != nil { - return err - } - - e.Extensions = extensions + raw := bytes.TrimSpace(aux.Extensions) + if bytes.Equal(raw, []byte("null")) { + e.Extensions = nil + } else { + extensions, err := astjson.ParseBytes(raw) + if err != nil { + return err + } + e.Extensions = extensions + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/resolve/errors.go` around lines 43 - 50, The code currently parses aux.Extensions unconditionally when its length>0 which converts JSON null into a non-nil astjson.Value and changes semantics; update the conditional around aux.Extensions in errors.go to detect and treat JSON "null" as absent before calling astjson.ParseBytes (e.g., check aux.Extensions equals the bytes for "null" or parse and verify the resulting value is not a JSON null) and only assign to e.Extensions when a non-null parsed value is present, keeping references to aux.Extensions, astjson.ParseBytes, and e.Extensions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@v2/pkg/engine/resolve/errors.go`:
- Around line 43-50: The code currently parses aux.Extensions unconditionally
when its length>0 which converts JSON null into a non-nil astjson.Value and
changes semantics; update the conditional around aux.Extensions in errors.go to
detect and treat JSON "null" as absent before calling astjson.ParseBytes (e.g.,
check aux.Extensions equals the bytes for "null" or parse and verify the
resulting value is not a JSON null) and only assign to e.Extensions when a
non-null parsed value is present, keeping references to aux.Extensions,
astjson.ParseBytes, and e.Extensions.
Noroth
pushed a commit
that referenced
this pull request
Feb 25, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.257](v2.0.0-rc.256...v2.0.0-rc.257) (2026-02-25) ### Bug Fixes * keep order of error extension fields after sanitization ([#1407](#1407)) ([e974cd3](e974cd3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed an issue where the order of error extension fields could be lost during sanitization in v2. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
3 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.
When sanitizing the fields in the error extensions, we did not keep the order. This made the response rather non-deterministic. This PR makes sure that the order of fields are retained.
Summary by CodeRabbit
Improvements
Tests
Checklist