From 0f21cfcd52f30d4d9e72f2f30a1f5d397b5f9bf4 Mon Sep 17 00:00:00 2001 From: Tim Scheuermann Date: Mon, 11 Jul 2022 16:04:07 +0300 Subject: [PATCH] General GraphQL resolver optimizations (#20611) --- api4/resolver_channel.go | 36 ++++++++++++++++++++--------------- api4/resolver_channel_test.go | 35 +++++++++++++++++++++++++++++----- api4/resolver_team.go | 12 ++++++------ 3 files changed, 57 insertions(+), 26 deletions(-) diff --git a/api4/resolver_channel.go b/api4/resolver_channel.go index c44e02931a..04f926b894 100644 --- a/api4/resolver_channel.go +++ b/api4/resolver_channel.go @@ -71,16 +71,16 @@ func parseChannelCursor(cursor string) (channelID string, ok bool) { return "", false } - parts := strings.Split(string(decoded), "-") - if len(parts) != 2 { + prefix, id, found := strings.Cut(string(decoded), "-") + if !found { return "", false } - if cursorPrefix(parts[0]) != channelCursorPrefix { + if cursorPrefix(prefix) != channelCursorPrefix { return "", false } - return parts[1], true + return id, true } func postProcessChannels(c *web.Context, channels []*model.Channel) ([]*channel, error) { @@ -111,8 +111,9 @@ func postProcessChannels(c *web.Context, channels []*model.Channel) ([]*channel, } // Convert to the wrapper format. - res := make([]*channel, 0, len(channels)) - for _, ch := range channels { + nameCache := make(map[string]string) + res := make([]*channel, len(channels)) + for i, ch := range channels { prettyName := ch.DisplayName if ch.IsGroupOrDirect() { @@ -121,37 +122,39 @@ func postProcessChannels(c *web.Context, channels []*model.Channel) ([]*channel, if users == nil { return nil, fmt.Errorf("user info not found for channel id: %s", ch.Id) } - prettyName = getPrettyDNForUsers(nameFormat, users, c.AppContext.Session().UserId) + prettyName = getPrettyDNForUsers(nameFormat, users, c.AppContext.Session().UserId, nameCache) } - res = append(res, &channel{Channel: *ch, PrettyDisplayName: prettyName}) + res[i] = &channel{Channel: *ch, PrettyDisplayName: prettyName} } return res, nil } -func getPrettyDNForUsers(displaySetting string, users []*model.User, omitUserId string) string { +func getPrettyDNForUsers(displaySetting string, users []*model.User, omitUserId string, cache map[string]string) string { displayNames := make([]string, 0, len(users)) - // TODO: optimize this logic. - // Name computation happens repeatedly for the same user from - // multiple channels. for _, u := range users { if u.Id == omitUserId { continue } - displayNames = append(displayNames, getPrettyDNForUser(displaySetting, u)) + displayNames = append(displayNames, getPrettyDNForUser(displaySetting, u, cache)) } sort.Strings(displayNames) result := strings.Join(displayNames, ", ") if result == "" { // Self DM - result = getPrettyDNForUser(displaySetting, users[0]) + result = getPrettyDNForUser(displaySetting, users[0], cache) } return result } -func getPrettyDNForUser(displaySetting string, user *model.User) string { +func getPrettyDNForUser(displaySetting string, user *model.User, cache map[string]string) string { + // use the cache first + if name, ok := cache[user.Id]; ok { + return name + } + var displayName string switch displaySetting { case "nickname_full_name": @@ -171,5 +174,8 @@ func getPrettyDNForUser(displaySetting string, user *model.User) string { displayName = user.Username } + // update the cache + cache[user.Id] = displayName + return displayName } diff --git a/api4/resolver_channel_test.go b/api4/resolver_channel_test.go index 6fd543bba1..11f1a7f11e 100644 --- a/api4/resolver_channel_test.go +++ b/api4/resolver_channel_test.go @@ -440,7 +440,7 @@ func TestGetPrettyDNForUsers(t *testing.T) { LastName: "last2", }, } - assert.Equal(t, "nick2", getPrettyDNForUsers("nickname_full_name", users, "user1")) + assert.Equal(t, "nick2", getPrettyDNForUsers("nickname_full_name", users, "user1", map[string]string{})) users = []*model.User{ { @@ -456,7 +456,7 @@ func TestGetPrettyDNForUsers(t *testing.T) { LastName: "last2", }, } - assert.Equal(t, "first2 last2", getPrettyDNForUsers("nickname_full_name", users, "user1")) + assert.Equal(t, "first2 last2", getPrettyDNForUsers("nickname_full_name", users, "user1", map[string]string{})) }) t.Run("full_name", func(t *testing.T) { @@ -476,7 +476,7 @@ func TestGetPrettyDNForUsers(t *testing.T) { LastName: "last2", }, } - assert.Equal(t, "first2 last2", getPrettyDNForUsers("full_name", users, "user1")) + assert.Equal(t, "first2 last2", getPrettyDNForUsers("full_name", users, "user1", map[string]string{})) users = []*model.User{ { @@ -488,7 +488,7 @@ func TestGetPrettyDNForUsers(t *testing.T) { Username: "user2", }, } - assert.Equal(t, "user2", getPrettyDNForUsers("full_name", users, "user1")) + assert.Equal(t, "user2", getPrettyDNForUsers("full_name", users, "user1", map[string]string{})) }) t.Run("username", func(t *testing.T) { @@ -508,8 +508,33 @@ func TestGetPrettyDNForUsers(t *testing.T) { LastName: "last2", }, } - assert.Equal(t, "user2", getPrettyDNForUsers("username", users, "user1")) + assert.Equal(t, "user2", getPrettyDNForUsers("username", users, "user1", map[string]string{})) }) + + t.Run("cache", func(t *testing.T) { + users := []*model.User{ + { + Id: "user1", + Nickname: "nick1", + Username: "user1", + FirstName: "first1", + LastName: "last1", + }, + { + Id: "user2", + Nickname: "nick2", + Username: "user2", + FirstName: "first2", + LastName: "last2", + }, + } + + cache := map[string]string{} + assert.Equal(t, "first2 last2", getPrettyDNForUsers("full_name", users, "user1", cache)) + cache["user2"] = "teststring!!" + assert.Equal(t, "teststring!!", getPrettyDNForUsers("full_name", users, "user1", cache)) + }) + } func TestChannelCursor(t *testing.T) { diff --git a/api4/resolver_team.go b/api4/resolver_team.go index de122807de..9dd864f6f0 100644 --- a/api4/resolver_team.go +++ b/api4/resolver_team.go @@ -61,23 +61,23 @@ func getGraphQLTeams(c *web.Context, teamIDs []string) ([]*model.Team, error) { } if len(teams) != len(teamIDs) { - return nil, fmt.Errorf("All teams were not found. Requested %d; Found %d", len(teamIDs), len(teams)) + return nil, fmt.Errorf("all teams were not found. Requested %d; Found %d", len(teamIDs), len(teams)) } // We pre-calculate this so that it's not computed in separate goroutines outside // the dataloader. - for i := range teams { - if (!teams[i].AllowOpenInvite || teams[i].Type != model.TeamOpen) && - !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), teams[i].Id, model.PermissionViewTeam) { + for i, team := range teams { + if (!team.AllowOpenInvite || team.Type != model.TeamOpen) && + !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), team.Id, model.PermissionViewTeam) { c.SetPermissionError(model.PermissionViewTeam) return nil, c.Err } - teams[i] = c.App.SanitizeTeam(*c.AppContext.Session(), teams[i]) + teams[i] = c.App.SanitizeTeam(*c.AppContext.Session(), team) } // The teams need to be in the exact same order as the input slice. - tmp := make(map[string]*model.Team) + tmp := make(map[string]*model.Team, len(teams)) for _, ch := range teams { tmp[ch.Id] = ch }