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..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 @@ -328,3 +328,109 @@ describe('[#13435] non-regression — updateMany and bulkCreate still behave as expect(await driver.count('doc')).toBe(3); }); }); + +/** + * [#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 + * 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('[#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. */ + 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..c5397c9ac9 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)); + + // [#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` + // 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 });