Merge branch 'test/forms-1285' of https://github.com/nimya-aot/common…

…-hosted-form-service into test/forms-1285

app/app.js

@@ -24,7 +24,7 @@ let probeId;
 const app = express();
 app.use(compression());
 app.use(express.json({ limit: config.get('server.bodyLimit') }));
-app.use(express.urlencoded());
+app.use(express.urlencoded({ extended: true }));
 
 // Express needs to know about the OpenShift proxy. With this setting Express
 // pulls the IP address from the headers, rather than use the proxy IP address.
@@ -75,7 +75,7 @@ apiRouter.get('/api', (_req, res) => {
 // Host API endpoints
 apiRouter.use(config.get('server.apiPath'), v1Router);
 app.use(config.get('server.basePath'), apiRouter);
-app.use(middleware.dataErrors);
+app.use(middleware.errorHandler);
 
 // Host the static frontend assets
 const staticFilesPath = config.get('frontend.basePath');

app/package.json

@@ -49,7 +49,7 @@
   "dependencies": {
     "@json2csv/node": "^6.1.3",
     "@json2csv/transforms": "^6.1.3",
-    "api-problem": "^7.0.4",
+    "api-problem": "^9.0.2",
     "aws-sdk": "^2.1376.0",
     "axios": "^0.28.1",
     "axios-oauth-client": "^1.5.0",

app/src/forms/common/middleware/errorHandler.js

@@ -0,0 +1,77 @@
+const Problem = require('api-problem');
+const Objection = require('objection');
+
+/**
+ * Given a subclass of DBError will create and throw the corresponding Problem.
+ * If the error is of an unknown type it will not be converted.
+ *
+ * @param {DBError} error the error to convert to a Problem.
+ * @returns nothing
+ */
+const _throwObjectionProblem = (error) => {
+  if (error instanceof Objection.DataError) {
+    throw new Problem(422, {
+      detail: 'Sorry... the database does not like the data you provided :(',
+    });
+  }
+
+  if (error instanceof Objection.NotFoundError) {
+    throw new Problem(404, {
+      detail: "Sorry... we still haven't found what you're looking for :(",
+    });
+  }
+
+  if (error instanceof Objection.ConstraintViolationError) {
+    throw new Problem(422, {
+      detail: 'Constraint Violation Error',
+    });
+  }
+
+  if (error instanceof Objection.ValidationError) {
+    throw new Problem(422, {
+      detail: 'Validation Error',
+      errors: error.data,
+    });
+  }
+};
+
+/**
+ * Send an error response for all errors except 500s, which are handled by the
+ * code in app.js.
+ *
+ * @param {*} err the Error that occurred.
+ * @param {*} _req the Express object representing the HTTP request - unused.
+ * @param {*} res the Express object representing the HTTP response.
+ * @param {*} next the Express chaining function.
+ * @returns nothing
+ */
+module.exports.errorHandler = async (err, _req, res, next) => {
+  try {
+    if (err instanceof Objection.DBError || err instanceof Objection.NotFoundError || err instanceof Objection.ValidationError) {
+      _throwObjectionProblem(err);
+    }
+
+    // Express throws Errors that are not Problems, but do have an HTTP status
+    // code. For example, 400 is thrown when POST bodies are malformed JSON.
+    if (!(err instanceof Problem) && (err.status || err.statusCode)) {
+      throw new Problem(err.status || err.statusCode, {
+        detail: err.message,
+      });
+    }
+
+    // Not sure what it is, so also handle it in the catch block.
+    throw err;
+  } catch (error) {
+    if (error instanceof Problem && error.status !== 500) {
+      // Handle here when not an internal error. These are mostly from buggy
+      // systems using API Keys, but could also be from frontend bugs. Note that
+      // this does not log the error (see below).
+      error.send(res);
+    } else {
+      // HTTP 500 Problems and all other exceptions should be handled by the
+      // error handler in app.js. It will log them at the ERROR level and
+      // include a full stack trace.
+      next(error);
+    }
+  }
+};

app/src/forms/common/middleware/index.js

@@ -1,4 +1,4 @@
 module.exports = {
-  ...require('./dataErrors'),
+  ...require('./errorHandler'),
   ...require('./rateLimiter'),
 };

app/tests/unit/forms/common/middleware/errorHandler.spec.js

@@ -0,0 +1,120 @@
+const { getMockRes } = require('@jest-mock/express');
+const Problem = require('api-problem');
+const Objection = require('objection');
+
+const middleware = require('../../../../../src/forms/common/middleware');
+
+describe('test error handler middleware', () => {
+  it('should handle an objection data error', () => {
+    const error = new Objection.DataError({
+      nativeError: { message: 'This is a DataError' },
+    });
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).not.toBeCalled();
+    expect(res.end).toBeCalledWith(expect.stringContaining('422'));
+  });
+
+  it('should handle an objection not found error', () => {
+    const error = new Objection.NotFoundError({
+      nativeError: { message: 'This is a NotFoundError' },
+    });
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).not.toBeCalled();
+    expect(res.end).toBeCalledWith(expect.stringContaining('404'));
+  });
+
+  it('should handle an objection unique violation error', () => {
+    const error = new Objection.UniqueViolationError({
+      nativeError: { message: 'This is a UniqueViolationError' },
+    });
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).not.toBeCalled();
+    expect(res.end).toBeCalledWith(expect.stringContaining('422'));
+  });
+
+  it('should handle an objection validation error', () => {
+    const error = new Objection.ValidationError({
+      nativeError: { message: 'This is a ValidationError' },
+    });
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).not.toBeCalled();
+    expect(res.end).toBeCalledWith(expect.stringContaining('422'));
+  });
+
+  it('should handle non-problem errors with a status', () => {
+    const error = new Error('This is a 400 status.');
+    error.status = 400;
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).not.toBeCalled();
+    expect(res.end).toBeCalledWith(expect.stringContaining('400'));
+    expect(res.end).toBeCalledWith(expect.stringContaining('This is a 400 status.'));
+  });
+
+  it('should handle non-problem errors with a status code', () => {
+    const error = new Error('This is a 400 status code.');
+    error.statusCode = 400;
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).not.toBeCalled();
+    expect(res.end).toBeCalledWith(expect.stringContaining('400'));
+    expect(res.end).toBeCalledWith(expect.stringContaining('This is a 400 status code.'));
+  });
+
+  it('should handle any non-500 Problems', () => {
+    const error = new Problem(429);
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).not.toBeCalled();
+    expect(res.end).toBeCalledWith(expect.stringContaining('429'));
+  });
+
+  it('should pass through unknown objection errors', () => {
+    const error = new Objection.DBError({
+      nativeError: {
+        message: 'This base class is never actually instantiated',
+      },
+    });
+    const { res, next } = getMockRes();
+
+    middleware.errorHandler(error, {}, res, next);
+
+    expect(next).toBeCalledWith(error);
+  });
+
+  it('should pass through any 500s', () => {
+    const error = new Problem(500);
+    const { next } = getMockRes();
+
+    middleware.errorHandler(error, {}, {}, next);
+
+    expect(next).toBeCalledWith(error);
+  });
+
+  it('should pass through any Errors without statuses', () => {
+    const error = new Error();
+    const { next } = getMockRes();
+
+    middleware.errorHandler(error, {}, {}, next);
+
+    expect(next).toBeCalledWith(error);
+  });
+});