diff --git a/.changeset/client-error-envelope-normalisation.md b/.changeset/client-error-envelope-normalisation.md new file mode 100644 index 0000000000..b02a320157 --- /dev/null +++ b/.changeset/client-error-envelope-normalisation.md @@ -0,0 +1,51 @@ +--- +"@objectstack/client": patch +--- + +fix(client): normalize both server error envelopes so `err.code` / `err.fields` mean one thing (#3918 follow-up) + +Two envelopes are in play and they disagree about where the semantic code and +the per-field list live: + +``` +@objectstack/rest, flat: + { error, code: 'VALIDATION_FAILED', fields: [...] } + +runtime dispatcher, wrapped: + { success: false, error: { message, code: 400, + details: { code: 'VALIDATION_FAILED', fields: [...] } } } +``` + +`error.code` in the **wrapped** form is the HTTP status, not a semantic code. +The client read it straight through, so `err.code` was the **number 400** where +the flat envelope gave `'VALIDATION_FAILED'` — meaning the branch our own docs +teach, + +```js +if (err.code === 'VALIDATION_FAILED') err.fields.forEach(…) +``` + +never matched on a dispatcher-served surface, and the field list (put on the +wire for those routes by #3918) was unreachable at `err.details.error.details.fields`. + +Now normalized at the throw site: + +- **`err.code` is always the semantic string.** It is read from the flat + `code`, else the wrapped `error.details.code`, else a *string* `error.code` — + a numeric value is never reported as a code. The HTTP status is on + `err.httpStatus`, where it always was. +- **`err.fields` is the per-field list** whenever the server sent one, from + either envelope. It is left **unset** (not `[]`) when there is none, so + `if (err.fields)` is a safe test for "this failure is field-anchored". +- **`err.details`** prefers a top-level `details` (unchanged), then the wrapped + envelope's own `details`, then the whole body. The flat envelope has no + top-level `details` and so keeps falling through to the whole body exactly as + before — only the wrapped shape changes, and only from "the entire response" + to the structured object it actually carries. + +**Behaviour change worth noting:** code that read `err.code` from a +dispatcher-served route previously got a number and now gets a string (or +`undefined` where the server sent no semantic code). Nothing in this repo did — +`err.httpStatus` was always the correct source for the status, and remains +untouched — but a consumer that branched on `err.code === 400` should move to +`err.httpStatus === 400`. diff --git a/content/docs/api/client-sdk.mdx b/content/docs/api/client-sdk.mdx index 57e0e5aecc..81cfd53a79 100644 --- a/content/docs/api/client-sdk.mdx +++ b/content/docs/api/client-sdk.mdx @@ -517,6 +517,34 @@ try { } ``` +`error.code` is always the **semantic** code as a string — the numeric HTTP +status lives on `error.httpStatus` and nowhere else. This holds regardless of +which server surface answered: the REST server replies with a flat +`{ error, code, fields }` body while the runtime dispatcher replies with a +wrapped `{ success, error: { message, code, details } }` body whose `error.code` +is the HTTP status, and the client normalizes both before throwing. + +### Per-field validation errors + +A validation failure additionally carries `error.fields` — one entry per +offending field, ready to attach to the inputs that produced them: + +```typescript +try { + await client.data.create('contact', { email: 'not-an-email' }); +} catch (error) { + if (error.code === 'VALIDATION_FAILED') { + for (const f of error.fields ?? []) { + showFieldError(f.field, f.message); // 'email', 'email must be a valid email address' + } + } +} +``` + +`error.fields` is left **unset** (not `[]`) when the server reported no +per-field detail, so `if (error.fields)` is a safe test for "this failure is +field-anchored". + ### Error Codes | Code | HTTP | Category | Retryable | Description | diff --git a/content/docs/api/error-catalog.mdx b/content/docs/api/error-catalog.mdx index 018058e5ef..69d9946661 100644 --- a/content/docs/api/error-catalog.mdx +++ b/content/docs/api/error-catalog.mdx @@ -389,30 +389,49 @@ interface FieldError { ## Client-Side Error Handling -The `@objectstack/client` SDK's built-in fetch error handling attaches only -`code`, `category`, `httpStatus`, `retryable`, and `details` to the thrown -error — not `fieldErrors`, `retryAfter`, or `requestId` directly. If your -server populates those on the response body, read them from -`apiError.details` (e.g. `apiError.details?.fieldErrors`) until the client -surfaces them at the top level. +The `@objectstack/client` SDK's built-in fetch error handling attaches +`code`, `category`, `httpStatus`, `retryable`, `details`, and — for validation +failures — `fields`. It does **not** attach `retryAfter` or `requestId` +directly; if your server populates those on the response body, read them from +`apiError.details` until the client surfaces them at the top level. + +Two naming details matter here: + +- The per-field list is `apiError.fields`, matching the wire (`fields[]`), not + the spec contract's `fieldErrors`. Each entry is `{ field, code, message }`. + It is left **unset** when the server reported no per-field detail, so + `if (apiError.fields)` tests "this failure is field-anchored". +- `apiError.code` is always the semantic code as a **string**. The numeric HTTP + status is on `apiError.httpStatus` and nowhere else. This holds for both wire + formats: the flat REST envelope carries the code at the top level, the runtime + dispatcher's wrapped envelope carries it in `error.details.code` (its + `error.code` is the HTTP status), and the client normalizes both before + throwing. ### TypeScript Example ```typescript -import type { EnhancedApiError } from '@objectstack/spec/api'; +import type { EnhancedApiError, FieldError } from '@objectstack/spec/api'; + +// The spec type names the per-field list `fieldErrors`; what the client +// actually attaches at runtime is `fields` (the wire name). Widen the cast +// until the two names converge. +type ThrownApiError = EnhancedApiError & { fields?: FieldError[] }; async function handleApiCall() { try { const result = await client.data.create('task', { title: 'New Task' }); return result; } catch (error) { - const apiError = error as EnhancedApiError; + const apiError = error as ThrownApiError; switch (apiError.category) { case 'validation': - // Show field-level errors to the user - apiError.fieldErrors?.forEach(fe => { + // Show field-level errors to the user. `fields` is what the client + // attaches (the wire name); the spec type calls the same idea + // `fieldErrors`. + apiError.fields?.forEach(fe => { showFieldError(fe.field, fe.message); }); break; diff --git a/packages/client/src/client.test.ts b/packages/client/src/client.test.ts index 2457c095cc..c9740edb50 100644 --- a/packages/client/src/client.test.ts +++ b/packages/client/src/client.test.ts @@ -1458,6 +1458,111 @@ describe('HTTP error shaping', () => { }); }); +// --------------------------------------------------------------------------- +// #3918 follow-up — one error shape across both server envelopes. +// +// `@objectstack/rest` answers flat (`{ error, code, fields }`); the runtime +// dispatcher answers wrapped (`{ success, error: { message, code, details } }`) +// where `error.code` is the HTTP STATUS and the semantic code lives in +// `details.code`. Reading the wrapped `error.code` straight through handed +// callers the number 400 where the flat form handed them 'VALIDATION_FAILED', +// so the branch our own docs teach never matched on a dispatcher-served +// surface. These pin the normalisation that makes `err.code` / `err.fields` +// mean the same thing whichever surface answered. +// --------------------------------------------------------------------------- +describe('HTTP error shaping — envelope normalisation', () => { + const FIELDS = [ + { field: 'email', code: 'invalid_email', message: 'email must be a valid email address' }, + ]; + + /** What @objectstack/rest's `mapDataError` puts on the wire. */ + const FLAT = { error: 'Validation failed', code: 'VALIDATION_FAILED', fields: FIELDS }; + + /** What the runtime dispatcher puts on the wire since #3918. */ + const WRAPPED = { + success: false, + error: { + message: 'Validation failed', + code: 400, + details: { code: 'VALIDATION_FAILED', fields: FIELDS }, + }, + }; + + it('exposes the SEMANTIC code from the flat envelope', async () => { + const { client } = createMockClient(FLAT, 400); + const caught: any = await client.data.delete('pm_base', 'rec_1').catch((e) => e); + + expect(caught.code).toBe('VALIDATION_FAILED'); + expect(caught.httpStatus).toBe(400); + }); + + it('exposes the SEMANTIC code from the wrapped envelope, not the HTTP status', async () => { + // The regression this guards: `caught.code` used to be the number 400. + const { client } = createMockClient(WRAPPED, 400); + const caught: any = await client.data.delete('pm_base', 'rec_1').catch((e) => e); + + expect(caught.code).toBe('VALIDATION_FAILED'); + expect(caught.code).not.toBe(400); + // The status is still available — it just isn't `code`. + expect(caught.httpStatus).toBe(400); + }); + + it('exposes `fields[]` at the same place for BOTH envelopes', async () => { + const flat: any = await createMockClient(FLAT, 400).client.data + .delete('pm_base', 'rec_1').catch((e) => e); + const wrapped: any = await createMockClient(WRAPPED, 400).client.data + .delete('pm_base', 'rec_1').catch((e) => e); + + expect(flat.fields).toEqual(FIELDS); + expect(wrapped.fields).toEqual(FIELDS); + }); + + it('leaves `fields` unset when the server sent none', async () => { + // Callers branch on presence; an empty array would be a lie about a + // failure that had nothing to do with per-field validation. + const { client } = createMockClient({ error: 'nope', code: 'PERMISSION_DENIED' }, 403); + const caught: any = await client.data.delete('pm_base', 'rec_1').catch((e) => e); + + expect(caught.fields).toBeUndefined(); + expect(caught.code).toBe('PERMISSION_DENIED'); + }); + + it('never reports a numeric code, even with no details to fall back on', async () => { + // A pre-#3918 dispatcher body: wrapped, but no `details` at all. + const { client } = createMockClient( + { success: false, error: { message: 'boom', code: 500 } }, + 500, + ); + const caught: any = await client.data.delete('pm_base', 'rec_1').catch((e) => e); + + expect(caught.code).toBeUndefined(); + expect(caught.message).toBe('boom'); + expect(caught.httpStatus).toBe(500); + }); + + it('keeps `details` = the whole body for the flat envelope (unchanged)', async () => { + const { client } = createMockClient(FLAT, 400); + const caught: any = await client.data.delete('pm_base', 'rec_1').catch((e) => e); + + expect(caught.details).toEqual(FLAT); + }); + + it('points `details` at the structured object for the wrapped envelope', async () => { + const { client } = createMockClient(WRAPPED, 400); + const caught: any = await client.data.delete('pm_base', 'rec_1').catch((e) => e); + + expect(caught.details).toEqual({ code: 'VALIDATION_FAILED', fields: FIELDS }); + }); + + it('still honours a top-level `details` when the server sends one', async () => { + const body = { message: 'bad', code: 'X', details: { limit: 1000 } }; + const { client } = createMockClient(body, 400); + const caught: any = await client.data.delete('pm_base', 'rec_1').catch((e) => e); + + expect(caught.details).toEqual({ limit: 1000 }); + }); +}); + describe('packages.install', () => { const MANIFEST = { id: 'com.acme.crm', name: 'Acme CRM', version: '1.0.0', type: 'app' }; diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 4aec1e47a1..29a19e8a1f 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -4378,20 +4378,55 @@ export class ObjectStackClient { ?? errorBody?.error?.message ?? (typeof errorBody?.error === 'string' ? errorBody.error : undefined) ?? res.statusText; - const errorCode = errorBody?.code || errorBody?.error?.code; + // Two server envelopes are in play, and they disagree about where the + // SEMANTIC code and the per-field list live: + // + // @objectstack/rest, flat: + // { error, code: 'VALIDATION_FAILED', fields: [...] } + // runtime dispatcher, wrapped: + // { success: false, error: { message, code: 400, + // details: { code: 'VALIDATION_FAILED', fields: [...] } } } + // + // Note `error.code` in the WRAPPED form is the HTTP status, not a + // semantic code. Reading it straight into `err.code` handed callers the + // number 400 where the flat form handed them 'VALIDATION_FAILED', so the + // branch our own docs teach — + // `if (err.code === 'VALIDATION_FAILED') err.fields.forEach(…)` + // — simply never matched on a dispatcher-served surface. #3918 put the + // field list on the wire for those routes; this is what lets a caller + // reach it without knowing which surface answered. + // + // So: `err.code` is always the semantic STRING (the numeric status is on + // `err.httpStatus`, where it always was), and `err.fields` is always the + // per-field list when the server sent one. + const asSemanticCode = (v: unknown) => (typeof v === 'string' && v ? v : undefined); + const errorCode = + asSemanticCode(errorBody?.code) + ?? asSemanticCode(errorBody?.error?.details?.code) + ?? asSemanticCode(errorBody?.error?.code); + const fieldErrors = + Array.isArray(errorBody?.fields) ? errorBody.fields + : Array.isArray(errorBody?.error?.details?.fields) ? errorBody.error.details.fields + : undefined; // `.message` is what UIs (e.g. the console's error toast) show to end // users verbatim, so keep it to the server's human-readable message — // no `[ObjectStack]` branding and no `CODE:` prefix. The code stays // available programmatically via `error.code`, and the full response // body was already logged above for debugging. const error = new Error(errorMessage) as any; - + // Attach error details for programmatic access error.code = errorCode; error.category = errorBody?.category; error.httpStatus = res.status; error.retryable = errorBody?.retryable; - error.details = errorBody?.details || errorBody; + // Prefer the wrapped envelope's own `details` over the whole body. The + // flat envelope has no top-level `details`, so it keeps falling through + // to `errorBody` exactly as before — only the wrapped shape changes, + // and only from "the entire response" to the structured object it + // actually carries. + error.details = errorBody?.details ?? errorBody?.error?.details ?? errorBody; + if (fieldErrors) error.fields = fieldErrors; throw error; }