From 872978b4c902ddff50e283b144d22642e2d3dd94 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 05:46:04 +0000 Subject: [PATCH] fix(plugin-sharing)!: a failed team-graph manager read refuses instead of answering no manager TeamGraphService.managerOf reads sys_user through readMembershipToCompletion as a lookup: a failed read throws and nothing is cached for it; a missing object still answers null. holderReadRefusal gains a lookup arm, used by BusinessUnitGraphService.headOf and the standalone managerOf too. The spec JSDoc of ITeamGraphService.managerOf and IBusinessUnitGraphService.headOf says so instead of best-effort. Claude-Session: https://claude.ai/code/session_01JQwxbXvyuTnWc7sv5i5DBw Co-authored-by: Claude --- .changeset/29792-team-manager-read-refuses.md | 25 ++++++ .../plugin-sharing/src/business-unit-graph.ts | 7 +- .../business-unit-head-manager-reads.test.ts | 86 ++++++++++++++++--- .../plugins/plugin-sharing/src/team-graph.ts | 54 ++++++++---- .../spec/src/contracts/sharing-service.ts | 10 ++- 5 files changed, 151 insertions(+), 31 deletions(-) create mode 100644 .changeset/29792-team-manager-read-refuses.md diff --git a/.changeset/29792-team-manager-read-refuses.md b/.changeset/29792-team-manager-read-refuses.md new file mode 100644 index 00000000000..42d3af61061 --- /dev/null +++ b/.changeset/29792-team-manager-read-refuses.md @@ -0,0 +1,25 @@ +--- +'@objectstack/plugin-sharing': patch +'@objectstack/spec': patch +--- + +fix(plugin-sharing)!: a failed team-graph manager read refuses instead of answering no manager + +Clause-②: no (narrowing) +Metadata-protocol: none + + + +**BREAKING** only where the read fails: `TeamGraphService.managerOf` used to answer `null` then, and now refuses. + +The holder reads of `TeamGraphService`, and the head and standalone manager lookups of `BusinessUnitGraphService`, already refuse on a failed read. This was the last lookup of that family that did not. + +- **`TeamGraphService.managerOf(userId, organizationId)`.** A failed `sys_user` read now throws. Before, it answered `null` (no manager) and kept that answer for the rest of the pass, so the manager stayed missing after the read recovered. Nothing is cached after a failed read now: the next call reads again. +- **Who sees it.** The `manager` arm of the exported `expandPrincipal`, and `BusinessUnitGraphService.managerOf` when it is built with a `teamGraph`, pass the refusal through instead of answering no manager. +- **A missing object is still nobody.** A stack that never registers, or never creates, `sys_user` keeps answering `null`. That is judged by `isObjectAbsentError`; any other error stays loud. +- **The organization screen is unchanged.** When `sys_member` cannot be read, the manager is still kept, as before. +- **The refusal names a lookup.** A failed head or manager lookup now says it is a lookup of one person, not a holder set or an expansion. +- **The contract says so.** The documentation of `ITeamGraphService.managerOf` and `IBusinessUnitGraphService.headOf` in `@objectstack/spec` now reads "refuses (throws) on a failed read; `null` when none" instead of "best-effort". No key or type changes. +- **No caller on this release changes behaviour.** No sharing rule, approval or ownership queue on this release calls `managerOf` or `expandPrincipal`. A caller of the exported class or function sees the refusal instead of `null`. + +**The remedy.** None is needed. A refusal means the user table could not be read: fix that read, and the next lookup succeeds. diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index ffdef39d42d..cd768f5de10 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -278,6 +278,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { where: this.orgScope({ id: businessUnitId }), fields: ['manager_user_id'], subject: `the head of business unit '${businessUnitId}'`, + kind: 'lookup', }); const row = rows?.[0]; const head = row?.manager_user_id ? String(row.manager_user_id) : null; @@ -302,8 +303,9 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { * `sys_user` has no manager on file and answers `null`. The fallback caches * nothing, before or after. The organization screen keeps its ruled * fail-open posture ({@link managerIsProvablyOutsideOrg}), and the - * delegating limb is {@link TeamGraphService.managerOf}'s own answer, - * unchanged here. + * delegating limb is {@link TeamGraphService.managerOf}'s own answer. + * [#29792] That answer now refuses on a failed read too, so both limbs + * agree on the failure as well as on the screen. */ async managerOf(userId: string, organizationId?: string): Promise { if (this.teamGraph) return this.teamGraph.managerOf(userId, organizationId); @@ -315,6 +317,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { where: { id: userId }, fields: ['manager_id'], subject: `the manager of user '${userId}'`, + kind: 'lookup', }); const row = rows?.[0]; const managerId = row?.manager_id ? String(row.manager_id) : null; diff --git a/packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts b/packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts index 3a4bbbc0cb1..0acaa23c304 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-head-manager-reads.test.ts @@ -11,6 +11,10 @@ * the read recovered. * 2. The standalone `managerOf` (no `TeamGraphService` wired) caught any * error from its `sys_user` read to `null`. It caches nothing. + * 3. [#29792] `TeamGraphService.managerOf` — the wired limb, and the + * `manager` arm of `expandPrincipal` — caught any error from its own + * `sys_user` read to `null` and cached that `null` under `org::userId` + * for the pass, so the manager stayed missing after the read recovered. * * Absence is the one failure that IS an answer — the object is not * registered, or its table was never created — judged by `isObjectAbsentError` @@ -26,7 +30,7 @@ import { ObjectQL } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; import { SysBusinessUnit, SysMember, SysOrganization, SysUser } from '@objectstack/platform-objects/identity'; import { BusinessUnitGraphService } from './business-unit-graph.js'; -import { TeamGraphService } from './team-graph.js'; +import { TeamGraphService, expandPrincipal } from './team-graph.js'; const ORG = 'org_head'; const UNIT = 'bu_sales'; @@ -39,7 +43,7 @@ interface Rig { engine: ObjectQL; driver: SqlDriver; /** Fail the next `times` reads of `object`; every other read runs as before. */ - failReads: (object: 'sys_business_unit' | 'sys_user', times?: number) => void; + failReads: (object: 'sys_business_unit' | 'sys_user' | 'sys_member', times?: number) => void; } const engines: ObjectQL[] = []; @@ -107,6 +111,12 @@ async function boot(opts: { omit?: string[] } = {}): Promise { const graph = (rig: Rig, extra: { teamGraph?: TeamGraphService } = {}) => new BusinessUnitGraphService({ engine: rig.engine as never, organizationId: ORG, ...extra }); +const team = (rig: Rig) => new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }); + +/** An answer or a refusal, as one comparable value. */ +const settled = (p: Promise) => + p.then((value) => ({ value }), (err: unknown) => ({ refused: err instanceof Error ? err.message : String(err) })); + describe('[#29713] headOf: a failed unit read refuses, and nothing is cached for it', () => { it('control — a healthy read answers the head, and a unit with no head answers null', async () => { const g = graph(await boot()); @@ -187,22 +197,76 @@ describe('[#29713] standalone managerOf: a failed user read refuses', () => { }); }); -describe('[#29713] CONTROL — the wired TeamGraphService limb is unchanged', () => { +describe('[#29713] CONTROL — the wired limb answers what TeamGraphService.managerOf answers', () => { it('a wired managerOf answers exactly what TeamGraphService.managerOf answers, on a healthy and on a failed read', async () => { const rig = await boot(); - const team = new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }); - const g = graph(rig, { teamGraph: team }); + const g = graph(rig, { teamGraph: team(rig) }); expect(await g.managerOf(ALICE, ORG)).toBe(BOB); expect(await g.managerOf(ALICE, ORG)).toBe(await new TeamGraphService({ engine: rig.engine as never }).managerOf(ALICE, ORG)); - // TeamGraphService's own failure posture, not this card's: it is passed - // through as it stands, and this card changes nothing on that limb. + // The wired limb passes TeamGraphService's answer through, its refusal + // included: the two outcomes are equal, whatever that outcome is. + rig.failReads('sys_user'); + const failed = graph(rig, { teamGraph: team(rig) }); + expect(await settled(failed.managerOf(ALICE, ORG))).toEqual(await settled(team(rig).managerOf(ALICE, ORG))); + }); +}); + +describe('[#29792] TeamGraphService.managerOf: a failed user read refuses, and nothing is cached for it', () => { + it('control — a healthy read answers the manager, and a user with none answers null', async () => { + const t = team(await boot()); + + expect(await t.managerOf(ALICE, ORG)).toBe(BOB); + expect(await t.managerOf(BOB, ORG)).toBeNull(); + }); + + it('⛔ a failed read refuses — it never answers an invented null', async () => { + const rig = await boot(); + rig.failReads('sys_user'); + + const refusal = team(rig).managerOf(ALICE, ORG); + await expect(refusal).rejects.toThrow(/\[sharing\] the manager of user 'usr_alice' could not be read: SQLITE_IOERR/); + await expect(refusal).rejects.toMatchObject({ cause: { code: 'SQLITE_IOERR' } }); + }); + + it('⛔ a failed read is not cached: the next read on the same pass answers the real manager', async () => { + const rig = await boot(); + rig.failReads('sys_user', 1); + const t = team(rig); + + await expect(t.managerOf(ALICE, ORG)).rejects.toThrow(/could not be read/); + expect(await t.managerOf(ALICE, ORG)).toBe(BOB); + }); + + it('⛔ expandPrincipal\'s manager arm refuses with it, rather than answering nobody', async () => { + const rig = await boot(); + const principal = { type: 'manager', value: 'owner_id', record: { owner_id: ALICE } }; + + expect(await expandPrincipal(principal, { team: team(rig), organizationId: ORG })).toEqual([BOB]); rig.failReads('sys_user'); - const failed = graph(rig, { teamGraph: new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }) }); - expect(await failed.managerOf(ALICE, ORG)).toBeNull(); - expect(await failed.managerOf(ALICE, ORG)).toBe( - await new TeamGraphService({ engine: rig.engine as never, organizationId: ORG }).managerOf(ALICE, ORG), + await expect(expandPrincipal(principal, { team: team(rig), organizationId: ORG })).rejects.toThrow( + /the manager of user 'usr_alice' could not be read/, ); }); + + it('absence — a composition that never registers sys_user has no manager on file: null', async () => { + const t = team(await boot({ omit: ['sys_user'] })); + + expect(await t.managerOf(ALICE, ORG)).toBeNull(); + }); + + it('absence — a stack whose sys_user table was never created has no manager on file: null', async () => { + const rig = await boot(); + await (rig.driver as any).knex.schema.dropTable('sys_user'); + + expect(await team(rig).managerOf(ALICE, ORG)).toBeNull(); + }); + + it('control — the organization screen stays fail-open: an unreadable sys_member keeps the manager', async () => { + const rig = await boot(); + rig.failReads('sys_member'); + + expect(await team(rig).managerOf(ALICE, ORG)).toBe(BOB); + }); }); diff --git a/packages/plugins/plugin-sharing/src/team-graph.ts b/packages/plugins/plugin-sharing/src/team-graph.ts index b8325c0938d..611685b093b 100644 --- a/packages/plugins/plugin-sharing/src/team-graph.ts +++ b/packages/plugins/plugin-sharing/src/team-graph.ts @@ -136,15 +136,28 @@ function roleHolderPredicate(roleName: string, org: string | null | undefined): */ const MEMBERSHIP_PAGE_ROWS = 10000; +/** + * What a refused read was for, which decides how its refusal words the + * consequence: a holder-set `expansion`, a user-scoped holder `check`, or a + * `lookup` of the one person a head or manager column names. + */ +type HolderReadKind = 'expansion' | 'check' | 'lookup'; + /** * [#29503] Why a holder read refused, as one sentence family: what could not * be read, the reason, and why the answer is a refusal rather than "nobody". * The driver's error rides along as `cause`. + * + * [#29792] A `lookup` reads one person (a unit's head, a user's manager), not + * a set of holders, so its refusal says so: answering "none" would drop that + * person from whatever the caller routes through the answer. */ -function holderReadRefusal(subject: string, reason: string, cause: unknown, kind: 'expansion' | 'check'): Error { +function holderReadRefusal(subject: string, reason: string, cause: unknown, kind: HolderReadKind): Error { const consequence = kind === 'expansion' ? 'A shortened or empty holder set would withhold or revoke the grants it confers, so the expansion refuses instead.' - : 'Answering "not a holder" would revoke the grants the holding confers, so the check refuses instead.'; + : kind === 'check' + ? 'Answering "not a holder" would revoke the grants the holding confers, so the check refuses instead.' + : 'Answering "none" would drop the person it names from everything routed through the answer, so the lookup refuses instead.'; return Object.assign( new Error(`[sharing] ${subject} could not be read: ${reason}. ${consequence}`), cause === undefined ? {} : { cause }, @@ -188,8 +201,11 @@ export async function readMembershipToCompletion( fields: readonly string[]; subject: string; pageRows?: number; - /** A user-scoped holder check words its refusal as a check, not an expansion. */ - kind?: 'expansion' | 'check'; + /** + * A user-scoped holder check words its refusal as a check, and a single + * head or manager lookup as a lookup; the default is an expansion. + */ + kind?: HolderReadKind; }, ): Promise[] | null> { const rows: Record[] = []; @@ -374,24 +390,30 @@ export class TeamGraphService implements ITeamGraphService { * * Resolution order matches `expandRoleUsers` exactly — the argument, else the * instance's `organizationId`, else no screen at all. + * + * [#29792] The `sys_user` read goes through {@link readMembershipToCompletion} + * as a `lookup`, the read `BusinessUnitGraphService`'s standalone fallback + * uses: a read that FAILS throws and nothing is cached for it, so the next + * caller on the same pass reads again. It caught any error to `null` ("no + * manager") and cached that `null` under `org::userId` for the pass, so a + * consumer naming "the owner's manager" dropped the manager on an outage and + * kept dropping it after the read recovered. A stack that never materialises + * `sys_user` has no manager on file and answers `null`. The organization + * screen keeps its ruled fail-open posture ({@link managerIsProvablyOutsideOrg}). */ async managerOf(userId: string, organizationId?: string): Promise { if (!userId) return null; const org = organizationId ?? this.organizationId; const key = `${org ?? '*'}::${userId}`; if (this.cache.manager!.has(key)) return this.cache.manager!.get(key) ?? null; - let row: any = null; - try { - const rows = await this.engine.find('sys_user', { - where: { id: userId }, - fields: ['id', 'manager_id'], - limit: 1, - context: SYSTEM_CTX, - }); - row = Array.isArray(rows) ? rows[0] : null; - } catch { - row = null; - } + const rows = await readMembershipToCompletion(this.engine, { + object: 'sys_user', + where: { id: userId }, + fields: ['manager_id'], + subject: `the manager of user '${userId}'`, + kind: 'lookup', + }); + const row = rows?.[0]; let mgr = row?.manager_id ? String(row.manager_id) : null; if (mgr && (await managerIsProvablyOutsideOrg(this.engine, mgr, org))) mgr = null; this.cache.manager!.set(key, mgr); diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index 40dea8f0338..4a3fc83fa83 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -664,7 +664,10 @@ export interface ITeamGraphService { expandUsers(teamId: string): Promise; /** Return all user ids that hold a role of `roleName` in `organizationId`. */ expandRoleUsers(roleName: string, organizationId?: string): Promise; - /** Return the manager id for a user (best-effort; null when none). */ + /** + * Return the manager id for a user: refuses (throws) on a failed read; + * `null` when none. + */ managerOf(userId: string, organizationId?: string): Promise; } @@ -683,7 +686,10 @@ export interface IBusinessUnitGraphService { descendants(businessUnitId: string): Promise; /** Return all user ids in `businessUnitId` or any descendant business unit. */ expandUsers(businessUnitId: string): Promise; - /** Return the department head (manager_user_id) — best-effort, null when none. */ + /** + * Return the department head (manager_user_id): refuses (throws) on a failed + * read; `null` when none. + */ headOf(businessUnitId: string): Promise; /** Return the manager id for a user — proxy to {@link ITeamGraphService.managerOf}. */ managerOf(userId: string, organizationId?: string): Promise;