diff --git a/.changeset/index-drift-migrate-plan.md b/.changeset/index-drift-migrate-plan.md new file mode 100644 index 0000000000..fcefb85a7c --- /dev/null +++ b/.changeset/index-drift-migrate-plan.md @@ -0,0 +1,51 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/spec": patch +"@objectstack/cli": patch +--- + +feat(driver-sql)!: make index drift visible to `os migrate plan` — no more silent DDL at boot (#3728) + +The #3696 unique-scope migration converged **in place**: `syncTableIndexes` ran a +`DROP` + `CREATE UNIQUE INDEX` during `initObjects`, in every environment, +leaving one log line behind. `os migrate plan` showed nothing, because +`detectManagedDrift` was column-only — `ManagedDriftOp` had no index dimension at +all. An operator who wanted to review the DDL before it reached their database +had no way to, and a managed schema was being auto-altered in production, which +the #2186 contract explicitly forbids. + +Index drift is now a first-class dimension, reconciled through the same path as +column drift: + +- **`syncTableIndexes` is additive only.** It creates indexes; it never drops or + rewrites one. `dropLegacyGlobalUniques` is gone. +- **New `DriftOp` variants** — `replace_unique_index` (safe: retire the legacy + platform-wide unique in favour of the tenant composite), `create_index` (safe), + `recreate_index` (needs-confirm; destructive when it tightens to `UNIQUE`), and + `drop_index` (destructive). +- **`detectManagedDrift` reports them**, `os migrate plan` renders them (index + ops display as `table [index_name]`), and `os migrate apply` executes them. + Index DDL is portable, so it applies directly on every dialect — no SQLite + table rebuild. +- **`replace_unique_index` creates before it drops**, so uniqueness is never + unenforced mid-migration and a failed create leaves the schema untouched. +- **Declared `indexes[]` drift is covered too**: an index metadata declares but + the database lacks, and one whose definition no longer matches the declaration + (the additive sync skips those by name, so they could never self-heal). +- **Orphan detection is limited to ObjectStack's own generated naming** + (`uniq_…` / `idx_…`, plus the pre-#3696 `__unique` knex + spelling). A hand-rolled operational index is never reported as drift and + `--allow-destructive` will not delete it. + +**Behaviour change.** Boot no longer rewrites the index unconditionally. Dev +(`autoMigrate: 'safe'`, what `os dev` / `os serve` use) still self-heals on +restart, so local workflows are unchanged. Production now **warns** with an +actionable `os migrate` hint and leaves the schema alone — the deployment stays +on the legacy global unique (multi-tenant inserts still collide) until someone +runs `os migrate apply`. That is the deliberate trade: a visible, pre-inspectable +migration instead of an invisible one. + +Also fixed: `managedObjectIndexes` was never cleared when an object dropped its +`indexes[]`, so drift detection kept expecting an index nobody declared. + +`SchemaDiffEntryKind` gains `index_mismatch` and `unmapped_index`. diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index 79ad3ab179..2e862af038 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -477,12 +477,12 @@ os info --json # JSON output for tooling ### Schema migrations The metadata→database sync is **additive-only**: on boot it creates missing -tables and adds new columns, but never alters or drops existing ones. So a -*non-additive* change to an object already backed by a database — relaxing -`required` (drop `NOT NULL`), changing a field's type/length, or removing a -field — silently diverges from the live schema, and the physical column wins at -write time. `os migrate` reconciles the database to the metadata (the source of -truth). +tables, adds new columns and creates missing indexes, but never alters or drops +existing ones. So a *non-additive* change to an object already backed by a +database — relaxing `required` (drop `NOT NULL`), changing a field's +type/length, removing a field, or re-scoping a `unique` constraint — silently +diverges from the live schema, and the physical column wins at write time. +`os migrate` reconciles the database to the metadata (the source of truth). | Command | Description | |---------|-------------| @@ -499,16 +499,32 @@ os migrate plan --json # Machine-readable output | Category | Examples | Applied by | |----------|----------|------------| -| `safe` | relax `NOT NULL` → nullable, widen a `varchar` | `os migrate apply` (and dev auto-reconcile) | -| `needs_confirm` | non-narrowing type change | `os migrate apply` | -| `destructive` | drop an orphaned column, tighten `NOT NULL`, narrow a type | `os migrate apply --allow-destructive` | +| `safe` | relax `NOT NULL` → nullable, widen a `varchar`, create a declared index, replace a legacy global unique with its tenant-scoped composite | `os migrate apply` (and dev auto-reconcile) | +| `needs_confirm` | non-narrowing type change, rebuild a non-unique index whose columns changed | `os migrate apply` | +| `destructive` | drop an orphaned column or index, tighten `NOT NULL`, narrow a type, rebuild an index as `UNIQUE` | `os migrate apply --allow-destructive` | + +#### Index drift + +`plan` covers indexes as well as columns: + +| Op | What it means | +|----|---------------| +| `create_index` | Metadata declares an index the database does not have | +| `replace_unique_index` | A field's `unique` used to be enforced platform-wide, but metadata now scopes it per tenant — the legacy single-column index is swapped for the `(tenantField, field)` composite. A pure relaxation: it creates before it drops, and cannot fail | +| `recreate_index` | An index exists under the declared name but with different columns/uniqueness. The additive sync skips it by name, so it must be dropped and rebuilt | +| `drop_index` | An index carrying ObjectStack's generated naming (`uniq_…` / `idx_…`) that metadata no longer declares | + +Orphan detection is deliberately limited to indexes ObjectStack itself +generated. A hand-rolled covering index you added in `psql` is never reported as +drift, and `--allow-destructive` will not delete it. **Dev self-heal.** `os dev` runs the SQL driver with `autoMigrate: 'safe'`, so -loosening changes (e.g. you just made a field optional) are applied to your -existing dev database automatically on restart — no `os migrate` needed, no data -loss. Auto-reconcile is **dev-only and never destructive**; it is force-disabled -under `NODE_ENV=production`, where you run `os migrate` deliberately. +safe changes (you just made a field optional; a `unique` field became +tenant-scoped) are applied to your existing dev database automatically on +restart — no `os migrate` needed, no data loss. Auto-reconcile is **dev-only and +never destructive**; it is force-disabled under `NODE_ENV=production`, where +every change is shown by `os migrate plan` before you apply it deliberately. diff --git a/packages/cli/src/utils/schema-migrate.integration.test.ts b/packages/cli/src/utils/schema-migrate.integration.test.ts index 9ef6ab0d77..ccd7dfb4e2 100644 --- a/packages/cli/src/utils/schema-migrate.integration.test.ts +++ b/packages/cli/src/utils/schema-migrate.integration.test.ts @@ -8,10 +8,16 @@ import { SqlDriver } from '@objectstack/driver-sql'; import { bootSchemaStack } from './schema-migrate.js'; /** - * End-to-end (#2186): boot the real standalone stack via `bootSchemaStack` - * against a pre-seeded "legacy" SQLite DB (organization_id created NOT NULL), - * then verify `os migrate`'s engine detects the drift and reconciles it — - * exercising the full createStandaloneStack → AppPlugin → ObjectQL → driver path. + * End-to-end (#2186, #3728): boot the real standalone stack via + * `bootSchemaStack` against a pre-seeded "legacy" SQLite DB — `organization_id` + * created NOT NULL, and a platform-wide UNIQUE index on a field metadata now + * scopes per tenant — then verify `os migrate`'s engine detects BOTH drifts and + * reconciles them, exercising the full createStandaloneStack → AppPlugin → + * ObjectQL → driver path. + * + * The index half is the #3728 acceptance: under `NODE_ENV=production` nothing + * self-heals at boot, so whatever `plan` reports here is exactly what an + * operator would see before any DDL touches their database. */ describe('bootSchemaStack + migrate engine (integration)', () => { let dir: string; @@ -36,13 +42,15 @@ describe('bootSchemaStack + migrate engine (integration)', () => { fields: { name: { type: 'text', required: true }, organization_id: { type: 'text', required: false }, // optional now + code: { type: 'text', unique: true }, // tenant-scoped since #3696 }, }, ], }), ); - // Seed a "legacy" DB where organization_id is NOT NULL (the #2178 shape). + // Seed a "legacy" DB: organization_id NOT NULL (the #2178 shape) plus the + // platform-wide unique index on `code` the pre-#3696 driver emitted. const seed = new SqlDriver({ client: 'better-sqlite3', connection: { filename: dbFile }, useNullAsDefault: true }); const k = (seed as any).knex; await k.schema.createTable('mig_biz_unit', (t: any) => { @@ -51,8 +59,10 @@ describe('bootSchemaStack + migrate engine (integration)', () => { t.timestamp('updated_at'); t.string('name').notNullable(); t.string('organization_id').notNullable(); + t.string('code'); }); - await k('mig_biz_unit').insert({ id: '1', name: 'Acme', organization_id: 'org1' }); + await k.raw('CREATE UNIQUE INDEX mig_biz_unit_code_unique ON mig_biz_unit (code)'); + await k('mig_biz_unit').insert({ id: '1', name: 'Acme', organization_id: 'org1', code: 'BU-00001' }); await k.destroy(); savedEnv.OS_ARTIFACT_PATH = process.env.OS_ARTIFACT_PATH; @@ -67,24 +77,41 @@ describe('bootSchemaStack + migrate engine (integration)', () => { try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } }); - it('detects the NOT NULL drift, applies it, and self-verifies in-sync', async () => { + it('detects the NOT NULL + legacy-unique drift, applies both, and self-verifies in-sync', async () => { const stack = await bootSchemaStack({ databaseUrl: `file:${dbFile}` }); try { expect(stack.driver).toBeTruthy(); expect(stack.managedTableCount).toBeGreaterThan(0); + // Boot changed nothing — this is exactly what `os migrate plan` renders. const drift = await stack.driver!.detectManagedDrift(); + const org = drift.find((d) => d.table === 'mig_biz_unit' && d.column === 'organization_id'); expect(org, 'expected drift on mig_biz_unit.organization_id').toBeDefined(); expect(org!.category).toBe('safe'); expect(org!.op.type).toBe('relax_not_null'); + // #3728: the legacy platform-wide unique is PLANNED, not silently rewritten. + const idx = drift.find((d) => d.op.type === 'replace_unique_index'); + expect(idx, 'expected replace_unique_index drift on mig_biz_unit.code').toBeDefined(); + expect(idx!.category).toBe('safe'); + expect(idx!.op).toMatchObject({ + table: 'mig_biz_unit', + dropIndexNames: ['mig_biz_unit_code_unique'], + createColumns: ['organization_id', 'code'], + }); + const { applied, skipped } = await stack.driver!.applyMigrationEntries(drift, { allowDestructive: false }); expect(applied.some((d) => d.op.type === 'relax_not_null')).toBe(true); + expect(applied.some((d) => d.op.type === 'replace_unique_index')).toBe(true); expect(skipped).toHaveLength(0); const after = await stack.driver!.detectManagedDrift(); expect(after.find((d) => d.table === 'mig_biz_unit' && d.column === 'organization_id')).toBeUndefined(); + expect(after.find((d) => d.op.type === 'replace_unique_index')).toBeUndefined(); + + // The seeded row survived the reconcile. + expect(await (stack.driver as any).count('mig_biz_unit', {})).toBe(1); } finally { await stack.shutdown(); } diff --git a/packages/cli/src/utils/schema-migrate.ts b/packages/cli/src/utils/schema-migrate.ts index 121f690984..e3d884369b 100644 --- a/packages/cli/src/utils/schema-migrate.ts +++ b/packages/cli/src/utils/schema-migrate.ts @@ -136,6 +136,17 @@ export function groupByCategory(drift: ManagedDriftEntry[]): Record = new Set([ + 'replace_unique_index', + 'create_index', + 'drop_index', + 'recreate_index', +]); + +export type IndexDriftOp = Extract< + DriftOp, + { type: 'replace_unique_index' | 'create_index' | 'drop_index' | 'recreate_index' } +>; +/** Ops that act on a single column — the only ones with a guaranteed `column`. */ +export type ColumnDriftOp = Exclude; + +/** True when this op mutates an index rather than a column. */ +export function isIndexDriftOp(op: DriftOp): op is IndexDriftOp { + return INDEX_DRIFT_OPS.has(op.type); +} /** * A managed-schema drift finding: a {@link SchemaDiffEntry} enriched with the @@ -216,5 +284,364 @@ export function diffManagedTable(args: { /** Stable de-dup / sort key for a drift entry. */ export function driftKey(d: ManagedDriftEntry): string { - return `${d.table}.${d.column ?? ''}:${d.kind}`; + const op: any = d.op; + // Several index findings can share a table+column+kind (a table can have more + // than one index over the same leading column), so the index name has to be + // part of the key or the boot-time warn de-dup swallows all but the first. + const idx = op.indexName ?? op.createIndexName ?? ''; + return `${d.table}.${d.column ?? ''}:${d.kind}:${d.op.type}${idx ? `:${idx}` : ''}`; +} + +// ─────────────────────────────────────────────────────────────────────── +// Index dimension (#3728) +// +// `diffManagedTable` above is column-only, which left one whole class of +// divergence invisible to `os migrate plan`: indexes. The #3696 unique-scope +// migration used to paper over that by executing its DROP + CREATE inline at +// boot — DDL nobody could pre-inspect, in every environment, in violation of +// the #2186 rule that a managed schema is never auto-altered in production. +// Everything below exists so that migration (and declared-index drift +// generally) is *detected* rather than silently performed, and reconciled +// through the same `os migrate plan` / `apply` path as column drift. +// ─────────────────────────────────────────────────────────────────────── + +/** Identifier budget for generated index names (Postgres caps at 63, MySQL 64). */ +const INDEX_NAME_MAX = 60; +/** Chars kept from `_
` before the `_` suffix of a truncated name. */ +const INDEX_NAME_HEAD = INDEX_NAME_MAX - 9; + +/** + * Build a deterministic index name so repeated syncs converge on the same + * identifier (and an already-materialized index is recognisable by name). + * Long names are hash-suffixed to stay inside the dialect identifier limits. + * + * This is the single definition — `SqlDriver.buildIndexName` delegates here, so + * the names the driver *creates* and the names the differ *looks for* cannot + * drift apart. + */ +export function buildIndexName(table: string, columns: string[], unique: boolean): string { + const prefix = unique ? 'uniq' : 'idx'; + const base = `${prefix}_${table}_${columns.join('_')}`; + if (base.length <= INDEX_NAME_MAX) return base; + const hash = createHash('sha1').update(base).digest('hex').slice(0, 8); + return `${`${prefix}_${table}`.slice(0, INDEX_NAME_HEAD)}_${hash}`; +} + +/** An index metadata says should exist. */ +export interface ExpectedIndex { + name: string; + columns: string[]; + unique: boolean; +} + +/** An index that physically exists (see `SqlDriver.introspectIndexes`). */ +export interface PhysicalIndex { + name: string; + columns: string[]; + unique: boolean; + /** Backing index of the PRIMARY KEY — never metadata-managed. */ + primary?: boolean; +} + +/** + * Translate field-level `unique` declarations into concrete index descriptors, + * applying tenant scoping (#3696). + * + * This is the ONLY place field-level uniqueness becomes an index, so the + * create-table, alter-table, SQLite-rebuild and drift-detection paths cannot + * disagree about what a `unique: true` field is supposed to produce. + * + * Scoping rule: + * - `unique: 'global'` → single-column `(field)`, platform-wide. + * - `unique: true` on a tenant-scoped table → composite `(tenantField, + * field)`: unique *within* the tenant, matching the per-tenant autonumber + * sequence, the RLS read predicate and the write-path tenant stamp. + * - `unique: true` with no tenant column → single-column `(field)`. + * Single-tenant deployments therefore see byte-identical DDL to before. + * + * The tenant column comes FIRST in the composite so the index also serves the + * `WHERE tenant = ?` prefix scans every tenant-scoped read issues. + */ +export function uniqueIndexesFromFields( + table: string, + fields: Record, + tenantField: string | null, +): ExpectedIndex[] { + const out: ExpectedIndex[] = []; + for (const [name, field] of Object.entries(fields ?? {})) { + if (!isUniqueDeclared(field?.unique)) continue; + // A unique declaration ON the tenant column itself ("one row per tenant") + // cannot be tenant-scoped — `(organization_id, organization_id)` is not a + // constraint. Keep it single-column. + const scoped = !isGlobalUnique(field.unique) && tenantField != null && tenantField !== name; + const columns = scoped ? [tenantField, name] : [name]; + out.push({ name: buildIndexName(table, columns, true), columns, unique: true }); + } + return out; +} + +/** Normalize one entry of an object's declared `indexes[]`, or null if unusable. */ +export function normalizeDeclaredIndex( + table: string, + idx: { name?: string; fields?: string[]; unique?: boolean | 'global' } | undefined, +): ExpectedIndex | null { + const columns = Array.isArray(idx?.fields) + ? idx.fields.filter((f): f is string => typeof f === 'string' && f.length > 0) + : []; + if (columns.length === 0) return null; + const unique = isUniqueDeclared(idx?.unique); + const name = + typeof idx?.name === 'string' && idx.name.trim() + ? idx.name.trim() + : buildIndexName(table, columns, unique); + return { name, columns, unique }; +} + +/** + * The full index set metadata asks for on a table: field-level `unique` + * (tenancy-aware) plus the object's declared `indexes[]`, taken verbatim. + * + * Indexes referencing a column that was never materialized (a virtual `formula` + * field, a column an earlier sync skipped) are dropped from the expected set — + * the sync skips creating them, so reporting them as drift would be a finding + * `os migrate apply` could never clear. + */ +export function expectedIndexes(args: { + table: string; + fields: Record; + tenantField: string | null; + declaredIndexes?: Array<{ name?: string; fields?: string[]; unique?: boolean | 'global' }>; + physicalColumns: Set; +}): ExpectedIndex[] { + const { table, fields, tenantField, declaredIndexes, physicalColumns } = args; + const out = uniqueIndexesFromFields(table, fields, tenantField); + for (const idx of Array.isArray(declaredIndexes) ? declaredIndexes : []) { + const norm = normalizeDeclaredIndex(table, idx); + if (norm) out.push(norm); + } + return out.filter((i) => i.columns.every((c) => physicalColumns.has(c))); +} + +/** + * The two names a tenant-scoped field's *legacy* single-column unique index + * could have been materialized under before #3696: + * + * - `
__unique` — knex's default name for `col.unique()`, + * emitted by the old `createColumn`. A real CONSTRAINT on Postgres, a plain + * index on SQLite/MySQL. + * - `uniq_
_` — {@link buildIndexName}, emitted by the old + * SQLite rebuild path. + */ +export function legacyUniqueIndexNames(table: string, column: string): string[] { + return [`${table}_${column}_unique`, buildIndexName(table, [column], true)]; +} + +/** A tenant-scoped unique field, plus the legacy index names that would supersede it. */ +export interface LegacyUniqueReplacement { + column: string; + legacyNames: string[]; + replacement: ExpectedIndex; +} + +/** + * For every field whose `unique` is now tenant-scoped, the legacy global index + * names to look for and the composite that replaces them. `unique: 'global'` + * fields are excluded — their single-column index is the declared intent now, + * not legacy debt. + */ +export function legacyUniqueReplacements(args: { + table: string; + fields: Record; + tenantField: string | null; + physicalColumns: Set; +}): LegacyUniqueReplacement[] { + const { table, fields, tenantField, physicalColumns } = args; + if (!tenantField) return []; // Nothing was ever mis-scoped on a tenant-less table. + // Without a physical tenant column there is no composite to replace the + // legacy index with, and dropping it unreplaced would remove the constraint + // outright rather than relax it. Leave it alone. + if (!physicalColumns.has(tenantField)) return []; + const out: LegacyUniqueReplacement[] = []; + for (const [name, field] of Object.entries(fields ?? {})) { + if (!isUniqueDeclared(field?.unique)) continue; + if (isGlobalUnique(field.unique)) continue; + if (name === tenantField || !physicalColumns.has(name)) continue; + const columns = [tenantField, name]; + out.push({ + column: name, + legacyNames: legacyUniqueIndexNames(table, name), + replacement: { name: buildIndexName(table, columns, true), columns, unique: true }, + }); + } + return out; +} + +/** + * Is this index one ObjectStack generated from metadata? + * + * Orphan detection is deliberately restricted to *our* naming conventions. A + * DBA's hand-rolled covering index is not drift — reporting every undeclared + * index would drown the plan in false positives and invite `os migrate apply + * --allow-destructive` to delete someone's carefully tuned index. Only the + * shapes {@link buildIndexName} and the pre-#3696 `createColumn` could have + * emitted are candidates. + */ +export function isManagedIndexName(table: string, index: PhysicalIndex): boolean { + const { name, columns, unique } = index; + for (const prefix of ['uniq', 'idx']) { + if (name.startsWith(`${prefix}_${table}_`)) return true; + // Hash-suffixed long form: `_
` truncated, then `_`. + if ( + name.length > 9 && + /^_[0-9a-f]{8}$/.test(name.slice(-9)) && + name.slice(0, -9) === `${prefix}_${table}`.slice(0, INDEX_NAME_HEAD) + ) { + return true; + } + } + // knex's default name for the `col.unique()` the pre-#3696 `createColumn` emitted. + return unique && columns.length === 1 && name === `${table}_${columns[0]}_unique`; +} + +/** `(a, b)` UNIQUE vs `(a, b)` — the identity a physical index is compared on. */ +function indexSignature(columns: string[], unique: boolean): string { + return `${unique ? 'UNIQUE ' : ''}(${columns.join(', ')})`; +} + +/** + * Diff a table's expected index set against the physical one. + * + * Presence is matched by NAME, not by column signature, because that is exactly + * what `syncDeclaredIndexes` does when deciding whether to skip a create. Using + * a different rule here would produce findings the reconciler could never + * clear (or, worse, hide ones it will never fix on its own). + */ +export function diffManagedIndexes(args: { + table: string; + expected: ExpectedIndex[]; + legacy: LegacyUniqueReplacement[]; + physical: PhysicalIndex[]; +}): ManagedDriftEntry[] { + const { table, expected, legacy, physical } = args; + const out: ManagedDriftEntry[] = []; + const byName = new Map(physical.map((p) => [p.name, p])); + /** Physical index names accounted for — either declared, or already reported. */ + const explained = new Set(); + + // ── 1. Legacy platform-wide unique superseded by a tenant composite ── + for (const l of legacy) { + // Only a *single-column unique on that very column* is the legacy shape. + // Matching on the name alone would let an unrelated index that happens to + // collide with the legacy spelling be dropped. + const present = l.legacyNames.filter((n) => { + const p = byName.get(n); + return !!p && !p.primary && p.unique && p.columns.length === 1 && p.columns[0] === l.column; + }); + if (present.length === 0) continue; + for (const n of present) explained.add(n); + out.push({ + kind: 'index_mismatch', + remoteName: table, + table, + column: l.column, + expected: indexSignature(l.replacement.columns, true), + actual: indexSignature([l.column], true), + severity: 'warning', + category: 'safe', + op: { + type: 'replace_unique_index', + table, + column: l.column, + dropIndexNames: present, + createIndexName: l.replacement.name, + createColumns: l.replacement.columns, + }, + message: + `${table}.${l.column}: a legacy platform-wide UNIQUE index (${present.join(', ')}) still enforces ` + + `uniqueness across ALL tenants, but metadata scopes it per '${l.replacement.columns[0]}' — a second ` + + `tenant reusing the value is rejected on insert (#3696). Replacing it with ${indexSignature(l.replacement.columns, true)} ` + + `is a pure relaxation: run "os migrate apply".`, + }); + } + + // ── 2. Declared index missing, or present with the wrong definition ── + for (const e of expected) { + explained.add(e.name); + const p = byName.get(e.name); + if (!p) { + out.push({ + kind: 'index_mismatch', + remoteName: table, + table, + column: e.columns[0], + expected: indexSignature(e.columns, e.unique), + actual: '(absent)', + severity: 'warning', + category: 'safe', + op: { + type: 'create_index', + table, + column: e.columns[0], + indexName: e.name, + columns: e.columns, + unique: e.unique, + }, + message: + `${table}: metadata declares index '${e.name}' ${indexSignature(e.columns, e.unique)} but the database ` + + `has no such index — run "os migrate apply" to create it.`, + }); + continue; + } + if (p.unique === e.unique && p.columns.join(',') === e.columns.join(',')) continue; + // Same name, different definition. `syncDeclaredIndexes` skips by name, so + // this never self-heals: it has to be dropped and rebuilt. Tightening to + // UNIQUE is destructive — the CREATE can fail on existing duplicates, and + // by then the old index is already gone. + out.push({ + kind: 'index_mismatch', + remoteName: table, + table, + column: e.columns[0], + expected: indexSignature(e.columns, e.unique), + actual: indexSignature(p.columns, p.unique), + severity: e.unique ? 'error' : 'warning', + category: e.unique ? 'destructive' : 'needs_confirm', + op: { + type: 'recreate_index', + table, + column: e.columns[0], + indexName: e.name, + columns: e.columns, + unique: e.unique, + }, + message: + `${table}: index '${e.name}' is ${indexSignature(p.columns, p.unique)} but metadata declares ` + + `${indexSignature(e.columns, e.unique)} — the additive sync skips it by name, so it must be rebuilt` + + (e.unique + ? `. Creating the UNIQUE index can fail on existing duplicates: "os migrate apply --allow-destructive".` + : ` via "os migrate apply".`), + }); + } + + // ── 3. Orphans: an index WE generated that metadata no longer declares ── + for (const p of physical) { + if (p.primary || p.columns.length === 0 || explained.has(p.name)) continue; + if (!isManagedIndexName(table, p)) continue; + out.push({ + kind: 'unmapped_index', + remoteName: table, + table, + column: p.columns[0], + expected: '(absent)', + actual: indexSignature(p.columns, p.unique), + severity: 'warning', + category: 'destructive', + op: { type: 'drop_index', table, column: p.columns[0], indexName: p.name }, + message: + `${table}: index '${p.name}' ${indexSignature(p.columns, p.unique)} carries ObjectStack's generated naming ` + + `but matches no declared index (orphaned) — "os migrate apply --allow-destructive" to drop it.`, + }); + } + + return out; } diff --git a/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts b/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts new file mode 100644 index 0000000000..b3feec1191 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts @@ -0,0 +1,398 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { SqlDriver, diffManagedIndexes, isManagedIndexName } from '../src/index.js'; + +/** + * Index-dimension managed-schema drift (#3728). + * + * The gap this closes: `detectManagedDrift` was column-only, so the #3696 + * unique-scope migration had nowhere to surface. It compensated by executing + * its DROP + CREATE inline during `initObjects` — in every environment, + * production included — leaving one log line and nothing for `os migrate plan` + * to show. An operator who wanted to eyeball the DDL before it hit the database + * had no way to. + * + * The contract now: + * - `syncTableIndexes` is ADDITIVE ONLY. It never drops or rewrites an index. + * - The legacy retirement is a `replace_unique_index` drift entry: visible in + * `os migrate plan`, applied by `os migrate apply`, and auto-applied at boot + * only under the same dev `autoMigrate: 'safe'` policy that governs every + * other safe drift (#2186). + * - Declared-index drift (missing / redefined / orphaned) is detected too. + */ +describe('SqlDriver index drift (#3728)', () => { + let knexInstance: any; + + const makeDriver = (opts: any = {}) => { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + ...opts, + }); + knexInstance = (d as any).knex; + (d as any).logger = { warn: vi.fn(), info: vi.fn() }; + return d; + }; + + afterEach(async () => { + await knexInstance?.destroy(); + knexInstance = undefined; + }); + + /** The pre-#3696 physical shape: a global unique index on a tenant-scoped field. */ + const seedLegacyGlobalUnique = async (indexName = 'product_code_unique') => { + await knexInstance.schema.createTable('product', (t: any) => { + t.string('id').primary(); + t.string('organization_id'); + t.string('code'); + }); + await knexInstance.raw(`CREATE UNIQUE INDEX ${indexName} ON product (code)`); + await knexInstance('product').insert([ + { id: 'r1', organization_id: 'org_a', code: 'PROD-00001' }, + { id: 'r2', organization_id: 'org_a', code: 'PROD-00002' }, + ]); + }; + + const productMeta = [ + { + name: 'product', + fields: { + organization_id: { type: 'string' }, + code: { type: 'string', unique: true }, + }, + }, + ]; + + const uniqueIndexColumns = async (table: string): Promise> => { + const list: any = await knexInstance.raw(`PRAGMA index_list(${table})`); + const out: Record = {}; + for (const idx of list) { + if (idx.origin === 'pk' || idx.unique !== 1) continue; + const info: any = await knexInstance.raw(`PRAGMA index_info("${idx.name}")`); + out[idx.name] = info.map((c: any) => c.name); + } + return out; + }; + + // ── The issue: the migration is now VISIBLE before it runs ──────────────── + + describe('the legacy unique migration is planned, not silently executed', () => { + it('boot leaves the legacy index in place (autoMigrate off) and reports it as safe drift', async () => { + const driver = makeDriver(); // autoMigrate defaults to 'off' — i.e. production + await seedLegacyGlobalUnique(); + await driver.initObjects(productMeta); + + // The DDL did NOT happen behind the operator's back... + const uniques = await uniqueIndexColumns('product'); + expect(uniques['product_code_unique']).toEqual(['code']); + + // ...and `os migrate plan` can see exactly what it would do. + const drift = await driver.detectManagedDrift(); + const entry = drift.find((d) => d.op.type === 'replace_unique_index'); + expect(entry).toBeDefined(); + expect(entry!.kind).toBe('index_mismatch'); + expect(entry!.category).toBe('safe'); + expect(entry!.table).toBe('product'); + expect(entry!.op).toMatchObject({ + dropIndexNames: ['product_code_unique'], + createColumns: ['organization_id', 'code'], + }); + expect(entry!.message).toMatch(/os migrate apply/); + }); + + it('warns at boot with an actionable hint instead of running the DDL', async () => { + const driver = makeDriver(); + await seedLegacyGlobalUnique(); + await driver.initObjects(productMeta); + + const warned = ((driver as any).logger.warn as ReturnType).mock.calls + .map((c: any[]) => String(c[0])) + .join('\n'); + expect(warned).toMatch(/os migrate/); + expect(warned).toMatch(/product\.code/); + }); + + it('detects the `uniq_
_` spelling the old rebuild path emitted', async () => { + const driver = makeDriver(); + await seedLegacyGlobalUnique('uniq_product_code'); + await driver.initObjects(productMeta); + + const drift = await driver.detectManagedDrift(); + const entry = drift.find((d) => d.op.type === 'replace_unique_index'); + expect((entry!.op as any).dropIndexNames).toEqual(['uniq_product_code']); + }); + + it("does NOT report a unique: 'global' field's single-column index", async () => { + const driver = makeDriver(); + await knexInstance.schema.createTable('runtime', (t: any) => { + t.string('id').primary(); + t.string('organization_id'); + t.string('hostname'); + }); + await knexInstance.raw('CREATE UNIQUE INDEX uniq_runtime_hostname ON runtime (hostname)'); + + await driver.initObjects([ + { + name: 'runtime', + fields: { + organization_id: { type: 'string' }, + hostname: { type: 'string', unique: 'global' }, + }, + }, + ]); + + expect(await driver.detectManagedDrift()).toHaveLength(0); + }); + + it('reports nothing once the composite is the only unique index', async () => { + const driver = makeDriver(); + await driver.initObjects(productMeta); // fresh table, built to spec + expect(await driver.detectManagedDrift()).toHaveLength(0); + }); + }); + + // ── Applying it through `os migrate apply` ──────────────────────────────── + + describe('applyMigrationEntries', () => { + it('replaces the legacy global unique WITHOUT --allow-destructive, preserving data', async () => { + const driver = makeDriver(); + await seedLegacyGlobalUnique(); + await driver.initObjects(productMeta); + + const drift = await driver.detectManagedDrift(); + const { applied, skipped } = await driver.applyMigrationEntries(drift, { allowDestructive: false }); + expect(applied.some((d) => d.op.type === 'replace_unique_index')).toBe(true); + expect(skipped).toHaveLength(0); + + const uniques = await uniqueIndexColumns('product'); + expect(uniques['product_code_unique']).toBeUndefined(); + expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); + + // Existing rows survived, and the cross-tenant insert the issue is about works. + expect(await driver.count('product', {} as any)).toBe(2); + const b = await driver.create('product', { organization_id: 'org_b', code: 'PROD-00001' }); + expect(b.code).toBe('PROD-00001'); + + // Converged: re-running finds nothing. + expect(await driver.detectManagedDrift()).toHaveLength(0); + }); + + it('is idempotent — applying twice is a no-op the second time', async () => { + const driver = makeDriver(); + await seedLegacyGlobalUnique(); + await driver.initObjects(productMeta); + + const drift = await driver.detectManagedDrift(); + await driver.applyMigrationEntries(drift, { allowDestructive: false }); + const again = await driver.applyMigrationEntries(drift, { allowDestructive: false }); + expect(again.skipped).toHaveLength(0); + expect(Object.values(await uniqueIndexColumns('product'))).toContainEqual([ + 'organization_id', + 'code', + ]); + }); + }); + + // ── Boot-time self-heal still exists, under the #2186 policy ────────────── + + describe("dev auto-reconcile (autoMigrate: 'safe')", () => { + it('self-heals the legacy unique on restart, so the cross-tenant insert succeeds', async () => { + const driver = makeDriver({ autoMigrate: 'safe' }); + await seedLegacyGlobalUnique(); + + await driver.initObjects(productMeta); // simulates restart after pull+rebuild + + const uniques = await uniqueIndexColumns('product'); + expect(uniques['product_code_unique']).toBeUndefined(); + expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); + expect(await driver.detectManagedDrift()).toHaveLength(0); + + const b = await driver.create('product', { organization_id: 'org_b', code: 'PROD-00001' }); + expect(b.code).toBe('PROD-00001'); + }); + + it('is force-disabled under NODE_ENV=production — the index survives untouched', async () => { + const prev = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + try { + const driver = makeDriver({ autoMigrate: 'safe' }); + await seedLegacyGlobalUnique(); + await driver.initObjects(productMeta); + + expect((await uniqueIndexColumns('product'))['product_code_unique']).toEqual(['code']); + expect( + (await driver.detectManagedDrift()).some((d) => d.op.type === 'replace_unique_index'), + ).toBe(true); + } finally { + process.env.NODE_ENV = prev; + } + }); + }); + + // ── Declared `indexes[]` drift (the issue's "附带发现") ──────────────────── + + describe('declared index drift', () => { + it('flags a declared index the database is missing (safe / create_index)', async () => { + const driver = makeDriver(); + await knexInstance.schema.createTable('slug_reservation', (t: any) => { + t.string('id').primary(); + t.string('slug'); + }); + // Register the metadata without letting the additive sync materialize it, + // so we observe what an operator sees when a create was skipped or failed. + (driver as any).managedObjectFields.set('slug_reservation', { slug: { type: 'string' } }); + (driver as any).managedObjectIndexes.set('slug_reservation', [ + { fields: ['slug'], unique: true }, + ]); + + const drift = await driver.detectManagedDrift(); + const entry = drift.find((d) => d.op.type === 'create_index'); + expect(entry).toBeDefined(); + expect(entry!.category).toBe('safe'); + expect((entry!.op as any).indexName).toBe('uniq_slug_reservation_slug'); + + await driver.applyMigrationEntries(drift, { allowDestructive: false }); + expect(Object.values(await uniqueIndexColumns('slug_reservation'))).toContainEqual(['slug']); + }); + + it('flags an index whose definition no longer matches the declaration (recreate_index)', async () => { + const driver = makeDriver(); + await driver.initObjects([ + { + name: 'domain', + fields: { organization_id: { type: 'string' }, host: { type: 'string' } }, + indexes: [{ name: 'domain_host_idx', fields: ['host'] }], + } as any, + ]); + + // Metadata now wants (organization_id, host) under the SAME name — the + // additive sync skips by name, so this can never self-heal. + const drift = await driver.detectManagedDrift([ + { + name: 'domain', + fields: { organization_id: { type: 'string' }, host: { type: 'string' } }, + indexes: [{ name: 'domain_host_idx', fields: ['organization_id', 'host'] }], + } as any, + ]); + const entry = drift.find((d) => d.op.type === 'recreate_index'); + expect(entry).toBeDefined(); + expect(entry!.category).toBe('needs_confirm'); // non-unique → cannot fail + expect(entry!.actual).toBe('(host)'); + expect(entry!.expected).toBe('(organization_id, host)'); + + await driver.applyMigrationEntries(drift, { allowDestructive: false }); + const list: any = await knexInstance.raw(`PRAGMA index_info("domain_host_idx")`); + expect(list.map((c: any) => c.name)).toEqual(['organization_id', 'host']); + }); + + it('rates a redefinition that TIGHTENS to UNIQUE as destructive', async () => { + const driver = makeDriver(); + await driver.initObjects([ + { + name: 'domain', + fields: { host: { type: 'string' } }, + indexes: [{ name: 'domain_host_idx', fields: ['host'] }], + } as any, + ]); + + const drift = await driver.detectManagedDrift([ + { + name: 'domain', + fields: { host: { type: 'string' } }, + indexes: [{ name: 'domain_host_idx', fields: ['host'], unique: true }], + } as any, + ]); + const entry = drift.find((d) => d.op.type === 'recreate_index'); + expect(entry!.category).toBe('destructive'); + + // Skipped without the flag — the CREATE can fail on existing duplicates. + const r = await driver.applyMigrationEntries(drift, { allowDestructive: false }); + expect(r.skipped.some((d) => d.op.type === 'recreate_index')).toBe(true); + }); + + it('flags an orphaned index we generated as destructive, and drops it with the flag', async () => { + const driver = makeDriver(); + await driver.initObjects([ + { + name: 'domain', + fields: { organization_id: { type: 'string' }, host: { type: 'string' } }, + indexes: [{ fields: ['host'] }], + } as any, + ]); + expect(await (driver as any).getExistingIndexNames('domain')).toContain('idx_domain_host'); + + // The declaration is gone from metadata; the index is not. + const objects = [ + { name: 'domain', fields: { organization_id: { type: 'string' }, host: { type: 'string' } } }, + ]; + const drift = await driver.detectManagedDrift(objects); + const entry = drift.find((d) => d.op.type === 'drop_index'); + expect(entry).toBeDefined(); + expect(entry!.kind).toBe('unmapped_index'); + expect(entry!.category).toBe('destructive'); + + expect( + (await driver.applyMigrationEntries(drift, { allowDestructive: false })).skipped, + ).toHaveLength(1); + await driver.applyMigrationEntries(drift, { allowDestructive: true }); + expect(await (driver as any).getExistingIndexNames('domain')).not.toContain('idx_domain_host'); + }); + + it("leaves a DBA's hand-rolled index alone — only our naming is orphan-eligible", async () => { + const driver = makeDriver(); + await driver.initObjects([{ name: 'domain', fields: { host: { type: 'string' } } }]); + await knexInstance.raw('CREATE INDEX host_lookup_covering ON domain (host)'); + + expect(await driver.detectManagedDrift()).toHaveLength(0); + }); + }); + + // ── Pure differ units ───────────────────────────────────────────────────── + + describe('diffManagedIndexes / isManagedIndexName', () => { + it('ignores a legacy NAME whose definition is not the legacy shape', async () => { + // Same name, but it indexes a different column — dropping it would be a + // pure mistake, so name matching alone must not be enough. + const entries = diffManagedIndexes({ + table: 'product', + expected: [{ name: 'uniq_product_organization_id_code', columns: ['organization_id', 'code'], unique: true }], + legacy: [ + { + column: 'code', + legacyNames: ['product_code_unique'], + replacement: { name: 'uniq_product_organization_id_code', columns: ['organization_id', 'code'], unique: true }, + }, + ], + physical: [ + { name: 'product_code_unique', columns: ['sku'], unique: true }, + { name: 'uniq_product_organization_id_code', columns: ['organization_id', 'code'], unique: true }, + ], + }); + expect(entries.filter((e) => e.op.type === 'replace_unique_index')).toHaveLength(0); + }); + + it('never treats the primary key index as drift', () => { + const entries = diffManagedIndexes({ + table: 'product', + expected: [], + legacy: [], + physical: [{ name: 'uniq_product_id', columns: ['id'], unique: true, primary: true }], + }); + expect(entries).toHaveLength(0); + }); + + it('recognises the hash-suffixed long form of a generated name', () => { + const table = 'a_very_long_table_name_that_forces_the_hash_suffix_path_here'; + expect( + isManagedIndexName(table, { + name: `uniq_${table}`.slice(0, 51) + '_deadbeef', + columns: ['x'], + unique: true, + }), + ).toBe(true); + expect(isManagedIndexName(table, { name: 'ops_custom_idx', columns: ['x'], unique: false })).toBe(false); + }); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts b/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts index e4a4a07cbc..1d423822fa 100644 --- a/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts @@ -19,16 +19,25 @@ import { SqlDriver } from '../src/index.js'; * - `unique: true`, no tenant column → single-column (single-tenant: unchanged) * - `unique: 'global'` → single-column, always * - declared `indexes[]` → verbatim columns, never rewritten + * + * Retiring the legacy global index is no longer inline DDL at boot: since + * #3728 it is a `replace_unique_index` drift entry, auto-applied on restart + * only under the dev `autoMigrate: 'safe'` policy and otherwise left to + * `os migrate`. The migration tests below therefore opt into that policy. */ describe('SqlDriver unique × tenancy (#3696)', () => { let driver: SqlDriver; - beforeEach(async () => { - driver = new SqlDriver({ + const makeDriver = (opts: any = {}) => + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + ...opts, }); + + beforeEach(async () => { + driver = makeDriver(); }); afterEach(async () => { @@ -254,6 +263,7 @@ describe('SqlDriver unique × tenancy (#3696)', () => { // ── Legacy migration: drop the global index, create the composite ───────── it('retires a legacy global unique index and replaces it with the composite', async () => { + driver = makeDriver({ autoMigrate: 'safe' }); const k = (driver as any).knex; // Reproduce exactly what the OLD code left behind: knex's `col.unique()` @@ -295,6 +305,7 @@ describe('SqlDriver unique × tenancy (#3696)', () => { }); it('retires the legacy `uniq_
_` index left by the drift rebuild path', async () => { + driver = makeDriver({ autoMigrate: 'safe' }); const k = (driver as any).knex; await k.schema.createTable('product', (t: any) => { t.string('id').primary(); @@ -319,6 +330,7 @@ describe('SqlDriver unique × tenancy (#3696)', () => { }); it("does NOT retire the single-column index of a unique: 'global' field", async () => { + driver = makeDriver({ autoMigrate: 'safe' }); const k = (driver as any).knex; await k.schema.createTable('runtime', (t: any) => { t.string('id').primary(); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 687b5554dc..f0c6674b14 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -15,10 +15,17 @@ import { StorageNameMapping } from '@objectstack/spec/system'; import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared'; import { resolveMultiOrgEnabled } from '@objectstack/types'; import { + buildIndexName, + diffManagedIndexes, diffManagedTable, driftKey, + expectedIndexes, + isIndexDriftOp, + legacyUniqueReplacements, + uniqueIndexesFromFields, type ManagedDriftEntry, type DriftOp, + type PhysicalIndex, type SqlDialectName, type PhysicalColumn, } from './schema-drift.js'; @@ -2138,8 +2145,13 @@ export class SqlDriver implements IDataDriver { // #2186: remember the authoritative metadata field set for this table so // drift detection / `os migrate` can diff the physical schema against it. this.managedObjectFields.set(tableName, obj.fields ?? {}); + // Always overwrite — a metadata change that REMOVES `indexes` must clear + // the previous entry, or drift detection keeps expecting an index nobody + // declares any more (and never reports it as orphaned). if (Array.isArray((obj as any).indexes)) { this.managedObjectIndexes.set(tableName, (obj as any).indexes); + } else { + this.managedObjectIndexes.delete(tableName); } const jsonCols: string[] = []; @@ -2282,12 +2294,14 @@ export class SqlDriver implements IDataDriver { ); } - // #2186: the additive sync above only ever ADDs tables/columns. For a - // table that already existed, detect (and in dev, auto-reconcile) any - // non-additive divergence (relaxed NOT NULL, widened varchar, orphaned - // column) between metadata and the physical schema. + // #2186: the additive sync above only ever ADDs tables/columns/indexes. + // For a table that already existed, detect (and in dev, auto-reconcile) + // any non-additive divergence between metadata and the physical schema — + // relaxed NOT NULL, widened varchar, orphaned column, and since #3728 the + // index dimension too (a legacy global unique that is now tenant-scoped, + // a redefined or orphaned index). if (exists) { - await this.reconcileAndWarnDrift(tableName, obj.fields ?? {}); + await this.reconcileAndWarnDrift(tableName, obj.fields ?? {}, declaredIndexes); } } @@ -2326,10 +2340,18 @@ export class SqlDriver implements IDataDriver { } } - /** Diff one table's metadata fields against its physical columns. */ + /** + * Diff one table's metadata (fields + indexes) against its physical schema. + * + * `declaredIndexes` is authoritative and complete: `undefined` means the + * object declares none, NOT "look it up". Every caller already holds the + * object it is diffing, and falling back to the last-synced set would make a + * removed `indexes[]` undetectable as an orphan. + */ protected async detectTableDrift( tableName: string, fields: Record, + declaredIndexes?: any[], ): Promise { const cols = await this.introspectColumns(tableName); const physical: PhysicalColumn[] = cols.map((c) => ({ @@ -2338,7 +2360,32 @@ export class SqlDriver implements IDataDriver { nullable: c.nullable, maxLength: c.maxLength, })); - return diffManagedTable({ table: tableName, fields, columns: physical, dialect: this.dialectName }); + const out = diffManagedTable({ table: tableName, fields, columns: physical, dialect: this.dialectName }); + out.push(...(await this.detectTableIndexDrift(tableName, fields, declaredIndexes, new Set(cols.map((c) => c.name))))); + return out; + } + + /** + * Diff one table's expected index set against the physical one (#3728). + * + * Kept separate from {@link diffManagedTable} because it needs a second + * introspection round-trip (indexes, not columns) and two inputs the column + * differ has no use for: the object's declared `indexes[]` and its tenant + * column. + */ + protected async detectTableIndexDrift( + tableName: string, + fields: Record, + declaredIndexes: any[] | undefined, + physicalColumns: Set, + ): Promise { + const tenantField = this.resolveTenantField(tableName); + return diffManagedIndexes({ + table: tableName, + expected: expectedIndexes({ table: tableName, fields, tenantField, declaredIndexes, physicalColumns }), + legacy: legacyUniqueReplacements({ table: tableName, fields, tenantField, physicalColumns }), + physical: await this.introspectIndexes(tableName), + }); } /** @@ -2346,23 +2393,35 @@ export class SqlDriver implements IDataDriver { * database. Metadata is the source of truth. Returns one entry per drift, * sorted by table then column. Used by `os migrate` (P3) and tests. * - * @param objects optional explicit object list; defaults to whatever - * `initObjects` last synced (captured in {@link managedObjectFields}). + * Covers both the column dimension ({@link diffManagedTable}) and, since + * #3728, the index dimension ({@link detectTableIndexDrift}). + * + * @param objects optional explicit object list — `fields` and `indexes` are + * then authoritative for those tables. Defaults to whatever `initObjects` + * last synced (captured in {@link managedObjectFields} / + * {@link managedObjectIndexes}). */ async detectManagedDrift( - objects?: Array<{ name: string; fields?: Record }>, + objects?: Array<{ name: string; fields?: Record; indexes?: any[] }>, ): Promise { - const tables = new Map>(); + const tables = new Map; indexes?: any[] }>(); if (objects) { - for (const o of objects) tables.set(StorageNameMapping.resolveTableName(o), o.fields ?? {}); + for (const o of objects) { + tables.set(StorageNameMapping.resolveTableName(o), { + fields: o.fields ?? {}, + indexes: (o as any).indexes, + }); + } } else { - for (const [t, f] of this.managedObjectFields) tables.set(t, f); + for (const [t, f] of this.managedObjectFields) { + tables.set(t, { fields: f, indexes: this.managedObjectIndexes.get(t) }); + } } const out: ManagedDriftEntry[] = []; - for (const [tableName, fields] of tables) { + for (const [tableName, meta] of tables) { if (!(await this.knex.schema.hasTable(tableName))) continue; - out.push(...(await this.detectTableDrift(tableName, fields))); + out.push(...(await this.detectTableDrift(tableName, meta.fields, meta.indexes))); } out.sort((a, b) => (a.table === b.table ? (a.column ?? '').localeCompare(b.column ?? '') : a.table.localeCompare(b.table))); return out; @@ -2373,10 +2432,14 @@ export class SqlDriver implements IDataDriver { * auto-reconcile the *safe* (loosening) subset when `autoMigrate==='safe'`, * then WARN once per remaining divergence with an actionable hint. */ - protected async reconcileAndWarnDrift(tableName: string, fields: Record): Promise { + protected async reconcileAndWarnDrift( + tableName: string, + fields: Record, + declaredIndexes?: any[], + ): Promise { let drift: ManagedDriftEntry[]; try { - drift = await this.detectTableDrift(tableName, fields); + drift = await this.detectTableDrift(tableName, fields, declaredIndexes); } catch (e: any) { this.logger.warn(`[schema-drift] could not introspect '${tableName}' for drift detection`, e?.message ?? e); return; @@ -2394,10 +2457,10 @@ export class SqlDriver implements IDataDriver { try { const { applied } = await this.applyMigrationEntries(safe, { allowDestructive: false }); for (const d of applied) { - (this.logger.info ?? this.logger.warn)(`[schema-drift] auto-reconciled ${d.op.type} on ${d.table}.${d.column}`); + (this.logger.info ?? this.logger.warn)(`[schema-drift] auto-reconciled ${d.op.type} on ${d.table}.${d.column ?? ''}`); } // Re-detect so the warnings below reflect the post-reconcile state. - drift = await this.detectTableDrift(tableName, fields); + drift = await this.detectTableDrift(tableName, fields, declaredIndexes); } catch (e: any) { this.logger.warn(`[schema-drift] dev auto-reconcile failed for '${tableName}' — falling back to warning`, e?.message ?? e); } @@ -2440,9 +2503,17 @@ export class SqlDriver implements IDataDriver { }); if (candidates.length === 0) return { applied, skipped }; + // Index ops (#3728) are portable DDL on every dialect — no ALTER COLUMN, no + // SQLite table rebuild — so they take their own path. Column ops run FIRST: + // a `drop_column` takes its indexes with it, and on SQLite the rebuild + // re-materializes the whole index set from metadata, which may already + // satisfy the index entries below (they are idempotent, so that is fine). + const columnEntries = candidates.filter((d) => !isIndexDriftOp(d.op)); + const indexEntries = candidates.filter((d) => isIndexDriftOp(d.op)); + // Group by table — SQLite reconciles a whole table in one rebuild. const byTable = new Map(); - for (const d of candidates) { + for (const d of columnEntries) { (byTable.get(d.table) ?? byTable.set(d.table, []).get(d.table)!).push(d); } @@ -2462,11 +2533,71 @@ export class SqlDriver implements IDataDriver { for (const d of ents) if (!applied.includes(d)) skipped.push(d); } } + + for (const d of indexEntries) { + try { + const ok = await this.applyIndexDriftOp(d.op); + (ok ? applied : skipped).push(d); + } catch (e: any) { + this.logger.warn(`[schema-drift] failed to reconcile index on '${d.table}'`, e?.message ?? e); + skipped.push(d); + } + } return { applied, skipped }; } + /** + * Apply one index drift op (#3728). Portable across dialects: index DDL needs + * neither `ALTER COLUMN` nor the SQLite table rebuild that column ops do. + */ + protected async applyIndexDriftOp(op: DriftOp): Promise { + const physicalColumns = new Set(Object.keys(await this.knex(op.table).columnInfo())); + const ensure = (name: string, columns: string[], unique: boolean) => + this.syncDeclaredIndexes(op.table, [{ name, fields: columns, unique }], physicalColumns); + + switch (op.type) { + case 'replace_unique_index': { + // CREATE before DROP: the composite and the legacy index have different + // names, so uniqueness is never unenforced in between. If the create + // fails we have not dropped anything yet and the schema is untouched. + await ensure(op.createIndexName, op.createColumns, true); + // …and only drop once the replacement is confirmed present. This is a + // relaxation, not a removal: if `syncDeclaredIndexes` skipped the create + // (a column it references is not materialized), dropping the legacy + // index would leave the field with NO uniqueness at all. + if (!(await this.getExistingIndexNames(op.table)).has(op.createIndexName)) { + this.logger.warn( + `[schema-drift] keeping legacy unique index(es) ${op.dropIndexNames.join(', ')} on '${op.table}' — ` + + `the replacement '${op.createIndexName}' could not be created.`, + ); + return false; + } + for (const name of op.dropIndexNames) { + await this.dropIndexIfExists(op.table, name); + } + return true; + } + case 'create_index': + await ensure(op.indexName, op.columns, op.unique); + return true; + case 'drop_index': + return await this.dropIndexIfExists(op.table, op.indexName); + case 'recreate_index': + // Same name on both sides — the drop has to come first, and a UNIQUE + // target can fail on existing duplicates. That is why this op is + // categorised destructive when unique (see `diffManagedIndexes`). + await this.dropIndexIfExists(op.table, op.indexName); + await ensure(op.indexName, op.columns, op.unique); + return true; + default: + return false; + } + } + /** Apply a single drift op in place (Postgres / MySQL). Returns false if unsupported. */ protected async applyDriftOpInPlace(op: DriftOp): Promise { + // Index ops need no dialect-specific ALTER — route them to the portable path. + if (isIndexDriftOp(op)) return this.applyIndexDriftOp(op); const { table, column } = op; if (this.isPostgres) { switch (op.type) { @@ -2609,122 +2740,123 @@ export class SqlDriver implements IDataDriver { /** * Build a deterministic index name for a declared index so repeated * `initObjects` runs converge on the same identifier (and can detect an - * already-materialized index by name). Long names are hash-suffixed to - * stay within the 63/64-char identifier limits of Postgres/MySQL. + * already-materialized index by name). + * + * Delegates to the shared {@link buildIndexName} so the names the driver + * *creates* and the names the drift differ *looks for* can never diverge. */ protected buildIndexName(tableName: string, fields: string[], unique: boolean): string { - const prefix = unique ? 'uniq' : 'idx'; - const base = `${prefix}_${tableName}_${fields.join('_')}`; - const MAX = 60; - if (base.length <= MAX) return base; - const hash = createHash('sha1').update(base).digest('hex').slice(0, 8); - return `${`${prefix}_${tableName}`.slice(0, MAX - 9)}_${hash}`; + return buildIndexName(tableName, fields, unique); } /** - * Read the names of indexes that already exist on a table, per dialect. - * Used to make declared-index sync idempotent across repeated runs. - * Failures are swallowed — at worst we attempt a create and absorb the - * "already exists" error in `syncDeclaredIndexes`. + * Read the indexes that physically exist on a table — name, ordered columns, + * uniqueness, and whether it backs the PRIMARY KEY — per dialect. + * + * Used both for sync idempotency ({@link getExistingIndexNames}) and for index + * drift detection (#3728), which needs the full definition rather than just + * the name. Failures are swallowed: at worst we attempt a create and absorb + * the "already exists" error in {@link syncDeclaredIndexes}. + * + * Postgres reads `pg_index` rather than `pg_indexes` so indexes backing a + * UNIQUE CONSTRAINT (which is exactly what knex's old `col.unique()` produced) + * are returned too — the drift detector cannot see the #3696 legacy shape + * otherwise. */ - protected async getExistingIndexNames(tableName: string): Promise> { - const names = new Set(); + protected async introspectIndexes(tableName: string): Promise { + const byName = new Map(); + const upsert = (name: string, unique: boolean, primary: boolean): PhysicalIndex => { + let e = byName.get(name); + if (!e) byName.set(name, (e = { name, columns: [], unique, primary })); + return e; + }; try { if (this.isSqlite) { const safe = tableName.replace(/[^a-zA-Z0-9_]/g, ''); - const rows: any = await this.knex.raw(`PRAGMA index_list(${safe})`); - for (const r of rows) names.add(r.name); + const list: any = await this.knex.raw(`PRAGMA index_list(${safe})`); + for (const r of list) { + const entry = upsert(r.name, r.unique === 1 || r.unique === true, r.origin === 'pk'); + const info: any = await this.knex.raw(`PRAGMA index_info(${JSON.stringify(r.name)})`); + // An expression index reports a null column name — skip those parts; + // a partial column list only ever makes the differ *less* eager. + for (const c of info) if (c.name != null) entry.columns.push(c.name); + } } else if (this.isPostgres) { const res: any = await this.knex.raw( - `SELECT indexname FROM pg_indexes WHERE schemaname = 'public' AND tablename = ?`, + `SELECT i.relname AS index_name, ix.indisunique AS is_unique, ix.indisprimary AS is_primary, + a.attname AS column_name + FROM pg_class t + JOIN pg_namespace n ON n.oid = t.relnamespace + JOIN pg_index ix ON t.oid = ix.indrelid + JOIN pg_class i ON i.oid = ix.indexrelid + JOIN LATERAL unnest(ix.indkey) WITH ORDINALITY AS k(attnum, ord) ON true + JOIN pg_attribute a ON a.attrelid = t.oid AND a.attnum = k.attnum + WHERE t.relname = ? AND n.nspname = 'public' + ORDER BY i.relname, k.ord`, [tableName], ); - for (const r of res.rows) names.add(r.indexname); + for (const r of res.rows) { + upsert(r.index_name, r.is_unique === true, r.is_primary === true).columns.push(r.column_name); + } } else if (this.isMysql) { const res: any = await this.knex.raw( - `SELECT INDEX_NAME FROM information_schema.STATISTICS WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ?`, + `SELECT INDEX_NAME, NON_UNIQUE, COLUMN_NAME + FROM information_schema.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ? + ORDER BY INDEX_NAME, SEQ_IN_INDEX`, [tableName], ); - for (const r of res[0]) names.add(r.INDEX_NAME); + for (const r of res[0]) { + upsert( + r.INDEX_NAME, + Number(r.NON_UNIQUE) === 0, + r.INDEX_NAME === 'PRIMARY', + ).columns.push(r.COLUMN_NAME); + } } } catch { // Best-effort — fall through and let creation handle conflicts. } - return names; + return [...byName.values()]; + } + + /** + * Names of the indexes that already exist on a table. Used to make + * declared-index sync idempotent across repeated runs. + */ + protected async getExistingIndexNames(tableName: string): Promise> { + return new Set((await this.introspectIndexes(tableName)).map((i) => i.name)); } /** * Translate field-level `unique` declarations into concrete index - * descriptors, applying tenant scoping (#3696). - * - * This is the ONLY place field-level uniqueness becomes physical, so the - * create-table, alter-table and SQLite-rebuild paths cannot drift apart the - * way they had (each used to inline its own single-column DDL). - * - * Scoping rule: - * - `unique: 'global'` → single-column `(field)`, platform-wide. - * - `unique: true` on a tenant-scoped table → composite `(tenantField, - * field)`: unique *within* the tenant, matching the per-tenant autonumber - * sequence, the RLS read predicate and the write-path tenant stamp. - * - `unique: true` with no tenant column → single-column `(field)`. Single- - * tenant deployments therefore see byte-identical DDL to before. - * - * The tenant column comes FIRST in the composite so the index also serves the - * `WHERE tenant = ?` prefix scans every tenant-scoped read issues. + * descriptors, applying tenant scoping (#3696). Delegates to the shared + * {@link uniqueIndexesFromFields} — see there for the scoping rule. */ protected uniqueIndexesFromFields( tableName: string, fields: Record, tenantField: string | null, ): Array<{ name: string; fields: string[]; unique: true }> { - const out: Array<{ name: string; fields: string[]; unique: true }> = []; - for (const [name, field] of Object.entries(fields ?? {})) { - if (!isUniqueDeclared(field?.unique)) continue; - // A unique declaration ON the tenant column itself ("one row per tenant") - // cannot be tenant-scoped — `(organization_id, organization_id)` is not a - // constraint. Keep it single-column. - const scoped = - !isGlobalUnique(field.unique) && tenantField != null && tenantField !== name; - const cols = scoped ? [tenantField, name] : [name]; - out.push({ name: this.buildIndexName(tableName, cols, true), fields: cols, unique: true }); - } - return out; - } - - /** - * Legacy single-column unique indexes that a tenant-scoped field's constraint - * used to be materialized as, and which must be dropped once the composite - * replaces them (#3696). Two spellings ever existed: - * - * - `
__unique` — knex's default name for `col.unique()`, - * emitted by the old `createColumn`. A real CONSTRAINT on Postgres, a - * plain index on SQLite/MySQL. - * - `uniq_
_` — {@link buildIndexName}, emitted by the old - * SQLite rebuild path. - * - * Dropping them is a pure RELAXATION: the old global constraint is strictly - * stronger than the new per-tenant one, so existing data satisfies the - * replacement by construction. No dedup, no cleanup, no possible failure — - * which is why this converges inline at sync time instead of waiting for a - * deliberate `os migrate` run (a deployment that never ran migrate would - * otherwise stay broken, and the failure mode is cross-tenant data loss on - * insert). - */ - protected legacySingleColumnUniqueNames(tableName: string, column: string): string[] { - return [`${tableName}_${column}_unique`, this.buildIndexName(tableName, [column], true)]; + return uniqueIndexesFromFields(tableName, fields, tenantField).map((i) => ({ + name: i.name, + fields: i.columns, + unique: true as const, + })); } /** - * Drop a unique index/constraint by name if present, across dialects. + * Drop an index (or the constraint backing it) by name if present, across + * dialects. * * Postgres materializes knex's `col.unique()` as a table CONSTRAINT (not a * bare index), so `DROP INDEX` alone silently leaves it in place — the exact - * failure that would have made this migration a no-op on the deployments that - * matter most. Try the constraint form first, then the index form. + * failure that would have made the #3696 migration a no-op on the deployments + * that matter most. Try the constraint form first, then the index form. * * Returns true when something was actually dropped. */ - protected async dropUniqueIndexIfExists(tableName: string, indexName: string): Promise { + protected async dropIndexIfExists(tableName: string, indexName: string): Promise { const attempts: string[] = this.isPostgres ? [ `ALTER TABLE ?? DROP CONSTRAINT IF EXISTS ??`, @@ -2750,55 +2882,22 @@ export class SqlDriver implements IDataDriver { return dropped; } - /** - * Retire the legacy global unique indexes for every field whose constraint is - * now tenant-scoped (#3696). Runs before the composites are created so a - * Postgres constraint and its replacement never contend for a name. - */ - protected async dropLegacyGlobalUniques( - tableName: string, - fields: Record, - tenantField: string | null, - physicalColumns: Set, - ): Promise { - if (!tenantField) return; // Nothing was ever mis-scoped on a tenant-less table. - const existing = await this.getExistingIndexNames(tableName); - for (const [name, field] of Object.entries(fields ?? {})) { - if (!isUniqueDeclared(field?.unique)) continue; - // `'global'` keeps its single-column index — that is now the declared - // intent, not legacy debt. - if (isGlobalUnique(field.unique)) continue; - if (name === tenantField || !physicalColumns.has(name)) continue; - - for (const legacy of this.legacySingleColumnUniqueNames(tableName, name)) { - // On Postgres a unique CONSTRAINT may not surface in `pg_indexes` under - // a name we can match, so attempt the drop when either the name is - // known to exist or the dialect hides constraints from us. - if (!existing.has(legacy) && !this.isPostgres) continue; - try { - const dropped = await this.dropUniqueIndexIfExists(tableName, legacy); - if (dropped) { - (this.logger.info ?? this.logger.warn)( - `[sql-driver] dropped legacy global unique index '${legacy}' on '${tableName}' — ` + - `${tableName}.${name} is now unique per '${tenantField}' (#3696). ` + - `Declare \`unique: 'global'\` if platform-wide uniqueness was intended.`, - ); - } - } catch (e: any) { - this.logger.warn( - `[sql-driver] could not drop legacy unique index '${legacy}' on '${tableName}'`, - e?.message ?? e, - ); - } - } - } - } - /** * Materialize a table's full index set: field-level `unique` (tenancy-aware, * see {@link uniqueIndexesFromFields}) plus the object's declared `indexes`. * Single entry point so every caller — create, alter, SQLite rebuild — lands * on identical DDL. + * + * ADDITIVE ONLY (#3728). This creates indexes; it never drops or rewrites + * one. Retiring the legacy platform-wide unique index a tenant-scoped field + * used to carry (#3696) used to happen right here, unconditionally, at every + * boot — a DROP that `os migrate plan` could not show and an operator could + * not pre-inspect, and which altered a managed schema in production in + * violation of the #2186 contract. That DROP is now a + * `replace_unique_index` drift entry: detected by + * {@link detectManagedDrift}, rendered by `os migrate plan`, applied by + * `os migrate apply` — and still auto-reconciled at boot in dev, via the same + * `autoMigrate: 'safe'` policy that governs every other safe drift. */ protected async syncTableIndexes( tableName: string, @@ -2807,7 +2906,6 @@ export class SqlDriver implements IDataDriver { tenantField: string | null, physicalColumns: Set, ): Promise { - await this.dropLegacyGlobalUniques(tableName, fields, tenantField, physicalColumns); const fromFields = this.uniqueIndexesFromFields(tableName, fields, tenantField); const declared = Array.isArray(declaredIndexes) ? declaredIndexes : []; if (fromFields.length === 0 && declared.length === 0) return; diff --git a/packages/spec/src/shared/external-errors.ts b/packages/spec/src/shared/external-errors.ts index 9f9174de1e..578c850305 100644 --- a/packages/spec/src/shared/external-errors.ts +++ b/packages/spec/src/shared/external-errors.ts @@ -37,7 +37,12 @@ export type SchemaDiffEntryKind = | 'type_mismatch' | 'nullability_mismatch' | 'unmapped_column' - | 'pk_mismatch'; + | 'pk_mismatch' + /** A declared index is absent, or the index under that name has a different + * definition than metadata declares (#3728). */ + | 'index_mismatch' + /** A physical index ObjectStack generated that metadata no longer declares. */ + | 'unmapped_index'; /** * A single divergence entry. Produced by the validation gate (ADR §5.2) diff --git a/skills/objectstack-data/SKILL.md b/skills/objectstack-data/SKILL.md index fb7165221c..f5b4f8c287 100644 --- a/skills/objectstack-data/SKILL.md +++ b/skills/objectstack-data/SKILL.md @@ -292,7 +292,9 @@ schema, and the **database column wins at write time** (#2186): |--------|------------------------| | add object / field / index | ✅ applied automatically (additive) | | `required: true → false` (relax `NOT NULL`) | dev auto-heals (`autoMigrate:'safe'`); otherwise `os migrate apply` | +| `unique` re-scoped global → per-tenant (#3696) | dev auto-heals; otherwise `os migrate apply` (`replace_unique_index`) | | type / length change, drop field, rename | `os migrate apply` (`--allow-destructive` for drops / tightenings) | +| declared index removed, or its columns changed | `os migrate apply` (`--allow-destructive` when it drops, or rebuilds as `UNIQUE`) | Tell-tale: `/meta` reports a field optional but a write still 400s `" is required"` — that is a stale `NOT NULL` column (physical drift),