Repository: nestjs/swagger
Pull request #2549 (2023-08-02 18:10:10)

feat(swagger): Provide URI version to operationIdFactory 
This is based on the work completed in #1949. Those changes were reverted due to test failures.
This change adapts those changes to also support neutral versions which resolves the test failures.

Adds a new optional parameter to operationIdFactory to allow providing the API version prefix added to the operation's path.

This allows the operationIdFactory to generate unique IDs in the case of the same controller method being used for multiple API versions.

Default behavior is unchanged - the operation IDs will be of the form controllerKey_methodKey, but where necessary this can be customised using the URI version prefix.

Closes nestjs#2537

## PR Checklist
Please check if your PR fulfills the following requirements:

- [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)


## PR Type
What kind of change does this PR introduce?

<!-- Please check the one that applies to this PR using "x". -->
- [ ] Bugfix
- [x] Feature
- [ ] Code style update (formatting, local variables)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] CI related changes
- [ ] Other... Please describe:

## What is the current behavior?
When using Nest's API versioning feature, and having a controller method cover multiple versions, `nestjs-swagger `generates invalid OpenAPI spec, due to duplicate operation IDs.

It generates duplicate operation IDs because only the controller and method names are available to `operationIdFactory - but in these cases the same controller and method is used multiple times.

Issue Number: 2537


## What is the new behavior?

When URI versioning is used, an additional parameter containing the version is provided to `operationIdFactory`. This allows developers to provide unique operation IDs in the case of a method being used in multiple versions.

## Does this PR introduce a breaking change?
- [ ] Yes
- [x] No

## Other information



Patch:
diff --git a/lib/interfaces/swagger-document-options.interface.ts b/lib/interfaces/swagger-document-options.interface.ts
index 041c6222..489e9178 100644
--- a/lib/interfaces/swagger-document-options.interface.ts
+++ b/lib/interfaces/swagger-document-options.interface.ts
@@ -1,3 +1,9 @@
+export type OperationIdFactory = (
+  controllerKey: string,
+  methodKey: string,
+  version?: string
+) => string;
+
 export interface SwaggerDocumentOptions {
   /**
    * List of modules to include in the specification
@@ -24,5 +30,5 @@ export interface SwaggerDocumentOptions {
    * based on the `controllerKey` and `methodKey`
    * @default () => controllerKey_methodKey
    */
-  operationIdFactory?: (controllerKey: string, methodKey: string) => string;
+  operationIdFactory?: OperationIdFactory;
 }
diff --git a/lib/swagger-explorer.ts b/lib/swagger-explorer.ts
index 2e052031..d6e9da38 100644
--- a/lib/swagger-explorer.ts
+++ b/lib/swagger-explorer.ts
@@ -7,6 +7,7 @@ import {
 import {
   Controller,
   Type,
+  VERSION_NEUTRAL,
   VersioningOptions,
   VersionValue
 } from '@nestjs/common/interfaces';
@@ -53,6 +54,7 @@ import {
   exploreApiTagsMetadata,
   exploreGlobalApiTagsMetadata
 } from './explorers/api-use-tags.explorer';
+import { OperationIdFactory } from './interfaces';
 import { DenormalizedDocResolvers } from './interfaces/denormalized-doc-resolvers.interface';
 import { DenormalizedDoc } from './interfaces/denormalized-doc.interface';
 import {
@@ -68,8 +70,10 @@ export class SwaggerExplorer {
   private readonly mimetypeContentWrapper = new MimetypeContentWrapper();
   private readonly metadataScanner = new MetadataScanner();
   private readonly schemas: Record<string, SchemaObject> = {};
-  private operationIdFactory = (controllerKey: string, methodKey: string) =>
-    controllerKey ? `${controllerKey}_${methodKey}` : methodKey;
+  private operationIdFactory: OperationIdFactory = (
+    controllerKey: string,
+    methodKey: string
+  ) => (controllerKey ? `${controllerKey}_${methodKey}` : methodKey);
   private routePathFactory?: RoutePathFactory;
 
   constructor(private readonly schemaObjectFactory: SchemaObjectFactory) {}
@@ -79,7 +83,7 @@ export class SwaggerExplorer {
     applicationConfig: ApplicationConfig,
     modulePath?: string | undefined,
     globalPrefix?: string | undefined,
-    operationIdFactory?: (controllerKey: string, methodKey: string) => string
+    operationIdFactory?: OperationIdFactory
   ) {
     this.routePathFactory = new RoutePathFactory(applicationConfig);
     if (operationIdFactory) {
@@ -271,10 +275,18 @@ export class SwaggerExplorer {
       VERSION_METADATA,
       method
     );
+    const versioningOptions = applicationConfig.getVersioning();
     const controllerVersion = this.getVersionMetadata(
       metatype,
-      applicationConfig.getVersioning()
+      versioningOptions
     );
+
+    const versionOrVersions = methodVersion ?? controllerVersion;
+    const versions = this.getRoutePathVersions(
+      versionOrVersions,
+      versioningOptions
+    );
+
     const allRoutePaths = this.routePathFactory.create(
       {
         methodPath,
@@ -302,27 +314,58 @@ export class SwaggerExplorer {
           return validMethods.map((meth) => ({
             method: meth.toLowerCase(),
             path: fullPath === '' ? '/' : fullPath,
-            operationId: `${this.getOperationId(instance, method)}_${meth.toLowerCase()}`,
+            operationId: `${this.getOperationId(
+              instance,
+              method
+            )}_${meth.toLowerCase()}`,
             ...apiExtension
           }));
         }
+        const pathVersion = versions.find((v) => fullPath.includes(`/${v}/`));
         return {
           method: RequestMethod[requestMethod].toLowerCase(),
           path: fullPath === '' ? '/' : fullPath,
-          operationId: this.getOperationId(instance, method),
+          operationId: this.getOperationId(instance, method, pathVersion),
           ...apiExtension
         };
       })
     );
   }
 
-  private getOperationId(instance: object, method: Function): string {
+  private getOperationId(
+    instance: object,
+    method: Function,
+    version?: string
+  ): string {
     return this.operationIdFactory(
       instance.constructor?.name || '',
-      method.name
+      method.name,
+      version
     );
   }
 
+  private getRoutePathVersions(
+    versionValue?: VersionValue,
+    versioningOptions?: VersioningOptions
+  ) {
+    let versions: string[] = [];
+
+    if (!versionValue || versioningOptions?.type !== VersioningType.URI) {
+      return versions;
+    }
+
+    if (Array.isArray(versionValue)) {
+      versions = versionValue.filter((v) => v !== VERSION_NEUTRAL) as string[];
+    } else if (versionValue !== VERSION_NEUTRAL) {
+      versions = [versionValue];
+    }
+
+    const prefix = this.routePathFactory.getVersionPrefix(versioningOptions);
+    versions = versions.map((v) => `${prefix}${v}`);
+
+    return versions;
+  }
+
   private reflectControllerPath(metatype: Type<unknown>): string {
     return Reflect.getMetadata(PATH_METADATA, metatype);
   }
diff --git a/lib/swagger-scanner.ts b/lib/swagger-scanner.ts
index 01b1d477..a0582b90 100644
--- a/lib/swagger-scanner.ts
+++ b/lib/swagger-scanner.ts
@@ -4,7 +4,11 @@ import { ApplicationConfig, NestContainer } from '@nestjs/core';
 import { InstanceWrapper } from '@nestjs/core/injector/instance-wrapper';
 import { Module } from '@nestjs/core/injector/module';
 import { flatten, isEmpty } from 'lodash';
-import { OpenAPIObject, SwaggerDocumentOptions } from './interfaces';
+import {
+  OpenAPIObject,
+  OperationIdFactory,
+  SwaggerDocumentOptions
+} from './interfaces';
 import { ModuleRoute } from './interfaces/module-route.interface';
 import {
   ReferenceObject,
@@ -106,7 +110,7 @@ export class SwaggerScanner {
     modulePath: string | undefined,
     globalPrefix: string | undefined,
     applicationConfig: ApplicationConfig,
-    operationIdFactory?: (controllerKey: string, methodKey: string) => string
+    operationIdFactory?: OperationIdFactory
   ): ModuleRoute[] {
     const denormalizedArray = [...controller.values()].map((ctrl) =>
       this.explorer.exploreController(


Test patch:
diff --git a/test/explorer/swagger-explorer.spec.ts b/test/explorer/swagger-explorer.spec.ts
index 5c37b2f7..9a5c4593 100644
--- a/test/explorer/swagger-explorer.spec.ts
+++ b/test/explorer/swagger-explorer.spec.ts
@@ -47,6 +47,14 @@ describe('SwaggerExplorer', () => {
     controllerKey: string,
     methodKey: string
   ) => `${controllerKey}.${methodKey}`;
+  const controllerKeyMethodKeyVersionKeyOperationIdFactory = (
+    controllerKey: string,
+    methodKey: string,
+    versionKey?: string
+  ) =>
+    versionKey
+      ? `${controllerKey}.${methodKey}.${versionKey}`
+      : `${controllerKey}.${methodKey}`;
 
   describe('when module only uses metadata', () => {
     class Foo {}
@@ -1325,6 +1333,10 @@ describe('SwaggerExplorer', () => {
       const CONTROLLER_VERSION: VersionValue = '1';
       const METHOD_VERSION: VersionValue = '2';
       const CONTROLLER_MULTIPLE_VERSIONS: VersionValue = ['3', '4'];
+      const CONTROLLER_MULTIPLE_VERSIONS_NEUTRAL: VersionValue = [
+        '5',
+        VERSION_NEUTRAL
+      ];
 
       @Controller({ path: 'with-version', version: CONTROLLER_VERSION })
       class WithVersionController {
@@ -1345,6 +1357,15 @@ describe('SwaggerExplorer', () => {
         foo(): void {}
       }
 
+      @Controller({
+        path: 'with-multiple-version-neutral',
+        version: CONTROLLER_MULTIPLE_VERSIONS_NEUTRAL
+      })
+      class WithMultipleVersionsNeutralController {
+        @Get()
+        foo(): void {}
+      }
+
       beforeAll(() => {
         explorer = new SwaggerExplorer(schemaObjectFactory);
 
@@ -1355,6 +1376,209 @@ describe('SwaggerExplorer', () => {
         });
       });
 
+      describe('and using the default operationIdFactory', () => {
+        it('should use controller version defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithVersionController(),
+              metatype: WithVersionController
+            } as InstanceWrapper<WithVersionController>,
+            config,
+            'modulePath',
+            'globalPrefix'
+          );
+
+          expect(routes[0].root.path).toEqual(
+            `/globalPrefix/v${CONTROLLER_VERSION}/modulePath/with-version`
+          );
+          expect(routes[0].root.operationId).toEqual(
+            `WithVersionController_foo`
+          );
+        });
+
+        it('should use route version defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithVersionController(),
+              metatype: WithVersionController
+            } as InstanceWrapper<WithVersionController>,
+            config,
+            'modulePath',
+            'globalPrefix'
+          );
+
+          expect(routes[1].root.path).toEqual(
+            `/globalPrefix/v${METHOD_VERSION}/modulePath/with-version`
+          );
+          expect(routes[1].root.operationId).toEqual(
+            `WithVersionController_bar`
+          );
+        });
+
+        it('should use multiple versions defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithMultipleVersionsController(),
+              metatype: WithMultipleVersionsController
+            } as InstanceWrapper<WithMultipleVersionsController>,
+            config,
+            'modulePath',
+            'globalPrefix'
+          );
+
+          expect(routes[0].root.path).toEqual(
+            `/globalPrefix/v${
+              CONTROLLER_MULTIPLE_VERSIONS[0] as string
+            }/modulePath/with-multiple-version`
+          );
+          expect(routes[0].root.operationId).toEqual(
+            `WithMultipleVersionsController_foo`
+          );
+          expect(routes[1].root.path).toEqual(
+            `/globalPrefix/v${
+              CONTROLLER_MULTIPLE_VERSIONS[1] as string
+            }/modulePath/with-multiple-version`
+          );
+          expect(routes[1].root.operationId).toEqual(
+            `WithMultipleVersionsController_foo`
+          );
+        });
+
+        it('should use multiple versions with neutral defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithMultipleVersionsNeutralController(),
+              metatype: WithMultipleVersionsNeutralController
+            } as InstanceWrapper<WithMultipleVersionsNeutralController>,
+            config,
+            'modulePath',
+            'globalPrefix'
+          );
+
+          expect(routes[0].root.path).toEqual(
+            `/globalPrefix/v${
+              CONTROLLER_MULTIPLE_VERSIONS_NEUTRAL[0] as string
+            }/modulePath/with-multiple-version-neutral`
+          );
+          expect(routes[0].root.operationId).toEqual(
+            `WithMultipleVersionsNeutralController_foo`
+          );
+
+          expect(routes[1].root.path).toEqual(
+            `/globalPrefix/modulePath/with-multiple-version-neutral`
+          );
+          expect(routes[1].root.operationId).toEqual(
+            `WithMultipleVersionsNeutralController_foo`
+          );
+        });
+      });
+
+      describe('and has an operationIdFactory that uses the method version', () => {
+        it('should use controller version defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithVersionController(),
+              metatype: WithVersionController
+            } as InstanceWrapper<WithVersionController>,
+            config,
+            'modulePath',
+            'globalPrefix',
+            controllerKeyMethodKeyVersionKeyOperationIdFactory
+          );
+
+          expect(routes[0].root.path).toEqual(
+            `/globalPrefix/v${CONTROLLER_VERSION}/modulePath/with-version`
+          );
+          expect(routes[0].root.operationId).toEqual(
+            `WithVersionController.foo.v${CONTROLLER_VERSION}`
+          );
+        });
+
+        it('should use route version defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithVersionController(),
+              metatype: WithVersionController
+            } as InstanceWrapper<WithVersionController>,
+            config,
+            'modulePath',
+            'globalPrefix',
+            controllerKeyMethodKeyVersionKeyOperationIdFactory
+          );
+
+          expect(routes[1].root.path).toEqual(
+            `/globalPrefix/v${METHOD_VERSION}/modulePath/with-version`
+          );
+          expect(routes[1].root.operationId).toEqual(
+            `WithVersionController.bar.v${METHOD_VERSION}`
+          );
+        });
+
+        it('should use multiple versions defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithMultipleVersionsController(),
+              metatype: WithMultipleVersionsController
+            } as InstanceWrapper<WithMultipleVersionsController>,
+            config,
+            'modulePath',
+            'globalPrefix',
+            controllerKeyMethodKeyVersionKeyOperationIdFactory
+          );
+
+          expect(routes[0].root.path).toEqual(
+            `/globalPrefix/v${
+              CONTROLLER_MULTIPLE_VERSIONS[0] as string
+            }/modulePath/with-multiple-version`
+          );
+          expect(routes[0].root.operationId).toEqual(
+            `WithMultipleVersionsController.foo.v${
+              CONTROLLER_MULTIPLE_VERSIONS[0] as string
+            }`
+          );
+          expect(routes[1].root.path).toEqual(
+            `/globalPrefix/v${
+              CONTROLLER_MULTIPLE_VERSIONS[1] as string
+            }/modulePath/with-multiple-version`
+          );
+          expect(routes[1].root.operationId).toEqual(
+            `WithMultipleVersionsController.foo.v${
+              CONTROLLER_MULTIPLE_VERSIONS[1] as string
+            }`
+          );
+        });
+
+        it('should use multiple versions with neutral defined', () => {
+          const routes = explorer.exploreController(
+            {
+              instance: new WithMultipleVersionsNeutralController(),
+              metatype: WithMultipleVersionsNeutralController
+            } as InstanceWrapper<WithMultipleVersionsNeutralController>,
+            config,
+            'modulePath',
+            'globalPrefix',
+            controllerKeyMethodKeyVersionKeyOperationIdFactory
+          );
+
+          expect(routes[0].root.path).toEqual(
+            `/globalPrefix/v${
+              CONTROLLER_MULTIPLE_VERSIONS_NEUTRAL[0] as string
+            }/modulePath/with-multiple-version-neutral`
+          );
+          expect(routes[0].root.operationId).toEqual(
+            `WithMultipleVersionsNeutralController.foo.v${
+              CONTROLLER_MULTIPLE_VERSIONS_NEUTRAL[0] as string
+            }`
+          );
+          expect(routes[1].root.path).toEqual(
+            `/globalPrefix/modulePath/with-multiple-version-neutral`
+          );
+          expect(routes[1].root.operationId).toEqual(
+            `WithMultipleVersionsNeutralController.foo`
+          );
+        });
+      });
+
       it('should use controller version defined', () => {
         const routes = explorer.exploreController(
           {