Skip to content

fix(metadata-protocol): enforce sys_setting's declared row identity on the tenant and global layers (#8629)#8719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hotlong merged 1 commit into from
Aug 14, 2026
Merged

fix(metadata-protocol): enforce sys_setting's declared row identity on the tenant and global layers (#8629) #8719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions .changeset/sys-setting-null-safe-row-identity.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
---
"@objectstack/metadata-protocol": patch
---

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)

<!-- adr-0087: not-required (no-migration-prescription) Adds one runtime index
migration module and its exports to `@objectstack/metadata-protocol`, armed at
`kernel:ready`. No authorable metadata surface is added, renamed, retired or
tombstoned — `packages/spec` and the `sys_setting` declaration itself are
untouched, deliberately: expressing NULL-safe uniqueness in the declared
vocabulary is the deferred route-2 half. Nothing exists for `objectstack migrate
meta` to rewrite, and no author has to change any file. -->

`sys-setting.object.ts` declares the object's row identity as
`{ fields: ['namespace', 'key', 'scope', 'user_id'], unique: 'organization' }`,
and the object's own header calls that the row identity. It was not one.
`user_id` is NULL on every row that is not `scope='user'` — `SettingsService.set`
computes it as `scope === 'user' ? ctx.userId ?? null : null` — and SQL UNIQUE
treats NULLs as mutually distinct, so the constraint was **void on the `tenant`
and `global` limbs**: exactly the two carrying organization-level and
platform-level configuration.

Measured on a real engine, before this fix: two identical `scope='tenant'` rows
in ONE organization both landed (`201`, `201`), two identical `scope='global'`
platform defaults both landed, while the same rows with a non-NULL `user_id`
were refused — the control that identifies the mechanism as the NULL rather than
the `scope` value. `SettingsService` then resolves a layer with a positional
`rows.find(...)` and `set()` upserts against `{ namespace, key, scope, user_id }`,
so which value an organization got for a tenant-scoped key was unspecified and
two rows could disagree indefinitely with no way for an admin to see why the
effective value was not the one they set. `lifecycle.retention_overrides` is a
live tenant-scoped key, so this reached real retention behaviour.

The fix follows the paradigm that has shipped twice in this package
(`ensureOverlayIndex`, `ensureViewDefinitionActiveIndex`): at `kernel:ready` the
declared index is rebuilt in raw SQL with both nullable key parts folded —
`COALESCE(organization_id, '__global__')` (ADR-0120 D3's tenant form, unchanged
from what the driver already emits) and `COALESCE(user_id, '')` (the
`ensureOverlayIndex` spelling for a non-tenant nullable discriminator). Storage
is untouched: the row keeps its NULL, only the index folds it. The index reuses
the **declared name**, so the additive `syncDeclaredIndexes` — which skips by
name — never re-imposes the NULL-distinct form on a later boot, and the drift
reconciler leaves it alone because an index carrying a non-tenant expression key
part is not sync-reproducible.

**⚠️ Operator-visible: this is a TIGHTENING, and on an installation that has
already accumulated duplicate settings rows it will REFUSE to build the index.**
That is the intended behaviour, not a failure mode to work around. Those
duplicates exist precisely because the constraint has been void, and settings
rows are admin-authored configuration, so no row is discarded automatically and
no deterministic keep-one rule is applied. On refusal:

- **nothing is deleted, rewritten or reordered**, and the boot continues;
- the **previous index stays in place** — the tightening is proved buildable
under a throwaway probe name before the declared name is ever dropped, so the
table never spends a moment with no unique index at all;
- one `error` line names the key that is not enforced, the consequence (duplicate
tenant-scope and global-scope rows can still be created, and `SettingsService`
has no defined answer for which one wins), and ships the **exact query that
lists the offending rows**, so the operator has the list from the boot log
without waiting for `os migrate plan`;
- the migration keeps refusing on every boot until an operator decides which row
survives, then converges on the next restart.

Two hosts are deliberately quiet rather than degraded: a kernel composed without
the optional `service-settings` has no `sys_setting` table at all, which is
probed for and is a silent no-op; and a MySQL/MariaDB server that rejects
functional key parts keeps the previous index and is told what is not enforced,
the same degradation `SqlDriver.createNullSafeUniqueIndex` already reports for
this class of event.
192 changes: 158 additions & 34 deletions packages/drivers/driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -47,9 +47,12 @@ import { SqlDriver, classifyIndexKeyPart, parseIndexDdl, legacyUniqueReplacement
* 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.
* duplicate is accepted. It is a second, independent defect, it was filed
* separately as #8629, and **this respelling does not fix it** — that is still
* true and section 4 still measures it on both spellings, so the suite cannot
* be read as claiming otherwise. What section 4 additionally pins since #8629
* landed is the fix that DOES close it: a runtime NULL-safe UNIQUE index over
* `COALESCE(user_id, '')`, claiming this same declared index name.
*
* ## Why this suite is at the DRIVER level
*
Expand DownExpand Up@@ -490,45 +493,77 @@ describe('#8555 — sys_setting: the declared unique index becomes per-organizat
});

// ─────────────────────────────────────────────────────────────────────────
// 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
// 4. The OTHER defect — filed as #8629 and CLOSED there. What was pinned
// here as a live hole is now pinned as the fix, on the same rows.
// ─────────────────────────────────────────────────────────────────────────

describe('the NULL-distinct user_id hole (out of scope here — filed as #8629)', () => {
describe('the NULL-distinct user_id hole — closed by the #8629 runtime migration', () => {
/**
* `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
* UNIQUE treats NULLs as distinct. The declared row identity was therefore
* void on exactly the two limbs that carry organization-level and
* platform-level configuration, BEFORE and AFTER this card.
* platform-level configuration, both before and after #8555.
*
* 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.
* #8629 closed it the way the maintainer ruled on 2026-08-14: **route 1**,
* a runtime NULL-safe UNIQUE index issued at `kernel:ready` (the PR #6666
* paradigm), reusing this same declared index NAME so the additive sync —
* which skips by name — never re-imposes the NULL-distinct form. The
* declaration itself is unchanged; extending the authorable vocabulary so it
* could state this is route 2, deferred to v18.
*
* 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.
* ## What these cases assert, and what they deliberately do not
*
* The migration lives in `@objectstack/metadata-protocol`, which this
* package must not depend on (the boundary #8461 / #8554 / #8556 kept, and
* the reason the declaration above is hand-copied too). So the DDL is
* applied here through the driver's own raw seam, from a literal
* hand-copied from `sys-setting-identity-index.ts`'s
* `buildSysSettingIdentityIndexSql`.
*
* ⚠️ Guarded in ONE direction, like the declaration mirror above:
* `metadata-protocol`'s `sys-setting-identity-index.test.ts` pins the
* builder's output against its own literal, and pins the index-name literal
* that `REPLACEMENT_NAME` also spells — so a change to the shipped migration
* that is not mirrored here goes red over there. The reverse is unguarded —
* change these only together.
*
* What is asserted here is the half only a driver can answer: that THIS DDL,
* over the REAL declared index this driver materializes, closes the hole
* without weakening anything else, survives a later boot's additive sync,
* and is not reported as drift the reconciler would undo.
*/
const NULL_SAFE_IDENTITY_DDL =
`CREATE UNIQUE INDEX IF NOT EXISTS ${REPLACEMENT_NAME} ON ${TABLE} ` +
"(COALESCE(organization_id, '__global__'), namespace, key, scope, COALESCE(user_id, ''))";

/** The migration's probe-first order, through the driver's raw seam. */
const runIdentityMigration = async (d: SqlDriver): Promise<void> => {
await d.execute(`DROP INDEX IF EXISTS ${REPLACEMENT_NAME}`);
await d.execute(NULL_SAFE_IDENTITY_DDL);
};

const tenantRow = { scope: 'tenant', user_id: null, namespace: 'lifecycle', key: 'retention_overrides' };
const globalRow = { scope: 'global', user_id: null, key: 'platform_default' };

it('BEFORE: two organizations CAN both hold the same tenant-scope key — the constraint is void, not oracular', async () => {
// Kept as measured on the PRE declaration: the installation-wide index
// did not refuse this either, and the reason was never the organization
// scope — it is that the index cannot see these rows as equal at all.
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 () => {
it('BEFORE: a SAME-organization tenant-scope duplicate is accepted — the declared row identity is unenforced', async () => {
// The defect, still measured on both spellings of the declaration, because
// it is what the runtime migration below has to be measured AGAINST.
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);
Expand All@@ -538,29 +573,118 @@ describe('#8555 — sys_setting: the declared unique index becomes per-organizat
}
});

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);
it('AFTER the migration: the SAME-organization tenant-scope duplicate is refused — 201 flips to 409', async () => {
const d = makeDriver();
await d.initObjects(app('fixed') as any);
await runIdentityMigration(d);

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);
expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status).toBe(201);
expect(await createAsApi(d, row('b', 'org_jia', tenantRow))).toMatchObject(CONFLICT_ENVELOPE);
expect(await d.count(TABLE, {})).toBe(1);
});

await d.disconnect();
driver = undefined;
}
it('AFTER the migration: two platform defaults on the `scope=global` layer are refused', async () => {
const d = makeDriver();
await d.initObjects(app('fixed') as any);
await runIdentityMigration(d);

expect((await createAsApi(d, row('a', undefined, globalRow))).status).toBe(201);
expect(await createAsApi(d, row('b', undefined, globalRow))).toMatchObject(CONFLICT_ENVELOPE);
});

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.
it('AFTER the migration (anti-vacuity): the key is still PER-ORGANIZATION', async () => {
// The tightening must not quietly become the installation-wide constraint
// #8555 just relaxed — a strictly worse index would satisfy the two cases
// above just as well.
const d = makeDriver();
await d.initObjects(app('fixed') as any);
await runIdentityMigration(d);

expect((await createAsApi(d, row('a', 'org_jia', tenantRow))).status).toBe(201);
expect((await createAsApi(d, row('b', 'org_yi', tenantRow))).status).toBe(201);
});

it('AFTER the migration: the user layer is untouched — same user refused, different user allowed', async () => {
const d = makeDriver();
await d.initObjects(app('fixed') as any);
await runIdentityMigration(d);

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);
expect((await createAsApi(d, row('c', 'org_jia', { ...named, user_id: 'usr_2' }))).status).toBe(201);
});

it('the hole was the NULL, not the layer — the control still reads the same on both sides', async () => {
// The control that identifies the mechanism. It was already 409 before the
// migration and stays 409 after, which is what makes the two flips above
// attributable to the NULL folding and to nothing else.
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);
});

it('the migrated index survives a later boot — the additive sync skips the name', async () => {
// The durability half of route 1, and the reason the migration reuses the
// DECLARED name: `syncDeclaredIndexes` skips by name, so a differently
// named index would be silently undone on the next boot.
const d = makeDriver();
await d.initObjects(app('fixed') as any);
await runIdentityMigration(d);

await d.initObjects(app('fixed') as any);

expect(await uniqueKeyParts()).toEqual({
[REPLACEMENT_NAME]: ['COALESCE(organization_id)', 'namespace', 'key', 'scope', 'COALESCE(user_id)'],
});
expect(await createAsApi(d, row('a', 'org_jia', tenantRow))).toMatchObject({ status: 201 });
expect(await createAsApi(d, row('b', 'org_jia', tenantRow))).toMatchObject(CONFLICT_ENVELOPE);
});

it('is NOT reported as drift — the reconciler must never propose rebuilding it away', async () => {
// `recreate_index` drops before it creates, so a drift finding here would
// be a proposal to replace this index with the NULL-distinct one. It is
// silent by construction, not by luck: `isSyncReproducibleIndex` admits
// only the tenant column's COALESCE, this index carries a second one over
// `user_id`, so `isRuntimeManagedIndex` claims it (#4884).
const d = makeDriver();
await d.initObjects(app('fixed') as any);
await runIdentityMigration(d);

expect(await d.detectManagedDrift()).toHaveLength(0);
// Again after a later boot's sync, when the runtime ledger no longer
// remembers issuing the DDL — the durable witness is the index's shape.
await d.initObjects(app('fixed') as any);
expect(await d.detectManagedDrift()).toHaveLength(0);
});

it('on a duplicate-carrying database the tightening is REFUSED, and nothing is deleted', async () => {
// The maintainer's 2026-08-14 disposition, at the layer that would do the
// deleting: the CREATE fails on the existing rows, every row survives, and
// the previous index is still the one on the table. `metadata-protocol`
// owns the probe-first order that keeps it there; this asserts the fact
// the driver can see — the refusal is a UNIQUE violation over real rows.
const d = makeDriver();
await d.initObjects(app('fixed') as any);
await d.create(TABLE, row('a', 'org_jia', tenantRow) as any);
await d.create(TABLE, row('b', 'org_jia', tenantRow) as any);

let refusal: unknown;
try {
await d.execute(NULL_SAFE_IDENTITY_DDL.replace(REPLACEMENT_NAME, 'idx_sys_setting_identity_probe'));
} catch (error) {
refusal = error;
}

expect(isUniqueViolationError(refusal)).toBe(true);
expect(await d.count(TABLE, {})).toBe(2);
expect(Object.keys(await uniqueKeyParts())).toEqual([REPLACEMENT_NAME]);
expect(await uniqueKeyParts()).toEqual({
[REPLACEMENT_NAME]: ['COALESCE(organization_id)', ...KEY_COLUMNS],
});
});
});

Expand Down
Loading
Loading