diff --git a/.changeset/driver-sql-upsert-autonumber-immutable.md b/.changeset/driver-sql-upsert-autonumber-immutable.md new file mode 100644 index 0000000000..e6adf8e48b --- /dev/null +++ b/.changeset/driver-sql-upsert-autonumber-immutable.md @@ -0,0 +1,41 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): a merge-path `upsert` no longer rewrites an existing row's autonumber (#7011) + +Measured on a completely healthy counter, single row throughout: + +``` +create → CASE-00001 last_value 1 +upsert same id (1st time) → CASE-00002 last_value 2 +upsert same id (2nd time) → CASE-00003 last_value 3 +``` + +`fillAutoNumberFields` reserves a number before the statement knows whether it +will insert or merge, and the autonumber column sat in `mergeColumns` — so +every `ON CONFLICT … DO UPDATE` wrote the freshly reserved number over the +row's existing one, silently replacing an externally visible business +identifier the caller never asked to change. + +Per the triage ruling on the card: an autonumber is an **immutable business +identifier once assigned**. `auto_number` columns are now excluded from the +merge column list, exactly like `created_at` (both are insert-only facts about +the row's birth). After the fix the same sequence keeps `CASE-00001` through +both upserts. The exclusion is unconditional — an explicit autonumber value in +the upsert payload does not renumber an existing row on the merge branch +either; `update()` writes what it is given and remains the deliberate +renumbering path. Insert-path upserts still assign fresh numbers, and every +non-autonumber column (including `updated_at`) merges as before. + +Deliberately out of scope (#6943's reseed family): the reservation itself still +happens before insert-vs-merge is known, so a merge-only upsert still consumes +one sequence value per call — now a permanent gap in the sequence rather than a +rewrite of the row (measured post-fix: row keeps `CASE-00001`, `last_value` +walks 1 → 2 → 3, the next inserted row gets `CASE-00004`). + +Covered faces: `SqliteWasmDriver` inherits `upsert` unchanged; `TursoDriver` +local/replica routes its override to `super` — both pinned by their own tests. +Turso remote (`RemoteTransport.upsert`) never enters `fillAutoNumberFields` and +has neither the defect nor the fix. Rows already renumbered by past merges +cannot be restored from the driver side. diff --git a/packages/drivers/driver-sql/src/sql-driver-upsert-autonumber-immutable.test.ts b/packages/drivers/driver-sql/src/sql-driver-upsert-autonumber-immutable.test.ts new file mode 100644 index 0000000000..66eec03a25 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-upsert-autonumber-immutable.test.ts @@ -0,0 +1,167 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7011] A merge-path upsert must never rewrite an existing row's autonumber. + * + * # The defect, measured on a HEALTHY counter (no staleness involved) + * + * ``` + * create → CASE-00001 last_value 1 + * upsert same id (1st time) → CASE-00002 last_value 2 + * upsert same id (2nd time) → CASE-00003 last_value 3 + * ``` + * + * One row throughout — and its `case_number` was rewritten twice. The cause: + * `fillAutoNumberFields` reserves a number before the statement knows whether + * it will insert or merge, and the autonumber column sat in `mergeColumns`, so + * the `ON CONFLICT … DO UPDATE` branch wrote the freshly reserved number over + * the row's existing one. + * + * # The ruling this file pins (triage, 2026-08-09, on the card) + * + * An autonumber is an **immutable business identifier once assigned** — it is + * usually an externally visible document number (`CASE-00001`), and a caller + * asking to update a row did not ask for a new one. The fix excludes + * `auto_number` columns from `mergeColumns`, exactly like `created_at` (both + * are insert-only facts about the row's birth). The exclusion is unconditional: + * even an EXPLICIT autonumber value in the upsert payload does not rewrite an + * existing row's number on the merge branch — `update()` writes whatever it is + * given and remains the deliberate renumbering path. + * + * # Out of scope here, deliberately + * + * The reservation itself still happens before insert-vs-merge is known, so a + * merge-only upsert still consumes a sequence value (leaves a gap). That + * pre-burn half belongs to #6943's reseed family and is NOT pinned by this + * file — no test here asserts `last_value` on the merge path, so deferring the + * reservation later cannot turn this file red. + * + * # Reverse verification (direction predicted before running) + * + * Restoring the deleted limb — removing the autonumber exclusion from + * `mergeColumns` — turns exactly the merge-path pins below red, with the + * filing's own values (received `CASE-00002` / `CASE-00003` where `CASE-00001` + * was asserted). The insert-path and non-autonumber-merge cases stay green + * either way; they are here to pin that the exclusion does not over-reach. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from './index.js'; + +const CRM_CASE = { + name: 'crm_case', + fields: { + organization_id: { type: 'string' }, + case_number: { type: 'autonumber', format: 'CASE-{00000}', unique: true }, + title: { type: 'string' }, + status: { type: 'string' }, + }, +} as any; + +describe('[#7011] merge-path upsert keeps the assigned autonumber', () => { + let driver: SqlDriver; + + const knex = () => (driver as any).knex; + + const rowCount = async () => Number((await knex()('crm_case').count({ c: '*' }).first()).c); + + beforeEach(async () => { + driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + await driver.initObjects([CRM_CASE]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('keeps the existing number across repeated merges to the same row (the filing repro)', async () => { + const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original' }); + expect(created.case_number).toBe('CASE-00001'); + + const first = await driver.upsert('crm_case', { id: created.id, organization_id: 'orgA', title: 'edit 1' }); + expect(first.case_number).toBe('CASE-00001'); // was CASE-00002 before the fix + expect(first.title).toBe('edit 1'); + + const second = await driver.upsert('crm_case', { id: created.id, organization_id: 'orgA', title: 'edit 2' }); + expect(second.case_number).toBe('CASE-00001'); // was CASE-00003 before the fix + expect(second.title).toBe('edit 2'); + + // One row throughout, and the stored value agrees with the returned one. + expect(await rowCount()).toBe(1); + const stored = await knex()('crm_case').where({ id: created.id }).first(); + expect(stored.case_number).toBe('CASE-00001'); + }); + + it('does not rewrite the number even when the payload carries an explicit one', async () => { + const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original' }); + expect(created.case_number).toBe('CASE-00001'); + + // An explicit value is skipped by fillAutoNumberFields (caller-supplied), + // but it still sits in the INSERT's column list — the exclusion is what + // keeps it off the merge branch. `update()` remains the deliberate + // renumbering path for the caller who really means it. + const merged = await driver.upsert('crm_case', { + id: created.id, + organization_id: 'orgA', + case_number: 'CASE-99999', + title: 'tried to renumber', + }); + expect(merged.case_number).toBe('CASE-00001'); + expect(merged.title).toBe('tried to renumber'); + }); + + it('insert-path upsert still assigns a fresh number', async () => { + const first = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'new row' }); + expect(first.case_number).toBe('CASE-00001'); + + const second = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'another new row' }); + expect(second.case_number).toBe('CASE-00002'); + + expect(await rowCount()).toBe(2); + }); + + it('still merges every non-autonumber column normally', async () => { + const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original', status: 'open' }); + + const merged = await driver.upsert('crm_case', { + id: created.id, + organization_id: 'orgA', + title: 'renamed', + status: 'closed', + }); + + expect(merged.title).toBe('renamed'); + expect(merged.status).toBe('closed'); + expect(merged.case_number).toBe('CASE-00001'); + expect(await rowCount()).toBe(1); + }); + + it('a merge on a non-id conflict key keeps the number too', async () => { + // The exclusion is per-column, not per-conflict-target: merging on a + // business key instead of `id` must protect the number the same way. + await driver.initObjects([ + { + name: 'crm_ticket', + fields: { + organization_id: { type: 'string' }, + ticket_number: { type: 'autonumber', format: 'TKT-{0000}', unique: true }, + external_ref: { type: 'string', unique: 'global' }, + title: { type: 'string' }, + }, + } as any, + ]); + + const created = await driver.create('crm_ticket', { organization_id: 'orgA', external_ref: 'ext-1', title: 'first' }); + expect(created.ticket_number).toBe('TKT-0001'); + + const merged = await driver.upsert( + 'crm_ticket', + { id: created.id, organization_id: 'orgA', external_ref: 'ext-1', title: 'merged by ref' }, + ['external_ref'], + ); + expect(merged.title).toBe('merged by ref'); + + const stored = await knex()('crm_ticket').where({ external_ref: 'ext-1' }).first(); + expect(stored.ticket_number).toBe('TKT-0001'); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 0f6551c614..284645a10b 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -3983,6 +3983,27 @@ export class SqlDriver implements IDataDriver { return this.formatOutput(object, updated) || null; } + /** + * Columns `upsert`'s merge branch must never write ([#7011]): `created_at` + * (the row's birth timestamp belongs to the original insert) and every + * `auto_number` column — an autonumber is an immutable business identifier + * once assigned, so an upsert that lands on an existing row keeps that row's + * number (see the fuller rationale at the merge site). Autonumber columns + * are returned under their PHYSICAL names (an external object can remap + * logical fields via `external.columnMap`), matching the + * `applyWriteColumnMap`-processed row the merge column list is derived from; + * `created_at` stays the literal post-map key it has always been filtered as. + */ + protected insertOnlyUpsertColumns(object: string): Set { + // Same config resolution as `fillAutoNumberFields`: object name first, + // then the storage-mapped table name. + const tableName = this.physicalTableByObject[object] ?? StorageNameMapping.resolveTableName({ name: object } as any); + const cfgs = this.autoNumberFields[object] || this.autoNumberFields[tableName] || []; + const columns = new Set(['created_at']); + for (const cfg of cfgs) columns.add(this.remoteColumn(object, cfg.name, cfg.name)); + return columns; + } + async upsert(object: string, data: Record, conflictKeys?: string[], options?: DriverOptions): Promise> { const { _id, ...rest } = data; const toUpsert = { ...rest }; @@ -4022,9 +4043,22 @@ export class SqlDriver implements IDataDriver { const builder = this.getBuilder(this.rotationWriteTarget(object) ?? object, options); // `created_at` is insert-only — never overwrite it when an existing row is // merged on conflict (the stamped/seeded value belongs to the original - // insert). Everything else (incl. `updated_at`) merges as before, so an - // upsert that updates a row still advances `updated_at`. - const mergeColumns = Object.keys(formatted).filter((c) => c !== 'created_at'); + // insert). [#7011] `auto_number` columns are insert-only for the same + // reason, and a stronger one: the number is an externally visible + // business identifier once assigned (`CASE-00001`), and + // `fillAutoNumberFields` above reserved a FRESH value before the + // statement could know it would merge — leaving these columns in the + // merge set rewrote the existing row's number on every merge-path upsert + // (measured on a healthy counter: create → CASE-00001, two upserts of + // the same id → CASE-00002 then CASE-00003, one row throughout). The + // exclusion is unconditional — an explicit payload value does not + // renumber on merge either; `update()` is the deliberate renumbering + // path. The reservation itself still happens on the merge path (a gap, + // not a rewrite) — that pre-burn half is #6943's reseed family, not + // this exclusion's. Everything else (incl. `updated_at`) merges as + // before, so an upsert that updates a row still advances `updated_at`. + const insertOnlyColumns = this.insertOnlyUpsertColumns(object); + const mergeColumns = Object.keys(formatted).filter((c) => !insertOnlyColumns.has(c)); const insertion = builder.insert(formatted).onConflict(mergeKeys); try { diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts index 4788b0a32e..377eab64e3 100644 --- a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-autonumber-batch-resync.test.ts @@ -76,4 +76,16 @@ describe('[#6943] driver-sqlite-wasm inherits the batch/upsert autonumber re-see const upserted = await driver.upsert('crm_case', { organization_id: 'orgA', title: 'u1' }); expect(upserted.case_number).toBe('CASE-00031'); }); + + it('[#7011] a merge-path upsert keeps the existing autonumber on this transport too', async () => { + // Inherited from `SqlDriver.upsert` (no override) — pinned here because the + // exclusion is applied to the ON CONFLICT merge column list, and this + // driver swaps the transport under that statement. + const created = await driver.create('crm_case', { organization_id: 'orgA', title: 'original' }); + expect(created.case_number).toBe('CASE-00001'); + + const merged = await driver.upsert('crm_case', { id: created.id, organization_id: 'orgA', title: 'edited' }); + expect(merged.case_number).toBe('CASE-00001'); + expect(merged.title).toBe('edited'); + }); }); diff --git a/packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts b/packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts index 38d5dab945..9c2177006a 100644 --- a/packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts +++ b/packages/drivers/driver-turso/src/turso-autonumber-batch-resync.test.ts @@ -89,6 +89,27 @@ describe('[#6943] TursoDriver batch/upsert autonumber re-seed', () => { expect(upserted.case_number).toBe('CASE-00031'); }); + it('LOCAL: [#7011] a merge-path upsert keeps the existing autonumber', async () => { + // Turso OVERRIDES `upsert` (remote → RemoteTransport, else `super`), so + // "the base class was fixed" is not on its own an answer about this face — + // the local route through `super.upsert` is pinned here. + const created = await driver.create( + 'crm_case', + { organization_id: 'orgA', title: 'original' }, + { bypassTenantAudit: true }, + ); + expect(created.case_number).toBe('CASE-00001'); + + const merged = await driver.upsert( + 'crm_case', + { id: created.id, organization_id: 'orgA', title: 'edited' }, + undefined, + { bypassTenantAudit: true } as any, + ); + expect(merged.case_number).toBe('CASE-00001'); + expect(merged.title).toBe('edited'); + }); + it('REMOTE: the transport that bypasses this path has no autonumber machinery to re-seed', async () => { const remote = new TursoDriver({ url: 'libsql://example.turso.io', authToken: 'placeholder' }); expect(remote.transportMode).toBe('remote');