From 217cdf447a995fd8f2700b14ba790360ccaeabf6 Mon Sep 17 00:00:00 2001 From: Joram Wilander Date: Fri, 23 Dec 2016 10:32:30 -0500 Subject: PLT-5073 Improve performance of /channels/view endpoint (#4881) * Improve performance of /channels/view endpoint * Fix store unit test --- api/channel.go | 71 ++++++++++++++++++++--------------------- api/channel_test.go | 4 +-- api/deprecated.go | 2 +- api/post.go | 2 +- model/channel_view.go | 1 - model/client.go | 5 ++- model/status.go | 2 +- store/sql_channel_store.go | 25 +++++++++++---- store/sql_channel_store_test.go | 4 +-- store/sql_upgrade.go | 3 ++ store/store.go | 2 +- webapp/client/client.jsx | 2 +- 12 files changed, 67 insertions(+), 56 deletions(-) diff --git a/api/channel.go b/api/channel.go index 44efc5efb..9c2baf50c 100644 --- a/api/channel.go +++ b/api/channel.go @@ -919,10 +919,6 @@ func SetActiveChannel(userId string, channelId string) *model.AppError { AddStatusCache(status) - if result := <-Srv.Store.Status().SaveOrUpdate(status); result.Err != nil { - return result.Err - } - return nil } @@ -1067,7 +1063,7 @@ func addMember(c *Context, w http.ResponseWriter, r *http.Request) { go PostUserAddRemoveMessage(c, channel.Id, fmt.Sprintf(utils.T("api.channel.add_member.added"), nUser.Username, oUser.Username), model.POST_ADD_REMOVE) - <-Srv.Store.Channel().UpdateLastViewedAt(id, oUser.Id) + <-Srv.Store.Channel().UpdateLastViewedAt([]string{id}, oUser.Id) w.Write([]byte(cm.ToJson())) } } @@ -1258,47 +1254,48 @@ func autocompleteChannels(c *Context, w http.ResponseWriter, r *http.Request) { func viewChannel(c *Context, w http.ResponseWriter, r *http.Request) { view := model.ChannelViewFromJson(r.Body) - go func() { - if err := SetActiveChannel(c.Session.UserId, view.ChannelId); err != nil { - l4g.Error(err.Error()) - } - }() + if err := SetActiveChannel(c.Session.UserId, view.ChannelId); err != nil { + c.Err = err + return + } - if len(view.ChannelId) > 0 { + if len(view.ChannelId) == 0 { + ReturnStatusOK(w) + return + } - if view.Time == 0 { - if result := <-Srv.Store.Channel().UpdateLastViewedAt(view.ChannelId, c.Session.UserId); result.Err != nil { - c.Err = result.Err - return - } - } else { - if result := <-Srv.Store.Channel().SetLastViewedAt(view.ChannelId, c.Session.UserId, view.Time); result.Err != nil { - c.Err = result.Err - return - } + channelIds := []string{view.ChannelId} + + var pchan store.StoreChannel + if len(view.PrevChannelId) > 0 { + channelIds = append(channelIds, view.PrevChannelId) + + if *utils.Cfg.EmailSettings.SendPushNotifications && !c.Session.IsMobileApp() { + pchan = Srv.Store.User().GetUnreadCountForChannel(c.Session.UserId, view.ChannelId) } + } + + uchan := Srv.Store.Channel().UpdateLastViewedAt(channelIds, c.Session.UserId) - if len(view.PrevChannelId) > 0 { - Srv.Store.Channel().UpdateLastViewedAt(view.PrevChannelId, c.Session.UserId) - - // Only clear push notifications if a channel switch occured - if *utils.Cfg.EmailSettings.SendPushNotifications && !c.Session.IsMobileApp() { - go func() { - if result := <-Srv.Store.User().GetUnreadCountForChannel(c.Session.UserId, view.ChannelId); result.Err != nil { - l4g.Error(utils.T("api.channel.update_last_viewed_at.get_unread_count_for_channel.error"), c.Session.UserId, view.ChannelId, result.Err.Error()) - } else { - if result.Data.(int64) > 0 { - clearPushNotification(c.Session.UserId, view.ChannelId) - } - } - }() + if pchan != nil { + if result := <-pchan; result.Err != nil { + c.Err = result.Err + return + } else { + if result.Data.(int64) > 0 { + clearPushNotification(c.Session.UserId, view.ChannelId) } } + } - message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_CHANNEL_VIEWED, c.TeamId, "", c.Session.UserId, nil) - message.Add("channel_id", view.ChannelId) + if result := <-uchan; result.Err != nil { + c.Err = result.Err + return } + message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_CHANNEL_VIEWED, c.TeamId, "", c.Session.UserId, nil) + message.Add("channel_id", view.ChannelId) + ReturnStatusOK(w) } diff --git a/api/channel_test.go b/api/channel_test.go index dae4feab6..25fd885ca 100644 --- a/api/channel_test.go +++ b/api/channel_test.go @@ -1862,14 +1862,12 @@ func TestViewChannel(t *testing.T) { } view.PrevChannelId = "" - view.Time = 1234567890 if _, resp := Client.ViewChannel(view); resp.Error != nil { t.Fatal(resp.Error) } view.PrevChannelId = "junk" - view.Time = 0 if _, resp := Client.ViewChannel(view); resp.Error != nil { t.Fatal(resp.Error) @@ -1878,6 +1876,8 @@ func TestViewChannel(t *testing.T) { rdata := Client.Must(Client.GetChannel(th.BasicChannel.Id, "")).Data.(*model.ChannelData) if rdata.Channel.TotalMsgCount != rdata.Member.MsgCount { + t.Log(rdata.Channel.Id) + t.Log(rdata.Member.UserId) t.Log(rdata.Channel.TotalMsgCount) t.Log(rdata.Member.MsgCount) t.Fatal("message counts don't match") diff --git a/api/deprecated.go b/api/deprecated.go index 183552414..4865ab5e0 100644 --- a/api/deprecated.go +++ b/api/deprecated.go @@ -76,7 +76,7 @@ func updateLastViewedAt(c *Context, w http.ResponseWriter, r *http.Request) { } }() - Srv.Store.Channel().UpdateLastViewedAt(id, c.Session.UserId) + Srv.Store.Channel().UpdateLastViewedAt([]string{id}, c.Session.UserId) // Must be after update so that unread count is correct if doClearPush { diff --git a/api/post.go b/api/post.go index acbd0acd7..5cce766c0 100644 --- a/api/post.go +++ b/api/post.go @@ -95,7 +95,7 @@ func createPost(c *Context, w http.ResponseWriter, r *http.Request) { } else { // Update the LastViewAt only if the post does not have from_webhook prop set (eg. Zapier app) if _, ok := post.Props["from_webhook"]; !ok { - if result := <-Srv.Store.Channel().UpdateLastViewedAt(post.ChannelId, c.Session.UserId); result.Err != nil { + if result := <-Srv.Store.Channel().UpdateLastViewedAt([]string{post.ChannelId}, c.Session.UserId); result.Err != nil { l4g.Error(utils.T("api.post.create_post.last_viewed.error"), post.ChannelId, c.Session.UserId, result.Err) } } diff --git a/model/channel_view.go b/model/channel_view.go index 9803c4bbc..8be7af175 100644 --- a/model/channel_view.go +++ b/model/channel_view.go @@ -11,7 +11,6 @@ import ( type ChannelView struct { ChannelId string `json:"channel_id"` PrevChannelId string `json:"prev_channel_id"` - Time int64 `json:"time"` } func (o *ChannelView) ToJson() string { diff --git a/model/client.go b/model/client.go index 1f87c619e..4a6cb169f 100644 --- a/model/client.go +++ b/model/client.go @@ -1350,10 +1350,9 @@ func (c *Client) UpdateLastViewedAt(channelId string, active bool) (*Result, *Ap } // ViewChannel performs all the actions related to viewing a channel. This includes marking -// the channel and the previous one as read, marking the channel as being actively viewed. +// the channel and the previous one as read, and marking the channel as being actively viewed. // ChannelId is required but may be blank to indicate no channel is being viewed. -// PrevChannelId is optional, populate to indicate a channel switch occurred. Optionally -// provide a non-zero Time, in Unix milliseconds, to manually set the viewing time. +// PrevChannelId is optional, populate to indicate a channel switch occurred. func (c *Client) ViewChannel(params ChannelView) (bool, *ResponseMetadata) { if r, err := c.DoApiPost(c.GetTeamRoute()+"/channels/view", params.ToJson()); err != nil { return false, &ResponseMetadata{StatusCode: r.StatusCode, Error: err} diff --git a/model/status.go b/model/status.go index 324866427..8637f60a3 100644 --- a/model/status.go +++ b/model/status.go @@ -22,7 +22,7 @@ type Status struct { Status string `json:"status"` Manual bool `json:"manual"` LastActivityAt int64 `json:"last_activity_at"` - ActiveChannel string `json:"active_channel"` + ActiveChannel string `json:"active_channel" db:"-"` } func (o *Status) ToJson() string { diff --git a/store/sql_channel_store.go b/store/sql_channel_store.go index a961781dd..47c172f77 100644 --- a/store/sql_channel_store.go +++ b/store/sql_channel_store.go @@ -6,6 +6,7 @@ package store import ( "database/sql" "fmt" + "strconv" "strings" l4g "github.com/alecthomas/log4go" @@ -13,7 +14,6 @@ import ( "github.com/mattermost/platform/einterfaces" "github.com/mattermost/platform/model" "github.com/mattermost/platform/utils" - "strconv" ) const ( @@ -961,13 +961,24 @@ func (s SqlChannelStore) SetLastViewedAt(channelId string, userId string, newLas return storeChannel } -func (s SqlChannelStore) UpdateLastViewedAt(channelId string, userId string) StoreChannel { +func (s SqlChannelStore) UpdateLastViewedAt(channelIds []string, userId string) StoreChannel { storeChannel := make(StoreChannel, 1) go func() { result := StoreResult{} var query string + props := make(map[string]interface{}) + + idQuery := "" + for index, channelId := range channelIds { + if len(idQuery) > 0 { + idQuery += " OR " + } + + props["channelId"+strconv.Itoa(index)] = channelId + idQuery += "ChannelId = :channelId" + strconv.Itoa(index) + } if utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_POSTGRES { query = `UPDATE @@ -982,7 +993,7 @@ func (s SqlChannelStore) UpdateLastViewedAt(channelId string, userId string) Sto WHERE Channels.Id = ChannelMembers.ChannelId AND UserId = :UserId - AND ChannelId = :ChannelId` + AND (` + idQuery + `)` } else if utils.Cfg.SqlSettings.DriverName == model.DATABASE_DRIVER_MYSQL { query = `UPDATE ChannelMembers, Channels @@ -994,12 +1005,14 @@ func (s SqlChannelStore) UpdateLastViewedAt(channelId string, userId string) Sto WHERE Channels.Id = ChannelMembers.ChannelId AND UserId = :UserId - AND ChannelId = :ChannelId` + AND (` + idQuery + `)` } - _, err := s.GetMaster().Exec(query, map[string]interface{}{"ChannelId": channelId, "UserId": userId}) + props["UserId"] = userId + + _, err := s.GetMaster().Exec(query, props) if err != nil { - result.Err = model.NewLocAppError("SqlChannelStore.UpdateLastViewedAt", "store.sql_channel.update_last_viewed_at.app_error", nil, "channel_id="+channelId+", user_id="+userId+", "+err.Error()) + result.Err = model.NewLocAppError("SqlChannelStore.UpdateLastViewedAt", "store.sql_channel.update_last_viewed_at.app_error", nil, "channel_ids="+strings.Join(channelIds, ",")+", user_id="+userId+", "+err.Error()) } storeChannel <- result diff --git a/store/sql_channel_store_test.go b/store/sql_channel_store_test.go index b2a644c15..64e44cbb3 100644 --- a/store/sql_channel_store_test.go +++ b/store/sql_channel_store_test.go @@ -808,12 +808,12 @@ func TestChannelStoreUpdateLastViewedAt(t *testing.T) { m1.NotifyProps = model.GetDefaultChannelNotifyProps() Must(store.Channel().SaveMember(&m1)) - err := (<-store.Channel().UpdateLastViewedAt(m1.ChannelId, m1.UserId)).Err + err := (<-store.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, m1.UserId)).Err if err != nil { t.Fatal("failed to update", err) } - err = (<-store.Channel().UpdateLastViewedAt(m1.ChannelId, "missing id")).Err + err = (<-store.Channel().UpdateLastViewedAt([]string{m1.ChannelId}, "missing id")).Err if err != nil { t.Fatal("failed to update") } diff --git a/store/sql_upgrade.go b/store/sql_upgrade.go index a275f664c..4c4d83504 100644 --- a/store/sql_upgrade.go +++ b/store/sql_upgrade.go @@ -225,6 +225,9 @@ func UpgradeDatabaseToVersion36(sqlStore *SqlStore) { // Add a Position column to users. sqlStore.CreateColumnIfNotExists("Users", "Position", "varchar(64)", "varchar(64)", "") + // Remove ActiveChannel column from Status + sqlStore.RemoveColumnIfExists("Status", "ActiveChannel") + //saveSchemaVersion(sqlStore, VERSION_3_6_0) //} } diff --git a/store/store.go b/store/store.go index 8521713d5..2f5065b88 100644 --- a/store/store.go +++ b/store/store.go @@ -110,7 +110,7 @@ type ChannelStore interface { GetMemberCount(channelId string, allowFromCache bool) StoreChannel RemoveMember(channelId string, userId string) StoreChannel PermanentDeleteMembersByUser(userId string) StoreChannel - UpdateLastViewedAt(channelId string, userId string) StoreChannel + UpdateLastViewedAt(channelIds []string, userId string) StoreChannel SetLastViewedAt(channelId string, userId string, newLastViewedAt int64) StoreChannel IncrementMentionCount(channelId string, userId string) StoreChannel AnalyticsTypeCount(teamId string, channelType string) StoreChannel diff --git a/webapp/client/client.jsx b/webapp/client/client.jsx index 782b74571..0c4e04524 100644 --- a/webapp/client/client.jsx +++ b/webapp/client/client.jsx @@ -1397,7 +1397,7 @@ export default class Client { end(this.handleResponse.bind(this, 'updateLastViewedAt', success, error)); } - // SCHEDULED FOR DEPRECATION IN 3.8 - use viewChannel instead + // SCHEDULED FOR DEPRECATION IN 3.8 setLastViewedAt(channelId, lastViewedAt, success, error) { request. post(`${this.getChannelNeededRoute(channelId)}/set_last_viewed_at`). -- cgit v1.2.3-1-g7c22