From c4e841368b03361488523de711caccffa339873d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 11:37:27 +0000 Subject: [PATCH 1/2] fix(driver-memory): make bulkCreate all-or-nothing so a refused batch leaves no prefix `InMemoryDriver.bulkCreate` was `Promise.all(dataArray.map(data => this.create(...)))`, and `create` writes into the table synchronously, so a batch was not atomic: when any row was refused, every row accepted BEFORE it stayed in the store. Measured on a two-row table, a refused two-row batch left three rows behind, so a caller's retry of the same array was unsafe for reasons unrelated to constraints. Build and check every row -- against the table AND against the rest of the batch -- before pushing any of them. This is the posture `updateMany` has had since #13197, one method over, and the one `driver-sql` gets from sending a batch as a single insert. `create`'s own single-row path is untouched, and the record-building expression is copied from it deliberately so the two doors cannot drift on what a row looks like. The recorded-behaviour assertion in memory-declared-index-unique.test.ts is INVERTED in place with the old numbers kept in the comment, not re-baselined. The field-level suite gains the row-count control it lacked -- asserting the envelope alone was vacuous, since the refusal was already correct while the first batch row was still landing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- .changeset/bulk-create-all-or-nothing.md | 22 ++ .../src/memory-bulk-create-atomicity.test.ts | 205 ++++++++++++++++++ .../src/memory-declared-index-unique.test.ts | 25 ++- .../driver-memory/src/memory-driver.ts | 63 +++++- .../src/memory-unique-constraint.test.ts | 7 + 5 files changed, 307 insertions(+), 15 deletions(-) create mode 100644 .changeset/bulk-create-all-or-nothing.md create mode 100644 packages/drivers/driver-memory/src/memory-bulk-create-atomicity.test.ts diff --git a/.changeset/bulk-create-all-or-nothing.md b/.changeset/bulk-create-all-or-nothing.md new file mode 100644 index 0000000000..b441d1aba6 --- /dev/null +++ b/.changeset/bulk-create-all-or-nothing.md @@ -0,0 +1,22 @@ +--- +"@objectstack/driver-memory": patch +--- + +fix(driver-memory): `bulkCreate` refuses before writing, so a rejected batch leaves no surviving prefix (#13340) + +`InMemoryDriver.bulkCreate` was `Promise.all(dataArray.map(data => this.create(...)))`, and +`create` writes into the table synchronously. So a batch was **not atomic**: when any row was +refused, every row accepted *before* it stayed in the store, and the caller got a rejection +describing a batch that had partly landed. Measured on a two-row table, a refused two-row +batch left **three** rows behind, which made retrying the same array unsafe for reasons that +had nothing to do with constraints. + +`bulkCreate` now builds and checks every row — against the table **and against the rest of +the batch** — before pushing any of them. That is the posture `updateMany` has had since +#13197, one method over, and the one `driver-sql` gets from sending a batch as a single +insert; the two batch doors of this driver no longer give opposite answers to "is a batch +atomic?". + +`create`'s own single-row path is unchanged, and this does not give the store a primary key: +an undeclared duplicate `id` is still not a constraint violation, so such a batch still lands +in full. diff --git a/packages/drivers/driver-memory/src/memory-bulk-create-atomicity.test.ts b/packages/drivers/driver-memory/src/memory-bulk-create-atomicity.test.ts new file mode 100644 index 0000000000..461a446ab2 --- /dev/null +++ b/packages/drivers/driver-memory/src/memory-bulk-create-atomicity.test.ts @@ -0,0 +1,205 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13340] `bulkCreate` is ALL-OR-NOTHING: a refused row leaves the table + * exactly as it found it. + * + * ## The defect this pins, and why the obvious assertion would not have caught it + * + * `bulkCreate` used to be one line — `Promise.all(dataArray.map(data => + * this.create(object, data, options)))` — and `create` writes into the table + * synchronously. So when any row of a batch was refused, every row accepted + * BEFORE it stayed in the store and the caller got a rejection describing a + * batch that had partly landed. Measured on `main` before the fix, on a + * two-row table whose incoming batch collides with itself: + * + * ``` + * before: 2 rows + * bulkCreate([A9/Z, A9/Z]) -> rejects UNIQUE_VIOLATION / 409 + * after: 3 rows <- the FIRST batch row landed. That is the defect. + * ``` + * + * ⛔ Asserting "the refusal still happens" proves NOTHING here — the refusal + * was already correct, and #13197 / #13239 already pin it. That assertion was + * green throughout the defect's entire life. **The discriminating fact is that + * THE ROW COUNT DOES NOT MOVE**, so every test below reads the store after the + * refusal rather than stopping at the envelope. + * + * The fix is `updateMany`'s posture, one method over: #13197 made that method + * prepare and check every row before mutating any of it, precisely so a + * half-applied batch cannot happen. The two batch paths of this driver now + * give the SAME answer to "is a batch atomic?" — they disagreed for as long as + * `bulkCreate` existed, seven lines apart in one file. + * + * It is also the batch posture of the family this driver stands in for: + * `driver-sql` sends the whole batch as one insert, so a constraint failure + * there leaves the table untouched. + * + * ## What this does NOT claim + * + * Atomicity here is the driver refusing before it writes — not a transaction. + * There is no rollback: nothing is written until the whole batch has been + * checked. And the store still has **no primary key** (see the boundary + * describe at the bottom): an undeclared duplicate `id` is not a constraint + * violation on this driver, so such a batch lands in full. Atomicity is about + * what happens when a row IS refused, not about what gets refused. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { InMemoryDriver } from './memory-driver.js'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +/** Run `fn`, requiring it to reject; hand back the rejection for inspection. */ +async function refusalOf(fn: () => Promise): Promise { + try { + await fn(); + } catch (e) { + return e as WireBearingError; + } + throw new Error('expected the driver to refuse this write, but it resolved'); +} + +const DOC_SCHEMA = { + name: 'doc', + fields: { + id: { type: 'text' }, + doc_no: { type: 'text', unique: 'global' }, + title: { type: 'text' }, + }, +} as any; + +describe('[#13340] bulkCreate refuses BEFORE writing — no surviving prefix', () => { + let driver: InMemoryDriver; + + beforeEach(async () => { + driver = new InMemoryDriver(); + await driver.syncSchema('doc', DOC_SCHEMA); + await driver.create('doc', { id: '1', doc_no: 'D-0001' }); + await driver.create('doc', { id: '2', doc_no: 'D-0002' }); + }); + + it('a batch colliding with ITSELF leaves the row count where it was', async () => { + // The card's own measured reading, inverted: this used to leave 3 rows. + const err = await refusalOf(() => + driver.bulkCreate('doc', [ + { id: 'a', doc_no: 'D-0100' }, + { id: 'b', doc_no: 'D-0100' }, + ]), + ); + expect(err.code).toBe('UNIQUE_VIOLATION'); + expect(err.status).toBe(409); + + expect(await driver.count('doc')).toBe(2); + // Named explicitly: it is the row ACCEPTED BEFORE the refusal that used to + // survive, so its absence is the whole fix. + expect(await driver.find('doc', { fields: ['id'], where: { id: 'a' } })).toHaveLength(0); + expect(await driver.find('doc', { fields: ['id'], where: { id: 'b' } })).toHaveLength(0); + }); + + it('a batch colliding with a STORED row leaves the row count where it was', async () => { + // The collision is in the LAST row, so rows 'a' and 'b' were both accepted + // before the refusal — two survivors under the old behaviour, not one. + const err = await refusalOf(() => + driver.bulkCreate('doc', [ + { id: 'a', doc_no: 'D-0100' }, + { id: 'b', doc_no: 'D-0101' }, + { id: 'c', doc_no: 'D-0001' }, + ]), + ); + expect(err.code).toBe('UNIQUE_VIOLATION'); + expect(err.status).toBe(409); + + expect(await driver.count('doc')).toBe(2); + const survivors = await driver.find('doc', { fields: ['id'] }); + expect(survivors.map((r: any) => r.id).sort()).toEqual(['1', '2']); + }); + + it('the FIRST row colliding refuses the batch too — the check is not order-dependent', async () => { + const err = await refusalOf(() => + driver.bulkCreate('doc', [ + { id: 'a', doc_no: 'D-0001' }, + { id: 'b', doc_no: 'D-0200' }, + ]), + ); + expect(err.code).toBe('UNIQUE_VIOLATION'); + expect(await driver.count('doc')).toBe(2); + // The row AFTER the refusal must not land either — a check that bailed out + // of the loop but had already pushed would still pass the count above if + // it pushed nothing, so name the later row directly. + expect(await driver.find('doc', { fields: ['id'], where: { id: 'b' } })).toHaveLength(0); + }); + + it('a clean batch still lands in FULL and returns every row', async () => { + // The non-vacuity control for the controls: a fix that refused everything + // would pass every assertion above. + const out = await driver.bulkCreate('doc', [ + { id: 'a', doc_no: 'D-0100' }, + { id: 'b', doc_no: 'D-0101' }, + ]); + expect(out).toHaveLength(2); + expect(out.map((r: any) => r.doc_no)).toEqual(['D-0100', 'D-0101']); + expect(await driver.count('doc')).toBe(4); + }); + + it('an empty batch is a no-op that resolves to an empty array', async () => { + expect(await driver.bulkCreate('doc', [])).toEqual([]); + expect(await driver.count('doc')).toBe(2); + }); + + it('the two batch doors of this driver now AGREE that a batch is atomic', async () => { + // The card's actual complaint: `bulkCreate` and `updateMany` gave opposite + // answers to one question, in one file, seven lines apart. Both are asked + // here so the pair cannot drift apart again silently. + const createErr = await refusalOf(() => + driver.bulkCreate('doc', [ + { id: 'a', doc_no: 'D-0100' }, + { id: 'b', doc_no: 'D-0100' }, + ]), + ); + const updateErr = await refusalOf(() => driver.updateMany('doc', { where: {} }, { doc_no: 'D-0009' })); + + expect(createErr.code).toBe('UNIQUE_VIOLATION'); + expect(updateErr.code).toBe('UNIQUE_VIOLATION'); + // Neither door moved the store. + expect(await driver.count('doc')).toBe(2); + const rows = await driver.find('doc', { fields: ['id', 'doc_no'], orderBy: [{ field: 'id', order: 'asc' }] }); + expect(rows.map((r: any) => r.doc_no)).toEqual(['D-0001', 'D-0002']); + }); +}); + +describe('[#13340] the boundary — atomicity did NOT give this store a primary key', () => { + it('an UNDECLARED duplicate `id` is still not a violation, so that batch lands in full', async () => { + // Load-bearing for the docstring on `InMemoryDriver`, which tells readers + // this store has no primary key and that `bulkCreate` lands two rows with + // the same `id` where a SQL driver raises. #13340 did NOT change that: the + // batch is refused only when a DECLARED constraint refuses a row, and + // `id` here declares nothing. Without this test the atomicity work above + // reads as "bulkCreate now rejects duplicate ids", which it does not. + const driver = new InMemoryDriver(); + await driver.syncSchema('note', { + name: 'note', + fields: { id: { type: 'text' }, body: { type: 'text' } }, + } as any); + + const out = await driver.bulkCreate('note', [ + { id: 'dup', body: 'first' }, + { id: 'dup', body: 'second' }, + ]); + + expect(out).toHaveLength(2); + expect(await driver.count('note')).toBe(2); + // ...and a read returns BOTH, exactly as the docstring says. + expect(await driver.find('note', { fields: ['id', 'body'], where: { id: 'dup' } })).toHaveLength(2); + }); + + it('an object never passed through syncSchema is unconstrained, batch included', async () => { + const driver = new InMemoryDriver(); + const out = await driver.bulkCreate('undeclared', [{ id: 'x', v: 1 }, { id: 'x', v: 2 }]); + expect(out).toHaveLength(2); + expect(await driver.count('undeclared')).toBe(2); + }); +}); diff --git a/packages/drivers/driver-memory/src/memory-declared-index-unique.test.ts b/packages/drivers/driver-memory/src/memory-declared-index-unique.test.ts index 1b48bd0726..a3d0b1d2c8 100644 --- a/packages/drivers/driver-memory/src/memory-declared-index-unique.test.ts +++ b/packages/drivers/driver-memory/src/memory-declared-index-unique.test.ts @@ -473,19 +473,26 @@ describe('[#13239] every write path is checked, not just create', () => { ]), ); expectUniqueViolationEnvelope(err, 'account_id', 'code'); - // The DUPLICATE did not land — that is the constraint, and it is what this - // asserts. ⚠️ `bulkCreate` is `Promise.all(map(create))`, so rows accepted - // BEFORE the refusal stay: the batch is not atomic. That is older than - // either uniqueness card (any mid-batch `create` failure has always left a - // partial batch) and identical on the field surface, so it is recorded here - // as a known boundary rather than silently re-baselined — never "the - // constraint half-applied". + // [#13340] ⛔ The refusal is NOT what this test is for — the refusal was + // already correct, and #13197/#13239 already pin it. What it asserts is + // that THE ROW COUNT DOES NOT MOVE: the batch is all-or-nothing. + // + // This assertion was INVERTED in place, not re-baselined. It used to read + // `toHaveLength(1)` / `toBe(3)` and recorded the opposite behaviour as a + // known boundary: `bulkCreate` was `Promise.all(map(create))`, so the row + // accepted BEFORE the refusal stayed in the store and a refused 2-row + // batch left a 2-row table holding THREE rows. #13340 gave `bulkCreate` + // the check-then-push posture `updateMany` has had since #13197, so + // neither row lands now. The old numbers are kept in this comment + // deliberately: they are the discriminating reading, and an assertion + // that only checked "the refusal still happens" would pass in both + // worlds. const colliding = await driver.find('ledger', { fields: ['id'], where: { account_id: 'A9', code: 'Z' }, }); - expect(colliding).toHaveLength(1); - expect(await driver.count('ledger')).toBe(3); + expect(colliding).toHaveLength(0); + expect(await driver.count('ledger')).toBe(2); }); it('updateMany refuses BEFORE mutating anything — no half-applied batch', async () => { diff --git a/packages/drivers/driver-memory/src/memory-driver.ts b/packages/drivers/driver-memory/src/memory-driver.ts index 2cc2341659..9f1201ff8a 100644 --- a/packages/drivers/driver-memory/src/memory-driver.ts +++ b/packages/drivers/driver-memory/src/memory-driver.ts @@ -202,12 +202,26 @@ interface MemoryTransaction { * (`createWithAutonumberResync`) is triggered by the STORE rejecting the * duplicate and this store rejected nothing. * + * Since #13340 a batch write is also ALL-OR-NOTHING: {@link bulkCreate} builds + * and checks every row — against the table AND against the rest of the batch — + * before it pushes any of them, so a refused row leaves the table exactly as + * it found it. That is the posture {@link updateMany} has had since #13197, + * and the one `driver-sql` gets from sending a batch as a single insert. + * `bulkCreate` used to be `Promise.all(map(create))`, where a refusal left + * every row accepted BEFORE it standing — a refused 2-row batch on a 2-row + * table left THREE rows — which made a caller's retry of the same array + * unsafe for reasons that had nothing to do with constraints. + * * Everything else is still unenforced. {@link syncSchema} allocates an array, * indexes temporal fields and records those unique constraints — and nothing * more — so there is no primary key, no `NOT NULL`, no foreign key and no - * column typing. `bulkCreate` will still happily land two rows with the same - * `id` (unless `id` itself declares `unique`, or a declared index lists it) - * where a SQL driver raises a constraint violation, and a read returns both. + * column typing. Two rows with the same `id` still land — through `create` and + * `bulkCreate` alike, unless `id` itself declares `unique` or a declared index + * lists it — where a SQL driver raises a constraint violation, and a read + * returns both. ⚠️ Read that as the missing primary key, NOT as a batch that + * half-applied: an undeclared duplicate is not a refusal at all, so there is + * nothing to refuse and the whole batch lands (pinned in + * `memory-bulk-create-atomicity.test.ts`). * A declared index WITHOUT `unique` is an access path and buys nothing here: * this store is a linear scan. * @@ -611,9 +625,46 @@ export class InMemoryDriver implements IDataDriver { async bulkCreate(object: string, dataArray: Record[], options?: DriverOptions): Promise[]> { this.logger.debug('BulkCreate operation', { object, count: dataArray.length }); - const results = await Promise.all(dataArray.map(data => this.create(object, data, options))); - this.logger.debug('BulkCreate completed', { object, count: results.length }); - return results; + + const table = this.getTable(object); + + // [#13340] Build and CHECK every row before pushing ANY of them — the + // posture `updateMany` took in #13197, one method over. This used to be + // `Promise.all(dataArray.map(data => this.create(...)))`, and `create` + // writes into the table synchronously, so every row accepted BEFORE a + // refusal stayed in the store: the caller got a rejection describing a + // batch that had partly landed, and retrying the same array was unsafe + // for reasons that had nothing to do with constraints. Measured on a + // 2-row table, a refused 2-row batch left THREE rows behind. + // + // The pending rows are checked against the table AND against each other, + // which a per-row check against `table` alone would miss — nothing is + // written yet, so a duplicate WITHIN the batch is invisible to the live + // table. Same reason `updateMany` projects its own pending set. + // + // This is the batch posture of the family this driver stands in for: + // `driver-sql` sends the whole batch as one insert, so a constraint + // failure there leaves the table untouched. ⭐ `create`'s own single-row + // path is unchanged — the record-building expression below is a copy of + // it on purpose, so the two doors cannot drift on what a row looks like + // (own-key-`undefined` included). + const pending: Record[] = []; + for (const data of dataArray) { + const newRecord = this.toStoredRecord(object, { + id: data.id || this.generateId(object), + ...data, + created_at: data.created_at || new Date().toISOString(), + updated_at: data.updated_at || new Date().toISOString(), + }); + this.assertUnique(object, newRecord, undefined, [...table, ...pending]); + pending.push(newRecord); + } + + for (const row of pending) table.push(row); + + if (pending.length > 0) this.markDirty(); + this.logger.debug('BulkCreate completed', { object, count: pending.length }); + return pending.map((row) => ({ ...row })); } async updateMany(object: string, query: DriverQuery, data: Record, options?: DriverOptions): Promise { diff --git a/packages/drivers/driver-memory/src/memory-unique-constraint.test.ts b/packages/drivers/driver-memory/src/memory-unique-constraint.test.ts index 57baca4279..1fffada3b6 100644 --- a/packages/drivers/driver-memory/src/memory-unique-constraint.test.ts +++ b/packages/drivers/driver-memory/src/memory-unique-constraint.test.ts @@ -150,6 +150,13 @@ describe('[#13197] every write path goes through the constraint, not just create driver.bulkCreate('doc', [{ id: 'a', doc_no: 'D-0100' }, { id: 'b', doc_no: 'D-0100' }]), ); expectUniqueViolationEnvelope(err, 'doc_no'); + + // [#13340] The envelope alone is a VACUOUS control here — the refusal was + // already correct before the batch was made atomic, so this assertion + // passed while the first row of the batch was still landing. The row + // count is what discriminates: 2 before, 2 after, nothing half-applied. + expect(await driver.count('doc')).toBe(2); + expect(await driver.find('doc', { fields: ['id'], where: { doc_no: 'D-0100' } })).toHaveLength(0); }); it('updateMany refuses BEFORE mutating anything — no half-applied batch', async () => { From 714b7b46a84c609a6de22a320457ee291c90a917 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 11:50:59 +0000 Subject: [PATCH 2/2] docs(driver-memory): scope the new batch-atomicity paragraph to bulkCreate and updateMany The paragraph added with the bulkCreate fix opened "a batch write is ALL-OR-NOTHING", which reads as covering all four batch doors. It does not: `bulkUpdate` and `bulkDelete` are still `Promise.all(map(...))` and still leave the rows applied before a refusal standing (filed as #13435). Name them, so this docstring does not assert behaviour the code has for only two of them -- the same defect class the `bulkCreate` sentence at the top of this file had before it was corrected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- .../driver-memory/src/memory-driver.ts | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/drivers/driver-memory/src/memory-driver.ts b/packages/drivers/driver-memory/src/memory-driver.ts index 9f1201ff8a..ae365253c8 100644 --- a/packages/drivers/driver-memory/src/memory-driver.ts +++ b/packages/drivers/driver-memory/src/memory-driver.ts @@ -202,15 +202,20 @@ interface MemoryTransaction { * (`createWithAutonumberResync`) is triggered by the STORE rejecting the * duplicate and this store rejected nothing. * - * Since #13340 a batch write is also ALL-OR-NOTHING: {@link bulkCreate} builds - * and checks every row — against the table AND against the rest of the batch — - * before it pushes any of them, so a refused row leaves the table exactly as - * it found it. That is the posture {@link updateMany} has had since #13197, - * and the one `driver-sql` gets from sending a batch as a single insert. - * `bulkCreate` used to be `Promise.all(map(create))`, where a refusal left - * every row accepted BEFORE it standing — a refused 2-row batch on a 2-row - * table left THREE rows — which made a caller's retry of the same array - * unsafe for reasons that had nothing to do with constraints. + * Since #13340 {@link bulkCreate} is ALL-OR-NOTHING: it builds and checks every + * row — against the table AND against the rest of the batch — before it pushes + * any of them, so a refused row leaves the table exactly as it found it. That + * is the posture {@link updateMany} has had since #13197, and the one + * `driver-sql` gets from sending a batch as a single insert. `bulkCreate` used + * to be `Promise.all(map(create))`, where a refusal left every row accepted + * BEFORE it standing — a refused 2-row batch on a 2-row table left THREE rows + * — which made a caller's retry of the same array unsafe for reasons that had + * nothing to do with constraints. + * + * ⚠️ That is a statement about those TWO doors, not about batches in general. + * {@link bulkUpdate} and {@link bulkDelete} are still `Promise.all(map(...))` + * and still leave the rows applied before a refusal standing — #13435. Do not + * read the paragraph above as covering them. * * Everything else is still unenforced. {@link syncSchema} allocates an array, * indexes temporal fields and records those unique constraints — and nothing