chore: improve joi errors (#3836)

This PR improves our handling of internal Joi errors, to make them more
sensible to the end user. It does that by providing a better description
of the errors and by telling the user what they value they provided was.

Previous conversion:
```json
{
  "id": "705a8dc0-1198-4894-9015-f1e5b9992b48",
  "name": "BadDataError",
  "message": "\"value\" must contain at least 1 items",
  "details": [
    {
      "message": "\"value\" must contain at least 1 items",
      "description": "\"value\" must contain at least 1 items"
    }
  ]
}
```

New conversion:
```json
{
  "id": "87fb4715-cbdd-48bb-b4d7-d354e7d97380",
  "name": "BadDataError",
  "message": "Request validation failed: your request body contains invalid data. Refer to the `details` list for more information.",
  "details": [
    {
      "description": "\"value\" must contain at least 1 items. You provided [].",
      "message": "\"value\" must contain at least 1 items. You provided []."
    }
  ]
}
```

## Restructuring

This PR moves some code out of `unleash-error.ts` and into a new file.
The purpose of this is twofold:
1. avoid circular dependencies (we need imports from both UnleashError
and BadDataError)
2. carve out a clearer separation of concerns, keeping `unleash-error` a
little more focused.

src/lib/error/bad-data-error.ts

@@ -1,4 +1,5 @@
 import { ErrorObject } from 'ajv';
+import { ValidationError } from 'joi';
 import getProp from 'lodash.get';
 import { ApiErrorSchema, UnleashError } from './unleash-error';
 
@@ -17,7 +18,7 @@ class BadDataError extends UnleashError {
         const topLevelMessage =
             'Request validation failed: your request body contains invalid data' +
             (errors
-                ? '. Refer to the `details` list to see what happened.'
+                ? '. Refer to the `details` list for more information.'
                 : `: ${message}`);
         super(topLevelMessage);
 
@@ -121,3 +122,29 @@ export const fromOpenApiValidationErrors = (
         [firstDetail, ...remainingDetails],
     );
 };
+
+export const fromJoiError = (err: ValidationError): BadDataError => {
+    const details = err.details.map((detail) => {
+        const messageEnd = detail.context?.value
+            ? `. You provided ${JSON.stringify(detail.context.value)}.`
+            : '.';
+        const description = detail.message + messageEnd;
+        return {
+            description,
+            message: description,
+        };
+    });
+
+    const [first, ...rest] = details;
+
+    if (first) {
+        return new BadDataError(
+            'A validation error occurred while processing your request data. Refer to the `details` property for more information.',
+            [first, ...rest],
+        );
+    } else {
+        return new BadDataError(
+            'A validation error occurred while processing your request data. Please make sure it conforms to the request data schema.',
+        );
+    }
+};

src/lib/error/from-legacy-error.ts

@@ -0,0 +1,54 @@
+import { fromJoiError } from './bad-data-error';
+import { ValidationError as JoiValidationError } from 'joi';
+import {
+    GenericUnleashError,
+    UnleashApiErrorName,
+    UnleashApiErrorTypes,
+    UnleashError,
+} from './unleash-error';
+
+export const fromLegacyError = (e: Error): UnleashError => {
+    if (e instanceof UnleashError) {
+        return e;
+    }
+    const name = UnleashApiErrorTypes.includes(e.name as UnleashApiErrorName)
+        ? (e.name as UnleashApiErrorName)
+        : 'UnknownError';
+
+    if (name === 'NoAccessError') {
+        return new GenericUnleashError({
+            name: 'NoAccessError',
+            message: e.message,
+        });
+    }
+
+    if (e instanceof JoiValidationError) {
+        return fromJoiError(e);
+    }
+
+    if (name === 'ValidationError' || name === 'BadDataError') {
+        return new GenericUnleashError({
+            name: 'BadDataError',
+            message: e.message,
+        });
+    }
+
+    if (name === 'OwaspValidationError') {
+        return new GenericUnleashError({
+            name: 'OwaspValidationError',
+            message: e.message,
+        });
+    }
+
+    if (name === 'AuthenticationRequired') {
+        return new GenericUnleashError({
+            name: 'AuthenticationRequired',
+            message: `You must be authenticated to view this content. Please log in.`,
+        });
+    }
+
+    return new GenericUnleashError({
+        name: name as UnleashApiErrorName,
+        message: e.message,
+    });
+};

src/lib/error/unleash-error.test.ts

@@ -1,7 +1,7 @@
 import owasp from 'owasp-password-strength-test';
 import { ErrorObject } from 'ajv';
 import AuthenticationRequired from '../types/authentication-required';
-import { ApiErrorSchema, fromLegacyError } from './unleash-error';
+import { ApiErrorSchema } from './unleash-error';
 import BadDataError, {
     fromOpenApiValidationError,
     fromOpenApiValidationErrors,
@@ -12,6 +12,8 @@ import IncompatibleProjectError from './incompatible-project-error';
 import PasswordUndefinedError from './password-undefined';
 import ProjectWithoutOwnerError from './project-without-owner-error';
 import NotFoundError from './notfound-error';
+import { validateString } from '../util/validators/constraint-types';
+import { fromLegacyError } from './from-legacy-error';
 
 describe('v5 deprecation: backwards compatibility', () => {
     it(`Adds details to error types that don't specify it`, () => {
@@ -352,6 +354,32 @@ describe('Error serialization special cases', () => {
             ],
         });
     });
+
+    it('Converts Joi errors in a sensible fashion', async () => {
+        // if the validation doesn't fail, this test does nothing, so ensure
+        // that an error is thrown.
+        let validationThrewAnError = false;
+        try {
+            await validateString([]);
+        } catch (e) {
+            validationThrewAnError = true;
+            const convertedError = fromLegacyError(e);
+
+            expect(convertedError.toJSON()).toMatchObject({
+                message:
+                    expect.stringContaining('validation error') &&
+                    expect.stringContaining('details'),
+                details: [
+                    {
+                        description:
+                            '"value" must contain at least 1 items. You provided [].',
+                    },
+                ],
+            });
+        }
+
+        expect(validationThrewAnError).toBeTruthy();
+    });
 });
 
 describe('Error serialization special cases', () => {

src/lib/error/unleash-error.ts

@@ -1,7 +1,7 @@
 import { v4 as uuidV4 } from 'uuid';
 import { FromSchema } from 'json-schema-to-ts';
 
-const UnleashApiErrorTypes = [
+export const UnleashApiErrorTypes = [
     'ContentTypeError',
     'DisabledError',
     'FeatureHasTagError',
@@ -32,7 +32,7 @@ const UnleashApiErrorTypes = [
     'InternalError',
 ] as const;
 
-type UnleashApiErrorName = typeof UnleashApiErrorTypes[number];
+export type UnleashApiErrorName = typeof UnleashApiErrorTypes[number];
 
 const statusCode = (errorName: string): number => {
     switch (errorName) {
@@ -127,7 +127,7 @@ export abstract class UnleashError extends Error {
     }
 }
 
-class GenericUnleashError extends UnleashError {
+export class GenericUnleashError extends UnleashError {
     constructor({
         name,
         message,
@@ -168,46 +168,4 @@ export const apiErrorSchema = {
     components: {},
 } as const;
 
-export const fromLegacyError = (e: Error): UnleashError => {
-    if (e instanceof UnleashError) {
-        return e;
-    }
-    const name = UnleashApiErrorTypes.includes(e.name as UnleashApiErrorName)
-        ? (e.name as UnleashApiErrorName)
-        : 'UnknownError';
-
-    if (name === 'NoAccessError') {
-        return new GenericUnleashError({
-            name: 'NoAccessError',
-            message: e.message,
-        });
-    }
-
-    if (name === 'ValidationError' || name === 'BadDataError') {
-        return new GenericUnleashError({
-            name: 'BadDataError',
-            message: e.message,
-        });
-    }
-
-    if (name === 'OwaspValidationError') {
-        return new GenericUnleashError({
-            name: 'OwaspValidationError',
-            message: e.message,
-        });
-    }
-
-    if (name === 'AuthenticationRequired') {
-        return new GenericUnleashError({
-            name: 'AuthenticationRequired',
-            message: `You must be authenticated to view this content. Please log in.`,
-        });
-    }
-
-    return new GenericUnleashError({
-        name: name as UnleashApiErrorName,
-        message: e.message,
-    });
-};
-
 export type ApiErrorSchema = FromSchema<typeof apiErrorSchema>;

src/lib/routes/admin-api/strategy.test.ts

@@ -68,7 +68,7 @@ test('require parameters array when creating a new strategy', async () => {
         .send({ name: 'TestStrat' })
         .expect(400)
         .expect((res) => {
-            expect(res.body.details[0].description).toEqual(
+            expect(res.body.details[0].description).toMatch(
                 '"parameters" is required',
             );
         });

src/lib/routes/util.ts

@@ -1,7 +1,8 @@
 import joi from 'joi';
 import { Response } from 'express';
 import { Logger } from '../logger';
-import { fromLegacyError, UnleashError } from '../error/unleash-error';
+import { UnleashError } from '../error/unleash-error';
+import { fromLegacyError } from '../error/from-legacy-error';
 
 export const customJoi = joi.extend((j) => ({
     type: 'isUrlFriendly',

src/test/e2e/api/admin/tag-types.e2e.test.ts

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

src/test/e2e/api/admin/tags.e2e.test.ts

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