Merge pull request #48 from CS3219-AY2425S1/fix-async-error-handling-…

…user-service

Fix async error handling

backend/question-service/__tests__/routes/question.routes.test.ts

@@ -1,10 +1,11 @@
 import { MongoDBContainer, StartedMongoDBContainer } from '@testcontainers/mongodb'
 import express, { Express } from 'express'
-
+import 'express-async-errors'
 import mongoose from 'mongoose'
 import config from '../../src/common/config.util'
 import logger from '../../src/common/logger.util'
 import connectToDatabase from '../../src/common/mongodb.util'
+import defaultErrorHandler from '../../src/middlewares/errorHandler.middleware'
 import questionRouter from '../../src/routes/question.routes'
 
 jest.mock('../../src/common/config.util', () => ({
@@ -29,6 +30,7 @@ describe('Question Routes', () => {
         app = express()
         app.use(express.json())
         app.use('/questions', questionRouter)
+        app.use(defaultErrorHandler)
 
         await connectToDatabase(connectionString)
     }, 60000)

backend/question-service/src/index.ts

@@ -1,8 +1,9 @@
 import cors from 'cors'
 import express, { Express, NextFunction, Request, Response } from 'express'
+import 'express-async-errors'
 import helmet from 'helmet'
 import passport from 'passport'
-import logger from './common/logger.util'
+import defaultErrorHandler from './middlewares/errorHandler.middleware'
 import questionRouter from './routes/question.routes'
 
 const app: Express = express()
@@ -45,9 +46,6 @@ app.use((_: Request, response: Response) => {
 })
 
 // Default Error Handler
-app.use((error: Error, request: Request, response: Response) => {
-    logger.error(`[Controller] [${request.method}  ${request.baseUrl + request.path}] ${error.message}`)
-    response.status(500).send()
-})
+app.use(defaultErrorHandler)
 
 export default app

backend/question-service/src/middlewares/errorHandler.middleware.ts

@@ -0,0 +1,16 @@
+import { NextFunction, Request, Response } from 'express'
+import mongoose from 'mongoose'
+import logger from '../common/logger.util'
+
+export default (error: Error, request: Request, response: Response, _: NextFunction) => {
+    if (error instanceof mongoose.Error.CastError) {
+        response.status(400).json('INVALID_ID').send()
+    } else if (error instanceof mongoose.Error.DocumentNotFoundError) {
+        response.status(404).json('NOT_FOUND').send()
+    } else if (error instanceof mongoose.Error.ValidationError) {
+        response.status(400).json('INVALID_DATA').send()
+    } else {
+        logger.error(`[Controller] [${request.method}  ${request.baseUrl + request.path}] ${error.message}`)
+        response.status(500).send()
+    }
+}

backend/user-service/__tests__/routes/user.routes.test.ts

@@ -1,12 +1,13 @@
 import { MongoDBContainer, StartedMongoDBContainer } from '@testcontainers/mongodb'
 import { generateKeyPairSync } from 'crypto'
 import express, { Express } from 'express'
-
+import 'express-async-errors'
 import mongoose from 'mongoose'
 import request from 'supertest'
 import config from '../../src/common/config.util'
 import logger from '../../src/common/logger.util'
 import connectToDatabase from '../../src/common/mongodb.util'
+import defaultErrorHandler from '../../src/middlewares/errorHandler.middleware'
 import userRouter from '../../src/routes/user.routes'
 import { Proficiency } from '../../src/types/Proficiency'
 import { Role } from '../../src/types/Role'
@@ -59,6 +60,7 @@ describe('User Routes', () => {
         app = express()
         app.use(express.json())
         app.use('/users', userRouter)
+        app.use(defaultErrorHandler)
 
         await connectToDatabase(connectionString)
     }, 60000)
@@ -109,13 +111,12 @@ describe('User Routes', () => {
             expect(response.body.username).toEqual('test3')
             expect(response.body.proficiency).toEqual(Proficiency.ADVANCED)
         })
-        it('should return 500 for requests with invalid ids', async () => {
+        it('should return 400 for requests with invalid ids', async () => {
             const response = await request(app).put('/users/111').send({
                 username: 'test3',
                 proficiency: Proficiency.ADVANCED,
             })
-            expect(response.status).toBe(500)
-            expect(response.body).toHaveLength(1)
+            expect(response.status).toBe(400)
         })
         it('should return 400 for invalid requests and a list of errors', async () => {
             const response = await request(app).put('/users/111').send({})

backend/user-service/package.json

@@ -18,6 +18,7 @@
         "cors": "^2.8.5",
         "dotenv": "^16.4.5",
         "express": "^4.21.0",
+        "express-async-errors": "^3.1.1",
         "helmet": "^7.1.0",
         "jsonwebtoken": "^9.0.2",
         "mongoose": "^8.6.3",

backend/user-service/src/controllers/user.controller.ts

@@ -1,13 +1,12 @@
 import { createUser, findOneUserByEmail, findOneUserByUsername, updateUser } from '../models/user.repository'
 
-import { CreateUserDto } from '../types/CreateUserDto'
+import { ValidationError } from 'class-validator'
 import { Response } from 'express'
+import { CreateUserDto } from '../types/CreateUserDto'
 import { TypedRequest } from '../types/TypedRequest'
 import { UserDto } from '../types/UserDto'
 import { UserProfileDto } from '../types/UserProfileDto'
-import { ValidationError } from 'class-validator'
 import { hashPassword } from './auth.controller'
-import logger from '../common/logger.util'
 
 export async function handleCreateUser(request: TypedRequest<CreateUserDto>, response: Response): Promise<void> {
     const createDto = CreateUserDto.fromRequest(request)
@@ -53,11 +52,6 @@ export async function handleUpdateProfile(request: TypedRequest<UserProfileDto>,
         return
     }
 
-    try {
-        const user = await updateUser(id, createDto)
-        response.status(200).json(user).send()
-    } catch (e) {
-        logger.error(e)
-        response.status(500).json(['INVALID_USER_ID']).send()
-    }
+    const user = await updateUser(id, createDto)
+    response.status(200).json(user).send()
 }

backend/user-service/src/index.ts

@@ -1,8 +1,9 @@
 import cors from 'cors'
 import express, { Express, NextFunction, Request, Response } from 'express'
+import 'express-async-errors'
 import helmet from 'helmet'
 import passport from 'passport'
-import logger from './common/logger.util'
+import defaultErrorHandler from './middlewares/errorHandler.middleware'
 import authRouter from './routes/auth.routes'
 import userRouter from './routes/user.routes'
 
@@ -17,7 +18,7 @@ app.use(helmet())
 app.use(passport.initialize())
 
 // To handle CORS Errors
-app.use(async (request: Request, response: Response, next: NextFunction): Promise<void> => {
+app.use((request: Request, response: Response, next: NextFunction) => {
     response.header('Access-Control-Allow-Origin', '*') // "*" -> Allow all links to access
 
     response.header('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept, Authorization')
@@ -37,19 +38,16 @@ app.use('/auth', authRouter)
 app.use('/users', userRouter)
 
 // Health Check Route
-app.get('/', async (_: Request, response: Response): Promise<void> => {
+app.get('/', (_: Request, response: Response) => {
     response.status(200).send()
 })
 
 //  Not Found Route
-app.use(async (_: Request, response: Response): Promise<void> => {
+app.use((_: Request, response: Response) => {
     response.status(404).send()
 })
 
 // Default Error Handler
-app.use(async (error: Error, request: Request, response: Response): Promise<void> => {
-    logger.error(`[Controller] [${request.method}  ${request.baseUrl + request.path}] ${error.message}`)
-    response.status(500).send()
-})
+app.use(defaultErrorHandler)
 
 export default app

backend/user-service/src/middlewares/errorHandler.middleware.ts

@@ -0,0 +1,16 @@
+import { NextFunction, Request, Response } from 'express'
+import mongoose from 'mongoose'
+import logger from '../common/logger.util'
+
+export default (error: Error, request: Request, response: Response, _: NextFunction) => {
+    if (error instanceof mongoose.Error.CastError) {
+        response.status(400).json('INVALID_ID').send()
+    } else if (error instanceof mongoose.Error.DocumentNotFoundError) {
+        response.status(404).json('NOT_FOUND').send()
+    } else if (error instanceof mongoose.Error.ValidationError) {
+        response.status(400).json('INVALID_DATA').send()
+    } else {
+        logger.error(`[Controller] [${request.method}  ${request.baseUrl + request.path}] ${error.message}`)
+        response.status(500).send()
+    }
+}

eslint.config.mjs

@@ -1,12 +1,17 @@
-import globals from 'globals'
 import pluginJs from '@eslint/js'
+import globals from 'globals'
 import tseslint from 'typescript-eslint'
 
 export default [
     { files: ['**/*.{js,mjs,cjs,ts}'] },
     { languageOptions: { globals: globals.browser } },
     pluginJs.configs.recommended,
     ...tseslint.configs.recommended,
+    {
+        rules: {
+            '@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_' }],
+        },
+    },
     {
         ignores: [
             '**/dist/*',