From d334ea12694351ad3c57f3386da87c34d8c28f16 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 09:21:20 +0000 Subject: [PATCH] fix(metadata): sys_view_definition active-row index gets a NULL-safe key (#6417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQL UNIQUE treats NULLs as mutually distinct, so `(name, organization_id, owner)` constrained PERSONAL views only: `owner` is NULL for SHARED views and `organization_id` is NULL for environment-level ones, and two same-name active shared views could coexist inside one tenant while `name` is declared as the globally unique qualified view id. Per the maintainer ruling of 2026-08-08 that is now forbidden. The runtime migration #5839 introduced materializes the key NULL-safe, copying one in-repo precedent per column class rather than inventing a form: ADR-0120 D3's `COALESCE(organization_id, '__global__')` for the tenant column, and `ensureOverlayIndex`'s `COALESCE(package_id, '')` form as `COALESCE(owner, '')` for the non-tenant discriminator. Storage is untouched. Unlike #5839 this is a tightening and can fail to build on existing data, so the conflict branch is now a live path and takes ADR-0120 D4's disposition in full: keep the previous index, name the key that is not enforced, ship the exact query that lists the offending rows, point at `os migrate plan`, never block boot. The degradation reports are raised from `info`/`warn` to `error`: what the missing DDL costs changed in kind, from slot recycling (functional, the next user finds out) to an integrity guarantee the platform states it enforces while continuing to look healthy — AGENTS.md's durability arm. PR #6415's honest pin (`does NOT close the pre-existing NULL-distinct hole for shared views`) is flipped to the positive assertion. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw --- .../view-definition-null-safe-active-index.md | 72 +++++ .../src/objects/sys-view-definition.object.ts | 22 +- .../view-definition-active-index.test.ts | 269 +++++++++++++++--- .../view-definition-active-index.ts | 237 ++++++++++++--- 4 files changed, 531 insertions(+), 69 deletions(-) create mode 100644 .changeset/view-definition-null-safe-active-index.md 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, ); }