1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-25 00:07:47 +01:00

chore!: [v6] remove error.description in error messages (#7157)

In preparation for v6, this PR removes usage and references to
`error.description` instead favoring `error.message` (as mentioned
#4380)

I found no references in the front end, so this might be (I believe it
to be) all the required changes.
This commit is contained in:
Thomas Heartman 2024-05-27 11:26:19 +02:00 committed by GitHub
parent 17720d6185
commit f518b12b07
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
15 changed files with 70 additions and 86 deletions

View File

@ -4,7 +4,6 @@ import getProp from 'lodash.get';
import { type ApiErrorSchema, UnleashError } from './unleash-error'; import { type ApiErrorSchema, UnleashError } from './unleash-error';
type ValidationErrorDescription = { type ValidationErrorDescription = {
description?: string;
message: string; message: string;
path?: string; path?: string;
}; };
@ -24,7 +23,7 @@ class BadDataError extends UnleashError {
}`; }`;
super(topLevelMessage); super(topLevelMessage);
this.details = errors ?? [{ message: message, description: message }]; this.details = errors ?? [{ message: message }];
} }
toJSON(): ApiErrorSchema { toJSON(): ApiErrorSchema {
@ -45,11 +44,10 @@ const missingRequiredPropertyMessage = (
missingPropertyName: string, missingPropertyName: string,
) => { ) => {
const path = constructPath(pathToParentObject, missingPropertyName); const path = constructPath(pathToParentObject, missingPropertyName);
const description = `The \`${path}\` property is required. It was not present on the data you sent.`; const message = `The \`${path}\` property is required. It was not present on the data you sent.`;
return { return {
path, path,
description, message,
message: description,
}; };
}; };
@ -58,14 +56,13 @@ const additionalPropertiesMessage = (
additionalPropertyName: string, additionalPropertyName: string,
) => { ) => {
const path = constructPath(pathToParentObject, additionalPropertyName); const path = constructPath(pathToParentObject, additionalPropertyName);
const description = `The ${ const message = `The ${
pathToParentObject ? `\`${pathToParentObject}\`` : 'root' pathToParentObject ? `\`${pathToParentObject}\`` : 'root'
} object of the request body does not allow additional properties. Your request included the \`${path}\` property.`; } object of the request body does not allow additional properties. Your request included the \`${path}\` property.`;
return { return {
path, path,
description, message,
message: description,
}; };
}; };
@ -77,10 +74,9 @@ const genericErrorMessage = (
const input = getProp(requestBody, propertyName.split('/')); const input = getProp(requestBody, propertyName.split('/'));
const youSent = JSON.stringify(input); const youSent = JSON.stringify(input);
const description = `The \`${propertyName}\` property ${errorMessage}. You sent ${youSent}.`; const message = `The \`${propertyName}\` property ${errorMessage}. You sent ${youSent}.`;
return { return {
description, message,
message: description,
path: propertyName, path: propertyName,
}; };
}; };
@ -92,11 +88,10 @@ const oneOfMessage = (
const errorPosition = const errorPosition =
propertyName === '' ? 'root object' : `"${propertyName}" property`; propertyName === '' ? 'root object' : `"${propertyName}" property`;
const description = `The ${errorPosition} ${errorMessage}. The data you provided matches more than one option in the schema. These options are mutually exclusive. Please refer back to the schema and remove any excess properties.`; const message = `The ${errorPosition} ${errorMessage}. The data you provided matches more than one option in the schema. These options are mutually exclusive. Please refer back to the schema and remove any excess properties.`;
return { return {
description, message,
message: description,
path: propertyName, path: propertyName,
}; };
}; };
@ -107,7 +102,7 @@ const enumMessage = (
allowedValues: string[], allowedValues: string[],
suppliedValue: string | null | undefined, suppliedValue: string | null | undefined,
) => { ) => {
const description = `The \`${propertyName}\` property ${ const fullMessage = `The \`${propertyName}\` property ${
message ?? 'must match one of the allowed values' message ?? 'must match one of the allowed values'
}: ${allowedValues }: ${allowedValues
.map((value) => `"${value}"`) .map((value) => `"${value}"`)
@ -116,8 +111,7 @@ const enumMessage = (
)}. You provided "${suppliedValue}", which is not valid. Please use one of the allowed values instead..`; )}. You provided "${suppliedValue}", which is not valid. Please use one of the allowed values instead..`;
return { return {
description, message: fullMessage,
message: description,
path: propertyName, path: propertyName,
}; };
}; };
@ -176,10 +170,9 @@ export const fromJoiError = (err: ValidationError): BadDataError => {
const messageEnd = detail.context?.value const messageEnd = detail.context?.value
? `. You provided ${JSON.stringify(detail.context.value)}.` ? `. You provided ${JSON.stringify(detail.context.value)}.`
: '.'; : '.';
const description = detail.message + messageEnd; const message = detail.message + messageEnd;
return { return {
description, message,
message: description,
}; };
}); });

View File

@ -14,7 +14,6 @@ export default class IncompatibleProjectError extends UnleashError {
{ {
validationErrors: [], validationErrors: [],
message: this.message, message: this.message,
description: this.message,
}, },
], ],
}; };

View File

@ -14,7 +14,6 @@ export default class PasswordUndefinedError extends UnleashError {
{ {
validationErrors: [], validationErrors: [],
message: this.message, message: this.message,
description: this.message,
}, },
], ],
}; };

View File

@ -12,7 +12,6 @@ export default class ProjectWithoutOwnerError extends UnleashError {
...super.toJSON(), ...super.toJSON(),
details: [ details: [
{ {
description: this.message,
message: this.message, message: this.message,
validationErrors: [], validationErrors: [],
}, },

View File

@ -25,7 +25,6 @@ describe('v5 deprecation: backwards compatibility', () => {
details: [ details: [
{ {
message, message,
description: message,
}, },
], ],
}); });
@ -40,7 +39,6 @@ describe('Standard/legacy error conversion', () => {
expect(result.details).toStrictEqual([ expect(result.details).toStrictEqual([
{ {
message, message,
description: message,
}, },
]); ]);
}); });
@ -61,14 +59,14 @@ describe('OpenAPI error conversion', () => {
const result = fromOpenApiValidationError({})(error); const result = fromOpenApiValidationError({})(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: message:
// it tells the user that the property is required // it tells the user that the property is required
expect.stringContaining('required'), expect.stringContaining('required'),
path: 'enabled', path: 'enabled',
}); });
// it tells the user the name of the missing property // it tells the user the name of the missing property
expect(result.description).toContain(error.params.missingProperty); expect(result.message).toContain(error.params.missingProperty);
}); });
it('Gives useful error messages for type errors', () => { it('Gives useful error messages for type errors', () => {
@ -89,14 +87,14 @@ describe('OpenAPI error conversion', () => {
})(error); })(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: message:
// it provides the message // it provides the message
expect.stringContaining(error.message), expect.stringContaining(error.message),
path: 'parameters', path: 'parameters',
}); });
// it tells the user what they provided // it tells the user what they provided
expect(result.description).toContain(JSON.stringify(parameterValue)); expect(result.message).toContain(JSON.stringify(parameterValue));
}); });
it.each(['/body', '/body/subObject'])( it.each(['/body', '/body/subObject'])(
@ -119,18 +117,16 @@ describe('OpenAPI error conversion', () => {
})(error); })(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: message:
// it provides the message // it provides the message
expect.stringContaining(error.message), expect.stringContaining(error.message),
path: instancePath.substring('/body/'.length), path: instancePath.substring('/body/'.length),
}); });
// it tells the user what happened // it tells the user what happened
expect(result.description).toContain( expect(result.message).toContain('matches more than one option');
'matches more than one option',
);
// it tells the user what part of the request body this pertains to // it tells the user what part of the request body this pertains to
expect(result.description).toContain( expect(result.message).toContain(
instancePath === '/body' instancePath === '/body'
? 'root object' ? 'root object'
: `"${instancePath.substring('/body/'.length)}" property`, : `"${instancePath.substring('/body/'.length)}" property`,
@ -156,11 +152,10 @@ describe('OpenAPI error conversion', () => {
})(error); })(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: expect.stringContaining(error.params.pattern), message: expect.stringContaining(error.params.pattern),
path: 'description', path: 'description',
}); });
expect(result.description).toContain('description'); expect(result.message).toContain('description');
expect(result.description).toContain(requestDescription);
}); });
it('Gives useful enum error messages', () => { it('Gives useful enum error messages', () => {
@ -184,13 +179,13 @@ describe('OpenAPI error conversion', () => {
const result = fromOpenApiValidationError(request)(error); const result = fromOpenApiValidationError(request)(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: expect.stringContaining('weightType'), message: expect.stringContaining('weightType'),
path: 'weightType', path: 'weightType',
}); });
expect(result.description).toContain('one of the allowed values'); expect(result.message).toContain('one of the allowed values');
expect(result.description).toContain('fix'); expect(result.message).toContain('fix');
expect(result.description).toContain('variable'); expect(result.message).toContain('variable');
expect(result.description).toContain('party'); expect(result.message).toContain('party');
}); });
it('Gives useful min/maxlength error messages', () => { it('Gives useful min/maxlength error messages', () => {
@ -211,15 +206,15 @@ describe('OpenAPI error conversion', () => {
})(error); })(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: message:
// it tells the user what the limit is // it tells the user what the limit is
expect.stringContaining(error.params.limit.toString()), expect.stringContaining(error.params.limit.toString()),
}); });
// it tells the user which property it pertains to // it tells the user which property it pertains to
expect(result.description).toContain('description'); expect(result.message).toContain('description');
// it tells the user what they provided // it tells the user what they provided
expect(result.description).toContain(requestDescription); expect(result.message).toContain(requestDescription);
}); });
it('Handles numerical min/max errors', () => { it('Handles numerical min/max errors', () => {
@ -242,17 +237,17 @@ describe('OpenAPI error conversion', () => {
})(error); })(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: message:
// it tells the user what the limit is // it tells the user what the limit is
expect.stringContaining(error.params.limit.toString()), expect.stringContaining(error.params.limit.toString()),
}); });
// it tells the user what kind of comparison it performed // it tells the user what kind of comparison it performed
expect(result.description).toContain(error.params.comparison); expect(result.message).toContain(error.params.comparison);
// it tells the user which property it pertains to // it tells the user which property it pertains to
expect(result.description).toContain('newprop'); expect(result.message).toContain('newprop');
// it tells the user what they provided // it tells the user what they provided
expect(result.description).toContain(propertyValue.toString()); expect(result.message).toContain(propertyValue.toString());
}); });
it('Handles multiple errors', () => { it('Handles multiple errors', () => {
@ -290,10 +285,10 @@ describe('OpenAPI error conversion', () => {
message: expect.stringContaining('`details`'), message: expect.stringContaining('`details`'),
details: [ details: [
{ {
description: expect.stringContaining('newprop'), message: expect.stringContaining('newprop'),
}, },
{ {
description: expect.stringContaining('enabled'), message: expect.stringContaining('enabled'),
}, },
], ],
}); });
@ -315,14 +310,14 @@ describe('OpenAPI error conversion', () => {
); );
expect(error).toMatchObject({ expect(error).toMatchObject({
description: expect.stringContaining( message: expect.stringContaining(
openApiError.params.additionalProperty, openApiError.params.additionalProperty,
), ),
path: 'bogus', path: 'bogus',
}); });
expect(error.description).toMatch(/\broot\b/i); expect(error.message).toMatch(/\broot\b/i);
expect(error.description).toMatch(/\badditional properties\b/i); expect(error.message).toMatch(/\badditional properties\b/i);
}); });
it('gives useful messages for nested properties', () => { it('gives useful messages for nested properties', () => {
@ -343,14 +338,14 @@ describe('OpenAPI error conversion', () => {
const error = fromOpenApiValidationError(request2)(openApiError); const error = fromOpenApiValidationError(request2)(openApiError);
expect(error).toMatchObject({ expect(error).toMatchObject({
description: expect.stringContaining('nestedObject/nested2'), message: expect.stringContaining('nestedObject/nested2'),
path: 'nestedObject/nested2/extraPropertyName', path: 'nestedObject/nested2/extraPropertyName',
}); });
expect(error.description).toContain( expect(error.message).toContain(
openApiError.params.additionalProperty, openApiError.params.additionalProperty,
); );
expect(error.description).toMatch(/\badditional properties\b/i); expect(error.message).toMatch(/\badditional properties\b/i);
}); });
}); });
@ -369,11 +364,11 @@ describe('OpenAPI error conversion', () => {
})(error); })(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: expect.stringMatching(/\bnestedObject\/a\/b\b/), message: expect.stringMatching(/\bnestedObject\/a\/b\b/),
path: 'nestedObject/a/b', path: 'nestedObject/a/b',
}); });
expect(result.description).toContain('[]'); expect(result.message).toContain('[]');
}); });
it('Handles deeply nested properties on referenced schemas', () => { it('Handles deeply nested properties on referenced schemas', () => {
@ -391,11 +386,11 @@ describe('OpenAPI error conversion', () => {
})(error); })(error);
expect(result).toMatchObject({ expect(result).toMatchObject({
description: expect.stringContaining(illegalValue), message: expect.stringContaining(illegalValue),
path: 'nestedObject/a/b', path: 'nestedObject/a/b',
}); });
expect(result.description).toMatch(/\bnestedObject\/a\/b\b/); expect(result.message).toMatch(/\bnestedObject\/a\/b\b/);
}); });
}); });
@ -430,7 +425,7 @@ describe('Error serialization special cases', () => {
message: expect.stringContaining('details'), message: expect.stringContaining('details'),
details: [ details: [
{ {
description: message:
'"value" must contain at least 1 items. You provided [].', '"value" must contain at least 1 items. You provided [].',
}, },
], ],
@ -494,14 +489,13 @@ describe('Error serialization special cases', () => {
}); });
it('BadDataError: adds `details` with error details', () => { it('BadDataError: adds `details` with error details', () => {
const description = 'You did **this** wrong'; const message = 'You did **this** wrong';
const error = new BadDataError(description).toJSON(); const error = new BadDataError(message).toJSON();
expect(error).toMatchObject({ expect(error).toMatchObject({
details: [ details: [
{ {
message: description, message,
description,
}, },
], ],
}); });

View File

@ -60,7 +60,7 @@ export abstract class UnleashError extends Error {
id: this.id, id: this.id,
name: this.name, name: this.name,
message: this.message, message: this.message,
details: [{ message: this.message, description: this.message }], details: [{ message: this.message }],
}; };
} }

View File

@ -966,7 +966,7 @@ test('reject import with unknown context fields', async () => {
400, 400,
); );
expect(body.details[0].description).toMatch(/\bContextField1\b/); expect(body.details[0].message).toMatch(/\bContextField1\b/);
}); });
test('reject import with unsupported strategies', async () => { test('reject import with unsupported strategies', async () => {
@ -989,7 +989,7 @@ test('reject import with unsupported strategies', async () => {
400, 400,
); );
expect(body.details[0].description).toMatch(/\bcustomStrategy\b/); expect(body.details[0].message).toMatch(/\bcustomStrategy\b/);
}); });
test('reject import with duplicate features', async () => { test('reject import with duplicate features', async () => {
@ -1007,7 +1007,7 @@ test('reject import with duplicate features', async () => {
409, 409,
); );
expect(body.details[0].description).toBe( expect(body.details[0].message).toBe(
'A flag with that name already exists', 'A flag with that name already exists',
); );
}); });

View File

@ -101,7 +101,7 @@ test('Should not remove environment from project if project only has one environ
.delete(`/api/admin/projects/default/environments/default`) .delete(`/api/admin/projects/default/environments/default`)
.expect(400) .expect(400)
.expect((r) => { .expect((r) => {
expect(r.body.details[0].description).toBe( expect(r.body.details[0].message).toBe(
'You must always have one active environment', 'You must always have one active environment',
); );
}); });

View File

@ -90,7 +90,7 @@ test('Invalid tag types gets rejected', async () => {
.set('Content-Type', 'application/json') .set('Content-Type', 'application/json')
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
'"name" must be URL friendly', '"name" must be URL friendly',
); );
}); });
@ -164,7 +164,7 @@ test('Invalid tag-types get refused by validator', async () => {
.set('Content-Type', 'application/json') .set('Content-Type', 'application/json')
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
'"name" must be URL friendly', '"name" must be URL friendly',
); );
}); });

View File

@ -44,7 +44,7 @@ test('require a name when creating a new strategy', async () => {
.expect((res) => { .expect((res) => {
expect( expect(
['name', 'property', 'required'].every((word) => ['name', 'property', 'required'].every((word) =>
res.body.details[0].description.includes(word), res.body.details[0].message.includes(word),
), ),
).toBeTruthy(); ).toBeTruthy();
}); });
@ -57,7 +57,7 @@ test('require parameters array when creating a new strategy', async () => {
.send({ name: 'TestStrat' }) .send({ name: 'TestStrat' })
.expect(400); .expect(400);
const detailsDescription = body.details[0].description; const detailsDescription = body.details[0].message;
expect(detailsDescription).toEqual(expect.stringMatching('parameters')); expect(detailsDescription).toEqual(expect.stringMatching('parameters'));
expect(detailsDescription).toEqual(expect.stringMatching('required')); expect(detailsDescription).toEqual(expect.stringMatching('required'));
}); });

View File

@ -289,7 +289,7 @@ test('should not create token for invalid projectId', async () => {
.set('Content-Type', 'application/json') .set('Content-Type', 'application/json')
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
/bogus-project-something/, /bogus-project-something/,
); );
}); });
@ -306,7 +306,7 @@ test('should not create token for invalid environment', async () => {
.set('Content-Type', 'application/json') .set('Content-Type', 'application/json')
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
/bogus-environment-something/, /bogus-environment-something/,
); );
}); });

View File

@ -519,7 +519,7 @@ test('PUTing an invalid variant throws 400 exception', async () => {
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details).toHaveLength(1); expect(res.body.details).toHaveLength(1);
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
/.*weightType` property must be equal to one of the allowed values/, /.*weightType` property must be equal to one of the allowed values/,
); );
}); });
@ -555,7 +555,7 @@ test('Invalid variant in PATCH also throws 400 exception', async () => {
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details).toHaveLength(1); expect(res.body.details).toHaveLength(1);
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
/.*weight" must be less than or equal to 1000/, /.*weight" must be less than or equal to 1000/,
); );
}); });
@ -685,7 +685,7 @@ test('PATCHING with no variable variants fails with 400', async () => {
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details).toHaveLength(1); expect(res.body.details).toHaveLength(1);
expect(res.body.details[0].description).toEqual( expect(res.body.details[0].message).toEqual(
'There must be at least one "variable" variant', 'There must be at least one "variable" variant',
); );
}); });
@ -883,7 +883,7 @@ test('If sum of fixed variant weight exceed 1000 fails with 400', async () => {
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details).toHaveLength(1); expect(res.body.details).toHaveLength(1);
expect(res.body.details[0].description).toEqual( expect(res.body.details[0].message).toEqual(
'The traffic distribution total must equal 100%', 'The traffic distribution total must equal 100%',
); );
}); });
@ -998,7 +998,7 @@ test('PATCH endpoint validates uniqueness of variant names', async () => {
.send(patch) .send(patch)
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
/contains a duplicate value/, /contains a duplicate value/,
); );
}); });
@ -1035,7 +1035,7 @@ test('PUT endpoint validates uniqueness of variant names', async () => {
]) ])
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
/contains a duplicate value/, /contains a duplicate value/,
); );
}); });

View File

@ -89,7 +89,7 @@ test('Can validate a tag', async () =>
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details.length).toBe(1); expect(res.body.details.length).toBe(1);
expect(res.body.details[0].description).toMatch( expect(res.body.details[0].message).toMatch(
'"type" must be URL friendly', '"type" must be URL friendly',
); );
})); }));

View File

@ -155,7 +155,7 @@ test('should require username or email on create', async () => {
.set('Content-Type', 'application/json') .set('Content-Type', 'application/json')
.expect(400) .expect(400)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toEqual( expect(res.body.details[0].message).toEqual(
'You must specify username or email', 'You must specify username or email',
); );
}); });

View File

@ -188,7 +188,7 @@ test('Trying to reset password with same token twice does not work', async () =>
}) })
.expect(401) .expect(401)
.expect((res) => { .expect((res) => {
expect(res.body.details[0].description).toBeTruthy(); expect(res.body.details[0].message).toBeTruthy();
}); });
}); });