fix(plugin-sharing)!: a failed team-graph manager read refuses instead of answering no manager

.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
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped and no metadata changes: the lookup reads the row it already read, and refuses a read that already failed. The spec change is interface documentation only. No author has anything to rewrite. -->
+
+**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.

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<string | null> {
     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;

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<Rig> {
 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<unknown>) =>
+  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);
+  });
 });

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<Record<string, unknown>[] | null> {
   const rows: Record<string, unknown>[] = [];
@@ -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<string | null> {
     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);

packages/spec/src/contracts/sharing-service.ts

@@ -664,7 +664,10 @@ export interface ITeamGraphService {
   expandUsers(teamId: string): Promise<string[]>;
   /** Return all user ids that hold a role of `roleName` in `organizationId`. */
   expandRoleUsers(roleName: string, organizationId?: string): Promise<string[]>;
-  /** 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<string | null>;
 }
 
@@ -683,7 +686,10 @@ export interface IBusinessUnitGraphService {
   descendants(businessUnitId: string): Promise<string[]>;
   /** Return all user ids in `businessUnitId` or any descendant business unit. */
   expandUsers(businessUnitId: string): Promise<string[]>;
-  /** 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<string | null>;
   /** Return the manager id for a user — proxy to {@link ITeamGraphService.managerOf}. */
   managerOf(userId: string, organizationId?: string): Promise<string | null>;