From 0a73a35547ce72d7bd33d53c2f49b0f55dbd1d8d Mon Sep 17 00:00:00 2001 From: Sabir Hassan Date: Mon, 27 Jun 2022 23:21:48 +0500 Subject: [PATCH] chore: improve error handling --- api/src/controllers/permission.ts | 54 ++++++++++++++++++---- api/src/routes/api/permission.ts | 16 +++++-- api/src/routes/api/spec/permission.spec.ts | 30 ++++++------ web/src/containers/Settings/permission.tsx | 24 ++++++++-- 4 files changed, 91 insertions(+), 33 deletions(-) diff --git a/api/src/controllers/permission.ts b/api/src/controllers/permission.ts index 608df5b..a5d8bf6 100644 --- a/api/src/controllers/permission.ts +++ b/api/src/controllers/permission.ts @@ -180,10 +180,19 @@ const createPermission = async ({ switch (principalType) { case 'user': { const userInDB = await User.findOne({ id: principalId }) - if (!userInDB) throw new Error('User not found.') + if (!userInDB) + throw { + code: 404, + status: 'Not Found', + message: 'User not found.' + } if (userInDB.isAdmin) - throw new Error('Can not add permission for admin user.') + throw { + code: 400, + status: 'Bad Request', + message: 'Can not add permission for admin user.' + } const alreadyExists = await Permission.findOne({ uri, @@ -191,7 +200,11 @@ const createPermission = async ({ }) if (alreadyExists) - throw new Error('Permission already exists with provided URI and User.') + throw { + code: 409, + status: 'Conflict', + message: 'Permission already exists with provided URI and User.' + } permission.user = userInDB._id @@ -205,16 +218,23 @@ const createPermission = async ({ } case 'group': { const groupInDB = await Group.findOne({ groupId: principalId }) - if (!groupInDB) throw new Error('Group not found.') + if (!groupInDB) + throw { + code: 404, + status: 'Not Found', + message: 'Group not found.' + } const alreadyExists = await Permission.findOne({ uri, group: groupInDB._id }) if (alreadyExists) - throw new Error( - 'Permission already exists with provided URI and Group.' - ) + throw { + code: 409, + status: 'Conflict', + message: 'Permission already exists with provided URI and Group.' + } permission.group = groupInDB._id @@ -226,7 +246,11 @@ const createPermission = async ({ break } default: - throw new Error('Invalid principal type. Valid types are user or group.') + throw { + code: 400, + status: 'Bad Request', + message: 'Invalid principal type. Valid types are user or group.' + } } const savedPermission = await permission.save() @@ -262,13 +286,23 @@ const updatePermission = async ( path: 'group', select: 'groupId name description -_id' })) as unknown as PermissionDetailsResponse - if (!updatedPermission) throw new Error('Unable to update permission') + if (!updatedPermission) + throw { + code: 404, + status: 'Not Found', + message: 'Permission not found.' + } return updatedPermission } const deletePermission = async (id: number) => { const permission = await Permission.findOne({ id }) - if (!permission) throw new Error('Permission is not found.') + if (!permission) + throw { + code: 404, + status: 'Not Found', + message: 'Permission not found.' + } await Permission.deleteOne({ id }) } diff --git a/api/src/routes/api/permission.ts b/api/src/routes/api/permission.ts index 8abc0a1..0dd98b0 100644 --- a/api/src/routes/api/permission.ts +++ b/api/src/routes/api/permission.ts @@ -14,7 +14,9 @@ permissionRouter.get('/', authenticateAccessToken, async (req, res) => { const response = await controller.getAllPermissions() res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + const statusCode = err.code + delete err.code + res.status(statusCode).send(err.message) } }) @@ -30,7 +32,9 @@ permissionRouter.post( const response = await controller.createPermission(body) res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + const statusCode = err.code + delete err.code + res.status(statusCode).send(err.message) } } ) @@ -49,7 +53,9 @@ permissionRouter.patch( const response = await controller.updatePermission(permissionId, body) res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + const statusCode = err.code + delete err.code + res.status(statusCode).send(err.message) } } ) @@ -65,7 +71,9 @@ permissionRouter.delete( await controller.deletePermission(permissionId) res.status(200).send('Permission Deleted!') } catch (err: any) { - res.status(403).send(err.toString()) + const statusCode = err.code + delete err.code + res.status(statusCode).send(err.message) } } ) diff --git a/api/src/routes/api/spec/permission.spec.ts b/api/src/routes/api/spec/permission.spec.ts index b740540..57295b9 100644 --- a/api/src/routes/api/spec/permission.spec.ts +++ b/api/src/routes/api/spec/permission.spec.ts @@ -190,7 +190,7 @@ describe('permission', () => { expect(res.body).toEqual({}) }) - it('should respond with forbidden Request (403) if user is not found', async () => { + it('should respond with not found (404) if user is not found', async () => { const res = await request(app) .post('/SASjsApi/permission') .auth(adminAccessToken, { type: 'bearer' }) @@ -198,13 +198,13 @@ describe('permission', () => { ...permission, principalId: 123 }) - .expect(403) + .expect(404) - expect(res.text).toEqual('Error: User not found.') + expect(res.text).toEqual('User not found.') expect(res.body).toEqual({}) }) - it('should respond with forbidden Request (403) if group is not found', async () => { + it('should respond with not found (404) if group is not found', async () => { const res = await request(app) .post('/SASjsApi/permission') .auth(adminAccessToken, { type: 'bearer' }) @@ -212,13 +212,13 @@ describe('permission', () => { ...permission, principalType: 'group' }) - .expect(403) + .expect(404) - expect(res.text).toEqual('Error: Group not found.') + expect(res.text).toEqual('Group not found.') expect(res.body).toEqual({}) }) - it('should respond with forbidden Request (403) if principal type is not valid', async () => { + it('should respond with Bad Request if principal type is not valid', async () => { const res = await request(app) .post('/SASjsApi/permission') .auth(adminAccessToken, { type: 'bearer' }) @@ -226,10 +226,10 @@ describe('permission', () => { ...permission, principalType: 'invalid' }) - .expect(403) + .expect(400) expect(res.text).toEqual( - 'Error: Invalid principal type. Valid types are user or group.' + 'Invalid principal type. Valid types are user or group.' ) expect(res.body).toEqual({}) }) @@ -295,16 +295,16 @@ describe('permission', () => { expect(res.body).toEqual({}) }) - it('should respond with forbidden Request (403) if permission with provided id does not exists', async () => { + it('should respond with not found (404) if permission with provided id does not exists', async () => { const res = await request(app) .patch('/SASjsApi/permission/123') .auth(adminAccessToken, { type: 'bearer' }) .send({ setting: 'deny' }) - .expect(403) + .expect(404) - expect(res.text).toEqual('Error: Unable to update permission') + expect(res.text).toEqual('Permission not found.') expect(res.body).toEqual({}) }) }) @@ -324,14 +324,14 @@ describe('permission', () => { expect(res.text).toEqual('Permission Deleted!') }) - it('should respond with forbidden Request (403) if permission with provided id does not exists', async () => { + it('should respond with not found (404) if permission with provided id does not exists', async () => { const res = await request(app) .delete('/SASjsApi/permission/123') .auth(adminAccessToken, { type: 'bearer' }) .send() - .expect(403) + .expect(404) - expect(res.text).toEqual('Error: Permission is not found.') + expect(res.text).toEqual('Permission not found.') }) }) diff --git a/web/src/containers/Settings/permission.tsx b/web/src/containers/Settings/permission.tsx index c6682ce..d4652b4 100644 --- a/web/src/containers/Settings/permission.tsx +++ b/web/src/containers/Settings/permission.tsx @@ -78,7 +78,11 @@ const Permission = () => { }) .catch((err) => { setModalTitle('Abort') - setModalPayload(typeof err === 'object' ? err.toSting() : err) + setModalPayload( + typeof err.response.data === 'object' + ? JSON.stringify(err.response.data) + : err.response.data + ) setOpenModal(true) }) }, []) @@ -176,7 +180,11 @@ const Permission = () => { }) .catch((err) => { setModalTitle('Abort') - setModalPayload(typeof err === 'object' ? err.toSting() : err) + setModalPayload( + typeof err.response.data === 'object' + ? JSON.stringify(err.response.data) + : err.response.data + ) setOpenModal(true) }) .finally(() => { @@ -204,7 +212,11 @@ const Permission = () => { }) .catch((err) => { setModalTitle('Abort') - setModalPayload(typeof err === 'object' ? err.toSting() : err) + setModalPayload( + typeof err.response.data === 'object' + ? JSON.stringify(err.response.data) + : err.response.data + ) setOpenModal(true) }) .finally(() => { @@ -231,7 +243,11 @@ const Permission = () => { }) .catch((err) => { setModalTitle('Abort') - setModalPayload(typeof err === 'object' ? err.toSting() : err) + setModalPayload( + typeof err.response.data === 'object' + ? JSON.stringify(err.response.data) + : err.response.data + ) setOpenModal(true) }) .finally(() => {