From 00b2b62af45ddb67108ed50b36660fa42aabdf68 Mon Sep 17 00:00:00 2001 From: Deon Groenewald Date: Wed, 2 Aug 2023 14:00:01 -0400 Subject: [PATCH 1/3] feat(swagger): Provide URI version to operationIdFactory 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. This is based on the work completed in https://github.com/nestjs/swagger/pull/1949. Those changes were reverted due to test failures. This change adapts those changes to also support neutral versions which resolves the test failures. Closes nestjs#2537 --- .../swagger-document-options.interface.ts | 14 +- lib/swagger-explorer.ts | 46 +++- lib/swagger-scanner.ts | 8 +- test/explorer/swagger-explorer.spec.ts | 224 ++++++++++++++++++ 4 files changed, 282 insertions(+), 10 deletions(-) diff --git a/lib/interfaces/swagger-document-options.interface.ts b/lib/interfaces/swagger-document-options.interface.ts index 041c62227..3c8c96300 100644 --- a/lib/interfaces/swagger-document-options.interface.ts +++ b/lib/interfaces/swagger-document-options.interface.ts @@ -1,3 +1,15 @@ +export type OperationIdFactory = + | (( + controllerKey: string, + methodKey: string, + pathVersionKey?: string + ) => string) + | (( + controllerKey: string, + methodKey: string, + pathVersionKey?: string + ) => string); + export interface SwaggerDocumentOptions { /** * List of modules to include in the specification @@ -24,5 +36,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 2e0520317..1978e4498 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 = {}; - 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) { @@ -275,6 +279,25 @@ export class SwaggerExplorer { metatype, applicationConfig.getVersioning() ); + + const versionOrVersions = methodVersion ?? controllerVersion; + let versions: string[] = []; + if (!!versionOrVersions) { + if (Array.isArray(versionOrVersions)) { + versions = versionOrVersions.filter( + (v) => v !== VERSION_NEUTRAL + ) as string[]; + } else if (versionOrVersions !== VERSION_NEUTRAL) { + versions = [versionOrVersions]; + } + + const versionConfig = applicationConfig.getVersioning(); + if (versionConfig?.type == VersioningType.URI) { + const prefix = this.routePathFactory.getVersionPrefix(versionConfig); + versions = versions.map((v) => `${prefix}${v}`); + } + } + const allRoutePaths = this.routePathFactory.create( { methodPath, @@ -302,24 +325,33 @@ 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, + pathVersion?: string + ): string { return this.operationIdFactory( instance.constructor?.name || '', - method.name + method.name, + pathVersion ); } diff --git a/lib/swagger-scanner.ts b/lib/swagger-scanner.ts index 01b1d477c..a0582b90e 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( diff --git a/test/explorer/swagger-explorer.spec.ts b/test/explorer/swagger-explorer.spec.ts index 5c37b2f70..9a5c4593b 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, + 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, + 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, + 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, + 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, + 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, + 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, + 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, + 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( { From 473133649eae0e09aab6ef96c817bc61be08bc2b Mon Sep 17 00:00:00 2001 From: Deon Groenewald Date: Thu, 3 Aug 2023 09:25:23 -0400 Subject: [PATCH 2/3] Fix incorrect `OperationIdFactory` type. --- .../swagger-document-options.interface.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/interfaces/swagger-document-options.interface.ts b/lib/interfaces/swagger-document-options.interface.ts index 3c8c96300..3efdc7ed5 100644 --- a/lib/interfaces/swagger-document-options.interface.ts +++ b/lib/interfaces/swagger-document-options.interface.ts @@ -1,14 +1,8 @@ -export type OperationIdFactory = - | (( - controllerKey: string, - methodKey: string, - pathVersionKey?: string - ) => string) - | (( - controllerKey: string, - methodKey: string, - pathVersionKey?: string - ) => string); +export type OperationIdFactory = ( + controllerKey: string, + methodKey: string, + pathVersionKey?: string +) => string; export interface SwaggerDocumentOptions { /** From 7cbe0ef89875b9086648cbbd95ecddb2d855b4c1 Mon Sep 17 00:00:00 2001 From: Deon Groenewald Date: Fri, 4 Aug 2023 09:43:41 -0400 Subject: [PATCH 3/3] Address additional PR feedback --- .../swagger-document-options.interface.ts | 2 +- lib/swagger-explorer.ts | 49 ++++++++++++------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/lib/interfaces/swagger-document-options.interface.ts b/lib/interfaces/swagger-document-options.interface.ts index 3efdc7ed5..489e91781 100644 --- a/lib/interfaces/swagger-document-options.interface.ts +++ b/lib/interfaces/swagger-document-options.interface.ts @@ -1,7 +1,7 @@ export type OperationIdFactory = ( controllerKey: string, methodKey: string, - pathVersionKey?: string + version?: string ) => string; export interface SwaggerDocumentOptions { diff --git a/lib/swagger-explorer.ts b/lib/swagger-explorer.ts index 1978e4498..d6e9da386 100644 --- a/lib/swagger-explorer.ts +++ b/lib/swagger-explorer.ts @@ -275,28 +275,17 @@ export class SwaggerExplorer { VERSION_METADATA, method ); + const versioningOptions = applicationConfig.getVersioning(); const controllerVersion = this.getVersionMetadata( metatype, - applicationConfig.getVersioning() + versioningOptions ); const versionOrVersions = methodVersion ?? controllerVersion; - let versions: string[] = []; - if (!!versionOrVersions) { - if (Array.isArray(versionOrVersions)) { - versions = versionOrVersions.filter( - (v) => v !== VERSION_NEUTRAL - ) as string[]; - } else if (versionOrVersions !== VERSION_NEUTRAL) { - versions = [versionOrVersions]; - } - - const versionConfig = applicationConfig.getVersioning(); - if (versionConfig?.type == VersioningType.URI) { - const prefix = this.routePathFactory.getVersionPrefix(versionConfig); - versions = versions.map((v) => `${prefix}${v}`); - } - } + const versions = this.getRoutePathVersions( + versionOrVersions, + versioningOptions + ); const allRoutePaths = this.routePathFactory.create( { @@ -346,15 +335,37 @@ export class SwaggerExplorer { private getOperationId( instance: object, method: Function, - pathVersion?: string + version?: string ): string { return this.operationIdFactory( instance.constructor?.name || '', method.name, - pathVersion + 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): string { return Reflect.getMetadata(PATH_METADATA, metatype); }