diff --git a/.changeset/11628-external-envelope.md b/.changeset/11628-external-envelope.md new file mode 100644 index 0000000000..82d7d8e2d2 --- /dev/null +++ b/.changeset/11628-external-envelope.md @@ -0,0 +1,11 @@ +--- +'@object-ui/app-shell': patch +--- + +The External Datasource panel in Setup and Studio reads the `{ success, data }` envelope its routes answer (objectui#11628). On a federated datasource such as the showcase's `showcase_external`, the Tables tab listed no remote tables. "Refresh catalog" showed no snapshot time, and "Run validation" replaced the panel with a render error (`Cannot read properties of undefined (reading 'length')`). Because the list was empty, the import dialog could not be opened. The client read `tables`, `draft`, `catalog` and the validation verdict from the top of the body, but the server puts each one under `data`. The panel now lists the remote tables, shows the snapshot time after a refresh, renders the validation rows, and opens the import dialog on the generated draft. + +Refusals on these routes now reach the user. Every refusal used to read `[object Object]`, because the client turned the ADR-0112 `{ error: { code, message } }` object into a string. That covered a missing capability, an unknown remote table, an unreachable datasource, and a capability refusal from "Import as Object". The panel now shows the server's message and code. The "federation is not enabled on this server" hint now appears when the server answers `503 SERVICE_UNAVAILABLE`. Before, it never did, because the client compared against a code the server no longer sends. + +A successful response that is not the `{ success: true, data }` envelope is now an error that names the route. Before, the panel showed it as an empty list. + +**Clause-②: no.** Nothing is added to or removed from the package entry. The changed module is not exported from it. diff --git a/packages/app-shell/src/views/metadata-admin/external/api.test.ts b/packages/app-shell/src/views/metadata-admin/external/api.test.ts index f6dfb7a209..a1fb9d7c6a 100644 --- a/packages/app-shell/src/views/metadata-admin/external/api.test.ts +++ b/packages/app-shell/src/views/metadata-admin/external/api.test.ts @@ -4,6 +4,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { listRemoteTables, generateObjectDraft, + refreshCatalog, validateDatasource, importObjectDraft, ExternalServiceUnavailableError, @@ -11,7 +12,7 @@ import { } from './api'; /** - * Build a minimal Response-like object the client's `jsonOrThrow` accepts. + * Build a minimal Response-like object the client's readers accept. * * `headers` is real (objectui#4467): these calls go out through * `createAuthenticatedFetch`, which reads `set-auth-token` off every API @@ -32,6 +33,22 @@ function jsonResponse(body: unknown, init: { status?: number; ok?: boolean } = { } as unknown as Response; } +/* + * The WIRE shapes of the five `/datasources/:name/external/*` routes + * (objectui#11628). Every body there is written by the shared `sendOk` / + * `sendError` of `@objectstack/types` (`registerExternalDatasourceRoutes` in + * `@objectstack/rest`): a success is `{ success: true, data }` with the + * route's payload one level down, and a refusal is the ADR-0112 envelope + * `{ success: false, error: { code, message } }`. + * + * These fixtures used to be the BARE payloads (`{ tables }`, `{ draft }`, a + * string `error`), which is the shape the client read and the server never + * sent — so this file stayed green while the live panel listed no tables, + * showed no catalog timestamp and crashed on Validation. + */ +const ok = (data: unknown) => ({ success: true, data }); +const refusal = (code: string, message: string) => ({ success: false, error: { code, message } }); + function stubFetch(impl: (url: string, init?: RequestInit) => Response | Promise) { const spy = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => impl(typeof input === 'string' ? input : String(input), init), @@ -46,50 +63,154 @@ afterEach(() => { }); describe('external datasource api', () => { - it('lists remote tables and unwraps the { tables } envelope', async () => { + it('lists remote tables out of the { success, data: { tables } } envelope', async () => { const spy = stubFetch(() => - jsonResponse({ tables: [{ schema: 'public', name: 'orders', columnCount: 7 }] }), + jsonResponse( + ok({ + tables: [ + { name: 'customers', columnCount: 7 }, + { name: 'orders', columnCount: 7 }, + ], + }), + ), ); - const tables = await listRemoteTables('warehouse'); - expect(tables).toEqual([{ schema: 'public', name: 'orders', columnCount: 7 }]); + const tables = await listRemoteTables('showcase_external'); + expect(tables).toEqual([ + { name: 'customers', columnCount: 7 }, + { name: 'orders', columnCount: 7 }, + ]); const url = String(spy.mock.calls[0][0]); - expect(url).toContain('/api/v1/datasources/warehouse/external/tables'); + expect(url).toContain('/api/v1/datasources/showcase_external/external/tables'); }); it('passes the ?schema filter through', async () => { - const spy = stubFetch(() => jsonResponse({ tables: [] })); - await listRemoteTables('warehouse', { schema: 'analytics' }); + const spy = stubFetch(() => jsonResponse(ok({ tables: [] }))); + await expect(listRemoteTables('warehouse', { schema: 'analytics' })).resolves.toEqual([]); expect(String(spy.mock.calls[0][0])).toContain('tables?schema=analytics'); }); - it('maps 503 external_service_unavailable to a typed error', async () => { - stubFetch(() => jsonResponse({ error: 'external_service_unavailable' }, { status: 503, ok: false })); - await expect(listRemoteTables('warehouse')).rejects.toBeInstanceOf( - ExternalServiceUnavailableError, - ); - }); - - it('surfaces a generic server error message', async () => { - stubFetch(() => jsonResponse({ error: 'boom' }, { status: 500, ok: false })); - await expect(validateDatasource('warehouse')).rejects.toThrow('boom'); - }); - - it('POSTs draft generation with the remote schema option', async () => { + it('POSTs draft generation with the remote schema option and reads data.draft', async () => { const spy = stubFetch(() => - jsonResponse({ - draft: { name: 'orders', datasource: 'warehouse', definition: {}, source: '', review: [] }, - }), + jsonResponse( + ok({ + draft: { name: 'orders', datasource: 'warehouse', definition: {}, source: '', review: [] }, + }), + ), ); const draft = await generateObjectDraft('warehouse', 'orders', { remoteSchema: 'public' }); expect(draft.name).toBe('orders'); + expect(draft.review).toEqual([]); const [url, init] = spy.mock.calls[0]; expect(String(url)).toContain('/external/tables/orders/draft'); expect(init?.method).toBe('POST'); expect(JSON.parse(String(init?.body))).toEqual({ remoteSchema: 'public' }); }); + it('refreshes the catalog and reads data.catalog, snapshotAt included', async () => { + const catalog = { + name: 'showcase_external_catalog', + datasource: 'showcase_external', + snapshotAt: '2026-10-05T02:00:00.000Z', + tables: [], + }; + const spy = stubFetch(() => jsonResponse(ok({ catalog }))); + const got = await refreshCatalog('showcase_external'); + expect(got).toEqual(catalog); + expect(got.snapshotAt).toBe('2026-10-05T02:00:00.000Z'); + const [url, init] = spy.mock.calls[0]; + expect(String(url)).toContain('/datasources/showcase_external/external/refresh-catalog'); + expect(init?.method).toBe('POST'); + }); + + it('validates and reads the { ok, results } verdict out of data', async () => { + const results = [ + { object: 'showcase_ext_customer', ok: true, diffs: [] }, + { object: 'showcase_ext_order', ok: true, diffs: [] }, + ]; + const spy = stubFetch(() => jsonResponse(ok({ ok: true, results }))); + const report = await validateDatasource('showcase_external'); + expect(report).toEqual({ ok: true, results }); + expect(report.results).toHaveLength(2); + const [url, init] = spy.mock.calls[0]; + expect(String(url)).toContain('/datasources/showcase_external/external/validate'); + expect(init?.method).toBe('POST'); + }); + + it('refuses a 200 that is not the declared envelope instead of reading it as empty', async () => { + // The bare payload this file used to mock. Read through the old client it + // was "no tables"; it is not a body these routes send, so it is a loud + // failure naming the route rather than an empty list. + stubFetch(() => jsonResponse({ tables: [{ name: 'orders', columnCount: 7 }] })); + const err = await listRemoteTables('warehouse').then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(Error); + expect(err).not.toBeInstanceOf(ExternalServiceUnavailableError); + expect((err as Error).message).toContain('/external/tables'); + }); + + it.each([ + ['listRemoteTables', () => listRemoteTables('warehouse')], + ['generateObjectDraft', () => generateObjectDraft('warehouse', 'orders')], + ['refreshCatalog', () => refreshCatalog('warehouse')], + ['validateDatasource', () => validateDatasource('warehouse')], + ])('%s maps 503 SERVICE_UNAVAILABLE to the typed error', async (_name, call) => { + stubFetch(() => + jsonResponse( + refusal('SERVICE_UNAVAILABLE', 'The external-datasource service is not available.'), + { status: 503 }, + ), + ); + await expect(call()).rejects.toBeInstanceOf(ExternalServiceUnavailableError); + }); + + it('surfaces the refusal envelope message and code, never "[object Object]"', async () => { + stubFetch(() => + jsonResponse(refusal('EXTERNAL_DATASOURCE_ERROR', 'no such schema: analytics'), { + status: 400, + }), + ); + const err = await validateDatasource('warehouse').then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(Error); + expect(err).not.toBeInstanceOf(ExternalServiceUnavailableError); + expect((err as Error).message).not.toContain('[object Object]'); + expect((err as Error).message).toContain('no such schema: analytics'); + expect((err as Error).message).toContain('EXTERNAL_DATASOURCE_ERROR'); + }); + + it('a 503 whose envelope names another code is not read as "federation not wired"', async () => { + stubFetch(() => jsonResponse(refusal('INTERNAL_ERROR', 'Internal server error'), { status: 503 })); + const err = await listRemoteTables('warehouse').then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(Error); + expect(err).not.toBeInstanceOf(ExternalServiceUnavailableError); + expect((err as Error).message).toContain('INTERNAL_ERROR'); + }); + + it('falls back to the HTTP status when a failure carries no readable body', async () => { + stubFetch(() => ({ + ok: false, + status: 502, + statusText: 'Bad Gateway', + headers: new Headers(), + json: async () => { + throw new SyntaxError('Unexpected token <'); + }, + }) as unknown as Response); + await expect(refreshCatalog('warehouse')).rejects.toThrow('502 Bad Gateway'); + }); + it('imports a draft as an object via PUT /meta/object/:name', async () => { - const spy = stubFetch(() => jsonResponse({ ok: true })); + // The `/meta` door's own success body — the save result, no `data`. + const spy = stubFetch(() => + jsonResponse({ success: true, version: 'v1', seq: 1, state: 'active', message: 'Saved' }), + ); const draft: ObjectDraft = { name: 'orders', datasource: 'warehouse', @@ -103,4 +224,54 @@ describe('external datasource api', () => { expect(init?.method).toBe('PUT'); expect(JSON.parse(String(init?.body))).toEqual({ name: 'orders', label: 'Orders' }); }); + + /* + * The `/meta` door answers a refused save in two dialects, both measured + * live: the capability gate's nested `{ error: { code, message } }` and the + * spec-validation flat `{ error: '', code }`. + */ + const importDraft: ObjectDraft = { + name: 'orders', + datasource: 'warehouse', + definition: { name: 'orders', label: 'Orders' }, + source: '', + review: [], + }; + + it('surfaces the /meta capability refusal (nested error) instead of "[object Object]"', async () => { + stubFetch(() => + jsonResponse( + { + error: { + code: 'FORBIDDEN', + message: 'Saving a metadata item requires the `manage_metadata` capability.', + }, + }, + { status: 403 }, + ), + ); + const err = await importObjectDraft(importDraft).then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).not.toContain('[object Object]'); + expect((err as Error).message).toContain('manage_metadata'); + expect((err as Error).message).toContain('FORBIDDEN'); + }); + + it('surfaces the /meta spec-validation refusal (flat string error)', async () => { + stubFetch(() => + jsonResponse( + { + error: 'object/orders failed spec validation: 1 issue — fields [invalid_type]', + code: 'INVALID_METADATA', + }, + { status: 422 }, + ), + ); + await expect(importObjectDraft(importDraft)).rejects.toThrow( + 'object/orders failed spec validation', + ); + }); }); diff --git a/packages/app-shell/src/views/metadata-admin/external/api.ts b/packages/app-shell/src/views/metadata-admin/external/api.ts index ab5a2c450b..a9443dd7c9 100644 --- a/packages/app-shell/src/views/metadata-admin/external/api.ts +++ b/packages/app-shell/src/views/metadata-admin/external/api.ts @@ -4,16 +4,21 @@ * Thin REST client for the External Datasource Federation routes * (ADR-0015 §6.2, framework `registerExternalDatasourceRoutes`). * - * Mounted server-side under `/api/v1/datasources/:name/external/*`: + * Mounted server-side under `/api/v1/datasources/:name/external/*`. Each + * payload below arrives INSIDE the `{ success: true, data }` envelope — the + * shared `sendOk` writes every success body on these routes — and + * {@link readExternalData} is the one place that unwraps it: * - * GET /tables[?schema=] → { tables: RemoteTable[] } - * POST /tables/:remote/draft → { draft: ObjectDraft } - * POST /refresh-catalog → { catalog: ExternalCatalog } - * POST /validate → { ok, results: SchemaValidationResult[] } + * GET /tables[?schema=] → data: { tables: RemoteTable[] } + * POST /tables/:remote/draft → data: { draft: ObjectDraft } + * POST /refresh-catalog → data: { catalog: ExternalCatalog } + * POST /validate → data: { ok, results: SchemaValidationResult[] } * - * Every route degrades to `503 external_service_unavailable` when the host - * has not wired the `external-datasource` service — callers surface that as a - * "federation not enabled on this server" hint rather than a hard error. + * A refusal is the ADR-0112 envelope `{ success: false, error: { code, + * message } }`, written by the shared `sendError`. Every route degrades to + * `503` with `error.code` `SERVICE_UNAVAILABLE` when the host has not wired + * the `external-datasource` service — callers surface that as a "federation + * not enabled on this server" hint rather than a hard error. * * All calls go through `createAuthenticatedFetch()` so the Bearer token, * `X-Tenant-ID`, and `Accept-Language` are injected exactly like every other @@ -26,6 +31,7 @@ import { createAuthenticatedFetch } from '@object-ui/auth'; // see: it is a hand-rolled fetch. It writes `object` metadata, so it applies the // same invariant `MetadataClient.save` applies, from the same module. import { assertObjectMetadataWritable } from '@object-ui/data-objectstack'; +import { readEnvelopeFailureText } from '../../../utils/apiErrorEnvelope.js'; import type { GenerateDraftOpts, ObjectDraft, @@ -77,9 +83,10 @@ export type { SchemaDiffEntry, SchemaDiffEntryKind } from '@objectstack/spec/sha export type { ExternalCatalog, ExternalColumn, ExternalTable } from '@objectstack/spec/data'; /** - * Raised when the server replies `503 external_service_unavailable` — the - * federation service is not wired into this host. Callers render a friendly - * "enable federation on the server" message instead of a generic failure. + * Raised when the server replies `503` with the envelope code + * `SERVICE_UNAVAILABLE` — the federation service is not wired into this host. + * Callers render a friendly "enable federation on the server" message instead + * of a generic failure. */ export class ExternalServiceUnavailableError extends Error { constructor() { @@ -103,34 +110,65 @@ function externalBase(datasource: string): string { return `${serverBase()}/api/v1/datasources/${encodeURIComponent(datasource)}/external`; } -async function jsonOrThrow(res: Response): Promise { - if (res.status === 503) { - // Body is `{ error: 'external_service_unavailable' }` — treat distinctly. - let body: unknown; - try { - body = await res.json(); - } catch { - body = undefined; - } - const code = - body && typeof body === 'object' && 'error' in body - ? String((body as Record).error) - : ''; - if (code === 'external_service_unavailable') throw new ExternalServiceUnavailableError(); +/** The parsed body, or `undefined` when there is none to parse (a proxy's HTML 502). */ +async function readBody(res: Response): Promise { + try { + return await res.json(); + } catch { + return undefined; } +} + +/** `error.code` of an ADR-0112 refusal envelope, or `''` when the body is not one. */ +function envelopeErrorCode(body: unknown): string { + const error = (body as { error?: unknown } | null | undefined)?.error; + if (!error || typeof error !== 'object') return ''; + const { code } = error as { code?: unknown }; + return typeof code === 'string' ? code : ''; +} + +/** + * Read one answer of the `/external/*` routes: the route's payload, unwrapped + * from the `{ success: true, data }` envelope (objectui#11628). + * + * ## Why the unwrap lives here, once + * + * This client used to return the WHOLE body and read `tables` / `draft` / + * `catalog` / `{ ok, results }` off its top level, where the server never puts + * them. The panel therefore listed no remote tables (`tables ?? []` turned the + * miss into an empty list), showed no catalog timestamp, and crashed the + * Validation tab on `results.length`, while this module's tests mocked the bare + * payload and stayed green. Every route here answers through the same `sendOk`, + * so the envelope is unwrapped in this one reader and each caller reads its + * own payload key off `data`. + * + * A 2xx body that is NOT that envelope is refused, naming the route, rather + * than read as an empty answer: these routes have one success shape, and + * reading anything else as "nothing there" is exactly how the defect hid. + * + * ## Refusals + * + * The ADR-0112 envelope `{ success: false, error: { code, message } }`, read by + * {@link readEnvelopeFailureText} — app-shell's one rule for the prose a + * person is shown. The old reader ran `String(body.error)` over the nested + * object, so every refusal on this panel read `[object Object]`, and its 503 + * arm compared that same string with the pre-ADR-0112 code + * `external_service_unavailable`, so the "federation not enabled" hint could + * never show. The 503 is recognised by the envelope's own `code` now. + */ +async function readExternalData(res: Response, url: string): Promise { + const body = await readBody(res); if (!res.ok) { - let detail = `${res.status} ${res.statusText}`; - try { - const body = await res.json(); - if (body && typeof body === 'object' && 'error' in body) { - detail = String((body as Record).error); - } - } catch { - /* keep status-text detail */ + if (res.status === 503 && envelopeErrorCode(body) === 'SERVICE_UNAVAILABLE') { + throw new ExternalServiceUnavailableError(); } - throw new Error(detail); + throw new Error(readEnvelopeFailureText(body) ?? `${res.status} ${res.statusText}`); + } + const envelope = body as { success?: unknown; data?: unknown } | null | undefined; + if (envelope?.success !== true || !envelope.data || typeof envelope.data !== 'object') { + throw new Error(`${url} answered ${res.status} without the { success: true, data } envelope.`); } - return (await res.json()) as T; + return envelope.data as T; } /** List remote tables, optionally filtered to a single remote schema. */ @@ -139,12 +177,13 @@ export async function listRemoteTables( opts: { schema?: string } = {}, ): Promise { const qs = opts.schema ? `?schema=${encodeURIComponent(opts.schema)}` : ''; - const res = await authFetch(`${externalBase(datasource)}/tables${qs}`, { + const url = `${externalBase(datasource)}/tables${qs}`; + const res = await authFetch(url, { method: 'GET', headers: { Accept: 'application/json' }, }); - const data = await jsonOrThrow<{ tables: RemoteTable[] }>(res); - return data.tables ?? []; + const data = await readExternalData<{ tables: RemoteTable[] }>(res, url); + return data.tables; } /** Generate an Object draft (structured + `*.object.ts` source) from a table. */ @@ -153,25 +192,24 @@ export async function generateObjectDraft( remoteName: string, opts: GenerateDraftOpts = {}, ): Promise { - const res = await authFetch( - `${externalBase(datasource)}/tables/${encodeURIComponent(remoteName)}/draft`, - { - method: 'POST', - headers: { Accept: 'application/json', 'Content-Type': 'application/json' }, - body: JSON.stringify(opts), - }, - ); - const data = await jsonOrThrow<{ draft: ObjectDraft }>(res); + const url = `${externalBase(datasource)}/tables/${encodeURIComponent(remoteName)}/draft`; + const res = await authFetch(url, { + method: 'POST', + headers: { Accept: 'application/json', 'Content-Type': 'application/json' }, + body: JSON.stringify(opts), + }); + const data = await readExternalData<{ draft: ObjectDraft }>(res, url); return data.draft; } /** Refresh and return the cached remote-schema snapshot. */ export async function refreshCatalog(datasource: string): Promise { - const res = await authFetch(`${externalBase(datasource)}/refresh-catalog`, { + const url = `${externalBase(datasource)}/refresh-catalog`; + const res = await authFetch(url, { method: 'POST', headers: { Accept: 'application/json' }, }); - const data = await jsonOrThrow<{ catalog: ExternalCatalog }>(res); + const data = await readExternalData<{ catalog: ExternalCatalog }>(res, url); return data.catalog; } @@ -179,17 +217,29 @@ export async function refreshCatalog(datasource: string): Promise { - const res = await authFetch(`${externalBase(datasource)}/validate`, { + const url = `${externalBase(datasource)}/validate`; + const res = await authFetch(url, { method: 'POST', headers: { Accept: 'application/json' }, }); - return jsonOrThrow<{ ok: boolean; results: SchemaValidationResult[] }>(res); + return readExternalData<{ ok: boolean; results: SchemaValidationResult[] }>(res, url); } /** * Persist a generated Object draft as a real `object` metadata item * (PUT `/api/v1/meta/object/:name`, mirroring `MetadataClient.save`). The * draft's `definition` is the parseable ObjectSchema body. + * + * ⛔ Not {@link readExternalData}: this is the `/meta` door, not an + * `/external/*` route, and it answers in its own shapes (objectui#11628, + * measured against a live showcase). Its success body is the save result + * itself (`{ success, version, seq, state, message }`, no `data`), which + * nothing here reads. Its refusals come in two dialects, and the door answers + * both: the capability gate writes the nested `{ error: { code, message } }` + * (a caller without `manage_metadata` gets `403 FORBIDDEN` in it), while a + * spec-validation refusal writes the flat `{ error: '', code }` + * (`422 INVALID_METADATA`, `400 VALIDATION_ERROR`). The old reader handled the + * flat one only, and printed the 403 as `[object Object]`. */ export async function importObjectDraft(draft: ObjectDraft): Promise { assertObjectMetadataWritable('object', draft.definition, 'importObjectDraft'); @@ -201,5 +251,11 @@ export async function importObjectDraft(draft: ObjectDraft): Promise { body: JSON.stringify(draft.definition), }, ); - await jsonOrThrow(res); + if (res.ok) return; + const body = await readBody(res); + const flat = (body as { error?: unknown } | null | undefined)?.error; + throw new Error( + readEnvelopeFailureText(body) ?? + (typeof flat === 'string' && flat ? flat : `${res.status} ${res.statusText}`), + ); }