From 3106b9547546eb4ba7f1f652f3f44a9f1507ece2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 05:57:00 +0000 Subject: [PATCH] fix(objectql): wantOwner becomes an allow-list; inject owning_business_unit_id (ADR-0117 D1) (#5677) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `applySystemFields` decided owner injection with a DENY-list — only 'org' and 'none' opted out — so ANY fourth `ownership` value fell through to the default branch and was stamped with `owner_id`. ADR-0117 D1's new `business_unit` tier means the opposite ("owned by a UNIT, not a person"): owner_id no, owning_business_unit_id yes. #4611's one-shot probe measured the inversion. The judgement is now a positive list (`undefined | 'user'`), which is behaviour-identical for the three values that exist today and stops a new tier from inheriting the owner branch by accident. A second anchor covers the union of the owner tiers with `business_unit` and injects `owning_business_unit_id` (lookup to sys_business_unit). The column is shaped after `organization_id` (server-stamped scope anchor), not after `owner_id` (user-assignable field): readonly, hidden, nullable, unindexed. None of those presumes ADR-0117 D2's undecided stamping policy — they grant no capability, so every D2 outcome stays reachable; the column is provisioned but inert until the stamping middleware (D2/D4) and backfill (D8) land. The objectql conformance test migrates the name from the documented-reserved group to Group A, where it is derived from the live injection code — the migration #4611 left a note for. Scope is the issue's work items 1-3. The spec-side JSDoc/description sync is #5767 and the `ownership` enum member is #5678, both strictly after this PR. Fixes #5677 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V7WetGmnfoXNn8cLieKKmx --- .changeset/owning-business-unit-injection.md | 42 +++++++ packages/objectql/src/registry.test.ts | 116 ++++++++++++++++++ packages/objectql/src/registry.ts | 109 +++++++++++++++- .../system-managed-fields-conformance.test.ts | 38 +++--- 4 files changed, 285 insertions(+), 20 deletions(-) create mode 100644 .changeset/owning-business-unit-injection.md diff --git a/.changeset/owning-business-unit-injection.md b/.changeset/owning-business-unit-injection.md new file mode 100644 index 0000000000..b4aade369e --- /dev/null +++ b/.changeset/owning-business-unit-injection.md @@ -0,0 +1,42 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): `wantOwner` 由排除式改为正面清单,并注入 `owning_business_unit_id`(ADR-0117 D1,#5677) + +`applySystemFields` 判定是否注入 `owner_id` 的 `wantOwner` 原本是**排除式**的 +(`ownership !== 'org' && ownership !== 'none'`):只有两个值被排除,任何第四个 +`ownership` 值都会掉进默认分支、照常被盖上 `owner_id`。这与 ADR-0117 D1 的表格 +**恰好相反**——新增的 `business_unit` 档的全部含义就是「归属于组织单元而非个人」 +(`owner_id` ❌、`owning_business_unit_id` ✅)。#4611 的一次性探针已实测确认过这个 +反向结果。 + +**翻面。** 判定改为正面清单 `ownership === undefined || ownership === 'user'` +(`managedBy` 平台表与 `sys_` 命名空间的跳过规则不变)。对今天存在的三个值 +**行为完全不变**:`undefined`/`user` 照常注入,`org`/`none` 照常排除;唯一的变化是 +新档位不再靠「落进默认分支」继承 owner 语义。这是先决单点:先翻面,枚举扩展 +(#5678)才是安全的。 + +**注入 `owning_business_unit_id`。** 记录级组织单元归属(lookup → `sys_business_unit`), +按 D1 表格覆盖 `undefined`/`user`/`business_unit` 三档,`org`/`none` 不注入。 + +| `ownership` | `owner_id` | `owning_business_unit_id` | +|---|---|---| +| `undefined` / `user` | ✅ | ✅ | +| `business_unit` | ❌ | ✅ | +| `org` / `none` | ❌ | ❌ | + +**用户可见的行为变化**:声明 `ownership: 'business_unit'` 的对象此前会被误注入 +`owner_id`,现在改为注入 `owning_business_unit_id`。(该值今天仍被 `ObjectSchema` +的枚举拒收 —— 枚举扩展在 #5678,严格后置于本单;所以本次发布中这条路径只有 +引擎侧就绪,尚无法从元数据声明触达。) + +列的形态比照 `organization_id`(服务端盖章的作用域锚点)而非 `owner_id`(用户可指派 +的业务字段):`readonly: true` + `hidden: true` + `required: false`,不建索引。三者 +都**不预设** ADR-0117 D2 尚未裁定的盖章策略(`pinned`/`follow_owner`/`transferable`) +——它们不授予任何能力,因此 D2 的每种结论都仍然可达;盖章中间件(D2/D4)与回填 +(D8)落地前,该列被提供但处于惰性状态(恒为 NULL)。 + +`packages/spec` 未改动:`SystemFieldName.OWNING_BUSINESS_UNIT_ID` 的 JSDoc 与 +`object.zod.ts` 的描述同步在 #5767。该名早已在公开表单 server-managed 拒收名单上 +(#4611 提前登记),因此新列从第一天起就不可由匿名面客户端提供。 diff --git a/packages/objectql/src/registry.test.ts b/packages/objectql/src/registry.test.ts index 4400274e08..a62bf04b9c 100644 --- a/packages/objectql/src/registry.test.ts +++ b/packages/objectql/src/registry.test.ts @@ -732,6 +732,122 @@ describe('applySystemFields', () => { expect(out.fields.owner_id.readonly).toBe(true); }); + // ── ADR-0117 D1 — owning_business_unit_id + the wantOwner allow-list ── + // + // The flip under test: `wantOwner` used to be a DENY-list + // (`ownership !== 'org' && ownership !== 'none' && …`), so any value + // outside those two — including D1's new `business_unit` tier — fell into + // the default branch and got stamped with `owner_id`, the exact INVERSE of + // D1's table. It is now an allow-list (`undefined | 'user'`), and a second + // anchor covers the union of the owner tiers with `business_unit`: + // + // ownership owner_id owning_business_unit_id + // undefined/user ✅ ✅ + // business_unit ❌ ✅ + // org ❌ ❌ + // none ❌ ❌ + // + // ⚠️ `ownership: 'business_unit'` is NOT a legal ObjectSchema value yet — + // the spec enum gains it in #5678, strictly AFTER this PR (that ordering is + // #5677's whole point: the engine must honour the tier before the schema + // can emit it, or the tier's first appearance gets the inverse result). + // These fixtures therefore duck-type the schema, exactly as every other + // opt-out case in this suite already does — `applySystemFields` takes a + // `ServiceObject`, and the registry reads `ownership` as a value, not as a + // Zod-parsed enum. When #5678 lands, the `as any` here can be dropped + // without changing a single assertion. + describe('[ADR-0117 D1] owning_business_unit_id injection', () => { + it("does NOT inject owner_id for ownership: 'business_unit' — but DOES inject owning_business_unit_id", () => { + // THE regression this issue exists to prevent. Under the old + // deny-list this object was stamped `owner_id` (a person) even + // though the tier's entire meaning is "owned by a unit, not a + // person" — see #4611's one-shot probe. + const unitOwned: any = { ...baseLead, name: 'inventory_item', ownership: 'business_unit' }; + const out = applySystemFields(unitOwned, { multiTenant: false }); + + expect(out.fields.owner_id).toBeUndefined(); + expect(out.fields.owning_business_unit_id).toMatchObject({ + type: 'lookup', + reference: 'sys_business_unit', + system: true, + }); + // Tenant + audit columns are orthogonal to the ownership axis. + expect(out.fields.organization_id).toBeDefined(); + expect(out.fields.created_at).toBeDefined(); + }); + + it("injects BOTH anchors on the default tier and on an explicit ownership: 'user'", () => { + for (const schema of [baseLead, { ...baseLead, ownership: 'user' } as any]) { + const out = applySystemFields(schema, { multiTenant: false }); + expect(out.fields.owner_id).toBeDefined(); + expect(out.fields.owning_business_unit_id).toBeDefined(); + } + }); + + it("injects NEITHER anchor for ownership: 'org' / 'none'", () => { + // The BU column follows `owner_id` out the door on the opt-out + // tiers: a catalog/junction table has no per-record owner of + // EITHER kind. (The pre-existing owner_id half of this is pinned + // separately above — that test is what proves the allow-list flip + // left the three existing values untouched.) + for (const ownership of ['org', 'none'] as const) { + const opted: any = { ...baseLead, ownership }; + const out = applySystemFields(opted, { multiTenant: false }); + expect(out.fields.owner_id).toBeUndefined(); + expect(out.fields.owning_business_unit_id).toBeUndefined(); + } + }); + + it('does NOT inject owning_business_unit_id for managedBy / sys_* tables', () => { + const platform: any = { name: 'proj_thing', managedBy: 'platform', fields: { msg: { type: 'text' } } }; + expect(applySystemFields(platform, { multiTenant: false }).fields.owning_business_unit_id).toBeUndefined(); + + const sysish: any = { name: 'sys_widget', fields: { msg: { type: 'text' } } }; + expect(applySystemFields(sysish, { multiTenant: false }).fields.owning_business_unit_id).toBeUndefined(); + + // …and the skip is not a side effect of the ownership tier: even + // the tier that WANTS the column doesn't get it on a managed table. + const managedUnitOwned: any = { + name: 'proj_thing', managedBy: 'platform', ownership: 'business_unit', fields: { msg: { type: 'text' } }, + }; + expect(applySystemFields(managedUnitOwned, { multiTenant: false }).fields.owning_business_unit_id).toBeUndefined(); + }); + + it('is shaped like organization_id (server-stamped anchor), not like owner_id (assignable field)', () => { + // The distinction is load-bearing, not cosmetic: D3 requires the + // value to be derived and validated server-side and D4 makes + // reassignment a transfer-class operation — neither guard has + // landed, so no client write path may reach the column. `readonly` + // + `hidden` grant no capability, which is what keeps every + // undecided D2 policy (pinned / follow_owner / transferable) + // reachable from here. + const out = applySystemFields(baseLead, { multiTenant: true }); + const bu = out.fields.owning_business_unit_id; + + expect(bu.readonly).toBe(true); // ≠ owner_id, which is reassignable + expect(out.fields.owner_id.readonly).toBe(false); + expect(bu.hidden).toBe(true); + expect(bu.system).toBe(true); + expect(bu.required).toBe(false); // nullable — nothing stamps it yet + // No index: the hierarchical predicate that would use one ships + // with the enterprise scope resolver (D6). Unlike organization_id, + // it is not gated on multiTenant — it is simply absent. + expect(bu.indexed).toBeUndefined(); + }); + + it('does NOT overwrite an author-declared owning_business_unit_id', () => { + // Same precedence rule as every other injected column: `additions` + // lose to `schema.fields`. + const declared: any = { + name: 'lead', + fields: { owning_business_unit_id: { type: 'lookup', reference: 'sys_business_unit', label: 'Dept', readonly: false } }, + }; + const out = applySystemFields(declared, { multiTenant: false }); + expect(out.fields.owning_business_unit_id.label).toBe('Dept'); + expect(out.fields.owning_business_unit_id.readonly).toBe(false); + }); + }); + it('SchemaRegistry({ multiTenant: true }) auto-injects on registerObject', () => { const reg = new SchemaRegistry({ multiTenant: true }); reg.registerObject({ name: 'lead', fields: { first_name: { type: 'text' } } } as any, 'crm', 'crm', 'own'); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 633835c6b1..6a3fa47327 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { ServiceObject, ObjectSchema, ObjectOwnership, provisionPrimary, resolveCrudAffordances, isTenancyDisabled, LEGACY_API_METHODS, AUDIT_PROVENANCE_FIELDS, type AuditProvenanceField } from '@objectstack/spec/data'; +import { SystemFieldName } from '@objectstack/spec/system'; import { resolveTenancyPosture, resolveSearchPinyinEnabled } from '@objectstack/types'; import { postureEnforcesWall } from '@objectstack/spec/security'; import { provisionSearchCompanion } from './search-companion.js'; @@ -233,6 +234,17 @@ export interface SchemaRegistryOptions { * opt-out is harmless (a spare nullable column), whereas forgetting to * ADD ownership — the failure mode we are eliminating — silently breaks * every owner-keyed feature. + * - `owning_business_unit_id` — [ADR-0117 D1] record-level ORG-UNIT + * ownership (lookup to `sys_business_unit`): the tier between `owner_id` + * (a person) and `organization_id` (a tenant). Injected on the same + * objects `owner_id` is, PLUS the `ownership: 'business_unit'` tier which + * has an owning unit but deliberately no owning person (inventory, asset + * ledgers, departmental budgets). Withheld for `org` / `none` exactly like + * `owner_id`. Shaped like `organization_id`, not like `owner_id` + * (`readonly: true`, `hidden: true`): it is a SERVER-STAMPED scope anchor, + * and the stamping middleware (ADR-0117 D2/D4) has not landed — so the + * column is provisioned but inert. See {@link applySystemFields}' injection + * site for why that shape presumes nothing about the undecided D2 policy. */ /** * Column definitions for the audit-provenance family, keyed by the spec's @@ -299,6 +311,15 @@ const AUDIT_FIELD_GOVERNANCE: Record [name, { readonly: true, system: true }]), ) as unknown as Record>; +/** + * [ADR-0117 D1] The injected BU-ownership column name, spelled out so it greps + * from this file, and annotated with the spec's registered protocol name so the + * two cannot drift: a rename in `SystemFieldName.OWNING_BUSINESS_UNIT_ID` makes + * this line a compile error rather than a silently orphaned injection. + */ +const OWNING_BUSINESS_UNIT_FIELD: typeof SystemFieldName.OWNING_BUSINESS_UNIT_ID = + 'owning_business_unit_id'; + export function applySystemFields( schema: ServiceObject, opts: { multiTenant: boolean } @@ -355,12 +376,42 @@ export function applySystemFields( // authors silently ship objects with no working ownership at all. // `ownership` is now a declared ObjectSchema field (record-ownership model), // so it reads off the typed schema — no `as any` (#3175). - const ownership = schema.ownership; - const wantOwner = - ownership !== 'org' && - ownership !== 'none' && - !(schema as any).managedBy && - !schema.name.startsWith('sys_'); + // + // [ADR-0117 D1 / #5677] Widened to `string` on purpose. The spec enum is + // `'user' | 'org' | 'none'` TODAY; D1's fourth tier `'business_unit'` is + // declared in #5678, strictly AFTER this PR — that ordering is the whole + // point of #5677: the engine must recognise the tier BEFORE the schema can + // emit it, or the tier's first appearance would be judged by the branch + // below and get the INVERSE of what D1 declares. Without the widening `tsc` + // rejects the comparison as a no-overlap literal test; with it the engine is + // ready and the enum lands into a runtime that already honours it. + const ownership: string | undefined = schema.ownership; + + // Platform-managed tables and the `sys_*` namespace never carry a per-record + // ownership anchor, whichever tier is declared — unchanged, and shared by + // both anchors below so they cannot drift apart. + const ownershipEligible = !(schema as any).managedBy && !schema.name.startsWith('sys_'); + + // [ADR-0117 D1 / #5677] POSITIVE LIST, deliberately — this used to read + // `ownership !== 'org' && ownership !== 'none'`, i.e. a DENY-list, so ANY + // value outside the two exclusions fell through to "inject `owner_id`". + // That default is safe only while the enum has exactly three members: D1's + // `business_unit` tier means "owned by a UNIT, not a person" (`owner_id` ❌, + // `owning_business_unit_id` ✅), and under the deny-list it would have been + // stamped with `owner_id` — the exact inverse. Behaviour for the three + // values that exist today is IDENTICAL (`undefined`/`user` inject, `org`/ + // `none` do not); the change is only that a NEW tier no longer inherits the + // owner branch by accident. + const wantOwner = ownershipEligible && (ownership === undefined || ownership === 'user'); + + // [ADR-0117 D1] The BU anchor covers the owner tiers PLUS `business_unit`: + // + // ownership owner_id owning_business_unit_id + // undefined/user ✅ ✅ + // business_unit ❌ ✅ + // org ❌ ❌ + // none ❌ ❌ + const wantOwningBusinessUnit = wantOwner || (ownershipEligible && ownership === 'business_unit'); const additions: Record = {}; // Platform-owned field settings that must WIN over a declared field, rather @@ -436,6 +487,52 @@ export function applySystemFields( }; } + // [ADR-0117 D1] Record-level business-unit ownership. Shaped after + // `organization_id` (server-stamped scope anchor), NOT after `owner_id` + // (a user-assignable business field) — the two differ in exactly the keys + // that decide who may write and whether it renders: + // + // • `readonly: true` — D3 requires the value to be derived and validated + // server-side (`record.organization_id` must equal the unit's org), so a + // client-supplied value is overwritten, never trusted. Reassignment is a + // TRANSFER-class operation gated on `allowTransfer` (D4), which has not + // landed; until it does, no client write path may set this column. + // • `hidden: true` — nothing stamps the column yet (D2's policy is + // undecided, D8's backfill unwritten), so it is provisioned-but-inert. + // Surfacing a permanently-NULL lookup on every business object's layout + // would advertise a capability the runtime does not deliver (Prime + // Directive #10). Presentation is a one-key flip for the PR that lands + // stamping — a capability grant is not. + // • `required: false` — nullable, like every other injected column. + // + // ⚠️ None of the three presumes ADR-0117 D2 (`pinned`/`follow_owner`/ + // `transferable`), which is NOT YET RULED. They are the fail-closed shape: + // they grant no capability, so every D2 outcome remains reachable. D2's + // `owningBusinessUnit.required` is an INSERT-time rejection rule, not column + // nullability — it cannot be read off this definition either. `system: true` + // writes (the future stamping middleware, seeds) are checked downstream of + // `readonly`, so the flag blocks clients without blocking the platform — + // exactly how `organization_id` is stamped today. + // + // No index: the hierarchical predicate that would use one (`… IN (units)`, + // D6) ships with the enterprise scope resolver. An index on a column nothing + // writes and nothing filters is dead weight — the same reasoning that gates + // `organization_id`'s index on `multiTenant`. + if (wantOwningBusinessUnit && !schema.fields?.[OWNING_BUSINESS_UNIT_FIELD]) { + additions[OWNING_BUSINESS_UNIT_FIELD] = { + type: 'lookup', + reference: 'sys_business_unit', + label: 'Owning Business Unit', + required: false, + hidden: true, + readonly: true, + system: true, + description: + 'Record-level business-unit ownership (ADR-0117 D1). Server-stamped scope anchor; ' + + 'NULL until the stamping middleware lands.', + }; + } + if (Object.keys(additions).length === 0 && Object.keys(overrides).length === 0) return schema; return { diff --git a/packages/objectql/src/system-managed-fields-conformance.test.ts b/packages/objectql/src/system-managed-fields-conformance.test.ts index 02583df148..e790566d8c 100644 --- a/packages/objectql/src/system-managed-fields-conformance.test.ts +++ b/packages/objectql/src/system-managed-fields-conformance.test.ts @@ -40,7 +40,9 @@ describe('[#3058] PUBLIC_FORM_SERVER_MANAGED_FIELDS conformance', () => { const activelyInjected = (): Set => new Set([ - ...injectedByRegistry(), // organization_id, created_at, created_by, updated_at, updated_by, owner_id + // organization_id, created_at, created_by, updated_at, updated_by, + // owner_id, owning_business_unit_id + ...injectedByRegistry(), SEARCH_COMPANION_FIELD, // '__search' — hidden search-normalization companion (search-companion.ts) SystemFieldName.ID, // 'id' — driver-provisioned primary key ]); @@ -51,29 +53,37 @@ describe('[#3058] PUBLIC_FORM_SERVER_MANAGED_FIELDS conformance', () => { // • tenant_id — legacy/enterprise tenant key (not injected by open-core). // • is_deleted / deleted_at — soft-delete state, written by the lifecycle/ // trash layer at runtime, never client-suppliable on a public form. - // • owning_business_unit_id — ADR-0117 D1's record-level BU ownership stamp. - // The NAME is reserved (SystemFieldName.OWNING_BUSINESS_UNIT_ID, #4611) but - // open-core does NOT inject it yet: `applySystemFields`' `wantOwner` is a - // DENY-list (only 'org'/'none' opt out), so the `ownership: 'business_unit'` - // tier cannot land until that flips to an allow-list — until then the enum - // value stays rejected by ObjectSchema on purpose. Denied here in advance - // because it is an ownership anchor of exactly the owner_id/organization_id - // forge class, and a denylist entry added only once the column ships is a - // hole with a release in it. MOVE THIS to the injected group (Group A) in - // the same PR that lands the injection — it is derived from live code - // there, so leaving it here would then fail as a stray entry. + // + // `owning_business_unit_id` USED TO LIVE HERE and has MOVED to Group A + // (#5677): ADR-0117 D1's injection landed, `applySystemFields` now provisions + // the column on the default (`ownership: undefined` / `'user'`) tier this + // group's probe object uses, so Group A derives it from live code. Leaving it + // listed here as well would now fail the partition test as a stray entry — + // which is exactly the signal #4611 wrote this migration note for. const reservedDefenseInDepth = new Set([ SystemFieldName.TENANT_ID, // 'tenant_id' 'is_deleted', SystemFieldName.DELETED_AT, // 'deleted_at' - SystemFieldName.OWNING_BUSINESS_UNIT_ID, // 'owning_business_unit_id' ]); it('registry injection actually produces the fields this test reasons about', () => { // Guards the test itself: if applySystemFields stops injecting these, the // conformance assertions below would pass vacuously. const injected = new Set(injectedByRegistry()); - for (const f of ['organization_id', 'created_at', 'created_by', 'updated_at', 'updated_by', 'owner_id']) { + for (const f of [ + 'organization_id', + 'created_at', + 'created_by', + 'updated_at', + 'updated_by', + 'owner_id', + // [ADR-0117 D1 / #5677] The migrated name. Listed explicitly so the move + // out of the reserved group is PINNED: without this, a regression that + // stopped injecting the column would silently make Group A shrink back + // and the partition test would then demand it be re-listed as reserved — + // a green suite describing the opposite of the accepted decision. + SystemFieldName.OWNING_BUSINESS_UNIT_ID, + ]) { expect(injected.has(f)).toBe(true); } });