Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Role and user mentions are returned out of order #2669

Closed
cmdlime opened this issue Jul 22, 2018 · 9 comments

Comments

@cmdlime
Copy link

commented Jul 22, 2018

Please describe the problem you are having in as much detail as possible:

User and role mentions are returned out of order in which they appear in a message. They always appear in the same, fixed order (which appears to be by role/user creation date, oldest first). It's worth noting that channel mentions do always appear in the correct order.

Include a reproducible code sample here, if possible:

For a simple (albeit silly) pseudocode demo, here are two messages that mention 3 channels, 3 roles, and 3 users in a different order...

Scenario A

Message:

#channel1 #channel2 #channel3 @role1 @role2 @role3 @user1 @user2 @user3

Result:

msg.mentions.channels = [#channel1, #channel2, #channel3] (correct order)
msg.mentions.roles = [@role2, @role3, @role1] (wrong order)
msg.mentions.users = [@user3, @user1, @user2] (wrong order)

Scenario B

Message:

#channel3 #channel2 #channel1 @role3 @role2 @role1 @user3 @user2 @user1

Result:

msg.mentions.channels = [#channel3, #channel2, #channel1] (correct order)
msg.mentions.roles = [@role2, @role3, @role1] (wrong order)
msg.mentions.users = [@user3, @user1, @user2] (wrong order)

Further details:

  • discord.js version: 11.3.2 (and master)
  • node.js version: 10.4.1
  • Operating system: macOS 10.13.5
  • Priority this issue should have – please be realistic and elaborate if possible: High -- see below

A similar issue was raised (and fixed) in Jan 2016 (#166), but appeared to resurface in late 2017.

As it stands today, this issue is caused by relying on the raw data coming back from the Discord API. Inspecting the raw return data from Discord shows the mentions coming back ordered by user/role creation date.

This can be dangerous behavior, IMO. Storing in a Collection (Map) means we should be able to rely on them being returned in insertion order. In the context of a message, the implication is that it would be the order in which they were mentioned.

In Discord.JS's own docs, an example recommends using message.mentions.users.first() in a command to kick someone from the server.
https://github.com/discordjs/discord.js/blob/master/docs/examples/moderation.md

But let's say someone sets up a command with the syntax !kick @user Reason here (my own server uses a similar syntax). Running the command !kick @offender Because they were bullying @victim would actually kick @victim if their account was created before @offender. 😬

The question is -- is this something the Discord.js project would be willing to fix (parsing the message for those mentions on its own)? Or would we have to rely on Discord to make that change? (Based on my past experience with them and their massive bug backlog, I'd guess the chances of them changing it are slim to none.)

If the latter, then this completely rules out using any commands in which the order of mentions would be significant -- and in that case, the docs should be updated to warn users not to rely on it.

  • I found this issue while running code on a user account
  • I have also tested the issue on latest master, commit hash: 717e7f0
@PLASMAchicken

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

We have Here

MessageMentions.USERS_PATTERN = /<@!?(1|\d{17,19})>/g;
MessageMentions.ROLES_PATTERN = /<@&(\d{17,19})>/g;
MessageMentions.CHANNELS_PATTERN = /<#(\d{17,19})>/g;

But we only use CHANNELS_PATTERN in the code to sort the channels

@appellation

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

Channel mentions are artificially constructed from the message content; we rely on Discord to provide data on which users and roles are mentioned. In this case, it seems very likely that Discord is sending data that is out of order. It is possible to correct this by simply generating all mentions from the message content, but I'm not sure that's a design change we would like to make.

Personally I have always been a fan of manually parsing message content to ensure that it meets command requirements. Using the mentions object is further complicated by bots that support mention prefix (which all bots should). Personally, I would prefer to see the docs updated to encourage parsing the message content manually.

@JMTK

This comment has been minimized.

Copy link

commented Jul 23, 2018

Couldn't you just use the position property to order them how you need to?

@appellation

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

I'm not entirely sure what "position" you could be referring to. The position of roles in the guild is entirely irrelevant, and mentions are supplied by the API as an array of IDs (in the case of roles) or base objects (in the case of users). Neither of these provide a mention order outside of the array itself, which seems to be incorrect.

@JMTK

This comment has been minimized.

Copy link

commented Jul 23, 2018

Sorry I misunderstood the question.

@cmdlime

This comment has been minimized.

Copy link
Author

commented Jul 23, 2018

From the Discord.js homepage:

discord.js is a powerful node.js module that allows you to interact with the Discord API very easily. It takes a much more object-oriented approach than most other JS Discord libraries, making your bot's code significantly tidier and easier to comprehend. Usability, consistency, and performance are key focuses of discord.js

I don't think anyone would disagree that returning mentions unexpectedly out of order is harmful to usability and consistency. And asking people to parse messages on their own if they want it to work properly because they can't rely on one of the objects the library returns certainly makes the library less "easy" to use.

Correct me if I'm wrong, but I think performance would be the only reason to consider not fixing this issue?

The Jan 2016 fix (in #166, resolved by e3173d1) was to parse them in the library. What kind of a performance hit are we actually talking about if we go this route again?

And from a design perspective -- if we're already parsing the messages to artificially construct channel mentions, is it that big a departure to also do the same for roles and users? Especially when the library has done that before in the recent past?

@Gawdl3y

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

Discord.js just uses the list of mentions provided by the API. Neither the API documentation nor our own documentation ever list the properties as ordered in any specific way. Furthermore, they're placed in a Collection (a Map), which, while technically ordered, relying on them being so is typically not recommended.

Nobody should be relying on the mentions property for the purpose of getting arguments for a command, for this reason and others. This has always been the case and always will be. Parse arguments properly.

The performance impact of running two additional regular expressions against every single message received by a bot would be significant, and is not something we want to do for data that is already provided by the API anyways.

@Gawdl3y

This comment has been minimized.

Copy link
Member

commented Jul 23, 2018

Here's another fun reason you absolutely should not be relying on a simple list of mentions for your arguments: If someone mentions your bot with a command (which every bot should support, no exceptions), i.e. @SomeBot#4444 ban @SomeDude#6969, and your command is designed to rely on the first mention... yeah.

@cmdlime

This comment has been minimized.

Copy link
Author

commented Aug 3, 2018

Nobody should be relying on the mentions property for the purpose of getting arguments for a command, for this reason and others. This has always been the case and always will be.

This may be true -- but apparently there is confusion even among the maintainers as to the use of these collections which I will reference again here (a particularly dangerous example in this case, since it can kick the wrong user from the server).

https://github.com/discordjs/discord.js/blob/master/docs/examples/moderation.md

Library users are going to expect the collection ordering to work as advertised in the docs. I'd suggest that this issue remain open until any examples referencing mentions.[users|roles].first() are rewritten to manually parse the mentions as discussed above. It would also be beneficial to put a warning on the users and roles pages to let people know that first() and last() (or anything else involving order) are not reliable for those collections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.