Skip to content

Commit

Permalink
chore: remove uses of type errors from user-facing code (#3553)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
thomasheartman committed Apr 18, 2023
1 parent e11fae5 commit 34204c3
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 11 deletions.
35 changes: 29 additions & 6 deletions 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';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -543,12 +554,18 @@ export default class ProjectService {
): Promise<void> {
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....
Expand Down Expand Up @@ -592,11 +609,17 @@ export default class ProjectService {
): Promise<void> {
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....
Expand Down
4 changes: 3 additions & 1 deletion 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 {
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions 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 {
Expand Down Expand Up @@ -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');
Expand Down
4 changes: 2 additions & 2 deletions 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;
Expand Down Expand Up @@ -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');
Expand Down

0 comments on commit 34204c3

Please sign in to comment.