1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-02-23 00:22:19 +01:00

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
This commit is contained in:
Thomas Heartman 2023-04-18 13:42:07 +02:00 committed by GitHub
parent e11fae56e9
commit 34204c3565
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 36 additions and 11 deletions

View File

@ -1,4 +1,5 @@
import { subDays } from 'date-fns'; import { subDays } from 'date-fns';
import { ValidationError } from 'joi';
import User, { IUser } from '../types/user'; import User, { IUser } from '../types/user';
import { AccessService } from './access-service'; import { AccessService } from './access-service';
import NameExistsError from '../error/name-exists-error'; import NameExistsError from '../error/name-exists-error';
@ -423,7 +424,12 @@ export default class ProjectService {
const role = await this.accessService.getRole(roleId); const role = await this.accessService.getRole(roleId);
const group = await this.groupService.getGroup(groupId); const group = await this.groupService.getGroup(groupId);
const project = await this.getProject(projectId); 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( await this.accessService.addGroupToRole(
group.id, group.id,
@ -454,7 +460,12 @@ export default class ProjectService {
const group = await this.groupService.getGroup(groupId); const group = await this.groupService.getGroup(groupId);
const role = await this.accessService.getRole(roleId); const role = await this.accessService.getRole(roleId);
const project = await this.getProject(projectId); 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( await this.accessService.removeGroupFromRole(
group.id, group.id,
@ -543,12 +554,18 @@ export default class ProjectService {
): Promise<void> { ): Promise<void> {
const usersWithRoles = await this.getAccessToProject(projectId); const usersWithRoles = await this.getAccessToProject(projectId);
const user = usersWithRoles.users.find((u) => u.id === userId); 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( const currentRole = usersWithRoles.roles.find(
(r) => r.id === user.roleId, (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) { if (currentRole.id === roleId) {
// Nothing to do.... // Nothing to do....
@ -592,11 +609,17 @@ export default class ProjectService {
): Promise<void> { ): Promise<void> {
const usersWithRoles = await this.getAccessToProject(projectId); const usersWithRoles = await this.getAccessToProject(projectId);
const user = usersWithRoles.groups.find((u) => u.id === userId); 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( const currentRole = usersWithRoles.roles.find(
(r) => r.id === user.roleId, (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) { if (currentRole.id === roleId) {
// Nothing to do.... // Nothing to do....

View File

@ -1,4 +1,6 @@
import { ApiTokenType } from './models/api-token'; import { ApiTokenType } from './models/api-token';
import { ValidationError } from 'joi';
import { CLIENT } from './permissions'; import { CLIENT } from './permissions';
interface IApiUserData { interface IApiUserData {
@ -36,7 +38,7 @@ export default class ApiUser {
secret, secret,
}: IApiUserData) { }: IApiUserData) {
if (!username) { if (!username) {
throw new TypeError('username is required'); throw new ValidationError('username is required', [], undefined);
} }
this.username = username; this.username = username;
this.permissions = permissions; this.permissions = permissions;

View File

@ -1,4 +1,4 @@
import Joi from 'joi'; import Joi, { ValidationError } from 'joi';
import { IUser } from './user'; import { IUser } from './user';
export interface IGroup { export interface IGroup {
@ -71,7 +71,7 @@ export default class Group implements IGroup {
createdAt, createdAt,
}: IGroup) { }: IGroup) {
if (!id) { if (!id) {
throw new TypeError('Id is required'); throw new ValidationError('Id is required', [], undefined);
} }
Joi.assert(name, Joi.string(), 'Name'); Joi.assert(name, Joi.string(), 'Name');

View File

@ -1,4 +1,4 @@
import Joi from 'joi'; import Joi, { ValidationError } from 'joi';
import { generateImageUrl } from '../util/generateImageUrl'; import { generateImageUrl } from '../util/generateImageUrl';
export const AccountTypes = ['User', 'Service Account'] as const; export const AccountTypes = ['User', 'Service Account'] as const;
@ -70,7 +70,7 @@ export default class User implements IUser {
isService, isService,
}: UserData) { }: UserData) {
if (!id) { if (!id) {
throw new TypeError('Id is required'); throw new ValidationError('Id is required', [], undefined);
} }
Joi.assert(email, Joi.string().email(), 'Email'); Joi.assert(email, Joi.string().email(), 'Email');
Joi.assert(username, Joi.string(), 'Username'); Joi.assert(username, Joi.string(), 'Username');