diff --git a/.changeset/remote-tables-twins-forward-schema.md b/.changeset/remote-tables-twins-forward-schema.md new file mode 100644 index 0000000000..61f72cfe9d --- /dev/null +++ b/.changeset/remote-tables-twins-forward-schema.md @@ -0,0 +1,37 @@ +--- +"@objectstack/service-datasource": patch +--- + +fix(service-datasource): the admin `remote-tables` route honours `?schema=` instead of dropping it (#7955) + +`IExternalDatasourceService.listRemoteTables` is reachable through two live +routes, and only one of them read the query: + +- `GET /api/v1/datasources/:name/external/tables` (federation, `packages/rest`) + forwards `?schema=` to the service. +- `GET /api/v1/datasources/:name/remote-tables` (admin, this package) never + touched `req.query`, so `?schema=public` came back as the UNFILTERED listing — + not the filtered set, and not a refusal either. + +Wire-visible change, on the admin spelling only: `?schema=` now narrows the +listing to that remote schema, exactly as the federation twin already did. A +request with no `?schema=` is unchanged — it still returns the full listing, so +every existing caller (none of which can have been passing the parameter +meaningfully) sees the same bytes as before. + +The coercion is copied from the federation route rather than reinvented, down to +its treatment of a non-string: a repeated `?schema=a&schema=b` reaches the +handler as an array and both spellings drop it to "no filter". No refusal, no +warning, no deprecation is added here — whether an unusable query parameter +should be REFUSED is the global ingress-policy question tracked by #7606, and +honouring the parameter is correct under either answer it reaches, so the twins +can move together then. + +This finishes on the REQUEST path what #4249 did for the failure path ("one +operation, one failure contract now, on both paths"). The equivalence is pinned +across the two packages by +`packages/rest/src/remote-tables-twin.equivalence.test.ts`, which drives the same +query at BOTH spellings against one service and compares the answers — a test +that exercised only the fixed route could not fail if the twins drift apart +again. `@objectstack/rest` gains two dev-only workspace dependencies so that test +can mount both registrars; its published surface is unchanged. diff --git a/packages/rest/package.json b/packages/rest/package.json index ded8cc481a..f1c0977689 100644 --- a/packages/rest/package.json +++ b/packages/rest/package.json @@ -35,8 +35,10 @@ "@objectstack/metadata": "workspace:*", "@objectstack/metadata-protocol": "workspace:*", "@objectstack/objectql": "workspace:*", + "@objectstack/plugin-hono-server": "workspace:*", "@objectstack/plugin-security": "workspace:*", "@objectstack/service-analytics": "workspace:*", + "@objectstack/service-datasource": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/rest/src/remote-tables-twin.equivalence.test.ts b/packages/rest/src/remote-tables-twin.equivalence.test.ts new file mode 100644 index 0000000000..f16ef678e5 --- /dev/null +++ b/packages/rest/src/remote-tables-twin.equivalence.test.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `listRemoteTables` REQUEST-shape equivalence across its two live spellings + * (#7955). + * + * `IExternalDatasourceService.listRemoteTables` is reachable through two + * mounted routes, in two packages: + * + * GET /api/v1/datasources/:name/external/tables ← this package, federation + * GET /api/v1/datasources/:name/remote-tables ← @objectstack/service-datasource, admin + * + * They are not near-duplicates that happen to look alike: both resolve the SAME + * `external-datasource` service slot and call the SAME method with the same + * datasource name. #4249 already reconciled what happens when that one call + * THROWS ("One operation, one failure contract now, on both paths", + * `external-datasource-routes.ts`). What was never compared is the other half — + * what the two paths do with the REQUEST — and #7955 is the residue: the admin + * spelling never read `req.query`, so `?schema=public` came back as the + * UNFILTERED listing there and the filtered one here. Not an error, not the + * filter: the quietest form of "declared ≠ enforced". + * + * ## Why this test is here, and why it drives BOTH routes + * + * A test that exercised only the fixed route could not fail if the twins drift + * apart again — it would pin one path's behaviour and say nothing about the + * relationship, which IS the invariant. So every case below issues the same + * query to both spellings and asserts the two answers are equal, against ONE + * service instance mounted on ONE server: the difference between the readings + * can then only come from the two handlers. + * + * It lives in `packages/rest` because that is the side that can reach both + * halves without widening anyone's public API: `registerDatasourceAdminRoutes` + * is exported from `@objectstack/service-datasource`'s index, while + * `registerExternalDatasourceRoutes` is deliberately internal here (it is + * composed by `rest-api-plugin.ts`, not published). The dependency is dev-only + * and points rest → service-datasource, which is not a cycle: that package + * depends on `core`/`spec`/`types` and never on this one. Same reasoning the + * client-side ledger guard used when it chose its side (`service-route-ledger- + * coverage.test.ts`, "a service→client package edge would be backwards"). + * + * The service under the routes is the REAL `ExternalDatasourceService` over a + * fake introspector, not a `vi.fn()` recording its arguments. A mock would only + * prove the admin handler now passes an options bag; what the card asks is that + * the two routes return the same SET, which takes the real filter. + * + * Driven through the real `HonoHttpServer` — the adapter `os serve` mounts — so + * `?schema=` is parsed by the code that parses it in production. That matters + * for the last case: a repeated key reaches a handler as an ARRAY, and only a + * real adapter produces one. + */ + +import { describe, it, expect } from 'vitest'; +import { HonoHttpServer } from '@objectstack/plugin-hono-server'; +import { + ExternalDatasourceService, + registerDatasourceAdminRoutes, +} from '@objectstack/service-datasource'; +import type { IntrospectedColumn, IntrospectedSchema } from '@objectstack/spec/contracts'; +import { registerExternalDatasourceRoutes } from './external-datasource-routes.js'; + +const DS = 'demo_ext'; + +/** One remote column, spelled in full so the fixture needs no cast to be an + * `IntrospectedSchema` — `primaryKey` and `nullable` are both required. */ +const col = (name: string, primaryKey = false): IntrospectedColumn => ({ + name, + type: name === 'id' ? 'uuid' : 'text', + nullable: !primaryKey, + primaryKey, +}); + +/** Two remote schemas, so a `?schema=` filter has something to exclude. */ +const REMOTE: IntrospectedSchema = { + dialect: 'postgres', + introspectedAt: '2026-08-12T00:00:00.000Z', + tables: { + 'public.customers': { + name: 'public.customers', + columns: [col('id', true), col('email')], + indexes: [], + }, + 'public.orders': { + name: 'public.orders', + columns: [col('id', true)], + indexes: [], + }, + 'analytics.events': { + name: 'analytics.events', + columns: [col('id', true)], + indexes: [], + }, + }, +}; + +/** + * One server, one service, both registrars — the point of the fixture. + * + * `registerDatasourceAdminRoutes` also mounts the datasource-lifecycle family, + * whose services are absent here; those routes answer 503 and are simply never + * driven. The two paths under test both resolve `external-datasource`, which is + * the one service wired. + */ +function mountBoth() { + const service = new ExternalDatasourceService({ + introspect: async () => REMOTE, + getDatasource: async (name: string) => ({ name }), + getObject: async () => undefined, + listObjects: async () => [], + }); + const server = new HonoHttpServer(0); + const ctx = { + getService: (name: string) => { + if (name === 'external-datasource') return service; + throw new Error(`no service: ${name}`); + }, + } as any; + registerExternalDatasourceRoutes(server, ctx, '/api/v1'); + registerDatasourceAdminRoutes(server, ctx, '/api/v1'); + return server.getRawApp(); +} + +/** The two wire spellings of the one operation, keyed by how they are named. */ +const SPELLING = { + federation: (qs: string) => `/api/v1/datasources/${DS}/external/tables${qs}`, + admin: (qs: string) => `/api/v1/datasources/${DS}/remote-tables${qs}`, +} as const; + +interface Reading { + status: number; + tables: Array<{ schema?: string; name: string }>; +} + +/** Drive one spelling and read back the table set it answers with. */ +async function read(app: any, spelling: keyof typeof SPELLING, qs: string): Promise { + const res = await app.fetch(new Request(`http://local${SPELLING[spelling](qs)}`)); + const body = (await res.json()) as { success: boolean; data?: { tables?: Reading['tables'] } }; + return { status: res.status, tables: body.data?.tables ?? [] }; +} + +/** Both spellings, same query — the comparison every case makes. */ +async function readBoth(qs: string): Promise<{ federation: Reading; admin: Reading }> { + const app = mountBoth(); + return { + federation: await read(app, 'federation', qs), + admin: await read(app, 'admin', qs), + }; +} + +const qualified = (r: Reading) => r.tables.map((t) => `${t.schema}.${t.name}`).sort(); + +describe('listRemoteTables twins agree on the request shape (#7955)', () => { + it('?schema= returns the SAME filtered set on both spellings', async () => { + const { federation, admin } = await readBoth('?schema=public'); + + expect(federation.status).toBe(200); + expect(admin.status).toBe(200); + // The filter really filtered — otherwise "equal" could mean "both unfiltered", + // which is precisely the pre-#7955 reading on one of the two paths. + expect(qualified(federation)).toEqual(['public.customers', 'public.orders']); + expect(qualified(admin)).toEqual(qualified(federation)); + }); + + it('no ?schema= returns the SAME unfiltered set on both spellings', async () => { + const { federation, admin } = await readBoth(''); + + expect(qualified(federation)).toEqual([ + 'analytics.events', + 'public.customers', + 'public.orders', + ]); + // The absent-parameter arm is not a formality: a fix that filtered + // unconditionally would satisfy the case above and break every existing + // caller of the admin spelling, which has never passed one. + expect(qualified(admin)).toEqual(qualified(federation)); + }); + + it('a ?schema= that matches nothing returns the SAME empty set on both spellings', async () => { + const { federation, admin } = await readBoth('?schema=nonexistent'); + + expect(federation.tables).toEqual([]); + expect(admin.tables).toEqual([]); + expect(admin.status).toBe(federation.status); + }); + + it('a repeated ?schema= degrades to no filter on both spellings, identically', async () => { + // The adapter surfaces a repeated key as an array; `typeof … === 'string'` + // is what both handlers do with it, so both fall back to the unfiltered + // listing rather than filtering by an arbitrary one of the two. Whether such + // a request should be REFUSED instead is #7606's global ingress question — + // this case pins that the twins answer it the SAME way today, whatever that + // card decides tomorrow. + const { federation, admin } = await readBoth('?schema=public&schema=analytics'); + + expect(qualified(federation)).toEqual([ + 'analytics.events', + 'public.customers', + 'public.orders', + ]); + expect(qualified(admin)).toEqual(qualified(federation)); + expect(admin.status).toBe(federation.status); + }); + + it('an empty ?schema= is no filter on both spellings, identically', async () => { + // `?schema=` parses to the empty string, which is a string — so the + // coercion keeps it and the service's own `opts?.schema &&` guard is what + // treats it as "no filter". Both spellings inherit that from the one + // service, and this pins that neither route second-guesses it. + const { federation, admin } = await readBoth('?schema='); + + expect(qualified(federation)).toEqual([ + 'analytics.events', + 'public.customers', + 'public.orders', + ]); + expect(qualified(admin)).toEqual(qualified(federation)); + }); +}); diff --git a/packages/rest/vitest.config.ts b/packages/rest/vitest.config.ts index 782c6a40a4..bf9784f482 100644 --- a/packages/rest/vitest.config.ts +++ b/packages/rest/vitest.config.ts @@ -1,10 +1,50 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { defineConfig } from 'vitest/config'; +import path from 'path'; export default defineConfig({ test: { globals: true, environment: 'node', }, + resolve: { + // Both entries exist for `remote-tables-twin.equivalence.test.ts` (#7955), + // the one suite here that imports sibling packages as VALUES: it mounts the + // `service-datasource` admin registrar next to this package's federation + // registrar to pin that the two `listRemoteTables` spellings answer `?schema=` + // identically, and drives both through the real Hono adapter. + // + // Unaliased, those two specifiers resolve through the workspace link to + // `dist/` — a BUILD ARTIFACT — which makes this suite's verdict a function of + // build state rather than of the source in the checkout. The loud failure + // (missing export) is the mild half; a dist merely BEHIND lets the test run + // GREEN against the dependency's old behaviour, and nothing in the output + // says so. That is exactly the hazard for a CROSS-PACKAGE equivalence pin: + // its whole job is to notice when one of the two twins moves, and a stale + // `service-datasource` dist would report the pre-fix admin route as agreeing + // with the federation one — the #7955 defect itself, passing. + // + // Turbo already orders `test` after `^build`, so `turbo run test` was never + // the failing path. The paths it does not mediate are: `pnpm test` inside + // this package, `vitest run `, an editor runner, or an agent working + // in a tree built at an older commit — which are precisely the ways this pin + // gets re-run WHILE someone is changing one of the two routes. + // + // Array form with anchored patterns, deliberately: the object form matches + // by PREFIX, so a bare `@objectstack/service-datasource` key with a FILE + // replacement would also swallow `@objectstack/service-datasource/contracts` + // and resolve it to `…/src/index.ts/contracts` (ENOTDIR) at run time, in a + // config that looks right. Same shape as `service-storage`'s config (#7778). + alias: [ + { + find: /^@objectstack\/plugin-hono-server$/, + replacement: path.resolve(__dirname, '../plugins/plugin-hono-server/src/index.ts'), + }, + { + find: /^@objectstack\/service-datasource$/, + replacement: path.resolve(__dirname, '../services/service-datasource/src/index.ts'), + }, + ], + }, }); 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 544248ff3c..b56e36626b 100644 --- a/packages/services/service-datasource/src/__tests__/admin-routes.test.ts +++ b/packages/services/service-datasource/src/__tests__/admin-routes.test.ts @@ -77,7 +77,23 @@ describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => { const res = await app.fetch(json('/api/v1/datasources/demo_ext/remote-tables')); expect(res.status).toBe(200); expect(await res.json()).toEqual({ success: true, data: { tables: [{ name: 'customers', columnCount: 4 }] } }); - expect(listRemoteTables).toHaveBeenCalledWith('demo_ext'); + // No `?schema=` ⇒ the options bag carries no filter (#7955). The bag itself + // is always passed; `{ schema: undefined }` is what the service reads as + // "unfiltered", and it is the same value the federation twin hands it. + expect(listRemoteTables).toHaveBeenCalledWith('demo_ext', { schema: undefined }); + }); + + // The forwarding half of #7955 at THIS spelling. The cross-package half — + // that the two spellings answer the same SET — is + // `packages/rest/src/remote-tables-twin.equivalence.test.ts`, which has to + // live where both registrars are reachable; this case is what fails first if + // the query stops being read here at all. + it('GET /api/v1/datasources/:name/remote-tables forwards ?schema= to the service', async () => { + const listRemoteTables = vi.fn().mockResolvedValue([]); + const app = mount({ listRemoteTables }); + const res = await app.fetch(json('/api/v1/datasources/demo_ext/remote-tables?schema=public')); + expect(res.status).toBe(200); + expect(listRemoteTables).toHaveBeenCalledWith('demo_ext', { schema: 'public' }); }); it('POST /api/v1/datasources/:name/object-draft generates a draft (400 without table)', async () => { diff --git a/packages/services/service-datasource/src/admin-routes.ts b/packages/services/service-datasource/src/admin-routes.ts index b5db6ff3c0..55a028f80a 100644 --- a/packages/services/service-datasource/src/admin-routes.ts +++ b/packages/services/service-datasource/src/admin-routes.ts @@ -57,7 +57,7 @@ const SERVICE_ERROR_CODE: Record = { * * Served by `external-datasource`: * - * GET /datasources/:name/remote-tables → listRemoteTables + * GET /datasources/:name/remote-tables → listRemoteTables (?schema= filters) * POST /datasources/:name/test → testConnection (a SAVED datasource) * POST /datasources/:name/object-draft → generateObjectDraft * @@ -197,11 +197,28 @@ export function registerDatasourceAdminRoutes( // `POST /datasources/:name/object-draft` generates an ObjectStack object // definition draft for one table (introspect + type-map, no persistence — // the caller creates the object through the normal metadata channel). + // + // `?schema=` narrows the listing to one remote schema, and is forwarded here + // for the same reason #4249 gave the two spellings one FAILURE contract: they + // are one operation — `IExternalDatasourceService.listRemoteTables`, resolved + // from the same `external-datasource` slot — reached two ways. Until #7955 + // this handler never read the query, so `?schema=public` came back UNFILTERED: + // neither the filtered answer nor a refusal, which is the "declared ≠ + // enforced" shape (Prime Directive #10) in its quietest form — the twin one + // path over honoured the same parameter. The coercion below is copied from + // that twin (`packages/rest/src/external-datasource-routes.ts`) deliberately, + // down to what it does with a NON-string: a repeated `?schema=a&schema=b` + // reaches the handler as an array (the adapter surfaces repeated keys that + // way), and both spellings drop it to `undefined` — no filter. Whether an + // unusable query parameter should instead be REFUSED is the ingress-policy + // question #7606 owns globally; honouring it is correct under either answer, + // so this route does not pre-empt it. server.get(`${root}/:name/remote-tables`, async (req: any, res: any) => { const svc = resolve(res, 'external-datasource', 'listRemoteTables'); if (!svc) return; try { - const tables = await svc.listRemoteTables(req.params.name); + const schema = typeof req.query?.schema === 'string' ? req.query.schema : undefined; + const tables = await svc.listRemoteTables(req.params.name, { schema }); sendOk(res, { tables }); } catch (err) { badRequest(res, 'external-datasource', err); diff --git a/packages/services/service-datasource/src/datasource-route-ledger.ts b/packages/services/service-datasource/src/datasource-route-ledger.ts index b5b2cb0723..5276d4f70e 100644 --- a/packages/services/service-datasource/src/datasource-route-ledger.ts +++ b/packages/services/service-datasource/src/datasource-route-ledger.ts @@ -34,13 +34,17 @@ * the fix. It is not: the two spellings are different mounted routes, in * different packages, and BOTH are live. Two of them do overlap — * `GET /:name/remote-tables` here and `GET /:name/external/tables` there both - * reach `IExternalDatasourceService.listRemoteTables` (they diverge on `?schema=`, - * which only the federation twin forwards — #7955), as do + * reach `IExternalDatasourceService.listRemoteTables`, as do * `POST /:name/object-draft` and `POST /:name/external/tables/:remote/draft` * over `generateObjectDraft`. That overlap is known and was deliberately * reconciled rather than removed: #4249 gave the two paths ONE failure contract * ("One operation, one failure contract now, on both paths", - * `packages/rest/src/external-datasource-routes.ts`). A ledger describes what is + * `packages/rest/src/external-datasource-routes.ts`), and #7955 finished the + * other half of that same principle on the REQUEST path — the admin spelling + * had never read `?schema=`, so the filter the federation twin honoured came + * back silently unapplied here. Both spellings now forward it, with the same + * coercion, pinned across the two packages by + * `packages/rest/src/remote-tables-twin.equivalence.test.ts`. A ledger describes what is * mounted; renaming a live route to close a bookkeeping gap would be an API * break performed for the bookkeeping's benefit. So every row below carries the * spelling the mount actually uses, and the conformance test derives its @@ -143,7 +147,7 @@ export const DATASOURCE_ROUTE_LEDGER: readonly DatasourceRouteLedgerEntry[] = [ // Served by `external-datasource`, so a refusal is EXTERNAL_DATASOURCE_ERROR // and the 503 names that service rather than `datasource-admin` (#4225/#4249). { route: 'GET /api/v1/datasources/:name/remote-tables', family: 'datasource-introspection', disposition: 'server-only', - note: 'lists a datasource\'s remote tables. The #7744 row: this is the LIVE admin spelling, and it is a different mounted route from the federation twin `GET /:name/external/tables` in packages/rest, which the REST ledger carries as `datasources.external.listTables`. Both reach `listRemoteTables` and share one failure contract by design (#4249); only the federation twin forwards `?schema=`, and only it is SDK-expressed.' }, + note: 'lists a datasource\'s remote tables, optionally narrowed by `?schema=`. The #7744 row: this is the LIVE admin spelling, and it is a different mounted route from the federation twin `GET /:name/external/tables` in packages/rest, which the REST ledger carries as `datasources.external.listTables`. Both reach `listRemoteTables`, share one failure contract by design (#4249) and — since #7955 — one request shape; only the federation twin is SDK-expressed.' }, { route: 'POST /api/v1/datasources/:name/object-draft', family: 'datasource-introspection', disposition: 'server-only', note: 'generates an ObjectStack object draft for one remote table (introspect + type-map, no persistence). Federation twin: `POST /:name/external/tables/:remote/draft` — same `generateObjectDraft` operation, and the twin is the SDK-expressed one (`datasources.external.draft`).' }, { route: 'POST /api/v1/datasources/:name/test', family: 'datasource-introspection', disposition: 'server-only', diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0b934c6356..a66e8677ea 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1971,12 +1971,18 @@ importers: '@objectstack/objectql': specifier: workspace:* version: link:../objectql + '@objectstack/plugin-hono-server': + specifier: workspace:* + version: link:../plugins/plugin-hono-server '@objectstack/plugin-security': specifier: workspace:* version: link:../plugins/plugin-security '@objectstack/service-analytics': specifier: workspace:* version: link:../services/service-analytics + '@objectstack/service-datasource': + specifier: workspace:* + version: link:../services/service-datasource '@types/node': specifier: ^26.1.2 version: 26.1.2