From 76abd5d98464e3323fadce027b6e79251b03daf2 Mon Sep 17 00:00:00 2001 From: vejrj <77059398+vejrj@users.noreply.github.com> Date: Tue, 17 Dec 2024 19:53:18 +0100 Subject: [PATCH 1/3] subscribe hooks fixed --- .../supermassive/src/__tests__/hooks.test.ts | 106 ++++++++++++++---- .../supermassive/src/executeWithoutSchema.ts | 95 +++++++--------- packages/supermassive/src/hooks/types.ts | 5 +- 3 files changed, 126 insertions(+), 80 deletions(-) diff --git a/packages/supermassive/src/__tests__/hooks.test.ts b/packages/supermassive/src/__tests__/hooks.test.ts index 77471e53..c44f10e2 100644 --- a/packages/supermassive/src/__tests__/hooks.test.ts +++ b/packages/supermassive/src/__tests__/hooks.test.ts @@ -1093,7 +1093,7 @@ describe.each([ }); }); - describe("error in subscription is thrown", () => { + describe("error in beforeSubscriptionEventEmit", () => { beforeEach(() => { jest.clearAllMocks(); }); @@ -1168,14 +1168,14 @@ describe.each([ ); }); - describe("Error in subscription during creating event stream", () => { + describe("afterFieldSubscribe and beforeFieldSubscribe hook errors during creating event stream. It should throw if an error is thrown or returned", () => { beforeEach(() => { jest.clearAllMocks(); }); const testCases: Array = [ { - name: "beforeFieldSubscribe (Error is returned)", + name: "afterFieldSubscribe (Error is returned)", document: `subscription EmitPersons($limit: Int!) { emitPersons(limit: $limit) { @@ -1186,15 +1186,15 @@ describe.each([ limit: 1, }, hooks: { - beforeFieldSubscribe: jest.fn().mockImplementation(() => { + afterFieldSubscribe: jest.fn().mockImplementation(() => { return new Error("Hook error"); }), }, expectedErrorMessage: - "Unexpected error in beforeFieldSubscribe hook: Hook error", + "Unexpected error in afterFieldSubscribe hook: Hook error", }, { - name: "afterFieldSubscribe (Error is returned)", + name: "afterFieldSubscribe (Error is thrown)", document: `subscription EmitPersons($limit: Int!) { emitPersons(limit: $limit) { @@ -1206,14 +1206,14 @@ describe.each([ }, hooks: { afterFieldSubscribe: jest.fn().mockImplementation(() => { - return new Error("Hook error"); + throw new Error("Hook error"); }), }, expectedErrorMessage: "Unexpected error in afterFieldSubscribe hook: Hook error", }, { - name: "beforeFieldSubscribe (Error is thrown)", + name: "afterFieldSubscribe (string is thrown)", document: `subscription EmitPersons($limit: Int!) { emitPersons(limit: $limit) { @@ -1224,15 +1224,15 @@ describe.each([ limit: 1, }, hooks: { - beforeFieldSubscribe: jest.fn().mockImplementation(() => { - throw new Error("Hook error"); + afterFieldSubscribe: jest.fn().mockImplementation(() => { + throw "Hook error"; }), }, expectedErrorMessage: - "Unexpected error in beforeFieldSubscribe hook: Hook error", + 'Unexpected error in afterFieldSubscribe hook: "Hook error"', }, { - name: "beforeFieldSubscribe (string is thrown)", + name: "beforeFieldSubscribe (Error is thrown)", document: `subscription EmitPersons($limit: Int!) { emitPersons(limit: $limit) { @@ -1244,14 +1244,14 @@ describe.each([ }, hooks: { beforeFieldSubscribe: jest.fn().mockImplementation(() => { - throw "Hook error"; + throw new Error("Hook error"); }), }, expectedErrorMessage: - 'Unexpected error in beforeFieldSubscribe hook: "Hook error"', + "Unexpected error in beforeFieldSubscribe hook: Hook error", }, { - name: "afterFieldSubscribe (Error is thrown)", + name: "beforeFieldSubscribe (Error is returned)", document: `subscription EmitPersons($limit: Int!) { emitPersons(limit: $limit) { @@ -1262,15 +1262,15 @@ describe.each([ limit: 1, }, hooks: { - afterFieldSubscribe: jest.fn().mockImplementation(() => { - throw new Error("Hook error"); + beforeFieldSubscribe: jest.fn().mockImplementation(() => { + return new Error("Hook error"); }), }, expectedErrorMessage: - "Unexpected error in afterFieldSubscribe hook: Hook error", + "Unexpected error in beforeFieldSubscribe hook: Hook error", }, { - name: "afterFieldSubscribe (string is thrown)", + name: "beforeFieldSubscribe (string is thrown)", document: `subscription EmitPersons($limit: Int!) { emitPersons(limit: $limit) { @@ -1281,12 +1281,12 @@ describe.each([ limit: 1, }, hooks: { - afterFieldSubscribe: jest.fn().mockImplementation(() => { + beforeFieldSubscribe: jest.fn().mockImplementation(() => { throw "Hook error"; }), }, expectedErrorMessage: - 'Unexpected error in afterFieldSubscribe hook: "Hook error"', + 'Unexpected error in beforeFieldSubscribe hook: "Hook error"', }, ]; @@ -1527,6 +1527,70 @@ describe.each([ ); }); + describe("Error in afterBuildResponse", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + test("afterBuildResponse (Error is thrown)", async () => { + expect.assertions(5); + + const response = await drainExecution( + await execute( + parse(`{ + film(id: 1) { + title + } + }`), + resolvers as UserResolvers, + { + afterBuildResponse: jest.fn().mockImplementation(() => { + throw new Error("Hook error"); + }), + }, + ), + ); + const result = Array.isArray(response) ? response[0] : response; + expect(isTotalExecutionResult(result)).toBe(true); + const errors = result.errors; + expect(result.data).toBeUndefined(); + expect(errors).toBeDefined(); + expect(errors).toHaveLength(1); + expect(errors?.[0].message).toBe( + "Unexpected error in afterBuildResponse hook: Hook error", + ); + }); + + test("afterBuildResponse (Error is returned)", async () => { + expect.assertions(5); + + const response = await drainExecution( + await execute( + parse(`{ + film(id: 1) { + title + } + }`), + resolvers as UserResolvers, + { + afterBuildResponse: jest.fn().mockImplementation(() => { + return new Error("Hook error"); + }), + }, + ), + ); + const result = Array.isArray(response) ? response[0] : response; + expect(isTotalExecutionResult(result)).toBe(true); + const errors = result.errors; + expect(result.data).toBeDefined(); + expect(errors).toBeDefined(); + expect(errors).toHaveLength(1); + expect(errors?.[0].message).toBe( + "Unexpected error in afterBuildResponse hook: Hook error", + ); + }); + }); + describe("Error thrown in the BEFORE OPERATION hook breaks execution", () => { beforeEach(() => { jest.clearAllMocks(); diff --git a/packages/supermassive/src/executeWithoutSchema.ts b/packages/supermassive/src/executeWithoutSchema.ts index ed7e69b6..302acb37 100644 --- a/packages/supermassive/src/executeWithoutSchema.ts +++ b/packages/supermassive/src/executeWithoutSchema.ts @@ -401,10 +401,16 @@ function buildResponse( }; } else { if (hooks?.afterBuildResponse) { - invokeAfterBuildResponseHook(exeContext, initialResult); + const hookResult = invokeAfterBuildResponseHook( + exeContext, + initialResult, + ); if (exeContext.errors.length > (initialResult.errors?.length ?? 0)) { initialResult.errors = exeContext.errors; } + if (hookResult instanceof GraphQLError) { + return { errors: initialResult.errors }; + } } return initialResult; } @@ -693,6 +699,12 @@ function executeSubscriptionImpl( // Implements the "ResolveFieldEventStream" algorithm from GraphQL specification. // It differs from "ResolveFieldValue" due to providing a different `resolveFn`. + let result: unknown; + + if (!isDefaultResolverUsed && hooks?.beforeFieldSubscribe) { + hookContext = invokeBeforeFieldSubscribeHook(info, exeContext); + } + // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. const args = getArgumentValues(exeContext, fieldDef, fieldGroup[0]); @@ -702,27 +714,16 @@ function executeSubscriptionImpl( // used to represent an authenticated user, or request-specific caches. const contextValue = exeContext.contextValue; - if (!isDefaultResolverUsed && hooks?.beforeFieldSubscribe) { - hookContext = invokeBeforeFieldSubscribeHook(info, exeContext); - } - - let result: unknown; - if (hookContext) { if (isPromise(hookContext)) { result = hookContext.then((context) => { hookContext = context; - if (hookContext instanceof GraphQLError) { - return null; - } - return resolveFn(rootValue, args, contextValue, info); }); - } else if (hookContext instanceof GraphQLError) { - result = null; } } + // Call the `subscribe()` resolver or the default resolver to produce an // AsyncIterable yielding raw payloads. if (result === undefined) { @@ -738,40 +739,20 @@ function executeSubscriptionImpl( resolved, error, ); - - if (hookContext instanceof GraphQLError) { - throw hookContext; - } } }; if (isPromise(result)) { - return result.then(assertEventStream).then( - (resolved) => { - if (resolved instanceof GraphQLError) { - throw resolved; - } - - if (!isDefaultResolverUsed && hooks?.afterFieldSubscribe) { - hookContext = invokeAfterFieldSubscribeHook( - info, - exeContext, - hookContext, - resolved, - ); - - if (hookContext instanceof GraphQLError) { - throw hookContext; - } - } - return resolved; - }, - (error) => { + return result + .then(assertEventStream, (error) => { afterFieldSubscribeHandle(undefined, error); - throw locatedError(error, fieldGroup, pathToArray(path)); - }, - ); + }) + .then((resolved) => { + afterFieldSubscribeHandle(resolved); + + return resolved; + }); } const stream = assertEventStream(result); @@ -779,7 +760,7 @@ function executeSubscriptionImpl( return stream; } catch (error) { if (!isDefaultResolverUsed && hooks?.afterFieldSubscribe) { - hookContext = invokeAfterFieldSubscribeHook( + invokeAfterFieldSubscribeHook( info, exeContext, hookContext, @@ -788,10 +769,6 @@ function executeSubscriptionImpl( ); } - if (hookContext instanceof GraphQLError) { - throw hookContext; - } - throw locatedError(error, fieldGroup, pathToArray(path)); } } @@ -1091,15 +1068,11 @@ function resolveAndCompleteField( (rawError) => { const error = locatedError(rawError, fieldGroup, pathToArray(path)); - const hookResult = handleAfterFieldHooks( + handleAfterFieldHooks( invokeAfterFieldCompleteHook, !!hooks?.afterFieldComplete, )(undefined, error); - if (hookResult === null) { - return null; - } - handleFieldError( rawError, exeContext, @@ -1896,7 +1869,7 @@ function invokeBeforeFieldSubscribeHook( ); exeContext.errors.push(error); - return error; + throw error; } else if (result instanceof Error) { const error = toGraphQLError( result, @@ -1905,7 +1878,7 @@ function invokeBeforeFieldSubscribeHook( ); exeContext.errors.push(error); - return error; + throw error; } return result; @@ -2025,7 +1998,7 @@ function invokeAfterFieldSubscribeHook( ); exeContext.errors.push(error); - return error; + throw error; } else if (result instanceof Error) { const error = toGraphQLError( result, @@ -2034,7 +2007,7 @@ function invokeAfterFieldSubscribeHook( ); exeContext.errors.push(error); - return error; + throw error; } return result; @@ -2175,7 +2148,7 @@ function invokeAfterBuildResponseHook( operation: exeContext.operation, result, }), - (_, rawError) => { + (result, rawError) => { if (rawError) { const error = toGraphQLError( rawError, @@ -2183,6 +2156,16 @@ function invokeAfterBuildResponseHook( "Unexpected error in afterBuildResponse hook", ); exeContext.errors.push(error); + + return error; + } else if (result instanceof Error) { + const error = toGraphQLError( + result, + undefined, + "Unexpected error in afterBuildResponse hook", + ); + + exeContext.errors.push(error); } }, ); diff --git a/packages/supermassive/src/hooks/types.ts b/packages/supermassive/src/hooks/types.ts index 18be641f..21360685 100644 --- a/packages/supermassive/src/hooks/types.ts +++ b/packages/supermassive/src/hooks/types.ts @@ -64,8 +64,7 @@ export interface BeforeFieldSubscribe< > { (args: BaseExecuteFieldHookArgs): | Promise - | BeforeHookContext - | GraphQLError; + | BeforeHookContext; } export interface AfterFieldResolveHook< @@ -98,7 +97,7 @@ export interface AfterFieldCompleteHook< } export interface AfterBuildResponseHook { - (args: AfterBuildResponseHookArgs): void; + (args: AfterBuildResponseHookArgs): void | GraphQLError; } export interface BeforeOperationExecuteHook { From fcdb8ef104cd21264382a6ab747d0110a753fc88 Mon Sep 17 00:00:00 2001 From: vejrj <77059398+vejrj@users.noreply.github.com> Date: Tue, 17 Dec 2024 19:56:16 +0100 Subject: [PATCH 2/3] Change files --- ...-supermassive-b4309e82-a399-4bdb-86a0-43dde45b1ac6.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@graphitation-supermassive-b4309e82-a399-4bdb-86a0-43dde45b1ac6.json diff --git a/change/@graphitation-supermassive-b4309e82-a399-4bdb-86a0-43dde45b1ac6.json b/change/@graphitation-supermassive-b4309e82-a399-4bdb-86a0-43dde45b1ac6.json new file mode 100644 index 00000000..a564057e --- /dev/null +++ b/change/@graphitation-supermassive-b4309e82-a399-4bdb-86a0-43dde45b1ac6.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "subscribe hooks fixed", + "packageName": "@graphitation/supermassive", + "email": "77059398+vejrj@users.noreply.github.com", + "dependentChangeType": "none" +} From b60a2d86d30a43a5b5ca6f3d833b1701f3018376 Mon Sep 17 00:00:00 2001 From: vejrj <77059398+vejrj@users.noreply.github.com> Date: Tue, 17 Dec 2024 20:10:13 +0100 Subject: [PATCH 3/3] fix --- .../supermassive/src/__tests__/hooks.test.ts | 16 ++++++++++++++++ .../supermassive/src/executeWithoutSchema.ts | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/supermassive/src/__tests__/hooks.test.ts b/packages/supermassive/src/__tests__/hooks.test.ts index c44f10e2..2c18f90b 100644 --- a/packages/supermassive/src/__tests__/hooks.test.ts +++ b/packages/supermassive/src/__tests__/hooks.test.ts @@ -1672,6 +1672,22 @@ describe.each([ expectedErrorMessage: "Unexpected error in beforeFieldResolve hook: Hook error", }, + { + name: "beforeOperationExecute (Error is thrown)", + document: ` + { + film(id: 1) { + title + } + }`, + hooks: { + beforeOperationExecute: jest.fn().mockImplementation(() => { + return new Error("Hook error"); + }), + }, + expectedErrorMessage: + "Unexpected error in beforeOperationExecute hook: Hook error", + }, { name: "afterFieldResolve (Error is thrown)", document: ` diff --git a/packages/supermassive/src/executeWithoutSchema.ts b/packages/supermassive/src/executeWithoutSchema.ts index 302acb37..4309285c 100644 --- a/packages/supermassive/src/executeWithoutSchema.ts +++ b/packages/supermassive/src/executeWithoutSchema.ts @@ -2082,7 +2082,7 @@ function invokeBeforeOperationExecuteHook(exeContext: ExecutionContext) { if (result instanceof Error) { const error = toGraphQLError( - rawError, + result, undefined, "Unexpected error in beforeOperationExecute hook", );