From 0d5d072f79bc6f34c0080b8c626a4548ffaebb16 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Thu, 27 Apr 2023 14:28:55 +0200 Subject: [PATCH] fix: Make aggregated errors prettier --- src/util/errors/HttpErrorUtil.ts | 17 +++++++++++++---- test/unit/util/errors/HttpErrorUtil.test.ts | 15 ++++++++++++++- test/unit/util/handlers/BooleanHandler.test.ts | 4 ++-- test/unit/util/handlers/HandlerUtil.test.ts | 6 +++--- .../unit/util/handlers/WaterfallHandler.test.ts | 6 +++--- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/util/errors/HttpErrorUtil.ts b/src/util/errors/HttpErrorUtil.ts index a30bf64b1..da9b3e922 100644 --- a/src/util/errors/HttpErrorUtil.ts +++ b/src/util/errors/HttpErrorUtil.ts @@ -16,14 +16,23 @@ export function getStatusCode(error: Error): number { * If they are all within the 4xx range, 400 will be used, otherwise 500. * * @param errors - Errors to combine. - * @param messagePrefix - Prefix for the aggregate error message. Will be followed with an array of all the messages. */ -export function createAggregateError(errors: Error[], messagePrefix = 'No handler supports the given input:'): +export function createAggregateError(errors: Error[]): HttpError { const httpErrors = errors.map((error): HttpError => HttpError.isInstance(error) ? error : new InternalServerError(createErrorMessage(error))); - const joined = httpErrors.map((error: Error): string => error.message).join(', '); - const message = `${messagePrefix} [${joined}]`; + const messages = httpErrors.map((error: Error): string => error.message).filter((msg): boolean => msg.length > 0); + + // Let message depend on the messages that were present. + // This prevents a bunch of empty strings being joined in the case most of them were 404s. + let message: string; + if (messages.length === 0) { + message = ''; + } else if (messages.length === 1) { + message = messages[0]; + } else { + message = `Multiple handler errors: ${messages.join(', ')}`; + } // Check if all errors have the same status code if (httpErrors.length > 0 && httpErrors.every((error): boolean => error.statusCode === httpErrors[0].statusCode)) { diff --git a/test/unit/util/errors/HttpErrorUtil.test.ts b/test/unit/util/errors/HttpErrorUtil.test.ts index d257bf9fb..b5ab84663 100644 --- a/test/unit/util/errors/HttpErrorUtil.test.ts +++ b/test/unit/util/errors/HttpErrorUtil.test.ts @@ -3,7 +3,7 @@ import { createAggregateError, getStatusCode } from '../../../../src/util/errors import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError'; describe('ErrorUtil', (): void => { - describe('createAggregateError', (): void => { + describe('#createAggregateError', (): void => { const error401 = new HttpError(401, 'UnauthorizedHttpError'); const error415 = new HttpError(415, 'UnsupportedMediaTypeHttpError'); const error501 = new HttpError(501, 'NotImplementedHttpError'); @@ -36,6 +36,19 @@ describe('ErrorUtil', (): void => { name: 'InternalServerError', }); }); + + it('has no error message if none of the errors had one.', async(): Promise => { + expect(createAggregateError([ error401, error501 ]).message).toBe(''); + }); + + it('copies the error message if there is one error with a message.', async(): Promise => { + expect(createAggregateError([ error, error501 ]).message).toBe('noStatusCode'); + }); + + it('joins the error messages if there are multiple.', async(): Promise => { + expect(createAggregateError([ error, error, error501 ]).message) + .toBe('Multiple handler errors: noStatusCode, noStatusCode'); + }); }); describe('#getStatusCode', (): void => { diff --git a/test/unit/util/handlers/BooleanHandler.test.ts b/test/unit/util/handlers/BooleanHandler.test.ts index 1612f0e70..f3d46eeff 100644 --- a/test/unit/util/handlers/BooleanHandler.test.ts +++ b/test/unit/util/handlers/BooleanHandler.test.ts @@ -26,7 +26,7 @@ describe('A BooleanHandler', (): void => { it('errors if none of its handlers supports the input.', async(): Promise => { const handler = new BooleanHandler([ handlerCanNotHandle, handlerCanNotHandle ]); - await expect(handler.canHandle(null)).rejects.toThrow('[Not supported, Not supported]'); + await expect(handler.canHandle(null)).rejects.toThrow('Not supported, Not supported'); }); it('returns true if any of its handlers returns true.', async(): Promise => { @@ -51,6 +51,6 @@ describe('A BooleanHandler', (): void => { it('throws the canHandle error when calling handleSafe with unsupported input.', async(): Promise => { const handler = new BooleanHandler([ handlerCanNotHandle, handlerCanNotHandle ]); - await expect(handler.handleSafe(null)).rejects.toThrow('[Not supported, Not supported]'); + await expect(handler.handleSafe(null)).rejects.toThrow('Not supported, Not supported'); }); }); diff --git a/test/unit/util/handlers/HandlerUtil.test.ts b/test/unit/util/handlers/HandlerUtil.test.ts index 718d03900..a3cb7498b 100644 --- a/test/unit/util/handlers/HandlerUtil.test.ts +++ b/test/unit/util/handlers/HandlerUtil.test.ts @@ -17,12 +17,12 @@ describe('HandlerUtil', (): void => { }); it('errors if there is no matching handler.', async(): Promise => { - await expect(findHandler([ handlerFalse, handlerFalse ], null)).rejects.toThrow('[Not supported, Not supported]'); + await expect(findHandler([ handlerFalse, handlerFalse ], null)).rejects.toThrow('Not supported, Not supported'); }); it('supports non-native Errors.', async(): Promise => { handlerFalse.canHandle = jest.fn().mockRejectedValue('apple'); - await expect(findHandler([ handlerFalse ], null)).rejects.toThrow('[Unknown error: apple]'); + await expect(findHandler([ handlerFalse ], null)).rejects.toThrow('Unknown error: apple'); }); }); @@ -42,7 +42,7 @@ describe('HandlerUtil', (): void => { it('errors if there is no matching handler.', async(): Promise => { await expect(filterHandlers([ handlerFalse, handlerFalse ], null)) - .rejects.toThrow('[Not supported, Not supported]'); + .rejects.toThrow('Not supported, Not supported'); }); }); }); diff --git a/test/unit/util/handlers/WaterfallHandler.test.ts b/test/unit/util/handlers/WaterfallHandler.test.ts index dae3bdf7f..1c137e07b 100644 --- a/test/unit/util/handlers/WaterfallHandler.test.ts +++ b/test/unit/util/handlers/WaterfallHandler.test.ts @@ -42,7 +42,7 @@ describe('A WaterfallHandler', (): void => { it('can not handle data if no handler supports it.', async(): Promise => { const handler = new WaterfallHandler([ handlerFalse, handlerFalse ]); - await expect(handler.canHandle(null)).rejects.toThrow('[Not supported, Not supported]'); + await expect(handler.canHandle(null)).rejects.toThrow('Not supported, Not supported'); }); it('throws unknown errors if no Error objects are thrown.', async(): Promise => { @@ -51,7 +51,7 @@ describe('A WaterfallHandler', (): void => { }; const handler = new WaterfallHandler([ handlerFalse, handlerFalse ]); - await expect(handler.canHandle(null)).rejects.toThrow('[Unknown error: apple, Unknown error: apple]'); + await expect(handler.canHandle(null)).rejects.toThrow('Unknown error: apple, Unknown error: apple'); }); it('handles data if a handler supports it.', async(): Promise => { @@ -79,7 +79,7 @@ describe('A WaterfallHandler', (): void => { it('throws the canHandle error when calling handleSafe if the data is not supported.', async(): Promise => { const handler = new WaterfallHandler([ handlerFalse, handlerFalse ]); - await expect(handler.handleSafe(null)).rejects.toThrow('[Not supported, Not supported]'); + await expect(handler.handleSafe(null)).rejects.toThrow('Not supported, Not supported'); }); }); });