From 3205965afbd9c4fce00dc03cccd6b6223ddb1d15 Mon Sep 17 00:00:00 2001 From: Kevin J Hoerr Date: Sun, 8 Dec 2019 14:17:23 -0500 Subject: Overhaul error handling for controllers This change fixes various issues with the GET endpoints to return more accurate error messages. It adds business logic error objects that are returned in a union type in the controllers. This change should help separate the business logic from the actual errors from upstream services. There may be required changes still with handling those upstream errors via Promises. Integration tests should be added to verify issues. --- src/errors.ts | 13 ++++++++++ src/formats.ts | 6 +++-- src/index.ts | 1 + src/metadata.ts | 5 ++-- src/routes.ts | 77 ++++++++++++++++++++++++++++++++++++++++----------------- 5 files changed, 75 insertions(+), 27 deletions(-) create mode 100644 src/errors.ts (limited to 'src') diff --git a/src/errors.ts b/src/errors.ts new file mode 100644 index 0000000..4026164 --- /dev/null +++ b/src/errors.ts @@ -0,0 +1,13 @@ +export class BranchNotFoundError extends Error { + constructor() { + super(); + this.message = "Branch not found"; + } +} + +export class InvalidReportDocumentError extends Error { + constructor() { + super(); + this.message = "Invalid report document"; + } +} diff --git a/src/formats.ts b/src/formats.ts index 59a966e..1b5a572 100644 --- a/src/formats.ts +++ b/src/formats.ts @@ -1,6 +1,8 @@ +import { InvalidReportDocumentError } from "./errors"; + export interface Format { // returns the coverage value as %: Number(90.0), Number(100.0), Number(89.5) - parse_coverage: (file: Document) => number; + parse_coverage: (file: Document) => number | InvalidReportDocumentError; match_color: (coverage: number, stage_1: number, stage_2: number) => string; } @@ -35,7 +37,7 @@ const FormatsObj: FormatObj = { parse_coverage: (file: Document) => { const scripts = file.getElementsByTagName("script"); if (scripts.length == 0) { - throw new Error("Invalid report document"); + return new InvalidReportDocumentError(); } const data = scripts[0].text; const accumFunc = (regex: RegExp) => { diff --git a/src/index.ts b/src/index.ts index 7bf2354..fdcdcaf 100644 --- a/src/index.ts +++ b/src/index.ts @@ -43,6 +43,7 @@ new MongoClient(MONGO_URI, { useUnifiedTopology: true }).connect( app.use( expressWinston.logger({ ...logger_config("HTTP"), + colorize: true, // filter out token query param from URL msg: '{{req.method}} {{req.url.replace(/token=[-\\w.~]*(&*)/, "token=$1")}} - {{res.statusCode}} {{res.responseTime}}ms' diff --git a/src/metadata.ts b/src/metadata.ts index c297b38..07e420d 100644 --- a/src/metadata.ts +++ b/src/metadata.ts @@ -2,6 +2,7 @@ import { Db } from "mongodb"; import winston from "winston"; import logger_config from "./util/logger"; +import { BranchNotFoundError } from "./errors"; interface Branch { head: string; @@ -37,7 +38,7 @@ class Metadata { organization: string, repository: string, branch: string - ): Promise { + ): Promise { const result = await this.database .collection("repository") .findOne({ @@ -57,7 +58,7 @@ class Metadata { ); return limb.head; } else { - throw Error("Branch not found"); + return new BranchNotFoundError(); } } diff --git a/src/routes.ts b/src/routes.ts index 2c9cf45..4f622a2 100644 --- a/src/routes.ts +++ b/src/routes.ts @@ -7,11 +7,15 @@ import fs from "fs"; import formats, { Format } from "./formats"; import Metadata from "./metadata"; import { config_or_error } from "./util/config"; +import logger_config from "./util/logger"; +import winston from "winston"; const TOKEN = process.env.TOKEN || ""; const UPLOAD_LIMIT = Number(process.env.UPLOAD_LIMIT || 4194304); const HOST_DIR = config_or_error("HOST_DIR"); +const logger = winston.createLogger(logger_config("HTTP")); + export default (metadata: Metadata) => { const router = express.Router(); @@ -38,13 +42,14 @@ export default (metadata: Metadata) => { } }); req.on("end", () => { - let formatter: Format, coverage: number; - try { - const doc = new JSDOM(contents).window.document; - formatter = formats.get_format(format); - coverage = formatter.parse_coverage(doc); - } catch { - return res.status(400).send("Invalid report document"); + let coverage: number; + const doc = new JSDOM(contents).window.document; + const formatter = formats.get_format(format); + const result = formatter.parse_coverage(doc); + if (typeof result === "number") { + coverage = result; + } else { + return res.status(400).send(result.message); } const badge = badgen({ @@ -84,13 +89,24 @@ export default (metadata: Metadata) => { const { org, repo, branch } = req.params; metadata.getHeadCommit(org, repo, branch).then( - commit => { - res.sendFile( - path.join(HOST_DIR, org, repo, branch, commit, "badge.svg") - ); + result => { + if (typeof result === "string") { + res.sendFile( + path.join( + HOST_DIR, + org, + repo, + branch, + result.toString(), + "badge.svg" + ) + ); + } else { + res.status(404).send(result.message); + } }, - () => { - //FIXME if ORB DNE, should be 404 + err => { + logger.error(err); res.status(500).send("Unknown error occurred"); } ); @@ -100,13 +116,24 @@ export default (metadata: Metadata) => { const { org, repo, branch } = req.params; metadata.getHeadCommit(org, repo, branch).then( - commit => { - res.sendFile( - path.join(HOST_DIR, org, repo, branch, commit, "index.html") - ); + result => { + if (typeof result === "string") { + res.sendFile( + path.join( + HOST_DIR, + org, + repo, + branch, + result.toString(), + "index.html" + ) + ); + } else { + res.status(404).send(result.message); + } }, - () => { - //FIXME if ORB DNE, should be 404 + err => { + logger.error(err); res.status(500).send("Unknown error occurred"); } ); @@ -116,16 +143,20 @@ export default (metadata: Metadata) => { router.get("/v1/:org/:repo/:branch/:commit.svg", (req, res) => { const { org, repo, branch, commit } = req.params; - //FIXME prettify error message? - res.sendFile(path.join(HOST_DIR, org, repo, branch, commit, "badge.svg")); + const file = path.join(HOST_DIR, org, repo, branch, commit, "badge.svg"); + fs.access(file, fs.constants.R_OK, err => + err === null ? res.sendFile(file) : res.status(404).send("File not found") + ); }); // provide hard link for commit router.get("/v1/:org/:repo/:branch/:commit.html", (req, res) => { const { org, repo, branch, commit } = req.params; - //FIXME prettify error message? - res.sendFile(path.join(HOST_DIR, org, repo, branch, commit, "index.html")); + const file = path.join(HOST_DIR, org, repo, branch, commit, "index.html"); + fs.access(file, fs.constants.R_OK, err => + err === null ? res.sendFile(file) : res.status(404).send("File not found") + ); }); return router; -- cgit