diff --git a/.changeset/null-safe-org-unique-driver.md b/.changeset/null-safe-org-unique-driver.md new file mode 100644 index 0000000000..6183f36533 --- /dev/null +++ b/.changeset/null-safe-org-unique-driver.md @@ -0,0 +1,59 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/plugin-auth": patch +--- + +feat(driver-sql)!: organization-scoped uniques are NULL-safe — `COALESCE(organization_id, '__global__')` key part + `unique: 'organization'` on declared indexes (ADR-0120 D3/D4, #5030) + +SQL UNIQUE is NULL-distinct, so the `(organization_id, field)` composite #3696 +introduced enforced **nothing** on rows whose organization is NULL — which on a +single-tenant stack (where the kernel injects the column and never fills it) is +**every row**: field-level `unique: true` was a silent no-op there, measured in +#5030. Per ADR-0120 D3, every organization-scoped unique now materializes its +organization key part as `COALESCE(organization_id, '__global__')`: NULL-organization +rows collapse into one platform bucket, unique among themselves; non-NULL rows +are untouched. Storage stays NULL — the sentinel exists only inside the index +key, and it is the same word the autonumber sequence table already uses +(`GLOBAL_TENANT`), so a constraint-violation error reads as "the platform +bucket collided", not as corrupt data. + +What changes, concretely: + +- **Field-level `unique: true`** (and the new explicit synonym + `'organization'`) on a tenant-scoped object → composite + `(COALESCE(tenantField, '__global__'), field)`. `unique: 'global'` and + tenant-less objects are unchanged. +- **Declared indexes gain the ADR-0120 D1 scope vocabulary at the driver**: + `unique: 'organization'` prepends the NULL-safe organization key part to the + listed columns (degrading to the listed columns on a tenant-less object; a + listed tenant column is made NULL-safe in place instead — the S6 respelling). + `unique: true` / `'global'` on a declared index stays **verbatim** — the + #3696 contract, now the `'global'` arm; the nine engine dedup/idempotency + keys keep their exact physical shape. (The spec/lint side of the vocabulary + lands separately via #4986; the driver deliberately merges first.) +- **Drift detection reads both sides through one normalization** + (the #4884 discipline, extended to the tenant key part): the physical + `COALESCE(organization_id, )` form is attributed to the column, + compared **literal-agnostically**, and recognised as the sync's own + vocabulary — a healthy database reports zero drift on every dialect. +- **Existing bare composites migrate through the ceremony (ADR-0120 D4)**: + `(organization_id, X) → (COALESCE(organization_id, '__global__'), X)` + surfaces as a `recreate_index` drift op — a pure tightening — gated by a + **duplicate pre-flight probe**. Clean probe → the op grades `safe` and dev + `autoMigrate: 'safe'` / a plain `os migrate apply` applies it. Duplicates + (data the void constraint wrongly admitted) → the op is **blocked** with a + per-group row report, the old index stays in place, and apply re-probes so + even `--allow-destructive` cannot drop a constraint whose replacement is not + creatable. Deduplicate, re-plan, apply. +- **`'__global__'` is reserved at the organization-minting seam** + (plugin-auth): an organization whose id or slug equals the sentinel is + rejected at creation with a prescriptive error (ADR-0120 D3 guardrail). + +Migration note for operators: on databases with pre-existing +organization-composite uniques, the first `os migrate plan` after upgrading +shows one `recreate_index` per affected index. On healthy data it auto-applies +in dev and is a no-op content-wise; a blocked op means the #5030 defect +admitted real duplicate rows — resolve the listed rows first. MySQL < 8.0.13 / +MariaDB cannot express the functional key part: the driver degrades to the +bare composite, says exactly what is not enforced at `error` level, and keeps +reporting the tightening as drift for after the server upgrade. diff --git a/packages/plugins/driver-sql/src/index.ts b/packages/plugins/driver-sql/src/index.ts index 7f6d65fedb..80e662d8ad 100644 --- a/packages/plugins/driver-sql/src/index.ts +++ b/packages/plugins/driver-sql/src/index.ts @@ -34,8 +34,14 @@ export { parseIndexDdl, uniqueIndexesFromFields, INDEX_DRIFT_OPS, + // Unique-scope vocabulary + NULL-safe organization key part (ADR-0120 D1/D3) + GLOBAL_TENANT, + isUniqueScopeDeclared, + isOrganizationScopedUnique, + organizationKeyPartSql, } from './schema-drift.js'; export type { + DeclaredIndexInput, ManagedDriftEntry, DriftOp, DriftCategory, diff --git a/packages/plugins/driver-sql/src/schema-drift.ts b/packages/plugins/driver-sql/src/schema-drift.ts index a9f2819798..6a8bbb1ec5 100644 --- a/packages/plugins/driver-sql/src/schema-drift.ts +++ b/packages/plugins/driver-sql/src/schema-drift.ts @@ -30,9 +30,68 @@ import { createHash } from 'node:crypto'; -import { isAppResolvedDefaultToken, isGlobalUnique, isUniqueDeclared } from '@objectstack/spec/data'; +import { isAppResolvedDefaultToken, isUniqueDeclared } from '@objectstack/spec/data'; import type { SchemaDiffEntry } from '@objectstack/spec/shared'; +// ─────────────────────────────────────────────────────────────────────── +// Unique-scope vocabulary (ADR-0120) +// ─────────────────────────────────────────────────────────────────────── + +/** + * Sentinel naming the NULL-organization ("platform") bucket — ADR-0120 D3. + * + * Every organization-scoped unique index materializes its organization key + * part as `COALESCE(organization_id, '__global__')` instead of the raw column: + * SQL UNIQUE is NULL-distinct, so the raw column enforced NOTHING on rows + * whose organization is NULL — which is every row on a single-tenant stack + * (#5030). The COALESCE folds all NULL-organization rows into one bucket, + * unique among themselves, without touching the other rows. + * + * Three invariants, all deliberate (ADR-0120 D3, maintainer-resolved): + * - **Storage stays NULL.** Only the index folds NULL into the bucket; a + * `WHERE organization_id = '__global__'` matches nothing, by design. + * - **The word is the platform's existing name for this bucket** — the + * autonumber sequence table keys global rows by the same sentinel + * (`SqlDriver`'s `GLOBAL_TENANT`), so a constraint-violation error reading + * `(__global__, a@b.com)` says "platform bucket", not "corrupt data". + * - **The token is reserved**: an organization id may never equal it + * (guarded at the organization-creation seam in plugin-auth). + */ +export const GLOBAL_TENANT = '__global__'; + +/** + * Driver-side unique-scope vocabulary — ADR-0120 D1. + * + * `'organization'` is accepted here AHEAD of the spec schema: #4986 lands the + * spec/lint token separately, and the merge order is deliberately driver first + * so spec-side acceptance never outruns driver-side enforcement. Until then, + * spec's `isUniqueDeclared` / `isGlobalUnique` know nothing of + * `'organization'`, so these wrappers are the single judgment point inside + * driver-sql — every scope decision in this package reads them, never the + * spec helpers directly. + */ +export function isUniqueScopeDeclared(unique: unknown): boolean { + return unique === 'organization' || isUniqueDeclared(unique); +} + +/** + * The organization-scoped spellings: field-level `true` (unchanged since + * #3696) and the explicit `'organization'` synonym (ADR-0120 D1) — on either + * spelling (field-level `unique` or a declared index's `unique`). + */ +export function isOrganizationScopedUnique(unique: unknown): boolean { + return unique === true || unique === 'organization'; +} + +/** + * The organization key part of an organization-scoped unique index, spelled + * once (ADR-0120 D3). Display/signature form — DDL emission quotes the + * identifier per dialect in `SqlDriver.syncDeclaredIndexes`. + */ +export function organizationKeyPartSql(column: string): string { + return `COALESCE(${column}, '${GLOBAL_TENANT}')`; +} + export type SqlDialectName = 'sqlite' | 'postgres' | 'mysql' | 'unknown'; export type DriftCategory = 'safe' | 'needs_confirm' | 'destructive'; @@ -77,6 +136,8 @@ export type DriftOp = dropIndexNames: string[]; createIndexName: string; createColumns: string[]; + /** Columns whose key part is the NULL-safe organization form (ADR-0120 D3). */ + nullSafeColumns?: string[]; } /** Materialize a declared index that has no physical counterpart. */ | { @@ -86,6 +147,8 @@ export type DriftOp = indexName: string; columns: string[]; unique: boolean; + /** Columns whose key part is the NULL-safe organization form (ADR-0120 D3). */ + nullSafeColumns?: string[]; } /** Drop an index ObjectStack generated that metadata no longer declares. */ | { type: 'drop_index'; table: string; column?: string; indexName: string } @@ -101,6 +164,18 @@ export type DriftOp = indexName: string; columns: string[]; unique: boolean; + /** Columns whose key part is the NULL-safe organization form (ADR-0120 D3). */ + nullSafeColumns?: string[]; + /** + * ADR-0120 D4: the divergence is EXACTLY the bare organization column + * tightening into its NULL-safe COALESCE form — same column identities, + * same uniqueness, physical index fully plain. This is the one recreate + * whose danger is data-dependent rather than structural, so it goes + * through the duplicate pre-flight probe: clean → `safe` (dev + * `autoMigrate: 'safe'` may apply it), duplicates found → blocked with a + * row report, old index left in place. + */ + tightenNullSafeOnly?: boolean; }; /** @@ -494,6 +569,14 @@ export interface ExpectedIndex { name: string; columns: string[]; unique: boolean; + /** + * Columns (by identity, always a subset of {@link columns}) whose key part + * materializes as the NULL-safe organization form + * `COALESCE(, '__global__')` rather than the bare column + * (ADR-0120 D3). Practically this is the table's tenant column on every + * organization-scoped unique. Absent/empty for plain indexes. + */ + nullSafeColumns?: string[]; } // ─────────────────────────────────────────────────────────────────────── @@ -515,8 +598,16 @@ export type IndexKeyPart = | { 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; +/** + * A literal any dialect might print inside `COALESCE`, optional `::type` cast + * included. MySQL's `information_schema.STATISTICS.EXPRESSION` decorates a + * string literal with a charset introducer and backslash-escaped quotes + * (`_utf8mb4\'__global__\'`), so both decorations are accepted too — the + * attribution is deliberately literal-AGNOSTIC (#4884, ADR-0120 D3): what the + * literal says never changes which column the key part pins. + */ +const SQL_LITERAL = + /^(?:(?:_[A-Za-z][A-Za-z0-9]*\s*)?\\?'(?:[^'\\]|''|\\.)*\\?'|-?\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 { @@ -695,7 +786,14 @@ export function applyIndexKeyParts(index: PhysicalIndex, rawParts: string[]): vo continue; } (index.expressions ??= []).push(part.sql); - if (part.column !== null) index.columns.push(part.column); + if (part.column !== null) { + index.columns.push(part.column); + // An attributable expression is by construction the COALESCE(col, + // ) form — record the column so the differ can compare the key + // part's FORM, not just its identity (ADR-0120 D3: the NULL-safe + // organization key part vs the bare column are different constraints). + (index.nullSafeColumns ??= []).push(part.column); + } } } @@ -727,6 +825,15 @@ export interface PhysicalIndex { * {@link isSyncReproducibleIndex}. */ expressions?: string[]; + /** + * The columns that expression key parts ATTRIBUTE to — i.e. every key part + * of the recognised `COALESCE(col, )` form contributes its column + * here (and to {@link columns}). Lets the differ tell the NULL-safe + * organization key part (ADR-0120 D3) apart from the bare column while + * staying literal-agnostic. A subset of {@link columns}; absent when every + * key part is a plain column. + */ + nullSafeColumns?: string[]; } /** @@ -748,9 +855,23 @@ export interface PhysicalIndex { * 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. + * + * Since ADR-0120 D3 the sync's own vocabulary includes ONE expression shape: + * the NULL-safe organization key part `COALESCE(, '__global__')`. + * An index whose every expression is that form — attributed to the table's + * OWN tenant column — is therefore reproducible again (pass `tenantField`). + * The attribution is deliberately column-scoped: `COALESCE(package_id, '')` + * (the ADR-0048 overlay key) attributes to a non-tenant column and stays out, + * exactly as before — loosening this to "any attributable COALESCE" would + * resurrect the #4884 false-orphan on the overlay indexes. */ -export function isSyncReproducibleIndex(index: PhysicalIndex): boolean { - return index.partial !== true && (index.expressions?.length ?? 0) === 0; +export function isSyncReproducibleIndex(index: PhysicalIndex, tenantField?: string | null): boolean { + if (index.partial === true) return false; + const expressions = index.expressions?.length ?? 0; + if (expressions === 0) return true; + if (!tenantField) return false; + const nullSafe = index.nullSafeColumns ?? []; + return nullSafe.length === expressions && nullSafe.every((c) => c === tenantField); } /** @@ -768,8 +889,9 @@ export function isSyncReproducibleIndex(index: PhysicalIndex): boolean { export function isRuntimeManagedIndex( index: PhysicalIndex, runtimeCreated?: ReadonlySet, + tenantField?: string | null, ): boolean { - return runtimeCreated?.has(index.name) === true || !isSyncReproducibleIndex(index); + return runtimeCreated?.has(index.name) === true || !isSyncReproducibleIndex(index, tenantField); } /** @@ -780,13 +902,18 @@ export function isRuntimeManagedIndex( * create-table, alter-table, SQLite-rebuild and drift-detection paths cannot * disagree about what a `unique: true` field is supposed to produce. * - * Scoping rule: + * Scoping rule (ADR-0120 D1/D3): * - `unique: 'global'` → single-column `(field)`, platform-wide. - * - `unique: true` on a tenant-scoped table → composite `(tenantField, - * field)`: unique *within* the tenant, matching the per-tenant autonumber - * sequence, the RLS read predicate and the write-path tenant stamp. - * - `unique: true` with no tenant column → single-column `(field)`. - * Single-tenant deployments therefore see byte-identical DDL to before. + * - `unique: true` / `unique: 'organization'` on a tenant-scoped table → + * composite `(COALESCE(tenantField, '__global__'), field)`: unique + * *within* the organization, matching the per-tenant autonumber sequence, + * the RLS read predicate and the write-path tenant stamp. The organization + * key part is NULL-safe: rows without an organization form one platform + * bucket, unique among themselves — a bare `(tenantField, field)` under + * SQL's NULL-distinct UNIQUE enforced NOTHING on those rows, which on a + * single-tenant stack is every row (#5030). + * - `unique: true` / `'organization'` with no tenant column → single-column + * `(field)`. * * The tenant column comes FIRST in the composite so the index also serves the * `WHERE tenant = ?` prefix scans every tenant-scoped read issues. @@ -798,37 +925,82 @@ export function uniqueIndexesFromFields( ): ExpectedIndex[] { const out: ExpectedIndex[] = []; for (const [name, field] of Object.entries(fields ?? {})) { - if (!isUniqueDeclared(field?.unique)) continue; + if (!isUniqueScopeDeclared(field?.unique)) continue; // A unique declaration ON the tenant column itself ("one row per tenant") // cannot be tenant-scoped — `(organization_id, organization_id)` is not a // constraint. Keep it single-column. - const scoped = !isGlobalUnique(field.unique) && tenantField != null && tenantField !== name; + const scoped = + isOrganizationScopedUnique(field.unique) && tenantField != null && tenantField !== name; const columns = scoped ? [tenantField, name] : [name]; - out.push({ name: buildIndexName(table, columns, true), columns, unique: true }); + out.push({ + name: buildIndexName(table, columns, true), + columns, + unique: true, + ...(scoped ? { nullSafeColumns: [tenantField] } : {}), + }); } return out; } -/** Normalize one entry of an object's declared `indexes[]`, or null if unusable. */ +/** The declared-index shape this module normalizes (spec's `IndexSchema` + the driver-side extras). */ +export interface DeclaredIndexInput { + name?: string; + fields?: string[]; + /** `'organization'` is the ADR-0120 D1 explicit per-organization scope; see {@link isUniqueScopeDeclared}. */ + unique?: boolean | 'global' | 'organization'; + /** Pre-resolved NULL-safe key parts — used by the drift-op apply path, which re-feeds already-normalized shapes. */ + nullSafeColumns?: string[]; +} + +/** + * Normalize one entry of an object's declared `indexes[]`, or null if unusable. + * + * Scope handling (ADR-0120 D1/D3): + * - `unique: true` / `'global'` → the listed columns, VERBATIM — the #3696 + * contract, now the `'global'` arm of the explicit vocabulary. + * - `unique: 'organization'` → the organization key part is PREPENDED to the + * listed columns in its NULL-safe form (`COALESCE(tenantField, + * '__global__')`), resolved against the table's tenant column at + * registration — the one place tenancy is knowable. With no tenant column + * the index degrades to the listed columns alone, mirroring field-level + * behavior (S11). A listed column that IS the tenant column is not + * prepended again — its own key part becomes the NULL-safe form instead + * (the hand-written S6 spelling, opted in). + */ export function normalizeDeclaredIndex( table: string, - idx: { name?: string; fields?: string[]; unique?: boolean | 'global' } | undefined, + idx: DeclaredIndexInput | undefined, + tenantField?: string | null, ): ExpectedIndex | null { - const columns = Array.isArray(idx?.fields) + const listed = Array.isArray(idx?.fields) ? idx.fields.filter((f): f is string => typeof f === 'string' && f.length > 0) : []; - if (columns.length === 0) return null; - const unique = isUniqueDeclared(idx?.unique); + if (listed.length === 0) return null; + const unique = isUniqueScopeDeclared(idx?.unique); + + let columns = listed; + let nullSafeColumns: string[] | undefined; + if (Array.isArray(idx?.nullSafeColumns) && idx.nullSafeColumns.length > 0) { + // Already-normalized shape (drift-op apply path) — honour it verbatim. + nullSafeColumns = idx.nullSafeColumns.filter((c) => listed.includes(c)); + if (nullSafeColumns.length === 0) nullSafeColumns = undefined; + } else if (idx?.unique === 'organization' && tenantField) { + columns = listed.includes(tenantField) ? listed : [tenantField, ...listed]; + nullSafeColumns = [tenantField]; + } + const name = typeof idx?.name === 'string' && idx.name.trim() ? idx.name.trim() : buildIndexName(table, columns, unique); - return { name, columns, unique }; + return { name, columns, unique, ...(nullSafeColumns ? { nullSafeColumns } : {}) }; } /** * The full index set metadata asks for on a table: field-level `unique` - * (tenancy-aware) plus the object's declared `indexes[]`, taken verbatim. + * (tenancy-aware) plus the object's declared `indexes[]` — `'global'`/bare + * `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 — @@ -839,13 +1011,13 @@ export function expectedIndexes(args: { table: string; fields: Record; tenantField: string | null; - declaredIndexes?: Array<{ name?: string; fields?: string[]; unique?: boolean | 'global' }>; + declaredIndexes?: DeclaredIndexInput[]; physicalColumns: Set; }): ExpectedIndex[] { const { table, fields, tenantField, declaredIndexes, physicalColumns } = args; const out = uniqueIndexesFromFields(table, fields, tenantField); for (const idx of Array.isArray(declaredIndexes) ? declaredIndexes : []) { - const norm = normalizeDeclaredIndex(table, idx); + const norm = normalizeDeclaredIndex(table, idx, tenantField); if (norm) out.push(norm); } return out.filter((i) => i.columns.every((c) => physicalColumns.has(c))); @@ -895,7 +1067,7 @@ export function legacyUniqueReplacements(args: { fields: Record; tenantField: string | null; physicalColumns: Set; - declaredIndexes?: Array<{ name?: string; fields?: string[]; unique?: boolean | 'global' }>; + declaredIndexes?: DeclaredIndexInput[]; }): LegacyUniqueReplacement[] { const { table, fields, tenantField, physicalColumns, declaredIndexes } = args; if (!tenantField) return []; // Nothing was ever mis-scoped on a tenant-less table. @@ -907,13 +1079,13 @@ export function legacyUniqueReplacements(args: { // declared index is named" is answered once, not guessed at twice. const declaredNames = new Set( (Array.isArray(declaredIndexes) ? declaredIndexes : []) - .map((idx) => normalizeDeclaredIndex(table, idx)?.name) + .map((idx) => normalizeDeclaredIndex(table, idx, tenantField)?.name) .filter((n): n is string => typeof n === 'string'), ); const out: LegacyUniqueReplacement[] = []; for (const [name, field] of Object.entries(fields ?? {})) { - if (!isUniqueDeclared(field?.unique)) continue; - if (isGlobalUnique(field.unique)) continue; + if (!isUniqueScopeDeclared(field?.unique)) continue; + if (!isOrganizationScopedUnique(field.unique)) continue; if (name === tenantField || !physicalColumns.has(name)) continue; const legacyNames = legacyUniqueIndexNames(table, name).filter((n) => !declaredNames.has(n)); if (legacyNames.length === 0) continue; @@ -921,7 +1093,16 @@ export function legacyUniqueReplacements(args: { out.push({ column: name, legacyNames, - replacement: { name: buildIndexName(table, columns, true), columns, unique: true }, + // NULL-safe organization key part (ADR-0120 D3). Still a pure + // relaxation to create from under the legacy GLOBAL single-column + // unique: any two rows colliding in the new key already collided in the + // old one, so the replacement can neither fail nor lose data. + replacement: { + name: buildIndexName(table, columns, true), + columns, + unique: true, + nullSafeColumns: [tenantField], + }, }); } return out; @@ -954,9 +1135,32 @@ export function isManagedIndexName(table: string, index: PhysicalIndex): boolean return unique && columns.length === 1 && name === `${table}_${columns[0]}_unique`; } -/** `(a, b)` UNIQUE vs `(a, b)` — the identity a physical index is compared on. */ -function indexSignature(columns: string[], unique: boolean): string { - return `${unique ? 'UNIQUE ' : ''}(${columns.join(', ')})`; +/** + * `UNIQUE (a, b)` vs `(a, b)` — the display form of an index definition. A + * NULL-safe organization key part (ADR-0120 D3) renders as its COALESCE form + * so plan/warn messages describe the constraint that will actually exist. + */ +function indexSignature( + columns: string[], + unique: boolean, + nullSafeColumns?: ReadonlyArray | null, +): string { + const ns = new Set(nullSafeColumns ?? []); + const parts = columns.map((c) => (ns.has(c) ? organizationKeyPartSql(c) : c)); + return `${unique ? 'UNIQUE ' : ''}(${parts.join(', ')})`; +} + +/** + * Comparison identity of an index key — the SAME normalization for the + * expected and the physical side (ADR-0120 D3; the #4884 lesson). A key + * part's FORM matters (`COALESCE(organization_id, …)` is a different + * constraint from bare `organization_id`), but the COALESCE literal does not: + * any `COALESCE(col, )` folds NULL into one bucket, so two spellings + * of the literal are the same constraint and must not read as drift. + */ +function canonicalIndexKey(columns: string[], nullSafeColumns?: ReadonlyArray | null): string { + const ns = new Set(nullSafeColumns ?? []); + return columns.map((c) => (ns.has(c) ? `coalesce:${c}` : c)).join(','); } /** @@ -978,8 +1182,14 @@ export function diffManagedIndexes(args: { * {@link isRuntimeManagedIndex}. */ runtimeCreated?: ReadonlySet; + /** + * The table's tenant column, when it has one. Lets the differ recognise the + * NULL-safe organization key part as the sync's OWN vocabulary + * (ADR-0120 D3) — see {@link isSyncReproducibleIndex}. + */ + tenantField?: string | null; }): ManagedDriftEntry[] { - const { table, expected, legacy, physical, runtimeCreated } = args; + const { table, expected, legacy, physical, runtimeCreated, tenantField } = args; const out: ManagedDriftEntry[] = []; const byName = new Map(physical.map((p) => [p.name, p])); /** Physical index names accounted for — either declared, or already reported. */ @@ -992,7 +1202,7 @@ export function diffManagedIndexes(args: { // collide with the legacy spelling be dropped. const present = l.legacyNames.filter((n) => { const p = byName.get(n); - if (!p || p.primary || isRuntimeManagedIndex(p, runtimeCreated)) return false; + if (!p || p.primary || isRuntimeManagedIndex(p, runtimeCreated, tenantField)) return false; return p.unique && p.columns.length === 1 && p.columns[0] === l.column; }); if (present.length === 0) continue; @@ -1002,7 +1212,7 @@ export function diffManagedIndexes(args: { remoteName: table, table, column: l.column, - expected: indexSignature(l.replacement.columns, true), + expected: indexSignature(l.replacement.columns, true, l.replacement.nullSafeColumns), actual: indexSignature([l.column], true), severity: 'warning', category: 'safe', @@ -1013,11 +1223,12 @@ export function diffManagedIndexes(args: { dropIndexNames: present, createIndexName: l.replacement.name, createColumns: l.replacement.columns, + ...(l.replacement.nullSafeColumns ? { nullSafeColumns: l.replacement.nullSafeColumns } : {}), }, message: `${table}.${l.column}: a legacy platform-wide UNIQUE index (${present.join(', ')}) still enforces ` + `uniqueness across ALL tenants, but metadata scopes it per '${l.replacement.columns[0]}' — a second ` + - `tenant reusing the value is rejected on insert (#3696). Replacing it with ${indexSignature(l.replacement.columns, true)} ` + + `tenant reusing the value is rejected on insert (#3696). Replacing it with ${indexSignature(l.replacement.columns, true, l.replacement.nullSafeColumns)} ` + `is a pure relaxation: run "os migrate apply".`, }); } @@ -1032,7 +1243,7 @@ export function diffManagedIndexes(args: { remoteName: table, table, column: e.columns[0], - expected: indexSignature(e.columns, e.unique), + expected: indexSignature(e.columns, e.unique, e.nullSafeColumns), actual: '(absent)', severity: 'warning', category: 'safe', @@ -1043,14 +1254,22 @@ export function diffManagedIndexes(args: { indexName: e.name, columns: e.columns, unique: e.unique, + ...(e.nullSafeColumns ? { nullSafeColumns: e.nullSafeColumns } : {}), }, message: - `${table}: metadata declares index '${e.name}' ${indexSignature(e.columns, e.unique)} but the database ` + + `${table}: metadata declares index '${e.name}' ${indexSignature(e.columns, e.unique, e.nullSafeColumns)} but the database ` + `has no such index — run "os migrate apply" to create it.`, }); continue; } - if (p.unique === e.unique && p.columns.join(',') === e.columns.join(',')) continue; + // Same normalization on BOTH sides (#4884, ADR-0120 D3): column identity + // AND key-part form, literal-agnostic on the COALESCE literal. + if ( + p.unique === e.unique && + canonicalIndexKey(p.columns, p.nullSafeColumns) === canonicalIndexKey(e.columns, e.nullSafeColumns) + ) { + 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 @@ -1059,18 +1278,32 @@ export function diffManagedIndexes(args: { // 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; + if (isRuntimeManagedIndex(p, runtimeCreated, tenantField)) continue; // Same name, different definition. `syncDeclaredIndexes` skips by name, so // this never self-heals: it has to be dropped and rebuilt. Tightening to // UNIQUE is destructive — the CREATE can fail on existing duplicates, and // by then the old index is already gone. + // + // ADR-0120 D4: ONE redefinition is data-dependent rather than structural — + // the bare organization composite tightening into its NULL-safe COALESCE + // form (same identities, same uniqueness, physical fully plain). It is + // marked so the driver can run the duplicate pre-flight probe on it: + // clean → recategorised `safe` (dev autoMigrate may apply); duplicates → + // blocked with a row report, the old index left in place. + const tightenNullSafeOnly = + e.unique && + p.unique && + (e.nullSafeColumns?.length ?? 0) > 0 && + (p.expressions?.length ?? 0) === 0 && + p.partial !== true && + p.columns.join(',') === e.columns.join(','); out.push({ kind: 'index_mismatch', remoteName: table, table, column: e.columns[0], - expected: indexSignature(e.columns, e.unique), - actual: indexSignature(p.columns, p.unique), + expected: indexSignature(e.columns, e.unique, e.nullSafeColumns), + actual: indexSignature(p.columns, p.unique, p.nullSafeColumns), severity: e.unique ? 'error' : 'warning', category: e.unique ? 'destructive' : 'needs_confirm', op: { @@ -1080,13 +1313,19 @@ export function diffManagedIndexes(args: { indexName: e.name, columns: e.columns, unique: e.unique, + ...(e.nullSafeColumns ? { nullSafeColumns: e.nullSafeColumns } : {}), + ...(tightenNullSafeOnly ? { tightenNullSafeOnly: true } : {}), }, - message: - `${table}: index '${e.name}' is ${indexSignature(p.columns, p.unique)} but metadata declares ` + - `${indexSignature(e.columns, e.unique)} — the additive sync skips it by name, so it must be rebuilt` + - (e.unique - ? `. Creating the UNIQUE index can fail on existing duplicates: "os migrate apply --allow-destructive".` - : ` via "os migrate apply".`), + message: tightenNullSafeOnly + ? `${table}: index '${e.name}' is ${indexSignature(p.columns, p.unique, p.nullSafeColumns)} but metadata declares ` + + `${indexSignature(e.columns, e.unique, e.nullSafeColumns)} (ADR-0120 D3: the organization key part is NULL-safe, ` + + `so rows without an organization are constrained too). Pure tightening — eligibility is decided by the ` + + `duplicate pre-flight probe.` + : `${table}: index '${e.name}' is ${indexSignature(p.columns, p.unique, p.nullSafeColumns)} but metadata declares ` + + `${indexSignature(e.columns, e.unique, e.nullSafeColumns)} — the additive sync skips it by name, so it must be rebuilt` + + (e.unique + ? `. Creating the UNIQUE index can fail on existing duplicates: "os migrate apply --allow-destructive".` + : ` via "os migrate apply".`), }); } @@ -1100,19 +1339,19 @@ export function diffManagedIndexes(args: { // 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; + if (isRuntimeManagedIndex(p, runtimeCreated, tenantField)) continue; out.push({ kind: 'unmapped_index', remoteName: table, table, column: p.columns[0], expected: '(absent)', - actual: indexSignature(p.columns, p.unique), + actual: indexSignature(p.columns, p.unique, p.nullSafeColumns), severity: 'warning', category: 'destructive', op: { type: 'drop_index', table, column: p.columns[0], indexName: p.name }, message: - `${table}: index '${p.name}' ${indexSignature(p.columns, p.unique)} carries ObjectStack's generated naming ` + + `${table}: index '${p.name}' ${indexSignature(p.columns, p.unique, p.nullSafeColumns)} carries ObjectStack's generated naming ` + `but matches no declared index (orphaned) — "os migrate apply --allow-destructive" to drop it.`, }); } diff --git a/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts b/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts index e1587ea931..95fdc155af 100644 --- a/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts @@ -1,7 +1,13 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, afterEach, vi } from 'vitest'; -import { SqlDriver, diffManagedIndexes, isManagedIndexName } from '../src/index.js'; +import { + SqlDriver, + classifyIndexKeyPart, + diffManagedIndexes, + isManagedIndexName, + parseIndexDdl, +} from '../src/index.js'; /** * Index-dimension managed-schema drift (#3728). @@ -65,13 +71,37 @@ describe('SqlDriver index drift (#3728)', () => { }, ]; + /** + * Unique index name → canonical key parts, read from the index DDL so + * expression keys are visible (`PRAGMA index_info` reports a NULL name for + * them). A plain column reads as its name; the NULL-safe organization key + * part (ADR-0120 D3) reads as `COALESCE()` — literal elided, the + * same literal-agnostic identity the drift differ compares on. + */ const uniqueIndexColumns = async (table: string): Promise> => { const list: any = await knexInstance.raw(`PRAGMA index_list(${table})`); + const master: any = await knexInstance.raw( + `SELECT name, sql FROM sqlite_master WHERE type = 'index' AND tbl_name = ?`, + [table], + ); + const ddlByName = new Map(); + for (const r of Array.isArray(master) ? master : (master?.rows ?? [])) { + if (typeof r?.sql === 'string' && r.sql) ddlByName.set(r.name, r.sql); + } const out: Record = {}; for (const idx of list) { if (idx.origin === 'pk' || idx.unique !== 1) continue; - const info: any = await knexInstance.raw(`PRAGMA index_info("${idx.name}")`); - out[idx.name] = info.map((c: any) => c.name); + const parsed = parseIndexDdl(ddlByName.get(idx.name) ?? ''); + if (parsed) { + out[idx.name] = parsed.keyParts.map((p) => { + const part = classifyIndexKeyPart(p); + if (part.kind === 'column') return part.column; + return part.column === null ? p : `COALESCE(${part.column})`; + }); + } else { + const info: any = await knexInstance.raw(`PRAGMA index_info("${idx.name}")`); + out[idx.name] = info.map((c: any) => c.name); + } } return out; }; @@ -185,7 +215,7 @@ describe('SqlDriver index drift (#3728)', () => { // Both are current intent: the tenant composite from the field-level // `unique: true`, and the verbatim declared global unique. const uniques = await uniqueIndexColumns('hp_contact'); - expect(uniques['uniq_hp_contact_organization_id_email']).toEqual(['organization_id', 'email']); + expect(uniques['uniq_hp_contact_organization_id_email']).toEqual(['COALESCE(organization_id)', 'email']); expect(uniques['uniq_hp_contact_email']).toEqual(['email']); // Before the fix this reported `replace_unique_index` — proposing to drop @@ -260,7 +290,7 @@ describe('SqlDriver index drift (#3728)', () => { const uniques = await uniqueIndexColumns('product'); expect(uniques['product_code_unique']).toBeUndefined(); - expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); + expect(Object.values(uniques)).toContainEqual(['COALESCE(organization_id)', 'code']); // Existing rows survived, and the cross-tenant insert the issue is about works. expect(await driver.count('product', { object: 'product' })).toBe(2); @@ -281,7 +311,7 @@ describe('SqlDriver index drift (#3728)', () => { const again = await driver.applyMigrationEntries(drift, { allowDestructive: false }); expect(again.skipped).toHaveLength(0); expect(Object.values(await uniqueIndexColumns('product'))).toContainEqual([ - 'organization_id', + 'COALESCE(organization_id)', 'code', ]); }); @@ -298,7 +328,7 @@ describe('SqlDriver index drift (#3728)', () => { const uniques = await uniqueIndexColumns('product'); expect(uniques['product_code_unique']).toBeUndefined(); - expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); + expect(Object.values(uniques)).toContainEqual(['COALESCE(organization_id)', 'code']); expect(await driver.detectManagedDrift()).toHaveLength(0); const b = await driver.create('product', { organization_id: 'org_b', code: 'PROD-00001' }); 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 index e57c668a1b..716bc5dab1 100644 --- 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 @@ -357,4 +357,91 @@ describe('overlay index drift on a fresh database (#4884)', () => { expect(index.expressions).toEqual(["COALESCE(package_id, '')", 'lower(name)']); expect(isSyncReproducibleIndex(index)).toBe(false); }); + + // ── ADR-0120 D3: the NULL-safe organization key part in the drift reader ── + + describe('NULL-safe organization key part attribution (ADR-0120 D3)', () => { + it("attributes COALESCE(organization_id, '__global__') to organization_id in every dialect spelling", () => { + for (const sql of [ + // SQLite: `sqlite_master.sql`, as the sync wrote it. + "COALESCE(organization_id, '__global__')", + 'COALESCE("organization_id", \'__global__\')', + // Postgres: `pg_get_indexdef` casts a varchar key part. + "COALESCE((organization_id)::text, '__global__'::text)", + // MySQL: `information_schema.STATISTICS.EXPRESSION` decorates the + // literal with a charset introducer and backslash-escaped quotes. + "coalesce(`organization_id`,_utf8mb4\\'__global__\\')", + ]) { + expect(classifyIndexKeyPart(sql)).toEqual({ + kind: 'expression', + sql, + column: 'organization_id', + }); + } + }); + + it('applyIndexKeyParts records the attributed key part as a NULL-safe column', () => { + const index: PhysicalIndex = { name: 'i', columns: [], unique: true }; + applyIndexKeyParts(index, ["COALESCE(organization_id, '__global__')", 'email']); + expect(index.columns).toEqual(['organization_id', 'email']); + expect(index.nullSafeColumns).toEqual(['organization_id']); + }); + + it("isSyncReproducibleIndex: the org key part is the sync's OWN vocabulary — for the tenant column only", () => { + const org: PhysicalIndex = { name: 'i', columns: [], unique: true }; + applyIndexKeyParts(org, ["COALESCE(organization_id, '__global__')", 'email']); + expect(isSyncReproducibleIndex(org, 'organization_id')).toBe(true); + expect(isSyncReproducibleIndex(org, null)).toBe(false); + expect(isSyncReproducibleIndex(org)).toBe(false); + // The ADR-0048 overlay key attributes to a NON-tenant column and stays + // out — loosening the column scoping to "any attributable COALESCE" + // would resurrect the #4884 false orphan on the overlay indexes. + const overlay: PhysicalIndex = { name: 'o', columns: [], unique: true }; + applyIndexKeyParts(overlay, ['type', "COALESCE(package_id, '')"]); + expect(isSyncReproducibleIndex(overlay, 'organization_id')).toBe(false); + }); + + it('the differ compares key-part FORM literal-agnostically: COALESCE ≡ COALESCE, bare ≠ COALESCE', () => { + const expected = [ + { + name: 'uniq_t_organization_id_email', + columns: ['organization_id', 'email'], + unique: true, + nullSafeColumns: ['organization_id'], + }, + ]; + // Physical carries the COALESCE form with a DIFFERENT literal: any + // literal folds NULL into one bucket, so it is the same constraint — + // zero drift (the #4884 lesson applied to the tenant key part). + const physSame: PhysicalIndex = { name: 'uniq_t_organization_id_email', columns: [], unique: true }; + applyIndexKeyParts(physSame, ["COALESCE(organization_id, '')", 'email']); + expect( + diffManagedIndexes({ + table: 't', + expected, + legacy: [], + physical: [physSame], + tenantField: 'organization_id', + }), + ).toEqual([]); + // Physical is the bare NULL-distinct composite: a DIFFERENT constraint + // (void on NULL-organization rows, #5030) → the D4 tightening op. + const physBare: PhysicalIndex = { + name: 'uniq_t_organization_id_email', + columns: ['organization_id', 'email'], + unique: true, + }; + const out = diffManagedIndexes({ + table: 't', + expected, + legacy: [], + physical: [physBare], + tenantField: 'organization_id', + }); + expect(out).toHaveLength(1); + expect(out[0].op).toMatchObject({ type: 'recreate_index', tightenNullSafeOnly: true }); + expect(out[0].expected).toBe("UNIQUE (COALESCE(organization_id, '__global__'), email)"); + expect(out[0].actual).toBe('UNIQUE (organization_id, email)'); + }); + }); }); diff --git a/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts b/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts index aa5e6f75b0..1adfa38ca1 100644 --- a/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-unique-tenancy.test.ts @@ -1,29 +1,49 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { SqlDriver } from '../src/index.js'; +import { SqlDriver, classifyIndexKeyPart, parseIndexDdl } from '../src/index.js'; /** - * Tenant-scoped `unique` materialization (#3696). + * Unique-scope materialization: tenancy composites (#3696) + the explicit + * scope vocabulary and NULL-safe organization key part (ADR-0120, #5030). * - * The defect this locks down: `unique: true` used to become a single-column + * The defect #3696 locked down: `unique: true` used to become a single-column * global index that ignored `tenancy` entirely, while the autonumber sequence - * table is keyed by `(object, tenant_id, field, scope)` and hands every tenant - * its own counter starting at 1. Two subsystems of the same platform therefore - * disagreed — tenant B's `PROD-00001` was rejected by an index it could not - * see, with no user error involved — and the rejection itself leaked that some - * OTHER tenant held the value (a cross-tenant existence oracle). + * table hands every tenant its own counter starting at 1 — tenant B's + * `PROD-00001` was rejected by an index it could not see, and the rejection + * leaked that some OTHER tenant held the value (an existence oracle). * - * The contract now: - * - `unique: true` + tenant column → composite `(tenantField, field)` - * - `unique: true`, no tenant column → single-column (single-tenant: unchanged) - * - `unique: 'global'` → single-column, always - * - declared `indexes[]` → verbatim columns, never rewritten + * The defect #5030 measured on top of it: SQL UNIQUE is NULL-distinct, so the + * bare `(organization_id, field)` composite enforced NOTHING on rows whose + * organization is NULL — which on a single-tenant stack (where the kernel + * injects the column and never fills it) is EVERY row. ADR-0120 D3 makes the + * organization key part NULL-safe: `COALESCE(organization_id, '__global__')`. + * + * The contract now (ADR-0120 D1/D3; scope is a vocabulary, not a position): + * + * field `unique: true` / `'organization'` + tenant column + * → composite `(COALESCE(tenantField, '__global__'), field)` — unique per + * organization; NULL-organization rows form one platform bucket, unique + * among themselves (#5030). + * field `unique: true` / `'organization'`, no tenant column + * → single-column `(field)`. + * field `unique: 'global'` + * → single-column `(field)`, platform-wide, always. + * declared index `unique: true` / `'global'` + * → VERBATIM listed columns, never rewritten — the #3696 verbatim contract, + * now the `'global'` arm of the vocabulary (bare `true` retires at + * protocol 18; the synonym pin below retires with it). + * declared index `unique: 'organization'` + * → NULL-safe organization key part PREPENDED to the listed columns; with + * no tenant column it degrades to the listed columns (S11); a listed + * tenant column is made NULL-safe in place instead (S6 respelling). * * Retiring the legacy global index is no longer inline DDL at boot: since * #3728 it is a `replace_unique_index` drift entry, auto-applied on restart * only under the dev `autoMigrate: 'safe'` policy and otherwise left to - * `os migrate`. The migration tests below therefore opt into that policy. + * `os migrate`. The #5030 tightening (bare composite → NULL-safe composite) + * goes through the same ceremony as `recreate_index`, gated by the ADR-0120 D4 + * duplicate pre-flight probe. The migration tests below opt into that policy. */ describe('SqlDriver unique × tenancy (#3696)', () => { let driver: SqlDriver; @@ -57,14 +77,38 @@ describe('SqlDriver unique × tenancy (#3696)', () => { return out; } + /** + * Unique index name → canonical key parts, read from the index DDL so + * expression keys are visible (`PRAGMA index_info` reports a NULL name for + * them). A plain column reads as its name; the NULL-safe organization key + * part (ADR-0120 D3) reads as `COALESCE()` — literal elided, the + * same literal-agnostic identity the drift differ compares on. + */ async function uniqueIndexColumns(table: string): Promise> { const k = (driver as any).knex; const list: any = await k.raw(`PRAGMA index_list(${table})`); + const master: any = await k.raw( + `SELECT name, sql FROM sqlite_master WHERE type = 'index' AND tbl_name = ?`, + [table], + ); + const ddlByName = new Map(); + for (const r of Array.isArray(master) ? master : (master?.rows ?? [])) { + if (typeof r?.sql === 'string' && r.sql) ddlByName.set(r.name, r.sql); + } const out: Record = {}; for (const idx of list) { if (idx.origin === 'pk' || idx.unique !== 1) continue; - const info: any = await k.raw(`PRAGMA index_info(${idx.name})`); - out[idx.name] = info.map((c: any) => c.name); + const parsed = parseIndexDdl(ddlByName.get(idx.name) ?? ''); + if (parsed) { + out[idx.name] = parsed.keyParts.map((p) => { + const part = classifyIndexKeyPart(p); + if (part.kind === 'column') return part.column; + return part.column === null ? p : `COALESCE(${part.column})`; + }); + } else { + const info: any = await k.raw(`PRAGMA index_info(${idx.name})`); + out[idx.name] = info.map((c: any) => c.name); + } } return out; } @@ -132,7 +176,7 @@ describe('SqlDriver unique × tenancy (#3696)', () => { // ── Physical shape ──────────────────────────────────────────────────────── - it('materializes a COMPOSITE (tenant, field) unique index, tenant column first', async () => { + it('materializes a COMPOSITE (tenant, field) unique index, NULL-safe tenant key part first', async () => { await driver.initObjects([ { name: 'product', @@ -144,9 +188,21 @@ describe('SqlDriver unique × tenancy (#3696)', () => { ]); const uniques = await uniqueIndexColumns('product'); - expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); - // No leftover single-column global unique on `code`. + expect(Object.values(uniques)).toContainEqual(['COALESCE(organization_id)', 'code']); + // No leftover single-column global unique on `code`, and no bare + // (NULL-distinct — i.e. void on NULL-org rows, #5030) composite. expect(Object.values(uniques)).not.toContainEqual(['code']); + expect(Object.values(uniques)).not.toContainEqual(['organization_id', 'code']); + + // The literal is pinned too (ADR-0120 D3, maintainer-resolved): the + // constraint-violation error must read as "the platform bucket collided" + // — `('__global__', …)` — and the word must match the autonumber sequence + // table's GLOBAL_TENANT, not a bare ''. + const k = (driver as any).knex; + const master: any = await k.raw( + `SELECT sql FROM sqlite_master WHERE type = 'index' AND name = 'uniq_product_organization_id_code'`, + ); + expect(String(master[0]?.sql)).toMatch(/COALESCE\([`"]?organization_id[`"]?, '__global__'\)/i); }); it('materializes a SINGLE-column unique when the object has no tenant column', async () => { @@ -213,7 +269,7 @@ describe('SqlDriver unique × tenancy (#3696)', () => { ]); const uniques = await uniqueIndexColumns('env_setting'); - expect(Object.values(uniques)).toContainEqual(['environment_id', 'key']); + expect(Object.values(uniques)).toContainEqual(['COALESCE(environment_id)', 'key']); await driver.create('env_setting', { environment_id: 'env_a', key: 'k' }); const other = await driver.create('env_setting', { environment_id: 'env_b', key: 'k' }); @@ -296,7 +352,7 @@ describe('SqlDriver unique × tenancy (#3696)', () => { const uniques = await uniqueIndexColumns('product'); expect(uniques['product_code_unique']).toBeUndefined(); - expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); + expect(Object.values(uniques)).toContainEqual(['COALESCE(organization_id)', 'code']); // Existing data survived, and the cross-tenant insert now works. expect(await driver.count('product', { object: 'product' })).toBe(2); @@ -326,7 +382,7 @@ describe('SqlDriver unique × tenancy (#3696)', () => { const uniques = await uniqueIndexColumns('product'); expect(uniques['uniq_product_code']).toBeUndefined(); - expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); + expect(Object.values(uniques)).toContainEqual(['COALESCE(organization_id)', 'code']); }); it("does NOT retire the single-column index of a unique: 'global' field", async () => { @@ -397,12 +453,273 @@ describe('SqlDriver unique × tenancy (#3696)', () => { ]); const uniques = await uniqueIndexColumns('product'); - expect(Object.values(uniques)).toContainEqual(['organization_id', 'code']); + expect(Object.values(uniques)).toContainEqual(['COALESCE(organization_id)', 'code']); expect(Object.values(uniques)).not.toContainEqual(['code']); + expect(Object.values(uniques)).not.toContainEqual(['organization_id', 'code']); // Data preserved, and cross-tenant reuse still works post-rebuild. expect(await driver.count('product', { object: 'product' })).toBe(1); const b = await driver.create('product', { organization_id: 'org_b', code: 'C1', note: 'n' }); expect(b.code).toBe('C1'); }); + + // ── ADR-0120 D3: the NULL-organization bucket is constrained (#5030) ────── + + describe('NULL-safe organization uniqueness (ADR-0120 D3, #5030)', () => { + it('#5030 regression: unique IS enforced when organization_id exists but is NULL on every row', async () => { + // The measured defect shape, verbatim from the issue: the kernel injects + // `organization_id` on single-tenant stacks too, every row leaves it + // NULL, and the bare NULL-distinct composite admitted any number of + // duplicates — field-level `unique: true` enforced NOTHING. + await driver.initObjects([ + { + name: 'account', + fields: { + organization_id: { type: 'string' }, + email: { type: 'text', unique: true }, + }, + }, + ]); + + const first = await driver.create('account', { email: 'dup@example.com' }); + expect(first.organization_id ?? null).toBeNull(); + await expect(driver.create('account', { email: 'dup@example.com' })).rejects.toThrow( + /UNIQUE constraint failed|duplicate key value/, + ); + }); + + it('the NULL bucket and organizations coexist: one platform holder, plus one per organization (S8)', async () => { + await driver.initObjects([ + { + name: 'template', + fields: { + organization_id: { type: 'string' }, + key: { type: 'string', unique: true }, + }, + }, + ]); + + // Platform (NULL-organization) row takes the value… + await driver.create('template', { key: 'welcome' }); + // …an organization may still hold the same value (per-org scope)… + const org = await driver.create('template', { organization_id: 'org_a', key: 'welcome' }); + expect(org.key).toBe('welcome'); + // …but a SECOND platform row may not: NULL rows are one bucket, unique + // among themselves. + await expect(driver.create('template', { key: 'welcome' })).rejects.toThrow( + /UNIQUE constraint failed|duplicate key value/, + ); + }); + + it("accepts field-level unique: 'organization' as the explicit synonym of true (ADR-0120 D1)", async () => { + await driver.initObjects([ + { + name: 'contact', + fields: { + organization_id: { type: 'string' }, + email: { type: 'string', unique: 'organization' }, + }, + } as any, + ]); + + const uniques = await uniqueIndexColumns('contact'); + expect(Object.values(uniques)).toContainEqual(['COALESCE(organization_id)', 'email']); + + await driver.create('contact', { organization_id: 'org_a', email: 'a@x.test' }); + const other = await driver.create('contact', { organization_id: 'org_b', email: 'a@x.test' }); + expect(other.email).toBe('a@x.test'); + await expect( + driver.create('contact', { organization_id: 'org_a', email: 'a@x.test' }), + ).rejects.toThrow(/UNIQUE constraint failed|duplicate key value/); + }); + }); + + // ── ADR-0120 D1: declared-index `unique: 'organization'` ────────────────── + + describe("declared-index unique: 'organization' (ADR-0120 D1)", () => { + it('prepends the NULL-safe organization key part to the listed columns', async () => { + await driver.initObjects([ + { + name: 'case_record', + fields: { + organization_id: { type: 'string' }, + department: { type: 'string' }, + code: { type: 'string' }, + }, + indexes: [{ fields: ['department', 'code'], unique: 'organization' }], + } as any, + ]); + + const uniques = await uniqueIndexColumns('case_record'); + expect(uniques['uniq_case_record_organization_id_department_code']).toEqual([ + 'COALESCE(organization_id)', + 'department', + 'code', + ]); + + // Per-organization semantics: two orgs may hold the same (department, + // code); the same org may not; the NULL bucket is constrained too. + await driver.create('case_record', { organization_id: 'org_a', department: 'sales', code: 'C-1' }); + await driver.create('case_record', { organization_id: 'org_b', department: 'sales', code: 'C-1' }); + await expect( + driver.create('case_record', { organization_id: 'org_a', department: 'sales', code: 'C-1' }), + ).rejects.toThrow(/UNIQUE constraint failed|duplicate key value/); + await driver.create('case_record', { department: 'sales', code: 'C-2' }); + await expect( + driver.create('case_record', { department: 'sales', code: 'C-2' }), + ).rejects.toThrow(/UNIQUE constraint failed|duplicate key value/); + }); + + it('degrades to the listed columns alone when the object has no tenant column (S11)', async () => { + await driver.initObjects([ + { + name: 'plain_catalog', + fields: { department: { type: 'string' }, code: { type: 'string' } }, + indexes: [{ fields: ['department', 'code'], unique: 'organization' }], + } as any, + ]); + + const uniques = await uniqueIndexColumns('plain_catalog'); + expect(uniques['uniq_plain_catalog_department_code']).toEqual(['department', 'code']); + }); + + it('makes a LISTED tenant column NULL-safe in place — the S6 respelling, not double-prepended', async () => { + // The hand-written legacy shape, opted into the explicit vocabulary: + // the author already listed the tenant column, so the org key part is + // not prepended again — the listed column's own key part becomes the + // NULL-safe form. + await driver.initObjects([ + { + name: 'team', + fields: { organization_id: { type: 'string' }, name: { type: 'string' } }, + indexes: [{ fields: ['organization_id', 'name'], unique: 'organization' }], + } as any, + ]); + + const uniques = await uniqueIndexColumns('team'); + expect(uniques['uniq_team_organization_id_name']).toEqual([ + 'COALESCE(organization_id)', + 'name', + ]); + + await driver.create('team', { name: 'Core' }); + await expect(driver.create('team', { name: 'Core' })).rejects.toThrow( + /UNIQUE constraint failed|duplicate key value/, + ); + }); + }); + + // ── ADR-0120 D4: the tightening goes through the ceremony ───────────────── + + describe('bare-composite tightening + duplicate pre-flight (ADR-0120 D4)', () => { + /** A pre-ADR-0120 database: the bare NULL-distinct composite, in place. */ + const seedBareComposite = async () => { + const k = (driver as any).knex; + await k.schema.createTable('product', (t: any) => { + t.string('id').primary(); + t.timestamp('created_at'); + t.timestamp('updated_at'); + t.string('organization_id'); + t.string('code'); + }); + await k.raw( + 'CREATE UNIQUE INDEX uniq_product_organization_id_code ON product (organization_id, code)', + ); + return k; + }; + + const productMeta = [ + { + name: 'product', + fields: { + organization_id: { type: 'string' }, + code: { type: 'string', unique: true }, + }, + }, + ]; + + it("auto-tightens at boot under autoMigrate: 'safe' when the probe is clean", async () => { + await driver.disconnect(); + driver = makeDriver({ autoMigrate: 'safe' }); + const k = await seedBareComposite(); + // Clean data: distinct codes, including one NULL-organization row. + await k('product').insert([ + { id: 'r1', organization_id: 'org_a', code: 'C1' }, + { id: 'r2', organization_id: null, code: 'C1' }, + ]); + + await driver.initObjects(productMeta); + + const uniques = await uniqueIndexColumns('product'); + expect(uniques['uniq_product_organization_id_code']).toEqual([ + 'COALESCE(organization_id)', + 'code', + ]); + // Healthy database: converged, zero drift. + expect(await driver.detectManagedDrift()).toHaveLength(0); + // The NULL bucket is now enforced: a second platform-bucket C1 is refused. + await expect(driver.create('product', { code: 'C1' })).rejects.toThrow( + /UNIQUE constraint failed|duplicate key value/, + ); + }); + + it('BLOCKS the tightening on duplicate NULL-organization rows — old index kept, rows reported', async () => { + await driver.disconnect(); + driver = makeDriver({ autoMigrate: 'safe' }); + const k = await seedBareComposite(); + // The #5030 defect made data: duplicates the NULL-distinct index admitted. + await k('product').insert([ + { id: 'r1', organization_id: null, code: 'DUP' }, + { id: 'r2', organization_id: null, code: 'DUP' }, + ]); + + // Boot survives — the tightening is not applied, the old index stays. + await driver.initObjects(productMeta); + expect((await uniqueIndexColumns('product'))['uniq_product_organization_id_code']).toEqual([ + 'organization_id', + 'code', + ]); + + // The plan reports the op as BLOCKED, with the offending group named — + // and the COALESCE key makes the report self-describing ('__global__' = + // the platform bucket). + const drift = await driver.detectManagedDrift(); + const entry = drift.find((d) => d.op.type === 'recreate_index'); + expect(entry).toBeDefined(); + expect(entry!.category).toBe('destructive'); + expect(entry!.severity).toBe('error'); + expect(entry!.message).toMatch(/BLOCKED/); + expect(entry!.message).toMatch(/__global__/); + expect(entry!.message).toMatch(/"DUP"/); + expect(entry!.message).toMatch(/#5030/); + + // Even `--allow-destructive` cannot force it: apply re-probes and + // refuses — at no point is a constraint dropped without its replacement + // being creatable. + const { applied, skipped } = await driver.applyMigrationEntries([entry!], { + allowDestructive: true, + }); + expect(applied).toHaveLength(0); + expect(skipped).toHaveLength(1); + expect((await uniqueIndexColumns('product'))['uniq_product_organization_id_code']).toEqual([ + 'organization_id', + 'code', + ]); + + // Deduplicating unblocks the op: the probe comes back clean, the entry + // re-grades `safe`, and a plain apply tightens the index. + await k('product').where({ id: 'r2' }).delete(); + const drift2 = await driver.detectManagedDrift(); + const entry2 = drift2.find((d) => d.op.type === 'recreate_index'); + expect(entry2).toBeDefined(); + expect(entry2!.category).toBe('safe'); + const res2 = await driver.applyMigrationEntries([entry2!], { allowDestructive: false }); + expect(res2.applied).toHaveLength(1); + expect((await uniqueIndexColumns('product'))['uniq_product_organization_id_code']).toEqual([ + 'COALESCE(organization_id)', + 'code', + ]); + expect(await driver.detectManagedDrift()).toHaveLength(0); + }); + }); }); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index afe23344fc..0eb281b78c 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -8,7 +8,7 @@ */ import type { QueryAST, DriverOptions, SchemaMode } from '@objectstack/spec/data'; -import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, isGlobalUnique, isUniqueDeclared, type AutonumberToken } from '@objectstack/spec/data'; +import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, type AutonumberToken } from '@objectstack/spec/data'; import { STRUCTURED_JSON_TYPES, FILE_REFERENCE_TYPES, MULTI_OPTION_TYPES, NUMERIC_VALUE_TYPES } from '@objectstack/spec/data'; import { canonicalAstOperator } from '@objectstack/spec/data'; // `defaultValue` runtime tokens (#4560). The DDL below asks the SPEC — not a @@ -30,10 +30,15 @@ import { driftKey, expectedIndexes, fieldHasColumn, + GLOBAL_TENANT, isIndexDriftOp, + isUniqueScopeDeclared, legacyUniqueReplacements, + normalizeDeclaredIndex, + organizationKeyPartSql, parseIndexDdl, uniqueIndexesFromFields, + type DeclaredIndexInput, type ManagedDriftEntry, type DriftOp, type PhysicalIndex, @@ -97,12 +102,13 @@ function lastIdentifierSegment(raw: string): string { */ const SEQUENCES_TABLE = '_objectstack_sequences'; -/** - * Sentinel tenant_id used when an object has no tenant field (org-less - * objects like Setup-side singletons). Keeps the (object, tenant, field) - * primary key non-null. - */ -const GLOBAL_TENANT = '__global__'; +// GLOBAL_TENANT ('__global__') — the sentinel for the NULL-organization +// ("platform") bucket — is defined ONCE in schema-drift.ts and imported here: +// since ADR-0120 D3 it names the same bucket in two subsystems (the autonumber +// sequence table's tenant key, and the COALESCE organization key part of every +// organization-scoped unique index), and #3696's root lesson was two +// subsystems naming one concept differently. Storage stays NULL — see the +// definition site. /** * Field types whose value is an array or object and must be stored as a JSON @@ -707,8 +713,16 @@ export class SqlDriver implements IDataDriver { protected logger: { warn: (msg: string, meta?: any) => void; info?: (msg: string, meta?: any) => void; + /** + * Durability-degradation channel (see AGENTS.md §Degradation log levels): + * used when a constraint the metadata claims is enforced is NOT — e.g. a + * NULL-safe unique that could not be (re)built (ADR-0120 D4). Falls back + * to `warn` when the injected sink has no `error`. + */ + error?: (msg: string, meta?: any) => void; } = { warn: (msg, meta) => console.warn(msg, meta ?? ''), + error: (msg, meta) => console.error(msg, meta ?? ''), }; /** Whether the underlying database is a SQLite variant (sqlite3 or better-sqlite3). */ @@ -2719,7 +2733,7 @@ export class SqlDriver implements IDataDriver { /** Create/column-sync one physical shard table (mirrors the managed-table * branch of {@link initObjects}, scoped to a shard). */ - protected async ensureShardTable(shardName: string, obj: { fields?: Record }): Promise { + protected async ensureShardTable(shardName: string, obj: { fields?: Record; tenancy?: any }): Promise { const builtinColumns = new Set(['id', 'created_at', 'updated_at']); const exists = await this.knex.schema.hasTable(shardName); if (!exists) { @@ -2754,7 +2768,11 @@ export class SqlDriver implements IDataDriver { ...idx, name: typeof idx?.name === 'string' && idx.name.trim() ? `${shardName}__${idx.name.trim()}` : undefined, })); - await this.syncDeclaredIndexes(shardName, perShard, new Set(Object.keys(colInfo))); + // Shard bookkeeping is aliased AFTER this method runs, so resolve the + // tenant column from the object schema itself — a declared + // `unique: 'organization'` index (ADR-0120 D1) must scope identically on + // every shard of the base table. + await this.syncDeclaredIndexes(shardName, perShard, new Set(Object.keys(colInfo)), this.computeTenantField(obj)); } } @@ -3106,7 +3124,7 @@ export class SqlDriver implements IDataDriver { // needs the tenant column to already be there. const declaredIndexes = (obj as any).indexes; const uniqueFields = Object.values(obj.fields ?? {}).some((f) => - isUniqueDeclared(f?.unique), + isUniqueScopeDeclared(f?.unique), ); if (uniqueFields || (Array.isArray(declaredIndexes) && declaredIndexes.length > 0)) { const colInfo = await this.knex(tableName).columnInfo(); @@ -3738,7 +3756,7 @@ export class SqlDriver implements IDataDriver { physicalColumns: Set, ): Promise { const tenantField = this.resolveTenantField(tableName); - return diffManagedIndexes({ + const entries = diffManagedIndexes({ table: tableName, expected: expectedIndexes({ table: tableName, fields, tenantField, declaredIndexes, physicalColumns }), // `declaredIndexes` goes to BOTH: it is what the table should have, and @@ -3748,7 +3766,118 @@ export class SqlDriver implements IDataDriver { // 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), + // Lets the differ recognise the NULL-safe organization key part as the + // sync's own vocabulary (ADR-0120 D3). + tenantField, }); + // ADR-0120 D4: the duplicate pre-flight probe decides each NULL-safe + // unique op's fate — clean data upgrades the pure tightening to `safe` + // (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); + return entries; + } + + /** + * ADR-0120 D4 — duplicate pre-flight for NULL-safe organization uniques. + * + * Probes every index op that would CREATE a unique index whose organization + * key part is the NULL-safe COALESCE form, by grouping over that exact key: + * + * - `recreate_index` marked `tightenNullSafeOnly` (the bare composite + * tightening into its COALESCE form — same identities, physical fully + * plain): a clean probe recategorises it `safe`, so dev + * `autoMigrate: 'safe'` and a plain `os migrate apply` may apply it; a + * dirty probe keeps it blocked (`destructive` + a re-probe refusal in + * {@link applyIndexDriftOp}) and reports the offending rows. + * - `create_index` for a unique NULL-safe index: a dirty probe demotes the + * default `safe` to blocked with the same row report — the CREATE could + * only fail at apply time otherwise, with a raw driver error naming no + * rows. + * + * `replace_unique_index` is deliberately NOT probed: the legacy index it + * retires is a platform-wide unique, strictly stronger than the NULL-safe + * composite, so duplicates in the new key are impossible by construction. + */ + protected async applyNullSafeUniquePreflight(entries: ManagedDriftEntry[]): Promise { + for (const d of entries) { + const op = d.op; + if (op.type !== 'recreate_index' && op.type !== 'create_index') continue; + if (!op.unique || !op.nullSafeColumns || op.nullSafeColumns.length === 0) continue; + const tighten = op.type === 'recreate_index' && op.tightenNullSafeOnly === true; + // A generic unique recreate (columns differ beyond the key-part form) + // keeps its pre-ADR-0120 semantics untouched. + if (op.type === 'recreate_index' && !tighten) continue; + + let duplicates: Array<{ key: string; rows: number }>; + try { + duplicates = await this.probeNullSafeUniqueDuplicates(op.table, op.columns, op.nullSafeColumns); + } catch (e: any) { + // Probe failure must fail SAFE: without evidence the data is clean the + // op may not claim eligibility for auto-apply. + this.logger.warn( + `[schema-drift] duplicate pre-flight for '${op.indexName}' on '${op.table}' failed — leaving the op gated`, + e?.message ?? e, + ); + continue; + } + + const signature = d.expected; + if (duplicates.length === 0) { + if (tighten) { + d.category = 'safe'; + d.severity = 'warning'; + d.message = + `${op.table}: index '${op.indexName}' tightens to ${signature} — the organization key part becomes ` + + `NULL-safe (ADR-0120 D3), so rows without an organization are constrained too. The duplicate ` + + `pre-flight probe found no conflicting rows: pure tightening, applied by "os migrate apply" ` + + `(auto-applied at boot under dev autoMigrate: 'safe').`; + } + continue; + } + + const report = duplicates + .slice(0, 5) + .map((g) => `(${g.key}) × ${g.rows} rows`) + .join('; '); + const more = duplicates.length > 5 ? `; …and ${duplicates.length - 5} more group(s)` : ''; + d.category = 'destructive'; + d.severity = 'error'; + d.message = + `${op.table}: cannot ${tighten ? 'tighten' : 'create'} '${op.indexName}' as ${signature} — existing rows ` + + `already violate the NULL-safe unique constraint (duplicates the old index wrongly admitted, #5030): ` + + `${report}${more}. The op is BLOCKED: apply re-probes and refuses, and the existing index stays in place ` + + `(ADR-0120 D4). Deduplicate the listed rows, then re-run "os migrate plan".`; + } + } + + /** + * Find duplicate groups under a NULL-safe organization unique key: GROUP BY + * the exact key the index will enforce — `COALESCE(, '__global__')` for + * the NULL-safe parts, the bare column otherwise — HAVING COUNT(*) > 1. + * Returns one entry per conflicting group, `key` naming columns and values. + */ + protected async probeNullSafeUniqueDuplicates( + table: string, + columns: string[], + nullSafeColumns: string[], + ): Promise> { + const ns = new Set(nullSafeColumns); + const q = (id: string) => this.knex.ref(id).toQuery(); + const parts = columns.map((c) => + ns.has(c) ? `COALESCE(${q(c)}, '${GLOBAL_TENANT}')` : q(c), + ); + const selectList = parts.map((p, i) => `${p} AS k${i}`).join(', '); + const groupList = parts.join(', '); + const sql = + `SELECT ${selectList}, COUNT(*) AS n FROM ${q(table)} ` + + `GROUP BY ${groupList} HAVING COUNT(*) > 1 ORDER BY n DESC LIMIT 20`; + const res: any = await this.knex.raw(sql); + const rows: any[] = Array.isArray(res) ? (Array.isArray(res[0]) ? res[0] : res) : (res?.rows ?? []); + return rows.map((r: any) => ({ + key: columns.map((c, i) => `${c}=${JSON.stringify(r[`k${i}`])}`).join(', '), + rows: Number(r.n ?? r.N ?? 0), + })); } /** @@ -3915,15 +4044,15 @@ export class SqlDriver implements IDataDriver { */ protected async applyIndexDriftOp(op: DriftOp): Promise { const physicalColumns = new Set(Object.keys(await this.knex(op.table).columnInfo())); - const ensure = (name: string, columns: string[], unique: boolean) => - this.syncDeclaredIndexes(op.table, [{ name, fields: columns, unique }], physicalColumns); + const ensure = (name: string, columns: string[], unique: boolean, nullSafeColumns?: string[]) => + this.syncDeclaredIndexes(op.table, [{ name, fields: columns, unique, nullSafeColumns }], physicalColumns); switch (op.type) { case 'replace_unique_index': { // CREATE before DROP: the composite and the legacy index have different // names, so uniqueness is never unenforced in between. If the create // fails we have not dropped anything yet and the schema is untouched. - await ensure(op.createIndexName, op.createColumns, true); + await ensure(op.createIndexName, op.createColumns, true, op.nullSafeColumns); // …and only drop once the replacement is confirmed present. This is a // relaxation, not a removal: if `syncDeclaredIndexes` skipped the create // (a column it references is not materialized), dropping the legacy @@ -3941,22 +4070,87 @@ export class SqlDriver implements IDataDriver { return true; } case 'create_index': - await ensure(op.indexName, op.columns, op.unique); - return true; + await ensure(op.indexName, op.columns, op.unique, op.nullSafeColumns); + // Honest applied-reporting: `syncDeclaredIndexes` degrades some + // failures (a NULL-safe unique over data that still violates it) into + // a loud log instead of a throw, so presence is the only proof. + return (await this.getExistingIndexNames(op.table)).has(op.indexName); case 'drop_index': return await this.dropIndexIfExists(op.table, op.indexName); - case 'recreate_index': + case 'recreate_index': { // Same name on both sides — the drop has to come first, and a UNIQUE // target can fail on existing duplicates. That is why this op is // categorised destructive when unique (see `diffManagedIndexes`). + // + // ADR-0120 D4: the NULL-safe tightening re-runs the duplicate + // pre-flight HERE, immediately before the drop — the plan-time probe + // may be stale, and at no point may a constraint be dropped without + // its replacement being creatable. Duplicates → refuse, old index + // untouched. + const nullSafe = op.unique && (op.nullSafeColumns?.length ?? 0) > 0; + if (nullSafe) { + const duplicates = await this.probeNullSafeUniqueDuplicates( + op.table, + op.columns, + op.nullSafeColumns!, + ); + if (duplicates.length > 0) { + (this.logger.error ?? this.logger.warn)( + `[schema-drift] REFUSING to rebuild '${op.indexName}' on '${op.table}' as a NULL-safe unique — ` + + `${duplicates.length} duplicate group(s) violate it (e.g. ${duplicates[0].key} × ${duplicates[0].rows} rows). ` + + `The existing index is left in place; deduplicate and re-run "os migrate plan" (ADR-0120 D4).`, + ); + return false; + } + } await this.dropIndexIfExists(op.table, op.indexName); - await ensure(op.indexName, op.columns, op.unique); - return true; + try { + await ensure(op.indexName, op.columns, op.unique, op.nullSafeColumns); + } catch (e) { + if (nullSafe) await this.restoreBareIndexAfterFailedTighten(op, e); + throw e; + } + const present = (await this.getExistingIndexNames(op.table)).has(op.indexName); + if (!present && nullSafe) await this.restoreBareIndexAfterFailedTighten(op, undefined); + return present; + } default: return false; } } + /** + * Last-resort restore for the ADR-0120 D4 tightening: the old index is + * already dropped and the NULL-safe replacement could not be created (a + * write raced the probe, or the dialect refused the expression key). Put the + * previous BARE composite back under the same name so the constraint that + * existed before the attempt keeps existing — then say, at `error` level, + * exactly what is NOT enforced and how to fix it, because from the outside + * everything keeps looking normal (the durability-degradation rule). + */ + protected async restoreBareIndexAfterFailedTighten( + op: Extract, + cause: unknown, + ): Promise { + try { + await this.syncDeclaredIndexes( + op.table, + [{ name: op.indexName, fields: op.columns, unique: op.unique }], + new Set(Object.keys(await this.knex(op.table).columnInfo())), + ); + } catch { + /* the error below reports the state either way */ + } + const restored = (await this.getExistingIndexNames(op.table)).has(op.indexName); + (this.logger.error ?? this.logger.warn)( + `[schema-drift] could not create the NULL-safe unique '${op.indexName}' on '${op.table}' after dropping ` + + `the old index${restored ? ' — restored the previous bare composite' : ' — AND the restore failed, so the ' + + 'constraint is currently NOT enforced'}. Rows without an organization are ${restored ? 'still ' : ''}not ` + + `constrained (#5030); re-run "os migrate plan" and apply the reported op (ADR-0120 D4).`, + (cause as any)?.message ?? cause, + ); + } + /** Apply a single drift op in place (Postgres / MySQL). Returns false if unsupported. */ protected async applyDriftOpInPlace(op: DriftOp): Promise { // Index ops need no dialect-specific ALTER — route them to the portable path. @@ -4269,11 +4463,12 @@ export class SqlDriver implements IDataDriver { tableName: string, fields: Record, tenantField: string | null, - ): Array<{ name: string; fields: string[]; unique: true }> { + ): Array<{ name: string; fields: string[]; unique: true; nullSafeColumns?: string[] }> { return uniqueIndexesFromFields(tableName, fields, tenantField).map((i) => ({ name: i.name, fields: i.columns, unique: true as const, + ...(i.nullSafeColumns ? { nullSafeColumns: i.nullSafeColumns } : {}), })); } @@ -4341,65 +4536,83 @@ export class SqlDriver implements IDataDriver { const fromFields = this.uniqueIndexesFromFields(tableName, fields, tenantField); const declared = Array.isArray(declaredIndexes) ? declaredIndexes : []; if (fromFields.length === 0 && declared.length === 0) return; - await this.syncDeclaredIndexes(tableName, [...fromFields, ...declared], physicalColumns); + // Pass the tenant column through rather than letting `syncDeclaredIndexes` + // 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); } /** * Materialize declared object-level indexes. * * - Multi-column and single-column indexes are both supported. - * - `unique: true` emits a UNIQUE index. NULL-distinct semantics are the - * default across SQLite/Postgres/MySQL, so multiple NULL rows remain + * - `unique: true` / `'global'` emits a UNIQUE index over the column list + * taken VERBATIM — no tenant column is injected. That verbatim behavior is + * the `'global'` arm of the ADR-0120 D1 scope vocabulary (it was the only + * arm before that ADR): a `'global'` declared index already names its + * columns and is frequently platform-wide on purpose (a DNS hostname, a + * reserved slug, an external provider id, every engine dedup key), so + * rewriting it would break real constraints. NULL-distinct semantics are + * the default across SQLite/Postgres/MySQL, so multiple NULL rows remain * allowed while non-NULL duplicates are rejected — matching the * convergence-on-conflict pattern the messaging pipeline relies on. - * - The column list is taken VERBATIM — no tenant column is injected here. - * Field-level `unique` is tenancy-scoped upstream in - * {@link uniqueIndexesFromFields}; a declared index already names its - * columns and is frequently platform-wide on purpose (a DNS hostname, a - * reserved slug, an external provider id), so guessing would break it. + * - `unique: 'organization'` (ADR-0120 D1/D3) prepends the table's tenant + * column in its NULL-safe form — `COALESCE(tenantField, '__global__')` — + * resolved here at registration, where tenancy is known; with no tenant + * column it degrades to the listed columns (S11). Field-level `unique` is + * scoped upstream in {@link uniqueIndexesFromFields} and arrives here + * pre-resolved (`nullSafeColumns`). Both routes land on + * {@link normalizeDeclaredIndex}, the differ's normalizer, so what the + * 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. + * - 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 + * durability-degradation rule) and surfaces as drift with a row report via + * the ADR-0120 D4 pre-flight, instead of failing the whole boot. */ protected async syncDeclaredIndexes( tableName: string, - indexes: Array<{ name?: string; fields?: string[]; unique?: boolean | 'global' }>, + indexes: DeclaredIndexInput[], physicalColumns: Set, + tenantField?: string | null, ): Promise { const existing = await this.getExistingIndexNames(tableName); + const resolvedTenantField = tenantField !== undefined ? tenantField : this.resolveTenantField(tableName); for (const idx of indexes) { - const fields = Array.isArray(idx?.fields) - ? idx.fields.filter((f): f is string => typeof f === 'string' && f.length > 0) - : []; - if (fields.length === 0) continue; + const norm = normalizeDeclaredIndex(tableName, idx, resolvedTenantField); + if (!norm) continue; + const { name, columns, unique } = norm; + const nullSafe = new Set(norm.nullSafeColumns ?? []); - const missing = fields.filter((f) => !physicalColumns.has(f)); + 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 }, + { tableName, fields: columns }, ); continue; } - const unique = isUniqueDeclared(idx.unique); - const name = - typeof idx.name === 'string' && idx.name.trim() - ? idx.name.trim() - : this.buildIndexName(tableName, fields, unique); - if (existing.has(name)) continue; try { - await this.knex.schema.alterTable(tableName, (table) => { - if (unique) { - table.unique(fields, { indexName: name }); - } else { - table.index(fields, name); - } - }); + if (nullSafe.size > 0) { + await this.createNullSafeUniqueIndex(tableName, name, columns, nullSafe); + } else { + await this.knex.schema.alterTable(tableName, (table) => { + if (unique) { + table.unique(columns, { indexName: name }); + } else { + table.index(columns, name); + } + }); + } existing.add(name); } catch (e: any) { const msg = String(e?.message ?? e); @@ -4407,11 +4620,71 @@ export class SqlDriver implements IDataDriver { // different name can race us here — both are benign for our intent // (the index exists). Anything else is a real failure. if (/already exists|duplicate key name|exists/i.test(msg)) continue; + if (nullSafe.size > 0 && /unique constraint failed|duplicate entry|duplicate key value/i.test(msg)) { + // Existing rows violate the NULL-safe unique — the #5030 defect made + // visible. Do not take the boot down: the declared constraint is not + // enforced yet, say so at `error` (from the outside everything looks + // normal), and let the D4 drift pre-flight report the exact rows. + (this.logger.error ?? this.logger.warn)( + `[sql-driver] cannot create NULL-safe unique index '${name}' on "${tableName}" — existing rows ` + + `violate it (duplicates the previous NULL-distinct index admitted, #5030). The constraint ` + + `'${columns.join(', ')}' is NOT enforced until the data is deduplicated: run "os migrate plan" ` + + `for the conflicting rows (ADR-0120 D4).`, + msg, + ); + continue; + } throw e; } } } + /** + * Raw DDL for an organization-scoped unique index (ADR-0120 D3): knex's + * schema builder cannot express an expression key part, so the CREATE is + * spelled out. SQLite and Postgres take the function call as a key part + * directly; MySQL requires functional key parts to be parenthesized. + * + * On a MySQL that predates functional key parts (< 8.0.13) or MariaDB, the + * expression form is rejected — degrade to the BARE composite so non-NULL + * rows keep their constraint, and say at `error` level exactly what is not + * enforced (the NULL-organization bucket) and what fixes it. A silent bare + * fallback would re-open #5030 unnamed; failing the boot would brick every + * such deployment for every unique field. The drift differ keeps reporting + * the tightening for the day the server is upgraded. + */ + protected async createNullSafeUniqueIndex( + tableName: string, + name: string, + columns: string[], + nullSafe: ReadonlySet, + ): Promise { + const q = (id: string) => this.knex.ref(id).toQuery(); + const parts = columns.map((c) => { + if (!nullSafe.has(c)) return q(c); + const expr = `COALESCE(${q(c)}, '${GLOBAL_TENANT}')`; + return this.isMysql ? `(${expr})` : expr; + }); + const sql = `CREATE UNIQUE INDEX ${q(name)} ON ${q(tableName)} (${parts.join(', ')})`; + try { + await this.knex.raw(sql); + } catch (e: any) { + const msg = String(e?.message ?? e); + const functionalUnsupported = + this.isMysql && /syntax|functional|not supported|near '\(/i.test(msg) && !/duplicate/i.test(msg); + if (!functionalUnsupported) throw e; + (this.logger.error ?? this.logger.warn)( + `[sql-driver] this MySQL/MariaDB server rejects functional key parts — created '${name}' on ` + + `"${tableName}" over the BARE columns instead. Rows without an organization are NOT constrained ` + + `by it (#5030): upgrade to MySQL >= 8.0.13 and re-run "os migrate plan" to tighten it (ADR-0120 D3).`, + msg, + ); + await this.knex.schema.alterTable(tableName, (table) => { + table.unique(columns, { indexName: name }); + }); + } + } + // =================================== // Schema Introspection // =================================== diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index 5f9738dfa7..e6d4b23491 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -1035,6 +1035,64 @@ describe('AuthManager', () => { expect(assertIssuable).not.toHaveBeenCalled(); }); + // ── [ADR-0120 D3] `'__global__'` is reserved at the org-minting seam ── + // + // The token names the NULL-organization ("platform") bucket in every + // organization-scoped unique index (`COALESCE(organization_id, + // '__global__')`) and in the autonumber sequence table. An organization + // minted with it as id or slug would collide with that bucket. + describe("'__global__' organization id/slug reservation (ADR-0120 D3)", () => { + const OLD_MULTI = process.env.OS_MULTI_ORG_ENABLED; + beforeEach(() => { + process.env.OS_MULTI_ORG_ENABLED = 'true'; + }); + afterEach(() => { + if (OLD_MULTI === undefined) delete process.env.OS_MULTI_ORG_ENABLED; + else process.env.OS_MULTI_ORG_ENABLED = OLD_MULTI; + }); + + it("rejects an organization whose slug is '__global__'", async () => { + const orgPlugin = await bootOrgPlugin(); + await expect( + orgPlugin._opts.organizationHooks.beforeCreateOrganization({ + organization: { name: 'Global Inc', slug: '__global__' }, + user: { id: 'u1' }, + }), + ).rejects.toThrow(/'__global__' is reserved .* \(ADR-0120 D3\)/); + }); + + it("rejects an organization whose id is '__global__'", async () => { + const orgPlugin = await bootOrgPlugin(); + await expect( + orgPlugin._opts.organizationHooks.beforeCreateOrganization({ + organization: { id: '__global__', name: 'Global Inc', slug: 'global-inc' }, + user: { id: 'u1' }, + }), + ).rejects.toThrow(/'__global__' is reserved .* \(ADR-0120 D3\)/); + }); + + it('an ordinary organization passes the reservation guard', async () => { + const orgPlugin = await bootOrgPlugin(); + await expect( + orgPlugin._opts.organizationHooks.beforeCreateOrganization({ + organization: { name: 'Acme', slug: 'acme' }, + user: { id: 'u1' }, + }), + ).resolves.toBeUndefined(); + }); + + it('the reservation is judged before the multi-org gate (precise error in single-org mode too)', async () => { + delete process.env.OS_MULTI_ORG_ENABLED; // default: single-org + const orgPlugin = await bootOrgPlugin(); + await expect( + orgPlugin._opts.organizationHooks.beforeCreateOrganization({ + organization: { name: 'Global Inc', slug: '__global__' }, + user: { id: 'u1' }, + }), + ).rejects.toThrow(/'__global__' is reserved/); + }); + }); + it('should register twoFactor plugin with schema mapping when enabled', async () => { let capturedConfig: any; (betterAuth as any).mockImplementation((config: any) => { diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 5b0991b00d..813abf5b62 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1897,7 +1897,22 @@ export class AuthManager { // deployment is provisioned in single-org mode. Resolution order: // `OS_MULTI_ORG_ENABLED` (default `'false'` → single-org / // per-env runtime). - beforeCreateOrganization: async () => { + beforeCreateOrganization: async ({ organization }: any = {}) => { + // [ADR-0120 D3] `'__global__'` is the platform's name for the + // NULL-organization bucket: the autonumber sequence table keys + // org-less rows by it, and every organization-scoped unique index + // folds NULL into it via `COALESCE(organization_id, '__global__')`. + // An organization minted with that token as its id (or slug — the + // only caller-controllable identifier here) would collide with the + // platform bucket, so the token is reserved at this seam. + if (organization?.id === '__global__' || organization?.slug === '__global__') { + const { APIError } = await import('better-auth/api'); + throw new APIError('BAD_REQUEST', { + message: + "'__global__' is reserved for the platform (no-organization) bucket " + + '(ADR-0120 D3) and cannot be used as an organization id or slug.', + }); + } if (!resolveMultiOrgEnabled()) { const { APIError } = await import('better-auth/api'); throw new APIError('FORBIDDEN', { diff --git a/scripts/adr-anchors.json b/scripts/adr-anchors.json index 64ad269944..bf0a225c2e 100644 --- a/scripts/adr-anchors.json +++ b/scripts/adr-anchors.json @@ -100,6 +100,16 @@ "file": "packages/spec/src/kernel/functional-completeness.ts", "adrs": ["ADR-0078"], "invariant": "Every rule here cites the runtime line that silently skips the instance, and every deliberate NON-rule cites the evidence that exempts it (ADR-0078 §6). `multiselect` without `options` is NOT flagged — `record-validator.ts` blesses it verbatim as free-form tags, which is §1 case (3) genuinely-optional; `user` relationships and `timeline`/`tree` views are exempt for their own stated reasons. A rule added without its skip-site citation, or an exemption 'fixed', is a false prescription: it tells an AI author to change working metadata, which is the failure this gate exists to prevent." + }, + { + "file": "packages/plugins/driver-sql/src/schema-drift.ts", + "adrs": ["ADR-0120"], + "invariant": "The organization key part of every organization-scoped unique index is the NULL-safe COALESCE form — `COALESCE(, '__global__')` — never the bare column: SQL UNIQUE is NULL-distinct, so the bare composite enforces NOTHING on NULL-organization rows, which on a single-tenant stack is every row (#5030). Declared-index `unique: 'global'`/bare `true` stays VERBATIM (the #3696 contract, now the 'global' arm of the vocabulary); `'organization'` prepends the key part at registration. Expected and physical sides compare through the SAME normalization, literal-agnostic on the COALESCE literal — two spellings of the literal are one constraint, never drift (#4884)." + }, + { + "file": "packages/plugins/driver-sql/src/sql-driver.ts", + "adrs": ["ADR-0120"], + "invariant": "The bare-composite → NULL-safe tightening migrates through the ceremony (ADR-0120 D4): a `recreate_index` gated by the duplicate pre-flight probe — clean data grades it `safe` (dev autoMigrate may apply), duplicates BLOCK it with a row report and the old index stays in place; apply re-probes, so even --allow-destructive cannot drop a constraint whose replacement is not creatable. Storage stays NULL — GLOBAL_TENANT is an index-key fold, never written to the organization column." } ] }