Repository: nestjs/swagger
Pull request #747 (2020-05-16 15:00:42)

feat(): add use method name for operations option
Make the behavior change for operation names that was introduced in https://github.com/nestjs/swagger/pull/487 configurable, and allow users whose client libraries have broken because of this (see https://github.com/nestjs/swagger/issues/482) to configure this behavior.

## 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)
- [x] Docs have been added / updated (for bug fixes / features) yes, see https://github.com/nestjs/docs.nestjs.com/pull/1247


## 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?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
Since #487 was merged, operation IDs generated by NestJS changed from e.g.
```
addUser
```
to
```
UserController_addUser
```
... this is causing users of generated client libraries (e.g. Angular) to have to update all their service calls to unnecessarily long names, e.g. from `designService.updateImages` to `designService.frontendDesignsControllerUpdateImages` (see https://github.com/nestjs/swagger/issues/482#issuecomment-577777620).

Issue Number: N/A


## What is the new behavior?
This PR allows users to configure this behavior. By default, nothing changes, unless the user configures @nestjs/swagger with a custom `operationIdFactory`:

```TypeScript
const app = await NestFactory.create(AppModule);
const operationIdFactory = (
  controllerKey: string,
  methodKey: string
) => `${methodKey}`;

const options = new DocumentBuilder()
  .setTitle('Cats example')
  .setDescription('The cats API description')
  .setVersion('1.0')
  .addTag('cats')
  .build();
const document = SwaggerModule.createDocument(app, options, {
  operationIdFactory
});
SwaggerModule.setup('api', app, document);
```

## Does this PR introduce a breaking change?
```
[ ] Yes
[x] No
```

<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. -->


## Other information

kamilmysliwiec: Thanks for your contribution!

Instead of adding a new configuration property (`useMethodNameForOperations`), could we just add a capability to define a custom operation ID factory? This would make it even more flexible so everyone can decide what schema to use for operations.

kamilmysliwiec: Closed by accident! Sorry about that
dennisameling: Sure, will have a look during the weekend 👍 
maxfriedmann: any news? desperately waiting for your fix to get released, blocks our swagger update since that issue got introduced 😊
dennisameling: Sorry, got very busy with work and other open source projects. Thanks for the heads up @maxfriedmann. If you have time for this that'd be fantastic, otherwise I can spend some time in the coming weekend 👍 
maxfriedmann: Now I know what you mean... Won't make it this week :( 
nartc: @dennisameling https://github.com/dennisameling/swagger/pull/1 I checked out your fork and adjusted the branch to use Kamil's suggestion. Please check it out.
kamilmysliwiec: @dennisameling any chance you can look into this soon? Otherwise, @nartc would you like to create a separate PR?
nartc: @kamilmysliwiec Yup. I can totally create a separate PR. 
dennisameling: @kamilmysliwiec @nartc Apologies for the late response 🤯 

Thanks @nartc for continuing work on this; if you could create a separate PR that'd be fantastic. Earliest I could look into this would be August 9 unfortunately. Feel free to close this PR then 🚀 
nartc: @dennisameling No worries. I’ll close this one once I have a new PR up
dennisameling: @nartc Are you still planning to work on this? I'll have some time in the coming weekend to work on this if necessary 😊 
nartc: @dennisameling Work has been hectic for me the last couple of weeks and yeah, feel free to work on it. I haven't started anything yet as of the last PR on your fork. 
kamilmysliwiec: Thanks @dennisameling, looks great! I've left 1 minor comment and once resolved, we should be good to merge!

Patch:
diff --git a/lib/interfaces/swagger-document-options.interface.ts b/lib/interfaces/swagger-document-options.interface.ts
index 59c6c6a9..1f1f9ab2 100644
--- a/lib/interfaces/swagger-document-options.interface.ts
+++ b/lib/interfaces/swagger-document-options.interface.ts
@@ -18,4 +18,11 @@ export interface SwaggerDocumentOptions {
    * If `true`, swagger will also load routes from the modules imported by `include` modules
    */
   deepScanRoutes?: boolean;
+
+  /**
+   * Custom operationIdFactory that will be used to generate the `operationId` based on the `controllerKey` and
+   * `methodKey`
+   * @default () => controllerKey_methodKey
+   */
+  operationIdFactory?: (controllerKey: string, methodKey: string) => string;
 }
diff --git a/lib/swagger-explorer.ts b/lib/swagger-explorer.ts
index e9ad56e9..c2b35b8b 100644
--- a/lib/swagger-explorer.ts
+++ b/lib/swagger-explorer.ts
@@ -56,14 +56,25 @@ export class SwaggerExplorer {
   private readonly metadataScanner = new MetadataScanner();
   private readonly schemas: SchemaObject[] = [];
   private readonly schemaRefsStack: string[] = [];
+  private operationIdFactory = (controllerKey: string, methodKey: string) => {
+    if (controllerKey) {
+      return `${controllerKey}_${methodKey}`;
+    } else {
+      return methodKey;
+    }
+  };
 
   constructor(private readonly schemaObjectFactory: SchemaObjectFactory) {}
 
   public exploreController(
     wrapper: InstanceWrapper<Controller>,
     modulePath?: string,
-    globalPrefix?: string
+    globalPrefix?: string,
+    operationIdFactory?: (controllerKey: string, methodKey: string) => string
   ) {
+    if (operationIdFactory) {
+      this.operationIdFactory = operationIdFactory;
+    }
     const { instance, metatype } = wrapper;
     const prototype = Object.getPrototypeOf(instance);
     const documentResolvers: DenormalizedDocResolvers = {
@@ -226,10 +237,10 @@ export class SwaggerExplorer {
   }
 
   private getOperationId(instance: object, method: Function): string {
-    if (instance.constructor) {
-      return `${instance.constructor.name}_${method.name}`;
-    }
-    return method.name;
+    return this.operationIdFactory(
+      instance.constructor?.name || '',
+      method.name
+    );
   }
 
   private reflectControllerPath(metatype: Type<unknown>): string {
diff --git a/lib/swagger-scanner.ts b/lib/swagger-scanner.ts
index dcf9c199..17c1fc84 100644
--- a/lib/swagger-scanner.ts
+++ b/lib/swagger-scanner.ts
@@ -32,7 +32,8 @@ export class SwaggerScanner {
       deepScanRoutes,
       include: includedModules = [],
       extraModels = [],
-      ignoreGlobalPrefix = false
+      ignoreGlobalPrefix = false,
+      operationIdFactory
     } = options;
 
     const container: NestContainer = (app as any).container;
@@ -64,7 +65,12 @@ export class SwaggerScanner {
           ? Reflect.getMetadata(MODULE_PATH, metatype)
           : undefined;
 
-        return this.scanModuleRoutes(allRoutes, path, globalPrefix);
+        return this.scanModuleRoutes(
+          allRoutes,
+          path,
+          globalPrefix,
+          operationIdFactory
+        );
       }
     );
 
@@ -85,10 +91,16 @@ export class SwaggerScanner {
   public scanModuleRoutes(
     routes: Map<string, InstanceWrapper>,
     modulePath?: string,
-    globalPrefix?: string
+    globalPrefix?: string,
+    operationIdFactory?: (controllerKey: string, methodKey: string) => string
   ): Array<Omit<OpenAPIObject, 'openapi' | 'info'> & Record<'root', any>> {
     const denormalizedArray = [...routes.values()].map((ctrl) =>
-      this.explorer.exploreController(ctrl, modulePath, globalPrefix)
+      this.explorer.exploreController(
+        ctrl,
+        modulePath,
+        globalPrefix,
+        operationIdFactory
+      )
     );
     return flatten(denormalizedArray) as any;
   }


Test patch:
diff --git a/test/explorer/swagger-explorer.spec.ts b/test/explorer/swagger-explorer.spec.ts
index 7340b30f..924bf4d2 100644
--- a/test/explorer/swagger-explorer.spec.ts
+++ b/test/explorer/swagger-explorer.spec.ts
@@ -19,12 +19,19 @@ import { ModelPropertiesAccessor } from '../../lib/services/model-properties-acc
 import { SchemaObjectFactory } from '../../lib/services/schema-object-factory';
 import { SwaggerTypesMapper } from '../../lib/services/swagger-types-mapper';
 import { SwaggerExplorer } from '../../lib/swagger-explorer';
+import { DenormalizedDoc } from '../../lib/interfaces/denormalized-doc.interface';
 
 describe('SwaggerExplorer', () => {
   const schemaObjectFactory = new SchemaObjectFactory(
     new ModelPropertiesAccessor(),
     new SwaggerTypesMapper()
   );
+  const methodKeyOperationIdFactory = (_, methodKey: string) => methodKey;
+  const controllerKeyMethodKeyOperationIdFactory = (
+    controllerKey: string,
+    methodKey: string
+  ) => `${controllerKey}.${methodKey}`;
+
   describe('when module only uses metadata', () => {
     class Foo {}
 
@@ -76,11 +83,51 @@ describe('SwaggerExplorer', () => {
         } as InstanceWrapper<FooController>,
         'path'
       );
+      const operationPrefix = 'FooController_';
+
+      validateRoutes(routes, operationPrefix);
+    });
+
+    it('sees two controller operations and their responses with custom operationIdFactory to return methodKey', () => {
+      const explorer = new SwaggerExplorer(schemaObjectFactory);
+      const routes = explorer.exploreController(
+        {
+          instance: new FooController(),
+          metatype: FooController
+        } as InstanceWrapper<FooController>,
+        'path',
+        undefined,
+        methodKeyOperationIdFactory
+      );
+      const operationPrefix = '';
+
+      validateRoutes(routes, operationPrefix);
+    });
+
+    it('sees two controller operations and their responses with custom operationIdFactory to return controllerKey.methodKey', () => {
+      const explorer = new SwaggerExplorer(schemaObjectFactory);
+      const routes = explorer.exploreController(
+        {
+          instance: new FooController(),
+          metatype: FooController
+        } as InstanceWrapper<FooController>,
+        'path',
+        undefined,
+        controllerKeyMethodKeyOperationIdFactory
+      );
+      const operationPrefix = 'FooController.';
 
+      validateRoutes(routes, operationPrefix);
+    });
+
+    const validateRoutes = (
+      routes: DenormalizedDoc[],
+      operationPrefix: string
+    ) => {
       expect(routes.length).toEqual(2);
 
       // POST
-      expect(routes[0].root.operationId).toEqual('FooController_create');
+      expect(routes[0].root.operationId).toEqual(operationPrefix + 'create');
       expect(routes[0].root.method).toEqual('post');
       expect(routes[0].root.path).toEqual('/path/foos');
       expect(routes[0].root.summary).toEqual('Create foo');
@@ -141,7 +188,7 @@ describe('SwaggerExplorer', () => {
       });
 
       // GET
-      expect(routes[1].root.operationId).toEqual('FooController_find');
+      expect(routes[1].root.operationId).toEqual(operationPrefix + 'find');
       expect(routes[1].root.method).toEqual('get');
       expect(routes[1].root.path).toEqual('/path/foos/{objectId}');
       expect(routes[1].root.summary).toEqual('List all Foos');
@@ -179,8 +226,9 @@ describe('SwaggerExplorer', () => {
           }
         }
       });
-    });
+    };
   });
+
   describe('when explicit decorators and metadata are used', () => {
     class Foo {}
 
@@ -223,11 +271,51 @@ describe('SwaggerExplorer', () => {
         } as InstanceWrapper<FooController>,
         'path'
       );
+      const prefix = 'FooController_';
+
+      validateRoutes(routes, prefix);
+    });
+
+    it('sees two controller operations and their responses with custom operationIdFactory to return methodKey', () => {
+      const explorer = new SwaggerExplorer(schemaObjectFactory);
+      const routes = explorer.exploreController(
+        {
+          instance: new FooController(),
+          metatype: FooController
+        } as InstanceWrapper<FooController>,
+        'path',
+        undefined,
+        methodKeyOperationIdFactory
+      );
+      const prefix = '';
+
+      validateRoutes(routes, prefix);
+    });
+
+    it('sees two controller operations and their responses with custom operationIdFactory to return controllerKey.methodKey', () => {
+      const explorer = new SwaggerExplorer(schemaObjectFactory);
+      const routes = explorer.exploreController(
+        {
+          instance: new FooController(),
+          metatype: FooController
+        } as InstanceWrapper<FooController>,
+        'path',
+        undefined,
+        controllerKeyMethodKeyOperationIdFactory
+      );
+      const prefix = 'FooController.';
 
+      validateRoutes(routes, prefix);
+    });
+
+    const validateRoutes = (
+      routes: DenormalizedDoc[],
+      operationPrefix: string
+    ) => {
       expect(routes.length).toEqual(2);
 
       // POST
-      expect(routes[0].root.operationId).toEqual('FooController_create');
+      expect(routes[0].root.operationId).toEqual(operationPrefix + 'create');
       expect(routes[0].root.method).toEqual('post');
       expect(routes[0].root.path).toEqual('/path/foos');
       expect(routes[0].root.summary).toEqual('Create foo');
@@ -260,7 +348,7 @@ describe('SwaggerExplorer', () => {
       });
 
       // GET
-      expect(routes[1].root.operationId).toEqual('FooController_find');
+      expect(routes[1].root.operationId).toEqual(operationPrefix + 'find');
       expect(routes[1].root.method).toEqual('get');
       expect(routes[1].root.path).toEqual('/path/foos/{objectId}');
       expect(routes[1].root.summary).toEqual('List all Foos');
@@ -310,7 +398,7 @@ describe('SwaggerExplorer', () => {
           }
         }
       });
-    });
+    };
   });
   describe('when only explicit decorators are used', () => {
     class Foo {}
@@ -351,11 +439,51 @@ describe('SwaggerExplorer', () => {
         } as InstanceWrapper<FooController>,
         'path'
       );
+      const operationPrefix = 'FooController_';
+
+      validateRoutes(routes, operationPrefix);
+    });
+
+    it('sees two controller operations and their responses with custom operationIdFactory to return methodKey', () => {
+      const explorer = new SwaggerExplorer(schemaObjectFactory);
+      const routes = explorer.exploreController(
+        {
+          instance: new FooController(),
+          metatype: FooController
+        } as InstanceWrapper<FooController>,
+        'path',
+        undefined,
+        methodKeyOperationIdFactory
+      );
+      const operationPrefix = '';
+
+      validateRoutes(routes, operationPrefix);
+    });
+
+    it('sees two controller operations and their responses with custom operationIdFactory to return controllerKey.methodKey', () => {
+      const explorer = new SwaggerExplorer(schemaObjectFactory);
+      const routes = explorer.exploreController(
+        {
+          instance: new FooController(),
+          metatype: FooController
+        } as InstanceWrapper<FooController>,
+        'path',
+        undefined,
+        controllerKeyMethodKeyOperationIdFactory
+      );
+      const operationPrefix = 'FooController.';
 
+      validateRoutes(routes, operationPrefix);
+    });
+
+    const validateRoutes = (
+      routes: DenormalizedDoc[],
+      operationPrefix: string
+    ) => {
       expect(routes.length).toEqual(2);
 
       // POST
-      expect(routes[0].root.operationId).toEqual('FooController_create');
+      expect(routes[0].root.operationId).toEqual(operationPrefix + 'create');
       expect(routes[0].root.method).toEqual('post');
       expect(routes[0].root.path).toEqual('/path/foos');
       expect(routes[0].root.summary).toEqual('Create foo');
@@ -385,7 +513,7 @@ describe('SwaggerExplorer', () => {
       });
 
       // GET
-      expect(routes[1].root.operationId).toEqual('FooController_find');
+      expect(routes[1].root.operationId).toEqual(operationPrefix + 'find');
       expect(routes[1].root.method).toEqual('get');
       expect(routes[1].root.path).toEqual('/path/foos/{objectId}');
       expect(routes[1].root.summary).toEqual('List all Foos');
@@ -424,7 +552,7 @@ describe('SwaggerExplorer', () => {
           }
         }
       });
-    });
+    };
   });
   describe('when custom properties are passed', () => {
     class Foo {}
@@ -492,6 +620,24 @@ describe('SwaggerExplorer', () => {
         'path'
       );
 
+      validateRoutes(routes);
+    });
+
+    it('should merge implicit metadata with explicit options and use default operationIdFactory', () => {
+      const explorer = new SwaggerExplorer(schemaObjectFactory);
+      const routes = explorer.exploreController(
+        {
+          instance: new FooController(),
+          metatype: FooController
+        } as InstanceWrapper<FooController>,
+        'path',
+        undefined
+      );
+
+      validateRoutes(routes);
+    });
+
+    const validateRoutes = (routes: DenormalizedDoc[]) => {
       expect(routes.length).toEqual(2);
 
       // POST
@@ -581,7 +727,7 @@ describe('SwaggerExplorer', () => {
           }
         }
       });
-    });
+    };
   });
   describe('when enum is used', () => {
     enum ParamEnum {