Standardize server responses to use success/failure functions

package.json

@@ -71,6 +71,7 @@
 		"dotenv": "~16.4.5",
 		"escape-html": "~1.0.3",
 		"express": "~4.19.2",
+		"express-async-errors": "~3.1.1",
 		"express-rate-limit": "~7.2.0",
 		"history": "~5.3.0",
 		"ini": "~4.1.3",

src/server/app.js

@@ -4,6 +4,11 @@
 
 const fs = require('fs');
 const express = require('express');
+// TODO Temporary patch for Express 4, which does not forward rejected async route handler
+// promises to the global error handler on its own (Express 5 does this natively). Remove this
+// import (and the express-async-errors dependency in package.json) once Express 5 is adopted —
+// see issue #1676 for that migration.
+require('express-async-errors');
 const rateLimit = require('express-rate-limit');
 const path = require('path');
 const favicon = require('serve-favicon');
@@ -182,7 +187,9 @@ app.use((err, req, res, next) => {
 		return res.status(HTTP_CODES.BAD_REQUEST).send('Bad Request');
 	}
 
-	log.error('Unhandled request error caught by global error handler; logging forwarded err object.', err);
+	// Include the method/URL so the admin-visible log entry can be traced back to a specific
+	// request instead of just showing a generic message with no route context.
+	log.error(`Unhandled request error caught by global error handler for ${req.method} ${req.originalUrl}; logging forwarded err object.`, err);
 	// If response headers are already sent, Express cannot safely change the response
 	// Forward to the default Express handler to finish error
 	if (res.headersSent) {

src/server/log.js

@@ -60,6 +60,13 @@ class Logger {
 		if (error !== null) {
 			if (error.stack) {
 				messageToLog += `Stacktrace: \n${error.stack}\n`;
+				// If this error wraps another one (e.g. new Error(msg, { cause: originalErr })),
+				// also print the original's stack so console/file output isn't limited to just
+				// where the wrapper was created — this only affects console/file output, not the
+				// message stored in the DB, which is unaffected either way.
+				if (error.cause && error.cause.stack) {
+					messageToLog += `Caused by: \n${error.cause.stack}\n`;
+				}
 			} else {
 				// It's possible someone passed in an error that isn't actually an Error object
 				// because javascript lets you throw anything. In that case, the error won't have

src/server/routes/baseline.js

@@ -6,20 +6,20 @@
 const { getConnection } = require('../db');
 const express = require('express');
 const Baseline = require('../models/Baseline');
-const log = require('../log');
 const validate = require('jsonschema').validate;
 const { adminAuthMiddleware } = require('./authenticator');
 const { STRING_GENERAL_MAX_LENGTH } = require('../util/validationConstants');
 const { HTTP_CODES } = require('../util/httpCodes');
 const { isValidIsoDateTime } = require('../util/timeValidation');
+const { success, failure } = require('./response');
 const router = express.Router();
 router.get('/', async (req, res) => {
 	const conn = getConnection();
 	try {
 		const rawBaselines = await Baseline.getAllBaselines(conn);
-		res.json(rawBaselines);
+		success(res, rawBaselines);
 	} catch (err) {
-		log(`Error while getting all baselines: ${err}`, 'error');
+		failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while getting all baselines: ${err.message}`, { cause: err }));
 	}
 });
 router.post('/new', adminAuthMiddleware('create baselines'), async (req, res) => {
@@ -59,14 +59,14 @@ router.post('/new', adminAuthMiddleware('create baselines'), async (req, res) =>
 	};
 
 	if (!validate(req.body, validParams).valid) {
-		res.sendStatus(HTTP_CODES.BAD_REQUEST);
+		failure(res, HTTP_CODES.BAD_REQUEST);
 		return;
 	}
 	// baseline.js does not use moment; validate date strings directly
 	// TODO This might not stay and is not used in OED now but need to see if it has a timezone for the check.
 	if (!isValidIsoDateTime(req.body.applyStart) || !isValidIsoDateTime(req.body.applyEnd) ||
 		!isValidIsoDateTime(req.body.calcStart) || !isValidIsoDateTime(req.body.calcEnd)) {
-		res.sendStatus(HTTP_CODES.BAD_REQUEST);
+		failure(res, HTTP_CODES.BAD_REQUEST);
 		return;
 	}
 
@@ -80,10 +80,9 @@ router.post('/new', adminAuthMiddleware('create baselines'), async (req, res) =>
 			req.body.calcEnd,
 			req.body.note);
 		await baseline.insert(conn);
-		res.sendStatus(HTTP_CODES.OK);
+		success(res);
 	} catch (err) {
-		res.sendStatus(HTTP_CODES.INTERNAL_SERVER_ERROR);
-		log(`Error while adding baseline: ${err}`, 'error');
+		failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while adding baseline: ${err.message}`, { cause: err }));
 	}
 });
 module.exports = router;

src/server/routes/ciks.js

@@ -3,9 +3,10 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const express = require('express');
-const { log } = require('../log');
 const { getConnection } = require('../db');
 const Cik = require('../models/Cik');
+const { success, failure } = require('./response');
+const { HTTP_CODES } = require('../util/httpCodes');
 
 const router = express.Router();
 
@@ -27,8 +28,8 @@ router.get('/', async (req, res) => {
   const conn = getConnection();
   try {
     const rows = await Cik.getAll(conn);
-    res.json(rows.map(formatCikForResponse));
+    success(res, rows.map(formatCikForResponse));
   } catch (err) {
-    log.error(`Error while performing GET ciks details query: ${err}`);
+    failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while performing GET ciks details query: ${err.message}`, { cause: err }));
   }
 });

src/server/routes/compareReadings.js

@@ -12,6 +12,7 @@ const Reading = require('../models/Reading');
 const { STRING_GENERAL_MAX_LENGTH, NUMERIC_ID_MAX_LENGTH } = require('../util/validationConstants');
 const { HTTP_CODES } = require('../util/httpCodes');
 const { isValidIsoDuration } = require('../util/timeValidation');
+const { success, failure } = require('./response');
 
 const DATE_TIME_WITH_TIME_REGEX = /^\d{4}-\d{2}-\d{2}(?:T| )\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:?\d{2})?$/;
 
@@ -118,7 +119,7 @@ function createRouter() {
 
 	router.get('/meters/:meter_ids', async (req, res) => {
 		if (!(validateMeterCompareReadingsParams(req.params) && validateQueryParams(req.query))) {
-			res.sendStatus(HTTP_CODES.BAD_REQUEST);
+			failure(res, HTTP_CODES.BAD_REQUEST);
 			return;
 		}
 		const meterIDs = req.params.meter_ids.split(',').map(id => parseInt(id));
@@ -128,20 +129,24 @@ function createRouter() {
 		const shiftRaw = req.query.shift;
 
 		if (!isValidCompareDateTime(currStartRaw) || !isValidCompareDateTime(currEndRaw) || !isValidIsoDuration(shiftRaw)) {
-			res.sendStatus(HTTP_CODES.BAD_REQUEST);
+			failure(res, HTTP_CODES.BAD_REQUEST);
 			return;
 		}
 
-		// The string sent should set the timezone to UTC so honor that as OED uses UTC.
-		const currStart = moment.parseZone(currStartRaw, moment.ISO_8601, true);
-		const currEnd = moment.parseZone(currEndRaw, moment.ISO_8601, true);
-		const shift = moment.duration(shiftRaw);
-		res.json(await meterCompareReadings(meterIDs, graphicUnitID, currStart, currEnd, shift));
+		try {
+			// The string sent should set the timezone to UTC so honor that as OED uses UTC.
+			const currStart = moment.parseZone(currStartRaw, moment.ISO_8601, true);
+			const currEnd = moment.parseZone(currEndRaw, moment.ISO_8601, true);
+			const shift = moment.duration(shiftRaw);
+			success(res, await meterCompareReadings(meterIDs, graphicUnitID, currStart, currEnd, shift));
+		} catch (err) {
+			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while performing GET meter compare readings: ${err.message}`, { cause: err }));
+		}
 	});
 
 	router.get('/groups/:group_ids', async (req, res) => {
 		if (!(validateGroupCompareReadingsParams(req.params) && validateQueryParams(req.query))) {
-			res.sendStatus(HTTP_CODES.BAD_REQUEST);
+			failure(res, HTTP_CODES.BAD_REQUEST);
 			return;
 		}
 		const groupIDs = req.params.group_ids.split(',').map(id => parseInt(id));
@@ -151,15 +156,19 @@ function createRouter() {
 		const shiftRaw = req.query.shift;
 
 		if (!isValidCompareDateTime(currStartRaw) || !isValidCompareDateTime(currEndRaw) || !isValidIsoDuration(shiftRaw)) {
-			res.sendStatus(HTTP_CODES.BAD_REQUEST);
+			failure(res, HTTP_CODES.BAD_REQUEST);
 			return;
 		}
 
-		// The string sent should set the timezone to UTC so honor that as OED uses UTC.
-		const currStart = moment.parseZone(currStartRaw, moment.ISO_8601, true);
-		const currEnd = moment.parseZone(currEndRaw, moment.ISO_8601, true);
-		const shift = moment.duration(shiftRaw);
-		res.json(await groupCompareReadings(groupIDs, graphicUnitID, currStart, currEnd, shift));
+		try {
+			// The string sent should set the timezone to UTC so honor that as OED uses UTC.
+			const currStart = moment.parseZone(currStartRaw, moment.ISO_8601, true);
+			const currEnd = moment.parseZone(currEndRaw, moment.ISO_8601, true);
+			const shift = moment.duration(shiftRaw);
+			success(res, await groupCompareReadings(groupIDs, graphicUnitID, currStart, currEnd, shift));
+		} catch (err) {
+			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while performing GET group compare readings: ${err.message}`, { cause: err }));
+		}
 	});
 
 	return router;

src/server/routes/conversionArray.js

@@ -9,6 +9,7 @@ const { refreshAllReadingViews } = require('../services/refreshAllReadingViews')
 const validate = require('jsonschema').validate;
 const { adminAuthMiddleware } = require('./authenticator');
 const { HTTP_CODES } = require('../util/httpCodes');
+const { success, failure } = require('./response');
 
 const router = express.Router();
 
@@ -30,20 +31,22 @@ router.post('/refresh', adminAuthMiddleware('conversion refresh system data'), a
 	};
 
 	if (!validate(req.body, validParams).valid) {
-		res.sendStatus(HTTP_CODES.BAD_REQUEST);
+		failure(res, HTTP_CODES.BAD_REQUEST);
 		return;
 	}
 
-	// TODO: Add try/catch error handling to properly handle failures during Cik refresh
-	// or reading view refresh operations and return appropriate error responses.
-	if (req.body.redoCik) {
-		const conn = getConnection();
-		await redoCik(conn);
-	}
-	if (req.body.refreshReadingViews) {
-		await refreshAllReadingViews();
+	try {
+		if (req.body.redoCik) {
+			const conn = getConnection();
+			await redoCik(conn);
+		}
+		if (req.body.refreshReadingViews) {
+			await refreshAllReadingViews();
+		}
+		success(res);
+	} catch (err) {
+		failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while performing conversion array refresh: ${err.message}`, { cause: err }));
 	}
-	res.sendStatus(HTTP_CODES.OK);
 });
 
 module.exports = router;

src/server/routes/conversions.js

@@ -3,10 +3,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const express = require('express');
-const { log } = require('../log');
 const { getConnection } = require('../db');
 const Conversion = require('../models/Conversion');
-const { success, failure } = require('./response');
+const { success, failure, LogLevel } = require('./response');
 const { HTTP_CODES } = require('../util/httpCodes');
 const validate = require('jsonschema').validate;
 
@@ -79,8 +78,7 @@ router.get('/', optionalAuthMiddleware, async (req, res) => {
 		const rows = await Conversion.getAll(conn);
 		res.json(rows.map(formatConversionForResponse));
 	} catch (err) {
-		res.sendStatus(HTTP_CODES.INTERNAL_SERVER_ERROR);
-		log.error(`Error while performing GET conversions details query: ${err}`);
+		failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while performing GET conversions details query: ${err.message}`, { cause: err }));
 	}
 });
 
@@ -91,20 +89,19 @@ router.post('/edit', adminAuthMiddleware('edit conversions'), async (req, res) =
 	const validatorResult = validateConversionsParams(req.body);
 
 	if (!validatorResult.valid) {
-		log.warn(`Got request to edit conversions with invalid conversion data, errors: ${validatorResult.errors}`);
-		failure(res, HTTP_CODES.BAD_REQUEST, `Got request to edit conversions with invalid conversion data, errors: ${validatorResult.errors}`);
+		const message = `Got request to edit conversions with invalid conversion data, errors: ${validatorResult.errors}`;
+		failure(res, HTTP_CODES.BAD_REQUEST, message, message, LogLevel.WARN);
 		return;
 	} else {
 		const conn = getConnection();
 		try {
 			const updatedConversion = new Conversion(req.body.sourceId, req.body.destinationId, req.body.bidirectional,
 				req.body.slope, req.body.intercept, req.body.note);
 			await updatedConversion.update(conn);
+			success(res);
 		} catch (err) {
-			log.error(`Error while editing conversion with error(s): ${err}`);
-			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while editing conversion with error(s): ${err}`);
+			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while editing conversion with error(s): ${err.message}`, { cause: err }));
 		}
-		success(res);
 	}
 });
 
@@ -115,8 +112,8 @@ router.post('/addConversion', adminAuthMiddleware('add conversions'), async (req
 	const validatorResult = validateConversionsParams(req.body);
 
 	if (!validatorResult.valid) {
-		log.error(`Got request to insert conversion with invalid conversion data, errors: ${validatorResult.errors}`);
-		failure(res, HTTP_CODES.BAD_REQUEST, `Got request to insert conversion with invalid conversion data. Error(s): ${validatorResult.errors}`);
+		const message = `Got request to insert conversion with invalid conversion data. Error(s): ${validatorResult.errors}`;
+		failure(res, HTTP_CODES.BAD_REQUEST, message, message);
 	} else {
 		const conn = getConnection();
 		try {
@@ -131,10 +128,9 @@ router.post('/addConversion', adminAuthMiddleware('add conversions'), async (req
 				);
 				await newConversion.insert(t);
 			});
-			res.sendStatus(HTTP_CODES.OK);
+			success(res);
 		} catch (err) {
-			log.error(`Error while inserting new conversion with error(s): ${err}`);
-			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while inserting new conversion with errors(s): ${err}`);
+			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while inserting new conversion with error(s): ${err.message}`, { cause: err }));
 		}
 	}
 });
@@ -174,8 +170,8 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re
 
 	const validatorResult = validate(req.body, validConversion);
 	if (!validatorResult.valid) {
-		log.error(`Got request to delete conversions with invalid conversion data, errors: ${validatorResult.errors}`);
-		failure(res, HTTP_CODES.BAD_REQUEST, `Got request to delete conversions with invalid conversion data. Error(s): ${validatorResult.errors}`);
+		const message = `Got request to delete conversions with invalid conversion data. Error(s): ${validatorResult.errors}`;
+		failure(res, HTTP_CODES.BAD_REQUEST, message, message);
 	} else {
 		const { sourceId, destinationId, meterIds = [], groupIds = [] } = req.body;
 		const conn = getConnection();
@@ -194,8 +190,7 @@ router.post('/delete', adminAuthMiddleware('delete conversions'), async (req, re
 			});
 			success(res, 'Successfully deleted conversion and updated meters/groups');
 		} catch (err) {
-			log.error(`Error while deleting conversion and updating meters/groups: ${err}`);
-			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while deleting conversion and updating meters/groups: ${err}`);
+			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while deleting conversion and updating meters/groups: ${err.message}`, { cause: err }));
 		}
 	}
 });
@@ -211,16 +206,15 @@ router.post('/simulate-delete', adminAuthMiddleware('simulate deleting conversio
 	};
 	const validatorResult = validate(req.body, validConversion);
 	if (!validatorResult.valid) {
-		log.warn(`Got request to simulate deletion of conversions with invalid conversion data, errors: ${validatorResult.errors}`);
-		failure(res, HTTP_CODES.BAD_REQUEST, `Got request to delete conversions with invalid conversion data. Error(s): ${validatorResult.errors}`);
+		const message = `Got request to simulate deletion of conversions with invalid conversion data. Error(s): ${validatorResult.errors}`;
+		failure(res, HTTP_CODES.BAD_REQUEST, message, message, LogLevel.WARN);
 	} else {
 		try {
 			const conn = getConnection();
 			const result = await simulateDeleteConversion(req.body, conn);
-			return res.json(result);
+			success(res, result);
 		} catch (err) {
-			log.error(`Error while simulating deletion of conversion with error(s): ${err}`);
-			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, `Error while simulating deletion of conversion with errors(s): ${err}`);
+			failure(res, HTTP_CODES.INTERNAL_SERVER_ERROR, new Error(`Error while simulating deletion of conversion with error(s): ${err.message}`, { cause: err }));
 		}
 	}
 });