Skip to content

Commit ca54eac

Browse files
fix(service-datasource)!: external validate answers an unknown datasource 404 RESOURCE_NOT_FOUND (#27911)
<!-- claude-projects-attribution: {"github_login":"os-elon-musk"} --> _Requested by **Elon** · [project thread](https://claude.ai/code/project/chan_01WedDzV26nniwxv7jC88B9D?thread=cmsg_01WedDzV26nniwxv7jC88B9D3czSGepvH7z6EH8Tx6ZZYv)_ Fixes #27696 Clause-②: no (narrowing) Metadata-protocol: none Skills-docs: none — no `skills/**` file or authored doc teaches what validate answers for an unknown name (`git grep "external/validate" -- skills content docs` hits only release notes and QA checklist items about the auth floor and the in-sync report) **BREAKING** for a caller of `POST /api/v1/datasources/:name/external/validate`, or of `ExternalDatasourceService.validateDatasource`, that addresses a datasource name that does not exist: `200 { ok: true, results: [] }` becomes `404 RESOURCE_NOT_FOUND`. The changeset carries the before / now / fix and its ADR-0087 marker (`not-required (no-migration-prescription)`). ## 维护者速读 - 改了什么:外部数据源校验接口 `POST /datasources/NAME/external/validate` 遇到不存在的数据源名时,改为返回 404「数据源不存在」,与查看数据源和其它联邦接口一致。 - 为什么改:以前返回 200 和一份空的「全部通过」报告,调用方分不清「这个数据源没有联邦对象」和「这个数据源根本不存在」。 - 风险与回滚:只影响不存在的名字;已保存的数据源(含托管数据源与主数据源)照旧 200。后台漂移检查若碰到已删除的数据源,会多记一条 warn。回滚即还原本 PR。 - 先看:`packages/services/service-datasource/src/external-datasource-service.ts` 的 `validateDatasource` 开头三行;再看 `packages/rest/src/external-datasource-unknown-name.test.ts` 新增的三个用例。 - 两分钟验证:`pnpm --filter @objectstack/rest exec vitest run src/external-datasource-unknown-name.test.ts`,预期 15 个用例全绿,其中未知名字 404、`warehouse` 与 `default` 仍 200 空报告。 - 需要你做的:无,席位落地。 ## What changes - `ExternalDatasourceService.validateDatasource(name)` (service-datasource) now checks the datasource exists before anything is listed or introspected, the same check `testConnection` makes (#27510): the primary datasource is not looked up (it may have no stored record), and any other name with no saved record throws the package's one `datasourceNotFoundError` (`404`, `RESOURCE_NOT_FOUND`, `Datasource "NAME" does not exist.`). - The REST route needed no code change: its catch already relays that envelope as `404` since #27618 (`answerThrown`). It gains a comment naming the answer. - Only existence is asked, not the full federation guard (`assertFederationSource`): that guard refuses managed datasources and the primary one with `400`, and the ruling keeps a datasource that exists and has nothing federated at its `200` empty report. - The method's doc comment, which said the unknown-name question was left for a separate ruling, now records this ruling. - Side effect, in the changeset: the runtime drift check (`external-validation-plugin.ts`) also calls `validateDatasource`; for a datasource deleted after its timer was armed, a tick lands in its existing catch (one `warn`, no events) instead of returning an empty report silently. No code change there. - File surface: the claim was amended (seat, before push) to add `external-datasource-service.ts` and its test; the dispatch order had named only the route file. ## Tests - `packages/rest/src/external-datasource-unknown-name.test.ts`: validate on `no_such_ds` answers `404 RESOURCE_NOT_FOUND` in `GET /datasources/:name`'s words; validate on a saved managed datasource (`warehouse`) and on `default` keeps `200 { ok: true, results: [] }`. Real `ExternalDatasourceService` over the real `HonoHttpServer`. - `packages/rest/src/external-datasource-validate-scope.test.ts`: the cell that pinned the old empty `200` for an unknown name now pins the `404`, still with no introspection and no `validateAll` call. - `packages/services/service-datasource/src/__tests__/external-datasource-service.test.ts`: `validateDatasource` on an unknown name rejects with `status 404` / `code RESOURCE_NOT_FOUND` / `datasourceNotFoundError`'s message, with `listObjects` never called and nothing introspected; a saved datasource nothing is bound to still answers the empty report. <details><summary>Red on main, ablation, results</summary> - On `main` (da94c11) with only the new pins: rest validate cell red, `expected { status: 200, success: true, … } to deeply equal { status: 404, success: false, … }`; the two `200` cells green. - Ablation (the three-line check removed from the service, then restored): service pin red (1 failed / 42), rest pin red (1 failed / 14); restored: 43/43 and 15/15. Both suites resolve `@objectstack/service-datasource` to `src/` (rest's vitest alias), so no rebuild was in the path. - `packages/rest`: Test Files 352 passed (352), Tests 7023 passed / 350 skipped. - `packages/services/service-datasource`: Test Files 63 passed (63), Tests 1029 passed. - `packages/runtime` `src/external-validation*`: Test Files 4 passed, Tests 44 passed. - `tsc --noEmit`: rest exit 0, service-datasource exit 0. </details> ## Acceptance notes - None outside the card. ## Verification <details><summary>Gates at bdbdaae</summary> - `node scripts/pm/dispatch-gates.mjs --commands`: 62 families derived. All run; 61 exit 0; `pnpm check:dual-build-cjs-loads` exit 3 (PREREQUISITE NOT MET, needs every package's dist), NOT MEASURED locally, CI runs it. - `dispatch-gates --ran`: 62 derived, 61 run, 1 NOT-MEASURED, 0 UNRUN. - `pnpm lint`: exit 0. - `node scripts/check-commit-card-trailers.mjs --range origin/main..HEAD`: exit 0. - Full dogfood battery at the final head: reported in the os-dev-report comment. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_016WV4cx56cEs32AZSfABRR8)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent ed8b237 commit ca54eac

6 files changed

Lines changed: 137 additions & 15 deletions

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
'@objectstack/service-datasource': patch
3+
---
4+
5+
fix(service-datasource): `POST /api/v1/datasources/:name/external/validate` answers a name no saved datasource has `404 RESOURCE_NOT_FOUND`, as `GET /datasources/:name` and the other federation doors do
6+
7+
Clause-②: no (narrowing)
8+
Metadata-protocol: none
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) No key of any schema, accepted value, stored row, stored shape or export moves. What narrows is one HTTP answer: a validate request for a datasource name that does not exist now gets the 404 the sibling by-name doors already give it, instead of an empty 200 report. There is no metadata for `objectstack migrate meta` to rewrite and nothing to tombstone; the fix for a caller is to stop addressing a datasource that does not exist. -->
11+
12+
`ExternalDatasourceService.validateDatasource(name)` selected the federated objects bound to `name` and validated them. For a name with no saved datasource it selected nothing and returned an empty, vacuously `ok` report, so the validate door answered `200 { ok: true, results: [] }` and a caller could not tell "nothing on this datasource is federated" from "this datasource does not exist".
13+
14+
It now checks that the datasource exists first, the check `testConnection` makes (#27510): a name that resolves no saved datasource throws the package's one `datasourceNotFoundError` (`404`, `RESOURCE_NOT_FOUND`, `Datasource "NAME" does not exist.`) before anything is listed or introspected, and the REST route relays it as such. Only existence is asked, not the full federation guard: a saved datasource with nothing federated, a managed one included, and the platform's primary datasource (not looked up, as it may have no stored record) keep their `200` empty report.
15+
16+
**Side effect.** The runtime's background drift check (`external.validation.checkIntervalMs`) also calls `validateDatasource`. For a datasource that was deleted after its timer was armed, a tick now lands in the check's existing catch (one `warn`, "drift check could not be performed", no events) where it used to return an empty report silently.
17+
18+
**BREAKING** for a caller of `POST /api/v1/datasources/:name/external/validate`, or of `validateDatasource`, that addresses a datasource name that does not exist.
19+
20+
- **Before.** `200` with `{ ok: true, results: [] }`; `validateDatasource` resolved to `{ ok: true, results: [] }`.
21+
- **Now.** `404 RESOURCE_NOT_FOUND`; `validateDatasource` rejects with that error.
22+
- **Fix.** Address a saved datasource, or treat the `404` as "no such datasource".

‎packages/rest/src/external-datasource-routes.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,11 @@ export function registerExternalDatasourceRoutes(
599599
// the post-filter used to keep are exactly the rows this returns (see
600600
// `external-datasource-validate-scope.test.ts`, which pins the two answers
601601
// against each other and the introspection call record beside them).
602+
//
603+
// [#27696] A name no saved datasource has is the service's 404
604+
// `datasourceNotFoundError`, thrown before any validation and relayed as
605+
// such by `refused`; a datasource with nothing federated keeps its empty
606+
// `200` report.
602607
{
603608
method: 'POST',
604609
path: `${ext}/validate`,

‎packages/rest/src/external-datasource-unknown-name.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,3 +118,46 @@ describe('the federation twins on a datasource that exists and is not a federati
118118
expect(answer.body.error.message).toContain("is the platform's managed primary datasource");
119119
});
120120
});
121+
122+
/**
123+
* [#27696] `POST …/external/validate`, the one by-name federation door the
124+
* pins above left out. It answered an unknown name `200` with an empty,
125+
* vacuously `ok` report, so a caller could not tell "nothing on this
126+
* datasource is federated" from "this datasource does not exist". The ruling:
127+
* no definition ⇒ the same `404`, before any validation; a datasource that
128+
* exists and has nothing federated keeps its `200` empty report.
129+
*/
130+
describe('POST …/external/validate on a name that resolves no saved datasource', () => {
131+
it('answers 404 RESOURCE_NOT_FOUND in the words GET /datasources/:name uses', async () => {
132+
const call = mount();
133+
134+
const answer = await call('POST', '/no_such_ds/external/validate');
135+
136+
expect({
137+
status: answer.status,
138+
success: answer.body.success,
139+
code: answer.body.error?.code,
140+
message: answer.body.error?.message,
141+
}).toEqual({
142+
status: 404,
143+
success: false,
144+
code: 'RESOURCE_NOT_FOUND',
145+
message: 'Datasource "no_such_ds" does not exist.',
146+
});
147+
expect(answer.body).not.toHaveProperty('data');
148+
});
149+
150+
it.each([
151+
['a saved datasource', 'warehouse'],
152+
['the primary datasource', 'default'],
153+
])('keeps the 200 empty report for %s with nothing federated', async (_what, name) => {
154+
const call = mount();
155+
156+
const answer = await call('POST', `/${name}/external/validate`);
157+
158+
expect({ status: answer.status, data: answer.body.data }).toEqual({
159+
status: 200,
160+
data: { ok: true, results: [] },
161+
});
162+
});
163+
});

‎packages/rest/src/external-datasource-validate-scope.test.ts‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -213,16 +213,18 @@ describe('[#10537] POST /external/validate scopes its WORK to :name', () => {
213213
expect(body.data).toEqual(reference);
214214
});
215215

216-
it('an unknown :name answers the same empty report as before — and dials nothing', async () => {
216+
// [#27696] Until this ruling an unknown name answered an empty report with
217+
// a vacuously true verdict; it is now the 404 every by-name door answers.
218+
// `external-datasource-unknown-name.test.ts` pins the 200 a saved datasource
219+
// with nothing federated keeps.
220+
it('an unknown :name answers 404 RESOURCE_NOT_FOUND — and dials nothing', async () => {
217221
const { app, introspected, validateAllSpy } = mount();
218222
const { status, body } = await validate(app, 'no_such_ds');
219223

220-
expect(status).toBe(200);
221-
expect(body.success).toBe(true);
222-
// Unchanged: an unknown name is not a 404 on this route, it is an empty
223-
// report with a vacuously true verdict.
224-
expect(body.data).toEqual(await referenceAnswer('no_such_ds'));
225-
expect(body.data).toEqual({ ok: true, results: [] });
224+
expect(status).toBe(404);
225+
expect(body.success).toBe(false);
226+
expect(body.error.code).toBe('RESOURCE_NOT_FOUND');
227+
expect(body.error.message).toBe('Datasource "no_such_ds" does not exist.');
226228
expect(introspected).toEqual([]);
227229
expect(validateAllSpy).not.toHaveBeenCalled();
228230
});

‎packages/services/service-datasource/src/__tests__/external-datasource-service.test.ts‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
type DatasourceLike,
1212
type ObjectLike,
1313
} from '../external-datasource-service.js';
14+
import { datasourceNotFoundError } from '../datasource-admin-service.js';
1415

1516
/** Build a fake introspected schema for the `warehouse` datasource. */
1617
function warehouseSchema(): IntrospectedSchema {
@@ -662,14 +663,52 @@ describe('validateDatasource', () => {
662663
});
663664
});
664665

665-
it('answers an empty report for a datasource nothing is bound to — and introspects nothing', async () => {
666-
const { svc, introspected } = makeMulti();
666+
it('answers an empty report for a saved datasource nothing is bound to — and introspects nothing', async () => {
667+
const introspected: string[] = [];
668+
const svc = new ExternalDatasourceService({
669+
introspect: async (datasource: string) => {
670+
introspected.push(datasource);
671+
return REMOTE;
672+
},
673+
getDatasource: async (name: string) => (name === 'wh_empty' ? { name, schemaMode: 'external' } : undefined),
674+
getObject: async () => undefined,
675+
listObjects: async () => OBJECTS,
676+
});
667677

668-
const report = await svc.validateDatasource('no_such_ds');
678+
const report = await svc.validateDatasource('wh_empty');
669679

670680
expect(report).toEqual({ ok: true, results: [] });
671681
expect(introspected).toEqual([]);
672682
});
683+
684+
// [#27696] A name with no saved datasource is not a datasource with nothing
685+
// federated: it is the 404 every by-name door answers, thrown before
686+
// anything is listed or introspected.
687+
it('throws the 404 datasourceNotFoundError for a name that resolves no saved datasource', async () => {
688+
let listed = 0;
689+
const introspected: string[] = [];
690+
const svc = new ExternalDatasourceService({
691+
introspect: async (datasource: string) => {
692+
introspected.push(datasource);
693+
return REMOTE;
694+
},
695+
getDatasource: async () => undefined,
696+
getObject: async () => undefined,
697+
listObjects: async () => {
698+
listed++;
699+
return OBJECTS;
700+
},
701+
});
702+
703+
const thrown = await svc.validateDatasource('no_such_ds').catch((err: unknown) => err);
704+
705+
expect(thrown).toMatchObject({
706+
status: 404,
707+
code: 'RESOURCE_NOT_FOUND',
708+
message: datasourceNotFoundError('no_such_ds').message,
709+
});
710+
expect({ listed, introspected }).toEqual({ listed: 0, introspected: [] });
711+
});
673712
});
674713

675714
/**

‎packages/services/service-datasource/src/external-datasource-service.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1425,11 +1425,19 @@ export class ExternalDatasourceService implements IExternalDatasourceService {
14251425
* the sweep would have kept" and "the objects this selects" are the same set
14261426
* (pinned in `__tests__/external-datasource-service.test.ts`).
14271427
*
1428-
* A name nothing is bound to selects nothing and answers an empty, vacuously
1429-
* `ok` report — the sweep-then-filter answer for an unknown name, kept rather
1430-
* than upgraded to a throw: whether an unknown datasource is an error is a
1431-
* separate question from this one, and this method must not decide it in
1432-
* passing.
1428+
* A saved datasource nothing is bound to selects nothing and answers an
1429+
* empty, vacuously `ok` report.
1430+
*
1431+
* [#27696] A name that resolves no saved datasource is not that datasource:
1432+
* it THROWS the 404 {@link datasourceNotFoundError} before anything is
1433+
* listed or validated, the check {@link testConnection} makes (#27510), so
1434+
* `POST …/external/validate` answers it as `GET /datasources/:name` and its
1435+
* federation siblings do. It answered the empty `ok` report above, and a
1436+
* caller could not tell "nothing here is federated" from "no such
1437+
* datasource". Only existence is asked, not the federation guard
1438+
* ({@link assertFederationSource}): a managed datasource, and the primary
1439+
* one (not looked up, as it may have no stored record), keep their empty
1440+
* report.
14331441
*
14341442
* NOT on `IExternalDatasourceService` (spec): the route composition this
14351443
* serves was authorized as a service-side helper, while adding a
@@ -1439,6 +1447,9 @@ export class ExternalDatasourceService implements IExternalDatasourceService {
14391447
* scoped spelling, rather than silently falling back to the fan-out.
14401448
*/
14411449
async validateDatasource(datasource: string): Promise<SchemaValidationReport> {
1450+
if (datasource !== DEFAULT_DATASOURCE_NAME && !(await this.config.getDatasource(datasource))) {
1451+
throw datasourceNotFoundError(datasource);
1452+
}
14421453
const objects = await this.config.listObjects();
14431454
return this.validateEach(
14441455
objects.filter((o) => this.isFederated(o) && (o.datasource ?? 'default') === datasource),

0 commit comments

Comments
 (0)