diff --git a/.changeset/schema-drift-runtime-managed-index.md b/.changeset/schema-drift-runtime-managed-index.md new file mode 100644 index 0000000000..7bcbb5c434 --- /dev/null +++ b/.changeset/schema-drift-runtime-managed-index.md @@ -0,0 +1,54 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): a fresh database no longer boots "drifted", and the drift +detector never points `--allow-destructive` at an index the framework created +(#4884) + +Booting `examples/app-showcase` on a brand-new empty SQLite file printed two +`[schema-drift]` warnings before the server was even ready, both about the +ADR-0048 overlay indexes the same boot had just created. Both were false, and +one of them was dangerous: + +> `[schema-drift] sys_metadata: index 'idx_sys_metadata_overlay_draft' UNIQUE +> (type, name, organization_id) carries ObjectStack's generated naming but +> matches no declared index (orphaned) — "os migrate apply --allow-destructive" +> to drop it.` + +`idx_sys_metadata_overlay_draft` is the unique index enforcing **draft-overlay +uniqueness**. An operator following our own boot advice would have dropped a +live data-integrity guarantee to fix a problem that did not exist — and, worse, +learned to treat `--allow-destructive` as routine boot hygiene, which is exactly +what makes the *next*, real drift warning dangerous. + +Three fixes, in the driver's detector only (no metadata declaration changed — +`sys-metadata.object.ts` documents its four-column `indexes[]` entry as *the +fallback shape for drivers without the runtime migration*, and that contract +still holds for the drivers that rely on it): + +- **The index key is now read as written.** Introspection took the key from each + dialect's per-column catalogue view (`PRAGMA index_info`, `pg_attribute`, + `STATISTICS.COLUMN_NAME`), which describes an expression key as a NULL column + and nothing else. The canonical + `(type, name, organization_id, COALESCE(package_id,''))` overlay index + therefore arrived as three columns and was reported as a mismatch against its + own four-column declaration. SQLite and Postgres now parse the index + definition (`sqlite_master.sql` / `pg_get_indexdef`), MySQL reads + `STATISTICS.EXPRESSION` where the server has it, and `COALESCE(col, )` + is recognised as keying on `col` — which is what ADR-0048 uses it for: a plain + UNIQUE index treats NULLs as distinct, so package-less globals would not be + unique among themselves. +- **Partial predicates are captured.** A `WHERE`-restricted index is something + `syncDeclaredIndexes` can neither create nor rebuild, so the detector no + longer claims authorship of one, no longer calls it orphaned, and never + proposes a remedy it could not undo. +- **The driver keeps a ledger of the index DDL it executed.** An index this + process created through raw `execute()` — how `metadata-protocol`'s + `ensureOverlayIndex` issues its migration — is the framework's to manage. This + also covers the plain-index fallback the same migration takes on dialects that + reject partial indexes. + +Genuine drift is unaffected: an orphaned generated index, a redefined declared +index and the #3696 legacy-unique replacement are all still detected, still +categorised exactly as before, and still remediable through `os migrate`. diff --git a/packages/plugins/driver-sql/src/index.ts b/packages/plugins/driver-sql/src/index.ts index 5b5d437a8a..7f6d65fedb 100644 --- a/packages/plugins/driver-sql/src/index.ts +++ b/packages/plugins/driver-sql/src/index.ts @@ -14,6 +14,8 @@ export type { // Managed-schema drift / reconcile (#2186), incl. the index dimension (#3728) export { + applyIndexKeyParts, + classifyIndexKeyPart, diffManagedTable, driftKey, fieldHasColumn, @@ -24,9 +26,12 @@ export { isIndexDriftOp, isInPlaceSchemaWork, isManagedIndexName, + isRuntimeManagedIndex, + isSyncReproducibleIndex, legacyUniqueIndexNames, legacyUniqueReplacements, normalizeDeclaredIndex, + parseIndexDdl, uniqueIndexesFromFields, INDEX_DRIFT_OPS, } from './schema-drift.js'; @@ -35,6 +40,8 @@ export type { DriftOp, DriftCategory, SqlDialectName, + IndexKeyPart, + ParsedIndexDdl, PhysicalColumn, PhysicalIndex, ExpectedIndex, diff --git a/packages/plugins/driver-sql/src/schema-drift.ts b/packages/plugins/driver-sql/src/schema-drift.ts index e6c2352a28..a9f2819798 100644 --- a/packages/plugins/driver-sql/src/schema-drift.ts +++ b/packages/plugins/driver-sql/src/schema-drift.ts @@ -496,13 +496,280 @@ export interface ExpectedIndex { unique: boolean; } +// ─────────────────────────────────────────────────────────────────────── +// Physical index key parts (#4884) +// +// Introspection used to read an index's key from the dialect's *column* view +// (`PRAGMA index_info`, `pg_attribute`, `STATISTICS.COLUMN_NAME`), which +// reports NOTHING for an expression key. A four-column index whose last key is +// `COALESCE(package_id,'')` therefore read as three columns, and the differ +// reported a mismatch against a four-column declaration on a database that was +// exactly right. Everything below exists so the key is read as written, and so +// an expression that pins one column is recognised as that column. +// ─────────────────────────────────────────────────────────────────────── + +/** One key part of a physical index, as introspection recovered it. */ +export type IndexKeyPart = + | { kind: 'column'; column: string } + /** `column` is the identity the expression pins, or null when unattributable. */ + | { kind: 'expression'; sql: string; column: string | null }; + +const BARE_IDENTIFIER = /^(?:"([^"]+)"|`([^`]+)`|\[([^\]]+)\]|([A-Za-z_][A-Za-z0-9_$]*))$/; +/** A literal any dialect might print inside `COALESCE`, optional `::type` cast included. */ +const SQL_LITERAL = /^(?:'(?:[^']|'')*'|-?\d+(?:\.\d+)?|null|true|false)(?:::[A-Za-z_][A-Za-z0-9_ ."]*)?$/i; + +/** Unwrap `"x"` / `` `x` `` / `[x]`, or null when `s` is not a single identifier. */ +function matchIdentifier(s: string): string | null { + const m = BARE_IDENTIFIER.exec(s.trim()); + if (!m) return null; + return m[1] ?? m[2] ?? m[3] ?? m[4] ?? null; +} + +/** Drop a trailing `::type` cast (Postgres prints them everywhere). */ +function stripCast(s: string): string { + const i = s.indexOf('::'); + return i > 0 ? s.slice(0, i).trim() : s.trim(); +} + +/** Peel redundant wrapping parens: `(package_id)` → `package_id`. */ +function stripWrappingParens(s: string): string { + let out = s.trim(); + while (out.startsWith('(') && out.endsWith(')')) { + const inner = out.slice(1, -1).trim(); + if (splitTopLevelList(inner).length !== 1) break; + out = inner; + } + return out; +} + +/** + * Peel casts and redundant parens until stable, then read the identifier. + * Postgres nests both — a varchar column inside `COALESCE` prints as + * `(package_id)::text` — so one pass in either order is not enough. + */ +function bareColumnOf(s: string): string | null { + let cur = s.trim(); + for (let i = 0; i < 4; i++) { + const next = stripWrappingParens(stripCast(stripWrappingParens(cur))); + if (next === cur) break; + cur = next; + } + return matchIdentifier(cur); +} + +/** + * Split a comma-separated SQL list at TOP level only — a comma inside nested + * parens or inside a quoted literal/identifier belongs to the part, not to the + * list. `a, b, COALESCE(c, '')` → `['a', 'b', "COALESCE(c, '')"]`. + */ +function splitTopLevelList(list: string): string[] { + const out: string[] = []; + let depth = 0; + let quote: string | null = null; + let start = 0; + for (let i = 0; i < list.length; i++) { + const ch = list[i]; + if (quote) { + // SQL escapes a quote by doubling it; skipping the pair keeps us in-quote. + if (ch === quote) { + if (list[i + 1] === quote) i++; + else quote = null; + } + continue; + } + if (ch === "'" || ch === '"' || ch === '`') quote = ch; + else if (ch === '(') depth++; + else if (ch === ')') depth--; + else if (ch === ',' && depth === 0) { + out.push(list.slice(start, i).trim()); + start = i + 1; + } + } + const tail = list.slice(start).trim(); + if (tail) out.push(tail); + return out; +} + +/** Strip the per-key decorations `pg_get_indexdef` prints (`DESC`, `COLLATE …`). */ +function stripKeyPartModifiers(part: string): string { + let s = part.trim(); + s = s.replace(/\s+nulls\s+(?:first|last)$/i, ''); + s = s.replace(/\s+(?:asc|desc)$/i, ''); + s = s.replace(/\s+collate\s+(?:"[^"]+"|[A-Za-z_][A-Za-z0-9_$.]*)$/i, ''); + return s.trim(); +} + +/** + * Classify one physical index key part. + * + * The single equivalence this recognises is `COALESCE(col, )` ≡ `col`, + * and it is recognised deliberately, not liberally. ADR-0048 makes that form + * the canonical spelling of the overlay key: a plain UNIQUE index treats NULLs + * as distinct, so package-less globals would not be unique among themselves, + * and `COALESCE(package_id,'')` is how the runtime pins them. The expression + * therefore keys on *exactly* `package_id` — for identity purposes it is that + * column, and a UNIQUE index over it is strictly STRONGER than the same key + * spelled plainly. Reading it as a missing column is what produced the false + * "declares 4 columns, has 3" warning in #4884. + * + * Nothing else is coerced. An expression this cannot attribute to one column + * stays `{ column: null }`, which keeps the index out of every claim the + * differ makes ({@link isSyncReproducibleIndex}) rather than guessing at it. + */ +export function classifyIndexKeyPart(rawPart: string): IndexKeyPart { + const s = stripKeyPartModifiers(rawPart); + const bare = matchIdentifier(s); + if (bare !== null) return { kind: 'column', column: bare }; + + const coalesce = /^coalesce\s*\(([\s\S]*)\)$/i.exec(stripWrappingParens(s)); + if (coalesce) { + const args = splitTopLevelList(coalesce[1]); + if (args.length >= 2) { + const column = bareColumnOf(args[0]); + const restAreLiterals = args.slice(1).every((a) => SQL_LITERAL.test(a.trim())); + if (column !== null && restAreLiterals) return { kind: 'expression', sql: rawPart, column }; + } + } + return { kind: 'expression', sql: rawPart, column: null }; +} + +/** An index definition recovered from its `CREATE INDEX` text. */ +export interface ParsedIndexDdl { + /** Ordered key parts exactly as written — a column name, or an expression. */ + keyParts: string[]; + /** The definition carries a `WHERE` predicate (a partial index). */ + partial: boolean; +} + +/** + * Parse the key list and partial predicate out of a `CREATE INDEX` statement. + * + * Used for the two dialects that hand back the definition verbatim — + * `sqlite_master.sql` and `pg_get_indexdef()` — because their per-column + * catalogue views cannot express an expression key at all. Quote-aware, so a + * comma or paren inside a string literal never splits a key part. + */ +export function parseIndexDdl(sql: string): ParsedIndexDdl | null { + if (typeof sql !== 'string' || sql.length === 0) return null; + let depth = 0; + let quote: string | null = null; + let open = -1; + let close = -1; + for (let i = 0; i < sql.length; i++) { + const ch = sql[i]; + if (quote) { + if (ch === quote) { + if (sql[i + 1] === quote) i++; + else quote = null; + } + continue; + } + if (ch === "'" || ch === '"' || ch === '`') quote = ch; + else if (ch === '(') { + if (depth === 0) open = i; + depth++; + } else if (ch === ')') { + depth--; + if (depth === 0) { + close = i; + break; + } + } + } + if (open < 0 || close < 0) return null; + const keyParts = splitTopLevelList(sql.slice(open + 1, close)); + if (keyParts.length === 0) return null; + return { keyParts, partial: /(?:^|[\s)])where[\s(]/i.test(sql.slice(close + 1)) }; +} + +/** + * Fold parsed key parts into a {@link PhysicalIndex}, in key order: attributable + * parts land in `columns`, expression parts are additionally recorded verbatim + * so {@link isSyncReproducibleIndex} can see them. + */ +export function applyIndexKeyParts(index: PhysicalIndex, rawParts: string[]): void { + for (const raw of rawParts) { + const part = classifyIndexKeyPart(raw); + if (part.kind === 'column') { + index.columns.push(part.column); + continue; + } + (index.expressions ??= []).push(part.sql); + if (part.column !== null) index.columns.push(part.column); + } +} + /** An index that physically exists (see `SqlDriver.introspectIndexes`). */ export interface PhysicalIndex { name: string; + /** + * The column identities the index keys on, in key order. + * + * An EXPRESSION key part contributes the column it resolves to when + * {@link classifyIndexKeyPart} can attribute it to one (`COALESCE(pkg,'')` → + * `pkg`, #4884) — the column IS in the key, it is merely written as an + * expression, and dropping it here is what made a healthy database read as + * three-column drift against a four-column declaration. A part that resolves + * to no single column is omitted (and recorded in {@link expressions}). + */ columns: string[]; unique: boolean; /** Backing index of the PRIMARY KEY — never metadata-managed. */ primary?: boolean; + /** + * The index is restricted by a `WHERE` predicate (a SQLite / Postgres partial + * index). See {@link isSyncReproducibleIndex} for why this is load-bearing. + */ + partial?: boolean; + /** + * Key parts written as SQL EXPRESSIONS rather than bare columns, verbatim and + * in key order. Empty/absent for an ordinary index. See + * {@link isSyncReproducibleIndex}. + */ + expressions?: string[]; +} + +/** + * Could the additive index sync have produced this exact physical index — and, + * decisively, could it RECREATE it after a drop? (#4884) + * + * Every remedy this module proposes for an index rests on that second half. + * `drop_index` is safe only because a still-declared index would be + * re-materialized on the next sync; `recreate_index` drops before it creates. + * `syncDeclaredIndexes` builds indexes through knex's `table.unique(fields)` / + * `table.index(fields)` — plain columns, no predicate, no expressions — so an + * index carrying either is one it can NEITHER have created NOR rebuild. Naming + * it drift asserts an authorship we do not have, and pointing + * `--allow-destructive` at it proposes an unrecoverable drop. + * + * That is exactly what a fresh `app-showcase` boot did to + * `idx_sys_metadata_overlay_draft`: the ADR-0048 partial UNIQUE index the + * runtime creates for draft-overlay uniqueness matched no declaration, carried + * ObjectStack's `idx__` naming, and was therefore reported as an orphan + * to be dropped — i.e. the boot advised destroying a live data-integrity + * guarantee the same boot had just created. + */ +export function isSyncReproducibleIndex(index: PhysicalIndex): boolean { + return index.partial !== true && (index.expressions?.length ?? 0) === 0; +} + +/** + * Is this index the framework's to manage rather than the additive sync's? + * + * Two independent witnesses, either of which is sufficient: + * 1. `runtimeCreated` — this very process executed the `CREATE INDEX` through + * the driver's raw `execute()` seam. A database created seconds ago by this + * build cannot be drifted from its own declaration, so a remedy here is + * false by construction. + * 2. The index is not {@link isSyncReproducibleIndex} — durable across + * restarts, and the only witness available on the second boot, when the + * runtime ledger starts empty again. + */ +export function isRuntimeManagedIndex( + index: PhysicalIndex, + runtimeCreated?: ReadonlySet, +): boolean { + return runtimeCreated?.has(index.name) === true || !isSyncReproducibleIndex(index); } /** @@ -705,8 +972,14 @@ export function diffManagedIndexes(args: { expected: ExpectedIndex[]; legacy: LegacyUniqueReplacement[]; physical: PhysicalIndex[]; + /** + * Index names THIS process created through the driver's raw `execute()` DDL + * (#4884). Honoured as a runtime-managed marker — see + * {@link isRuntimeManagedIndex}. + */ + runtimeCreated?: ReadonlySet; }): ManagedDriftEntry[] { - const { table, expected, legacy, physical } = args; + const { table, expected, legacy, physical, runtimeCreated } = args; const out: ManagedDriftEntry[] = []; const byName = new Map(physical.map((p) => [p.name, p])); /** Physical index names accounted for — either declared, or already reported. */ @@ -719,7 +992,8 @@ export function diffManagedIndexes(args: { // 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 (!p || p.primary || isRuntimeManagedIndex(p, runtimeCreated)) return false; + return p.unique && p.columns.length === 1 && p.columns[0] === l.column; }); if (present.length === 0) continue; for (const n of present) explained.add(n); @@ -777,6 +1051,15 @@ export function diffManagedIndexes(args: { continue; } if (p.unique === e.unique && p.columns.join(',') === e.columns.join(',')) continue; + // The framework's own runtime migrations own some declared names — ADR-0048 + // rebuilds `idx_sys_metadata_overlay_active` as a partial UNIQUE over + // `COALESCE(package_id,'')`, and `sys-metadata.object.ts` says in so many + // words that its four-column declaration is "the fallback shape for drivers + // without the runtime migration". Proposing a rebuild FROM that fallback + // would replace a stronger index with a weaker one, under a remedy + // (`recreate_index` → drop first) this differ cannot undo. Not ours to + // reconcile (#4884). + if (isRuntimeManagedIndex(p, runtimeCreated)) 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 @@ -811,6 +1094,13 @@ export function diffManagedIndexes(args: { for (const p of physical) { if (p.primary || p.columns.length === 0 || explained.has(p.name)) continue; if (!isManagedIndexName(table, p)) continue; + // "Generated naming" is a NAME heuristic; it cannot tell an index the sync + // emitted from one the framework's runtime built under a similar name. The + // orphan remedy is a DROP, so the claim has to be stronger than a prefix: + // an index this differ could not recreate is never proposed for deletion + // (#4884 — the boot advised dropping `idx_sys_metadata_overlay_draft`, the + // partial UNIQUE enforcing draft-overlay uniqueness, on a healthy fresh DB). + if (isRuntimeManagedIndex(p, runtimeCreated)) continue; out.push({ kind: 'unmapped_index', remoteName: table, diff --git a/packages/plugins/driver-sql/src/sql-driver-overlay-index-drift.test.ts b/packages/plugins/driver-sql/src/sql-driver-overlay-index-drift.test.ts new file mode 100644 index 0000000000..e57c668a1b --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-overlay-index-drift.test.ts @@ -0,0 +1,360 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { + SqlDriver, + applyIndexKeyParts, + classifyIndexKeyPart, + diffManagedIndexes, + isRuntimeManagedIndex, + isSyncReproducibleIndex, + parseIndexDdl, + type PhysicalIndex, +} from '../src/index.js'; + +/** + * A fresh database must not boot "drifted" (#4884). + * + * `examples/app-showcase` on a brand-new empty SQLite file printed two + * `[schema-drift]` warnings before the server was ready, both about the ADR-0048 + * overlay indexes the framework itself had created seconds earlier: + * + * 1. `idx_sys_metadata_overlay_active` — the detector read the physical index + * as `(type, name, organization_id)` because `PRAGMA index_info` reports a + * NULL column for the `COALESCE(package_id,'')` key part, and the old + * introspection dropped NULL parts. It then reported a mismatch against the + * four-column declaration. The fourth column was there all along. + * 2. `idx_sys_metadata_overlay_draft` — the partial UNIQUE index enforcing + * draft-overlay uniqueness, which no object declares, was classified as an + * orphan and the operator was told to run + * `os migrate apply --allow-destructive` to DROP it: our own boot advising + * the destruction of a live data-integrity guarantee, on a healthy database. + * + * The invariants pinned here: a database this build just created reports zero + * drift, and no `--allow-destructive` remedy is ever printed for an index the + * framework created rather than the additive sync. + */ +describe('overlay index drift on a fresh database (#4884)', () => { + let knexInstance: any; + const tempDirs: string[] = []; + + const makeDriver = (connection: any = { filename: ':memory:' }) => { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection, + useNullAsDefault: true, + }); + knexInstance = (d as any).knex; + (d as any).logger = { warn: vi.fn(), info: vi.fn() }; + return d; + }; + + afterEach(async () => { + await knexInstance?.destroy(); + knexInstance = undefined; + for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + const tempDbFile = (): string => { + const dir = mkdtempSync(join(tmpdir(), 'os-4884-')); + tempDirs.push(dir); + return join(dir, 'fresh.db'); + }; + + /** + * The `sys_metadata` shape that matters here, verbatim from + * `packages/metadata-core/src/objects/sys-metadata.object.ts` — including the + * comment's claim that the four-column declaration is "the fallback shape for + * drivers without the runtime migration", which is precisely why the detector + * must not try to rebuild the runtime's index from it. + */ + const sysMetadataObjects = () => [ + { + name: 'sys_metadata', + fields: { + type: { type: 'string' }, + name: { type: 'string' }, + organization_id: { type: 'string' }, + package_id: { type: 'string' }, + state: { type: 'string' }, + }, + indexes: [ + { + name: 'idx_sys_metadata_overlay_active', + fields: ['type', 'name', 'organization_id', 'package_id'], + unique: true, + partial: "state = 'active'", + }, + { name: 'idx_sys_metadata_org_type', fields: ['organization_id', 'type'] }, + { fields: ['state'] }, + ], + } as any, + ]; + + /** + * Exactly what `metadata-protocol`'s `ensureOverlayIndex` issues (protocol.ts + * ~L2165-2215) — through `execute()`, the same seam it uses, so the runtime + * ledger sees what a real boot would. + */ + const runEnsureOverlayIndex = async (driver: SqlDriver): Promise => { + await driver.execute('DROP INDEX IF EXISTS idx_sys_metadata_overlay_active'); + await driver.execute( + 'CREATE UNIQUE INDEX IF NOT EXISTS idx_sys_metadata_overlay_active ' + + "ON sys_metadata (type, name, organization_id, COALESCE(package_id, '')) " + + "WHERE state = 'active'", + ); + await driver.execute('DROP INDEX IF EXISTS idx_sys_metadata_overlay_draft'); + await driver.execute( + 'CREATE UNIQUE INDEX IF NOT EXISTS idx_sys_metadata_overlay_draft ' + + "ON sys_metadata (type, name, organization_id, COALESCE(package_id, '')) " + + "WHERE state = 'draft'", + ); + }; + + // ── The issue itself ────────────────────────────────────────────────────── + + it('a fresh DB created by this build reports ZERO drift for the two overlay indexes', async () => { + const driver = makeDriver(); + const objects = sysMetadataObjects(); + await driver.initObjects(objects); + await runEnsureOverlayIndex(driver); + + const drift = await driver.detectManagedDrift(objects); + expect(drift.map((d) => d.message)).toEqual([]); + }); + + it('never points --allow-destructive at an index the framework created', async () => { + const driver = makeDriver(); + const objects = sysMetadataObjects(); + await driver.initObjects(objects); + await runEnsureOverlayIndex(driver); + + const drift = await driver.detectManagedDrift(objects); + expect(drift.filter((d) => d.message.includes('--allow-destructive'))).toEqual([]); + // Neither of the two remedies that would destroy the runtime's index. + expect( + drift.filter( + (d) => + (d.op.type === 'drop_index' || d.op.type === 'recreate_index') && + (d.op as any).indexName?.startsWith('idx_sys_metadata_overlay_'), + ), + ).toEqual([]); + }); + + it('stays silent on the SECOND boot, when the runtime ledger starts empty again', async () => { + // The `runtimeCreated` ledger is process-scoped by design, so the durable + // half of the guarantee — the index's own definition — has to carry a + // restart on its own. Same file, brand-new driver, nothing recorded. + const filename = tempDbFile(); + const first = makeDriver({ filename }); + const objects = sysMetadataObjects(); + await first.initObjects(objects); + await runEnsureOverlayIndex(first); + await (first as any).knex.destroy(); + + const second = makeDriver({ filename }); + expect((second as any).runtimeCreatedIndexes.size).toBe(0); + await second.initObjects(objects); + + const drift = await second.detectManagedDrift(objects); + expect(drift.map((d) => d.message)).toEqual([]); + }); + + it('boot-time drift handling logs no [schema-drift] warning for a fresh overlay DB', async () => { + const driver = makeDriver(); + const objects = sysMetadataObjects(); + await driver.initObjects(objects); + await runEnsureOverlayIndex(driver); + + await (driver as any).reconcileAndWarnDrift( + 'sys_metadata', + objects[0].fields, + objects[0].indexes, + ); + expect((driver as any).logger.warn).not.toHaveBeenCalled(); + }); + + // ── Introspection reads the key as WRITTEN ──────────────────────────────── + + it('reads the COALESCE key part as the column it pins, and flags the index partial', async () => { + const driver = makeDriver(); + await driver.initObjects(sysMetadataObjects()); + await runEnsureOverlayIndex(driver); + + const physical: PhysicalIndex[] = await (driver as any).introspectIndexes('sys_metadata'); + const active = physical.find((p) => p.name === 'idx_sys_metadata_overlay_active')!; + expect(active.columns).toEqual(['type', 'name', 'organization_id', 'package_id']); + expect(active.unique).toBe(true); + expect(active.partial).toBe(true); + expect(active.expressions).toEqual(["COALESCE(package_id, '')"]); + expect(isSyncReproducibleIndex(active)).toBe(false); + + const draft = physical.find((p) => p.name === 'idx_sys_metadata_overlay_draft')!; + expect(draft.columns).toEqual(['type', 'name', 'organization_id', 'package_id']); + expect(draft.partial).toBe(true); + + // A plain declared index is unaffected — same columns, no expressions. + const orgType = physical.find((p) => p.name === 'idx_sys_metadata_org_type')!; + expect(orgType.columns).toEqual(['organization_id', 'type']); + expect(orgType.partial).toBeFalsy(); + expect(isSyncReproducibleIndex(orgType)).toBe(true); + }); + + // ── The runtime ledger is a record of fact ──────────────────────────────── + + it('records index DDL it executed, and forgets it again on DROP', async () => { + const driver = makeDriver(); + await driver.initObjects(sysMetadataObjects()); + await runEnsureOverlayIndex(driver); + + const ledger = (driver as any).runtimeCreatedIndexes.get('sys_metadata') as Set; + expect([...ledger].sort()).toEqual([ + 'idx_sys_metadata_overlay_active', + 'idx_sys_metadata_overlay_draft', + ]); + + await driver.execute('DROP INDEX idx_sys_metadata_overlay_draft'); + expect(ledger.has('idx_sys_metadata_overlay_draft')).toBe(false); + }); + + it('reads the table out of quoted and schema-qualified index DDL', async () => { + const driver = makeDriver(); + const note = (sql: string) => (driver as any).noteRuntimeIndexDdl(sql); + const ledger = (driver as any).runtimeCreatedIndexes as Map>; + + note('CREATE UNIQUE INDEX CONCURRENTLY "idx_a" ON "public"."sys_metadata" (a)'); + note('CREATE INDEX IF NOT EXISTS `idx_b` ON `sys_metadata` (b)'); + expect([...(ledger.get('sys_metadata') ?? [])].sort()).toEqual(['idx_a', 'idx_b']); + + note('DROP INDEX IF EXISTS "public"."idx_a"'); + expect([...(ledger.get('sys_metadata') ?? [])]).toEqual(['idx_b']); + }); + + it('honours the ledger even for an index the sync could otherwise reproduce', async () => { + // The MySQL shape of the same migration: `ensureOverlayIndex` falls back to a + // plain, non-partial index when the dialect rejects the WHERE clause, so the + // definition alone cannot exonerate it — only the ledger can. + const plain: PhysicalIndex = { + name: 'idx_sys_metadata_overlay_draft', + columns: ['type', 'name', 'organization_id', 'package_id'], + unique: false, + }; + expect(isSyncReproducibleIndex(plain)).toBe(true); + expect(isRuntimeManagedIndex(plain)).toBe(false); + expect(isRuntimeManagedIndex(plain, new Set(['idx_sys_metadata_overlay_draft']))).toBe(true); + + const entries = diffManagedIndexes({ + table: 'sys_metadata', + expected: [], + legacy: [], + physical: [plain], + runtimeCreated: new Set(['idx_sys_metadata_overlay_draft']), + }); + expect(entries).toEqual([]); + }); + + // ── …without going soft on real orphans ─────────────────────────────────── + + it('still flags a genuinely orphaned generated index as destructive', async () => { + const driver = makeDriver(); + await driver.initObjects([ + { name: 'domain', fields: { host: { type: 'string' } }, indexes: [{ fields: ['host'] }] } as any, + ]); + expect(await (driver as any).getExistingIndexNames('domain')).toContain('idx_domain_host'); + + // Declaration gone, index still there — nothing runtime-managed about it. + const drift = await driver.detectManagedDrift([{ name: 'domain', fields: { host: { type: 'string' } } }]); + const entry = drift.find((d) => d.op.type === 'drop_index'); + expect(entry?.category).toBe('destructive'); + expect(entry?.message).toContain('--allow-destructive'); + }); + + it('still flags a redefined declared index whose physical form is plain', async () => { + const driver = makeDriver(); + await driver.initObjects([ + { name: 'domain', fields: { host: { type: 'string' }, tld: { type: 'string' } } } as any, + ]); + await knexInstance.raw('CREATE INDEX idx_domain_host ON domain (tld)'); + + const drift = await driver.detectManagedDrift([ + { + name: 'domain', + fields: { host: { type: 'string' }, tld: { type: 'string' } }, + indexes: [{ name: 'idx_domain_host', fields: ['host'] }], + }, + ]); + expect(drift.find((d) => d.op.type === 'recreate_index')).toBeDefined(); + }); + + // ── Pure units ──────────────────────────────────────────────────────────── + + describe('classifyIndexKeyPart', () => { + it('reads a bare or quoted column as that column', () => { + expect(classifyIndexKeyPart('package_id')).toEqual({ kind: 'column', column: 'package_id' }); + expect(classifyIndexKeyPart('"package_id"')).toEqual({ kind: 'column', column: 'package_id' }); + expect(classifyIndexKeyPart('`package_id`')).toEqual({ kind: 'column', column: 'package_id' }); + expect(classifyIndexKeyPart('host DESC NULLS LAST')).toEqual({ kind: 'column', column: 'host' }); + }); + + it('attributes COALESCE(col, ) to col — the ADR-0048 canonical form', () => { + for (const sql of [ + "COALESCE(package_id, '')", + "coalesce(package_id,'')", + "COALESCE((package_id)::text, ''::text)", // pg_get_indexdef on a varchar + "COALESCE(\"package_id\", '', 'x')", + ]) { + expect(classifyIndexKeyPart(sql)).toEqual({ kind: 'expression', sql, column: 'package_id' }); + } + }); + + it('refuses to attribute anything it cannot pin to one column', () => { + for (const sql of ['lower(name)', 'COALESCE(a, b)', 'a || b', "COALESCE(name)"]) { + expect(classifyIndexKeyPart(sql)).toEqual({ kind: 'expression', sql, column: null }); + } + }); + }); + + describe('parseIndexDdl', () => { + it('splits the key list at top level only and detects the predicate', () => { + expect( + parseIndexDdl( + 'CREATE UNIQUE INDEX IF NOT EXISTS i ON sys_metadata ' + + "(type, name, organization_id, COALESCE(package_id, '')) WHERE state = 'active'", + ), + ).toEqual({ + keyParts: ['type', 'name', 'organization_id', "COALESCE(package_id, '')"], + partial: true, + }); + }); + + it('parses pg_get_indexdef output', () => { + expect( + parseIndexDdl( + 'CREATE UNIQUE INDEX idx_x ON public.sys_metadata USING btree ' + + "(type, COALESCE(package_id, ''::text)) WHERE (state = 'active'::text)", + ), + ).toEqual({ keyParts: ['type', "COALESCE(package_id, ''::text)"], partial: true }); + }); + + it('is quote-aware and reports a non-partial index as such', () => { + expect(parseIndexDdl("CREATE INDEX i ON t (a, COALESCE(b, ','))")).toEqual({ + keyParts: ['a', "COALESCE(b, ',')"], + partial: false, + }); + expect(parseIndexDdl('')).toBeNull(); + expect(parseIndexDdl('CREATE INDEX i ON t')).toBeNull(); + }); + }); + + it('applyIndexKeyParts keeps key order and records expressions verbatim', () => { + const index: PhysicalIndex = { name: 'i', columns: [], unique: true }; + applyIndexKeyParts(index, ['type', "COALESCE(package_id, '')", 'lower(name)']); + expect(index.columns).toEqual(['type', 'package_id']); + expect(index.expressions).toEqual(["COALESCE(package_id, '')", 'lower(name)']); + expect(isSyncReproducibleIndex(index)).toBe(false); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 44a08ee743..afe23344fc 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -23,6 +23,7 @@ import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared'; import { resolveMultiOrgEnabled } from '@objectstack/types'; import { nextUtcCalendarDay } from '@objectstack/core'; import { + applyIndexKeyParts, buildIndexName, diffManagedIndexes, diffManagedTable, @@ -31,6 +32,7 @@ import { fieldHasColumn, isIndexDriftOp, legacyUniqueReplacements, + parseIndexDdl, uniqueIndexesFromFields, type ManagedDriftEntry, type DriftOp, @@ -49,6 +51,45 @@ import { currentPerfTiming, perfNow, type PerfTiming } from '@objectstack/observ */ const DEFAULT_ID_LENGTH = 16; +// ── Raw index DDL the driver executes on the framework's behalf (#4884) ────── +/** An SQL identifier in any dialect's quoting, or bare. */ +const SQL_IDENTIFIER = '(?:`[^`]+`|"[^"]+"|\\[[^\\]]+\\]|[A-Za-z_][A-Za-z0-9_$]*)'; +/** Cheap prefilter so ordinary `execute()` traffic never touches the parsers below. */ +const INDEX_DDL_PREFIX = /^\s*(?:create|drop)\s+(?:unique\s+)?index\b/i; +const CREATE_INDEX_DDL = new RegExp( + `^\\s*create\\s+(?:unique\\s+)?index\\s+(?:concurrently\\s+)?(?:if\\s+not\\s+exists\\s+)?` + + `(${SQL_IDENTIFIER})\\s+on\\s+(${SQL_IDENTIFIER}(?:\\.${SQL_IDENTIFIER})?)`, + 'i', +); +const DROP_INDEX_DDL = new RegExp( + `^\\s*drop\\s+index\\s+(?:concurrently\\s+)?(?:if\\s+exists\\s+)?` + + `(${SQL_IDENTIFIER}(?:\\.${SQL_IDENTIFIER})?)`, + 'i', +); + +/** Peel one layer of `"…"` / `` `…` `` / `[…]` quoting off an identifier. */ +function unquoteSqlIdentifier(raw: string): string { + const s = raw.trim(); + if (s.length >= 2) { + const first = s[0]; + const last = s[s.length - 1]; + if ((first === '"' && last === '"') || (first === '`' && last === '`') || (first === '[' && last === ']')) { + return s.slice(1, -1); + } + } + return s; +} + +/** + * The unquoted final segment of a possibly schema-qualified reference — + * `"public"."sys_metadata"` → `sys_metadata`. Tokenised rather than split on + * `.`, so a dot INSIDE a quoted identifier does not become a separator. + */ +function lastIdentifierSegment(raw: string): string { + const parts = raw.match(new RegExp(SQL_IDENTIFIER, 'g')); + return unquoteSqlIdentifier(parts && parts.length > 0 ? parts[parts.length - 1] : raw); +} + /** * Internal table that persists per-(object, tenant, field) auto-number * counters so sequences are monotonic, tenant-isolated, and resilient to @@ -2163,7 +2204,48 @@ export class SqlDriver implements IDataDriver { ? this.knex.raw(command, params || []).transacting(options.transaction as Knex.Transaction) : this.knex.raw(command, params || []); - return await builder; + const result = await builder; + // Only after the statement actually succeeded — an index we failed to + // create is not one we own (#4884). + if (INDEX_DDL_PREFIX.test(command)) this.noteRuntimeIndexDdl(command); + return result; + } + + /** + * Index names this process created through raw {@link execute} DDL, keyed by + * table (#4884). + * + * The framework runs a handful of index migrations the additive metadata sync + * cannot express — ADR-0048's partial UNIQUE overlay indexes on `sys_metadata` + * are the reference case, issued by `metadata-protocol`'s `ensureOverlayIndex` + * through this very seam. The drift detector has no other way to tell those + * apart from an index a stale metadata declaration abandoned, and it used to + * tell an operator to `--allow-destructive` the *draft-overlay uniqueness + * guarantee* on a database this build had created seconds earlier. + * + * This is a ledger of fact, not a heuristic: an entry means "this process ran + * that CREATE INDEX and it succeeded". Process-scoped by design — a restart + * starts empty, and the durable half of the guarantee is + * {@link isSyncReproducibleIndex}, which reads the index's own definition. + */ + protected readonly runtimeCreatedIndexes = new Map>(); + + /** Record (or, on a DROP, forget) an index this driver just created via raw DDL. */ + protected noteRuntimeIndexDdl(sql: string): void { + const created = CREATE_INDEX_DDL.exec(sql); + if (created) { + const table = lastIdentifierSegment(created[2]); + let names = this.runtimeCreatedIndexes.get(table); + if (!names) this.runtimeCreatedIndexes.set(table, (names = new Set())); + names.add(unquoteSqlIdentifier(created[1])); + return; + } + const dropped = DROP_INDEX_DDL.exec(sql); + if (!dropped) return; + // SQLite / Postgres `DROP INDEX` names no table, so forget the name + // wherever it is recorded — the ledger must never outlive the index. + const name = lastIdentifierSegment(dropped[1]); + for (const names of this.runtimeCreatedIndexes.values()) names.delete(name); } // =================================== @@ -3663,6 +3745,9 @@ export class SqlDriver implements IDataDriver { // therefore also what must never be mistaken for legacy debt (#3955). legacy: legacyUniqueReplacements({ table: tableName, fields, tenantField, physicalColumns, declaredIndexes }), physical: await this.introspectIndexes(tableName), + // Indexes the framework built through raw DDL on this boot are its own to + // manage — never this differ's to propose dropping (#4884). + runtimeCreated: this.runtimeCreatedIndexes.get(tableName), }); } @@ -4071,6 +4156,15 @@ export class SqlDriver implements IDataDriver { * 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. + * + * ⚠️ The key is read from the index DEFINITION, not from the dialect's + * per-column catalogue view (#4884). Those views describe an expression key + * with a NULL column and nothing else, so `(type, name, organization_id, + * COALESCE(package_id,''))` used to arrive here as three columns — and a + * healthy ADR-0048 overlay index read as drift against its own four-column + * declaration. The partial predicate is captured for the same reason: it is + * what tells the differ this index is none of its business + * (`isSyncReproducibleIndex`). */ protected async introspectIndexes(tableName: string): Promise { const byName = new Map(); @@ -4082,45 +4176,74 @@ export class SqlDriver implements IDataDriver { try { if (this.isSqlite) { const safe = tableName.replace(/[^a-zA-Z0-9_]/g, ''); + // `sqlite_master.sql` is the only place an expression key or a WHERE + // predicate survives; it is NULL for the indexes SQLite auto-creates + // for a UNIQUE/PK constraint, which are plain by construction. + const ddlByName = new Map(); + const master: any = await this.knex.raw( + `SELECT name, sql FROM sqlite_master WHERE type = 'index' AND tbl_name = ?`, + [tableName], + ); + for (const r of Array.isArray(master) ? master : (master?.rows ?? [])) { + if (typeof r?.sql === 'string' && r.sql) ddlByName.set(r.name, r.sql); + } 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); + const parsed = parseIndexDdl(ddlByName.get(r.name) ?? ''); + if (parsed) { + applyIndexKeyParts(entry, parsed.keyParts); + if (parsed.partial) entry.partial = true; + } else { + const info: any = await this.knex.raw(`PRAGMA index_info(${JSON.stringify(r.name)})`); + for (const c of info) if (c.name != null) entry.columns.push(c.name); + } + // `PRAGMA index_list` reports partiality directly (SQLite ≥ 3.8.9); + // belt-and-braces with the parsed predicate above. + if (r.partial === 1 || r.partial === true) entry.partial = true; } } else if (this.isPostgres) { const res: any = await this.knex.raw( `SELECT i.relname AS index_name, ix.indisunique AS is_unique, ix.indisprimary AS is_primary, - a.attname AS column_name + (ix.indpred IS NOT NULL) AS is_partial, + pg_get_indexdef(ix.indexrelid) AS indexdef 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`, + ORDER BY i.relname`, [tableName], ); for (const r of res.rows) { - upsert(r.index_name, r.is_unique === true, r.is_primary === true).columns.push(r.column_name); + const entry = upsert(r.index_name, r.is_unique === true, r.is_primary === true); + if (r.is_partial === true) entry.partial = true; + const parsed = parseIndexDdl(r.indexdef); + if (parsed) applyIndexKeyParts(entry, parsed.keyParts); } } else if (this.isMysql) { - const res: any = await this.knex.raw( - `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], - ); + // `EXPRESSION` only exists from MySQL 8.0.13 (functional indexes); on an + // older server or MariaDB the column is unknown and the query errors, so + // fall back rather than lose index introspection wholesale. + const columns = 'INDEX_NAME, NON_UNIQUE, COLUMN_NAME, EXPRESSION'; + const statisticsQuery = (select: string) => + this.knex.raw( + `SELECT ${select} + FROM information_schema.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ? + ORDER BY INDEX_NAME, SEQ_IN_INDEX`, + [tableName], + ); + let res: any; + try { + res = await statisticsQuery(columns); + } catch { + res = await statisticsQuery('INDEX_NAME, NON_UNIQUE, COLUMN_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); + const entry = upsert(r.INDEX_NAME, Number(r.NON_UNIQUE) === 0, r.INDEX_NAME === 'PRIMARY'); + if (r.COLUMN_NAME != null) entry.columns.push(r.COLUMN_NAME); + else if (r.EXPRESSION != null) applyIndexKeyParts(entry, [String(r.EXPRESSION)]); } } } catch {