feat: Add recursiveModuleScan option to scanner #3186
Conversation
|
|
||
| @ApiTags('depth1-dogs') | ||
| @Controller('depth1-dogs') | ||
| export class Depth1DogsController { |
There was a problem hiding this comment.
The files under e2e/src/dogs are modules used for testing purposes.
| @@ -215,3 +216,115 @@ describe('Validate OpenAPI schema', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Nested module scanning', () => { | |||
There was a problem hiding this comment.
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.
| * | ||
| * @default Infinity | ||
| */ | ||
| maxScanDepth?: number; |
There was a problem hiding this comment.
The default value for maxScanDepth is currently set to Infinity. If you have any suggestions, let me know.
hee-c
changed the title
hee-c
changed the title
hee-c
changed the title
The module scanning now correctly respects: - Default scan depth (1 level) with `deepScanRoutes` - Full recursive scanning with `recursiveModuleScan` - Custom depth limits with `maxScanDepth`
hee-c
force-pushed
the
hee/allow-swagger-scanner-deepScanRoute
branch
from
9ece4d5
to
4ca7799
Compare
| } | ||
|
|
||
| private getModuleId(metatype: Type<any>): string { | ||
| return metatype.name || Math.random().toString(36).substring(2); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
| return metatype.name || Math.random().toString(36).substring(2); | |
| return metatype.toString(); |
There was a problem hiding this comment.
Sounds reasonable. I'm afraid the size of the array might become notably large for bigger projects though
There was a problem hiding this comment.
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?
| 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)) { |
There was a problem hiding this comment.
| 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
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The Swagger scanner only scans up to 1-depth for modules included via the
includeoption, even whendeepScanRoutesis set totrue.Issue Number: #3102
What is the new behavior?
When
deepScanRoutesis set totrueand the newly addedrecursiveModuleScanflag is also set totrue, the modules included viaincludewill be scanned recursively up to the depth defined bymaxScanDepth.deepScanRoutes: false: Only scans root modules imported byinclude(current behavior).deepScanRoutes: true: Scans 1-depth modules imported byinclude(current behavior).deepScanRoutes: true, recursiveModuleScan: true: Scans all modules recursively, with no depth limit.deepScanRoutes: true, recursiveModuleScan: true, maxScanDepth: 5: Scans modules recursively up to a depth of 5.Does this PR introduce a breaking change?
Other information