Merge pull request #3 from mrc-ide/mrc-6085-error-handling

mrc-6085 Error handling

.github/workflows/test-and-push.yml

@@ -12,6 +12,11 @@ jobs:
   test-and-push:
     runs-on: ubuntu-latest
     steps:
+      - name: Set Pull Request Head SHA
+        if: ${{ github.event_name == 'pull_request' }}
+        run: |
+          long_sha=${{ github.event.pull_request.head.sha }}
+          echo "PR_HEAD_SHA=${long_sha:0:7}" >> $GITHUB_ENV
       - uses: actions/checkout@v4
       - uses: actions/setup-node@v4
         with:

docker/common

@@ -1,7 +1,11 @@
 REGISTRY=ghcr.io
 PACKAGE_ORG=mrc-ide
 PACKAGE_NAME=grout
-GIT_SHA=$(git -C "$PACKAGE_ROOT" rev-parse --short=7 HEAD)
+if [[ -z "${PR_HEAD_SHA}" ]]; then
+  GIT_SHA=$(git -C "$PACKAGE_ROOT" rev-parse --short=7 HEAD)
+else
+  GIT_SHA=${PR_HEAD_SHA}
+fi
 if [[ -v "BRANCH_NAME" ]]; then
     GIT_BRANCH=${BRANCH_NAME}
 else

docker/run

@@ -7,5 +7,6 @@ docker run -d --rm \
     --pull=always \
     -p 5000:5000 \
     -v ./data:/data \
+    -e GROUT_ERROR_TEST=true \
     --name=$PACKAGE_NAME \
     $TAG_SHA

package.json

@@ -26,7 +26,8 @@
     "express": "^4.21.1",
     "morgan": "^1.10.0",
     "sqlite": "^5.1.1",
-    "sqlite3": "^5.1.7"
+    "sqlite3": "^5.1.7",
+    "uid": "^2.0.2"
   },
   "devDependencies": {
     "@types/cors": "^2.8.17",

src/controllers/indexController.ts

@@ -1,8 +1,9 @@
 import { Request, Response } from "express";
+import { jsonResponseSuccess } from "../jsonResponse";
 
 export class IndexController {
     static getIndex = (_req: Request, res: Response) => {
         const version = process.env.npm_package_version;
-        res.status(200).json({ version });
+        jsonResponseSuccess({ version }, res);
     };
 }

src/controllers/tileController.ts

@@ -1,28 +1,49 @@
-import { Request, Response } from "express";
+import { NextFunction, Request, Response } from "express";
 import { AppLocals } from "../types/app";
+import asyncControllerHandler from "../errors/asyncControllerHandler";
+import notFound from "../errors/notFound";
+import { GroutError } from "../errors/groutError";
+import { ErrorType } from "../errors/errorType";
+
+const parseIntParam = (param: string): number => {
+    // Native parseInt is not strict (ignores whitespace and trailing chars) so test with a regex
+    if (!/^\d+$/.test(param)) {
+        throw new GroutError(
+            `"${param}" is not an integer`,
+            ErrorType.BAD_REQUEST
+        );
+    }
+    return parseInt(param, 10);
+};
 
 export class TileController {
-    static getTile = async (req: Request, res: Response) => {
-        const { dataset, level, z, x, y } = req.params;
-        const { tileDatasets } = req.app.locals as AppLocals;
+    static getTile = async (
+        req: Request,
+        res: Response,
+        next: NextFunction
+    ) => {
+        await asyncControllerHandler(next, async () => {
+            const { dataset, level, z, x, y } = req.params;
+            const { tileDatasets } = req.app.locals as AppLocals;
 
-        let tileData = null;
-        if (tileDatasets[dataset] && tileDatasets[dataset][level]) {
-            const db = tileDatasets[dataset][level];
-            tileData = await db.getTileData(
-                parseInt(z),
-                parseInt(x),
-                parseInt(y)
-            );
-        }
+            let tileData = null;
+            if (tileDatasets[dataset] && tileDatasets[dataset][level]) {
+                const db = tileDatasets[dataset][level];
+                tileData = await db.getTileData(
+                    parseIntParam(z),
+                    parseIntParam(x),
+                    parseIntParam(y)
+                );
+            }
 
-        if (tileData) {
-            res.writeHead(200, {
-                "Content-Type": "application/octet-stream",
-                "Content-Encoding": "gzip"
-            }).end(tileData);
-        } else {
-            res.writeHead(404).end();
-        }
+            if (tileData) {
+                res.writeHead(200, {
+                    "Content-Type": "application/octet-stream",
+                    "Content-Encoding": "gzip"
+                }).end(tileData);
+            } else {
+                notFound(req);
+            }
+        });
     };
 }

src/errors/asyncControllerHandler.ts

@@ -0,0 +1,10 @@
+import { NextFunction } from "express";
+
+// This method should be used to wrap any async controller methods to ensure error handling is applied
+export default async (next: NextFunction, method: () => void) => {
+    try {
+        await method();
+    } catch (error) {
+        next(error);
+    }
+};

src/errors/errorType.ts

@@ -0,0 +1,11 @@
+export const enum ErrorType {
+    BAD_REQUEST = "BAD_REQUEST",
+    NOT_FOUND = "NOT_FOUND",
+    UNEXPECTED_ERROR = "UNEXPECTED_ERROR"
+}
+
+export const ErrorTypeStatuses: { [key in ErrorType]: number } = {
+    [ErrorType.BAD_REQUEST]: 400,
+    [ErrorType.NOT_FOUND]: 404,
+    [ErrorType.UNEXPECTED_ERROR]: 500
+};

src/errors/groutError.ts

@@ -0,0 +1,16 @@
+import { ErrorType, ErrorTypeStatuses } from "./errorType";
+
+export class GroutError extends Error {
+    errorType: ErrorType;
+
+    constructor(message: string, errorType: ErrorType) {
+        super(message);
+
+        this.name = "GroutError";
+        this.errorType = errorType;
+    }
+
+    get status() {
+        return ErrorTypeStatuses[this.errorType];
+    }
+}

src/errors/handleError.ts

@@ -0,0 +1,31 @@
+import { NextFunction, Response } from "express";
+import { uid } from "uid";
+import { GroutError } from "./groutError";
+import { ErrorType } from "./errorType";
+import { RequestWithError } from "../logging";
+import { jsonResponseError } from "../jsonResponse";
+
+// We need to include the unused next var for this to be used correctly as an error handler
+export const handleError = (
+    err: Error,
+    req: RequestWithError,
+    res: Response,
+    _: NextFunction // eslint-disable-line @typescript-eslint/no-unused-vars
+) => {
+    const groutError = err instanceof GroutError;
+
+    const status = groutError ? err.status : 500;
+    const type = groutError ? err.errorType : ErrorType.UNEXPECTED_ERROR;
+
+    // Do not return raw messages from unexpected errors to the front end
+    const detail = groutError
+        ? err.message
+        : `An unexpected error occurred. Please contact support and quote error code ${uid()}`;
+
+    // Set error type, detail and stack on req so morgan logs them
+    req.errorType = type;
+    req.errorDetail = detail;
+    req.errorStack = err.stack;
+
+    jsonResponseError(status, type, detail, res);
+};

src/errors/notFound.ts

@@ -0,0 +1,8 @@
+import { Request } from "express";
+import { ErrorType } from "./errorType";
+import { GroutError } from "./groutError";
+
+export default (req: Request) => {
+    const { url } = req;
+    throw new GroutError(`Route not found: ${url}`, ErrorType.NOT_FOUND);
+};

src/jsonResponse.ts

@@ -0,0 +1,31 @@
+import { Response } from "express";
+
+const addContentType = (res: Response) => {
+    res.header("Content-Type", "application/json");
+};
+
+export const jsonResponseSuccess = (data: object | string, res: Response) => {
+    addContentType(res);
+    const responseObject = {
+        status: "success",
+        errors: null,
+        data
+    };
+    res.end(JSON.stringify(responseObject));
+};
+
+export const jsonResponseError = (
+    httpStatus: number,
+    error: string,
+    detail: string,
+    res: Response
+) => {
+    addContentType(res);
+    const responseObject = {
+        status: "failure",
+        errors: [{ error, detail }],
+        data: null
+    };
+    res.status(httpStatus);
+    res.end(JSON.stringify(responseObject));
+};

src/logging.ts

@@ -2,9 +2,20 @@ import { Application, Request, Response } from "express";
 import morgan from "morgan";
 import { Dict } from "./types/utils";
 
+export type RequestWithError = Request & {
+    errorType?: string;
+    errorDetail?: string;
+    errorStack?: string;
+};
+
 export const initialiseLogging = (app: Application) => {
+    // Log error details appended to request by handleError
+    morgan.token("error-type", (req: RequestWithError) => req.errorType);
+    morgan.token("error-detail", (req: RequestWithError) => req.errorDetail);
+    morgan.token("error-stack", (req: RequestWithError) => req.errorStack);
+
     const customFormat = (
-        tokens: Dict<(req: Request, res: Response, header?: string) => string>,
+        tokens: Dict<(req: Request, res?: Response, header?: string) => string>,
         req: Request,
         res: Response
     ) => {
@@ -17,7 +28,10 @@ export const initialiseLogging = (app: Application) => {
             tokens.res(req, res, "content-length"),
             "-",
             tokens["response-time"](req, res),
-            "ms"
+            "ms",
+            tokens["error-type"](req),
+            tokens["error-detail"](req),
+            tokens["error-stack"](req)
         ].join(" ");
     };
 

src/routes.ts

@@ -1,10 +1,23 @@
 import { Router } from "express";
 import { IndexController } from "./controllers/indexController";
 import { TileController } from "./controllers/tileController";
+import notFound from "./errors/notFound";
 
 export const registerRoutes = () => {
     const router = Router();
     router.get("/", IndexController.getIndex);
     router.get("/tile/:dataset/:level/:z/:x/:y", TileController.getTile);
+
+    // provide an endpoint we can use to test 500 response behaviour by throwing an "unexpected error" - but only if we
+    // are running in a non-production mode indicated by an env var
+    if (process.env.GROUT_ERROR_TEST) {
+        router.get("/error-test", () => {
+            throw Error("Testing error behaviour");
+        });
+    }
+
+    // Throw 404 error for any unmatched routes
+    router.use(notFound);
+
     return router;
 };

src/server.ts

@@ -7,6 +7,7 @@ import { GroutConfig } from "./types/app";
 import { registerRoutes } from "./routes";
 import { initialiseLogging } from "./logging";
 import { discoverTileDatasets } from "./discover";
+import { handleError } from "./errors/handleError";
 
 // Wrap the main server set-up functionality in a non-top-level method so we can use async - we can revert this in
 // https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-6134/Add-Vite-build-and-related-tidy-up
@@ -32,7 +33,7 @@ const main = async () => {
     Object.freeze(app.locals); // We don't expect anything else to modify app.locals
 
     app.use("/", registerRoutes());
-
+    app.use(handleError);
     app.listen(port, () => {
         console.log(`Grout is running on port ${port}`);
     });

test.html

@@ -27,7 +27,7 @@
         </style>
     </head>
     <body>
-        <h1>GROUT</h1>
+        <h1>GROUT Test Page</h1>
         <div id="map"></div>
         <script>
             const map = L.map("map").setView({lon: 0, lat: 0}, 3);
@@ -79,7 +79,7 @@ <h1>GROUT</h1>
                 style: {
                     weight: 1,
                     fill: false,
-                    color: "#000000"
+                    color: "#777"
                 },
                 ...options
             };