diff --git a/.changeset/6375-concurrent-update-subsumed-guard.md b/.changeset/6375-concurrent-update-subsumed-guard.md new file mode 100644 index 0000000000..d780ec58a6 --- /dev/null +++ b/.changeset/6375-concurrent-update-subsumed-guard.md @@ -0,0 +1,23 @@ +--- +--- + +Internal only, no behaviour change: `@object-ui/data-objectstack`'s +`normaliseClientError` carried two stacked `CONCURRENT_UPDATE` guards whose +first could never decide an outcome — its condition +(`code !== 'CONCURRENT_UPDATE' && httpStatus !== 409`) is strictly stronger +than the line below it, so every input it would have returned was returned +one line later anyway. Its `httpStatus !== 409` half advertised a second +acceptance path (a bare 409 still being re-wrapped) that never existed, on the +one function whose whole job is deciding which errors get re-wrapped. Deleted, +with the effective rule — the wire `code` is the sole discriminator — written +where the dead line used to be. + +Also aligned the doc comment above the exported `isConcurrentUpdateError` with +the predicate underneath it: the doc named only the wire shape while the code +accepts `name === 'ConcurrentUpdateError'` as well. The `name` limb is kept — +it is the deliberate cross-realm discriminator that +`isViewConfigPermissionDeniedError`'s doc already cites this function as its +precedent for — and the doc now says so. + +Both accepted sets (the re-wrap's and the predicate's) are now pinned as an +explicit truth table in `packages/data-objectstack/src/occ.test.ts`. diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index 19c8dfc1dc..fd389f5c97 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -1036,6 +1036,25 @@ export class ConcurrentUpdateError extends Error { * Detect "concurrent update" errors raised by the platform. The wire * shape is `409` + `code: 'CONCURRENT_UPDATE'`. The client surfaces * extra details on `error.details` (full response body). + * + * Accepts EITHER that wire `code` OR `name === 'ConcurrentUpdateError'`, and + * reads `httpStatus` for neither. This paragraph exists because the doc used + * to name only the wire shape, which left the `name` limb reading as drift + * (objectui#6375). It is not drift: it is the deliberate cross-realm + * duck-check that {@link isViewConfigPermissionDeniedError} documents and + * cites *this* function as its precedent for — a host that bundles this + * package twice (or re-throws across a worker boundary) ends up holding two + * copies of the class, `instanceof` fails, and the `name` string is the only + * discriminator left. That host is out of tree by construction, so an in-repo + * consumer census cannot see the case the limb was written for and is not + * evidence against it; `@object-ui/plugin-form` and `@object-ui/plugin-detail` + * each carry their own copy of the same two-limb check for adapters they must + * not depend on. + * + * Deliberately WIDER than {@link normaliseClientError}'s re-wrap, which keys + * on the wire `code` alone: an error carrying only the class name is + * recognised here and passed through there. Both accepted sets are pinned in + * `occ.test.ts`. */ export function isConcurrentUpdateError(error: unknown): error is ConcurrentUpdateError { if (!error || typeof error !== 'object') return false; @@ -1147,7 +1166,14 @@ export function normaliseClientError(error: unknown): unknown { ); } - if (e.code !== 'CONCURRENT_UPDATE' && e.httpStatus !== 409) return error; + // The wire `code` is the sole discriminator. A + // `code !== 'CONCURRENT_UPDATE' && httpStatus !== 409` guard used to sit + // directly above this line and could never decide an outcome: its condition + // is strictly stronger, so everything it would have returned is returned + // here anyway. Its `httpStatus !== 409` half advertised a second acceptance + // path — a bare 409 still getting re-wrapped — that never existed, on the + // one function whose whole job is deciding what gets re-wrapped + // (objectui#6375). The truth table is pinned in `occ.test.ts`. if (e.code !== 'CONCURRENT_UPDATE') return error; return new ConcurrentUpdateError({ currentVersion: typeof details.currentVersion === 'string' ? details.currentVersion : null, diff --git a/packages/data-objectstack/src/occ.test.ts b/packages/data-objectstack/src/occ.test.ts index 947638e865..9803c9c6a8 100644 --- a/packages/data-objectstack/src/occ.test.ts +++ b/packages/data-objectstack/src/occ.test.ts @@ -98,4 +98,82 @@ describe('Optimistic Concurrency Control errors', () => { expect(normalised.currentRecord).toBeNull(); }); }); + + // --------------------------------------------------------------------- + // objectui#6375 — the discriminator truth table, one row per input class. + // + // `normaliseClientError`'s re-wrap is decided by the wire `code` ALONE; + // `httpStatus` participates in no outcome. That was already true before + // objectui#6375 deleted the subsumed `code !== ... && httpStatus !== 409` + // guard, and it is still true after — the deletion is a no-op by + // construction, so "behaviour unchanged" on its own would pin nothing. + // + // What earns these rows their keep is the ASYMMETRY: the two 409-carrying + // passthrough rows below go RED under the *other* possible deletion (drop + // the `code !== 'CONCURRENT_UPDATE'` line and keep the conjunction), under + // which a 409 whose code is something else would fall through and be + // re-wrapped as a ConcurrentUpdateError it never was. So this block is + // about which line was deleted, not about the function existing. + // --------------------------------------------------------------------- + describe('normaliseClientError: the wire code alone decides the re-wrap', () => { + /** A client error carrying exactly the fields under test (`name` stays `Error`). */ + const clientError = (props: Record) => + Object.assign(new Error('upstream'), props); + + it('passes through an error with neither the code nor a 409', () => { + const e = clientError({ code: 'NOT_FOUND', httpStatus: 404 }); + expect(normaliseClientError(e)).toBe(e); + }); + + // ASYMMETRY ROW: red under the wrong deletion, green under this one. + it('passes through a 409 whose code is NOT CONCURRENT_UPDATE', () => { + const e = clientError({ code: 'PRECONDITION_FAILED', httpStatus: 409 }); + expect(normaliseClientError(e)).toBe(e); + expect(normaliseClientError(e)).not.toBeInstanceOf(ConcurrentUpdateError); + }); + + // ASYMMETRY ROW: red under the wrong deletion, green under this one. + it('passes through a bare 409 carrying no code at all', () => { + const e = clientError({ httpStatus: 409 }); + expect(normaliseClientError(e)).toBe(e); + expect(normaliseClientError(e)).not.toBeInstanceOf(ConcurrentUpdateError); + }); + + it('re-wraps the canonical 409 + CONCURRENT_UPDATE', () => { + const e = clientError({ code: 'CONCURRENT_UPDATE', httpStatus: 409 }); + expect(normaliseClientError(e)).toBeInstanceOf(ConcurrentUpdateError); + }); + + // The two rows that show `httpStatus` decides nothing on the accepting + // side either: the code re-wraps with a wrong status, and with none. + it('re-wraps CONCURRENT_UPDATE carrying no httpStatus', () => { + const e = clientError({ code: 'CONCURRENT_UPDATE' }); + expect(normaliseClientError(e)).toBeInstanceOf(ConcurrentUpdateError); + }); + + it('re-wraps CONCURRENT_UPDATE carrying a non-409 httpStatus', () => { + const e = clientError({ code: 'CONCURRENT_UPDATE', httpStatus: 500 }); + expect(normaliseClientError(e)).toBeInstanceOf(ConcurrentUpdateError); + }); + }); + + // The accepted set of the exported predicate, pinned because objectui#6375 + // DECIDED to keep its `name === 'ConcurrentUpdateError'` limb rather than + // narrow it to the code. The limb is the cross-realm discriminator — see + // the rationale quoted in the doc comment above `isConcurrentUpdateError` + // and stated in full above `isViewConfigPermissionDeniedError`. A future + // reader who removes it as drift meets this row first. + describe('isConcurrentUpdateError: code OR class name, and never the status', () => { + it('accepts the class name with no wire code — the cross-realm limb', () => { + expect(isConcurrentUpdateError({ name: 'ConcurrentUpdateError' })).toBe(true); + }); + + it('rejects a bare 409: the status is not a discriminator here either', () => { + expect(isConcurrentUpdateError({ httpStatus: 409 })).toBe(false); + }); + + it('rejects a different class name', () => { + expect(isConcurrentUpdateError({ name: 'ValidationError' })).toBe(false); + }); + }); });