diff --git a/.changeset/batch-row-unique-violation-metadata-protocol.md b/.changeset/batch-row-unique-violation-metadata-protocol.md new file mode 100644 index 0000000000..918ad7a355 --- /dev/null +++ b/.changeset/batch-row-unique-violation-metadata-protocol.md @@ -0,0 +1,49 @@ +--- +"@objectstack/metadata-protocol": minor +--- + +fix(metadata-protocol)!: a batch ROW reports a unique-constraint refusal as `UNIQUE_VIOLATION` — the same wire spelling as the whole-request failure on the same route (#14723) + + + +**BREAKING** on the per-row report of `POST /api/v1/data/:object/batch` (and +the multi-object `POST /api/v1/batch`, which rides the same protocol): a row +refused by the engine's `DuplicateRecordError` envelope now reports +`errors[].code: 'UNIQUE_VIOLATION'` where it reported `'DUPLICATE_RECORD'`. +Shipped as `minor` under the repo's launch-window convention for breaking +changes. Maintainer ruling 2026-09-03 on #14723 (verbatim 「同意，然后执行契约 +复审」), adopting option A: one wire spelling for a unique-constraint refusal on +every route. + +**Why.** `toRowApiError` put a thrown REGISTERED code on the row verbatim, and +`DUPLICATE_RECORD` is registered, so a `DuplicateRecordError` row said +`DUPLICATE_RECORD` while the whole-request failure on the very same route (the +bulk door's classification in `@objectstack/rest`) answered `UNIQUE_VIOLATION` +— the standard-catalog member `content/docs/protocol/kernel/http-protocol.mdx` +documents for the 409 constraint-violation body. Since the bulk doors were +restored to `UNIQUE_VIOLATION`, the two spellings of one condition sat side by +side in one route's responses, which ADR-0112's one-name-per-concept and the +error-code ledger's own header both forbid. The duplication is removed, not +declared: no ledger waiver is added. + +**What changes.** The row derivation recognises the engine's envelope by the +same two-part gate the whole-request arm uses — the registered code AND the +class name `DuplicateRecordError`, never message text — and reports +`UNIQUE_VIOLATION`. Everything else on the row is unchanged: `httpStatus: 409`, +the platform sentence (no driver text, no bound value — the driver's error +stays on `cause` and never reaches the row), and the sibling `NOT_ATTEMPTED` / +`ROLLED_BACK` rows. + +**What does NOT change.** The engine's thrown identity: `DuplicateRecordError.code` +is still `DUPLICATE_RECORD` for an in-process caller of `engine.insert` / +`engine.update` (a hook, a flow node), and the objectql pins on `insert` / +`insertMany` hold. The single-record `/data` door, which has answered +`UNIQUE_VIOLATION` throughout, does not move. A producer that merely THROWS the +registered `DUPLICATE_RECORD` from its own body without being the engine's +class keeps its own code on the row, exactly as it does at the door. + +**Consumer note.** A batch client that branched on a row's `code` reading +`DUPLICATE_RECORD` reads `UNIQUE_VIOLATION` there now — the same value it +already handles for the whole-request 409 on that route and on the +single-record door. Measured in-repo and in the sibling repos (hotcrm, objectui, +non-test sources): zero consumers branch on either spelling of a row code. diff --git a/.changeset/duplicate-record-error-developer-message-wire-spelling.md b/.changeset/duplicate-record-error-developer-message-wire-spelling.md new file mode 100644 index 0000000000..06f899585a --- /dev/null +++ b/.changeset/duplicate-record-error-developer-message-wire-spelling.md @@ -0,0 +1,18 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): `DuplicateRecordError.developerMessage` names the wire spelling a client branches on (#14723) + +The envelope's `developerMessage` — the remedy sentence addressed to the +application author — told its reader to "branch on `code === 'DUPLICATE_RECORD'`", +which is the engine's THROWN identity and holds only for an in-process caller of +`engine.insert` / `engine.update`. Every REST route reports the same refusal as +`UNIQUE_VIOLATION`, and since #14723 the per-row reports of the batch and import +surfaces do too, so the sentence was a platform contradicting itself on the one +line an author is most likely to copy. It now says both halves: over the HTTP +API branch on `code === 'UNIQUE_VIOLATION'` on every route, whole-request and +per-row alike; inside the engine the thrown class carries `DUPLICATE_RECORD`. +The class's own docblock says the same. Nothing else about the envelope moves: +`code`, `status`, `cause`, `field`, `object` and the user-facing `message` are +byte-identical, and every pin on the engine's thrown code holds. diff --git a/.changeset/import-row-unique-violation-rest.md b/.changeset/import-row-unique-violation-rest.md new file mode 100644 index 0000000000..ce6d84a8b5 --- /dev/null +++ b/.changeset/import-row-unique-violation-rest.md @@ -0,0 +1,44 @@ +--- +"@objectstack/rest": minor +--- + +fix(rest)!: an import ROW report spells a unique-constraint refusal `UNIQUE_VIOLATION` — the same wire code as the whole-request failure on the same route (#14723) + + + +**BREAKING** on the per-row results of the import runner +(`POST /api/v1/data/:object/import` and the import job): a row refused by the +engine's `DuplicateRecordError` envelope now reports `code: 'UNIQUE_VIOLATION'` +where it reported `'DUPLICATE_RECORD'`. Shipped as `minor` under the repo's +launch-window convention for breaking changes. Maintainer ruling 2026-09-03 on +#14723 (verbatim 「同意，然后执行契约复审」), adopting option A: one wire +spelling for a unique-constraint refusal on every route. + +**Why.** `toFailedResult` relayed the thrown error's own `code`, and the engine's +envelope carries the registered `DUPLICATE_RECORD` — while the whole-request +failure on the same import route answered `UNIQUE_VIOLATION` through +`mapDataError`. Two spellings of one condition on one route, which ADR-0112's +one-name-per-concept and the error-code ledger's header both forbid. The +duplication is removed, not declared: no ledger waiver is added. + +**What changes.** The import row derivation applies the whole-request arm's own +predicate — the registered code AND the class name `DuplicateRecordError`, +exported from `error-response.ts` as `isEngineDuplicateRecordEnvelope` and now +shared by the arm and the row report — and reports `UNIQUE_VIOLATION`. A +field-level finding still takes precedence (the envelope carries none), the +row's sentence is unchanged (the platform sentence, sanitised as before; no +driver text), and a producer that merely throws the registered +`DUPLICATE_RECORD` without being the engine's class keeps its own code. + +**What does NOT change.** The whole-request doors (single-record, bulk, import, +metadata, UI) already answered `UNIQUE_VIOLATION` and keep doing so; the arm's +logic is untouched beyond reading the shared predicate. The engine's thrown +identity stays `DUPLICATE_RECORD` in-process. This package's `error-response.ts` +docblock that disclosed the fork under the #14541 contract review now states +the converged rule. + +**Consumer note.** An import client that branched on a row's `code` reading +`DUPLICATE_RECORD` reads `UNIQUE_VIOLATION` there now — the same value it +already handles for the whole-request 409. Measured in-repo and in the sibling +repos (hotcrm, objectui, non-test sources): zero consumers branch on either +spelling of a row code. diff --git a/packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts b/packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts index 37f5cd79f3..b5d8284899 100644 --- a/packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts +++ b/packages/metadata-protocol/src/protocol.batch-row-http-status.test.ts @@ -59,6 +59,7 @@ import { describe, it, expect, vi } from 'vitest'; import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core'; import { resolveThrownHttpError } from '@objectstack/types'; +import { ErrorCode } from '@objectstack/spec/api'; import { ObjectStackProtocolImplementation } from './protocol.js'; const SCHEMA = { @@ -452,3 +453,141 @@ describe('[#8570] section 6 — anti-vacuity: the doubles are the shapes they cl expect(resolveThrownHttpError(declaredServiceUnavailable()).declaredStatus).toBe(503); }); }); + +// ─── [#14723] The row speaks the WIRE spelling of a unique-constraint refusal ─ + +/** + * MEASURED — `@objectstack/objectql`'s `DuplicateRecordError` as it reaches + * these catches (`node`, the built class, `new DuplicateRecordError('leave_request', + * raw, 'title')`): own properties `[stack, message, object, field, code, + * status, name, cause, developerMessage]`, `code: 'DUPLICATE_RECORD'`, + * `status: 409`, `name: 'DuplicateRecordError'`, the driver's error whole on + * `cause`, and the platform sentence on `message` — no statement, no bound + * value. Assignment ORDER matches the BUILT class: the two parameter + * properties land first, then the `readonly` initialisers, then the + * constructor body sets `name`, `cause` and `developerMessage`. The class + * itself is deliberately not imported — + * this package does not depend on `@objectstack/objectql`, and the row's rule + * reads the envelope's declared `code` / `name`, never its prototype. + */ +function engineDuplicateRecordEnvelope(object = 'leave_request', field = 'title'): Error { + const cause = driverFault( + `insert into \`${object}\` (\`${field}\`, \`id\`) values ('dup-value', 'new-4') returning * - ` + + `UNIQUE constraint failed: ${object}.${field}`, + ); + const err = new Error( + `Duplicate record refused on '${object}': a unique constraint on '${field}' already holds this value. ` + + 'No record was written.', + ) as Error & { code: string; status: number; object: string; field: string; cause: unknown; developerMessage: string }; + err.object = object; + err.field = field; + err.code = 'DUPLICATE_RECORD'; + err.status = 409; + err.name = 'DuplicateRecordError'; + err.cause = cause; + err.developerMessage = 'The driver refused this write as a unique-constraint violation.'; + return err; +} + +/** A hook that deliberately THROWS the registered `DUPLICATE_RECORD` — a different producer. */ +function hookSpeakingDuplicateRecord(): Error { + const err = new Error('this row is already there, says the hook') as Error & { code: string; status: number }; + err.code = 'DUPLICATE_RECORD'; + err.status = 409; + return err; +} + +const ENVELOPE_SENTENCE = + "Duplicate record refused on 'leave_request': a unique constraint on 'title' already holds this value. " + + 'No record was written.'; + +describe('[#14723] section 7 — a `DuplicateRecordError` row reports `UNIQUE_VIOLATION`, the route\'s one wire spelling', () => { + it('the create loop: `UNIQUE_VIOLATION` / 409 / the platform sentence, and no `DUPLICATE_RECORD` anywhere in the payload', async () => { + const { protocol } = makeEngine((verb) => (verb === 'insert' ? engineDuplicateRecordEnvelope() : undefined)); + + const res: any = await protocol.batchData({ + object: 'leave_request', + request: { operation: 'create', records: [{ data: { title: 'dup-value' } }] }, + }); + + // The whole row, exactly: the code moved, the status and the sentence + // did not (a declared 409 is quoted — #8502's positive list, unchanged). + expect(res.results[0].errors[0]).toEqual({ + code: 'UNIQUE_VIOLATION', + message: ENVELOPE_SENTENCE, + httpStatus: 409, + }); + const payload = JSON.stringify(res); + expect(payload).not.toContain('DUPLICATE_RECORD'); + // …and the driver's text stays on `cause`, which never reaches a row. + expect(payload).not.toContain('insert into'); + expect(payload).not.toContain('UNIQUE constraint failed'); + expect(payload).not.toContain('SQLITE_ERROR'); + }); + + it('the update and upsert loops report the same spelling — one helper, every loop', async () => { + const a = makeEngine((verb) => (verb === 'update' ? engineDuplicateRecordEnvelope() : undefined)); + const updateRes: any = await a.protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { title: 'dup-value' } }], + }); + expect(updateRes.results[0].errors[0].code).toBe('UNIQUE_VIOLATION'); + expect(updateRes.results[0].errors[0].httpStatus).toBe(409); + + const b = makeEngine((verb) => (verb === 'update' ? engineDuplicateRecordEnvelope() : undefined)); + const upsertRes: any = await b.protocol.batchData({ + object: 'leave_request', + request: { operation: 'upsert', records: [{ id: 'r1', data: { title: 'dup-value' } }] }, + }); + expect(upsertRes.results[0].errors[0].code).toBe('UNIQUE_VIOLATION'); + expect(JSON.stringify(upsertRes)).not.toContain('DUPLICATE_RECORD'); + }); + + it('[GUARD] a producer that merely SPEAKS `DUPLICATE_RECORD` is not the engine\'s envelope and keeps its own code', async () => { + // The same discrimination the whole-request arm makes (#14389 §5): + // the gate is the registered code AND the class name. A hook throwing + // the registered member from its own body is a different producer + // speaking a member of the vocabulary; the verbatim rule still applies. + const { protocol } = makeEngine((verb) => (verb === 'insert' ? hookSpeakingDuplicateRecord() : undefined)); + + const res: any = await protocol.batchData({ + object: 'leave_request', + request: { operation: 'create', records: [{ data: { title: 'x' } }] }, + }); + + expect(res.results[0].errors[0]).toEqual({ + code: 'DUPLICATE_RECORD', + message: 'this row is already there, says the hook', + httpStatus: 409, + }); + }); + + it('[GUARD] the name alone does not qualify either — a `DuplicateRecordError` carrying another code keeps that code', async () => { + const impostor = approvalsRecordLock('r1'); + impostor.name = 'DuplicateRecordError'; + const { protocol } = makeEngine((verb) => (verb === 'update' ? impostor : undefined)); + + const res: any = await protocol.updateManyData({ + object: 'leave_request', records: [{ id: 'r1', data: { progress: 1 } }], + }); + + expect(res.results[0].errors[0].code).toBe('RECORD_LOCKED'); + }); + + it('anti-vacuity: BOTH spellings are registered, so the verbatim rule alone would have kept `DUPLICATE_RECORD`', () => { + // The mapping is the only thing standing between the envelope and the + // row's old spelling: `toRowApiError`'s verbatim limb admits any + // registered code, and the engine's is registered. Reverting the + // mapping therefore reddens section 7's first case with the row + // reading `DUPLICATE_RECORD` again — measured, not assumed. + expect(ErrorCode.safeParse('DUPLICATE_RECORD').success).toBe(true); + expect(ErrorCode.safeParse('UNIQUE_VIOLATION').success).toBe(true); + + const env = engineDuplicateRecordEnvelope() as any; + expect(Object.getOwnPropertyNames(env)).toEqual([ + 'stack', 'message', 'object', 'field', 'code', 'status', 'name', 'cause', 'developerMessage', + ]); + expect(env.name).toBe('DuplicateRecordError'); + expect(env.code).toBe('DUPLICATE_RECORD'); + expect(resolveThrownHttpError(env).declaredStatus).toBe(409); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index d3716a0267..6e66fe99b5 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -2031,11 +2031,42 @@ type BatchDataRowResult = BatchOperationResult; * `declaredStatus` would mint incoherent rows — `{ code: 'INTERNAL_ERROR', * httpStatus: 409 }` for a `statusCode`-spelled refusal whose own code the * ledger does not know. + * + * ## One wire spelling for a unique-constraint refusal (#14723) + * + * The engine answers a driver's unique-constraint refusal with its + * `DuplicateRecordError` envelope — `code: 'DUPLICATE_RECORD'`, `status: 409`, + * the driver's error whole on `cause`. `DUPLICATE_RECORD` is a registered + * member, so the verbatim limb above used to put it on the row as-is, while + * every WHOLE-REQUEST door in `@objectstack/rest` answers the same class as + * `UNIQUE_VIOLATION` — the standard-catalog member the published protocol + * docs give for the 409 constraint-violation body. After #14541 the two + * spellings sat side by side in one route's responses: a whole-request + * failure on `POST /data/:object/batch` said `UNIQUE_VIOLATION`, a row on the + * same route said `DUPLICATE_RECORD`. + * + * Maintainer ruling (2026-09-03, #14723): one wire spelling on every route, + * `UNIQUE_VIOLATION`; the row reports it too. So this limb maps the engine's + * envelope to the wire spelling BEFORE the verbatim registered-code rule — + * keyed exactly as the whole-request arm keys it (`error-response.ts`'s + * `structuredCodeAnswer`: the registered code AND the class name), never by + * message text. `httpStatus` still reads the declared 409 and `message` is + * still the envelope's platform sentence (a declared 4xx, so it is quoted); the + * driver's text stays on `cause`, which never reaches the row. + * + * ⛔ The engine's thrown identity is NOT renamed: `DuplicateRecordError.code` + * stays `DUPLICATE_RECORD` (an in-process caller — a hook, a flow node — + * branches on that). What moves is what a ROW REPORTS across the HTTP + * boundary. And a producer that merely SPEAKS `DUPLICATE_RECORD` — a hook + * throwing the registered code without being the engine's class — keeps its + * own code on the row, the same discrimination the whole-request arm makes. */ function toRowApiError(err: any, fallback: string): ApiError { - const thrown = typeof err?.code === 'string' && ErrorCode.safeParse(err.code).success - ? (err.code as ApiError['code']) - : undefined; + const thrown = isEngineDuplicateRecordEnvelope(err) + ? 'UNIQUE_VIOLATION' + : typeof err?.code === 'string' && ErrorCode.safeParse(err.code).success + ? (err.code as ApiError['code']) + : undefined; const { declaredStatus } = resolveThrownHttpError(err); return { code: thrown ?? (declaredStatus !== undefined ? standardErrorCodeForHttpStatus(declaredStatus) : 'INTERNAL_ERROR'), @@ -2044,6 +2075,25 @@ function toRowApiError(err: any, fallback: string): ApiError { }; } +/** + * [#14723] Is this thrown value the ENGINE's unique-violation envelope? + * + * The same two-part gate `@objectstack/rest`'s whole-request arm applies — + * the registered code AND the class name — so a batch row and a whole-request + * failure recognise the engine's envelope by one rule. Name alone would miss + * nothing today but would let a future class of the same name speak a + * different code; code alone would swallow a hook that deliberately throws + * the registered `DUPLICATE_RECORD` from its own body, which is a different + * producer speaking a member of the vocabulary and keeps its own spelling. + * The class itself is not imported: this package does not depend on + * `@objectstack/objectql`, and the envelope's contract is its declared + * `code` / `name` / `status`, not its prototype. + */ +function isEngineDuplicateRecordEnvelope(err: unknown): boolean { + const e = err as { code?: unknown; name?: unknown } | null | undefined; + return e?.code === 'DUPLICATE_RECORD' && e?.name === 'DuplicateRecordError'; +} + /** * [#8502] The stable sentence a failed batch row says when nothing may be * quoted — the operation, named, and no interpolation of any kind. diff --git a/packages/objectql/src/duplicate-record-error.ts b/packages/objectql/src/duplicate-record-error.ts index 971641742c..cffbc8b276 100644 --- a/packages/objectql/src/duplicate-record-error.ts +++ b/packages/objectql/src/duplicate-record-error.ts @@ -32,7 +32,18 @@ import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/type * * - **`code: 'DUPLICATE_RECORD'`** — already a member of `StandardErrorCode` * (the 409 conflict group), so nothing in `packages/spec` had to grow a - * member for this. It is what an application branches on, on every driver. + * member for this. It is the engine's THROWN identity — what an in-process + * caller of `engine.insert` / `engine.update` (a hook, a flow node, a + * script holding the engine) branches on, on every driver. ⛔ It is not + * the WIRE spelling: every REST route — the single-record door, the + * whole-request bulk / import doors, and since #14723 the per-row reports + * of `POST /data/:object/batch` and the import runner alike — reports a + * unique-constraint refusal as `UNIQUE_VIOLATION`, the standard-catalog + * member the published protocol docs give for the 409 constraint-violation + * body (maintainer ruling 2026-09-03: one wire spelling on every route). + * A client of the HTTP API branches on `UNIQUE_VIOLATION`; only code that + * catches this class in-process sees `DUPLICATE_RECORD`. One condition, + * one spelling per boundary, and `developerMessage` below says both. * - **`status: 409`** — the conflict status the engine's sibling refusals * already declare (`DELETE_RESTRICTED`, `CONCURRENT_UPDATE`), so REST's * declared-status passthrough answers 409 instead of the sanitised 500 an @@ -93,9 +104,11 @@ export class DuplicateRecordError extends Error { this.cause = cause; this.developerMessage = `The driver refused this write as a unique-constraint violation. Its own error is attached ` + - `as \`cause\` — branch on \`code === '${DUPLICATE_RECORD_CODE}'\` (ADR-0112) rather than on a ` + - `dialect's code or message, so the handling survives a change of store. To make the write ` + - `idempotent, catch this code and treat the row as already present.`; + `as \`cause\`. Branch on the platform code (ADR-0112), never on a dialect's code or message, ` + + `so the handling survives a change of store: over the HTTP API this refusal is reported as ` + + `\`code === 'UNIQUE_VIOLATION'\` on every route, whole-request and per-row alike; inside the ` + + `engine the thrown class carries \`code === '${DUPLICATE_RECORD_CODE}'\`. To make the write ` + + `idempotent, catch it and treat the row as already present.`; } } diff --git a/packages/rest/src/error-response.ts b/packages/rest/src/error-response.ts index 9e0787b8b8..7d21ae96cd 100644 --- a/packages/rest/src/error-response.ts +++ b/packages/rest/src/error-response.ts @@ -754,6 +754,34 @@ function fiveXxArmDisplacesDeclared4xx( return typeof declared === 'number' && declared >= 400 && declared < 500; } +/** + * [#14389 / #14723] Is this thrown value the ENGINE's unique-violation + * envelope — `@objectstack/objectql`'s `DuplicateRecordError`? + * + * Gated on the envelope, name AND code, not on the code alone: a hook that + * deliberately throws the registered `DUPLICATE_RECORD` from a sandbox body is + * a different producer speaking a member of the vocabulary and keeps its own + * sentence and its own code (`rest-duplicate-record-arm.test.ts` §5). The + * class is recognised by its declared contract rather than by `instanceof` + * so the import runner's row report, which never sees the class, applies the + * identical rule. `metadata-protocol`'s `toRowApiError` carries the same + * predicate for the batch rows — one rule, stated at each boundary that + * crosses to a client, so a whole-request failure and a row agree on which + * throws are the engine's envelope. + * + * The arm in {@link structuredCodeAnswer} spells the same two-part gate + * INLINE rather than calling this: `error-response-sandbox-arm-message.test.ts` + * §6 keys every arm of that classification by its `error?.code === '…'` + * literal (the drift guard that proves each producer-sentence relay asks the + * shared sandbox rule), and an arm hidden behind a call would drop out of + * that scan. Two spellings of one predicate in one file, each pinned: the + * import-row pin drives this function, the arm test drives the arm. + */ +export function isEngineDuplicateRecordEnvelope(error: unknown): boolean { + const e = error as { code?: unknown; name?: unknown } | null | undefined; + return e?.code === 'DUPLICATE_RECORD' && e?.name === 'DuplicateRecordError'; +} + /** * [#14541] The bespoke structured arms, in ONE place, so BOTH REST error doors * can ask them FIRST. @@ -829,28 +857,30 @@ function fiveXxArmDisplacesDeclared4xx( * not. ⛔ So "the code is new there" is the wrong way round; the withheld value * is the change. * - * ## ⚠️ A vocabulary fork this lifts onto routes where the other side lives - * - * Disclosed under the #14541 contract review (condition 2) rather than implied - * away by the "one condition, one wire code" framing above, which is true of - * the DOORS and not of the rows beside them. - * - * The `DUPLICATE_RECORD` arm answers the wire spelling `UNIQUE_VIOLATION` - * (#14389's ruling). A batch or import ROW does not go through this - * classification at all: `metadata-protocol`'s `toRowApiError` puts a thrown - * REGISTERED code on the row verbatim, and `import-runner`'s row report does - * the same, so a `DuplicateRecordError` row reports `DUPLICATE_RECORD` — - * deliberately, per #14095. ⇒ after this change a WHOLE-REQUEST failure on - * `POST /data/:object/batch` or `POST /data/:object/import` answers - * `UNIQUE_VIOLATION` while a ROW failure on the SAME route answers - * `DUPLICATE_RECORD`. Neither half is new and neither is a regression; what is - * new is that the two now sit side by side in one route's responses. - * - * ⛔ Not decided here, and deliberately not decided by this file: the ledger's - * "if it merely re-spells a standard member, that registration is a recorded - * waiver" and ADR-0112's one-name-per-concept both bear on it, and moving - * either spelling is a published-contract change rather than a door's call. - * **#14723 carries the decision.** + * ## One wire spelling on the route — the rows too (#14723) + * + * The #14541 contract review (condition 2) disclosed a fork this file's "one + * condition, one wire code" framing did not cover: the DOORS answered a + * `DuplicateRecordError` as `UNIQUE_VIOLATION` (#14389's ruling, the arm + * below), while a batch or import ROW did not go through this classification + * at all — `metadata-protocol`'s `toRowApiError` put the thrown REGISTERED + * code on the row verbatim and `import-runner`'s row report did the same — so + * after #14541 a whole-request failure on `POST /data/:object/batch` or + * `POST /data/:object/import` said `UNIQUE_VIOLATION` while a row on the SAME + * route said `DUPLICATE_RECORD`. + * + * Maintainer ruling (2026-09-03, #14723): a unique-constraint refusal has ONE + * wire spelling on every route, `UNIQUE_VIOLATION` — the standard-catalog + * member the published protocol docs give for the 409 constraint-violation + * body. The row derivations now apply the same mapping this arm applies, keyed + * the same way ({@link isEngineDuplicateRecordEnvelope}: registered code AND + * class name, never message text): `toRowApiError` for the rows of + * `POST /data/:object/batch`, and `toFailedResult` for the import runner's + * row reports. The single-record door's code did not move (#14389's refusal + * stands), no ledger waiver was added — the duplication is removed, not + * declared — and the ENGINE's thrown identity is unchanged: + * `DuplicateRecordError.code` is still `DUPLICATE_RECORD` in-process; only + * what crosses the HTTP boundary spells `UNIQUE_VIOLATION`. */ function structuredCodeAnswer( error: any, @@ -940,8 +970,8 @@ function structuredCodeAnswer( // engine's own sentence (`message`): it names the object and the column // and carries no value. The envelope's own `developerMessage` is // deliberately NOT relayed — it addresses the in-process caller of - // `engine.insert` ("attached as `cause`", "branch on `code === - // 'DUPLICATE_RECORD'`"), and neither holds on this wire. + // `engine.insert` ("attached as `cause`", and the in-process spelling + // beside the wire one), and `cause` never reaches this wire. // // **The body echoes nothing the driver said.** `cause` never reaches the // wire and the sentence is fixed text. This matters most for diff --git a/packages/rest/src/import-runner-unique-violation-row.test.ts b/packages/rest/src/import-runner-unique-violation-row.test.ts new file mode 100644 index 0000000000..f41ff14466 --- /dev/null +++ b/packages/rest/src/import-runner-unique-violation-row.test.ts @@ -0,0 +1,177 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14723] An import ROW reports a unique-constraint refusal as + * `UNIQUE_VIOLATION` — the one wire spelling the route has. + * + * ## The fork this closes + * + * `toFailedResult` relayed the thrown error's own `code`. The engine's + * `DuplicateRecordError` envelope carries the REGISTERED `DUPLICATE_RECORD`, + * so an import row said `DUPLICATE_RECORD` while the whole-request failure on + * the very same `POST /data/:object/import` answered `UNIQUE_VIOLATION` + * through `mapDataError` — two spellings of one condition in one route's + * responses. Maintainer ruling (2026-09-03): converge on `UNIQUE_VIOLATION`; + * the engine's thrown identity does not move. + * + * ## What is driven + * + * The REAL `DuplicateRecordError` class (what `p.createData` throws when the + * protocol relays the engine's refusal), through the real `runImport`, on both + * write paths the runner has — the per-row `createData` fallback and the + * batched `createManyData` path, whose logical failure degrades to per-row + * writes. §3 pins the door-to-row agreement directly: the row's `code` IS the + * whole-request door's `code` for the same envelope. + * + * ## Reverse verification — predicted before running + * + * Revert the mapping in `toFailedResult` (relay `e?.code` again): §1 and §2 + * go red with the row reading `DUPLICATE_RECORD`; §3's parity case goes red + * the same way; the two GUARD cases stay green in both directions, because + * they pin what the mapping must NOT do. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { DuplicateRecordError } from '@objectstack/objectql'; +import { uniqueViolationColumn } from '@objectstack/types'; +import { runImport, type ImportProtocolLike } from './import-runner'; +import { mapDataError } from './error-response.js'; +import type { ExportFieldMeta } from './export-format.js'; + +const metaMap = new Map([ + ['name', { name: 'name', type: 'text' }], +]); + +const baseOpts = { + objectName: 'task', + metaMap, + writeMode: 'insert' as const, + matchFields: [] as string[], + dryRun: false, + runAutomations: false, + trimWhitespace: true, + createMissingOptions: false, + skipBlankMatchKey: false, +}; + +/** The offending user data — must never reach the row report. */ +const OFFENDING_VALUE = 'dup@example.com'; + +/** better-sqlite3 via knex: the compiled statement, with the bound value, and the column behind it. */ +const sqliteRaw = () => + Object.assign( + new Error( + `insert into \`task\` (\`name\`, \`id\`) values ('${OFFENDING_VALUE}', 'x') returning * - ` + + 'UNIQUE constraint failed: task.name', + ), + { code: 'SQLITE_CONSTRAINT_UNIQUE' }, + ); + +/** The engine's envelope exactly as `engine.insert` builds it. */ +const envelope = () => new DuplicateRecordError('task', sqliteRaw(), uniqueViolationColumn(sqliteRaw())); + +function protocolWith(overrides: Partial): ImportProtocolLike { + return { + findData: vi.fn(async () => []), + createData: vi.fn(async (args: { data: { name: string } }) => ({ id: `id_${args.data.name}` })), + updateData: vi.fn(), + ...overrides, + }; +} + +function expectNothingLeaked(summary: unknown): void { + const payload = JSON.stringify(summary); + expect(payload).not.toContain('DUPLICATE_RECORD'); + expect(payload).not.toContain('insert into'); + expect(payload).not.toContain('UNIQUE constraint failed'); + expect(payload).not.toContain('SQLITE_CONSTRAINT'); + expect(payload).not.toContain(OFFENDING_VALUE); +} + +describe('[#14723] §1 — the per-row `createData` path', () => { + it('a `DuplicateRecordError` row reports `UNIQUE_VIOLATION`, and nothing of the driver', async () => { + const p = protocolWith({ + createData: vi.fn(async (args: { data: { name: string } }) => { + if (args.data.name === 'r1') throw envelope(); + return { id: `id_${args.data.name}` }; + }), + }); + + const summary = await runImport({ ...baseOpts, p, rows: [{ name: 'r0' }, { name: 'r1' }, { name: 'r2' }] }); + + expect(summary.errors).toBe(1); + expect(summary.created).toBe(2); + expect(summary.results[1]).toMatchObject({ row: 2, ok: false, action: 'failed', code: 'UNIQUE_VIOLATION' }); + // The sentence is the sanitised platform sentence, not the driver's. + expect(typeof summary.results[1].error).toBe('string'); + expectNothingLeaked(summary); + }); +}); + +describe('[#14723] §2 — the batched `createManyData` path, degraded to per-row writes', () => { + it('the conflicting row alone reports `UNIQUE_VIOLATION`; its siblings are created', async () => { + const createManyData = vi.fn(async () => { throw envelope(); }); + const createData = vi.fn(async (args: { data: { name: string } }) => { + if (args.data.name === 'r1') throw envelope(); + return { id: `id_${args.data.name}` }; + }); + const p = protocolWith({ createData, createManyData }); + + const summary = await runImport({ ...baseOpts, p, rows: [{ name: 'r0' }, { name: 'r1' }, { name: 'r2' }] }); + + expect(createManyData).toHaveBeenCalledTimes(1); + expect(createData).toHaveBeenCalledTimes(3); + expect(summary.results[0]).toMatchObject({ ok: true, action: 'created' }); + expect(summary.results[1]).toMatchObject({ ok: false, action: 'failed', code: 'UNIQUE_VIOLATION' }); + expect(summary.results[2]).toMatchObject({ ok: true, action: 'created' }); + expectNothingLeaked(summary); + }); +}); + +describe('[#14723] §3 — the row and the whole-request door agree on the spelling', () => { + it('the same envelope answers `UNIQUE_VIOLATION` on both surfaces of the route', async () => { + const env = envelope(); + const door = mapDataError(env, 'task'); + expect(door.status).toBe(409); + expect(door.body.code).toBe('UNIQUE_VIOLATION'); + + const p = protocolWith({ createData: vi.fn(async () => { throw env; }) }); + const summary = await runImport({ ...baseOpts, p, rows: [{ name: 'r1' }] }); + + expect(summary.results[0].code).toBe(door.body.code); + }); + + it('anti-vacuity: the envelope\'s own code is the OTHER registered spelling — the mapping, not the relay, decides', () => { + const env = envelope(); + expect(env.code).toBe('DUPLICATE_RECORD'); + expect(env.name).toBe('DuplicateRecordError'); + expect(env.status).toBe(409); + expect(env.field).toBe('name'); + }); +}); + +describe('[#14723] [GUARD] what the mapping must NOT do', () => { + it('a producer that merely SPEAKS `DUPLICATE_RECORD` is not the engine\'s envelope and keeps its own code', async () => { + // The same discrimination the whole-request arm makes (#14389 §5): the + // gate is the registered code AND the class name. + const hookRefusal = Object.assign(new Error('already there, says the hook'), { code: 'DUPLICATE_RECORD', status: 409 }); + const p = protocolWith({ createData: vi.fn(async () => { throw hookRefusal; }) }); + + const summary = await runImport({ ...baseOpts, p, rows: [{ name: 'r1' }] }); + + expect(summary.results[0]).toMatchObject({ ok: false, code: 'DUPLICATE_RECORD', error: 'already there, says the hook' }); + }); + + it('a field-level finding still wins over the thrown code (#4633 precedence is untouched)', async () => { + const validation = Object.assign(new Error('name must be ≤ 4 characters'), { + code: 'VALIDATION_FAILED', + name: 'ValidationError', + fields: [{ field: 'name', code: 'max_length', message: 'name must be ≤ 4 characters' }], + }); + const p = protocolWith({ createData: vi.fn(async () => { throw validation; }) }); + + const summary = await runImport({ ...baseOpts, p, rows: [{ name: 'r1' }] }); + + expect(summary.results[0]).toMatchObject({ ok: false, code: 'max_length', field: 'name' }); + }); +}); diff --git a/packages/rest/src/import-runner.ts b/packages/rest/src/import-runner.ts index 39c5885eee..3a4895c733 100644 --- a/packages/rest/src/import-runner.ts +++ b/packages/rest/src/import-runner.ts @@ -7,6 +7,7 @@ import type { ValidationMessageTranslator } from '@objectstack/spec/system'; import type { ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api'; import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core'; import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/types'; +import { isEngineDuplicateRecordEnvelope } from './error-response.js'; /** * import-runner — the shared row-processing core for bulk import. @@ -285,12 +286,29 @@ export function sanitizeRowError(raw: unknown): string { * * `code` therefore speaks one vocabulary across the whole row report: the * field-level catalog (ADR-0114) that `coerceRow`'s cell failures already use. + * + * ## One wire spelling for a unique-constraint refusal (#14723) + * + * The engine answers a driver's unique violation with its `DuplicateRecordError` + * envelope (`code: 'DUPLICATE_RECORD'`, `status: 409`, the driver's error on + * `cause`), and this report used to relay that code verbatim — while the + * WHOLE-REQUEST failure on the very same `POST /data/:object/import` answered + * `UNIQUE_VIOLATION` through `mapDataError`. Maintainer ruling (2026-09-03, + * #14723): one wire spelling on every route. So the engine's envelope is mapped + * to `UNIQUE_VIOLATION` here, by the same predicate the whole-request arm uses + * ({@link isEngineDuplicateRecordEnvelope}: registered code AND class name), + * before the producer's own code is read. A field-level finding still wins + * (the envelope carries none), and a producer that merely SPEAKS the registered + * `DUPLICATE_RECORD` without being the engine's class keeps its own code, as it + * does at the door. The engine's thrown identity is unchanged; only the row's + * wire spelling moves. */ function toFailedResult(rowNo: number, err: unknown): ImportRowResult { const e = err as { code?: unknown; message?: unknown; fields?: unknown } | null | undefined; const fields = Array.isArray(e?.fields) ? (e.fields as Array<{ field?: unknown; code?: unknown }>) : []; const first = fields[0]; - const code = first?.code ?? e?.code ?? 'IMPORT_ROW_FAILED'; + const thrownCode = isEngineDuplicateRecordEnvelope(e) ? 'UNIQUE_VIOLATION' : e?.code; + const code = first?.code ?? thrownCode ?? 'IMPORT_ROW_FAILED'; const message = sanitizeRowError(e?.message); return { row: rowNo, ok: false, action: 'failed', error: message, code: String(code), diff --git a/packages/rest/src/rest-duplicate-record-arm.test.ts b/packages/rest/src/rest-duplicate-record-arm.test.ts index 11dbc2d4fc..e2ff3651d4 100644 --- a/packages/rest/src/rest-duplicate-record-arm.test.ts +++ b/packages/rest/src/rest-duplicate-record-arm.test.ts @@ -236,9 +236,9 @@ describe('#14389 §1 — the engine\'s DUPLICATE_RECORD envelope answers 409 UNI 'No record was written.', ); // The envelope's OWN `developerMessage` addresses the in-process caller - // of `engine.insert` ("attached as `cause`", "branch on `code === - // 'DUPLICATE_RECORD'`") and neither holds on this wire — it is not - // relayed. + // of `engine.insert` ("attached as `cause`", and the in-process + // spelling beside the wire one — #14723) and `cause` never reaches + // this wire — it is not relayed. expect(r.body.developerMessage).not.toBe(env.developerMessage); expect(String(r.body.developerMessage)).not.toContain('cause'); }); diff --git a/packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts b/packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts index 5312ffc6ae..2f7f50a625 100644 --- a/packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts +++ b/packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts @@ -279,8 +279,21 @@ describe('[#8502] a REAL driver fault is withheld from every batch row', () => { // The row now carries the machine-readable half too, which is what an // idempotent batch writer branches on — it was `INTERNAL_ERROR` with no // status while the sentence was withheld. - expect(res.results[0].errors[0].code).toBe('DUPLICATE_RECORD'); + // + // ── [#14723] …and the row speaks the WIRE spelling ───────────────── + // The engine's envelope is `code: 'DUPLICATE_RECORD'` in-process (the + // objectql pins on `insert` / `insertMany` hold that), and this row + // used to relay it verbatim while the whole-request failure on the + // same route answered `UNIQUE_VIOLATION`. Maintainer ruling + // (2026-09-03): one wire spelling on every route. `toRowApiError` now + // maps the engine's envelope — name AND code, the whole-request arm's + // own gate — to `UNIQUE_VIOLATION`; the status and the platform + // sentence above are untouched by it. Pinned over the whole payload + // so a sibling `NOT_ATTEMPTED` / `ROLLED_BACK` row cannot carry the + // other spelling either. + expect(res.results[0].errors[0].code).toBe('UNIQUE_VIOLATION'); expect(res.results[0].errors[0].httpStatus).toBe(409); + expect(payload).not.toContain('DUPLICATE_RECORD'); // ── …and NOT ONE leak assertion moved ────────────────────────────── // These are what this file exists for, and they hold against the new