refactor: switch to upstream express-openapi

package.json

@@ -2,12 +2,7 @@
   "name": "unleash-server",
   "description": "Unleash is an enterprise ready feature toggles service. It provides different strategies for handling feature toggles.",
   "version": "5.6.0",
-  "keywords": [
-    "unleash",
-    "feature toggle",
-    "feature",
-    "toggle"
-  ],
+  "keywords": ["unleash", "feature toggle", "feature", "toggle"],
   "files": [
     "dist",
     "docs",
@@ -80,23 +75,11 @@
     "testTimeout": 10000,
     "globalSetup": "./scripts/jest-setup.js",
     "transform": {
-      "^.+\\.tsx?$": [
-        "@swc/jest"
-      ]
+      "^.+\\.tsx?$": ["@swc/jest"]
     },
     "testRegex": "(/__tests__/.*|(\\.|/)(test|spec))\\.(jsx?|tsx?)$",
-    "testPathIgnorePatterns": [
-      "/dist/",
-      "/node_modules/",
-      "/frontend/"
-    ],
-    "moduleFileExtensions": [
-      "ts",
-      "tsx",
-      "js",
-      "jsx",
-      "json"
-    ],
+    "testPathIgnorePatterns": ["/dist/", "/node_modules/", "/frontend/"],
+    "moduleFileExtensions": ["ts", "tsx", "js", "jsx", "json"],
     "coveragePathIgnorePatterns": [
       "/node_modules/",
       "/dist/",
@@ -106,7 +89,7 @@
   },
   "dependencies": {
     "@slack/web-api": "^6.9.0",
-    "@unleash/express-openapi": "^0.3.0",
+    "@wesleytodd/openapi": "^0.3.0",
     "ajv": "^8.12.0",
     "ajv-formats": "^2.1.1",
     "async": "^3.2.4",
@@ -232,11 +215,7 @@
     "tough-cookie": "4.1.3"
   },
   "lint-staged": {
-    "*.{js,ts}": [
-      "biome check --apply"
-    ],
-    "*.json": [
-      "biome format --write --no-errors-on-unmatched"
-    ]
+    "*.{js,ts}": ["biome check --apply"],
+    "*.json": ["biome format --write --no-errors-on-unmatched"]
   }
 }

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

@@ -45,7 +45,7 @@ const missingRequiredPropertyMessage = (
     missingPropertyName: string,
 ) => {
     const path = constructPath(pathToParentObject, missingPropertyName);
-    const description = `The ${path} property is required. It was not present on the data you sent.`;
+    const description = `The \`${path}\` property is required. It was not present on the data you sent.`;
     return {
         path,
         description,
@@ -77,7 +77,7 @@ const genericErrorMessage = (
     const input = getProp(requestBody, propertyName);
 
     const youSent = JSON.stringify(input);
-    const description = `The .${propertyName} property ${errorMessage}. You sent ${youSent}.`;
+    const description = `The \`.${propertyName}\` property ${errorMessage}. You sent ${youSent}.`;
     return {
         description,
         message: description,
@@ -90,7 +90,7 @@ const oneOfMessage = (
     errorMessage: string = 'is invalid',
 ) => {
     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.`;
 
@@ -101,32 +101,62 @@ const oneOfMessage = (
     };
 };
 
+const enumMessage = (
+    propertyName: string,
+    message: string | undefined,
+    allowedValues: string[],
+    suppliedValue: string | null | undefined,
+) => {
+    const description = `The \`${propertyName}\` property ${
+        message ?? 'must match one of the allowed values'
+    }: ${allowedValues
+        .map((value) => `"${value}"`)
+        .join(
+            ', ',
+        )}. You provided "${suppliedValue}", which is not valid. Please use one of the allowed values instead..`;
+
+    return {
+        description,
+        message: description,
+        path: propertyName,
+    };
+};
+
+// Sometimes, the error object contains a dataPath, even if it's not
+type ActualErrorObject = ErrorObject & { dataPath?: string };
+
 export const fromOpenApiValidationError =
     (requestBody: object) =>
-    (validationError: ErrorObject): ValidationErrorDescription => {
-        // @ts-expect-error Unsure why, but the `dataPath` isn't listed on the type definition for error objects. However, it's always there. Suspect this is a bug in the library.
-        const dataPath = validationError.dataPath;
-        const propertyName = dataPath.substring('.body.'.length);
+    (validationError: ActualErrorObject): ValidationErrorDescription => {
+        const { instancePath, params, message, dataPath } = validationError;
+        const propertyName = dataPath?.substring('.body.'.length) ?? '';
 
         switch (validationError.keyword) {
             case 'required':
                 return missingRequiredPropertyMessage(
                     propertyName,
-                    validationError.params.missingProperty,
+                    params.missingProperty,
                 );
             case 'additionalProperties':
                 return additionalPropertiesMessage(
                     propertyName,
-                    validationError.params.additionalProperty,
+                    params.additionalProperty,
+                );
+            case 'enum':
+                return enumMessage(
+                    instancePath.substring(instancePath.lastIndexOf('/') + 1),
+                    message,
+                    params.allowedValues,
+                    getProp(
+                        requestBody,
+                        instancePath.substring('/body/'.length).split('/'),
+                    ),
                 );
+
             case 'oneOf':
                 return oneOfMessage(propertyName, validationError.message);
             default:
-                return genericErrorMessage(
-                    requestBody,
-                    propertyName,
-                    validationError.message,
-                );
+                return genericErrorMessage(requestBody, propertyName, message);
         }
     };
 

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

@@ -136,7 +136,7 @@ describe('OpenAPI error conversion', () => {
             expect(result.description).toContain(
                 dataPath === '.body'
                     ? 'root object'
-                    : `${dataPath.substring('.body.'.length)} property`,
+                    : `"${dataPath.substring('.body.'.length)}" property`,
             );
         },
     );
@@ -167,6 +167,36 @@ describe('OpenAPI error conversion', () => {
         expect(result.description).toContain(requestDescription);
     });
 
+    it('Gives useful enum error messages', () => {
+        const error = {
+            instancePath: '/body/0/weightType',
+            schemaPath: '#/properties/weightType/enum',
+            keyword: 'enum',
+            params: { allowedValues: ['variable', 'fix'] },
+            message: 'must be equal to one of the allowed values',
+        };
+
+        const request = [
+            {
+                name: 'variant',
+                weight: 500,
+                weightType: 'party',
+                stickiness: 'userId',
+            },
+        ];
+
+        const result = fromOpenApiValidationError(request)(error);
+
+        expect(result).toMatchObject({
+            description: expect.stringContaining('weightType'),
+            path: 'weightType',
+        });
+        expect(result.description).toContain('one of the allowed values');
+        expect(result.description).toContain('fix');
+        expect(result.description).toContain('variable');
+        expect(result.description).toContain('party');
+    });
+
     it('Gives useful min/maxlength error messages', () => {
         const error = {
             instancePath: '',

src/lib/openapi/spec/patch-schema.ts

@@ -28,7 +28,6 @@ export const patchSchema = {
             description:
                 'The value to add or replace, if performing one of those operations',
             example: 'kill-switch',
-            nullable: true,
         },
     },
     components: {},

src/lib/services/openapi-service.ts

@@ -1,4 +1,4 @@
-import openapi, { IExpressOpenApi } from '@unleash/express-openapi';
+import openapi, { IExpressOpenApi } from '@wesleytodd/openapi';
 import { Express, RequestHandler, Response } from 'express';
 import { IUnleashConfig } from '../types/option';
 import {
@@ -30,7 +30,11 @@ export class OpenApiService {
         this.api = openapi(
             this.docsPath(),
             createOpenApiSchema(config.server),
-            { coerce: true, extendRefs: true },
+            {
+                coerce: true,
+                extendRefs: true,
+                basePath: config.server.baseUriPath,
+            },
         );
     }
 

src/test/e2e/api/admin/project/variants.e2e.test.ts

@@ -509,7 +509,7 @@ test('PUTing an invalid variant throws 400 exception', async () => {
         .expect((res) => {
             expect(res.body.details).toHaveLength(1);
             expect(res.body.details[0].description).toMatch(
-                /.*weightType property should be equal to one of the allowed values/,
+                /.*weightType` property must be equal to one of the allowed values/,
             );
         });
 });

src/test/e2e/api/openapi/openapi.e2e.test.ts

@@ -1,4 +1,4 @@
-import { setupApp } from '../../helpers/test-helper';
+import { setupApp, setupAppWithBaseUrl } from '../../helpers/test-helper';
 import dbInit from '../../helpers/database-init';
 import getLogger from '../../../fixtures/no-logger';
 import SwaggerParser from '@apidevtools/swagger-parser';
@@ -54,6 +54,47 @@ test('should serve the OpenAPI spec with a `version` property', async () => {
         });
 });
 
+describe('subpath handling', () => {
+    let appWithSubPath;
+    const subPath = '/absolute-nonsense';
+
+    beforeAll(async () => {
+        appWithSubPath = await setupAppWithBaseUrl(db.stores, subPath);
+    });
+
+    afterAll(async () => {
+        await appWithSubPath?.destroy();
+    });
+
+    test('the OpenAPI spec has the base path appended to its server', async () => {
+        const {
+            body: { servers },
+        } = await appWithSubPath.request
+            .get(`${subPath}/docs/openapi.json`)
+            .expect('Content-Type', /json/)
+            .expect(200);
+
+        expect(servers[0].url).toMatch(new RegExp(`.+${subPath}$`));
+    });
+
+    test('When the server has a base path, that base path is stripped from the endpoints', async () => {
+        const {
+            body: { paths },
+        } = await appWithSubPath.request
+            .get(`${subPath}/docs/openapi.json`)
+            .expect('Content-Type', /json/)
+            .expect(200);
+
+        // ensure that paths on this server don't start with the base
+        // uri path.
+        const noPathsStartWithSubpath = Object.keys(paths).every(
+            (p) => !p.startsWith(subPath),
+        );
+
+        expect(noPathsStartWithSubpath).toBe(true);
+    });
+});
+
 test('the generated OpenAPI spec is valid', async () => {
     const { body } = await app.request
         .get('/docs/openapi.json')

src/test/e2e/helpers/test-helper.ts

@@ -331,11 +331,12 @@ export async function setupAppWithCustomAuth(
 
 export async function setupAppWithBaseUrl(
     stores: IUnleashStores,
+    baseUriPath = '/hosted',
 ): Promise<IUnleashTest> {
     return createApp(stores, undefined, undefined, {
         server: {
             unleashUrl: 'http://localhost:4242',
-            baseUriPath: '/hosted',
+            baseUriPath,
         },
     });
 }