From 54624ffb620c29cb21edefea68b88e76321fcc5b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 01:08:17 +0000 Subject: [PATCH 1/3] fix(plugins)!: a failed business-unit hierarchy read refuses instead of answering nobody (WIP) Claude-Session: https://claude.ai/code/session_01JQwxbXcxCoeCAw1FyTWwwf Co-authored-by: Claude --- .../plugin-approvals/src/approval-service.ts | 100 ++++++-- ...nit-hierarchy-complete.integration.test.ts | 208 ++++++++++++++++ .../plugin-sharing/src/business-unit-graph.ts | 86 ++++--- ...ness-unit-hierarchy-reads-complete.test.ts | 232 ++++++++++++++++++ 4 files changed, 568 insertions(+), 58 deletions(-) create mode 100644 packages/plugins/plugin-approvals/src/business-unit-hierarchy-complete.integration.test.ts create mode 100644 packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 6d3a54231f3..4f51e376811 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -1127,6 +1127,25 @@ const MEMBER_SCREEN_READ_LIMIT = 50000; */ const HOLDER_PAGE_ROWS = 10000; +/** + * [#29655] Rows per page of the business-unit descent's child-unit read — the + * bound that read always carried, so a parent with fewer children issues the + * one read it always did, and one with more is read in further pages. + */ +const CHILD_UNIT_PAGE_ROWS = 1000; + +/** + * [#29362, #29655] Why a directory read refused: what could not be read, the + * driver's reason, and why the answer is a refusal rather than nobody. The + * driver's error rides along as `cause`. + */ +function holderReadRefusal(scope: string, err: unknown): Error { + return Object.assign(new Error( + `[approvals] ${scope} could not be read: ${err instanceof Error ? err.message : String(err)}. ` + + 'Routing to a shortened slate would leave holders unasked, so the expansion refuses instead.', + ), { cause: err }); +} + /** * Approver types resolved by QUERYING a graph rather than by taking `value` * literally (#3807). Each can legitimately come back empty — an unstaffed @@ -3214,25 +3233,37 @@ export class ApprovalService implements IApprovalService { */ private async expandBusinessUnitUsers(businessUnitId: string, organizationId?: string | null): Promise { if (!businessUnitId) return []; + const inOrg = organizationId ? ` in organization '${organizationId}'` : ''; // Seed sanity check: skip if dept doesn't exist or is inactive within tenant. + // [#29655] A failed seed read refuses — the slot then says the read failed + // rather than "expanded to nobody". A stack with no `sys_business_unit` + // has no departments. + let live: boolean; try { - if (!(await this.businessUnitIsLive(businessUnitId, organizationId))) return []; - } catch { return []; } + live = await this.businessUnitIsLive(businessUnitId, organizationId); + } catch (err) { + if (isObjectAbsentError(err, 'sys_business_unit')) return []; + throw holderReadRefusal(`business unit '${businessUnitId}'${inOrg}`, err); + } + if (!live) return []; const seen = new Set([businessUnitId]); const queue: string[] = [businessUnitId]; while (queue.length) { const parent = queue.shift()!; - let kids: any[] = []; - try { - const filter = this.businessUnitOrgScope( - { parent_business_unit_id: parent, active: { $ne: false } }, - organizationId, - ); - kids = await this.engine.find('sys_business_unit', { filter, fields: ['id'], limit: 1000, context: SYSTEM_CTX } as any); - } catch { kids = []; } + // [#29655] Every active child unit, read to completion — or a refusal. + // It read once at a 1,000-row bound and answered `[]` on any failure: + // a child past the bound, or every child of a parent whose read failed, + // left the slate with its members and no signal. + const kids = await this.readRowsToCompletion( + 'sys_business_unit', + this.businessUnitOrgScope({ parent_business_unit_id: parent, active: { $ne: false } }, organizationId), + [], + `the child units of business unit '${parent}'${inOrg}`, + CHILD_UNIT_PAGE_ROWS, + ); for (const k of kids ?? []) { - const kid = String((k as any).id ?? ''); + const kid = String(k?.id ?? ''); if (kid && !seen.has(kid)) { seen.add(kid); queue.push(kid); } } } @@ -3249,8 +3280,7 @@ export class ApprovalService implements IApprovalService { // proved their tenancy; these rows have not, and the // shared seeded unit id is exactly where other tenants' rows sit. this.businessUnitMemberScope({ business_unit_id: { $in: Array.from(seen) } }, organizationId), - `the members of business unit '${businessUnitId}' and its descendant units` - + `${organizationId ? ` in organization '${organizationId}'` : ''}`, + `the members of business unit '${businessUnitId}' and its descendant units${inOrg}`, ); } @@ -3488,24 +3518,40 @@ export class ApprovalService implements IApprovalService { * attributed to `object`, so an absence of another object is still loud. */ private async readHoldersToCompletion(object: string, where: Record, scope: string): Promise { + const rows = await this.readRowsToCompletion(object, where, ['user_id'], scope, HOLDER_PAGE_ROWS); const users = new Set(); + for (const row of rows ?? []) { + const uid = String(row?.user_id ?? ''); + if (uid) users.add(uid); + } + return Array.from(users); + } + + /** + * [#29362, #29503, #29655] Every `object` row matching `where` (its `id` + * plus `fields`), read to COMPLETION through the platform's one keyset walk + * — or a refusal; `null` when `object` does not exist on this stack, so each + * caller says what absence means for it. The one walk behind + * {@link readHoldersToCompletion} and the business-unit descent of + * {@link expandBusinessUnitUsers}. + */ + private async readRowsToCompletion( + object: string, + where: Record, + fields: readonly string[], + scope: string, + pageSize: number, + ): Promise[] | null> { + const rows: Record[] = []; const walk = keysetWalk>( - (page) => this.engine.find(object, { ...page, fields: ['id', 'user_id'], context: SYSTEM_CTX }), - { where, pageSize: HOLDER_PAGE_ROWS }, + (page) => this.engine.find(object, { ...page, fields: ['id', ...fields], context: SYSTEM_CTX }), + { where, pageSize }, ); try { - for await (const page of walk.pages()) { - for (const row of page) { - const uid = String(row?.user_id ?? ''); - if (uid) users.add(uid); - } - } + for await (const page of walk.pages()) rows.push(...page); } catch (err) { - if (isObjectAbsentError(err, object)) return []; - throw Object.assign(new Error( - `[approvals] ${scope} could not be read: ${err instanceof Error ? err.message : String(err)}. ` - + 'Routing to a shortened slate would leave holders unasked, so the expansion refuses instead.', - ), { cause: err }); + if (isObjectAbsentError(err, object)) return null; + throw holderReadRefusal(scope, err); } if (walk.truncated) { throw new Error( @@ -3514,7 +3560,7 @@ export class ApprovalService implements IApprovalService { + 'holders unasked, so the expansion refuses instead.', ); } - return Array.from(users); + return rows; } /** diff --git a/packages/plugins/plugin-approvals/src/business-unit-hierarchy-complete.integration.test.ts b/packages/plugins/plugin-approvals/src/business-unit-hierarchy-complete.integration.test.ts new file mode 100644 index 00000000000..0946f3598c5 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/business-unit-hierarchy-complete.integration.test.ts @@ -0,0 +1,208 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#29655] The approver business-unit expansion's `sys_business_unit` reads — + * the seed check and the child-unit descent — answer EVERY unit, or refuse. + * #29606 closed the member read; this closes the unit reads it is keyed by. + * + * The seed check answered `[]` on a read that threw, and the descent read + * each parent's child units once at a 1,000-row bound and answered `[]` on any + * failure: a failed seed read logged "expanded to nobody", indistinguishable + * from an unstaffed unit, and a failed or cut descent routed to a shrunk slate + * with no signal at all. + * + * What a caller sees now: the expansion throws, the approver-spec resolver's + * directory catch takes it, and the slot falls back to its literal with the + * warning that names the failed read and carries its `error`. A unit object + * that does not exist is still no members. + * + * Measured on a REAL engine (ObjectQL + SqlDriver over in-memory SQLite) + * carrying the shipped `SysBusinessUnit` and `SysBusinessUnitMember`. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SysBusinessUnit, SysBusinessUnitMember, SysMember, SysOrganization } from '@objectstack/platform-objects/identity'; +import { ApprovalService } from './approval-service.js'; + +const ORG = 'org_tree'; +const ROOT = 'bu_root'; +const CHILD = 'bu_child'; +/** The bound each child-unit read is paged at. */ +const PAGE = 1000; + +const pad = (n: number) => String(n).padStart(5, '0'); + +/** Which `sys_business_unit` read a fault targets. */ +type UnitRead = 'seed' | 'descent'; + +interface Rig { + engine: ObjectQL; + driver: SqlDriver; + svc: ApprovalService; + warn: ReturnType; + /** Every member of the tree, sorted. */ + members: string[]; + /** The child-unit reads the driver ran for ROOT's children. */ + rootChildReads: () => number; + /** Fail every read of one kind; every other read runs as before. */ + failUnitReads: (which: UnitRead) => void; +} + +const engines: ObjectQL[] = []; +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +/** + * ROOT, its child CHILD, and `extra` more child units of ROOT, each with one + * member. Unit ids are scrambled against creation order. + */ +async function boot(extra: number, opts: { omit?: string[] } = {}): Promise { + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never); + const engine = new ObjectQL(); + engine.registerDriver(driver as never, true); + await engine.init(); + const omit = new Set(opts.omit ?? []); + engine.registerApp({ + id: 'com.objectstack.qa.business-unit-hierarchy-complete', + name: 'business unit hierarchy complete', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [SysOrganization, SysMember, SysBusinessUnit, SysBusinessUnitMember].filter((o) => !omit.has(o.name)), + } as never); + await engine.syncSchemas(); + engines.push(engine); + + const e = engine as any; + await e.insert('sys_organization', { id: ORG, name: ORG, slug: 'tree' }, { context: { isSystem: true } }); + const kids = Array.from({ length: extra }, (_, i) => `bu_kid_${pad((i * 7919) % 100003)}`); + const units = [ROOT, CHILD, ...kids]; + if (!omit.has('sys_business_unit')) { + const rows: Record[] = [ + { id: ROOT, name: 'Root', kind: 'division', active: true, organization_id: ORG }, + { id: CHILD, name: 'Child', kind: 'department', active: true, parent_business_unit_id: ROOT, organization_id: ORG }, + ...kids.map((id) => ({ id, name: id, kind: 'department', active: true, parent_business_unit_id: ROOT, organization_id: ORG })), + ]; + const d = e.getDriverForObject('sys_business_unit'); + for (let i = 0; i < rows.length; i += 250) await d.bulkCreate('sys_business_unit', rows.slice(i, i + 250)); + } + const members = units.map((u) => `usr_of_${u}`); + const memberRows = units.map((u, i) => ({ + id: `bum_${pad(i)}`, user_id: `usr_of_${u}`, business_unit_id: u, organization_id: ORG, + })); + const md = e.getDriverForObject('sys_business_unit_member'); + for (let i = 0; i < memberRows.length; i += 250) await md.bulkCreate('sys_business_unit_member', memberRows.slice(i, i + 250)); + + let rootChildReads = 0; + (driver as any).knex.on('query', (q: { sql: string; bindings?: unknown[] }) => { + if (/^select .* from [`"]sys_business_unit[`"]/i.test(q.sql) && /parent_business_unit_id/.test(q.sql) + && (q.bindings ?? []).includes(ROOT)) rootChildReads += 1; + }); + const failUnitReads = (which: UnitRead) => { + const d = e.getDriverForObject('sys_business_unit'); + const real = d.find.bind(d); + vi.spyOn(d, 'find').mockImplementation(((obj: string, ...rest: unknown[]) => { + const descent = JSON.stringify(rest).includes('parent_business_unit_id'); + if (obj === 'sys_business_unit' && (which === 'descent') === descent) { + return Promise.reject(Object.assign(new Error('SQLITE_IOERR: disk I/O error'), { code: 'SQLITE_IOERR' })); + } + return real(obj, ...rest); + }) as never); + }; + const warn = vi.fn(); + const svc = new ApprovalService({ + engine: engine as any, + logger: { warn, info: () => {}, error: () => {}, debug: () => {} } as any, + }); + return { engine, driver, svc, warn, members: [...members].sort(), rootChildReads: () => rootChildReads, failUnitReads }; +} + +/** The private expansion and the resolver, as the request opener calls them. */ +const department = (rig: Rig, value = ROOT) => + (rig.svc as any).expandBusinessUnitUsers(value, ORG) as Promise; +const resolveSpec = (rig: Rig, a: { type: string; value: string }) => + (rig.svc as any).resolveApproverSpec(a, undefined, ORG, Date.now()) as Promise; +const failureWarnings = (rig: Rig) => rig.warn.mock.calls.filter(([, meta]) => typeof meta?.error === 'string'); + +describe('[#29655] approvals business-unit expansion: a failed seed read refuses', () => { + it('⛔ the expansion refuses — it never answers an invented empty slate', async () => { + const rig = await boot(0); + rig.failUnitReads('seed'); + + await expect(department(rig)).rejects.toThrow( + /\[approvals\] business unit 'bu_root' in organization 'org_tree' could not be read: SQLITE_IOERR/, + ); + }); + + it('⛔ what the caller sees: the department slot falls back to its literal, and the warning names the failed read', async () => { + const rig = await boot(0); + rig.failUnitReads('seed'); + + expect(await resolveSpec(rig, { type: 'department', value: ROOT })).toEqual([`department:${ROOT}`]); + const failed = failureWarnings(rig); + expect(failed.map(([, meta]) => meta.type)).toEqual(['department']); + expect(failed[0][0]).toMatch(/could not be expanded — a directory or read-filter lookup failed/); + expect(failed[0][1].error).toMatch(/SQLITE_IOERR/); + }); +}); + +describe('[#29655] approvals business-unit expansion: a failed descent read refuses', () => { + it('⛔ the expansion refuses — the slate never shrinks to the seed unit', async () => { + const rig = await boot(0); + rig.failUnitReads('descent'); + + await expect(department(rig)).rejects.toThrow( + /\[approvals\] the child units of business unit 'bu_root' in organization 'org_tree' could not be read: SQLITE_IOERR/, + ); + }); + + it('⛔ what the caller sees: the literal and the failed-read warning, not a shrunk slate', async () => { + const rig = await boot(0); + rig.failUnitReads('descent'); + + expect(await resolveSpec(rig, { type: 'department', value: ROOT })).toEqual([`department:${ROOT}`]); + expect(failureWarnings(rig).map(([, meta]) => meta.type)).toEqual(['department']); + }); +}); + +describe('[#29655] approvals business-unit expansion reads every child unit past the 1,000-row bound', () => { + it('⭐ 1,001 child units: every member of every child is on the slate', async () => { + // CHILD plus 1,000 more: 1,001 children of ROOT. + const rig = await boot(PAGE); + + expect([...(await department(rig))].sort()).toEqual(rig.members); + expect(rig.members).toHaveLength(PAGE + 2); + expect(rig.rootChildReads()).toBe(2); + }, 60_000); + + it('control — under the bound: one child-unit read, the same slate as before', async () => { + // CHILD plus 998 more: 999 children of ROOT. + const rig = await boot(PAGE - 2); + + expect([...(await department(rig))].sort()).toEqual(rig.members); + expect(rig.rootChildReads()).toBe(1); + }, 60_000); +}); + +describe('[#29655] approvals business-unit expansion: absence still answers nobody', () => { + it('a composition that never registers sys_business_unit has no departments: the literal, with no failure warning', async () => { + const rig = await boot(0, { omit: ['sys_business_unit'] }); + + expect(await department(rig)).toEqual([]); + expect(await resolveSpec(rig, { type: 'department', value: ROOT })).toEqual([`department:${ROOT}`]); + expect(failureWarnings(rig)).toEqual([]); + }); + + it('a stack whose sys_business_unit table was never created has no departments', async () => { + const rig = await boot(0); + await (rig.driver as any).knex.schema.dropTable('sys_business_unit'); + + expect(await department(rig)).toEqual([]); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index ddbfccfdd9f..a27bf6e27fa 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -6,6 +6,13 @@ import { TeamGraphService, managerIsProvablyOutsideOrg, readMembershipToCompleti const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; +/** + * [#29655] Rows per page of a child-unit read — the bound the read always + * carried, so a parent with fewer children issues the one read it always did, + * and one with more is read in further pages rather than cut. + */ +const CHILD_UNIT_PAGE_ROWS = 1000; + type DeptCache = { descendants?: Map; expandUsers?: Map; @@ -86,7 +93,8 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { const cached = this.cache.descendants!.get(businessUnitId); if (cached) return cached; - // Verify seed itself is active + within tenant scope. + // Verify seed itself is active + within tenant scope. [#29655] A failed + // seed read throws, so nothing is cached for it. const seedActive = await this.seedIsUsable(businessUnitId); if (!seedActive) { this.cache.descendants!.set(businessUnitId, []); @@ -97,20 +105,8 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { const queue: string[] = [businessUnitId]; while (queue.length) { const parent = queue.shift()!; - let children: any[] = []; - try { - children = await this.engine.find('sys_business_unit', { - where: this.orgScope({ parent_business_unit_id: parent, active: { $ne: false } }), - fields: ['id'], - limit: 1000, - context: SYSTEM_CTX, - }); - } catch { - children = []; - } - for (const c of children ?? []) { - const cid = String((c as any).id ?? ''); - if (cid && !seen.has(cid)) { + for (const cid of await this.readChildUnits(parent)) { + if (!seen.has(cid)) { seen.add(cid); queue.push(cid); } @@ -121,30 +117,58 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { return out; } + /** + * [#29655] The ids of every active child unit of `parent`, read to + * completion through the one walk the member reads share + * ({@link readMembershipToCompletion}) — or a refusal. + * + * It read once at a 1,000-row bound and answered `[]` on any failure, and + * the subtree feeds the `unit_and_subordinates` member read: a child past + * the bound, or every child of a parent whose read failed, dropped out of + * the subtree with its members, and the reconcilers revoked their grants. A + * failed read, or a walk that cannot vouch for completion, now throws before + * {@link descendants} caches anything; a stack that never materialises + * `sys_business_unit` has no child units. + */ + private async readChildUnits(parent: string): Promise { + const rows = await readMembershipToCompletion(this.engine, { + object: 'sys_business_unit', + where: this.orgScope({ parent_business_unit_id: parent, active: { $ne: false } }), + fields: [], + subject: `the child units of business unit '${parent}'`, + pageRows: CHILD_UNIT_PAGE_ROWS, + }); + if (!rows) return []; + return rows.map((r) => String(r?.id ?? '')).filter(Boolean); + } + /** * Is the seed unit itself active and inside the tenant scope? * * Shared by both widths so they agree on what an unusable seed is: a unit * that does not exist, sits in another organization, or carries * `active: false` contributes NOBODY — it is never merely "expanded without - * its descendants". A read failure answers `false` (fail closed: an - * unreadable unit must not grant). + * its descendants". + * + * [#29655] A read that FAILS is not an unusable seed: it throws, so neither + * width caches an answer for it. It answered `false` ("fail closed: an + * unreadable unit must not grant"), but that `false` was cached as nobody + * for the pass, and a business-unit graph write's recompute read it as + * "this rule now reaches nobody" and revoked every grant the rule gave. A + * refusal grants nothing either, and revokes nothing. A stack that never + * materialises `sys_business_unit` has no usable seed and answers `false`. */ private async seedIsUsable(businessUnitId: string): Promise { - try { - const seedRows = await this.engine.find('sys_business_unit', { - where: this.orgScope({ id: businessUnitId }), - fields: ['id', 'active'], - limit: 1, - context: SYSTEM_CTX, - }); - const seedRow: any = Array.isArray(seedRows) ? seedRows[0] : null; - if (!seedRow) return false; - if (seedRow.active === false) return false; - return true; - } catch { - return false; - } + const seedRows = await readMembershipToCompletion(this.engine, { + object: 'sys_business_unit', + where: this.orgScope({ id: businessUnitId }), + fields: ['active'], + subject: `business unit '${businessUnitId}'`, + }); + const seedRow = seedRows?.[0]; + if (!seedRow) return false; + if (seedRow.active === false) return false; + return true; } /** diff --git a/packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts b/packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts new file mode 100644 index 00000000000..3a11d17c84b --- /dev/null +++ b/packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts @@ -0,0 +1,232 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#29655] The `sys_business_unit` hierarchy reads of + * `BusinessUnitGraphService` answer EVERY unit, or refuse — never a shortened + * or an invented empty subtree. #29606 closed the member table; this closes + * the unit table those member reads are keyed by. + * + * 1. The seed check (`seedIsUsable`) answered `false` on a read that threw, + * and both widths cached that `[]` for the pass — so a business-unit + * graph write's recompute read "this rule reaches nobody" and revoked + * every grant the rule gave. + * 2. The descent (`descendants`) read each parent's child units once at a + * 1,000-row bound and answered `[]` on any failure: a child past the + * bound, or every child of a parent whose read failed, left the subtree + * with its members. + * + * Absence is the one failure that IS an answer — the object is not + * registered, or its table was never created — checked by + * `isObjectAbsentError` against `sys_business_unit`. + * + * Measured on a REAL engine (ObjectQL + SqlDriver over in-memory SQLite) + * carrying the shipped `SysBusinessUnit` and `SysBusinessUnitMember`, because + * completion is a keyset read (`id > cursor`, ordered by `id`) that a double + * ignoring the cursor cannot vouch for. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SysBusinessUnit, SysBusinessUnitMember, SysOrganization } from '@objectstack/platform-objects/identity'; +import { BusinessUnitGraphService } from './business-unit-graph.js'; + +const ORG = 'org_tree'; +const ROOT = 'bu_root'; +const CHILD = 'bu_child'; +/** The bound each child-unit read is paged at. */ +const PAGE = 1000; + +const pad = (n: number) => String(n).padStart(5, '0'); + +/** Which `sys_business_unit` read a fault targets. */ +type UnitRead = 'seed' | 'descent'; + +interface Rig { + engine: ObjectQL; + driver: SqlDriver; + /** Every unit of the tree under (and including) ROOT, sorted. */ + units: string[]; + /** Every member of the tree, sorted. */ + members: string[]; + /** The child-unit reads the driver ran for ROOT's children. */ + rootChildReads: () => number; + /** Fail the next `times` reads of one kind; every other read runs as before. */ + failUnitReads: (which: UnitRead, times?: number) => void; +} + +const engines: ObjectQL[] = []; +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +/** + * ROOT, its child CHILD, and `extra` more child units of ROOT, each with one + * member. Unit ids are scrambled against creation order, so the 1,001st row by + * id is not the 1,001st unit created. + */ +async function boot(extra: number, opts: { omit?: string[] } = {}): Promise { + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never); + const engine = new ObjectQL(); + engine.registerDriver(driver as never, true); + await engine.init(); + const omit = new Set(opts.omit ?? []); + engine.registerApp({ + id: 'com.objectstack.qa.business-unit-hierarchy-reads-complete', + name: 'business unit hierarchy reads complete', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [SysOrganization, SysBusinessUnit, SysBusinessUnitMember].filter((o) => !omit.has(o.name)), + } as never); + await engine.syncSchemas(); + engines.push(engine); + + const e = engine as any; + await e.insert('sys_organization', { id: ORG, name: ORG, slug: 'tree' }, { context: { isSystem: true } }); + const kids = Array.from({ length: extra }, (_, i) => `bu_kid_${pad((i * 7919) % 100003)}`); + const units = [ROOT, CHILD, ...kids]; + if (!omit.has('sys_business_unit')) { + const rows: Record[] = [ + { id: ROOT, name: 'Root', kind: 'division', active: true, organization_id: ORG }, + { id: CHILD, name: 'Child', kind: 'department', active: true, parent_business_unit_id: ROOT, organization_id: ORG }, + ...kids.map((id) => ({ id, name: id, kind: 'department', active: true, parent_business_unit_id: ROOT, organization_id: ORG })), + ]; + const d = e.getDriverForObject('sys_business_unit'); + for (let i = 0; i < rows.length; i += 250) await d.bulkCreate('sys_business_unit', rows.slice(i, i + 250)); + } + const members = units.map((u) => `usr_of_${u}`); + if (!omit.has('sys_business_unit_member')) { + const rows = units.map((u, i) => ({ + id: `bum_${pad(i)}`, user_id: `usr_of_${u}`, business_unit_id: u, organization_id: ORG, + })); + const d = e.getDriverForObject('sys_business_unit_member'); + for (let i = 0; i < rows.length; i += 250) await d.bulkCreate('sys_business_unit_member', rows.slice(i, i + 250)); + } + + let rootChildReads = 0; + (driver as any).knex.on('query', (q: { sql: string; bindings?: unknown[] }) => { + if (/^select .* from [`"]sys_business_unit[`"]/i.test(q.sql) && /parent_business_unit_id/.test(q.sql) + && (q.bindings ?? []).includes(ROOT)) rootChildReads += 1; + }); + const failUnitReads = (which: UnitRead, times = Number.POSITIVE_INFINITY) => { + const d = e.getDriverForObject('sys_business_unit'); + const real = d.find.bind(d); + let left = times; + vi.spyOn(d, 'find').mockImplementation(((obj: string, ...rest: unknown[]) => { + const descent = JSON.stringify(rest).includes('parent_business_unit_id'); + if (obj === 'sys_business_unit' && (which === 'descent') === descent && left > 0) { + left -= 1; + return Promise.reject(Object.assign(new Error('SQLITE_IOERR: disk I/O error'), { code: 'SQLITE_IOERR' })); + } + return real(obj, ...rest); + }) as never); + }; + return { + engine, driver, units: [...units].sort(), members: [...members].sort(), + rootChildReads: () => rootChildReads, failUnitReads, + }; +} + +const graph = (rig: Rig) => new BusinessUnitGraphService({ engine: rig.engine as never, organizationId: ORG }); + +describe('[#29655] a failed seed read refuses, and nothing is cached for it', () => { + it('⛔ both widths and the descent refuse — none answers an invented empty set', async () => { + const rig = await boot(0); + rig.failUnitReads('seed'); + const g = graph(rig); + + await expect(g.expandUnitMembers(ROOT)).rejects.toThrow(/\[sharing\] business unit 'bu_root' could not be read: SQLITE_IOERR/); + await expect(g.expandUsers(ROOT)).rejects.toThrow(/\[sharing\] business unit 'bu_root' could not be read: SQLITE_IOERR/); + await expect(g.descendants(ROOT)).rejects.toThrow(/business unit 'bu_root' could not be read/); + }); + + it('⛔ a failed seed read is not cached: the next read on the same pass answers the real set', async () => { + const rig = await boot(0); + rig.failUnitReads('seed', 1); + const g = graph(rig); + + await expect(g.expandUnitMembers(ROOT)).rejects.toThrow(/could not be read/); + expect(await g.expandUnitMembers(ROOT)).toEqual([`usr_of_${ROOT}`]); + + rig.failUnitReads('seed', 1); + await expect(g.expandUsers(ROOT)).rejects.toThrow(/could not be read/); + expect([...(await g.expandUsers(ROOT))].sort()).toEqual(rig.members); + }); +}); + +describe('[#29655] a failed descent read refuses, and nothing is cached for it', () => { + it('⛔ the subtree width and the descent refuse — the subtree never shrinks to the seed', async () => { + const rig = await boot(0); + rig.failUnitReads('descent'); + const g = graph(rig); + + await expect(g.descendants(ROOT)).rejects.toThrow( + /\[sharing\] the child units of business unit 'bu_root' could not be read: SQLITE_IOERR/, + ); + await expect(g.expandUsers(ROOT)).rejects.toThrow(/the child units of business unit 'bu_root' could not be read/); + }); + + it('control — the single-unit width reads no child units, so a failed descent does not touch it', async () => { + const rig = await boot(0); + rig.failUnitReads('descent'); + + expect(await graph(rig).expandUnitMembers(ROOT)).toEqual([`usr_of_${ROOT}`]); + }); + + it('⛔ a failed descent read is not cached: the next read on the same pass answers the whole subtree', async () => { + const rig = await boot(0); + rig.failUnitReads('descent', 1); + const g = graph(rig); + + await expect(g.descendants(ROOT)).rejects.toThrow(/could not be read/); + expect([...(await g.descendants(ROOT))].sort()).toEqual(rig.units); + expect([...(await g.expandUsers(ROOT))].sort()).toEqual(rig.members); + }); +}); + +describe('[#29655] the descent reads every child unit past the 1,000-row bound', () => { + it('⭐ 1,001 child units: every unit is in the subtree and every member is in the set', async () => { + // CHILD plus 1,000 more: 1,001 children of ROOT. + const rig = await boot(PAGE); + const g = graph(rig); + + expect([...(await g.descendants(ROOT))].sort()).toEqual(rig.units); + expect([...(await g.expandUsers(ROOT))].sort()).toEqual(rig.members); + expect(rig.units).toHaveLength(PAGE + 2); + expect(rig.rootChildReads()).toBe(2); + }, 60_000); + + it('control — under the bound: one child-unit read, the same subtree as before', async () => { + // CHILD plus 998 more: 999 children of ROOT. + const rig = await boot(PAGE - 2); + const g = graph(rig); + + expect([...(await g.descendants(ROOT))].sort()).toEqual(rig.units); + expect(rig.rootChildReads()).toBe(1); + }, 60_000); +}); + +describe('[#29655] absence still answers nobody', () => { + it('a composition that never registers sys_business_unit has no units: [] as before', async () => { + const rig = await boot(0, { omit: ['sys_business_unit'] }); + const g = graph(rig); + + expect(await g.descendants(ROOT)).toEqual([]); + expect(await g.expandUnitMembers(ROOT)).toEqual([]); + expect(await g.expandUsers(ROOT)).toEqual([]); + }); + + it('a stack whose sys_business_unit table was never created has no units: [] as before', async () => { + const rig = await boot(0); + await (rig.driver as any).knex.schema.dropTable('sys_business_unit'); + const g = graph(rig); + + expect(await g.descendants(ROOT)).toEqual([]); + expect(await g.expandUnitMembers(ROOT)).toEqual([]); + expect(await g.expandUsers(ROOT)).toEqual([]); + }); +}); From 6ad6774bce692020420350a66367ad3215f5e009 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 01:09:12 +0000 Subject: [PATCH 2/3] test(plugins): fix the second fault wrapping the first spy (WIP) Claude-Session: https://claude.ai/code/session_01JQwxbXcxCoeCAw1FyTWwwf Co-authored-by: Claude --- .../src/business-unit-hierarchy-reads-complete.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts b/packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts index 3a11d17c84b..cac2e35c44a 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-hierarchy-reads-complete.test.ts @@ -112,9 +112,11 @@ async function boot(extra: number, opts: { omit?: string[] } = {}): Promise if (/^select .* from [`"]sys_business_unit[`"]/i.test(q.sql) && /parent_business_unit_id/.test(q.sql) && (q.bindings ?? []).includes(ROOT)) rootChildReads += 1; }); + // The driver's own `find`, taken once: a second fault wraps it again rather + // than wrapping the first fault's spy. + const d = e.getDriverForObject('sys_business_unit'); + const real = d.find.bind(d); const failUnitReads = (which: UnitRead, times = Number.POSITIVE_INFINITY) => { - const d = e.getDriverForObject('sys_business_unit'); - const real = d.find.bind(d); let left = times; vi.spyOn(d, 'find').mockImplementation(((obj: string, ...rest: unknown[]) => { const descent = JSON.stringify(rest).includes('parent_business_unit_id'); From 483976d87fcab886af74a7c8d7d5966ead76d63f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 01:13:52 +0000 Subject: [PATCH 3/3] docs(plugins): changeset and approvals docs for the business-unit hierarchy reads (WIP) Claude-Session: https://claude.ai/code/session_01JQwxbXcxCoeCAw1FyTWwwf Co-authored-by: Claude --- ...55-business-unit-hierarchy-reads-refuse.md | 25 +++++++++++++++++++ content/docs/automation/approvals.mdx | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 .changeset/29655-business-unit-hierarchy-reads-refuse.md diff --git a/.changeset/29655-business-unit-hierarchy-reads-refuse.md b/.changeset/29655-business-unit-hierarchy-reads-refuse.md new file mode 100644 index 00000000000..6c33c384fc0 --- /dev/null +++ b/.changeset/29655-business-unit-hierarchy-reads-refuse.md @@ -0,0 +1,25 @@ +--- +'@objectstack/plugin-sharing': patch +'@objectstack/plugin-approvals': patch +--- + +fix(plugins)!: a failed `sys_business_unit` read refuses instead of answering nobody, and a business unit with more than 1,000 child units keeps every child + +Clause-②: no (narrowing) +Metadata-protocol: none + + + +**BREAKING** only where a `sys_business_unit` read fails: the business-unit expansions used to answer nobody (an empty list) then, and now refuse. + +The member reads of a business unit (`sys_business_unit_member`) read to completion and refuse on failure. The reads of the unit table they are keyed by did not: the check that the unit exists and is active, and the read of each unit's child units, still turned any read error into nobody, and the child-unit read stopped at 1,000 rows. + +- **A failed unit read refuses.** In plugin-sharing, `BusinessUnitGraphService.descendants`, `expandUnitMembers` (every `business_unit` sharing recipient) and `expandUsers` (every `unit_and_subordinates` recipient) now throw when the unit's own row or a child-unit read fails. In plugin-approvals, the `department` / `business_unit` / `bu` approver and an `expression` approver with `resolveAs: 'department'` do the same. + - Sharing: the expansion rejects, so that rule's reconcile or revoke pass fails, its callers log their existing per-rule warning, and the grants it already holds stay. Before, a failed unit read answered `[]` and that answer was kept for the rest of the pass, so the recompute a business-unit graph write triggers revoked every grant the rule gave. + - Approvals: the approver slot falls back to its literal (`department:ID`) and logs `could not be expanded — a directory or read-filter lookup failed` with its `error`. Before, a failed seed read logged "expanded to nobody", and a failed child-unit read routed to the units that did read, with no signal. + - Ownership queues: a `business_unit` or `unit_and_subordinates` queue specification whose unit read fails contributes nobody, the posture the queue declares for a failed expansion, and logs it once. +- **Nothing is cached after a failed read.** The next expansion on the same pass reads again. +- **Every child unit.** The child units of each unit are read in pages of 1,000 ordered by id until they are exhausted. A unit with more than 1,000 active child units used to lose the children past the 1,000th row, with all their members and descendants. A unit with fewer issues one read, as before. +- **A missing object is still nobody.** A stack that never registers, or never creates, `sys_business_unit` keeps answering no members. That is judged by `isObjectAbsentError(error, 'sys_business_unit')`; any other error stays loud. + +**The remedy.** None is needed. A refusal means the business-unit table could not be read: fix that read, and the next expansion succeeds. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index c9460f9669c..41bd0f8ab8f 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -458,7 +458,8 @@ What an entry that resolves to nobody leaves on the slate depends on its kind: `behavior` that needs every slot (`unanimous`) waits on it too. A `team`, `position`, `department` or `org_membership_level` entry whose holder read failed (`sys_team_member`, `sys_user_position`, `sys_business_unit_member` - or `sys_member`) keeps its literal the same way. It logs `could not be + or `sys_member`, and for a `department` also the `sys_business_unit` read of + the unit and of its child units) keeps its literal the same way. It logs `could not be expanded — a directory or read-filter lookup failed` with the `error`, instead of `expanded to nobody`, and never routes to the holders that happened to read. An `expression` value whose `resolveAs` read failed keeps