From 616688888e5943c59ee0117a0d3431cf506ff78d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 17:04:08 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(metadata):=20sys=5Fview=5Fdefinition=20?= =?UTF-8?q?=E7=9A=84=E3=80=8C=E6=B4=BB=E8=B7=83=E8=A1=8C=E5=94=AF=E4=B8=80?= =?UTF-8?q?=E3=80=8D=E8=A1=A5=E8=BF=90=E8=A1=8C=E6=97=B6=20partial=20UNIQU?= =?UTF-8?q?E=20=E8=BF=81=E7=A7=BB=20(#5839)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `idx_sys_view_def_active` 的注释一直承诺「among active rows」，但该语义从未 在任何一层交付：声明面的 `partial` 键没有任何 driver 消费者（knex 的 `table.unique()` 无法表达 `WHERE`），已随 #5248 / #4943 退役；而与 `sys_metadata` 不同，这张表背后没有等价的运行时迁移。结果建出来的一直是无 谓词的全量 UNIQUE 索引——用户归档一个视图后无法再新建同名视图。 补 `ensureViewDefinitionActiveIndex`（照 `ensureOverlayIndex` 范式），在 `kernel:ready` 用 raw SQL 发 `CREATE UNIQUE INDEX … WHERE state = 'active'`， 复用声明的索引名以便 `syncDeclaredIndexes`（按名跳过）不会在后续启动把全量 索引加回来。 与范式的两处有意偏离，均在模块头注释里写明理由： - 先用临时探针索引验证方言与数据确实能建出部分索引，成功后才替换既有索引， 因此「旧索引已删、新索引没建成」的无约束窗口不存在（范式存在该窗口）； - `resolveIndexExec` 逐个 probe 加 try/catch，并优先 `getDriverForObject`： `ObjectQL.getDriver(objectName)` 必须带对象名、否则抛错，范式因整体 try/catch 而掩盖了这一点。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .../view-definition-active-row-unique.md | 19 + .../src/objects/sys-view-definition.object.ts | 32 +- packages/metadata-protocol/package.json | 1 + packages/metadata-protocol/src/index.ts | 20 + .../view-definition-active-index.test.ts | 341 ++++++++++++++++++ .../view-definition-active-index.ts | 312 ++++++++++++++++ packages/metadata-protocol/src/plugin.ts | 44 +++ pnpm-lock.yaml | 3 + 8 files changed, 761 insertions(+), 11 deletions(-) create mode 100644 .changeset/view-definition-active-row-unique.md create mode 100644 packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts create mode 100644 packages/metadata-protocol/src/migrations/view-definition-active-index.ts diff --git a/.changeset/view-definition-active-row-unique.md b/.changeset/view-definition-active-row-unique.md new file mode 100644 index 0000000000..5b1a37ff7d --- /dev/null +++ b/.changeset/view-definition-active-row-unique.md @@ -0,0 +1,19 @@ +--- +'@objectstack/metadata-protocol': patch +'@objectstack/metadata-core': patch +--- + +fix(metadata): `sys_view_definition` 的「活跃行唯一」真正生效——归档视图不再占用 (name, organization_id, owner) 名额 + +`sys_view_definition` 的 `idx_sys_view_def_active` 索引注释一直承诺「among active rows」，但这个语义从未在任何一层交付：声明面的 `partial: "state = 'active'"` 没有任何 driver 消费者（`syncDeclaredIndexes` 走 knex 的 `table.unique()`，无法表达 `WHERE`），该键已随 #5248 / #4943 退役；而与 `sys_metadata` 不同，这张表背后**没有**任何等价的运行时迁移。结果是建出来的一直是无谓词的全量 UNIQUE 索引——用户归档（或软删、重置）一个视图后，**无法再新建同名视图**，被一条自己刚扔掉的记录挡住。 + +现在补上运行时迁移 `ensureViewDefinitionActiveIndex`（照 `metadata-protocol` 既有的 `ensureOverlayIndex` 范式），在 `kernel:ready` 用 raw SQL 发 `CREATE UNIQUE INDEX idx_sys_view_def_active … WHERE state = 'active'`： + +- **名额可回收**——归档视图不再占用名额，同名视图可以重建； +- **唯一性不放宽**——两条 `state='active'` 的同名同域行仍然被拒； +- **复用声明的索引名**——`syncDeclaredIndexes` 按名跳过，后续每次启动都不会把全量 UNIQUE 索引重新加回来； +- **降级只会退回今天的行为，不会更低**——迁移先用一个临时探针索引验证当前方言与数据确实能建出部分索引，成功后才替换既有索引。因此 MySQL / MariaDB（无部分索引）上原有的全量 UNIQUE 索引原样保留（归档行在该方言上仍占名额，以 `info` 记录），不会出现「旧索引已删、新索引没建成」的无约束窗口。 + +`metadata-core` 侧只更新了 `sys-view-definition.object.ts` 的注释：该声明现在被明确记为**降级形态**（供无部分索引的方言与不跑该迁移的宿主使用），不应删除。 + +已知未涵盖：`owner` 为 NULL 的共享视图与 `organization_id` 为 NULL 的环境级视图，因 SQL UNIQUE 的 NULL-distinct 语义本来就不受该索引约束。这是早于本次修复的既有缺口，本迁移只改变**行范围**（`WHERE state = 'active'`）而不动键的拼写——这也正是它严格弱于被替换的索引、因而不可能在存量数据上建失败的原因。该缺口已另单记录。 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 0f0be07148..19d29b4b29 100644 --- a/packages/metadata-core/src/objects/sys-view-definition.object.ts +++ b/packages/metadata-core/src/objects/sys-view-definition.object.ts @@ -122,17 +122,27 @@ export const SysViewDefinitionObject = ObjectSchema.create({ // A given view name is unique per (organization, owner) — a shared view // (owner NULL) and each user's personal views don't collide. // - // ⚠️ This entry carried `partial: "state = 'active'"` until #5248 / #4943 - // retired the key, intending "among ACTIVE rows". No driver ever emitted - // the predicate (`syncDeclaredIndexes` builds indexes through knex's - // `table.unique()`, which cannot express a `WHERE`), so the index that has - // always been created is the unrestricted one below — dropping the key is - // a zero-DDL change. Unlike `sys_metadata`, there is NO runtime migration - // issuing the partial form for this table, so the active-row scoping is - // simply not delivered anywhere today: an archived/reset view still - // occupies its (name, organization_id, owner) slot. Tracked separately — - // deciding whether this table wants an `ensureOverlayIndex`-style - // migration is a behaviour change, out of scope for the key retirement. + // ⚠️ This entry is the FALLBACK shape, not the delivered one. It carried + // `partial: "state = 'active'"` until #5248 / #4943 retired the key, + // intending "among ACTIVE rows"; no driver ever emitted the predicate + // (`syncDeclaredIndexes` builds indexes through knex's `table.unique()`, + // which cannot express a `WHERE`), so what this declaration produces is the + // unrestricted UNIQUE below — and an archived view kept occupying its + // (name, organization_id, owner) slot, so a user could not re-create a view + // they had just archived. + // + // #5839 delivers the promised scoping the same way `sys_metadata` always + // had it — a runtime migration, not a declaration: + // `metadata-protocol`'s `ensureViewDefinitionActiveIndex` issues + // `CREATE UNIQUE INDEX idx_sys_view_def_active … WHERE state = 'active'` + // in raw SQL at `kernel:ready`, reusing THIS index's name so + // `syncDeclaredIndexes` (which skips by name) never re-imposes the + // unrestricted form on a later boot. + // + // 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. { name: 'idx_sys_view_def_active', fields: ['name', 'organization_id', 'owner'], diff --git a/packages/metadata-protocol/package.json b/packages/metadata-protocol/package.json index 71c4ed2d45..060de99cd7 100644 --- a/packages/metadata-protocol/package.json +++ b/packages/metadata-protocol/package.json @@ -43,6 +43,7 @@ }, "devDependencies": { "@types/node": "^26.1.2", + "better-sqlite3": "^13.0.2", "tsup": "^8.5.1", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 6f354ce8eb..26ee2a7fb9 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -7,6 +7,26 @@ export { ObjectStackProtocolImplementation, ConcurrentUpdateError, normalizeView export { recordNotFoundError } from './protocol.js'; export { createMetadataProtocolPlugin, assembleMetadataProtocol } from './plugin.js'; export type { MetadataProtocolPluginOptions } from './plugin.js'; + +// [#5839] `sys_view_definition`'s active-row uniqueness, delivered as a runtime +// partial-UNIQUE migration (the `ensureOverlayIndex` paradigm, for the one other +// table that declared the same intent with nothing behind it). +export { + ensureViewDefinitionActiveIndex, + resolveIndexExec, + buildActiveIndexSql, + classifyIndexFailure, + VIEW_DEFINITION_TABLE, + VIEW_ACTIVE_INDEX_NAME, + VIEW_ACTIVE_PROBE_INDEX_NAME, + VIEW_ACTIVE_INDEX_COLUMNS, +} from './migrations/view-definition-active-index.js'; +export type { + IndexExec, + EnsureViewIndexLogger, + EnsureViewIndexStatus, + EnsureViewIndexResult, +} from './migrations/view-definition-active-index.js'; export type { UninstallCleanup, UninstallCleanupOutcome } from './protocol.js'; export type { MetadataMutationEvent, MetadataMutationProjector, MutationProjectionOutcome } from './protocol.js'; export type { MetadataAuthoringGate, MetadataAuthoringGateContext } from './protocol.js'; 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 new file mode 100644 index 0000000000..1da0b759b5 --- /dev/null +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts @@ -0,0 +1,341 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import Database from 'better-sqlite3'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +import { + ensureViewDefinitionActiveIndex, + resolveIndexExec, + buildActiveIndexSql, + classifyIndexFailure, + VIEW_ACTIVE_INDEX_NAME, + VIEW_ACTIVE_PROBE_INDEX_NAME, + type IndexExec, +} from './view-definition-active-index.js'; + +/** + * `sys_view_definition` — "unique among ACTIVE rows" (#5839). + * + * 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. + * The starting index is byte-for-byte what `SqlDriver.syncDeclaredIndexes` + * emits today — `packages/drivers/driver-sql/src/declared-index-retired-keys.test.ts` + * pins that string against the same engine, so the fixture below is that test's + * measured output rather than a guess at it. + */ +describe('sys_view_definition active-row uniqueness (#5839)', () => { + let db: InstanceType; + let exec: IndexExec; + + /** Exactly the DDL `syncDeclaredIndexes` produces for the declaration. */ + const DECLARED_INDEX_DDL = + 'CREATE UNIQUE INDEX `idx_sys_view_def_active` on `sys_view_definition` ' + + '(`name`, `organization_id`, `owner`)'; + + const indexDdl = (name: string): string | undefined => + (db.prepare("SELECT sql FROM sqlite_master WHERE type='index' AND name=?").get(name) as + | { sql?: string } + | undefined)?.sql ?? undefined; + + const insert = ( + id: string, + name: string, + org: string | null, + owner: string | null, + state: string, + ): { ok: boolean; error?: string } => { + try { + db.prepare( + 'INSERT INTO sys_view_definition (id, name, organization_id, owner, state) VALUES (?,?,?,?,?)', + ).run(id, name, org, owner, state); + return { ok: true }; + } catch (e) { + return { ok: false, error: e instanceof Error ? e.message : String(e) }; + } + }; + + const archive = (id: string): void => { + db.prepare("UPDATE sys_view_definition SET state='archived' WHERE id=?").run(id); + }; + + beforeEach(() => { + db = new Database(':memory:'); + db.exec(`CREATE TABLE sys_view_definition ( + id TEXT PRIMARY KEY, name TEXT, organization_id TEXT, owner TEXT, state TEXT + );`); + db.exec(DECLARED_INDEX_DDL); + exec = async (sql: string) => db.exec(sql); + }); + + afterEach(() => { + db.close(); + }); + + // ── The nail: an archived view frees its name slot ──────────────────── + + it('BEFORE the migration, an archived view still occupies its slot (the defect)', () => { + expect(insert('v1', 'lead.my_pipeline', 'org1', 'user1', 'active').ok).toBe(true); + archive('v1'); + + const retry = insert('v2', 'lead.my_pipeline', 'org1', 'user1', 'active'); + expect(retry.ok).toBe(false); + expect(retry.error).toContain('UNIQUE constraint failed'); + }); + + it('AFTER the migration, an archived view frees its slot', async () => { + expect(insert('v1', 'lead.my_pipeline', 'org1', 'user1', 'active').ok).toBe(true); + archive('v1'); + + const result = await ensureViewDefinitionActiveIndex(exec); + expect(result.status).toBe('created'); + + // The whole point of the issue: the user can re-create the view they + // archived, under the same name. + 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 () => { + await ensureViewDefinitionActiveIndex(exec); + + const ddl = indexDdl(VIEW_ACTIVE_INDEX_NAME); + expect(ddl).toBeDefined(); + // The predicate the declaration always promised and never delivered. + expect(ddl!.toLowerCase()).toContain("where state = 'active'"); + expect(ddl!.toLowerCase()).toContain('unique'); + // 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); + // And the throwaway probe never survives. + expect(indexDdl(VIEW_ACTIVE_PROBE_INDEX_NAME)).toBeUndefined(); + }); + + // ── Uniqueness is scoped, NOT relaxed ───────────────────────────────── + + it('still rejects two ACTIVE 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); + expect(dup.error).toContain('UNIQUE constraint failed'); + }); + + it('admits MANY archived rows under one name — the slot is scoped, not shared', async () => { + await ensureViewDefinitionActiveIndex(exec); + + expect(insert('a1', 'lead.rev', 'org1', 'user1', 'archived').ok).toBe(true); + expect(insert('a2', 'lead.rev', 'org1', 'user1', 'archived').ok).toBe(true); + // …and an active one alongside them. + expect(insert('a3', 'lead.rev', 'org1', 'user1', 'active').ok).toBe(true); + // …but only ONE active one. + expect(insert('a4', 'lead.rev', 'org1', 'user1', 'active').ok).toBe(false); + }); + + it('keeps distinct owners and orgs independent', async () => { + await ensureViewDefinitionActiveIndex(exec); + + expect(insert('o1', 'lead.mine', 'org1', 'user1', 'active').ok).toBe(true); + // Same name, different user → a personal view of their own. + expect(insert('o2', 'lead.mine', 'org1', 'user2', 'active').ok).toBe(true); + // Same name, different tenant. + expect(insert('o3', 'lead.mine', 'org2', 'user1', 'active').ok).toBe(true); + }); + + /** + * 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. + */ + it('does NOT close the pre-existing NULL-distinct hole for shared views (recorded, not fixed)', 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); + }); + + // ── Idempotence ─────────────────────────────────────────────────────── + + it('is idempotent — a second run leaves the schema byte-identical', async () => { + const first = await ensureViewDefinitionActiveIndex(exec); + const afterFirst = indexDdl(VIEW_ACTIVE_INDEX_NAME); + + const second = await ensureViewDefinitionActiveIndex(exec); + const afterSecond = indexDdl(VIEW_ACTIVE_INDEX_NAME); + + expect(first.status).toBe('created'); + expect(second.status).toBe('created'); + expect(afterSecond).toEqual(afterFirst); + // No probe residue accumulates across runs. + expect(indexDdl(VIEW_ACTIVE_PROBE_INDEX_NAME)).toBeUndefined(); + }); + + it('is idempotent in BEHAVIOUR too — slot recycling survives a re-run', async () => { + await ensureViewDefinitionActiveIndex(exec); + expect(insert('v1', 'lead.p', 'org1', 'user1', 'active').ok).toBe(true); + archive('v1'); + await ensureViewDefinitionActiveIndex(exec); + expect(insert('v2', 'lead.p', 'org1', 'user1', 'active').ok).toBe(true); + }); + + it('converges from a table that never had the declared index at all', async () => { + db.exec(`DROP INDEX ${VIEW_ACTIVE_INDEX_NAME}`); + const result = await ensureViewDefinitionActiveIndex(exec); + expect(result.status).toBe('created'); + expect(indexDdl(VIEW_ACTIVE_INDEX_NAME)!.toLowerCase()).toContain("where state = 'active'"); + }); + + // ── Degradation: the constraint is never destroyed ──────────────────── + + /** + * MySQL has no partial indexes. 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. + */ + it('a dialect without partial indexes 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)) { + throw new Error( + "You have an error in your SQL syntax; check the manual … near 'WHERE state = 'active''", + ); + } + return db.exec(sql); + }; + + const result = await ensureViewDefinitionActiveIndex(mysqlish, logger); + + expect(result.status).toBe('unsupported'); + // The pre-existing constraint is untouched — degraded to yesterday's + // behaviour, never below it. + 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(); + }); + + /** + * 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. + */ + it('conflicting rows are named at error level and the old index survives', 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', + ); + } + return db.exec(sql); + }; + + const result = await ensureViewDefinitionActiveIndex(conflicting, logger); + + expect(result.status).toBe('conflict'); + 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('os migrate plan'); + }); + + 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); + expect(result.status).toBe('no-driver'); + expect(logger.error).not.toHaveBeenCalled(); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + // ── Seams ───────────────────────────────────────────────────────────── + + it('classifies duplicate-row wording as a conflict even when it also says "key"', () => { + // MySQL's duplicate message mentions the key name; the data verdict has + // to win over the dialect verdict or a real conflict reads as "no + // partial index support here". + expect(classifyIndexFailure("Duplicate entry 'a-b-c' for key 'idx_sys_view_def_active'")).toBe( + 'conflict', + ); + expect(classifyIndexFailure('near "WHERE": syntax error')).toBe('unsupported'); + expect(classifyIndexFailure('disk I/O error')).toBe('failed'); + }); + + it('buildActiveIndexSql scopes rows without changing the declared key', () => { + const sql = buildActiveIndexSql(VIEW_ACTIVE_INDEX_NAME); + expect(sql).toContain('(name, organization_id, owner)'); + expect(sql).toContain("WHERE state = 'active'"); + expect(sql).toContain('IF NOT EXISTS'); + }); + + it('resolveIndexExec prefers raw(), falls back to execute(), else undefined', async () => { + const raw = vi.fn(async () => undefined); + const execute = vi.fn(async () => undefined); + + await resolveIndexExec({ driver: { raw, execute } })!('SELECT 1'); + expect(raw).toHaveBeenCalledWith('SELECT 1'); + expect(execute).not.toHaveBeenCalled(); + + await resolveIndexExec({ driver: { execute } })!('SELECT 2'); + expect(execute).toHaveBeenCalledWith('SELECT 2'); + + // getDriver() and the drivers Map, the two other shapes the paradigm walks. + expect(resolveIndexExec({ getDriver: () => ({ raw }) })).toBeTypeOf('function'); + expect(resolveIndexExec({ drivers: new Map([['a', { execute }]]) })).toBeTypeOf('function'); + expect(resolveIndexExec({})).toBeUndefined(); + expect(resolveIndexExec({ driver: {} })).toBeUndefined(); + }); + + it('asks which driver OWNS sys_view_definition before taking any default', () => { + const owner = { raw: vi.fn(async () => undefined) }; + const fallback = { raw: vi.fn(async () => undefined) }; + const getDriverForObject = vi.fn(() => owner); + + const resolved = resolveIndexExec({ getDriverForObject, driver: fallback }); + + // The table-scoped answer wins over the engine-wide default: on a + // multi-datasource kernel the platform objects can live elsewhere. + expect(getDriverForObject).toHaveBeenCalledWith('sys_view_definition'); + void resolved!('SELECT 1'); + expect(owner.raw).toHaveBeenCalled(); + expect(fallback.raw).not.toHaveBeenCalled(); + }); + + /** + * Regression pin. `ObjectQL.getDriver(objectName)` REQUIRES an object name + * and throws `No driver available for object 'undefined'` without one, so + * the paradigm's bare `getDriver?.()` probe throws on a memory-driver + * kernel. `ensureOverlayIndex` never notices because its entire body sits + * in a swallow-everything try/catch; this resolver runs from a + * `kernel:ready` hook, where a throw failed 16 ObjectQL boot tests before + * each probe was guarded individually. + */ + it('never throws when the engine\'s driver accessors do', () => { + const thrower = () => { + throw new Error("[ObjectQL] No driver available for object 'undefined'"); + }; + + expect(resolveIndexExec({ getDriver: thrower, getDriverForObject: thrower })).toBeUndefined(); + expect(() => resolveIndexExec({ getDriver: thrower })).not.toThrow(); + expect( + resolveIndexExec({ + getDriverForObject: thrower, + get driver() { + throw new Error('boom'); + }, + drivers: new Map([['memory', { execute: vi.fn(async () => undefined) }]]), + }), + ).toBeTypeOf('function'); + }); +}); diff --git a/packages/metadata-protocol/src/migrations/view-definition-active-index.ts b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts new file mode 100644 index 0000000000..7db2c13868 --- /dev/null +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts @@ -0,0 +1,312 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `sys_view_definition` — active-row uniqueness, delivered at runtime (#5839). + * + * ## What was broken + * + * `metadata-core`'s `sys-view-definition.object.ts` declares + * + * ```ts + * { name: 'idx_sys_view_def_active', fields: ['name', 'organization_id', 'owner'], unique: true } + * ``` + * + * and its comment has always promised uniqueness **among ACTIVE rows**. It + * never delivered that. The declaration carried `partial: "state = 'active'"` + * until #5248 / #4943 retired the key, but no driver ever emitted the + * predicate — `SqlDriver.syncDeclaredIndexes` builds indexes through knex's + * `table.unique(fields, { indexName })`, which cannot express a `WHERE`. So + * the index that has always been created is the UNRESTRICTED one, and an + * archived view keeps occupying its `(name, organization_id, owner)` slot: + * archive "my pipeline", try to create "my pipeline" again, and the insert is + * rejected by a constraint about a row the user already threw away. + * + * Measured on real SQLite before this module existed: + * + * ```text + * insert active personal view : OK + * archive it; re-create same : REJECTED: UNIQUE constraint failed: + * sys_view_definition.name, sys_view_definition.organization_id, sys_view_definition.owner + * ``` + * + * `sys_metadata` never had this problem because `metadata-protocol`'s + * `ensureOverlayIndex` issues the partial form in raw SQL at runtime. This + * module is the same paradigm for the one other table that declared the same + * intent and had no runtime migration behind it (maintainer ruling + * 2026-08-06: view-name slots ARE recyclable). + * + * ## Why the index REUSES the declared name + * + * `syncDeclaredIndexes` skips by name (`if (existing.has(name)) continue`). + * Creating the partial index under `idx_sys_view_def_active` — the same name + * the object declares — is therefore what makes the fix durable: every later + * boot sees the name occupied and never re-imposes the unrestricted UNIQUE. + * A differently-named index would be silently undone on the next boot, and + * dropping the declaration instead would leave drivers that never run this + * migration with no uniqueness at all (the declaration is the fallback shape). + * + * ## Why it PROBES before dropping anything + * + * `ensureOverlayIndex` drops the legacy index and then creates the partial + * one. If that create fails — no partial-index support (MySQL), or rows that + * violate the new key — the table is left with NO unique constraint at all, + * silently. This module inverts the order: it first builds the partial index + * under a throwaway probe name, and only once that has demonstrably succeeded + * does it drop the legacy index and rebuild it under the declared name. On any + * dialect or dataset that cannot take the partial form, the existing + * unrestricted UNIQUE is left exactly as it was — degraded to today's + * behaviour, never below it. The cost is building a small index twice on the + * 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 one table this migration touches. */ +export const VIEW_DEFINITION_TABLE = 'sys_view_definition'; + +/** + * The index name — deliberately the SAME one `sys-view-definition.object.ts` + * declares, so `syncDeclaredIndexes` treats the slot as filled forever after. + */ +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. */ +export const VIEW_ACTIVE_INDEX_COLUMNS = ['name', 'organization_id', 'owner'] as const; + +/** Raw-SQL seam. Mirrors `ensureOverlayIndex`: `raw()` first, `execute()` second. */ +export type IndexExec = (sql: string) => Promise; + +/** + * Minimal logger surface, structurally compatible with `@objectstack/spec`'s + * `Logger` (every method optional so a bare console or a test double fits). + * Signatures mirror that contract exactly — notably `error(msg, Error, meta)` + * versus `warn(msg, meta)` — so a host `Logger` is assignable as-is. + */ +export interface EnsureViewIndexLogger { + info?(message: string, meta?: Record): void; + warn?(message: string, meta?: Record): void; + error?(message: string, error?: Error, meta?: Record): void; +} + +/** + * Report a problem at the loudest level the host offers, bridging the two + * different shapes (`error` takes an Error, `warn` takes metadata) so callers + * never have to care which one exists. + */ +function logProblem( + logger: EnsureViewIndexLogger | undefined, + message: string, + detail: string, +): void { + if (logger?.error) { + logger.error(message, new Error(detail)); + return; + } + logger?.warn?.(message, { detail }); +} + +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. */ + | 'unsupported' + /** Existing rows violate the key. Legacy index kept, operator told. */ + | 'conflict' + /** No raw-SQL-capable driver reachable (memory/mock hosts). No-op. */ + | 'no-driver' + /** Anything else, best-effort. Legacy index kept. */ + | 'failed'; + +export interface EnsureViewIndexResult { + status: EnsureViewIndexStatus; + /** Driver error text, when there was one. */ + detail?: string; +} + +/** `CREATE UNIQUE INDEX … WHERE state = 'active'` under the given name. */ +export function buildActiveIndexSql(indexName: string): string { + return ( + `CREATE UNIQUE INDEX IF NOT EXISTS ${indexName} ` + + `ON ${VIEW_DEFINITION_TABLE} (${VIEW_ACTIVE_INDEX_COLUMNS.join(', ')}) ` + + `WHERE state = 'active'` + ); +} + +/** + * Classify a failed `CREATE UNIQUE INDEX … WHERE`. + * + * Duplicate-row wording is checked BEFORE predicate 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". + */ +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'; + return 'failed'; +} + +/** + * Resolve a raw-SQL seam for `sys_view_definition`. + * + * Asks the engine which driver OWNS this table first + * (`getDriverForObject`) rather than grabbing the engine-wide default: on a + * multi-datasource kernel the platform objects can sit on their own datasource, + * and issuing this DDL to the wrong connection would either fail or tighten a + * table in the wrong database. + * + * ⚠️ Deliberately does NOT use `ensureOverlayIndex`'s bare `getDriver?.()`. + * `ObjectQL.getDriver(objectName)` is REQUIRED to take an object name and + * THROWS `No driver available for object 'undefined'` without one; the + * paradigm gets away with it only because its whole body sits inside a + * swallow-everything try/catch. Every probe here is individually guarded so + * this function returns `undefined` instead of throwing into a boot hook. + * + * Returns `undefined` on hosts with no raw-SQL-capable driver — memory + * engines and test doubles, where there is no DDL to issue and nothing to + * warn about. + */ +export function resolveIndexExec(engine: unknown): IndexExec | undefined { + const engineAny = engine as any; + const attempt = (fn: () => unknown): any => { + try { + return fn(); + } catch { + return undefined; + } + }; + const canRunSql = (d: any): boolean => + !!d && (typeof d.raw === 'function' || typeof d.execute === 'function'); + + let driver: any = attempt(() => engineAny?.getDriverForObject?.(VIEW_DEFINITION_TABLE)); + if (!canRunSql(driver)) driver = attempt(() => engineAny?.driver); + if (!canRunSql(driver)) driver = attempt(() => engineAny?.getDriver?.(VIEW_DEFINITION_TABLE)); + if (!canRunSql(driver) && engineAny?.drivers instanceof Map) { + driver = undefined; + for (const candidate of engineAny.drivers.values()) { + if (canRunSql(candidate)) { + driver = candidate; + break; + } + } + } + if (!canRunSql(driver)) return undefined; + if (typeof driver.raw === 'function') return (sql: string) => driver.raw(sql); + return (sql: string) => driver.execute(sql); +} + +/** + * Replace `sys_view_definition`'s unrestricted UNIQUE index with the + * active-row-scoped 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 + * index could not be tightened, which is why every branch returns a status + * instead of throwing. + */ +export async function ensureViewDefinitionActiveIndex( + exec: IndexExec | undefined, + logger?: EnsureViewIndexLogger, +): Promise { + if (!exec) return { status: 'no-driver' }; + + const drop = async (indexName: string): Promise => { + try { + await exec(`DROP INDEX IF EXISTS ${indexName}`); + } catch { + // Best-effort. MySQL has no `DROP INDEX IF EXISTS ` form at + // all; on that path the probe below has already bailed out. + } + }; + + // ── Step 1: prove the partial form is possible WITHOUT touching the + // constraint that is currently protecting the table. ────────────────── + await drop(VIEW_ACTIVE_PROBE_INDEX_NAME); + try { + await exec(buildActiveIndexSql(VIEW_ACTIVE_PROBE_INDEX_NAME)); + } catch (err: unknown) { + const detail = err instanceof Error ? err.message : String(err); + const status = classifyIndexFailure(detail); + await drop(VIEW_ACTIVE_PROBE_INDEX_NAME); + reportDegradation(status, detail, logger); + return { status, detail }; + } + await drop(VIEW_ACTIVE_PROBE_INDEX_NAME); + + // ── Step 2: the partial index is known-buildable here. Claim the + // DECLARED name so `syncDeclaredIndexes` never re-imposes the full one. ─ + await drop(VIEW_ACTIVE_INDEX_NAME); + try { + await exec(buildActiveIndexSql(VIEW_ACTIVE_INDEX_NAME)); + } catch (err: unknown) { + // Only reachable on a race with another process between the drop and + // the create — the probe already cleared dialect and data. Say so + // rather than leaving a table that now has no unique index at all. + const detail = err instanceof Error ? err.message : String(err); + logProblem( + 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).`, + detail, + ); + return { status: 'failed', detail }; + } + return { status: 'created' }; +} + +/** + * Say what is NOT enforced and what fixes it — ADR-0120 D4's wording contract, + * which `SqlDriver.createNullSafeUniqueIndex` already follows for the same + * class of event. Never fails the boot: from the outside everything else looks + * normal, so silence here is what makes the gap expensive. + */ +function reportDegradation( + status: EnsureViewIndexStatus, + detail: string, + logger?: EnsureViewIndexLogger, +): void { + const columns = VIEW_ACTIVE_INDEX_COLUMNS.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).`, + ); + return; + } + if (status === 'conflict') { + 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).`, + 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 }, + ); +} diff --git a/packages/metadata-protocol/src/plugin.ts b/packages/metadata-protocol/src/plugin.ts index 44b998bc60..4ee4ec7a7b 100644 --- a/packages/metadata-protocol/src/plugin.ts +++ b/packages/metadata-protocol/src/plugin.ts @@ -31,6 +31,10 @@ import { SysMetadataAuditObject, SysViewDefinitionObject, } from '@objectstack/metadata-core'; +import { + ensureViewDefinitionActiveIndex, + resolveIndexExec, +} from './migrations/view-definition-active-index.js'; import { ObjectStackProtocolImplementation } from './protocol.js'; export interface MetadataProtocolPluginOptions { @@ -127,6 +131,46 @@ export function assembleMetadataProtocol( ctx.registerService('protocol', protocolShim); ctx.logger.info('Protocol service registered (MetadataProtocolPlugin)'); + // #5839 — `sys_view_definition`'s "unique among ACTIVE rows" was + // never delivered by anything: the declaration's `partial` key was + // DDL-inert (and is now retired, #5248 / #4943), and unlike + // `sys_metadata` this table had no runtime migration behind it, so + // an archived view kept occupying its (name, organization_id, + // owner) slot and the user could not re-create a view they had + // just thrown away. Same paradigm as the protocol's own + // `ensureOverlayIndex`, armed from THIS assembly because this is + // the one seam both mounts share (MetadataProtocolPlugin's + // delegated mode AND ObjectQLPlugin's built-in + // `registerProtocol !== false` convenience mode) — a hook on the + // delegated plugin alone would miss the default mount entirely. + // + // Gated on `environmentId === undefined` for exactly the reason the + // registerApp block above is: per-project (cloud) kernels do not + // provision these tables locally, so there is no index of ours to + // tighten there. + // + // Deferred to `kernel:ready` because the table has to EXIST first — + // ObjectQLPlugin creates it in `start()` via `syncRegisteredSchemas`, + // which runs after every plugin's `init()`. + // + // Wrapped so it can NEVER fail a bootstrap: `kernel:ready` handlers + // propagate, and an index we could not tighten is not a reason to + // refuse to boot. (`ensureOverlayIndex` gets this by wrapping its + // whole body in a swallow-everything try/catch; the same guarantee, + // stated once here, keeps the migration itself readable.) + if (environmentId === undefined) { + (ctx as any)?.hook?.('kernel:ready', async () => { + try { + await ensureViewDefinitionActiveIndex(resolveIndexExec(ql), ctx.logger); + } catch (e: unknown) { + ctx.logger.warn( + '[metadata-protocol] sys_view_definition active-row index migration skipped (#5839)', + { error: e instanceof Error ? e.message : String(e) }, + ); + } + }); + } + // NO `analytics` fallback rides here anymore (#3891 / #3878). The // degraded shim this assembly used to register dropped the request's // ExecutionContext (aggregates ran without RLS/tenant predicates) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 520eadf03b..5befc3928f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1189,6 +1189,9 @@ importers: '@types/node': specifier: ^26.1.2 version: 26.1.2 + better-sqlite3: + specifier: ^13.0.2 + version: 13.0.2 tsup: specifier: ^8.5.1 version: 8.5.1(jiti@2.7.0)(postcss@8.5.25)(tsx@4.23.1)(typescript@6.0.3)(yaml@2.9.0) From 0773dc1eebfd5611c28653eb8f481c572cc2b790 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 17:23:58 +0000 Subject: [PATCH 2/2] =?UTF-8?q?test(metadata-protocol):=20=E8=BF=81?= =?UTF-8?q?=E7=A7=BB=E6=B5=8B=E8=AF=95=E6=94=B9=E7=94=A8=E5=86=85=E7=BD=AE?= =?UTF-8?q?=20node:sqlite=EF=BC=8C=E4=B8=8D=E5=86=8D=E6=96=B0=E5=A2=9E=20b?= =?UTF-8?q?etter-sqlite3=20=E4=BE=9D=E8=B5=96?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 本包除该测试外不需要任何 SQL 依赖，为跑测试而把一个原生模块写进 lockfile 并不划算。Node 内置的 `node:sqlite` 提供同样真实的 SQLite —— 真实的 partial index、真实的 UNIQUE 约束 —— 且零依赖。 顺带的好处：pnpm-lock.yaml 回到与 main 完全一致，本 PR 不再触发 「Validate Package Dependencies」的 OSV 扫描。该扫描只在 lockfile 变动时 运行，而它当前会因 main 上既有的 dompurify@3.4.12 公告 (GHSA-55q2-fjhq-7xh7, dependabot #16) 判红 —— 与本 PR 无关。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- packages/metadata-protocol/package.json | 1 - .../migrations/view-definition-active-index.test.ts | 13 ++++++++++--- pnpm-lock.yaml | 3 --- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/metadata-protocol/package.json b/packages/metadata-protocol/package.json index 060de99cd7..71c4ed2d45 100644 --- a/packages/metadata-protocol/package.json +++ b/packages/metadata-protocol/package.json @@ -43,7 +43,6 @@ }, "devDependencies": { "@types/node": "^26.1.2", - "better-sqlite3": "^13.0.2", "tsup": "^8.5.1", "typescript": "^6.0.3", "vitest": "^4.1.10" 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 1da0b759b5..d24443a198 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 @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import Database from 'better-sqlite3'; +import { DatabaseSync } from 'node:sqlite'; + import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { @@ -22,9 +23,15 @@ import { * emits today — `packages/drivers/driver-sql/src/declared-index-retired-keys.test.ts` * pins that string against the same engine, so the fixture below is that test's * measured output rather than a guess at it. + * + * Uses Node's built-in `node:sqlite` rather than `better-sqlite3` (which the + * driver packages use) on purpose: this package needs no SQL dependency of its + * own for anything else, and adding one to run a test would put a native module + * 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)', () => { - let db: InstanceType; + let db: DatabaseSync; let exec: IndexExec; /** Exactly the DDL `syncDeclaredIndexes` produces for the declaration. */ @@ -59,7 +66,7 @@ describe('sys_view_definition active-row uniqueness (#5839)', () => { }; beforeEach(() => { - db = new Database(':memory:'); + db = new DatabaseSync(':memory:'); db.exec(`CREATE TABLE sys_view_definition ( id TEXT PRIMARY KEY, name TEXT, organization_id TEXT, owner TEXT, state TEXT );`); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5befc3928f..520eadf03b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1189,9 +1189,6 @@ importers: '@types/node': specifier: ^26.1.2 version: 26.1.2 - better-sqlite3: - specifier: ^13.0.2 - version: 13.0.2 tsup: specifier: ^8.5.1 version: 8.5.1(jiti@2.7.0)(postcss@8.5.25)(tsx@4.23.1)(typescript@6.0.3)(yaml@2.9.0)