Skip to content

Commit 24c9013

Browse files
os-zhuangclaude
andauthored
fix(driver-sql): materialize declared object-level indexes (#1461)
* fix(driver-sql): materialize declared object-level indexes (#1459) The SQL driver synced columns and field-level `unique`, but silently dropped object-level declared `indexes` ([{ fields, unique }]). As a result documented multi-column UNIQUE guarantees (notification delivery dedup, receipt upserts, dedup_key convergence, api_key/slug uniqueness) were never enforced at the DB level — a fresh `dev --fresh` sqlite DB showed only PK autoindexes. Wire declared-index sync into `initObjects`: - materialize single- and multi-column indexes after the table is created/altered, including UNIQUE - NULL-distinct semantics are the cross-dialect default, so multiple NULL rows stay insertable while non-NULL duplicates are rejected - idempotent: deterministic, length-bounded index names + per-dialect existing-index introspection (sqlite/pg/mysql); "already exists" races are absorbed - indexes referencing a non-materialized (virtual/formula) column are skipped with a warning instead of failing sync Flip the `indexes` capability flag to true and add tests asserting the index is materialized (PRAGMA index_list), rejects duplicates, allows multiple NULLs, covers compound + non-unique indexes, and is idempotent. https://claude.ai/code/session_01AgJ1guiaybKRbMfqAPCDeN * chore: add changeset for driver-sql declared-index materialization https://claude.ai/code/session_01AgJ1guiaybKRbMfqAPCDeN --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ffb5422 commit 24c9013

3 files changed

Lines changed: 294 additions & 1 deletion

File tree

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
"@objectstack/driver-sql": minor
3+
---
4+
5+
fix(driver-sql): materialize declared object-level indexes (#1459)
6+
7+
The SQL driver synced columns and field-level `unique`, but silently dropped
8+
object-level declared `indexes` (`ObjectSchema.indexes: [{ fields, unique }]`).
9+
As a result several documented multi-column UNIQUE / dedup guarantees were
10+
never enforced at the DB level — a fresh `dev --fresh` sqlite DB showed only
11+
primary-key autoindexes.
12+
13+
`initObjects` now materializes declared indexes (`syncDeclaredIndexes`) after
14+
the table is created/altered:
15+
16+
- single- and multi-column indexes, including `UNIQUE`
17+
- NULL-distinct semantics (the cross-dialect default), so multiple NULL rows
18+
stay insertable while non-NULL duplicates are rejected — matching the
19+
convergence-on-conflict pattern the messaging pipeline relies on
20+
- idempotent: deterministic, length-bounded index names + per-dialect
21+
existing-index introspection (sqlite/pg/mysql); "already exists" races are
22+
absorbed
23+
- indexes referencing a non-materialized (virtual `formula`) column are skipped
24+
with a warning instead of failing sync
25+
26+
The `indexes` driver capability flag is now `true`.

packages/plugins/driver-sql/src/sql-driver-schema.test.ts

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,148 @@ describe('SqlDriver Schema Sync (SQLite)', () => {
291291
expect(columns).toHaveProperty('display_name');
292292
});
293293

294+
it('should materialize a declared single-column unique index and reject duplicates', async () => {
295+
const objects = [
296+
{
297+
name: 'idx_unique_obj',
298+
fields: {
299+
slug: { type: 'string' },
300+
},
301+
indexes: [{ fields: ['slug'], unique: true }],
302+
},
303+
];
304+
305+
await driver.initObjects(objects as any);
306+
307+
// PRAGMA proves a non-PK unique index was actually created.
308+
const list: any = await knexInstance.raw('PRAGMA index_list(idx_unique_obj)');
309+
const declared = list.filter((i: any) => !String(i.name).startsWith('sqlite_autoindex'));
310+
expect(declared.length).toBe(1);
311+
expect(declared[0].unique).toBe(1);
312+
313+
await driver.create('idx_unique_obj', { slug: 's1' });
314+
await expect(driver.create('idx_unique_obj', { slug: 's1' })).rejects.toThrow(
315+
/UNIQUE constraint failed|duplicate key value/,
316+
);
317+
});
318+
319+
it('should materialize a declared multi-column unique index (compound dedup)', async () => {
320+
const objects = [
321+
{
322+
name: 'idx_multi_obj',
323+
fields: {
324+
notification_id: { type: 'string' },
325+
recipient_id: { type: 'string' },
326+
channel: { type: 'string' },
327+
},
328+
indexes: [{ fields: ['notification_id', 'recipient_id', 'channel'], unique: true }],
329+
},
330+
];
331+
332+
await driver.initObjects(objects as any);
333+
334+
const list: any = await knexInstance.raw('PRAGMA index_list(idx_multi_obj)');
335+
const declared = list.filter((i: any) => !String(i.name).startsWith('sqlite_autoindex'));
336+
expect(declared.length).toBe(1);
337+
expect(declared[0].unique).toBe(1);
338+
339+
await driver.create('idx_multi_obj', { notification_id: 'n1', recipient_id: 'r1', channel: 'bell' });
340+
// Same tuple → rejected.
341+
await expect(
342+
driver.create('idx_multi_obj', { notification_id: 'n1', recipient_id: 'r1', channel: 'bell' }),
343+
).rejects.toThrow(/UNIQUE constraint failed|duplicate key value/);
344+
// Differing channel → allowed.
345+
await driver.create('idx_multi_obj', { notification_id: 'n1', recipient_id: 'r1', channel: 'email' });
346+
const rows = await driver.find('idx_multi_obj', {});
347+
expect(rows.length).toBe(2);
348+
});
349+
350+
it('should allow multiple NULLs under a declared unique index (NULL-distinct)', async () => {
351+
const objects = [
352+
{
353+
name: 'idx_null_obj',
354+
fields: {
355+
dedup_key: { type: 'string' },
356+
},
357+
indexes: [{ fields: ['dedup_key'], unique: true }],
358+
},
359+
];
360+
361+
await driver.initObjects(objects as any);
362+
363+
// Two rows with no dedup_key (NULL) must both be insertable.
364+
await driver.create('idx_null_obj', {});
365+
await driver.create('idx_null_obj', {});
366+
const rows = await driver.find('idx_null_obj', {});
367+
expect(rows.length).toBe(2);
368+
});
369+
370+
it('should materialize a declared non-unique index', async () => {
371+
const objects = [
372+
{
373+
name: 'idx_plain_obj',
374+
fields: {
375+
status: { type: 'string' },
376+
partition_key: { type: 'string' },
377+
},
378+
indexes: [{ fields: ['status', 'partition_key'] }],
379+
},
380+
];
381+
382+
await driver.initObjects(objects as any);
383+
384+
const list: any = await knexInstance.raw('PRAGMA index_list(idx_plain_obj)');
385+
const declared = list.filter((i: any) => !String(i.name).startsWith('sqlite_autoindex'));
386+
expect(declared.length).toBe(1);
387+
expect(declared[0].unique).toBe(0);
388+
});
389+
390+
it('should be idempotent when initObjects runs repeatedly with declared indexes', async () => {
391+
const objects = [
392+
{
393+
name: 'idx_idem_obj',
394+
fields: { slug: { type: 'string' } },
395+
indexes: [{ fields: ['slug'], unique: true }],
396+
},
397+
];
398+
399+
await driver.initObjects(objects as any);
400+
// Second run must not throw "index already exists".
401+
await driver.initObjects(objects as any);
402+
403+
const list: any = await knexInstance.raw('PRAGMA index_list(idx_idem_obj)');
404+
const declared = list.filter((i: any) => !String(i.name).startsWith('sqlite_autoindex'));
405+
expect(declared.length).toBe(1);
406+
});
407+
408+
it('should skip a declared index referencing a non-materialized (virtual) column', async () => {
409+
const warnings: string[] = [];
410+
(driver as any).logger = { warn: (msg: string) => warnings.push(msg) };
411+
412+
const objects = [
413+
{
414+
name: 'idx_virtual_obj',
415+
fields: {
416+
name: { type: 'string' },
417+
total: { type: 'formula', expression: 'a + b', data_type: 'number' },
418+
},
419+
indexes: [{ fields: ['total'], unique: true }],
420+
},
421+
];
422+
423+
// Must not throw even though `total` has no physical column.
424+
await driver.initObjects(objects as any);
425+
426+
const list: any = await knexInstance.raw('PRAGMA index_list(idx_virtual_obj)');
427+
const declared = list.filter((i: any) => !String(i.name).startsWith('sqlite_autoindex'));
428+
expect(declared.length).toBe(0);
429+
expect(warnings.some((w) => w.includes('total'))).toBe(true);
430+
});
431+
432+
it('reports indexes capability as supported', () => {
433+
expect((driver as any).supports.indexes).toBe(true);
434+
});
435+
294436
it('should use short object name as physical table name in initObjects', async () => {
295437
const objects = [
296438
{

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

Lines changed: 126 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { StorageNameMapping } from '@objectstack/spec/system';
1313
import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared';
1414
import knex, { Knex } from 'knex';
1515
import { nanoid } from 'nanoid';
16+
import { createHash } from 'node:crypto';
1617

1718
/**
1819
* Default ID length for auto-generated IDs.
@@ -134,7 +135,9 @@ export class SqlDriver implements IDataDriver {
134135
schemaSync: true,
135136
batchSchemaSync: false,
136137
migrations: false,
137-
indexes: false,
138+
// Object-level declared `indexes` (incl. multi-column UNIQUE) are
139+
// materialized during `initObjects` — see `syncDeclaredIndexes`.
140+
indexes: true,
138141

139142
// Performance & Optimization
140143
connectionPooling: true,
@@ -1128,6 +1131,128 @@ export class SqlDriver implements IDataDriver {
11281131
}
11291132
});
11301133
}
1134+
1135+
// Materialize object-level declared indexes (`indexes: [{ fields,
1136+
// unique }]`). These are distinct from field-level `unique` (handled
1137+
// in `createColumn`) and carry the multi-column UNIQUE guarantees that
1138+
// dedup/convergence paths rely on (ADR-0030). Done after the table is
1139+
// created/altered so every referenced column physically exists.
1140+
const declaredIndexes = (obj as any).indexes;
1141+
if (Array.isArray(declaredIndexes) && declaredIndexes.length > 0) {
1142+
const colInfo = await this.knex(tableName).columnInfo();
1143+
const physicalColumns = new Set(Object.keys(colInfo));
1144+
await this.syncDeclaredIndexes(tableName, declaredIndexes, physicalColumns);
1145+
}
1146+
}
1147+
}
1148+
1149+
/**
1150+
* Build a deterministic index name for a declared index so repeated
1151+
* `initObjects` runs converge on the same identifier (and can detect an
1152+
* already-materialized index by name). Long names are hash-suffixed to
1153+
* stay within the 63/64-char identifier limits of Postgres/MySQL.
1154+
*/
1155+
protected buildIndexName(tableName: string, fields: string[], unique: boolean): string {
1156+
const prefix = unique ? 'uniq' : 'idx';
1157+
const base = `${prefix}_${tableName}_${fields.join('_')}`;
1158+
const MAX = 60;
1159+
if (base.length <= MAX) return base;
1160+
const hash = createHash('sha1').update(base).digest('hex').slice(0, 8);
1161+
return `${`${prefix}_${tableName}`.slice(0, MAX - 9)}_${hash}`;
1162+
}
1163+
1164+
/**
1165+
* Read the names of indexes that already exist on a table, per dialect.
1166+
* Used to make declared-index sync idempotent across repeated runs.
1167+
* Failures are swallowed — at worst we attempt a create and absorb the
1168+
* "already exists" error in `syncDeclaredIndexes`.
1169+
*/
1170+
protected async getExistingIndexNames(tableName: string): Promise<Set<string>> {
1171+
const names = new Set<string>();
1172+
try {
1173+
if (this.isSqlite) {
1174+
const safe = tableName.replace(/[^a-zA-Z0-9_]/g, '');
1175+
const rows: any = await this.knex.raw(`PRAGMA index_list(${safe})`);
1176+
for (const r of rows) names.add(r.name);
1177+
} else if (this.isPostgres) {
1178+
const res: any = await this.knex.raw(
1179+
`SELECT indexname FROM pg_indexes WHERE schemaname = 'public' AND tablename = ?`,
1180+
[tableName],
1181+
);
1182+
for (const r of res.rows) names.add(r.indexname);
1183+
} else if (this.isMysql) {
1184+
const res: any = await this.knex.raw(
1185+
`SELECT INDEX_NAME FROM information_schema.STATISTICS WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ?`,
1186+
[tableName],
1187+
);
1188+
for (const r of res[0]) names.add(r.INDEX_NAME);
1189+
}
1190+
} catch {
1191+
// Best-effort — fall through and let creation handle conflicts.
1192+
}
1193+
return names;
1194+
}
1195+
1196+
/**
1197+
* Materialize declared object-level indexes.
1198+
*
1199+
* - Multi-column and single-column indexes are both supported.
1200+
* - `unique: true` emits a UNIQUE index. NULL-distinct semantics are the
1201+
* default across SQLite/Postgres/MySQL, so multiple NULL rows remain
1202+
* allowed while non-NULL duplicates are rejected — matching the
1203+
* convergence-on-conflict pattern the messaging pipeline relies on.
1204+
* - Idempotent: indexes already present (by deterministic name) are
1205+
* skipped, and an "already exists" race is absorbed.
1206+
* - Indexes referencing a column that wasn't materialized (e.g. a virtual
1207+
* `formula` field) are skipped with a warning rather than failing sync.
1208+
*/
1209+
protected async syncDeclaredIndexes(
1210+
tableName: string,
1211+
indexes: Array<{ name?: string; fields?: string[]; unique?: boolean }>,
1212+
physicalColumns: Set<string>,
1213+
): Promise<void> {
1214+
const existing = await this.getExistingIndexNames(tableName);
1215+
1216+
for (const idx of indexes) {
1217+
const fields = Array.isArray(idx?.fields)
1218+
? idx.fields.filter((f): f is string => typeof f === 'string' && f.length > 0)
1219+
: [];
1220+
if (fields.length === 0) continue;
1221+
1222+
const missing = fields.filter((f) => !physicalColumns.has(f));
1223+
if (missing.length > 0) {
1224+
this.logger.warn(
1225+
`[sql-driver] skipping declared index on "${tableName}" — column(s) not materialized: ${missing.join(', ')}`,
1226+
{ tableName, fields },
1227+
);
1228+
continue;
1229+
}
1230+
1231+
const unique = idx.unique === true;
1232+
const name =
1233+
typeof idx.name === 'string' && idx.name.trim()
1234+
? idx.name.trim()
1235+
: this.buildIndexName(tableName, fields, unique);
1236+
1237+
if (existing.has(name)) continue;
1238+
1239+
try {
1240+
await this.knex.schema.alterTable(tableName, (table) => {
1241+
if (unique) {
1242+
table.unique(fields, { indexName: name });
1243+
} else {
1244+
table.index(fields, name);
1245+
}
1246+
});
1247+
existing.add(name);
1248+
} catch (e: any) {
1249+
const msg = String(e?.message ?? e);
1250+
// A concurrent creator or a pre-existing equivalent index under a
1251+
// different name can race us here — both are benign for our intent
1252+
// (the index exists). Anything else is a real failure.
1253+
if (/already exists|duplicate key name|exists/i.test(msg)) continue;
1254+
throw e;
1255+
}
11311256
}
11321257
}
11331258

0 commit comments

Comments
 (0)