Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
295 changes: 295 additions & 0 deletions v2/pkg/engine/resolve/cache_load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1976,6 +1976,301 @@
})
}

// ErrorLoaderCache wraps FakeLoaderCache but returns errors on Get/Set calls
// when configured to do so. Used for testing L2 error resilience.
type ErrorLoaderCache struct {
*FakeLoaderCache

Check failure on line 1982 in v2/pkg/engine/resolve/cache_load_test.go

View workflow job for this annotation

GitHub Actions / Linters (1.25)

there must be an empty line separating embedded fields from regular fields (embeddedstructfieldcheck)
getErr error
setErr error
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func (e *ErrorLoaderCache) Get(ctx context.Context, keys []string) ([]*CacheEntry, error) {
if e.getErr != nil {
return nil, e.getErr
}
return e.FakeLoaderCache.Get(ctx, keys)
}

func (e *ErrorLoaderCache) Set(ctx context.Context, entries []*CacheEntry, ttl time.Duration) error {
if e.setErr != nil {
return e.setErr
}
return e.FakeLoaderCache.Set(ctx, entries, ttl)
Comment on lines +1988 to +1999

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Record injected cache failures in the wrapper.

Get/Set return before updating FakeLoaderCache.log, so the resilience subtests cannot prove the L2 path was actually exercised. As written, they still pass if the loader skips L2 entirely.

Suggested change
 func (e *ErrorLoaderCache) Get(ctx context.Context, keys []string) ([]*CacheEntry, error) {
 	if e.getErr != nil {
+		e.mu.Lock()
+		e.log = append(e.log, CacheLogEntry{
+			Operation: "get",
+			Keys:      append([]string(nil), keys...),
+			Hits:      make([]bool, len(keys)),
+		})
+		e.mu.Unlock()
 		return nil, e.getErr
 	}
 	return e.FakeLoaderCache.Get(ctx, keys)
 }
 
 func (e *ErrorLoaderCache) Set(ctx context.Context, entries []*CacheEntry, ttl time.Duration) error {
 	if e.setErr != nil {
+		keys := make([]string, 0, len(entries))
+		for _, entry := range entries {
+			if entry != nil {
+				keys = append(keys, entry.Key)
+			}
+		}
+		e.mu.Lock()
+		e.log = append(e.log, CacheLogEntry{
+			Operation: "set",
+			Keys:      keys,
+		})
+		e.mu.Unlock()
 		return e.setErr
 	}
 	return e.FakeLoaderCache.Set(ctx, entries, ttl)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/engine/resolve/cache_load_test.go` around lines 1988 - 1999, The
wrapper ErrorLoaderCache currently returns early when e.getErr or e.setErr is
set, which prevents FakeLoaderCache.log from being updated and hides whether L2
was exercised; change ErrorLoaderCache.Get and ErrorLoaderCache.Set to always
invoke the underlying FakeLoaderCache.Get/Get or Set (to allow it to record to
FakeLoaderCache.log) and then, if e.getErr or e.setErr is non-nil, return that
injected error (i.e., call FakeLoaderCache.Get/Set first to record the
operation, capture its result, then override the returned error with
e.getErr/e.setErr before returning).

}

// buildProductEntityResponse creates a GraphQLResponse for a single product entity fetch.
// Used by error resilience and mutation skip tests to avoid repeating boilerplate.
func buildProductEntityResponse(rootDS, entityDS DataSource, cacheKeyTemplate CacheKeyTemplate, providesData *Object, operationType ast.OperationType) *GraphQLResponse {
rootOpName := "query"
rootFieldType := "Query"
rootFieldName := "product"
if operationType == ast.OperationTypeMutation {
rootOpName = "mutation"
rootFieldType = "Mutation"
rootFieldName = "updateUser"
}

return &GraphQLResponse{
Info: &GraphQLResponseInfo{OperationType: operationType},
Fetches: Sequence(
SingleWithPath(&SingleFetch{
FetchConfiguration: FetchConfiguration{
DataSource: rootDS,
PostProcessing: PostProcessingConfiguration{SelectResponseDataPath: []string{"data"}},
},
InputTemplate: InputTemplate{Segments: []TemplateSegment{
{Data: []byte(`{"method":"POST"}`), SegmentType: StaticSegmentType},
}},
DataSourceIdentifier: []byte("graphql_datasource.Source"),
Info: &FetchInfo{
DataSourceID: "ds", DataSourceName: "ds",
RootFields: []GraphCoordinate{{TypeName: rootFieldType, FieldName: rootFieldName}},
OperationType: operationType,
},
}, rootOpName),
SingleWithPath(&SingleFetch{
FetchConfiguration: FetchConfiguration{
DataSource: entityDS,
PostProcessing: PostProcessingConfiguration{SelectResponseDataPath: []string{"data", "_entities", "0"}},
Caching: FetchCacheConfiguration{
Enabled: true,
CacheName: "default",
TTL: 30 * time.Second,
CacheKeyTemplate: cacheKeyTemplate,
UseL1Cache: true,
},
},
InputTemplate: InputTemplate{Segments: []TemplateSegment{
{Data: []byte(`{"method":"POST","url":"http://ds.service","body":{"query":"...","variables":{"representations":[`), SegmentType: StaticSegmentType},
{SegmentType: VariableSegmentType, VariableKind: ResolvableObjectVariableKind, Renderer: NewGraphQLVariableResolveRenderer(&Object{
Fields: []*Field{
{Name: []byte("__typename"), Value: &String{Path: []string{"__typename"}}},
{Name: []byte("id"), Value: &String{Path: []string{"id"}}},
},
})},
{Data: []byte(`]}}}`), SegmentType: StaticSegmentType},
}},
DataSourceIdentifier: []byte("graphql_datasource.Source"),
Info: &FetchInfo{
DataSourceID: "ds", DataSourceName: "ds",
RootFields: []GraphCoordinate{{TypeName: "Product", FieldName: "name"}},
OperationType: ast.OperationTypeQuery, ProvidesData: providesData,
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}, rootOpName+"."+rootFieldName, ObjectPath(rootFieldName)),
),
Data: &Object{
Fields: []*Field{{
Name: []byte(rootFieldName),
Value: &Object{
Path: []string{rootFieldName},
Fields: []*Field{
{Name: []byte("id"), Value: &String{Path: []string{"id"}}},
{Name: []byte("name"), Value: &String{Path: []string{"name"}}},
},
},
}},
},
}
}

func TestL2CacheErrorResilience(t *testing.T) {
productCacheKeyTemplate := &EntityQueryCacheKeyTemplate{
Keys: NewResolvableObjectVariable(&Object{
Fields: []*Field{
{Name: []byte("__typename"), Value: &String{Path: []string{"__typename"}}},
{Name: []byte("id"), Value: &String{Path: []string{"id"}}},
},
}),
}
providesData := &Object{
Fields: []*Field{
{Name: []byte("name"), Value: &Scalar{}},
},
}

t.Run("L2 Get error falls through to fetch", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

errorCache := &ErrorLoaderCache{
FakeLoaderCache: NewFakeLoaderCache(),
getErr: assert.AnError,
}

rootDS := NewMockDataSource(ctrl)
rootDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"product":{"__typename":"Product","id":"prod-1"}}}`), nil
}).Times(1)

entityDS := NewMockDataSource(ctrl)
entityDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"_entities":[{"__typename":"Product","id":"prod-1","name":"Product One"}]}}`), nil
}).Times(1)

response := buildProductEntityResponse(rootDS, entityDS, productCacheKeyTemplate, providesData, ast.OperationTypeQuery)

loader := &Loader{caches: map[string]LoaderCache{"default": errorCache}}
ctx := NewContext(t.Context())
ctx.ExecutionOptions.DisableSubgraphRequestDeduplication = true
ctx.ExecutionOptions.Caching.EnableL1Cache = true
ctx.ExecutionOptions.Caching.EnableL2Cache = true

ar := arena.NewMonotonicArena(arena.WithMinBufferSize(1024))
resolvable := NewResolvable(ar, ResolvableOptions{})
err := resolvable.Init(ctx, nil, ast.OperationTypeQuery)
require.NoError(t, err)

err = loader.LoadGraphQLResponseData(ctx, response, resolvable)
require.NoError(t, err)

out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors)
assert.Equal(t, `{"data":{"product":{"__typename":"Product","id":"prod-1","name":"Product One"}}}`, out)
})

t.Run("L2 Set error does not fail request", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

errorCache := &ErrorLoaderCache{
FakeLoaderCache: NewFakeLoaderCache(),
setErr: assert.AnError,
}

rootDS := NewMockDataSource(ctrl)
rootDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"product":{"__typename":"Product","id":"prod-1"}}}`), nil
}).Times(1)

entityDS := NewMockDataSource(ctrl)
entityDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"_entities":[{"__typename":"Product","id":"prod-1","name":"Product One"}]}}`), nil
}).Times(1)

response := buildProductEntityResponse(rootDS, entityDS, productCacheKeyTemplate, providesData, ast.OperationTypeQuery)

loader := &Loader{caches: map[string]LoaderCache{"default": errorCache}}
ctx := NewContext(t.Context())
ctx.ExecutionOptions.DisableSubgraphRequestDeduplication = true
ctx.ExecutionOptions.Caching.EnableL1Cache = true
ctx.ExecutionOptions.Caching.EnableL2Cache = true

ar := arena.NewMonotonicArena(arena.WithMinBufferSize(1024))
resolvable := NewResolvable(ar, ResolvableOptions{})
err := resolvable.Init(ctx, nil, ast.OperationTypeQuery)
require.NoError(t, err)

err = loader.LoadGraphQLResponseData(ctx, response, resolvable)
require.NoError(t, err)

out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors)
assert.Equal(t, `{"data":{"product":{"__typename":"Product","id":"prod-1","name":"Product One"}}}`, out)
})

t.Run("corrupted cache entry treated as miss", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

cache := NewFakeLoaderCache()
// Pre-populate cache with corrupted JSON
_ = cache.Set(t.Context(), []*CacheEntry{
{Key: "Product:prod-1", Value: []byte(`{not valid json!!!}`)},
}, 30*time.Second)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

rootDS := NewMockDataSource(ctrl)
rootDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"product":{"__typename":"Product","id":"prod-1"}}}`), nil
}).Times(1)

entityDS := NewMockDataSource(ctrl)
entityDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"_entities":[{"__typename":"Product","id":"prod-1","name":"Product One"}]}}`), nil
}).Times(1) // Must fetch because cached entry is corrupted

response := buildProductEntityResponse(rootDS, entityDS, productCacheKeyTemplate, providesData, ast.OperationTypeQuery)

loader := &Loader{caches: map[string]LoaderCache{"default": cache}}
ctx := NewContext(t.Context())
ctx.ExecutionOptions.DisableSubgraphRequestDeduplication = true
ctx.ExecutionOptions.Caching.EnableL1Cache = true
ctx.ExecutionOptions.Caching.EnableL2Cache = true

ar := arena.NewMonotonicArena(arena.WithMinBufferSize(1024))
resolvable := NewResolvable(ar, ResolvableOptions{})
err := resolvable.Init(ctx, nil, ast.OperationTypeQuery)
require.NoError(t, err)

err = loader.LoadGraphQLResponseData(ctx, response, resolvable)
require.NoError(t, err)

out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors)
assert.Equal(t, `{"data":{"product":{"__typename":"Product","id":"prod-1","name":"Product One"}}}`, out)
})
}

func TestMutationSkipsL2Read(t *testing.T) {
t.Run("mutation operation type skips L2 read and always fetches", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

cache := NewFakeLoaderCache()
// Pre-populate cache with stale data
_ = cache.Set(t.Context(), []*CacheEntry{
{Key: "Product:prod-1", Value: []byte(`{"__typename":"Product","id":"prod-1","name":"Old Name"}`)},
}, 30*time.Second)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

userCacheKeyTemplate := &EntityQueryCacheKeyTemplate{
Keys: NewResolvableObjectVariable(&Object{
Fields: []*Field{
{Name: []byte("__typename"), Value: &String{Path: []string{"__typename"}}},
{Name: []byte("id"), Value: &String{Path: []string{"id"}}},
},
}),
}
providesData := &Object{
Fields: []*Field{
{Name: []byte("name"), Value: &Scalar{}},
},
}

rootDS := NewMockDataSource(ctrl)
rootDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"updateUser":{"__typename":"Product","id":"prod-1"}}}`), nil
}).Times(1)

entityDS := NewMockDataSource(ctrl)
entityDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) {
return []byte(`{"data":{"_entities":[{"__typename":"Product","id":"prod-1","name":"New Name"}]}}`), nil
}).Times(1) // Must fetch fresh data despite cache having stale entry

response := buildProductEntityResponse(rootDS, entityDS, userCacheKeyTemplate, providesData, ast.OperationTypeMutation)

loader := &Loader{caches: map[string]LoaderCache{"default": cache}}
ctx := NewContext(t.Context())
ctx.ExecutionOptions.DisableSubgraphRequestDeduplication = true
ctx.ExecutionOptions.Caching.EnableL1Cache = true
ctx.ExecutionOptions.Caching.EnableL2Cache = true

ar := arena.NewMonotonicArena(arena.WithMinBufferSize(1024))
resolvable := NewResolvable(ar, ResolvableOptions{})
err := resolvable.Init(ctx, nil, ast.OperationTypeMutation)
require.NoError(t, err)

err = loader.LoadGraphQLResponseData(ctx, response, resolvable)
require.NoError(t, err)

out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors)
assert.Equal(t, `{"data":{"updateUser":{"__typename":"Product","id":"prod-1","name":"New Name"}}}`, out, "mutation should fetch fresh data, not use cached stale data")
Comment on lines +2230 to +2279

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Assert that mutations never perform an L2 read.

Right now this only proves the stale value was not served. If the loader still does an L2 Get before fetching, the test stays green and misses the regression this PR is meant to catch.

Suggested change
 		_ = cache.Set(t.Context(), []*CacheEntry{
 			{Key: `{"__typename":"Product","key":{"id":"prod-1"}}`, Value: []byte(`{"__typename":"Product","id":"prod-1","name":"Old Name"}`)},
 		}, 30*time.Second)
+		cache.ClearLog()
@@
 		out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors)
 		assert.Equal(t, `{"data":{"updateUser":{"__typename":"Product","id":"prod-1","name":"New Name"}}}`, out, "mutation should fetch fresh data, not use cached stale data")
+
+		for _, entry := range cache.GetLog() {
+			assert.NotEqual(t, "get", entry.Operation, "mutations should bypass L2 reads entirely")
+		}
 	})
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/engine/resolve/cache_load_test.go` around lines 2230 - 2279, The test
must assert that no L2 read occurs for mutations: replace or augment the fake
cache to verify Get is not called (e.g., use a mock implementing LoaderCache or
add a spy counter on NewFakeLoaderCache and assert cache.Get call count == 0
after loader.LoadGraphQLResponseData). Specifically, keep using
NewFakeLoaderCache / cache.Set to pre-populate, then after calling
Loader.LoadGraphQLResponseData(ctx, response, resolvable) assert the fake
cache's Get was never invoked (or set an expectation of zero calls on a mock
LoaderCache) so the test fails if any L2 Get occurs during a mutation.

})
}

func TestWriteCanonicalJSON(t *testing.T) {
canonicalize := func(input string) string {
v, err := astjson.Parse(input)
Expand Down
Loading
Loading