From 28ea0c59e8bcad4474ee3b23ee8575496e64ef7f Mon Sep 17 00:00:00 2001 From: Sasha Date: Tue, 24 Sep 2024 20:29:53 +0300 Subject: [PATCH] feat!: improve afterError hook to accept array of functions, change to object args (#8389) Changes the `afterError` hook structure, adds tests / more docs. Ensures that the `req.responseHeaders` property is respected in the error handler. **Breaking** `afterError` now accepts an array of functions instead of a single function: ```diff - afterError: () => {...} + afterError: [() => {...}] ``` The args are changed to accept an object with the following properties: | Argument | Description | | ------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | **`error`** | The error that occurred. | | **`context`** | Custom context passed between Hooks. [More details](./context). | | **`graphqlResult`** | The GraphQL result object, available if the hook is executed within a GraphQL context. | | **`req`** | The [Request](https://developer.mozilla.org/en-US/docs/Web/API/Request) object containing the currently authenticated `user` | | **`collection`** | The [Collection](../configuration/collections) in which this Hook is running against. This will be `undefined` if the hook is executed from a non-collection endpoint or GraphQL. | | **`result`** | The formatted error result object, available if the hook is executed from a REST context. | --- docs/hooks/collections.mdx | 25 +++++++ docs/hooks/overview.mdx | 21 +++--- packages/next/src/routes/graphql/handler.ts | 48 +++++++----- packages/next/src/routes/rest/routeError.ts | 69 +++++++++++------- .../payload/src/collections/config/types.ts | 16 ++-- packages/payload/src/config/types.ts | 33 ++++++++- packages/payload/src/index.ts | 5 +- packages/plugin-sentry/src/plugin.ts | 10 ++- test/_community/payload-types.ts | 1 - test/collections-rest/config.ts | 10 ++- test/collections-rest/int.spec.ts | 73 ++++++++++++++++++- test/hooks/config.ts | 4 +- 12 files changed, 238 insertions(+), 77 deletions(-) diff --git a/docs/hooks/collections.mdx b/docs/hooks/collections.mdx index dd34966fa7..b87881583a 100644 --- a/docs/hooks/collections.mdx +++ b/docs/hooks/collections.mdx @@ -46,6 +46,7 @@ export const CollectionWithHooks: CollectionConfig = { afterRead: [(args) => {...}], afterDelete: [(args) => {...}], afterOperation: [(args) => {...}], + afterError: [(args) => {....}], // Auth-enabled Hooks beforeLogin: [(args) => {...}], @@ -289,6 +290,30 @@ The following arguments are provided to the `afterOperation` hook: | **`operation`** | The name of the operation that this hook is running within. | | **`result`** | The result of the operation, before modifications. | +### afterError + +The `afterError` Hook is triggered when an error occurs in the Payload application. This can be useful for logging errors to a third-party service, sending an email to the development team, logging the error to Sentry or DataDog, etc. The output can be used to transform the result object / status code. + +```ts +import type { CollectionAfterErrorHook } from 'payload'; + +const afterDeleteHook: CollectionAfterErrorHook = async ({ + req, + id, + doc, +}) => {...} +``` +The following arguments are provided to the `afterError` Hook: + +| Argument | Description | +| ------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **`error`** | The error that occurred. | +| **`context`** | Custom context passed between Hooks. [More details](./context). | +| **`graphqlResult`** | The GraphQL result object, available if the hook is executed within a GraphQL context. | +| **`req`** | The `PayloadRequest` object that extends [Web Request](https://developer.mozilla.org/en-US/docs/Web/API/Request). Contains currently authenticated `user` and the Local API instance `payload`. | +| **`collection`** | The [Collection](../configuration/collections) in which this Hook is running against. | +| **`result`** | The formatted error result object, available if the hook is executed from a REST context. | + ### beforeLogin For [Auth-enabled Collections](../authentication/overview), this hook runs during `login` operations where a user with the provided credentials exist, but before a token is generated and added to the response. You can optionally modify the user that is returned, or throw an error in order to deny the login operation. diff --git a/docs/hooks/overview.mdx b/docs/hooks/overview.mdx index 05c3aa0dcb..c51107a791 100644 --- a/docs/hooks/overview.mdx +++ b/docs/hooks/overview.mdx @@ -43,7 +43,7 @@ export default buildConfig({ // ... // highlight-start hooks: { - afterError: () => {...} + afterError:[() => {...}] }, // highlight-end }) @@ -57,7 +57,7 @@ The following options are available: ### afterError -The `afterError` Hook is triggered when an error occurs in the Payload application. This can be useful for logging errors to a third-party service, sending an email to the development team, logging the error to Sentry or DataDog, etc. +The `afterError` Hook is triggered when an error occurs in the Payload application. This can be useful for logging errors to a third-party service, sending an email to the development team, logging the error to Sentry or DataDog, etc. The output can be used to transform the result object / status code. ```ts import { buildConfig } from 'payload' @@ -65,20 +65,23 @@ import { buildConfig } from 'payload' export default buildConfig({ // ... hooks: { - afterError: async ({ error }) => { + afterError: [async ({ error }) => { // Do something - } + }] }, }) ``` The following arguments are provided to the `afterError` Hook: -| Argument | Description | -|----------|-----------------------------------------------------------------------------------------------| -| **`error`** | The error that occurred. | -| **`context`** | Custom context passed between Hooks. [More details](./context). | - +| Argument | Description | +| ------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **`error`** | The error that occurred. | +| **`context`** | Custom context passed between Hooks. [More details](./context). | +| **`graphqlResult`** | The GraphQL result object, available if the hook is executed within a GraphQL context. | +| **`req`** | The `PayloadRequest` object that extends [Web Request](https://developer.mozilla.org/en-US/docs/Web/API/Request). Contains currently authenticated `user` and the Local API instance `payload`. | +| **`collection`** | The [Collection](../configuration/collections) in which this Hook is running against. This will be `undefined` if the hook is executed from a non-collection endpoint or GraphQL. | +| **`result`** | The formatted error result object, available if the hook is executed from a REST context. | ## Async vs. Synchronous All Hooks can be written as either synchronous or asynchronous functions. Choosing the right type depends on your use case, but switching between the two is as simple as adding or removing the `async` keyword. diff --git a/packages/next/src/routes/graphql/handler.ts b/packages/next/src/routes/graphql/handler.ts index 7f64de347e..0e93f12a05 100644 --- a/packages/next/src/routes/graphql/handler.ts +++ b/packages/next/src/routes/graphql/handler.ts @@ -1,5 +1,5 @@ import type { GraphQLError, GraphQLFormattedError } from 'graphql' -import type { CollectionAfterErrorHook, Payload, SanitizedConfig } from 'payload' +import type { APIError, Payload, PayloadRequest, SanitizedConfig } from 'payload' import { configToSchema } from '@payloadcms/graphql' import { createHandler } from 'graphql-http/lib/use/fetch' @@ -11,28 +11,30 @@ import { createPayloadRequest } from '../../utilities/createPayloadRequest.js' import { headersWithCors } from '../../utilities/headersWithCors.js' import { mergeHeaders } from '../../utilities/mergeHeaders.js' -const handleError = async ( - payload: Payload, - err: any, - debug: boolean, - afterErrorHook: CollectionAfterErrorHook, - // eslint-disable-next-line @typescript-eslint/require-await -): Promise => { - const status = err.originalError.status || httpStatus.INTERNAL_SERVER_ERROR +const handleError = async ({ + err, + payload, + req, +}: { + err: GraphQLError + payload: Payload + req: PayloadRequest +}): Promise => { + const status = (err.originalError as APIError).status || httpStatus.INTERNAL_SERVER_ERROR let errorMessage = err.message payload.logger.error(err.stack) // Internal server errors can contain anything, including potentially sensitive data. // Therefore, error details will be hidden from the response unless `config.debug` is `true` - if (!debug && status === httpStatus.INTERNAL_SERVER_ERROR) { + if (!payload.config.debug && status === httpStatus.INTERNAL_SERVER_ERROR) { errorMessage = 'Something went wrong.' } let response: GraphQLFormattedError = { extensions: { name: err?.originalError?.name || undefined, - data: (err && err.originalError && err.originalError.data) || undefined, - stack: debug ? err.stack : undefined, + data: (err && err.originalError && (err.originalError as APIError).data) || undefined, + stack: payload.config.debug ? err.stack : undefined, statusCode: status, }, locations: err.locations, @@ -40,9 +42,20 @@ const handleError = async ( path: err.path, } - if (afterErrorHook) { - ;({ response } = afterErrorHook(err, response, null, null) || { response }) - } + await payload.config.hooks.afterError?.reduce(async (promise, hook) => { + await promise + + const result = await hook({ + context: req.context, + error: err, + graphqlResult: response, + req, + }) + + if (result) { + response = result.graphqlResult || response + } + }, Promise.resolve()) return response } @@ -95,9 +108,6 @@ export const POST = const { payload } = req - const afterErrorHook = - typeof payload.config.hooks.afterError === 'function' ? payload.config.hooks.afterError : null - const headers = {} const apiResponse = await createHandler({ context: { headers, req }, @@ -113,7 +123,7 @@ export const POST = if (response.errors) { const errors = (await Promise.all( result.errors.map((error) => { - return handleError(payload, error, payload.config.debug, afterErrorHook) + return handleError({ err: error, payload, req }) }), )) as GraphQLError[] // errors type should be FormattedGraphQLError[] but onOperation has a return type of ExecutionResult instead of FormattedExecutionResult diff --git a/packages/next/src/routes/rest/routeError.ts b/packages/next/src/routes/rest/routeError.ts index e106ad06a7..c9dd6c61d8 100644 --- a/packages/next/src/routes/rest/routeError.ts +++ b/packages/next/src/routes/rest/routeError.ts @@ -1,14 +1,13 @@ -import type { Collection, PayloadRequest, SanitizedConfig } from 'payload' +import type { Collection, ErrorResult, PayloadRequest, SanitizedConfig } from 'payload' import httpStatus from 'http-status' import { APIError, APIErrorName, ValidationErrorName } from 'payload' import { getPayloadHMR } from '../../utilities/getPayloadHMR.js' import { headersWithCors } from '../../utilities/headersWithCors.js' +import { mergeHeaders } from '../../utilities/mergeHeaders.js' -export type ErrorResponse = { data?: any; errors: unknown[]; stack?: string } - -const formatErrors = (incoming: { [key: string]: unknown } | APIError): ErrorResponse => { +const formatErrors = (incoming: { [key: string]: unknown } | APIError): ErrorResult => { if (incoming) { // Cannot use `instanceof` to check error type: https://github.com/microsoft/TypeScript/issues/13965 // Instead, get the prototype of the incoming error and check its constructor name @@ -73,14 +72,14 @@ export const routeError = async ({ collection, config: configArg, err, - req, + req: incomingReq, }: { collection?: Collection config: Promise | SanitizedConfig err: APIError - req: Partial + req: PayloadRequest | Request }) => { - let payload = req?.payload + let payload = 'payload' in incomingReq && incomingReq?.payload if (!payload) { try { @@ -95,6 +94,8 @@ export const routeError = async ({ } } + const req = incomingReq as PayloadRequest + req.payload = payload const headers = headersWithCors({ headers: new Headers(), @@ -119,26 +120,44 @@ export const routeError = async ({ response.stack = err.stack } - if (collection && typeof collection.config.hooks.afterError === 'function') { - ;({ response, status } = collection.config.hooks.afterError( - err, - response, - req?.context, - collection.config, - ) || { response, status }) + if (collection) { + await collection.config.hooks.afterError?.reduce(async (promise, hook) => { + await promise + + const result = await hook({ + collection: collection.config, + context: req.context, + error: err, + req, + result: response, + }) + + if (result) { + response = (result.response as ErrorResult) || response + status = result.status || status + } + }, Promise.resolve()) } - if (typeof config.hooks.afterError === 'function') { - ;({ response, status } = config.hooks.afterError( - err, - response, - req?.context, - collection?.config, - ) || { - response, - status, + await config.hooks.afterError?.reduce(async (promise, hook) => { + await promise + + const result = await hook({ + collection: collection?.config, + context: req.context, + error: err, + req, + result: response, }) - } - return Response.json(response, { headers, status }) + if (result) { + response = (result.response as ErrorResult) || response + status = result.status || status + } + }, Promise.resolve()) + + return Response.json(response, { + headers: req.responseHeaders ? mergeHeaders(req.responseHeaders, headers) : headers, + status, + }) } diff --git a/packages/payload/src/collections/config/types.ts b/packages/payload/src/collections/config/types.ts index b9a23c89c4..14e5765707 100644 --- a/packages/payload/src/collections/config/types.ts +++ b/packages/payload/src/collections/config/types.ts @@ -16,6 +16,8 @@ import type { import type { Auth, ClientUser, IncomingAuthType } from '../../auth/types.js' import type { Access, + AfterErrorHookArgs, + AfterErrorResult, CustomComponent, EditConfig, Endpoint, @@ -178,14 +180,6 @@ export type AfterOperationHook > -export type AfterErrorHook = ( - err: Error, - res: unknown, - context: RequestContext, - /** The collection which this hook is being run on. This is null if the AfterError hook was be added to the payload-wide config */ - collection: null | SanitizedCollectionConfig, -) => { response: any; status: number } | void - export type BeforeLoginHook = (args: { /** The collection which this hook is being run on */ collection: SanitizedCollectionConfig @@ -237,6 +231,10 @@ export type AfterRefreshHook = (args: { token: string }) => any +export type AfterErrorHook = ( + args: { collection: SanitizedCollectionConfig } & AfterErrorHookArgs, +) => AfterErrorResult | Promise + export type AfterForgotPasswordHook = (args: { args?: any /** The collection which this hook is being run on */ @@ -402,7 +400,7 @@ export type CollectionConfig = { hooks?: { afterChange?: AfterChangeHook[] afterDelete?: AfterDeleteHook[] - afterError?: AfterErrorHook + afterError?: AfterErrorHook[] afterForgotPassword?: AfterForgotPasswordHook[] afterLogin?: AfterLoginHook[] afterLogout?: AfterLogoutHook[] diff --git a/packages/payload/src/config/types.ts b/packages/payload/src/config/types.ts index 73707f76fd..4b05ca2db8 100644 --- a/packages/payload/src/config/types.ts +++ b/packages/payload/src/config/types.ts @@ -6,6 +6,7 @@ import type { } from '@payloadcms/translations' import type { BusboyConfig } from 'busboy' import type GraphQL from 'graphql' +import type { GraphQLFormattedError } from 'graphql' import type { JSONSchema4 } from 'json-schema' import type { DestinationStream, pino } from 'pino' import type React from 'react' @@ -23,7 +24,6 @@ import type { InternalImportMap, } from '../bin/generateImportMap/index.js' import type { - AfterErrorHook, Collection, CollectionConfig, SanitizedCollectionConfig, @@ -31,7 +31,7 @@ import type { import type { DatabaseAdapterResult } from '../database/types.js' import type { EmailAdapter, SendEmailOptions } from '../email/types.js' import type { GlobalConfig, Globals, SanitizedGlobalConfig } from '../globals/config/types.js' -import type { Payload, TypedUser } from '../index.js' +import type { Payload, RequestContext, TypedUser } from '../index.js' import type { PayloadRequest, Where } from '../types/index.js' import type { PayloadLogger } from '../utilities/logger.js' @@ -621,6 +621,33 @@ export type FetchAPIFileUploadOptions = { useTempFiles?: boolean | undefined } & Partial +export type ErrorResult = { data?: any; errors: unknown[]; stack?: string } + +export type AfterErrorResult = { + graphqlResult?: GraphQLFormattedError + response?: Partial & Record + status?: number +} | void + +export type AfterErrorHookArgs = { + /** The Collection that the hook is operating on. This will be undefined if the hook is executed from a non-collection endpoint or GraphQL. */ + collection?: SanitizedCollectionConfig + /** Custom context passed between hooks */ + context: RequestContext + /** The error that occurred. */ + error: Error + /** The GraphQL result object, available if the hook is executed within a GraphQL context. */ + graphqlResult?: GraphQLFormattedError + /** The Request object containing the currently authenticated user. */ + req: PayloadRequest + /** The formatted error result object, available if the hook is executed from a REST context. */ + result?: ErrorResult +} + +export type AfterErrorHook = ( + args: AfterErrorHookArgs, +) => AfterErrorResult | Promise + /** * This is the central configuration * @@ -895,7 +922,7 @@ export type Config = { * @see https://payloadcms.com/docs/hooks/overview */ hooks?: { - afterError?: AfterErrorHook + afterError?: AfterErrorHook[] } /** i18n config settings */ // eslint-disable-next-line @typescript-eslint/no-empty-object-type diff --git a/packages/payload/src/index.ts b/packages/payload/src/index.ts index 8dacfd3a2a..c8caf1f997 100644 --- a/packages/payload/src/index.ts +++ b/packages/payload/src/index.ts @@ -24,6 +24,7 @@ import type { DataFromCollectionSlug, TypeWithID, } from './collections/config/types.js' +export type * from './admin/types.js' import type { Options as CountOptions } from './collections/operations/local/count.js' import type { Options as CreateOptions } from './collections/operations/local/create.js' import type { @@ -31,6 +32,7 @@ import type { ManyOptions as DeleteManyOptions, Options as DeleteOptions, } from './collections/operations/local/delete.js' +export type { MappedView } from './admin/views/types.js' import type { Options as DuplicateOptions } from './collections/operations/local/duplicate.js' import type { Options as FindOptions } from './collections/operations/local/find.js' import type { Options as FindByIDOptions } from './collections/operations/local/findByID.js' @@ -654,8 +656,6 @@ interface RequestContext { // eslint-disable-next-line @typescript-eslint/no-empty-object-type export interface DatabaseAdapter extends BaseDatabaseAdapter {} export type { Payload, RequestContext } -export type * from './admin/types.js' -export type { MappedView } from './admin/views/types.js' export { default as executeAccess } from './auth/executeAccess.js' export { executeAuthStrategies } from './auth/executeAuthStrategies.js' export { getAccessResults } from './auth/getAccessResults.js' @@ -687,7 +687,6 @@ export type { VerifyConfig, } from './auth/types.js' export { generateImportMap } from './bin/generateImportMap/index.js' - export type { ImportMap } from './bin/generateImportMap/index.js' export { genImportMapIterateFields } from './bin/generateImportMap/iterateFields.js' export type { ClientCollectionConfig } from './collections/config/client.js' diff --git a/packages/plugin-sentry/src/plugin.ts b/packages/plugin-sentry/src/plugin.ts index ce76d36235..bf31a93d90 100644 --- a/packages/plugin-sentry/src/plugin.ts +++ b/packages/plugin-sentry/src/plugin.ts @@ -16,10 +16,12 @@ export const sentryPlugin = config.hooks = { ...(incomingConfig.hooks || {}), - // eslint-disable-next-line @typescript-eslint/no-explicit-any - afterError: (err: any) => { - captureException(err) - }, + + afterError: [ + ({ error }) => { + captureException(error) + }, + ], } config.onInit = async (payload) => { diff --git a/test/_community/payload-types.ts b/test/_community/payload-types.ts index d0d220c544..74282f914a 100644 --- a/test/_community/payload-types.ts +++ b/test/_community/payload-types.ts @@ -186,7 +186,6 @@ export interface PayloadLockedDocument { relationTo: 'users'; value: string | User; } | null); - editedAt?: string | null; globalSlug?: string | null; user: { relationTo: 'users'; diff --git a/test/collections-rest/config.ts b/test/collections-rest/config.ts index 8ca8d9a49c..d41148b3d9 100644 --- a/test/collections-rest/config.ts +++ b/test/collections-rest/config.ts @@ -2,7 +2,7 @@ import { fileURLToPath } from 'node:url' import path from 'path' const filename = fileURLToPath(import.meta.url) const dirname = path.dirname(filename) -import type { CollectionConfig } from 'payload' +import { APIError, type CollectionConfig } from 'payload' import { buildConfigWithDefaults } from '../buildConfigWithDefaults.js' import { devUser } from '../credentials.js' @@ -288,6 +288,14 @@ export default buildConfigWithDefaults({ method: 'get', path: '/internal-error-here', }, + { + handler: () => { + // Throwing an internal error with potentially sensitive data + throw new APIError('Connected to the Pentagon. Secret data: ******') + }, + method: 'get', + path: '/api-error-here', + }, ], onInit: async (payload) => { await payload.create({ diff --git a/test/collections-rest/int.spec.ts b/test/collections-rest/int.spec.ts index 5c03053d2d..9d1c53ba57 100644 --- a/test/collections-rest/int.spec.ts +++ b/test/collections-rest/int.spec.ts @@ -1,6 +1,8 @@ +import type { Payload, SanitizedCollectionConfig } from 'payload' + import { randomBytes, randomUUID } from 'crypto' import path from 'path' -import { NotFound, type Payload } from 'payload' +import { APIError, NotFound } from 'payload' import { fileURLToPath } from 'url' import type { NextRESTClient } from '../helpers/NextRESTClient.js' @@ -1533,6 +1535,75 @@ describe('collections-rest', () => { expect(Array.isArray(result.errors)).toEqual(true) expect(result.errors[0].message).toStrictEqual('Something went wrong.') }) + + it('should execute afterError hook on root level and modify result/status', async () => { + let err: unknown + let errResult: any + + payload.config.hooks.afterError = [ + ({ error, result }) => { + err = error + errResult = result + + return { status: 400, response: { modified: true } } + }, + ] + + const response = await restClient.GET(`/api-error-here`) + expect(response.status).toBe(400) + + expect(err).toBeInstanceOf(APIError) + expect(errResult).toStrictEqual({ + errors: [ + { + message: 'Something went wrong.', + }, + ], + }) + const result = await response.json() + + expect(result.modified).toBe(true) + + payload.config.hooks.afterError = [] + }) + + it('should execute afterError hook on collection level and modify result', async () => { + let err: unknown + let errResult: any + let collection: SanitizedCollectionConfig + + payload.collections.posts.config.hooks.afterError = [ + ({ error, result, collection: incomingCollection }) => { + err = error + errResult = result + collection = incomingCollection + + return { response: { modified: true } } + }, + ] + + const post = await createPost({}) + + const response = await restClient.GET( + `/${slug}/${typeof post.id === 'number' ? 1000 : randomUUID()}`, + ) + expect(response.status).toBe(404) + + expect(collection.slug).toBe(slug) + expect(err).toBeInstanceOf(NotFound) + expect(errResult).toStrictEqual({ + errors: [ + { + message: 'Not Found', + }, + ], + }) + const result = await response.json() + + expect(result.modified).toBe(true) + + payload.collections.posts.config.hooks.afterError = [] + }) }) describe('Local', () => { diff --git a/test/hooks/config.ts b/test/hooks/config.ts index 5202597961..a34b92af94 100644 --- a/test/hooks/config.ts +++ b/test/hooks/config.ts @@ -45,14 +45,14 @@ export const HooksConfig: Promise = buildConfigWithDefaults({ }, ], hooks: { - afterError: () => console.log('Running afterError hook'), + afterError: [() => console.log('Running afterError hook')], }, onInit: async (payload) => { await seedHooksUsers(payload) await payload.create({ collection: hooksSlug, data: { - check: 'update', + check: true, fieldBeforeValidate: false, collectionBeforeValidate: false, fieldBeforeChange: false,