refactor(data-objectstack): delete the subsumed CONCURRENT_UPDATE guard and align the predicate's doc

.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`.

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,

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<string, unknown>) =>
+ 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);
+ });
+ });
 });