From 950222f94ec791bf144b5e38c5de53e046477b7d Mon Sep 17 00:00:00 2001 From: Alex Holliday Date: Mon, 27 May 2024 10:47:14 -0700 Subject: [PATCH 1/2] Implemented error handling middleware --- Server/controllers/authController.js | 35 ++++---- Server/controllers/monitorController.js | 110 +++++++++++++----------- Server/index.js | 7 ++ Server/middleware/handleErrors.js | 9 ++ Server/middleware/verifyJWT.js | 14 +-- Server/middleware/verifyOwnership.js | 23 ++--- 6 files changed, 106 insertions(+), 92 deletions(-) create mode 100644 Server/middleware/handleErrors.js diff --git a/Server/controllers/authController.js b/Server/controllers/authController.js index 1285726e1..a3a43ba0d 100644 --- a/Server/controllers/authController.js +++ b/Server/controllers/authController.js @@ -21,35 +21,32 @@ const issueToken = (payload) => { * @param {express.Response} res * @returns {{success: Boolean, msg: String}} */ -const registerController = async (req, res) => { +const registerController = async (req, res, next) => { // joi validation try { await registerValidation.validateAsync(req.body); } catch (error) { - return res - .status(400) - .json({ success: false, msg: error.details[0].message }); + error.status = 400; + error.service = SERVICE_NAME; + error.message = error.details[0].message; + next(error); + return; } // Check if the user exists try { const isUser = await req.db.getUserByEmail(req, res); if (isUser) { - logger.error("User already exists", { - service: SERVICE_NAME, - userId: isUser._id, - }); - return res - .status(400) - .json({ success: false, msg: "User already exists" }); + throw new Error("User already exists"); } } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); + return; } + // Create a new user try { - // Create a new user const newUser = await req.db.insertUser(req, res); // TODO: Send an email to user // Will add this later @@ -63,8 +60,8 @@ const registerController = async (req, res) => { .status(200) .json({ success: true, msg: "User created", data: token }); } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; @@ -76,7 +73,7 @@ const registerController = async (req, res) => { * @returns {Promise} * @throws {Error} */ -const loginController = async (req, res) => { +const loginController = async (req, res, next) => { try { // Validate input await loginValidation.validateAsync(req.body); @@ -104,9 +101,9 @@ const loginController = async (req, res) => { .status(200) .json({ success: true, msg: "Found user", data: token }); } catch (error) { + error.status = 500; // Anything else should be an error - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + next(error); } }; diff --git a/Server/controllers/monitorController.js b/Server/controllers/monitorController.js index b8e2e1ec7..250dc55b8 100644 --- a/Server/controllers/monitorController.js +++ b/Server/controllers/monitorController.js @@ -4,7 +4,6 @@ const { monitorValidation, } = require("../validation/joi"); -const logger = require("../utils/logger"); const SERVICE_NAME = "monitorController"; /** @@ -15,13 +14,13 @@ const SERVICE_NAME = "monitorController"; * @returns {Promise} * @throws {Error} */ -const getAllMonitors = async (req, res) => { +const getAllMonitors = async (req, res, next) => { try { const monitors = await req.db.getAllMonitors(); return res.json({ success: true, msg: "Monitors found", data: monitors }); } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; @@ -33,25 +32,28 @@ const getAllMonitors = async (req, res) => { * @returns {Promise} * @throws {Error} */ -const getMonitorById = async (req, res) => { - const { error } = getMonitorByIdValidation.validate(req.params); - if (error) { - return res - .status(422) - .json({ success: false, msg: error.details[0].message }); +const getMonitorById = async (req, res, next) => { + try { + await getMonitorByIdValidation.validateAsync(req.params); + } catch (error) { + error.status = 422; + error.message = error.details[0].message; + next(error); + return; } try { const monitor = await req.db.getMonitorById(req, res); if (!monitor) { - logger.error("Monitor not found", { service: SERVICE_NAME }); - return res.status(404).json({ success: false, msg: "Monitor not found" }); + const error = new Error("Monitor not found"); + error.status = 404; + throw error; } return res.json({ success: true, msg: "Monitor found", data: monitor }); } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; @@ -63,7 +65,7 @@ const getMonitorById = async (req, res) => { * @returns {Promise} * @throws {Error} */ -const getMonitorsByUserId = async (req, res) => { +const getMonitorsByUserId = async (req, res, next) => { const { error } = getMonitorsByUserIdValidation.validate(req.params); if (error) { return res @@ -76,9 +78,9 @@ const getMonitorsByUserId = async (req, res) => { const monitors = await req.db.getMonitorsByUserId(req, res); if (monitors && monitors.length === 0) { - return res - .status(404) - .json({ success: false, msg: "No monitors not found" }); + const err = new Error("No monitors found"); + err.status = 404; + throw err; } return res.json({ @@ -87,8 +89,8 @@ const getMonitorsByUserId = async (req, res) => { data: monitors, }); } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; @@ -101,12 +103,15 @@ const getMonitorsByUserId = async (req, res) => { * @throws {Error} */ -const createMonitor = async (req, res) => { - const { error } = monitorValidation.validate(req.body); - if (error) { - return res - .status(422) - .json({ success: false, msg: error.details[0].message }); +const createMonitor = async (req, res, next) => { + try { + await monitorValidation.validateAsync(req.body); + } catch (error) { + error.status = 422; + error.service = SERVICE_NAME; + error.message = error.details[0].message; + next(error); + return; } try { @@ -115,8 +120,8 @@ const createMonitor = async (req, res) => { .status(201) .json({ success: true, msg: "Monitor created", data: monitor }); } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; @@ -129,15 +134,19 @@ const createMonitor = async (req, res) => { * @throws {Error} */ -const deleteMonitor = async (req, res) => { - const { error } = getMonitorByIdValidation.validate(req.params); - if (error) { - return res - .status(422) - .json({ success: false, msg: error.details[0].message }); - } +const deleteMonitor = async (req, res, next) => { try { - const monitor = await req.db.deleteMonitor(req, res); + await getMonitorByIdValidation.validateAsync(req.params); + } catch (error) { + error.status = 422; + error.service = SERVICE_NAME; + error.message = error.details[0].message; + next(error); + return; + } + + try { + const monitor = await req.db.deleteMonitor(req, res, next); /** * TODO * We should remove all checks and alerts associated with this monitor @@ -146,8 +155,8 @@ const deleteMonitor = async (req, res) => { */ return res.status(200).json({ success: true, msg: "Monitor deleted" }); } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; @@ -160,18 +169,15 @@ const deleteMonitor = async (req, res) => { * @throws {Error} */ const editMonitor = async (req, res, next) => { - let { paramError } = getMonitorByIdValidation.validate(req.params); - if (paramError) { - return res - .status(422) - .json({ success: false, msg: paramError.error.details[0].message }); - } - - let { error } = monitorValidation.validate(req.body); - if (error) { - return res - .status(422) - .json({ success: false, msg: error.details[0].message }); + try { + await getMonitorByIdValidation.validateAsync(req.params); + await monitorValidation.validateAsync(req.body); + } catch (error) { + error.status = 422; + error.service = SERVICE_NAME; + error.message = error.details[0].message; + next(error); + return; } try { @@ -180,8 +186,8 @@ const editMonitor = async (req, res, next) => { .status(200) .json({ success: true, msg: "Monitor edited", data: editedMonitor }); } catch (error) { - logger.error(error.message, { service: SERVICE_NAME }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; diff --git a/Server/index.js b/Server/index.js index 11e2b5b5b..db3be459c 100644 --- a/Server/index.js +++ b/Server/index.js @@ -7,6 +7,7 @@ const { connectDbAndRunServer } = require("./configs/db"); require("dotenv").config(); const logger = require("./utils/logger"); const { verifyJWT } = require("./middleware/verifyJWT"); +const { handleErrors } = require("./middleware/handleErrors"); // const { sendEmail } = require('./utils/sendEmail') @@ -62,6 +63,12 @@ app.use((req, res, next) => { app.use("/api/v1/auth", authRouter); app.use("/api/v1/monitors", verifyJWT, monitorRouter); +/** + * Error handler middleware + * MUST be called after all routes + */ +app.use(handleErrors); + // Testing email service // app.use('/sendEmail', async (req, res) => { // const response = sendEmail(['veysel.boybay@bluewavelabs.ca'], 'Testing email service', '

Testing Bluewavelabs

'); diff --git a/Server/middleware/handleErrors.js b/Server/middleware/handleErrors.js new file mode 100644 index 000000000..2febc6b31 --- /dev/null +++ b/Server/middleware/handleErrors.js @@ -0,0 +1,9 @@ +const logger = require("../utils/logger"); + +const handleErrors = (error, req, res, next) => { + error.status = error.status ? error.status : 500; + logger.error(error.message, { service: error.service }); + res.status(error.status).json({ success: false, msg: error.message }); +}; + +module.exports = { handleErrors }; diff --git a/Server/middleware/verifyJWT.js b/Server/middleware/verifyJWT.js index 53464c606..2e8895623 100644 --- a/Server/middleware/verifyJWT.js +++ b/Server/middleware/verifyJWT.js @@ -15,8 +15,11 @@ const verifyJWT = (req, res, next) => { const token = req.headers["authorization"]; // Make sure a token is provided if (!token) { - logger.error("No token provided", { service: SERVICE_NAME }); - return res.status(401).json({ success: false, msg: "No token provided" }); + const error = new Error("No token provided"); + error.status = 401; + error.service = SERVICE_NAME; + next(error); + return; } // Make sure it is properly formatted if (token.startsWith(TOKEN_PREFIX)) { @@ -32,10 +35,9 @@ const verifyJWT = (req, res, next) => { next(); }); } else { - logger.error("Invalid token format", { service: SERVICE_NAME }); - return res - .status(400) - .json({ success: false, msg: "Invalid token format" }); + error.status = 400; + error.service = SERVICE_NAME; + next(error); } }; diff --git a/Server/middleware/verifyOwnership.js b/Server/middleware/verifyOwnership.js index 1b496fbce..00f7b0787 100644 --- a/Server/middleware/verifyOwnership.js +++ b/Server/middleware/verifyOwnership.js @@ -12,28 +12,21 @@ const verifyOwnership = (Model, paramName) => { logger.error("Document not found", { service: SERVICE_NAME, }); - return res - .status(404) - .json({ success: false, msg: "Document not found" }); + const error = new Error("Document not found"); + error.status = 404; + throw error; } // If the userID does not match the document's userID, return a 403 error if (userId.toString() !== doc.userId.toString()) { - logger.error("Unauthorized access", { - service: SERVICE_NAME, - }); - - return res.status(403).json({ - success: false, - msg: "You are not authorized to perform this action", - }); + const error = new Error("Unauthorized access"); + error.status = 403; + throw error; } next(); } catch (error) { - logger.error(error.message, { - service: SERVICE_NAME, - }); - return res.status(500).json({ success: false, msg: error.message }); + error.service = SERVICE_NAME; + next(error); } }; }; From 5fa73a1f17f34af26b93c2b232e2c2cf386a04b5 Mon Sep 17 00:00:00 2001 From: Alex Holliday Date: Mon, 27 May 2024 11:03:12 -0700 Subject: [PATCH 2/2] Added documentation for error handling --- README.md | 37 ++++++++++++++++++++++++++++++- Server/middleware/handleErrors.js | 9 +++++--- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5440f0e7d..a8e1b1f32 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,8 @@ BlueWave uptime monitoring application - POST [/api/v1/monitors](#post-monitors) - POST [/api/v1/monitors/delete/{monitorId}](#post-monitors-del-id) - POST [/api/v1/monitors/edit/{monitorId}](#post-monitors-edit-id) -5. [Contributors](#contributors) +5. [Error Handling](#error-handling) +6. [Contributors](#contributors) --- @@ -544,6 +545,40 @@ curl --request POST \ --- +### Error handling {#error-handling} + +Errors are returned in a standard format: + +`{"success": false, "msg": "No token provided"}` + +Errors are handled by error handling middleware and should be thrown with the following parameters + +| Name | Type | Default | Notes | +| ------- | --------- | ---------------------- | ------------------------------------ | +| status | `integer` | 500 | Standard HTTP codes | +| message | `string` | "Something went wrong" | An error message | +| service | `string` | "Unknown Service" | Name of service that threw the error | + +Example: + +``` +const myRoute = async(req, res, next) => { + try{ + const result = myRiskyOperationHere(); + } + catch(error){ + error.status = 404 + error.message = "Resource not found" + error.service = service name + next(error) + return; + } +} + +``` + +Errors should not be handled at the controller level and should be left to the middleware to handle. + ## Contributors diff --git a/Server/middleware/handleErrors.js b/Server/middleware/handleErrors.js index 2febc6b31..78764c1e1 100644 --- a/Server/middleware/handleErrors.js +++ b/Server/middleware/handleErrors.js @@ -1,9 +1,12 @@ const logger = require("../utils/logger"); const handleErrors = (error, req, res, next) => { - error.status = error.status ? error.status : 500; - logger.error(error.message, { service: error.service }); - res.status(error.status).json({ success: false, msg: error.message }); + const status = error.status || 500; + const message = error.message || "Something went wrong"; + const service = error.errorService || "Unknown service"; + + logger.error(error.message, { service: service }); + res.status(status).json({ success: false, msg: message }); }; module.exports = { handleErrors };