From 362cd2f32ea61c144595a9e898a21c0b89eea035 Mon Sep 17 00:00:00 2001 From: Sachin Ahya <57462397+sachinahya@users.noreply.github.com> Date: Wed, 22 May 2024 14:32:14 +0100 Subject: [PATCH 1/3] add tests --- .../test/response-cache.spec.ts | 230 ++++++++++++++++++ 1 file changed, 230 insertions(+) diff --git a/packages/plugins/response-cache/test/response-cache.spec.ts b/packages/plugins/response-cache/test/response-cache.spec.ts index 61abd5ef3..8bb9703d1 100644 --- a/packages/plugins/response-cache/test/response-cache.spec.ts +++ b/packages/plugins/response-cache/test/response-cache.spec.ts @@ -3977,6 +3977,236 @@ describe('useResponseCache', () => { expect(result).toEqual({ data: { users: [{ id: '1' }] } }); }); + it('handles errors thrown from an asynchronous cache set', async () => { + const schema = makeExecutableSchema({ + typeDefs: /* GraphQL */ ` + type Query { + users: [User!]! + } + + type User { + id: ID! + } + `, + resolvers: { + Query: { + users: () => [{ id: 1 }], + }, + }, + }); + + const cache: Cache = { + get: jest.fn(), + invalidate: jest.fn(), + set: jest.fn(() => Promise.reject(new Error('Failed to set cache'))), + }; + + const testInstance = createTestkit( + [ + useResponseCache({ + session: () => null, + cache, + }), + ], + schema, + ); + + const query = /* GraphQL */ ` + query test { + users { + id + } + } + `; + + await expect(testInstance.execute(query)).rejects.toThrow('Failed to set cache'); + expect(cache.set).toHaveBeenCalledTimes(1); + }); + + it('handles errors thrown from an asynchronous cache invalidate', async () => { + const schema = makeExecutableSchema({ + typeDefs: /* GraphQL */ ` + type Query { + users: [User!]! + } + + type Mutation { + updateUser(id: ID!): User! + } + + type User { + id: ID! + } + `, + resolvers: { + Query: { + users: () => [{ id: 1 }], + }, + Mutation: { + updateUser(_, { id }) { + return { + id, + }; + }, + }, + }, + }); + + const cache: Cache = { + get: jest.fn(), + set: jest.fn(), + invalidate: jest.fn(() => Promise.reject(new Error('Failed to invalidate cache'))), + }; + + const testInstance = createTestkit( + [ + useResponseCache({ + session: () => null, + cache, + }), + ], + schema, + ); + + const query = /* GraphQL */ ` + query test { + users { + id + } + } + `; + + await waitForResult(testInstance.execute(query)); + + const mutation = /* GraphQL */ ` + mutation { + updateUser(id: "1") { + id + } + } + `; + + await expect(testInstance.execute(mutation)).rejects.toThrow('Failed to invalidate cache'); + expect(cache.invalidate).toHaveBeenCalledTimes(1); + }); + + it('handles errors thrown from a synchronous cache set', async () => { + const schema = makeExecutableSchema({ + typeDefs: /* GraphQL */ ` + type Query { + users: [User!]! + } + + type User { + id: ID! + } + `, + resolvers: { + Query: { + users: () => [{ id: 1 }], + }, + }, + }); + + const cache: Cache = { + get: jest.fn(), + invalidate: jest.fn(), + set: jest.fn(() => { + throw new Error('Failed to set cache'); + }), + }; + + const testInstance = createTestkit( + [ + useResponseCache({ + session: () => null, + cache, + }), + ], + schema, + ); + + const query = /* GraphQL */ ` + query test { + users { + id + } + } + `; + + await expect(testInstance.execute(query)).rejects.toThrow('Failed to set cache'); + expect(cache.set).toHaveBeenCalledTimes(1); + }); + + it('handles errors thrown from a synchronous cache invalidate', async () => { + const schema = makeExecutableSchema({ + typeDefs: /* GraphQL */ ` + type Query { + users: [User!]! + } + + type Mutation { + updateUser(id: ID!): User! + } + + type User { + id: ID! + } + `, + resolvers: { + Query: { + users: () => [{ id: 1 }], + }, + Mutation: { + updateUser(_, { id }) { + return { + id, + }; + }, + }, + }, + }); + + const cache: Cache = { + get: jest.fn(), + set: jest.fn(), + invalidate: jest.fn(() => { + throw new Error('Failed to invalidate cache'); + }), + }; + + const testInstance = createTestkit( + [ + useResponseCache({ + session: () => null, + cache, + }), + ], + schema, + ); + + const query = /* GraphQL */ ` + query test { + users { + id + } + } + `; + + await waitForResult(testInstance.execute(query)); + + const mutation = /* GraphQL */ ` + mutation { + updateUser(id: "1") { + id + } + } + `; + + await expect(testInstance.execute(mutation)).rejects.toThrow('Failed to invalidate cache'); + expect(cache.invalidate).toHaveBeenCalledTimes(1); + }); + async function waitForResult(result: any) { result = await result; if (result.next) { From 5cb7d029ff0da62c55e27b7e8a3b7f92cec701ff Mon Sep 17 00:00:00 2001 From: Sachin Ahya <57462397+sachinahya@users.noreply.github.com> Date: Wed, 22 May 2024 14:36:21 +0100 Subject: [PATCH 2/3] ensure cache methods are awaited --- packages/plugins/response-cache/src/plugin.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/plugins/response-cache/src/plugin.ts b/packages/plugins/response-cache/src/plugin.ts index 287e62329..db5006034 100644 --- a/packages/plugins/response-cache/src/plugin.ts +++ b/packages/plugins/response-cache/src/plugin.ts @@ -472,13 +472,13 @@ export function useResponseCache = {}> } } - function invalidateCache( + async function invalidateCache( result: ExecutionResult, setResult: (newResult: ExecutionResult) => void, - ): void { + ): Promise { processResult(result.data); - cache.invalidate(identifier.values()); + await cache.invalidate(identifier.values()); if (includeExtensionMetadata) { setResult( resultWithMetadata(result, { @@ -535,7 +535,7 @@ export function useResponseCache = {}> }); } - function maybeCacheResult( + async function maybeCacheResult( result: ExecutionResult, setResult: (newResult: ExecutionResult) => void, ) { @@ -550,7 +550,7 @@ export function useResponseCache = {}> return; } - cache.set(cacheKey, result, identifier.values(), finalTtl); + await cache.set(cacheKey, result, identifier.values(), finalTtl); if (includeExtensionMetadata) { setResult(resultWithMetadata(result, { hit: false, didCache: true, ttl: finalTtl })); } @@ -580,7 +580,10 @@ export function useResponseCache = {}> } function handleAsyncIterableResult = {}>( - handler: (result: ExecutionResult, setResult: (newResult: ExecutionResult) => void) => void, + handler: ( + result: ExecutionResult, + setResult: (newResult: ExecutionResult) => void, + ) => void | Promise, ): OnExecuteDoneHookResult { // When the result is an AsyncIterable, it means the query is using @defer or @stream. // This means we have to build the final result by merging the incremental results. @@ -612,7 +615,7 @@ function handleAsyncIterableResult = { if (!hasNext) { // The query is complete, we can process the final result - handler(result, payload.setResult); + return handler(result, payload.setResult); } } }, From f38e8d53992ac2eeab384c199f05233730e3c056 Mon Sep 17 00:00:00 2001 From: Sachin Ahya <57462397+sachinahya@users.noreply.github.com> Date: Wed, 22 May 2024 14:54:32 +0100 Subject: [PATCH 3/3] changeset --- .changeset/old-moles-develop.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/old-moles-develop.md diff --git a/.changeset/old-moles-develop.md b/.changeset/old-moles-develop.md new file mode 100644 index 000000000..6c68fd698 --- /dev/null +++ b/.changeset/old-moles-develop.md @@ -0,0 +1,6 @@ +--- +'@envelop/response-cache': patch +--- + +Fixes an issue where rejected promises returned from the cache were not being handled correctly, +causing an unhandled promise rejection which would terminate the process.