Skip to content

Commit 77a675d

Browse files
fix(driver-sql): upsert() and paged reads honour a columnMap-ed key (#31378)
<!-- claude-projects-attribution: {"github_login":"marchtian"} --> _Requested by **田騏** · [project thread](https://claude.ai/code/project/chan_01VmuUhXU9SSUzqJYyXSmYSi?thread=cmsg_01VmuUhXU9SSUzqJYyXSmYSiCJSKjyyyiryKpcYHe8ewtb)_ Fixes #31342 Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:外部对象用 `external.columnMap` 映射主键时,`upsert()` 的冲突目标(默认的和调用方显式传的 `conflictKeys`)、MySQL 的预检与落点探测、以及写完后的回读,现在都按映射后的远端列名生成;分页读取在调用方排序之后,用映射后的主键列做唯一的兜底排序。 - 为什么改:以前 upsert 写的是 `ON CONFLICT ("id")`。远端表没有 `id` 列时每次都报错;远端表另有一个不是主键的 `id` 列时,已存在的键会报主键冲突而不是合并,新插入的行回读时会读到另一行,而且不报错。分页没有兜底排序时,并列的行在页与页之间可能重复或丢失。 - 风险与回滚:没有 `columnMap` 的对象生成的 SQL 与之前完全一致;没有映射主键的外部对象分页行为不变,不猜列。只改 `packages/drivers/driver-sql`,两个 patch changeset。回滚就是还原这个 PR。 - 先看:`sql-driver-sequence-semantics.ts` 里 `upsert` 的 `conflictColumns`;`sql-driver-field-mapping.ts` 的 `paginationTieBreaker`;`sql-driver-external-columnmap-writes.test.ts` 末尾两个 upsert 的 describe;`sql-driver-external-columnmap-paging.test.ts`。 - 两分钟验证:`pnpm --filter @objectstack/driver-sql exec vitest run src/sql-driver-external-columnmap-writes.test.ts src/sql-driver-external-columnmap-paging.test.ts`,预期 13 个用例全部通过。 - 需要你做的:无,席位落地。 ## What changes Two defects, one commit and one patch changeset each, following #31349 on the same federated-key mapping. Nothing Oracle-specific: the paging fix is a port of the Oracle card's branch commit with its Oracle prose removed. **Upsert.** `upsert()` derives `conflictColumns` (each conflict key through `remoteColumn`) and the key column once, and uses them wherever the statement or a probe names a column: the `ON CONFLICT` target, the no-op merge fallback, the plain-INSERT backing check, the MySQL pre-flight (`assertConflictTargetHonoured`, which compares against physical indexes), the MySQL fence and identity probes (`assertMergeLandedInWrittenOrganization`, `assertMergeLandedOnSuppliedIdentity`), and the read-back. The payload was already in column form (`applyWriteColumnMap`), and `insertOnlyUpsertColumns` already mapped the key. Refusals keep naming the fields the caller passed. **Paged reads.** `paginationTieBreaker` returns `id` for a federated object whose `columnMap` maps a remote column to `id`, so a paged `find()` breaks ties on the mapped key after the caller's keys, as a managed table does. A federated object with no mapped key is unchanged: no column is guessed. ## Why ADR-0015 federation maps a field to a remote column. After #31349 the by-id doors and the analytic reads honoured it; `upsert()` and the pagination tie-breaker still did not. Measured on SQLite before the fix: on a table keyed `CUST_NO`, every upsert failed with `no such column: id`; on a table with its own surrogate `id` beside the mapped key `CODE`, upserting an existing key raised a primary-key violation, and upserting key `1` inserted it but answered the row whose surrogate is 1. ## Verification Measured on this branch (sqlite, the in-tree driver) at a6b16b4, and the pins re-run on the merge head cd627ef: - New pins red with the fix reverted, green with it applied: - upsert, 5 pins in `sql-driver-external-columnmap-writes.test.ts`: 5 of 5 fail with `sql-driver-sequence-semantics.ts` restored to the commit before the fix; the 5 by-id pins in that file stay green as controls. - paging, `sql-driver-external-columnmap-paging.test.ts`: 2 of 3 fail with `sql-driver-field-mapping.ts` restored to `origin/main`; the third is the no-mapped-key control and passes both ways. - All 21 columnMap pins (writes, paging, aggregate) pass with the fix. - `pnpm --filter @objectstack/driver-sql test`: 241 files passed, 12 skipped; 3888 tests passed, 218 skipped (the live PG and MySQL cells, which run in `Temporal Conformance (live PG + MySQL)` on `main`). - `pnpm --filter @objectstack/driver-sql run typecheck`: exit 0. (The first run caught `Array#at` in the ported test, TS2550 under this package's lib; replaced.) - eslint over the five changed source and test files: exit 0. - `dispatch-gates --commands`: 68 derived; all 68 run, every one exit 0; `dispatch-gates --ran`: 68 accounted, 0 NOT-MEASURED, 0 UNRUN. Not measured locally: the MySQL-only probes now name the mapped key column; no MySQL server here, and the existing MySQL cells run on `main`. ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (`packages/spec` not touched.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. (eslint over the changed files, exit 0.) - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. (`@objectstack/driver-sql` patch, two changesets.) - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (Not touched.) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. - [x] The *Before Submitting* list in CONTRIBUTING.md is done. --- _Generated by [Claude Code](https://claude.ai/code/session_017ihL2euiyuLXizr6FW32R3)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 03533d2 commit 77a675d

7 files changed

Lines changed: 204 additions & 18 deletions
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
fix(driver-sql): paged reads of a federated object keyed through `external.columnMap` are a partition, ordered by that key, on every dialect
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
A paged `find()` gets a unique tie-breaker so its pages partition the result set (objectui#3106, #4363). That tie-breaker was imposed only on tables this driver created. A federated object got none, even when its `external.columnMap` names the remote key column (`T_NO: 'id'`), which is the column the by-id doors already address. Ties within the caller's sort were not ordered at all, and an unsorted paged read had no order. SQLite mostly hid this, because a plain table scan returns rowid order. PostgreSQL and MySQL promise no order among ties, so a page walk there can repeat some rows and never serve others.
11+
12+
A paged read of such an object is now ordered by the mapped key after the caller's own keys, the same rule a managed table follows. A federated object with no mapped key keeps the old behaviour exactly: no column is guessed, and the existing warning still names the gap.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/driver-sql': patch
3+
---
4+
5+
fix(driver-sql): `upsert()` resolves its conflict target and its read-back through `external.columnMap` on every dialect
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
A federated object whose `external.columnMap` maps its key to a remote column (`CUST_NO: 'id'`) could not be upserted correctly. The payload was mapped, but the conflict target stayed the field names: the default target compiled `ON CONFLICT ("id")`, and an explicit `conflictKeys` naming the key did the same. On SQLite, PostgreSQL and MySQL alike, on a remote table without an `id` column every upsert failed. On a remote table that also has its own `id` column, which is not the mapped key, the conflict target was that other column: an upsert of an existing key raised a primary-key violation instead of merging, and the read-back of an inserted row matched that other column and answered a different row.
11+
12+
The conflict target, the MySQL pre-flight and identity probes, and the read-back now name the mapped columns. Refusals still name the fields the caller passed. An object without a `columnMap` compiles the statement it compiled before.
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* A paged read of a federated object whose `external.columnMap` names its key
5+
* column is a PARTITION of the result set (objectui#3106, #4363), ordered by
6+
* that key — the column the by-id doors already address.
7+
*
8+
* The tie-breaker used to be imposed only on tables this driver created, so
9+
* every federated page walk was unordered among ties. SQLite mostly hides it
10+
* (a plain table scan returns rowid order), which is why these pins read the
11+
* compiled ORDER BY as well as the rows; a dialect that promises no order
12+
* among ties repeats and loses rows across pages.
13+
*
14+
* A federated object with NO mapped key keeps the old behaviour exactly — no
15+
* column is guessed (`paginationTieBreaker`'s docblock says why).
16+
*/
17+
18+
import { describe, it, expect, afterEach } from 'vitest';
19+
import { SqlDriver } from './index.js';
20+
21+
let driver: any;
22+
afterEach(async () => { await driver?.disconnect(); driver = undefined; });
23+
24+
async function over(columnMap: Record<string, string> | undefined): Promise<{ driver: any; sql: string[] }> {
25+
const d = new SqlDriver({
26+
client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, schemaMode: 'external',
27+
} as any) as any;
28+
d.name = 'extds';
29+
await d.connect();
30+
await d.knex.raw('create table REMOTE_T (T_NO text primary key, STATUS text)');
31+
await d.knex.raw("insert into REMOTE_T values ('b', 'open'), ('a', 'open'), ('c', 'done')");
32+
d.registerExternalObject({
33+
name: 't',
34+
external: { remoteName: 'REMOTE_T', ...(columnMap ? { columnMap } : {}) },
35+
fields: { status: { type: 'text' } },
36+
});
37+
const sql: string[] = [];
38+
d.knex.on('query', (q: { sql: string }) => sql.push(q.sql));
39+
return { driver: d, sql };
40+
}
41+
42+
describe('paged reads of a columnMap-keyed federated object', () => {
43+
it('an unsorted page is ordered by the mapped key column', async () => {
44+
const run = await over({ T_NO: 'id', STATUS: 'status' });
45+
driver = run.driver;
46+
const rows = await driver.find('t', { limit: 2 });
47+
expect(rows.map((r: any) => r.id)).toEqual(['a', 'b']);
48+
expect(run.sql[run.sql.length - 1]).toMatch(/order by [`"]T_NO[`"] asc/);
49+
});
50+
51+
it('a sorted page breaks its ties on the mapped key, in the last key’s direction', async () => {
52+
const run = await over({ T_NO: 'id', STATUS: 'status' });
53+
driver = run.driver;
54+
await driver.find('t', { orderBy: [{ field: 'status', order: 'desc' }], limit: 2 });
55+
expect(run.sql[run.sql.length - 1]).toMatch(/order by [`"]STATUS[`"] desc, [`"]T_NO[`"] desc/);
56+
});
57+
58+
it('an object with no mapped key is left alone: no column is guessed', async () => {
59+
const run = await over(undefined);
60+
driver = run.driver;
61+
await driver.find('t', { orderBy: [{ field: 'status', order: 'asc' }], limit: 2 }).catch(() => undefined);
62+
expect(run.sql[run.sql.length - 1]).not.toMatch(/[`"]id[`"]/);
63+
});
64+
});

‎packages/drivers/driver-sql/src/sql-driver-external-columnmap-writes.test.ts‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,75 @@ describe('a remote table with its own unmapped `id` column (the silent case)', (
9999
expect(await driver.knex('legacy_part').count({ n: '*' })).toEqual([{ n: 2 }]);
100100
});
101101
});
102+
103+
describe('upsert() over a columnMap-ed key', () => {
104+
// The conflict target and the read-back both name the key's mapped column.
105+
// Unmapped, the statement said `on conflict ("id")` — a column this table
106+
// does not have — and every upsert failed.
107+
const ddl = 'create table legacy_cust (CUST_NO text primary key, FULL_NAME text)';
108+
const rows = ["insert into legacy_cust values ('c1', 'Aurora'), ('c2', 'Borealis')"];
109+
const register = (d: any) => d.registerExternalObject({
110+
name: 'cust',
111+
external: { remoteName: 'legacy_cust', columnMap: { CUST_NO: 'id', FULL_NAME: 'name' } },
112+
fields: { name: { type: 'text' } },
113+
});
114+
115+
it('merges onto the row an existing key names', async () => {
116+
driver = await driverOver(ddl, rows);
117+
register(driver);
118+
expect(await driver.upsert('cust', { id: 'c2', name: 'Bore' })).toEqual({ id: 'c2', name: 'Bore' });
119+
const raw = await driver.knex('legacy_cust').select('CUST_NO', 'FULL_NAME').orderBy('CUST_NO');
120+
expect(raw).toEqual([
121+
{ CUST_NO: 'c1', FULL_NAME: 'Aurora' },
122+
{ CUST_NO: 'c2', FULL_NAME: 'Bore' },
123+
]);
124+
});
125+
126+
it('inserts a new key and reads it back', async () => {
127+
driver = await driverOver(ddl, rows);
128+
register(driver);
129+
expect(await driver.upsert('cust', { id: 'c9', name: 'Nine' })).toEqual({ id: 'c9', name: 'Nine' });
130+
expect(await driver.knex('legacy_cust').count({ n: '*' })).toEqual([{ n: 3 }]);
131+
});
132+
133+
it('an explicit conflictKeys naming the key field resolves through the map', async () => {
134+
driver = await driverOver(ddl, rows);
135+
register(driver);
136+
expect(await driver.upsert('cust', { id: 'c1', name: 'Aurora2' }, ['id'])).toEqual({ id: 'c1', name: 'Aurora2' });
137+
expect(await driver.knex('legacy_cust').where('CUST_NO', 'c1').first('FULL_NAME')).toEqual({ FULL_NAME: 'Aurora2' });
138+
});
139+
});
140+
141+
describe('upsert() on a remote table with its own unmapped `id` column (the silent case)', () => {
142+
// `id` is the remote system's surrogate; the object's key is CODE.
143+
const ddl = 'create table legacy_part (id integer unique, CODE text primary key, LABEL text)';
144+
const rows = ["insert into legacy_part values (1, 'B', 'bolt'), (2, 'A', 'anchor')"];
145+
const register = (d: any) => d.registerExternalObject({
146+
name: 'part',
147+
external: { remoteName: 'legacy_part', columnMap: { CODE: 'id', LABEL: 'label' } },
148+
fields: { label: { type: 'text' } },
149+
});
150+
151+
it('merges on the mapped key, not on the surrogate', async () => {
152+
driver = await driverOver(ddl, rows);
153+
register(driver);
154+
// Unmapped, the conflict target was the surrogate, so the insert of an
155+
// existing CODE raised the primary-key violation instead of merging.
156+
expect(await driver.upsert('part', { id: 'A', label: 'anchor!' })).toEqual({ id: 'A', label: 'anchor!' });
157+
const raw = await driver.knex('legacy_part').select('id', 'CODE', 'LABEL').orderBy('id');
158+
expect(raw).toEqual([
159+
{ id: 1, CODE: 'B', LABEL: 'bolt' },
160+
{ id: 2, CODE: 'A', LABEL: 'anchor!' },
161+
]);
162+
});
163+
164+
it('answers the row it wrote, not the row whose surrogate equals the key', async () => {
165+
driver = await driverOver(ddl, rows);
166+
register(driver);
167+
// Key '1' names no row by CODE, so this inserts. The unmapped read-back
168+
// matched surrogate id 1 and answered row B.
169+
expect(await driver.upsert('part', { id: '1', label: 'one' })).toEqual({ id: '1', label: 'one' });
170+
expect(await driver.knex('legacy_part').where('CODE', '1').first('LABEL')).toEqual({ LABEL: 'one' });
171+
expect(await driver.knex('legacy_part').where('CODE', 'B').first('LABEL')).toEqual({ LABEL: 'bolt' });
172+
});
173+
});

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,16 @@ export class FieldMappingFacet {
6060
}
6161

6262
/** The body of `SqlDriver#paginationTieBreaker`; the method's docblock there is its contract. */
63-
static paginationTieBreaker(this: Pick<FieldMappingHost, 'managedObjectFields'>, object: string): string | null {
63+
static paginationTieBreaker(this: Pick<FieldMappingHost, 'managedObjectFields' | 'fieldColumnByObject'>, object: string): string | null {
6464
const tableName = StorageNameMapping.resolveTableName({ name: object } as any);
6565
const managed =
6666
this.managedObjectFields.has(tableName) || this.managedObjectFields.has(object);
67-
return managed ? 'id' : null;
67+
if (managed) return 'id';
68+
// A federated object whose binding NAMES its key column — `external.columnMap`
69+
// mapping a remote column to `id` — is not a guess: the binding declares
70+
// which remote column identifies a row, the column the by-id doors already
71+
// address. Without it, nothing is known and nothing is imposed.
72+
return this.fieldColumnByObject[object]?.id ? 'id' : null;
6873
}
6974

7075
/** The body of `SqlDriver#orderKeysFor`; the method's docblock there is its contract. */

0 commit comments

Comments
 (0)