From 753abdab14dc036ae898e6180978ca2cdb6e44dc Mon Sep 17 00:00:00 2001
From: Walter Moar <Walter.Moar@gov.bc.ca>
Date: Wed, 5 Jun 2024 14:43:45 -0700
Subject: [PATCH] refactor: FORMS-1139 add db error handler (#1384)

---
 .../forms/common/middleware/errorHandler.js   | 92 +++++++++++++------
 .../common/middleware/errorHandler.spec.js    | 13 +++
 2 files changed, 75 insertions(+), 30 deletions(-)

diff --git a/app/src/forms/common/middleware/errorHandler.js b/app/src/forms/common/middleware/errorHandler.js
index 7595f661b..084aa213d 100644
--- a/app/src/forms/common/middleware/errorHandler.js
+++ b/app/src/forms/common/middleware/errorHandler.js
@@ -1,45 +1,77 @@
 const Problem = require('api-problem');
 const Objection = require('objection');
 
-module.exports.errorHandler = async (err, _req, res, next) => {
-  let error;
-
-  if (err instanceof Objection.DataError) {
-    error = new Problem(422, {
+/**
+ * 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 :(',
     });
-  } else if (err instanceof Objection.NotFoundError) {
-    error = new Problem(404, {
+  }
+
+  if (error instanceof Objection.NotFoundError) {
+    throw new Problem(404, {
       detail: "Sorry... we still haven't found what you're looking for :(",
     });
-  } else if (err instanceof Objection.UniqueViolationError) {
-    error = new Problem(422, {
-      detail: 'Unique Validation Error',
+  }
+
+  if (error instanceof Objection.ConstraintViolationError) {
+    throw new Problem(422, {
+      detail: 'Constraint Violation Error',
     });
-  } else if (err instanceof Objection.ValidationError) {
-    error = new Problem(422, {
+  }
+
+  if (error instanceof Objection.ValidationError) {
+    throw new Problem(422, {
       detail: 'Validation Error',
-      errors: err.data,
+      errors: error.data,
     });
-  } else if (!(err instanceof Problem) && (err.status || err.statusCode)) {
+  }
+};
+
+/**
+ * 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.
-    error = new Problem(err.status || err.statusCode, {
-      detail: err.message,
-    });
-  } else {
-    error = err;
-  }
+    if (!(err instanceof Problem) && (err.status || err.statusCode)) {
+      throw new Problem(err.status || err.statusCode, {
+        detail: err.message,
+      });
+    }
 
-  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. Save the
-    // ERROR level logs (below) for only the things that need investigation.
-    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);
+    // 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);
+    }
   }
 };
diff --git a/app/tests/unit/forms/common/middleware/errorHandler.spec.js b/app/tests/unit/forms/common/middleware/errorHandler.spec.js
index 9d0c2476a..508d1df29 100644
--- a/app/tests/unit/forms/common/middleware/errorHandler.spec.js
+++ b/app/tests/unit/forms/common/middleware/errorHandler.spec.js
@@ -87,6 +87,19 @@ describe('test error handler middleware', () => {
     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();