feat: improve memory management & request deduplication#1336
feat: improve memory management & request deduplication#1336
Conversation
…to resolve Context
# Conflicts: # go.work.sum # v2/go.mod # v2/go.sum # v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
jensneuse
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
ysmolski
as code owners
📝 WalkthroughWalkthroughDatasource Load APIs now accept http.Header and return response bytes; arena-based pooling and arena.Arena usage was added widely (resolver, loader, gRPC json builder, AST visitor); sharded single‑flight deduplication for subgraph/inbound requests introduced; subscription UniqueRequestID/hash logic and ParallelListItemFetch removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver
participant SF as SubgraphSingleFlight
participant DataSource
Client->>Resolver: GraphQL request (headers, variables)
Resolver->>SF: GetOrCreateItem(fetchItem, input, extraKey)
alt follower (shared)
SF-->>Resolver: existing item (shared)
Resolver->>Resolver: wait on item.loaded
else leader (new)
SF-->>Resolver: new item (leader)
Resolver->>DataSource: Load(ctx, headers, input)
DataSource-->>Resolver: (data []byte, err)
Resolver->>SF: Finish(item)
end
Resolver-->>Client: Response (assembled with arena)
sequenceDiagram
participant App
participant Resolver
participant ArenaPool
participant Loader
participant DataSource
App->>Resolver: ArenaResolveGraphQLResponse(ctx, response)
Resolver->>ArenaPool: Acquire()
ArenaPool-->>Resolver: arena
Resolver->>Loader: Init with arena
Loader->>DataSource: Load(ctx, headers, input)
DataSource-->>Loader: (data []byte, err)
Loader->>Loader: Parse & Merge (with arena)
Loader->>Resolver: assembled response bytes
Resolver->>ArenaPool: Release(arena)
Resolver-->>App: Write response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
v2/pkg/engine/resolve/inputtemplate.go (3)
161-161: Fix unchecked error to resolve pipeline failure.The
errchecklinter flags this line becauseWriteStringcan return an error. With the interface abstraction, implementations might fail, unlike*bytes.Bufferwhich never returns errors.Apply this diff to handle the error:
- preparedInput.WriteString(value[0]) - return nil + _, err := preparedInput.WriteString(value[0]) + return err
168-168: Handle WriteString error for consistency.Similar to line 161, this
WriteStringcall ignores potential errors. Although not flagged by the pipeline (possibly because the linter stopped at the first error), it should be handled consistently.Apply this diff to handle the error:
- preparedInput.WriteString(value[j]) + if _, err := preparedInput.WriteString(value[j]); err != nil { + return err + }
39-51: Update caller type compatibility in loader.goThe signature change to
InputTemplateWriterintroduces type mismatches at both call sites. The callers in loader.go still initializepreparedInputas*bytes.Bufferand pass it toSetInputUndefinedVariables, which now requiresInputTemplateWriter:
- Line 1340 & 1386:
preparedInput := bytes.NewBuffer(nil)passed to function expectingInputTemplateWriter- Line 1428 & 1500:
preparedInput := bytes.NewBuffer(make([]byte, 0, 64))passed to function expectingInputTemplateWriterEither revert the signature change to
*bytes.Bufferor update the callers in loader.go to useInputTemplateWriter.v2/pkg/engine/resolve/loader_test.go (1)
1029-1049: Fix benchmark assertion to match new response format.The benchmark expects a response with an
"errors"field, but the new response format no longer includes an empty errors array by default. The pipeline failure indicates this assertion is failing.Update the expected output to match the actual response format:
- expected := `{"errors":[],"data":{"topProducts":[{"name":"Table","__typename":"Product","upc":"1","reviews":[{"body":"Love Table!","author":{"__typename":"User","id":"1","name":"user-1"}},{"body":"Prefer other Table.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":8},{"name":"Couch","__typename":"Product","upc":"2","reviews":[{"body":"Couch Too expensive.","author":{"__typename":"User","id":"1","name":"user-1"}}],"stock":2},{"name":"Chair","__typename":"Product","upc":"3","reviews":[{"body":"Chair Could be better.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":5}]}}` + expected := `{"data":{"topProducts":[{"name":"Table","__typename":"Product","upc":"1","reviews":[{"body":"Love Table!","author":{"__typename":"User","id":"1","name":"user-1"}},{"body":"Prefer other Table.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":8},{"name":"Couch","__typename":"Product","upc":"2","reviews":[{"body":"Couch Too expensive.","author":{"__typename":"User","id":"1","name":"user-1"}}],"stock":2},{"name":"Chair","__typename":"Product","upc":"3","reviews":[{"body":"Chair Could be better.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":5}]}}`v2/pkg/engine/resolve/resolvable.go (1)
109-121: Non-nil arena precondition across parse/merge/error pathsThese sites assume r.astjsonArena is non-nil (ParseBytesWithArena, MergeValues, SetArrayItem, ArrayValue, AppendError*). This is fine if callers set the arena first; otherwise it’s crash-prone. The fix is in resolve.go (ensure arena is acquired and injected before Init), see my comment there.
Also applies to: 470-479, 862-863, 1198-1212, 1286-1295, 1298-1312
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
252-257: Avoid mutating message.Fields via slice aliasing.validFields := message.Fields followed by append(...) can mutate the underlying slice, causing duplicated fields across calls.
Apply:
- validFields := message.Fields + validFields := append(RPCFields(nil), message.Fields...) if message.IsOneOf() { - validFields = append(validFields, message.FieldSelectionSet.SelectFieldsForTypes( + validFields = append(validFields, message.FieldSelectionSet.SelectFieldsForTypes( message.SelectValidTypes(string(data.Type().Descriptor().Name())))...) }Replace RPCFields with the actual slice type alias (e.g., []RPCField) if different.
🧹 Nitpick comments (32)
v2/pkg/engine/resolve/inputtemplate.go (1)
47-47: Consider handling Write errors consistently.These
Writecalls ignore potential errors. While not flagged by the pipeline, handling errors consistently would improve robustness, especially since the interface abstraction now allows for implementations that might fail.Consider handling errors from all
Writecalls similar to theWriteStringfixes. For example, at line 84:- _, _ = preparedInput.Write(segment.Data) + if _, err = preparedInput.Write(segment.Data); err != nil { + return err + }Apply similar patterns to lines 47, 106, 123, 144, and 166 if error handling is desired.
Also applies to: 84-84, 106-106, 123-123, 144-144, 166-166
v2/pkg/engine/resolve/context.go (2)
39-48: Clarify SubgraphHeadersBuilder contractAdd a short doc that the returned uint64 is the dedup scope key (stable within a request, distinct across requests/users) and that headers must be treated as immutable.
-type SubgraphHeadersBuilder interface { - HeadersForSubgraph(subgraphName string) (http.Header, uint64) -} +// SubgraphHeadersBuilder provides per-subgraph request headers and a dedup scope key. +// The returned header must be treated as immutable by callers. +// The returned uint64 should uniquely scope deduplication (e.g., request ID), +// so concurrent operations with different auth headers do not deduplicate together. +type SubgraphHeadersBuilder interface { + HeadersForSubgraph(subgraphName string) (http.Header, uint64) +}Confirm all call sites of HeadersForSubgraphRequest use the returned uint64 as the SingleFlight extraKey.
211-224: Hygiene: reset Request.ID and SubgraphHeadersBuilder on Free()Minor cleanup to avoid leaking state when Contexts are pooled/reused.
func (c *Context) Free() { c.ctx = nil c.Variables = nil c.Files = nil c.Request.Header = nil + c.Request.ID = 0 c.RenameTypeNames = nil c.RemapVariables = nil c.TracingOptions.DisableAll() c.Extensions = nil c.subgraphErrors = nil c.authorizer = nil c.LoaderHooks = nil + c.SubgraphHeadersBuilder = nil }v2/pkg/engine/resolve/singleflight.go (3)
16-23: Remove unused cleanup channel
cleanup chan func()is never used; drop it to reduce noise.type SingleFlight struct { mu *sync.RWMutex items map[uint64]*SingleFlightItem sizes map[uint64]*fetchSize xxPool *sync.Pool - cleanup chan func() } @@ func NewSingleFlight() *SingleFlight { return &SingleFlight{ items: make(map[uint64]*SingleFlightItem), sizes: make(map[uint64]*fetchSize), mu: new(sync.RWMutex), xxPool: &sync.Pool{ New: func() any { return xxhash.New() }, }, - cleanup: make(chan func()), } }Also applies to: 29-41
118-136: Size hint rollover: consider EMA for smoother adaptation (optional)Current reset at 50 samples turns the moving average into ~50% weight on the last sample. An exponential moving average would be smoother under traffic variance.
If desired, replace the rollover with
avg = alpha*avg + (1-alpha)*len(response)stored as fixed‑point to avoid floats.
75-83: Hash extraKey into the digest instead of adding it to reduce collision riskCurrently,
sfKeyis computed ash.Sum64() + extraKey, which can collide (e.g., 100+50 and 75+75 both yield 150). SinceextraKeycomes from request headers viaHeadersForSubgraph, it should be mixed into the hash to ensure distinct headers produce distinct keys.@@ -import ( - "sync" - - "github.com/cespare/xxhash/v2" -) +import ( + "encoding/binary" + "sync" + + "github.com/cespare/xxhash/v2" +) @@ -func (s *SingleFlight) sfKey(h *xxhash.Digest, fetchItem *FetchItem, input []byte, extraKey uint64) uint64 { +func (s *SingleFlight) sfKey(h *xxhash.Digest, fetchItem *FetchItem, input []byte, extraKey uint64) uint64 { if fetchItem != nil && fetchItem.Fetch != nil { info := fetchItem.Fetch.FetchInfo() if info != nil { _, _ = h.WriteString(info.DataSourceID) _, _ = h.WriteString(":") } } _, _ = h.Write(input) - return h.Sum64() + extraKey + var ek [8]byte + binary.LittleEndian.PutUint64(ek[:], extraKey) + _, _ = h.Write(ek[:]) + return h.Sum64() }Also applies to: 85-96
v2/pkg/engine/datasource/introspection_datasource/source_test.go (1)
31-41: Return-based Load usage looks good; simplify newline trim.
The switch to returning responseData is correct. You can simplify the newline removal from json.Indent with TrimSuffix for clarity.-// Trim the trailing newline that json.Indent adds -responseBytes := actualResponse.Bytes() -if len(responseBytes) > 0 && responseBytes[len(responseBytes)-1] == '\n' { - responseBytes = responseBytes[:len(responseBytes)-1] -} +// Trim the trailing newline that json.Indent adds +responseBytes := bytes.TrimSuffix(actualResponse.Bytes(), []byte{'\n'})v2/pkg/variablesvalidation/variablesvalidation.go (1)
101-101: Normalize empty/null variables to "{}" before parsing.
To match GraphQL semantics and mirror UploadFinder, guard nil/empty/"null" variables so ParseBytes never errors on absent variables.func (v *VariablesValidator) Validate(operation, definition *ast.Document, variables []byte) error { v.visitor.definition = definition v.visitor.operation = operation - v.visitor.variables, v.visitor.err = astjson.ParseBytes(variables) + // Normalize absent variables to {} + if variables == nil || bytes.Equal(variables, []byte("null")) || bytes.Equal(variables, []byte("")) { + variables = []byte("{}") + } + v.visitor.variables, v.visitor.err = astjson.ParseBytes(variables) if v.visitor.err != nil { return v.visitor.err }Please confirm callers never pass nil/empty variables inadvertently. If they do, add/keep this guard to prevent regressions.
v2/pkg/engine/resolve/resolvable_test.go (1)
15-15: Constructor updates LGTM; consider a tiny test helper to DRY.
All call sites now pass the arena argument (nil) correctly. Optionally add a newTestResolvable(opts) helper to cut duplication.func newTestResolvable(opts ResolvableOptions) *Resolvable { return NewResolvable(nil, opts) }Also applies to: 87-87, 160-160, 234-234, 262-262, 337-337, 373-373, 443-443, 473-473, 504-504, 552-552, 626-626, 656-656, 690-690, 722-722, 758-758, 841-841, 926-926, 953-953, 976-976, 998-998, 1021-1021, 1049-1049, 1147-1147, 1245-1245, 1345-1345
v2/pkg/engine/resolve/variables_renderer.go (1)
350-356: Use sync.Pool.New to remove the nil-branch; ensure Reset clears arena.
Minor micro-optimization: initialize the pool with New to simplify getResolvable.-var ( - _graphQLVariableResolveRendererPool = &sync.Pool{} -) +var ( + _graphQLVariableResolveRendererPool = &sync.Pool{ + New: func() any { return NewResolvable(nil, ResolvableOptions{}) }, + } +) func (g *GraphQLVariableResolveRenderer) getResolvable() *Resolvable { - v := _graphQLVariableResolveRendererPool.Get() - if v == nil { - return NewResolvable(nil, ResolvableOptions{}) - } - return v.(*Resolvable) + return _graphQLVariableResolveRendererPool.Get().(*Resolvable) }Please confirm Resolvable.Reset releases any arena-backed state (if ever set on this path), so pooled instances don’t retain large allocations between uses.
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
78-80: Consider returning an error instead of panicking.The panic for unimplemented functionality could cause runtime crashes if this method is accidentally called. Consider returning a descriptive error instead:
-func (Source) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { - panic("not implemented") -} +func (Source) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { + return nil, fmt.Errorf("static data source does not support file uploads") +}Note: You'll need to add
"fmt"to the imports if you adopt this change.v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (1)
8455-8455: Updated Start signature: ensure nil-safe header handlingCalls now use Start(ctx, headers, options, updater) with headers=nil. Verify the implementation treats nil as empty and does not mutate the provided map. Consider adding one test that passes non-empty headers and asserts they reach the upstream.
Also applies to: 8461-8461, 8474-8474, 8486-8486, 8504-8504, 8527-8527, 8591-8591, 8611-8611, 8635-8635
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (2)
1957-1964: Avoid clobbering configured headers; prefer merge with precedence.Overwriting options.Header loses statically configured subscription headers. Merge instead, letting passed headers override duplicates.
Apply:
err := json.Unmarshal(input, &options) if err != nil { return err } - options.Header = headers + // merge configured headers with passed headers; passed headers take precedence + if options.Header == nil { + options.Header = make(http.Header) + } + for k, v := range headers { + options.Header[k] = v + }
1977-1984: Same header-merge concern as AsyncStart.Use the same merge strategy here to preserve configured headers.
- options.Header = headers + if options.Header == nil { + options.Header = make(http.Header) + } + for k, v := range headers { + options.Header[k] = v + }v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
154-156: Do not panic; return a proper error for unsupported file uploads.Panics in datasource paths can crash the process. Return an error or GraphQL error JSON.
-func (d *DataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { - panic("unimplemented") -} +func (d *DataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { + // gRPC transport does not support multipart uploads + return nil, fmt.Errorf("gRPC datasource: file uploads are not supported") +}v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
237-239: Remove leftover debug printing.Avoid noisy stdout in tests.
- bytes := output - fmt.Println(string(bytes)) + bytes := outputv2/pkg/engine/datasource/pubsub_datasource/pubsub_kafka.go (2)
59-61: Avoid panic; return unsupported operation error.Never panic in datasource code paths.
-func (s *KafkaPublishDataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { - panic("not implemented") -} +func (s *KafkaPublishDataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { + return nil, fmt.Errorf("kafka publish: file uploads are not supported") +}
46-57: Choose one error strategy: return error or JSON status, not both.Returning non-nil error and a JSON body can cause double-handling upstream. Prefer nil error with explicit JSON or return only error.
Option A (error-only):
- if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { - return []byte(`{"success": false}`), err - } - return []byte(`{"success": true}`), nil + if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { + return nil, err + } + return []byte(`{"success": true}`), nilOption B (JSON-only, nil error):
- if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { - return []byte(`{"success": false}`), err - } + if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { + return []byte(`{"success": false","error":"publish failed"}`), nil + }Pick consistently across datasources.
Please confirm the engine’s expectation for datasource error handling so we align consistently.
v2/pkg/engine/resolve/resolvable.go (1)
81-100: Reset clears the arena; ensure it is set before next Init/ResolveReset sets r.astjsonArena = nil. Any subsequent Init/Resolve must set the arena first, or calls like astjson.ObjectValue/ParseBytesWithArena will likely panic or mis-allocate. Add a comment or a guard, and ensure all call sites set the arena before use.
Would you like me to add a SetArena(a arena.Arena) helper and guard Init when arena is nil?v2/pkg/engine/resolve/resolve.go (3)
1089-1114: Set arena in subscription SkipLoader path for consistencySkipLoader still constructs Resolvable with nil arena. It often works, but error/value-completion paths would allocate via arena. Acquire/assign an arena as in ResolveGraphQLResponse to avoid surprises.
Example:
- t := newTools(r.options, r.allowedErrorExtensionFields, r.allowedErrorFields, r.sf) + t := newTools(r.options, r.allowedErrorExtensionFields, r.allowedErrorFields, r.sf) + resolveArena := r.resolveArenaPool.Acquire(ctx.Request.ID) + t.loader.jsonArena = resolveArena.Arena + t.resolvable.astjsonArena = resolveArena.Arena + defer r.resolveArenaPool.Release(ctx.Request.ID, resolveArena)
1199-1224: Also set arena in Async subscription SkipLoader pathSame rationale as above; apply the same acquire/assign/release here.
1168-1174: Improve uniqueID mixing: write headersHash into hasher instead of addingAdding 64-bit hashes can increase collisions. Prefer feeding headersHash bytes into xxhash and use Sum64().
Suggested change (add import "encoding/binary"):
+ import "encoding/binary" @@ - _, _ = xxh.Write(input) - // the hash for subgraph headers is pre-computed - // we can just add it to the input hash to get a unique id - uniqueID := xxh.Sum64() + headersHash + _, _ = xxh.Write(input) + var hb [8]byte + binary.LittleEndian.PutUint64(hb[:], headersHash) + _, _ = xxh.Write(hb[:]) + uniqueID := xxh.Sum64()Also applies to: 1226-1233
v2/pkg/engine/datasource/pubsub_datasource/pubsub_nats.go (2)
43-51: Headers currently unusedStart receives headers but the underlying NatsPubSub.Subscribe doesn’t. If headers are intentionally ignored for NATS, add a short comment; otherwise consider plumbing usage or dropping the param in this source.
79-93: LGTM: request path returns data slice and propagates errorReadability nit: rename subscriptionConfiguration -> requestConfiguration. Optional.
v2/pkg/engine/resolve/resolve_test.go (1)
112-136: Remove or use unused parameter enableSingleFlight.The enableSingleFlight bool is unused in testFn; either wire it to options or remove it to avoid confusion.
v2/pkg/engine/datasource/httpclient/nethttpclient.go (4)
315-325: Confirm FileUpload lifetime; avoid deleting user files.The defer removes every opened file (os.Remove). If FileUpload.Path() can point to non-temporary user files, this deletes user data.
Option: only delete when FileUpload is marked temporary, or move cleanup behind an explicit flag.
- if err = os.Remove(file.Name()); err != nil { + if fileShouldBeDeleted(file) { // e.g., a field or method on FileUpload + if err = os.Remove(file.Name()); err != nil { return - } + } + }Please confirm the contract of FileUpload.Path().
232-241: Close decompressor readers after use to free resources.gzip/deflate readers implement io.ReadCloser. Close them after ReadFrom to release resources early.
out := buffer(ctx) -_, err = out.ReadFrom(respReader) +_, err = out.ReadFrom(respReader) if err != nil { return nil, err } +if rc, ok := respReader.(io.ReadCloser); ok { + _ = rc.Close() +}
209-216: Prefer a single Accept-Encoding value.Set “gzip, deflate” once instead of two separate headers; clearer and avoids duplicate values.
-request.Header.Set(AcceptEncodingHeader, EncodingGzip) -request.Header.Add(AcceptEncodingHeader, EncodingDeflate) +request.Header.Set(AcceptEncodingHeader, EncodingGzip+", "+EncodingDeflate)
238-265: Trace injection assumes JSON object; guard or fallback for non-objects.jsonparser.Set will fail for non-object bodies (arrays, scalars). Consider detecting object first; otherwise return original data and put trace into ResponseContext only.
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (2)
406-415: Clarify nesting bound check.if level > md.NestingLevel likely intends >=; using >= avoids an unnecessary extra frame and is easier to reason about.
- if level > md.NestingLevel { + if level >= md.NestingLevel { return current, nil }
114-121: Consider exposing a Reset to reuse jsonBuilder arenas safely.If jsonBuilder instances are reused, add a Reset method to re-init jsonArena (or allocate per call) to avoid unbounded growth.
v2/pkg/engine/resolve/loader.go (1)
432-446: Consider using arena for array creation.The function now accepts an arena parameter and uses it for
SetArrayItem, but the array itself is created withMustParseBytes(line 441) without the arena. For consistency and to fully leverage arena benefits, consider:- arr := astjson.MustParseBytes([]byte(`[]`)) + arr := astjson.ArrayValue(a) for i, item := range items { arr.SetArrayItem(a, i, item) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumv2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (45)
v2/go.mod(1 hunks)v2/pkg/astnormalization/uploads/upload_finder.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(3 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go(14 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go(0 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go(0 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(5 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(25 hunks)v2/pkg/engine/datasource/grpc_datasource/json_builder.go(15 hunks)v2/pkg/engine/datasource/httpclient/httpclient_test.go(2 hunks)v2/pkg/engine/datasource/httpclient/nethttpclient.go(9 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/type_introspection.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/source.go(3 hunks)v2/pkg/engine/datasource/introspection_datasource/source_test.go(1 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource_test.go(4 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_kafka.go(3 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_nats.go(3 hunks)v2/pkg/engine/datasource/staticdatasource/static_datasource.go(2 hunks)v2/pkg/engine/plan/planner_test.go(2 hunks)v2/pkg/engine/plan/visitor.go(1 hunks)v2/pkg/engine/resolve/arena.go(1 hunks)v2/pkg/engine/resolve/authorization_test.go(3 hunks)v2/pkg/engine/resolve/context.go(2 hunks)v2/pkg/engine/resolve/datasource.go(1 hunks)v2/pkg/engine/resolve/event_loop_test.go(2 hunks)v2/pkg/engine/resolve/inputtemplate.go(7 hunks)v2/pkg/engine/resolve/loader.go(50 hunks)v2/pkg/engine/resolve/loader_hooks_test.go(14 hunks)v2/pkg/engine/resolve/loader_test.go(14 hunks)v2/pkg/engine/resolve/resolvable.go(14 hunks)v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go(2 hunks)v2/pkg/engine/resolve/resolvable_test.go(26 hunks)v2/pkg/engine/resolve/resolve.go(20 hunks)v2/pkg/engine/resolve/resolve_federation_test.go(21 hunks)v2/pkg/engine/resolve/resolve_mock_test.go(3 hunks)v2/pkg/engine/resolve/resolve_test.go(29 hunks)v2/pkg/engine/resolve/response.go(1 hunks)v2/pkg/engine/resolve/singleflight.go(1 hunks)v2/pkg/engine/resolve/tainted_objects_test.go(3 hunks)v2/pkg/engine/resolve/variables_renderer.go(1 hunks)v2/pkg/fastjsonext/fastjsonext.go(1 hunks)v2/pkg/fastjsonext/fastjsonext_test.go(1 hunks)v2/pkg/variablesvalidation/variablesvalidation.go(1 hunks)
💤 Files with no reviewable changes (2)
- v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go
🧰 Additional context used
🧬 Code graph analysis (30)
v2/pkg/engine/resolve/variables_renderer.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/resolve/context.go (1)
v2/pkg/engine/plan/planner.go (1)
IncludeQueryPlanInResponse(92-96)
v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/resolve/loader_test.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/datasource/introspection_datasource/source.go (2)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/resolve_federation_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)
v2/pkg/engine/resolve/response.go (4)
v2/pkg/engine/resolve/inputtemplate.go (1)
InputTemplate(31-37)v2/pkg/engine/resolve/variables.go (1)
Variables(27-27)v2/pkg/engine/resolve/datasource.go (1)
SubscriptionDataSource(15-19)v2/pkg/engine/resolve/fetch.go (2)
PostProcessingConfiguration(116-132)QueryPlan(251-254)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (3)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
DoMultipartForm(272-328)Do(267-270)
v2/pkg/engine/resolve/resolve_mock_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/resolvable_test.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (3)
v2/pkg/engine/resolve/context.go (1)
NewContext(168-175)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
SubscriptionSource(1953-1955)v2/pkg/engine/datasource/httpclient/file.go (2)
FileUpload(3-7)NewFileUpload(9-15)
v2/pkg/engine/resolve/resolvable.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(181-245)v2/pkg/fastjsonext/fastjsonext.go (2)
AppendErrorToArray(8-15)AppendErrorWithExtensionsCodeToArray(17-27)
v2/pkg/engine/plan/planner_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/resolve_test.go (10)
v2/pkg/engine/resolve/context.go (2)
Context(16-37)ExecutionOptions(50-56)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
Do(267-270)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)v2/pkg/engine/resolve/response.go (2)
GraphQLResponse(35-43)GraphQLResponseInfo(45-47)v2/pkg/engine/resolve/fetchtree.go (6)
Single(59-67)SingleWithPath(69-82)Sequence(26-31)Parallel(33-38)ArrayPath(52-57)ObjectPath(40-45)v2/pkg/engine/resolve/fetch.go (4)
SingleFetch(91-99)SingleFetch(153-155)FetchConfiguration(270-302)PostProcessingConfiguration(116-132)v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)v2/pkg/engine/resolve/inputtemplate.go (5)
InputTemplate(31-37)TemplateSegment(22-29)SegmentType(15-15)StaticSegmentType(18-18)VariableSegmentType(19-19)v2/pkg/engine/resolve/variables.go (4)
VariableKind(7-7)ContextVariableKind(10-10)Variables(27-27)ResolvableObjectVariableKind(13-13)v2/pkg/engine/resolve/subscription_filter.go (2)
SubscriptionFilter(16-21)SubscriptionFieldFilter(23-26)
v2/pkg/engine/resolve/singleflight.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(181-245)v2/pkg/engine/resolve/fetch.go (3)
FetchItem(29-34)Fetch(20-27)FetchInfo(376-397)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_kafka.go (3)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)v2/pkg/engine/datasource/pubsub_datasource/kafka_event_manager.go (1)
KafkaPublishEventConfiguration(17-21)
v2/pkg/engine/resolve/loader.go (5)
v2/pkg/engine/resolve/singleflight.go (1)
SingleFlight(16-22)v2/pkg/engine/resolve/fetch.go (5)
FetchItemPathElement(78-82)FetchItem(29-34)DataSourceLoadTrace(405-419)Fetch(20-27)FetchInfo(376-397)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/pool/hash64.go (1)
Hash64(10-16)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
WithHTTPClientSizeHint(139-141)
v2/pkg/engine/resolve/inputtemplate.go (1)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)
v2/pkg/engine/resolve/datasource.go (3)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)
v2/pkg/engine/resolve/event_loop_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (4)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
Source(1829-1831)v2/pkg/engine/datasource/introspection_datasource/source.go (1)
Source(18-20)v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/fastjsonext/fastjsonext_test.go (1)
v2/pkg/fastjsonext/fastjsonext.go (3)
AppendErrorToArray(8-15)PathElement(29-32)CreateErrorObjectWithPath(34-50)
v2/pkg/engine/resolve/resolve.go (6)
v2/pkg/engine/resolve/arena.go (2)
ArenaPool(13-17)NewArenaPool(30-34)v2/pkg/engine/resolve/singleflight.go (2)
SingleFlight(16-22)NewSingleFlight(29-41)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)v2/pkg/engine/resolve/context.go (4)
Context(16-37)Request(163-166)ExecutionOptions(50-56)SubgraphHeadersBuilder(39-41)v2/pkg/engine/resolve/response.go (3)
GraphQLResponse(35-43)GraphQLSubscription(12-16)SubscriptionResponseWriter(68-74)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
RPCMessage(71-85)ListMetadata(190-195)LevelInfo(198-201)
v2/pkg/engine/resolve/loader_hooks_test.go (1)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_nats.go (4)
v2/pkg/engine/resolve/context.go (2)
Context(16-37)Request(163-166)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)v2/pkg/engine/datasource/pubsub_datasource/nats_event_manager.go (1)
NatsPublishAndRequestEventConfiguration(31-35)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/authorization_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)
v2/pkg/engine/datasource/httpclient/httpclient_test.go (1)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
Do(267-270)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (2)
v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
🪛 GitHub Actions: v2-ci
v2/pkg/engine/resolve/loader_test.go
[error] 1046-1046: BenchmarkLoader_LoadGraphQLResponseData failed: expected a response containing an errors field, but got a response without 'errors' field. Deep comparison mismatch detected in test.
v2/pkg/engine/resolve/inputtemplate.go
[error] 161-161: golangci-lint: Error return value of preparedInput.WriteString is not checked (errcheck).
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/resolvable.go
[failure] 14-14:
File is not properly formatted (gci)
v2/pkg/engine/resolve/loader.go
[failure] 20-20:
File is not properly formatted (gci)
v2/pkg/engine/resolve/resolve.go
[failure] 15-15:
File is not properly formatted (gci)
🔇 Additional comments (52)
v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden (1)
514-514: LGTM - Formatting-only change.The trailing newline adjustment has no functional impact on the fixture data.
v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden (1)
366-366: LGTM - Formatting-only change.The trailing newline adjustment has no functional impact on the fixture data.
v2/pkg/engine/datasource/introspection_datasource/fixtures/type_introspection.golden (1)
59-59: LGTM - Formatting-only change.The trailing newline adjustment has no functional impact on the fixture data.
v2/pkg/engine/resolve/inputtemplate.go (3)
7-7: LGTM!The
ioimport is necessary for theInputTemplateWriterinterface definition.
58-63: LGTM! Clean abstraction for arena-based memory management.The interface is well-designed and appropriately scoped, including only the methods needed for the rendering flow. This abstraction enables the arena-based memory management improvements mentioned in the PR objectives while maintaining compatibility with
*bytes.Buffer.
65-65: All callers are compatible with the interface change.The exported methods
RenderandRenderAndCollectUndefinedVariableshave been correctly updated to acceptInputTemplateWriter. Sincebytes.Bufferimplements the required interface (io.Writer, io.StringWriter, Reset(), Bytes()), all existing callers—in loader.go, resolve.go, subscription_filter.go, and tests—pass compatible types without requiring updates.v2/pkg/engine/resolve/tainted_objects_test.go (1)
73-75: LGTM: switched to astjson.ParseBytesThe change is straightforward; parsing behavior in tests remains correct.
Also applies to: 97-99, 323-327
v2/pkg/engine/plan/visitor.go (1)
1293-1296: Populate SourceName/ID on subscription triggerGood addition; aligns tests and surfaces subgraph identity on triggers.
Confirm objectFetchConfiguration always sets both sourceName and sourceID for subscription fetches; if either can be empty, consider defaulting SourceName to sourceID (and vice versa) for consistency.
v2/pkg/engine/resolve/response.go (1)
19-26: LGTM: GraphQLSubscriptionTrigger includes SourceName/SourceIDPublic surface extended as intended; no behavioral changes.
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource_test.go (1)
427-429: Tests updated for SourceName/SourceIDExpectations match the new trigger fields and datasource id/name.
Also applies to: 492-494, 539-541, 592-594
v2/pkg/engine/resolve/resolve_federation_test.go (1)
19-28: Mocks updated for new Load signature and response []byteGood updates: include headers param and return []byte. Assertions validate inputs properly.
Also applies to: 23-27
v2/pkg/engine/resolve/context.go (1)
164-166: Request.ID type change verified — all consumers compatibleThe type change from prior type to
uint64is verified as safe:
- Pooling/dedup: Both
ArenaPool.Acquire()andArenaPool.Release()have signatures expectinguint64; all call sites passctx.Request.IDdirectly—fully compatible.- Logging: No code logs
Request.ID.- Tracing:
RequestData(the struct included in trace output) contains noIDfield;Request.IDis not part of trace output.- JSON marshaling: No JSON marshaling of
Requestor itsIDfield.v2/go.mod (1)
31-33: All concerns verified—no issues found.The repository correctly uses Go 1.25 across both
go.moddirective and CI workflows (matrix values in v2.yml and execution.yml both specifygo: [ "1.25" ]). Thesetup-go@v5action is compatible with this version. The astjson v1.0.0 and go-arena v1.0.0 dependencies are properly listed in the require section, go.sum is present and synchronized (226 entries), and no deprecated astjson APIs are in use. The module is tidy.v2/pkg/astnormalization/uploads/upload_finder.go (1)
77-77: ParseBytes migration verified—behavior parity and concurrency characteristics confirmed.Edge case handling (nil, "", "null" → "{}") is explicit at lines 74–76, ensuring normalized input regardless of ParseBytes implementation. Variables are accessed only within the same
FindUploadsinvocation, with no shared Parser instance—each call is independent. The migration is safe and complete (noParseBytesWithoutCachecalls remain in the codebase).v2/pkg/engine/resolve/authorization_test.go (2)
512-519: LGTM! Mock signature correctly updated.The mock expectation properly reflects the new DataSource Load signature with HTTP headers and direct byte slice return. The DoAndReturn handler is correctly structured to return
([]byte, error).
817-824: LGTM! Consistent mock pattern.All mock data source setups in this test file follow the same pattern with the updated Load signature. The test data continues to return valid GraphQL responses wrapped in the expected format.
v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go (1)
443-443: LGTM! Constructor signature updated correctly.The NewResolvable call now includes the arena.Arena parameter (nil) as the first argument, aligning with the updated constructor signature in the broader PR.
v2/pkg/fastjsonext/fastjsonext_test.go (1)
24-29: LGTM! Arena parameter migration applied consistently.The test correctly passes
nilfor the arena parameter toAppendErrorToArray, aligning with the PR's migration from*astjson.Arenatoarena.Arenainterface.v2/pkg/engine/resolve/event_loop_test.go (1)
74-85: LGTM! Subscription Start signature updated.The Start method now correctly accepts HTTP headers as the second parameter, consistent with the PR's objective to propagate headers through the data flow. The method implementation remains functionally correct.
v2/pkg/engine/resolve/loader_test.go (3)
20-34: LGTM! Mock responses correctly structured.The mock data sources now return responses wrapped under a top-level
"data"field, consistent with the GraphQL response format and the broader changes in this PR.
290-300: LGTM! Test setup correctly updated.Both the NewResolvable constructor call and the expected output assertion have been updated to match the new patterns introduced in the PR.
1524-1530: LGTM! Error path rewriting correctly updated.The rewriteErrorPaths function calls now pass
nilas the first parameter (arena), consistent with the arena-based memory management changes in this PR.v2/pkg/engine/datasource/httpclient/httpclient_test.go (1)
82-85: LGTM! Test correctly updated for new Do signature.The test now properly handles the new Do signature that returns
([]byte, error)directly instead of writing to a buffer. The assertion compares the returned bytes with the expected output.v2/pkg/engine/resolve/loader_hooks_test.go (1)
52-56: LGTM! Mock expectations correctly updated.The mock Load method now expects three parameters (context, headers, input) and the DoAndReturn handler correctly returns
([]byte, error), aligning with the updated DataSource interface.v2/pkg/engine/datasource/introspection_datasource/source.go (2)
22-33: LGTM! Load method signature correctly updated.The Load method now:
- Accepts
headers http.Headeras the second parameter- Returns
(data []byte, err error)instead of writing to a buffer- Uses the new
singleTypeByteshelper for type-specific responsesAll error paths correctly return
(nil, err)on failure.
61-68: LGTM! Helper method properly implements byte-returning pattern.The new
singleTypeByteshelper correctly mirrors the previoussingleTypelogic but returns bytes directly instead of writing to an io.Writer. The nil type handling properly returns thenullbyte slice.v2/pkg/engine/plan/planner_test.go (3)
7-7: LGTM - Import added for new API.The
net/httpimport is correctly added to support the newhttp.Headerparameter in the updated DataSource method signatures.
1078-1080: LGTM - Test mock updated to new API.The
Loadmethod signature correctly reflects the new DataSource API that accepts HTTP headers and returns data directly instead of writing to a buffer. Returningnil, nilis appropriate for a test fake.
1082-1084: LGTM - Test mock updated consistently.The
LoadWithFilesmethod signature correctly mirrors theLoadmethod changes with HTTP headers and direct return values. The test fake implementation is appropriate.v2/pkg/engine/datasource/staticdatasource/static_datasource.go (2)
5-5: LGTM - Import added for new API.The
net/httpimport correctly supports the updated method signatures.
74-76: LGTM - Static source correctly returns input.The implementation appropriately returns the input bytes directly, which is the expected behavior for a static data source.
v2/pkg/engine/resolve/resolve_mock_test.go (1)
1-67: LGTM - Auto-generated mock updated correctly.This is an auto-generated mock file (by MockGen) that has been correctly regenerated to match the new DataSource interface signatures with HTTP headers and direct return values. No manual review concerns.
v2/pkg/fastjsonext/fastjsonext.go (5)
5-5: LGTM - Arena dependency added for memory pooling.The
go-arenaimport supports the PR's objective of improving memory management through arena-based pooling.
8-15: LGTM - Arena-based error appending refactored correctly.The function signature and implementation correctly adopt the new arena-centric API, passing the arena context through value creation and mutation calls.
17-27: LGTM - Consistent arena usage for error with extensions.The function correctly uses arena-based value construction throughout, maintaining consistency with the new API pattern.
34-50: LGTM - Error object creation properly arena-aware.The function correctly constructs error objects using arena-based value creation, ensuring memory efficiency and proper lifecycle management.
52-59: No issues found - nil arena usage is intentional and correct.After thorough verification:
- The
PrintGraphQLResponsefunction usesout.Set(nil, ...)to set fields on a value created byastjson.MustParse- This nil arena pattern is consistent with the codebase: the codebase explicitly uses
ParseBytesWithArena(nil, data)in loader_test.go, demonstrating nil arena is an established, intentional pattern- Tests pass successfully across multiple scenarios (loader_test.go lines 297, 759, 1044, 1431), confirming the function works correctly
- The function serves as a terminal operation that composes pre-constructed values and serializes them—it does not manage arena lifetimes like the building functions (
AppendErrorToArray,CreateErrorObjectWithPath)- The design is appropriate: building functions receive and manage explicit arena parameters, while terminal functions like
PrintGraphQLResponseuse nil arena for simpler composition and serializationv2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (2)
4023-4025: Include of SourceName/SourceID on subscription trigger looks rightGood addition for tracing/observability and source attribution. Please just confirm the values match the configured DataSource ID in the surrounding plan to avoid misleading logs/metrics.
Also applies to: 4066-4068
8773-8776: Load now returns bytes — assertions look goodThe switch to capturing data from Load and asserting on the exact JSON payload is correct and matches the new API.
Also applies to: 8795-8798
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
1909-1917: Good: headers plumbed and return-bytes API adhered to.The methods correctly compact variables, forward headers to httpclient, and return data bytes.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
404-409: Assertion aligns with error-as-data policy; keep consistent with Load behavior.No change required; just ensure datasource consistently returns nil error in all error paths (see datasource comment).
If you adopt the merge-error change, re-run this test to confirm it still passes.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
101-129: ****The repository declares Go 1.25 in its go.mod files. Since Go 1.22+, loop variables are scoped per-iteration by default, automatically preventing the closure capture issue the review comment warns about. The suggested rebinding refactor is unnecessary—the original code is correct and does not have a closure capture bug. No changes are required.
Likely an incorrect or invalid review comment.
v2/pkg/engine/resolve/resolve.go (2)
5-20: Fix import ordering to satisfy gciLinters report “File is not properly formatted (gci)”. Run gci/fmt over this file (and module-wide) to fix grouping/order.
Example:
- standard lib
- third-party
- project-local
306-349: ArenaResolveGraphQLResponse looks solidAcquire/assign/release of arenas and buffered write via arena buffer are correct.
v2/pkg/engine/resolve/loader.go (8)
360-373: Good use of arena allocation for items.The arena-based allocation in
selectItemsForPathproperly uses the loader's arena for memory management, which aligns with the PR's memory management improvements.
547-549: Proper arena allocation before parsing.The code correctly allocates a slice copy in the arena before parsing, ensuring the parsed data is arena-managed throughout its lifecycle.
1576-1592: Operation type context propagation looks good.The new context key pattern for operation type propagation is implemented correctly with a safe default fallback.
1594-1603: Headers extraction method looks correct.The
headersForSubgraphRequestmethod properly handles nil checks and extracts headers for subgraph requests, with the extraKey used for request deduplication.
1656-1666: Direct load implementation is correct.The
loadByContextDirectmethod properly calls the updatedDataSourceinterface methods with headers, aligning with the PR's API changes.
1806-1814: Execution now properly uses loadByContext.The refactored execution flow correctly delegates to
loadByContext, which encapsulates SingleFlight deduplication logic while respecting loader hooks.
791-791: Consistent arena usage for error object creation.All error object creation now uses
ParseWithArena, ensuring arena-based memory management is applied consistently throughout the error handling paths.Also applies to: 1018-1028, 1065-1065, 1079-1079, 1091-1091, 1109-1109, 1144-1144, 1150-1150, 1160-1160, 1166-1166, 1186-1186, 1191-1191, 1198-1198, 1203-1203, 1210-1210, 1214-1214
185-186: ****The initialization concern is unfounded. Both
jsonArenaandsfare properly initialized before use:
sfis assigned innewTools()(line 265 in resolve.go) from the Resolver'ssffield, which is created withNewSingleFlight()at Resolver initialization.jsonArenais assigned from the arena pool inresolve.go(lines 319, 485) immediately before the loader is used, after whichLoadGraphQLResponseDatais called.The fields don't need cleanup in
Free()because they're lifecycle-managed by their respective owners (Resolver forsf, arena pool forjsonArena), not by the Loader.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (3)
163-165: Avoid mutating/capturing caller's headers; clone before use.Direct assignment
request.Header = baseHeadersaliases the map, allowing subsequentAdd/Setcalls to mutate the caller's map, which can cause race conditions across concurrent requests.Apply this diff:
if baseHeaders != nil { - request.Header = baseHeaders + request.Header = baseHeaders.Clone() }
219-223: Set Request.ContentLength field instead of the header.Manually setting the
Content-Lengthheader is ignored or overwritten by net/http. Therequest.ContentLengthfield should be set instead.Apply this diff:
if contentLength > 0 { - // always set the Content-Length Header so that chunking can be avoided - // and other parties can more efficiently parse - request.Header.Set(ContentLengthHeader, fmt.Sprintf("%d", contentLength)) + request.ContentLength = int64(contentLength) }Note: Remove the header manipulation; net/http will populate the header from the field. For streaming bodies with unknown length, leave
ContentLengthat its default (-1).
314-319: Check os.Open error before appending; prevent nil deref in cleanup.
temporaryFileis appended totempFilesbefore checking the error fromos.Open. If the open fails,tempFileswill contain a nil entry, causing the defer cleanup at lines 329-339 to panic when attempting to close or remove the file.Apply this diff:
temporaryFile, err := os.Open(file.Path()) -tempFiles = append(tempFiles, temporaryFile) if err != nil { return nil, err } +tempFiles = append(tempFiles, temporaryFile) formValues[key] = bufio.NewReader(temporaryFile)v2/pkg/engine/resolve/loader.go (1)
20-20: Fix import ordering per static analysis.The
gcilinter indicates this file is not properly formatted. Ensure imports are grouped correctly (standard library, external, internal).Run the following to fix:
#!/bin/bash # Format the file with gci gci write --skip-generated -s standard -s default -s "prefix(github.com/wundergraph/graphql-go-tools)" v2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/resolvable.go (1)
14-14: Fix import ordering per static analysis.The
gcilinter indicates this file is not properly formatted. Ensure imports are grouped correctly (standard library, external, internal).Run the following to fix:
#!/bin/bash # Format the file with gci gci write --skip-generated -s standard -s default -s "prefix(github.com/wundergraph/graphql-go-tools)" v2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolve.go (1)
15-15: Fix import ordering per static analysis.The
gcilinter indicates this file is not properly formatted. Ensure imports are grouped correctly (standard library, external, internal).Run the following to fix:
#!/bin/bash # Format the file with gci gci write --skip-generated -s standard -s default -s "prefix(github.com/wundergraph/graphql-go-tools)" v2/pkg/engine/resolve/resolve.go
🧹 Nitpick comments (3)
v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go (1)
2440-2440: LGTM - discarding return values is acceptable here.Explicitly discarding both return values from
fmt.Fprintfis a common pattern in test mock handlers. Since this test focuses on WebSocket upgrade failure handling rather than the mock server's write success, this is acceptable.If you want to be more defensive, consider checking the error:
- _, _ = fmt.Fprintf(w, `{"error": "WebSocket upgrade failed", "status": %d}`, tc.statusCode) + _, err := fmt.Fprintf(w, `{"error": "WebSocket upgrade failed", "status": %d}`, tc.statusCode) + require.NoError(t, err)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
329-339: Consider completing cleanup even on error.The early
returnonClose()orRemove()errors prevents cleanup of remaining files, potentially leaking file descriptors. While minor compared to the nil entry issue at lines 314-319, consider logging errors and continuing the cleanup loop.Example approach:
defer func() { multipartBody.Close() for _, file := range tempFiles { - if err := file.Close(); err != nil { - return - } - if err = os.Remove(file.Name()); err != nil { - return - } + if closeErr := file.Close(); closeErr != nil { + // log closeErr but continue cleanup + } + if removeErr := os.Remove(file.Name()); removeErr != nil { + // log removeErr but continue cleanup + } } }()v2/pkg/engine/resolve/loader.go (1)
1636-1686: Consider defensive copy when caching single-flight responses.Line 1684 stores
res.outdirectly into the shared cache without copying. WhilemergeResult(lines 556-558) makes a defensive copy before parsing, the cacheditem.responseslice could be problematic if:
- The
DataSource.Load()implementation reuses the backing array across calls, or- Concurrent readers access the cached response while the backing array is modified elsewhere.
A defensive copy at line 1684 would eliminate this risk:
- item.response = res.out + item.response = append([]byte(nil), res.out...)This aligns with the defensive copy pattern already used at line 547-548 (in earlier code) and lines 556-558.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go(1 hunks)v2/pkg/engine/datasource/httpclient/nethttpclient.go(9 hunks)v2/pkg/engine/resolve/context.go(2 hunks)v2/pkg/engine/resolve/loader.go(50 hunks)v2/pkg/engine/resolve/resolvable.go(14 hunks)v2/pkg/engine/resolve/resolve.go(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/context.go
🧰 Additional context used
🧬 Code graph analysis (4)
v2/pkg/engine/resolve/resolve.go (7)
v2/pkg/engine/resolve/arena.go (2)
ArenaPool(13-17)NewArenaPool(30-34)v2/pkg/engine/resolve/singleflight.go (2)
SingleFlight(16-22)NewSingleFlight(29-41)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/context.go (4)
Context(16-37)Request(170-173)ExecutionOptions(57-63)SubgraphHeadersBuilder(43-47)v2/pkg/engine/resolve/response.go (3)
GraphQLResponse(35-43)GraphQLSubscription(12-16)SubscriptionResponseWriter(68-74)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/resolve/loader.go (5)
v2/pkg/engine/resolve/singleflight.go (1)
SingleFlight(16-22)v2/pkg/engine/resolve/fetch.go (5)
FetchItemPathElement(78-82)FetchItem(29-34)DataSourceLoadTrace(405-419)Fetch(20-27)FetchInfo(376-397)v2/pkg/engine/resolve/context.go (3)
RateLimitOptions(129-142)Context(16-37)ExecutionOptions(57-63)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/resolve/resolvable.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(187-251)v2/pkg/fastjsonext/fastjsonext.go (2)
AppendErrorToArray(8-15)AppendErrorWithExtensionsCodeToArray(17-27)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/resolve.go
[failure] 15-15:
File is not properly formatted (gci)
v2/pkg/engine/resolve/loader.go
[failure] 20-20:
File is not properly formatted (gci)
v2/pkg/engine/resolve/resolvable.go
[failure] 14-14:
File is not properly formatted (gci)
🔇 Additional comments (19)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (6)
30-30: LGTM: constant addition.The
ContentLengthHeaderconstant is properly defined and aligns with other header constants in the file.
133-154: LGTM: well-designed size hint mechanism.The context-based size hint with a custom key type prevents collisions, and the buffer sizing logic balances memory efficiency with allocation overhead.
240-250: LGTM: thoughtful buffer lifecycle management.The comment explaining the GC-based approach over pooling is valuable documentation. The buffer creation leverages the size hint mechanism to reduce allocations efficiently.
252-278: LGTM: clean trace extension handling.The conditional trace embedding is well-structured, and error propagation follows the consistent
(nil, err)pattern throughout.
281-284: LGTM: signature change aligns with PR objectives.The updated signature to return
[]bytedirectly and acceptbaseHeadersmatches the PR's goal of improving memory management and moving request deduplication into the engine.
341-341: LGTM: appropriate contentLength for multipart.Passing
contentLength=0for the multipart body is correct since the body is a pipe reader with unknown length at request creation time.v2/pkg/engine/resolve/loader.go (5)
186-196: Arena and single-flight integration looks good.The documentation clearly explains the arena's thread-safety constraints and lifecycle. The warning about tying parsed JSON's underlying bytes to the arena lifecycle is crucial for avoiding segfaults.
369-383: Arena-aware helper functions are well-structured.The consistent pattern of accepting
arena.Arenaas the first parameter and using arena-backed allocations throughoutselectItemsForPath,selectItems, anditemsDataensures memory is managed correctly.Also applies to: 401-439, 441-455
556-558: Defensive copy before parsing is essential.This pattern correctly ties the lifecycle of the parsed JSON to the arena-allocated slice, preventing segfaults as noted in the comments at lines 190-193.
752-831: Arena-aware error handling is consistently implemented.The lazy initialization via
l.resolvable.ensureErrorsInitialized()(lines 788, 828, etc.) is a good optimization, and arena-backed object creation throughout error rendering paths is correct.Also applies to: 873-1044
1606-1624: New context helper looks good.
GetOperationTypeFromContextprovides a clean way to access the operation type from context, useful for the transport layer to make decisions (e.g., disabling single-flight for mutations).v2/pkg/engine/resolve/resolvable.go (3)
72-80: Constructor signature updated for arena support.Accepting
arena.Arenaas a parameter is correct. Callers must ensure they provide a valid arena before calling methods that use it (likeInit), or rely on the newArenaResolveGraphQLResponsepath in resolve.go.
240-245: Lazy error initialization is an excellent optimization.This pattern avoids allocating the errors array upfront, which can significantly reduce memory usage when most operations complete without errors. The consistent use of
ensureErrorsInitialized()throughout the file (lines 773, 1215, 1286, 1292, 1298) ensures correctness.
110-128: Arena integration in Init methods looks correct.Both
InitandInitSubscriptionproperly use arena-backed object creation and parsing. The explicit comments about not initializing errors (lines 115-116, 134-135) helpfully document the lazy initialization strategy.Also applies to: 130-166
v2/pkg/engine/resolve/resolve.go (5)
75-85: Arena pools and single-flight initialization look excellent.The separation of
resolveArenaPoolandresponseBufferPoolis well-reasoned (as explained in the comments), and both pools along with the single-flight cache are properly initialized.Also applies to: 239-241
313-356: ArenaResolveGraphQLResponse has excellent arena lifecycle management.This new method demonstrates proper arena handling:
- Acquires arenas before use (lines 325, 343)
- Sets arenas on both loader and resolvable (lines 326-327)
- Releases arenas on all error paths (lines 331, 338, 347-349)
- Releases arenas after successful completion (lines 352, 354)
The use of arena-backed buffers (line 344) for response writing is also correct.
488-530: Subscription update arena handling is correct.The pattern of acquiring the arena (line 490), setting it on tools (lines 491-492), and releasing on all exit paths (lines 495, 507, 519, 530) is consistent and safe.
1079-1084: Header propagation helper is clean and straightforward.The
triggerHeadersmethod correctly delegates toSubgraphHeadersBuilderwhen available and provides sensible defaults.
253-275: Clarify the misleading "we set the arena manually" comment and verify nil arena handling in ResolveGraphQLResponse.The code shows both ResolveGraphQLResponse and ArenaResolveGraphQLResponse exist in parallel, but the comment at line 255 is inaccurate. ResolveGraphQLResponse never sets the arena before calling Init (line 293)—only ArenaResolveGraphQLResponse does. The data parameter difference between them (ResolveGraphQLResponse passes data bytes, ArenaResolveGraphQLResponse passes nil) may influence arena behavior, but without examining the astjson library's nil-arena handling, it's unclear if this is an intentional design or a latent issue.
Either:
- Update the comment to clarify that newTools intentionally creates Resolvable with nil arena for the non-arena code path, and verify tests cover this path
- Or verify that astjson functions (ObjectValue, ParseBytesWithArena, MergeValues) safely handle nil arenas
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
314-318: Critical: os.Open error still checked after append - can cause nil dereference.The past review comment on this issue was not addressed. When
os.Openfails,temporaryFileis nil but still appended totempFiles. The defer cleanup at line 332 will then panic when callingfile.Close()on the nil entry.Apply this diff to fix:
temporaryFile, err := os.Open(file.Path()) - tempFiles = append(tempFiles, temporaryFile) if err != nil { return nil, err } + tempFiles = append(tempFiles, temporaryFile)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/pkg/engine/datasource/httpclient/nethttpclient.go(9 hunks)v2/pkg/engine/resolve/inputtemplate.go(8 hunks)v2/pkg/engine/resolve/loader.go(50 hunks)v2/pkg/engine/resolve/loader_test.go(14 hunks)v2/pkg/engine/resolve/resolvable.go(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
v2/pkg/engine/resolve/resolvable.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(187-251)v2/pkg/fastjsonext/fastjsonext.go (2)
AppendErrorToArray(8-15)AppendErrorWithExtensionsCodeToArray(17-27)
v2/pkg/engine/resolve/inputtemplate.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/lexer/literal/literal.go (1)
COMMA(23-23)
v2/pkg/engine/resolve/loader_test.go (2)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/loader.go (1)
Loader(159-196)
v2/pkg/engine/resolve/loader.go (5)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
ResponseContext(72-76)WithHTTPClientSizeHint(142-144)v2/pkg/engine/resolve/fetch.go (11)
SingleFetch(91-99)SingleFetch(153-155)BatchEntityFetch(160-169)BatchEntityFetch(194-196)EntityFetch(200-209)EntityFetch(226-228)FetchItemPathElement(78-82)FetchItem(29-34)DataSourceLoadTrace(405-419)Fetch(20-27)FetchInfo(376-397)v2/pkg/engine/resolve/context.go (3)
RateLimitOptions(129-142)Context(16-37)ExecutionOptions(57-63)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
🪛 GitHub Actions: v2-ci
v2/pkg/engine/resolve/loader.go
[error] 23-23: golangci-lint: File is not properly formatted (gci).
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/resolvable.go
[failure] 16-16:
File is not properly formatted (gci)
v2/pkg/engine/resolve/loader.go
[failure] 23-23:
File is not properly formatted (gci)
🔇 Additional comments (12)
v2/pkg/engine/resolve/resolvable.go (3)
239-244: LGTM: Well-designed lazy initialization helper.The
ensureErrorsInitialized()helper cleanly implements the lazy initialization pattern for the errors array, supporting the memory optimization goal mentioned in the PR objectives. The consistent usage throughout the file (lines 772, 1214, 1285, 1291, 1297) ensures errors are only allocated when needed.
113-115: Effective memory optimization through lazy initialization.Setting
r.errors = nilinstead of eager initialization is a good optimization that defers allocation until errors actually occur. The inline comments clearly document this design decision, and theensureErrorsInitialized()helper ensures safe usage throughout the codebase.Also applies to: 133-134, 172-173
71-79: Intentional API change for arena integration.The constructor signature change from
NewResolvable(context.Context, ResolvableOptions)toNewResolvable(arena.Arena, ResolvableOptions)is a breaking change that aligns with the PR's arena-based memory management objectives. All test call sites have been correctly updated to passnilor an appropriate arena instance.v2/pkg/engine/resolve/loader_test.go (1)
290-290: Test updates correctly reflect API changes.All test instantiations of
NewResolvablehave been properly updated to passnilas the first arena parameter, consistent with the new constructor signature. This approach is appropriate for tests that don't require specific arena lifecycle management.Also applies to: 379-379, 470-470, 752-752, 1027-1027, 1128-1128, 1424-1424
v2/pkg/engine/resolve/inputtemplate.go (2)
58-63: Well-designed abstraction with InputTemplateWriter interface.The new
InputTemplateWriterinterface cleanly abstracts buffer operations (io.Writer,io.StringWriter,Reset(),Bytes()) and enables controlled reuse across the template rendering pipeline. This design improves testability and flexibility.
161-174: Error handling improvement for write operations.Previously, write errors in
renderHeaderVariablewere silently ignored using_, _ = preparedInput.Write(...). The updated code now properly checks and propagates these errors (lines 161-163, 168-174), which improves robustness and makes potential issues visible to callers.v2/pkg/engine/resolve/loader.go (3)
185-195: Excellent documentation of arena thread-safety constraints.The detailed comments on
jsonArenaclearly explain critical constraints:
- Not thread-safe (use only from main goroutine)
- Lifecycle managed by Resolver (don't Reset or Release)
- Critical requirement: underlying bytes must be arena-allocated to tie lifecycles
This documentation will help prevent common arena misuse patterns that lead to segfaults.
552-557: Critical arena lifecycle management pattern.Lines 555-557 implement a crucial pattern: allocating a new slice on the arena and copying
res.outbefore parsing withParseBytesWithArena. This ensures the parsed AST and the underlying bytes share the same arena lifecycle, preventing segfaults when the arena is released. As noted in the arena comments (line 191), this pattern is essential for correct arena usage.
1635-1685: SingleFlight integration properly handles mutations and context.The
loadByContextfunction correctly:
- Disables deduplication for mutations (line 1644) to avoid side-effect issues
- Respects the
DisableRequestDeduplicationflag (line 1645)- Handles context cancellation via select with
item.loadedandctx.Done()(lines 1657-1661)- Provides size hints to the HTTP client (line 1672)
- Properly stores shared responses (line 1667, 1683)
The integration aligns with the PR's request deduplication objectives.
v2/pkg/engine/datasource/httpclient/nethttpclient.go (3)
133-154: LGTM: Buffer size hint mechanism is well-designed.The context-based size hint and the
bufferfunction provide an efficient way to pre-allocate buffers based on expected response sizes, reducing allocations and copies. The 64-byte default for cases without a hint is a reasonable middle ground.
240-278: LGTM: Response buffering and trace handling are well-implemented.The buffer allocation strategy is clearly documented, and the trace injection using
jsonparser.Setis correct. Error handling is consistent throughout.
281-283: LGTM: Do function correctly updated to new signature.The function now properly accepts
baseHeadersand returns data directly, aligning with the PR objectives. Passinglen(body)ascontentLengthis correct for regular HTTP requests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
222-223: Fix JSON envelope: use"body"instead of"variables"This test is inconsistent with all other tests in the file. It should use
"body"to wrap variables, not a top-level"variables"key:Apply this diff:
- _, err = ds.Load(context.Background(), nil, []byte(`{"query":"`+query+`","variables":`+variables+`}`)) + _, err = ds.Load(context.Background(), nil, []byte(`{"query":"`+query+`","body":`+variables+`}`))This issue was flagged in previous reviews but the fix appears incomplete.
v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
36-44: Data race onreq.Errremains unfixed.Multiple goroutines read/write
InflightRequest.Errwithout synchronization:
- Follower reads at line 91
- Follower writes at line 96 (also incorrect: shouldn't modify shared state on local context cancellation)
- Leader writes at line 127
Additionally, line 96 has a logic error: when a follower's context is cancelled, it should return its own error without mutating the shared
request.Err, which belongs to the leader and other followers.Protect
Errwith the existingMu:type InflightRequest struct { Done chan struct{} Data []byte - Err error ID uint64 HasFollowers bool - Mu sync.Mutex + Mu sync.Mutex // protects Err and HasFollowers + Err error }Then at line 91-97, read Err under lock:
select { case <-request.Done: + request.Mu.Lock() + err := request.Err + request.Mu.Unlock() - if request.Err != nil { - return nil, request.Err + if err != nil { + return nil, err } return request, nil case <-ctx.ctx.Done(): - request.Err = ctx.ctx.Err() - return nil, request.Err + return nil, ctx.ctx.Err() }And at line 127, write Err under lock:
func (r *InboundRequestSingleFlight) FinishErr(req *InflightRequest, err error) { if req == nil { return } shard := r.shardFor(req.ID) shard.m.Delete(req.ID) + req.Mu.Lock() req.Err = err + req.Mu.Unlock() close(req.Done) }
104-119: TOCTOU race between follower attachment and FinishOk remains unfixed.The race window persists:
- Follower obtains
requestfromLoadOrStoreat line 83 (before leader's Delete)- Leader executes line 109 (Delete), reads
HasFollowers == falseat lines 110-112, skips copy, closesDoneat line 118- Follower then sets
HasFollowers = trueat line 87 but it's too late—Done is closed- Follower wakes at line 90 with
request.Data == nildespiterequest.Err == nilThis breaks the contract that deduplicated requests with
Err == nilhave validData.Remove the
HasFollowersoptimization and always copy:type InflightRequest struct { Done chan struct{} Data []byte - Err error ID uint64 - HasFollowers bool - Mu sync.Mutex + Mu sync.Mutex // protects Err + Err error }func (r *InboundRequestSingleFlight) FinishOk(req *InflightRequest, data []byte) { if req == nil { return } shard := r.shardFor(req.ID) shard.m.Delete(req.ID) - req.Mu.Lock() - hasFollowers := req.HasFollowers - req.Mu.Unlock() - if hasFollowers { - // optimization to only copy when we actually have to + // Always copy to ensure followers see valid immutable data + if data != nil { req.Data = make([]byte, len(data)) copy(req.Data, data) } close(req.Done) }And remove the HasFollowers logic from GetOrCreate:
inflight, shared := shard.m.LoadOrStore(key, request) if shared { request = inflight.(*InflightRequest) - request.Mu.Lock() - request.HasFollowers = true - request.Mu.Unlock() select { case <-request.Done:
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
309-309: Remove or comment out debug print statementThis
fmt.Printlnstatement should be commented out or removed to avoid cluttering test output.Apply this diff:
- fmt.Println(string(bytes)) + // fmt.Println(string(bytes))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(29 hunks)v2/pkg/engine/resolve/inbound_request_singleflight.go(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/resolve/inbound_request_singleflight.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/resolve/inbound_request_singleflight.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/resolve/inbound_request_singleflight.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/resolve/inbound_request_singleflight.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.go
📚 Learning: 2025-09-19T14:51:33.724Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:339-347
Timestamp: 2025-09-19T14:51:33.724Z
Learning: In the graphql-go-tools resolver, the event loop follows a strict single-threaded design where all subscription lifecycle management (add/remove from triggers) must happen through the events channel to the main processEvents() goroutine. Worker goroutines should not directly modify subscription state or call unsubscribe methods, as this would break the single-threaded event loop architecture and create race conditions.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.go
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
v2/pkg/engine/resolve/context.go (4)
Context(16-38)ExecutionOptions(60-78)Request(185-188)SubgraphHeadersBuilder(44-50)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
⏰ 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). (3)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (2)
v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
10-34: LGTM: Clean sharded design with sensible defaults.The sharded architecture with sync.Map per shard and the constructor's handling of invalid shard counts are both sound.
62-74: LGTM: Robust key derivation with collision-free hashing.The 24-byte composite buffer hashed with xxhash eliminates collision risks from the earlier addition-based approach.
# Conflicts: # v2/pkg/engine/resolve/context.go # v2/pkg/engine/resolve/loader.go # v2/pkg/engine/resolve/loader_hooks_test.go # v2/pkg/engine/resolve/loader_test.go # v2/pkg/engine/resolve/resolve_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
v2/pkg/engine/resolve/resolve_federation_test.go (1)
1913-1915: Fix invalid JSON: escape inner quotes inline3(test: "BOOM")stringsThe three JSON blobs embedding
line3(test: "BOOM")are still invalid JSON because the quotes aroundBOOMare unescaped inside a JSON string literal contained in a Go raw string. They must be\"BOOM\"so the resulting JSON is valid.Apply this diff to all three occurrences (datasource expectation,
FetchConfiguration.Input, andInputTemplatesegment):- `{"method":"POST","url":"http://address.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){__typename ... on Address {line3(test: "BOOM") zip}}}","variables":{"representations":[{"__typename":"Address","id":"address-1","country":"country-1","city":"city-1"}]}}}`, + `{"method":"POST","url":"http://address.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){__typename ... on Address {line3(test: \"BOOM\") zip}}}","variables":{"representations":[{"__typename":"Address","id":"address-1","country":"country-1","city":"city-1"}]}}}`, - Input: `{"method":"POST","url":"http://address.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){__typename ... on Address {line3(test: "BOOM") zip}}}","variables":{"representations":$$0$$}}}`, + Input: `{"method":"POST","url":"http://address.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){__typename ... on Address {line3(test: \"BOOM\") zip}}}","variables":{"representations":$$0$$}}}`, - Data: []byte(`{"method":"POST","url":"http://address.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){__typename ... on Address {line3(test: "BOOM") zip}}}","variables":{"representations":[`), + Data: []byte(`{"method":"POST","url":"http://address.service","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){__typename ... on Address {line3(test: \"BOOM\") zip}}}","variables":{"representations":[`),Also applies to: 1984-1985, 1995-1996
v2/pkg/engine/resolve/resolve_test.go (2)
431-436: UseDoAndReturninstead ofDo(...).Return(...)on gomock expectationHere
Do’s return values are ignored by gomock and you also chainReturn, which is confusing. Prefer a singleDoAndReturnwith the full(context.Context, http.Header, []byte) ([]byte, error)signature so the callback both performs side effects and supplies the mock return value:- mockDataSource.EXPECT(). - Load(gomock.Any(), gomock.Any(), []byte(`{"id":1}`)). - Do(func(ctx context.Context, headers http.Header, input []byte) ([]byte, error) { - return []byte(`{"name":"Jens"}`), nil - }). - Return([]byte(`{"name":"Jens"}`), nil) + mockDataSource.EXPECT(). + Load(gomock.Any(), gomock.Any(), []byte(`{"id":1}`)). + DoAndReturn(func(ctx context.Context, headers http.Header, input []byte) ([]byte, error) { + return []byte(`{"name":"Jens"}`), nil + })
4534-4650: Arena request-dedup test logic and helpers look solidThe
blockingDataSource/blockingWriterhelpers andTestResolver_ArenaResolveGraphQLResponse_RequestDeduplicationexercise the single-flight/dedup path in a realistic way: one leader request drives the actual datasource call while followers share the result and are marked withResolveDeduplicated=true. Channel usage (Ready,Release, follower coordination) and the result slice updates per index are concurrency-safe, and the assertions check both correctness and deduplication flags.Aside from the
Contextconstruction issue noted above, the overall structure of this test is robust.
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/resolve_test.go (1)
7119-7425: Arena benchmark reuses pooled Contexts afterFree; safe today but fragile if error paths start usingsubgraphErrorsIn
Benchmark_NestedBatchingArenayou pool*Contextinstances viaNewContext, then in the hot loop you:
- grab
ctx := ctxPool.Get().(*Context)- mutate
ctx.ctx = context.Background()- call
resolver.ArenaResolveGraphQLResponse(ctx, plan, buf)ctx.Free()and put it back.Given
FreesetssubgraphErrors = nil, this reuse pattern is fine as long as the benchmark plan never triggers error paths that callappendSubgraphErrors. If you later extend the benchmark to include failing subgraph calls, this could hit the same nil-map panic seen in tests unless you reinitialize viaNewContextor explicitly recreatesubgraphErrorsbefore reuse.Not a blocker now, but consider either (a) documenting that pooled contexts must not be reused across erroring runs, or (b) reinitializing the error map before reuse if you ever add negative-path coverage here.
v2/pkg/engine/resolve/loader.go (1)
1422-1519: Sophisticated deduplication and memory management.The implementation correctly:
- Uses pooled tools with dedicated arena for temporary allocations (lines 1422-1425)
- Implements hash-based deduplication to reduce batch size (lines 1469-1489)
- Copies batchStats pointers off the tools arena before return (lines 1513-1519)
- Defensively clears local batchStats in defer after copy completes (lines 1426-1435)
The comment on line 1514 says "copy the *astjson.Value's off the arena" but the code copies the pointers (which is correct since the Values themselves are on l.jsonArena and will remain valid). Consider clarifying: "copy the *astjson.Value pointers off the tools arena".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
v2/pkg/engine/resolve/authorization_test.go(3 hunks)v2/pkg/engine/resolve/context.go(3 hunks)v2/pkg/engine/resolve/loader.go(48 hunks)v2/pkg/engine/resolve/loader_hooks_test.go(13 hunks)v2/pkg/engine/resolve/loader_test.go(15 hunks)v2/pkg/engine/resolve/resolvable.go(14 hunks)v2/pkg/engine/resolve/resolve_federation_test.go(21 hunks)v2/pkg/engine/resolve/resolve_test.go(31 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/loader_hooks_test.gov2/pkg/engine/resolve/context.gov2/pkg/engine/resolve/resolve_federation_test.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/authorization_test.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/resolvable.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/resolve/loader_hooks_test.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/resolve/loader_hooks_test.gov2/pkg/engine/resolve/resolve_federation_test.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/resolve/loader_hooks_test.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolvable.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/resolve/loader_hooks_test.gov2/pkg/engine/resolve/resolve_federation_test.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/resolve/resolve_federation_test.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-10-16T08:52:33.278Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go:59-63
Timestamp: 2025-10-16T08:52:33.278Z
Learning: Go 1.24 (released February 2025) introduced the testing.B.Loop() method for benchmarks. Use `for b.Loop() { /* code */ }` instead of `for i := 0; i < b.N; i++` in benchmark functions when the Go version is 1.24 or higher. The Loop() method provides more predictable benchmarking results by running the loop exactly once per -count and preventing certain compiler optimizations.
Applied to files:
v2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/resolvable.go
📚 Learning: 2025-11-17T12:47:08.376Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/grpctest/mockservice_resolve.go:721-734
Timestamp: 2025-11-17T12:47:08.376Z
Learning: In Go, protobuf-generated getter methods (GetX()) are nil-safe: they check if the receiver is nil and return the field's default value (nil for message fields, zero values for primitives). This means chained calls like `req.GetFieldArgs().GetThreshold().GetValue()` are safe even when intermediate values are nil. Setters and clearers, however, are NOT nil-safe and will panic on nil receivers.
Applied to files:
v2/pkg/engine/resolve/resolvable.go
🧬 Code graph analysis (7)
v2/pkg/engine/resolve/loader_hooks_test.go (1)
v2/pkg/engine/resolve/context.go (1)
Context(18-40)
v2/pkg/engine/resolve/context.go (1)
v2/pkg/engine/plan/planner.go (1)
IncludeQueryPlanInResponse(92-96)
v2/pkg/engine/resolve/resolve_federation_test.go (3)
v2/pkg/engine/resolve/context.go (1)
Context(18-40)v2/pkg/engine/resolve/buf_pair.go (1)
NewBufPair(10-15)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)
v2/pkg/engine/resolve/resolve_test.go (1)
v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)
v2/pkg/engine/resolve/authorization_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(18-40)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)
v2/pkg/engine/resolve/loader.go (3)
v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
WithHTTPClientSizeHint(142-144)
v2/pkg/engine/resolve/resolvable.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(191-256)v2/pkg/fastjsonext/fastjsonext.go (2)
AppendErrorToArray(8-15)AppendErrorWithExtensionsCodeToArray(17-27)
🪛 GitHub Actions: v2-ci
v2/pkg/engine/resolve/context.go
[error] 199-199: Test failed with runtime panic: 'assignment to entry in nil map' during TestResolver_ArenaResolveGraphQLResponse (error_handling).
⏰ 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). (2)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (20)
v2/pkg/engine/resolve/resolve_federation_test.go (1)
19-27: Mocked datasource helper correctly updated to newLoadcontractUsing
Load(gomock.Any(), gomock.Any(), gomock.Any())with a(context.Context, http.Header, []byte) ([]byte, error)stub and asserting the input string keeps the tests aligned with the new headers-aware, byte-slice-returning datasource API. The helper centralizes this pattern nicely.v2/pkg/engine/resolve/resolvable.go (2)
27-37: Arena wiring inResolvablelooks coherent; watch Reset semantics with callers
Resolvablenow owns anastjsonArenasupplied viaNewResolvable(a, options)and cleared inReset(). All JSON allocations/parsing in this file consistently use that arena. This is fine as long as callers either construct a freshResolvableper arena or reassign a new arena before reusing an instance afterReset().Also applies to: 72-80, 82-108
240-245: Lazy initialization ofr.errorsand value-completion arrays is correct and reduces overhead
ensureErrorsInitializedplus the updatedaddNonNullableFieldError,addRejectFieldError, andaddError*helpers correctly avoid allocating the errors array until the first error is actually added, while still satisfyingfastjsonext.Append*ToArray’s expectation of an array-typed value. The same pattern forvalueCompletionviaastjson.ArrayValue(r.astjsonArena)is consistent.Also applies to: 760-776, 1215-1217, 1284-1300, 1303-1316
v2/pkg/engine/resolve/authorization_test.go (1)
539-569: Authorization mocks correctly adapted to header-aware, byte-returningLoadThe
userService,reviewsService, andproductServicemocks now match the newLoad(ctx, headers, input) ([]byte, error)signature, still assert the full request JSON, and return GraphQL-style{"data": ...}payloads that align with the configuredSelectResponseDataPath/SelectResponseErrorsPath. This keeps the authorization tests focused on coordinates and subgraph error handling without changing semantics.Also applies to: 845-873
v2/pkg/engine/resolve/loader_hooks_test.go (1)
50-56: Loader hook tests correctly updated to newLoadAPI and error-return patternAll the mocked datasources now implement
Load(ctx, headers, input) ([]byte, error)and directly return JSON payloads witherrorsarrays. Given thePostProcessing.SelectResponseErrorsPath: []string{"errors"}, this keeps the LoaderHooks behavior and subgraph error propagation tests intact while matching the new loader interface.Also applies to: 121-126, 184-190, 245-251, 306-312, 341-347, 376-382, 411-417, 446-452, 481-487, 516-522, 551-557
v2/pkg/engine/resolve/loader_test.go (2)
287-299: Tests and benchmark now exerciseNewResolvable(nil, ResolvableOptions{})consistentlyUpdating all loader tests and the benchmark to construct
ResolvableviaNewResolvable(nil, ResolvableOptions{})keeps them aligned with the new constructor while still validating merge logic, batch behavior, and extension handling. The repeatedInit→LoadGraphQLResponseData→PrintGraphQLResponsepattern confirms the arena-aware resolvable integrates correctly with the loader in both test and benchmark scenarios.Also applies to: 374-382, 463-471, 742-755, 1016-1038, 1409-1422
1425-1517: Error-path rewrite tests correctly deep-copy values before mutationIn
TestRewriteErrorPaths, cloning input errors viaMarshalTo+ parse into new*astjson.Valueinstances before callingrewriteErrorPathsavoids mutating the originals and keeps expected vs. actual comparisons precise. Passing the same arena parameter (nilhere) through both parse and rewrite ensures the code under test sees a realistic value shape without leaking implementation details into the tests.v2/pkg/engine/resolve/context.go (1)
21-52: New context metadata and dedup controls look consistentThe additions of
VariablesHash,SubgraphHeadersBuilder(plusHeadersForSubgraphRequest), the new execution flags, andRequest.IDasuint64are all wired in cleanly and respect existing patterns (e.g., no defensive nil checks aroundsubgraphErrors, clone via struct copy, simple nil-safe delegation inHeadersForSubgraphRequest). I don’t see any correctness or API-shape issues in this file.Also applies to: 62-80, 202-205
v2/pkg/engine/resolve/resolve_test.go (1)
5213-5240: Updated_fakeStream.Startsignature correctly ignores headersThe
_fakeStream.Startimplementation has been adjusted to the new signature(ctx *Context, headers http.Header, input []byte, updater SubscriptionUpdater)while preserving its previous behavior (respectingctx.ctx.Done(), invokingonStart, and drivingmessageFuncwith proper completion and delay). Ignoring theheadersargument here is appropriate for these tests and keeps the mock aligned with the production interface.v2/pkg/engine/resolve/loader.go (11)
175-188: Excellent arena lifecycle documentation.The comments clearly document the arena's thread-safety constraints and the critical requirement to allocate bytes on the same arena before parsing. This guidance is properly followed throughout the implementation (e.g., lines 492-497).
226-231: Proper pool resource management.The defer correctly ensures batchEntityTools are returned to the pool after resolveParallel completes, with the Put method safely handling nil (lines 1397-1400).
364-402: Clean arena integration in selectItems.The function correctly uses arena-backed allocations for the selected slice and arena.SliceAppend for growth, properly tying memory lifecycle to the arena.
404-418: Concurrency-safe approach for temporary array construction.The function correctly avoids arena allocation due to concurrent calls (as the comment explains). Using
nilarena in SetArrayItem is appropriate here since the array is temporary and only used as rendering input, not stored long-term.
492-497: Reference implementation of arena parsing pattern.This segment exemplifies the correct approach: allocate bytes on the arena, copy the data, then parse with ParseBytesWithArena. This pattern correctly ties the lifecycle of the parsed JSON to the underlying bytes, preventing segfaults (as warned in lines 175-183).
1367-1407: Well-designed pooling pattern for batch entity processing.The batchEntityTools struct properly encapsulates temporary resources (hash generator, deduplication map, and arena). The pool implementation correctly initializes resources, resets state before returning to the pool, and safely handles nil.
1592-1609: Clean context helper for operation type detection.GetOperationTypeFromContext is properly exported with clear documentation and defensive nil handling, returning a sensible default (Query) when the context is nil or the value is missing.
1622-1644: Clear single-flight eligibility logic with good rationale.The function correctly limits deduplication to Query operations (which are idempotent) while disallowing it for Mutations. The comment clearly explains the GraphQL Federation scenario where sub-queries within Mutations can still be deduplicated.
1646-1694: Solid single-flight implementation with proper synchronization.The leader/follower pattern is correctly implemented:
- Leader performs the load and signals completion via
Finish(line 1683)- Followers wait on
item.loadedchannel with context cancellation support (lines 1666-1670)- Operation type and headers are properly propagated through context (lines 1648-1652)
- Single-flight eligibility check prevents deduplication of mutations (line 1654)
1887-1906: Appropriate arena avoidance with clear justification.The comments correctly explain why arena is not used here: the compacted result may outlive the arena scope (used for tracing output), and this is not a performance-critical path. Using non-arena parsing is the right choice in this context.
721-724: Consistent lazy initialization pattern for error arrays.The repeated comments make clear this is an intentional, measured performance optimization. The pattern is consistently applied: always call
ensureErrorsInitialized()before appending. The strong "don't change this, it's measurable" directive indicates this has been benchmarked and validated.Also applies to: 761-764, 1011-1014, 1028-1031, 1043-1046, 1065-1068, 1093-1096, 1175-1178
There was a problem hiding this comment.
Actionable comments posted: 1
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/resolve_test.go (1)
4949-4954: UseNewContext(...)instead ofContext{...}literals in new tests.These tests construct
Contextdirectly:ctx := &Context{ ctx: context.Background(), Request: Request{ Header: header }, }and
ctx := &Context{ ctx: context.Background(), Variables: tc.variables, RemapVariables: tc.remap, }The resolver code assumes all
Contextinstances are created viaNewContext(), which initializes internal fields likesubgraphErrorsand other maps. BypassingNewContextrisks panics if these tests start exercising error paths or subgraph error propagation in the future.Safer pattern (then set the needed fields):
- ctx := &Context{ - ctx: context.Background(), - Request: Request{ - Header: header, - }, - } + ctx := NewContext(context.Background()) + ctx.Request.Header = headerand
- ctx := &Context{ - ctx: context.Background(), - Variables: tc.variables, - RemapVariables: tc.remap, - } + ctx := NewContext(context.Background()) + ctx.Variables = tc.variables + ctx.RemapVariables = tc.remapThis keeps the tests aligned with the established invariant for
Context.
Based on learnings, allContextinstances in this package should come fromNewContext().Also applies to: 5021-5025
♻️ Duplicate comments (3)
v2/pkg/engine/resolve/resolve_test.go (1)
429-435: PreferDoAndReturnoverDo(...).Return(...)for gomock expectations.Here the mock both calls
Dowith a function that returns([]byte, error)and then chainsReturnwith the same values:mockDataSource.EXPECT(). Load(gomock.Any(), gomock.Any(), []byte(`{"id":1}`)). Do(func(ctx context.Context, headers http.Header, input []byte) ([]byte, error) { return []byte(`{"name":"Jens"}`), nil }). Return([]byte(`{"name":"Jens"}`), nil)gomock ignores the return values of
Do; onlyReturncontrols what the call returns. For clarity and to avoid surprises if the callback logic changes, it’s better to useDoAndReturnso the same function both runs side‑effects and provides the return values:- mockDataSource.EXPECT(). - Load(gomock.Any(), gomock.Any(), []byte(`{"id":1}`)). - Do(func(ctx context.Context, headers http.Header, input []byte) ([]byte, error) { - return []byte(`{"name":"Jens"}`), nil - }). - Return([]byte(`{"name":"Jens"}`), nil) + mockDataSource.EXPECT(). + Load(gomock.Any(), gomock.Any(), []byte(`{"id":1}`)). + DoAndReturn(func(ctx context.Context, headers http.Header, input []byte) ([]byte, error) { + return []byte(`{"name":"Jens"}`), nil + })In gomock, do return values from `Do` influence the mocked method's return values, or must `DoAndReturn` be used to both execute a callback and supply the mock's return values?v2/pkg/engine/resolve/inbound_request_singleflight.go (1)
38-46: Fix races and shared‑state bugs inInflightRequestfollower/finish protocol.Two intertwined issues here:
Shared
Errwritten from multiple goroutines without proper semantics.
- Followers on context cancellation do
request.Err = ctx.ctx.Err()(Line 98), while the leader writesreq.Err = errinFinishErr(Line 129), and other followers readrequest.Errafter<-request.Done(Lines 93–96).- This is a multi‑writer pattern with unsynchronized access and also lets a cancelled follower overwrite the leader’s eventual success: if the leader later calls
FinishOk,Errremains the cancellation error, so other followers will treat a successful request as failed.
HasFollowersoptimization is racy and can yield empty data to followers.
- A follower calls
LoadOrStoreand then setsHasFollowers = trueunderMu(Lines 88–90).- The leader in
FinishOkreadsHasFollowersunder the same mutex (Lines 112–114) to decide whether to copydataintoreq.Data(Lines 115–119).- There is a window where the leader can observe
HasFollowers == falseand skip the copy, then closeDone(Line 120), while a follower has already obtainedreqfromLoadOrStorebut hasn’t yet setHasFollowers. That follower wakes withErr == nilbutData == nil, violating the deduplication contract.Given this affects correctness for deduplicated requests and introduces data races, this should be fixed before release.
A simple, robust approach:
- Treat
Erras leader‑only state; followers onctxcancellation should not mutate it—just returnctx.ctx.Err().- Always copy
dataintoreq.Databefore closingDone; dropHasFollowersandMuentirely. The extra copy happens at most once per in‑flight key and is cheap compared to the request work.Suggested patch:
type InflightRequest struct { Done chan struct{} Data []byte - Err error - ID uint64 - - HasFollowers bool - Mu sync.Mutex + Err error + ID uint64 } @@ inflight, shared := shard.m.LoadOrStore(key, request) if shared { request = inflight.(*InflightRequest) - request.Mu.Lock() - request.HasFollowers = true - request.Mu.Unlock() select { case <-request.Done: if request.Err != nil { return nil, request.Err } return request, nil case <-ctx.ctx.Done(): - request.Err = ctx.ctx.Err() - return nil, request.Err + // Do not modify shared state on follower cancellation; this is per‑caller. + return nil, ctx.ctx.Err() } } @@ func (r *InboundRequestSingleFlight) FinishOk(req *InflightRequest, data []byte) { if req == nil { return } shard := r.shardFor(req.ID) shard.m.Delete(req.ID) - req.Mu.Lock() - hasFollowers := req.HasFollowers - req.Mu.Unlock() - if hasFollowers { - // optimization to only copy when we actually have to - req.Data = make([]byte, len(data)) - copy(req.Data, data) - } + // Always copy so any followers see a valid immutable slice. + if data != nil { + buf := make([]byte, len(data)) + copy(buf, data) + req.Data = buf + } close(req.Done) } @@ func (r *InboundRequestSingleFlight) FinishErr(req *InflightRequest, err error) { if req == nil { return } shard := r.shardFor(req.ID) shard.m.Delete(req.ID) req.Err = err close(req.Done) }You can also remove the now‑unused
HasFollowersandMuimports/fields. This aligns the in‑flight protocol with the expectations of the deduplication tests and avoids data races.Also applies to: 85-100, 106-131
v2/pkg/engine/resolve/resolve.go (1)
89-89: Clarify comment to match variable purpose.The comment states "de-duplicate subgraph requests" but
inboundRequestSingleFlightis used to de-duplicate inbound client requests, not subgraph requests.Apply this diff:
- // inboundRequestSingleFlight is used to de-duplicate subgraph requests + // inboundRequestSingleFlight is used to de-duplicate inbound client requestsBased on past review comments and the variable's actual usage in
ArenaResolveGraphQLResponse.
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/resolve_test.go (1)
4554-4662: Arena request‑deduplication test harness looks solid; consider slightly looser timeouts if CI gets slow.The new
TestResolver_ArenaResolveGraphQLResponse_RequestDeduplicationusesblockingDataSourceplusblockingWriterand actxTemplateto exercise the inbound single‑flight path with one leader and multiple followers. The sequencing (ds.Ready(), synchronizing follower start, thends.Release(), thenleaderWriter.Ready()/Release()) correctly forces followers to attach before the leader completes, and the assertions onResolveDeduplicatedand identical outputs validate the contract well.If you ever see flakes in slower environments, you might want to bump the
time.Secondtimeouts used while waiting forReady/followers to, say, 2–3 seconds, but functionally this test looks good.v2/pkg/engine/resolve/resolve.go (1)
78-85: Optional: Consider adding punctuation to multi-line comments for readability.As noted in previous reviews, adding proper punctuation (periods, commas) to multi-line comments improves readability by clearly delineating sentences.
Based on past review comments from ysmolski.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/plan/visitor.go(1 hunks)v2/pkg/engine/resolve/inbound_request_singleflight.go(1 hunks)v2/pkg/engine/resolve/resolve.go(23 hunks)v2/pkg/engine/resolve/resolve_test.go(31 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/plan/visitor.go
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-09-19T14:51:33.724Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:339-347
Timestamp: 2025-09-19T14:51:33.724Z
Learning: In the graphql-go-tools resolver, the event loop follows a strict single-threaded design where all subscription lifecycle management (add/remove from triggers) must happen through the events channel to the main processEvents() goroutine. Worker goroutines should not directly modify subscription state or call unsubscribe methods, as this would break the single-threaded event loop architecture and create race conditions.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-10-16T08:52:33.278Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go:59-63
Timestamp: 2025-10-16T08:52:33.278Z
Learning: Go 1.24 (released February 2025) introduced the testing.B.Loop() method for benchmarks. Use `for b.Loop() { /* code */ }` instead of `for i := 0; i < b.N; i++` in benchmark functions when the Go version is 1.24 or higher. The Loop() method provides more predictable benchmarking results by running the loop exactly once per -count and preventing certain compiler optimizations.
Applied to files:
v2/pkg/engine/resolve/resolve_test.go
🧬 Code graph analysis (2)
v2/pkg/engine/resolve/resolve_test.go (1)
v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)
v2/pkg/engine/resolve/resolve.go (5)
v2/pkg/engine/resolve/subgraph_request_singleflight.go (2)
SubgraphRequestSingleFlight(15-17)NewSingleFlight(54-62)v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
InboundRequestSingleFlight(16-18)NewRequestSingleFlight(28-36)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/context.go (3)
Context(18-40)Request(205-208)SubgraphHeadersBuilder(46-52)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (2)
v2/pkg/engine/resolve/resolve_test.go (2)
6711-6818: Nice negative test for invalid subscription filter templates.The
"should err when subscription filter has multiple templates"case builds aSubscriptionFilterwhose value template composes multiple context variables (aandb) into one segment, then asserts that each upstream event produces a GraphQL error response with"invalid subscription filter template". This is a good regression guard around the template validation logic for filters.No issues spotted with the plan construction or expectations.
6823-7136: Benchmarks for nested batching and arena paths are well‑structured and pool‑friendly.The three new benchmarks:
- Reuse
bytes.BufferandContextviasync.PoolandNewContext, callingctx.Free()before putting back.- Exercise both
ResolveGraphQLResponseandArenaResolveGraphQLResponseunderb.RunParallel, with realistic federation‑style plans.- Set
b.ReportAllocs()andb.SetBytes(...)against a fixed expected payload, which makes the numbers comparable across runs.This setup should give useful signal on the memory/throughput impact of the arena and request batching changes without introducing obvious data races.
Also applies to: 7138-7451, 7453-7761
There was a problem hiding this comment.
Actionable comments posted: 0
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/loader.go (1)
885-885: Fix pipeline failure: Use loader's jsonArena instead of local Arena.Line 885 creates a local
astjson.Arena{}which causes the compilation error flagged by the pipeline. According to the PR's arena integration pattern and the lifecycle comments elsewhere in this file (lines 175-188), you should use the loader'sjsonArenafield instead.🔧 Proposed fix
- arena := astjson.Arena{} + // Use a separate arena for validLocations since we're building a temporary structure + // that will be Set back into the value, and we don't want to tie its lifecycle to l.jsonArena + arena := astjson.Arena{}Actually, on closer inspection, this function is building a temporary
validLocationsarray that gets Set back into the error value. The pattern suggests this should still use the loader's arena to maintain lifecycle consistency:- arena := astjson.Arena{} + // Reuse the loader's arena for temporary allocations + // The validLocations array will be Set into value, tying lifecycles togetherHowever, based on the pipeline failure
undefined: astjson.Arena, it appears theastjsonpackage doesn't export anArenatype. You should usearena.Arenafrom thego-arenapackage instead, or usel.jsonArenadirectly if appropriate.- arena := astjson.Arena{} + validLocations := l.jsonArena.NewArray()And remove the
arenavariable declaration since you'd usel.jsonArena.NewArray()directly on line 895.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/pkg/engine/plan/visitor.gov2/pkg/engine/resolve/const.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve.go
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/plan/visitor.go
- v2/pkg/engine/resolve/const.go
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/resolve/loader_test.go
📚 Learning: 2025-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-09-19T14:51:33.724Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:339-347
Timestamp: 2025-09-19T14:51:33.724Z
Learning: In the graphql-go-tools resolver, the event loop follows a strict single-threaded design where all subscription lifecycle management (add/remove from triggers) must happen through the events channel to the main processEvents() goroutine. Worker goroutines should not directly modify subscription state or call unsubscribe methods, as this would break the single-threaded event loop architecture and create race conditions.
Applied to files:
v2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/resolve/loader.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader_test.go (2)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/loader.go (1)
Loader(148-188)
🪛 GitHub Actions: execution
v2/pkg/engine/resolve/loader.go
[error] 885-885: undefined: astjson.Arena (possible missing type in dependency or API change) while compiling loader.go during 'go test -count=1 ./...'
🪛 GitHub Actions: v2-ci
v2/pkg/engine/resolve/loader.go
[error] 885-885: undefined: astjson.Arena
🔇 Additional comments (5)
v2/pkg/engine/resolve/loader.go (2)
175-188: Excellent arena lifecycle documentation.The comments clearly explain the arena lifecycle requirements and thread-safety constraints. The emphasis on "tying lifecycles together" (lines 492-494) is crucial for preventing segfaults with arena-backed parsing. This documentation will help prevent future mistakes.
Also applies to: 489-497
1540-1545: Proper arena lifecycle boundary handling.Correctly copies the batchStats off the arena before returning to avoid memory corruption. The comment on line 1540 clearly explains why this copy is necessary, which will help maintainers understand the pattern.
v2/pkg/engine/resolve/resolve.go (3)
247-260: Well-designed auto-scaling for deduplication shards.The GOMAXPROCS-based scaling option allows the system to automatically adapt to available CPU resources, which is particularly valuable in containerized environments where cgroup limits are respected (as noted in the comment). This will help the deduplication scale appropriately across different deployment sizes.
355-420: Well-structured arena-based resolution path.The implementation properly manages two separate arenas (resolve and response) with correct lifecycle boundaries. The split allows releasing the resolve arena (line 409) before the potentially slow client write operation, which is an excellent optimization. Error paths consistently release both arenas, preventing leaks.
1220-1238: Correctly implements trigger deduplication with headers.The
prepareTriggerfunction properly combines both the input and headers hash to create a unique trigger ID. This ensures that subscriptions with different headers don't share the same trigger, which is critical for correctness when headers affect subscription behavior (e.g., authorization).
# Conflicts: # v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go # v2/pkg/engine/resolve/context.go
There was a problem hiding this comment.
Actionable comments posted: 1
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/context.go (1)
3-15:⚠️ Potential issue | 🟡 MinorFix gci import grouping.
gci is failing because third‑party imports are split into two groups. Please keep external imports together.
🧹 Suggested import grouping
import ( "context" "encoding/json" "errors" "io" "net/http" "sort" "time" "github.com/wundergraph/astjson" - "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/datasource/httpclient" )
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/resolve/context.go`:
- Around line 205-208: NewContext must initialize the Context.subgraphErrors map
and all defensive nil checks for that field must be removed so the codebase
fails fast on improper Context construction; update NewContext to allocate
c.subgraphErrors and delete the nil-guard in appendSubgraphErrors (and the
similar guard around lines 217-223) so callers rely on NewContext rather than
allowing Context{} literals, referencing the NewContext constructor and the
appendSubgraphErrors method and the subgraphErrors field when making the
changes.
| func (c *Context) appendSubgraphErrors(ds DataSourceInfo, errs ...error) { | ||
| if c.subgraphErrors == nil { | ||
| c.subgraphErrors = make(map[string]error) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restore fail‑fast invariant for subgraphErrors.
NewContext no longer initializes subgraphErrors and appendSubgraphErrors now guards nil, which weakens the enforced “NewContext only” invariant. Please keep initialization in NewContext and remove the defensive nil check.
♻️ Suggested change
func (c *Context) appendSubgraphErrors(ds DataSourceInfo, errs ...error) {
- if c.subgraphErrors == nil {
- c.subgraphErrors = make(map[string]error)
- }
c.subgraphErrors[ds.Name] = errors.Join(c.subgraphErrors[ds.Name], errors.Join(errs...))
}
func NewContext(ctx context.Context) *Context {
if ctx == nil {
panic("nil context.Context")
}
return &Context{
ctx: ctx,
+ subgraphErrors: make(map[string]error),
}
}Based on learnings: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Also applies to: 217-223
🤖 Prompt for AI Agents
In `@v2/pkg/engine/resolve/context.go` around lines 205 - 208, NewContext must
initialize the Context.subgraphErrors map and all defensive nil checks for that
field must be removed so the codebase fails fast on improper Context
construction; update NewContext to allocate c.subgraphErrors and delete the
nil-guard in appendSubgraphErrors (and the similar guard around lines 217-223)
so callers rely on NewContext rather than allowing Context{} literals,
referencing the NewContext constructor and the appendSubgraphErrors method and
the subgraphErrors field when making the changes.
There was a problem hiding this comment.
Actionable comments posted: 0
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/context.go (1)
29-47:⚠️ Potential issue | 🟠 MajorClear new Context fields in
Free()to avoid cross‑request leakage.
Context.Free()resets most fields for pooling, but the newly addedVariablesHashandSubgraphHeadersBuilderare left intact, so stale hashes/header builders can bleed into the next request. Consider resetting them when freeing.♻️ Proposed fix
func (c *Context) Free() { c.ctx = nil c.Variables = nil + c.VariablesHash = 0 c.Files = nil c.Request.Header = nil c.RenameTypeNames = nil c.RemapVariables = nil c.TracingOptions.DisableAll() c.Extensions = nil c.subgraphErrors = nil c.authorizer = nil c.LoaderHooks = nil + c.SubgraphHeadersBuilder = nil }Also applies to: 267-279
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.247](v2.0.0-rc.246...v2.0.0-rc.247) (2026-02-06) ### Features * improve memory management & request deduplication ([#1336](#1336)) ([938d1a9](938d1a9)) --- 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 * **New Features** * Improved memory management performance * Added request deduplication functionality <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [1.7.0](execution/v1.6.0...execution/v1.7.0) (2026-02-06) ### Features * compute static costs ([#1359](#1359)) ([004f68e](004f68e)) * improve memory management & request deduplication ([#1336](#1336)) ([938d1a9](938d1a9)) ### Bug Fixes * add ability to exclude introspection queries from complexity limits ([#1342](#1342)) ([19c8693](19c8693)) --- 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 * **New Features** * Compute static costs capability * Memory management improvements * Request deduplication * **Bug Fixes** * Improved query complexity filtering for introspection requests <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 participants
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores