diff --git a/.changeset/sys-setting-organization-scoped-unique-index.md b/.changeset/sys-setting-organization-scoped-unique-index.md new file mode 100644 index 0000000000..4ada0443ef --- /dev/null +++ b/.changeset/sys-setting-organization-scoped-unique-index.md @@ -0,0 +1,109 @@ +--- +"@objectstack/platform-objects": patch +--- + +fix(platform-objects): `sys_setting`'s declared unique index becomes per-organization (#8555) + +`sys_setting` declared its row identity as a table-level index with bare +`unique: true`. At the DECLARED-index level that is the positional spelling of +`'global'` — the listed columns verbatim — so `(namespace, key, scope, user_id)` +materialized as an **installation-wide** unique index on a tenant-scoped object. +(Field-level `unique: true` means the opposite, per-organization, and has since +#3696; `packages/lint` names that divergence "the #4986 trap".) This is the sixth +instance of the class ruled on 2026-08-13, after #8461, #8556 and #8554's five. + +| object | package | was | now | +|---|---|---|---| +| `sys_setting` | `platform-objects` | `[namespace, key, scope, user_id]` global | same columns, per organization | + +## Why per-organization, when this object had an argument for staying global + +`sys_setting` carries a `scope` column, so the card that filed this asked a real +question first: if `scope` itself encoded tenancy, the installation-wide key was +correct and the fix was to spell `'global'` explicitly. It does not. `scope` is +the cascade LAYER — `global | tenant | user`, a priority ladder walked +env > global > tenant > user > default — and the organization is carried by +`organization_id` and nothing else. `SettingsService.loadRows` says so outright +("per-tenant isolation for `tenant`-scope rows is still enforced by the engine"), +`upsertRow` bypasses the tenant audit only for `scope='global'` rows "because +global rows are platform-wide", and the `lifecycle` manifest depends on the +per-organization reading: `retention_overrides` is `scope: 'tenant'` precisely so +that "regulated tenants set years; dev sets days ... one deployment can carry +both" (ADR-0057 §3.2). + +The `scope='global'` layer is **not** lost by scoping the index. The organization +key part is NULL-safe (`COALESCE(organization_id, '__global__')`, ADR-0120 D3), +and platform rows carry no organization — so they share one bucket and stay +unique among themselves, which is exactly the installation-wide platform default +the resolver reads at rung 2. + +## Measured live on a real engine before the fix + +Two organizations, the same `(namespace, key)`, `OS_TENANCY_POSTURE=isolated`, +driving the real shipped declaration: + +``` +scope='user' org_jia POST (mail, smtp_host, user, usr_1) → 201 + org_yi POST the SAME → 409 UNIQUE_VIOLATION + org_yi POST an unused key → 201 ← the control + org_yi GET the colliding key → total 0 +scope='tenant' org_jia 201 / org_yi the SAME → 201 +scope='global' platform 201 / platform the SAME → 201 +``` + +The 409 is the class defect: a per-value refusal on a row the caller cannot read +is a cross-tenant existence oracle, and two organizations could not hold +independent per-user settings for the same key. + +⚠️ **The two 201s are a second, independent defect that this release does NOT +fix.** `user_id` is NULL on every `tenant` and `global` row, and SQL UNIQUE is +NULL-distinct, so the declared row identity is unenforced on those limbs — even +within one organization, two rows for the same `(namespace, key, scope)` are +accepted. The organization key part is NULL-safe; the author-declared `user_id` +column is not. Closing that needs a contract decision about null-safety on +author-declared columns plus a duplicate pre-flight for databases that have +already accumulated duplicates, so it is filed as #8629 rather than smuggled in +here. It is pinned as a live fact in the driver suite so this change cannot be +read as having fixed it. + +## ⚠️ Operators: a migration is REQUIRED, and deploying this release is not it + +Respelling a declared index changes its generated **name**. On an existing +database `initObjects` is additive: it creates the new per-organization composite +at boot and **never drops the old global index**, which goes on enforcing. Until +the retirement is applied, a deployed installation that has taken this release +still refuses a second organization's per-user setting — that is asserted as a +test, not assumed. + +``` +os migrate plan # one `replace_unique_index` on sys_setting, categorised safe +os migrate apply # no --allow-destructive needed +``` + +It plans as **one pure relaxation**, not as two findings. That matters: if it +read as "composite missing" (safe) plus "old global index orphaned" +(destructive, opt-in), an operator applying only the safe half would keep the +global index — keep the defect — while the plan read as applied. The `#8461` +`replace_unique_index` arm covers it unchanged (no driver change in this +release), applies CREATE-before-DROP so uniqueness is never unenforced in +between, drops the legacy index only once the replacement is confirmed present, +and converges to no drift. + +Two notes worth an operator's attention: + +- The replacement index name, + `uniq_sys_setting_organization_id_namespace_key_scope_user_id`, is exactly 60 + characters — the limit — so it is emitted untruncated rather than + hash-suffixed. +- Because the replacement does **not** tighten the `user_id` column, the + migration still applies cleanly to a database that already carries duplicate + `scope='tenant'` rows (which the old index permitted). Row counts are + preserved; nothing is deduplicated. + +## Not breaking + +A relaxation admits key pairs that were previously refused and refuses nothing +that previously succeeded, so no caller that worked before fails now. Every write +to `sys_setting` goes through `SettingsService.set()`, whose upsert keys on +`(namespace, key, scope, user_id)` under the engine's tenant scoping — the shape +this index now matches. diff --git a/packages/drivers/driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts b/packages/drivers/driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts new file mode 100644 index 0000000000..82c5daecc5 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts @@ -0,0 +1,695 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, afterEach, beforeEach, vi } from 'vitest'; +import { isUniqueViolationError } from '@objectstack/types'; +import { SqlDriver, classifyIndexKeyPart, parseIndexDdl, legacyUniqueReplacements } from '../src/index.js'; + +/** + * #8555 — `sys_setting`, the SIXTH instance of the #8323 class, and the first + * one whose direction had to be MEASURED rather than inherited. + * + * ## Why this object got its own card instead of joining #8554's five + * + * A DECLARED index's bare `unique: true` is the positional spelling of + * `'global'` — the listed columns verbatim — so + * `(namespace, key, scope, user_id)` materialized as an installation-wide key + * on a tenant-scoped object. But `sys_setting` had a real argument the other + * five did not: it carries a `scope` column, and IF `scope` encoded tenancy the + * installation-wide key would be correct and the fix would be to spell + * `'global'` explicitly. The card refused to guess and asked for the reading. + * + * The reading: `scope` is the cascade LAYER, not the tenant. `global | tenant | + * user` is a priority ladder (`scopeRank`), the organization is carried by + * `organization_id` and nothing else (`loadRows`: "per-tenant isolation for + * `tenant`-scope rows is still enforced by the engine"), and the `lifecycle` + * manifest is built on per-organization `scope: 'tenant'` values — "regulated + * tenants set years; dev sets days ... one deployment can carry both". So the + * key is per-organization, and this object inherits the 2026-08-13 ruling. + * + * ## What the probe measured, live, before the fix + * + * Real driver, the REAL shipped declaration, `OS_TENANCY_POSTURE=isolated`: + * + * ``` + * CREATE UNIQUE INDEX uniq_sys_setting_namespace_key_scope_user_id + * on sys_setting (namespace, key, scope, user_id) + * + * scope='user' org_jia POST (mail, smtp_host, user, usr_1) → 201 + * org_yi POST the SAME → 409 UNIQUE_VIOLATION + * org_yi POST an unused key → 201 ← the ORACLE control + * org_yi GET the colliding key → 0 rows + * scope='tenant' org_jia 201 / org_yi the SAME → 201 + * scope='global' platform 201 / platform the SAME → 201 + * ``` + * + * ⚠️ Read the two 201s carefully — they are the thing this card did NOT expect + * and the reason the probe was worth running. The class symptom reproduces on + * the `user` limb exactly as predicted, but on the `tenant` and `global` limbs + * the installation-wide index enforces **nothing at all**: `user_id` is NULL on + * every such row and SQL UNIQUE is NULL-distinct, so even a SAME-organization + * duplicate is accepted. Section 4 pins that as a live fact. It is a second, + * independent defect, it is filed separately, and this respelling does not fix + * it — stated here so the suite cannot be read as claiming otherwise. + * + * ## Why this suite is at the DRIVER level + * + * The driver is the only layer that materializes a unique constraint and the + * only layer that can insert the violating row. `rest-server.ts` maps any error + * satisfying `isUniqueViolationError` to `409 UNIQUE_VIOLATION` and a + * successful create to `201`, so each case asserts the ENVELOPE the API would + * put on the wire — never a bare `.toThrow()`, which a driver throwing a plain + * `Error` would satisfy while REST answered 500. + * + * ## The half a fresh-database suite cannot see + * + * Respelling changes the index's generated NAME, and `initObjects` is + * additive — it creates the new composite at boot and never drops the old one. + * A deployed installation that takes this release without running the plan + * keeps the defect. Section 3 is therefore the load-bearing part of this file: + * it builds installations that ALREADY HAVE the old index and real rows, then + * migrates them. + */ + +/** The wire shape a duplicate insert must produce, per `rest-server.ts`. */ +const CONFLICT_ENVELOPE = { status: 409, code: 'UNIQUE_VIOLATION' } as const; + +const TABLE = 'sys_setting'; +const KEY_COLUMNS = ['namespace', 'key', 'scope', 'user_id'] as const; +const LEGACY_NAME = 'uniq_sys_setting_namespace_key_scope_user_id'; +const REPLACEMENT_NAME = 'uniq_sys_setting_organization_id_namespace_key_scope_user_id'; + +const s = { type: 'string' }; +const b = { type: 'boolean' }; + +/** Physical columns the constraint and the rows need. */ +const FIELDS: Record = { + id: s, + organization_id: s, + namespace: s, + key: s, + scope: s, + user_id: s, + value: s, + encrypted: b, + locked: b, +}; + +/** + * ⚠️ Hand-copied from the shipped declaration, keeping the package boundary + * (`driver-sql` must not depend on `platform-objects` — the shape #8461, #8556 + * and #8554 used). Guarded in ONE direction only: + * `platform-objects/src/system/sys-setting.organization-unique.test.ts` asserts + * the real `SysSetting.indexes` against its own inline literal, so a change to + * the SHIPPED declaration that is not mirrored here goes red over there. The + * reverse is unguarded. Change these only together with the declaration. + * + * The `unique: false` on the non-unique entries is not decoration: + * `ObjectSchema.create` normalizes an authored `{ fields: [...] }` into that + * shape, so this is what a driver is actually handed at registration. + */ +const PRE_INDEXES: Array> = [ + { fields: ['namespace', 'key', 'scope', 'user_id'], unique: true }, // ← the defect + { fields: ['namespace', 'scope'], unique: false }, + { fields: ['user_id', 'namespace'], unique: false }, +]; +const FIXED_INDEXES: Array> = [ + { fields: ['namespace', 'key', 'scope', 'user_id'], unique: 'organization' }, + { fields: ['namespace', 'scope'], unique: false }, + { fields: ['user_id', 'namespace'], unique: false }, +]; + +async function createAsApi( + driver: SqlDriver, + record: Record, +): Promise<{ status: number; code?: string; raw?: unknown }> { + try { + await driver.create(TABLE, record as any); + return { status: 201 }; + } catch (error) { + if (isUniqueViolationError(error)) return { status: 409, code: 'UNIQUE_VIOLATION', raw: error }; + return { status: 500, code: 'INTERNAL_ERROR', raw: error }; + } +} + +describe('#8555 — sys_setting: the declared unique index becomes per-organization', () => { + let driver: SqlDriver | undefined; + let savedPosture: string | undefined; + let savedMultiOrg: string | undefined; + + const makeDriver = () => { + const d = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + (d as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn(), debug: vi.fn() }; + driver = d; + return d; + }; + + const app = (which: 'pre' | 'fixed') => [ + { name: TABLE, fields: FIELDS, indexes: which === 'pre' ? PRE_INDEXES : FIXED_INDEXES }, + ]; + + beforeEach(() => { + savedPosture = process.env.OS_TENANCY_POSTURE; + savedMultiOrg = process.env.OS_MULTI_ORG_ENABLED; + // The posture the probe was run on. ADR-0120's invariant is that no index + // shape reads the posture — section 5 pins that — so this is context for + // the reader, not an input the assertions depend on. + process.env.OS_TENANCY_POSTURE = 'isolated'; + process.env.OS_MULTI_ORG_ENABLED = 'true'; + }); + + afterEach(async () => { + await driver?.disconnect(); + driver = undefined; + if (savedPosture === undefined) delete process.env.OS_TENANCY_POSTURE; + else process.env.OS_TENANCY_POSTURE = savedPosture; + if (savedMultiOrg === undefined) delete process.env.OS_MULTI_ORG_ENABLED; + else process.env.OS_MULTI_ORG_ENABLED = savedMultiOrg; + }); + + /** Unique index name → canonical key parts, COALESCE literal elided. */ + async function uniqueKeyParts(): Promise> { + const k = (driver as any).knex; + const list: any = await k.raw(`PRAGMA index_list(${TABLE})`); + const master: any = await k.raw( + `SELECT name, sql FROM sqlite_master WHERE type = 'index' AND tbl_name = ?`, + [TABLE], + ); + const ddlByName = new Map(); + for (const r of Array.isArray(master) ? master : (master?.rows ?? [])) { + if (typeof r?.sql === 'string' && r.sql) ddlByName.set(r.name, r.sql); + } + const out: Record = {}; + for (const idx of list) { + if (idx.origin === 'pk' || idx.unique !== 1) continue; + const parsed = parseIndexDdl(ddlByName.get(idx.name) ?? ''); + if (parsed) { + out[idx.name] = parsed.keyParts.map((p) => { + const part = classifyIndexKeyPart(p); + if (part.kind === 'column') return part.column; + return part.column === null ? p : `COALESCE(${part.column})`; + }); + } else { + const info: any = await k.raw(`PRAGMA index_info("${idx.name}")`); + out[idx.name] = info.map((c: any) => c.name); + } + } + return out; + } + + /** + * One settings row. + * + * `user_id` defaults to a REAL id, not null: the `user` limb is the one where + * the class defect is observable, and a null there would silently move every + * assertion into the NULL-distinct hole documented in section 4. + */ + const row = ( + id: string, + org: string | undefined, + over: Record = {}, + ): Record => ({ + id, + ...(org ? { organization_id: org } : {}), + namespace: 'mail', + key: 'smtp_host', + scope: 'user', + user_id: 'usr_1', + value: '"smtp.example.test"', + ...over, + }); + + /** The colliding key, and the controls, as the probe drove them. */ + const KEY = {}; + const CONTROL = { key: 'only_in_org_yi' }; // differs in a LEADING key column + const TRAILING_CONTROL = { user_id: 'usr_2' }; // differs in the LAST key column only + + // ───────────────────────────────────────────────────────────────────────── + // 1. Materialized shape — both spellings, kept side by side + // ───────────────────────────────────────────────────────────────────────── + + it('the fixed declaration keys on the NULL-safe organization part', async () => { + const d = makeDriver(); + await d.initObjects(app('fixed') as any); + + expect(await uniqueKeyParts()).toEqual({ + [REPLACEMENT_NAME]: ['COALESCE(organization_id)', ...KEY_COLUMNS], + }); + }); + + it('the pre-fix declaration keyed on the bare business columns — installation-wide', async () => { + // Kept permanently rather than measured once: this is the contrast that + // makes every "AFTER" assertion below mean something. + const d = makeDriver(); + await d.initObjects(app('pre') as any); + + expect(await uniqueKeyParts()).toEqual({ [LEGACY_NAME]: [...KEY_COLUMNS] }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 2. The card's reproduction — the live probe the ruling required + // ───────────────────────────────────────────────────────────────────────── + + it('BEFORE: a key held by another organization is refused — 409 on a row you cannot read', async () => { + const d = makeDriver(); + await d.initObjects(app('pre') as any); + + expect((await createAsApi(d, row('a', 'org_jia', KEY))).status).toBe(201); + expect(await createAsApi(d, row('b', 'org_yi', KEY))).toMatchObject(CONFLICT_ENVELOPE); + + // The control that makes the refusal an ORACLE rather than a blanket + // rejection: an unused value from the same caller is accepted, so the 409 + // is a per-value answer about another tenant's data. + expect((await createAsApi(d, row('c', 'org_yi', CONTROL))).status).toBe(201); + + // …and the other half of the oracle: the caller's own read of the colliding + // key returns nothing. It is refused by a row it cannot see. + const visible = (await d.find(TABLE, {})).filter( + (r: any) => + r.organization_id === 'org_yi' && + r.namespace === 'mail' && + r.key === 'smtp_host' && + r.scope === 'user', + ); + expect(visible).toHaveLength(0); + }); + + it('AFTER: the same cross-organization create is accepted — 409 flips to 201', async () => { + const d = makeDriver(); + await d.initObjects(app('fixed') as any); + + expect((await createAsApi(d, row('a', 'org_jia', KEY))).status).toBe(201); + expect((await createAsApi(d, row('b', 'org_yi', KEY))).status).toBe(201); + + const held = (await d.find(TABLE, {})).filter( + (r: any) => r.namespace === 'mail' && r.key === 'smtp_host' && r.scope === 'user', + ); + expect(held.map((r: any) => r.organization_id).sort()).toEqual(['org_jia', 'org_yi']); + }); + + it('AFTER (anti-vacuity): a SAME-organization duplicate is still refused', async () => { + // If this ever goes green-by-acceptance the constraint was REMOVED, not + // scoped — strictly worse than the defect being fixed, and + // indistinguishable from the fix by the 409-flips-to-201 assertion alone. + const d = makeDriver(); + await d.initObjects(app('fixed') as any); + + expect((await createAsApi(d, row('a', 'org_jia', KEY))).status).toBe(201); + expect(await createAsApi(d, row('b', 'org_jia', KEY))).toMatchObject(CONFLICT_ENVELOPE); + expect(await d.count(TABLE, {})).toBe(1); + }); + + it('AFTER: rows with no organization stay unique among THEMSELVES (ADR-0120 D3)', async () => { + // This is the arm that answers the card's "argument for keeping it global". + // The `scope='global'` LAYER — the platform default every organization + // resolves against at rung 2 — is preserved by the NULL-safe key part, not + // by making the whole index installation-wide: platform rows carry no + // organization, so they share the one `__global__` bucket and remain unique + // within it, while an organization may still hold its own row of the same + // key at its own layer. + const d = makeDriver(); + await d.initObjects(app('fixed') as any); + + const platform = { scope: 'global', namespace: 'branding', key: 'logo_url' }; + expect((await createAsApi(d, row('seed1', undefined, platform))).status).toBe(201); + expect(await createAsApi(d, row('seed2', undefined, platform))).toMatchObject( + CONFLICT_ENVELOPE, + ); + + expect((await createAsApi(d, row('own1', 'org_jia', platform))).status).toBe(201); + }); + + it('the key is the whole COMPOSITE — varying only the trailing column was always accepted', async () => { + // Guards the fixture's own claim about which columns the constraint spans. + // Green BEFORE and AFTER by design: it describes behaviour this card does + // not change, which is exactly what makes it a control. If the key were + // silently narrowed to the leading columns this goes red. + const d = makeDriver(); + await d.initObjects(app('pre') as any); + + expect((await createAsApi(d, row('a', 'org_jia', KEY))).status).toBe(201); + expect((await createAsApi(d, row('b', 'org_yi', TRAILING_CONTROL))).status).toBe(201); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 3. The deployed-installation half — the #8323 trap + // ───────────────────────────────────────────────────────────────────────── + + describe('migration on a database built before the respelling', () => { + /** + * An installation that ALREADY HAS the old global index and real rows — + * i.e. every deployment in the field. A fresh-provision test cannot reach + * any of the assertions in this block. + * + * ⚠️ `dup1`/`dup2` are not padding. A deployed `sys_setting` can ALREADY + * carry duplicate `scope='tenant'` rows, because the pre-fix index is + * NULL-distinct on `user_id` (section 4). Seeding them here is what proves + * the migration stays applicable on a database the old constraint let rot — + * the replacement index does not tighten the `user_id` column, so it can + * still be created. A fix that DID tighten it would fail to build the index + * on exactly these rows, which is why that half is a separate card with a + * duplicate pre-flight rather than a rider on this one. + */ + const seedDeployed = async (d: SqlDriver) => { + await d.initObjects(app('pre') as any); + await d.create(TABLE, row('r1', 'org_jia', KEY) as any); + await d.create(TABLE, row('r2', 'org_jia', CONTROL) as any); + await d.create(TABLE, row('r3', undefined, { scope: 'global', key: 'platform_default' }) as any); + await d.create(TABLE, { + id: 'dup1', organization_id: 'org_jia', namespace: 'lifecycle', + key: 'retention_overrides', scope: 'tenant', user_id: null, value: '"x"', + } as any); + await d.create(TABLE, { + id: 'dup2', organization_id: 'org_jia', namespace: 'lifecycle', + key: 'retention_overrides', scope: 'tenant', user_id: null, value: '"y"', + } as any); + }; + + it('the seeded database really carries the pre-fix index, and the defect is live on it (harness guard)', async () => { + // Without this the whole block could be exercising a fresh schema and + // every assertion below would still pass. Named as a guard on purpose. + const d = makeDriver(); + await seedDeployed(d); + + expect(await uniqueKeyParts()).toEqual({ [LEGACY_NAME]: [...KEY_COLUMNS] }); + expect(await d.count(TABLE, {})).toBe(5); + expect(await createAsApi(d, row('x', 'org_yi', KEY))).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('is planned as ONE pure relaxation, categorised safe — not a destructive orphan drop', async () => { + const d = makeDriver(); + await seedDeployed(d); + // The new metadata arrives (a deploy), same database. + await d.initObjects(app('fixed') as any); + + const drift = await d.detectManagedDrift(); + expect(drift.filter((e) => e.table === TABLE)).toHaveLength(1); + const entry = drift.find((e) => e.table === TABLE && e.op.type === 'replace_unique_index'); + expect(entry, 'the respelling must be a replacement, not two unrelated findings').toBeDefined(); + expect(entry!.category).toBe('safe'); + expect(entry!.op).toMatchObject({ + dropIndexNames: [LEGACY_NAME], + createIndexName: REPLACEMENT_NAME, + createColumns: ['organization_id', ...KEY_COLUMNS], + nullSafeColumns: ['organization_id'], + }); + + // ⛔ The old index must NOT ALSO surface as an orphan. An orphan drop is + // `destructive`, so an operator applying only the safe half would keep the + // global index — keep the defect — while the plan read as applied. That is + // the exact failure #8323 measured. + expect( + drift.filter( + (e) => e.op.type === 'drop_index' && (e.op as any).indexName === LEGACY_NAME, + ), + ).toHaveLength(0); + }); + + it('applies WITHOUT --allow-destructive, keeps every row, and converges', async () => { + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(app('fixed') as any); + + const drift = await d.detectManagedDrift(); + const { applied, skipped } = await d.applyMigrationEntries(drift, { allowDestructive: false }); + expect(applied.some((e) => e.op.type === 'replace_unique_index')).toBe(true); + expect(skipped).toHaveLength(0); + + expect(await uniqueKeyParts()).toEqual({ + [REPLACEMENT_NAME]: ['COALESCE(organization_id)', ...KEY_COLUMNS], + }); + // Every row survives — including the two pre-existing duplicates the old + // index allowed. A migration that silently dropped them would be data loss + // dressed as a fix. + expect(await d.count(TABLE, {})).toBe(5); + + // Re-running finds nothing: the plan is not a drop/create cycle. + expect(await d.detectManagedDrift()).toHaveLength(0); + }); + + it('after applying, BOTH halves hold on the MIGRATED database', async () => { + // The assertion the card is actually about: the fix reaches a deployed + // installation, not merely a freshly provisioned one. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(app('fixed') as any); + await d.applyMigrationEntries(await d.detectManagedDrift(), { allowDestructive: false }); + + expect((await createAsApi(d, row('z1', 'org_yi', KEY))).status).toBe(201); + + // The anti-vacuity arm, on the SAME migrated index. + expect(await createAsApi(d, row('z2', 'org_jia', KEY))).toMatchObject(CONFLICT_ENVELOPE); + + // …and the organization-less bucket survived the migration intact: the + // seeded platform row still blocks a duplicate of ITSELF. + expect( + await createAsApi(d, row('z3', undefined, { scope: 'global', key: 'platform_default' })), + ).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('boot creates the replacement ADDITIVELY, so the defect is STILL LIVE until the plan runs', async () => { + // `initObjects` is additive-only: it materializes the newly-declared + // composite at boot and never drops anything. So a deployed installation + // that has taken the new code but not run the plan is still enumerable — + // deploying the respelling is not, by itself, the fix. This is the + // sentence an operator needs, stated as an assertion. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(app('fixed') as any); + + expect(Object.keys(await uniqueKeyParts()).sort()).toEqual( + [LEGACY_NAME, REPLACEMENT_NAME].sort(), + ); + expect(await createAsApi(d, row('y', 'org_yi', KEY))).toMatchObject(CONFLICT_ENVELOPE); + }); + + it('DROP happens only once the replacement is confirmed present', async () => { + // The safety argument in the direction that can actually go wrong: if the + // replacement is not there, the legacy index must be left alone rather + // than dropped into a gap with no uniqueness at all. + const d = makeDriver(); + await seedDeployed(d); + await d.initObjects(app('fixed') as any); + const drift = await d.detectManagedDrift(); + + await (d as any).knex.raw(`DROP INDEX ${REPLACEMENT_NAME}`); + (d as any).syncDeclaredIndexes = async () => undefined; + + const { applied, skipped } = await d.applyMigrationEntries(drift, { allowDestructive: false }); + const isReplace = (e: { table: string; op: { type: string } }) => + e.table === TABLE && e.op.type === 'replace_unique_index'; + expect(applied.some(isReplace)).toBe(false); + expect(skipped.some(isReplace)).toBe(true); + + // The pre-migration constraint is intact: never left with neither index. + expect(Object.keys(await uniqueKeyParts())).toEqual([LEGACY_NAME]); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 4. The OTHER defect this card does not fix — pinned so it cannot be + // mistaken for fixed, and so the follow-up card has a live repro + // ───────────────────────────────────────────────────────────────────────── + + describe('the NULL-distinct user_id hole (out of scope here — filed as #8629)', () => { + /** + * `user_id` is NULL on every `scope='tenant'` and `scope='global'` row — + * `SettingsService` writes `scope === 'user' ? ctx.userId : null` — and SQL + * UNIQUE treats NULLs as distinct. The declared row identity is therefore + * void on exactly the two limbs that carry organization-level and + * platform-level configuration, BEFORE and AFTER this card. + * + * The organization key part is NULL-safe (ADR-0120 D3); the author-declared + * `user_id` column is not, and extending null-safety to arbitrary declared + * columns is a contract decision plus a duplicate pre-flight for the + * databases that have already accumulated duplicates. Hence a separate card: + * #8629. + * + * These assertions are written to go RED when that card lands — a fix must + * come here and rewrite them, rather than leaving a stale "known hole" + * comment behind. + */ + it('BEFORE: two organizations CAN both hold the same tenant-scope key — the constraint is void, not oracular', async () => { + const d = makeDriver(); + await d.initObjects(app('pre') as any); + + const tenantRow = { scope: 'tenant', user_id: null, namespace: 'lifecycle', key: 'retention_overrides' }; + expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status).toBe(201); + // 201, not 409: the card predicted a refusal here. The refusal never + // happens because the index cannot see these rows as equal at all. + expect((await createAsApi(d, row('b', 'org_yi', tenantRow))).status).toBe(201); + }); + + it('BEFORE and AFTER: a SAME-organization tenant-scope duplicate is accepted — declared row identity unenforced', async () => { + for (const which of ['pre', 'fixed'] as const) { + const d = makeDriver(); + await d.initObjects(app(which) as any); + + const tenantRow = { scope: 'tenant', user_id: null, namespace: 'lifecycle', key: 'retention_overrides' }; + expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status, which).toBe(201); + expect((await createAsApi(d, row('b', 'org_jia', tenantRow))).status, which).toBe(201); + expect(await d.count(TABLE, {}), which).toBe(2); + + await d.disconnect(); + driver = undefined; + } + }); + + it('BEFORE and AFTER: the same hole on the platform (`scope=global`) layer', async () => { + for (const which of ['pre', 'fixed'] as const) { + const d = makeDriver(); + await d.initObjects(app(which) as any); + + const globalRow = { scope: 'global', user_id: null, key: 'platform_default' }; + expect((await createAsApi(d, row('a', undefined, globalRow))).status, which).toBe(201); + expect((await createAsApi(d, row('b', undefined, globalRow))).status, which).toBe(201); + + await d.disconnect(); + driver = undefined; + } + }); + + it('the hole is the NULL, not the layer — the same rows with a non-null user_id ARE constrained', async () => { + // The control that identifies the mechanism. Without it, "tenant rows are + // unconstrained" could be read as something about the `scope` value. + const d = makeDriver(); + await d.initObjects(app('fixed') as any); + + const named = { scope: 'tenant', user_id: 'usr_1', namespace: 'lifecycle', key: 'retention_overrides' }; + expect((await createAsApi(d, row('a', 'org_jia', named))).status).toBe(201); + expect(await createAsApi(d, row('b', 'org_jia', named))).toMatchObject(CONFLICT_ENVELOPE); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 5. The #8461 arm and its guards, exercised on THIS object + // ───────────────────────────────────────────────────────────────────────── + + describe('the declared-index replacement arm', () => { + const physicalColumns = () => new Set(Object.keys(FIELDS)); + + it('proposes exactly one retirement, keyed on the listed columns', () => { + const [entry, ...rest] = legacyUniqueReplacements({ + table: TABLE, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: FIXED_INDEXES, + } as any); + expect(rest).toHaveLength(0); + expect(entry).toMatchObject({ + // ⚠️ `legacyColumns` is the whole listed key, not the leading column. + // `column` is the LEADING one and is reporting only — this four-column + // key is what makes the distinction observable. + legacyColumns: [...KEY_COLUMNS], + legacyNames: [LEGACY_NAME], + replacement: { + name: REPLACEMENT_NAME, + columns: ['organization_id', ...KEY_COLUMNS], + unique: true, + nullSafeColumns: ['organization_id'], + }, + }); + }); + + it('claims nothing for an EXPLICITLY NAMED index — that transition is a recreate', () => { + // #8461 guard 1. If this ever starts proposing a replacement it would ask + // to drop the very index `recreate_index` is rebuilding. + expect( + legacyUniqueReplacements({ + table: TABLE, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: [{ name: 'uq_hand_named', fields: [...KEY_COLUMNS], unique: 'organization' }], + } as any), + ).toHaveLength(0); + }); + + it('claims nothing when the legacy name IS the replacement name (the S6 composite)', () => { + // #8461 guard 2 — what protects sys_team / sys_business_unit / sys_member + // and the S6 objects this card's own sweep re-triaged and left alone. + expect( + legacyUniqueReplacements({ + table: TABLE, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: [{ fields: ['organization_id', ...KEY_COLUMNS], unique: 'organization' }], + } as any), + ).toHaveLength(0); + }); + + it('claims nothing for the BARE spelling — an unrespelled declaration is untouched (#5082)', () => { + expect( + legacyUniqueReplacements({ + table: TABLE, + fields: {}, + tenantField: 'organization_id', + physicalColumns: physicalColumns(), + declaredIndexes: [{ fields: [...KEY_COLUMNS], unique: true }], + } as any), + ).toHaveLength(0); + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 6. ADR-0120: no index shape reads the posture + // ───────────────────────────────────────────────────────────────────────── + + describe('postureIndependence', () => { + it('materializes the same key parts under single / group / isolated', async () => { + for (const posture of ['single', 'group', 'isolated']) { + process.env.OS_TENANCY_POSTURE = posture; + const d = makeDriver(); + await d.initObjects(app('fixed') as any); + expect(await uniqueKeyParts(), `posture=${posture}`).toEqual({ + [REPLACEMENT_NAME]: ['COALESCE(organization_id)', ...KEY_COLUMNS], + }); + await d.disconnect(); + driver = undefined; + } + }); + }); + + // ───────────────────────────────────────────────────────────────────────── + // 7. The index-name length boundary + // ───────────────────────────────────────────────────────────────────────── + + describe('the replacement index name', () => { + /** + * The replacement name is EXACTLY 60 characters — `INDEX_NAME_MAX`. It is + * emitted verbatim; one more character in the table name or any key column + * would push it over and `buildIndexName` would truncate it to a sha1-suffixed + * form instead. Worth pinning because a name-based retirement is precisely + * where that would go wrong: if the legacy and replacement names ever + * collapsed to the same string, the `legacyName === replacement.name` guard + * would read the respelling as "nothing was superseded" and emit NO + * migration — a fresh database would look right and every deployed + * installation would keep the global index forever. + */ + it('sits exactly on the 60-character boundary and is emitted untruncated', () => { + expect(REPLACEMENT_NAME.length).toBe(60); + expect(REPLACEMENT_NAME).not.toMatch(/_[0-9a-f]{8}$/); + }); + + it('differs from the legacy name, so the S6 guard does not swallow the retirement', () => { + expect(LEGACY_NAME.length).toBeLessThanOrEqual(60); + expect(REPLACEMENT_NAME).not.toBe(LEGACY_NAME); + }); + + it('the pinned name is what actually materializes, and the migration converges on it', async () => { + const d = makeDriver(); + await d.initObjects(app('pre') as any); + await d.create(TABLE, row('r1', 'org_jia', KEY) as any); + await d.initObjects(app('fixed') as any); + await d.applyMigrationEntries(await d.detectManagedDrift(), { allowDestructive: false }); + + expect(Object.keys(await uniqueKeyParts())).toEqual([REPLACEMENT_NAME]); + expect(await d.detectManagedDrift()).toHaveLength(0); + }); + }); +}); diff --git a/packages/platform-objects/src/system/sys-setting.object.ts b/packages/platform-objects/src/system/sys-setting.object.ts index ed799ff6a3..91c6c30bd9 100644 --- a/packages/platform-objects/src/system/sys-setting.object.ts +++ b/packages/platform-objects/src/system/sys-setting.object.ts @@ -10,7 +10,13 @@ import { ObjectSchema, Field } from '@objectstack/spec/data'; * per-namespace tables (e.g. `sys_mail_config`); they declare a manifest * and the value persists here. * - * Row identity: (namespace, key, scope, user_id?). + * Row identity: (organization_id, namespace, key, scope, user_id?). + * + * ⚠️ `scope` is the CASCADE LAYER, not the tenant. It names which rung of the + * resolution ladder below a row sits on; WHICH organization owns the row is + * carried by the kernel-injected `organization_id` column, and by nothing else + * (#8555). `scope='tenant'` therefore means "the organization layer" — one row + * PER organization, not one row for the installation. * * Resolution order (handled by `SettingsService.get`): * 1. process.env override (source='env', locked=true) @@ -188,7 +194,45 @@ export const SysSetting = ObjectSchema.create({ // Primary lookup path: (namespace, key, scope, user_id?) is what // SettingsService.get hits on every resolve. The composite UNIQUE // covers both the row-identity constraint and the read path. - { fields: ['namespace', 'key', 'scope', 'user_id'], unique: true }, + // + // [#8555] Scope spelled EXPLICITLY (ADR-0120 D1). On a DECLARED index bare + // `unique: true` is the positional spelling of `'global'` — the listed + // columns verbatim — so this was an installation-wide key on a + // tenant-scoped object. + // + // The card left the direction open: if `scope` itself encoded tenancy, the + // right end state was an explicit `'global'`. It does not. `scope` is the + // cascade LAYER (`global | tenant | user`, a priority ladder — see + // `scopeRank` in `SettingsService`), and the organization is carried by + // `organization_id`: `loadRows` says so outright ("per-tenant isolation for + // `tenant`-scope rows is still enforced by the engine"), and the `lifecycle` + // manifest depends on it — `retention_overrides` is `scope: 'tenant'` so + // that "regulated tenants set years; dev sets days ... one deployment can + // carry both". A per-organization value is the feature, so the key is + // per-organization. + // + // Measured live before the fix, real driver, OS_TENANCY_POSTURE=isolated: + // scope='user' org_jia 201 / org_yi SAME 409 UNIQUE_VIOLATION + // / org_yi unused 201 / org_yi's own GET 0 rows + // scope='tenant' org_jia 201 / org_yi SAME *201* + // scope='global' platform 201 / platform SAME *201* + // The 409 is the #8323 cross-tenant existence oracle. The two 201s are a + // SECOND defect this respelling does NOT fix: `user_id` is NULL on every + // `tenant`/`global` row and SQL UNIQUE is NULL-distinct, so the declared + // row identity is void on those limbs — even within ONE organization. + // Closing it means null-safety on an author-declared column plus a + // duplicate pre-flight for databases that already carry duplicates, so it + // is #8629 rather than a rider here: this respelling is a pure relaxation + // and applies to any database, while that one is a TIGHTENING that cannot + // build its index on an installation which has already accumulated the + // duplicates this hole permits. + // + // The organization key part is NULL-safe (`COALESCE(organization_id, + // '__global__')`, ADR-0120 D3), which is what preserves the `scope='global'` + // LAYER: platform rows carry no organization, so they share one bucket and + // stay unique among themselves — the installation-wide platform default the + // resolver reads at rung 2 survives, without the whole index being global. + { fields: ['namespace', 'key', 'scope', 'user_id'], unique: 'organization' }, // Common range read: full namespace dump for SettingsService.getNamespace. { fields: ['namespace', 'scope'], unique: false }, // Per-user listing on the user-prefs scope. diff --git a/packages/platform-objects/src/system/sys-setting.organization-unique.test.ts b/packages/platform-objects/src/system/sys-setting.organization-unique.test.ts new file mode 100644 index 0000000000..72b0128902 --- /dev/null +++ b/packages/platform-objects/src/system/sys-setting.organization-unique.test.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { IndexSchema, resolveInjectedSystemColumns } from '@objectstack/spec/data'; +import { SpecifierScopeSchema } from '@objectstack/spec/system'; +import { SysSetting } from './sys-setting.object.js'; + +/** + * #8555 — `sys_setting`'s declared uniqueness is organization-scoped. + * + * ## The fork this card was filed on, and how it was decided + * + * The card deliberately asserted no defect. A DECLARED index's `unique: true` + * is the positional spelling of `'global'` (the listed columns verbatim), so + * `(namespace, key, scope, user_id)` was an installation-wide key on a + * tenant-scoped object — but `sys_setting` had an argument the other five + * #8554 objects did not: if `scope` ITSELF encoded tenancy, the installation- + * wide key was correct and the right end state was an explicit `'global'`. + * + * It does not. `scope` is the cascade LAYER, not the tenant: + * + * - Its value domain is `global | tenant | user` — a priority ladder walked + * env > global > tenant > user > default, ranked by `scopeRank`. Pinned + * against the spec enum in `sys-setting.scope-options.test.ts`. + * - `SettingsService.loadRows` says the organization dimension lives + * elsewhere outright: "per-tenant isolation for `tenant`-scope rows is + * still enforced by the engine". The column is `organization_id`. + * - `upsertRow` keys on `(namespace, key, scope, user_id)` and bypasses the + * tenant audit ONLY for `scope='global'` rows, "because global rows are + * platform-wide" — i.e. `tenant`/`user` rows do carry an organization. + * - The `lifecycle` manifest is built on it: `retention_overrides` is + * `scope: 'tenant'` precisely so "regulated tenants set years; dev sets + * days ... one deployment can carry both" (ADR-0057 §3.2). + * + * So `scope='tenant'` means "the organization layer" — one row per + * organization — and the card's second branch applies: this is the sixth + * instance of the #8323 class and inherits the 2026-08-13 ruling. + * + * ## What was measured live, before the fix + * + * Real `SqlDriver`, the real shipped declaration, `OS_TENANCY_POSTURE=isolated`: + * + * ``` + * CREATE UNIQUE INDEX uniq_sys_setting_namespace_key_scope_user_id + * on sys_setting (namespace, key, scope, user_id) + * + * scope='user' org_jia POST (mail, smtp_host, user, usr_1) → 201 + * org_yi POST the SAME → 409 UNIQUE_VIOLATION + * org_yi POST an unused key → 201 ← the ORACLE control + * org_yi GET the colliding key → 0 rows + * scope='tenant' org_jia 201 / org_yi the SAME → 201 + * scope='global' platform 201 / platform the SAME → 201 + * ``` + * + * The 409 is the class defect, and it is the limb that carries real user + * preferences. The two 201s are a SECOND, independent defect that this card + * does not close and this test does not claim: `user_id` is NULL on every + * `tenant`/`global` row, SQL UNIQUE is NULL-distinct, so the declared row + * identity is unenforced there — even inside one organization. See the driver + * suite's section 4, which pins that hole as a live fact rather than leaving + * the respelling to imply a fix it does not deliver. + * + * The materialized shape, the anti-vacuity twin, and the migration of an + * installation that already carries the old index are pinned driver-side in + * `driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts`. This + * test pins the declaration that suite's fixture copies. + */ +describe('sys_setting — declared uniqueness is organization-scoped (#8555)', () => { + const uniqueIndexes = (SysSetting.indexes ?? []).filter((i: any) => i.unique); + + it('declares exactly one unique index, on (namespace, key, scope, user_id)', () => { + expect(uniqueIndexes).toHaveLength(1); + expect((uniqueIndexes[0] as any).fields).toEqual(['namespace', 'key', 'scope', 'user_id']); + }); + + it("spells the scope 'organization' — NOT bare `true`", () => { + // ⛔ Asserted by EQUALITY, never by truthiness. Bare `true` here IS the bug + // (it is the positional spelling of `'global'`), so a truthy check passes + // on the exact value this card removed — the way #8556's equivalent pin + // stayed green under its own ablation. + expect((uniqueIndexes[0] as any).unique).toBe('organization'); + }); + + it('is not left as a positional default in EITHER direction (ADR-0120 D1)', () => { + // The card's own closing condition: whichever branch the reading landed on, + // the end state must state its scope. This is the assertion that would also + // have held had the measurement gone the other way (to `'global'`), so it + // survives a future re-reading of `SettingsService` without being rewritten. + expect(['organization', 'global']).toContain((uniqueIndexes[0] as any).unique); + expect((uniqueIndexes[0] as any).unique).not.toBe(true); + }); + + it('is a valid IndexSchema — the spec accepts the explicit vocabulary', () => { + expect(IndexSchema.parse(uniqueIndexes[0])).toMatchObject({ + fields: ['namespace', 'key', 'scope', 'user_id'], + unique: 'organization', + }); + }); + + it('matches the fixture the driver suite copies, entry for entry', () => { + // The driver suite hand-copies this declaration to keep the package + // boundary (the shape #8461, #8556 and #8554 used). This catches ONE + // direction of drift: the shipped declaration changing while the driver + // fixture does not. The reverse is unguarded — nothing compares the two + // copies directly. + // + // Asserted on the BUILT value: `ObjectSchema.create` normalizes an authored + // `{ fields: [...] }` into `{ fields: [...], unique: false }`, so a fixture + // copied from the source text alone would be subtly wrong about what the + // driver is handed. + expect(SysSetting.indexes).toEqual([ + { fields: ['namespace', 'key', 'scope', 'user_id'], unique: 'organization' }, + { fields: ['namespace', 'scope'], unique: false }, + { fields: ['user_id', 'namespace'], unique: false }, + ]); + }); + + it('takes no tenancy opt-out, so organization_id is injected (the scope has a column)', () => { + // Derived from the BUILT value, not a regex over source. If this ever goes + // false the `'organization'` spelling has no column to key on and the whole + // fix is silently inert — a failure the index assertions above cannot see. + const plan = resolveInjectedSystemColumns(SysSetting); + expect(SysSetting.tenancy).toBeUndefined(); + expect(plan.tenant).toBe(true); + expect(plan.names.has('organization_id')).toBe(true); + }); + + it('`scope` is the cascade layer, not a tenant identity — the fact that decided the fork', () => { + // The load-bearing reading, pinned so a future sweep re-raising "shouldn't + // sys_setting be global?" is answered by a test rather than by re-reading + // the resolver. `scope`'s domain is the spec's cascade enum; none of its + // members names an organization, and `organization_id` is not among them. + const layers = SpecifierScopeSchema.options as readonly string[]; + expect([...layers].sort()).toEqual(['global', 'tenant', 'user']); + expect(layers).not.toContain('organization_id'); + + const scopeField = (SysSetting.fields as any).scope; + expect(scopeField.options.map((o: any) => o.value)).toEqual([...layers]); + // …and the unique key lists `scope` as an ordinary key column, so it + // partitions the ladder, not the tenants. + expect((uniqueIndexes[0] as any).fields).toContain('scope'); + }); +});