diff --git a/.changeset/datasource-validate-scoped-to-url.md b/.changeset/datasource-validate-scoped-to-url.md new file mode 100644 index 0000000000..3b3c26b4e3 --- /dev/null +++ b/.changeset/datasource-validate-scoped-to-url.md @@ -0,0 +1,56 @@ +--- +"@objectstack/rest": patch +"@objectstack/service-datasource": patch +--- + +fix(rest): `POST /datasources/:name/external/validate` does URL-scoped work (#10537) + +The route asked the `external-datasource` service for `validateAll()` — every +federated object on every federated datasource, each validation driving a live +`introspect(datasource)` remote-schema read — and then kept only the rows whose +`datasource` matched the URL. The rows it kept were correct; the *work* was not +scoped, so one datasource's health check paid for N datasources' remote +round-trips and threw most of the measurement away. An unreachable *unrelated* +remote slowed the answer for the datasource actually asked about (and produced +rows that were then filtered off). + +Measured at the branch point, through the real Hono adapter and the real +`ExternalDatasourceService` over a recording introspector: a request for one of +three federated datasources introspected `['wh_a', 'wh_b', 'wh_c']`. A request +naming a datasource that does not exist introspected all three as well, to +answer the empty report it already answered. + +`ExternalDatasourceService` now carries `validateDatasource(datasource)`, the +scoped twin of the sweep composed from the same primitives (`listObjects` → +filter → `validateObject`) and the same per-object catch, and the route calls +it. Same request answers `['wh_a']`; an unknown name answers `[]`. + +**No response change.** The rows the post-filter used to keep are the rows the +scoped composition returns — same objects, same diffs, same `data.ok` verdict, +same `200`, the same `400 EXTERNAL_DATASOURCE_ERROR` when the service refuses, +the same `503 SERVICE_UNAVAILABLE` when federation is not wired in, and an +unknown `:name` still answers an empty, vacuously `ok` report rather than a +`404`. The selection is keyed on `o.datasource ?? 'default'`, which is exactly +the value `validateObject` reports back as `result.datasource`, so "the rows the +sweep would have kept" and "the objects this selects" are the same set — pinned +directly, in both packages, by comparing the scoped answer against the +sweep-then-filter answer rather than against a remembered body. + +Because the output was already right, the pins that matter here are about the +CALL RECORD, not the body: `external-datasource-validate-scope.test.ts` asserts +which datasources were introspected and that `validateAll()` is not called at +all, over a fixture carrying three federated datasources so the assertion can +actually fail. A body-only test passes on both sides of this change. + +`validateDatasource` is **not** on `IExternalDatasourceService`: the contract +offers `validateObject(objectName)` and `validateAll()`, and adding a +per-datasource spelling to it is a spec-surface decision to take on its own +terms. The composition therefore lives in the service — the only registrant of +the `external-datasource` slot — and the REST registrar probes for it. A wired +service with no scoped spelling takes the same `503` arm every other route in +this family takes when the service cannot serve it, deliberately *not* a silent +fallback to the fan-out: a fallback would leave the old behaviour reachable on a +path no test drives. + +Unchanged: `validateAll()` itself, and the boot-validation sweep in +`packages/runtime` that legitimately validates every federated object. diff --git a/packages/rest/src/external-datasource-envelope.conformance.test.ts b/packages/rest/src/external-datasource-envelope.conformance.test.ts index cc3736574d..6d94bc83b5 100644 --- a/packages/rest/src/external-datasource-envelope.conformance.test.ts +++ b/packages/rest/src/external-datasource-envelope.conformance.test.ts @@ -149,7 +149,7 @@ describe('external-datasource envelope (#3843) — success bodies', () => { status: 200, dataKeys: ['ok', 'results'], run: () => drive( - mount({ validateAll: async () => ({ results: [{ datasource: 'ext', ok: true }] }) }), + mount({ validateDatasource: async () => ({ results: [{ datasource: 'ext', ok: true }] }) }), 'POST', `${EXT}/validate`, ), @@ -192,7 +192,7 @@ describe('external-datasource envelope (#3843) — success bodies', () => { it("POST /validate keeps its `ok` — a domain verdict, not a second `success`", async () => { // All results valid → data.ok true, while `success` reports the request. const pass = await drive( - mount({ validateAll: async () => ({ results: [{ datasource: 'ext', ok: true }] }) }), + mount({ validateDatasource: async () => ({ results: [{ datasource: 'ext', ok: true }] }) }), 'POST', `${EXT}/validate`, ); @@ -204,7 +204,7 @@ describe('external-datasource envelope (#3843) — success bodies', () => { // `success` the way storage's was. const fail = await drive( mount({ - validateAll: async () => ({ + validateDatasource: async () => ({ results: [{ datasource: 'ext', ok: true }, { datasource: 'ext', ok: false }], }), }), @@ -279,7 +279,7 @@ describe('external-datasource envelope (#3843) — error bodies', () => { status: 400, code: 'EXTERNAL_DATASOURCE_ERROR', run: () => drive( - mount({ validateAll: async () => { throw new Error('metadata store offline'); } }), + mount({ validateDatasource: async () => { throw new Error('metadata store offline'); } }), 'POST', `${EXT}/validate`, ), diff --git a/packages/rest/src/external-datasource-routes-auth-guard.test.ts b/packages/rest/src/external-datasource-routes-auth-guard.test.ts index 24435da5b8..c15d83cb07 100644 --- a/packages/rest/src/external-datasource-routes-auth-guard.test.ts +++ b/packages/rest/src/external-datasource-routes-auth-guard.test.ts @@ -61,7 +61,7 @@ * authentication floor — pinned here as an explicit `capability: null` row so * that gating it later had to change the table. That later card is #10255, * ruled 2026-08-20 (verbatim: 「同意你的意见。」, accepting option A): validate - * takes the READ capability, because `validateAll` drives the same live + * takes the READ capability, because validation drives the same live * remote-schema introspection the read twins gate and reports on it. The row * now carries `READ_CAPABILITY`, and its case below flips from "still served * holding nothing" to "refused holding nothing" — deliberately, not by a @@ -106,8 +106,13 @@ type Handler = (req: any, res: any) => any; * /external/validate` carried one — spelled as an explicit `null` rather than * omitted, so that a later edit gating it had to change this table — and the * 2026-08-20 #10255 ruling is that later edit: validate is a read - * (`validateAll` drives the same live remote introspection the read twins + * (validation drives the same live remote introspection the read twins * gate), so its row now carries `READ_CAPABILITY` like its two read siblings. + * + * [#10537] The validate row's `call` is the SCOPED composition + * (`validateDatasource`), which is what the route dispatches to since the + * fan-out fix; the fixture keeps a `validateAll` spy beside it precisely so a + * regression to the whole-farm sweep is visible here rather than silent. */ const READ_CAPABILITY = 'manage_platform_settings'; const WRITE_CAPABILITY = 'manage_metadata'; @@ -117,7 +122,7 @@ const FAMILY = [ { method: 'POST', url: `${BASE}/datasources/${DS}/external/tables/customers/draft`, ok: 200, call: 'generateObjectDraft', writes: false, capability: READ_CAPABILITY }, { method: 'POST', url: `${BASE}/datasources/${DS}/external/tables/customers/import`, ok: 201, call: 'importObject', writes: true, capability: WRITE_CAPABILITY }, { method: 'POST', url: `${BASE}/datasources/${DS}/external/refresh-catalog`, ok: 200, call: 'refreshCatalog', writes: true, capability: WRITE_CAPABILITY }, - { method: 'POST', url: `${BASE}/datasources/${DS}/external/validate`, ok: 200, call: 'validateAll', writes: false, capability: READ_CAPABILITY }, + { method: 'POST', url: `${BASE}/datasources/${DS}/external/validate`, ok: 200, call: 'validateDatasource', writes: false, capability: READ_CAPABILITY }, ] as const; /** Every capability an entitled caller needs to clear all five routes. */ @@ -174,7 +179,13 @@ function federationServiceSpies() { generateObjectDraft: vi.fn(async () => ({ name: 'customers' })), importObject: vi.fn(async () => ({ name: 'customers' })), refreshCatalog: vi.fn(async () => ({ tables: {} })), + // [#10537] `POST /external/validate` dispatches to the SCOPED composition + // now. The whole-farm `validateAll` stays in the set, spied and never + // expected to run: "the service never ran" then means the method the + // route actually reaches, and a regression to the sweep shows up as a + // call on a spy nobody expects rather than as silence. validateAll: vi.fn(async () => ({ results: [{ datasource: DS, ok: true }] })), + validateDatasource: vi.fn(async (name: string) => ({ results: [{ datasource: name, ok: true }] })), }; } @@ -475,14 +486,14 @@ describe('[#9901] the family requires a capability above authentication', () => // 2026-08-20 #10255 ruling it asserted the exact opposite — an // authenticated caller holding nothing was SERVED here while refused the // other four — because #9901's ruling did not name this route. The ruling - // that changed it is recorded on #10255 (option A): `validateAll` drives + // that changed it is recorded on #10255 (option A): validation drives // the same live remote-schema introspection the read twins gate, so // validate is a read and answers to the read capability. const { table, service } = await bootFederation({ withAuth: true, withEngine: true, grants: [], }); - const validate = FAMILY.find((r) => r.call === 'validateAll')!; + const validate = FAMILY.find((r) => r.call === 'validateDatasource')!; const { statusCode, body } = await call(table, validate, { authorization: `Bearer ${SESSION}` }); // Refused with the capability NAMED — the one thing the refused caller can @@ -492,6 +503,7 @@ describe('[#9901] the family requires a capability above authentication', () => expect(body?.success).toBe(false); expect(body?.error?.code).toBe('PERMISSION_DENIED'); expect(body?.error?.message).toContain(READ_CAPABILITY); + expect(service.validateDatasource).not.toHaveBeenCalled(); expect(service.validateAll).not.toHaveBeenCalled(); }); @@ -503,12 +515,14 @@ describe('[#9901] the family requires a capability above authentication', () => withAuth: true, withEngine: true, grants: [READ_CAPABILITY], }); - const validate = FAMILY.find((r) => r.call === 'validateAll')!; + const validate = FAMILY.find((r) => r.call === 'validateDatasource')!; const { statusCode, body } = await call(table, validate, { authorization: `Bearer ${SESSION}` }); expect(statusCode).toBe(validate.ok); expect(body?.success).toBe(true); - expect(service.validateAll).toHaveBeenCalled(); + expect(service.validateDatasource).toHaveBeenCalledWith(DS); + // [#10537] …and the served request did NOT fan out across every datasource. + expect(service.validateAll).not.toHaveBeenCalled(); }); it('a capability the caller does not hold is not granted by an api key either', async () => { diff --git a/packages/rest/src/external-datasource-routes.ts b/packages/rest/src/external-datasource-routes.ts index d42a98ec0c..fcf23f9081 100644 --- a/packages/rest/src/external-datasource-routes.ts +++ b/packages/rest/src/external-datasource-routes.ts @@ -10,7 +10,11 @@ import { ANONYMOUS_DENY_MESSAGE, type PluginContext, } from '@objectstack/core'; -import type { IExternalDatasourceService, IHttpServer } from '@objectstack/spec/contracts'; +import type { + IExternalDatasourceService, + IHttpServer, + SchemaValidationReport, +} from '@objectstack/spec/contracts'; // The declared envelope is written in ONE place for the whole platform (#3973). import { sendOk, sendError } from '@objectstack/types'; import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; @@ -27,7 +31,7 @@ import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; * POST /datasources/:name/external/tables/:remote/draft → generateObjectDraft * POST /datasources/:name/external/tables/:remote/import → importObject * POST /datasources/:name/external/refresh-catalog → refreshCatalog - * POST /datasources/:name/external/validate → validateAll (this ds) + * POST /datasources/:name/external/validate → validateDatasource(:name) * * NOTE: the datasource *lifecycle* routes (`/api/v1/datasources` — * list / test / create / update / remove, ADR-0015 Addendum) were extracted @@ -68,6 +72,17 @@ import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; * (`results.every(r => r.ok)`). A domain verdict that happens to share the name * is not a second envelope flag, so it belongs inside `data` rather than being * dropped. + * + * [#10537] `POST /validate` does URL-SCOPED WORK. It used to call + * `validateAll()` — every federated object on every federated datasource, each + * validation driving a live `introspect(datasource)` — and then keep only the + * rows matching `:name`. The rows were right; the work was not scoped, so one + * datasource's health check paid for N datasources' remote round-trips and an + * unreachable *unrelated* remote slowed the answer for the datasource actually + * asked about (measured at head: a request for one of three federated + * datasources introspected all three). It now calls the service's scoped + * composition — see {@link scopedValidation} for what that is and why it is + * probed rather than declared on `IExternalDatasourceService`. */ export interface ExternalDatasourceRoutesOptions { /** @@ -128,12 +143,16 @@ export interface ExternalDatasourceRoutesOptions { * the #9686 authentication floor and filed the question instead of deciding * it. The follow-up ruling (maintainer, 2026-08-20, verbatim: * 「同意你的意见。」, accepting option A on #10255) converged it here: what - * `validateAll` does is drive the SAME live remote-schema introspection the + * validation does is drive the SAME live remote-schema introspection the * two read twins gate (`introspect` per datasource, in * `service-datasource/src/external-datasource-service.ts`), and its report — * schema diffs naming remote columns and types, driver error strings for * unreachable remotes — is a read of the same federation surface. One family, * one door-type: reads here, writes on {@link FEDERATION_WRITE_CAPABILITY}. + * + * [#10537] The reasoning is unchanged by the scoping fix and was never about + * the sweep's WIDTH: it is the same introspection whether one datasource is + * read or all of them, so `validate` answers to the read capability either way. */ export const FEDERATION_READ_CAPABILITY = 'manage_platform_settings'; @@ -321,6 +340,45 @@ export function registerExternalDatasourceRoutes( } }; + /** + * [#10537] The scoped validation the `POST /validate` route needs: validate + * the federated objects bound to ONE datasource, composed service-side from + * the same primitives the whole-farm sweep uses (`listObjects` → filter → + * `validateObject`). + * + * ## Why it is PROBED rather than read off the contract + * + * `IExternalDatasourceService` declares `validateObject(objectName)` and + * `validateAll()` and no per-datasource spelling. Adding one is a + * spec-surface change that has to be decided on its own terms, so this fix + * takes the other authorized shape: the composition lives in the service + * (`ExternalDatasourceService.validateDatasource`, the only registrant of + * this slot) and the route probes for it. Everything about the ANSWER is + * still contract-typed — {@link SchemaValidationReport} is the contract's + * own report type — so what is asserted here rather than checked is the + * method's name, and nothing about the shape it returns. + * + * ## Absence answers 503, deliberately not a fan-out fallback + * + * A wired service with no scoped spelling could be served by falling back to + * `validateAll()` and post-filtering — which is precisely the behaviour + * #10537 removed. A silent fallback would leave the fan-out reachable, on a + * path no test drives, for exactly the deployments nobody is looking at. So + * absence takes the same 503 arm every other route here takes when the + * service cannot serve it: loud, and already the declared shape of "this + * deployment's federation service cannot do this". + */ + interface ScopedValidation { + validateDatasource(datasource: string): Promise; + } + + const scopedValidation = (): ScopedValidation | undefined => { + const svc = externalService() as + | (IExternalDatasourceService & Partial) + | undefined; + return typeof svc?.validateDatasource === 'function' ? (svc as ScopedValidation) : undefined; + }; + const unavailable = (res: any) => sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The external-datasource service is not available.'); @@ -437,20 +495,29 @@ export function registerExternalDatasourceRoutes( }, // Validate the federated objects on this datasource. [#10255] A 'read': - // validateAll drives the same live remote-schema introspection the two + // validation drives the same live remote-schema introspection the two // read twins gate, so it answers to the same capability (ruled 2026-08-20; // the constant's doc carries the reasoning). + // + // [#10537] The work is scoped by the CALL, not by a filter over a + // whole-farm sweep: `validateDatasource(:name)` introspects the named + // datasource's remote and no other. The response is unchanged — the rows + // 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). { method: 'POST', path: `${ext}/validate`, metadata: { summary: 'Validate the federated objects on a datasource', tags: ['datasources'] }, handler: async (req: any, res: any) => { if (await refuseFederationRequest(req, res, 'read')) return; - const svc = externalService(); - if (!svc?.validateAll) return unavailable(res); + const scoped = scopedValidation(); + if (!scoped) return unavailable(res); try { - const report = await svc.validateAll(); - const results = (report.results ?? []).filter((r) => r.datasource === req.params.name); + const report = await scoped.validateDatasource(req.params.name); + const results = report.results ?? []; + // The domain verdict stays this route's own computation over the rows + // it answers with — the same expression as before the scoping fix. sendOk(res, { ok: results.every((r) => r.ok), results }); } catch (err) { refused(res, err); diff --git a/packages/rest/src/external-datasource-validate-scope.test.ts b/packages/rest/src/external-datasource-validate-scope.test.ts new file mode 100644 index 0000000000..56d9f499dc --- /dev/null +++ b/packages/rest/src/external-datasource-validate-scope.test.ts @@ -0,0 +1,256 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10537] `POST /datasources/:name/external/validate` does URL-SCOPED WORK. + * + * ## The defect this file measures + * + * The route used to call `validateAll()` — every federated object on every + * federated datasource, each validation driving a LIVE remote-schema + * `introspect(datasource)` — and then keep only the rows whose `datasource` + * matched `:name`. The answer was correct; the WORK was not scoped. One + * datasource's health check paid for N datasources' remote round-trips, and an + * unreachable *unrelated* remote slowed (and added rows to) a request that was + * about to discard them. + * + * ## Why every assertion here is about the CALL RECORD, not the body + * + * The output was already right, so a test that only compared response bodies + * would have passed just as well BEFORE the fix — a vacuous pin on a + * cost/shape defect. The load-bearing assertions are therefore: + * + * - which datasources were INTROSPECTED (`introspected`), and + * - that `validateAll()` was not called at all (`validateAllSpy`). + * + * The fixture carries {@link DATASOURCES}.length = 3 federated datasources on + * purpose: with one, "introspected 1" and "introspected all" are the same + * reading and nothing here could fail. The body IS pinned too — against the + * pre-fix composition computed live from a second service instance + * (`referenceAnswer`), so "same answer, less work" is asserted as one claim + * rather than assumed. + * + * ## The service under the route is the REAL one + * + * `ExternalDatasourceService` over a fake introspector, not a hand-written + * stand-in whose `validateAll` fans out because this file wrote it that way — + * the fan-out being measured is the production composition's. The specifier is + * aliased to that package's `src/` by this package's `vitest.config.ts` (the + * alias predates this file; see its comment), so the reading is a function of + * the checkout rather than of `dist/` build state. + * + * Driven through the real `HonoHttpServer` — the adapter `os serve` mounts — so + * `:name` is parsed from the URL by the code that parses it in production. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { HonoHttpServer } from '@objectstack/plugin-hono-server'; +import { ExternalDatasourceService } from '@objectstack/service-datasource'; +import type { IntrospectedSchema, SchemaValidationReport } from '@objectstack/spec/contracts'; +import { registerExternalDatasourceRoutes } from './external-datasource-routes.js'; + +/** + * Three federated datasources — N > 1 is what makes the scoping assertion + * falsifiable. Each carries exactly one federated object, so "one datasource + * introspected" and "one introspection" are the same count and neither hides + * the other. + */ +const DATASOURCES = ['wh_a', 'wh_b', 'wh_c'] as const; + +/** The remote every fixture datasource exposes: one table, one column. */ +const REMOTE: IntrospectedSchema = { + dialect: 'postgres', + introspectedAt: '2026-08-21T00:00:00.000Z', + tables: { + orders: { + name: 'orders', + indexes: [], + columns: [{ name: 'order_id', type: 'text', nullable: false, primaryKey: true }], + }, + }, +}; + +/** One federated object per datasource, plus a local one the sweep must skip. */ +const OBJECTS = [ + ...DATASOURCES.map((ds) => ({ + name: `${ds}_orders`, + datasource: ds, + external: { remoteName: 'orders' }, + fields: { order_id: { type: 'text' } }, + })), + // Not federated: no `external`, and the default datasource. `validateAll` + // skips it, so the scoped path must skip it too. + { name: 'local_thing', datasource: 'default', fields: { id: { type: 'text' } } }, +]; + +/** An entitled caller — the capability gate (#9901/#10255) is not this file's subject. */ +const CREDENTIALED = async () => ({ + userId: 'u_validate_scope', + systemPermissions: ['manage_platform_settings'], +}); + +interface Fixture { + /** Every datasource name `introspect` was called with, in call order. */ + introspected: string[]; + service: ExternalDatasourceService; +} + +/** + * A real service over a recording introspector. + * + * `unreachable` makes a datasource's remote refuse the connection, the way a + * dead sibling remote behaves in production. + */ +function makeService(opts: { unreachable?: readonly string[]; listObjectsThrows?: string } = {}): Fixture { + const introspected: string[] = []; + const unreachable = new Set(opts.unreachable ?? []); + const service = new ExternalDatasourceService({ + introspect: async (datasource: string) => { + introspected.push(datasource); + if (unreachable.has(datasource)) { + throw new Error(`connect ECONNREFUSED (${datasource})`); + } + return REMOTE; + }, + getDatasource: async (name: string) => + (DATASOURCES as readonly string[]).includes(name) + ? { name, schemaMode: 'external' as const } + : undefined, + getObject: async (name: string) => OBJECTS.find((o) => o.name === name), + listObjects: async () => { + if (opts.listObjectsThrows) throw new Error(opts.listObjectsThrows); + return OBJECTS; + }, + // The per-object catch inside the sweep logs; keep the run quiet. + logger: { warn: () => {} }, + }); + return { introspected, service }; +} + +/** Mount the federation family over one service, on the real adapter. */ +function mount(opts: Parameters[0] = {}) { + const { introspected, service } = makeService(opts); + const validateAllSpy = vi.spyOn(service, 'validateAll'); + 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', { + resolveExecutionContext: CREDENTIALED, + }); + return { app: server.getRawApp(), introspected, validateAllSpy, service }; +} + +/** POST the scoped validate route for one datasource name. */ +async function validate(app: any, name: string) { + const res = await app.fetch( + new Request(`http://local/api/v1/datasources/${name}/external/validate`, { method: 'POST' }), + ); + return { status: res.status, body: (await res.json()) as any }; +} + +/** + * What the PRE-FIX composition answered: the whole-farm sweep, post-filtered to + * one datasource. Computed from a SEPARATE service instance so it neither + * pollutes the fixture's call record nor trips the `validateAll` spy — this is + * the behaviour-unchanged half of the card, and it must be measured, not + * remembered. + */ +async function referenceAnswer( + name: string, + opts: Parameters[0] = {}, +): Promise<{ ok: boolean; results: SchemaValidationReport['results'] }> { + const { service } = makeService(opts); + const report = await service.validateAll(); + const results = (report.results ?? []).filter((r) => r.datasource === name); + return { ok: results.every((r) => r.ok), results }; +} + +describe('[#10537] POST /external/validate scopes its WORK to :name', () => { + it('introspects only the URL datasource — one remote, not three', async () => { + // The fixture must be able to tell the two behaviours apart at all. + expect(DATASOURCES.length).toBeGreaterThan(1); + + const { app, introspected, validateAllSpy } = mount(); + const { status } = await validate(app, 'wh_a'); + + expect(status).toBe(200); + // The whole card, in one line: the sweep read three remotes and kept one. + expect(introspected).toEqual(['wh_a']); + // …and the fan-out entry point is not on the scoped path at all. + expect(validateAllSpy).not.toHaveBeenCalled(); + }); + + it('answers exactly what the post-filtered sweep answered', async () => { + const { app } = mount(); + const { status, body } = await validate(app, 'wh_a'); + const reference = await referenceAnswer('wh_a'); + + expect(status).toBe(200); + expect(body.success).toBe(true); + // `ok` is the domain verdict this route has always carried inside `data`. + expect(body.data.ok).toBe(reference.ok); + expect(body.data.results).toEqual(reference.results); + // Named explicitly so a future refactor that widened the set would fail + // here rather than in a deep-equal diff nobody reads. + expect(body.data.results.map((r: { object: string }) => r.object)).toEqual(['wh_a_orders']); + }); + + it('never dials an unrelated unreachable remote', async () => { + const opts = { unreachable: ['wh_b'] } as const; + const { app, introspected } = mount(opts); + const { status, body } = await validate(app, 'wh_a'); + const reference = await referenceAnswer('wh_a', opts); + + expect(status).toBe(200); + expect(introspected).toEqual(['wh_a']); + expect(introspected).not.toContain('wh_b'); + // The dead sibling changed neither the verdict nor the rows — it only ever + // cost time and produced rows that were filtered away. + expect(body.data.ok).toBe(true); + expect(body.data).toEqual(reference); + }); + + it('an unknown :name answers the same empty report as before — 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(introspected).toEqual([]); + expect(validateAllSpy).not.toHaveBeenCalled(); + }); + + it('a refusal from the service is still 400 EXTERNAL_DATASOURCE_ERROR', async () => { + const { app } = mount({ listObjectsThrows: 'metadata store offline' }); + const { status, body } = await validate(app, 'wh_a'); + + expect(status).toBe(400); + expect(body.success).toBe(false); + expect(body.error.code).toBe('EXTERNAL_DATASOURCE_ERROR'); + expect(body.error.message).toBe('metadata store offline'); + }); + + it('still degrades to 503 when federation is not wired into the host', async () => { + const server = new HonoHttpServer(0); + const ctx = { + getService: (name: string) => { + throw new Error(`no service: ${name}`); + }, + } as any; + registerExternalDatasourceRoutes(server, ctx, '/api/v1', { + resolveExecutionContext: CREDENTIALED, + }); + const { status, body } = await validate(server.getRawApp(), 'wh_a'); + + expect(status).toBe(503); + expect(body.success).toBe(false); + expect(body.error.code).toBe('SERVICE_UNAVAILABLE'); + }); +}); 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 42f7e9e522..65f31c2108 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 @@ -313,6 +313,163 @@ describe('validateAll', () => { }); }); +/** + * [#10537] `validateDatasource` — the same sweep, scoped to one datasource. + * + * The card it closes is a COST/SHAPE defect, not a wrong answer: + * `POST /datasources/:name/external/validate` used to run `validateAll()` and + * post-filter the report, so a URL-scoped request drove a live + * `introspect(datasource)` against EVERY federated datasource and discarded + * most of what it measured. So the assertions below are about the introspection + * CALL RECORD — a test comparing only returned rows would have passed before + * the method existed, since post-filtering already produced the right rows. + * + * Every case also pins the rows against `validateAll()` filtered the old way: + * "same answer, less work" is one claim, and the equivalence half is what keeps + * the selection predicate (federated AND bound to this datasource) from + * drifting away from the sweep's. + */ +describe('validateDatasource', () => { + /** Three datasources — with one, "scoped" and "all" are the same reading. */ + const DATASOURCES = ['wh_a', 'wh_b', 'wh_c']; + + const REMOTE: IntrospectedSchema = { + dialect: 'postgres', + introspectedAt: '2026-08-21T00:00:00.000Z', + tables: { + orders: { + name: 'orders', + indexes: [], + columns: [{ name: 'order_id', type: 'text', nullable: false, primaryKey: true }], + }, + }, + }; + + /** + * One federated object per datasource, one local object, and one object that + * is federated by its `external` binding while sitting on the DEFAULT + * datasource — the edge where "bound to `:name`" and "federated" come apart, + * and the reason the scoped filter cannot simply be `datasource === name`. + */ + const OBJECTS: ObjectLike[] = [ + ...DATASOURCES.map((ds) => ({ + name: `${ds}_orders`, + datasource: ds, + external: { remoteName: 'orders' }, + fields: { order_id: { type: 'text' } }, + })), + { name: 'local_thing', datasource: 'default', fields: { id: { type: 'text' } } }, + { + name: 'default_bound_orders', + datasource: 'default', + external: { remoteName: 'orders' }, + fields: { order_id: { type: 'text' } }, + }, + ]; + + function makeMulti(opts: { unreachable?: readonly string[] } = {}) { + const introspected: string[] = []; + const unreachable = new Set(opts.unreachable ?? []); + const svc = new ExternalDatasourceService({ + introspect: async (datasource: string) => { + introspected.push(datasource); + if (unreachable.has(datasource)) throw new Error(`connect ECONNREFUSED (${datasource})`); + return REMOTE; + }, + getDatasource: async (name: string) => + DATASOURCES.includes(name) ? { name, schemaMode: 'external' } : undefined, + getObject: async (name: string) => OBJECTS.find((o) => o.name === name), + listObjects: async () => OBJECTS, + logger: { warn: () => {} }, + }); + return { svc, introspected }; + } + + /** What the pre-#10537 route computed: the whole sweep, filtered afterwards. */ + async function sweptThenFiltered(datasource: string, opts?: { unreachable?: readonly string[] }) { + const { svc, introspected } = makeMulti(opts); + const report = await svc.validateAll(); + return { + results: report.results.filter((r) => r.datasource === datasource), + introspected, + }; + } + + it('introspects only the named datasource', async () => { + expect(DATASOURCES.length).toBeGreaterThan(1); + const { svc, introspected } = makeMulti(); + + const report = await svc.validateDatasource('wh_a'); + + expect(introspected).toEqual(['wh_a']); + expect(report.ok).toBe(true); + expect(report.results.map((r) => r.object)).toEqual(['wh_a_orders']); + }); + + it('returns exactly the rows the whole-farm sweep would have kept', async () => { + const { svc } = makeMulti(); + for (const ds of DATASOURCES) { + const scoped = await svc.validateDatasource(ds); + const swept = await sweptThenFiltered(ds); + expect(scoped.results).toEqual(swept.results); + expect(scoped.ok).toBe(swept.results.every((r) => r.ok)); + // The sweep really was wider — otherwise the line above is vacuous. + expect(swept.introspected.length).toBeGreaterThan(1); + } + }); + + it('keeps the federated-only predicate: a `default`-bound external object is the default datasource\'s row', async () => { + const { svc } = makeMulti(); + + const scoped = await svc.validateDatasource('default'); + const swept = await sweptThenFiltered('default'); + + // `local_thing` is not federated and appears in neither reading; + // `default_bound_orders` is federated and appears in both. + expect(scoped.results.map((r) => r.object)).toEqual(['default_bound_orders']); + expect(scoped.results).toEqual(swept.results); + }); + + it('never touches an unrelated unreachable remote', async () => { + const opts = { unreachable: ['wh_b'] } as const; + const { svc, introspected } = makeMulti(opts); + + const report = await svc.validateDatasource('wh_a'); + + expect(introspected).toEqual(['wh_a']); + expect(report.ok).toBe(true); + expect(report.results).toEqual((await sweptThenFiltered('wh_a', opts)).results); + }); + + it('keeps the per-object failure row the sweep produces', async () => { + const opts = { unreachable: ['wh_b'] } as const; + const { svc, introspected } = makeMulti(opts); + + const report = await svc.validateDatasource('wh_b'); + + // The scoped path still turns a per-object throw into a row rather than + // rejecting the whole report — the sweep's `catch`, not a second one. + expect(introspected).toEqual(['wh_b']); + expect(report.ok).toBe(false); + expect(report.results).toEqual((await sweptThenFiltered('wh_b', opts)).results); + expect(report.results[0]).toMatchObject({ + ok: false, + datasource: 'wh_b', + object: 'wh_b_orders', + diffs: [expect.objectContaining({ kind: 'missing_table', severity: 'error' })], + }); + }); + + it('answers an empty report for a datasource nothing is bound to — and introspects nothing', async () => { + const { svc, introspected } = makeMulti(); + + const report = await svc.validateDatasource('no_such_ds'); + + expect(report).toEqual({ ok: true, results: [] }); + expect(introspected).toEqual([]); + }); +}); + describe('refreshCatalog', () => { it('produces a snapshot with suggested field types', async () => { const svc = makeService(); diff --git a/packages/services/service-datasource/src/external-datasource-service.ts b/packages/services/service-datasource/src/external-datasource-service.ts index 52f95d136c..48a5aa0384 100644 --- a/packages/services/service-datasource/src/external-datasource-service.ts +++ b/packages/services/service-datasource/src/external-datasource-service.ts @@ -409,14 +409,29 @@ export class ExternalDatasourceService implements IExternalDatasourceService { return { ok, datasource, object: objectName, diffs }; } - async validateAll(): Promise { - const objects = await this.config.listObjects(); - const federated = objects.filter( - (o) => o.external !== undefined || (o.datasource && o.datasource !== 'default'), - ); + /** + * Is this object part of the federated sweep at all? + * + * ONE spelling of the predicate, deliberately — {@link validateAll} and + * {@link validateDatasource} select from the same population, and the scoped + * one exists to do LESS WORK, not to answer a different question. Two copies + * would be two answers to "is this object federated" waiting to diverge. + */ + private isFederated(o: ObjectLike): boolean { + return o.external !== undefined || Boolean(o.datasource && o.datasource !== 'default'); + } + /** + * Validate a chosen set of objects, one report. + * + * A per-object throw becomes a `missing_table` row carrying the thrower's + * message rather than rejecting the whole report: one unreachable remote (or + * one object whose definition vanished mid-sweep) must not erase the verdicts + * of the objects that did validate. + */ + private async validateEach(objects: ObjectLike[]): Promise { const results = await Promise.all( - federated.map((o) => + objects.map((o) => this.validateObject(o.name).catch((err): SchemaValidationResult => { this.logger?.warn(`validateObject('${o.name}') failed`, err); return { @@ -439,6 +454,51 @@ export class ExternalDatasourceService implements IExternalDatasourceService { const ok = results.every((r) => r.ok); return { ok, results }; } + + async validateAll(): Promise { + const objects = await this.config.listObjects(); + return this.validateEach(objects.filter((o) => this.isFederated(o))); + } + + /** + * [#10537] Validate the federated objects bound to ONE datasource. + * + * The scoped twin of {@link validateAll}, composed from the same primitives + * (`listObjects` → filter → `validateObject`) so a caller that asked about + * one datasource drives live remote introspection against THAT datasource + * only. `POST /api/v1/datasources/:name/external/validate` used to reach this + * by running the whole-farm sweep and post-filtering the report: the rows + * were right, but a request scoped by its URL paid for every OTHER federated + * datasource's remote round-trips and discarded the results — and an + * unreachable *unrelated* remote slowed the answer for the datasource that + * was actually asked about. + * + * Row-for-row identical to that composition, by construction: the same + * federation predicate, the same `validateObject`, the same per-object catch, + * and a selection keyed on `o.datasource ?? 'default'` — which is exactly the + * value `validateObject` reports back as `result.datasource`, so "the rows + * 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. + * + * NOT on `IExternalDatasourceService` (spec): the route composition this + * serves was authorized as a service-side helper, while adding a + * per-datasource validate to the contract is a spec-surface change that has + * to be decided on its own. `packages/rest`'s federation registrar therefore + * probes for this method and answers `503` when the wired service has no + * scoped spelling, rather than silently falling back to the fan-out. + */ + async validateDatasource(datasource: string): Promise { + const objects = await this.config.listObjects(); + return this.validateEach( + objects.filter((o) => this.isFederated(o) && (o.datasource ?? 'default') === datasource), + ); + } } /** Render a reviewable `*.object.ts` source string for an object draft. */