diff --git a/.changeset/8740-upsert-nothing-to-merge.md b/.changeset/8740-upsert-nothing-to-merge.md new file mode 100644 index 00000000000..c857529cfc6 --- /dev/null +++ b/.changeset/8740-upsert-nothing-to-merge.md @@ -0,0 +1,20 @@ +--- +'@objectstack/driver-sql': patch +--- + +fix(driver-sql): an `upsert` with nothing left to merge is a plain INSERT, so a collision surfaces as the driver's unique violation instead of merging every column + +Clause-②: no (narrowing) +Metadata-protocol: none + + + +**BREAKING** (a served-set narrowing) for an `upsert` whose payload carries only insert-only columns (`id`, `created_at`, `auto_number` columns) and none of its conflict-target columns, when that row collides with a stored row. Such a call used to merge EVERY payload column into the stored row, insert-only columns included. It now raises the driver's unique-violation error and leaves the stored row as it was. + +`SqlDriver.upsert` merges every payload column that is not insert-only, or, when there is none, the conflict-target columns the payload carries (a no-op). When the payload carries neither, the statement used to fall back to knex's bare `merge()`, which compiles to merge-ALL on every dialect (`do update set` / `on duplicate key update` over every inserted column). That fallback is removed: with nothing to merge, the statement is a plain `INSERT` with no conflict clause. It is not `insert ignore` / `do nothing` either, which would hide a collision. + +Unchanged: a payload with a column to merge, or one that carries its conflict target, upserts exactly as before, and a payload that collides with nothing is inserted and answered as before. + +A conflict target that no PRIMARY KEY or UNIQUE index backs is still refused on that path, with the same `VALIDATION_ERROR` / 400 every dialect answers when there is something to merge. A plain INSERT asks the database nothing about the target, so the driver checks it against the table's keys before the statement, on every dialect. + +**What changes for you.** If you upsert a payload made only of insert-only columns under a business conflict key the payload omits, catch the unique-violation error on a collision, or use `update()` to change a stored row. diff --git a/packages/drivers/driver-sql/src/sql-driver-upsert-empty-merge-set.test.ts b/packages/drivers/driver-sql/src/sql-driver-upsert-empty-merge-set.test.ts new file mode 100644 index 00000000000..e351f9324e4 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-upsert-empty-merge-set.test.ts @@ -0,0 +1,261 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * An upsert with nothing left to merge is a plain INSERT. + * + * `upsert`'s merge leg writes every payload column that is not insert-only + * (`insertOnlyUpsertColumns`), or, when that set is empty, the conflict-target + * columns the payload carries (a no-op: a conflict means they matched). When + * both are empty the statement used to fall back to knex's bare `merge()`, + * which compiles to merge-ALL on every dialect: `do update set` / + * `on duplicate key update` over EVERY inserted column, insert-only ones + * included. That fallback is gone. With nothing to merge the statement carries + * no conflict clause, so a collision surfaces as the driver's own + * unique-violation error and the stored row is left as it was. + * + * Pinned here: + * - the compiled SQL per dialect, with no server (`toSQL()`); + * - on every provisioned cell: a collision on a conflict-target column that + * the payload omits (the column has a non-null default) is refused and + * rewrites nothing; a collision on the primary key with only insert-only + * columns in the payload is refused and rewrites nothing; + * - a conflict target that no PRIMARY KEY or UNIQUE index backs is still + * refused (`VALIDATION_ERROR` / 400) on that path, and nothing is written — + * the same answer every dialect gives when there IS something to merge; + * - controls: a payload that collides with nothing still inserts and answers + * the row, and a payload that carries its conflict target still upserts + * onto the stored row. + * + * The tables are created by hand and registered DDL-free, so the driver has not + * observed an `updated_at` column on them and the payloads below reach the + * empty merge set. Live cells run where their URL is provisioned; elsewhere they + * are declared un-run, never silently passed. + */ + +import { describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest'; +import knex from 'knex'; +import { isUniqueViolationError } from '@objectstack/types'; +import { SqlDriver } from '../src/index.js'; +import { withUpsertConflictClause } from './upsert-conflict-clause.js'; +import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +// ───────────────────────────────────────────────────────────────────────── +// The compiled statement, per dialect — no server +// ───────────────────────────────────────────────────────────────────────── + +describe('SqlDriver.upsert — the statement compiled when nothing is left to merge', () => { + const compile = (client: string, columnsToMerge: string[]): string => { + const k = knex({ client, connection: {} } as any); + try { + return withUpsertConflictClause(k('os8740_t').insert({ id: '1', created_at: 'x' }), ['email'], columnsToMerge) + .statement.toSQL().sql; + } finally { + void k.destroy(); + } + }; + + const EXPECTED_PLAIN: Record = { + 'better-sqlite3': 'insert into `os8740_t` (`created_at`, `id`) values (?, ?)', + pg: 'insert into "os8740_t" ("created_at", "id") values (?, ?)', + mysql2: 'insert into `os8740_t` (`created_at`, `id`) values (?, ?)', + }; + + for (const [client, expected] of Object.entries(EXPECTED_PLAIN)) { + it(`compiles a plain INSERT with no conflict clause (${client})`, () => { + const sql = compile(client, []); + expect(sql).not.toMatch(/do update set/i); + expect(sql).not.toMatch(/on duplicate key update/i); + expect(sql).not.toMatch(/on conflict/i); + expect(sql).not.toMatch(/insert ignore/i); + expect(sql).toBe(expected); + }); + } + + // The contrast: with a column to merge, the same helper still emits the + // conflict clause and merges exactly that column, never every column. + it('keeps the conflict clause, merging only the named columns, when there is something to merge', () => { + expect(compile('better-sqlite3', ['title'])).toBe( + 'insert into `os8740_t` (`created_at`, `id`) values (?, ?) on conflict (`email`) do update set `title` = excluded.`title`', + ); + expect(compile('pg', ['title'])).toBe( + 'insert into "os8740_t" ("created_at", "id") values (?, ?) on conflict ("email") do update set "title" = excluded."title"', + ); + expect(compile('mysql2', ['title'])).toBe( + 'insert into `os8740_t` (`created_at`, `id`) values (?, ?) on duplicate key update `title` = values(`title`)', + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────── +// Behaviour, per provisioned dialect +// ───────────────────────────────────────────────────────────────────────── + +/** A conflict-target column with a non-null default, so an omitted value still collides. */ +const DEFAULTED = { + name: 'os8740_defaulted', + fields: { email: { type: 'string' }, title: { type: 'string' } }, +} as any; + +/** A business key the payloads below omit, beside insert-only `created_at`. */ +const STAMPED = { + name: 'os8740_stamped', + fields: { email: { type: 'string' }, title: { type: 'string' } }, +} as any; + +/** No index on `email` at all: a conflict target nothing backs. */ +const UNBACKED = { + name: 'os8740_unbacked', + fields: { email: { type: 'string' }, title: { type: 'string' } }, +} as any; + +const KEPT_ID = 'os8740_kept_row'; +const KEPT_CREATED = '2020-01-01 00:00:00'; + +/** Each dialect's native unique-violation code, as its client library reports it. */ +const UNIQUE_VIOLATION_CODES: Record = { + sqlite: ['SQLITE_CONSTRAINT_PRIMARYKEY', 'SQLITE_CONSTRAINT_UNIQUE'], + pg: ['23505'], + mysql: ['ER_DUP_ENTRY'], +}; + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, 'upsert with nothing left to merge', declareEmptyMergeSetPins); +} + +function declareEmptyMergeSetPins(cell: DialectCell): void { + describe(`SqlDriver.upsert — nothing left to merge (${cell.label})`, () => { + let driver: SqlDriver; + let knexInstance: any; + + const rows = async (table: string): Promise>> => { + const found = await knexInstance(table).select('id', 'email', 'title').orderBy('id'); + return found.map((r: Record) => ({ ...r })); + }; + + const createdAtOf = async (id: string): Promise => { + const row = await knexInstance(STAMPED.name).where('id', id).first(); + const value = row?.created_at; + return value instanceof Date ? value.toISOString() : String(value); + }; + + /** + * The dialect's own unique-violation code, beside the shared predicate: the + * predicate matches its vocabulary anywhere in the message, and a statement + * echoed into an unrelated error's message can carry that vocabulary. + */ + const expectUniqueViolation = (err: unknown): void => { + expect(err, 'the collision was merged instead of refused').not.toBeNull(); + const { code, message } = err as { code?: string; message?: string }; + expect(isUniqueViolationError(err), `not a unique violation: ${String(message)}`).toBe(true); + expect(UNIQUE_VIOLATION_CODES[cell.id], `not a unique violation: ${String(message)}`).toContain(code); + }; + + const captureError = async (run: () => Promise): Promise => { + try { + await run(); + return null; + } catch (e) { + return e; + } + }; + + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + knexInstance = (driver as any).knex; + await knexInstance.schema.dropTableIfExists(DEFAULTED.name); + await knexInstance.schema.dropTableIfExists(STAMPED.name); + await knexInstance.schema.dropTableIfExists(UNBACKED.name); + await knexInstance.schema.createTable(DEFAULTED.name, (t: any) => { + t.string('id', 64).primary(); + t.string('email', 191).notNullable().defaultTo('unset').unique(); + t.string('title', 191).nullable(); + }); + await knexInstance.schema.createTable(STAMPED.name, (t: any) => { + t.string('id', 64).primary(); + t.timestamp('created_at').nullable(); + t.string('email', 191).nullable().unique(); + t.string('title', 191).nullable(); + }); + driver.registerExternalObject({ ...DEFAULTED, external: { remoteName: DEFAULTED.name } }); + await knexInstance.schema.createTable(UNBACKED.name, (t: any) => { + t.string('id', 64).primary(); + t.string('email', 191).nullable(); + t.string('title', 191).nullable(); + }); + driver.registerExternalObject({ ...STAMPED, external: { remoteName: STAMPED.name } }); + driver.registerExternalObject({ ...UNBACKED, external: { remoteName: UNBACKED.name } }); + }); + + afterAll(async () => { + await knexInstance?.schema.dropTableIfExists(DEFAULTED.name).catch(() => {}); + await knexInstance?.schema.dropTableIfExists(STAMPED.name).catch(() => {}); + await knexInstance?.schema.dropTableIfExists(UNBACKED.name).catch(() => {}); + await driver?.disconnect?.(); + }); + + beforeEach(async () => { + await knexInstance(DEFAULTED.name).delete(); + await knexInstance(STAMPED.name).delete(); + await knexInstance(UNBACKED.name).delete(); + await knexInstance(DEFAULTED.name).insert({ id: KEPT_ID, email: 'unset', title: 'kept' }); + await knexInstance(STAMPED.name).insert({ + id: KEPT_ID, + created_at: new Date(`${KEPT_CREATED.replace(' ', 'T')}Z`), + email: 'kept@os8740.test', + title: 'kept', + }); + }); + + it('refuses a collision on an omitted conflict-target column with a unique violation, and rewrites no column', async () => { + const before = await rows(DEFAULTED.name); + + const err = await captureError(() => driver.upsert(DEFAULTED.name, { id: 'os8740_second_row' }, ['email'])); + + expectUniqueViolation(err); + expect(await rows(DEFAULTED.name), 'the stored row was rewritten, or a row was added').toEqual(before); + }); + + it('refuses a primary-key collision whose payload carries only insert-only columns, and rewrites no column', async () => { + const before = await rows(STAMPED.name); + const createdBefore = await createdAtOf(KEPT_ID); + + const err = await captureError(() => + driver.upsert(STAMPED.name, { id: KEPT_ID, created_at: '2030-06-01 00:00:00' }, ['email']), + ); + + expectUniqueViolation(err); + expect(await rows(STAMPED.name)).toEqual(before); + expect(await createdAtOf(KEPT_ID), 'an insert-only column was rewritten').toBe(createdBefore); + }); + + it('refuses a conflict target that no unique index backs, and writes nothing', async () => { + const err = (await captureError(() => driver.upsert(UNBACKED.name, { id: 'os8740_unbacked_row' }, ['email']))) as { + code?: string; + status?: number; + } | null; + + expect(err, 'the unbacked conflict target was accepted').not.toBeNull(); + expect(err!.code).toBe('VALIDATION_ERROR'); + expect(err!.status).toBe(400); + expect(await rows(UNBACKED.name), 'a row was written').toEqual([]); + }); + + it('still inserts, and answers the row, when the payload collides with nothing', async () => { + const written = await driver.upsert(STAMPED.name, { id: 'os8740_fresh_row' }, ['email']); + + expect(written.id).toBe('os8740_fresh_row'); + expect((await rows(STAMPED.name)).map((r) => r.id).sort()).toEqual(['os8740_fresh_row', KEPT_ID].sort()); + }); + + it('still upserts onto the stored row when the payload carries its conflict target', async () => { + const before = await rows(STAMPED.name); + const createdBefore = await createdAtOf(KEPT_ID); + + const written = await driver.upsert(STAMPED.name, { id: KEPT_ID }, ['id']); + + expect(written.id).toBe(KEPT_ID); + expect(await rows(STAMPED.name)).toEqual(before); + expect(await createdAtOf(KEPT_ID)).toBe(createdBefore); + }); + }); +} diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 6789b4f9d46..3c5654b0c83 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -157,6 +157,7 @@ import { type UndeliveredAttribute, } from './builtin-column-collision.js'; import { recoverUnencodedJsonText } from './unencoded-json-text.js'; +import { withUpsertConflictClause } from './upsert-conflict-clause.js'; // [#25521 cut 1] The catalog-introspection helpers (`introspectColumnOrder`, // `introspectColumns` and the foreign-key, primary-key and unique-constraint reads), // [cut 4] `introspectSchema`, the whole-schema read built on them, and [cut 5] @@ -2256,6 +2257,30 @@ function refuseUndeclaredFieldType(column: string): never { * matching the twin: the two-argument form needs the ES2022 `ErrorOptions` * overload. */ +/** + * Whether a physical key index covers exactly the conflict target — the same + * columns, order-insensitively, which is how both `ON CONFLICT` dialects infer + * an arbiter index and the only reading under which a composite key means + * anything. Shared by {@link SqlDriver.assertConflictTargetHonoured} and + * {@link SqlDriver.assertConflictTargetBackedWithoutClause}. + */ +function indexCoversConflictTarget(index: PhysicalIndex, mergeKeys: readonly string[]): boolean { + const wanted = new Set(mergeKeys); + return index.columns.length === wanted.size && index.columns.every((c) => wanted.has(c)); +} + +/** + * The `cause` an introspection-based unbacked-target refusal carries, where the + * server's own sentence stands on a server-judged one: schema identifiers only, + * no row values. + */ +function unbackedConflictTargetCause(tableName: string, mergeKeys: readonly string[], keys: PhysicalIndex[]): Error { + const found = keys.map((i) => `${i.name}(${i.columns.join(', ')})`).join(', '); + return new Error( + `no PRIMARY KEY or UNIQUE index on "${tableName}" covers (${mergeKeys.join(', ')}); ` + `keys present: ${found}`, + ); +} + function refuseUnbackedConflictTarget(object: string, mergeKeys: string[], cause: unknown): Error { const keys = mergeKeys.map((k) => `"${k}"`).join(', '); const err = new Error( @@ -9495,9 +9520,7 @@ export class SqlDriver implements IDataDriver { const target = this.rotationWriteTarget(object) ?? object; const tableName = this.physicalTableByObject[target] ?? target; - const wanted = new Set(mergeKeys); - const covers = (i: PhysicalIndex): boolean => - i.columns.length === wanted.size && i.columns.every((c) => wanted.has(c)); + const covers = (i: PhysicalIndex): boolean => indexCoversConflictTarget(i, mergeKeys); type Verdict = | { kind: 'honoured' } @@ -9588,15 +9611,36 @@ export class SqlDriver implements IDataDriver { // `cause` carries the ground truth an operator needs to act. Schema // identifiers only — no row values, matching the payload contract the // refusal already keeps. - const found = keys.map((i) => `${i.name}(${i.columns.join(', ')})`).join(', '); - throw refuseUnbackedConflictTarget( - object, - mergeKeys, - new Error( - `no PRIMARY KEY or UNIQUE index on "${tableName}" covers (${mergeKeys.join(', ')}); ` + - `keys present: ${found}`, - ), - ); + throw refuseUnbackedConflictTarget(object, mergeKeys, unbackedConflictTargetCause(tableName, mergeKeys, keys)); + } + + /** + * [#8740] The unbacked-conflict-target refusal for an upsert statement that + * carries NO conflict clause — the one {@link upsert} sends when nothing is + * left to merge ({@link withUpsertConflictClause}). + * + * With a conflict clause, SQLite and PostgreSQL refuse an unbacked target + * themselves (the planner finds no arbiter index) and MySQL refuses it in + * {@link assertConflictTargetHonoured}. A plain INSERT asks the server + * nothing about the target, so without this check that path would accept on + * two dialects a call every dialect refuses when there is something to merge. + * So the answer is the same refusal, on every dialect, read from the same + * cached introspection MySQL's pre-flight reads, with the same certainty + * rule: a failed read or a table with no keys at all proceeds, and a refusal + * is re-read fresh first, so a stale cache can never refuse a backed target. + */ + private async assertConflictTargetBackedWithoutClause( + object: string, + writeTable: string, + mergeKeys: string[], + ): Promise { + const tableName = this.physicalTableByObject[writeTable] ?? writeTable; + const backed = (keys: PhysicalIndex[] | null): boolean => + keys === null || keys.length === 0 || keys.some((i) => indexCoversConflictTarget(i, mergeKeys)); + if (backed(await this.introspectKeyIndexes(tableName))) return; + const fresh = await this.introspectKeyIndexes(tableName, { fresh: true }); + if (backed(fresh)) return; + throw refuseUnbackedConflictTarget(object, mergeKeys, unbackedConflictTargetCause(tableName, mergeKeys, fresh!)); } /** @@ -9974,8 +10018,8 @@ export class SqlDriver implements IDataDriver { // never reached it: `stampInsertTimestamps` puts a mergeable `updated_at` // in the payload there. [#11176] `stampUpsertUpdatedAt` now does the same // on Postgres and MySQL, so a TIMESTAMPED object no longer reaches this - // fallback on any dialect either — it is still reachable, and still #8740's - // to decide, for an object with no observed `updated_at` column.) + // fallback on any dialect either — it is still reachable for an object + // with no observed `updated_at` column.) // // The remedy keeps the emitted SQL byte-identical to what `main` emits // for this shape. Falling back to the CONFLICT-TARGET columns present in @@ -9985,16 +10029,20 @@ export class SqlDriver implements IDataDriver { // mergeable column in this case. So this PR's net behavioural delta on the // degenerate path is nil rather than a #7011 regression. // - // The last-ditch `merge()` is left as-is: it needs the conflict target to - // be absent from the payload, which means the INSERT never supplies it, so - // on every dialect here the target is NULL and cannot collide — the DO - // UPDATE clause is unreachable rather than merely unused. Filed separately - // rather than widened into: merge-ALL is the wrong shape for "nothing to - // merge" on any dialect, and the alternatives are not accept-set-neutral - // (knex compiles `.ignore()` to MySQL's `insert ignore`, which swallows - // unrelated errors too — measured). + // [#8740] When the payload carries no conflict-target column either, there + // is nothing the merge leg may write, and the statement is a plain INSERT + // with no conflict clause: a collision surfaces as the driver's own + // unique-violation error and the stored row is left as it was. This used + // to fall back to knex's bare `merge()`, which is merge-ALL on every + // dialect: it re-admitted every insert-only column the lines above + // exclude. ⛔ Not `ignore()` either: knex lowers it on MySQL to + // `insert ignore`, which swallows unrelated errors and drops the conflict + // target. See {@link withUpsertConflictClause}. const noopMergeColumns = Object.keys(formatted).filter((c) => mergeKeys.includes(c)); const columnsToMerge = mergeColumns.length > 0 ? mergeColumns : noopMergeColumns; + // …and that plain INSERT still refuses a conflict target nothing backs, + // as every dialect does when there is something to merge. + if (columnsToMerge.length === 0) await this.assertConflictTargetBackedWithoutClause(object, writeTable, mergeKeys); // [#21185] The fence for THIS attempt: the tenant column, and the value // the row is written under in storage form (what the statement sends). @@ -10017,8 +10065,8 @@ export class SqlDriver implements IDataDriver { // is `injectTenantOnInsert`'s job (called above), which is exactly why // this one is deliberately not scoped. const builder = this.getBuilder(writeTable, writeOptions); - const insertion = builder.insert(formatted).onConflict(mergeKeys); - const merging = columnsToMerge.length > 0 ? insertion.merge(columnsToMerge) : insertion.merge(); + const insertion = builder.insert(formatted); + const { statement, merges } = withUpsertConflictClause(insertion, mergeKeys, columnsToMerge); // [#21185] SQLite and PostgreSQL: the fence is a predicate INSIDE the // merge statement — `… DO UPDATE SET … WHERE` the stored tenant column // equals the written one — so a conflict on a row of another @@ -10030,8 +10078,11 @@ export class SqlDriver implements IDataDriver { // a platform row with no organization is fenced off too. The target // table is named by its bare physical name, which both dialects accept // for the INSERT target in this clause, schema-qualified or not. - // MySQL takes no `WHERE` here; its fence runs after the statement. - await (fence && !this.isMysql ? merging.where(this.mergeLandsInWrittenOrganization(writeTable, fence.column)) : merging); + // MySQL takes no `WHERE` here; its fence runs after the statement. A + // plain INSERT ([#8740]) has no merge leg to fence. + await (merges && fence && !this.isMysql + ? statement.where(this.mergeLandsInWrittenOrganization(writeTable, fence.column)) + : statement); if (mysqlFence) { // [#21185] One read, under the written tenant exactly; #8807's // identity verdict rides on it when a rival key made one owed. diff --git a/packages/drivers/driver-sql/src/upsert-conflict-clause.ts b/packages/drivers/driver-sql/src/upsert-conflict-clause.ts new file mode 100644 index 00000000000..7f104971ea0 --- /dev/null +++ b/packages/drivers/driver-sql/src/upsert-conflict-clause.ts @@ -0,0 +1,31 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { Knex } from 'knex'; + +/** + * The conflict clause of `SqlDriver.upsert`'s statement, or none. + * + * `columnsToMerge` is what the merge leg may write: every payload column that + * is not insert-only, or — when that is empty — the conflict-target columns the + * payload carries, a provable no-op (a conflict means those columns matched). + * When BOTH are empty there is nothing the merge leg may write, so the + * statement is a plain INSERT with no conflict clause, and a collision surfaces + * as the driver's own unique-violation error. + * + * ⛔ Never a bare `merge()` here: knex compiles it to merge-ALL on every dialect + * (`do update set` / `on duplicate key update` over EVERY inserted column), the + * inverse of the insert-only set the merge leg exists to respect. ⛔ Nor + * `ignore()`: knex lowers it on MySQL to `insert ignore`, which swallows + * unrelated errors and drops the conflict target. + * + * Kept free of the driver so its compiled SQL is checkable per dialect with no + * server at all (`toSQL()`). + */ +export function withUpsertConflictClause( + insertion: Knex.QueryBuilder, + mergeKeys: readonly string[], + columnsToMerge: readonly string[], +): { statement: Knex.QueryBuilder; merges: boolean } { + if (columnsToMerge.length === 0) return { statement: insertion, merges: false }; + return { statement: insertion.onConflict([...mergeKeys]).merge([...columnsToMerge]), merges: true }; +}