Skip to content

Commit d69fdb2

Browse files
committed
Merge remote-tracking branch 'origin/main' into claude/issue-21042-native-aggregate-policies
2 parents 61aab50 + ebdb6f2 commit d69fdb2

6 files changed

Lines changed: 354 additions & 16 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/driver-turso': patch
3+
'@objectstack/driver-sql': patch
4+
---
5+
6+
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.
7+
8+
Clause-②: no
9+
10+
**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.
11+
12+
**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.
13+
14+
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‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -466,6 +466,42 @@ function declareRefusalSweep(cell: DialectCell): void {
466466
expect(rows[0].title, 'the alias call must still have merged its other columns').toBe('third');
467467
});
468468

469+
/**
470+
* [#21166] The pins above read the STORE. This one reads the ANSWER, which
471+
* they never did: the call kept the stored `id` and then answered the
472+
* payload's. The read-back looked the row up by the payload's `id`, which a
473+
* merge on a business key never writes, found nothing, and fell back to
474+
* the payload. Measured before the fix, on both cells:
475+
*
476+
* ```
477+
* upsert({ id: 'os21166_new', email, title: 'second' }, ['email'])
478+
* stored id 'os21166_seed' answered id 'os21166_new'
479+
* upsert({ email, title: 'third' }, ['email'])
480+
* stored id 'os21166_seed' answered id = the nanoid minted for the insert that lost
481+
* ```
482+
*
483+
* The insert leg is the control: there the payload's `id` IS the stored
484+
* one, and a fix that answered the wrong row on that leg goes red here.
485+
*/
486+
it('answers the row the merge landed on: its stored `id`, not the payload’s', async () => {
487+
await driver.upsert(BACKED.name, { id: 'os21166_seed', email: 'ans@b.com', title: 'first' }, ['email']);
488+
489+
const supplied = await driver.upsert(BACKED.name, { id: 'os21166_new', email: 'ans@b.com', title: 'second' }, ['email']);
490+
expect(supplied.id, 'the answer names an id no stored row has').toBe('os21166_seed');
491+
expect(supplied.title, 'the answer must be the merged row, merged columns included').toBe('second');
492+
493+
const minted = await driver.upsert(BACKED.name, { email: 'ans@b.com', title: 'third' }, ['email']);
494+
expect(minted.id, 'the answer names the nanoid minted for the insert that lost').toBe('os21166_seed');
495+
expect(minted.title).toBe('third');
496+
497+
const stored = await driver.find(BACKED.name, { where: { email: 'ans@b.com' } });
498+
expect(stored.map((r: any) => ({ id: r.id, title: r.title }))).toEqual([{ id: 'os21166_seed', title: 'third' }]);
499+
500+
const inserted = await driver.upsert(BACKED.name, { id: 'os21166_ins', email: 'ins@b.com', title: 'new' }, ['email']);
501+
expect(inserted.id, 'the insert leg answers the id it wrote').toBe('os21166_ins');
502+
expect(inserted.email).toBe('ins@b.com');
503+
});
504+
469505
/**
470506
* The counterweight, and the reason the three pins above are a repair rather
471507
* than a capability removal: re-keying a row is still possible, through the

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8813,10 +8813,14 @@ export class SqlDriver implements IDataDriver {
88138813
// transaction (inside one the sequence UPDATE rolls back with the refused
88148814
// INSERT, so nothing is burned and there is nothing to repair — measured).
88158815
const mayRetry = options?.transaction === undefined;
8816+
// The row the last attempt SENT, in storage form: the read-back below
8817+
// looks the landed row up by its conflict-key values.
8818+
let sent: Record<string, any> = {};
88168819
for (let attempt = 0; ; attempt++) {
88178820
const reservations = await this.fillAutoNumberFields(object, toUpsert, options);
88188821

88198822
const formatted = this.applyWriteColumnMap(object, this.formatInput(object, toUpsert));
8823+
sent = formatted;
88208824
this.stampInsertTimestamps(object, formatted);
88218825
// [#11176] …and the same slot filled on Postgres/MySQL, where the line
88228826
// above returns early. Without it `updated_at` is not in `formatted`, so
@@ -9000,7 +9004,23 @@ export class SqlDriver implements IDataDriver {
90009004
}
90019005
}
90029006

9003-
const readback = this.getBuilder(object, options).where('id', toUpsert.id);
9007+
// [#21166] Read back the row the statement landed on, by the identity it
9008+
// MATCHED on: the conflict-key values. Reading it back by `toUpsert.id`
9009+
// answers the wrong row on exactly the call #8622 protects: a merge on a
9010+
// business key keeps the stored row's `id`, so the payload's `id` (or the
9011+
// nanoid minted above) names no row, the read finds nothing, and the
9012+
// fallback below answered the PAYLOAD. Measured on SQLite and live
9013+
// Postgres 16: the row stored as `row-a` answered `id: 'row-NEW'`, an id
9014+
// no stored row has. The conflict-key values name the landed row on both
9015+
// legs: the inserted row carries them, and the merged row is the one that
9016+
// matched them. A conflict key the row leaves empty cannot have matched
9017+
// (NULL never conflicts), so the statement inserted and the row carries
9018+
// `toUpsert.id`. On the default `['id']` target the two readings are the
9019+
// same query. The tenant scope is applied to either, as before.
9020+
const matchedOn = mergeKeys.every((k) => sent[k] !== undefined && sent[k] !== null);
9021+
const readback = this.getBuilder(object, options);
9022+
if (matchedOn) for (const k of mergeKeys) readback.where(k, sent[k]);
9023+
else readback.where('id', toUpsert.id);
90049024
this.applyTenantScope(readback, object, options);
90059025
const result = await readback.first();
90069026
return this.formatOutput(object, result) || toUpsert;

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1651,10 +1651,21 @@ export class RemoteTransport {
16511651
throw e;
16521652
}
16531653

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

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

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2615,17 +2615,6 @@ export class TursoDriver extends SqlDriver {
26152615
}
26162616
}
26172617

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

0 commit comments

Comments
 (0)