diff --git a/.changeset/27510-datasource-test-unknown-name-404.md b/.changeset/27510-datasource-test-unknown-name-404.md new file mode 100644 index 00000000000..5856fb17301 --- /dev/null +++ b/.changeset/27510-datasource-test-unknown-name-404.md @@ -0,0 +1,19 @@ +--- +'@objectstack/service-datasource': patch +--- + +fix(service-datasource)!: `POST /api/v1/datasources/:name/test` refuses a name that no saved datasource has with `404 RESOURCE_NOT_FOUND`, as the other by-name doors do, instead of answering `200 { ok: false }` + +Clause-②: no (narrowing) — `POST /api/v1/datasources/:name/test` and the in-process `ExternalDatasourceService.testConnection(name)` refuse a name that resolves no saved datasource with `404 RESOURCE_NOT_FOUND`, which they answered `200 { ok: false }` before. Nothing is widened. +Metadata-protocol: none + + + +**BREAKING** accept-set narrowing at the saved-datasource Test door (`@objectstack/service-datasource`), served at `POST /api/v1/datasources/:name/test`. + +- **Before.** Testing a saved datasource by a name that no saved datasource has (a typo, a removed datasource, a name from another deployment) was answered `200` with `{ ok: false, error: "[ObjectQL] Datasource '…' has no registered driver to introspect." }`. That is the answer a saved datasource that is down gets, so an admin tool or script could not tell "this datasource is down" from "this datasource does not exist". `GET`, `PATCH` and `DELETE /api/v1/datasources/:name` already answered the same name `404`. +- **Now.** That request is refused `404` with `error.code` `RESOURCE_NOT_FOUND` and the message `GET /api/v1/datasources/:name` gives for the same name. Nothing is probed. The name is resolved through the same stored-record read the sibling by-name doors make. In process, `ExternalDatasourceService.testConnection(name)` throws that `404` error instead of returning `{ ok: false }`. +- **Unchanged.** A saved datasource that fails its probe still answers `200 { ok: false, error }`, and one that is up still answers `200 { ok: true, … }`. The platform's primary datasource (`default`) is still tested through its live driver, whether or not a datasource record is stored for it. Every other outcome of `testConnection` is still returned, never thrown. +- **Who sees it.** A caller holding the platform-settings capability who tests a name that no saved datasource has. + +**The remedy.** None needed: pick the datasource from `GET /api/v1/datasources`, or check the name. A script that read `ok: false` as "does not exist" now reads `404`. diff --git a/packages/services/service-datasource/src/__tests__/primary-datasource-test-and-sync.test.ts b/packages/services/service-datasource/src/__tests__/primary-datasource-test-and-sync.test.ts index 31233f46a20..b504110e5a9 100644 --- a/packages/services/service-datasource/src/__tests__/primary-datasource-test-and-sync.test.ts +++ b/packages/services/service-datasource/src/__tests__/primary-datasource-test-and-sync.test.ts @@ -67,13 +67,17 @@ type Table = { string(n: string): { primary(): void } }; * the same records (`metadata.get('datasource', name)`, as `plugin.ts` wires * it), so a datasource's Sync answer and its listed `schemaMode` are one fact. * `stream` is a FEDERATED datasource on a driver that cannot introspect, the - * subject of the engine-reason pin below. + * subject of the engine-reason pin below. `ghost` is saved but never + * connected, the no-live-driver subject of the Test pin below: [#27510] it is + * a stored record because Test answers a name with no record `404` + * (`saved-datasource-test-unknown-name.test.ts`). */ const RECORDS = [ { name: 'default', driver: 'sqlite', origin: 'code', config: {} }, { name: 'warehouse', driver: 'sqlite', origin: 'runtime', config: {} }, { name: 'scratch', driver: 'memory', origin: 'runtime', config: {} }, { name: 'stream', driver: 'memory', origin: 'runtime', schemaMode: 'external', external: {}, config: {} }, + { name: 'ghost', driver: 'sqlite', origin: 'runtime', config: {} }, ]; /** @@ -248,7 +252,8 @@ describe('a driver with no introspectSchema', () => { throw new Error('introspect must not be reached'); }, liveDriver: () => ({ checkHealth }), - getDatasource: async () => undefined, + // [#27510] A saved datasource: Test answers a name with no record 404. + getDatasource: async (name) => ({ name }), getObject: async () => undefined, listObjects: async () => [], }); diff --git a/packages/services/service-datasource/src/__tests__/saved-datasource-test-after-failed-connect.test.ts b/packages/services/service-datasource/src/__tests__/saved-datasource-test-after-failed-connect.test.ts index 3f5e8f41897..3eed54b3c2f 100644 --- a/packages/services/service-datasource/src/__tests__/saved-datasource-test-after-failed-connect.test.ts +++ b/packages/services/service-datasource/src/__tests__/saved-datasource-test-after-failed-connect.test.ts @@ -126,7 +126,7 @@ async function boot() { return { status: res.status, body: (await res.json()) as any }; }; - return { call, connection, build }; + return { call, connection, build, records }; } const listed = async (call: Awaited>['call'], name: string) => @@ -192,7 +192,10 @@ describe('Test on a datasource whose connect failed', () => { describe('what does not move', () => { it('a datasource with no verdict and no driver keeps the driver-missing sentence', async () => { - const { call } = await boot(); + const { call, records } = await boot(); + // Saved, never connected. [#27510] It is a stored record because Test + // answers a name with no record 404 (`saved-datasource-test-unknown-name.test.ts`). + records.set('ghost', { name: 'ghost', driver: 'postgres', origin: 'runtime', config: UNREACHABLE }); const test = await call('POST', '/ghost/test'); expect(test.status).toBe(200); @@ -234,7 +237,8 @@ describe('only a failed verdict is answered', () => { }, liveDriver: () => undefined, connectionState: () => ({ availability, ...(reason ? { reason } : {}) }), - getDatasource: async () => undefined, + // [#27510] A saved datasource: Test answers a name with no record 404. + getDatasource: async (name) => ({ name }), getObject: async () => undefined, listObjects: async () => [], }); diff --git a/packages/services/service-datasource/src/__tests__/saved-datasource-test-unknown-name.test.ts b/packages/services/service-datasource/src/__tests__/saved-datasource-test-unknown-name.test.ts new file mode 100644 index 00000000000..c070804377b --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/saved-datasource-test-unknown-name.test.ts @@ -0,0 +1,182 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `POST /datasources/:name/test` on a name that resolves no saved datasource. + * + * Every other by-name door answers an unknown name `404 RESOURCE_NOT_FOUND` + * (`datasource-unknown-name.test.ts` holds the four `datasource-admin` ones). + * Test answered it `200 { ok: false }` with the engine's "has no registered + * driver to introspect" sentence, which is the answer it gives a saved + * datasource that is down. An admin tool or script testing a datasource by + * name could not tell "this datasource is down" from "this datasource does not + * exist". + * + * The triage ruling these pins hold: resolve the saved name with the lookup + * the sibling routes use and answer `RESOURCE_NOT_FOUND`; keep `{ ok: false }` + * for a datasource that exists and fails its probe. Two controls ride along: + * a saved datasource that is up still answers `ok: true`, and the platform's + * primary datasource, which may have no stored record, is still tested through + * its live driver. + * + * The composition is the real one, end to end: a real `ObjectQL` engine, the + * real connection service and driver factory, the real admin service, the real + * `ExternalDatasourceServicePlugin` wiring, and the real admin routes over a + * Hono server. Both services read the same stored records, as the two plugins + * do (`metadata.get('datasource', name)`). + */ + +import { afterEach, describe, expect, it } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { HonoHttpServer } from '@objectstack/plugin-hono-server'; +import { DatasourceConnectionService, DEFAULT_DATASOURCE_NAME } from '../datasource-connection-service.js'; +import { createDefaultDatasourceDriverFactory } from '../default-datasource-driver-factory.js'; +import { ExternalDatasourceServicePlugin } from '../plugin.js'; +import { ExternalDatasourceService } from '../external-datasource-service.js'; +import { DatasourceAdminService, type StoredDatasource } from '../datasource-admin-service.js'; +import { registerDatasourceAdminRoutes } from '../admin-routes.js'; +import { ENTITLED_CREDENTIAL, createSessionAuthService, createGrantsEngine } from './entitled-caller.fixture.js'; + +const teardowns: Array<() => Promise> = []; +afterEach(async () => { + for (const t of teardowns.splice(0)) await t(); +}); + +/** + * The saved datasources. `warehouse` is connected; `offline` is saved but has + * no live driver (nothing connected it), so its probe fails. The primary + * datasource has NO record here: it is connected `asDefault`, as + * `DefaultDatasourcePlugin.init()` does, and nothing stores it. + */ +const RECORDS: StoredDatasource[] = [ + { name: 'warehouse', driver: 'memory', origin: 'runtime', config: {} }, + { name: 'offline', driver: 'memory', origin: 'runtime', config: {} }, +]; + +async function boot() { + const ql = new ObjectQL(); + const connection = new DatasourceConnectionService({ + factory: () => createDefaultDatasourceDriverFactory({}), + engine: () => ql as never, + }); + teardowns.push(() => connection.disconnectAll()); + + await connection.connect( + { name: DEFAULT_DATASOURCE_NAME, driver: 'memory', config: {}, origin: 'code', bootCritical: true }, + { asDefault: true, context: { origin: 'code', trigger: 'declared-auto' } }, + ); + await connection.connect(RECORDS[0], { context: { origin: 'runtime', trigger: 'runtime-admin' } }); + + const records = new Map(RECORDS.map((r) => [r.name, r])); + const admin = new DatasourceAdminService({ + listDatasourceRecords: async () => [...records.values()], + getDatasourceRecord: async (n: string) => records.get(n), + connectionStates: () => connection.listConnectionStates(), + } as never); + + const services = new Map([ + ['data', ql], + ['metadata', { get: async (type: string, n: string) => (type === 'datasource' ? records.get(n) : undefined) }], + ['datasource-connection', connection], + ]); + await new ExternalDatasourceServicePlugin().init({ + getService: (n: string) => { + if (!services.has(n)) throw new Error(`no service: ${n}`); + return services.get(n); + }, + registerService: (n: string, s: unknown) => void services.set(n, s), + } as never); + const external = services.get('external-datasource') as ExternalDatasourceService; + + const server = new HonoHttpServer(0); + const authService = createSessionAuthService(); + const grantsEngine = createGrantsEngine(); + registerDatasourceAdminRoutes( + server, + { + getService: (n: string) => { + if (n === 'auth') return authService; + if (n === 'objectql' || n === 'data') return grantsEngine; + if (n === 'external-datasource') return external; + if (n === 'datasource-admin') return admin; + throw new Error(`no service: ${n}`); + }, + } as never, + '/api/v1', + ); + const app = server.getRawApp(); + const call = async (method: 'GET' | 'POST', path: string) => { + const res = await app.fetch( + new Request(`http://local/api/v1/datasources${path}`, { + method, + headers: { authorization: ENTITLED_CREDENTIAL, 'content-type': 'application/json' }, + ...(method === 'POST' ? { body: '{}' } : {}), + }), + ); + return { status: res.status, body: (await res.json()) as any }; + }; + + return { call, records }; +} + +describe('POST /datasources/:name/test on a name that resolves no saved datasource', () => { + it('answers 404 RESOURCE_NOT_FOUND naming the datasource, not 200 { ok: false }', async () => { + const { call } = await boot(); + + const test = await call('POST', '/no_such_ds/test'); + + expect({ status: test.status, success: test.body.success, code: test.body.error?.code }).toEqual({ + status: 404, + success: false, + code: 'RESOURCE_NOT_FOUND', + }); + expect(test.body.error.message).toContain('no_such_ds'); + expect(test.body).not.toHaveProperty('data'); + }); + + it('gives the answer GET /datasources/:name gives the same name: status, code and message', async () => { + const { call } = await boot(); + + const read = await call('GET', '/no_such_ds'); + const test = await call('POST', '/no_such_ds/test'); + + expect(read.status).toBe(404); + expect({ status: test.status, code: test.body.error?.code, message: test.body.error?.message }).toEqual({ + status: read.status, + code: read.body.error.code, + message: read.body.error.message, + }); + }); +}); + +describe('POST /datasources/:name/test on a datasource that exists', () => { + it('a saved datasource that fails its probe still answers 200 { ok: false, error }', async () => { + const { call } = await boot(); + + const test = await call('POST', '/offline/test'); + + expect(test.status).toBe(200); + expect(test.body.data.ok).toBe(false); + expect(typeof test.body.data.error).toBe('string'); + expect(test.body.data.error).toContain('offline'); + }); + + it('a saved datasource that is up still answers 200 { ok: true }', async () => { + const { call } = await boot(); + + const test = await call('POST', '/warehouse/test'); + + expect(test.status).toBe(200); + expect(test.body.data).toMatchObject({ ok: true }); + }); + + it('the primary datasource with no stored record is still tested through its live driver', async () => { + const { call, records } = await boot(); + expect(records.has(DEFAULT_DATASOURCE_NAME)).toBe(false); + + const test = await call('POST', `/${DEFAULT_DATASOURCE_NAME}/test`); + + expect(test.status).toBe(200); + expect(test.body.data).toMatchObject({ ok: true }); + expect(typeof test.body.data.latencyMs).toBe('number'); + }); +}); diff --git a/packages/services/service-datasource/src/admin-routes.ts b/packages/services/service-datasource/src/admin-routes.ts index 5e1cdce613b..664cf9d25bf 100644 --- a/packages/services/service-datasource/src/admin-routes.ts +++ b/packages/services/service-datasource/src/admin-routes.ts @@ -86,7 +86,7 @@ const SERVICE_ERROR_CODE: Record = { * `DATASOURCE_ADMIN_ERROR` or `EXTERNAL_DATASOURCE_ERROR`. A name that resolves * no stored datasource is not a refusal: every `datasource-admin` door that * takes `:name` answers it `404` `RESOURCE_NOT_FOUND`, the read and the writes - * alike (see `badRequest`). + * alike, and so does `POST /datasources/:name/test` (see `badRequest`). * * Served by `datasource-admin`: * @@ -641,7 +641,8 @@ export function registerDatasourceAdminRoutes( * unknown datasource reads the same at every by-name door, and a caller can * tell "no such datasource" from "your request was refused". The throwers * today are the admin service's `updateDatasource`, `removeDatasource` and - * `migrateCredential` (`datasourceNotFoundError`). + * `migrateCredential`, and the external service's `testConnection` (each + * through its `datasourceNotFoundError`). */ const badRequest = (res: any, service: ServiceName, err: unknown) => { const envelope = err as { code?: unknown; status?: unknown } | null | undefined; diff --git a/packages/services/service-datasource/src/external-datasource-service.ts b/packages/services/service-datasource/src/external-datasource-service.ts index bcb6aad5408..4750f4f6e7f 100644 --- a/packages/services/service-datasource/src/external-datasource-service.ts +++ b/packages/services/service-datasource/src/external-datasource-service.ts @@ -447,6 +447,21 @@ function importNameRefusedError(message: string): Error { return err; } +/** + * [#27510] What the saved-datasource Test answers for a name that resolves no + * stored datasource: the `404` / `RESOURCE_NOT_FOUND` envelope (ADR-0112), in + * the words `GET /datasources/:name` uses, so the admin route relays it as the + * read sibling answers the same name. It mirrors the admin service's helper of + * the same name; `saved-datasource-test-unknown-name.test.ts` holds the two + * doors' answers equal. + */ +function datasourceNotFoundError(name: string): Error { + const err = new Error(`Datasource "${name}" does not exist.`) as Error & { code?: string; status?: number }; + err.code = 'RESOURCE_NOT_FOUND'; + err.status = 404; + return err; +} + /** * [#24772] The refusal every federation door answers for the platform's * primary datasource: listing its remote tables, drafting or importing one, @@ -696,10 +711,24 @@ export class ExternalDatasourceService implements IExternalDatasourceService { * rebuild does, and replaces the verdict either way. A datasource whose * failed rebuild kept its old pool serving has a live driver, so it is * probed as before. + * + * [#27510] A name that resolves no saved datasource is not a datasource that + * failed its probe, so it is the one outcome that THROWS: the 404 + * {@link datasourceNotFoundError}, which the route answers exactly as + * `GET /datasources/:name` answers the same name. It answered `ok: false` + * with the engine's "has no registered driver" sentence, the answer a saved + * datasource that is down gets, so a caller could not tell the two apart. + * The name is resolved through `getDatasource`, the stored-record read the + * sibling `:name` doors make (both plugins wire it to + * `metadata.get('datasource', name)`). The primary datasource is not looked + * up: it may have no stored record, and its live driver answers for it. */ async testConnection( datasource: string, ): Promise<{ ok: boolean; latencyMs?: number; tableCount?: number; error?: string }> { + if (datasource !== DEFAULT_DATASOURCE_NAME && !(await this.config.getDatasource(datasource))) { + throw datasourceNotFoundError(datasource); + } const started = Date.now(); try { const driver = this.config.liveDriver?.(datasource);