Adding a check in reaction methods before calling getTextChannel() #1216

DManstrator · 2020-03-04T22:40:23Z

Pull Request Etiquette

I have checked the PRs for upcoming features/bug fixes.
I have read the contributing guidelines.

Changes

Internal code
Library interface (affecting end-user code)
Documentation
Other: _____

Closes Issue: NaN

Description

This PR adds a check in removing / clearing reaction methods, checking if the message was sent in a TextChannel and else tells the user that he is unable to remove reactions of others or clear them if the message is not from a TextChannel.

Before, getTextChannel() was called without a check throwing This message was not sent in a text channel if the message was from a PrivateChannel (or group) which could be misleading (and actually was which is why I make that PR) in that context.

This also fixes a bug where if the SelfUser was passed to the function, it would still have thrown that exception even that would be allowed.

gpluscb

The cases in which these Exceptions are thrown should be documented in the Message class.

src/main/java/net/dv8tion/jda/internal/entities/ReceivedMessage.java

…s and also checked for the SelfUser

DManstrator · 2020-03-04T23:50:52Z

Docs and code should be correct now, waiting for requested changes.

duncte123 · 2020-03-05T07:12:20Z

src/main/java/net/dv8tion/jda/internal/entities/ReceivedMessage.java

        return getTextChannel().clearReactionsById(getId(), unicode);
    }

    @Nonnull
    @Override
    public RestAction<Void> clearReactions(@Nonnull Emote emote)
    {
+        if (!isFromType(ChannelType.TEXT))
+            throw new IllegalStateException("Cannot clear reactions from a message in a Group or PrivateChannel.");


You should probably remove the group from here since bots can't be in groups

I am fully aware of this but since this was the original output and the throws section saying "if was not sent in a TextChannel", I decided to stay with this.

However in my JavaDoc additions, I explicitly talk about private messages since it's easier to understand that than telling it has to come from a TextChannel imo.

TL;DR: I would wait for Minn's response since this is likely to be unwanted.

MinnDevelopment · 2020-03-05T11:50:22Z

src/main/java/net/dv8tion/jda/api/entities/Message.java

+     *         If the provided unicode emoji is null or empty or if the provided user is null.
+     * @throws java.lang.IllegalStateException
+     *         If this message was <b>not</b> sent in a
+     *         {@link net.dv8tion.jda.api.entities.TextChannel TextChannel}


I'd say not sent in a guild to also account for other channel types such as news channels

There is no extra ChannelType for news channels, they are treated as ChannelType.TEXT. ChannelType#isGuild returns true for TEXT, VOICE, CATEGORY and STORE and only TEXT can have messages. So I don't exactly see why this is needed.

But nevertheless I changed it everywhere to isFromGuild() except

public TextChannel getTextChannel() { if (!isFromType(ChannelType.TEXT)) throw new IllegalStateException("This message was not sent in a text channel"); return (TextChannel) channel; }

to stay safe here.

Edit:
Changed that to also use isFromGuild() to not run into unexpected errors (methods checking isFromGuild(), getTextChannel() checking only for ChannelType#TEXT), added a comment to it.

Current behavior doesn't mean its permanent, being vague allows us to make changes more easily.

How should I implement it then? As it is now or should I revert that back? Or do I get your comment wrong?

MinnDevelopment · 2020-03-05T11:50:41Z

src/main/java/net/dv8tion/jda/internal/entities/ReceivedMessage.java

+            // we can safely remove that
+            return channel.removeReactionById(getIdLong(), emote);
+
+        if (!isFromType(ChannelType.TEXT))


These checks should be changed to isFromGuild checks

MinnDevelopment · 2020-03-05T11:52:46Z

src/main/java/net/dv8tion/jda/api/entities/Message.java

@@ -1324,6 +1336,8 @@ default boolean isFromGuild()
    /**
     * Removes a {@link net.dv8tion.jda.api.entities.User User's} reaction from this Message using an {@link net.dv8tion.jda.api.entities.Emote Emote}.
     *
+     * <p>Please note that you <b>can't</b> remove reactions of other users if this is a private message / this message was sent in a {@link net.dv8tion.jda.api.entities.PrivateChannel PrivateChannel}!


Suggested change

* Please note that you can't remove reactions of other users if this is a private message / this message was sent in a {@link net.dv8tion.jda.api.entities.PrivateChannel PrivateChannel}!

* Please note that you can't remove reactions of other users if this message was sent in a {@link net.dv8tion.jda.api.entities.PrivateChannel PrivateChannel}!

Also adjusted the docs as requested

MinnDevelopment · 2020-03-06T01:30:37Z

src/main/java/net/dv8tion/jda/internal/entities/ReceivedMessage.java

-        if (!isFromType(ChannelType.TEXT))
+        // this is not really safe but at the moment this is the only GuildChannel with messages
+        // and a MessageChannel can just be Text or Private so this is assumable as of now
+        if (!isFromGuild())


This check should be reverted.

DManstrator · 2020-03-14T14:40:23Z

In case it went under the radar, I re-requested a review.

src/main/java/net/dv8tion/jda/internal/entities/ReceivedMessage.java

DV8FromTheWorld · 2020-04-02T13:53:56Z

What is the progress on this PR? I see @MinnDevelopment has approved already.

…iscord-jda#1216)

…nel() (discord-jda#1216)" This reverts commit d74ee2e.

* Fix description for RestAction#flatMap * Remove invalid NSFW info * Fix bug in buffer allocation for ZlibDecompressor * Don't insert null into override cache * Keep a strong reference to user from members Fake members will use fake users that are not cached * Add functionality to set/get state of embed suppression (discord-jda#1117) * Add functionality to set/get state of embed suppression * Provide Enum for MessageFlags * Improve handling of events for unavailable guilds (discord-jda#1184) * Improve handling of events for unavailable guilds * Add UnavailableGuildLeaveEvent * Update docs for Guild#isAvailable * Improve ratelimit bucket handling (discord-jda#1103) * Let ChannelUpdateHandler add overrides to cache This is a bug since the override shouldn't be in the interface if the member is not actually in the cache. With the current implementation we have a cache inconsistency that could NPE. * Change handling of GuildDeleteHandler to avoid deadlock There really is no point in holding a write-lock here * Bump version * Add South Korea Region * Add getPermissionHolder method to PermissionOverride (discord-jda#1183) * Make ignored events for unavailable guilds a debug log * Add ErrorHandler utility class (discord-jda#1200) * Add ErrorHandler utility class * Improve logging of 429 warnings * Add clearReactions(String) (discord-jda#1192) * Add clearReactions(String) * Add MessageReactionRemoveEmoteEvent * Fix some outdated documentation * Log and rethrow errors in BotRateLimiter workers * Properly encode reaction emoji for clearReactionsById(...) * Updated all copyright headers for 2020 (discord-jda#1198) * Add (Guild) Invite Events (discord-jda#1196) Adds GuildInviteCreateEvent and GuildInviteDeleteEvent * Allow queued requests to finish * Support gateway intents and configurable member caching (discord-jda#1190) * First pass on member cache policy * Add INVALID_INTENTS to close code enum * Deprecate AccountType.CLIENT * Add Guild#unloadMembers * Update README with better examples * Improve handling of channel overrides * Add documentation for intents and member cache policy * Remove references to guild subscriptions * Add GuildMemberRemoveEvent * Improve retrieveMemberById and retrieveUserById * Changes to the builders, we use factory methods now :) * Disable presence cache by default * Add JDA#getGatewayIntents * Fix NPE for GUILD_UPDATE handling * Fix GuildChannel#createCopy for uncached members (discord-jda#1229) * Deprecate GuildUpdateBannerEvent#getNewBannerIdUrl (discord-jda#1217) * Allow to open private channel with user id (discord-jda#1224) * Change defaults for requester to be non-daemon and have a shorter timeout (discord-jda#1215) * Added Server Insights permission (discord-jda#1221) * Add RestAction#timeout and RestAction#deadline (discord-jda#1220) * Fix NPE in PermissionOverrideAction constructor * Fix another NPE in success handler * Documentation updates (discord-jda#1222) * Swapped manage server and manage channel in Invite#isExpanded docs (discord-jda#1210) * Document another possible null return case of Message#getMember (discord-jda#1204) * Fix EmbedBuilder constructor javadocs (discord-jda#1214) * Update example for jda-nas * Add more details to modifyMemberRoles docs * Fix typo * Added getGuildChannelById methods to ShardManager (discord-jda#1234) * Add better configuration methods to the builders (discord-jda#1240) * Update README * Add enableCache and disableCache * Add enableIntents and disableIntents * Update docs for events to mention required intents when necessary. * Deprecate the old setters for cache flags * Include voice server endpoint in connection failure logs * Initialize SelfUser for all shards before logging them in * Fix bug with new channel overrides having incorrect permissions * Add the close code to the disconnect warn * Adding a check in reaction methods before calling getTextChannel() (discord-jda#1216) * Add JDA#cancelRequests (discord-jda#1223) * Fix boost time not being handled in guild create * Add GuildVoiceStreamEvent * Fix IllegalStateException in Requester Closes discord-jda#1254 * Change reconnect code to 4900 to avoid confusion (discord-jda#1250) * Change reconnect code to 4900 to avoid confusion * And update the disconnect handling to better log things * Make resume reconnect log on debug * Log server error on error level * Add catch in gateway worker * Add Guild#getMaxFileSize and Guild.BoostTier#getMaxFileSize (discord-jda#1244) Fixes discord-jda#1243 * Add fallback operators onErrorMap and onErrorFlatMap to RestAction (discord-jda#1219) * Fix incorrect file size check * Documentation updates (discord-jda#1241) * Add note about presence update and member intents * Fix typo in readme * Add MessageChannel#retrieveReactionUsersById and update reaction related docs (discord-jda#1236) * Add requirements section to member/user events * Json Parsing Optimizations (discord-jda#1267) * Remove initial reconnect delay for resumes * Add prune overload to disable wait parameter * Add Guild#retrieveMetaData (discord-jda#1171) * Lazy load member limits for guilds * Improve implementation of update handler * Add Guild#retrieveMetaData * Add check for heartbeats in WebSocketClient (discord-jda#1282) * Add retrieveMembersByPrefix (discord-jda#1276) * Add retrieveMemberByPrefix * Add event pool configuration * Fix javadoc errors * Rename chunkSyncQueue -> chunkQueue * Create file if it doesn't exist in downloadToFile * Add members to cache from retrieveMember methods * Improve handling of join times for members * Add Message.JUMP_URL_PATTERN * Replace disconnect handling to recover from strange thread states (discord-jda#1295) * Shutdown the WebSocket connection after everything else * Use executor to shutdown in DefaultShardManager * Add methods to bulk retrieve members (discord-jda#1292) * Add methods to bulk retrieve members * Add documentation * Add missing annotations to Task interface * Use random number as nonce * Improvements to handling of Error (discord-jda#1285) * Remove usage of AssertionError * Improve handling of Errors * Change domain to discord.com (discord-jda#1288) * Fix NPE in ChannelManager#sync * Add User#getFlags (discord-jda#1271) * Add Emote#isAvailable() method Co-authored-by: Florian Spieß <business@minnced.club> Co-authored-by: Michael Ritter <ritter.michael92@gmail.com> Co-authored-by: horyu1234 <horyu1234@naver.com> Co-authored-by: DMan <DManstrator@users.noreply.github.com> Co-authored-by: Austin Keener <keeneraustin@yahoo.com> Co-authored-by: averen <31487840+averen@users.noreply.github.com> Co-authored-by: Mathéo CIMBARO <25774021+DiscowZombie@users.noreply.github.com> Co-authored-by: Artuto <artutogamer@gmail.com> Co-authored-by: Nik <nik@monsterlyrics.co> Co-authored-by: Sebo Molnár <git@mlnr.dev> Co-authored-by: gpluscb <20143778+gpluscb@users.noreply.github.com> Co-authored-by: Napster <napster@npstr.space>

Added a check everytime getTextChannel() would be called

7e9661f

gpluscb suggested changes Mar 4, 2020

View reviewed changes

src/main/java/net/dv8tion/jda/internal/entities/ReceivedMessage.java Show resolved Hide resolved

DManstrator added 2 commits March 5, 2020 00:30

Throwing the correct Exception now, improved docs of the Message clas…

b6403bb

…s and also checked for the SelfUser

Fixing possible NPEs and adding null-users to the throws section

e4af795

gpluscb approved these changes Mar 4, 2020

View reviewed changes

duncte123 reviewed Mar 5, 2020

View reviewed changes

MinnDevelopment reviewed Mar 5, 2020

View reviewed changes

DManstrator added 2 commits March 6, 2020 01:25

Checking for isFromGuild instead of ChannelType#TEXT

4e4bb0a

Also adjusted the docs as requested

fixed a typo

5d2d89f

MinnDevelopment reviewed Mar 6, 2020

View reviewed changes

Reverted check back to isFromType

076d2e4

DManstrator requested a review from MinnDevelopment March 14, 2020 14:40

MinnDevelopment added size: small type: enhancement labels Mar 14, 2020

MinnDevelopment approved these changes Mar 16, 2020

View reviewed changes

src/main/java/net/dv8tion/jda/internal/entities/ReceivedMessage.java Show resolved Hide resolved

Moved the comment above the code

52a478b

MinnDevelopment merged commit 3162dfa into discord-jda:development Apr 4, 2020

Andre601 pushed a commit to Andre601/JDA that referenced this pull request Apr 21, 2020

Adding a check in reaction methods before calling getTextChannel() (d…

d74ee2e

…iscord-jda#1216)

Andre601 added a commit to Andre601/JDA that referenced this pull request Apr 21, 2020

Revert "Adding a check in reaction methods before calling getTextChan…

c9b68b9

…nel() (discord-jda#1216)" This reverts commit d74ee2e.

Andre601 added a commit to Andre601/JDA that referenced this pull request Apr 21, 2020

Revert "Adding a check in reaction methods before calling getTextChan…

e726328

…nel() (discord-jda#1216)" This reverts commit d74ee2e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a check in reaction methods before calling getTextChannel() #1216

Adding a check in reaction methods before calling getTextChannel() #1216

DManstrator commented Mar 4, 2020 •

edited

gpluscb left a comment

DManstrator commented Mar 4, 2020

duncte123 Mar 5, 2020

DManstrator Mar 5, 2020

MinnDevelopment Mar 5, 2020

DManstrator Mar 5, 2020 •

edited

MinnDevelopment Mar 6, 2020

DManstrator Mar 6, 2020

MinnDevelopment Mar 5, 2020

MinnDevelopment Mar 5, 2020

MinnDevelopment Mar 6, 2020

DManstrator commented Mar 14, 2020

DV8FromTheWorld commented Apr 2, 2020

	* <p>Please note that you <b>can't</b> remove reactions of other users if this is a private message / this message was sent in a {@link net.dv8tion.jda.api.entities.PrivateChannel PrivateChannel}!
	* <p>Please note that you <b>can't</b> remove reactions of other users if this message was sent in a {@link net.dv8tion.jda.api.entities.PrivateChannel PrivateChannel}!

Adding a check in reaction methods before calling getTextChannel() #1216

Adding a check in reaction methods before calling getTextChannel() #1216

Conversation

DManstrator commented Mar 4, 2020 • edited

Pull Request Etiquette

Changes

Description

gpluscb left a comment

Choose a reason for hiding this comment

DManstrator commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DManstrator Mar 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DManstrator commented Mar 14, 2020

DV8FromTheWorld commented Apr 2, 2020

DManstrator commented Mar 4, 2020 •

edited

DManstrator Mar 5, 2020 •

edited