From 458580ec87fb1cad31f8e3c4e5dac929ae52e50f Mon Sep 17 00:00:00 2001 From: Sebastien Bruel <93573440+sbruel@users.noreply.github.com> Date: Sat, 5 Jul 2025 00:51:46 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=A5=85=20refactor:=20Express=20App=20defa?= =?UTF-8?q?ult=20Error=20Handling=20with=20`ErrorController`=20(#8249)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/server/controllers/ErrorController.js | 18 +- .../controllers/ErrorController.spec.js | 241 ++++++++++++++++++ api/server/index.js | 4 +- api/server/index.spec.js | 25 +- api/server/middleware/checkBan.js | 2 +- 5 files changed, 281 insertions(+), 9 deletions(-) create mode 100644 api/server/controllers/ErrorController.spec.js diff --git a/api/server/controllers/ErrorController.js b/api/server/controllers/ErrorController.js index 234cb90fb3..6907e63d9c 100644 --- a/api/server/controllers/ErrorController.js +++ b/api/server/controllers/ErrorController.js @@ -24,17 +24,23 @@ const handleValidationError = (err, res) => { } }; -// eslint-disable-next-line no-unused-vars -module.exports = (err, req, res, next) => { +module.exports = (err, _req, res, _next) => { try { if (err.name === 'ValidationError') { - return (err = handleValidationError(err, res)); + return handleValidationError(err, res); } if (err.code && err.code == 11000) { - return (err = handleDuplicateKeyError(err, res)); + return handleDuplicateKeyError(err, res); } - } catch (err) { + // Special handling for errors like SyntaxError + if (err.statusCode && err.body) { + return res.status(err.statusCode).send(err.body); + } + logger.error('ErrorController => error', err); - res.status(500).send('An unknown error occurred.'); + return res.status(500).send('An unknown error occurred.'); + } catch (err) { + logger.error('ErrorController => processing error', err); + return res.status(500).send('Processing error in ErrorController.'); } }; diff --git a/api/server/controllers/ErrorController.spec.js b/api/server/controllers/ErrorController.spec.js new file mode 100644 index 0000000000..c46315a5e5 --- /dev/null +++ b/api/server/controllers/ErrorController.spec.js @@ -0,0 +1,241 @@ +const errorController = require('./ErrorController'); +const { logger } = require('~/config'); + +// Mock the logger +jest.mock('~/config', () => ({ + logger: { + error: jest.fn(), + }, +})); + +describe('ErrorController', () => { + let mockReq, mockRes, mockNext; + + beforeEach(() => { + mockReq = {}; + mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn(), + }; + mockNext = jest.fn(); + logger.error.mockClear(); + }); + + describe('ValidationError handling', () => { + it('should handle ValidationError with single error', () => { + const validationError = { + name: 'ValidationError', + errors: { + email: { message: 'Email is required', path: 'email' }, + }, + }; + + errorController(validationError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.send).toHaveBeenCalledWith({ + messages: '["Email is required"]', + fields: '["email"]', + }); + expect(logger.error).toHaveBeenCalledWith('Validation error:', validationError.errors); + }); + + it('should handle ValidationError with multiple errors', () => { + const validationError = { + name: 'ValidationError', + errors: { + email: { message: 'Email is required', path: 'email' }, + password: { message: 'Password is required', path: 'password' }, + }, + }; + + errorController(validationError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.send).toHaveBeenCalledWith({ + messages: '"Email is required Password is required"', + fields: '["email","password"]', + }); + expect(logger.error).toHaveBeenCalledWith('Validation error:', validationError.errors); + }); + + it('should handle ValidationError with empty errors object', () => { + const validationError = { + name: 'ValidationError', + errors: {}, + }; + + errorController(validationError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.send).toHaveBeenCalledWith({ + messages: '[]', + fields: '[]', + }); + }); + }); + + describe('Duplicate key error handling', () => { + it('should handle duplicate key error (code 11000)', () => { + const duplicateKeyError = { + code: 11000, + keyValue: { email: 'test@example.com' }, + }; + + errorController(duplicateKeyError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(409); + expect(mockRes.send).toHaveBeenCalledWith({ + messages: 'An document with that ["email"] already exists.', + fields: '["email"]', + }); + expect(logger.error).toHaveBeenCalledWith('Duplicate key error:', duplicateKeyError.keyValue); + }); + + it('should handle duplicate key error with multiple fields', () => { + const duplicateKeyError = { + code: 11000, + keyValue: { email: 'test@example.com', username: 'testuser' }, + }; + + errorController(duplicateKeyError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(409); + expect(mockRes.send).toHaveBeenCalledWith({ + messages: 'An document with that ["email","username"] already exists.', + fields: '["email","username"]', + }); + expect(logger.error).toHaveBeenCalledWith('Duplicate key error:', duplicateKeyError.keyValue); + }); + + it('should handle error with code 11000 as string', () => { + const duplicateKeyError = { + code: '11000', + keyValue: { email: 'test@example.com' }, + }; + + errorController(duplicateKeyError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(409); + expect(mockRes.send).toHaveBeenCalledWith({ + messages: 'An document with that ["email"] already exists.', + fields: '["email"]', + }); + }); + }); + + describe('SyntaxError handling', () => { + it('should handle errors with statusCode and body', () => { + const syntaxError = { + statusCode: 400, + body: 'Invalid JSON syntax', + }; + + errorController(syntaxError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.send).toHaveBeenCalledWith('Invalid JSON syntax'); + }); + + it('should handle errors with different statusCode and body', () => { + const customError = { + statusCode: 422, + body: { error: 'Unprocessable entity' }, + }; + + errorController(customError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(422); + expect(mockRes.send).toHaveBeenCalledWith({ error: 'Unprocessable entity' }); + }); + + it('should handle error with statusCode but no body', () => { + const partialError = { + statusCode: 400, + }; + + errorController(partialError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); + }); + + it('should handle error with body but no statusCode', () => { + const partialError = { + body: 'Some error message', + }; + + errorController(partialError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); + }); + }); + + describe('Unknown error handling', () => { + it('should handle unknown errors', () => { + const unknownError = new Error('Some unknown error'); + + errorController(unknownError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); + expect(logger.error).toHaveBeenCalledWith('ErrorController => error', unknownError); + }); + + it('should handle errors with code other than 11000', () => { + const mongoError = { + code: 11100, + message: 'Some MongoDB error', + }; + + errorController(mongoError, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(mockRes.send).toHaveBeenCalledWith('An unknown error occurred.'); + expect(logger.error).toHaveBeenCalledWith('ErrorController => error', mongoError); + }); + + it('should handle null/undefined errors', () => { + errorController(null, mockReq, mockRes, mockNext); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(mockRes.send).toHaveBeenCalledWith('Processing error in ErrorController.'); + expect(logger.error).toHaveBeenCalledWith( + 'ErrorController => processing error', + expect.any(Error), + ); + }); + }); + + describe('Catch block handling', () => { + beforeEach(() => { + // Restore logger mock to normal behavior for these tests + logger.error.mockRestore(); + logger.error = jest.fn(); + }); + + it('should handle errors when logger.error throws', () => { + // Create fresh mocks for this test + const freshMockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn(), + }; + + // Mock logger to throw on the first call, succeed on the second + logger.error + .mockImplementationOnce(() => { + throw new Error('Logger error'); + }) + .mockImplementation(() => {}); + + const testError = new Error('Test error'); + + errorController(testError, mockReq, freshMockRes, mockNext); + + expect(freshMockRes.status).toHaveBeenCalledWith(500); + expect(freshMockRes.send).toHaveBeenCalledWith('Processing error in ErrorController.'); + expect(logger.error).toHaveBeenCalledTimes(2); + }); + }); +}); diff --git a/api/server/index.js b/api/server/index.js index ac79a627e9..2da1adfcde 100644 --- a/api/server/index.js +++ b/api/server/index.js @@ -55,7 +55,6 @@ const startServer = async () => { /* Middleware */ app.use(noIndex); - app.use(errorController); app.use(express.json({ limit: '3mb' })); app.use(express.urlencoded({ extended: true, limit: '3mb' })); app.use(mongoSanitize()); @@ -121,6 +120,9 @@ const startServer = async () => { app.use('/api/tags', routes.tags); app.use('/api/mcp', routes.mcp); + // Add the error controller one more time after all routes + app.use(errorController); + app.use((req, res) => { res.set({ 'Cache-Control': process.env.INDEX_CACHE_CONTROL || 'no-cache, no-store, must-revalidate', diff --git a/api/server/index.spec.js b/api/server/index.spec.js index 25b5ab9f03..43ad57108f 100644 --- a/api/server/index.spec.js +++ b/api/server/index.spec.js @@ -1,5 +1,4 @@ const fs = require('fs'); -const path = require('path'); const request = require('supertest'); const { MongoMemoryServer } = require('mongodb-memory-server'); const mongoose = require('mongoose'); @@ -59,6 +58,30 @@ describe('Server Configuration', () => { expect(response.headers['pragma']).toBe('no-cache'); expect(response.headers['expires']).toBe('0'); }); + + it('should return 500 for unknown errors via ErrorController', async () => { + // Testing the error handling here on top of unit tests to ensure the middleware is correctly integrated + + // Mock MongoDB operations to fail + const originalFindOne = mongoose.models.User.findOne; + const mockError = new Error('MongoDB operation failed'); + mongoose.models.User.findOne = jest.fn().mockImplementation(() => { + throw mockError; + }); + + try { + const response = await request(app).post('/api/auth/login').send({ + email: 'test@example.com', + password: 'password123', + }); + + expect(response.status).toBe(500); + expect(response.text).toBe('An unknown error occurred.'); + } finally { + // Restore original function + mongoose.models.User.findOne = originalFindOne; + } + }); }); // Polls the /health endpoint every 30ms for up to 10 seconds to wait for the server to start completely diff --git a/api/server/middleware/checkBan.js b/api/server/middleware/checkBan.js index 91c31ab66a..ad4e4c86ec 100644 --- a/api/server/middleware/checkBan.js +++ b/api/server/middleware/checkBan.js @@ -18,7 +18,6 @@ const message = 'Your account has been temporarily banned due to violations of o * @function * @param {Object} req - Express Request object. * @param {Object} res - Express Response object. - * @param {String} errorMessage - Error message to be displayed in case of /api/ask or /api/edit request. * * @returns {Promise} - Returns a Promise which when resolved sends a response status of 403 with a specific message if request is not of api/ask or api/edit types. If it is, calls `denyRequest()` function. */ @@ -135,6 +134,7 @@ const checkBan = async (req, res, next = () => {}) => { return await banResponse(req, res); } catch (error) { logger.error('Error in checkBan middleware:', error); + return next(error); } };