[OAS] Beautify generated operation ids (#198132)

packages/kbn-router-to-openapispec/src/__snapshots__/generate_oas.test.ts.snap

@@ -44,7 +44,7 @@ Object {
   "paths": Object {
     "/foo/{id}": Object {
       "get": Object {
-        "operationId": "%2Ffoo%2F%7Bid%7D#0",
+        "operationId": "get-foo-id",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -138,7 +138,7 @@ Object {
     "/bar": Object {
       "get": Object {
         "deprecated": true,
-        "operationId": "%2Fbar#0",
+        "operationId": "get-bar",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -231,7 +231,7 @@ OK response oas-test-version-2",
     "/foo/{id}/{path*}": Object {
       "delete": Object {
         "description": "route description",
-        "operationId": "%2Ffoo%2F%7Bid%7D%2F%7Bpath*%7D#2",
+        "operationId": "delete-foo-id-path",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -269,7 +269,7 @@ OK response oas-test-version-2",
       },
       "get": Object {
         "description": "route description",
-        "operationId": "%2Ffoo%2F%7Bid%7D%2F%7Bpath*%7D#0",
+        "operationId": "get-foo-id-path",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -415,7 +415,7 @@ OK response oas-test-version-2",
       },
       "post": Object {
         "description": "route description",
-        "operationId": "%2Ffoo%2F%7Bid%7D%2F%7Bpath*%7D#1",
+        "operationId": "post-foo-id-path",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -573,7 +573,7 @@ OK response oas-test-version-2",
     "/no-xsrf/{id}/{path*}": Object {
       "post": Object {
         "deprecated": true,
-        "operationId": "%2Fno-xsrf%2F%7Bid%7D%2F%7Bpath*%7D#1",
+        "operationId": "post-no-xsrf-id-path-2",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -725,7 +725,7 @@ Object {
   "paths": Object {
     "/recursive": Object {
       "get": Object {
-        "operationId": "%2Frecursive#0",
+        "operationId": "get-recursive",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -808,7 +808,7 @@ Object {
   "paths": Object {
     "/foo/{id}": Object {
       "get": Object {
-        "operationId": "%2Ffoo%2F%7Bid%7D#0",
+        "operationId": "get-foo-id",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",
@@ -846,7 +846,7 @@ Object {
     },
     "/test": Object {
       "get": Object {
-        "operationId": "%2Ftest#0",
+        "operationId": "get-test",
         "parameters": Array [
           Object {
             "description": "The version of the API to use",

packages/kbn-router-to-openapispec/src/generate_oas.test.fixture.ts

@@ -35,7 +35,7 @@ export const sharedOas = {
       get: {
         deprecated: true,
         'x-discontinued': 'route discontinued version or date',
-        operationId: '%2Fbar#0',
+        operationId: 'get-bar',
         parameters: [
           {
             description: 'The version of the API to use',
@@ -154,7 +154,7 @@ export const sharedOas = {
     '/foo/{id}/{path*}': {
       get: {
         description: 'route description',
-        operationId: '%2Ffoo%2F%7Bid%7D%2F%7Bpath*%7D#0',
+        operationId: 'get-foo-id-path',
         parameters: [
           {
             description: 'The version of the API to use',
@@ -278,7 +278,7 @@ export const sharedOas = {
       },
       post: {
         description: 'route description',
-        operationId: '%2Ffoo%2F%7Bid%7D%2F%7Bpath*%7D#1',
+        operationId: 'post-foo-id-path',
         parameters: [
           {
             description: 'The version of the API to use',

packages/kbn-router-to-openapispec/src/generate_oas.ts

@@ -10,10 +10,9 @@
 import type { CoreVersionedRouter, Router } from '@kbn/core-http-router-server-internal';
 import type { OpenAPIV3 } from 'openapi-types';
 import { OasConverter } from './oas_converter';
-import { createOperationIdCounter } from './operation_id_counter';
 import { processRouter } from './process_router';
 import { processVersionedRouter } from './process_versioned_router';
-import { buildGlobalTags } from './util';
+import { buildGlobalTags, createOpIdGenerator } from './util';
 
 export const openApiVersion = '3.0.0';
 
@@ -40,8 +39,8 @@ export const generateOpenApiDocument = (
 ): OpenAPIV3.Document => {
   const { filters } = opts;
   const converter = new OasConverter();
-  const getOpId = createOperationIdCounter();
   const paths: OpenAPIV3.PathsObject = {};
+  const getOpId = createOpIdGenerator();
   for (const router of appRouters.routers) {
     const result = processRouter(router, converter, getOpId, filters);
     Object.assign(paths, result.paths);

packages/kbn-router-to-openapispec/src/operation_id_counter.test.ts

@@ -1,32 +0,0 @@
-/*
- * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
- * or more contributor license agreements. Licensed under the "Elastic License
- * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
- * Public License v 1"; you may not use this file except in compliance with, at
- * your election, the "Elastic License 2.0", the "GNU Affero General Public
- * License v3.0 only", or the "Server Side Public License, v 1".
- */
-
-import { createOperationIdCounter } from './operation_id_counter';
-
-test('empty case', () => {
-  const opIdCounter = createOperationIdCounter();
-  expect(opIdCounter('')).toBe('#0');
-});
-
-test('other cases', () => {
-  const opIdCounter = createOperationIdCounter();
-  const tests = [
-    ['/', '%2F#0'],
-    ['/api/cool', '%2Fapi%2Fcool#0'],
-    ['/api/cool', '%2Fapi%2Fcool#1'],
-    ['/api/cool', '%2Fapi%2Fcool#2'],
-    ['/api/cool/{variable}', '%2Fapi%2Fcool%2F%7Bvariable%7D#0'],
-    ['/api/cool/{optionalVariable?}', '%2Fapi%2Fcool%2F%7BoptionalVariable%3F%7D#0'],
-    ['/api/cool/{optionalVariable?}', '%2Fapi%2Fcool%2F%7BoptionalVariable%3F%7D#1'],
-  ];
-
-  tests.forEach(([input, expected]) => {
-    expect(opIdCounter(input)).toBe(expected);
-  });
-});

packages/kbn-router-to-openapispec/src/operation_id_counter.ts

@@ -1,24 +0,0 @@
-/*
- * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
- * or more contributor license agreements. Licensed under the "Elastic License
- * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
- * Public License v 1"; you may not use this file except in compliance with, at
- * your election, the "Elastic License 2.0", the "GNU Affero General Public
- * License v3.0 only", or the "Server Side Public License, v 1".
- */
-
-export type OperationIdCounter = (name: string) => string;
-
-export const createOperationIdCounter = () => {
-  const operationIdCounters = new Map<string, number>();
-  return (name: string): string => {
-    name = encodeURIComponent(name);
-    // Aliases an operationId to ensure it is unique across
-    // multiple method+path combinations sharing a name.
-    // "search" -> "search#0", "search#1", etc.
-    const operationIdCount = operationIdCounters.get(name) ?? 0;
-    const aliasedName = name + '#' + operationIdCount.toString();
-    operationIdCounters.set(name, operationIdCount + 1);
-    return aliasedName;
-  };
-};

packages/kbn-router-to-openapispec/src/process_router.test.ts

@@ -10,9 +10,9 @@
 import { schema } from '@kbn/config-schema';
 import { Router } from '@kbn/core-http-router-server-internal';
 import { OasConverter } from './oas_converter';
-import { createOperationIdCounter } from './operation_id_counter';
 import { extractResponses, processRouter } from './process_router';
 import { type InternalRouterRoute } from './type';
+import { createOpIdGenerator } from './util';
 
 describe('extractResponses', () => {
   let oasConverter: OasConverter;
@@ -86,18 +86,21 @@ describe('processRouter', () => {
   const testRouter = {
     getRoutes: () => [
       {
+        method: 'get',
         path: '/foo',
         options: { access: 'internal', deprecated: true, discontinued: 'discontinued router' },
         handler: jest.fn(),
         validationSchemas: { request: { body: schema.object({}) } },
       },
       {
+        method: 'get',
         path: '/bar',
         options: {},
         handler: jest.fn(),
         validationSchemas: { request: { body: schema.object({}) } },
       },
       {
+        method: 'get',
         path: '/baz',
         options: {},
         handler: jest.fn(),
@@ -125,20 +128,20 @@ describe('processRouter', () => {
   } as unknown as Router;
 
   it('only provides routes for version 2023-10-31', () => {
-    const result1 = processRouter(testRouter, new OasConverter(), createOperationIdCounter(), {
+    const result1 = processRouter(testRouter, new OasConverter(), createOpIdGenerator(), {
       version: '2023-10-31',
     });
 
     expect(Object.keys(result1.paths!)).toHaveLength(4);
 
-    const result2 = processRouter(testRouter, new OasConverter(), createOperationIdCounter(), {
+    const result2 = processRouter(testRouter, new OasConverter(), createOpIdGenerator(), {
       version: '2024-10-31',
     });
     expect(Object.keys(result2.paths!)).toHaveLength(0);
   });
 
   it('updates description with privileges required', () => {
-    const result = processRouter(testRouter, new OasConverter(), createOperationIdCounter(), {
+    const result = processRouter(testRouter, new OasConverter(), createOpIdGenerator(), {
       version: '2023-10-31',
     });
 

packages/kbn-router-to-openapispec/src/process_router.ts

@@ -24,8 +24,8 @@ import {
   mergeResponseContent,
   prepareRoutes,
   setXState,
+  GetOpId,
 } from './util';
-import type { OperationIdCounter } from './operation_id_counter';
 import type { GenerateOpenApiDocumentOptionsFilters } from './generate_oas';
 import type { CustomOperationObject, InternalRouterRoute } from './type';
 import { extractAuthzDescription } from './extract_authz_description';
@@ -33,7 +33,7 @@ import { extractAuthzDescription } from './extract_authz_description';
 export const processRouter = (
   appRouter: Router,
   converter: OasConverter,
-  getOpId: OperationIdCounter,
+  getOpId: GetOpId,
   filters?: GenerateOpenApiDocumentOptionsFilters
 ) => {
   const paths: OpenAPIV3.PathsObject = {};
@@ -91,7 +91,7 @@ export const processRouter = (
           : undefined,
         responses: extractResponses(route, converter),
         parameters,
-        operationId: getOpId(route.path),
+        operationId: getOpId({ path: route.path, method: route.method }),
       };
 
       setXState(route.options.availability, operation);

packages/kbn-router-to-openapispec/src/process_versioned_router.test.ts

@@ -11,13 +11,13 @@ import { schema } from '@kbn/config-schema';
 import type { CoreVersionedRouter } from '@kbn/core-http-router-server-internal';
 import { get } from 'lodash';
 import { OasConverter } from './oas_converter';
-import { createOperationIdCounter } from './operation_id_counter';
 import {
   processVersionedRouter,
   extractVersionedResponses,
   extractVersionedRequestBodies,
 } from './process_versioned_router';
 import { VersionedRouterRoute } from '@kbn/core-http-server';
+import { createOpIdGenerator } from './util';
 
 let oasConverter: OasConverter;
 beforeEach(() => {
@@ -125,7 +125,7 @@ describe('processVersionedRouter', () => {
     const baseCase = processVersionedRouter(
       { getRoutes: () => [createTestRoute()] } as unknown as CoreVersionedRouter,
       new OasConverter(),
-      createOperationIdCounter(),
+      createOpIdGenerator(),
       {}
     );
 
@@ -137,7 +137,7 @@ describe('processVersionedRouter', () => {
     const filteredCase = processVersionedRouter(
       { getRoutes: () => [createTestRoute()] } as unknown as CoreVersionedRouter,
       new OasConverter(),
-      createOperationIdCounter(),
+      createOpIdGenerator(),
       { version: '2023-10-31' }
     );
     expect(Object.keys(get(filteredCase, 'paths["/foo"].get.responses.200.content')!)).toEqual([
@@ -149,7 +149,7 @@ describe('processVersionedRouter', () => {
     const results = processVersionedRouter(
       { getRoutes: () => [createTestRoute()] } as unknown as CoreVersionedRouter,
       new OasConverter(),
-      createOperationIdCounter(),
+      createOpIdGenerator(),
       {}
     );
     expect(results.paths['/foo']).toBeDefined();

packages/kbn-router-to-openapispec/src/process_versioned_router.ts

@@ -18,7 +18,6 @@ import { extractAuthzDescription } from './extract_authz_description';
 import type { GenerateOpenApiDocumentOptionsFilters } from './generate_oas';
 import type { OasConverter } from './oas_converter';
 import { isReferenceObject } from './oas_converter/common';
-import type { OperationIdCounter } from './operation_id_counter';
 import {
   prepareRoutes,
   getPathParameters,
@@ -30,12 +29,13 @@ import {
   mergeResponseContent,
   getXsrfHeaderForMethod,
   setXState,
+  GetOpId,
 } from './util';
 
 export const processVersionedRouter = (
   appRouter: CoreVersionedRouter,
   converter: OasConverter,
-  getOpId: OperationIdCounter,
+  getOpId: GetOpId,
   filters?: GenerateOpenApiDocumentOptionsFilters
 ) => {
   const routes = prepareRoutes(appRouter.getRoutes(), filters);
@@ -121,7 +121,7 @@ export const processVersionedRouter = (
           ? extractVersionedResponse(handler, converter, contentType)
           : extractVersionedResponses(route, converter, contentType),
         parameters,
-        operationId: getOpId(route.path),
+        operationId: getOpId({ path: route.path, method: route.method }),
       };
 
       setXState(route.options.options?.availability, operation);

packages/kbn-router-to-openapispec/src/util.test.ts

@@ -15,6 +15,8 @@ import {
   mergeResponseContent,
   prepareRoutes,
   getPathParameters,
+  createOpIdGenerator,
+  GetOpId,
 } from './util';
 import { assignToPaths, extractTags } from './util';
 
@@ -260,3 +262,83 @@ describe('getPathParameters', () => {
     expect(getPathParameters(input)).toEqual(output);
   });
 });
+
+describe('createOpIdGenerator', () => {
+  let getOpId: GetOpId;
+  beforeEach(() => {
+    getOpId = createOpIdGenerator();
+  });
+  test('empty', () => {
+    expect(() => getOpId({ method: '', path: '/asd' })).toThrow(/Must provide method and path/);
+    expect(() => getOpId({ method: 'get', path: '' })).toThrow(/Must provide method and path/);
+    expect(() => getOpId({ method: '', path: '' })).toThrow(/Must provide method and path/);
+  });
+  test('disambiguate', () => {
+    expect(getOpId({ method: 'get', path: '/test' })).toBe('get-test');
+    expect(getOpId({ method: 'get', path: '/test' })).toBe('get-test-2');
+    expect(getOpId({ method: 'get', path: '/test' })).toBe('get-test-3');
+    expect(getOpId({ method: 'get', path: '/test' })).toBe('get-test-4');
+  });
+  test.each([
+    { input: { method: 'GET', path: '/api/file' }, output: 'get-file' },
+    { input: { method: 'GET', path: '///api/file///' }, output: 'get-file' },
+    { input: { method: 'POST', path: '/internal/api/file' }, output: 'post-file' },
+    { input: { method: 'PUT', path: '/internal/file' }, output: 'put-file' },
+    { input: { method: 'Put', path: 'fOO/fILe' }, output: 'put-foo-file' },
+    {
+      input: { method: 'delete', path: '/api/my/really/cool/domain/resource' },
+      output: 'delete-my-really-cool-domain-resource',
+    },
+    {
+      input: {
+        method: 'delete',
+        path: '/api/my/really/cool/domain/resource',
+      },
+      output: 'delete-my-really-cool-domain-resource',
+    },
+    {
+      input: {
+        method: 'get',
+        path: '/api/my/resource/{id}',
+      },
+      output: 'get-my-resource-id',
+    },
+    {
+      input: {
+        method: 'get',
+        path: '/api/my/resource/{id}/{type?}',
+      },
+      output: 'get-my-resource-id-type',
+    },
+    {
+      input: {
+        method: 'get',
+        path: '/api/my/resource/{id?}',
+      },
+      output: 'get-my-resource-id',
+    },
+    {
+      input: {
+        method: 'get',
+        path: '/api/my/resource/{path*}',
+      },
+      output: 'get-my-resource-path',
+    },
+    {
+      input: {
+        method: 'get',
+        path: '/api/my/underscore_resource',
+      },
+      output: 'get-my-underscore-resource',
+    },
+    {
+      input: {
+        method: 'get',
+        path: '/api/my/_underscore_resource',
+      },
+      output: 'get-my-underscore-resource',
+    },
+  ])('$input.method $input.path -> $output', ({ input, output }) => {
+    expect(getOpId(input)).toBe(output);
+  });
+});

packages/kbn-router-to-openapispec/src/util.ts

@@ -166,10 +166,10 @@ export const getXsrfHeaderForMethod = (
   ];
 };
 
-export function setXState(
+export const setXState = (
   availability: RouteConfigOptions<RouteMethod>['availability'],
   operation: CustomOperationObject
-): void {
+): void => {
   if (availability) {
     if (availability.stability === 'experimental') {
       operation['x-state'] = 'Technical Preview';
@@ -178,4 +178,45 @@ export function setXState(
       operation['x-state'] = 'Beta';
     }
   }
-}
+};
+
+export type GetOpId = (input: { path: string; method: string }) => string;
+
+/**
+ * Best effort to generate operation IDs from route values
+ */
+export const createOpIdGenerator = (): GetOpId => {
+  const idMap = new Map<string, number>();
+  return function getOpId({ path, method }) {
+    if (!method || !path) {
+      throw new Error(
+        `Must provide method and path, received: method: "${method}", path: "${path}"`
+      );
+    }
+
+    path = path
+      .trim()
+      .replace(/^[\/]+/, '')
+      .replace(/[\/]+$/, '')
+      .toLowerCase();
+
+    const removePrefixes = ['internal/api/', 'internal/', 'api/']; // longest to shortest
+    for (const prefix of removePrefixes) {
+      if (path.startsWith(prefix)) {
+        path = path.substring(prefix.length);
+        break;
+      }
+    }
+
+    path = path
+      .replace(/[\{\}\?\*]/g, '') // remove special chars
+      .replace(/[\/_]/g, '-') // everything else to dashes
+      .replace(/[-]+/g, '-'); // single dashes
+
+    const opId = `${method.toLowerCase()}-${path}`;
+
+    const cachedCount = idMap.get(opId) ?? 0;
+    idMap.set(opId, cachedCount + 1);
+    return cachedCount > 0 ? `${opId}-${cachedCount + 1}` : opId;
+  };
+};