Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add recursiveModuleScan option to scanner #3186

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions e2e/src/dogs/depth1-dogs/depth1-dogs.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Controller, Get } from '@nestjs/common';
import { ApiTags } from '../../../../lib';

@ApiTags('depth1-dogs')
@Controller('depth1-dogs')
export class Depth1DogsController {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files under e2e/src/dogs are modules used for testing purposes.

@Get()
findAll() {
return 'Depth1 Dogs';
}
}
9 changes: 9 additions & 0 deletions e2e/src/dogs/depth1-dogs/depth1-dogs.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { Module } from '@nestjs/common';
import { Depth1DogsController } from './depth1-dogs.controller';
import { Depth2DogsModule } from '../depth2-dogs/depth2-dogs.module';

@Module({
imports: [Depth2DogsModule],
controllers: [Depth1DogsController]
})
export class Depth1DogsModule {}
11 changes: 11 additions & 0 deletions e2e/src/dogs/depth2-dogs/depth2-dogs.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Controller, Get } from '@nestjs/common';
import { ApiTags } from '../../../../lib';

@ApiTags('depth2-dogs')
@Controller('depth2-dogs')
export class Depth2DogsController {
@Get()
findAll() {
return 'Depth2 Dogs';
}
}
9 changes: 9 additions & 0 deletions e2e/src/dogs/depth2-dogs/depth2-dogs.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { Module } from '@nestjs/common';
import { Depth2DogsController } from './depth2-dogs.controller';
import { Depth3DogsModule } from '../depth3-dogs/depth3-dogs.module';

@Module({
imports: [Depth3DogsModule],
controllers: [Depth2DogsController]
})
export class Depth2DogsModule {}
11 changes: 11 additions & 0 deletions e2e/src/dogs/depth3-dogs/depth3-dogs.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Controller, Get } from '@nestjs/common';
import { ApiTags } from '../../../../lib';

@ApiTags('depth3-dogs')
@Controller('depth3-dogs')
export class Depth3DogsController {
@Get()
findAll() {
return 'Depth3 Dogs';
}
}
7 changes: 7 additions & 0 deletions e2e/src/dogs/depth3-dogs/depth3-dogs.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { Module } from '@nestjs/common';
import { Depth3DogsController } from './depth3-dogs.controller';

@Module({
controllers: [Depth3DogsController]
})
export class Depth3DogsModule {}
16 changes: 16 additions & 0 deletions e2e/src/dogs/dogs.controller.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { Controller, Get } from '@nestjs/common';
import { ApiTags } from '../../../lib';

@ApiTags('dogs')
@Controller('dogs')
export class DogsController {
@Get()
findAll() {
return 'Dogs';
}

@Get('puppies')
findPuppies() {
return 'Puppies';
}
}
9 changes: 9 additions & 0 deletions e2e/src/dogs/dogs.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { Module } from '@nestjs/common';
import { DogsController } from './dogs.controller';
import { Depth1DogsModule } from './depth1-dogs/depth1-dogs.module';

@Module({
imports: [Depth1DogsModule],
controllers: [DogsController]
})
export class DogsModule {}
113 changes: 113 additions & 0 deletions e2e/validate-schema.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import { ApplicationModule } from './src/app.module';
import { Cat } from './src/cats/classes/cat.class';
import { TagDto } from './src/cats/dto/tag.dto';
import { DogsModule } from './src/dogs/dogs.module';

describe('Validate OpenAPI schema', () => {
let app: INestApplication;
Expand Down Expand Up @@ -215,3 +216,115 @@ describe('Validate OpenAPI schema', () => {
});
});
});

describe('Nested module scanning', () => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the tests here for now, but if you prefer to separate them into a dedicated file, let me know and I will update accordingly.

let app: INestApplication;
let options: Omit<OpenAPIObject, 'paths'>;

beforeEach(async () => {
app = await NestFactory.create(DogsModule, {
logger: false
});
app.setGlobalPrefix('api/');

options = new DocumentBuilder()
.setTitle('Cats example')
.setDescription('The cats API description')
.setVersion('1.0')
.build();
});

describe('deepScanRoutes', () => {
it('should include only 1-depth nested routes when deepScanRoutes is true', async () => {
const document = SwaggerModule.createDocument(app, options, {
deepScanRoutes: true,
include: [DogsModule]
});

// Root module routes should be included
expect(document.paths['/api/dogs']).toBeDefined();
expect(document.paths['/api/dogs/puppies']).toBeDefined();

// First depth routes should be included
expect(document.paths['/api/depth1-dogs']).toBeDefined();

// Deeper routes should NOT be included
expect(document.paths['/api/depth2-dogs']).toBeUndefined();
expect(document.paths['/api/depth3-dogs']).toBeUndefined();

// Verify controller tags are correct
expect(document.paths['/api/dogs'].get.tags).toContain('dogs');
expect(document.paths['/api/depth1-dogs'].get.tags).toContain('depth1-dogs');
});
});

describe('recursiveModuleScan', () => {
it('should include all nested routes when recursiveModuleScan is enabled', async () => {
const document = SwaggerModule.createDocument(app, options, {
include: [DogsModule],
deepScanRoutes: true,
recursiveModuleScan: true
});

// All routes at every depth should be included
expect(document.paths['/api/dogs']).toBeDefined();
expect(document.paths['/api/dogs/puppies']).toBeDefined();
expect(document.paths['/api/depth1-dogs']).toBeDefined();
expect(document.paths['/api/depth2-dogs']).toBeDefined();
expect(document.paths['/api/depth3-dogs']).toBeDefined();

// Verify all controller tags are correct
expect(document.paths['/api/dogs'].get.tags).toContain('dogs');
expect(document.paths['/api/depth1-dogs'].get.tags).toContain('depth1-dogs');
expect(document.paths['/api/depth2-dogs'].get.tags).toContain('depth2-dogs');
expect(document.paths['/api/depth3-dogs'].get.tags).toContain('depth3-dogs');
});
});

describe('maxScanDepth', () => {
it('should limit scanning depth when maxScanDepth is set', async () => {
const document = SwaggerModule.createDocument(app, options, {
include: [DogsModule],
deepScanRoutes: true,
recursiveModuleScan: true,
maxScanDepth: 1
});

// Routes up to depth 1 should be included
expect(document.paths['/api/dogs']).toBeDefined();
expect(document.paths['/api/dogs/puppies']).toBeDefined();
expect(document.paths['/api/depth1-dogs']).toBeDefined();

// Routes beyond depth 1 should NOT be included
expect(document.paths['/api/depth2-dogs']).toBeUndefined();
expect(document.paths['/api/depth3-dogs']).toBeUndefined();

// Verify included controller tags are correct
expect(document.paths['/api/dogs'].get.tags).toContain('dogs');
expect(document.paths['/api/depth1-dogs'].get.tags).toContain('depth1-dogs');
});

it('should include routes up to specified maxScanDepth', async () => {
const document = SwaggerModule.createDocument(app, options, {
include: [DogsModule],
deepScanRoutes: true,
recursiveModuleScan: true,
maxScanDepth: 2
});

// Routes up to depth 2 should be included
expect(document.paths['/api/dogs']).toBeDefined();
expect(document.paths['/api/dogs/puppies']).toBeDefined();
expect(document.paths['/api/depth1-dogs']).toBeDefined();
expect(document.paths['/api/depth2-dogs']).toBeDefined();

// Routes beyond depth 2 should NOT be included
expect(document.paths['/api/depth3-dogs']).toBeUndefined();

// Verify included controller tags are correct
expect(document.paths['/api/dogs'].get.tags).toContain('dogs');
expect(document.paths['/api/depth1-dogs'].get.tags).toContain('depth1-dogs');
expect(document.paths['/api/depth2-dogs'].get.tags).toContain('depth2-dogs');
});
});
});
16 changes: 16 additions & 0 deletions lib/interfaces/swagger-document-options.interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,20 @@ export interface SwaggerDocumentOptions {
* @default true
*/
autoTagControllers?: boolean;

/**
* If `true`, swagger will recursively scan all nested imported by `include` modules.
* When enabled, this overrides the default behavior of `deepScanRoutes` to scan all depths.
*
* @default false
*/
recursiveModuleScan?: boolean;

/**
* Maximum depth level for recursive module scanning.
* Only applies when `recursiveModuleScan` is `true`.
*
* @default Infinity
*/
maxScanDepth?: number;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value for maxScanDepth is currently set to Infinity. If you have any suggestions, let me know.

}
128 changes: 109 additions & 19 deletions lib/swagger-scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ export class SwaggerScanner {
ignoreGlobalPrefix = false,
operationIdFactory,
linkNameFactory,
autoTagControllers = true
autoTagControllers = true,
recursiveModuleScan = false,
maxScanDepth = Infinity
} = options;

const container = (app as any).container as NestContainer;
Expand All @@ -55,34 +57,55 @@ export class SwaggerScanner {
? stripLastSlash(getGlobalPrefix(app))
: '';

const processedModules = new Set<string>();

const denormalizedPaths = modules.map(
({ controllers, metatype, imports }) => {
let result: ModuleRoute[] = [];

if (deepScanRoutes) {
// Only load submodules routes if explicitly enabled
const isGlobal = (module: Type<any>) =>
!container.isGlobalModule(module);

Array.from(imports.values())
.filter(isGlobal as any)
.forEach(({ metatype, controllers }) => {
const modulePath = this.getModulePathMetadata(
if (!recursiveModuleScan) {
// Only load submodules routes if explicitly enabled
const isGlobal = (module: Type<any>) =>
!container.isGlobalModule(module);

Array.from(imports.values())
.filter(isGlobal as any)
.forEach(({ metatype, controllers }) => {
const modulePath = this.getModulePathMetadata(
container,
metatype
);
result = result.concat(
this.scanModuleControllers(
controllers,
modulePath,
globalPrefix,
internalConfigRef,
operationIdFactory,
linkNameFactory,
autoTagControllers
)
);
});
} else {
result = result.concat(
this.scanModuleImportsRecursively(
imports,
container,
metatype
);
result = result.concat(
this.scanModuleControllers(
controllers,
modulePath,
0,
maxScanDepth,
processedModules,
{
globalPrefix,
internalConfigRef,
operationIdFactory,
linkNameFactory,
autoTagControllers
)
);
});
autoTagControllers,
}
)
);
}
}
const modulePath = this.getModulePathMetadata(container, metatype);
result = result.concat(
Expand Down Expand Up @@ -170,4 +193,71 @@ export class SwaggerScanner {
);
return modulePath ?? Reflect.getMetadata(MODULE_PATH, metatype);
}

private scanModuleImportsRecursively(
imports: Set<Module>,
container: NestContainer,
currentDepth: number,
maxDepth: number | undefined,
processedModules: Set<string>,
options: {
globalPrefix: string;
internalConfigRef: ApplicationConfig;
operationIdFactory?: OperationIdFactory;
linkNameFactory?: (
controllerKey: string,
methodKey: string,
fieldKey: string
) => string;
autoTagControllers?: boolean;
}
): ModuleRoute[] {
let result: ModuleRoute[] = [];

for (const { metatype, controllers, imports: subImports } of imports.values()) {
// Skip if module has already been processed
const moduleId = this.getModuleId(metatype);
if (processedModules.has(moduleId) || container.isGlobalModule(metatype) || (maxDepth !== undefined && currentDepth > maxDepth)) {
Comment on lines +217 to +220
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (const { metatype, controllers, imports: subImports } of imports.values()) {
// Skip if module has already been processed
const moduleId = this.getModuleId(metatype);
if (processedModules.has(moduleId) || container.isGlobalModule(metatype) || (maxDepth !== undefined && currentDepth > maxDepth)) {
for (const { token, metatype, controllers, imports: subImports } of imports.values()) {
// Skip if module has already been processed
if (processedModules.has(token) || container.isGlobalModule(metatype) || (maxDepth !== undefined && currentDepth > maxDepth)) {

Here's an example implementation of the approach I suggested

continue;
}
processedModules.add(moduleId);

// Scan current module's controllers
const modulePath = this.getModulePathMetadata(container, metatype);
result = result.concat(
this.scanModuleControllers(
controllers,
modulePath,
options.globalPrefix,
options.internalConfigRef,
options.operationIdFactory,
options.linkNameFactory,
options.autoTagControllers
)
);

// Process sub-imports if any
if (subImports.size > 0) {
const nextDepth = currentDepth + 1;
if (maxDepth === undefined || nextDepth < maxDepth) {
result = result.concat(
this.scanModuleImportsRecursively(
subImports,
container,
nextDepth,
maxDepth,
processedModules,
options
)
);
}
}
}

return result;
}

private getModuleId(metatype: Type<any>): string {
return metatype.name || Math.random().toString(36).substring(2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand what's the purpose of having Math.random().toString(36).substring(2) if it isn't calculated based on the input argument value (metatype)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right about the random fallback being unnecessary. Let me explain the context better. The getModuleId is used with processedModules set to prevent duplicate scanning and circular dependencies while recursively scanning module imports. However, I realized that using just metatype.name could be problematic - if we have two different modules with the same name in different locations (like auth.module.ts in different directories), only one would be scanned. To properly handle such cases while still preventing infinite recursion, I think we should use metatype.toString(). While this generates longer strings, it ensures we don't miss any modules during the scanning process. What do you think about this approach?

Suggested change
return metatype.name || Math.random().toString(36).substring(2);
return metatype.toString();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. I'm afraid the size of the array might become notably large for bigger projects though

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at ModuleTokenFactory.create, I see that id uses uid(21) and token adds metatype.name and dynamicModuleMetadata(optional) to that. Since NestContainer.addModule seems to use token as the identifier, rather than using metatype, what if we destructure token along with the existing properties in the imports.values() loop within the scanModuleImportsRecursively method and use that as the moduleId?

}
}