diff --git a/.github/workflows/test-and-push.yml b/.github/workflows/test-and-push.yml index 61dcfe7..6ffd329 100644 --- a/.github/workflows/test-and-push.yml +++ b/.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: diff --git a/docker/common b/docker/common index e660d95..5a3d5b4 100644 --- a/docker/common +++ b/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 diff --git a/docker/run b/docker/run index b71a4cd..c1c6f26 100755 --- a/docker/run +++ b/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 \ No newline at end of file diff --git a/package-lock.json b/package-lock.json index cfd1181..d61e4c3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,7 +13,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", @@ -829,6 +830,15 @@ "tslib": "2" } }, + "node_modules/@lukeed/csprng": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/@lukeed/csprng/-/csprng-1.1.0.tgz", + "integrity": "sha512-Z7C/xXCiGWsg0KuKsHTKJxbWhpI3Vs5GwLfOean7MGyVFGqdRgBbAjOCh6u4bbjPc/8MJ2pZmK/0DLdCbivLDA==", + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/@nodelib/fs.scandir": { "version": "2.1.5", "resolved": "https://registry.npmjs.org/@nodelib/fs.scandir/-/fs.scandir-2.1.5.tgz", @@ -6111,6 +6121,18 @@ } } }, + "node_modules/uid": { + "version": "2.0.2", + "resolved": "https://registry.npmjs.org/uid/-/uid-2.0.2.tgz", + "integrity": "sha512-u3xV3X7uzvi5b1MncmZo3i2Aw222Zk1keqLA1YkHldREkAhAqi65wuPfe7lHx8H/Wzy+8CE7S7uS3jekIM5s8g==", + "license": "MIT", + "dependencies": { + "@lukeed/csprng": "^1.0.0" + }, + "engines": { + "node": ">=8" + } + }, "node_modules/undefsafe": { "version": "2.0.5", "resolved": "https://registry.npmjs.org/undefsafe/-/undefsafe-2.0.5.tgz", diff --git a/package.json b/package.json index 86724e9..0392f77 100644 --- a/package.json +++ b/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", diff --git a/src/controllers/indexController.ts b/src/controllers/indexController.ts index 8c70d80..cc8a495 100644 --- a/src/controllers/indexController.ts +++ b/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); }; } diff --git a/src/controllers/tileController.ts b/src/controllers/tileController.ts index 892c054..01bf479 100644 --- a/src/controllers/tileController.ts +++ b/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); + } + }); }; } diff --git a/src/errors/asyncControllerHandler.ts b/src/errors/asyncControllerHandler.ts new file mode 100644 index 0000000..8500c4d --- /dev/null +++ b/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); + } +}; diff --git a/src/errors/errorType.ts b/src/errors/errorType.ts new file mode 100644 index 0000000..dcf30bf --- /dev/null +++ b/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 +}; diff --git a/src/errors/groutError.ts b/src/errors/groutError.ts new file mode 100644 index 0000000..30bfb01 --- /dev/null +++ b/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]; + } +} diff --git a/src/errors/handleError.ts b/src/errors/handleError.ts new file mode 100644 index 0000000..e9ab13d --- /dev/null +++ b/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); +}; diff --git a/src/errors/notFound.ts b/src/errors/notFound.ts new file mode 100644 index 0000000..4323ae4 --- /dev/null +++ b/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); +}; diff --git a/src/jsonResponse.ts b/src/jsonResponse.ts new file mode 100644 index 0000000..cb085ab --- /dev/null +++ b/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)); +}; diff --git a/src/logging.ts b/src/logging.ts index 80171f5..0e83d51 100644 --- a/src/logging.ts +++ b/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(" "); }; diff --git a/src/routes.ts b/src/routes.ts index 1564e66..b23e6cb 100644 --- a/src/routes.ts +++ b/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; }; diff --git a/src/server.ts b/src/server.ts index b26b5ba..f2892bf 100644 --- a/src/server.ts +++ b/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}`); }); diff --git a/test.html b/test.html index 09d4173..67dd61c 100644 --- a/test.html +++ b/test.html @@ -27,7 +27,7 @@ -

GROUT

+

GROUT Test Page