From a7213bf70beed26bca8252a91fb332ac6d9106f1 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 26 Apr 2023 15:41:43 +0200 Subject: [PATCH] Return `details` arrays on all errors. (#3630) We used to use the `details` property to return a list of errors on a lot of our errors, but the new format doesn't do this anymore. However, some of the admin UI still expects this to be present, even when the data could go into `message`. So for now, the solution is to duplicate the data and put it both in `message` and in the first element of the `details` list. If the error has its own details lust (such as openapi errors etc), then they will overwrite this default `details` property. --- src/lib/error/api-error.test.ts | 38 +++++++++++++++++++++++++ src/lib/error/api-error.ts | 49 ++++++++++++++++----------------- src/lib/routes/util.ts | 7 ++--- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/src/lib/error/api-error.test.ts b/src/lib/error/api-error.test.ts index 7e1cae4039b..8077246fd89 100644 --- a/src/lib/error/api-error.test.ts +++ b/src/lib/error/api-error.test.ts @@ -1,9 +1,47 @@ import { ErrorObject } from 'ajv'; import { ApiErrorSchema, + fromLegacyError, fromOpenApiValidationError, fromOpenApiValidationErrors, + UnleashApiErrorNameWithoutExtraData, + UnleashApiErrorTypes, + UnleashError, } from './api-error'; +import BadDataError from './bad-data-error'; + +describe('v5 deprecation: backwards compatibility', () => { + it.each(UnleashApiErrorTypes)( + 'Adds details to error type: "%s"', + (name: UnleashApiErrorNameWithoutExtraData) => { + const message = `Error type: ${name}`; + const error = new UnleashError({ name, message }).toJSON(); + + expect(error.message).toBe(message); + expect(error.details).toStrictEqual([ + { + message, + description: message, + }, + ]); + }, + ); +}); + +describe('Standard/legacy error conversion', () => { + it('Moves message to the details list for baddataerror', () => { + const message = `: message!`; + const result = fromLegacyError(new BadDataError(message)).toJSON(); + + expect(result.message.includes('`details`')); + expect(result.details).toStrictEqual([ + { + message, + description: message, + }, + ]); + }); +}); describe('OpenAPI error conversion', () => { it('Gives useful error messages for missing properties', () => { diff --git a/src/lib/error/api-error.ts b/src/lib/error/api-error.ts index cdaff9c42c8..70fa805118d 100644 --- a/src/lib/error/api-error.ts +++ b/src/lib/error/api-error.ts @@ -2,32 +2,32 @@ import { v4 as uuidV4 } from 'uuid'; import { FromSchema } from 'json-schema-to-ts'; import { ErrorObject } from 'ajv'; -const UnleashApiErrorTypes = [ - 'OwaspValidationError', - 'PasswordUndefinedError', - 'NoAccessError', - 'UsedTokenError', - 'InvalidOperationError', +export const UnleashApiErrorTypes = [ + 'ContentTypeError', + 'DisabledError', + 'FeatureHasTagError', 'IncompatibleProjectError', - 'OperationDeniedError', - 'NotFoundError', + 'InvalidOperationError', + 'MinimumOneEnvironmentError', 'NameExistsError', - 'FeatureHasTagError', - 'RoleInUseError', - 'ProjectWithoutOwnerError', - 'UnknownError', + 'NoAccessError', + 'NotFoundError', + 'NotImplementedError', + 'OperationDeniedError', + 'OwaspValidationError', 'PasswordMismatch', 'PasswordMismatchError', - 'DisabledError', - 'ContentTypeError', - 'NotImplementedError', + 'PasswordUndefinedError', + 'ProjectWithoutOwnerError', + 'RoleInUseError', + 'UnknownError', + 'UsedTokenError', // server errors; not the end user's fault 'InternalError', ] as const; const UnleashApiErrorTypesWithExtraData = [ - 'MinimumOneEnvironmentError', 'BadDataError', 'BadRequestError', 'ValidationError', @@ -42,10 +42,8 @@ const AllUnleashApiErrorTypes = [ ] as const; type UnleashApiErrorName = typeof AllUnleashApiErrorTypes[number]; -type UnleashApiErrorNameWithoutExtraData = Exclude< - UnleashApiErrorName, - typeof UnleashApiErrorTypesWithExtraData[number] ->; +export type UnleashApiErrorNameWithoutExtraData = + typeof UnleashApiErrorTypes[number]; const statusCode = (errorName: UnleashApiErrorName): number => { switch (errorName) { @@ -174,6 +172,7 @@ export class UnleashError extends Error { id: this.id, name: this.name, message: this.message, + details: [{ message: this.message, description: this.message }], ...this.additionalParameters, }; } @@ -228,11 +227,10 @@ export const fromLegacyError = (e: Error): UnleashError => { if ( [ - 'ValidationError', - 'BadRequestError', 'BadDataError', + 'BadRequestError', 'InvalidTokenError', - 'MinimumOneEnvironmentError', + 'ValidationError', ].includes(name) ) { return new UnleashError({ @@ -240,10 +238,9 @@ export const fromLegacyError = (e: Error): UnleashError => { | 'ValidationError' | 'BadRequestError' | 'BadDataError' - | 'InvalidTokenError' - | 'MinimumOneEnvironmentError', + | 'InvalidTokenError', message: - 'Your request body failed to validate. Refer to the `details` list to see what happened.', + 'Request validation failed: your request body failed to validate. Refer to the `details` list to see what happened.', details: [{ description: e.message, message: e.message }], }); } diff --git a/src/lib/routes/util.ts b/src/lib/routes/util.ts index 078c6d89e99..3a262c7e804 100644 --- a/src/lib/routes/util.ts +++ b/src/lib/routes/util.ts @@ -26,14 +26,11 @@ export const handleErrors: ( logger: Logger, error: Error, ) => void = (res, logger, error) => { - logger.warn(error.message); - // @ts-expect-error - // eslint-disable-next-line no-param-reassign - error.isJoi = true; - const finalError = error instanceof UnleashError ? error : fromLegacyError(error); + logger.warn(finalError.id, finalError.message); + if (['InternalError', 'UnknownError'].includes(finalError.name)) { logger.error('Server failed executing request', error); }