fix(driver-turso,driver-sql): a business-key upsert keeps the stored id on the remote face, and both faces answer the stored row (#21166)

.changeset/21166-remote-upsert-id-insert-only.md

@@ -0,0 +1,14 @@
+---
+'@objectstack/driver-turso': patch
+'@objectstack/driver-sql': patch
+---
+
+An `upsert` keyed on a business column keeps the stored row's primary key on the Turso remote face, and both drivers answer the stored row.
+
+Clause-②: no
+
+**Remote face (`@objectstack/driver-turso`).** `upsert(object, data, ['email'])` on a remote (hosted) database used to replace the matched row's `id`: with the payload's `id` when it carried one, else with a freshly generated one. Every reference to the old id was left pointing at nothing, and no error was raised. The merge now leaves `id` and `created_at` alone, as the local and embedded-replica faces already do. It reads the columns to leave alone from the same list the local faces use, so `id`, `created_at` and the `auto_number` columns are kept on a merge on every face. An upsert on the primary key (no `conflictKeys`, or `['id']`) is unchanged, and an upsert that inserts still writes the payload's `id`, or a generated one.
+
+**The answer (`@objectstack/driver-sql`, and the remote face).** On such a merge, `upsert` returned the payload instead of the stored row, so the answer carried the payload's `id` (or the generated one), an id no stored row has. It now returns the stored row: its own `id`, with the merged values. The row is read back by the conflict-key values. When a conflict key is empty in the payload, nothing can have matched it, so the row was inserted and it is read back by its `id`, as before.
+
+To change a row's `id` on purpose, use `update()`. An `upsert` never changes it.

packages/drivers/driver-sql/src/sql-driver-upsert-conflict-target-dialects.test.ts

@@ -466,6 +466,42 @@ function declareRefusalSweep(cell: DialectCell): void {
       expect(rows[0].title, 'the alias call must still have merged its other columns').toBe('third');
     });
 
+    /**
+     * [#21166] The pins above read the STORE. This one reads the ANSWER, which
+     * they never did: the call kept the stored `id` and then answered the
+     * payload's. The read-back looked the row up by the payload's `id`, which a
+     * merge on a business key never writes, found nothing, and fell back to
+     * the payload. Measured before the fix, on both cells:
+     *
+     * ```
+     * upsert({ id: 'os21166_new', email, title: 'second' }, ['email'])
+     *   stored id 'os21166_seed'   answered id 'os21166_new'
+     * upsert({ email, title: 'third' }, ['email'])
+     *   stored id 'os21166_seed'   answered id = the nanoid minted for the insert that lost
+     * ```
+     *
+     * The insert leg is the control: there the payload's `id` IS the stored
+     * one, and a fix that answered the wrong row on that leg goes red here.
+     */
+    it('answers the row the merge landed on: its stored `id`, not the payload’s', async () => {
+      await driver.upsert(BACKED.name, { id: 'os21166_seed', email: 'ans@b.com', title: 'first' }, ['email']);
+
+      const supplied = await driver.upsert(BACKED.name, { id: 'os21166_new', email: 'ans@b.com', title: 'second' }, ['email']);
+      expect(supplied.id, 'the answer names an id no stored row has').toBe('os21166_seed');
+      expect(supplied.title, 'the answer must be the merged row, merged columns included').toBe('second');
+
+      const minted = await driver.upsert(BACKED.name, { email: 'ans@b.com', title: 'third' }, ['email']);
+      expect(minted.id, 'the answer names the nanoid minted for the insert that lost').toBe('os21166_seed');
+      expect(minted.title).toBe('third');
+
+      const stored = await driver.find(BACKED.name, { where: { email: 'ans@b.com' } });
+      expect(stored.map((r: any) => ({ id: r.id, title: r.title }))).toEqual([{ id: 'os21166_seed', title: 'third' }]);
+
+      const inserted = await driver.upsert(BACKED.name, { id: 'os21166_ins', email: 'ins@b.com', title: 'new' }, ['email']);
+      expect(inserted.id, 'the insert leg answers the id it wrote').toBe('os21166_ins');
+      expect(inserted.email).toBe('ins@b.com');
+    });
+
     /**
      * The counterweight, and the reason the three pins above are a repair rather
      * than a capability removal: re-keying a row is still possible, through the

packages/drivers/driver-sql/src/sql-driver.ts

@@ -8865,10 +8865,14 @@ export class SqlDriver implements IDataDriver {
     // transaction (inside one the sequence UPDATE rolls back with the refused
     // INSERT, so nothing is burned and there is nothing to repair — measured).
     const mayRetry = options?.transaction === undefined;
+    // The row the last attempt SENT, in storage form: the read-back below
+    // looks the landed row up by its conflict-key values.
+    let sent: Record<string, any> = {};
     for (let attempt = 0; ; attempt++) {
       const reservations = await this.fillAutoNumberFields(object, toUpsert, options);
 
       const formatted = this.applyWriteColumnMap(object, this.formatInput(object, toUpsert));
+      sent = formatted;
       this.stampInsertTimestamps(object, formatted);
       // [#11176] …and the same slot filled on Postgres/MySQL, where the line
       // above returns early. Without it `updated_at` is not in `formatted`, so
@@ -9052,7 +9056,23 @@ export class SqlDriver implements IDataDriver {
       }
     }
 
-    const readback = this.getBuilder(object, options).where('id', toUpsert.id);
+    // [#21166] Read back the row the statement landed on, by the identity it
+    // MATCHED on: the conflict-key values. Reading it back by `toUpsert.id`
+    // answers the wrong row on exactly the call #8622 protects: a merge on a
+    // business key keeps the stored row's `id`, so the payload's `id` (or the
+    // nanoid minted above) names no row, the read finds nothing, and the
+    // fallback below answered the PAYLOAD. Measured on SQLite and live
+    // Postgres 16: the row stored as `row-a` answered `id: 'row-NEW'`, an id
+    // no stored row has. The conflict-key values name the landed row on both
+    // legs: the inserted row carries them, and the merged row is the one that
+    // matched them. A conflict key the row leaves empty cannot have matched
+    // (NULL never conflicts), so the statement inserted and the row carries
+    // `toUpsert.id`. On the default `['id']` target the two readings are the
+    // same query. The tenant scope is applied to either, as before.
+    const matchedOn = mergeKeys.every((k) => sent[k] !== undefined && sent[k] !== null);
+    const readback = this.getBuilder(object, options);
+    if (matchedOn) for (const k of mergeKeys) readback.where(k, sent[k]);
+    else readback.where('id', toUpsert.id);
     this.applyTenantScope(readback, object, options);
     const result = await readback.first();
     return this.formatOutput(object, result) || toUpsert;

packages/drivers/driver-turso/src/remote-transport.ts

@@ -1651,10 +1651,21 @@ export class RemoteTransport {
       throw e;
     }
 
-    // Fetch the result row
+    // Fetch the row the statement landed on, by the identity it MATCHED on:
+    // the conflict-key values. [#21166] Reading it back by the payload's `id`
+    // answers the wrong row once `id` is insert-only: a merge on a business
+    // key keeps the stored row's `id`, so the payload's `id` (or the nanoid
+    // minted above) names no row, the read finds nothing, and the fallback
+    // below answered the PAYLOAD as if it had been stored. The conflict-key
+    // values name the landed row on both legs: the inserted row carries them,
+    // and the merged row is the one that matched them. A conflict key the
+    // payload leaves empty cannot have matched (NULL never conflicts), so the
+    // statement inserted and the row carries this call's `id`. On the default
+    // `['id']` target the two readings are the same statement.
+    const keyColumns = mergeKeys.every((k) => toUpsert[k] !== undefined && toUpsert[k] !== null) ? mergeKeys : ['id'];
     const result = await this.client!.execute({
-      sql: `SELECT * FROM ${this.tableSql(table)} WHERE "id" = ?`,
-      args: [toUpsert.id],
+      sql: `SELECT * FROM ${this.tableSql(table)} WHERE ${keyColumns.map((k) => `"${k}" = ?`).join(' AND ')}`,
+      args: keyColumns.map((k) => this.serializeValue(toUpsert[k])),
     });
     const rows = this.mapRows(result);
     return rows[0] || toUpsert;

packages/drivers/driver-turso/src/turso-driver.ts

@@ -2615,17 +2615,6 @@ export class TursoDriver extends SqlDriver {
     }
   }
 
-  /**
-   * The `auto_number` columns of `object`, looked up as `fillAutoNumberFields`
-   * looks them up (object name first, then the physical table it maps to), so
-   * the columns named insert-only to the transport are exactly the ones a
-   * number was issued for.
-   */
-  private remoteAutoNumberColumns(object: string, table: string): string[] {
-    const cfgs = this.autoNumberFields[object] || this.autoNumberFields[table];
-    return cfgs ? cfgs.map((cfg) => cfg.name) : [];
-  }
-
   // [#15267] The override declares the contract's type, as both of its branches
   // already do: `RemoteTransport.create()` answers `Record<string, unknown>`
   // through the generic `formatRemoteRow`, and the local branch forwards to
@@ -2683,7 +2672,19 @@ export class TursoDriver extends SqlDriver {
       // going unused exactly as it does on the local faces (a gap in the
       // sequence, never a renumbering). An explicit payload value does not
       // renumber a merged row either; `update()` is the renumbering path.
-      const insertOnly = this.remoteAutoNumberColumns(object, table);
+      //
+      // [#21166] The insert-only set is `insertOnlyUpsertColumns` itself, the
+      // list the local faces build their merge set from, so it names `id` and
+      // `created_at` beside the autonumber columns. This face once handed the
+      // transport the autonumber columns alone, from a lookup of its own, so a
+      // merge on a business key (`conflictKeys: ['email']`) wrote
+      // `"id" = excluded."id"` and replaced the stored row's primary key with
+      // the payload's, or with the nanoid minted for the insert that lost
+      // (#8622's re-key, on this face). One list, so the faces cannot drift on
+      // which columns a merge may write. A remote object never renames a
+      // column (`remoteTableFor` refuses a renaming column map first), so the
+      // list's physical names are the names the transport writes.
+      const insertOnly = [...this.insertOnlyUpsertColumns(object)];
       const written = await this.writeRemoteRowWithAutoNumbers(object, { ...data }, options, (filled) =>
         this.remoteTransport!.upsert(object, this.toRemoteWriteForms(object, filled), conflictKeys, table, insertOnly),
       );