From 9dbb6d2cc93a1aaf0da3f8a4b4669ded4d43126f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 05:34:51 +0000 Subject: [PATCH] fix(service-datasource,rest): the last three uncovered datasource routes answer their registered refusal code (#4264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4249 (#4263) gave the rest surface's two introspection routes a failure contract; three sibling routes still had no catch around their service call, so a throw was swallowed by the adapter and surfaced as the pre-#3675 non-envelope 500 { error: 'No response from handler' } — no success flag, no error.message, the real cause lost. Each now answers 400 in the declared envelope, under the code registered (ADR-0112) for the service the route dispatches to: - GET /api/v1/datasources → DATASOURCE_ADMIN_ERROR, matching its eight siblings in admin-routes.ts - POST /datasources/:name/external/refresh-catalog and POST /datasources/:name/external/validate → EXTERNAL_DATASOURCE_ERROR, the code #4249 gave the two introspection routes above them The issue left INTERNAL_ERROR open as an alternative; the per-service codes win on consistency — every other catch in both modules, including pure reads, already answers 400 with the service-attributed code, and refreshCatalog's dominant throw class is the one #4249 already adjudicated as a 400 refusal on listRemoteTables. A 500 would fork the failure contract within a module — the drift #4249 removed. No new codes (both registered by #4263). The envelope-conformance suites and the REFUSALS pin table gain one row per route. Closes #4264 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VDgGWS97x6vuikjmgMswtk --- .../datasource-routes-catch-service-throws.md | 38 +++++++++++++++++++ ...al-datasource-envelope.conformance.test.ts | 23 +++++++++++ .../rest/src/external-datasource-routes.ts | 35 ++++++++++++----- .../src/__tests__/admin-routes.test.ts | 3 ++ .../__tests__/envelope.conformance.test.ts | 9 +++++ .../service-datasource/src/admin-routes.ts | 13 +++++-- 6 files changed, 109 insertions(+), 12 deletions(-) create mode 100644 .changeset/datasource-routes-catch-service-throws.md diff --git a/.changeset/datasource-routes-catch-service-throws.md b/.changeset/datasource-routes-catch-service-throws.md new file mode 100644 index 0000000000..8c1125e9c4 --- /dev/null +++ b/.changeset/datasource-routes-catch-service-throws.md @@ -0,0 +1,38 @@ +--- +"@objectstack/service-datasource": patch +"@objectstack/rest": patch +--- + +fix(service-datasource,rest): the last three uncovered datasource routes answer their registered refusal code (#4264) + +#4249 (fixed in #4263) gave the rest surface's two introspection routes a +failure contract; this closes the same gap on the three sibling routes it left +uncovered. Each had no `catch` around its service call, so a service throw was +swallowed by the adapter and surfaced as the pre-#3675 non-envelope +`500 { error: 'No response from handler' }` — no `success` flag, no +`error.message`, no code to switch on, real cause lost. + +Wire-visible changes — each route now answers `400` in the declared envelope, +under the refusal code registered (ADR-0112) for the service it dispatches to, +with the service's own message at `error.message`: + +- `GET /api/v1/datasources` (`listDatasources` throw) → + `400 DATASOURCE_ADMIN_ERROR` — matching its eight siblings in + `service-datasource/admin-routes.ts`, which already answer their catches this + way. +- `POST /api/v1/datasources/:name/external/refresh-catalog` (`refreshCatalog` + throw) and `POST /api/v1/datasources/:name/external/validate` (`validateAll` + throw) → `400 EXTERNAL_DATASOURCE_ERROR` — the same code #4249 gave the two + introspection routes one block above them. + +The issue left the code choice open (`INTERNAL_ERROR` was the alternative); +the registered per-service codes win on consistency: every other catch in both +modules — including pure reads — already answers 400 with the service-attributed +code, and `refreshCatalog`'s dominant throw class (unknown datasource, +unreachable remote, no such schema) is the one #4249 already adjudicated as a +400 refusal on `listRemoteTables`. A 500 here would fork the failure contract +within a module — the drift #4249 removed. + +No new codes: both were registered in the error-code ledger by #4263. The +envelope-conformance suites and the `REFUSALS` pin table gain one row per +route. diff --git a/packages/rest/src/external-datasource-envelope.conformance.test.ts b/packages/rest/src/external-datasource-envelope.conformance.test.ts index 41ee696737..dc846a1d4c 100644 --- a/packages/rest/src/external-datasource-envelope.conformance.test.ts +++ b/packages/rest/src/external-datasource-envelope.conformance.test.ts @@ -227,6 +227,29 @@ describe('external-datasource envelope (#3843) — error bodies', () => { { params: { name: 'ext', remote: 'customers' } }, ), }, + { + // #4264: the two rows below are the routes #4249 left uncovered — no + // `catch` at all, so these throws surfaced as the adapter's non-envelope + // `500 { error: 'No response from handler' }`, the real cause swallowed. + name: 'a catalog refresh the service refuses', + status: 400, + code: 'EXTERNAL_DATASOURCE_ERROR', + run: () => drive( + mount({ refreshCatalog: async () => { throw new Error('unknown datasource "ext"'); } }), + 'POST', + `${EXT}/refresh-catalog`, + ), + }, + { + name: 'a validation sweep the service refuses', + status: 400, + code: 'EXTERNAL_DATASOURCE_ERROR', + run: () => drive( + mount({ validateAll: async () => { throw new Error('metadata store offline'); } }), + 'POST', + `${EXT}/validate`, + ), + }, ]; for (const c of CASES) { diff --git a/packages/rest/src/external-datasource-routes.ts b/packages/rest/src/external-datasource-routes.ts index 0ec04146d7..39a772718c 100644 --- a/packages/rest/src/external-datasource-routes.ts +++ b/packages/rest/src/external-datasource-routes.ts @@ -43,6 +43,10 @@ import { sendOk, sendError } from '@objectstack/types'; * non-envelope `500 { error: 'No response from handler' }` — while the SAME two * service operations, reached through `service-datasource/admin-routes.ts`, * answered 400. One operation, one failure contract now, on both paths. + * #4264 closed the same gap on the two routes #4249 left uncovered — + * `refreshCatalog` / `validateAll` — whose throws (unknown datasource, + * unreachable remote, metadata-store failure) took the same uncaught path to + * that adapter 500. Every service call this module makes is now wrapped. * * Note `POST /validate` keeps its `ok` — unlike the `{ ok: true, key }` #3689 * retired from storage, that one was a private second word for `success`, while @@ -69,8 +73,13 @@ export function registerExternalDatasourceRoutes( const unavailable = (res: any) => sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The external-datasource service is not available.'); - /** An introspection refusal — same code as the admin-routes path (#4249). */ - const introspectionRefused = (res: any, err: unknown) => + /** + * A refusal from the external-datasource service — same code as the + * admin-routes path (#4249). Named after the service, not one operation: + * since #4264 every route here except the import (which has its own + * registered code) answers its catch with this. + */ + const refused = (res: any, err: unknown) => sendError(res, 400, 'EXTERNAL_DATASOURCE_ERROR', err instanceof Error ? err.message : String(err)); // List remote tables (optionally filtered by ?schema=). @@ -82,7 +91,7 @@ export function registerExternalDatasourceRoutes( const tables = await svc.listRemoteTables(req.params.name, { schema }); sendOk(res, { tables }); } catch (err) { - introspectionRefused(res, err); + refused(res, err); } }); @@ -98,7 +107,7 @@ export function registerExternalDatasourceRoutes( ); sendOk(res, { draft }); } catch (err) { - introspectionRefused(res, err); + refused(res, err); } }); @@ -130,16 +139,24 @@ export function registerExternalDatasourceRoutes( server.post(`${ext}/refresh-catalog`, async (req: any, res: any) => { const svc = externalService(); if (!svc?.refreshCatalog) return unavailable(res); - const catalog = await svc.refreshCatalog(req.params.name); - sendOk(res, { catalog }); + try { + const catalog = await svc.refreshCatalog(req.params.name); + sendOk(res, { catalog }); + } catch (err) { + refused(res, err); + } }); // Validate the federated objects on this datasource. server.post(`${ext}/validate`, async (req: any, res: any) => { const svc = externalService(); if (!svc?.validateAll) return unavailable(res); - const report = await svc.validateAll(); - const results = (report.results ?? []).filter((r: any) => r.datasource === req.params.name); - sendOk(res, { ok: results.every((r: any) => r.ok), results }); + try { + const report = await svc.validateAll(); + const results = (report.results ?? []).filter((r: any) => r.datasource === req.params.name); + sendOk(res, { ok: results.every((r: any) => r.ok), results }); + } catch (err) { + refused(res, err); + } }); } 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 982a531d2c..544248ff3c 100644 --- a/packages/services/service-datasource/src/__tests__/admin-routes.test.ts +++ b/packages/services/service-datasource/src/__tests__/admin-routes.test.ts @@ -260,6 +260,9 @@ describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => { svc: Record; run: (app: any) => Promise; }> = [ + // The list row is #4264: the one route that had no refusal path at all — + // its throw surfaced as the adapter's non-envelope 500, not any code here. + { route: 'GET /datasources', service: 'datasource-admin', code: 'DATASOURCE_ADMIN_ERROR', svc: { listDatasources: async () => { throw new Error('backing store offline'); } }, run: (a) => a.fetch(json('/api/v1/datasources')) }, { route: 'GET /datasources/:name', service: 'datasource-admin', code: 'DATASOURCE_ADMIN_ERROR', svc: { getDatasource: async () => { throw new Error('backing store offline'); } }, run: (a) => a.fetch(json('/api/v1/datasources/pg')) }, { route: 'POST /datasources/test', service: 'datasource-admin', code: 'DATASOURCE_ADMIN_ERROR', svc: { testConnection: async () => { throw new Error('backing store offline'); } }, run: (a) => a.fetch(json('/api/v1/datasources/test', { method: 'POST', body: '{}' })) }, { route: 'POST /datasources', service: 'datasource-admin', code: 'DATASOURCE_ADMIN_ERROR', svc: { createDatasource: async () => { throw new Error('backing store offline'); } }, run: (a) => a.fetch(json('/api/v1/datasources', { method: 'POST', body: '{}' })) }, 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 3b7f2d5732..ae063f49cf 100644 --- a/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts +++ b/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts @@ -187,6 +187,15 @@ describe('datasource-admin envelope (#3843) — error bodies', () => { code: 'DATASOURCE_ADMIN_ERROR', run: () => drive(mount({ createDatasource: async () => { throw new Error('duplicate name'); } }), '/api/v1/datasources', { method: 'POST', body: JSON.stringify({ name: 'pg' }) }), }, + { + // #4264: the one route in the module that still had no `catch`, so this + // throw surfaced as the adapter's non-envelope + // `500 { error: 'No response from handler' }` instead of the 400 below. + name: 'a datasource listing failure', + status: 400, + code: 'DATASOURCE_ADMIN_ERROR', + run: () => drive(mount({ listDatasources: async () => { throw new Error('backing store offline'); } }), '/api/v1/datasources'), + }, { // On an external-datasource route, so the refusal carries THAT service's // registered code (#4249) — even though this one is raised by the route diff --git a/packages/services/service-datasource/src/admin-routes.ts b/packages/services/service-datasource/src/admin-routes.ts index 9cd9c8bdb7..b5db6ff3c0 100644 --- a/packages/services/service-datasource/src/admin-routes.ts +++ b/packages/services/service-datasource/src/admin-routes.ts @@ -170,12 +170,19 @@ export function registerDatasourceAdminRoutes( return { draft, secret: normalised }; }; - // List all datasources with provenance + health. + // List all datasources with provenance + health. The catch was missing + // until #4264 — the one route in this module without one, so a backing-store + // failure surfaced as the adapter's non-envelope 500 instead of the 400 its + // eight siblings answer. server.get(root, async (_req: any, res: any) => { const svc = resolve(res, 'datasource-admin', 'listDatasources'); if (!svc) return; - const datasources = await svc.listDatasources(); - sendOk(res, { datasources }); + try { + const datasources = await svc.listDatasources(); + sendOk(res, { datasources }); + } catch (err) { + badRequest(res, 'datasource-admin', err); + } }); // Catalog of connection drivers + their JSON-Schema config (drives the