discord.js icon indicating copy to clipboard operation
discord.js copied to clipboard

roles.remove() removes unintended roles when used adjacently to roles.add()

Open ModeratelyGreek opened this issue 3 years ago • 8 comments

Which package is this bug report for?

discord.js

Issue description

In one sentence, roles.remove() exhibits incorrect behavior when preceded by roles.add(), specifically when removing an array. More specifically, it removes role(s) that were just .add()ed (but not passed to the remove method call). Please see the code below for a few demonstrated/commented cases.

I set this at medium priority seeing as it is moderately problematic and a genuine occurrence of incorrect behavior.

Code sample

import { Client, GuildMember, Intents } from "discord.js";

const botToken = TOKEN_HERE;

const guildId = "970176225781878794";
const userId = "253338867950813194";

const roleIdToAdd = "970176225781878799";

// Does not contain roleIdToAdd
const roleIdsToRemove = [
  "970176225781878795",
  "970176225781878796",
  "970176225781878797",
  "970176225781878798",
];

const onReady = async (client: Client) => {
  const guild = await client.guilds.fetch(guildId);
  const user = await guild.members.fetch(userId);

  // Change to run different cases
  const caseId: number = 2;

  switch (caseId) {
    case 1:
      await case1(user);
      return;
    case 2:
      await case2(user);
      return;
  }
};

// Add single, remove array
// THIS DOES NOT WORK - Results in roleIdToAdd being added THEN REMOVED
const case1 = async (user: GuildMember) => {
  console.log(`Adding: ${roleIdToAdd}`);
  await user.roles.add(roleIdToAdd);

  console.log(`Removing: ${roleIdsToRemove.join(", ")}`);
  await user.roles.remove(roleIdsToRemove);
};

// Add single, remove one by one
// THIS WORKS - Results in roleIdToAdd being added
const case2 = async (user: GuildMember) => {
  console.log(`Adding: ${roleIdToAdd}`);
  await user.roles.add(roleIdToAdd);

  for (const roleIdToRemove of roleIdsToRemove) {
    console.log(`Removing: ${roleIdToRemove}`);
    await user.roles.remove(roleIdToRemove);
  }
};

const run = async () => {
  const client = new Client({ intents: [Intents.FLAGS.GUILDS] });

  client.on("ready", (x) => onReady(x));

  await client.login(botToken);
};

run().catch((e) => {
  console.log(e);
}); 

Package version

13.8.0

Node.js / Typescript versions

Node: 16.15.1 Typescript: 4.5.5

Operating system

Windows 10

Priority this issue should have

Medium (should be fixed soon)

Which partials do you have configured?

No Partials

Which gateway intents are you subscribing to?

Guilds

I have tested this issue on a development release

No response

ModeratelyGreek avatar Jun 23 '22 19:06 ModeratelyGreek

This is probably a race condition with the updates coming through the WebSocket 🤔

Edit: Probably also the reason it works when you remove them one by one, since the updates have just enough time before the next call happens.

Don't take this as a 100% though, it could be something else, but that would be my guess.

iCrawl avatar Jun 23 '22 19:06 iCrawl

Crazy fast reply, cheers for the insight!

Edit: I'll add that .remove()ing prior to .add()ing succeeds consistently, array or not!

ModeratelyGreek avatar Jun 23 '22 19:06 ModeratelyGreek

For a 100% success rate you can also do just 1 call and use .set, you'd just have to compute the resulting roles yourself

ImRodry avatar Jun 23 '22 20:06 ImRodry

Ideally, you would just take the current role ids in an Array, add/remove the ones as desired, then call GuildMember#roles#set yourself (single call).

KinectTheUnknown avatar Jun 23 '22 20:06 KinectTheUnknown

Also having this issue. The list of roles I am removing is an empty array and yet it removes all of the roles previously granted, please fix :)

let modules = courses.courses[course.toString()][getYearGroup(interaction)];
await interaction.guild.members.cache.get(interaction.user.id).roles.add(modules).then(() => {
    //Delete other modules
    let count = 0;
    let del = interaction.guild.members.cache.get(interaction.user.id).roles.cache.filter(role => role.color === 10181046);
    let RolesToRemove = [];
    del.forEach(role => {
        if(!modules.includes(role.id)) {
            RolesToRemove.push(role.id);
            count++;
        }
    });

    interaction.guild.members.cache.get(interaction.user.id).roles.remove(RolesToRemove);
    console.info("Removed " + count + " previous modules from user");
}).catch(err => {
    console.error(err);
});

This "RolesToRemove" variable is an empty array, however, as seen in the image below, all given roles are instantly removed DiscordCanary_PlURPb5m25

PenguinNexus avatar Aug 02 '22 20:08 PenguinNexus

Judging from that code sample, you should be using GuildMemberRoleManager#set(). It'd actually simplify your code too.

Jiralite avatar Aug 02 '22 21:08 Jiralite

Not in scope of the library, this has to do with how Discord handles and accumulates back-to-back role updates. This can even happen across bots, if you have various bots changing roles at roughly the same time (most popularly when joining a server and doing role state and a default role).

To prevent this, do all in one GuildMember#edit (if you want to do other updates on the member apart from roles), or the aforementioned #set.

Also make sure you do not have multiple bots handling roles automatically, as that, too, can produce the issue.

almostSouji avatar Aug 02 '22 21:08 almostSouji