feat(graph): add NestJS controller route resolver - #102
Conversation
a652f92 to
99de109
Compare
|
Hi @theDakshJaitly @Yashasvi2229 — this PR has been open since mid-July with no reviews, so I gave it some maintenance:
The implementation targets the frozen Is anything blocking this from review — or would you like changes first? Happy to iterate. (Tracking issue: #98, which this resolves.) |
|
Yeah our bad, got caught up in the new release, sorry for the delay in review We will review asap, thanks for the rebase as well |
99de109 to
972c49c
Compare
|
Thanks for this one @abhinav-phi, and for the rebase. The resolver is additive, detection is solid, and the happy path works end to end: I ran real I then probed the extractor against the decorator forms real NestJS code uses. Three things need fixing before merge. Must fix1. Two routes with the same method + path fail the whole graph buildThe route id is @Controller('users')
export class UsersController {
@Version('1') @Get() findAllV1() {}
@Version('2') @Get() findAllV2() {}
}
2. Non-string
|
| source | derived | expected |
|---|---|---|
@Controller({ path: 'users', version: '1' }) + @Get(':id') |
GET /:id |
GET /users/:id |
same, after an @Controller('admin') class in the file |
GET /admin/:id |
GET /users/:id |
@Controller(USERS_PATH) |
GET /:id |
unknown |
@Controller(['users', 'people']) + @Get() |
GET / |
GET /users, GET /people |
The object form is common (it's how controller versioning and host routing are declared), so please parse { path: '…' }. For the forms that can't be read statically (a constant, an array), skip that controller's routes rather than emitting a wrong path. Either way, every @Controller(...) should reset the prefix so nothing carries over.
3. Commented-out decorators create phantom routes
The regex runs over the raw file text, comments included:
@Controller('users')
export class UsersController {
// @Get('legacy')
@Get(':id')
findOne() {}
}This produces both GET /users/:id and a phantom GET /users/legacy, both pointing at findOne. Blanking comments out before scanning (with spaces, so offsets and line numbers stay correct) fixes this and the block-comment case in #1.
Should fix
4. Two controllers in one file lose their links
Resolution matches the handler by name within the file, so two controllers in one file that both have findAll leave both routes unresolved. I confirmed this in a real build. #98 asks to prefer the class context. The TypeScript extractor already names methods UsersController::findAll, so tracking the class that follows each @Controller and matching on qualifiedName resolves both.
Smaller things
- Parenthesis skipping isn't string-aware.
@ApiOperation({ summary: 'List users :)' })between@Get()and the method silently drops the route. Rare, but it fails without any signal. - Array method paths.
@Get(['list', 'all'])produces no route. - Integration test. The unit tests use a fake context. One test through
rebuildGraph(like the Next.js resolver's in feat(graph): add Next.js App Router route resolver #179) would have caught 1 and 4. - Edge label.
resolvedBy: "framework"/ confidence1doesn't identify the resolver; Express usesexpress-route-handler/0.8for the same evidence. Something likenestjs-route-handlerwould match. - Fixture and header comments. The fixture has leftover notes ("Missing handler name (anonymous function)… let's just make it a normal one"), and the file header mentions a single-controller-per-file assumption the code doesn't make. Worth tidying.
- CHANGELOG. Please add an entry under
## [Unreleased]→### Added, as feat(graph): add Next.js App Router route resolver #179 does.
Review-driven rework of the NestJS route resolver, addressing three must-fix findings from real-build probing: - Duplicate node ids killed whole builds: NestJS versioning (@Version('1') @get() findAllV1 / @Version('2') @get() findAllV2) emits two routes with the same method+path. The handler now rides in the signature (GET /users -> findAllV1) and an ordinal joins the role, mirroring Express, so same-named routes keep distinct ids. - @controller arguments beyond string literals were skipped entirely, so the prefix was lost or inherited from the previous controller. Arguments are now read string-aware (a ')' inside 'List users :)' no longer breaks anything): string form, object form ({ path: ... }), and empty are handled; constants and arrays skip that controller's routes rather than guessing. Every @controller resets the prefix and binds to the next class, so two controllers in one file each keep their own prefix. - Commented-out decorators invented phantom routes. Comments are blanked with spaces before scanning — offsets and line numbers survive, comments stop being code. Resolution now records the owning controller class in the reference candidates and prefers a same-file qualified-name match, so two controllers that both declare findAll no longer leave both routes unresolved. Edge label moves to nestjs-route-handler at confidence 0.8, matching Express's evidence class. Fixture notes tidied; a rebuildGraph integration test covers the versioned-route and comment-decorator cases end to end. Addresses review on mex-memory#102
972c49c to
ad61598
Compare
|
All findings addressed — thanks for probing the decorator forms; every one of these reproduced locally. Must fixes
Should fix 4 — the reference now carries Smaller things — paren skipping is string-aware ( The one deliberate non-change: |
What
FrameworkResolverfor decorator-defined HTTP controller routes.@nestjs/coreor@nestjs/commondependencies.@Controller()prefixes and HTTP method decorators such as@Get(),@Post(),@Put(),@Patch(),@Delete(),@Options(),@Head(), and@All().function_refreferences to controller methods.Why
Closes #98.
This teaches the code graph about NestJS controller routing without changing the frozen
FrameworkResolverinterface or graph-core semantics.Scope boundaries
This PR is limited to statically recognizable HTTP controller routes. Dependency-injection edges, guards, pipes, interceptors, middleware, gateways, GraphQL, microservices, and runtime decorator evaluation remain out of scope.
How to test
npm run typecheck npm test npm run buildFocused review should verify: