From 72596b0d056001a01e605255bfff82aabd7178ca Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Wed, 26 Aug 2026 17:55:43 +0000 Subject: [PATCH] fix(ingestion): stop decorated static members and accessors minting routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@Controller('v') export class C { @Get('s') static s() {} }` produced `GET /v/s`. Nest's `RequestMapping` writes to the prototype's `descriptor.value`, so a static method, a getter and a setter are never registered as request handlers — every route minted from one is a URL the application does not serve. Same wrong-fact class as the object-literal defect in 60021f9ab, and on base all four spellings were indistinguishable: @Get('s') static s() {} -> GET s @Get('s') get s(): string { return ''; } -> GET s @Get('s') set s(v: string) {} -> GET s @Get('s') s() {} -> GET s (the legitimate one) The modifiers are read off `member.children`, not `member.namedChildren`. They are anonymous tokens: a static method, a getter, a setter and a plain method all expose byte-identical `namedChildren`, so the idiom used everywhere else in this module cannot see them at all. Every child's type is tested rather than a fixed position, because tree-sitter-javascript puts a method's decorator at `children[0]` and positional indexing would read that instead. No second `pending.length = 0` was added. The existing unconditional clear at the end of the member loop already stops a non-handler donating its decorator run to the next method, and making the non-handler branch `continue` ahead of that clear would introduce exactly the donation bug it prevents. The donation test is honest about what it pins: it is green on base, so it is not evidence the modifier check works. It goes red only under the mutation that moves a `continue` ahead of the pending clear, and it asserts over the routes attributed to the following handler rather than the whole output, so it measures the clear rather than re-measuring the modifier check. --- .../core/ingestion/route-extractors/nest.ts | 54 +++++++++++- .../test/unit/nest-decorator-routes.test.ts | 82 +++++++++++++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) diff --git a/gitnexus/src/core/ingestion/route-extractors/nest.ts b/gitnexus/src/core/ingestion/route-extractors/nest.ts index 1f052bb25..e1b90124f 100644 --- a/gitnexus/src/core/ingestion/route-extractors/nest.ts +++ b/gitnexus/src/core/ingestion/route-extractors/nest.ts @@ -393,6 +393,48 @@ export function extractNestRoutes( return out; } +/** + * Modifiers that take a `method_definition` out of Nest's handler set. + * + * Nest's `RequestMapping` writes the handler onto the class PROTOTYPE's + * `descriptor.value`, and `RouterExplorer` scans prototype instance methods for + * that metadata. A `static` method lives on the constructor and is never + * scanned; an accessor's descriptor carries `get`/`set` and no `value` to + * register. A verb decorator on any of the three therefore mounts NOTHING, so a + * route minted from one is a URL the app does not serve — the invented fact + * this module refuses everywhere else. + */ +const NON_HANDLER_MODIFIERS: ReadonlySet = new Set(['static', 'get', 'set']); + +/** + * Whether Nest could register this `method_definition` as a request handler. + * + * Reads `children`, NOT `namedChildren`, and that is the whole difficulty: + * `static`, `get` and `set` are ANONYMOUS tokens in all three grammars this + * extractor runs under, so they never appear among named children. A static + * method, a getter, a setter and a plain method expose the IDENTICAL + * `namedChildren` (`property_identifier`, `formal_parameters`, + * `statement_block`) — probed, not assumed — so the module's usual + * `namedChildren` idiom cannot see the modifier at all and every one of the + * three reads as an ordinary handler. + * + * Tests every child's TYPE rather than indexing a position, because the + * position is not stable: under tree-sitter-javascript a method decorator is a + * CHILD of `method_definition`, so `children[0]` there is the `decorator` and + * the modifier sits behind it, while under tree-sitter-typescript the decorator + * is a preceding sibling and the modifier comes first. + * + * The scan is bounded by one method's own children (a handful), so it is not + * the uncached-getter trap {@link collectClassRoutes} documents — that one bites + * when a PARENT's child list is re-marshalled once per member. + */ +function isRequestHandler(member: Parser.SyntaxNode): boolean { + for (const child of member.children) { + if (NON_HANDLER_MODIFIERS.has(child.type)) return false; + } + return true; +} + function collectClassRoutes( classNode: Parser.SyntaxNode, prefix: string, @@ -435,8 +477,18 @@ function collectClassRoutes( // every `.js` Nest controller emitted nothing. On TypeScript the first // named child is the method name, so `leadingDecorators` contributes // nothing there and no route is collected twice. + // + // A static member, a getter and a setter are decorated exactly like a + // handler and registered as none, so they contribute no decorators (see + // `isRequestHandler`). They still fall THROUGH to the `pending.length = 0` + // below rather than `continue` past it: skipping the clear would hand their + // decorator run to the next method, trading a phantom route for a + // misattributed one — the strictly worse of the two, since it corrupts a + // route that is otherwise correct. const decorators = - member.type === 'method_definition' ? [...pending, ...leadingDecorators(member)] : []; + member.type === 'method_definition' && isRequestHandler(member) + ? [...pending, ...leadingDecorators(member)] + : []; for (const decorator of decorators) { const name = decoratorName(decorator); if (name === null) continue; diff --git a/gitnexus/test/unit/nest-decorator-routes.test.ts b/gitnexus/test/unit/nest-decorator-routes.test.ts index d471981d8..50aaa890d 100644 --- a/gitnexus/test/unit/nest-decorator-routes.test.ts +++ b/gitnexus/test/unit/nest-decorator-routes.test.ts @@ -599,4 +599,86 @@ describe('NestJS decorator routes', () => { `), ).toEqual(['GET /di/a']); }); + + // ─── Members Nest never registers as handlers ────────────────────── + + // Nest's `RequestMapping` writes the handler onto the class PROTOTYPE's + // `descriptor.value`, and `RouterExplorer` scans prototype instance methods + // for that metadata. A `static` method lives on the constructor and is never + // scanned; an accessor's descriptor carries `get`/`set` and no `value` to + // register. A verb decorator on any of the three mounts NOTHING, so a route + // minted from one is a URL the app does not serve — the same + // wrong-answer-dressed-as-fact the object-form refusals above exist for. + it.each([ + { label: 'a static method', member: "@Get('s') static s() {}" }, + { label: 'a getter', member: "@Get('s') get s(): string { return ''; }" }, + { label: 'a setter', member: "@Get('s') set s(v: string) {}" }, + ])('mints nothing for $label, which Nest never registers as a handler', ({ member }) => { + expect( + extract(` + @Controller('v') + export class C { + ${member} + } + `), + ).toEqual([]); + }); + + it('still emits the route for that same decorator on an instance method', () => { + // The control for the table above — identical source minus the modifier. + // It is what makes those three empty results evidence of the modifier + // check rather than of a fixture that happens to parse to nothing. + expect( + urls(` + @Controller('v') + export class C { + @Get('s') s() {} + } + `), + ).toEqual(['GET /v/s']); + }); + + it('drops a decorated static method under the JavaScript grammar too', () => { + // tree-sitter-javascript makes a method decorator a CHILD of + // `method_definition`, so `children` reads `decorator | static | + // property_identifier | …` and the modifier is NOT at a fixed index — the + // check has to test every child's type. The instance method beside it is + // the in-fixture control: its route proves this .js arm still measures + // something rather than passing on a fixture that parses to nothing. + expect( + jsUrls(` + @Controller('v') + export class C { + @Get('s') + static s() {} + + @Get('i') + i() {} + } + `), + ).toEqual(['GET /v/i']); + }); + + it("does not donate a non-handler member's decorator run to the method after it", () => { + // Pins the UNCONDITIONAL `pending.length = 0` at the end of the member + // loop, NOT the modifier check: a non-handler must fall through to that + // clear rather than `continue` past it. Deliberately green before the + // modifier check existed too — there the static member consumed the run + // into its own (wrong) route and then cleared it — so this goes red only + // if a future edit adds the early `continue`. Asserted over the routes + // attributed to `i`, because the whole-output form would instead be + // measuring the modifier check the table above already covers. + const routes = extract(` + @Controller('v') + export class C { + @Get('s') + static s() {} + + @Get('i') + i() {} + } + `); + + expect(format(routes.filter((route) => route.handlerName === 'i'))).toEqual(['GET /v/i']); + }); });