Skip to content

Commit e10cf34

Browse files
os-trumpclaude
andauthored
test(rest): state IHttpRequest once, in a typed request builder (#13466)
The four ledgered TS2345 in this package's test layer were hand-built request literals: IHttpRequest declares five required members and each literal supplied two or four of them. No annotation repairs them - adding the missing members changes what the handler RECEIVES, so inventing values silently changes what each test measures. src/http-request-test-builder.ts states the five once. It defaults only what has a neutral default (headers/params/query empty) and DERIVES the rest: method and path are read off the route under test, with path materialized from the request's own params, so the request and the route it is sent to cannot disagree. remoteAddress is deliberately left absent - its #4910 contract note makes it the unforgeable half of caller identification, and a plausible default would forge exactly the member whose worth is that it cannot be forged. Repairing all four did not empty the ledger: two IHttpResponse errors appeared at the same two call sites in rest.test.ts, on argument 2. That res expression is byte-identical to main - tsc reports at most one argument error per call, so the request literals were masking them. Filed separately; the ledger's authored _note now records the mechanism, since an EXACT per-file count measures a quantity and not an identity. Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8fcd081 commit e10cf34

5 files changed

Lines changed: 167 additions & 11 deletions

File tree

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The one place this package builds an `IHttpRequest` for a test (#13377).
5+
*
6+
* Test layer only — nothing in `src/index.ts` reaches it, so tsup (entry:
7+
* `src/index.ts`) never emits it into `dist` and it is not published. Same
8+
* placement, and for the same reason, as `src/xlsx-test-loader.ts`.
9+
*
10+
* ## The defect, stated once
11+
*
12+
* `IHttpRequest` (`packages/spec/src/contracts/http-server.ts`) declares FIVE
13+
* required members — `params`, `query`, `headers`, `method`, `path` — and a
14+
* route handler's first parameter IS that interface. So a test that hands a
15+
* handler an object literal owes all five. Before this file the package did
16+
* neither of the two honest things: 151 of its 281 `.handler(` call sites cast
17+
* the literal with `as any`, and four more sat in `test-typecheck-debt.json` as
18+
* ledgered `TS2345`:
19+
*
20+
* ```
21+
* src/rest.test.ts(2063,7): error TS2345: Argument of type
22+
* '{ params: { environmentId: string; object: string; }; query: {}; }'
23+
* is not assignable to parameter of type 'IHttpRequest'.
24+
* ```
25+
*
26+
* ⚠️ No annotation repairs those four. Adding the missing members changes what
27+
* the handler RECEIVES — the fixture's data, not its typing — so inventing
28+
* values silently changes what each test measures. That is why this is one
29+
* builder and not four edits, and it is why the defaults below are argued
30+
* rather than picked.
31+
*
32+
* ## Why each default is the default
33+
*
34+
* **`method` and `path` are NOT defaulted — they are read off the route under
35+
* test.** A constant default here is precisely the hazard #13377 names: a
36+
* `path` that does not match the route under test makes a passing test measure
37+
* something other than what it names, and `req.path` is live rather than
38+
* decorative — `RestServer.enforceAuth` feeds it to `isAuthGateAllowlisted`
39+
* (`src/rest-server.ts:1254`), where a wrong value decides whether the ADR-0069
40+
* gate fires at all. Every call site already locates its route
41+
* (`getRoutes().find(r => r.method === 'GET' && r.path === '…')`); handing that
42+
* same route object here means the request and the route it is sent to CANNOT
43+
* disagree, because one is derived from the other.
44+
*
45+
* **`path` is materialized from `params`, not copied from the pattern.** A
46+
* transport hands the handler a CONCRETE path; `/api/v1/data/:object` is a
47+
* pattern no request ever carries. So the builder substitutes the request's own
48+
* `params` into the pattern's `:segments` — which also means `path` and
49+
* `params` cannot drift apart, since one is computed from the other. A
50+
* `:segment` with no matching param is REFUSED loudly instead of emitted: a
51+
* path carrying a literal `:` is one no transport could produce, and quietly
52+
* shipping it is exactly the plausible-looking value this card exists to
53+
* prevent. A test that wants an unmatched path says so, by overriding `path`.
54+
*
55+
* **`headers: {}`** is the one required member with a genuinely neutral
56+
* default. It means "a request that carried no headers", which is what every
57+
* one of these fixtures intends, and every header read in this package is
58+
* either optional-chained or an index access — both yield `undefined` against
59+
* `{}`, identical to what the fixture meant by omitting it. It is also strictly
60+
* better-formed than what the ledgered literals produced: they left
61+
* `req.headers` `undefined`, so the unguarded `req.headers['if-none-match']`
62+
* (`src/rest-server.ts:5232`) would have THROWN had those tests reached it.
63+
*
64+
* **`params: {}` / `query: {}`** — empty is the honest reading of "the fixture
65+
* supplied none", and both are `Record`s every handler reads defensively.
66+
*
67+
* **`remoteAddress` is deliberately ABSENT, and has no default at all.** Its
68+
* contract note (#4910) is explicit that it is "the TRANSPORT's own peer
69+
* address … the unforgeable half of caller identification", which a client
70+
* cannot influence and which the inbound rate limiter keys anonymous traffic
71+
* off. A builder that supplied a plausible `'127.0.0.1'` would put a forged
72+
* value in the one member whose entire worth is that it cannot be forged. The
73+
* interface makes it optional because "not every runtime exposes it", so absent
74+
* is both legal and true; a test about it states it.
75+
*
76+
* **`body` / `rawBody`** are optional and stay absent unless stated: a GET has
77+
* no body, and the sites that have one pass it.
78+
*/
79+
80+
import type { RouteHandler } from '@objectstack/core';
81+
82+
/**
83+
* The request type a handler is actually handed, read off the handler's own
84+
* signature instead of spelled by hand — the discipline `xlsx-test-loader.ts`
85+
* applies to its dependency, applied here to ours. It resolves to
86+
* `IHttpRequest`; deriving it means this builder tracks whatever a handler's
87+
* first parameter becomes, and a change to that contract fails HERE, loudly, in
88+
* one file, instead of leaving a helper that still compiles and lies.
89+
*/
90+
type HandlerRequest = Parameters<RouteHandler>[0];
91+
92+
/**
93+
* The half of a mounted route this builder reads. Structurally what
94+
* `RestServer.getRoutes()` returns, so a `find(…)` result is passed straight
95+
* in — but declared narrowly on purpose: building a request must not be able to
96+
* reach the handler it is building the request FOR.
97+
*/
98+
export interface RouteUnderTest {
99+
/** HTTP verb as registered. */
100+
readonly method: string;
101+
/** Full wire path, with `:param` segments — the pattern, not a request. */
102+
readonly path: string;
103+
}
104+
105+
/**
106+
* Whatever this call site's assertion is actually about. Every member is the
107+
* contract's, so a site can state any of them — including `method` and `path`,
108+
* which override the route's and are then a deliberate, reviewable divergence
109+
* from the route the request is sent to.
110+
*/
111+
export type HttpRequestOverrides = Partial<HandlerRequest>;
112+
113+
/**
114+
* Build a complete `IHttpRequest` aimed at `route`, overriding only what this
115+
* test is about.
116+
*
117+
* @param route the mounted route the request is being sent to; supplies
118+
* `method`, and the pattern `path` is materialized from.
119+
* @param overrides the members this call site's assertion is about.
120+
* @throws if the route's pattern has a `:param` that `overrides.params` does
121+
* not supply and no explicit `path` was given.
122+
*/
123+
export function httpRequestForRoute(
124+
route: RouteUnderTest,
125+
overrides: HttpRequestOverrides = {},
126+
): HandlerRequest {
127+
const { params = {}, query = {}, headers = {}, method, path, ...optional } = overrides;
128+
return {
129+
...optional,
130+
params,
131+
query,
132+
headers,
133+
method: method ?? route.method,
134+
path: path ?? materializeRoutePath(route.path, params),
135+
};
136+
}
137+
138+
/**
139+
* Substitute `params` into a route pattern's `:segments`, yielding the concrete
140+
* path a transport would have produced for this request.
141+
*/
142+
function materializeRoutePath(pattern: string, params: Record<string, string>): string {
143+
return pattern
144+
.split('/')
145+
.map((segment) => {
146+
if (!segment.startsWith(':')) return segment;
147+
const name = segment.slice(1);
148+
if (!Object.prototype.hasOwnProperty.call(params, name)) {
149+
throw new Error(
150+
`httpRequestForRoute: route '${pattern}' has a ':${name}' segment but params supplied no '${name}'. ` +
151+
`A path holding a literal ':' is one no transport produces — supply params.${name}, or state ` +
152+
`the 'path' override deliberately if the mismatch is what the test is about.`,
153+
);
154+
}
155+
return params[name];
156+
})
157+
.join('/');
158+
}

packages/rest/src/meta-public-book-grant.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import { describe, it, expect, vi } from 'vitest';
1414
import { RestServer } from './rest-server';
15+
import { httpRequestForRoute } from './http-request-test-builder.js';
1516

1617
const PUBLIC_BOOK = { name: 'manual', label: 'Manual', audience: 'public', groups: [] };
1718
const ORG_BOOK = { name: 'internal', label: 'Internal', audience: 'org', groups: [] };
@@ -135,7 +136,7 @@ describe('the exemption does not widen past book/doc reads (#3963)', () => {
135136
const put = rest.getRoutes().find((r: any) => r.method === 'PUT' && r.path === ITEM);
136137
if (put) {
137138
const res = makeRes();
138-
await put.handler({ method: 'PUT', params: { type: 'book', name: 'manual' }, query: {}, body: {} }, res);
139+
await put.handler(httpRequestForRoute(put, { params: { type: 'book', name: 'manual' }, body: {} }), res);
139140
expect(res.statusCode).toBe(401);
140141
expect(protocol.saveMetaItem).not.toHaveBeenCalled();
141142
}

packages/rest/src/rest-batch-size-cap.test.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
import { describe, it, expect, vi } from 'vitest';
1313
import { RestServer } from './rest-server';
14+
import { httpRequestForRoute } from './http-request-test-builder.js';
1415

1516
function createMockServer() {
1617
return {
@@ -149,12 +150,9 @@ describe('cross-object batch reports the cap like everyone else (#3939)', () =>
149150
const { rest, ql } = setup(5);
150151
const route = rest.getRoutes().find((r: any) => r.method === 'POST' && r.path === '/api/v1/batch');
151152
const res = makeRes();
152-
await route!.handler({
153-
method: 'POST',
154-
params: {},
155-
query: {},
153+
await route!.handler(httpRequestForRoute(route!, {
156154
body: { operations: ids(6).map((id) => ({ object: 'invoice', action: 'delete', id })) },
157-
}, res);
155+
}), res);
158156

159157
expect(res.statusCode).toBe(400);
160158
expect(res.body).toMatchObject({ code: 'BATCH_TOO_LARGE', count: 6, max: 5 });

packages/rest/src/rest.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { RestServer, mapDataError } from './rest-server';
66
import { createRestApiPlugin } from './rest-api-plugin';
77
import type { RestApiPluginConfig } from './rest-api-plugin';
88
import { loadXlsxWorkbook } from './xlsx-test-loader.js';
9+
import { httpRequestForRoute } from './http-request-test-builder.js';
910

1011
// ---------------------------------------------------------------------------
1112
// Mocks & Helpers
@@ -2060,7 +2061,7 @@ describe('RestServer project-scoped routing', () => {
20602061

20612062
const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
20622063
await listRoute!.handler(
2063-
{ params: { environmentId: 'proj-123', object: 'task' }, query: {} },
2064+
httpRequestForRoute(listRoute!, { params: { environmentId: 'proj-123', object: 'task' } }),
20642065
res,
20652066
);
20662067

@@ -2085,7 +2086,7 @@ describe('RestServer project-scoped routing', () => {
20852086

20862087
const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
20872088
await unscoped!.handler(
2088-
{ params: { object: 'task' }, query: {} },
2089+
httpRequestForRoute(unscoped!, { params: { object: 'task' } }),
20892090
res,
20902091
);
20912092

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
{
22
"_comment": "Per-file tsc error debt of the @objectstack/rest TEST layer (#5286). `tsconfig.test.json` compiles `src/**/*.test.ts` — which `tsconfig.json` excludes and therefore no gate ever read — and every file below still carries errors from before that gate existed. THIS FIELD IS GENERATED: every regeneration rewrites it from scripts/check-test-typecheck.mts, and the EXACT ratchet below requires a regeneration on every repair — so an edit made here is gone by the next one. Anything true of THIS package goes in the sibling `_note` field, which is authored, is preserved verbatim, and is never written by the generator (#12624). This comment states NO cause for the errors, deliberately: the classes differ per package and per file, they move as the debt is paid down, and a cause written here is rewritten verbatim into every ledger by every regeneration — so it outlives its own repair and cannot be corrected in the file where it is read. Measure instead, before repairing anything: `tsc --noEmit --pretty false -p tsconfig.test.json` in the package prints the real classes with their TS codes. EXACT ratchet, judged by re-running tsc: a file that gains errors is red, a file that loses them is red until its number is re-recorded, a file that reaches zero is red until its entry is deleted, and a file NOT listed here may have no errors at all. Regenerate with: pnpm --filter @objectstack/rest gen:test-typecheck-debt",
3-
"_note": "Everything still recorded here is held by its own card, and none of it is an annotation repair. #13377 holds every entry that remains: the request literals this package builds by hand against IHttpRequest, which omit members that interface requires. Repairing them here would mean changing a fixture's data, so these entries shrink when that card lands and not before. The exceljs call that #13378 held is no longer in this ledger: that dependency declares its own module-local Buffer, which shadows Node's inside every exceljs signature, so no Node Buffer can be passed to Workbook.xlsx.load — the assertion that costs is now stated once, in src/xlsx-test-loader.ts, and every xlsx-reading test in this package goes through it.",
3+
"_note": "Everything still recorded here is held by its own card, and none of it is an annotation repair. #13454 holds the one entry that remains: src/rest.test.ts's two hand-built IHttpResponse literals, which omit the send and header members that interface requires and whose status is not typed as returning the interface, so res.status(...).json(...) does not chain. WARNING, and the reason this note exists: the count did NOT move when #13377 landed, but the errors underneath it were replaced wholesale. This file was recorded at 2 before that card and measures 2 after, and neither of the two is the same error. tsc reports at most ONE argument-assignability error per call expression, so the request literals #13377 removed had been masking these response literals at the very same two call sites. Read that as the warning an EXACT ledger cannot give you itself: a per-file count measures a QUANTITY, never an identity, and a constant number is not evidence that nothing changed. The request literals #13377 held are gone from this package: the five members IHttpRequest requires are stated once, in src/http-request-test-builder.ts, and a request built there takes its method and its path from the route under test instead of from a default, so the two cannot disagree. The exceljs call that #13378 held is likewise no longer here: that dependency declares its own module-local Buffer, which shadows Node's inside every exceljs signature, so no Node Buffer can be passed to Workbook.xlsx.load - the assertion that costs is stated once, in src/xlsx-test-loader.ts, and every xlsx-reading test in this package goes through it.",
44
"entries": {
5-
"src/meta-public-book-grant.test.ts": 1,
6-
"src/rest-batch-size-cap.test.ts": 1,
75
"src/rest.test.ts": 2
86
}
97
}

0 commit comments

Comments
 (0)