fix(service-datasource): the datasource-admin 503 names the service the route actually needs (#4225)

.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.

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)

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<string, unknown>) {
+ 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<Response> }> = [
+ { 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 });

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,