diff --git a/.changeset/19332-g2a-fieldgroups-indexes-form-rows.md b/.changeset/19332-g2a-fieldgroups-indexes-form-rows.md index 978548abd07..58eeaee7646 100644 --- a/.changeset/19332-g2a-fieldgroups-indexes-form-rows.md +++ b/.changeset/19332-g2a-fieldgroups-indexes-form-rows.md @@ -10,7 +10,7 @@ Two live structured object keys are authorable in the metadata form: `fieldGroup - `fieldGroups` (Basics, beside `highlightFields`) — six sub-rows, one per canonical group key: `key` and `label` (required text), `icon` (text), `description` (textarea), `collapse` (a `none` / `expanded` / `collapsed` select) and `visibleWhen` (`type: 'code'`, `language: 'expression'`, the `fields` grid's predicate rows). The three `[DEPRECATED → collapse]` aliases (`defaultExpanded`, `collapsible`, `collapsed`) are **not** offered; the metadata-form reconciliation ledger records a nested `omit` row for each. The parse still accepts them and derives `collapse` from one only when `collapse` is absent, so a stored entry keeps its meaning, and a `collapse` set in the form outranks any alias it carries. - `indexes` (Advanced, beside `datasource`) — three sub-rows over the keys the SQL driver reads: `name` (text), `fields` (`widget: 'string-tags'`, required) and `unique`, a select offering **only** `global` and `organization`. The deprecated bare `unique: true` is never offered: a schema-derived control would take the union's first arm and render a switch that writes it. An edit merges into the stored entry, so an index that already carries `true` or `false` keeps it until the author picks a scope, and the select can write only the two values the parse accepts. `type` and `partial` are tombstones and have no row. -The help text states what the runtime does with each value. `indexes[].fields` is free text, and no authoring door judges its names: not the schema parse, not the publish door, not `os validate`. A name that is not a stored column makes the SQL driver skip the whole index at sync with a warning in the server log, and the help text says exactly that. A field group has no field-name list: a field joins a group through its own `group` key. +The help text states what the runtime does with each value. `indexes[].fields` is free text, and the schema parse, so a draft save, does not judge its names; `os validate`, `os build`, `os lint` and the publish door refuse a name that is not a field of the object (`object-field-ref-unknown`, #20479, in the same release). A name that is not a stored column, a `formula` field say, makes the SQL driver skip the whole index at sync with an error in the server log, and the help text says exactly that; `os migrate plan` reports the skipped index too (#20432, in the same release). A field group has no field-name list: a field joins a group through its own `group` key. The two row schemas also carry a JSON Schema `title` on every property, as every repeater row schema must: `IndexSchema` on `name`, `fields` and `unique`, and `ObjectFieldGroupSchema` on its nine keys, the three deprecated aliases included. A property panel that reads the served schema's titles therefore shows a named column instead of a raw key. Each title is a `.meta({ title })` call and nothing more. diff --git a/.changeset/20432-skipped-index-durability.md b/.changeset/20432-skipped-index-durability.md new file mode 100644 index 00000000000..df9c6b6ad4b --- /dev/null +++ b/.changeset/20432-skipped-index-durability.md @@ -0,0 +1,63 @@ +--- +'@objectstack/driver-sql': minor +'@objectstack/spec': patch +'@objectstack/platform-objects': patch +'@objectstack/lint': patch +--- + +fix(driver-sql): a declared index that can never be built is logged at `error` and reported in drift + +**Clause-②: yes (widening)**: the exported `DriftOp` union gains one member, `unbuildable_index`. +No accept set changes. Nothing an author could write before is refused now. + +A declared index names a column that no declaration will ever create when: + +- the name is not a field of the object, for example a misspelling that the Studio save door + admits (`os validate` / `os build` already refuse it); or +- the name is a virtual `formula` field, which is computed on read and has no column. The same + applies to a field-level `unique` on a formula field. + +The SQL driver skips such an index at every sync. It used to say so at `warn`, and the drift +report dropped the index from the expected set, so `os migrate plan` showed nothing. For a +`unique` index, the declared constraint was not enforced and duplicate rows were accepted, +while everything looked normal. + +- **The sync logs the skip at `error`**, on the same durability channel as the duplicate-row + refusals in the same loop. One line per skipped index per sync names the object, the index, + each missing column with its reason (not a field of the object, or a formula field), and + whether the index is `UNIQUE`. The structured meta carries `index`, `missing` and `unique`. +- **Drift reports it** as a report-only entry: `kind: 'index_mismatch'`, `actual: '(absent)'`, + `category: 'needs_confirm'`, `severity: 'error'` for a unique index and `'warning'` otherwise. + Its op is the new member: + + ```ts + { type: 'unbuildable_index'; table: string; column?: string; indexName: string; + unique: boolean; missingColumns: string[] } + ``` + + `missingColumns` lists only the columns that will never materialize. A declared column that + is merely not added yet is pending additive work, not this finding. + +**What a consumer that reads `op.type` now sees.** A new value, `'unbuildable_index'`. It has +no reconciler arm, and none can exist, because there is no column to build over. The remedy is +a metadata edit. It is in `INDEX_DRIFT_OPS`, so `isIndexDriftOp` answers `true` and it never +triggers a SQLite table rebuild. `applyMigrationEntries` reports it `skipped` on every dialect. +`os migrate plan` lists it under "Needs confirmation", addressed by its index name. `os migrate +apply` counts it like any `needs_confirm` entry (so it asks for `--yes`), and then reports it +skipped. The artifact-pinned boot warns about it and still starts, because +only `destructive` entries refuse a boot. A `switch` over `op.type` that treats unknown values +as "not applied" needs no change. An exhaustive `switch` with a `never` check gets one more case +to handle. + +**The object form's help text follows.** The `indexes` → Fields help in the Studio object form +said the skip left "a warning in the server log". It now says an error, in English and in the +zh-CN, ja-JP and es-ES translations. Nothing else in the text changes. + +**The lint message follows too.** `object-field-ref-unknown`, on a misspelt `indexes[].fields` +name, said the SQL driver skips the index "with only a warning, and drift drops it too". It now +says the skip is logged at error and `os migrate plan` reports the index as unbuildable. The rule, +its severity and its prescription are unchanged. + +**Upgrade note:** on a database that already carries such an index, `os migrate plan` now +reports one entry per index, and so does the boot's drift warning. That entry clears only when +the metadata names stored fields or drops the index. diff --git a/packages/cli/src/utils/artifact-boot-migration.unbuildable-index.test.ts b/packages/cli/src/utils/artifact-boot-migration.unbuildable-index.test.ts new file mode 100644 index 00000000000..55050b96db7 --- /dev/null +++ b/packages/cli/src/utils/artifact-boot-migration.unbuildable-index.test.ts @@ -0,0 +1,126 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20432] What the CLI does with the driver's new report-only drift op, + * `unbuildable_index`: a declared index that can never be built, because a key + * column is not a field of the object or is a virtual `formula` field. + * + * ## Why this lives in `packages/cli` + * + * The driver owns the entry. The CLI owns the three places it lands: + * + * - `os migrate plan` renders it through `renderPlan` / `driftTarget`, which + * read `category`, `op.indexName`, `op.type` and `message` generically. + * This file proves that the new op needs no CLI source change to be shown. + * - `os migrate apply` hands it to `applyMigrationEntries`, which reports it + * `skipped`: there is no reconciler arm, and none can exist. + * - The artifact-pinned boot gate applies every non-destructive entry and + * refuses the boot only on `category === 'destructive'`. A report-only + * entry must WARN and let the boot continue. Refusing would take down + * every deployment carrying a misspelt index column at `kernel:ready`, + * over a declaration that no DDL can repair. That is the same asymmetry + * `artifact-boot-migration.report-only-drift.test.ts` pins for + * `manual_column_type_change`. + * + * Every entry here comes from the REAL driver (an in-memory SQLite + * `SqlDriver`) and goes through the REAL gate and renderer. A hand-stamped + * entry would stay green on the day the driver starts emitting the op as + * `destructive`, because nothing would connect the two. + * + * ⚠️ `@objectstack/driver-sql` resolves through its `exports` to its **dist** + * (no `resolve.alias` for it in this package, deliberately), so this file + * reads the BUILT driver. A stale `dist/` makes it a verdict about build + * state. The value import also puts it in the `integration` tier + * (`vitest-tiers.ts`, KERNEL). + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { SqlDriver, buildIndexName } from '@objectstack/driver-sql'; +import { runArtifactBootMigrationGate } from './artifact-boot-migration.js'; +import { driftTarget, groupByCategory, renderPlan, type SqlDriverLike } from './schema-migrate.js'; + +const T = 'os20432_boot'; +const INDEX = buildIndexName(T, ['statsu'], true); + +/** A table whose only real column is `status`, declaring a UNIQUE over the misspelling `statsu`. */ +const OBJECT = { + name: T, + tenancy: { enabled: false }, + fields: { status: { type: 'text', maxLength: 64 } }, + indexes: [{ fields: ['statsu'], unique: true as const }], +}; + +async function syncedDriver(): Promise { + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + // Quiet: the driver's own error line is pinned in driver-sql. This file is about the CLI's handling. + (driver as any).logger = { debug() {}, info() {}, warn() {}, error() {} }; + await driver.initObjects([OBJECT]); + return driver; +} + +describe('the unbuildable_index drift op through the CLI (#20432)', () => { + let driver: SqlDriver | undefined; + afterEach(async () => { + vi.restoreAllMocks(); + await driver?.disconnect().catch(() => {}); + driver = undefined; + }); + + it('the fixture is real: the driver reports exactly one unbuildable_index entry, needs_confirm', async () => { + driver = await syncedDriver(); + const drift = await driver.detectManagedDrift(); + expect(drift.map((d) => d.op.type)).toEqual(['unbuildable_index']); + expect(drift[0]).toMatchObject({ category: 'needs_confirm', op: { indexName: INDEX, unique: true } }); + }); + + it('the artifact boot gate warns about it and does NOT refuse the boot', async () => { + driver = await syncedDriver(); + const info: string[] = []; + const warn: string[] = []; + + // The gate's two members, delegated to the REAL driver. `bootSchemaStack` + // reaches the driver by duck type, and `SqlDriver` keeps `config` + // protected, so the class itself is not assignable to `SqlDriverLike`. + const real = driver; + const gateDriver: SqlDriverLike = { + detectManagedDrift: () => real.detectManagedDrift(), + applyMigrationEntries: (entries, opts) => real.applyMigrationEntries(entries, opts), + }; + const verdict = await runArtifactBootMigrationGate({ + driver: gateDriver, + artifactDisplay: 'https://artifacts.example.com/app.json', + info: (m) => info.push(m), + warn: (m) => warn.push(m), + }); + + expect(verdict.ok).toBe(true); + expect(verdict.refusal).toBeUndefined(); + expect(verdict.destructive).toEqual([]); + // Handed to the driver, which declined it: skipped, never "migrated". + expect(verdict.applied).toEqual([]); + expect(verdict.skipped.map((d) => d.op.type)).toEqual(['unbuildable_index']); + expect(info).toEqual([]); + // …and the skip is not silent: one warn, carrying the driver's message. + expect(warn).toHaveLength(1); + expect(warn[0]).toContain(T); + expect(warn[0]).toContain(INDEX); + }); + + it('os migrate plan renders it with no CLI change: grouped needs_confirm, targeted by index name, tagged by op', async () => { + driver = await syncedDriver(); + const drift = await driver.detectManagedDrift(); + + expect(groupByCategory(drift).needs_confirm).toEqual(drift); + expect(driftTarget(drift[0]!)).toBe(`${T} [${INDEX}]`); + + const lines: string[] = []; + vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { + lines.push(args.map(String).join(' ')); + }); + renderPlan(drift); + const out = lines.join('\n'); + expect(out).toContain(`${T} [${INDEX}]`); + expect(out).toContain('[unbuildable_index]'); + expect(out).toContain(drift[0]!.message); + }); +}); diff --git a/packages/drivers/driver-sql/src/schema-drift.ts b/packages/drivers/driver-sql/src/schema-drift.ts index 545bb61a24a..50ae8546842 100644 --- a/packages/drivers/driver-sql/src/schema-drift.ts +++ b/packages/drivers/driver-sql/src/schema-drift.ts @@ -264,6 +264,39 @@ export type DriftOp = * row report, old index left in place. */ tightenNullSafeOnly?: boolean; + } + /** + * REPORT ONLY. Metadata declares an index that can never be built, because + * a key column is one no declaration will ever materialize: the name is not + * a field of the object (a misspelling the Studio save door admits), or it + * is a virtual `formula` field, which is computed on read and has no column. + * `SqlDriver.syncDeclaredIndexes` skips the whole index and says so at + * `error`. For a UNIQUE index, the constraint it declares is NOT enforced + * while the object keeps working normally, which is the AGENTS.md + * durability-degradation shape. + * + * ⛔ There is NO reconciler arm, and none can exist: there is no column to + * build the index over. The remedy is a metadata edit (name stored fields in + * `indexes[].fields`, or drop the index), so `applyMigrationEntries` reports + * the entry as skipped, on every dialect. The op sits in + * {@link INDEX_DRIFT_OPS} so that skip takes the index path and never + * triggers a SQLite table rebuild. + * + * This op exists so that `os migrate plan` can show the skip. Before it, + * {@link expectedIndexes} dropped such an index from the expected set, and + * the plan reported nothing at all. + * + * `missingColumns` lists only the key columns that will never materialize. + * A declared column that is merely not added YET is pending additive work, + * not this finding. + */ + | { + type: 'unbuildable_index'; + table: string; + column?: string; + indexName: string; + unique: boolean; + missingColumns: string[]; }; /** @@ -337,11 +370,12 @@ export const INDEX_DRIFT_OPS: ReadonlySet = new Set([ 'create_index', 'drop_index', 'recreate_index', + 'unbuildable_index', ]); export type IndexDriftOp = Extract< DriftOp, - { type: 'replace_unique_index' | 'create_index' | 'drop_index' | 'recreate_index' } + { type: 'replace_unique_index' | 'create_index' | 'drop_index' | 'recreate_index' | 'unbuildable_index' } >; /** Ops that act on a single column — the only ones with a guaranteed `column`. */ export type ColumnDriftOp = Exclude; @@ -1845,10 +1879,13 @@ export function normalizeDeclaredIndex( * `true` taken verbatim, `'organization'` scoped through * {@link normalizeDeclaredIndex} (ADR-0120 D1). * - * 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. + * Indexes referencing a column that is not physically present are left out + * of the expected set. The sync cannot create them, so a `create_index` for + * one would name a remedy that `os migrate apply` can never perform. That does + * NOT make them silent. An index whose missing column will never materialize + * (a misspelt name, a virtual `formula` field) is reported by + * {@link diffUnbuildableIndexes} as a report-only `unbuildable_index` entry, + * and the sync logs its skip at `error`. */ export function expectedIndexes(args: { table: string; @@ -1857,13 +1894,131 @@ export function expectedIndexes(args: { declaredIndexes?: DeclaredIndexInput[]; physicalColumns: Set; }): ExpectedIndex[] { - const { table, fields, tenantField, declaredIndexes, physicalColumns } = args; + const { physicalColumns } = args; + return declaredIndexSet(args).filter((i) => i.columns.every((c) => physicalColumns.has(c))); +} + +/** + * Field-level `unique` plus the object's `indexes[]`, both normalized: the one + * composition that {@link expectedIndexes} and {@link diffUnbuildableIndexes} + * split between them. It is shared so the two can never disagree about which + * indexes metadata asks for. + */ +function declaredIndexSet(args: { + table: string; + fields: Record; + tenantField: string | null; + declaredIndexes?: DeclaredIndexInput[]; +}): ExpectedIndex[] { + const { table, fields, tenantField, declaredIndexes } = args; const out = uniqueIndexesFromFields(table, fields, tenantField); for (const idx of Array.isArray(declaredIndexes) ? declaredIndexes : []) { const norm = normalizeDeclaredIndex(table, idx, tenantField); if (norm) out.push(norm); } - return out.filter((i) => i.columns.every((c) => physicalColumns.has(c))); + return out; +} + +/** + * Why a declared index key column has no physical column, in words an + * operator can act on. The fields are the object's own when they are known. + * Without them, as on the drift-op apply path, the column is only named. + * + * Shared by the sync's `error` line (`SqlDriver.syncDeclaredIndexes`) and the + * `unbuildable_index` drift message, so the two cannot give different reasons + * for the same column. + */ +export function describeMissingIndexColumns(missing: string[], fields?: Record): string { + return missing + .map((column) => { + if (!fields) return `'${column}'`; + const field = fields[column]; + if (field == null) return `'${column}' (not a field of the object)`; + if (!fieldHasColumn(field)) return `'${column}' (a formula field: computed on read, never stored)`; + return `'${column}' (a declared field whose column the table does not have)`; + }) + .join(', '); +} + +/** + * Will metadata ever put a physical column under this name? Builtins and the + * tenant column always exist. Otherwise it must be a declared field that + * materializes a column ({@link fieldHasColumn}: not a virtual `formula`). + */ +function columnEverMaterializes(column: string, fields: Record, tenantField: string | null): boolean { + if (BUILTIN_COLUMNS.has(column) || column === tenantField) return true; + const field = fields?.[column]; + return field != null && fieldHasColumn(field); +} + +/** + * Report every declared index that can never be built, as a report-only + * `unbuildable_index` drift entry: the half of the declared set that + * {@link expectedIndexes} leaves out and could otherwise stay unseen. + * + * An index qualifies when at least one key column is missing from the table + * AND will never materialize: the name is not a field of the object (a + * misspelling), or it is a virtual `formula` field. The sync skips such an + * index on every run, so the declaration and the database stay apart for good, + * and the only remedy is a metadata edit. + * + * ⛔ A column that is merely not added YET is left out on purpose. A declared, + * column-materializing field that the table lacks is pending additive work + * (`os migrate plan`'s `add_columns`). The index is created in the same sync + * that adds the column, so reporting it here would be a false finding. + * + * Classified like the other report-only ops (`manual_column_type_change`): + * `needs_confirm`, so `os migrate apply` reports it skipped and the + * artifact-pinned boot warns about it without refusing to start. A UNIQUE + * index is `severity: 'error'`: the constraint it declares is not enforced. + * A plain index is `warning`, the same split as `recreate_index`. + */ +export function diffUnbuildableIndexes(args: { + table: string; + fields: Record; + tenantField: string | null; + declaredIndexes?: DeclaredIndexInput[]; + physicalColumns: Set; +}): ManagedDriftEntry[] { + const { table, fields, tenantField, physicalColumns } = args; + const out: ManagedDriftEntry[] = []; + const seen = new Set(); + for (const idx of declaredIndexSet(args)) { + if (seen.has(idx.name)) continue; + const missing = idx.columns.filter( + (c) => !physicalColumns.has(c) && !columnEverMaterializes(c, fields, tenantField), + ); + if (missing.length === 0) continue; + seen.add(idx.name); + const signature = indexSignature(idx.columns, idx.unique, idx.nullSafeColumns); + out.push({ + kind: 'index_mismatch', + remoteName: table, + table, + column: missing[0], + expected: signature, + actual: '(absent)', + severity: idx.unique ? 'error' : 'warning', + category: 'needs_confirm', + op: { + type: 'unbuildable_index', + table, + column: missing[0], + indexName: idx.name, + unique: idx.unique, + missingColumns: missing, + }, + message: + `${table}: metadata declares index '${idx.name}' ${signature}, but it can never be built: no column for ` + + `${describeMissingIndexColumns(missing, fields)}. ` + + (idx.unique + ? 'The uniqueness it declares is NOT enforced, and duplicate rows are accepted. ' + : 'The index does not exist. ') + + `"os migrate apply" cannot create it and reports it skipped. Fix the metadata: every column in the ` + + `index's fields must be a stored field of the object, or remove the index.`, + }); + } + return out; } /** diff --git a/packages/drivers/driver-sql/src/sql-driver-20432-unbuildable-declared-index.test.ts b/packages/drivers/driver-sql/src/sql-driver-20432-unbuildable-declared-index.test.ts new file mode 100644 index 00000000000..c83530cc1bb --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-20432-unbuildable-declared-index.test.ts @@ -0,0 +1,315 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20432] A declared index that can never be built is loud: logged at + * `error` on the durability channel, and reported in drift. + * + * ## The defect this pins + * + * An object declaring `indexes: [{ fields: ['statsu'], unique: true }]` on a + * table whose only column is `status` synced with the whole index skipped and + * one `warn` line. `expectedIndexes` then dropped the index from the expected + * set, so `detectManagedDrift` (what `os migrate plan` renders) answered `[]`. + * Measured on SQLite before the fix, the log read + * `[sql-driver] skipping declared index on "…" — column(s) not materialized: statsu` + * at `warn`, and the drift report was empty. The declared uniqueness was + * unenforced, and every instrument an operator would reach for said nothing + * was wrong. That is the AGENTS.md durability-degradation shape: "does the + * system still look normal from the outside while something it claims has + * not landed? Yes → error". The duplicate-row arms of the same loop already + * answered their version of it at `error`, through `logDurabilityFailure`. + * + * Step 1 closed the build doors: the lint rule refuses a misspelt index + * column at `os validate` / `os build`. The Studio save door + * (`ObjectSchema.safeParse`) still admits one, and a virtual `formula` field + * is a real field that passes the lint existence check yet has no column. So + * the driver's own answer still matters. + * + * ## The seam + * + * `logDurabilityFailure` writes to the driver's `logger.error`, the same + * channel its duplicate-row siblings are pinned through + * (`sql-driver-14902-plain-unique-duplicate-preflight.test.ts`). Every + * assertion reads the logger double: the level, the structured meta + * (`index`, `missing`, `unique`), and the named subjects in the line. The + * prose is not pinned beyond what names the subject, and the one word that + * says which kind of index was skipped. + * + * ## The matrix + * + * The skip is a pure column-presence check, and the report-only op never + * reaches a dialect's DDL. The fixture still runs on every cell, because what + * `initObjects` materializes (and so what is physically absent) is a + * per-dialect fact, and so is the index introspection the drift report reads. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from './sql-driver.js'; +import { + buildIndexName, + describeMissingIndexColumns, + diffUnbuildableIndexes, + expectedIndexes, + type ManagedDriftEntry, +} from './schema-drift.js'; +import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const MATRIX = 'unbuildable declared index'; + +interface LogLine { + level: 'debug' | 'info' | 'warn' | 'error'; + msg: string; + meta?: any; +} + +/** The logger double: every channel, with the structured meta kept. */ +function attachLogSpy(driver: SqlDriver): LogLine[] { + const logs: LogLine[] = []; + (driver as any).logger = { + debug: (msg: string, meta?: any) => logs.push({ level: 'debug', msg, meta }), + info: (msg: string, meta?: any) => logs.push({ level: 'info', msg, meta }), + warn: (msg: string, meta?: any) => logs.push({ level: 'warn', msg, meta }), + error: (msg: string, meta?: any) => logs.push({ level: 'error', msg, meta }), + }; + return logs; +} + +const existingIndexNames = (driver: SqlDriver, table: string): Promise> => + (driver as any).getExistingIndexNames(table); + +function declareUnbuildableIndexSuite(cell: DialectCell): void { + describe(`a declared index that can never be built is loud — ${cell.label} (#20432)`, () => { + let driver: SqlDriver; + let logs: LogLine[]; + const created: string[] = []; + + beforeEach(() => { + driver = new SqlDriver(cell.config()); + logs = attachLogSpy(driver); + }); + + afterEach(async () => { + for (const t of created.splice(0)) { + await driver.execute(`drop table if exists ${t}`).catch(() => {}); + } + await driver.disconnect().catch(() => {}); + }); + + /** Fresh table per test, dropped first in case a previous run left it. */ + const syncFresh = async (obj: { name: string; [k: string]: any }) => { + await driver.execute(`drop table if exists ${obj.name}`).catch(() => {}); + created.push(obj.name); + await driver.initObjects([obj as any]); + }; + + describe('a misspelt column in a UNIQUE index', () => { + const T = 'os20432_misspelt'; + const INDEX = buildIndexName(T, ['statsu'], true); + const obj = { + name: T, + tenancy: { enabled: false }, + fields: { status: { type: 'text', maxLength: 64 } }, + indexes: [{ fields: ['statsu'], unique: true as const }], + }; + + it('logs the skip once, at error, naming the object, the index, the column and UNIQUE', async () => { + await syncFresh(obj); + + const lines = logs.filter((l) => l.meta?.index === INDEX); + expect(lines).toHaveLength(1); + expect(lines[0]!.level).toBe('error'); + expect(lines[0]!.meta).toMatchObject({ tableName: T, index: INDEX, missing: ['statsu'], unique: true }); + expect(lines[0]!.msg).toContain(`"${T}"`); + expect(lines[0]!.msg).toContain(`'${INDEX}'`); + expect(lines[0]!.msg).toContain(`'statsu' (not a field of the object)`); + expect(lines[0]!.msg).toContain('UNIQUE'); + + // The level MOVED; it did not gain a second line. Before the fix the + // only trace of this skip was a warn naming the column. + expect(logs.filter((l) => l.level === 'warn' && l.msg.includes('statsu'))).toEqual([]); + // And the consequence the line reports is real: nothing was built. + expect((await existingIndexNames(driver, T)).has(INDEX)).toBe(false); + }); + + it('is reported in drift as a report-only unbuildable_index entry, and apply skips it', async () => { + await syncFresh(obj); + + const drift = await driver.detectManagedDrift([obj]); + const unbuildable = drift.filter((d) => d.op.type === 'unbuildable_index'); + expect(unbuildable).toHaveLength(1); + expect(unbuildable[0]).toMatchObject({ + kind: 'index_mismatch', + table: T, + column: 'statsu', + actual: '(absent)', + severity: 'error', + category: 'needs_confirm', + op: { type: 'unbuildable_index', table: T, indexName: INDEX, unique: true, missingColumns: ['statsu'] }, + }); + // No second finding claims a remedy for the same index: a + // `create_index` here would name a DDL that can never run. + expect(drift.filter((d) => d !== unbuildable[0] && (d.op as any).indexName === INDEX)).toEqual([]); + + // The apply path: the flags `os migrate apply` and the artifact boot + // gate pass. Skipped, never applied, on every dialect… + const { applied, skipped } = await driver.applyMigrationEntries(unbuildable, { allowDestructive: false }); + expect(applied).toEqual([]); + expect(skipped).toEqual(unbuildable); + // …and nothing changed, so the next plan reports it again. Only a + // metadata edit clears it. + expect((await existingIndexNames(driver, T)).has(INDEX)).toBe(false); + const again = await driver.detectManagedDrift([obj]); + expect(again.filter((d) => d.op.type === 'unbuildable_index').map((d) => (d.op as any).indexName)).toEqual([ + INDEX, + ]); + }); + }); + + describe('a virtual formula field in a plain index (the step-1 boundary)', () => { + const T = 'os20432_formula'; + const INDEX = buildIndexName(T, ['doubled'], false); + const obj = { + name: T, + tenancy: { enabled: false }, + fields: { + amount: { type: 'number' }, + doubled: { type: 'formula', expression: 'amount * 2' }, + }, + indexes: [{ fields: ['doubled'] }], + }; + + it('is not materialized, and the error names it as a formula field and says the index is not UNIQUE', async () => { + await syncFresh(obj); + + const lines = logs.filter((l) => l.meta?.index === INDEX); + expect(lines).toHaveLength(1); + expect(lines[0]!.level).toBe('error'); + expect(lines[0]!.meta).toMatchObject({ tableName: T, index: INDEX, missing: ['doubled'], unique: false }); + expect(lines[0]!.msg).toContain(`'doubled' (a formula field`); + expect(lines[0]!.msg).not.toContain('UNIQUE'); + expect((await existingIndexNames(driver, T)).has(INDEX)).toBe(false); + }); + + it('is reported in drift at warning severity, since no uniqueness is lost', async () => { + await syncFresh(obj); + + const unbuildable = (await driver.detectManagedDrift([obj])).filter((d) => d.op.type === 'unbuildable_index'); + expect(unbuildable).toHaveLength(1); + expect(unbuildable[0]).toMatchObject({ + severity: 'warning', + category: 'needs_confirm', + op: { indexName: INDEX, unique: false, missingColumns: ['doubled'] }, + }); + }); + }); + + describe('control: a buildable declared index', () => { + const T = 'os20432_control'; + const INDEX = buildIndexName(T, ['status'], true); + const obj = { + name: T, + tenancy: { enabled: false }, + fields: { status: { type: 'text', maxLength: 64 } }, + indexes: [{ fields: ['status'], unique: true as const }], + }; + + it('is built and enforced, with no error line and no unbuildable_index entry', async () => { + await syncFresh(obj); + + expect(logs.filter((l) => l.level === 'error')).toEqual([]); + expect((await existingIndexNames(driver, T)).has(INDEX)).toBe(true); + // The fixture can express a UNIQUE on this cell: it is enforced. + await driver.execute(`insert into ${T} (id, status) values ('r1', 'same')`); + await expect(driver.execute(`insert into ${T} (id, status) values ('r2', 'same')`)).rejects.toThrow(); + + const drift = await driver.detectManagedDrift([obj]); + expect(drift.filter((d) => d.op.type === 'unbuildable_index')).toEqual([]); + }); + }); + }); +} + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, MATRIX, declareUnbuildableIndexSuite); +} + +// ── The pure differ: which missing columns make an index unbuildable ──────── + +describe('diffUnbuildableIndexes — only a column that will NEVER materialize (#20432)', () => { + const T = 'os20432_pure'; + const physical = (...cols: string[]) => new Set(['id', 'created_at', 'updated_at', ...cols]); + const ops = (entries: ManagedDriftEntry[]) => entries.map((e) => e.op as any); + + it('a declared field whose column is merely not added yet is pending work, not this finding', () => { + const fields = { status: { type: 'text' }, code: { type: 'text' } }; + const entries = diffUnbuildableIndexes({ + table: T, + fields, + tenantField: null, + declaredIndexes: [{ fields: ['code'], unique: true }], + physicalColumns: physical('status'), + }); + expect(entries).toEqual([]); + }); + + it('names only the never-materializing columns when an index mixes both kinds', () => { + const fields = { status: { type: 'text' }, code: { type: 'text' } }; + const entries = diffUnbuildableIndexes({ + table: T, + fields, + tenantField: null, + declaredIndexes: [{ fields: ['code', 'statsu'] }], + physicalColumns: physical('status'), + }); + expect(ops(entries)).toEqual([ + { + type: 'unbuildable_index', + table: T, + column: 'statsu', + indexName: buildIndexName(T, ['code', 'statsu'], false), + unique: false, + missingColumns: ['statsu'], + }, + ]); + }); + + it('covers a field-level unique on a formula field, the other route into the same sync', () => { + const fields = { amount: { type: 'number' }, doubled: { type: 'formula', unique: true } }; + const entries = diffUnbuildableIndexes({ + table: T, + fields, + tenantField: null, + physicalColumns: physical('amount'), + }); + expect(ops(entries)).toEqual([ + expect.objectContaining({ indexName: buildIndexName(T, ['doubled'], true), unique: true, missingColumns: ['doubled'] }), + ]); + expect(entries[0]!.severity).toBe('error'); + }); + + it('splits the declared set with expectedIndexes: each index lands on exactly one side, or is pending', () => { + const fields = { status: { type: 'text' }, code: { type: 'text' }, doubled: { type: 'formula' } }; + const declaredIndexes = [ + { fields: ['status'], unique: true as const }, + { fields: ['statsu'], unique: true as const }, + { fields: ['doubled'] }, + { fields: ['code'] }, + ]; + const args = { table: T, fields, tenantField: null, declaredIndexes, physicalColumns: physical('status') }; + expect(expectedIndexes(args).map((i) => i.name)).toEqual([buildIndexName(T, ['status'], true)]); + expect(ops(diffUnbuildableIndexes(args)).map((o) => o.indexName)).toEqual([ + buildIndexName(T, ['statsu'], true), + buildIndexName(T, ['doubled'], false), + ]); + }); + + it('gives each column its own reason kind when the fields are known, and only the name when they are not', () => { + const fields = { code: { type: 'text' }, doubled: { type: 'formula' } }; + const said = describeMissingIndexColumns(['statsu', 'doubled', 'code'], fields); + expect(said).toContain(`'statsu' (not a field`); + expect(said).toContain(`'doubled' (a formula field`); + expect(said).toContain(`'code' (a declared field`); + // The drift-op apply path passes no fields, so it cannot claim a reason. + expect(describeMissingIndexColumns(['statsu'])).toBe(`'statsu'`); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index d58ddd20c9d..6c8223ab802 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -94,8 +94,10 @@ import { nextUtcCalendarDay, temporalStorageForm } from '@objectstack/core'; import { applyIndexKeyParts, buildIndexName, + describeMissingIndexColumns, diffManagedIndexes, diffManagedTable, + diffUnbuildableIndexes, driftKey, expectedIndexes, fieldHasColumn, @@ -11670,6 +11672,7 @@ export class SqlDriver implements IDataDriver { perShard, new Set(Object.keys(colInfo)), this.computeAndRecordTenantField(baseTable, obj), + obj.fields ?? {}, ); } } @@ -13423,6 +13426,12 @@ export class SqlDriver implements IDataDriver { // (dev autoMigrate may apply it); duplicates block the op with a row // report. Data-dependent, so it runs HERE, not in the pure differ. await this.applyNullSafeUniquePreflight(entries); + // The declared indexes `expectedIndexes` leaves out because a key column + // will never materialize (a misspelt name, a virtual `formula` field). + // `syncDeclaredIndexes` skips each one at `error` on every sync, and this + // is its plan-side face: a report-only entry, so `os migrate plan` shows + // the unenforced declaration instead of reporting nothing. + entries.push(...diffUnbuildableIndexes({ table: tableName, fields, tenantField, declaredIndexes, physicalColumns })); return entries; } @@ -13866,6 +13875,11 @@ export class SqlDriver implements IDataDriver { * neither `ALTER COLUMN` nor the SQLite table rebuild that column ops do. */ protected async applyIndexDriftOp(op: DriftOp): Promise { + // REPORT ONLY: there is no column to build the index over, so no DDL can + // apply it and the remedy is a metadata edit. Answered before any read, and + // on every dialect, so `applyMigrationEntries` reports it `skipped`. The + // `default` arm below would say the same; this names the reason. + if (op.type === 'unbuildable_index') return false; const physicalColumns = new Set(Object.keys(await this.knex(op.table).columnInfo())); const ensure = (name: string, columns: string[], unique: boolean, nullSafeColumns?: string[]) => this.syncDeclaredIndexes(op.table, [{ name, fields: columns, unique, nullSafeColumns }], physicalColumns); @@ -14555,7 +14569,7 @@ export class SqlDriver implements IDataDriver { // re-resolve it: every caller of this method already holds the value the // registration recorded, and a declared `unique: 'organization'` index // (ADR-0120 D1) must scope against exactly that column. - await this.syncDeclaredIndexes(tableName, [...fromFields, ...declared], physicalColumns, tenantField); + await this.syncDeclaredIndexes(tableName, [...fromFields, ...declared], physicalColumns, tenantField, fields); } /** @@ -14582,8 +14596,17 @@ export class SqlDriver implements IDataDriver { * sync CREATES and what the differ EXPECTS cannot drift apart. * - Idempotent: indexes already present (by deterministic name) are * skipped, and an "already exists" race is absorbed. - * - Indexes referencing a column that wasn't materialized (e.g. a virtual - * `formula` field) are skipped with a warning rather than failing sync. + * - An index referencing a column that is not materialized (a misspelt name + * the Studio save door admits, or a virtual `formula` field) is skipped + * rather than failing the sync. The skip is logged at `error` through + * {@link logDurabilityFailure}; it used to be a `warn`. A declared UNIQUE + * that is never created is exactly the durability-degradation rule's case: + * the constraint is not enforced while everything looks normal. The + * duplicate-row arms below already answer their version of it at `error`. + * A plain index is DDL that was supposed to run and did not, so it takes + * the same channel, and the line says which of the two it is. The drift + * report carries the same index as a report-only `unbuildable_index` entry + * (`diffUnbuildableIndexes`), so `os migrate plan` shows it too. * - A NULL-safe unique whose data already violates it (legacy duplicates the * void constraint admitted, #5030) is NOT created; the failure is logged * at `error` (a declared constraint is not enforced — the @@ -14606,6 +14629,12 @@ export class SqlDriver implements IDataDriver { indexes: DeclaredIndexInput[], physicalColumns: Set, tenantField?: string | null, + /** + * The object's fields, when the caller holds them. They only tell the + * skip line WHY a column is missing (not a field, or a virtual `formula`). + * The drift-op apply paths re-feed already-normalized shapes and pass none. + */ + fields?: Record, ): Promise { const existing = await this.getExistingIndexNames(tableName); const resolvedTenantField = tenantField !== undefined ? tenantField : this.resolveTenantField(tableName); @@ -14618,9 +14647,23 @@ export class SqlDriver implements IDataDriver { const missing = columns.filter((f) => !physicalColumns.has(f)); if (missing.length > 0) { - this.logger.warn( - `[sql-driver] skipping declared index on "${tableName}" — column(s) not materialized: ${missing.join(', ')}`, - { tableName, fields: columns }, + // Durability, not function (AGENTS.md "Degradation log levels"): the + // sync goes on and the object serves normally, while DDL the metadata + // declares did not run. For a UNIQUE index, that means duplicate rows + // are accepted. So this goes on the durability channel, like the + // duplicate-row arms further down this loop. One line per skipped index + // per sync. It states the consequence and the fix, and says whether + // the index was UNIQUE (the prose, and `unique` in the meta). + this.logDurabilityFailure( + `[sql-driver] declared ${unique ? 'UNIQUE ' : ''}index '${name}' on "${tableName}" was NOT created: ` + + `no column for ${describeMissingIndexColumns(missing, fields)}. ` + + (unique + ? `The uniqueness it declares is NOT enforced: duplicate rows are accepted, and the object keeps ` + + `working as if the constraint existed. ` + : `The index does not exist, and the object keeps working as if it did. `) + + `Fix the metadata so every column in the index's fields is a stored field of the object, or ` + + `remove the index ("os validate" refuses a name that is not a field).`, + { tableName, index: name, fields: columns, missing, unique }, ); continue; } diff --git a/packages/lint/src/validate-object-field-refs.test.ts b/packages/lint/src/validate-object-field-refs.test.ts index b4f1c8d13f4..27ebce485cc 100644 --- a/packages/lint/src/validate-object-field-refs.test.ts +++ b/packages/lint/src/validate-object-field-refs.test.ts @@ -603,7 +603,7 @@ describe('validateObjectFieldRefs — indexes[].fields (verbatim physical column }); expect(findings[0]!.message).toContain('"totl" is not a field on object "crm_invoice"'); expect(findings[0]!.message).toContain('Did you mean "total"?'); - expect(findings[0]!.message).toContain('`unique` index is then silently unenforced'); + expect(findings[0]!.message).toContain('`unique` index is then unenforced'); expect(findings[0]!.hint).toContain('Fields on "crm_invoice":'); }); diff --git a/packages/lint/src/validate-object-field-refs.ts b/packages/lint/src/validate-object-field-refs.ts index ac79635fce8..5464b2ce556 100644 --- a/packages/lint/src/validate-object-field-refs.ts +++ b/packages/lint/src/validate-object-field-refs.ts @@ -163,8 +163,11 @@ * still skips the index at sync. `indexes[].fields[]` used to sit in this * list whole, as a storage question for the registration path answered * against the physical column set. That left a MISSPELLING with no door at - * all: the sync's skip is a `warn` and drift drops the index, so nothing - * anywhere refused a name that is not a field. Existence is therefore + * all: the sync's skip was a `warn` and drift dropped the index, so nothing + * anywhere refused a name that is not a field. (Since #20432 step 2 the + * skip is logged at `error` through `logDurabilityFailure` and `os migrate + * plan` reports the unbuildable index, but both still come after the + * authoring doors.) Existence is therefore * judged here, against the authored field map plus the injected columns — * exactly the physical set a correct name can land in — and only the * materialization question stays with the sync (#20432 step 2, the @@ -322,9 +325,9 @@ const LIST_POSITIONS: readonly ListPosition[] = [ */ const INDEX_POSITION = { consequence: - 'The SQL driver skips the WHOLE index at sync with only a warning, and drift drops it too, ' - + 'so `os migrate plan` never reports it: a `unique` index is then silently unenforced ' - + 'while everything looks normal.', + 'The SQL driver skips the WHOLE index at sync. It logs the skip at error and `os migrate plan` ' + + 'reports the index as unbuildable, but the object keeps serving: a `unique` index is then ' + + 'unenforced until the name is fixed.', prescription: 'Fix the column name. An index column is a field of this object, or a column the platform ' + 'injects on it (`created_at`, `organization_id`, …), spelled exactly — never a dotted path.', diff --git a/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts index e7e3ef455cb..185bd19aaac 100644 --- a/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts @@ -327,7 +327,7 @@ export const enMetadataForms: NonNullable = { }, "indexes.fields": { label: "Fields", - helpText: "Column names of this object, in key order (e.g. status, owner). Saving does not check them; publishing and os validate refuse a name that is not a field of this object. A field that is not a stored column (a formula, say) makes the SQL driver skip the whole index, with a warning in the server log." + helpText: "Column names of this object, in key order (e.g. status, owner). Saving does not check them; publishing and os validate refuse a name that is not a field of this object. A field that is not a stored column (a formula, say) makes the SQL driver skip the whole index, with an error in the server log." }, "indexes.unique": { label: "Unique", diff --git a/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts index 4a6d8d03806..0b10b1298da 100644 --- a/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts @@ -327,7 +327,7 @@ export const esESMetadataForms: NonNullable = }, "indexes.fields": { label: "Campos", - helpText: "Nombres de columna de este objeto, en el orden de la clave (p. ej., status, owner). Guardar no los comprueba; publicar y os validate rechazan un nombre que no sea un campo de este objeto. Un campo que no sea una columna almacenada (una fórmula, por ejemplo) hace que el driver SQL omita el índice entero, con una advertencia en el registro del servidor." + helpText: "Nombres de columna de este objeto, en el orden de la clave (p. ej., status, owner). Guardar no los comprueba; publicar y os validate rechazan un nombre que no sea un campo de este objeto. Un campo que no sea una columna almacenada (una fórmula, por ejemplo) hace que el driver SQL omita el índice entero, con un error en el registro del servidor." }, "indexes.unique": { label: "Único", diff --git a/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts index fa3b8141d60..4b165cf354d 100644 --- a/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts @@ -327,7 +327,7 @@ export const jaJPMetadataForms: NonNullable = }, "indexes.fields": { label: "フィールド", - helpText: "このオブジェクトの列名を、キーの順に指定します(例:status、owner)。保存時には検査されませんが、公開時と os validate では、このオブジェクトのフィールドではない名前が拒否されます。保存される列ではないフィールド(数式など)があると、SQL ドライバーはそのインデックス全体をスキップし、サーバーログに警告を出します。" + helpText: "このオブジェクトの列名を、キーの順に指定します(例:status、owner)。保存時には検査されませんが、公開時と os validate では、このオブジェクトのフィールドではない名前が拒否されます。保存される列ではないフィールド(数式など)があると、SQL ドライバーはそのインデックス全体をスキップし、サーバーログにエラーを出します。" }, "indexes.unique": { label: "一意", diff --git a/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts index 0bec2ba3361..85143df7894 100644 --- a/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts @@ -327,7 +327,7 @@ export const zhCNMetadataForms: NonNullable = }, "indexes.fields": { label: "字段", - helpText: "本对象的列名,按键的顺序排列(例如 status、owner)。保存时不会检查它们;发布和 os validate 会拒绝不是本对象字段的名称。若某个字段不是已存储的列(例如公式字段),SQL 驱动会跳过整个索引,并在服务器日志中记录一条警告。" + helpText: "本对象的列名,按键的顺序排列(例如 status、owner)。保存时不会检查它们;发布和 os validate 会拒绝不是本对象字段的名称。若某个字段不是已存储的列(例如公式字段),SQL 驱动会跳过整个索引,并在服务器日志中记录一条错误。" }, "indexes.unique": { label: "唯一", diff --git a/packages/spec/src/data/object.form.ts b/packages/spec/src/data/object.form.ts index 60713e06f3a..0ce8abd4883 100644 --- a/packages/spec/src/data/object.form.ts +++ b/packages/spec/src/data/object.form.ts @@ -509,9 +509,9 @@ export const objectForm = defineForm({ // `os validate` refuse one that is not a field of this object // (`object-field-ref-unknown`, `error`, since #20432). The SQL driver's // `syncDeclaredIndexes` skips an index naming a column the table does - // not have, logging a warning, which is what still befalls a real field - // that is not a stored column (a formula). The help text claims exactly - // those three. + // not have, logging an error (the durability channel, since #20432), + // which is what still befalls a real field that is not a stored column + // (a formula). The help text claims exactly those three. // // `unique` is a select over `global` / `organization` ONLY, as the // ruling says. The node is `boolean | 'global' | 'organization'`: a @@ -528,7 +528,7 @@ export const objectForm = defineForm({ helpText: 'Database indexes on this object\'s table. The SQL driver creates each one the table lacks when it syncs the table; a sync never drops an index.', fields: [ { field: 'name', label: 'Name', type: 'text', helpText: 'Physical index name. Unset: generated from the table and the columns (e.g. idx_task_status).' }, - { field: 'fields', label: 'Fields', widget: 'string-tags', required: true, helpText: 'Column names of this object, in key order (e.g. status, owner). Saving does not check them; publishing and os validate refuse a name that is not a field of this object. A field that is not a stored column (a formula, say) makes the SQL driver skip the whole index, with a warning in the server log.' }, + { field: 'fields', label: 'Fields', widget: 'string-tags', required: true, helpText: 'Column names of this object, in key order (e.g. status, owner). Saving does not check them; publishing and os validate refuse a name that is not a field of this object. A field that is not a stored column (a formula, say) makes the SQL driver skip the whole index, with an error in the server log.' }, { field: 'unique', label: 'Unique', type: 'select', helpText: 'Uniqueness scope (ADR-0120). Unset: not unique. The deprecated bare true (it means global) is not offered; an index that carries it keeps it until you pick a scope.', options: [ { label: 'Global — one holder across the installation, over exactly these columns', value: 'global' }, { label: 'Organization — one holder per organization (the driver prepends the organization column)', value: 'organization' },