Repository: RobinTail/express-zod-api
Pull request #136 (2021-09-19 10:46:27)

Last resort: handling the ResultHandler failure cases
Issue #135 

RobinTail: I wanna have an additional system test

Patch:
diff --git a/src/endpoint.ts b/src/endpoint.ts
index b31e4346..19180002 100644
--- a/src/endpoint.ts
+++ b/src/endpoint.ts
@@ -3,6 +3,7 @@ import {Logger} from 'winston';
 import {z} from 'zod';
 import {ApiResponse} from './api-response';
 import {CommonConfig} from './config-type';
+import {ResultHandlerError} from './errors';
 import {
   combineEndpointAndMiddlewareInputSchemas,
   getInitialInput,
@@ -14,7 +15,7 @@ import {
 import {Method, MethodsDefinition} from './method';
 import {MiddlewareDefinition} from './middleware';
 import {mimeMultipart} from './mime';
-import {ResultHandlerDefinition} from './result-handler';
+import {lastResortHandler, ResultHandlerDefinition} from './result-handler';
 
 export type Handler<IN, OUT, OPT> = (params: {
   input: IN,
@@ -214,9 +215,11 @@ export class Endpoint<
       });
     } catch (e) {
       if (e instanceof Error) {
-        logger.error(`Result handler failure: ${e.message}.`);
+        lastResortHandler({
+          logger, response,
+          error: new ResultHandlerError(e.message, error)
+        });
       }
-      // throw e;
     }
   }
 
diff --git a/src/errors.ts b/src/errors.ts
index 3ac78e3c..44cfcbbb 100644
--- a/src/errors.ts
+++ b/src/errors.ts
@@ -6,3 +6,20 @@ export class DependsOnMethodError extends RoutingError {
 
 export class OpenAPIError extends Error {
 }
+
+export class ResultHandlerError extends Error {
+  protected readonly originalError?: Error;
+
+  constructor(message: string, originalError?: Error | null) {
+    super(message);
+    this.originalError = originalError || undefined;
+  }
+
+  public hasOriginalError() {
+    return this.originalError !== undefined;
+  }
+
+  public getOriginalErrorMessage() {
+    return this.originalError ? this.originalError.message : undefined;
+  }
+}
diff --git a/src/result-handler.ts b/src/result-handler.ts
index 95b43f77..53cdcb88 100644
--- a/src/result-handler.ts
+++ b/src/result-handler.ts
@@ -2,6 +2,7 @@ import {Request, Response} from 'express';
 import {Logger} from 'winston';
 import {z} from 'zod';
 import {ApiResponse, createApiResponse} from './api-response';
+import {ResultHandlerError} from './errors';
 import {
   getMessageFromError,
   getStatusCodeFromError,
@@ -9,6 +10,12 @@ import {
   markOutput
 } from './helpers';
 
+interface LastResortHandlerParams {
+  error: ResultHandlerError;
+  logger: Logger;
+  response: Response;
+}
+
 interface ResultHandlerParams<RES> {
   error: Error | null;
   input: any;
@@ -65,3 +72,11 @@ export const defaultResultHandler = createResultHandler({
     });
   }
 });
+
+export const lastResortHandler = ({error, logger, response}: LastResortHandlerParams) => {
+  logger.error(`Result handler failure: ${error.message}.`);
+  response.status(500).end(
+    `An error occurred while serving the result: ${error.message}.` +
+    (error.hasOriginalError() ? `\nOriginal error: ${error.getOriginalErrorMessage()}.` : '')
+  );
+};
diff --git a/src/server.ts b/src/server.ts
index 0c9e7322..65a8e9ea 100644
--- a/src/server.ts
+++ b/src/server.ts
@@ -1,13 +1,46 @@
-import express from 'express';
+import express, {ErrorRequestHandler, RequestHandler, json} from 'express';
 import fileUpload from 'express-fileupload';
 import {Server} from 'http';
+import {Logger} from 'winston';
 import {AppConfig, CommonConfig, ServerConfig} from './config-type';
+import {ResultHandlerError} from './errors';
 import {isLoggerConfig} from './helpers';
 import {createLogger} from './logger';
-import {defaultResultHandler} from './result-handler';
+import {defaultResultHandler, lastResortHandler} from './result-handler';
 import {initRouting, Routing} from './routing';
 import createHttpError from 'http-errors';
 
+type AnyResultHandler = NonNullable<CommonConfig['errorHandler']>;
+
+export const createParserFailureHandler = (errorHandler: AnyResultHandler, logger: Logger): ErrorRequestHandler =>
+  (error, request, response, next) => {
+    if (!error) { return next(); }
+    errorHandler.handler({
+      error, request, response, logger,
+      input: request.body,
+      output: null
+    });
+  };
+
+export const createNotFoundHandler = (errorHandler: AnyResultHandler, logger: Logger): RequestHandler =>
+  (request, response) => {
+    const error = createHttpError(404, `Can not ${request.method} ${request.path}`);
+    try {
+      errorHandler.handler({
+        request, response, logger, error,
+        input: null,
+        output: null
+      });
+    } catch (e) {
+      if (e instanceof Error) {
+        lastResortHandler({
+          response, logger,
+          error: new ResultHandlerError(e.message, error)
+        });
+      }
+    }
+  };
+
 export function attachRouting(config: AppConfig & CommonConfig, routing: Routing): void {
   const logger = isLoggerConfig(config.logger) ? createLogger(config.logger) : config.logger;
   return initRouting({app: config.app, routing, logger, config});
@@ -17,35 +50,17 @@ export function createServer(config: ServerConfig & CommonConfig, routing: Routi
   const logger = isLoggerConfig(config.logger) ? createLogger(config.logger) : config.logger;
   const app = express();
   const errorHandler = config.errorHandler || defaultResultHandler;
-  const jsonParser = config.server.jsonParser || express.json();
+  const jsonParser = config.server.jsonParser || json();
   const multipartParser = config.server.upload ? fileUpload({
     ...(typeof config.server.upload === 'object' ? config.server.upload : {}),
     abortOnLimit: false,
     parseNested: true,
   }) : undefined;
 
-  const parserFailureHandler: express.ErrorRequestHandler = (error, request, response, next) => {
-    if (!error) { return next(); }
-    errorHandler.handler({
-      error, request, response, logger,
-      input: request.body,
-      output: null
-    });
-  };
-
-  const lastResortHandler: express.RequestHandler = (request, response) => {
-    errorHandler.handler({
-      request, response, logger,
-      error: createHttpError(404, `Can not ${request.method} ${request.path}`),
-      input: null,
-      output: null
-    });
-  };
-
-  app.use(([jsonParser] as express.RequestHandler[]).concat(multipartParser || []));
-  app.use(parserFailureHandler);
+  app.use(([jsonParser] as RequestHandler[]).concat(multipartParser || []));
+  app.use(createParserFailureHandler(errorHandler, logger));
   initRouting({app, routing, logger, config});
-  app.use(lastResortHandler);
+  app.use(createNotFoundHandler(errorHandler, logger));
 
   return app.listen(config.server.listen, () => {
     logger.info(`Listening ${config.server.listen}`);


Test patch:
diff --git a/tests/system/system.spec.ts b/tests/system/system.spec.ts
index 2bcfc822..731a72af 100644
--- a/tests/system/system.spec.ts
+++ b/tests/system/system.spec.ts
@@ -1,6 +1,14 @@
 import http from 'http';
 import fetch from 'node-fetch';
-import {createServer, EndpointsFactory, Method, z, defaultResultHandler} from '../../src';
+import {
+  createServer,
+  EndpointsFactory,
+  Method,
+  z,
+  defaultResultHandler,
+  createResultHandler,
+  createApiResponse
+} from '../../src';
 import {waitFor} from '../helpers';
 
 describe('App', () => {
@@ -9,6 +17,22 @@ describe('App', () => {
   beforeAll(() => {
     const routing = {
       v1: {
+        faulty: new EndpointsFactory(createResultHandler({
+          getPositiveResponse: () => createApiResponse(z.object({})),
+          getNegativeResponse: () => createApiResponse(z.object({})),
+          handler: () => {
+            throw new Error('I am faulty');
+          }
+        })).build({
+          method: 'get',
+          input: z.object({}),
+          output: z.object({
+            test: z.string()
+          }),
+          handler: async () => ({
+            test: 'Should not work'
+          })
+        }),
         test: new EndpointsFactory(defaultResultHandler)
           .addMiddleware({
             input: z.object({
@@ -110,6 +134,20 @@ describe('App', () => {
     });
   });
 
+  describe('Negative', () => {
+    test('Should call Last Resort Handler in case of faulty ResultHandler', async () => {
+      const response = await fetch('http://127.0.0.1:8055/v1/faulty', {
+        method: 'GET',
+        headers: {
+          'Content-Type': 'application/json'
+        },
+      });
+      expect(response.status).toBe(500);
+      const text = await response.text();
+      expect(text).toBe('An error occurred while serving the result: I am faulty.');
+    });
+  });
+
   describe('Protocol', () => {
     test('Should fail on invalid method', async () => {
       const response = await fetch('http://127.0.0.1:8055/v1/test', {
diff --git a/tests/unit/__snapshots__/server.spec.ts.snap b/tests/unit/__snapshots__/server.spec.ts.snap
new file mode 100644
index 00000000..863ae992
--- /dev/null
+++ b/tests/unit/__snapshots__/server.spec.ts.snap
@@ -0,0 +1,31 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Server createNotFoundHandler() the handler should call ResultHandler with 404 error 1`] = `
+Array [
+  Object {
+    "error": [NotFoundError: Can not POST /v1/test],
+    "input": null,
+    "logger": Object {
+      "debug": [MockFunction],
+      "error": [MockFunction],
+      "info": [MockFunction],
+      "warn": [MockFunction],
+    },
+    "output": null,
+    "request": Object {
+      "body": Object {
+        "n": 453,
+      },
+      "header": [MockFunction],
+      "method": "POST",
+      "path": "/v1/test",
+    },
+    "response": Object {
+      "end": [MockFunction],
+      "json": [MockFunction],
+      "set": [MockFunction],
+      "status": [MockFunction],
+    },
+  },
+]
+`;
diff --git a/tests/unit/endpoint.spec.ts b/tests/unit/endpoint.spec.ts
index 3cf702e3..7f0f240e 100644
--- a/tests/unit/endpoint.spec.ts
+++ b/tests/unit/endpoint.spec.ts
@@ -312,6 +312,7 @@ describe('Endpoint', () => {
         header: jest.fn(() => mimeJson),
       };
       const responseMock: Record<string, jest.Mock> = {
+        end: jest.fn(),
         set: jest.fn().mockImplementation(() => responseMock),
         status: jest.fn().mockImplementation(() => responseMock),
         json: jest.fn().mockImplementation(() => responseMock)
@@ -327,8 +328,13 @@ describe('Endpoint', () => {
       });
       expect(loggerMock.error).toBeCalledTimes(1);
       expect(loggerMock.error.mock.calls[0][0]).toBe('Result handler failure: Something unexpected happened.');
-      expect(responseMock.status).toBeCalledTimes(0);
+      expect(responseMock.status).toBeCalledTimes(1);
+      expect(responseMock.status.mock.calls[0][0]).toBe(500);
       expect(responseMock.json).toBeCalledTimes(0);
+      expect(responseMock.end).toBeCalledTimes(1);
+      expect(responseMock.end.mock.calls[0][0]).toBe(
+        'An error occurred while serving the result: Something unexpected happened.'
+      );
     });
   });
 
diff --git a/tests/unit/errors.spec.ts b/tests/unit/errors.spec.ts
new file mode 100644
index 00000000..1dd50090
--- /dev/null
+++ b/tests/unit/errors.spec.ts
@@ -0,0 +1,43 @@
+import {DependsOnMethodError, OpenAPIError, RoutingError} from '../../src';
+import {ResultHandlerError} from '../../src/errors';
+
+describe('Errors', () => {
+  describe('RoutingError', () => {
+    test('should be an instance of Error', () => {
+      expect(new RoutingError('test') instanceof Error).toBeTruthy();
+    });
+  });
+
+  describe('OpenAPIError', () => {
+    test('should be an instance of Error', () => {
+      expect(new OpenAPIError('test') instanceof Error).toBeTruthy();
+    });
+  });
+
+  describe('DependsOnMethodError', () => {
+    test('should be an instance of Error', () => {
+      expect(new DependsOnMethodError('test') instanceof RoutingError).toBeTruthy();
+      expect(new DependsOnMethodError('test') instanceof Error).toBeTruthy();
+    });
+  });
+
+  describe('ResultHandlerError', () => {
+    test('should be an instance of Error', () => {
+      expect(new ResultHandlerError('test') instanceof Error).toBeTruthy();
+    });
+
+    test('.hasOriginalError() should depend on original error', () => {
+      const error = new ResultHandlerError('test', new Error('test2'));
+      expect(error.hasOriginalError()).toBeTruthy();
+      const error2 = new ResultHandlerError('test');
+      expect(error2.hasOriginalError()).toBeFalsy();
+    });
+
+    test('.getOriginalErrorMessage() should depend on original error', () => {
+      const error = new ResultHandlerError('test', new Error('test2'));
+      expect(error.getOriginalErrorMessage()).toBe('test2');
+      const error2 = new ResultHandlerError('test');
+      expect(error2.getOriginalErrorMessage()).toBeUndefined();
+    });
+  });
+});
diff --git a/tests/unit/server.spec.ts b/tests/unit/server.spec.ts
index 15e6833c..206c921b 100644
--- a/tests/unit/server.spec.ts
+++ b/tests/unit/server.spec.ts
@@ -17,9 +17,12 @@ const expressMock = jest.mock('express', () => {
   return returnFunction;
 });
 
-import express from 'express'; // mocked above
+import express, {Request, Response} from 'express';  // express is mocked above
+import {Logger} from 'winston';
 import {createServer, attachRouting, EndpointsFactory, z, defaultResultHandler} from '../../src';
 import {AppConfig, CommonConfig, ServerConfig} from '../../src/config-type';
+import {mimeJson} from '../../src/mime';
+import {createNotFoundHandler, createParserFailureHandler} from '../../src/server';
 
 describe('Server', () => {
   beforeEach(() => {
@@ -111,10 +114,7 @@ describe('Server', () => {
       expect(appMock.use).toBeCalledTimes(3);
       expect(Array.isArray(appMock.use.mock.calls[0][0])).toBeTruthy();
       expect(appMock.use.mock.calls[0][0][0]).toBe(configMock.server.jsonParser);
-      expect(typeof appMock.use.mock.calls[2][0]).toBe('function');
       expect(configMock.errorHandler.handler).toBeCalledTimes(0);
-      appMock.use.mock.calls[2][0]({method: 'get', path: '/v1/test'});
-      expect(configMock.errorHandler.handler).toBeCalledTimes(1);
       expect(configMock.logger.info).toBeCalledTimes(1);
       expect(configMock.logger.info).toBeCalledWith('Listening 8054');
       expect(appMock.get).toBeCalledTimes(1);
@@ -126,36 +126,96 @@ describe('Server', () => {
       expect(appMock.listen).toBeCalledTimes(1);
       expect(appMock.listen.mock.calls[0][0]).toBe(8054);
     });
+  });
 
-    test('parserFailureHandler should call next if there is no error', () => {
-      const configMock: ServerConfig & CommonConfig = {
-        server: {
-          listen: 8054,
-        },
-        cors: true,
-        logger: {
-          level: 'warn',
-          color: false
-        }
+  describe('createParserFailureHandler()', () => {
+    test('the handler should call next if there is no error', () => {
+      const loggerMock = {
+        info: jest.fn(),
+        warn: jest.fn(),
+        error: jest.fn(),
+        debug: jest.fn()
       };
-      const routingMock = {
-        v1: {
-          test: new EndpointsFactory(defaultResultHandler)
-            .build({
-              method: 'get',
-              input: z.object({}),
-              output: z.object({}),
-              handler: jest.fn()
-            })
+      const handler = createParserFailureHandler(defaultResultHandler, loggerMock as unknown as Logger);
+      const next = jest.fn();
+      handler(undefined, null as unknown as Request, null as unknown as Response, next);
+      expect(next).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('createNotFoundHandler()', () => {
+    test('the handler should call ResultHandler with 404 error', () => {
+      const loggerMock = {
+        info: jest.fn(),
+        warn: jest.fn(),
+        error: jest.fn(),
+        debug: jest.fn()
+      };
+      const resultHandler = {
+        ...defaultResultHandler,
+        handler: jest.fn()
+      };
+      const handler = createNotFoundHandler(resultHandler, loggerMock as unknown as Logger);
+      const next = jest.fn();
+      const requestMock = {
+        method: 'POST',
+        path: '/v1/test',
+        header: jest.fn(() => mimeJson),
+        body: {
+          n: 453
         }
       };
-      createServer(configMock, routingMock);
-      expect(appMock).toBeTruthy();
-      expect(appMock.use).toBeCalledTimes(3);
-      expect(typeof appMock.use.mock.calls[1][0]).toBe('function');
+      const responseMock: Record<string, jest.Mock> = {
+        end: jest.fn(),
+        set: jest.fn().mockImplementation(() => responseMock),
+        status: jest.fn().mockImplementation(() => responseMock),
+        json: jest.fn().mockImplementation(() => responseMock)
+      };
+      handler(requestMock as unknown as Request, responseMock as unknown as Response, next);
+      expect(next).toHaveBeenCalledTimes(0);
+      expect(resultHandler.handler).toHaveBeenCalledTimes(1);
+      expect(resultHandler.handler.mock.calls[0]).toMatchSnapshot();
+    });
+
+    test('should call Last Resort Handler in case of ResultHandler is faulty', () => {
+      const loggerMock = {
+        info: jest.fn(),
+        warn: jest.fn(),
+        error: jest.fn(),
+        debug: jest.fn()
+      };
+      const resultHandler = {
+        ...defaultResultHandler,
+        handler: jest.fn().mockImplementation(() => {
+          throw new Error('I am faulty');
+        })
+      };
+      const handler = createNotFoundHandler(resultHandler, loggerMock as unknown as Logger);
       const next = jest.fn();
-      appMock.use.mock.calls[1][0](undefined, null, null, next);
-      expect(next).toHaveBeenCalledTimes(1);
+      const requestMock = {
+        method: 'POST',
+        path: '/v1/test',
+        header: jest.fn(() => mimeJson),
+        body: {
+          n: 453
+        }
+      };
+      const responseMock: Record<string, jest.Mock> = {
+        end: jest.fn(),
+        set: jest.fn().mockImplementation(() => responseMock),
+        status: jest.fn().mockImplementation(() => responseMock),
+        json: jest.fn().mockImplementation(() => responseMock)
+      };
+      handler(requestMock as unknown as Request, responseMock as unknown as Response, next);
+      expect(next).toHaveBeenCalledTimes(0);
+      expect(resultHandler.handler).toHaveBeenCalledTimes(1);
+      expect(responseMock.status).toHaveBeenCalledTimes(1);
+      expect(responseMock.status.mock.calls[0][0]).toBe(500);
+      expect(responseMock.end).toHaveBeenCalledTimes(1);
+      expect(responseMock.end.mock.calls[0][0]).toBe(
+        'An error occurred while serving the result: I am faulty.\n' +
+        'Original error: Can not POST /v1/test.'
+      );
     });
   });