diff --git a/.changeset/gateway-keep-hierarchy-fetches.md b/.changeset/gateway-keep-hierarchy-fetches.md new file mode 100644 index 00000000..7e1228a3 --- /dev/null +++ b/.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()`. diff --git a/packages/plugin-gateway/README.md b/packages/plugin-gateway/README.md index de8df19f..640df0b4 100644 --- a/packages/plugin-gateway/README.md +++ b/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: diff --git a/packages/plugin-gateway/src/managers/GuildMemberRoleManager.ts b/packages/plugin-gateway/src/managers/GuildMemberRoleManager.ts index dbae5ca8..87fe4da4 100644 --- a/packages/plugin-gateway/src/managers/GuildMemberRoleManager.ts +++ b/packages/plugin-gateway/src/managers/GuildMemberRoleManager.ts @@ -41,7 +41,10 @@ function highestFirst(roles: Iterable): 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,8 +97,8 @@ 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> { return cachedRoles(this.client, this.guildId, [...this.ids, this.guildId]); @@ -103,6 +106,11 @@ export class GuildMemberRoleManager extends BaseManager { /** * 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 { 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 { return pick.highest(await this.fetch()); diff --git a/packages/plugin-gateway/src/structures/guilds/GuildMember.ts b/packages/plugin-gateway/src/structures/guilds/GuildMember.ts index 9508c652..7393f35d 100644 --- a/packages/plugin-gateway/src/structures/guilds/GuildMember.ts +++ b/packages/plugin-gateway/src/structures/guilds/GuildMember.ts @@ -299,6 +299,11 @@ export class GuildMember extends Structure { * 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> { @@ -319,9 +324,9 @@ export class GuildMember extends Structure { * 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> { const client = this.client; @@ -340,10 +345,11 @@ export class GuildMember extends Structure { * 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> { return computePermissionsIn(channel, this); @@ -354,6 +360,10 @@ export class GuildMember extends Structure { * 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> { @@ -453,6 +463,11 @@ export class GuildMember extends Structure { * 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,7 +508,7 @@ export class GuildMember extends Structure { /** * 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 { return cacheRead(this.managedWithCached("KickMembers")); @@ -501,7 +516,7 @@ export class GuildMember extends Structure { /** * 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 { return cacheRead(this.managedWithCached("BanMembers")); @@ -509,7 +524,8 @@ export class GuildMember extends Structure { /** * 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 { return cacheRead( diff --git a/packages/plugin-gateway/tests/sync-getters.test.ts b/packages/plugin-gateway/tests/sync-getters.test.ts index 9ff8cbb0..800d4c22 100644 --- a/packages/plugin-gateway/tests/sync-getters.test.ts +++ b/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 });