fix(plugin-gateway): keep the fetch calls that stay right on a cache miss

.changeset/gateway-keep-hierarchy-fetches.md

@@ -0,0 +1,5 @@
+---
+"@wolfstar/plugin-gateway": patch
+---
+
+Keep the single-call checks that stay right on a cache miss: `GuildMemberRoleManager#fetchHighest`, `GuildMember#fetchPermissions` and `GuildMember#fetchPermissionsIn` are no longer `@deprecated`, reversing part of the deprecation of the `fetch*` twins. The getters they mirror (`roles.highest`, `permissions`, `permissionsIn`, `manageable`, `kickable`, `bannable`, `moderatable`) skip a role the cache lacks without an error, so a hierarchy or permission check can under-report with a filtered cache or a `plugin-broker` worker. Their docs and the README now say so and point at the `fetch*` call or `roles.fetch()`.

packages/plugin-gateway/README.md

@@ -804,10 +804,23 @@ discord.js's derived getters are there too: `member.permissions`, `permissionsIn
 With an entity they need missing from the cache they throw `GuildUncached`, `GuildUncachedMe`,
 `ChannelUncached`, or `GuildMemberUncached`, as discord.js throws `GuildUncachedMe`.
 
-Each of them also has a `fetch*` twin (`fetchPermissions()`, `fetchKickable()`, `fetchDeletable()`,
-...) that asks the API for what is not cached. The twins are deprecated: the getter is the API, as
+Each of them also has a `fetch*` twin (`fetchKickable()`, `fetchDeletable()`, `fetchEditable()`,
+...) that asks the API for what is not cached. Most twins are deprecated: the getter is the API, as
 in discord.js. They will be removed in a later release.
 
+The getters skip a role that is not in the cache, without an error: `member.roles.highest` answers
+the highest role the cache holds and `member.permissions` the permissions of the roles it holds, so
+a role-hierarchy or permission check can under-report. These three twins are not deprecated, because
+they are the single call that stays right on a cache miss: `member.roles.fetchHighest()`,
+`member.fetchPermissions()` and `member.fetchPermissionsIn(channel)`. For `manageable`, `kickable`,
+`bannable` and `moderatable`, `await member.roles.fetch()` (and the bot's) before reading the getter.
+
+```ts
+// Right even when a role of the author is not cached:
+const [mine, theirs] = await Promise.all([me.roles.fetchHighest(), author.roles.fetchHighest()]);
+const canManageRoles = (await me.fetchPermissions()).has("ManageRoles");
+```
+
 When the cache may lack an entity (a size-limited or filtered cache, or a `plugin-broker` worker
 that only receives some dispatches), fetch what is missing yourself, then read the getter:
 

packages/plugin-gateway/src/managers/GuildMemberRoleManager.ts

@@ -41,7 +41,10 @@ function highestFirst(roles: Iterable<Role>): Role[] {
  * (`highest`, `hoist`, `color`, ...) are `Awaitable`, like `manager.cache.get`. They are synchronous with the default
  * in-memory cache and a promise with a remote store, so `await` works with both.
  *
- * They only know the roles the cache holds. The `fetch*` methods fall back to the API when a role is not cached.
+ * They only know the roles the cache holds, and skip the ones it lacks without an error. With a filtered cache or a
+ * `plugin-broker` worker that makes `highest` answer a lower role than the member's real one. Call
+ * {@link GuildMemberRoleManager.fetch} first, or {@link GuildMemberRoleManager.fetchHighest}, where a hierarchy check
+ * must not under-report. The other `fetch*` methods fall back to the API when a role is not cached.
  */
 export class GuildMemberRoleManager extends BaseManager {
   /**
@@ -94,15 +97,20 @@ export class GuildMemberRoleManager extends BaseManager {
   }
 
   /**
-   * The member's roles held in the cache, by ID, `@everyone` included. Roles that are not cached are skipped: use
-   * {@link GuildMemberRoleManager.fetch} to get them all.
+   * The member's roles held in the cache, by ID, `@everyone` included. Roles that are not cached are skipped, without
+   * an error: use {@link GuildMemberRoleManager.fetch} to get them all.
    */
   public get cache(): Awaitable<Collection<Snowflake, Role>> {
     return cachedRoles(this.client, this.guildId, [...this.ids, this.guildId]);
   }
 
   /**
    * The member's highest cached role. `null` when none of their roles is cached, `@everyone` included.
+   *
+   * @remarks
+   * Roles missing from the cache are skipped, not an error: the answer is the highest role the cache holds, so a
+   * hierarchy check can compare a higher-ranked member as a lower one. Use {@link GuildMemberRoleManager.fetchHighest}
+   * for an answer that is right on a cache miss, or `await member.roles.fetch()` before reading this.
    */
   public get highest(): Awaitable<Role | null> {
     return this.fromCache(pick.highest);
@@ -163,9 +171,10 @@ export class GuildMemberRoleManager extends BaseManager {
   /**
    * Fetches the member's highest role, `@everyone` when they have no other.
    *
-   * @deprecated Use {@link GuildMemberRoleManager.highest}. When some of the member's roles may be missing from the
-   * cache (a filtered cache, a `plugin-broker` worker), call {@link GuildMemberRoleManager.fetch} first, then read the
-   * getter.
+   * @remarks
+   * The call for a role-hierarchy check that must be right on a cache miss: unlike
+   * {@link GuildMemberRoleManager.highest}, it asks the API for the roles the cache lacks, so it never answers a lower
+   * role than the member's real one. With a complete cache it makes no request.
    */
   public async fetchHighest(): Promise<Role | null> {
     return pick.highest(await this.fetch());

packages/plugin-gateway/src/structures/guilds/GuildMember.ts

@@ -299,6 +299,11 @@ export class GuildMember extends Structure<CacheEntityTypes["members"]> {
    * The member's guild-wide permissions, before channel overwrites, like discord.js's `GuildMember#permissions`:
    * computed from the cached guild and roles, skipping the roles that are not cached.
    *
+   * @remarks
+   * A role missing from the cache is skipped, not an error, so the answer can hold fewer permissions than the member
+   * has. Use {@link GuildMember.fetchPermissions} for an answer that is right on a cache miss, or
+   * `await member.roles.fetch()` before reading this.
+   *
    * @throws A `GatewayError`: `GuildUncached` when the guild is not cached.
    */
   public get permissions(): CacheRead<Readonly<PermissionsBitField>> {
@@ -319,9 +324,9 @@ export class GuildMember extends Structure<CacheEntityTypes["members"]> {
    * Fetches the member's guild-wide permissions, before channel overwrites: {@link GuildMember.permissions} with the
    * guild and the roles fetched from the API when they are not cached, whatever the cache.
    *
-   * @deprecated Use {@link GuildMember.permissions}. When the guild or some roles may be missing from the cache (a
-   * filtered cache, a `plugin-broker` worker), fetch them first (`client.guilds.fetch(guildId)`,
-   * `member.roles.fetch()`), then read the getter.
+   * @remarks
+   * The call for a permission check that must be right on a cache miss: the getter skips the roles the cache lacks
+   * and can answer fewer permissions than the member has.
    */
   public async fetchPermissions(): Promise<Readonly<PermissionsBitField>> {
     const client = this.client;
@@ -340,10 +345,11 @@ export class GuildMember extends Structure<CacheEntityTypes["members"]> {
    * Fetches the member's permissions in a channel: their guild permissions with the channel's overwrites applied.
    * discord.js: `member.permissionsIn(channel)`.
    *
-   * @param channel The channel, or its ID. Threads use their parent's overwrites.
+   * @remarks
+   * Like {@link GuildMember.fetchPermissions}, the call for a check that must be right on a cache miss: it also
+   * fetches the channel and the roles when they are not cached.
    *
-   * @deprecated Use {@link GuildMember.permissionsIn}. See {@link GuildMember.fetchPermissions} for the cache-miss
-   * fallback; fetch the channel too (`client.channels.fetch(channelId)`).
+   * @param channel The channel, or its ID. Threads use their parent's overwrites.
    */
   public fetchPermissionsIn(channel: AnyChannel | string): Promise<Readonly<PermissionsBitField>> {
     return computePermissionsIn(channel, this);
@@ -354,6 +360,10 @@ export class GuildMember extends Structure<CacheEntityTypes["members"]> {
    * with the channel's overwrites applied, read from the cache.
    *
    * @param channel The channel, or the ID of a cached one. Threads use their parent's overwrites.
+   * @remarks
+   * A role missing from the cache is skipped, as in {@link GuildMember.permissions}: use
+   * {@link GuildMember.fetchPermissionsIn} for an answer that is right on a cache miss.
+   *
    * @throws A `GatewayError`: `ChannelUncached` or `GuildUncached` when they are not cached.
    */
   public permissionsIn(channel: AnyChannel | string): CacheRead<Readonly<PermissionsBitField>> {
@@ -453,6 +463,11 @@ export class GuildMember extends Structure<CacheEntityTypes["members"]> {
    * Whether the bot ranks above the member, like discord.js's `GuildMember#manageable`: it is not the guild owner, not
    * the bot itself, and the bot's highest role is higher than theirs (or the bot owns the guild).
    *
+   * @remarks
+   * It reads {@link GuildMemberRoleManager.highest}, which skips the roles missing from the cache: a higher role that
+   * is not cached makes the member look lower. Call `await member.roles.fetch()` and `await me.roles.fetch()` before
+   * reading this, or compare {@link GuildMemberRoleManager.fetchHighest} of both.
+   *
    * @throws A `GatewayError`: `GuildUncached` or `GuildUncachedMe` when the guild or the bot's member is
    * not cached.
    */
@@ -493,23 +508,24 @@ export class GuildMember extends Structure<CacheEntityTypes["members"]> {
 
   /**
    * Whether the bot can kick the member, like discord.js's `GuildMember#kickable`: it outranks them and has
-   * `KickMembers`. It throws like {@link GuildMember.manageable}.
+   * `KickMembers`. It throws, and skips uncached roles, like {@link GuildMember.manageable}.
    */
   public get kickable(): CacheRead<boolean> {
     return cacheRead(this.managedWithCached("KickMembers"));
   }
 
   /**
    * Whether the bot can ban the member, like discord.js's `GuildMember#bannable`: it outranks them and has
-   * `BanMembers`. It throws like {@link GuildMember.manageable}.
+   * `BanMembers`. It throws, and skips uncached roles, like {@link GuildMember.manageable}.
    */
   public get bannable(): CacheRead<boolean> {
     return cacheRead(this.managedWithCached("BanMembers"));
   }
 
   /**
    * Whether the bot can time the member out, like discord.js's `GuildMember#moderatable`: it outranks them, has
-   * `ModerateMembers`, and they are no administrator. It throws like {@link GuildMember.manageable}.
+   * `ModerateMembers`, and they are no administrator. It throws, and skips uncached roles, like
+   * {@link GuildMember.manageable}.
    */
   public get moderatable(): CacheRead<boolean> {
     return cacheRead(

packages/plugin-gateway/tests/sync-getters.test.ts

@@ -267,6 +267,34 @@ describe.each(synchronousModes)("member getters with %s", (_, options) => {
     }
   });
 
+  test("GIVEN a role missing from the cache THEN the getters under-report and fetchHighest, fetchPermissions and fetchPermissionsIn stay right", async () => {
+    const client = createClient(options());
+    await seed(client);
+    const [top, bot] = await Promise.all([topId, botId].map((id) => memberOf(client, id)));
+    await client.cache!.roles.delete(roleKey(guildId, topRoleId));
+    const get = vi
+      .spyOn(container.rest, "get")
+      .mockResolvedValue([
+        role(guildId, 0, flags.ViewChannel),
+        role(modRoleId, 2, flags.KickMembers | flags.BanMembers | flags.ModerateMembers),
+        role(topRoleId, 3, flags.ManageRoles),
+      ]);
+
+    // The cache lacks the member's only role: the getters fall back to @everyone.
+    expect((await top.roles.highest)?.id).toBe(guildId);
+    expect(top.permissions.has("ManageRoles")).toBe(false);
+    expect(top.manageable).toBe(true);
+    expect(get).not.toHaveBeenCalled();
+
+    expect((await top.roles.fetchHighest())?.id).toBe(topRoleId);
+    expect((await top.fetchPermissions()).has("ManageRoles")).toBe(true);
+    expect((await top.fetchPermissionsIn(channelId)).has("ManageRoles")).toBe(true);
+    await top.roles.fetch();
+    expect(top.manageable).toBe(false);
+    expect((await bot.roles.fetchHighest())?.id).toBe(modRoleId);
+    get.mockRestore();
+  });
+
   test("GIVEN the bot owns the guild THEN everyone else is manageable", async () => {
     const client = createClient(options());
     await seed(client, { guildOwner: botId });