From 3f97e06bf80c789ce8e2d572afdac65a73d417c8 Mon Sep 17 00:00:00 2001 From: Christopher Speller Date: Wed, 29 Jul 2015 11:30:56 -0400 Subject: Adding etag to channel extra_info api call. --- api/channel.go | 8 ++++- api/channel_benchmark_test.go | 2 +- api/channel_test.go | 68 +++++++++++++++++++++++++++++++++++++-- model/channel.go | 12 ++++++- model/client.go | 4 +-- store/sql_channel_store.go | 71 +++++++++++++++++++++++++++++++++-------- store/sql_channel_store_test.go | 34 ++++++++++++++++++-- 7 files changed, 176 insertions(+), 23 deletions(-) diff --git a/api/channel.go b/api/channel.go index 123fd8a35..9216dbb09 100644 --- a/api/channel.go +++ b/api/channel.go @@ -570,6 +570,11 @@ func getChannelExtraInfo(c *Context, w http.ResponseWriter, r *http.Request) { channel := cresult.Data.(*model.Channel) member := cmresult.Data.(model.ChannelMember) extraMembers := ecmresult.Data.([]model.ExtraMember) + extraEtag := channel.ExtraEtag() + + if HandleEtag(extraEtag, w, r) { + return + } if !c.HasPermissionsToTeam(channel.TeamId, "getChannelExtraInfo") { return @@ -586,6 +591,7 @@ func getChannelExtraInfo(c *Context, w http.ResponseWriter, r *http.Request) { } data := model.ChannelExtra{Id: channel.Id, Members: extraMembers} + w.Header().Set(model.HEADER_ETAG_SERVER, extraEtag) w.Header().Set("Expires", "-1") w.Write([]byte(data.ToJson())) } @@ -711,7 +717,7 @@ func removeChannelMember(c *Context, w http.ResponseWriter, r *http.Request) { } message := model.NewMessage(c.Session.TeamId, "", userId, model.ACTION_USER_REMOVED) - message.Add("channel_id",id) + message.Add("channel_id", id) message.Add("remover", c.Session.UserId) PublishAndForget(message) diff --git a/api/channel_benchmark_test.go b/api/channel_benchmark_test.go index 881638176..77e679c14 100644 --- a/api/channel_benchmark_test.go +++ b/api/channel_benchmark_test.go @@ -189,7 +189,7 @@ func BenchmarkGetChannelExtraInfo(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { for j := range channels { - Client.Must(Client.GetChannelExtraInfo(channels[j].Id)) + Client.Must(Client.GetChannelExtraInfo(channels[j].Id, "")) } } } diff --git a/api/channel_test.go b/api/channel_test.go index a8d53c4b5..d4fb11bd8 100644 --- a/api/channel_test.go +++ b/api/channel_test.go @@ -6,6 +6,7 @@ package api import ( "github.com/mattermost/platform/model" "github.com/mattermost/platform/store" + "github.com/mattermost/platform/utils" "net/http" "testing" "time" @@ -543,10 +544,73 @@ func TestGetChannelExtraInfo(t *testing.T) { channel1 := &model.Channel{DisplayName: "A Test API Name", Name: "a" + model.NewId() + "a", Type: model.CHANNEL_OPEN, TeamId: team.Id} channel1 = Client.Must(Client.CreateChannel(channel1)).Data.(*model.Channel) - rget := Client.Must(Client.GetChannelExtraInfo(channel1.Id)).Data.(*model.ChannelExtra) - if rget.Id != channel1.Id { + rget := Client.Must(Client.GetChannelExtraInfo(channel1.Id, "")) + data := rget.Data.(*model.ChannelExtra) + if data.Id != channel1.Id { t.Fatal("couldnt't get extra info") } + + // + // Testing etag caching + // + + currentEtag := rget.Etag + + if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil { + t.Fatal(err) + } else if cache_result.Data.(*model.ChannelExtra) != nil { + t.Log(cache_result.Data) + t.Fatal("response should be empty") + } else { + currentEtag = cache_result.Etag + } + + Client2 := model.NewClient("http://localhost:" + utils.Cfg.ServiceSettings.Port + "/api/v1") + + user2 := &model.User{TeamId: team.Id, Email: model.NewId() + "tester2@test.com", Nickname: "Tester 2", Password: "pwd"} + user2 = Client2.Must(Client2.CreateUser(user2, "")).Data.(*model.User) + store.Must(Srv.Store.User().VerifyEmail(user2.Id)) + + Client2.LoginByEmail(team.Name, user2.Email, "pwd") + Client2.Must(Client2.JoinChannel(channel1.Id)) + + if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil { + t.Fatal(err) + } else if cache_result.Data.(*model.ChannelExtra) == nil { + t.Log(cache_result.Data) + t.Fatal("response should not be empty") + } else { + currentEtag = cache_result.Etag + } + + if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil { + t.Fatal(err) + } else if cache_result.Data.(*model.ChannelExtra) != nil { + t.Log(cache_result.Data) + t.Fatal("response should be empty") + } else { + currentEtag = cache_result.Etag + } + + Client2.Must(Client2.LeaveChannel(channel1.Id)) + + if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil { + t.Fatal(err) + } else if cache_result.Data.(*model.ChannelExtra) == nil { + t.Log(cache_result.Data) + t.Fatal("response should not be empty") + } else { + currentEtag = cache_result.Etag + } + + if cache_result, err := Client.GetChannelExtraInfo(channel1.Id, currentEtag); err != nil { + t.Fatal(err) + } else if cache_result.Data.(*model.ChannelExtra) != nil { + t.Log(cache_result.Data) + t.Fatal("response should be empty") + } else { + currentEtag = cache_result.Etag + } } func TestAddChannelMember(t *testing.T) { diff --git a/model/channel.go b/model/channel.go index ab36d46a2..94b8fdb43 100644 --- a/model/channel.go +++ b/model/channel.go @@ -27,6 +27,7 @@ type Channel struct { Description string `json:"description"` LastPostAt int64 `json:"last_post_at"` TotalMsgCount int64 `json:"total_msg_count"` + ExtraUpdateAt int64 `json:"extra_update_at"` } func (o *Channel) ToJson() string { @@ -50,7 +51,11 @@ func ChannelFromJson(data io.Reader) *Channel { } func (o *Channel) Etag() string { - return Etag(o.Id, o.LastPostAt) + return Etag(o.Id, o.UpdateAt) +} + +func (o *Channel) ExtraEtag() string { + return Etag(o.Id, o.ExtraUpdateAt) } func (o *Channel) IsValid() *AppError { @@ -97,8 +102,13 @@ func (o *Channel) PreSave() { o.CreateAt = GetMillis() o.UpdateAt = o.CreateAt + o.ExtraUpdateAt = o.CreateAt } func (o *Channel) PreUpdate() { o.UpdateAt = GetMillis() } + +func (o *Channel) ExtraUpdated() { + o.ExtraUpdateAt = GetMillis() +} diff --git a/model/client.go b/model/client.go index 17bb898ca..a5016fa2c 100644 --- a/model/client.go +++ b/model/client.go @@ -457,8 +457,8 @@ func (c *Client) UpdateLastViewedAt(channelId string) (*Result, *AppError) { } } -func (c *Client) GetChannelExtraInfo(id string) (*Result, *AppError) { - if r, err := c.DoGet("/channels/"+id+"/extra_info", "", ""); err != nil { +func (c *Client) GetChannelExtraInfo(id string, etag string) (*Result, *AppError) { + if r, err := c.DoGet("/channels/"+id+"/extra_info", "", etag); err != nil { return nil, err } else { return &Result{r.Header.Get(HEADER_REQUEST_ID), diff --git a/store/sql_channel_store.go b/store/sql_channel_store.go index ce1a8c6fa..cac5c681b 100644 --- a/store/sql_channel_store.go +++ b/store/sql_channel_store.go @@ -36,6 +36,7 @@ func NewSqlChannelStore(sqlStore *SqlStore) ChannelStore { } func (s SqlChannelStore) UpgradeSchemaIfNeeded() { + s.CreateColumnIfNotExists("Channels", "ExtraUpdateAt", "TotalMsgCount", "bigint(20)", "0") } func (s SqlChannelStore) CreateIndexesIfNotExists() { @@ -142,6 +143,25 @@ func (s SqlChannelStore) Update(channel *model.Channel) StoreChannel { return storeChannel } +func (s SqlChannelStore) extraUpdated(channel *model.Channel) StoreChannel { + storeChannel := make(StoreChannel) + + go func() { + result := StoreResult{} + + channel.ExtraUpdated() + + if count, err := s.GetMaster().Update(channel); err != nil || count != 1 { + result.Err = model.NewAppError("SqlChannelStore.extraUpdated", "Problem updating members last updated time", "id="+channel.Id+", "+err.Error()) + } + + storeChannel <- result + close(storeChannel) + }() + + return storeChannel +} + func (s SqlChannelStore) Get(id string) StoreChannel { storeChannel := make(StoreChannel) @@ -288,20 +308,31 @@ func (s SqlChannelStore) SaveMember(member *model.ChannelMember) StoreChannel { go func() { result := StoreResult{} - member.PreSave() - if result.Err = member.IsValid(); result.Err != nil { - storeChannel <- result - return - } + // Grab the channel we are saving this member to + if cr := <-s.Get(member.ChannelId); cr.Err != nil { + result.Err = cr.Err + } else { + channel := cr.Data.(*model.Channel) + + member.PreSave() + if result.Err = member.IsValid(); result.Err != nil { + storeChannel <- result + return + } - if err := s.GetMaster().Insert(member); err != nil { - if IsUniqueConstraintError(err.Error(), "ChannelId", "channelmembers_pkey") { - result.Err = model.NewAppError("SqlChannelStore.SaveMember", "A channel member with that id already exists", "channel_id="+member.ChannelId+", user_id="+member.UserId+", "+err.Error()) + if err := s.GetMaster().Insert(member); err != nil { + if IsUniqueConstraintError(err.Error(), "ChannelId", "channelmembers_pkey") { + result.Err = model.NewAppError("SqlChannelStore.SaveMember", "A channel member with that id already exists", "channel_id="+member.ChannelId+", user_id="+member.UserId+", "+err.Error()) + } else { + result.Err = model.NewAppError("SqlChannelStore.SaveMember", "We couldn't save the channel member", "channel_id="+member.ChannelId+", user_id="+member.UserId+", "+err.Error()) + } } else { - result.Err = model.NewAppError("SqlChannelStore.SaveMember", "We couldn't save the channel member", "channel_id="+member.ChannelId+", user_id="+member.UserId+", "+err.Error()) + result.Data = member + // If sucessfull record members have changed in channel + if mu := <-s.extraUpdated(channel); mu.Err != nil { + result.Err = mu.Err + } } - } else { - result.Data = member } storeChannel <- result @@ -383,9 +414,21 @@ func (s SqlChannelStore) RemoveMember(channelId string, userId string) StoreChan go func() { result := StoreResult{} - _, err := s.GetMaster().Exec("DELETE FROM ChannelMembers WHERE ChannelId = :ChannelId AND UserId = :UserId", map[string]interface{}{"ChannelId": channelId, "UserId": userId}) - if err != nil { - result.Err = model.NewAppError("SqlChannelStore.RemoveMember", "We couldn't remove the channel member", "channel_id="+channelId+", user_id="+userId+", "+err.Error()) + // Grab the channel we are saving this member to + if cr := <-s.Get(channelId); cr.Err != nil { + result.Err = cr.Err + } else { + channel := cr.Data.(*model.Channel) + + _, err := s.GetMaster().Exec("DELETE FROM ChannelMembers WHERE ChannelId = :ChannelId AND UserId = :UserId", map[string]interface{}{"ChannelId": channelId, "UserId": userId}) + if err != nil { + result.Err = model.NewAppError("SqlChannelStore.RemoveMember", "We couldn't remove the channel member", "channel_id="+channelId+", user_id="+userId+", "+err.Error()) + } else { + // If sucessfull record members have changed in channel + if mu := <-s.extraUpdated(channel); mu.Err != nil { + result.Err = mu.Err + } + } } storeChannel <- result diff --git a/store/sql_channel_store_test.go b/store/sql_channel_store_test.go index ae29bc2b3..b14883843 100644 --- a/store/sql_channel_store_test.go +++ b/store/sql_channel_store_test.go @@ -197,6 +197,16 @@ func TestChannelStoreGetByName(t *testing.T) { func TestChannelMemberStore(t *testing.T) { Setup() + c1 := model.Channel{} + c1.TeamId = model.NewId() + c1.DisplayName = "NameName" + c1.Name = "a" + model.NewId() + "b" + c1.Type = model.CHANNEL_OPEN + c1 = *Must(store.Channel().Save(&c1)).(*model.Channel) + + c1t1 := (<-store.Channel().Get(c1.Id)).Data.(*model.Channel) + t1 := c1t1.ExtraUpdateAt + u1 := model.User{} u1.TeamId = model.NewId() u1.Email = model.NewId() @@ -210,17 +220,24 @@ func TestChannelMemberStore(t *testing.T) { Must(store.User().Save(&u2)) o1 := model.ChannelMember{} - o1.ChannelId = model.NewId() + o1.ChannelId = c1.Id o1.UserId = u1.Id o1.NotifyLevel = model.CHANNEL_NOTIFY_ALL Must(store.Channel().SaveMember(&o1)) o2 := model.ChannelMember{} - o2.ChannelId = o1.ChannelId + o2.ChannelId = c1.Id o2.UserId = u2.Id o2.NotifyLevel = model.CHANNEL_NOTIFY_ALL Must(store.Channel().SaveMember(&o2)) + c1t2 := (<-store.Channel().Get(c1.Id)).Data.(*model.Channel) + t2 := c1t2.ExtraUpdateAt + + if t2 <= t1 { + t.Fatal("Member update time incorrect") + } + members := (<-store.Channel().GetMembers(o1.ChannelId)).Data.([]model.ChannelMember) if len(members) != 2 { t.Fatal("should have saved 2 members") @@ -233,6 +250,13 @@ func TestChannelMemberStore(t *testing.T) { t.Fatal("should have removed 1 member") } + c1t3 := (<-store.Channel().Get(c1.Id)).Data.(*model.Channel) + t3 := c1t3.ExtraUpdateAt + + if t3 <= t2 || t3 <= t1 { + t.Fatal("Member update time incorrect on delete") + } + member := (<-store.Channel().GetMember(o1.ChannelId, o1.UserId)).Data.(model.ChannelMember) if member.ChannelId != o1.ChannelId { t.Fatal("should have go member") @@ -246,6 +270,12 @@ func TestChannelMemberStore(t *testing.T) { if err := (<-store.Channel().SaveMember(&o1)).Err; err == nil { t.Fatal("Should have been a duplicate") } + + c1t4 := (<-store.Channel().Get(c1.Id)).Data.(*model.Channel) + t4 := c1t4.ExtraUpdateAt + if t4 != t3 { + t.Fatal("Should not update time upon failure") + } } func TestChannelStorePermissionsTo(t *testing.T) { -- cgit v1.2.3-1-g7c22 From c7a8112c172a55741e1dcbf366ed637543c301ce Mon Sep 17 00:00:00 2001 From: Christopher Speller Date: Thu, 30 Jul 2015 08:24:06 -0400 Subject: Moving the handing of extra_info etag before the extra info is retrieved from the database --- api/channel.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/api/channel.go b/api/channel.go index 9216dbb09..803274d32 100644 --- a/api/channel.go +++ b/api/channel.go @@ -554,27 +554,31 @@ func getChannelExtraInfo(c *Context, w http.ResponseWriter, r *http.Request) { id := params["id"] sc := Srv.Store.Channel().Get(id) - scm := Srv.Store.Channel().GetMember(id, c.Session.UserId) - ecm := Srv.Store.Channel().GetExtraMembers(id, 20) - + var channel *model.Channel if cresult := <-sc; cresult.Err != nil { c.Err = cresult.Err return - } else if cmresult := <-scm; cmresult.Err != nil { + } else { + channel = cresult.Data.(*model.Channel) + } + + extraEtag := channel.ExtraEtag() + if HandleEtag(extraEtag, w, r) { + return + } + + scm := Srv.Store.Channel().GetMember(id, c.Session.UserId) + ecm := Srv.Store.Channel().GetExtraMembers(id, 20) + + if cmresult := <-scm; cmresult.Err != nil { c.Err = cmresult.Err return } else if ecmresult := <-ecm; ecmresult.Err != nil { c.Err = ecmresult.Err return } else { - channel := cresult.Data.(*model.Channel) member := cmresult.Data.(model.ChannelMember) extraMembers := ecmresult.Data.([]model.ExtraMember) - extraEtag := channel.ExtraEtag() - - if HandleEtag(extraEtag, w, r) { - return - } if !c.HasPermissionsToTeam(channel.TeamId, "getChannelExtraInfo") { return -- cgit v1.2.3-1-g7c22