Threads #922

phenpessoa · 2021-05-03T12:56:48Z

I've started the work on threads.
This PR also adds missing message types/flags.
Any feedback/help is greatly appreciated. Mainly what is missing are the endpoints and the RESTAPI funcs.

-- EDIT
Everything should be done now

phenpessoa · 2021-05-03T23:13:44Z

I think that's everything.
Waiting on feedbacks

phenpessoa · 2021-05-04T17:36:16Z

Updated the code to be in line with these changes on the threads docs: discord/discord-api-docs@663dc59

phenpessoa · 2021-05-11T16:32:48Z

Breaking change!

Webhooks now have an optional ThreadID field. Docs: discord/discord-api-docs@cd0cae8

Because of the way WebhookExecute is currently written, this is a breaking change.
Updated the code to be in line with discord.

Lemme know if there should be another/better approach to this.

belak · 2021-07-27T19:55:14Z

It looks like this is being pushed out publicly now - what would it take to get this merged?

phenpessoa · 2021-07-27T21:16:45Z

There were a few changes to the discord API since I last updated this PR (nothing major, though). This is why this is still a draft.
I also need to merge the new changes from the upstream.

I've been busy lately, that's why I haven't finished this. But I will finish and undraft this PR later this week.

phenpessoa · 2021-08-02T22:18:23Z

I think this is ready for review now. @CarsonHoffman

kovetskiy · 2021-08-03T07:10:14Z

state.go

+			}
+		}
+	}
+


I'm just lurking around since I'm waiting for this feature in the upstream and decided to give the PR a look.

Should not the method take into account AddedMembers field too? The documentation of the method says it updates the member object of the thread but it only takes care of the case when the user was removed.

I think you are right! Will push an update. Let me know what you think.

CarsonHoffman · 2021-08-09T00:25:15Z

restapi.go

+// ThreadEdit edits the given thread
+// threadID  : The ID of a Thread
+// name       : The new name to assign the thread.
+func (s *Session) ThreadEdit(channelID string, name string) (*Channel, error) {
+	return s.ThreadEditComplex(channelID, &ThreadEditData{
+		Name: name,
+	})
+}


Given that ThreadEditComplex clears out all properties not specified, I'm not sure that having this function as is here makes a ton of sense. It seems to me like it would quite quickly lead to unexpected behavior and not really have much use.

CarsonHoffman · 2021-08-09T00:25:43Z

structs.go

+type ThreadMember struct {
+	ID            string    `json:"id,omitempty"`
+	UserID        string    `json:"user_id,omitempty"`
+	JoinTimeStamp Timestamp `json:"join_timestamp"`


Should probably stick to JoinTimestamp to match ArchiveTimestamp.

CarsonHoffman · 2021-08-09T00:26:41Z

structs.go

+	// Defaults to `PRIVATE_THREAD` in order to match the behavior
+	// when thread documentation was first published.


I'm not sure that this comment is really relevant here given that we don't specify omitempty; there's no way for the "default" behavior on the API side to take effect. Perhaps having omitempty might make sense?

CarsonHoffman · 2021-08-09T00:29:27Z

restapi.go

+// ActiveThreads returns all active threads in the channel
+// GET /channels/{channel.id}/threads/active
+func (s *Session) ActiveThreads(channelID string) (st *ThreadsResponseBody, err error) {
+	body, err := s.RequestWithBucketID("GET", EndpointChannelActiveThreads(channelID), nil, EndpointThreadMembers(""))
+	if err != nil {
+		return
+	}
+
+	err = unmarshal(body, &st)
+	return
+}


It looks like this endpoint is already deprecated. We might just want to move to the guild active threads endpoint to begin with; that one also has a more reasonable response structure, such that we could return a slice of threads and a slice of members rather than a custom struct specifically for one function.

Edit: it looks like we already have that one in this PR! I'd just remove this and ThreadsResponseBody, to be honest.

CarsonHoffman · 2021-08-09T00:30:26Z

structs.go

+	// Voice region ID for the voice channel, automatic when set to null
+	RTCRegion *string `json:"rtc_region"`


It looks like this field is replacing Region, so we should probably remove that one while we're at it.

CarsonHoffman · 2021-08-09T00:32:14Z

restapi.go

+// ListActiveThreads returns all active threads in the guild,
+// including public and private threads.
+// Threads are ordered by their `id`, in descending order.
+func (s *Session) ListActiveThreads(guildID string) (st *ListActiveThreadsResponseBody, err error) {


I would just return the threads and members as two return values, rather than having ListActiveThreadsResponseBody exported just for this function.

phenpessoa · 2021-08-09T01:06:59Z

@CarsonHoffman I think that is everything you requested.
Let me know if I missed anything or if I did something wrongly please.

CarsonHoffman · 2021-08-18T22:58:06Z

structs.go

+
+// Constants for the Video Quality Modes of a channel
+const (
+	VideoQualityModeAuto VideoQualityMode = iota + 1


This is small, but please avoid using iota for constants defined by Discord. We've run into many instances in the past where changes on Discord's end have gone awry doing this, especially when they add gaps between values (which is not uncommon). You'll notice that none of the other blocks of constants that you changed in this PR use iota, either.

CarsonHoffman · 2021-08-18T23:00:26Z

restapi.go

-	if threadID != "" {
-		v.Set("thread_id", threadID)
-	}


We still need to add the thread ID in, as it's not part of the body; it's a query parameter.

CarsonHoffman · 2021-08-18T23:01:02Z

webhook.go

@@ -35,6 +36,7 @@ type WebhookParams struct {
 	Components      []MessageComponent      `json:"components"`
 	Embeds          []*MessageEmbed         `json:"embeds,omitempty"`
 	AllowedMentions *MessageAllowedMentions `json:"allowed_mentions,omitempty"`
+	ThreadID        string                  `json:"thread_id,omitempty"`


Given that the thread ID isn't part of the body, we should just have `json:"-"` here.

CarsonHoffman · 2021-08-19T00:45:22Z

state.go

+		return err
+	}
+
+	if len(t.ChannelIDs) > 0 {


The docs note that channel_ids could be omitted, in which case all channels for the guild are being synced. Should we really be doing nothing and returning ErrStateNotFound if this is the case?

CarsonHoffman · 2021-08-19T00:48:09Z

state.go

+		defer s.Unlock()
+
+		for _, parentChannelID := range t.ChannelIDs {
+			for _, stateChannel := range s.channelMap {


This seems really nasty—it's O(nm), where n is the list of all channels the bot has access to and m is the number of channels being synced. e.g. if the bot can see 2000 guilds, each has 20 channels, and 10 channels were synced (these numbers seem somewhat reasonable for a larger bot), we just looped four hundred thousand times. Perhaps we could limit the search to only the indicated guild's channels? We're already looping through g.Channels below, and we can delete from channelMap without looping through it (since the keys match).

phenpessoa · 2021-08-19T02:12:50Z

Updated it @CarsonHoffman
Let me know what you think!

CarsonHoffman · 2021-08-19T03:47:05Z

state.go

+OUTER:
+	for _, c := range g.Channels {
+		if !c.IsThread() || c.ThreadMetadata.Archived {
+			g.Channels[index] = c
+			index++
+			continue OUTER
+		}
+
+		if len(t.ChannelIDs) == 0 {
+			delete(s.channelMap, c.ID)
+			continue OUTER
+		}
+
+		for _, parentChannelID := range t.ChannelIDs {
+			if c.ParentID == parentChannelID {
+				delete(s.channelMap, c.ID)
+				continue OUTER
+			}
+		}
+
+		g.Channels[index] = c
+		index++
+	}


A labeled loop with three different (and similar) continue points seems really difficult to reason about to me. I would suggest inverting the loop—first loop over the thread sync event's channels, and then do the guild channels in the inner loop, since you'll then be starting the guild channels loop anew each time and won't need to worry about mutating in place.

phenpessoa · 2021-08-19T16:08:46Z

Updated @CarsonHoffman

CarsonHoffman · 2021-08-19T20:10:51Z

state.go

+		}
+	}
+
+	return ErrStateNotFound


It seems to me like this function always returns ErrStateNotFound if it makes it past the initial s == nil check. I'd probably invert the if statement to if !ok to return early and keep the happy path left (in addition to above in ThreadMemberUpdate).

CarsonHoffman · 2021-08-19T20:49:05Z

restapi.go

+// ThreadMembers lists the members of a thread
+// GET /channels/{channel.id}/threads-members
+func (s *Session) ThreadMembers(channelID string) (st []*ThreadMember, err error) {
+	body, err := s.RequestWithBucketID("GET", EndpointThreadMembers(channelID), nil, EndpointThreadMembers(""))
+	if err != nil {
+		return
+	}
+
+	err = unmarshal(body, &st)
+	return
+}


It doesn't look like this function works at all:

panic: HTTP 404 Not Found, {"message": "404: Not Found", "code": 0}

It seems like it's calling to /thread-members/ rather than /thread-members. The fact that EndpointThreadMembers doesn't take in the user ID as a parameter seems like a bit of a smell. Having two separate endpoint functions and taking in the user ID as a parameter in one (and consequently updating JoinThread, AddUserToThread, etc. might be a good idea.

CarsonHoffman

I think that things are nearing ready to go here! I noticed that we seem to be limiting many of the thread API calls too strictly by not taking into account the channel ID as a major parameter, but otherwise things are looking great.

CarsonHoffman · 2021-08-21T00:56:30Z

restapi.go

+//
+// POST /channels/{channel.id}/messages/{message.id}/threads
+func (s *Session) StartThreadWithMessage(channelID, messageID string, data *ThreadCreateData) (st *Channel, err error) {
+	body, err := s.RequestWithBucketID("POST", EndpointChannelMessageThreads(channelID, messageID), data, EndpointChannelMessageThreads("", ""))


Given the following passage from the rate limits documentation:

Additionally, rate limits take into account major parameters in the URL. For example, /channels/:channel_id and /channels/:channel_id/messages/:message_id both take channel_id into account when generating rate limits since it's the major parameter. Currently, the only major parameters are channel_id, guild_id, and webhook_id + webhook_token.

It seems to me like the bucket ID should include the channel ID here, since each different channel will be in a different bucket. I think that this also applies to StartThreadWithoutMessage, JoinThread, AddUserToThread, LeaveThread, RemoveUserFromThread, ThreadMembers, PublicArchivedThreads, PrivateArchivedThreads, and JoinedPrivateArchivedThreads (ThreadEditComplex and ListActiveThreads already do this).

CarsonHoffman

I had another chance to look more closely at the code here, and I am really concerned about what I see. It appears that much of the gateway handling code that was introduced in this commit is copied from #982's implementation (it was committed on July 29th there and August 2nd here). I listed three examples in this review; it seems like you attempted to obfuscate this by removing comments and changing != to >, but the code retains almost the exact same structure, the same logic, and the same bugs as the implementation in #982. Are you claiming that the work in this PR is your own? If you were looking to credit the author of #982, the time was when you added that code, not after (and I'm still not sure that this would be an acceptable route given another active PR, but at least it's not direct stealing). I hope that I'm somehow misinterpreting the situation here, but taking code from others' active PRs and trying to represent it as your own is in no way acceptable behavior.

CarsonHoffman · 2021-08-21T15:11:13Z

state.go

+// ThreadMembersUpdate updates the member object of the thread.
+// If the user was removed,  the ThreadMember object in the state
+// will be set to nil.
+func (s *State) ThreadMembersUpdate(t *ThreadMembersUpdate) error {
+	if s == nil {
+		return ErrNilState
+	}
+
+	s.Lock()
+	defer s.Unlock()
+
+	thread, ok := s.channelMap[t.ID]
+	if ok {
+		for _, memberID := range t.RemovedMembersIDs {
+			if memberID == s.User.ID {
+				thread.Member = nil
+				break
+			}
+		}
+	}
+
+	return ErrStateNotFound
+}


This appears to be copied from https://github.com/bwmarrin/discordgo/pull/982/files/ecd5d0d76e298de23c533d7e4f1f8c026930fb07#diff-320959764bfa2277e0bdd4eaa741f02891d123c42150d4ccb76de0bdbf6e591aR582-R603, and retains the same issue of always returning ErrStateNotFound.

CarsonHoffman · 2021-08-21T15:11:52Z

state.go

+// ThreadMemberUpdate updates the ThreadMember object for the user,
+// in a specific thread.
+func (s *State) ThreadMemberUpdate(t *ThreadMember) error {
+	if s == nil {
+		return ErrNilState
+	}
+
+	channel, ok := s.channelMap[t.ID]
+	if ok {
+		s.Lock()
+		defer s.Unlock()
+
+		channel.Member = t
+		return nil
+	}
+
+	return ErrStateNotFound
+}


This appears to be copied from https://github.com/bwmarrin/discordgo/pull/982/files/ecd5d0d76e298de23c533d7e4f1f8c026930fb07#diff-320959764bfa2277e0bdd4eaa741f02891d123c42150d4ccb76de0bdbf6e591aR564-R580, and retains the same bug of accessing s.channelMap without any synchronization.

CarsonHoffman · 2021-08-21T15:14:41Z

state.go

+// ThreadListSync syncs new threads and threads member that
+// the bot gained access to.
+// https://discord.com/developers/docs/topics/gateway#thread-list-sync
+func (s *State) ThreadListSync(t *ThreadListSync) error {
+	if s == nil {
+		return ErrNilState
+	}
+
+	g, err := s.Guild(t.GuildID)
+	if err != nil {
+		return err
+	}
+
+	if len(t.ChannelIDs) > 0 {
+		s.Lock()
+		defer s.Unlock()
+
+		for _, parentChannelID := range t.ChannelIDs {
+			for _, stateChannel := range s.channelMap {
+				if stateChannel.IsThread() && stateChannel.ParentID == parentChannelID && !stateChannel.ThreadMetadata.Archived {
+					for i, c := range g.Channels {
+						if c.ID == stateChannel.ID {
+							g.Channels = append(g.Channels[:i], g.Channels[i+1:]...)
+							break
+						}
+					}
+					delete(s.channelMap, stateChannel.ID)
+				}
+			}
+		}
+
+		for _, channel := range t.Threads {
+			s.channelMap[channel.ID] = channel
+			g.Channels = append(g.Channels, channel)
+		}
+
+		for _, member := range t.Members {
+			channel, ok := s.channelMap[member.ID]
+			if ok {
+				channel.Member = member
+			}
+		}
+
+		return nil
+	}
+
+	return ErrStateNotFound
+}


This appears to be copied from https://github.com/bwmarrin/discordgo/pull/982/files/ecd5d0d76e298de23c533d7e4f1f8c026930fb07#diff-320959764bfa2277e0bdd4eaa741f02891d123c42150d4ccb76de0bdbf6e591aR605-R659, and retains the same very unusual structure of the nested loops (it's so non-straightforward that I cannot imagine two people coming to this independently), the same misunderstanding of what the case of len(t.ChannelIDs) == 0 means, and the same return of ErrStateNotFound when that error really doesn't make a ton of sense in this context.

phenpessoa · 2021-08-21T16:17:52Z

I had another chance to look more closely at the code here, and I am really concerned about what I see. It appears that much of the gateway handling code that was introduced in this commit is copied from #982's implementation (it was committed on July 29th there and August 2nd here). I listed three examples in this review; it seems like you attempted to obfuscate this by removing comments and changing != to >, but the code retains almost the exact same structure, the same logic, and the same bugs as the implementation in #982. Are you claiming that the work in this PR is your own? If you were looking to credit the author of #982, the time was when you added that code, not after (and I'm still not sure that this would be an acceptable route given another active PR, but at least it's not direct stealing). I hope that I'm somehow misinterpreting the situation here, but taking code from others' active PRs and trying to represent it as your own is in no way acceptable behavior.

It is definitely not a copied work. This PR was opened way before that other one. I did take a look at that, to see if I missed something. And I missed the state work and the new webhook type.
Still it doesn't mean I copied anything from that. We had similar approaches.
Yes, I misinterpreted the threadlistsync handler because of how that PR handled it. Should have referred to the docs instead of that PR to work on it.

I also should have mentioned that I added these changes because I saw that PR. I apologize for that, it was my bad.
But code is not a copy. And it is definitely not stealing.

There are bugs there that are not on this PR. And vice versa.

A lot of code there is extremely similar to code here too.
Even some comments there are the same as mine.

CarsonHoffman · 2021-08-21T17:48:30Z

I'm not sure that "we had similar approaches" really does a good job of explaining the situation here. Other than changing one-line map-access-in-if-statements to two-line ones (which consistently changed in all three of them), removing comments and using the IsThread method, the three functions are essentially exactly the same character-for-character (ThreadMembersUpdate and ThreadMemberUpdate are). These are not methods where there's one defined pattern of doing things in the library, like a REST API method might have, or property changes directly corresponding to Discord's objects; there are choices to be made in how things are done. They retain the same bugs in all cases (including a function always returning the wrong thing). They have the same nested loop structure with exactly the same variable names; stateChannel.ParentID == parentChannelID in line 625 means that this whole nested loop is essentially just the same as s.channelMap[parentChannelID], but with more unnecessary looping. Roundabout ways of doing things like this don't coincidentally match, especially considering the other two methods.

This PR was opened way before that other one.

Sure. I'm talking specifically about the state management code, which was added after that one's, as you acknowledge.

There are bugs there that are not on this PR.

Not in the state management code.

And vice versa.

Sure, but I'm only referring to the state management code.

Even some comments there are the same as mine.

I fail to see anything significant that isn't either a natural way of describing things or from Discord's documentation.

I unfortunately see no path forward here. I cannot in good conscience accept code that began its journey as copied code, even if it is no longer directly in the final PR. It would be crediting you with someone else's work. I'd recommend really trying to reflect on this and the impact it might have on others if you're planning on contributing to other open-source projects in the future.

inital work on threads

c88e204

Strum355 mentioned this pull request May 3, 2021

Upgrade the API version and implement threads #917

Closed

phenpessoa added 7 commits May 3, 2021 10:10

golint

6a16c9e

additional comment to the threads field on the guild structure

d33dfbf

add errors codes

22f6ffb

add missing message flags

a23749d

add endpoints

91e8403

add restapi functions

a4413fe

golint

e96672c

update func names to be in sync with updated discord docs

c04bd96

optional threadID for WebhookExecute

c6fabbf

phenpessoa added 2 commits May 16, 2021 14:03

fix LeaveThread description

de9d0aa

fix RemoveUserFromThread description

331fa57

connorkuehl mentioned this pull request Jul 28, 2021

Thread support connorkuehl/popple#53

Closed

2 tasks

phenpessoa added 3 commits August 2, 2021 17:42

Merge remote-tracking branch 'upstream/master' into threads

c8b6185

latest threads updates

000fbde

state and archive duration update

d890873

phenpessoa marked this pull request as ready for review August 2, 2021 22:18

kovetskiy reviewed Aug 3, 2021

View reviewed changes

add member on ThreadMembersUpdate

d01681d

phenpessoa mentioned this pull request Aug 3, 2021

threads botlabs-gg/discordgo#14

Closed

CarsonHoffman requested changes Aug 9, 2021

View reviewed changes

CarsonHoffman reviewed Aug 9, 2021

View reviewed changes

requested changes

f209ccc

phenpessoa added 3 commits August 9, 2021 01:41

fix threadmembersupdate

3ac6a06

Merge remote-tracking branch 'upstream/master' into threads

1c38a05

update webhooks

adaed53

CarsonHoffman requested changes Aug 18, 2021

View reviewed changes

updates

dd745c6

CarsonHoffman reviewed Aug 19, 2021

View reviewed changes

reduce amount of loops

265bbf5

CarsonHoffman requested changes Aug 19, 2021

View reviewed changes

make code more readable

b319baf

CarsonHoffman requested changes Aug 19, 2021

View reviewed changes

better write threadmember and threadmembers update

c00b71f

CarsonHoffman requested changes Aug 19, 2021

View reviewed changes

requested changes

a352138

phenpessoa closed this Aug 20, 2021

phenpessoa reopened this Aug 20, 2021

CarsonHoffman requested changes Aug 21, 2021

View reviewed changes

CarsonHoffman reviewed Aug 21, 2021

View reviewed changes

CarsonHoffman closed this Aug 21, 2021

phenpessoa deleted the threads branch August 21, 2021 20:59

vys534 added a commit to vys534/discordgo that referenced this pull request Aug 24, 2021

Integrated bug fixes and additions from bwmarrin#922

8574857

vys534 mentioned this pull request Aug 24, 2021

v8 & v9 stuff #982

Closed

cheesycod added a commit to selectlist-archive/discordgo that referenced this pull request Dec 9, 2021

integrate pr Integrated bug fixes and additions from bwmarrin#922

3f54842

FedorLap2006 mentioned this pull request Dec 18, 2021

Threads reloaded #1058

Merged

5 tasks

		// Defaults to `PRIVATE_THREAD` in order to match the behavior
		// when thread documentation was first published.

		// Voice region ID for the voice channel, automatic when set to null
		RTCRegion *string `json:"rtc_region"`

Threads #922

Threads #922

Conversation

phenpessoa commented May 3, 2021 • edited

phenpessoa commented May 3, 2021

phenpessoa commented May 4, 2021

phenpessoa commented May 11, 2021

Breaking change!

belak commented Jul 27, 2021

phenpessoa commented Jul 27, 2021

phenpessoa commented Aug 2, 2021

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

CarsonHoffman Aug 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phenpessoa commented Aug 9, 2021

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

CarsonHoffman Aug 19, 2021 • edited

Choose a reason for hiding this comment

phenpessoa commented Aug 19, 2021

Choose a reason for hiding this comment

phenpessoa commented Aug 19, 2021

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

CarsonHoffman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarsonHoffman left a 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

phenpessoa commented Aug 21, 2021

CarsonHoffman commented Aug 21, 2021

phenpessoa commented May 3, 2021 •

edited

CarsonHoffman Aug 9, 2021 •

edited

CarsonHoffman Aug 19, 2021 •

edited