From 34204c3565181e90d03e0dbb03de5ae30ed4972e Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 18 Apr 2023 13:42:07 +0200 Subject: [PATCH] chore: remove uses of type errors from user-facing code (#3553) BREAKING CHANGE: This changes the `name` property of a small number of error responses that we return. The property would have been `TypeError`, but is now `ValidationError` instead. It's a grey area, but I'd rather be strict. --- This change removes uses of the `TypeError` type from user-facing code. Type errors are used by typescript when you provide it the wrong type. This is a valid concern. However, in the API, they're usually a signal that **we've** done something wrong rather than the user having done something wrong. As such, it makes more sense to return them as validation errors or bad request errors. ## Breaking changes Note that because of the way we handle errors, some of these changes will be made visible to the end user, but only in the response body. ```ts { "name": "TypeError", "message": "Something is wrong", "isJoi": true } ``` will become ```ts { "name": "ValidationError", "message": "Something is wrong", "isJoi": true } ``` Technically, this could be considered a breaking change. However, as we're gearing up for v5, this might be a good time to merge that? ## A return to 500 This PR also makes TypeErrors a 500-type error again because they should never be caused by invalid data provided by the user --- src/lib/services/project-service.ts | 35 ++++++++++++++++++++++++----- src/lib/types/api-user.ts | 4 +++- src/lib/types/group.ts | 4 ++-- src/lib/types/user.ts | 4 ++-- 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/lib/services/project-service.ts b/src/lib/services/project-service.ts index a50d5ed4b3..c5eb0e0ddc 100644 --- a/src/lib/services/project-service.ts +++ b/src/lib/services/project-service.ts @@ -1,4 +1,5 @@ import { subDays } from 'date-fns'; +import { ValidationError } from 'joi'; import User, { IUser } from '../types/user'; import { AccessService } from './access-service'; import NameExistsError from '../error/name-exists-error'; @@ -423,7 +424,12 @@ export default class ProjectService { const role = await this.accessService.getRole(roleId); const group = await this.groupService.getGroup(groupId); const project = await this.getProject(projectId); - if (group.id == null) throw new TypeError('Unexpected empty group id'); + if (group.id == null) + throw new ValidationError( + 'Unexpected empty group id', + [], + undefined, + ); await this.accessService.addGroupToRole( group.id, @@ -454,7 +460,12 @@ export default class ProjectService { const group = await this.groupService.getGroup(groupId); const role = await this.accessService.getRole(roleId); const project = await this.getProject(projectId); - if (group.id == null) throw new TypeError('Unexpected empty group id'); + if (group.id == null) + throw new ValidationError( + 'Unexpected empty group id', + [], + undefined, + ); await this.accessService.removeGroupFromRole( group.id, @@ -543,12 +554,18 @@ export default class ProjectService { ): Promise { const usersWithRoles = await this.getAccessToProject(projectId); const user = usersWithRoles.users.find((u) => u.id === userId); - if (!user) throw new TypeError('Unexpected empty user'); + if (!user) + throw new ValidationError('Unexpected empty user', [], undefined); const currentRole = usersWithRoles.roles.find( (r) => r.id === user.roleId, ); - if (!currentRole) throw new TypeError('Unexpected empty current role'); + if (!currentRole) + throw new ValidationError( + 'Unexpected empty current role', + [], + undefined, + ); if (currentRole.id === roleId) { // Nothing to do.... @@ -592,11 +609,17 @@ export default class ProjectService { ): Promise { const usersWithRoles = await this.getAccessToProject(projectId); const user = usersWithRoles.groups.find((u) => u.id === userId); - if (!user) throw new TypeError('Unexpected empty user'); + if (!user) + throw new ValidationError('Unexpected empty user', [], undefined); const currentRole = usersWithRoles.roles.find( (r) => r.id === user.roleId, ); - if (!currentRole) throw new TypeError('Unexpected empty current role'); + if (!currentRole) + throw new ValidationError( + 'Unexpected empty current role', + [], + undefined, + ); if (currentRole.id === roleId) { // Nothing to do.... diff --git a/src/lib/types/api-user.ts b/src/lib/types/api-user.ts index dfd325feb7..86a663fd0a 100644 --- a/src/lib/types/api-user.ts +++ b/src/lib/types/api-user.ts @@ -1,4 +1,6 @@ import { ApiTokenType } from './models/api-token'; +import { ValidationError } from 'joi'; + import { CLIENT } from './permissions'; interface IApiUserData { @@ -36,7 +38,7 @@ export default class ApiUser { secret, }: IApiUserData) { if (!username) { - throw new TypeError('username is required'); + throw new ValidationError('username is required', [], undefined); } this.username = username; this.permissions = permissions; diff --git a/src/lib/types/group.ts b/src/lib/types/group.ts index e987334372..8d5b505751 100644 --- a/src/lib/types/group.ts +++ b/src/lib/types/group.ts @@ -1,4 +1,4 @@ -import Joi from 'joi'; +import Joi, { ValidationError } from 'joi'; import { IUser } from './user'; export interface IGroup { @@ -71,7 +71,7 @@ export default class Group implements IGroup { createdAt, }: IGroup) { if (!id) { - throw new TypeError('Id is required'); + throw new ValidationError('Id is required', [], undefined); } Joi.assert(name, Joi.string(), 'Name'); diff --git a/src/lib/types/user.ts b/src/lib/types/user.ts index 44ee864cbe..a733503656 100644 --- a/src/lib/types/user.ts +++ b/src/lib/types/user.ts @@ -1,4 +1,4 @@ -import Joi from 'joi'; +import Joi, { ValidationError } from 'joi'; import { generateImageUrl } from '../util/generateImageUrl'; export const AccountTypes = ['User', 'Service Account'] as const; @@ -70,7 +70,7 @@ export default class User implements IUser { isService, }: UserData) { if (!id) { - throw new TypeError('Id is required'); + throw new ValidationError('Id is required', [], undefined); } Joi.assert(email, Joi.string().email(), 'Email'); Joi.assert(username, Joi.string(), 'Username');