From bdbdaae521a95b8baf08e553ac499060b3ccd2d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 02:51:23 +0000 Subject: [PATCH] fix(service-datasource): external validate answers an unknown datasource 404 validateDatasource(name) returned an empty, vacuously ok report for a name no saved datasource has, so POST /api/v1/datasources/:name/external/validate answered 200 where GET /datasources/:name and the other federation doors answer 404 RESOURCE_NOT_FOUND. It now checks existence first, as testConnection does, and throws the package's one datasourceNotFoundError before anything is listed or introspected; the route relays it as such. A saved datasource with nothing federated, and the primary one, keep their 200 empty report. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_016WV4cx56cEs32AZSfABRR8 --- ...7696-external-validate-unknown-name-404.md | 22 +++++++++ .../rest/src/external-datasource-routes.ts | 5 +++ .../external-datasource-unknown-name.test.ts | 43 ++++++++++++++++++ ...external-datasource-validate-scope.test.ts | 16 ++++--- .../external-datasource-service.test.ts | 45 +++++++++++++++++-- .../src/external-datasource-service.ts | 21 ++++++--- 6 files changed, 137 insertions(+), 15 deletions(-) create mode 100644 .changeset/27696-external-validate-unknown-name-404.md diff --git a/.changeset/27696-external-validate-unknown-name-404.md b/.changeset/27696-external-validate-unknown-name-404.md new file mode 100644 index 00000000000..e6bf007a5f6 --- /dev/null +++ b/.changeset/27696-external-validate-unknown-name-404.md @@ -0,0 +1,22 @@ +--- +'@objectstack/service-datasource': patch +--- + +fix(service-datasource): `POST /api/v1/datasources/:name/external/validate` answers a name no saved datasource has `404 RESOURCE_NOT_FOUND`, as `GET /datasources/:name` and the other federation doors do + +Clause-②: no (narrowing) +Metadata-protocol: none + + + +`ExternalDatasourceService.validateDatasource(name)` selected the federated objects bound to `name` and validated them. For a name with no saved datasource it selected nothing and returned an empty, vacuously `ok` report, so the validate door answered `200 { ok: true, results: [] }` and a caller could not tell "nothing on this datasource is federated" from "this datasource does not exist". + +It now checks that the datasource exists first, the check `testConnection` makes (#27510): a name that resolves no saved datasource throws the package's one `datasourceNotFoundError` (`404`, `RESOURCE_NOT_FOUND`, `Datasource "NAME" does not exist.`) before anything is listed or introspected, and the REST route relays it as such. Only existence is asked, not the full federation guard: a saved datasource with nothing federated, a managed one included, and the platform's primary datasource (not looked up, as it may have no stored record) keep their `200` empty report. + +**Side effect.** The runtime's background drift check (`external.validation.checkIntervalMs`) also calls `validateDatasource`. For a datasource that was deleted after its timer was armed, a tick now lands in the check's existing catch (one `warn`, "drift check could not be performed", no events) where it used to return an empty report silently. + +**BREAKING** for a caller of `POST /api/v1/datasources/:name/external/validate`, or of `validateDatasource`, that addresses a datasource name that does not exist. + +- **Before.** `200` with `{ ok: true, results: [] }`; `validateDatasource` resolved to `{ ok: true, results: [] }`. +- **Now.** `404 RESOURCE_NOT_FOUND`; `validateDatasource` rejects with that error. +- **Fix.** Address a saved datasource, or treat the `404` as "no such datasource". diff --git a/packages/rest/src/external-datasource-routes.ts b/packages/rest/src/external-datasource-routes.ts index 1629a50c6b7..3d4b7d2b348 100644 --- a/packages/rest/src/external-datasource-routes.ts +++ b/packages/rest/src/external-datasource-routes.ts @@ -599,6 +599,11 @@ export function registerExternalDatasourceRoutes( // the post-filter used to keep are exactly the rows this returns (see // `external-datasource-validate-scope.test.ts`, which pins the two answers // against each other and the introspection call record beside them). + // + // [#27696] A name no saved datasource has is the service's 404 + // `datasourceNotFoundError`, thrown before any validation and relayed as + // such by `refused`; a datasource with nothing federated keeps its empty + // `200` report. { method: 'POST', path: `${ext}/validate`, diff --git a/packages/rest/src/external-datasource-unknown-name.test.ts b/packages/rest/src/external-datasource-unknown-name.test.ts index b2e2f5b56f7..3afc2f42ab2 100644 --- a/packages/rest/src/external-datasource-unknown-name.test.ts +++ b/packages/rest/src/external-datasource-unknown-name.test.ts @@ -118,3 +118,46 @@ describe('the federation twins on a datasource that exists and is not a federati expect(answer.body.error.message).toContain("is the platform's managed primary datasource"); }); }); + +/** + * [#27696] `POST …/external/validate`, the one by-name federation door the + * pins above left out. It answered an unknown name `200` with an empty, + * vacuously `ok` report, so a caller could not tell "nothing on this + * datasource is federated" from "this datasource does not exist". The ruling: + * no definition ⇒ the same `404`, before any validation; a datasource that + * exists and has nothing federated keeps its `200` empty report. + */ +describe('POST …/external/validate on a name that resolves no saved datasource', () => { + it('answers 404 RESOURCE_NOT_FOUND in the words GET /datasources/:name uses', async () => { + const call = mount(); + + const answer = await call('POST', '/no_such_ds/external/validate'); + + expect({ + status: answer.status, + success: answer.body.success, + code: answer.body.error?.code, + message: answer.body.error?.message, + }).toEqual({ + status: 404, + success: false, + code: 'RESOURCE_NOT_FOUND', + message: 'Datasource "no_such_ds" does not exist.', + }); + expect(answer.body).not.toHaveProperty('data'); + }); + + it.each([ + ['a saved datasource', 'warehouse'], + ['the primary datasource', 'default'], + ])('keeps the 200 empty report for %s with nothing federated', async (_what, name) => { + const call = mount(); + + const answer = await call('POST', `/${name}/external/validate`); + + expect({ status: answer.status, data: answer.body.data }).toEqual({ + status: 200, + data: { ok: true, results: [] }, + }); + }); +}); diff --git a/packages/rest/src/external-datasource-validate-scope.test.ts b/packages/rest/src/external-datasource-validate-scope.test.ts index 25f11eb3667..9abd0580894 100644 --- a/packages/rest/src/external-datasource-validate-scope.test.ts +++ b/packages/rest/src/external-datasource-validate-scope.test.ts @@ -213,16 +213,18 @@ describe('[#10537] POST /external/validate scopes its WORK to :name', () => { expect(body.data).toEqual(reference); }); - it('an unknown :name answers the same empty report as before — and dials nothing', async () => { + // [#27696] Until this ruling an unknown name answered an empty report with + // a vacuously true verdict; it is now the 404 every by-name door answers. + // `external-datasource-unknown-name.test.ts` pins the 200 a saved datasource + // with nothing federated keeps. + it('an unknown :name answers 404 RESOURCE_NOT_FOUND — and dials nothing', async () => { const { app, introspected, validateAllSpy } = mount(); const { status, body } = await validate(app, 'no_such_ds'); - expect(status).toBe(200); - expect(body.success).toBe(true); - // Unchanged: an unknown name is not a 404 on this route, it is an empty - // report with a vacuously true verdict. - expect(body.data).toEqual(await referenceAnswer('no_such_ds')); - expect(body.data).toEqual({ ok: true, results: [] }); + expect(status).toBe(404); + expect(body.success).toBe(false); + expect(body.error.code).toBe('RESOURCE_NOT_FOUND'); + expect(body.error.message).toBe('Datasource "no_such_ds" does not exist.'); expect(introspected).toEqual([]); expect(validateAllSpy).not.toHaveBeenCalled(); }); diff --git a/packages/services/service-datasource/src/__tests__/external-datasource-service.test.ts b/packages/services/service-datasource/src/__tests__/external-datasource-service.test.ts index 0fde8164a29..5a1aad324bf 100644 --- a/packages/services/service-datasource/src/__tests__/external-datasource-service.test.ts +++ b/packages/services/service-datasource/src/__tests__/external-datasource-service.test.ts @@ -11,6 +11,7 @@ import { type DatasourceLike, type ObjectLike, } from '../external-datasource-service.js'; +import { datasourceNotFoundError } from '../datasource-admin-service.js'; /** Build a fake introspected schema for the `warehouse` datasource. */ function warehouseSchema(): IntrospectedSchema { @@ -662,14 +663,52 @@ describe('validateDatasource', () => { }); }); - it('answers an empty report for a datasource nothing is bound to — and introspects nothing', async () => { - const { svc, introspected } = makeMulti(); + it('answers an empty report for a saved datasource nothing is bound to — and introspects nothing', async () => { + const introspected: string[] = []; + const svc = new ExternalDatasourceService({ + introspect: async (datasource: string) => { + introspected.push(datasource); + return REMOTE; + }, + getDatasource: async (name: string) => (name === 'wh_empty' ? { name, schemaMode: 'external' } : undefined), + getObject: async () => undefined, + listObjects: async () => OBJECTS, + }); - const report = await svc.validateDatasource('no_such_ds'); + const report = await svc.validateDatasource('wh_empty'); expect(report).toEqual({ ok: true, results: [] }); expect(introspected).toEqual([]); }); + + // [#27696] A name with no saved datasource is not a datasource with nothing + // federated: it is the 404 every by-name door answers, thrown before + // anything is listed or introspected. + it('throws the 404 datasourceNotFoundError for a name that resolves no saved datasource', async () => { + let listed = 0; + const introspected: string[] = []; + const svc = new ExternalDatasourceService({ + introspect: async (datasource: string) => { + introspected.push(datasource); + return REMOTE; + }, + getDatasource: async () => undefined, + getObject: async () => undefined, + listObjects: async () => { + listed++; + return OBJECTS; + }, + }); + + const thrown = await svc.validateDatasource('no_such_ds').catch((err: unknown) => err); + + expect(thrown).toMatchObject({ + status: 404, + code: 'RESOURCE_NOT_FOUND', + message: datasourceNotFoundError('no_such_ds').message, + }); + expect({ listed, introspected }).toEqual({ listed: 0, introspected: [] }); + }); }); /** diff --git a/packages/services/service-datasource/src/external-datasource-service.ts b/packages/services/service-datasource/src/external-datasource-service.ts index 922e07c2df3..80880c3773a 100644 --- a/packages/services/service-datasource/src/external-datasource-service.ts +++ b/packages/services/service-datasource/src/external-datasource-service.ts @@ -1206,11 +1206,19 @@ export class ExternalDatasourceService implements IExternalDatasourceService { * the sweep would have kept" and "the objects this selects" are the same set * (pinned in `__tests__/external-datasource-service.test.ts`). * - * A name nothing is bound to selects nothing and answers an empty, vacuously - * `ok` report — the sweep-then-filter answer for an unknown name, kept rather - * than upgraded to a throw: whether an unknown datasource is an error is a - * separate question from this one, and this method must not decide it in - * passing. + * A saved datasource nothing is bound to selects nothing and answers an + * empty, vacuously `ok` report. + * + * [#27696] A name that resolves no saved datasource is not that datasource: + * it THROWS the 404 {@link datasourceNotFoundError} before anything is + * listed or validated, the check {@link testConnection} makes (#27510), so + * `POST …/external/validate` answers it as `GET /datasources/:name` and its + * federation siblings do. It answered the empty `ok` report above, and a + * caller could not tell "nothing here is federated" from "no such + * datasource". Only existence is asked, not the federation guard + * ({@link assertFederationSource}): a managed datasource, and the primary + * one (not looked up, as it may have no stored record), keep their empty + * report. * * NOT on `IExternalDatasourceService` (spec): the route composition this * serves was authorized as a service-side helper, while adding a @@ -1220,6 +1228,9 @@ export class ExternalDatasourceService implements IExternalDatasourceService { * scoped spelling, rather than silently falling back to the fan-out. */ async validateDatasource(datasource: string): Promise { + if (datasource !== DEFAULT_DATASOURCE_NAME && !(await this.config.getDatasource(datasource))) { + throw datasourceNotFoundError(datasource); + } const objects = await this.config.listObjects(); return this.validateEach( objects.filter((o) => this.isFederated(o) && (o.datasource ?? 'default') === datasource),