diff --git a/.changeset/view-definition-null-safe-active-index.md b/.changeset/view-definition-null-safe-active-index.md new file mode 100644 index 0000000000..4828322c82 --- /dev/null +++ b/.changeset/view-definition-null-safe-active-index.md @@ -0,0 +1,72 @@ +--- +"@objectstack/metadata-protocol": patch +"@objectstack/metadata-core": patch +--- + +fix(metadata): two same-name active SHARED views can no longer coexist — `sys_view_definition`'s active-row index gets a NULL-safe key (#6417) + +#5839 / PR #6415 delivered "unique among ACTIVE rows" for `sys_view_definition` +as a runtime partial UNIQUE index, and deliberately changed only the index's +**row scope** — that is what made it strictly weaker than the index it replaced +and therefore incapable of failing on existing data. It also left the other +half of the same index broken, and pinned that gap honestly rather than closing +it. + +SQL UNIQUE treats NULLs as mutually **distinct**. `owner` is NULL for SHARED +views and `organization_id` is NULL for environment-level ones, so +`(name, organization_id, owner)` constrained **personal views only**. Measured +on real SQLite over the driver's own DDL: + +```text +two ACTIVE personal views, same (name, org, owner) : REJECTED +two ACTIVE shared views (owner NULL) : OK ← unconstrained +two ACTIVE env-level views (organization_id NULL) : OK ← unconstrained +``` + +Two same-name shared views inside one tenant were therefore reachable, while +`name` is declared as the globally unique qualified view id (`object.viewKey`) +— so the view switcher, which aggregates and de-duplicates by `name`, and every +read path that locates a view by name, had no defined answer about which row +they got. + +**What changes.** Per the maintainer ruling of 2026-08-08 this is now forbidden. +The same runtime migration materializes the key NULL-safe, folding each nullable +part's NULLs into one bucket that is unique among itself: + +```sql +CREATE UNIQUE INDEX idx_sys_view_def_active ON sys_view_definition + (name, COALESCE(organization_id, '__global__'), COALESCE(owner, '')) + WHERE state = 'active' +``` + +Both spellings are copied from an existing in-repo precedent rather than +invented: `'__global__'` is ADR-0120 D3's reserved sentinel for the tenant +column (the driver's `GLOBAL_TENANT`), and `COALESCE(owner, '')` is +`ensureOverlayIndex`'s `COALESCE(package_id, '')` form for a non-tenant nullable +discriminator. Neither can collide with real data — an organization id may never +equal `'__global__'`, and an owner is a user id, never the empty string. +**Storage is untouched**: rows keep their NULLs, only the index folds them, so +`WHERE owner = ''` still matches nothing. + +Unchanged: archived rows stay exempt (#5839's active-only scoping survives, on +shared views too), a shared view and a personal view may still share a name, and +so may two tenants' or two environments' rows. + +**This is a tightening, so it can fail to build.** Unlike #5839, rows that +violate the new key exist in the wild today, precisely because nothing rejected +them. The migration probes before it replaces anything, and on a conflict takes +ADR-0120 D4's disposition: the previous index is left in place (the table is +never left unconstrained), the report names the key that is not enforced, ships +the exact `GROUP BY … HAVING COUNT(*) > 1` query that lists the offending rows, +points at `os migrate plan` — and the boot continues. Resolve the duplicate +active shared views, restart, and the tightening applies itself. + +Dialects with no partial indexes (MySQL/MariaDB) keep the declared bare +composite, which is ADR-0120 D3's own degradation. That report is **raised from +`info` to `error`**: under #5839 alone the dialect lost slot recycling, a +functional degradation the next user hits immediately, but it now loses an +integrity guarantee the platform states it enforces while continuing to look +healthy — AGENTS.md's durability arm. The line names both gaps that stay open +there and the duplicate-listing query. The unclassifiable-failure arm is raised +with it, so the failure nobody can name is never reported more quietly than the +one that has a name. diff --git a/packages/metadata-core/src/objects/sys-view-definition.object.ts b/packages/metadata-core/src/objects/sys-view-definition.object.ts index 19d29b4b29..93d94efbaa 100644 --- a/packages/metadata-core/src/objects/sys-view-definition.object.ts +++ b/packages/metadata-core/src/objects/sys-view-definition.object.ts @@ -120,7 +120,8 @@ export const SysViewDefinitionObject = ObjectSchema.create({ indexes: [ // A given view name is unique per (organization, owner) — a shared view - // (owner NULL) and each user's personal views don't collide. + // (owner NULL) and each user's personal views don't collide, AND two + // shared views may not share a name either (#6417). // // ⚠️ This entry is the FALLBACK shape, not the delivered one. It carried // `partial: "state = 'active'"` until #5248 / #4943 retired the key, @@ -139,10 +140,27 @@ export const SysViewDefinitionObject = ObjectSchema.create({ // `syncDeclaredIndexes` (which skips by name) never re-imposes the // unrestricted form on a later boot. // + // ⚠️ The KEY below is NULL-DISTINCT, which is a second gap the declaration + // cannot close on its own (#6417). `owner` is NULL for SHARED views and + // `organization_id` is NULL for environment-level ones, and SQL UNIQUE + // treats NULLs as mutually distinct — so what this entry constrains is + // PERSONAL views only, measured: two active shared views could carry one + // name. Per the maintainer ruling of 2026-08-08 that is forbidden, and the + // same runtime migration delivers it, again without touching this + // declaration: it materializes the key NULL-safe, as + // `(name, COALESCE(organization_id, '__global__'), COALESCE(owner, ''))` + // — ADR-0120 D3's sentinel for the tenant column, `ensureOverlayIndex`'s + // `COALESCE(package_id, '')` form for the non-tenant one. Storage keeps + // its NULLs; only the index folds them into a bucket. + // // Keep this declaration exactly as it is. It is what dialects without // partial indexes (MySQL) and hosts that never run the migration fall back // to, and the migration deliberately leaves it untouched when it cannot - // build the partial form — degraded to this behaviour, never below it. + // build the partial NULL-safe form — degraded to this behaviour, never + // below it. Rewriting it to `unique: 'organization'` would NOT be the same + // thing: that is ADR-0120 D1's declared-scope vocabulary, staged for the + // protocol-18 train (D7), and it scopes the tenant column only — `owner` + // would stay NULL-distinct. { name: 'idx_sys_view_def_active', fields: ['name', 'organization_id', 'owner'], diff --git a/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts b/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts index d24443a198..4a19ceb584 100644 --- a/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts @@ -8,14 +8,18 @@ import { ensureViewDefinitionActiveIndex, resolveIndexExec, buildActiveIndexSql, + buildDuplicateProbeSql, classifyIndexFailure, + viewActiveIndexKeyParts, VIEW_ACTIVE_INDEX_NAME, VIEW_ACTIVE_PROBE_INDEX_NAME, + VIEW_ACTIVE_NULL_SENTINELS, type IndexExec, } from './view-definition-active-index.js'; /** - * `sys_view_definition` — "unique among ACTIVE rows" (#5839). + * `sys_view_definition` — "unique among ACTIVE rows" (#5839), on a key that is + * NULL-SAFE (#6417). * * Every assertion here runs against a REAL SQLite database, because the whole * defect was a claim about DDL that no test ever asked the database to confirm. @@ -30,7 +34,7 @@ import { * in the lockfile purely for fixture purposes. The built-in gives the same real * SQLite — real partial indexes, real UNIQUE enforcement — for free. */ -describe('sys_view_definition active-row uniqueness (#5839)', () => { +describe('sys_view_definition active-row uniqueness (#5839) on a NULL-safe key (#6417)', () => { let db: DatabaseSync; let exec: IndexExec; @@ -101,7 +105,7 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { expect(insert('v2', 'lead.my_pipeline', 'org1', 'user1', 'active').ok).toBe(true); }); - it('the index it leaves behind is the PARTIAL one, under the DECLARED name', async () => { + it('the index it leaves behind is the PARTIAL, NULL-SAFE one, under the DECLARED name', async () => { await ensureViewDefinitionActiveIndex(exec); const ddl = indexDdl(VIEW_ACTIVE_INDEX_NAME); @@ -109,6 +113,11 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { // The predicate the declaration always promised and never delivered. expect(ddl!.toLowerCase()).toContain("where state = 'active'"); expect(ddl!.toLowerCase()).toContain('unique'); + // …over the NULL-safe key parts (#6417), each copied from its own + // in-repo precedent: ADR-0120 D3's sentinel for the tenant column, + // `ensureOverlayIndex`'s `COALESCE(package_id, '')` form for `owner`. + expect(ddl).toContain("COALESCE(organization_id, '__global__')"); + expect(ddl).toContain("COALESCE(owner, '')"); // Reusing the declared name is what stops `syncDeclaredIndexes` — which // skips by name — from re-imposing the unrestricted form next boot. expect(ddl).not.toEqual(DECLARED_INDEX_DDL); @@ -118,13 +127,17 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { // ── Uniqueness is scoped, NOT relaxed ───────────────────────────────── - it('still rejects two ACTIVE rows with the same (name, organization_id, owner)', async () => { + /** CASE 2 of #6417 — the one bucket that WAS already constrained. */ + it('still rejects two ACTIVE PERSONAL rows with the same (name, organization_id, owner)', async () => { await ensureViewDefinitionActiveIndex(exec); expect(insert('v3', 'lead.hot', 'org1', 'user1', 'active').ok).toBe(true); const dup = insert('v4', 'lead.hot', 'org1', 'user1', 'active'); expect(dup.ok).toBe(false); + // The constraint's IDENTITY, not merely "something threw": SQLite names + // the index for an expression key, so this pins WHICH constraint fired. expect(dup.error).toContain('UNIQUE constraint failed'); + expect(dup.error).toContain(VIEW_ACTIVE_INDEX_NAME); }); it('admits MANY archived rows under one name — the slot is scoped, not shared', async () => { @@ -148,22 +161,85 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { expect(insert('o3', 'lead.mine', 'org2', 'user1', 'active').ok).toBe(true); }); + // ── The NULL-distinct hole, now CLOSED (#6417) ──────────────────────── + /** - * Honest scope note. `owner` is NULL for SHARED views and `organization_id` - * is NULL for env-wide ones, and SQL UNIQUE treats NULLs as DISTINCT — so - * two active SHARED views may carry the same name. That hole is older than - * this migration and is NOT what #5839 decided: the partial index changes - * the ROW SCOPE (`WHERE state = 'active'`) and deliberately leaves the KEY - * spelling alone, which is also what makes it strictly weaker than the - * index it replaces and therefore incapable of failing on existing data. - * Pinned so the gap is a recorded fact rather than a surprise; closing it - * needs the NULL-safe key (`COALESCE`) and its own ruling — filed separately. + * The pin PR #6415 left here read `does NOT close the pre-existing + * NULL-distinct hole for shared views (recorded, not fixed)` and asserted + * that the second insert succeeded. The maintainer ruling of 2026-08-08 + * forbids that outcome, so the pin flips: same fixture, opposite verdict. + * + * `owner` is NULL for SHARED views, and SQL UNIQUE treats NULLs as mutually + * DISTINCT, so the raw column constrained nothing for them — + * `COALESCE(owner, '')` folds every shared row into ONE bucket that is + * unique among itself. CASE 3 of the issue, measured. */ - it('does NOT close the pre-existing NULL-distinct hole for shared views (recorded, not fixed)', async () => { + it('rejects a second ACTIVE SHARED view (owner NULL) under the same name', async () => { await ensureViewDefinitionActiveIndex(exec); expect(insert('s1', 'lead.team', 'org1', null, 'active').ok).toBe(true); - expect(insert('s2', 'lead.team', 'org1', null, 'active').ok).toBe(true); + const dup = insert('s2', 'lead.team', 'org1', null, 'active'); + expect(dup.ok).toBe(false); + expect(dup.error).toContain('UNIQUE constraint failed'); + expect(dup.error).toContain(VIEW_ACTIVE_INDEX_NAME); + }); + + /** CASE 4 — `organization_id` is NULL for environment-level views. */ + it('rejects a second ACTIVE ENVIRONMENT-LEVEL view (organization_id NULL) under the same name', async () => { + await ensureViewDefinitionActiveIndex(exec); + + expect(insert('e1', 'lead.env', null, 'user9', 'active').ok).toBe(true); + const dup = insert('e2', 'lead.env', null, 'user9', 'active'); + expect(dup.ok).toBe(false); + expect(dup.error).toContain('UNIQUE constraint failed'); + expect(dup.error).toContain(VIEW_ACTIVE_INDEX_NAME); + }); + + /** Both nullable parts NULL at once — an environment-level SHARED view. */ + it('rejects a second ACTIVE view with BOTH organization_id and owner NULL', async () => { + await ensureViewDefinitionActiveIndex(exec); + + expect(insert('b1', 'lead.both', null, null, 'active').ok).toBe(true); + const dup = insert('b2', 'lead.both', null, null, 'active'); + expect(dup.ok).toBe(false); + expect(dup.error).toContain('UNIQUE constraint failed'); + expect(dup.error).toContain(VIEW_ACTIVE_INDEX_NAME); + }); + + /** + * The tightening must not have swallowed #5839's row scoping. Shared views + * are the bucket #6417 newly constrains, so the archived exemption is + * re-proved THERE and not only on personal rows. + */ + it('archived SHARED rows stay exempt — the #5839 active-only scoping survives', async () => { + await ensureViewDefinitionActiveIndex(exec); + + expect(insert('sa1', 'lead.shared_arc', 'org1', null, 'archived').ok).toBe(true); + expect(insert('sa2', 'lead.shared_arc', 'org1', null, 'archived').ok).toBe(true); + // …plus exactly one active shared view alongside them. + expect(insert('sa3', 'lead.shared_arc', 'org1', null, 'active').ok).toBe(true); + expect(insert('sa4', 'lead.shared_arc', 'org1', null, 'active').ok).toBe(false); + + // And the recycling the whole of #5839 was about, on a shared view. + archive('sa3'); + expect(insert('sa5', 'lead.shared_arc', 'org1', null, 'active').ok).toBe(true); + }); + + /** + * The half of the declaration's comment that WAS true stays true: a shared + * view and a personal view may carry one name, and so may two environments' + * / two tenants' rows. The sentinels are chosen so they cannot collide with + * real data — an organization id may never be `'__global__'` (ADR-0120 D3 + * reserves the token) and an owner is a user id, never `''`. + */ + it('a SHARED view and a PERSONAL view still share a name, across tenants too', async () => { + await ensureViewDefinitionActiveIndex(exec); + + expect(insert('x1', 'lead.mix', 'org1', null, 'active').ok).toBe(true); + expect(insert('x2', 'lead.mix', 'org1', 'user1', 'active').ok).toBe(true); + // A shared view in another tenant, and the environment-level one. + expect(insert('x3', 'lead.mix', 'org2', null, 'active').ok).toBe(true); + expect(insert('x4', 'lead.mix', null, null, 'active').ok).toBe(true); }); // ── Idempotence ─────────────────────────────────────────────────────── @@ -197,17 +273,43 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { expect(indexDdl(VIEW_ACTIVE_INDEX_NAME)!.toLowerCase()).toContain("where state = 'active'"); }); + /** + * The upgrade every already-migrated deployment takes: the table arrives + * carrying #5839's partial index with the OLD, NULL-distinct key, and this + * run has to replace it in place — same name, tighter key. + */ + it('upgrades a table already carrying #5839\'s NULL-distinct partial index', async () => { + db.exec(`DROP INDEX ${VIEW_ACTIVE_INDEX_NAME}`); + db.exec( + `CREATE UNIQUE INDEX ${VIEW_ACTIVE_INDEX_NAME} ON sys_view_definition ` + + `(name, organization_id, owner) WHERE state = 'active'`, + ); + // The #5839 shape admits two shared views under one name… + expect(insert('pre1', 'lead.pre', 'org1', null, 'active').ok).toBe(true); + db.prepare("UPDATE sys_view_definition SET state='archived' WHERE id='pre1'").run(); + + const result = await ensureViewDefinitionActiveIndex(exec); + + expect(result.status).toBe('created'); + expect(indexDdl(VIEW_ACTIVE_INDEX_NAME)).toContain("COALESCE(owner, '')"); + // …and after the upgrade it does not. + expect(insert('post1', 'lead.pre', 'org1', null, 'active').ok).toBe(true); + expect(insert('post2', 'lead.pre', 'org1', null, 'active').ok).toBe(false); + }); + // ── Degradation: the constraint is never destroyed ──────────────────── /** - * MySQL has no partial indexes. The paradigm this module follows + * MySQL has no partial indexes (and, before 8.0.13, no functional key parts + * for the `COALESCE` parts either). The paradigm this module follows * (`ensureOverlayIndex`) drops the legacy index BEFORE attempting the * partial one, so a rejected `WHERE` leaves the table with no unique index * at all. This module probes first for exactly that reason, and this test * is the proof: after a dialect refusal the ORIGINAL index is still there, - * still enforcing, byte-for-byte unchanged. + * still enforcing, byte-for-byte unchanged — which IS ADR-0120 D3's + * bare-composite degradation, reached by keeping rather than rebuilding. */ - it('a dialect without partial indexes keeps the original UNIQUE index intact', async () => { + it('a dialect that cannot build the form keeps the original UNIQUE index intact', async () => { const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; const mysqlish: IndexExec = async (sql: string) => { if (/where/i.test(sql)) { @@ -226,24 +328,77 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { expect(indexDdl(VIEW_ACTIVE_INDEX_NAME)).toEqual(DECLARED_INDEX_DDL); expect(insert('m1', 'lead.x', 'org1', 'user1', 'active').ok).toBe(true); expect(insert('m2', 'lead.x', 'org1', 'user1', 'active').ok).toBe(false); - // Reported, and not as an operator error — this is expected on MySQL. - expect(logger.info).toHaveBeenCalledTimes(1); - expect(String(logger.info.mock.calls[0]![0])).toContain('no partial indexes'); - expect(logger.error).not.toHaveBeenCalled(); + // Reported at `error`, RAISED from #6415's `info` by #6417: the same + // missing DDL now costs an integrity guarantee the platform states it + // enforces (not merely slot recycling), so it lands in the durability + // arm of AGENTS.md's rule — the system keeps looking healthy while + // duplicates accumulate. An `error` owes the consequence and the fix, + // and both gaps that stay open are named. + expect(logger.error).toHaveBeenCalledTimes(1); + const note = String(logger.error.mock.calls[0]![0]); + expect(note).toContain('UNRESTRICTED and NULL-distinct'); + expect(note).toContain('keeps looking healthy'); + expect(note).toContain('#5839'); + expect(note).toContain('#6417'); + // The fix, and the query that surfaces the duplicates meanwhile. + expect(note).toContain('SQLite/PostgreSQL'); + expect(note).toContain(buildDuplicateProbeSql()); + expect(logger.info).not.toHaveBeenCalled(); + }); + + /** + * The tightening-failure path, on a REAL database rather than a mocked + * throw: seed the exact duplicate pair the old index admitted (#6417 CASE + * 3), then run the migration and assert ADR-0120 D4's whole disposition. + * + * This is the case #5839 could not have — its partial index was strictly + * WEAKER than the one it replaced, so it could not fail on existing data. + * A NULL-safe key can, and does. + */ + it('a pre-existing duplicate pair blocks the tightening — old index kept, rows named, boot survives', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + // Two ACTIVE shared views under one name — legal until today. + expect(insert('d1', 'lead.team', 'org1', null, 'active').ok).toBe(true); + expect(insert('d2', 'lead.team', 'org1', null, 'active').ok).toBe(true); + + const result = await ensureViewDefinitionActiveIndex(exec, logger); + + // Reported, never thrown: a boot must not fail over an index. + expect(result.status).toBe('conflict'); + expect(result.detail).toContain('UNIQUE constraint failed'); + // The PREVIOUS index survives byte-for-byte, and still enforces what it + // always did — at no point is the table left unconstrained. + expect(indexDdl(VIEW_ACTIVE_INDEX_NAME)).toEqual(DECLARED_INDEX_DDL); + expect(insert('d3', 'lead.hot', 'org1', 'user1', 'active').ok).toBe(true); + expect(insert('d4', 'lead.hot', 'org1', 'user1', 'active').ok).toBe(false); + // No probe residue, and no half-built index under either name. + expect(indexDdl(VIEW_ACTIVE_PROBE_INDEX_NAME)).toBeUndefined(); + + // D4's wording contract: what is not enforced, the rows, the command. + expect(logger.error).toHaveBeenCalledTimes(1); + const msg = String(logger.error.mock.calls[0]![0]); + expect(msg).toContain("COALESCE(owner, '')"); + expect(msg).toContain('os migrate plan'); + expect(msg).toContain(buildDuplicateProbeSql()); + + // …and that shipped query really does name the offending rows, on this + // very database. It is not a decorative string. + const offenders = db.prepare(buildDuplicateProbeSql()).all() as Array>; + expect(offenders).toHaveLength(1); + expect(offenders[0]!.name).toBe('lead.team'); + expect(offenders[0]!.duplicate_rows).toBe(2); }); /** - * ADR-0120 D4's wording contract: name what is NOT enforced and the command - * that lists the offending rows, at `error`, without failing the boot. + * The same disposition when the driver reports the conflict in MySQL's + * wording rather than SQLite's — the classification, not the dialect, is + * what selects the branch. */ - it('conflicting rows are named at error level and the old index survives', async () => { + it('conflicting rows are named at error level and the old index survives (MySQL wording)', async () => { const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; const conflicting: IndexExec = async (sql: string) => { if (/CREATE UNIQUE INDEX/i.test(sql)) { - throw new Error( - 'UNIQUE constraint failed: sys_view_definition.name, ' + - 'sys_view_definition.organization_id, sys_view_definition.owner', - ); + throw new Error("Duplicate entry 'lead.team-__global__-' for key 'idx_sys_view_def_active'"); } return db.exec(sql); }; @@ -254,10 +409,30 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { expect(indexDdl(VIEW_ACTIVE_INDEX_NAME)).toEqual(DECLARED_INDEX_DDL); expect(logger.error).toHaveBeenCalledTimes(1); const msg = String(logger.error.mock.calls[0]![0]); - expect(msg).toContain('name, organization_id, owner'); + expect(msg).toContain("COALESCE(organization_id, '__global__')"); expect(msg).toContain('os migrate plan'); }); + /** + * The catch-all arm. Same class as the dialect arm — the DDL did not run + * and nothing else looks wrong — so it is `error` too, and it must not be + * QUIETER than the failure we can name. + */ + it('an unclassifiable failure is reported at error and leaves the index alone', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const broken: IndexExec = async (sql: string) => { + if (/CREATE UNIQUE INDEX/i.test(sql)) throw new Error('disk I/O error'); + return db.exec(sql); + }; + + const result = await ensureViewDefinitionActiveIndex(broken, logger); + + expect(result.status).toBe('failed'); + expect(indexDdl(VIEW_ACTIVE_INDEX_NAME)).toEqual(DECLARED_INDEX_DDL); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(String(logger.error.mock.calls[0]![0])).toContain('can still coexist'); + }); + it('a host with no raw-SQL driver is a silent no-op, not a failure', async () => { const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; const result = await ensureViewDefinitionActiveIndex(undefined, logger); @@ -276,16 +451,46 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { 'conflict', ); expect(classifyIndexFailure('near "WHERE": syntax error')).toBe('unsupported'); + // MariaDB's refusal of a functional key part, which #6417 introduces. + expect(classifyIndexFailure('Functional index on a column is not supported')).toBe('unsupported'); expect(classifyIndexFailure('disk I/O error')).toBe('failed'); }); - it('buildActiveIndexSql scopes rows without changing the declared key', () => { + it('buildActiveIndexSql scopes rows AND spells the key NULL-safe', () => { const sql = buildActiveIndexSql(VIEW_ACTIVE_INDEX_NAME); - expect(sql).toContain('(name, organization_id, owner)'); + expect(sql).toContain("(name, COALESCE(organization_id, '__global__'), COALESCE(owner, ''))"); expect(sql).toContain("WHERE state = 'active'"); expect(sql).toContain('IF NOT EXISTS'); }); + /** + * The sentinels are the point of #6417, so they are asserted as literals + * rather than only through the builder that produces them: `'__global__'` + * is ADR-0120 D3's reserved token (the driver's `GLOBAL_TENANT`), `''` is + * `ensureOverlayIndex`'s `COALESCE(package_id, '')` form. A silent change + * to either would re-partition every existing index without a migration. + */ + it('pins the two sentinels and the key order', () => { + expect(VIEW_ACTIVE_NULL_SENTINELS).toEqual({ organization_id: '__global__', owner: '' }); + expect(viewActiveIndexKeyParts()).toEqual([ + 'name', + "COALESCE(organization_id, '__global__')", + "COALESCE(owner, '')", + ]); + }); + + it('buildDuplicateProbeSql groups by exactly the index key the CREATE uses', () => { + const probe = buildDuplicateProbeSql(); + // Same key parts as the index, so what it reports and what the index + // rejects cannot diverge. + expect(probe).toContain(`GROUP BY ${viewActiveIndexKeyParts().join(', ')}`); + // Selected columns are the RAW ones — an operator needs the stored + // values (NULLs included), not the folded bucket keys. + expect(probe).toContain('SELECT name, organization_id, owner, COUNT(*) AS duplicate_rows'); + expect(probe).toContain("WHERE state = 'active'"); + expect(probe).toContain('HAVING COUNT(*) > 1'); + }); + it('resolveIndexExec prefers raw(), falls back to execute(), else undefined', async () => { const raw = vi.fn(async () => undefined); const execute = vi.fn(async () => undefined); diff --git a/packages/metadata-protocol/src/migrations/view-definition-active-index.ts b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts index 7db2c13868..3ea5be0720 100644 --- a/packages/metadata-protocol/src/migrations/view-definition-active-index.ts +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts @@ -59,17 +59,62 @@ * boot that migrates; the benefit is that the failure mode cannot destroy a * live constraint. * - * ## Why a conflict is not expected (and is still reported) - * - * The partial index is strictly WEAKER than the unrestricted one it replaces — - * its active rows are a subset of all rows — so any database that satisfied - * the old constraint necessarily satisfies the new one. Existing "archived row - * occupies the slot" duplicates cannot exist yet, precisely because the old - * index rejected them. A conflict is therefore only reachable on a table whose - * unique index was never in force (created out-of-band, or an earlier sync - * that skipped it). That case is reported the way ADR-0120 D4 reports its own: - * at `error`, naming the columns that are not enforced and the command that - * lists the offending rows — and the boot continues. + * ## The KEY is NULL-safe too (#6417, maintainer ruling 2026-08-08) + * + * #5839 changed only the ROW SCOPE and deliberately left the key spelling + * alone. That left the other half of the same index broken, and #6415 pinned + * the gap honestly rather than closing it: SQL UNIQUE treats NULLs as + * DISTINCT, `owner` is NULL for SHARED views and `organization_id` is NULL for + * environment-level ones, so the index constrained PERSONAL views only. + * Measured on real SQLite over the driver's own DDL, before this half landed: + * + * ```text + * two ACTIVE personal views, same (name, org, owner) : REJECTED + * two ACTIVE shared views (owner NULL) : OK ← unconstrained + * two ACTIVE env-level views(organization_id NULL) : OK ← unconstrained + * ``` + * + * The maintainer ruling forbids that: two same-name active shared (or + * environment-level) views may not coexist, because `name` is declared as the + * globally unique qualified view id (`.`) and every read path + * that locates a view by name otherwise has no defined answer. + * + * The mechanism copies the two in-repo precedents rather than inventing a + * third — `COALESCE` the nullable key columns so their NULLs fold into ONE + * bucket that is unique among itself: + * + * - `organization_id` is the tenant column, so it takes **ADR-0120 D3**'s + * exact form, `COALESCE(organization_id, '__global__')` — the same sentinel + * `SqlDriver`'s `GLOBAL_TENANT` / `organizationKeyPartSql` materialize, so a + * violation message reads "the platform bucket collided" rather than as + * corrupt data. + * - `owner` is not a tenant column; it plays exactly `package_id`'s role in + * `ensureOverlayIndex` (a nullable discriminator whose NULL means "the one + * shared bucket"), so it takes THAT precedent's form, + * `COALESCE(owner, '')` — whose comment states this same NULL-distinct + * reason. + * + * Neither sentinel can collide with real data: an organization id may never + * equal `'__global__'` (reserved at the organization-creation seam, ADR-0120 + * D3) and an owner is a user id, never the empty string. Storage is untouched + * — only the INDEX folds NULL into a bucket, exactly as D3 specifies. + * + * ## Why a conflict IS expected now (and how it is reported) + * + * Under #5839 alone a conflict was near-unreachable: the partial index was + * strictly WEAKER than the unrestricted one it replaced — its active rows are + * a subset of all rows — so any database that satisfied the old constraint + * necessarily satisfied the new one. + * + * The NULL-safe key inverts that. It is a **tightening**: rows the old index + * admitted (two active shared views under one name) violate the new one, and + * such rows exist in the wild today precisely because nothing rejected them. + * So the probe-first order above stops being belt-and-braces and becomes the + * load-bearing part — and the conflict branch is a live path, not a corner. + * It is handled the way ADR-0120 D4 requires: the PREVIOUS index stays in + * place (never a table with no unique index at all), the report names the key + * that is not enforced, ships the exact query that lists the offending rows, + * points at `os migrate plan`, and the boot continues. */ /** The one table this migration touches. */ @@ -84,9 +129,53 @@ export const VIEW_ACTIVE_INDEX_NAME = 'idx_sys_view_def_active'; /** Throwaway name used to prove the partial form is possible before dropping. */ export const VIEW_ACTIVE_PROBE_INDEX_NAME = 'idx_sys_view_def_active_probe'; -/** The key the declaration promises, unchanged — only its ROW SCOPE changes. */ +/** + * The key COLUMNS the declaration names, unchanged. What #6417 changes is how + * two of them are SPELLED in the index — see {@link VIEW_ACTIVE_NULL_SENTINELS}. + */ export const VIEW_ACTIVE_INDEX_COLUMNS = ['name', 'organization_id', 'owner'] as const; +/** + * The nullable key columns and the sentinel each one's NULL folds to (#6417). + * + * A column listed here is materialized as `COALESCE(, '')` + * so its NULL rows form ONE bucket that is unique among itself, instead of + * being mutually DISTINCT and therefore unconstrained. Both spellings are + * copied from an existing in-repo precedent — neither is invented here: + * + * - `organization_id` → `'__global__'`, ADR-0120 D3's exact form for a tenant + * column (`SqlDriver`'s `GLOBAL_TENANT` / `organizationKeyPartSql`). NOT + * imported from `@objectstack/driver-sql`: this package must not depend on a + * driver. Both literals are therefore pinned as literals by the sibling + * test, so a silent edit here cannot re-partition every existing index. + * - `owner` → `''`, the `ensureOverlayIndex` precedent for a NON-tenant + * nullable discriminator (`COALESCE(package_id, '')`), whose comment states + * this same NULL-distinct reason. + * + * `name` is `required: true` and takes no sentinel. + * + * ⚠️ Storage is NOT touched: the row keeps its NULL, only the index folds it. + * `WHERE owner = ''` matches nothing, by design (ADR-0120 D3's invariant). + */ +export const VIEW_ACTIVE_NULL_SENTINELS: Readonly> = { + organization_id: '__global__', + owner: '', +}; + +/** + * The index's key parts, in key order: a bare column, or its NULL-safe + * `COALESCE` form when {@link VIEW_ACTIVE_NULL_SENTINELS} names one. + * + * One builder so the CREATE, the duplicate-listing query the conflict report + * ships, and the degradation messages can never describe different keys. + */ +export function viewActiveIndexKeyParts(): string[] { + return VIEW_ACTIVE_INDEX_COLUMNS.map((column) => { + const sentinel = VIEW_ACTIVE_NULL_SENTINELS[column]; + return sentinel === undefined ? column : `COALESCE(${column}, '${sentinel}')`; + }); +} + /** Raw-SQL seam. Mirrors `ensureOverlayIndex`: `raw()` first, `execute()` second. */ export type IndexExec = (sql: string) => Promise; @@ -122,7 +211,11 @@ function logProblem( export type EnsureViewIndexStatus = /** The partial UNIQUE index is in place under the declared name. */ | 'created' - /** The dialect rejects `CREATE INDEX … WHERE` (MySQL). Legacy index kept. */ + /** + * The dialect rejects the form — `CREATE INDEX … WHERE` (no dialect of + * MySQL has partial indexes) or the `COALESCE` functional key parts + * (MySQL < 8.0.13 / MariaDB). Legacy index kept. + */ | 'unsupported' /** Existing rows violate the key. Legacy index kept, operator told. */ | 'conflict' @@ -137,28 +230,58 @@ export interface EnsureViewIndexResult { detail?: string; } -/** `CREATE UNIQUE INDEX … WHERE state = 'active'` under the given name. */ +/** + * `CREATE UNIQUE INDEX … WHERE state = 'active'` under the given name, over the + * NULL-safe key parts (#6417). + */ export function buildActiveIndexSql(indexName: string): string { return ( `CREATE UNIQUE INDEX IF NOT EXISTS ${indexName} ` + - `ON ${VIEW_DEFINITION_TABLE} (${VIEW_ACTIVE_INDEX_COLUMNS.join(', ')}) ` + + `ON ${VIEW_DEFINITION_TABLE} (${viewActiveIndexKeyParts().join(', ')}) ` + `WHERE state = 'active'` ); } +/** + * The query that lists the rows blocking the tightening — ADR-0120 D4's + * "name the offending rows", shipped inside the conflict report so an operator + * has it without waiting for `os migrate plan`. + * + * It GROUPs by exactly the index's own key parts, so what it reports and what + * the index rejects cannot diverge. Dialect-neutral: `COALESCE`, `GROUP BY` + * and `HAVING` are ANSI on every engine this platform runs on. + */ +export function buildDuplicateProbeSql(): string { + return ( + `SELECT ${VIEW_ACTIVE_INDEX_COLUMNS.join(', ')}, COUNT(*) AS duplicate_rows ` + + `FROM ${VIEW_DEFINITION_TABLE} WHERE state = 'active' ` + + `GROUP BY ${viewActiveIndexKeyParts().join(', ')} HAVING COUNT(*) > 1` + ); +} + /** * Classify a failed `CREATE UNIQUE INDEX … WHERE`. * - * Duplicate-row wording is checked BEFORE predicate wording: MySQL's duplicate + * Duplicate-row wording is checked BEFORE dialect wording: MySQL's duplicate * error mentions the key, and some drivers wrap both facts in one string, so * the more specific verdict has to win or a real data conflict would be - * misreported as "this dialect has no partial indexes". + * misreported as "this dialect cannot build this index". That ordering matters + * more since #6417 — the tightening makes a data conflict a LIVE path, not the + * near-unreachable corner it was under #5839 alone. + * + * The dialect arm covers both refusals a single `unsupported` verdict has to + * stand for, because MySQL hits them together and one error string cannot be + * split: no partial indexes at all, and (before 8.0.13 / on MariaDB) no + * functional key parts for the `COALESCE` parts. Both leave the same outcome — + * the declared bare composite stays — so one verdict is enough. */ export function classifyIndexFailure(message: string): EnsureViewIndexStatus { if (/unique constraint failed|duplicate entry|duplicate key value|violates unique/i.test(message)) { return 'conflict'; } - if (/partial|where clause|near "where"|near 'where'|syntax/i.test(message)) return 'unsupported'; + if (/partial|where clause|near "where"|near 'where'|functional|syntax/i.test(message)) { + return 'unsupported'; + } return 'failed'; } @@ -212,8 +335,9 @@ export function resolveIndexExec(engine: unknown): IndexExec | undefined { } /** - * Replace `sys_view_definition`'s unrestricted UNIQUE index with the - * active-row-scoped partial UNIQUE the declaration has always described. + * Replace `sys_view_definition`'s unrestricted, NULL-distinct UNIQUE index with + * the active-row-scoped (#5839) NULL-safe (#6417) partial UNIQUE the + * declaration has always described. * * Idempotent: re-running rebuilds the same definition, so the resulting schema * is byte-identical. Best-effort by design — a boot must never fail because an @@ -263,7 +387,7 @@ export async function ensureViewDefinitionActiveIndex( logger, `[metadata-protocol] could not create '${VIEW_ACTIVE_INDEX_NAME}' on ` + `"${VIEW_DEFINITION_TABLE}" after the probe succeeded — the table may currently have NO ` + - `unique index on (${VIEW_ACTIVE_INDEX_COLUMNS.join(', ')}). Restart to retry (#5839).`, + `unique index on (${viewActiveIndexKeyParts().join(', ')}). Restart to retry (#5839).`, detail, ); return { status: 'failed', detail }; @@ -283,30 +407,73 @@ function reportDegradation( logger?: EnsureViewIndexLogger, ): void { const columns = VIEW_ACTIVE_INDEX_COLUMNS.join(', '); + const keyParts = viewActiveIndexKeyParts().join(', '); if (status === 'unsupported') { - // Expected on MySQL/MariaDB — no partial indexes. Not an operator - // error and not a regression: the unrestricted UNIQUE is still there, - // which is exactly the behaviour every dialect had before #5839. - logger?.info?.( - `[metadata-protocol] this database has no partial indexes — '${VIEW_ACTIVE_INDEX_NAME}' on ` + - `"${VIEW_DEFINITION_TABLE}" stays UNRESTRICTED over (${columns}). An archived view keeps ` + - `occupying its name slot on this dialect (#5839).`, + // Expected on MySQL/MariaDB, which has no partial indexes at all (and, + // before 8.0.13, no functional key parts either). The outcome is + // exactly ADR-0120 D3's degradation — the BARE composite stays in + // force — reached by keeping the declared index rather than by + // rebuilding it, which is also `createNullSafeUniqueIndex`'s handling. + // + // ⚠️ Level RAISED from `info` to `error` by #6417, and the reason is + // that the CONSEQUENCE of the same missing DDL changed in kind, not + // that #6415's judgment was wrong. Under #5839 alone what MySQL lost + // was slot RECYCLING: a functional degradation, visibly smaller, the + // next user to re-create an archived view finds out immediately — the + // `warn`/`info` arm of AGENTS.md's rule, exactly as #6415 argued. + // What it loses now is an INTEGRITY guarantee this platform states it + // enforces: two same-name active shared views can coexist, nothing + // looks broken, and the duplicate surfaces releases later to someone + // who cannot connect it to a boot line. That is the `error` arm's own + // description ("DDL that was supposed to run did not"), the #4420 + // class the rule exists for, and the level + // `SqlDriver.createNullSafeUniqueIndex` uses for the same event. + // + // As an `error` owes: the consequence concretely, and the fix. + logProblem( + logger, + `[metadata-protocol] this database cannot build the active-row NULL-safe index — ` + + `'${VIEW_ACTIVE_INDEX_NAME}' on "${VIEW_DEFINITION_TABLE}" stays UNRESTRICTED and NULL-distinct ` + + `over (${columns}), the bare-composite degradation of ADR-0120 D3. The system keeps looking ` + + `healthy while two consequences hold on this dialect: an archived view keeps occupying its ` + + `name slot (#5839), and two same-name ACTIVE shared views (owner NULL) or environment-level ` + + `views (organization_id NULL) can still coexist even though the platform states they cannot ` + + `(#6417). MySQL/MariaDB has no partial indexes, so there is no in-dialect fix: run this ` + + `platform on SQLite/PostgreSQL for the guarantee, and meanwhile watch for duplicates with: ` + + `${buildDuplicateProbeSql()}`, + detail, ); return; } if (status === 'conflict') { + // A LIVE path since #6417: the NULL-safe key is a tightening, so rows + // the previous index admitted — two active shared views under one name + // — now block the build. ADR-0120 D4's disposition, in full: keep the + // previous index (never an unconstrained table), name the key that is + // not enforced, hand over the exact query that lists the offending + // rows, point at `os migrate plan`, and let the boot continue. logProblem( logger, - `[metadata-protocol] cannot scope '${VIEW_ACTIVE_INDEX_NAME}' on "${VIEW_DEFINITION_TABLE}" to ` + - `active rows — existing rows violate (${columns}) among state='active'. The previous index is ` + - `left in place; run "os migrate plan" for the conflicting rows, then restart (ADR-0120 D4, #5839).`, + `[metadata-protocol] cannot tighten '${VIEW_ACTIVE_INDEX_NAME}' on "${VIEW_DEFINITION_TABLE}" — ` + + `existing rows violate (${keyParts}) among state='active'. The previous index is left in ` + + `place, so (${columns}) is enforced only as far as it was before; the NULL-safe key is NOT ` + + `enforced until the duplicates are resolved. List them with: ${buildDuplicateProbeSql()} — or ` + + `run "os migrate plan" — then restart (ADR-0120 D4, #6417).`, detail, ); return; } - logger?.warn?.( - `[metadata-protocol] could not scope '${VIEW_ACTIVE_INDEX_NAME}' on "${VIEW_DEFINITION_TABLE}" to ` + - `active rows; the existing index is unchanged (#5839).`, - { detail }, + // The catch-all ('failed'), raised alongside the dialect arm above and for + // the same reason. Leaving it at `warn` would report the case we UNDERSTAND + // (a named dialect limitation) more loudly than the one we do not, while + // the consequence is identical: the DDL did not run, the NULL-safe key is + // not in force, and nothing else looks wrong. + logProblem( + logger, + `[metadata-protocol] could not rebuild '${VIEW_ACTIVE_INDEX_NAME}' on "${VIEW_DEFINITION_TABLE}" as ` + + `the active-row NULL-safe index; the existing index is unchanged, so two same-name ACTIVE shared ` + + `views can still coexist while everything else looks healthy. Fix the cause below and restart ` + + `(#5839 / #6417).`, + detail, ); }