mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-09 03:17:54 +00:00
fix(ingestion): address review findings on the NestJS route extractor
Nine findings from the two-engine review digest, plus one the review missed.
Wrong-URL class (the only one that produced a fact rather than an absence):
- Escape sequences were DELETED, not decoded. tree-sitter splits a literal
around every `escape_sequence`, and `decoratorLiteralArg` kept only the
`string_fragment` children and joined them, so `@Get(':id(\\d+)')` — the
ordinary spelling of a Nest regex param, value `:id(\d+)` — was stored as
`:id(d+)`, and `@Get('/views')` as `/vews`. `api_impact` matches with
`n.name CONTAINS $route`, so a query using the real URL found nothing and
`route_map` printed a path the app never mounts — exactly what this module's
own header forbids. Fixed by delegating to `plainString`, the judge the
data-route-table extractor already uses (now exported), so the two agree on
what a readable literal is. It also subsumes the old non-literal fallback:
`plainString` already answers null for an array, identifier, member
expression, or call.
Silent whole-controller drops:
- `export abstract class` parses as `abstract_class_declaration`, a different
node type, so a decorated abstract base — the normal way to share CRUD
routes in Nest — produced zero routes.
- `@Controller({ path: 'cats', version: '1' })`, the documented form for URI
versioning, dropped every route on the class. The `path` value is a plain
literal sitting in the AST; this was an unimplemented shape, not a
constant-resolution problem. Quoted keys (`{ 'path': … }`) read too — dropping
a whole controller over a pair of quotes is the same asymmetry. A computed key
or a computed path still drops.
- The array form is deliberately still dropped: it forces
`decoratorLiteralArg` to `string[] | null` and a prefix-array x path-array
cross-product through the emit loop. Nest and Spring (#2281) disagree here.
Coverage the wiring already claimed:
- On the JavaScript provider the extractor could never emit a route.
`extractJsTsRoutes` is registered on `javascriptProvider` (.js/.jsx/.mjs/.cjs)
and those parse with tree-sitter-javascript, where a METHOD decorator is a
CHILD of `method_definition`, not a preceding sibling as in
tree-sitter-typescript. The class-level `@Controller` was still found, so the
walk early-returned and every `.js` controller emitted nothing — while
`javascriptProvider.astFrameworkPatterns` already declared `framework:
'nestjs'`. Now reads both positions; on TypeScript the leading position is
empty, so nothing is collected twice.
- `@Sse` mounts a real GET endpoint and was missing from the verb map. `@Search`
stays out on purpose: `normalizeRouteMethod` rejects SEARCH as non-standard
and would key the route by URL alone.
Performance:
- `collectClassRoutes` was O(n^2) in methods-per-controller. `namedChildren` is
an uncached getter in node-tree-sitter, so calling `precedingDecorators` once
per method re-marshalled the whole class body each time. One forward pass over
the array already read at the top of the function: 800 methods 360ms -> 7.4ms,
and per-method cost goes flat (450us -> 9us). `precedingDecorators` stays for
the class-level lookup, which runs once.
Found while fixing, not in the review:
- A decorator name is an arbitrary identifier read out of source, and the verb
table was an object literal, so `@toString()` or `@constructor()` on a method
inside a `@Controller` resolved to the inherited `Object.prototype` member.
Truthy, so the guard passed and the function was emitted verbatim as the
route's httpMethod. Latent rather than live — no real Nest decorator collides
— but a `ReadonlyMap` costs nothing.
Types, tests and docs:
- `decoratorLiteralArg` narrowed to `string | null`; all four returns were
already `''`, a string, or null, which made the `undefined` arm at the call
site dead. `controllerPrefix`'s `undefined` is real and stays.
- The precision-guard test for verb decorators outside a `@Controller` never
reached the code it named — its fixture had no `@Controller` at all, so it
short-circuited at the file-level substring gate. It now carries an unrelated
controller and tests the per-class check.
- New `test/integration/nest-route-pipeline.test.ts` + `test/fixtures/
nest-route-app/`. Every sibling extractor at this tier had a pipeline test and
a fixture app; this one had neither, and the unit suite cannot reach the tier
#3009 is actually about. It asserts the prefix/path join, `claim()` resolving
each handler, one Route node per verb on a shared URL, `prefix: null` reaching
the join as no prefix, and the `HANDLES_ROUTE` provenance — plus non-vacuity
and ingested-but-not-routed gates so the absence assertions cannot pass on an
empty graph. Note these tests load `dist/`, so `node scripts/build.js` must
run before they mean anything.
- Two behaviours are documented rather than fixed. URLs are controller-relative:
`setGlobalPrefix` and URI versioning live in the bootstrap file, and the
"drop rather than guess" floor is unavailable because honouring it would mean
dropping every Nest route in every repo (`spring.ts` has the same hole for
`server.servlet.context-path`). And the comment-skip in the decorator walk,
which is load-bearing for a JSDoc block between a decorator stack and its
method, also absorbs a decorator orphaned by a commented-out handler onto the
next method. At the AST level the two readings are indistinguishable, and
losing every documented route is the worse trade.
This commit is contained in:
parent
44e97798be
commit
d030dae5f8
9 changed files with 652 additions and 30 deletions
|
|
@ -174,7 +174,7 @@ converging on the routes phase's `(method, url)` registry:
|
|||
| Filesystem convention | path → URL, no parsing | Next.js `app/`, Expo, PHP |
|
||||
| Single-file framework route | `isRouteFile` + worker extraction | Laravel `routes/*.php` |
|
||||
| Cross-file framework route | `discoverRootRouteFiles` + `extractRoutes` | Django `urlpatterns` |
|
||||
| AST-level route in a normal file | `extractDecoratorRoutes` | Spring, FastAPI, NestJS (`@Controller` + `@Get`/`@Post`/…), **JS/TS dispatch guards and static data route tables** |
|
||||
| AST-level route in a normal file | `extractDecoratorRoutes` | Spring, FastAPI, NestJS (`@Controller` + `@Get`/`@Post`/…; URLs are controller-relative — `setGlobalPrefix` and URI versioning live in the bootstrap file and are not applied), **JS/TS dispatch guards and static data route tables** |
|
||||
|
||||
The last row is the one whose name undersells it. A route is DECLARED by a
|
||||
decorator, but it can also be **inferred** from a raw `node:http` server's own
|
||||
|
|
|
|||
|
|
@ -116,7 +116,14 @@ function decodeJavaScriptStringLiteral(raw: string): string | null {
|
|||
return decoded;
|
||||
}
|
||||
|
||||
function plainString(node: SyntaxNode): string | null {
|
||||
/**
|
||||
* A `string`/`template_string` node's decoded value, or `null` when it is not a
|
||||
* readable literal (an interpolated template, an unterminated escape). Shared
|
||||
* with the NestJS extractor so both agree on what a readable literal is —
|
||||
* notably that escapes must be DECODED, not dropped, because tree-sitter splits
|
||||
* a literal around every `escape_sequence`.
|
||||
*/
|
||||
export function plainString(node: SyntaxNode): string | null {
|
||||
if (node.type === 'string') return decodeJavaScriptStringLiteral(node.text);
|
||||
if (
|
||||
node.type === 'template_string' &&
|
||||
|
|
|
|||
|
|
@ -20,22 +20,55 @@
|
|||
* class. As there, the prefix travels on `ExtractedDecoratorRoute.prefix` and
|
||||
* the routes phase performs the join via `normalizeExtractedRoutePath`, so
|
||||
* NestJS routes are keyed identically to every other framework's.
|
||||
*
|
||||
* Known limitation: the URLs produced here are CONTROLLER-RELATIVE. A global
|
||||
* prefix (`app.setGlobalPrefix('api')`) and URI versioning are applied by the
|
||||
* bootstrap file, not by any decorator this file can see, so neither is
|
||||
* reflected — a route served at `/api/v1/venues/search` is stored as
|
||||
* `/venues/search`. The module's "drop rather than guess" floor is unavailable
|
||||
* for it: the evidence lives in a different file, so honouring it would mean
|
||||
* dropping every Nest route in every repo. `spring.ts` has the same hole for
|
||||
* `server.servlet.context-path`; `ExtractedDecoratorRoute.prefix` is the
|
||||
* channel a cross-file follow-up would use, the way FastAPI resolves its mount.
|
||||
*/
|
||||
|
||||
import type Parser from 'tree-sitter';
|
||||
import type { ExtractedDecoratorRoute } from '../workers/parse-worker.js';
|
||||
import { plainString } from './data-route-table.js';
|
||||
|
||||
/** NestJS method decorators → HTTP verb. */
|
||||
const NEST_METHOD_DECORATORS: Record<string, string> = {
|
||||
Get: 'GET',
|
||||
Post: 'POST',
|
||||
Put: 'PUT',
|
||||
Patch: 'PATCH',
|
||||
Delete: 'DELETE',
|
||||
Head: 'HEAD',
|
||||
Options: 'OPTIONS',
|
||||
All: '*',
|
||||
};
|
||||
/**
|
||||
* NestJS method decorators → HTTP verb. A Map rather than an object literal
|
||||
* because the lookup key is an arbitrary decorator name read out of source: a
|
||||
* plain object answers `@toString()` with `Object.prototype.toString`, which is
|
||||
* truthy and would be emitted verbatim as the route's httpMethod.
|
||||
*/
|
||||
const NEST_METHOD_DECORATORS: ReadonlyMap<string, string> = new Map([
|
||||
['Get', 'GET'],
|
||||
['Post', 'POST'],
|
||||
['Put', 'PUT'],
|
||||
['Patch', 'PATCH'],
|
||||
['Delete', 'DELETE'],
|
||||
['Head', 'HEAD'],
|
||||
['Options', 'OPTIONS'],
|
||||
['All', '*'],
|
||||
// `@Sse` mounts a real GET endpoint that streams; it is as much a route as
|
||||
// `@Get`. `@Search` is deliberately absent — `normalizeRouteMethod` rejects
|
||||
// SEARCH as non-standard and would key the route by URL alone, colliding
|
||||
// with every other verb on that path.
|
||||
['Sse', 'GET'],
|
||||
]);
|
||||
|
||||
/**
|
||||
* Class node types that can carry a `@Controller`. `export abstract class C`
|
||||
* parses as `abstract_class_declaration`, a DIFFERENT node type — and a
|
||||
* decorated abstract base sharing CRUD routes with its subclasses is ordinary
|
||||
* Nest, so matching `class_declaration` alone silently drops the whole
|
||||
* controller rather than one route.
|
||||
*/
|
||||
const CLASS_DECLARATION_TYPES: ReadonlySet<string> = new Set([
|
||||
'class_declaration',
|
||||
'abstract_class_declaration',
|
||||
]);
|
||||
|
||||
/**
|
||||
* Cheap parse-free gate. Every JS/TS file in every repo reaches this hook, so
|
||||
|
|
@ -76,24 +109,54 @@ function decoratorName(decorator: Parser.SyntaxNode): string | null {
|
|||
* (`@Controller(ROUTES.VENUES)`) whose value we cannot read must drop the route
|
||||
* rather than silently mount it at the wrong URL. `route_map` presents its
|
||||
* output as fact, and a wrong path is worse than a missing one.
|
||||
*
|
||||
* Reading the literal is delegated to `plainString`, the same judge the
|
||||
* data-route-table extractor uses, so both agree on what is readable. Filtering
|
||||
* `string_fragment` children and joining them looks equivalent and is not:
|
||||
* tree-sitter SPLITS a literal around each `escape_sequence`, and the join then
|
||||
* DELETES the escape rather than decoding it. `@Get(':id(\\d+)')` — the ordinary
|
||||
* spelling of a Nest regex param, whose value is `:id(\d+)` — came out as
|
||||
* `:id(d+)`, and `@Get('/v\u0069ews')` came out as `/vews`. Both are paths the
|
||||
* app never serves, i.e. the wrong-URL outcome the paragraph above forbids.
|
||||
*/
|
||||
function decoratorLiteralArg(decorator: Parser.SyntaxNode): string | null | undefined {
|
||||
/**
|
||||
* A property key's name, for the two spellings that carry one: `{ path: … }`
|
||||
* (a `property_identifier`) and `{ 'path': … }` (a `string`). Reading only the
|
||||
* first would drop the ENTIRE controller over a pair of quotes — the same
|
||||
* whole-class cost the object form is handled to avoid. A computed key
|
||||
* (`{ [KEY]: … }`) has no readable name and keeps the drop.
|
||||
*/
|
||||
function objectKeyName(pair: Parser.SyntaxNode): string | null {
|
||||
const key = pair.childForFieldName('key');
|
||||
if (!key) return null;
|
||||
return key.type === 'property_identifier' ? key.text : plainString(key);
|
||||
}
|
||||
|
||||
function decoratorLiteralArg(decorator: Parser.SyntaxNode): string | null {
|
||||
const call = decoratorCall(decorator);
|
||||
if (!call) return ''; // bare `@Get` with no call — no path of its own
|
||||
const args = call.childForFieldName('arguments');
|
||||
const first = args?.namedChild(0);
|
||||
if (!first) return ''; // `@Get()` — no path of its own
|
||||
|
||||
if (first.type === 'string' || first.type === 'template_string') {
|
||||
if (first.namedChildren.some((c) => c.type === 'template_substitution')) return null;
|
||||
const fragments = first.namedChildren.filter((c) => c.type === 'string_fragment');
|
||||
// An empty literal (`@Controller('')`) has no fragments and is a real ''.
|
||||
return fragments.map((f) => f.text).join('');
|
||||
// `@Controller({ path: 'cats', version: '1' })` is the documented form for
|
||||
// URI/header versioning, and its path is a plain literal sitting right there.
|
||||
// Worth reading rather than dropping, because the asymmetry is severe: an
|
||||
// unreadable METHOD path costs one route, an unreadable PREFIX costs every
|
||||
// route on the class.
|
||||
if (first.type === 'object') {
|
||||
const pathPair = first.namedChildren.find(
|
||||
(child) => child.type === 'pair' && objectKeyName(child) === 'path',
|
||||
);
|
||||
const value = pathPair?.childForFieldName('value') ?? null;
|
||||
// No `path` key, or a computed one, leaves the prefix unknowable.
|
||||
return value === null ? null : plainString(value);
|
||||
}
|
||||
|
||||
// An array form (`@Get(['a', 'b'])`), an identifier, a member expression, a
|
||||
// call — all unreadable here. Skip rather than guess.
|
||||
return null;
|
||||
// call — none of them is a readable literal, and `plainString` answers `null`
|
||||
// for every one of them. Skip rather than guess.
|
||||
return plainString(first);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -185,7 +248,7 @@ export function extractNestRoutes(
|
|||
const out: ExtractedDecoratorRoute[] = [];
|
||||
|
||||
const visit = (node: Parser.SyntaxNode): void => {
|
||||
if (node.type === 'class_declaration') {
|
||||
if (CLASS_DECLARATION_TYPES.has(node.type)) {
|
||||
const prefix = controllerPrefix(node);
|
||||
// `undefined` — not a controller at all. `null` — a controller whose
|
||||
// prefix could not be read, so its routes' URLs are unknowable.
|
||||
|
|
@ -211,17 +274,50 @@ function collectClassRoutes(
|
|||
const body = classNode.childForFieldName('body');
|
||||
if (!body) return;
|
||||
|
||||
for (const member of body.namedChildren) {
|
||||
if (member.type !== 'method_definition') continue;
|
||||
// ONE forward pass over the body, accumulating the decorator run and flushing
|
||||
// it at each method. Calling `precedingDecorators` per method instead is
|
||||
// quadratic in methods-per-controller for a reason that is invisible in the
|
||||
// source: `namedChildren` is an UNCACHED getter in node-tree-sitter, so every
|
||||
// call re-marshals the entire class body into fresh JS objects before the
|
||||
// `findIndex`. Measured here, 800 methods cost 362ms (450us/method, up from
|
||||
// 42us/method at 50); a single pass is flat. `spring.ts` never had this
|
||||
// because a Java annotation is a child of the declaration it annotates.
|
||||
const pending: Parser.SyntaxNode[] = [];
|
||||
|
||||
for (const decorator of precedingDecorators(member)) {
|
||||
for (const member of body.namedChildren) {
|
||||
if (member.type === 'decorator') {
|
||||
pending.push(member);
|
||||
continue;
|
||||
}
|
||||
// Same reason as in `precedingDecorators`: a JSDoc block between a
|
||||
// decorator stack and its method must not hide the route (a real
|
||||
// controller shape, pinned by the suite). Known limitation of that skip: a
|
||||
// decorator ORPHANED by a commented-out handler is then absorbed onto the
|
||||
// NEXT method, minting a phantom route with the wrong handler. There is no
|
||||
// AST fix — an orphan followed by a comment is indistinguishable from a
|
||||
// stack whose method happens to be documented — and losing every
|
||||
// documented route is the worse trade, so it is made deliberately.
|
||||
if (member.type === 'comment') continue;
|
||||
if (member.type !== 'method_definition') {
|
||||
pending.length = 0;
|
||||
continue;
|
||||
}
|
||||
|
||||
// tree-sitter-javascript makes a method decorator a CHILD of the
|
||||
// `method_definition`, not a preceding sibling as in tree-sitter-typescript
|
||||
// — and this extractor is registered on the JavaScript provider too, which
|
||||
// already advertises `framework: 'nestjs'`. Reading only siblings meant
|
||||
// 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.
|
||||
for (const decorator of [...pending, ...leadingDecorators(member)]) {
|
||||
const name = decoratorName(decorator);
|
||||
if (name === null) continue;
|
||||
const httpMethod = NEST_METHOD_DECORATORS[name];
|
||||
if (!httpMethod) continue;
|
||||
const httpMethod = NEST_METHOD_DECORATORS.get(name);
|
||||
if (httpMethod === undefined) continue;
|
||||
|
||||
const routePath = decoratorLiteralArg(decorator);
|
||||
if (routePath === null || routePath === undefined) continue; // unreadable → skip
|
||||
if (routePath === null) continue; // unreadable → skip
|
||||
|
||||
const handlerName = member.childForFieldName('name')?.text;
|
||||
|
||||
|
|
@ -241,5 +337,6 @@ function collectClassRoutes(
|
|||
...(handlerName === undefined ? {} : { handlerName }),
|
||||
});
|
||||
}
|
||||
pending.length = 0;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
14
gitnexus/test/fixtures/nest-route-app/src/health/health.controller.ts
vendored
Normal file
14
gitnexus/test/fixtures/nest-route-app/src/health/health.controller.ts
vendored
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
import { Controller, Get } from '@nestjs/common';
|
||||
|
||||
/**
|
||||
* A controller with no prefix of its own. `@Controller()` is legal NestJS and
|
||||
* means "mount at the root", so the method path must reach the graph bare —
|
||||
* this is the fixture half of the extractor's `prefix: '' -> null` mapping.
|
||||
*/
|
||||
@Controller()
|
||||
export class HealthController {
|
||||
@Get('health')
|
||||
health(): string {
|
||||
return 'ok';
|
||||
}
|
||||
}
|
||||
16
gitnexus/test/fixtures/nest-route-app/src/legacy/legacy.controller.ts
vendored
Normal file
16
gitnexus/test/fixtures/nest-route-app/src/legacy/legacy.controller.ts
vendored
Normal file
|
|
@ -0,0 +1,16 @@
|
|||
import { Controller, Get } from '@nestjs/common';
|
||||
|
||||
const ROUTE_PREFIXES = { legacy: 'legacy' };
|
||||
|
||||
/**
|
||||
* The prefix is not a literal, so the URLs of this controller's methods are
|
||||
* unknowable here. `route_map` presents its output as fact: a missing route is
|
||||
* recoverable, a route mounted at the wrong URL is not.
|
||||
*/
|
||||
@Controller(ROUTE_PREFIXES.legacy)
|
||||
export class LegacyController {
|
||||
@Get('reports')
|
||||
legacyReports(): string {
|
||||
return 'legacy';
|
||||
}
|
||||
}
|
||||
35
gitnexus/test/fixtures/nest-route-app/src/venues/venues.controller.ts
vendored
Normal file
35
gitnexus/test/fixtures/nest-route-app/src/venues/venues.controller.ts
vendored
Normal file
|
|
@ -0,0 +1,35 @@
|
|||
import { Body, Controller, Delete, Get, Param, Post, Query } from '@nestjs/common';
|
||||
import { VenuesService } from './venues.service';
|
||||
import type { Venue } from './venues.service';
|
||||
|
||||
/**
|
||||
* The shape #3009 is about: the URL is split across two decorators. The class
|
||||
* decorator carries the prefix and the method decorator carries the verb plus
|
||||
* the remainder, so neither half is a route on its own.
|
||||
*/
|
||||
@Controller('venues')
|
||||
export class VenuesController {
|
||||
constructor(private readonly venues: VenuesService) {}
|
||||
|
||||
// Pathless index route: its URL is the controller prefix and nothing else.
|
||||
@Get()
|
||||
findAll(): Venue[] {
|
||||
return this.venues.listVenues();
|
||||
}
|
||||
|
||||
@Get('search')
|
||||
search(@Query('q') term: string): Venue[] {
|
||||
return this.venues.searchVenues(term);
|
||||
}
|
||||
|
||||
// Same URL as findAll(), different verb — two Route nodes, not one.
|
||||
@Post()
|
||||
create(@Body() input: Venue): Venue {
|
||||
return this.venues.insertVenue(input);
|
||||
}
|
||||
|
||||
@Delete(':id')
|
||||
remove(@Param('id') id: string): void {
|
||||
this.venues.deleteVenue(id);
|
||||
}
|
||||
}
|
||||
36
gitnexus/test/fixtures/nest-route-app/src/venues/venues.service.ts
vendored
Normal file
36
gitnexus/test/fixtures/nest-route-app/src/venues/venues.service.ts
vendored
Normal file
|
|
@ -0,0 +1,36 @@
|
|||
import { Injectable } from '@nestjs/common';
|
||||
|
||||
export interface Venue {
|
||||
id: string;
|
||||
name: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Not a controller. It exists so each handler body calls a real symbol, and so
|
||||
* the `@Controller` file gate is shown skipping a decorated class that declares
|
||||
* no routes.
|
||||
*/
|
||||
@Injectable()
|
||||
export class VenuesService {
|
||||
private readonly rows: Venue[] = [];
|
||||
|
||||
listVenues(): Venue[] {
|
||||
return this.rows;
|
||||
}
|
||||
|
||||
searchVenues(term: string): Venue[] {
|
||||
return this.rows.filter((row) => row.name.includes(term));
|
||||
}
|
||||
|
||||
insertVenue(input: Venue): Venue {
|
||||
this.rows.push(input);
|
||||
return input;
|
||||
}
|
||||
|
||||
deleteVenue(id: string): void {
|
||||
this.rows.splice(
|
||||
this.rows.findIndex((row) => row.id === id),
|
||||
1,
|
||||
);
|
||||
}
|
||||
}
|
||||
225
gitnexus/test/integration/nest-route-pipeline.test.ts
Normal file
225
gitnexus/test/integration/nest-route-pipeline.test.ts
Normal file
|
|
@ -0,0 +1,225 @@
|
|||
/**
|
||||
* End-to-end coverage of NestJS `@Controller` + `@Get`/`@Post`/`@Delete` route
|
||||
* ingestion (#3009).
|
||||
*
|
||||
* The unit suite (`test/unit/nest-decorator-routes.test.ts`) pins what the
|
||||
* extractor RETURNS. Nothing there proves the return value becomes anything:
|
||||
* the reported symptom was not a wrong `ExtractedDecoratorRoute`, it was
|
||||
* `api_impact` — whose documented job is to be run BEFORE modifying a route
|
||||
* handler — reporting every live endpoint as non-existent, because the graph
|
||||
* held no `Route` nodes at all. That is the tier this file covers: the routes
|
||||
* phase performing the prefix/path join, `claim()` in call-processor resolving
|
||||
* `handlerName` to a real symbol UID, and `prefix: null` surviving both.
|
||||
*
|
||||
* The fixture lives at `test/fixtures/nest-route-app/`, mirroring
|
||||
* `spring-route-app/` — Spring solves the identical two-decorator shape and
|
||||
* `nest.ts` is modelled on it.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeAll } from 'vitest';
|
||||
import path from 'node:path';
|
||||
import type { GraphNode, GraphRelationship } from 'gitnexus-shared';
|
||||
import { runPipelineFromRepo } from '../../src/core/ingestion/pipeline.js';
|
||||
import type { PipelineResult } from '../../src/types/pipeline.js';
|
||||
|
||||
const FIXTURE = path.resolve(__dirname, '..', 'fixtures', 'nest-route-app');
|
||||
|
||||
// Compared against POSIX-normalized graph paths (see `routes()`), so these stay
|
||||
// forward-slashed rather than going through `path.join` — the Windows shard
|
||||
// would otherwise compare backslashes against the graph's forward slashes.
|
||||
const CONTROLLER_FILE = 'src/venues/venues.controller.ts';
|
||||
const HEALTH_FILE = 'src/health/health.controller.ts';
|
||||
|
||||
interface RouteView {
|
||||
/** `${method} ${url}` — the `routeNodeKey` identity, as a sortable string. */
|
||||
readonly identity: string;
|
||||
/** The handler the route resolved to, or the literal `'undefined'`. */
|
||||
readonly handler: string;
|
||||
readonly handlerLabel: string;
|
||||
readonly handlerFile: string;
|
||||
}
|
||||
|
||||
describe('NestJS decorator route ingestion pipeline', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(FIXTURE, () => {}, {});
|
||||
}, 120_000);
|
||||
|
||||
const nodes = (): readonly GraphNode[] => {
|
||||
const out: GraphNode[] = [];
|
||||
result.graph.forEachNode((node) => void out.push(node));
|
||||
return out;
|
||||
};
|
||||
|
||||
const relationships = (): readonly GraphRelationship[] => {
|
||||
const out: GraphRelationship[] = [];
|
||||
result.graph.forEachRelationship((rel) => void out.push(rel));
|
||||
return out;
|
||||
};
|
||||
|
||||
const routeNodes = (): readonly GraphNode[] => nodes().filter((node) => node.label === 'Route');
|
||||
|
||||
/**
|
||||
* Unresolved fields are stringified rather than branched on, so a route that
|
||||
* lost its handler reads as the literal `'undefined'` in the diff instead of
|
||||
* quietly skipping an assertion.
|
||||
*/
|
||||
const routes = (): readonly RouteView[] =>
|
||||
routeNodes()
|
||||
.map((node) => {
|
||||
const handler = result.graph.getNode(String(node.properties.handlerSymbolId));
|
||||
return {
|
||||
identity: `${String(node.properties.method)} ${String(node.properties.name)}`,
|
||||
handler: String(handler?.properties.name),
|
||||
handlerLabel: String(handler?.label),
|
||||
handlerFile: String(handler?.properties.filePath).replaceAll('\\', '/'),
|
||||
};
|
||||
})
|
||||
.sort((a, b) => a.identity.localeCompare(b.identity));
|
||||
|
||||
const identities = (): readonly string[] => routes().map((route) => route.identity);
|
||||
|
||||
it('emits Route nodes at all — the reported symptom was zero', () => {
|
||||
// Its own case because every expectation below is satisfiable by an empty
|
||||
// graph in the ways that matter least: `not.toContain` passes vacuously on
|
||||
// an empty array, and a full-table `toEqual` against `[]` reports a missing
|
||||
// element rather than "the extractor never ran".
|
||||
expect(routeNodes().length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it('joins the @Controller prefix with each method path into the Route node URL', () => {
|
||||
// `/venues/search`, not `search` and not `/search`: the join is what the
|
||||
// extractor cannot do alone — it ships `prefix` and the routes phase folds
|
||||
// it in via `normalizeExtractedRoutePath`.
|
||||
expect(identities()).toEqual([
|
||||
'DELETE /venues/:id',
|
||||
'GET /health',
|
||||
'GET /venues',
|
||||
'GET /venues/search',
|
||||
'POST /venues',
|
||||
]);
|
||||
});
|
||||
|
||||
it('mounts a pathless @Get() at the controller prefix WITH its handler attached', () => {
|
||||
// The reason `nest.ts` emits '/' instead of '' for a pathless decorator:
|
||||
// `claim()` short-circuits on a falsy `routePath`, so '' would still create
|
||||
// this Route node and silently drop `handlerSymbolId`. The unit test can
|
||||
// only see `routePath === '/'` at the extractor boundary; the guard being
|
||||
// load-bearing is observable here and nowhere else.
|
||||
expect(routes().find((route) => route.identity === 'GET /venues')).toEqual({
|
||||
identity: 'GET /venues',
|
||||
handler: 'findAll',
|
||||
handlerLabel: 'Method',
|
||||
handlerFile: CONTROLLER_FILE,
|
||||
});
|
||||
});
|
||||
|
||||
it('resolves every route to the controller method that declared it', () => {
|
||||
// `claim()` is first-writer-wins per `(method, url)`; a mis-keyed route
|
||||
// would show up here as a handler donated to the wrong URL, not as an
|
||||
// absence.
|
||||
expect(routes()).toEqual([
|
||||
{
|
||||
identity: 'DELETE /venues/:id',
|
||||
handler: 'remove',
|
||||
handlerLabel: 'Method',
|
||||
handlerFile: CONTROLLER_FILE,
|
||||
},
|
||||
{
|
||||
identity: 'GET /health',
|
||||
handler: 'health',
|
||||
handlerLabel: 'Method',
|
||||
handlerFile: HEALTH_FILE,
|
||||
},
|
||||
{
|
||||
identity: 'GET /venues',
|
||||
handler: 'findAll',
|
||||
handlerLabel: 'Method',
|
||||
handlerFile: CONTROLLER_FILE,
|
||||
},
|
||||
{
|
||||
identity: 'GET /venues/search',
|
||||
handler: 'search',
|
||||
handlerLabel: 'Method',
|
||||
handlerFile: CONTROLLER_FILE,
|
||||
},
|
||||
{
|
||||
identity: 'POST /venues',
|
||||
handler: 'create',
|
||||
handlerLabel: 'Method',
|
||||
handlerFile: CONTROLLER_FILE,
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it('splits one URL into one Route node per verb', () => {
|
||||
// `@Get()` and `@Post()` on the same controller share a URL. `routeNodeKey`
|
||||
// makes them distinct identities; collapsing them would drop a live
|
||||
// endpoint and hand its handler to the survivor.
|
||||
expect(
|
||||
routes()
|
||||
.filter((route) => route.identity.endsWith(' /venues'))
|
||||
.map((route) => `${route.identity} -> ${route.handler}`),
|
||||
).toEqual(['GET /venues -> findAll', 'POST /venues -> create']);
|
||||
});
|
||||
|
||||
it('carries a prefix-less @Controller() through to a bare URL', () => {
|
||||
// `@Controller()` maps to `prefix: null`, and null must reach the join as
|
||||
// "no prefix" rather than as the string 'null' or a leading empty segment.
|
||||
expect(identities()).toContain('GET /health');
|
||||
expect(identities().filter((id) => id.includes('//'))).toEqual([]);
|
||||
});
|
||||
|
||||
it('records per-decorator provenance on the HANDLES_ROUTE edge', () => {
|
||||
// Nest routes are DECLARED by an annotation, so they take the generic
|
||||
// `decorator-<name>` source rather than a bespoke one — the same channel
|
||||
// Spring and FastAPI use, which is the point of modelling on `spring.ts`.
|
||||
const routeIds = new Set(routeNodes().map((node) => node.id));
|
||||
expect(
|
||||
[
|
||||
...new Set(
|
||||
relationships()
|
||||
.filter((rel) => rel.type === 'HANDLES_ROUTE' && routeIds.has(rel.targetId))
|
||||
.map((rel) => String(rel.reason)),
|
||||
),
|
||||
].sort(),
|
||||
).toEqual(['decorator-Delete', 'decorator-Get', 'decorator-Post']);
|
||||
});
|
||||
|
||||
describe('precision — what must NOT become a route', () => {
|
||||
// Absence assertions are satisfied just as well by a file that was never
|
||||
// read, so prove ingestion first; otherwise this whole block is decoration.
|
||||
it('ingested the unreadable-prefix controller', () => {
|
||||
expect(
|
||||
nodes()
|
||||
.filter((node) => String(node.properties.filePath ?? '').endsWith('legacy.controller.ts'))
|
||||
.map((node) => String(node.properties.name)),
|
||||
).toEqual(expect.arrayContaining(['LegacyController', 'legacyReports']));
|
||||
});
|
||||
|
||||
it('drops a controller whose prefix is not a literal', () => {
|
||||
// `@Controller(ROUTE_PREFIXES.legacy)` — the URL is unknowable, and
|
||||
// `route_map` presents its output as fact. Emitting `/reports` here would
|
||||
// be a wrong route, which is worse than a missing one.
|
||||
expect(identities().filter((id) => id.includes('reports'))).toEqual([]);
|
||||
expect(identities().filter((id) => id.includes('legacy'))).toEqual([]);
|
||||
});
|
||||
|
||||
it('ingested the decorated non-controller service', () => {
|
||||
expect(
|
||||
nodes()
|
||||
.filter((node) => String(node.properties.filePath ?? '').endsWith('venues.service.ts'))
|
||||
.map((node) => String(node.properties.name)),
|
||||
).toEqual(expect.arrayContaining(['VenuesService', 'listVenues', 'searchVenues']));
|
||||
});
|
||||
|
||||
it('does not mint routes from a class with no @Controller', () => {
|
||||
// Verb decorators are believed only inside a `@Controller` class; without
|
||||
// that gate any library sharing the names would mint phantom endpoints.
|
||||
expect(routes().filter((route) => route.handlerFile.endsWith('venues.service.ts'))).toEqual(
|
||||
[],
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -1,12 +1,18 @@
|
|||
import { describe, expect, it } from 'vitest';
|
||||
import Parser from 'tree-sitter';
|
||||
import TypeScript from 'tree-sitter-typescript';
|
||||
import JavaScript from 'tree-sitter-javascript';
|
||||
import { extractNestRoutes } from '../../src/core/ingestion/route-extractors/nest.js';
|
||||
import { normalizeExtractedRoutePath } from '../../src/core/ingestion/route-extractors/route-path.js';
|
||||
|
||||
const tsParser = new Parser();
|
||||
tsParser.setLanguage(TypeScript.typescript);
|
||||
|
||||
// The extractor is registered on the JavaScript provider too, and the two
|
||||
// grammars place a method decorator differently, so both must be exercised.
|
||||
const jsParser = new Parser();
|
||||
jsParser.setLanguage(JavaScript);
|
||||
|
||||
const extract = (source: string) =>
|
||||
extractNestRoutes(tsParser.parse(source), 'src/x.controller.ts');
|
||||
|
||||
|
|
@ -16,6 +22,12 @@ const urls = (source: string) =>
|
|||
(r) => `${r.httpMethod} ${normalizeExtractedRoutePath(r.routePath, r.prefix ?? null)}`,
|
||||
);
|
||||
|
||||
/** The same, parsed with tree-sitter-javascript instead. */
|
||||
const jsUrls = (source: string) =>
|
||||
extractNestRoutes(jsParser.parse(source), 'src/x.controller.js').map(
|
||||
(r) => `${r.httpMethod} ${normalizeExtractedRoutePath(r.routePath, r.prefix ?? null)}`,
|
||||
);
|
||||
|
||||
describe('NestJS decorator routes', () => {
|
||||
it('joins the controller prefix with each method path', () => {
|
||||
expect(
|
||||
|
|
@ -166,15 +178,24 @@ describe('NestJS decorator routes', () => {
|
|||
it('ignores verb-named decorators on a class that is not a @Controller', () => {
|
||||
// `Get`/`Post` are ordinary identifiers; without the @Controller
|
||||
// requirement any library reusing those names mints phantom endpoints.
|
||||
// The unrelated controller is what makes this test reach the per-class
|
||||
// check: without a `@Controller` anywhere the file short-circuits at the
|
||||
// parse-free substring gate and the assertion proves nothing.
|
||||
expect(
|
||||
extract(`
|
||||
urls(`
|
||||
@Controller('y')
|
||||
export class RealController {
|
||||
@Get('real')
|
||||
real() {}
|
||||
}
|
||||
|
||||
@Injectable()
|
||||
export class NotAController {
|
||||
@Get('looks-like-a-route')
|
||||
nope() {}
|
||||
}
|
||||
`),
|
||||
).toEqual([]);
|
||||
).toEqual(['GET /y/real']);
|
||||
});
|
||||
|
||||
it('drops a route whose path is a constant it cannot read', () => {
|
||||
|
|
@ -229,4 +250,175 @@ describe('NestJS decorator routes', () => {
|
|||
it('returns nothing for a file with no @Controller at all', () => {
|
||||
expect(extract(`export function get() { return 1; }`)).toEqual([]);
|
||||
});
|
||||
|
||||
// ─── Literal decoding ──────────────────────────────────────────────
|
||||
|
||||
it('decodes an escape in a path instead of deleting it', () => {
|
||||
// The source below spells the Nest regex param the way a controller does,
|
||||
// `@Get(':id(\\d+)')`, whose runtime value is `:id(\d+)`. tree-sitter SPLITS
|
||||
// that literal around the escape_sequence, so keeping only the
|
||||
// string_fragment children and joining them yielded `:id(d+)` — a URL the
|
||||
// app never serves, i.e. the wrong-path outcome this module calls worse
|
||||
// than a missing one.
|
||||
const [route] = extract(`
|
||||
@Controller('users')
|
||||
export class UserController {
|
||||
@Get(':id(\\\\d+)')
|
||||
one() {}
|
||||
}
|
||||
`);
|
||||
expect(route.routePath).toBe(':id(\\d+)');
|
||||
});
|
||||
|
||||
it('decodes a unicode escape rather than dropping its payload', () => {
|
||||
expect(
|
||||
urls(`
|
||||
@Controller('v')
|
||||
export class C {
|
||||
@Get('/v\\u0069ews')
|
||||
views() {}
|
||||
}
|
||||
`),
|
||||
).toEqual(['GET /v/views']);
|
||||
});
|
||||
|
||||
it("treats an empty @Controller('') as carrying no prefix", () => {
|
||||
const [route] = extract(`
|
||||
@Controller('')
|
||||
export class C {
|
||||
@Get('a') a() {}
|
||||
}
|
||||
`);
|
||||
expect(route.prefix).toBeNull();
|
||||
expect(normalizeExtractedRoutePath(route.routePath, route.prefix ?? null)).toBe('/a');
|
||||
});
|
||||
|
||||
// ─── Controller shapes ─────────────────────────────────────────────
|
||||
|
||||
it('extracts routes from an abstract controller base class', () => {
|
||||
// `export abstract class` parses as abstract_class_declaration, a separate
|
||||
// node type — and a decorated abstract base sharing CRUD routes with its
|
||||
// subclasses is ordinary Nest, so missing it drops the whole controller.
|
||||
expect(
|
||||
urls(`
|
||||
@Controller('base')
|
||||
export abstract class BaseController {
|
||||
@Get('a') a() {}
|
||||
}
|
||||
`),
|
||||
).toEqual(['GET /base/a']);
|
||||
});
|
||||
|
||||
it('reads the path out of the object form used for URI versioning', () => {
|
||||
expect(
|
||||
urls(`
|
||||
@Controller({ path: 'cats', version: '1' })
|
||||
export class CatsController {
|
||||
@Get('breeds') breeds() {}
|
||||
}
|
||||
`),
|
||||
).toEqual(['GET /cats/breeds']);
|
||||
});
|
||||
|
||||
it('reads a quoted path key, rather than dropping the class over the quotes', () => {
|
||||
expect(
|
||||
urls(`
|
||||
@Controller({ 'path': 'cats' })
|
||||
export class CatsController {
|
||||
@Get('breeds') breeds() {}
|
||||
}
|
||||
`),
|
||||
).toEqual(['GET /cats/breeds']);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ label: 'no path key', argument: "{ version: '1' }" },
|
||||
{ label: 'a computed path', argument: '{ path: BASE_PATH }' },
|
||||
{ label: 'a computed path key', argument: "{ [PATH_KEY]: 'cats' }" },
|
||||
])('still drops a controller whose object form has $label', ({ argument }) => {
|
||||
expect(
|
||||
extract(`
|
||||
@Controller(${argument})
|
||||
export class C {
|
||||
@Get('a') a() {}
|
||||
}
|
||||
`),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('extracts from a .js controller, where a decorator is a CHILD of the method', () => {
|
||||
// tree-sitter-javascript nests a method decorator inside method_definition
|
||||
// rather than placing it before as a sibling. The same extractor serves the
|
||||
// JavaScript provider, so reading siblings only meant every .js Nest
|
||||
// controller emitted nothing while the wiring claimed nestjs coverage.
|
||||
expect(
|
||||
jsUrls(`
|
||||
@Controller('venues')
|
||||
export class VenueController {
|
||||
@UseGuards(AuthGuard)
|
||||
@Get('search')
|
||||
search() {}
|
||||
}
|
||||
`),
|
||||
).toEqual(['GET /venues/search']);
|
||||
});
|
||||
|
||||
// ─── Verb coverage ─────────────────────────────────────────────────
|
||||
|
||||
it('treats @Sse as the GET endpoint it mounts', () => {
|
||||
expect(
|
||||
urls(`
|
||||
@Controller('events')
|
||||
export class EventsController {
|
||||
@Sse('stream') stream() {}
|
||||
}
|
||||
`),
|
||||
).toEqual(['GET /events/stream']);
|
||||
});
|
||||
|
||||
it('emits nothing for a decorator that mounts no endpoint', () => {
|
||||
// Paired with the @Sse case above: without it, an unsupported route
|
||||
// decorator and a non-route decorator are the same silent [].
|
||||
expect(
|
||||
extract(`
|
||||
@Controller('events')
|
||||
export class EventsController {
|
||||
@UseGuards(AuthGuard) guarded() {}
|
||||
}
|
||||
`),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it.each(['toString', 'constructor'])(
|
||||
'does not mint a route for a decorator named @%s',
|
||||
(name) => {
|
||||
// The verb table is looked up by decorator name, so a plain object would
|
||||
// answer `Object.prototype.toString` here — truthy, and emitted verbatim
|
||||
// as the route's httpMethod.
|
||||
expect(
|
||||
extract(`
|
||||
@Controller('x')
|
||||
export class C {
|
||||
@${name}() f() {}
|
||||
}
|
||||
`),
|
||||
).toEqual([]);
|
||||
},
|
||||
);
|
||||
|
||||
it("does not carry a decorated property's decorators onto the next method", () => {
|
||||
// The decorator run is accumulated in one forward pass over the class body;
|
||||
// a non-method member must reset it, the way the backward walk used to stop.
|
||||
expect(
|
||||
urls(`
|
||||
@Controller('di')
|
||||
export class C {
|
||||
@Inject(SERVICE)
|
||||
private readonly svc: Service;
|
||||
|
||||
@Get('a') a() {}
|
||||
}
|
||||
`),
|
||||
).toEqual(['GET /di/a']);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue