🥅 refactor: Express App default Error Handling with ErrorController (#8249)

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.');
   }
 };

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);
+    });
+  });
+});

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',

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

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<Object>} - 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);
   }
 };