Skip to content

Commit 0f59584

Browse files
hotlongclaude
andauthored
fix(metadata-protocol): enforce sys_setting's declared row identity on the tenant and global layers (#8629) (#8719)
`sys-setting.object.ts` declares `{ fields: ['namespace','key','scope','user_id'], unique: 'organization' }` as the object's row identity, but `user_id` is NULL on every row that is not `scope='user'` and SQL UNIQUE is NULL-distinct — so the constraint was void on exactly the two limbs carrying organization-level and platform-level configuration. Route 1 per the maintainer's 2026-08-14 ruling: a runtime NULL-safe UNIQUE index issued at `kernel:ready`, folding both nullable key parts (`COALESCE(organization_id, '__global__')`, ADR-0120 D3, unchanged from what the driver emits; `COALESCE(user_id, '')`, the `ensureOverlayIndex` spelling for a non-tenant nullable discriminator) and reusing the DECLARED index name so the additive sync never re-imposes the NULL-distinct form. Duplicate-carrying installations: refuse-to-migrate, never keep-newest. The tightening is proved buildable under a throwaway probe name before the declared name is dropped, so a conflict leaves the previous index in place, deletes nothing, and reports at `error` with the exact query that lists the offending rows. Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5f5e234 commit 0f59584

8 files changed

Lines changed: 1298 additions & 76 deletions

File tree

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
fix(metadata-protocol): `sys_setting`'s declared row identity is enforced on the tenant and global layers — a runtime NULL-safe UNIQUE index over `COALESCE(user_id, '')` (#8629)
6+
7+
<!-- adr-0087: not-required (no-migration-prescription) Adds one runtime index
8+
migration module and its exports to `@objectstack/metadata-protocol`, armed at
9+
`kernel:ready`. No authorable metadata surface is added, renamed, retired or
10+
tombstoned — `packages/spec` and the `sys_setting` declaration itself are
11+
untouched, deliberately: expressing NULL-safe uniqueness in the declared
12+
vocabulary is the deferred route-2 half. Nothing exists for `objectstack migrate
13+
meta` to rewrite, and no author has to change any file. -->
14+
15+
`sys-setting.object.ts` declares the object's row identity as
16+
`{ fields: ['namespace', 'key', 'scope', 'user_id'], unique: 'organization' }`,
17+
and the object's own header calls that the row identity. It was not one.
18+
`user_id` is NULL on every row that is not `scope='user'` — `SettingsService.set`
19+
computes it as `scope === 'user' ? ctx.userId ?? null : null` — and SQL UNIQUE
20+
treats NULLs as mutually distinct, so the constraint was **void on the `tenant`
21+
and `global` limbs**: exactly the two carrying organization-level and
22+
platform-level configuration.
23+
24+
Measured on a real engine, before this fix: two identical `scope='tenant'` rows
25+
in ONE organization both landed (`201`, `201`), two identical `scope='global'`
26+
platform defaults both landed, while the same rows with a non-NULL `user_id`
27+
were refused — the control that identifies the mechanism as the NULL rather than
28+
the `scope` value. `SettingsService` then resolves a layer with a positional
29+
`rows.find(...)` and `set()` upserts against `{ namespace, key, scope, user_id }`,
30+
so which value an organization got for a tenant-scoped key was unspecified and
31+
two rows could disagree indefinitely with no way for an admin to see why the
32+
effective value was not the one they set. `lifecycle.retention_overrides` is a
33+
live tenant-scoped key, so this reached real retention behaviour.
34+
35+
The fix follows the paradigm that has shipped twice in this package
36+
(`ensureOverlayIndex`, `ensureViewDefinitionActiveIndex`): at `kernel:ready` the
37+
declared index is rebuilt in raw SQL with both nullable key parts folded —
38+
`COALESCE(organization_id, '__global__')` (ADR-0120 D3's tenant form, unchanged
39+
from what the driver already emits) and `COALESCE(user_id, '')` (the
40+
`ensureOverlayIndex` spelling for a non-tenant nullable discriminator). Storage
41+
is untouched: the row keeps its NULL, only the index folds it. The index reuses
42+
the **declared name**, so the additive `syncDeclaredIndexes` — which skips by
43+
name — never re-imposes the NULL-distinct form on a later boot, and the drift
44+
reconciler leaves it alone because an index carrying a non-tenant expression key
45+
part is not sync-reproducible.
46+
47+
**⚠️ Operator-visible: this is a TIGHTENING, and on an installation that has
48+
already accumulated duplicate settings rows it will REFUSE to build the index.**
49+
That is the intended behaviour, not a failure mode to work around. Those
50+
duplicates exist precisely because the constraint has been void, and settings
51+
rows are admin-authored configuration, so no row is discarded automatically and
52+
no deterministic keep-one rule is applied. On refusal:
53+
54+
- **nothing is deleted, rewritten or reordered**, and the boot continues;
55+
- the **previous index stays in place** — the tightening is proved buildable
56+
under a throwaway probe name before the declared name is ever dropped, so the
57+
table never spends a moment with no unique index at all;
58+
- one `error` line names the key that is not enforced, the consequence (duplicate
59+
tenant-scope and global-scope rows can still be created, and `SettingsService`
60+
has no defined answer for which one wins), and ships the **exact query that
61+
lists the offending rows**, so the operator has the list from the boot log
62+
without waiting for `os migrate plan`;
63+
- the migration keeps refusing on every boot until an operator decides which row
64+
survives, then converges on the next restart.
65+
66+
Two hosts are deliberately quiet rather than degraded: a kernel composed without
67+
the optional `service-settings` has no `sys_setting` table at all, which is
68+
probed for and is a silent no-op; and a MySQL/MariaDB server that rejects
69+
functional key parts keeps the previous index and is told what is not enforced,
70+
the same degradation `SqlDriver.createNullSafeUniqueIndex` already reports for
71+
this class of event.

‎packages/drivers/driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts‎

Lines changed: 158 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,12 @@ import { SqlDriver, classifyIndexKeyPart, parseIndexDdl, legacyUniqueReplacement
4747
* the `user` limb exactly as predicted, but on the `tenant` and `global` limbs
4848
* the installation-wide index enforces **nothing at all**: `user_id` is NULL on
4949
* every such row and SQL UNIQUE is NULL-distinct, so even a SAME-organization
50-
* duplicate is accepted. Section 4 pins that as a live fact. It is a second,
51-
* independent defect, it is filed separately, and this respelling does not fix
52-
* it — stated here so the suite cannot be read as claiming otherwise.
50+
* duplicate is accepted. It is a second, independent defect, it was filed
51+
* separately as #8629, and **this respelling does not fix it** — that is still
52+
* true and section 4 still measures it on both spellings, so the suite cannot
53+
* be read as claiming otherwise. What section 4 additionally pins since #8629
54+
* landed is the fix that DOES close it: a runtime NULL-safe UNIQUE index over
55+
* `COALESCE(user_id, '')`, claiming this same declared index name.
5356
*
5457
* ## Why this suite is at the DRIVER level
5558
*
@@ -490,45 +493,77 @@ describe('#8555 — sys_setting: the declared unique index becomes per-organizat
490493
});
491494

492495
// ─────────────────────────────────────────────────────────────────────────
493-
// 4. The OTHER defect this card does not fix — pinned so it cannot be
494-
// mistaken for fixed, and so the follow-up card has a live repro
496+
// 4. The OTHER defect — filed as #8629 and CLOSED there. What was pinned
497+
// here as a live hole is now pinned as the fix, on the same rows.
495498
// ─────────────────────────────────────────────────────────────────────────
496499

497-
describe('the NULL-distinct user_id hole (out of scope here — filed as #8629)', () => {
500+
describe('the NULL-distinct user_id hole — closed by the #8629 runtime migration', () => {
498501
/**
499502
* `user_id` is NULL on every `scope='tenant'` and `scope='global'` row —
500503
* `SettingsService` writes `scope === 'user' ? ctx.userId : null` — and SQL
501-
* UNIQUE treats NULLs as distinct. The declared row identity is therefore
504+
* UNIQUE treats NULLs as distinct. The declared row identity was therefore
502505
* void on exactly the two limbs that carry organization-level and
503-
* platform-level configuration, BEFORE and AFTER this card.
506+
* platform-level configuration, both before and after #8555.
504507
*
505-
* The organization key part is NULL-safe (ADR-0120 D3); the author-declared
506-
* `user_id` column is not, and extending null-safety to arbitrary declared
507-
* columns is a contract decision plus a duplicate pre-flight for the
508-
* databases that have already accumulated duplicates. Hence a separate card:
509-
* #8629.
508+
* #8629 closed it the way the maintainer ruled on 2026-08-14: **route 1**,
509+
* a runtime NULL-safe UNIQUE index issued at `kernel:ready` (the PR #6666
510+
* paradigm), reusing this same declared index NAME so the additive sync —
511+
* which skips by name — never re-imposes the NULL-distinct form. The
512+
* declaration itself is unchanged; extending the authorable vocabulary so it
513+
* could state this is route 2, deferred to v18.
510514
*
511-
* These assertions are written to go RED when that card lands — a fix must
512-
* come here and rewrite them, rather than leaving a stale "known hole"
513-
* comment behind.
515+
* ## What these cases assert, and what they deliberately do not
516+
*
517+
* The migration lives in `@objectstack/metadata-protocol`, which this
518+
* package must not depend on (the boundary #8461 / #8554 / #8556 kept, and
519+
* the reason the declaration above is hand-copied too). So the DDL is
520+
* applied here through the driver's own raw seam, from a literal
521+
* hand-copied from `sys-setting-identity-index.ts`'s
522+
* `buildSysSettingIdentityIndexSql`.
523+
*
524+
* ⚠️ Guarded in ONE direction, like the declaration mirror above:
525+
* `metadata-protocol`'s `sys-setting-identity-index.test.ts` pins the
526+
* builder's output against its own literal, and pins the index-name literal
527+
* that `REPLACEMENT_NAME` also spells — so a change to the shipped migration
528+
* that is not mirrored here goes red over there. The reverse is unguarded —
529+
* change these only together.
530+
*
531+
* What is asserted here is the half only a driver can answer: that THIS DDL,
532+
* over the REAL declared index this driver materializes, closes the hole
533+
* without weakening anything else, survives a later boot's additive sync,
534+
* and is not reported as drift the reconciler would undo.
514535
*/
536+
const NULL_SAFE_IDENTITY_DDL =
537+
`CREATE UNIQUE INDEX IF NOT EXISTS ${REPLACEMENT_NAME} ON ${TABLE} ` +
538+
"(COALESCE(organization_id, '__global__'), namespace, key, scope, COALESCE(user_id, ''))";
539+
540+
/** The migration's probe-first order, through the driver's raw seam. */
541+
const runIdentityMigration = async (d: SqlDriver): Promise<void> => {
542+
await d.execute(`DROP INDEX IF EXISTS ${REPLACEMENT_NAME}`);
543+
await d.execute(NULL_SAFE_IDENTITY_DDL);
544+
};
545+
546+
const tenantRow = { scope: 'tenant', user_id: null, namespace: 'lifecycle', key: 'retention_overrides' };
547+
const globalRow = { scope: 'global', user_id: null, key: 'platform_default' };
548+
515549
it('BEFORE: two organizations CAN both hold the same tenant-scope key — the constraint is void, not oracular', async () => {
550+
// Kept as measured on the PRE declaration: the installation-wide index
551+
// did not refuse this either, and the reason was never the organization
552+
// scope — it is that the index cannot see these rows as equal at all.
516553
const d = makeDriver();
517554
await d.initObjects(app('pre') as any);
518555

519-
const tenantRow = { scope: 'tenant', user_id: null, namespace: 'lifecycle', key: 'retention_overrides' };
520556
expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status).toBe(201);
521-
// 201, not 409: the card predicted a refusal here. The refusal never
522-
// happens because the index cannot see these rows as equal at all.
523557
expect((await createAsApi(d, row('b', 'org_yi', tenantRow))).status).toBe(201);
524558
});
525559

526-
it('BEFORE and AFTER: a SAME-organization tenant-scope duplicate is accepted — declared row identity unenforced', async () => {
560+
it('BEFORE: a SAME-organization tenant-scope duplicate is accepted — the declared row identity is unenforced', async () => {
561+
// The defect, still measured on both spellings of the declaration, because
562+
// it is what the runtime migration below has to be measured AGAINST.
527563
for (const which of ['pre', 'fixed'] as const) {
528564
const d = makeDriver();
529565
await d.initObjects(app(which) as any);
530566

531-
const tenantRow = { scope: 'tenant', user_id: null, namespace: 'lifecycle', key: 'retention_overrides' };
532567
expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status, which).toBe(201);
533568
expect((await createAsApi(d, row('b', 'org_jia', tenantRow))).status, which).toBe(201);
534569
expect(await d.count(TABLE, {}), which).toBe(2);
@@ -538,29 +573,118 @@ describe('#8555 — sys_setting: the declared unique index becomes per-organizat
538573
}
539574
});
540575

541-
it('BEFORE and AFTER: the same hole on the platform (`scope=global`) layer', async () => {
542-
for (const which of ['pre', 'fixed'] as const) {
543-
const d = makeDriver();
544-
await d.initObjects(app(which) as any);
576+
it('AFTER the migration: the SAME-organization tenant-scope duplicate is refused — 201 flips to 409', async () => {
577+
const d = makeDriver();
578+
await d.initObjects(app('fixed') as any);
579+
await runIdentityMigration(d);
545580

546-
const globalRow = { scope: 'global', user_id: null, key: 'platform_default' };
547-
expect((await createAsApi(d, row('a', undefined, globalRow))).status, which).toBe(201);
548-
expect((await createAsApi(d, row('b', undefined, globalRow))).status, which).toBe(201);
581+
expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status).toBe(201);
582+
expect(await createAsApi(d, row('b', 'org_jia', tenantRow))).toMatchObject(CONFLICT_ENVELOPE);
583+
expect(await d.count(TABLE, {})).toBe(1);
584+
});
549585

550-
await d.disconnect();
551-
driver = undefined;
552-
}
586+
it('AFTER the migration: two platform defaults on the `scope=global` layer are refused', async () => {
587+
const d = makeDriver();
588+
await d.initObjects(app('fixed') as any);
589+
await runIdentityMigration(d);
590+
591+
expect((await createAsApi(d, row('a', undefined, globalRow))).status).toBe(201);
592+
expect(await createAsApi(d, row('b', undefined, globalRow))).toMatchObject(CONFLICT_ENVELOPE);
553593
});
554594

555-
it('the hole is the NULL, not the layer — the same rows with a non-null user_id ARE constrained', async () => {
556-
// The control that identifies the mechanism. Without it, "tenant rows are
557-
// unconstrained" could be read as something about the `scope` value.
595+
it('AFTER the migration (anti-vacuity): the key is still PER-ORGANIZATION', async () => {
596+
// The tightening must not quietly become the installation-wide constraint
597+
// #8555 just relaxed — a strictly worse index would satisfy the two cases
598+
// above just as well.
558599
const d = makeDriver();
559600
await d.initObjects(app('fixed') as any);
601+
await runIdentityMigration(d);
602+
603+
expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status).toBe(201);
604+
expect((await createAsApi(d, row('b', 'org_yi', tenantRow))).status).toBe(201);
605+
});
606+
607+
it('AFTER the migration: the user layer is untouched — same user refused, different user allowed', async () => {
608+
const d = makeDriver();
609+
await d.initObjects(app('fixed') as any);
610+
await runIdentityMigration(d);
560611

561612
const named = { scope: 'tenant', user_id: 'usr_1', namespace: 'lifecycle', key: 'retention_overrides' };
562613
expect((await createAsApi(d, row('a', 'org_jia', named))).status).toBe(201);
563614
expect(await createAsApi(d, row('b', 'org_jia', named))).toMatchObject(CONFLICT_ENVELOPE);
615+
expect((await createAsApi(d, row('c', 'org_jia', { ...named, user_id: 'usr_2' }))).status).toBe(201);
616+
});
617+
618+
it('the hole was the NULL, not the layer — the control still reads the same on both sides', async () => {
619+
// The control that identifies the mechanism. It was already 409 before the
620+
// migration and stays 409 after, which is what makes the two flips above
621+
// attributable to the NULL folding and to nothing else.
622+
const d = makeDriver();
623+
await d.initObjects(app('fixed') as any);
624+
625+
const named = { scope: 'tenant', user_id: 'usr_1', namespace: 'lifecycle', key: 'retention_overrides' };
626+
expect((await createAsApi(d, row('a', 'org_jia', named))).status).toBe(201);
627+
expect(await createAsApi(d, row('b', 'org_jia', named))).toMatchObject(CONFLICT_ENVELOPE);
628+
});
629+
630+
it('the migrated index survives a later boot — the additive sync skips the name', async () => {
631+
// The durability half of route 1, and the reason the migration reuses the
632+
// DECLARED name: `syncDeclaredIndexes` skips by name, so a differently
633+
// named index would be silently undone on the next boot.
634+
const d = makeDriver();
635+
await d.initObjects(app('fixed') as any);
636+
await runIdentityMigration(d);
637+
638+
await d.initObjects(app('fixed') as any);
639+
640+
expect(await uniqueKeyParts()).toEqual({
641+
[REPLACEMENT_NAME]: ['COALESCE(organization_id)', 'namespace', 'key', 'scope', 'COALESCE(user_id)'],
642+
});
643+
expect(await createAsApi(d, row('a', 'org_jia', tenantRow))).toMatchObject({ status: 201 });
644+
expect(await createAsApi(d, row('b', 'org_jia', tenantRow))).toMatchObject(CONFLICT_ENVELOPE);
645+
});
646+
647+
it('is NOT reported as drift — the reconciler must never propose rebuilding it away', async () => {
648+
// `recreate_index` drops before it creates, so a drift finding here would
649+
// be a proposal to replace this index with the NULL-distinct one. It is
650+
// silent by construction, not by luck: `isSyncReproducibleIndex` admits
651+
// only the tenant column's COALESCE, this index carries a second one over
652+
// `user_id`, so `isRuntimeManagedIndex` claims it (#4884).
653+
const d = makeDriver();
654+
await d.initObjects(app('fixed') as any);
655+
await runIdentityMigration(d);
656+
657+
expect(await d.detectManagedDrift()).toHaveLength(0);
658+
// Again after a later boot's sync, when the runtime ledger no longer
659+
// remembers issuing the DDL — the durable witness is the index's shape.
660+
await d.initObjects(app('fixed') as any);
661+
expect(await d.detectManagedDrift()).toHaveLength(0);
662+
});
663+
664+
it('on a duplicate-carrying database the tightening is REFUSED, and nothing is deleted', async () => {
665+
// The maintainer's 2026-08-14 disposition, at the layer that would do the
666+
// deleting: the CREATE fails on the existing rows, every row survives, and
667+
// the previous index is still the one on the table. `metadata-protocol`
668+
// owns the probe-first order that keeps it there; this asserts the fact
669+
// the driver can see — the refusal is a UNIQUE violation over real rows.
670+
const d = makeDriver();
671+
await d.initObjects(app('fixed') as any);
672+
await d.create(TABLE, row('a', 'org_jia', tenantRow) as any);
673+
await d.create(TABLE, row('b', 'org_jia', tenantRow) as any);
674+
675+
let refusal: unknown;
676+
try {
677+
await d.execute(NULL_SAFE_IDENTITY_DDL.replace(REPLACEMENT_NAME, 'idx_sys_setting_identity_probe'));
678+
} catch (error) {
679+
refusal = error;
680+
}
681+
682+
expect(isUniqueViolationError(refusal)).toBe(true);
683+
expect(await d.count(TABLE, {})).toBe(2);
684+
expect(Object.keys(await uniqueKeyParts())).toEqual([REPLACEMENT_NAME]);
685+
expect(await uniqueKeyParts()).toEqual({
686+
[REPLACEMENT_NAME]: ['COALESCE(organization_id)', ...KEY_COLUMNS],
687+
});
564688
});
565689
});
566690

0 commit comments

Comments
 (0)