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

Restructure message creation to handle a lack of guild_id #2039

Merged
merged 3 commits into from
Feb 23, 2022

Conversation

DV8FromTheWorld
Copy link
Member

@DV8FromTheWorld DV8FromTheWorld commented Feb 22, 2022

Pull Request Etiquette

Changes

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

Closes Issue: NaN

Description

Discord does not provide guild_id on message objects. We only ever get this property on messages when they come over the Websocket Gateway. However, historically, JDA has been written to expect it and we've gotten lucky that nothing broke.

Recently we made a change to enforce nullability of getUser on PrivateChannel and restructured some code to enable this along with filling in the data whenever we receive it, which happens to happen on MESSAGE_CREATE. This has revealed our erroneous reliance on guild_id in non-gateway situations.

This PR restructured the EntityBuilder to enforce upon developers of JDA's internal code that they must either have a channel instance or define if the channel is guild or private when building a message. This makes explicit the methodologies used to determine isGuild instead of relying on a typically-missing guild_id json property.

Comment on lines +59 to +61
message = isGuild
? jda.getEntityBuilder().createMessageGuildChannel(content, true)
: jda.getEntityBuilder().createMessagePrivateChannel(content, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add createMessage(content, isGuild, true)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, I think the little bit of dupe we have that does this ternary is fine for now.

@MinnDevelopment
Copy link
Member

This PR should also add an implementation for getNewsChannel() in ReceivedMessage. This is currently not implemented properly and only throws an exception in here:

https://github.com/DV8FromTheWorld/JDA/blob/c0742f997a18d0abaf8b9dcb4a9c48eb6b0b62f4/src/main/java/net/dv8tion/jda/internal/entities/AbstractMessage.java#L320

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants