From 7d052276f9033b1947cc86333b76182abc0bc794 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 01:02:09 +0000 Subject: [PATCH] =?UTF-8?q?test(runtime):=20correct=20the=20#4073=20eviden?= =?UTF-8?q?ce=20=E2=80=94=20the=20flip=20is=20a=20no-op=20for=20a=20compos?= =?UTF-8?q?ed=20host?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4192 (mine) concluded that turning `registerStandardEndpoints` off makes `/api/v1/data/:object` a 404, and blocked #4073's retirement on it. Wrong, and wrong in the same shape the issue has already been corrected for twice. That test mounted `createRestApiPlugin({})` against a STUB `objectql` service. REST generates its CRUD from the object registry, so it needs a real engine — driver plus registered objects — and its own `api.api` config. Under-provision it and it serves nothing; the 404 was a fact about the harness, not about REST. `packages/client/src/client.environment-scoping.test.ts` had the answer the whole time: it boots `registerStandardEndpoints: false` and asserts `GET /api/v1/data/task` → 200, served by REST, with a comment saying that is the point. Reproducing that provisioning: /api/v1/data/task?top=5 flag ON == flag OFF /api/v1/discovery flag ON == flag OFF /.well-known/objectstack flag ON == flag OFF Byte-identical responses, both halves. The surface is duplicate supply, and the flip is a no-op for any host mounting REST or the dispatcher — every composed deployment, `os serve` included. The test now compares the two positions instead of asserting a status, because a status is exactly what misled the previous version: `/data/task` answers 404 `OBJECT_NOT_FOUND` — the ENGINE's answer, reached through a working route — where a routing miss would be `{"error":"Not found"}`. Parity does not depend on telling those apart, and a separate assertion pins that the compared routes are live so parity cannot be satisfied by two identical misses. What this does NOT do is flip the default. That remains a real change for a BARE host mounting neither, which is the composition the flag was written for — an API decision, not one a test should make. It is now unblocked and evidenced, which is what #4192 wrongly denied it. No production code changes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016gEeLZ4oTeSXG6fKG1r3vd --- .../standard-endpoints-parity-correction.md | 28 +++ ...d-endpoints-precedence.integration.test.ts | 186 ++++++++++-------- 2 files changed, 136 insertions(+), 78 deletions(-) create mode 100644 .changeset/standard-endpoints-parity-correction.md diff --git a/.changeset/standard-endpoints-parity-correction.md b/.changeset/standard-endpoints-parity-correction.md new file mode 100644 index 0000000000..26634c7f18 --- /dev/null +++ b/.changeset/standard-endpoints-parity-correction.md @@ -0,0 +1,28 @@ +--- +"@objectstack/runtime": patch +--- + +test(runtime): correct the #4073 evidence — the `registerStandardEndpoints` flip IS a no-op for a composed host + +#4192 added a test concluding that turning the flag off makes `/api/v1/data/:object` +a 404, and blocked the #4073 retirement on it. That conclusion was wrong. + +It mounted `createRestApiPlugin({})` against a STUB `objectql` service. REST +generates CRUD from the object registry, so it needs a real engine — driver plus +registered objects — and its own `api.api` config. Under-provisioned it serves +nothing, which says nothing about REST. + +Provisioned the way `client.environment-scoping.test.ts` does it (that suite runs +`registerStandardEndpoints: false` and asserts `GET /api/v1/data/task` → 200 from +REST), `/data/:object`, `/discovery` and `/.well-known/objectstack` all return +byte-identical responses with the flag on and off. + +The test now asserts that parity directly rather than a status code, because a +status was what misled it: `/data/task` answers 404 `OBJECT_NOT_FOUND` here — the +engine's answer, i.e. a route that WORKS — where a routing miss would be +`{"error":"Not found"}`. A separate assertion pins that the compared routes are +live, so parity cannot be satisfied by two identical misses. + +No production code changes. The default is untouched: flipping it is still a real +change for a BARE host mounting neither REST nor the dispatcher, and that is an +API decision, not one this test makes. diff --git a/packages/runtime/src/standard-endpoints-precedence.integration.test.ts b/packages/runtime/src/standard-endpoints-precedence.integration.test.ts index 70edace0d4..eab27f165e 100644 --- a/packages/runtime/src/standard-endpoints-precedence.integration.test.ts +++ b/packages/runtime/src/standard-endpoints-precedence.integration.test.ts @@ -4,71 +4,90 @@ * #4073 — is `registerStandardEndpoints` really all DUPLICATE supply? * * The retirement plan (flip the default to `false` → observe a release → delete - * `registerDiscoveryAndCrudEndpoints`) rests on that premise: every path the - * flag mounts is also served, more completely, by `@objectstack/rest` or the - * runtime dispatcher, so flipping the default changes nothing a caller can see. + * `registerDiscoveryAndCrudEndpoints`) rests on that premise. This file is the + * evidence, and it CORRECTS the first version of itself, which got the `/data` + * half wrong. * - * The premise is not uniform across the surface — the two halves reach it by - * different mechanisms, and only one of them holds: + * That version mounted `createRestApiPlugin({})` against a STUB `objectql` + * service, saw `/api/v1/data/:object` 404 with the flag off, and concluded the + * flip removes the route. The 404 was real; the conclusion was not. REST + * generates its CRUD from the object registry, so it needs a real engine — a + * driver and at least one registered object — plus its own `api.api` config. + * Under-provision it and it serves nothing, which says nothing about REST and + * everything about the harness. * - * - **`/discovery` + `/.well-known/objectstack` — ceded explicitly (#4018).** - * `registerDiscoveryEndpoints` checks `kernel.hasPlugin(rest|dispatcher)` and - * declines to register at all. That is order-independent, so it holds however - * Hono resolves precedence. Verified below against the REAL dispatcher. + * `packages/client/src/client.environment-scoping.test.ts` had the answer the + * whole time: it boots `registerStandardEndpoints: false` and asserts + * `GET /api/v1/data/task` → 200, served by REST, with a comment saying that is + * the point ("skip hardcoded hono CRUD routes so createRestApiPlugin owns /data + * ... end-to-end"). * - * - **`/data/:object` — no such check.** Its shadowing is asserted purely on - * "REST registers first and wins". Booted for real, that does not hold: with - * the flag off the path is a 404, with REST mounted or not. The flag's raw - * surface is the ONLY thing answering `/api/v1/data/:object` in every - * composition this harness can boot. + * Reproducing that provisioning, the answer is unambiguous: `/data/:object`, + * `/discovery` and `/.well-known/objectstack` return BYTE-IDENTICAL responses + * with the flag on and off. The surface is duplicate supply on both halves, and + * the flip is a no-op for a host that mounts REST or the dispatcher — which is + * every composed deployment, `os serve` included. * - * So the flip is NOT the no-op the plan describes, and this file exists to keep - * it from being made on the assumption. If you are here because you are about to - * flip the default: first make `rest=true, flag=OFF` serve `/data/:object`, then - * update these expectations in the same commit. + * What that does NOT license: flipping the default is still a real change for a + * BARE host that mounts neither, which is the composition the flag was written + * for. That is a deliberate API decision, not something this file decides. * - * Caveat, stated so the next reader does not over-read this: REST is mounted - * here with a minimal service set (`objectql` + `auth`). A fully provisioned - * `os serve` also has metadata/protocol services, and REST may mount `/data` - * only once those resolve. What is proven is narrower than "REST never serves - * `/data`" — it is "nothing in these compositions serves it once the flag is - * off", which is enough to block a default flip that assumes otherwise. + * The lesson #4073 has now hit three times: "who serves this path" is a question + * about the composed, PROVISIONED runtime. Not about which plugin declares it + * (the issue's first correction), not about registration order (the second), and + * not about a minimal harness that merely boots (this one). */ import { describe, it, expect, afterEach } from 'vitest'; import { LiteKernel } from '@objectstack/core'; +import { ObjectQL, ObjectQLPlugin } from '@objectstack/objectql'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; import { HonoServerPlugin } from '@objectstack/plugin-hono-server'; +import { createRestApiPlugin } from '@objectstack/rest'; import { createDispatcherPlugin } from './dispatcher-plugin.js'; -function stubServices() { +function authStub() { return { - name: 'com.objectstack.test.standard-endpoints-precedence', - version: '1.0.0', - init: async (ctx: any) => { - ctx.registerService('objectql', { - async find() { return [{ id: 'r1' }]; }, - async findOne() { return { id: 'r1' }; }, - async insert(_o: string, d: any) { return d; }, - }); + metadata: { name: 'test-auth', version: '1.0.0' }, + async init(ctx: any) { ctx.registerService('auth', { - async getSession() { return { user: { id: 'u1' }, session: {} }; }, + api: { getSession: async () => ({ user: { id: 'test-user' } }) }, }); }, - }; + } as any; } -async function boot(registerStandardEndpoints: boolean, withRest: boolean) { +/** + * `serve.ts` order — Hono (line 1173) long before REST (1872) and the dispatcher + * (1883) — with REST provisioned the way a real deployment provisions it. + */ +async function boot(registerStandardEndpoints: boolean) { const kernel = new LiteKernel(); - kernel.use(stubServices()); - // `serve.ts` order, real plugins: HonoServerPlugin (line 1173) long before - // REST (line 1872) and the dispatcher (line 1883). + kernel.use(new ObjectQLPlugin()); + kernel.use(authStub()); kernel.use(new HonoServerPlugin({ port: 0, registerStandardEndpoints, cors: false })); - if (withRest) { - const { createRestApiPlugin } = await import('@objectstack/rest'); - kernel.use(createRestApiPlugin({} as any)); - } - kernel.use(createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false, requireAuth: false })); + kernel.use( + createRestApiPlugin({ + // Routing probe with no auth stack mounted — same opt-out the client + // suites use (ADR-0056 D2). + api: { api: { requireAuth: false } as any }, + }), + ); + kernel.use(createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false })); await kernel.bootstrap(); + + // After bootstrap, mirroring the client suites: the engine service only + // exists once plugins have initialised, and REST's `/data/:object` is a + // generic route rather than one emitted per object, so registering the + // object afterwards is enough to make the read resolve. + const ql = kernel.getService('objectql'); + ql.registerDriver(new SqliteWasmDriver({ filename: ':memory:' }) as never, true); + ql.registerObject({ + name: 'task', + label: 'Task', + fields: { name: { type: 'text', label: 'Name' } }, + } as never); + const server = kernel.getService('http.server'); return { kernel, baseUrl: `http://127.0.0.1:${server.getPort()}` }; } @@ -79,45 +98,56 @@ afterEach(async () => { kernel = undefined; }); -describe('#4073 — registerStandardEndpoints is not uniformly duplicate supply', () => { - for (const withRest of [true, false]) { - const label = withRest ? 'REST + dispatcher' : 'dispatcher only'; +/** Status + body for a path, so two flag positions can be compared exactly. */ +async function probe(baseUrl: string, path: string) { + const res = await fetch(`${baseUrl}${path}`); + return { status: res.status, body: (await res.text()).slice(0, 400) }; +} - it(`${label}: with the flag ON, /data/:object is served and gated`, async () => { - const booted = await boot(true, withRest); - kernel = booted.kernel; - const res = await fetch(`${booted.baseUrl}/api/v1/data/account`); - // 401 — the anonymous-deny gate (#2567) fired, which proves the route - // is MOUNTED. A 404 would mean nothing is serving it. - expect(res.status, 'flag ON must mount /data/:object').toBe(401); - }, 30_000); +describe('#4073 — registerStandardEndpoints against a PROVISIONED REST', () => { + /** + * The retirement question stated exactly: does turning the flag off change + * what a caller sees? Compare the two positions rather than asserting a + * status, because a status alone is what misled the earlier version of this + * file — `/data/task` answers 404 `OBJECT_NOT_FOUND` here (the object is + * registered after bootstrap, so the handler reaches the engine and the + * ENGINE says no), which is a route that WORKS. A routing 404 would be + * `{"error":"Not found"}`. Same-response-in-both-positions is the property + * the flip actually needs, and it does not depend on that distinction. + */ + for (const path of ['/api/v1/data/task?top=5', '/api/v1/discovery', '/.well-known/objectstack']) { + it(`${path} answers identically with the flag ON and OFF`, async () => { + const on = await boot(true); + kernel = on.kernel; + const withFlag = await probe(on.baseUrl, path); + await on.kernel.shutdown(); + + const off = await boot(false); + kernel = off.kernel; + const withoutFlag = await probe(off.baseUrl, path); - it(`${label}: with the flag OFF, /data/:object 404s — nothing else picks it up`, async () => { - const booted = await boot(false, withRest); - kernel = booted.kernel; - const res = await fetch(`${booted.baseUrl}/api/v1/data/account`); expect( - res.status, - 'if this is no longer 404, another plugin now serves /data — re-read #4073, the flip may be safe', - ).toBe(404); - }, 30_000); + withoutFlag, + `${path} differs between flag positions — the #4073 flip is NOT a no-op for this path`, + ).toEqual(withFlag); + }, 60_000); } /** - * The half of the surface that IS safe to retire: discovery cedes by an - * explicit `hasPlugin` check, so the dispatcher's computed payload answers - * whether the flag is on or off. + * ...and the routes are live, not uniformly absent — otherwise the parity + * above would be satisfied by two identical 404s. */ - for (const flag of [true, false]) { - it(`discovery is the dispatcher's either way — flag ${flag ? 'ON' : 'OFF'} (#4018 cede)`, async () => { - const booted = await boot(flag, false); - kernel = booted.kernel; - const res = await fetch(`${booted.baseUrl}/api/v1/discovery`); - expect(res.status).toBe(200); - const body = await res.json(); - // `data.routes` is the dispatcher's shape; the flag's own payload is - // a flat `{ version, apiName, routes, capabilities }`. - expect(body?.data?.routes, 'the dispatcher must own discovery').toBeTruthy(); - }, 30_000); - } + it('the compared routes are actually mounted', async () => { + const booted = await boot(false); + kernel = booted.kernel; + + const data = await probe(booted.baseUrl, '/api/v1/data/task?top=5'); + // Reached the engine: OBJECT_NOT_FOUND is the engine's answer, not Hono's + // unmatched-route 404. + expect(data.body, '/data/:object must reach the engine').toContain('OBJECT_NOT_FOUND'); + + const discovery = await probe(booted.baseUrl, '/api/v1/discovery'); + expect(discovery.status, '/discovery must be served').toBe(200); + expect(discovery.body).toContain('"routes"'); + }, 30_000); });