From a067f54a5a84051eff598e90ceb906dff2fe4dff Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 16:51:28 +0000 Subject: [PATCH 1/3] fix(driver-memory): bulkUpdate's touched-row set must agree with its own id resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bulkUpdate` resolved each id with a loose `==` (matching `update`, one method up) but built its untouched-row set `settled` from the CALLER's ids with a strict `Set.has`. `IDataDriver.bulkUpdate` declares `id: string | number`, so a caller may name a stored `1` as `'1'` — and then the two lookups disagreed: `findIndex` resolved the row and updated it, while `settled` still carried that row's PRE-image. The row was represented twice in the projected set handed to `assertUnique` — once with the value it was vacating, once with the value it was taking — so a batch that merely HANDS a unique value from one row to another was refused with a false `UNIQUE_VIOLATION` / 409. `exceptId` does not help, since it only excludes the row currently being checked, never a sibling row of the same batch. Resolve every id to its table index first, then derive the touched set from the RESOLVED rows' own ids. Both lookups now read the same stored value and cannot drift apart — the property `updateMany` gets for free by drawing its `targetIds` from table rows. The loose resolution is deliberately preserved: narrowing it to `===` would silently change which ids resolve at all, well beyond this defect. `bulkDelete` needs no change, and this is checked rather than assumed: it has exactly ONE id comparison, and dedups on the RESOLVED table index rather than on caller input, so a mixed-type or repeated id collapses to one index by construction. Keying that set on caller ids instead would splice one index twice and take a neighbouring row with it — pinned by test. Regression test uses mixed id types, alongside a positive control with consistent id types so it cannot pass vacuously. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- ...ver-memory-bulkupdate-id-type-agreement.md | 29 +++++ ...emory-bulk-update-delete-atomicity.test.ts | 106 ++++++++++++++++++ .../driver-memory/src/memory-driver.ts | 25 ++++- 3 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 .changeset/driver-memory-bulkupdate-id-type-agreement.md diff --git a/.changeset/driver-memory-bulkupdate-id-type-agreement.md b/.changeset/driver-memory-bulkupdate-id-type-agreement.md new file mode 100644 index 0000000000..fb90be2da9 --- /dev/null +++ b/.changeset/driver-memory-bulkupdate-id-type-agreement.md @@ -0,0 +1,29 @@ +--- +"@objectstack/driver-memory": patch +--- + +fix(driver-memory): `bulkUpdate`'s touched-row set now agrees with its own id resolution, so a mixed id-type batch is no longer false-refused (#13911) + +`IDataDriver.bulkUpdate` declares `id: string | number`, and this driver +resolves an id to a row with a loose comparison — the way `update` and +`delete` always have — so naming a stored `1` as `'1'` finds the same row. +The all-or-nothing rework shipped one release earlier then built its +untouched-row set from the *caller's* ids using strict `Set` membership, so +for a mixed-type id the two lookups disagreed: the row was resolved and +updated, yet also stayed in the untouched set carrying its **pre-image**. It +faced the uniqueness check twice — once with the value it was vacating, once +with the value it was taking — and a batch that merely HANDS a unique value +from one row to another was refused with a false `UNIQUE_VIOLATION` / 409. + +`bulkUpdate` now resolves every id to its table index first and derives the +touched set from the *resolved rows' own ids*, so both lookups read the same +stored value and cannot drift apart — the property the sibling `updateMany` +gets for free by drawing its target ids from table rows. The loose resolution +is deliberately preserved: tightening it would silently change which ids +resolve at all, a far wider behaviour change than this defect. + +A genuine collision is still refused, and the stored id keeps its own type — +naming a row with a differently-typed id does not restamp it. `bulkDelete` +needed no change: it has exactly one id comparison and dedups on the resolved +table index rather than on caller input, so a mixed-type or repeated id +collapses to a single index by construction. diff --git a/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts b/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts index d93c83bab2..6c0be5b8e5 100644 --- a/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts +++ b/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts @@ -328,3 +328,109 @@ describe('[#13435] non-regression — updateMany and bulkCreate still behave as expect(await driver.count('doc')).toBe(3); }); }); + +/** + * [#13875] The batch's two id lookups must AGREE. + * + * `IDataDriver.bulkUpdate` declares `id: string | number`, so a caller may + * legitimately name a row with an id whose JS type differs from the stored + * row's — and `update`/`bulkUpdate` resolve ids with a LOOSE `==` precisely so + * that `'1'` still finds stored `1`. The first cut of #13435 then built its + * untouched-row set (`settled`) from the CALLER's ids with a STRICT `Set.has`, + * so for a mixed-type id the two disagreed: `findIndex` resolved the row (it + * got updated) while `settled` still carried that row's PRE-image. The row sat + * in the projected check set twice — once stale, once pending — and a batch + * that merely MOVES a unique value between rows was refused with a false + * `UNIQUE_VIOLATION`. + * + * The sibling `updateMany` never had this gap: its `targetIds` come from table + * ROWS and its `findIndex` is strict `===`, so both sides agree by + * construction. The fix restores that property here the other way round — + * keeping the loose resolution (narrowing it would silently change which ids + * resolve at all) and drawing the touched set from the RESOLVED rows' own ids. + * + * ⛔ The discriminating fact is that a legitimate batch SUCCEEDS. A test that + * only asserted "a collision still refuses" would pass against the defect. + */ +describe('[#13875] caller id TYPE never changes the outcome of a batch', () => { + let driver: InMemoryDriver; + + /** Numeric stored ids — the caller may still name them as strings. */ + beforeEach(async () => { + driver = new InMemoryDriver(); + await driver.syncSchema('doc', DOC_SCHEMA); + await driver.create('doc', { id: 1, doc_no: 'D-0001', title: 'One' }); + await driver.create('doc', { id: 2, doc_no: 'D-0002', title: 'Two' }); + }); + + it('POSITIVE CONTROL: the same hand-off with CONSISTENT id types succeeds', async () => { + // Row 1 vacates D-0001; row 2 takes it. Nothing about this batch is + // unusual — it is here so the mixed-type case below cannot pass vacuously. + const out = await driver.bulkUpdate('doc', [ + { id: 1, data: { doc_no: 'D-0900' } }, + { id: 2, data: { doc_no: 'D-0001' } }, + ]); + + expect(out).toHaveLength(2); + const rows = await snapshot(driver, 'doc'); + expect(rows.map((r: any) => [r.id, r.doc_no])).toEqual([ + [1, 'D-0900'], + [2, 'D-0001'], + ]); + }); + + it('a STRING id naming a NUMERIC row still hands a unique value over cleanly', async () => { + // Identical to the control except the first id is a string. It resolves + // (loose `==`), so row 1 really does vacate D-0001 — and row 2 taking it + // must therefore NOT collide. Against the defect this threw a false + // UNIQUE_VIOLATION, because row 1's stale pre-image stayed in `settled`. + const out = await driver.bulkUpdate('doc', [ + { id: '1', data: { doc_no: 'D-0900' } }, + { id: 2, data: { doc_no: 'D-0001' } }, + ]); + + expect(out).toHaveLength(2); + const rows = await snapshot(driver, 'doc'); + expect(rows.map((r: any) => [r.id, r.doc_no])).toEqual([ + [1, 'D-0900'], + [2, 'D-0001'], + ]); + }); + + it('the stored id KEEPS its own type — a string id in the batch does not restamp it', async () => { + await driver.bulkUpdate('doc', [{ id: '1', data: { title: 'Renamed' } }]); + + const row: any = (await driver.find('doc', { where: { id: 1 } }))[0]; + expect(row.id).toBe(1); + expect(row.title).toBe('Renamed'); + }); + + it('a REAL collision is still refused when the id types are mixed', async () => { + // The fix must not turn the check off: row 2 keeps D-0002, so row 1 taking + // it is a genuine violation however the caller spelled row 1's id. + const before = await snapshot(driver, 'doc'); + + const err = await refusalOf(() => driver.bulkUpdate('doc', [{ id: '1', data: { doc_no: 'D-0002' } }])); + + expect(err.code).toBe('UNIQUE_VIOLATION'); + expect(err.status).toBe(409); + expect(await snapshot(driver, 'doc')).toEqual(before); + }); + + it('bulkDelete: a mixed-type id removes exactly its own row', async () => { + await driver.bulkDelete('doc', ['1']); + + const rows = await snapshot(driver, 'doc'); + expect(rows.map((r: any) => r.id)).toEqual([2]); + }); + + it('bulkDelete: the SAME row named twice in two id types is removed once, and only it', async () => { + // `bulkDelete` dedups on the RESOLVED table index, not on the caller's id. + // Keying the set on caller input instead would make '1' and 1 two entries + // and splice index 0 twice — taking row 2 with it. + await driver.bulkDelete('doc', ['1', 1]); + + const rows = await snapshot(driver, 'doc'); + expect(rows.map((r: any) => [r.id, r.doc_no])).toEqual([[2, 'D-0002']]); + }); +}); diff --git a/packages/drivers/driver-memory/src/memory-driver.ts b/packages/drivers/driver-memory/src/memory-driver.ts index 20f35d075e..3c63af934c 100644 --- a/packages/drivers/driver-memory/src/memory-driver.ts +++ b/packages/drivers/driver-memory/src/memory-driver.ts @@ -907,14 +907,33 @@ export class InMemoryDriver implements IDataDriver { this.logger.debug('BulkUpdate operation', { object, count: updates.length }); const table = this.getTable(object); - const touchedIds = new Set(updates.map((u) => u.id)); + + // [#13875] Resolve every id to its table row FIRST, then draw the touched + // set from the RESOLVED rows' OWN ids — never from caller input. Ids are + // resolved with a loose `==` (matching `update`, one method up), but a + // `Set` membership test is always strict, so a caller naming a stored `1` + // as `'1'` — which `IDataDriver.bulkUpdate` explicitly allows, `id` being + // `string | number` — used to satisfy the resolving lookup while failing + // the `settled` one. That row was then updated AND left in `settled` + // carrying its PRE-image, so it faced the uniqueness check twice and a + // batch merely HANDING a unique value from one row to another was refused + // with a false `UNIQUE_VIOLATION`. Both lookups now read the same stored + // value, so they cannot disagree — the property `updateMany` gets for free + // by drawing its `targetIds` from table rows. + const resolvedIndexes = updates.map((u) => table.findIndex((r) => r.id == u.id)); + const touchedIds = new Set( + resolvedIndexes.filter((index) => index !== -1).map((index) => table[index].id), + ); const settled = table.filter((r) => !touchedIds.has(r.id)); const perUpdate: Array<{ index: number; row: Record } | null> = []; const pending: Record[] = []; - for (const u of updates) { - const index = table.findIndex((r) => r.id == u.id); + // Indexed rather than `for…of`, to read each id's ALREADY-resolved index: + // resolving a second time here is what let the two lookups drift apart. + for (let position = 0; position < updates.length; position++) { + const u = updates[position]; + const index = resolvedIndexes[position]; if (index === -1) { if (this.config.strictMode) { this.logger.warn('Record not found for bulk update', { object, id: u.id }); From 3a20e593717c2d24ac54513592f88eef31635c14 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 16:54:41 +0000 Subject: [PATCH 2/3] docs(driver-memory): cite the issue this fix closes, not the PR that introduced the defect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neighbouring comments in this file cite the ISSUE (`[#13435]`, `[#13197]`, `[#13340]`); this one cited #13875, which is the PR that introduced the defect. Comment text only — no behaviour change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- packages/drivers/driver-memory/src/memory-driver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/drivers/driver-memory/src/memory-driver.ts b/packages/drivers/driver-memory/src/memory-driver.ts index 3c63af934c..c5397c9ac9 100644 --- a/packages/drivers/driver-memory/src/memory-driver.ts +++ b/packages/drivers/driver-memory/src/memory-driver.ts @@ -908,7 +908,7 @@ export class InMemoryDriver implements IDataDriver { const table = this.getTable(object); - // [#13875] Resolve every id to its table row FIRST, then draw the touched + // [#13911] Resolve every id to its table row FIRST, then draw the touched // set from the RESOLVED rows' OWN ids — never from caller input. Ids are // resolved with a loose `==` (matching `update`, one method up), but a // `Set` membership test is always strict, so a caller naming a stored `1` From dcbcb1e22ae69c400adc6d9c617eb0348c67bb7e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 16:58:25 +0000 Subject: [PATCH 3/3] test(driver-memory): cite the issue the new block pins, matching the rest of the file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment and describe-title text only — no assertion changes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --- .../src/memory-bulk-update-delete-atomicity.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts b/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts index 6c0be5b8e5..7ac2521a76 100644 --- a/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts +++ b/packages/drivers/driver-memory/src/memory-bulk-update-delete-atomicity.test.ts @@ -330,7 +330,7 @@ describe('[#13435] non-regression — updateMany and bulkCreate still behave as }); /** - * [#13875] The batch's two id lookups must AGREE. + * [#13911] The batch's two id lookups must AGREE. * * `IDataDriver.bulkUpdate` declares `id: string | number`, so a caller may * legitimately name a row with an id whose JS type differs from the stored @@ -352,7 +352,7 @@ describe('[#13435] non-regression — updateMany and bulkCreate still behave as * ⛔ The discriminating fact is that a legitimate batch SUCCEEDS. A test that * only asserted "a collision still refuses" would pass against the defect. */ -describe('[#13875] caller id TYPE never changes the outcome of a batch', () => { +describe('[#13911] caller id TYPE never changes the outcome of a batch', () => { let driver: InMemoryDriver; /** Numeric stored ids — the caller may still name them as strings. */