From 874a5b077b7c9f27a2f5aae2f3a42daa3c9abd7e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:53:32 +0000 Subject: [PATCH] fix(service-datasource): the datasource-admin 503 names the service the route actually needs (#4225) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `admin-routes.ts` registered nine service-backed routes behind one hard-coded 503: const unavailable = (res) => sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The datasource-admin service is not available.'); Six of those routes resolve `datasource-admin`, so the message was right. Three resolve `external-datasource` — GET /:name/remote-tables, POST /:name/test, POST /:name/object-draft — and answered with the same sentence. An operator whose federation service was unwired was told to go look at datasource-admin, which was running fine. The code was never the bug. SERVICE_UNAVAILABLE is correct for all nine: ADR-0112's ledger asks generic conditions to reuse the standard catalog rather than register a per-service 503 synonym, and this module documents that decision inline. Which service is down is carried by `message`, exactly as intended — the `message` was simply wrong on three routes. Rather than parameterise the 503 helper and leave the name typed out a second time at each call site, the lookup and the message now come from one argument. The two adminService() / externalService() resolvers collapse into a single resolve(res, service, method) that answers the 503 itself, naming whatever service it just failed to resolve: const svc = resolve(res, 'external-datasource', 'listRemoteTables'); if (!svc) return; Fixing the three messages needed only the parameter; taking the name from the lookup is what stops a tenth route reintroducing the mismatch. The per-route capability check is preserved — a host may wire a partial implementation, so "the service is registered" and "this route can use it" stay separate facts. Wire-visible change, on those three routes only: the 503 body's `error.message` now reads "The external-datasource service is not available." — the same string packages/rest/src/external-datasource-routes.ts already emits for its own surface. Status and `error.code` are unchanged on all nine. Each of the nine 503s is now pinned to the service it names, driven through the real HonoHttpServer against a context that resolves services PER NAME. The mock every existing test used answers the same object for every lookup, which is why nothing could see this: it cannot tell the two services apart. One case covers the operator's actual situation — datasource-admin wired and answering 200s, external-datasource absent — including POST /:name/test, where the wired admin service has a testConnection of its own and must not answer for the external route. The one test this replaces asserted the same body the table's first row does; it is re-pointed at the resolver's other arm, a registry that THROWS on an unregistered name, which nothing covered. Pre-existing: #3843 carried every code string over verbatim and #3973 changed no bytes on the wire. Closes #4225 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015K7vXJAoSwrwdfcJrE3mZ8 --- ...asource-admin-503-names-its-own-service.md | 57 +++++++++ .../services/service-datasource/README.md | 9 +- .../src/__tests__/admin-routes.test.ts | 94 +++++++++++++- .../__tests__/envelope.conformance.test.ts | 9 ++ .../service-datasource/src/admin-routes.ts | 118 ++++++++++++------ 5 files changed, 240 insertions(+), 47 deletions(-) create mode 100644 .changeset/datasource-admin-503-names-its-own-service.md diff --git a/.changeset/datasource-admin-503-names-its-own-service.md b/.changeset/datasource-admin-503-names-its-own-service.md new file mode 100644 index 0000000000..9888da4c67 --- /dev/null +++ b/.changeset/datasource-admin-503-names-its-own-service.md @@ -0,0 +1,57 @@ +--- +"@objectstack/service-datasource": patch +--- + +fix(service-datasource): the datasource-admin 503 names the service the route actually needs (#4225) + +`admin-routes.ts` registered nine service-backed routes behind one hard-coded 503: + +```ts +const unavailable = (res) => + sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The datasource-admin service is not available.'); +``` + +Six of those routes resolve `datasource-admin`, so the message was right. Three +resolve `external-datasource` — `GET /:name/remote-tables`, `POST /:name/test`, +`POST /:name/object-draft` — and answered with the same sentence. An operator +whose federation service was unwired was told to go look at `datasource-admin`, +which was running fine. + +The code was never the bug. `SERVICE_UNAVAILABLE` is correct for all nine: +ADR-0112's ledger asks generic conditions to reuse the standard catalog rather +than register a per-service 503 synonym, and this module documents that decision +inline. Which service is down is carried by `message`, exactly as intended — the +`message` was simply wrong on three routes. + +Rather than parameterise the 503 helper and leave the name typed out a second +time at each call site, the lookup and the message now come from one argument. +The two `adminService()` / `externalService()` resolvers collapse into a single +`resolve(res, service, method)` that answers the 503 itself, naming whatever +service it just failed to resolve: + +```ts +const svc = resolve(res, 'external-datasource', 'listRemoteTables'); +if (!svc) return; +``` + +Fixing the three messages needed only the parameter; taking the name from the +lookup is what stops a tenth route reintroducing the mismatch. The per-route +capability check is preserved — a host may wire a partial implementation, so +"the service is registered" and "this route can use it" stay separate facts. + +Wire-visible change, on those three routes only: the 503 body's `error.message` +now reads `The external-datasource service is not available.` — the same string +`packages/rest/src/external-datasource-routes.ts` already emits for its own +surface. Status and `error.code` are unchanged on all nine. + +Each of the nine 503s is now pinned to the service it names, driven through the +real `HonoHttpServer` against a context that resolves services **per name**. The +mock every existing test used answers the same object for every lookup, which is +why nothing could see this: it cannot tell the two services apart. One case +covers the operator's actual situation — `datasource-admin` wired and answering +200s, `external-datasource` absent — including `POST /:name/test`, where the +wired admin service has a `testConnection` of its own and must not answer for the +external route. + +Pre-existing: #3843 carried every code string over verbatim and #3973 changed no +bytes on the wire. diff --git a/packages/services/service-datasource/README.md b/packages/services/service-datasource/README.md index dcab541719..795258d79a 100644 --- a/packages/services/service-datasource/README.md +++ b/packages/services/service-datasource/README.md @@ -37,7 +37,10 @@ The runtime admin owns only the `origin: 'runtime'` lifecycle. Mounted under `/api/v1/datasources` by `registerDatasourceAdminRoutes` (lifecycle + introspection) and the federation routes by the external service. Every route -degrades gracefully (`503` / "unavailable") when its service isn't wired. +degrades gracefully (`503` / "unavailable") when its service isn't wired, and the +message names **that** service — one registrar, but two services behind it: the +three routes marked below dispatch to `external-datasource`, the rest to +`datasource-admin` (#4225). **Lifecycle & connection** - `GET /datasources` — list (code + runtime, with provenance/health) @@ -47,9 +50,9 @@ degrades gracefully (`503` / "unavailable") when its service isn't wired. - `PATCH /datasources/:name` — update a runtime datasource - `DELETE /datasources/:name` — remove a runtime datasource (blocked while objects are bound) - `POST /datasources/test` — probe an unsaved draft (inline body) -- `POST /datasources/:name/test` — probe a **saved** datasource by name (backs the `test_connection` action) +- `POST /datasources/:name/test` — probe a **saved** datasource by name (backs the `test_connection` action) — *`external-datasource`* -**Introspection / sync (read-only)** +**Introspection / sync (read-only)** — all on `external-datasource` - `GET /datasources/:name/remote-tables` — list remote tables - `POST /datasources/:name/object-draft` — generate an object definition draft for one table (no persistence) - federation import/validate/refresh routes under `/datasources/:name/external/*` (ADR-0015) diff --git a/packages/services/service-datasource/src/__tests__/admin-routes.test.ts b/packages/services/service-datasource/src/__tests__/admin-routes.test.ts index e223afb4ae..a3155896e3 100644 --- a/packages/services/service-datasource/src/__tests__/admin-routes.test.ts +++ b/packages/services/service-datasource/src/__tests__/admin-routes.test.ts @@ -26,6 +26,22 @@ function mount(svc: unknown) { return server.getRawApp(); } +/** + * Mount with a service PER NAME, unlike `mount` above, which answers the same + * object for every lookup. + * + * That difference is the point: this module dispatches to two services, and a + * context that cannot tell them apart cannot show which one a route resolved. + * It is what let #4225 sit here — the 503 named `datasource-admin` on all nine + * routes, three of which resolve `external-datasource`, and no test could see it. + */ +function mountServices(services: Record) { + const server = new HonoHttpServer(0); + const ctx = { getService: vi.fn((name: string) => services[name]) } as any; + registerDatasourceAdminRoutes(server, ctx, '/api/v1'); + return server.getRawApp(); +} + describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => { it('GET /api/v1/datasources returns the service listing', async () => { const listDatasources = vi.fn().mockResolvedValue([ @@ -136,10 +152,19 @@ describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => { expect(await res.json()).toEqual({ success: true, data: { datasource: { name: 'pg', origin: 'runtime' } } }); }); - it('degrades to 503 when the datasource-admin service is not wired', async () => { - const app = mount(undefined); + it('degrades to 503 when the service registry THROWS, not just when it answers undefined', async () => { + // The other arm of the resolver's try/catch. `getService` throwing on an + // unregistered name is the shape a real `PluginContext` has; every mock in + // this file returns `undefined` instead, so nothing else drives this branch. + const server = new HonoHttpServer(0); + const ctx = { + getService: vi.fn(() => { + throw new Error('service "datasource-admin" is not registered'); + }), + } as any; + registerDatasourceAdminRoutes(server, ctx, '/api/v1'); - const res = await app.fetch(json('/api/v1/datasources')); + const res = await server.getRawApp().fetch(json('/api/v1/datasources')); expect(res.status).toBe(503); expect(await res.json()).toEqual({ @@ -151,6 +176,69 @@ describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => { }); }); + /** + * #4225 — the 503 names the service the route ACTUALLY resolves. + * + * Every route below is listed with the service it dispatches to, so the table + * is the module's service map as well as its test: a new route that resolves + * one service and reports the other has to disagree with a row here. + */ + const UNAVAILABLE: Array<{ route: string; service: string; run: (app: any) => Promise }> = [ + { route: 'GET /datasources', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources')) }, + { route: 'GET /datasources/:name', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/pg')) }, + { route: 'POST /datasources/test', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/test', { method: 'POST', body: '{}' })) }, + { route: 'POST /datasources', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources', { method: 'POST', body: '{}' })) }, + { route: 'PATCH /datasources/:name', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/pg', { method: 'PATCH', body: '{}' })) }, + { route: 'DELETE /datasources/:name', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/pg', { method: 'DELETE' })) }, + { route: 'GET /datasources/:name/remote-tables', service: 'external-datasource', run: (a) => a.fetch(json('/api/v1/datasources/ext/remote-tables')) }, + { route: 'POST /datasources/:name/test', service: 'external-datasource', run: (a) => a.fetch(json('/api/v1/datasources/ext/test', { method: 'POST', body: '{}' })) }, + { route: 'POST /datasources/:name/object-draft', service: 'external-datasource', run: (a) => a.fetch(json('/api/v1/datasources/ext/object-draft', { method: 'POST', body: JSON.stringify({ table: 'customers' }) })) }, + ]; + + for (const c of UNAVAILABLE) { + it(`${c.route} degrades to 503 naming ${c.service} (#4225)`, async () => { + const res = await c.run(mountServices({})); + expect(res.status).toBe(503); + expect(await res.json()).toEqual({ + success: false, + error: { + code: 'SERVICE_UNAVAILABLE', + message: `The ${c.service} service is not available.`, + }, + }); + }); + } + + it('a wired datasource-admin does not answer for an unwired external-datasource (#4225)', async () => { + // The operator's actual situation: lifecycle works, federation does not. The + // old message sent them to read the logs of the service that was running. + const app = mountServices({ + 'datasource-admin': { + listDatasources: async () => [{ name: 'ext', origin: 'runtime' }], + getDatasource: async () => ({ name: 'ext', driver: 'sqlite' }), + testConnection: async () => ({ ok: true }), + }, + // 'external-datasource' deliberately absent. + }); + + expect((await app.fetch(json('/api/v1/datasources'))).status).toBe(200); + + const remote = await app.fetch(json('/api/v1/datasources/ext/remote-tables')); + expect(remote.status).toBe(503); + expect(((await remote.json()) as any).error.message).toBe( + 'The external-datasource service is not available.', + ); + + // `POST /:name/test` resolves `external-datasource` even though its unsaved-draft + // sibling `POST /test` resolves `datasource-admin` — the wired admin service + // above has a `testConnection`, and it must not answer for this route. + const saved = await app.fetch(json('/api/v1/datasources/ext/test', { method: 'POST', body: '{}' })); + expect(saved.status).toBe(503); + expect(((await saved.json()) as any).error.message).toBe( + 'The external-datasource service is not available.', + ); + }); + it('surfaces lifecycle errors as 400 with the service message', async () => { const createDatasource = vi.fn().mockRejectedValue(new Error('duplicate name')); const app = mount({ createDatasource }); diff --git a/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts b/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts index 7deacb041f..9b3beb8bef 100644 --- a/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts +++ b/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts @@ -172,6 +172,15 @@ describe('datasource-admin envelope (#3843) — error bodies', () => { code: 'SERVICE_UNAVAILABLE', run: () => drive(mount(undefined), '/api/v1/datasources'), }, + { + // The same 503, from the three routes served by the OTHER service (#4225). + // Which service is named is asserted in `admin-routes.test.ts`; what this + // row adds is that the branch emits the declared envelope, like its twin. + name: 'the external-datasource service is not wired', + status: 503, + code: 'SERVICE_UNAVAILABLE', + run: () => drive(mount(undefined), '/api/v1/datasources/ext/remote-tables'), + }, { name: 'a lifecycle failure carries the service message', status: 400, diff --git a/packages/services/service-datasource/src/admin-routes.ts b/packages/services/service-datasource/src/admin-routes.ts index 958de9b32c..c4fae85e5b 100644 --- a/packages/services/service-datasource/src/admin-routes.ts +++ b/packages/services/service-datasource/src/admin-routes.ts @@ -4,20 +4,41 @@ import type { PluginContext } from '@objectstack/core'; import type { IHttpServer } from '@objectstack/spec/contracts'; import { DRIVER_CATALOG } from './driver-catalog.js'; +/** + * The services these routes dispatch to. + * + * Two, not one — which is the whole reason the 503 below is parameterised. The + * module is *named* after `datasource-admin` and most of it is served by that + * service, but the three schema-introspection routes are served by + * `external-datasource`. + */ +type ServiceName = 'datasource-admin' | 'external-datasource'; + /** * Datasource lifecycle REST routes (ADR-0015 Addendum §3.5). * - * Mounted under `/api/v1/datasources` and served by the `datasource-admin` - * service. Every route degrades gracefully - * (`503 SERVICE_UNAVAILABLE`) when the service is not wired in, and - * lifecycle/validation failures surface as `400` with the service's message. + * Mounted under `/api/v1/datasources`. Every route degrades gracefully + * (`503 SERVICE_UNAVAILABLE`) when the service *it* needs is not wired in — + * naming that service rather than the one this module is named after (#4225) — + * and lifecycle/validation failures surface as `400` with the service's message. + * + * Served by `datasource-admin`: * * GET /datasources → listDatasources (provenance + health) + * GET /datasources/:name → getDatasource (credential-stripped) * POST /datasources/test → testConnection (no persistence) * POST /datasources → createDatasource (origin: 'runtime') * PATCH /datasources/:name → updateDatasource (runtime only) * DELETE /datasources/:name → removeDatasource (runtime only) * + * Served by `external-datasource`: + * + * GET /datasources/:name/remote-tables → listRemoteTables + * POST /datasources/:name/test → testConnection (a SAVED datasource) + * POST /datasources/:name/object-draft → generateObjectDraft + * + * `GET /datasources/drivers` is static metadata and needs neither service. + * * Request bodies carry the connection draft inline with an optional cleartext * `secret` field; the route splits `secret` out so it never reaches the draft * the service persists. @@ -33,22 +54,6 @@ export function registerDatasourceAdminRoutes( ): void { const root = `${basePath}/datasources`; - const adminService = (): any => { - try { - return ctx.getService('datasource-admin'); - } catch { - return undefined; - } - }; - - const externalService = (): any => { - try { - return ctx.getService('external-datasource'); - } catch { - return undefined; - } - }; - /** * Emit an error in the DECLARED envelope — `BaseResponseSchema` + * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. @@ -80,7 +85,8 @@ export function registerDatasourceAdminRoutes( * * Which service is unavailable is carried by `message`; the ledger explicitly * asks generic conditions to reuse the catalog instead of registering a - * per-service 503. + * per-service 503. That puts the whole burden of naming the service on one + * string — see `resolve` below for how it is kept honest. */ const sendError = (res: any, status: number, code: string, message: string) => res.status(status).json({ success: false, error: { code, message } }); @@ -97,8 +103,38 @@ export function registerDatasourceAdminRoutes( const sendOk = (res: any, data: unknown, status = 200) => res.status(status).json({ success: true, data }); - const unavailable = (res: any) => - sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The datasource-admin service is not available.'); + /** + * Resolve the service a route dispatches to — or answer + * `503 SERVICE_UNAVAILABLE` naming THAT service and return `undefined`, which + * the caller returns on. + * + * The name that performs the lookup is the same one that writes the message, + * so the two cannot disagree. They did until #4225: a single `unavailable` + * helper hard-coded `datasource-admin` while three routes — `GET + * /:name/remote-tables`, `POST /:name/test`, `POST /:name/object-draft` — + * resolve `external-datasource`. An operator whose federation service was + * unwired got told to go look at `datasource-admin`, which was running fine. + * Passing the name to the helper instead would have fixed those three; taking + * it from the lookup is what stops a tenth route reintroducing the mismatch. + * + * `method` is the one call the route goes on to make. It is checked here + * because "the service is registered" and "this route can use it" are not the + * same fact — a host may wire a partial implementation — and this preserves + * the per-route capability check the call sites did before. + */ + const resolve = (res: any, service: ServiceName, method: string): any => { + let svc: any; + try { + svc = ctx.getService(service); + } catch { + svc = undefined; + } + if (!svc?.[method]) { + sendError(res, 503, 'SERVICE_UNAVAILABLE', `The ${service} service is not available.`); + return undefined; + } + return svc; + }; const badRequest = (res: any, err: unknown) => sendError(res, 400, 'DATASOURCE_ADMIN_ERROR', err instanceof Error ? err.message : String(err)); @@ -118,8 +154,8 @@ export function registerDatasourceAdminRoutes( // List all datasources with provenance + health. server.get(root, async (_req: any, res: any) => { - const svc = adminService(); - if (!svc?.listDatasources) return unavailable(res); + const svc = resolve(res, 'datasource-admin', 'listDatasources'); + if (!svc) return; const datasources = await svc.listDatasources(); sendOk(res, { datasources }); }); @@ -137,8 +173,8 @@ export function registerDatasourceAdminRoutes( // definition draft for one table (introspect + type-map, no persistence — // the caller creates the object through the normal metadata channel). server.get(`${root}/:name/remote-tables`, async (req: any, res: any) => { - const svc = externalService(); - if (!svc?.listRemoteTables) return unavailable(res); + const svc = resolve(res, 'external-datasource', 'listRemoteTables'); + if (!svc) return; try { const tables = await svc.listRemoteTables(req.params.name); sendOk(res, { tables }); @@ -155,8 +191,8 @@ export function registerDatasourceAdminRoutes( // `config` is non-sensitive, plus a `hasSecret` flag). Registered after the // static `/drivers` route so that literal segment is never captured as a name. server.get(`${root}/:name`, async (req: any, res: any) => { - const svc = adminService(); - if (!svc?.getDatasource) return unavailable(res); + const svc = resolve(res, 'datasource-admin', 'getDatasource'); + if (!svc) return; try { const datasource = await svc.getDatasource(req.params.name); if (!datasource) return sendError(res, 404, 'RESOURCE_NOT_FOUND', `Datasource "${req.params.name}" does not exist.`); @@ -167,8 +203,8 @@ export function registerDatasourceAdminRoutes( }); server.post(`${root}/:name/test`, async (req: any, res: any) => { - const svc = externalService(); - if (!svc?.testConnection) return unavailable(res); + const svc = resolve(res, 'external-datasource', 'testConnection'); + if (!svc) return; try { const result = await svc.testConnection(req.params.name); sendOk(res, result); @@ -178,8 +214,8 @@ export function registerDatasourceAdminRoutes( }); server.post(`${root}/:name/object-draft`, async (req: any, res: any) => { - const svc = externalService(); - if (!svc?.generateObjectDraft) return unavailable(res); + const svc = resolve(res, 'external-datasource', 'generateObjectDraft'); + if (!svc) return; const { table, ...opts } = (req.body as Record) ?? {}; if (!table) return badRequest(res, new Error('Body field "table" is required.')); try { @@ -193,8 +229,8 @@ export function registerDatasourceAdminRoutes( // Probe a connection without persisting anything. Registered before the // `:name` routes so the literal `test` segment is never captured as a name. server.post(`${root}/test`, async (req: any, res: any) => { - const svc = adminService(); - if (!svc?.testConnection) return unavailable(res); + const svc = resolve(res, 'datasource-admin', 'testConnection'); + if (!svc) return; const { draft, secret } = splitSecret(req.body); try { const result = await svc.testConnection(draft, secret); @@ -206,8 +242,8 @@ export function registerDatasourceAdminRoutes( // Create a runtime datasource. server.post(root, async (req: any, res: any) => { - const svc = adminService(); - if (!svc?.createDatasource) return unavailable(res); + const svc = resolve(res, 'datasource-admin', 'createDatasource'); + if (!svc) return; const { draft, secret } = splitSecret(req.body); try { const datasource = await svc.createDatasource(draft, secret); @@ -219,8 +255,8 @@ export function registerDatasourceAdminRoutes( // Patch a runtime datasource. server.patch(`${root}/:name`, async (req: any, res: any) => { - const svc = adminService(); - if (!svc?.updateDatasource) return unavailable(res); + const svc = resolve(res, 'datasource-admin', 'updateDatasource'); + if (!svc) return; const { draft, secret } = splitSecret(req.body); try { const datasource = await svc.updateDatasource(req.params.name, draft, secret); @@ -232,8 +268,8 @@ export function registerDatasourceAdminRoutes( // Remove a runtime datasource. server.delete(`${root}/:name`, async (req: any, res: any) => { - const svc = adminService(); - if (!svc?.removeDatasource) return unavailable(res); + const svc = resolve(res, 'datasource-admin', 'removeDatasource'); + if (!svc) return; try { await svc.removeDatasource(req.params.name); res.status(204).end();