mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
fix(ingestion): stop decorated static members and accessors minting routes
`@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.
This commit is contained in:
parent
60021f9ab8
commit
72596b0d05
2 changed files with 135 additions and 1 deletions
|
|
@ -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<string> = 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;
|
||||
|
|
|
|||
|
|
@ -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']);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue