Skip to content

Commit 477195c

Browse files
huangyiireneclaude
andauthored
fix(service-datasource): forward ?schema= on the admin remote-tables route (#7955) (#7998)
* fix(service-datasource): forward `?schema=` on the admin remote-tables route (#7955) `IExternalDatasourceService.listRemoteTables` is reachable through two live routes. The federation spelling — `GET /api/v1/datasources/:name/external/tables` in `packages/rest` — forwards `?schema=` to the service. The admin spelling, `GET /api/v1/datasources/:name/remote-tables`, never read `req.query` at all, so `?schema=public` came back as the UNFILTERED listing: not the filtered set, and not a refusal either — the "declared ≠ enforced" shape at its quietest, since the twin one path over honoured the same parameter. This finishes on the REQUEST path what #4249 did for the failure path ("one operation, one failure contract now, on both paths"). The two routes resolve the SAME `external-datasource` slot and call the SAME method with the same datasource name; the sibling operation `generateObjectDraft` already forwards its options bag on both spellings, so honouring the filter restores symmetry rather than inventing policy. 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 a handler as an array (the adapter surfaces repeated keys that way) and both spellings drop it to `undefined` — no filter. No refusal, warning or deprecation is added: whether an unusable query parameter should be REFUSED is the global ingress-policy question #7606 owns, and honouring the parameter is correct under either answer that card reaches. The invariant is pinned where both registrars are reachable — `packages/rest/src/remote-tables-twin.equivalence.test.ts` drives the same query at BOTH spellings, against one real `ExternalDatasourceService` on one real `HonoHttpServer`, and compares the answers. A test that exercised only the fixed route could not fail if the twins drift apart again. It covers the filtered set, the ABSENT parameter (still unfiltered, so an always-filtering fix cannot pass), a filter that matches nothing, an empty `?schema=`, and the repeated-key array. `@objectstack/rest` gains two dev-only workspace dependencies for it; its published surface is unchanged. `datasource-route-ledger.ts` recorded the divergence as live ("only the federation twin forwards `?schema=`") — that note now records the reconciliation. Fixes #7955 Co-authored-by: Claude <noreply@anthropic.com> * fix(rest): alias the twin test's sibling imports to source, and drop its fixture cast (#7955) Two repo-wide ratchets went red on the placement decision the PR flagged for review — the equivalence test living in `packages/rest`. Both are fixed where the gates prescribe; neither ledger is widened. `check-test-source-alias`: `remote-tables-twin.equivalence.test.ts` imports `@objectstack/service-datasource` and `@objectstack/plugin-hono-server` as VALUES, and unaliased both resolved through the workspace link to `dist/` — a build artifact. `packages/rest/vitest.config.ts` now aliases the two to their `src` entry points, anchored-regex array form so the prefix match cannot swallow a subpath (`@objectstack/service-datasource/contracts` → `…/src/index.ts/ contracts`, ENOTDIR). The hazard is specific here rather than generic. A cross-package equivalence pin exists to notice when one twin moves; resolved through a stale `service-datasource` dist it would report the PRE-fix admin route as agreeing with the federation route — the #7955 defect itself, passing green, with nothing in the output saying so. Re-ran the reverse-verification through the aliased (source) path to confirm the pin still bites: red 2/5 with `admin-routes.ts` at `origin/main`, green 5/5 with the fix — and now without a rebuild between them, because the verdict is about source. `check:type-check-debt --re-measure`: the test added one raw tsc error to `@objectstack/rest`'s hidden test layer (155 → 156). It was the fixture's `as IntrospectedSchema['tables']` assertion, which existed only because the column literals omitted the required `primaryKey`. Spelling the columns in full through a small `col()` helper removes the need for the cast. Measured back at 155 — the recorded number — so the ledger entry is untouched. New debt is not irreducible debt, and the ledger only shrinks (#5278). Behaviour, acceptance shape and the analysis are unchanged: both routes still driven by one test, the absent-parameter case retained, the federation coercion still mirrored, no refusal added, `external-datasource-routes.ts` untouched. Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7dc1067 commit 477195c

8 files changed

Lines changed: 347 additions & 7 deletions

File tree

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
"@objectstack/service-datasource": patch
3+
---
4+
5+
fix(service-datasource): the admin `remote-tables` route honours `?schema=` instead of dropping it (#7955)
6+
7+
`IExternalDatasourceService.listRemoteTables` is reachable through two live
8+
routes, and only one of them read the query:
9+
10+
- `GET /api/v1/datasources/:name/external/tables` (federation, `packages/rest`)
11+
forwards `?schema=` to the service.
12+
- `GET /api/v1/datasources/:name/remote-tables` (admin, this package) never
13+
touched `req.query`, so `?schema=public` came back as the UNFILTERED listing —
14+
not the filtered set, and not a refusal either.
15+
16+
Wire-visible change, on the admin spelling only: `?schema=<name>` now narrows the
17+
listing to that remote schema, exactly as the federation twin already did. A
18+
request with no `?schema=` is unchanged — it still returns the full listing, so
19+
every existing caller (none of which can have been passing the parameter
20+
meaningfully) sees the same bytes as before.
21+
22+
The coercion is copied from the federation route rather than reinvented, down to
23+
its treatment of a non-string: a repeated `?schema=a&schema=b` reaches the
24+
handler as an array and both spellings drop it to "no filter". No refusal, no
25+
warning, no deprecation is added here — whether an unusable query parameter
26+
should be REFUSED is the global ingress-policy question tracked by #7606, and
27+
honouring the parameter is correct under either answer it reaches, so the twins
28+
can move together then.
29+
30+
This finishes on the REQUEST path what #4249 did for the failure path ("one
31+
operation, one failure contract now, on both paths"). The equivalence is pinned
32+
across the two packages by
33+
`packages/rest/src/remote-tables-twin.equivalence.test.ts`, which drives the same
34+
query at BOTH spellings against one service and compares the answers — a test
35+
that exercised only the fixed route could not fail if the twins drift apart
36+
again. `@objectstack/rest` gains two dev-only workspace dependencies so that test
37+
can mount both registrars; its published surface is unchanged.

packages/rest/package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,10 @@
3535
"@objectstack/metadata": "workspace:*",
3636
"@objectstack/metadata-protocol": "workspace:*",
3737
"@objectstack/objectql": "workspace:*",
38+
"@objectstack/plugin-hono-server": "workspace:*",
3839
"@objectstack/plugin-security": "workspace:*",
3940
"@objectstack/service-analytics": "workspace:*",
41+
"@objectstack/service-datasource": "workspace:*",
4042
"@types/node": "^26.1.2",
4143
"typescript": "^6.0.3",
4244
"vitest": "^4.1.10"
Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `listRemoteTables` REQUEST-shape equivalence across its two live spellings
5+
* (#7955).
6+
*
7+
* `IExternalDatasourceService.listRemoteTables` is reachable through two
8+
* mounted routes, in two packages:
9+
*
10+
* GET /api/v1/datasources/:name/external/tables ← this package, federation
11+
* GET /api/v1/datasources/:name/remote-tables ← @objectstack/service-datasource, admin
12+
*
13+
* They are not near-duplicates that happen to look alike: both resolve the SAME
14+
* `external-datasource` service slot and call the SAME method with the same
15+
* datasource name. #4249 already reconciled what happens when that one call
16+
* THROWS ("One operation, one failure contract now, on both paths",
17+
* `external-datasource-routes.ts`). What was never compared is the other half —
18+
* what the two paths do with the REQUEST — and #7955 is the residue: the admin
19+
* spelling never read `req.query`, so `?schema=public` came back as the
20+
* UNFILTERED listing there and the filtered one here. Not an error, not the
21+
* filter: the quietest form of "declared ≠ enforced".
22+
*
23+
* ## Why this test is here, and why it drives BOTH routes
24+
*
25+
* A test that exercised only the fixed route could not fail if the twins drift
26+
* apart again — it would pin one path's behaviour and say nothing about the
27+
* relationship, which IS the invariant. So every case below issues the same
28+
* query to both spellings and asserts the two answers are equal, against ONE
29+
* service instance mounted on ONE server: the difference between the readings
30+
* can then only come from the two handlers.
31+
*
32+
* It lives in `packages/rest` because that is the side that can reach both
33+
* halves without widening anyone's public API: `registerDatasourceAdminRoutes`
34+
* is exported from `@objectstack/service-datasource`'s index, while
35+
* `registerExternalDatasourceRoutes` is deliberately internal here (it is
36+
* composed by `rest-api-plugin.ts`, not published). The dependency is dev-only
37+
* and points rest → service-datasource, which is not a cycle: that package
38+
* depends on `core`/`spec`/`types` and never on this one. Same reasoning the
39+
* client-side ledger guard used when it chose its side (`service-route-ledger-
40+
* coverage.test.ts`, "a service→client package edge would be backwards").
41+
*
42+
* The service under the routes is the REAL `ExternalDatasourceService` over a
43+
* fake introspector, not a `vi.fn()` recording its arguments. A mock would only
44+
* prove the admin handler now passes an options bag; what the card asks is that
45+
* the two routes return the same SET, which takes the real filter.
46+
*
47+
* Driven through the real `HonoHttpServer` — the adapter `os serve` mounts — so
48+
* `?schema=` is parsed by the code that parses it in production. That matters
49+
* for the last case: a repeated key reaches a handler as an ARRAY, and only a
50+
* real adapter produces one.
51+
*/
52+
53+
import { describe, it, expect } from 'vitest';
54+
import { HonoHttpServer } from '@objectstack/plugin-hono-server';
55+
import {
56+
ExternalDatasourceService,
57+
registerDatasourceAdminRoutes,
58+
} from '@objectstack/service-datasource';
59+
import type { IntrospectedColumn, IntrospectedSchema } from '@objectstack/spec/contracts';
60+
import { registerExternalDatasourceRoutes } from './external-datasource-routes.js';
61+
62+
const DS = 'demo_ext';
63+
64+
/** One remote column, spelled in full so the fixture needs no cast to be an
65+
* `IntrospectedSchema` — `primaryKey` and `nullable` are both required. */
66+
const col = (name: string, primaryKey = false): IntrospectedColumn => ({
67+
name,
68+
type: name === 'id' ? 'uuid' : 'text',
69+
nullable: !primaryKey,
70+
primaryKey,
71+
});
72+
73+
/** Two remote schemas, so a `?schema=` filter has something to exclude. */
74+
const REMOTE: IntrospectedSchema = {
75+
dialect: 'postgres',
76+
introspectedAt: '2026-08-12T00:00:00.000Z',
77+
tables: {
78+
'public.customers': {
79+
name: 'public.customers',
80+
columns: [col('id', true), col('email')],
81+
indexes: [],
82+
},
83+
'public.orders': {
84+
name: 'public.orders',
85+
columns: [col('id', true)],
86+
indexes: [],
87+
},
88+
'analytics.events': {
89+
name: 'analytics.events',
90+
columns: [col('id', true)],
91+
indexes: [],
92+
},
93+
},
94+
};
95+
96+
/**
97+
* One server, one service, both registrars — the point of the fixture.
98+
*
99+
* `registerDatasourceAdminRoutes` also mounts the datasource-lifecycle family,
100+
* whose services are absent here; those routes answer 503 and are simply never
101+
* driven. The two paths under test both resolve `external-datasource`, which is
102+
* the one service wired.
103+
*/
104+
function mountBoth() {
105+
const service = new ExternalDatasourceService({
106+
introspect: async () => REMOTE,
107+
getDatasource: async (name: string) => ({ name }),
108+
getObject: async () => undefined,
109+
listObjects: async () => [],
110+
});
111+
const server = new HonoHttpServer(0);
112+
const ctx = {
113+
getService: (name: string) => {
114+
if (name === 'external-datasource') return service;
115+
throw new Error(`no service: ${name}`);
116+
},
117+
} as any;
118+
registerExternalDatasourceRoutes(server, ctx, '/api/v1');
119+
registerDatasourceAdminRoutes(server, ctx, '/api/v1');
120+
return server.getRawApp();
121+
}
122+
123+
/** The two wire spellings of the one operation, keyed by how they are named. */
124+
const SPELLING = {
125+
federation: (qs: string) => `/api/v1/datasources/${DS}/external/tables${qs}`,
126+
admin: (qs: string) => `/api/v1/datasources/${DS}/remote-tables${qs}`,
127+
} as const;
128+
129+
interface Reading {
130+
status: number;
131+
tables: Array<{ schema?: string; name: string }>;
132+
}
133+
134+
/** Drive one spelling and read back the table set it answers with. */
135+
async function read(app: any, spelling: keyof typeof SPELLING, qs: string): Promise<Reading> {
136+
const res = await app.fetch(new Request(`http://local${SPELLING[spelling](qs)}`));
137+
const body = (await res.json()) as { success: boolean; data?: { tables?: Reading['tables'] } };
138+
return { status: res.status, tables: body.data?.tables ?? [] };
139+
}
140+
141+
/** Both spellings, same query — the comparison every case makes. */
142+
async function readBoth(qs: string): Promise<{ federation: Reading; admin: Reading }> {
143+
const app = mountBoth();
144+
return {
145+
federation: await read(app, 'federation', qs),
146+
admin: await read(app, 'admin', qs),
147+
};
148+
}
149+
150+
const qualified = (r: Reading) => r.tables.map((t) => `${t.schema}.${t.name}`).sort();
151+
152+
describe('listRemoteTables twins agree on the request shape (#7955)', () => {
153+
it('?schema= returns the SAME filtered set on both spellings', async () => {
154+
const { federation, admin } = await readBoth('?schema=public');
155+
156+
expect(federation.status).toBe(200);
157+
expect(admin.status).toBe(200);
158+
// The filter really filtered — otherwise "equal" could mean "both unfiltered",
159+
// which is precisely the pre-#7955 reading on one of the two paths.
160+
expect(qualified(federation)).toEqual(['public.customers', 'public.orders']);
161+
expect(qualified(admin)).toEqual(qualified(federation));
162+
});
163+
164+
it('no ?schema= returns the SAME unfiltered set on both spellings', async () => {
165+
const { federation, admin } = await readBoth('');
166+
167+
expect(qualified(federation)).toEqual([
168+
'analytics.events',
169+
'public.customers',
170+
'public.orders',
171+
]);
172+
// The absent-parameter arm is not a formality: a fix that filtered
173+
// unconditionally would satisfy the case above and break every existing
174+
// caller of the admin spelling, which has never passed one.
175+
expect(qualified(admin)).toEqual(qualified(federation));
176+
});
177+
178+
it('a ?schema= that matches nothing returns the SAME empty set on both spellings', async () => {
179+
const { federation, admin } = await readBoth('?schema=nonexistent');
180+
181+
expect(federation.tables).toEqual([]);
182+
expect(admin.tables).toEqual([]);
183+
expect(admin.status).toBe(federation.status);
184+
});
185+
186+
it('a repeated ?schema= degrades to no filter on both spellings, identically', async () => {
187+
// The adapter surfaces a repeated key as an array; `typeof … === 'string'`
188+
// is what both handlers do with it, so both fall back to the unfiltered
189+
// listing rather than filtering by an arbitrary one of the two. Whether such
190+
// a request should be REFUSED instead is #7606's global ingress question —
191+
// this case pins that the twins answer it the SAME way today, whatever that
192+
// card decides tomorrow.
193+
const { federation, admin } = await readBoth('?schema=public&schema=analytics');
194+
195+
expect(qualified(federation)).toEqual([
196+
'analytics.events',
197+
'public.customers',
198+
'public.orders',
199+
]);
200+
expect(qualified(admin)).toEqual(qualified(federation));
201+
expect(admin.status).toBe(federation.status);
202+
});
203+
204+
it('an empty ?schema= is no filter on both spellings, identically', async () => {
205+
// `?schema=` parses to the empty string, which is a string — so the
206+
// coercion keeps it and the service's own `opts?.schema &&` guard is what
207+
// treats it as "no filter". Both spellings inherit that from the one
208+
// service, and this pins that neither route second-guesses it.
209+
const { federation, admin } = await readBoth('?schema=');
210+
211+
expect(qualified(federation)).toEqual([
212+
'analytics.events',
213+
'public.customers',
214+
'public.orders',
215+
]);
216+
expect(qualified(admin)).toEqual(qualified(federation));
217+
});
218+
});

packages/rest/vitest.config.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,50 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import { defineConfig } from 'vitest/config';
4+
import path from 'path';
45

56
export default defineConfig({
67
test: {
78
globals: true,
89
environment: 'node',
910
},
11+
resolve: {
12+
// Both entries exist for `remote-tables-twin.equivalence.test.ts` (#7955),
13+
// the one suite here that imports sibling packages as VALUES: it mounts the
14+
// `service-datasource` admin registrar next to this package's federation
15+
// registrar to pin that the two `listRemoteTables` spellings answer `?schema=`
16+
// identically, and drives both through the real Hono adapter.
17+
//
18+
// Unaliased, those two specifiers resolve through the workspace link to
19+
// `dist/` — a BUILD ARTIFACT — which makes this suite's verdict a function of
20+
// build state rather than of the source in the checkout. The loud failure
21+
// (missing export) is the mild half; a dist merely BEHIND lets the test run
22+
// GREEN against the dependency's old behaviour, and nothing in the output
23+
// says so. That is exactly the hazard for a CROSS-PACKAGE equivalence pin:
24+
// its whole job is to notice when one of the two twins moves, and a stale
25+
// `service-datasource` dist would report the pre-fix admin route as agreeing
26+
// with the federation one — the #7955 defect itself, passing.
27+
//
28+
// Turbo already orders `test` after `^build`, so `turbo run test` was never
29+
// the failing path. The paths it does not mediate are: `pnpm test` inside
30+
// this package, `vitest run <file>`, an editor runner, or an agent working
31+
// in a tree built at an older commit — which are precisely the ways this pin
32+
// gets re-run WHILE someone is changing one of the two routes.
33+
//
34+
// Array form with anchored patterns, deliberately: the object form matches
35+
// by PREFIX, so a bare `@objectstack/service-datasource` key with a FILE
36+
// replacement would also swallow `@objectstack/service-datasource/contracts`
37+
// and resolve it to `…/src/index.ts/contracts` (ENOTDIR) at run time, in a
38+
// config that looks right. Same shape as `service-storage`'s config (#7778).
39+
alias: [
40+
{
41+
find: /^@objectstack\/plugin-hono-server$/,
42+
replacement: path.resolve(__dirname, '../plugins/plugin-hono-server/src/index.ts'),
43+
},
44+
{
45+
find: /^@objectstack\/service-datasource$/,
46+
replacement: path.resolve(__dirname, '../services/service-datasource/src/index.ts'),
47+
},
48+
],
49+
},
1050
});

packages/services/service-datasource/src/__tests__/admin-routes.test.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,23 @@ describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => {
7777
const res = await app.fetch(json('/api/v1/datasources/demo_ext/remote-tables'));
7878
expect(res.status).toBe(200);
7979
expect(await res.json()).toEqual({ success: true, data: { tables: [{ name: 'customers', columnCount: 4 }] } });
80-
expect(listRemoteTables).toHaveBeenCalledWith('demo_ext');
80+
// No `?schema=` ⇒ the options bag carries no filter (#7955). The bag itself
81+
// is always passed; `{ schema: undefined }` is what the service reads as
82+
// "unfiltered", and it is the same value the federation twin hands it.
83+
expect(listRemoteTables).toHaveBeenCalledWith('demo_ext', { schema: undefined });
84+
});
85+
86+
// The forwarding half of #7955 at THIS spelling. The cross-package half —
87+
// that the two spellings answer the same SET — is
88+
// `packages/rest/src/remote-tables-twin.equivalence.test.ts`, which has to
89+
// live where both registrars are reachable; this case is what fails first if
90+
// the query stops being read here at all.
91+
it('GET /api/v1/datasources/:name/remote-tables forwards ?schema= to the service', async () => {
92+
const listRemoteTables = vi.fn().mockResolvedValue([]);
93+
const app = mount({ listRemoteTables });
94+
const res = await app.fetch(json('/api/v1/datasources/demo_ext/remote-tables?schema=public'));
95+
expect(res.status).toBe(200);
96+
expect(listRemoteTables).toHaveBeenCalledWith('demo_ext', { schema: 'public' });
8197
});
8298

8399
it('POST /api/v1/datasources/:name/object-draft generates a draft (400 without table)', async () => {

packages/services/service-datasource/src/admin-routes.ts

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ const SERVICE_ERROR_CODE: Record<ServiceName, ErrorCode> = {
5757
*
5858
* Served by `external-datasource`:
5959
*
60-
* GET /datasources/:name/remote-tables → listRemoteTables
60+
* GET /datasources/:name/remote-tables → listRemoteTables (?schema= filters)
6161
* POST /datasources/:name/test → testConnection (a SAVED datasource)
6262
* POST /datasources/:name/object-draft → generateObjectDraft
6363
*
@@ -197,11 +197,28 @@ export function registerDatasourceAdminRoutes(
197197
// `POST /datasources/:name/object-draft` generates an ObjectStack object
198198
// definition draft for one table (introspect + type-map, no persistence —
199199
// the caller creates the object through the normal metadata channel).
200+
//
201+
// `?schema=` narrows the listing to one remote schema, and is forwarded here
202+
// for the same reason #4249 gave the two spellings one FAILURE contract: they
203+
// are one operation — `IExternalDatasourceService.listRemoteTables`, resolved
204+
// from the same `external-datasource` slot — reached two ways. Until #7955
205+
// this handler never read the query, so `?schema=public` came back UNFILTERED:
206+
// neither the filtered answer nor a refusal, which is the "declared ≠
207+
// enforced" shape (Prime Directive #10) in its quietest form — the twin one
208+
// path over honoured the same parameter. The coercion below is copied from
209+
// that twin (`packages/rest/src/external-datasource-routes.ts`) deliberately,
210+
// down to what it does with a NON-string: a repeated `?schema=a&schema=b`
211+
// reaches the handler as an array (the adapter surfaces repeated keys that
212+
// way), and both spellings drop it to `undefined` — no filter. Whether an
213+
// unusable query parameter should instead be REFUSED is the ingress-policy
214+
// question #7606 owns globally; honouring it is correct under either answer,
215+
// so this route does not pre-empt it.
200216
server.get(`${root}/:name/remote-tables`, async (req: any, res: any) => {
201217
const svc = resolve(res, 'external-datasource', 'listRemoteTables');
202218
if (!svc) return;
203219
try {
204-
const tables = await svc.listRemoteTables(req.params.name);
220+
const schema = typeof req.query?.schema === 'string' ? req.query.schema : undefined;
221+
const tables = await svc.listRemoteTables(req.params.name, { schema });
205222
sendOk(res, { tables });
206223
} catch (err) {
207224
badRequest(res, 'external-datasource', err);

0 commit comments

Comments
 (0)