draft: add response caching support - #1628
Conversation
Squash of the full tmp-caching series plus follow-ups: envelope types map removed, key format v1, comment hygiene, and the docs/caching documentation set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded a complete GraphQL response-caching system with declarative configuration, entity and root-field caching, L1/L2 storage, privacy partitions, cache-control handling, batching, partial loads, tracing, client metadata, planner integration, execution hooks, test infrastructure, and extensive documentation. ChangesResponse caching
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionEngine
participant Planner
participant CacheController
participant Subgraph
participant Store
Client->>ExecutionEngine: Execute GraphQL operation
ExecutionEngine->>Planner: Build cache-aware plan
Planner-->>ExecutionEngine: Return configured fetch plan
ExecutionEngine->>CacheController: Prepare fetch
CacheController->>Store: Read cache entries
Store-->>CacheController: Return hits and misses
CacheController->>Subgraph: Fetch missing data
Subgraph-->>CacheController: Return data and cache headers
CacheController->>Store: Persist cache entries
CacheController-->>ExecutionEngine: Return merged data and metadata
ExecutionEngine-->>Client: Return GraphQL response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (22)
v2/pkg/engine/plan/representationvariable/representation_variable.go (2)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale TODO.
remapPathsis implemented at Lines 244-248 and applied to the field path. The TODO no longer describes missing work.♻️ Proposed cleanup
-// TODO: add support for remapping path -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/representationvariable/representation_variable.go` at line 24, Remove the stale TODO comment about adding remapping path support, since remapPaths is already implemented and applied to the field path.
97-120: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDiscarded type assertions can panic on a shape mismatch.
mergeArraysandmergeObjectsignore the assertion result. Ifrightis not the same node kind asleft(the pairing is decided byfieldsHasField, which compares onlyNameandOnTypeNames),rightArrayorrightObjectis nil and the merge dereferences nil. Two sources that select the same field of the same type should always produce the same node kind, so this is defensive only. Consider returningleftunchanged when the assertion fails.🛡️ Proposed guard
func mergeArrays(left, right resolve.Node) resolve.Node { - leftArray, _ := left.(*resolve.Array) - rightArray, _ := right.(*resolve.Array) - - if leftArray.Item.NodeKind() == resolve.NodeKindObject { + leftArray, ok := left.(*resolve.Array) + if !ok { + return left + } + rightArray, ok := right.(*resolve.Array) + if !ok { + return leftArray + } + if leftArray.Item != nil && rightArray.Item != nil && leftArray.Item.NodeKind() == resolve.NodeKindObject { leftArray.Item = mergeObjects(leftArray.Item, rightArray.Item) } return leftArray } func mergeObjects(left, right resolve.Node) resolve.Node { - leftObject, _ := left.(*resolve.Object) - rightObject, _ := right.(*resolve.Object) - + leftObject, ok := left.(*resolve.Object) + if !ok { + return left + } + rightObject, ok := right.(*resolve.Object) + if !ok { + return leftObject + } for _, field := range rightObject.Fields {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/representationvariable/representation_variable.go` around lines 97 - 120, Guard the type assertions in mergeArrays and mergeObjects: if either operand cannot be asserted to the expected resolve.Array or resolve.Object type, return the original left node unchanged before dereferencing it. Preserve the existing merge behavior when both operands have the expected node kind.v2/pkg/engine/plan/representationvariable/representation_variable_test.go (1)
14-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the parse report in the test helpers.
astparser.ParseGraphqlDocumentStringreturns a report that both helpers discard (Line 15 here and Line 873 inbuild). A typo in a schema fixture then produces an empty document and a confusing node-mismatch failure instead of a clear parse error.💚 Proposed fix
- definition, _ := astparser.ParseGraphqlDocumentString(definitionStr) + definition, parseReport := astparser.ParseGraphqlDocumentString(definitionStr) + require.False(t, parseReport.HasErrors(), "parse definition: %s", parseReport.Error())Apply the same change in
buildat Line 873.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/representationvariable/representation_variable_test.go` around lines 14 - 24, Update the test helpers around runTest and build to capture the parse report returned by astparser.ParseGraphqlDocumentString and assert it contains no errors before using the parsed definition. Apply the same validation in both helpers so malformed schema fixtures fail with a clear parse error instead of a node mismatch.v2/pkg/engine/resolve/fetchtree.go (1)
202-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
deferwith a direct statement. The inner switch contains noreturn, so the deferred closure adds no control-flow benefit. It only delays the assignment to function exit and callsCacheConfig()twice. Placing the code after the switch is equivalent and easier to follow.♻️ Proposed simplification
case FetchTreeNodeKindSingle: - defer func() { - // The cache config reads through the Fetch interface (never a - // switch over concrete fetch types); an uncached fetch leaves the - // field empty and the plan output unchanged. - if queryPlan.Fetch != nil && n.Item.Fetch.CacheConfig() != nil { - queryPlan.Fetch.Cache = n.Item.Fetch.CacheConfig().String() - } - }() switch f := n.Item.Fetch.(type) {and after the inner switch closes:
default: } + // The cache config reads through the Fetch interface (never a switch + // over concrete fetch types); an uncached fetch leaves the field empty + // and the plan output unchanged. + if cfg := n.Item.Fetch.CacheConfig(); queryPlan.Fetch != nil && cfg != nil { + queryPlan.Fetch.Cache = cfg.String() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/fetchtree.go` around lines 202 - 209, Replace the deferred closure around the cache assignment with a direct statement after the inner switch in the surrounding fetch-resolution flow. Call n.Item.Fetch.CacheConfig() once, guard against nil queryPlan.Fetch and nil cache configuration, then assign its String() value to queryPlan.Fetch.Cache.v2/pkg/engine/postprocess/postprocess.go (1)
293-307: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe sequence heuristic silently degrades when the first child adds more than one tree.
childParentis set tobeforeonly whenwalk(node.ChildNodes[0], parentIndex)appended exactly one tree. IfChildNodes[0]is itself aSequenceorParallelnode that contributes several trees, the nested groups are parented to the enclosing parent instead of the real parent group, and the L1 narrowing pass then uses a wrong ancestry. The comment states thatbuildDeferTreealways emits aSinglenode in that slot. Consider making that invariant explicit, so a future change tobuildDeferTreefails loudly instead of producing a wrong parent index.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/postprocess/postprocess.go` around lines 293 - 307, The Sequence handling in walk should enforce that ChildNodes[0] is the single parent group emitted by buildDeferTree, rather than inferring childParent from the number of appended trees. Validate that walking the first child adds exactly one tree and fail loudly when the invariant is violated; then use that newly appended tree as childParent before walking the remaining children.execution/cachingtesting/art_e2e_test.go (1)
557-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd assertions for the writes this subtest describes.
The comment states that both fetches miss and both write. The test asserts only the trace, which shows the reads. Add
store.Ops()assertions for the twoSetManyitems, or at least assertproducts.Requests(). Without them a regression that stops writing entries still passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/cachingtesting/art_e2e_test.go` around lines 557 - 582, Extend the “two representations of one entity are two independent entries” subtest to assert the writes, not only the trace reads. Use store.Ops() to verify both expected SetMany entries, or products.Requests() to verify both product fetches occurred, preserving the distinct sku and upc representations.v2/pkg/engine/cache/fetch_cache_configurator_types_test.go (1)
126-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why this batch is not a mixed-type decline.
buildConfigdeclines caching wheninfo.RootFieldscarry differentTypeNamevalues. This row still receives a config because the two concrete types arrive through the representation'sOnTypeNames, whileFetchInfo.RootFieldsstays single-typed. Add one sentence stating that distinction. It separates this row from the "mixed entity types decline caching" row inv2/pkg/engine/cache/fetch_cache_configurator_test.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/fetch_cache_configurator_types_test.go` around lines 126 - 149, Add a one-sentence comment to the “a batch over two concrete types takes the shortest declared lifetime” test explaining that the concrete types come through the representation’s OnTypeNames while FetchInfo.RootFields remains single-typed, so buildConfig does not classify it as mixed-type caching.execution/cachingtesting/compose.sh (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the wgc version and anchor the working directory.
wgc@latestmakes fixture regeneration non-reproducible. A later wgc release can changeconfig.json(the committed fixture recordscompatibilityVersion1:0.62.2) and silently break the pinned e2e expectations. The relative paths also require the caller to run the script fromexecution/cachingtesting.♻️ Proposed refactor
#!/usr/bin/env bash set -euo pipefail +cd "$(dirname "${BASH_SOURCE[0]}")" + +WGC_VERSION="${WGC_VERSION:-0.x.y}" # pin the version that produced config.json + echo "Composing caching test subgraphs" -npx -y wgc@latest router compose -i graph.yaml -o config.json +npx -y "wgc@${WGC_VERSION}" router compose -i graph.yaml -o config.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/cachingtesting/compose.sh` around lines 1 - 10, Update the compose script to invoke the wgc release matching the committed fixture’s compatibilityVersion (1:0.62.2) instead of latest, and anchor execution to the script’s own directory before using graph.yaml and config.json so callers can run it from any working directory.v2/pkg/engine/cache/cache_control.go (1)
118-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign
Scopewith its documented contract.The doc states that
Scopeis empty when the result may not be stored at all.resolveCachingalways setsScopefromentryScope(in.Private), including whenin.L2is false and whenUncacheablePrivateis true. Both cases permit no store write. Either clearScopeon those return paths or correct the comment.♻️ Proposed change
if !in.L2 || decision.UncacheablePrivate { + decision.Scope = "" return decision }Also applies to: 142-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/cache_control.go` around lines 118 - 121, Update resolveCaching so Scope is empty whenever the result cannot be stored, specifically on the in.L2-disabled and UncacheablePrivate return paths; only assign entryScope(in.Private) when a store write is permitted, preserving the documented CacheControl contract.execution/cachingtesting/defer_l1_e2e_test.go (1)
35-48: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a fallback that closes the gate.
releasecloses only from the flush callback atlen(frames) == 2.executeDeferOnFlushappends the residual writer buffer at Line 45 without callingonFlush. If a regression delivers the second frame through that path,releasenever closes and the gated subgraph handler blocks until the Go test timeout. At.Cleanupthat closes the gate once turns that hang into a normal assertion failure.♻️ Proposed hardening
release := make(chan struct{}) + var releaseOnce sync.Once + closeGate := func() { releaseOnce.Do(func() { close(release) }) } + t.Cleanup(closeGate) gated := &SubgraphRule{Then call
closeGate()in the flush callback instead ofclose(release).Also applies to: 268-287, 353-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/cachingtesting/defer_l1_e2e_test.go` around lines 35 - 48, Harden executeDeferOnFlush and the corresponding release-gate tests so the gate always closes even when the final frame comes from writer.String() rather than the flush callback. Add a one-time cleanup fallback using the existing closeGate mechanism, and have the flush callback invoke closeGate instead of directly closing release; apply the same pattern to the other referenced test cases.v2/pkg/engine/plan/root_field_isolation.go (2)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the query root path constant.
Extract
"query"fromisParentPathIsRootOperationPathand reuse it inroot_field_isolation.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/root_field_isolation.go` around lines 19 - 21, Update isParentPathIsRootOperationPath to expose or reuse a shared query-root path constant, then replace the hardcoded "query" comparison in root_field_isolation.go with that constant. Keep the existing root-path check behavior unchanged.
22-23: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the resolved configuration by datasource ID if profiling shows this gate is hot.
Resolverebuilds scalar fields and applies subgraph overrides, but it does not merge type or root-field tiers.RootFieldthen performs a linear scan overEffectiveSubgraphConfig.RootFields. A per-datasource cache removes repeatedResolvework, but it does not remove theRootFieldscan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/root_field_isolation.go` around lines 22 - 23, Profile the root-field gate around Resolve and RootField, and if it is hot, cache the resolved configuration keyed by datasource ID (field.ds.Id()) to avoid repeated Resolve work. Preserve the RootField lookup behavior and note that caching Resolve does not eliminate its linear EffectiveSubgraphConfig.RootFields scan.v2/pkg/engine/plan/cacheconfig/cache_directive_test.go (1)
20-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake omitted-scope expectations explicit.
CacheScopePublicis the zero value. AddScope: CacheScopePublicto every expectedTypeCacheConfigfor an@cachedeclaration withoutscope, including lines 20–39 and 60–80.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/cacheconfig/cache_directive_test.go` around lines 20 - 39, Update every expected TypeCacheConfig for `@cache` declarations that omit scope in the cache directive tests, including the cases near the existing declarations and those later in the file, to explicitly set Scope: CacheScopePublic. Preserve all existing MaxAge and warning expectations.v2/pkg/engine/cache/optimize_l1_cache.go (1)
295-330: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueInclude
OnTypeNamesin the narrowing identity.Fields with the same schema name and arguments can have different inline-fragment type conditions across provider and consumer trees. Add a canonical sorted representation of
OnTypeNamestofieldNarrowingName; runtime coverage remains the final guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/optimize_l1_cache.go` around lines 295 - 330, Update fieldNarrowingName to include a canonical sorted representation of field.OnTypeNames in the generated identity, alongside the schema name and argument bindings. Ensure identical type-condition sets produce the same representation regardless of input order, while preserving the existing argument handling and findFieldByNarrowingName lookup behavior.v2/pkg/engine/cache/controller_shadow_test.go (1)
36-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort the recorded compares to keep multi-item rows deterministic.
CompareShadowranges overh.ShadowStash, which is a map. Go randomizes map iteration order. Every current row stashes one item, soobs.compares[0]is stable today. A future row with two stashed items would flake.Collect the entries by ascending item index, or sort
o.comparesbykeybefore the assertions read it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/controller_shadow_test.go` around lines 36 - 52, The CompareShadow method records map entries in nondeterministic iteration order; make the recorded compares deterministic by sorting o.compares by key before returning, or by collecting ShadowStash entries in ascending item-index order. Preserve the existing comparison fields and behavior.v2/pkg/engine/cache/cache_key_template.go (1)
440-472: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRoot-field keys include every request variable, also unused ones.
canonicalVariablesrenders the whole variables object. A variable that the cached root field does not consume still changes the key. Two otherwise identical requests then miss each other. The result stays correct; only the hit rate drops.If the plan already knows the variables a root-field fetch depends on, restrict the preimage to that set. Otherwise document the behavior next to the
PRECONDITIONnote.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/cache_key_template.go` around lines 440 - 472, Update canonicalVariables to include only variables consumed by the cached root-field fetch when the execution plan exposes that dependency set, preserving deterministic sorting and serialization for the selected variables. If no dependency metadata is available, document next to the existing PRECONDITION note that all request variables remain part of the cache key.v2/pkg/engine/cache/controller_store_test.go (1)
467-474: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the misalignment error by message, not by struct equality.
assert.Equalcompares the twoerrorvalues withreflect.DeepEqual. It passes today because both are*errors.errorStringwith the same text. It breaks unreadably if the controller wraps the error withfmt.Errorf("...: %w", err).Assert the stable fields, then assert the message separately.
♻️ Proposed change
- assert.Equal(t, []StoreError{ - { - Op: "GetMany", - Subgraph: "", - KeyCount: 2, - Err: errors.New("store returned 1 entries for 2 keys"), - }, - }, obs.storeErrors) + require.Len(t, obs.storeErrors, 1) + assert.Equal(t, "GetMany", obs.storeErrors[0].Op) + assert.Equal(t, "", obs.storeErrors[0].Subgraph) + assert.Equal(t, 2, obs.storeErrors[0].KeyCount) + assert.ErrorContains(t, obs.storeErrors[0].Err, "store returned 1 entries for 2 keys")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/controller_store_test.go` around lines 467 - 474, Update the GetMany misalignment assertion in the relevant test to compare stable StoreError fields separately, then assert the contained error’s message independently rather than using assert.Equal on the entire struct. Preserve the expected operation, subgraph, and key count while allowing the controller to wrap the underlying error.v2/pkg/engine/cache/partial_test.go (1)
272-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant closure.
The immediately invoked function returns
partialConfig(t)unchanged. PasspartialConfig(t)directly.♻️ Proposed simplification
- writeThrough(t, NewController(store, nil).BeginRequest(nil), func() *resolve.FetchCacheConfig { - plain := partialConfig(t) - return plain - }(), productItem(t, "2"), fresh("2")) + writeThrough(t, NewController(store, nil).BeginRequest(nil), partialConfig(t), productItem(t, "2"), fresh("2"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/partial_test.go` around lines 272 - 275, Remove the redundant immediately invoked closure in the writeThrough call and pass partialConfig(t) directly as the fetch cache configuration argument, leaving the surrounding NewController, productItem, and fresh arguments unchanged.v2/pkg/engine/cache/controller.go (1)
517-551: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the inner loop variable to avoid shadowing
state.Line 517 declares
state *handleState. Line 540 declares anotherstateof typeresolve.ItemCacheStateinside the loop. The outer variable is used again at line 562. The code is correct, but the reuse of one name for two different types reduces readability.♻️ Proposed rename
for _, item := range in.Items { - state := resolve.ItemCacheState{ + itemState := resolve.ItemCacheState{ Item: item, RenderedKey: key, Tags: tags, FromCache: fromCache, } if fromCache != nil { - state.RemainingTTL = entry.RemainingTTL - state.ServedFreshness = servedFreshness + itemState.RemainingTTL = entry.RemainingTTL + itemState.ServedFreshness = servedFreshness } - items = append(items, state) + items = append(items, itemState) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/controller.go` around lines 517 - 551, Rename the resolve.ItemCacheState variable declared inside the loop over in.Items to a distinct name, such as itemState, while preserving its field assignments and append to items. Keep the outer handleState variable state unchanged for its later use.v2/pkg/engine/resolve/cache_transaction.go (1)
42-54: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe
StructuralCopyerror fallback returns the aliased source.The doc comment promises value isolation. On line 51 the heap-mode fallback returns
vitself when the re-parse fails, so the caller receives an alias and the promised isolation is silently lost. A round-trip of a valid*astjson.Valueshould not fail, so this path is unlikely. Returningnilon that path would surface the failure at the caller instead of producing hidden merge aliasing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/cache_transaction.go` around lines 42 - 54, The error fallback in CacheTransaction.StructuralCopy must preserve value isolation instead of returning the aliased input. Replace the parse-failure return of v with nil, while keeping the DeepCopy path and successful copied result unchanged.v2/pkg/engine/cache/controller_test.go (2)
55-108: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
testStorehas no synchronization.
GetManyandSetManymutates.dataands.opswithout a mutex. Every test in this file drives the controller sequentially, so this is safe today. If a future test exercises parallel fetch groups against one store, the race detector will fire. Consider adding async.Mutexnow to keep the double usable for concurrent rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/controller_test.go` around lines 55 - 108, Add synchronization to testStore using a sync.Mutex, and lock access in GetMany, SetMany, and value while reading or mutating s.data and s.ops. Ensure all early-return paths release the lock so the test double remains safe for concurrent controller operations.
386-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRows [F3] and [F4] build identical inputs.
Both rows produce
MergeInput{Items: ..., FetchFailed: true, StatusCode: 200}. They therefore assert the same behavior, and neither models an empty body or a parse failure distinctly. Row [F2] already coversFetchFailed: true. Consider differentiating the inputs so each row exercises the signal its name describes, for example an empty-body row withResponseData: niland a parse-failure row with a non-2xx status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/controller_test.go` around lines 386 - 397, Differentiate the [F3] and [F4] cases in the test inputs: make the empty-body case use ResponseData: nil while retaining a successful status, and make the parse-failure case use a non-2xx status with appropriate response data. Avoid duplicating the [F2] FetchFailed-only input so each row exercises its named condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/caching/6-open-issues.md`:
- Around line 19-21: Implement the root-field cache fix so its key is derived
from the fetch’s rendered argument values, matching entity-key behavior and
distinguishing aliased calls with different arguments. If that implementation is
not available, reject or disable root-field cache configuration rather than
relying solely on the documented warning.
In `@execution/cachingtesting/cache_control_e2e_test.go`:
- Around line 228-235: Update the comment above the ops assertions to state that
there are two lookups, neither served, followed by one flush carrying only the
deferred fetch’s value; remove the claim that there was no write and correct any
reference to the write’s position from the second op to the flush represented by
ops[2].
In `@execution/cachingtesting/client_headers_e2e_test.go`:
- Around line 325-338: Guard the op-log accesses in the affected test with
require.Len checks before indexing: require at least one operation before
reading ops[len(ops)-1], then require at least two items before accessing
writes.Items[0] and writes.Items[1]. Follow the existing require.Len pattern
used by the other subtests so failures remain scoped to the subtest.
In `@execution/cachingtesting/privacy_e2e_test.go`:
- Around line 255-257: Update the seeded envelope in the privacy E2E test around
store.Seed to derive the JSON "created" timestamp from the current clock, using
fmt as needed, so it remains fresh within the declared 60-second TTL. Preserve
the private scope and existing store TTL, ensuring ScopeMismatch is the
fixture’s only discard cause.
In `@execution/cachingtesting/shadow_e2e_test.go`:
- Around line 37-59: Synchronize access to inventoryRule.Response between the
request handler and the test’s mutation before the second Execute call. Update
the SubgraphRule response read/write path, or use a synchronized per-request
response mechanism, so the handler’s read cannot race with the test goroutine’s
assignment while preserving the stock=7 response for the second request.
In `@v2/pkg/engine/cache/cache_control.go`:
- Around line 55-60: Clamp the parsed seconds in the max-age handling before
converting and multiplying by time.Second: update the seconds validation in the
surrounding cache-control parsing flow to cap values at the maximum
representable duration in seconds, while preserving rejection of parse errors
and negatives. Then set MaxAge and HasMaxAge from the safely bounded value.
In `@v2/pkg/engine/cache/cache_key_template.go`:
- Around line 391-405: Update renderCacheKey and its hashHex helper to use a
collision-resistant 128-bit digest, preferably truncated SHA-256 consistent with
sha256Hex, instead of unkeyed xxhash64; preserve the existing prefix/preimage
structure while producing the wider digest. Bump cacheFormatVersion to v1 so the
changed key layout is established before production entries exist.
- Around line 153-180: Validate or JSON-escape typeName before appending it in
cacheKeyTemplate.renderRepresentation, since it comes from itemTypeName and is
inserted directly into the quoted key preimage. Prefer the existing GraphQL name
validation helper if available, rejecting invalid names; otherwise use the
established escaping mechanism so hostile __typename values cannot create
ambiguous keys.
In `@v2/pkg/engine/cache/cachetesting/fakes.go`:
- Around line 342-347: Update FakeStore.Ops to deep-copy each StoreOp before
returning, including the Keys, Hits, and Items slices and every item's Tags
slice. Preserve the existing locking and return a cloned operation log so
NormalizeStoreOpsClock mutations cannot alter s.ops.
In `@v2/pkg/engine/cache/coverage.go`:
- Around line 67-102: Update coversNode to explicitly handle *resolve.Null,
matching the fallback nodes emitted by createFieldValue for unresolved or
unsupported schema types. Define the branch so these nodes can be considered
covered instead of falling through to the default false result; do not alter the
existing Scalar, Object, or Array behavior.
In `@v2/pkg/engine/cache/observer.go`:
- Around line 144-176: Update TraceObserver’s StoreErrors and UncacheablePrivate
accessors to atomically drain their recorded event slices after returning them,
clearing the buffers under the existing mutex while preserving occurrence order.
Ensure both buffers no longer retain events for the observer lifetime; use the
same drain behavior for store errors and uncacheable-private results.
In `@v2/pkg/engine/cache/transform.go`:
- Around line 58-70: Update computeArgSuffix to distinguish an omitted argument
from an explicitly supplied null when resolving ctx.Variables, using a distinct
absent-value marker or the argument’s effective schema default instead of
hashing omitted values as null. Preserve the existing null serialization for
explicitly provided null values and keep variable remapping behavior unchanged.
In `@v2/pkg/engine/plan/representationvariable/representation_variable.go`:
- Around line 356-417: The resolveOnTypeNames method must not panic when the
inline fragment type condition resolves to a union. Replace the union branch’s
panic with the established StopWithInternalErr(...) planning-error path, or
reuse the union-member handling from plan.Visitor.resolveOnTypeNames, while
preserving the existing interface and concrete-type behavior.
In `@v2/pkg/engine/resolve/fetch.go`:
- Around line 26-59: Keep the exported resolve.Fetch interface
backward-compatible by removing the newly added requirements from the interface,
or document the intentional breaking change in v2/CHANGELOG.md if those methods
must remain. Update the interface declaration around CacheConfig,
SetCacheConfig, IsEntityFetch, IsBatchEntityFetch, RepresentationInputTemplate,
LoadTrace, and SetDataSource without changing unrelated behavior.
In `@v2/pkg/engine/resolve/resolve.go`:
- Around line 529-537: Update the deferred-response flow around
ctx.deferredResponse and the initial frame rendering to call
ctx.flushCacheTraces() before the trace extension is rendered, ensuring cache
events from the initial fetch are included in that response trace. Keep
ctx.endCacheRequest() deferred until request processing completes.
---
Nitpick comments:
In `@execution/cachingtesting/art_e2e_test.go`:
- Around line 557-582: Extend the “two representations of one entity are two
independent entries” subtest to assert the writes, not only the trace reads. Use
store.Ops() to verify both expected SetMany entries, or products.Requests() to
verify both product fetches occurred, preserving the distinct sku and upc
representations.
In `@execution/cachingtesting/compose.sh`:
- Around line 1-10: Update the compose script to invoke the wgc release matching
the committed fixture’s compatibilityVersion (1:0.62.2) instead of latest, and
anchor execution to the script’s own directory before using graph.yaml and
config.json so callers can run it from any working directory.
In `@execution/cachingtesting/defer_l1_e2e_test.go`:
- Around line 35-48: Harden executeDeferOnFlush and the corresponding
release-gate tests so the gate always closes even when the final frame comes
from writer.String() rather than the flush callback. Add a one-time cleanup
fallback using the existing closeGate mechanism, and have the flush callback
invoke closeGate instead of directly closing release; apply the same pattern to
the other referenced test cases.
In `@v2/pkg/engine/cache/cache_control.go`:
- Around line 118-121: Update resolveCaching so Scope is empty whenever the
result cannot be stored, specifically on the in.L2-disabled and
UncacheablePrivate return paths; only assign entryScope(in.Private) when a store
write is permitted, preserving the documented CacheControl contract.
In `@v2/pkg/engine/cache/cache_key_template.go`:
- Around line 440-472: Update canonicalVariables to include only variables
consumed by the cached root-field fetch when the execution plan exposes that
dependency set, preserving deterministic sorting and serialization for the
selected variables. If no dependency metadata is available, document next to the
existing PRECONDITION note that all request variables remain part of the cache
key.
In `@v2/pkg/engine/cache/controller_shadow_test.go`:
- Around line 36-52: The CompareShadow method records map entries in
nondeterministic iteration order; make the recorded compares deterministic by
sorting o.compares by key before returning, or by collecting ShadowStash entries
in ascending item-index order. Preserve the existing comparison fields and
behavior.
In `@v2/pkg/engine/cache/controller_store_test.go`:
- Around line 467-474: Update the GetMany misalignment assertion in the relevant
test to compare stable StoreError fields separately, then assert the contained
error’s message independently rather than using assert.Equal on the entire
struct. Preserve the expected operation, subgraph, and key count while allowing
the controller to wrap the underlying error.
In `@v2/pkg/engine/cache/controller_test.go`:
- Around line 55-108: Add synchronization to testStore using a sync.Mutex, and
lock access in GetMany, SetMany, and value while reading or mutating s.data and
s.ops. Ensure all early-return paths release the lock so the test double remains
safe for concurrent controller operations.
- Around line 386-397: Differentiate the [F3] and [F4] cases in the test inputs:
make the empty-body case use ResponseData: nil while retaining a successful
status, and make the parse-failure case use a non-2xx status with appropriate
response data. Avoid duplicating the [F2] FetchFailed-only input so each row
exercises its named condition.
In `@v2/pkg/engine/cache/controller.go`:
- Around line 517-551: Rename the resolve.ItemCacheState variable declared
inside the loop over in.Items to a distinct name, such as itemState, while
preserving its field assignments and append to items. Keep the outer handleState
variable state unchanged for its later use.
In `@v2/pkg/engine/cache/fetch_cache_configurator_types_test.go`:
- Around line 126-149: Add a one-sentence comment to the “a batch over two
concrete types takes the shortest declared lifetime” test explaining that the
concrete types come through the representation’s OnTypeNames while
FetchInfo.RootFields remains single-typed, so buildConfig does not classify it
as mixed-type caching.
In `@v2/pkg/engine/cache/optimize_l1_cache.go`:
- Around line 295-330: Update fieldNarrowingName to include a canonical sorted
representation of field.OnTypeNames in the generated identity, alongside the
schema name and argument bindings. Ensure identical type-condition sets produce
the same representation regardless of input order, while preserving the existing
argument handling and findFieldByNarrowingName lookup behavior.
In `@v2/pkg/engine/cache/partial_test.go`:
- Around line 272-275: Remove the redundant immediately invoked closure in the
writeThrough call and pass partialConfig(t) directly as the fetch cache
configuration argument, leaving the surrounding NewController, productItem, and
fresh arguments unchanged.
In `@v2/pkg/engine/plan/cacheconfig/cache_directive_test.go`:
- Around line 20-39: Update every expected TypeCacheConfig for `@cache`
declarations that omit scope in the cache directive tests, including the cases
near the existing declarations and those later in the file, to explicitly set
Scope: CacheScopePublic. Preserve all existing MaxAge and warning expectations.
In `@v2/pkg/engine/plan/representationvariable/representation_variable_test.go`:
- Around line 14-24: Update the test helpers around runTest and build to capture
the parse report returned by astparser.ParseGraphqlDocumentString and assert it
contains no errors before using the parsed definition. Apply the same validation
in both helpers so malformed schema fixtures fail with a clear parse error
instead of a node mismatch.
In `@v2/pkg/engine/plan/representationvariable/representation_variable.go`:
- Line 24: Remove the stale TODO comment about adding remapping path support,
since remapPaths is already implemented and applied to the field path.
- Around line 97-120: Guard the type assertions in mergeArrays and mergeObjects:
if either operand cannot be asserted to the expected resolve.Array or
resolve.Object type, return the original left node unchanged before
dereferencing it. Preserve the existing merge behavior when both operands have
the expected node kind.
In `@v2/pkg/engine/plan/root_field_isolation.go`:
- Around line 19-21: Update isParentPathIsRootOperationPath to expose or reuse a
shared query-root path constant, then replace the hardcoded "query" comparison
in root_field_isolation.go with that constant. Keep the existing root-path check
behavior unchanged.
- Around line 22-23: Profile the root-field gate around Resolve and RootField,
and if it is hot, cache the resolved configuration keyed by datasource ID
(field.ds.Id()) to avoid repeated Resolve work. Preserve the RootField lookup
behavior and note that caching Resolve does not eliminate its linear
EffectiveSubgraphConfig.RootFields scan.
In `@v2/pkg/engine/postprocess/postprocess.go`:
- Around line 293-307: The Sequence handling in walk should enforce that
ChildNodes[0] is the single parent group emitted by buildDeferTree, rather than
inferring childParent from the number of appended trees. Validate that walking
the first child adds exactly one tree and fail loudly when the invariant is
violated; then use that newly appended tree as childParent before walking the
remaining children.
In `@v2/pkg/engine/resolve/cache_transaction.go`:
- Around line 42-54: The error fallback in CacheTransaction.StructuralCopy must
preserve value isolation instead of returning the aliased input. Replace the
parse-failure return of v with nil, while keeping the DeepCopy path and
successful copied result unchanged.
In `@v2/pkg/engine/resolve/fetchtree.go`:
- Around line 202-209: Replace the deferred closure around the cache assignment
with a direct statement after the inner switch in the surrounding
fetch-resolution flow. Call n.Item.Fetch.CacheConfig() once, guard against nil
queryPlan.Fetch and nil cache configuration, then assign its String() value to
queryPlan.Fetch.Cache.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3a3c510-ac6c-4031-8387-30634fbac28e
📒 Files selected for processing (126)
docs/caching/1-readme.mddocs/caching/2-behavior.mddocs/caching/3-onboarding.mddocs/caching/4-redis-adapter-report.mddocs/caching/5-testing-on-js-subgraphs.mddocs/caching/6-open-issues.mdexecution/cachingtesting/args_key_e2e_test.goexecution/cachingtesting/art_e2e_test.goexecution/cachingtesting/batch_e2e_test.goexecution/cachingtesting/cache_control_e2e_test.goexecution/cachingtesting/cachingtesting.goexecution/cachingtesting/cachingtesting_test.goexecution/cachingtesting/cascade_e2e_test.goexecution/cachingtesting/client_headers_e2e_test.goexecution/cachingtesting/compose.shexecution/cachingtesting/config.jsonexecution/cachingtesting/defer_l1_e2e_test.goexecution/cachingtesting/enginetesting.goexecution/cachingtesting/entity_config_test.goexecution/cachingtesting/entity_l2_test.goexecution/cachingtesting/envelope_e2e_test.goexecution/cachingtesting/graph.yamlexecution/cachingtesting/isolation_e2e_test.goexecution/cachingtesting/l1_e2e_test.goexecution/cachingtesting/loader_bench_test.goexecution/cachingtesting/negative_e2e_test.goexecution/cachingtesting/normalization_e2e_test.goexecution/cachingtesting/partial_e2e_test.goexecution/cachingtesting/privacy_e2e_test.goexecution/cachingtesting/provides_data_test.goexecution/cachingtesting/rootfield_e2e_test.goexecution/cachingtesting/shadow_e2e_test.goexecution/cachingtesting/store_batching_e2e_test.goexecution/cachingtesting/subgraphs/deals.graphqlexecution/cachingtesting/subgraphs/inventory.graphqlexecution/cachingtesting/subgraphs/products.graphqlexecution/cachingtesting/subgraphs/reviews.graphqlexecution/cachingtesting/subgraphs/users.graphqlexecution/cachingtesting/tags_e2e_test.goexecution/cachingtesting/type_declaration_e2e_test.goexecution/engine/engine_caching_config_test.goexecution/engine/engine_config.goexecution/engine/execution_engine.goexecution/engine/testdata/complex_nesting_query_with_art.jsonexecution/federationtesting/gateway/httphandler/http.gov2/pkg/engine/cache/cache_control.gov2/pkg/engine/cache/cache_control_test.gov2/pkg/engine/cache/cache_key_args_test.gov2/pkg/engine/cache/cache_key_builder.gov2/pkg/engine/cache/cache_key_builder_test.gov2/pkg/engine/cache/cache_key_partition_test.gov2/pkg/engine/cache/cache_key_template.gov2/pkg/engine/cache/cache_key_template_test.gov2/pkg/engine/cache/cache_response.gov2/pkg/engine/cache/cache_response_test.gov2/pkg/engine/cache/cache_tags.gov2/pkg/engine/cache/cache_tags_test.gov2/pkg/engine/cache/cachetesting/fakes.gov2/pkg/engine/cache/cachetesting/fakes_test.gov2/pkg/engine/cache/cachetesting/realish.gov2/pkg/engine/cache/configure_caching.gov2/pkg/engine/cache/configure_caching_test.gov2/pkg/engine/cache/controller.gov2/pkg/engine/cache/controller_batch_test.gov2/pkg/engine/cache/controller_cache_control_test.gov2/pkg/engine/cache/controller_l1_test.gov2/pkg/engine/cache/controller_negative_test.gov2/pkg/engine/cache/controller_privacy_test.gov2/pkg/engine/cache/controller_rootfield_test.gov2/pkg/engine/cache/controller_shadow_test.gov2/pkg/engine/cache/controller_store_test.gov2/pkg/engine/cache/controller_test.gov2/pkg/engine/cache/coverage.gov2/pkg/engine/cache/coverage_test.gov2/pkg/engine/cache/envelope.gov2/pkg/engine/cache/envelope_test.gov2/pkg/engine/cache/fetch_cache_configurator.gov2/pkg/engine/cache/fetch_cache_configurator_rootfield_test.gov2/pkg/engine/cache/fetch_cache_configurator_test.gov2/pkg/engine/cache/fetch_cache_configurator_types_test.gov2/pkg/engine/cache/observer.gov2/pkg/engine/cache/observer_test.gov2/pkg/engine/cache/optimize_l1_cache.gov2/pkg/engine/cache/optimize_l1_cache_test.gov2/pkg/engine/cache/partial.gov2/pkg/engine/cache/partial_test.gov2/pkg/engine/cache/transform.gov2/pkg/engine/cache/transform_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/plan/cache_provides_data_visitor.gov2/pkg/engine/plan/cache_provides_data_visitor_port_test.gov2/pkg/engine/plan/cache_provides_data_visitor_test.gov2/pkg/engine/plan/cacheconfig/cache_directive.gov2/pkg/engine/plan/cacheconfig/cache_directive_test.gov2/pkg/engine/plan/cacheconfig/cacheconfig.gov2/pkg/engine/plan/cacheconfig/cacheconfig_test.gov2/pkg/engine/plan/cacheconfig/type_cache_test.gov2/pkg/engine/plan/configuration.gov2/pkg/engine/plan/path_builder_visitor.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/representationvariable/representation_variable.gov2/pkg/engine/plan/representationvariable/representation_variable_test.gov2/pkg/engine/plan/root_field_isolation.gov2/pkg/engine/plan/root_field_isolation_test.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/postprocess/postprocess_caching_test.gov2/pkg/engine/resolve/batch_input_assembly.gov2/pkg/engine/resolve/batch_input_assembly_test.gov2/pkg/engine/resolve/cache_config.gov2/pkg/engine/resolve/cache_config_test.gov2/pkg/engine/resolve/cache_controller.gov2/pkg/engine/resolve/cache_controller_test.gov2/pkg/engine/resolve/cache_fetch_test.gov2/pkg/engine/resolve/cache_node_copy_test.gov2/pkg/engine/resolve/cache_noop_test.gov2/pkg/engine/resolve/cache_response.gov2/pkg/engine/resolve/cache_transaction.gov2/pkg/engine/resolve/context.gov2/pkg/engine/resolve/fetch.gov2/pkg/engine/resolve/fetchtree.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/node_object.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/response.gov2/pkg/engine/resolve/subgraph_request_singleflight_test.gov2/pkg/engine/resolve/tainted_objects_test.go
| Fix direction: derive the key from the argument values the fetch itself sends | ||
| (its rendered input), mirroring entity keys. | ||
| Until fixed: do not configure root-field caching for fields a single operation may call twice. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Block root-field caching until the key uses fetch arguments.
The documented failure serves one root-field result to another alias when both calls use different arguments. This changes a valid GraphQL response.
Derive the key from the rendered arguments of this fetch. If that fix cannot ship now, reject or disable root-field cache configuration. Documentation alone cannot prevent this corruption.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/caching/6-open-issues.md` around lines 19 - 21, Implement the root-field
cache fix so its key is derived from the fetch’s rendered argument values,
matching entity-key behavior and distinguishing aliased calls with different
arguments. If that implementation is not available, reject or disable root-field
cache configuration rather than relying solely on the documented warning.
| // Two lookups (one per fetch, neither served) and NOT ONE write: the | ||
| // deferred fetch answered without a Cache-Control and would have written, | ||
| // but it never reached the flush because its own result is a fresh miss | ||
| // whose write is the second op. | ||
| ops := store.Ops() | ||
| require.Len(t, ops, 3) | ||
| require.Len(t, ops[0].Keys, 1) | ||
| key := ops[0].Keys[0] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the contradictory comment.
The comment states "NOT ONE write", but the assertion below expects three ops including a SetMany. The comment also calls the write "the second op" while ops[2] holds it. Rewrite the comment to describe what the test asserts: two lookups, neither served, and one flush that carries the deferred fetch's value only.
📝 Proposed comment fix
- // Two lookups (one per fetch, neither served) and NOT ONE write: the
- // deferred fetch answered without a Cache-Control and would have written,
- // but it never reached the flush because its own result is a fresh miss
- // whose write is the second op.
+ // Two lookups, one per fetch, and neither is served. The no-store result
+ // contributes no write. The single flush carries only the deferred
+ // fetch's value, which answered without a Cache-Control.
ops := store.Ops()
require.Len(t, ops, 3)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Two lookups (one per fetch, neither served) and NOT ONE write: the | |
| // deferred fetch answered without a Cache-Control and would have written, | |
| // but it never reached the flush because its own result is a fresh miss | |
| // whose write is the second op. | |
| ops := store.Ops() | |
| require.Len(t, ops, 3) | |
| require.Len(t, ops[0].Keys, 1) | |
| key := ops[0].Keys[0] | |
| // Two lookups, one per fetch, and neither is served. The no-store result | |
| // contributes no write. The single flush carries only the deferred | |
| // fetch's value, which answered without a Cache-Control. | |
| ops := store.Ops() | |
| require.Len(t, ops, 3) | |
| require.Len(t, ops[0].Keys, 1) | |
| key := ops[0].Keys[0] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@execution/cachingtesting/cache_control_e2e_test.go` around lines 228 - 235,
Update the comment above the ops assertions to state that there are two lookups,
neither served, followed by one flush carrying only the deferred fetch’s value;
remove the claim that there was no write and correct any reference to the
write’s position from the second op to the flush represented by ops[2].
| ops := store.Ops() | ||
| writes := ops[len(ops)-1] | ||
| assert.Equal(t, "SetMany", writes.Kind) | ||
| assert.Equal(t, [][]string{ | ||
| { | ||
| "subgraph:0", | ||
| "type:0:Query", | ||
| }, | ||
| { | ||
| "subgraph:1", | ||
| "type:1:Product", | ||
| "entity:1:Product:d3cc039c7a9789e7", // upc "1" | ||
| }, | ||
| }, [][]string{writes.Items[0].Tags, writes.Items[1].Tags}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the op-log indexing before you read it.
ops[len(ops)-1] panics when store.Ops() is empty. writes.Items[1] panics when the flush carries fewer than two items. A panic aborts the whole package test binary instead of failing this subtest. The other subtests in this file already use require.Len before indexing.
💚 Proposed fix
ops := store.Ops()
+ require.NotEmpty(t, ops)
writes := ops[len(ops)-1]
assert.Equal(t, "SetMany", writes.Kind)
+ require.Len(t, writes.Items, 2)
assert.Equal(t, [][]string{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ops := store.Ops() | |
| writes := ops[len(ops)-1] | |
| assert.Equal(t, "SetMany", writes.Kind) | |
| assert.Equal(t, [][]string{ | |
| { | |
| "subgraph:0", | |
| "type:0:Query", | |
| }, | |
| { | |
| "subgraph:1", | |
| "type:1:Product", | |
| "entity:1:Product:d3cc039c7a9789e7", // upc "1" | |
| }, | |
| }, [][]string{writes.Items[0].Tags, writes.Items[1].Tags}) | |
| ops := store.Ops() | |
| require.NotEmpty(t, ops) | |
| writes := ops[len(ops)-1] | |
| assert.Equal(t, "SetMany", writes.Kind) | |
| require.Len(t, writes.Items, 2) | |
| assert.Equal(t, [][]string{ | |
| { | |
| "subgraph:0", | |
| "type:0:Query", | |
| }, | |
| { | |
| "subgraph:1", | |
| "type:1:Product", | |
| "entity:1:Product:d3cc039c7a9789e7", // upc "1" | |
| }, | |
| }, [][]string{writes.Items[0].Tags, writes.Items[1].Tags}) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@execution/cachingtesting/client_headers_e2e_test.go` around lines 325 - 338,
Guard the op-log accesses in the affected test with require.Len checks before
indexing: require at least one operation before reading ops[len(ops)-1], then
require at least two items before accessing writes.Items[0] and writes.Items[1].
Follow the existing require.Len pattern used by the other subtests so failures
remain scoped to the subtest.
| store.Seed("v1:1:4f796e3bbd360fce", | ||
| []byte(`{"data":{"__typename":"Product","stock":999},"cc":{"ttl":60,"created":1785852117,"scope":"private"}}`), | ||
| time.Minute) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Derive the seeded created value from the current clock.
The seeded envelope pins "created":1785852117. That epoch is fixed and already in the past, and the envelope declares "ttl":60. The seeded entry is therefore both scope-mismatched and past its own declared freshness window, while the store TTL (time.Minute) keeps it readable.
The assertion at Line 273 only records a ScopeMismatch, so the test passes today. The pass depends on the controller evaluating scope before freshness. If that order ever changes, this fixture fails for a reason unrelated to privacy.
Seed a created value that is fresh relative to the declared TTL. The fixture then isolates the scope mismatch as the single cause of the discard.
🧪 Proposed fix to make the seeded entry fresh
- store.Seed("v1:1:4f796e3bbd360fce",
- []byte(`{"data":{"__typename":"Product","stock":999},"cc":{"ttl":60,"created":1785852117,"scope":"private"}}`),
- time.Minute)
+ store.Seed("v1:1:4f796e3bbd360fce",
+ []byte(fmt.Sprintf(
+ `{"data":{"__typename":"Product","stock":999},"cc":{"ttl":60,"created":%d,"scope":"private"}}`,
+ time.Now().Unix())),
+ time.Minute)Add "fmt" to the import block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| store.Seed("v1:1:4f796e3bbd360fce", | |
| []byte(`{"data":{"__typename":"Product","stock":999},"cc":{"ttl":60,"created":1785852117,"scope":"private"}}`), | |
| time.Minute) | |
| store.Seed("v1:1:4f796e3bbd360fce", | |
| []byte(fmt.Sprintf( | |
| `{"data":{"__typename":"Product","stock":999},"cc":{"ttl":60,"created":%d,"scope":"private"}}`, | |
| time.Now().Unix())), | |
| time.Minute) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@execution/cachingtesting/privacy_e2e_test.go` around lines 255 - 257, Update
the seeded envelope in the privacy E2E test around store.Seed to derive the JSON
"created" timestamp from the current clock, using fmt as needed, so it remains
fresh within the declared 60-second TTL. Preserve the private scope and existing
store TTL, ensuring ScopeMismatch is the fixture’s only discard cause.
| inventoryRule := Rule("", `{"data":{"_entities":[{"__typename":"Product","stock":5}]}}`) | ||
| inventory := Rules(inventoryRule) | ||
| executionEngine := NewEngine(t, inventoryShadowCaching(), Subgraphs{"users": users, "products": products, "inventory": inventory}) | ||
|
|
||
| // Request 1: shadow MISS — plain fetch + write. | ||
| firstObserver := &cachetesting.RecordingObserver{} | ||
| firstBody := Execute(t, executionEngine, query, cachetesting.NewRealishCache(store, firstObserver)) | ||
| assert.Equal(t, `{"data":{"me":{"favoriteProduct":{"upc":"1","stock":5}}}}`, firstBody) | ||
| assert.Equal(t, int64(1), inventory.Requests()) | ||
| assert.Empty(t, firstObserver.Compares()) | ||
|
|
||
| ops := store.Ops() | ||
| require.Len(t, ops, 2) | ||
| require.Len(t, ops[0].Keys, 1) | ||
| key := ops[0].Keys[0] | ||
|
|
||
| // Request 2: L2 HIT under shadow — the subgraph now says stock=7 and the | ||
| // response MUST show 7 (fresh served, never the cached 5); the compare | ||
| // records the mismatch and L2 is overwritten with the fresh value. | ||
| inventoryRule.Response = `{"data":{"_entities":[{"__typename":"Product","stock":7}]}}` | ||
| secondObserver := &cachetesting.RecordingObserver{} | ||
| secondBody := Execute(t, executionEngine, query, cachetesting.NewRealishCache(store, secondObserver)) | ||
| assert.Equal(t, `{"data":{"me":{"favoriteProduct":{"upc":"1","stock":7}}}}`, secondBody) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm SubgraphRule field synchronization and race-detector usage.
set -uo pipefail
echo "=== SubgraphRule definition ==="
ast-grep run --pattern 'type SubgraphRule struct { $$$ }' --lang go .
echo "=== Handler reads of rule.Response / rule.Headers / rule.Gate ==="
rg -nP -C4 '\brule\.(Response|Headers|Gate)\b' --type=go
echo "=== Test-side in-place mutations of a rule's Response field ==="
rg -nP -C3 '\.Response\s*=' --type=go
echo "=== Race detector in CI and Makefiles ==="
fd -H -t f -e yml -e yaml . .github 2>/dev/null | xargs -r rg -n 'go test'
fd -H -t f -g 'Makefile*' . | xargs -r rg -n 'go test'Repository: wundergraph/graphql-go-tools
Length of output: 1075
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== enginetesting.go relevant definitions and handler ==="
sed -n '30,130p' execution/cachingtesting/enginetesting.go
echo "=== All Response assignments ==="
rg -n -C3 '\.Response[[:space:]]*=' --glob '*.go' .
echo "=== Target test context ==="
sed -n '1,100p' execution/cachingtesting/shadow_e2e_test.go
echo "=== Race-detector configuration ==="
rg -n -C2 --glob '*.yml' --glob '*.yaml' --glob 'Makefile*' \
'go test|race|GORACE|-race' .github . 2>/dev/null | head -300
echo "=== Response field access classification ==="
rg -n -C2 --glob '*.go' '\.Response\b|Response:' execution/cachingtestingRepository: wundergraph/graphql-go-tools
Length of output: 24097
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== Execute implementation and response-body handling ==="
rg -n -C12 'func Execute|func NewEngine' execution/cachingtesting --glob '*.go'
echo "=== Call sites that can run the subgraph handler concurrently ==="
rg -n -C8 'Execute\(t|NewRealishCache|httptest.NewServer' execution/cachingtesting --glob '*.go'
echo "=== Standalone race-detector probe for the same HTTP access pattern ==="
cat >/tmp/http_rule_race_test.go <<'EOF'
package main
import (
"io"
"net/http"
"net/http/httptest"
"sync"
)
type rule struct{ response string }
func main() {
r := &rule{response: "old"}
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
_, _ = w.Write([]byte(r.response))
}))
defer s.Close()
var wg sync.WaitGroup
for i := 0; i < 1000; i++ {
wg.Add(1)
go func(i int) {
defer wg.Done()
resp, err := http.Get(s.URL)
if err != nil {
return
}
_, _ = io.ReadAll(resp.Body)
_ = resp.Body.Close()
if i%2 == 0 {
r.response = "new"
}
}(i)
}
wg.Wait()
}
EOF
if command -v go >/dev/null 2>&1; then
go run -race /tmp/http_rule_race_test.go 2>&1 | head -80
else
echo "go is unavailable"
fi
rm -f /tmp/http_rule_race_test.goRepository: wundergraph/graphql-go-tools
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -uo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-tnodmf
echo "=== Standalone probe result ==="
rg -n -C3 'WARNING: DATA RACE|Found [0-9]+ data race|go is unavailable|fatal error|race' "$log" || true
tail -80 "$log"
echo "=== Exact Execute implementation ==="
sed -n '178,199p' execution/cachingtesting/enginetesting.go
echo "=== HTTP client body-read path used by execution tests ==="
rg -n -C8 'io.ReadAll|ReadAll|response.Body|http.NewRequest|client.Do' \
v2/pkg/engine execution --glob '*.go' | head -240Repository: wundergraph/graphql-go-tools
Length of output: 34012
🏁 Script executed:
#!/bin/bash
set -uo pipefail
cat >/tmp/http_rule_order.go <<'EOF'
package main
import (
"io"
"net/http"
"net/http/httptest"
)
type rule struct{ response string }
func main() {
r := &rule{response: "old"}
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
_, _ = w.Write([]byte(r.response))
}))
defer s.Close()
for i := 0; i < 1000; i++ {
resp, err := http.Get(s.URL)
if err != nil {
panic(err)
}
if _, err = io.ReadAll(resp.Body); err != nil {
panic(err)
}
_ = resp.Body.Close()
// Match the target test: mutate only after the complete response body
// has been read, then issue the next request.
if i == 0 {
r.response = "new"
}
}
}
EOF
if command -v go >/dev/null 2>&1; then
go run -race /tmp/http_rule_order.go 2>&1
else
echo "go is unavailable"
fi
rm -f /tmp/http_rule_order.go
echo "=== HTTP client response-body read ==="
sed -n '234,270p' v2/pkg/engine/datasource/httpclient/nethttpclient.goRepository: wundergraph/graphql-go-tools
Length of output: 1306
Synchronize SubgraphRule.Response before mutating it. The handler reads this plain string, while line 56 writes it from the test goroutine. Execute waits for the HTTP body but does not synchronize access to this field. Protect the access with a mutex or atomic accessor, or select responses through a synchronized per-request mechanism.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@execution/cachingtesting/shadow_e2e_test.go` around lines 37 - 59,
Synchronize access to inventoryRule.Response between the request handler and the
test’s mutation before the second Execute call. Update the SubgraphRule response
read/write path, or use a synchronized per-request response mechanism, so the
handler’s read cannot race with the test goroutine’s assignment while preserving
the stock=7 response for the second request.
| func renderCacheKey(prefix string, payload []byte) string { | ||
| preimage := make([]byte, 0, len(prefix)+1+len(payload)) | ||
| preimage = append(preimage, prefix...) | ||
| preimage = append(preimage, ':') | ||
| preimage = append(preimage, payload...) | ||
| return prefix + ":" + hashHex(preimage) | ||
| } | ||
|
|
||
| func hashHex(value []byte) string { | ||
| h := pool.Hash64.Get() | ||
| _, _ = h.Write(value) | ||
| sum := h.Sum64() | ||
| pool.Hash64.Put(h) | ||
| return hex64(sum) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Consider a collision-resistant digest for L2 keys.
renderCacheKey folds the whole preimage into 64 bits with xxhash64. xxhash is not collision resistant. An attacker who controls a representation value (for example an id or an argument bound to a variable) can compute a second value that produces the same digest. Both entities then share one L2 entry, so the attacker can place chosen data under the key a legitimate entity reads.
The partition segment already uses SHA-256 for the same class of reason (see the comment on sha256Hex). Consider one of:
- Truncate a SHA-256 (or keyed xxhash with a process-wide secret seed) to 128 bits for the L2 digest.
- Keep xxhash64 and document that the store must not be shared with untrusted key material.
This changes the key layout, so it belongs with cacheFormatVersion v1 before entries exist in production.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/pkg/engine/cache/cache_key_template.go` around lines 391 - 405, Update
renderCacheKey and its hashHex helper to use a collision-resistant 128-bit
digest, preferably truncated SHA-256 consistent with sha256Hex, instead of
unkeyed xxhash64; preserve the existing prefix/preimage structure while
producing the wider digest. Bump cacheFormatVersion to v1 so the changed key
layout is established before production entries exist.
| // Ops returns a copy of the ordered operation log. | ||
| func (s *FakeStore) Ops() []StoreOp { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| return slices.Clone(s.ops) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deep-copy nested operation data.
Ops only clones the outer []StoreOp. NormalizeStoreOpsClock modifies Items[i].Value in its returned value. That mutation also changes the retained log.
Clone Keys, Hits, Items, and each item's Tags before returning. This keeps later assertions independent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/pkg/engine/cache/cachetesting/fakes.go` around lines 342 - 347, Update
FakeStore.Ops to deep-copy each StoreOp before returning, including the Keys,
Hits, and Items slices and every item's Tags slice. Preserve the existing
locking and return a cloned operation log so NormalizeStoreOpsClock mutations
cannot alter s.ops.
| func coversNode(ctx *resolve.Context, value *astjson.Value, node resolve.Node) bool { | ||
| switch typed := node.(type) { | ||
| case *resolve.Scalar: | ||
| return value.Type() != astjson.TypeNull || typed.Nullable | ||
| case *resolve.Object: | ||
| if value.Type() == astjson.TypeNull { | ||
| return typed.Nullable | ||
| } | ||
| if value.Type() != astjson.TypeObject { | ||
| return false | ||
| } | ||
| return covers(ctx, value, typed) | ||
| case *resolve.Array: | ||
| if value.Type() == astjson.TypeNull { | ||
| return typed.Nullable | ||
| } | ||
| if value.Type() != astjson.TypeArray { | ||
| return false | ||
| } | ||
| if typed.Item == nil { | ||
| return true | ||
| } | ||
| items, err := value.Array() | ||
| if err != nil { | ||
| return false | ||
| } | ||
| for _, item := range items { | ||
| if !coversNode(ctx, item, typed.Item) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What node kinds does the ProvidesData builder produce?
fd -t f 'cache_provides_data_visitor.go|fetch_cache_configurator.go' v2/pkg/engine \
--exec rg -n -C3 'resolve\.(Scalar|String|Boolean|Integer|Float|BigInt|Null|StaticString|CustomNode|Object|Array)\{'
# All Node implementations in resolve.
ast-grep run --pattern 'func ($_ *$N) NodeKind() NodeKind { $$$ }' --lang go v2/pkg/engine/resolveRepository: wundergraph/graphql-go-tools
Length of output: 1843
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- builder symbols and createFieldValue ---'
rg -n -C6 'func \(v \*cacheProvidesDataVisitor\) (createFieldValue|visit|enter)|createFieldValue|ProvidesData' v2/pkg/engine/cache v2/pkg/engine
echo '--- resolve node declarations ---'
rg -n -C5 'type (Scalar|String|Boolean|Integer|Float|BigInt|Null|StaticString|CustomNode|Object|Array) struct|NodeKind\(\)' v2/pkg/engine/resolve
echo '--- all builder node constructors ---'
rg -n -C3 '&resolve\.(Scalar|String|Boolean|Integer|Float|BigInt|Null|StaticString|CustomNode|Object|Array)\b' v2/pkg/engineRepository: wundergraph/graphql-go-tools
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- matching cache files ---'
fd -t f . v2/pkg/engine/cache | rg 'cache_provides_data_visitor\.go|fetch_cache_configurator\.go|coverage\.go'
echo '--- builder file and createFieldValue ---'
builder=$(fd -t f 'cache_provides_data_visitor.go' v2/pkg/engine | head -n1)
echo "$builder"
rg -n -C8 'createFieldValue|&resolve\.(Scalar|String|Boolean|Integer|Float|BigInt|Null|StaticString|CustomNode|Object|Array)\b' "$builder"
echo '--- resolve node declarations ---'
rg -l '^type (Scalar|String|Boolean|Integer|Float|BigInt|Null|StaticString|CustomNode|Object|Array) struct' v2/pkg/engine/resolve \
| while read -r f; do
echo "FILE: $f"
rg -n -C4 '^type (Scalar|String|Boolean|Integer|Float|BigInt|Null|StaticString|CustomNode|Object|Array) struct|func \([^)]*\) NodeKind\(\)' "$f"
doneRepository: wundergraph/graphql-go-tools
Length of output: 7971
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
builder = Path("v2/pkg/engine/plan/cache_provides_data_visitor.go").read_text()
coverage = Path("v2/pkg/engine/cache/coverage.go").read_text()
start = builder.index("func (v *cacheProvidesDataVisitor) createFieldValue")
end = builder.index("\n}\n", start) + 2
# Include nested braces by scanning from the function's opening brace.
opening = builder.index("{", start)
depth = 0
for i in range(opening, len(builder)):
if builder[i] == "{":
depth += 1
elif builder[i] == "}":
depth -= 1
if depth == 0:
end = i + 1
break
body = builder[start:end]
produced = sorted(set(re.findall(r"&resolve\.([A-Za-z]+)\s*\{", body)))
handled = sorted(set(re.findall(r"case \*resolve\.([A-Za-z]+):", coverage)))
print("createFieldValue constructors:", ", ".join(produced))
print("coversNode handled cases:", ", ".join(handled))
print("unhandled constructors:", ", ".join(sorted(set(produced) - set(handled))))
PYRepository: wundergraph/graphql-go-tools
Length of output: 302
Handle the *resolve.Null fallback in coversNode
createFieldValue emits *resolve.Null for unresolved or unsupported schema types, but coversNode handles only Scalar, Object, and Array. The fallback therefore makes the affected data permanently uncoverable. Add an explicit *resolve.Null branch or prevent the builder from emitting this node.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/pkg/engine/cache/coverage.go` around lines 67 - 102, Update coversNode to
explicitly handle *resolve.Null, matching the fallback nodes emitted by
createFieldValue for unresolved or unsupported schema types. Define the branch
so these nodes can be considered covered instead of falling through to the
default false result; do not alter the existing Scalar, Object, or Array
behavior.
|
|
||
| // CacheConfig returns the per-fetch cache config; nil means "not cached". | ||
| // All caching code reads it through this method (never via a switch over | ||
| // concrete fetch types). | ||
| CacheConfig() *FetchCacheConfig | ||
|
|
||
| // SetCacheConfig sets the per-fetch cache config; the caching planner passes | ||
| // call it after the concrete fetch types are created. | ||
| SetCacheConfig(cfg *FetchCacheConfig) | ||
|
|
||
| // IsEntityFetch reports whether this is a single-entity fetch (an _entities | ||
| // fetch on an object field). | ||
| IsEntityFetch() bool | ||
|
|
||
| // IsBatchEntityFetch reports whether this is a batched entity fetch (an | ||
| // _entities fetch over array items). | ||
| IsBatchEntityFetch() bool | ||
|
|
||
| // RepresentationInputTemplate returns the input template holding the | ||
| // fetch's `representations` element — the ResolvableObjectVariable segment | ||
| // whose node IS the merged representation the fetch sends. It is nil for | ||
| // fetches that send no representation (plain single fetches). Caching reads | ||
| // it through this method (never via a switch over concrete fetch types). | ||
| RepresentationInputTemplate() *InputTemplate | ||
|
|
||
| // LoadTrace returns the fetch's ART trace destination; nil when tracing is | ||
| // disabled for the request. Caching/observability code reads it through | ||
| // this method (never via a switch over concrete fetch types). | ||
| LoadTrace() *DataSourceLoadTrace | ||
|
|
||
| // SetDataSource replaces the fetch's transport, e.g. to swap in an | ||
| // in-process fake; it exists so no caller needs a switch over concrete | ||
| // fetch types. | ||
| SetDataSource(ds DataSource) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all types implementing resolve.Fetch by locating FetchKind() implementations.
rg -nP -C 3 'func \([a-zA-Z_]+ \*?\w+\) FetchKind\(\)' --type=go
# Check which of them declare the new methods.
rg -nP 'func \([a-zA-Z_]+ \*?\w+\) (CacheConfig|SetCacheConfig|IsEntityFetch|IsBatchEntityFetch|RepresentationInputTemplate|LoadTrace|SetDataSource)\(' --type=goRepository: wundergraph/graphql-go-tools
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Fetch interface and related declarations ---'
rg -n -C 8 'type Fetch interface|FetchKind\(\)|CacheConfig\(\)|RepresentationInputTemplate\(\)' --glob '*.go' .
printf '%s\n' '--- Concrete fetch types and compile-time interface assertions ---'
rg -n -C 4 'type .*Fetch|resolve\.Fetch|Fetch\)' --glob '*.go' .Repository: wundergraph/graphql-go-tools
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Fetch interface ---'
cat -n v2/pkg/engine/resolve/fetch.go | sed -n '1,110p'
printf '%s\n' '--- FetchKind declarations and usages ---'
rg -n 'FetchKind' --glob '*.go' . | head -n 300
printf '%s\n' '--- Implementations of the added methods ---'
rg -n 'func .* (CacheConfig|SetCacheConfig|IsEntityFetch|IsBatchEntityFetch|RepresentationInputTemplate|LoadTrace|SetDataSource)\(' --glob '*.go' . | head -n 300
printf '%s\n' '--- Module and release-note files ---'
sed -n '1,80p' go.mod 2>/dev/null || true
find . -maxdepth 3 -type f \( -iname '*changelog*' -o -iname '*release*' -o -iname '*news*' \) -printRepository: wundergraph/graphql-go-tools
Length of output: 11177
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Current v2 changelog structure ---'
sed -n '1,100p' v2/CHANGELOG.md
printf '%s\n' '--- Public Fetch references outside implementation files ---'
rg -n 'resolve\.Fetch|Fetch interface|type .*Fetch interface' \
--glob '!v2/pkg/engine/resolve/fetch.go' \
--glob '!**/*_test.go' \
--glob '*.go' --glob '*.md' . | head -n 300
printf '%s\n' '--- Fetch interface assignments and function parameters ---'
rg -n 'Fetch[[:space:]]*=' --glob '*.go' v2/pkg/engine/resolve
rg -n '\bFetch\b' --glob '*.go' v2/pkg/engine/resolve | grep -vE 'FetchKind|SingleFetch|BatchEntityFetch|EntityFetch|fetch\.go' | head -n 300Repository: wundergraph/graphql-go-tools
Length of output: 50384
Avoid widening the exported resolve.Fetch interface. The interface now requires seven methods, not six. Repository implementations and test stubs are updated, but downstream implementations will fail to compile. Keep the interface stable or document this breaking API change in v2/CHANGELOG.md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/pkg/engine/resolve/fetch.go` around lines 26 - 59, Keep the exported
resolve.Fetch interface backward-compatible by removing the newly added
requirements from the interface, or document the intentional breaking change in
v2/CHANGELOG.md if those methods must remain. Update the interface declaration
around CacheConfig, SetCacheConfig, IsEntityFetch, IsBatchEntityFetch,
RepresentationInputTemplate, LoadTrace, and SetDataSource without changing
unrelated behavior.
| // EndRequest runs after the request arenas are released; it is arena-free | ||
| // by contract (see RequestCache.EndRequest). | ||
| defer ctx.endCacheRequest() | ||
|
|
||
| // Marked before the first fetch, which is what lazily begins the request's | ||
| // cache surface: parts of this response resolve after the initial frame is | ||
| // written, so no client cache answer may be computed for it. | ||
| ctx.deferredResponse = true | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Flush cache traces before the initial deferred frame.
The initial frame renders at Lines 592-603. ctx.endCacheRequest() runs only after the deferred response completes. Unlike Lines 398-402 and Lines 479-484, this path does not call ctx.flushCacheTraces() before the trace extension renders. Cache events from the initial fetch therefore cannot appear in that response trace.
Proposed fix
resolvable.deferMode = true
resolvable.currentDefer = nil
resolvable.deferDescriptors = response.DeferDescriptors
+ if ctx.TracingOptions.Enable && ctx.TracingOptions.IncludeTraceOutputInResponseExtensions {
+ ctx.flushCacheTraces()
+ }
+
// render initial response
err = resolvable.Resolve(ctx.ctx, response.Response.Data, response.Response.Fetches, writer)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/pkg/engine/resolve/resolve.go` around lines 529 - 537, Update the
deferred-response flow around ctx.deferredResponse and the initial frame
rendering to call ctx.flushCacheTraces() before the trace extension is rendered,
ensuring cache events from the initial fetch are included in that response
trace. Keep ctx.endCacheRequest() deferred until request processing completes.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (3)
v2/pkg/engine/cache/observer.go (1)
144-176: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find every construction and consumer of TraceObserver, and check for any reset/drain of its recorded slices. rg -nP -C5 '\bNewTraceObserver\s*\(' rg -nP -C3 '\.(StoreErrors|UncacheablePrivate|ScopeMismatches)\s*\('Repository: wundergraph/graphql-go-tools
Length of output: 166
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- candidate files ---' fd -i 'observer\.go$|traceobserver|observer' . printf '%s\n' '--- symbols and constructors ---' rg -n -C4 'type TraceObserver|NewTraceObserver|func \(.*TraceObserver.*\)(StoreErrors|UncacheablePrivate|ScopeMismatches)|storeErrors|uncacheablePrivate' . printf '%s\n' '--- all observer method calls ---' rg -n -C3 'StoreErrors|UncacheablePrivate|OnStoreError|OnUncacheablePrivate|TraceObserver' .Repository: wundergraph/graphql-go-tools
Length of output: 50384
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- observer implementation ---' sed -n '1,215p' v2/pkg/engine/cache/observer.go printf '%s\n' '--- production references outside the cache package ---' rg -n --glob '*.go' --glob '!**/*_test.go' 'NewTraceObserver|TraceObserver|\.StoreErrors\(|\.UncacheablePrivate\(' . printf '%s\n' '--- request lifecycle and observer interface references ---' rg -n -C4 --glob '*.go' 'CacheObserver|BeginRequest\(|EndRequest\(|OnFetchObserved\(' v2/pkg/engineRepository: wundergraph/graphql-go-tools
Length of output: 50385
🏁 Script executed:
#!/bin/bash set -eu python3 - <<'PY' from pathlib import Path import re tracked = [Path(p) for p in __import__("subprocess").check_output( ["git", "ls-files", "*.go"], text=True ).splitlines()] symbols = { "constructor": re.compile(r"\bNewTraceObserver\s*\("), "store_accessor": re.compile(r"\.StoreErrors\s*\("), "private_accessor": re.compile(r"\.UncacheablePrivate\s*\("), "store_append": re.compile(r"\bstoreErrors\s*=\s*append\s*\("), "private_append": re.compile(r"\buncacheablePrivate\s*=\s*append\s*\("), "store_reset": re.compile(r"\bstoreErrors\s*=\s*(?:nil|[^;\n]+)"), "private_reset": re.compile(r"\buncacheablePrivate\s*=\s*(?:nil|[^;\n]+)"), } for name, pattern in symbols.items(): matches = [] for path in tracked: if path.as_posix() == "v2/pkg/engine/cache/observer.go": continue for lineno, line in enumerate(path.read_text(errors="replace").splitlines(), 1): if pattern.search(line): matches.append(f"{path}:{lineno}:{line.strip()}") print(f"--- {name}: {len(matches)} ---") print("\n".join(matches[:20]) or "(none)") PYRepository: wundergraph/graphql-go-tools
Length of output: 2411
Bound or drain the
TraceObserverevent buffers
TraceObserverserves many concurrent requests.StoreErrors()andUncacheablePrivate()clone their slices without clearing them. Each store failure or private result therefore remains retained for the observer lifetime and can grow memory without bound. Cap both buffers with overflow counts, or make both accessors atomically drain their buffers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/observer.go` around lines 144 - 176, Update TraceObserver’s StoreErrors and UncacheablePrivate accessors to atomically drain their recorded event slices after returning them, clearing the buffers under the existing mutex while preserving occurrence order. Ensure both buffers no longer retain events for the observer lifetime; use the same drain behavior for store errors and uncacheable-private results.v2/pkg/engine/cache/transform.go (1)
58-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find where variable defaults are injected during normalization/variable extraction. rg -nP --type=go -C4 '\b(DefaultValue|defaultValue|InjectDefault|variablesDefault)\w*' v2/pkg/astnormalization | head -80 # Confirm CacheFieldArg carries no literal/default fallback. ast-grep run --pattern 'type CacheFieldArg struct { $$$ }' --lang go v2/pkg/engine/resolveRepository: wundergraph/graphql-go-tools
Length of output: 7696
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- normalization entry points and default handling ---' rg -n -P --type=go -C3 '\b(extractVariablesDefaultValue|VariableValues|Variables|DefaultValue|RemapVariables|normalize.*variable|variable.*default)\b' \ v2/pkg/astnormalization v2/pkg/engine v2/pkg/graphql | head -240 printf '%s\n' '--- cache suffix call graph ---' rg -n -P --type=go -C5 '\b(computeArgSuffix|CacheFieldArg|RemapVariables|Variables\.Get)\b' \ v2/pkg/engine v2/pkg | head -260 printf '%s\n' '--- candidate file outlines ---' for f in $(git ls-files 'v2/pkg/astnormalization/*.go' 'v2/pkg/engine/cache/*.go' 'v2/pkg/engine/resolve/*.go' | head -80); do ast-grep outline "$f" 2>/dev/null | head -80 doneRepository: wundergraph/graphql-go-tools
Length of output: 50384
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- variable normalizer order ---' sed -n '210,285p' v2/pkg/astnormalization/astnormalization.go sed -n '417,475p' v2/pkg/astnormalization/astnormalization.go printf '%s\n' '--- variable-default extraction implementation ---' sed -n '85,125p' v2/pkg/astnormalization/variables_default_value_extraction.go printf '%s\n' '--- argument default handling ---' rg -n -P --type=go -C5 'Argument.*Default|DefaultValue.*Argument|InputValueDefinition.*Default|ArgumentValue|FieldArguments' \ v2/pkg/astnormalization v2/pkg/engine/plan v2/pkg/engine/cache | head -220 printf '%s\n' '--- cache transform and callers ---' cat -n v2/pkg/engine/cache/transform.go | sed -n '1,125p' rg -n -P --type=go -C6 'normalizeToSchema|denormalizeFromSchema|normalizedFieldName|computeArgSuffix|CacheArgs' \ v2/pkg/engine | head -260 printf '%s\n' '--- context construction and variable sources ---' rg -n -P --type=go -C5 'NewContext\(|VariablesHash|Input\.Variables|operation\.Input\.Variables|RemapVariables' \ v2/pkg/engine | head -260Repository: wundergraph/graphql-go-tools
Length of output: 50384
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- all cache transform call sites ---' rg -n -P --type=go 'normalizeToSchema|denormalizeFromSchema|normalizedFieldName|computeArgSuffix' v2/pkg/engine/cache printf '%s\n' '--- context creation and resolver variables ---' rg -n -P --type=go 'NewContext|Variables\s*:' v2/pkg/engine | head -180 rg -n -P --type=go -C4 'Input\.Variables|VariablesHash|RemapVariables' v2/pkg/engine | tail -220 printf '%s\n' '--- normalizer construction and use ---' rg -n -P --type=go -C5 'NewVariablesNormalizer|VariablesNormalizer|NormalizeOperation\(.*definition' v2 | head -220 printf '%s\n' '--- cache argument capture ---' sed -n '210,250p' v2/pkg/engine/plan/cache_provides_data_visitor.goRepository: wundergraph/graphql-go-tools
Length of output: 50384
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- assignments to Context.Variables ---' rg -n -P --type=go -C6 'Variables\s*=' v2/pkg/engine | head -220 printf '%s\n' '--- request variable propagation into fetch contexts ---' rg -n -P --type=go -C8 'VariablesHash|VariablesView|RemapVariables|astjson\.MustParseBytes|astjson\.Parse' \ v2/pkg/engine/datasource v2/pkg/engine/resolve v2/pkg/engine/cache | head -280 printf '%s\n' '--- operation normalization around lines 368-415 ---' sed -n '368,415p' v2/pkg/astnormalization/astnormalization.go printf '%s\n' '--- default injection visitor registration and behavior ---' cat -n v2/pkg/astnormalization/inject_input_default_values.go | sed -n '1,145p' rg -n -P --type=go 'inputFieldDefaultInjectionVisitor|injectInputDefaultValues|With.*Default' v2/pkg/astnormalizationRepository: wundergraph/graphql-go-tools
Length of output: 50384
Preserve omitted and explicit-null argument values in cache keys.
Variable-definition defaults are materialized into
ctx.Variables, but field argument schema defaults are not.computeArgSuffixtherefore hashes an omitted variable asnull, which collides with$v: nulleven though the field default applies only to the omitted case. Hash the absent value with a distinct marker or its effective argument default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/transform.go` around lines 58 - 70, Update computeArgSuffix to distinguish an omitted argument from an explicitly supplied null when resolving ctx.Variables, using a distinct absent-value marker or the argument’s effective schema default instead of hashing omitted values as null. Preserve the existing null serialization for explicitly provided null values and keep variable remapping behavior unchanged.v2/pkg/engine/plan/representationvariable/representation_variable.go (1)
356-417: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find the origin of this logic and any error-returning variant in the planner. rg -nP -C 6 'resolveOnTypeNames' --type=go rg -nP -C 4 'union type' --type=go -g '!**/*_test.go'Repository: wundergraph/graphql-go-tools
Length of output: 166
🏁 Script executed:
#!/bin/bash set -u printf '%s\n' '--- candidate files ---' git ls-files | rg '(^|/)(representation_variable\.go|plan|federation|directive|selection).*\.go$' | head -200 printf '%s\n' '--- resolver references ---' rg -n -P -C 8 'resolveOnTypeNames|InlineFragmentTypeConditionName|NodeIsUnionMember|ObjectTypeDefinitionImplementsInterface' . --glob '*.go' || true printf '%s\n' '--- union and federation validation references ---' rg -n -i -P -C 5 'union type|union.*fragment|fragment.*union|`@key`|`@requires`|RequiredFieldsFragment|cache.key|cache key' . --glob '*.go' | head -500 || trueRepository: wundergraph/graphql-go-tools
Length of output: 50385
🏁 Script executed:
#!/bin/bash set -u printf '%s\n' '--- representation-variable call graph ---' rg -n -P -C 8 'BuildRepresentationVariable|representationVariable|resolveOnTypeNames|RequiredFieldsFragment|Walk\(' \ v2/pkg/engine/plan/representationvariable \ v2/pkg/engine/plan \ --glob '*.go' | head -700 printf '%s\n' '--- field-set parser and validation ---' rg -n -i -P -C 6 'FieldSet|field set|Parse.*Selection|selection set|InlineFragment' \ v2/pkg/federation v2/pkg/engine/plan v2/pkg/astvalidation \ --glob '*.go' | rg -i 'field.?set|key|requires|provides|selection|inline|fragment' | head -700 printf '%s\n' '--- tests containing union fragments in federation field sets ---' rg -n -i -P -C 5 'union|on [A-Za-z_][A-Za-z0-9_]*\s*\{' \ v2/pkg/engine/plan v2/pkg/federation execution --glob '*_test.go' | rg -i 'union|`@key`|`@requires`|`@provides`|on .*\\{' | head -700Repository: wundergraph/graphql-go-tools
Length of output: 50385
🏁 Script executed:
#!/bin/bash set -u printf '%s\n' '--- direct builder call sites ---' rg -n -P -C 12 'BuildRepresentationVariableNode|MergeRepresentationVariableNodes' v2 --glob '*.go' printf '%s\n' '--- field-set validation and inline-fragment handling ---' sed -n '1,180p' v2/pkg/engine/plan/required_fields_visitor.go sed -n '110,190p' v2/pkg/engine/plan/federation_metadata.go sed -n '60,180p' v2/pkg/astvalidation/operation_rule_fragments.go printf '%s\n' '--- focused federation field-set test references ---' rg -n -i -P -C 4 'union|`@key`|`@requires`|SelectionSet:' \ v2/pkg/engine/plan/representationvariable/representation_variable_test.go \ v2/pkg/engine/plan/*_test.go \ v2/pkg/federation/*_test.go \ --glob '*.go' | head -500Repository: wundergraph/graphql-go-tools
Length of output: 50384
🏁 Script executed:
#!/bin/bash set -u printf '%s\n' '--- all inline-fragment field-set literals ---' rg -n -i -P -C 3 '\.\.\.\s*on\s+[A-Za-z_][A-Za-z0-9_]*\s*\{' \ . --glob '*.go' --glob '*.graphql' --glob '*.gql' --glob '*.json' \ | head -500 printf '%s\n' '--- federation configuration construction ---' rg -n -P -C 10 'FederationFieldConfiguration\{|SelectionSet:|RequiredFields:' \ v2/pkg/engine --glob '*.go' | head -700 printf '%s\n' '--- AST walker behavior around field definitions and stop errors ---' rg -n -P -C 8 'func \(.*\) FieldDefinition|FieldDefinition\(ref|StopWithInternalErr|RegisterInlineFragmentVisitor|EnterInlineFragment' \ v2/pkg/astvisitor v2/pkg/engine/plan/representationvariable v2/pkg/engine/plan/visitor.go \ --glob '*.go' | head -600Repository: wundergraph/graphql-go-tools
Length of output: 50385
🏁 Script executed:
#!/bin/bash set -u printf '%s\n' '--- walker implementation ---' fd -i 'walker.*\.go$' v2/pkg/astvisitor v2/pkg | head -50 rg -n -P -C 10 'func \(.*Walker.*Walk|func \(.*Walker.*walk|RegisterFieldVisitor|FieldDefinition\(ref' \ v2/pkg/astvisitor --glob '*.go' | head -800 printf '%s\n' '--- representation visitor complete relevant methods ---' sed -n '193,417p' v2/pkg/engine/plan/representationvariable/representation_variable.go printf '%s\n' '--- union-fragment field-set tests in planner packages ---' rg -n -i -P -C 8 'fragments on union|on [A-Za-z_][A-Za-z0-9_]* \{.*\}|SelectionSet: `\.\.\. on|fieldSet: `\.\.\. on|field set' \ v2/pkg/engine/plan --glob '*_test.go' | head -500Repository: wundergraph/graphql-go-tools
Length of output: 50384
🌐 Web query:
Apollo Federation FieldSet@key@requiresinline fragment on union type condition allowed💡 Result:
In Apollo Federation, the use of inline fragments within a FieldSet—specifically for
@keyand@requiresdirectives—is supported [1]. A FieldSet is a string-serialized representation of a selection set [2]. When referencing fields on a union or interface type within a FieldSet, you can use the GraphQL inline fragment syntax (... on Type { fields }) to specify the fields to be included or required [1]. For example, when defining an entity with@key, you can use an inline fragment if the key is associated with specific types in a union: directive@key(fields: "... on Person { name address }") on OBJECT | INTERFACE This syntax is valid and allows for precise selection of fields within a federated schema [1]. It is important to note, however, that an entity's@keycannot include fields that return a union or interface type itself, nor can they include fields that require arguments [3]. While inline fragments are valid within a FieldSet for directives like@keyand@requires[1], ensure that your subgraph implementation and Apollo Federation version support the specific schema patterns you are employing, as complex usage of unions and interfaces in federated schemas may have implications for composition and entity resolution [4][5].Citations:
- 1: https://github.com/apollographql/federation/blob/main/docs/source/schema-design/federated-schemas/reference/directives.mdx
- 2: https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/reference/subgraph-spec.md
- 3: https://www.apollographql.com/docs/deploy-preview/7e5a48c46d3088f1d1b3ace7/graphos/schema-design/federated-schemas/entities/intro
- 4: https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/reference/subgraph-spec
- 5: https://www.apollographql.com/docs/deploy-preview/c3409d2c8246a30205d17d8c/graphos/schema-design/federated-schemas/sharing-types/
🌐 Web query:
site:apollographql.com/docs federation@keyfields union interface cannot include fields returning union interface@requiresFieldSet💡 Result:
In Apollo Federation, there is a fundamental restriction regarding the fields used within an entity's
@keydirective: they cannot include fields that return a union or interface type [1]. This limitation exists because the@keymust provide a reliable, unique identifier for an entity, and resolving unions or interfaces in a key definition would create ambiguity or complexity that the federation gateway cannot reliably manage [1]. Specifically, the following constraints apply to the fields argument of the@keydirective: - It cannot include fields that return a union or interface type [1]. - It cannot include fields that take arguments [1]. While you mentioned@requiresin your query, it is important to distinguish its behavior from@key. The@requiresdirective, used to annotate fields that depend on other fields, is more flexible than@key. For instance, you can use selection set syntax (e.g., "... on Person { name address }") with@requireswhen dealing with fields that return a union or interface [2][3][4]. Additionally, recent versions of Apollo Federation have significantly expanded the capabilities of@requires, allowing it to support fields that themselves take arguments (since v2.1.2) [5]. If you are encountering a composition error related to these directives, ensure that your@keyselection set consists only of scalar or enum-like fields that unambiguously identify the entity [1]. Fields that return unions or interfaces must be handled through other means, such as by exposing specific scalar fields that serve as the unique identifier.Citations:
- 1: https://www.apollographql.com/docs/deploy-preview/7ea9eade22b84c255014efa5/graphos/schema-design/federated-schemas/entities/intro
- 2: https://www.apollographql.com/docs/graphos/schema-design/federated-schemas/reference/directives
- 3: https://www.apollographql.com/docs/deploy-preview/99c5ddea7bd65ef8d9f261ad/graphos/schema-design/federated-schemas/reference/directives
- 4: https://www.apollographql.com/docs/deploy-preview/cd3435bc7bf7f372c0649aaf/graphos/schema-design/federated-schemas/reference/directives
- 5: https://www.apollographql.com/docs/deploy-preview/bede6912468739aecb1ad1cb/graphos/schema-design/federated-schemas/reference/versions
Return a planning error for union type conditions instead of panicking.
@requiresfield sets can use inline fragments on union fields, and this walker reachesEnterFieldfor those selections. Replace the panic withStopWithInternalErr(...), or handle union members asplan.Visitor.resolveOnTypeNamesdoes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/representationvariable/representation_variable.go` around lines 356 - 417, The resolveOnTypeNames method must not panic when the inline fragment type condition resolves to a union. Replace the union branch’s panic with the established StopWithInternalErr(...) planning-error path, or reuse the union-member handling from plan.Visitor.resolveOnTypeNames, while preserving the existing interface and concrete-type behavior.
…hape key direction Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/caching/6-open-issues.md`:
- Around line 41-44: Update the caching documentation sentence to distinguish
shared selection-digest variants from argument values: aliases, field order, and
fragment variants may share the selection digest, but rendered argument values
must remain in the full cache key and produce separate entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03a1aa43-ea0c-4949-8c66-8d6ce6b2b92a
📒 Files selected for processing (2)
docs/caching/5-testing-on-js-subgraphs.mddocs/caching/6-open-issues.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/caching/5-testing-on-js-subgraphs.md
| Because the digest is over normalized schema names, | ||
| alias/order/fragment variants and argument-value variants still share entries | ||
| (better than Apollo's raw query-shape hash), | ||
| and the entity tag still covers all shape variants, so one purge clears them all. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep rendered argument values in the full cache key.
Line 42 says that argument-value variants share entries. That is unsafe. v2/pkg/engine/cache/cache_key_args_test.go includes an args hash in the key preimage, and v2/pkg/engine/cache/controller_rootfield_test.go requires different variable values to produce different keys.
State that aliases, field order, and fragment variants share the selection digest. Different rendered argument values must use different entries.
Proposed wording
-Because the digest is over normalized schema names,
-alias/order/fragment variants and argument-value variants still share entries
-(better than Apollo's raw query-shape hash),
+Because the digest is over normalized schema names,
+alias/order/fragment variants share the same selection digest, while rendered
+argument values remain in the full cache key and use different entries
+(better than Apollo's raw query-shape hash),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/caching/6-open-issues.md` around lines 41 - 44, Update the caching
documentation sentence to distinguish shared selection-digest variants from
argument values: aliases, field order, and fragment variants may share the
selection digest, but rendered argument values must remain in the full cache key
and produce separate entries.
1 participant
Squash of the full tmp-caching series plus follow-ups: envelope types map removed, key format v1, comment hygiene, and the docs/caching documentation set.
@coderabbitai summary
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.