From 4d223ba3a2d8dbb8844bdf8507396383a8edae8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Villablanca=20V=C3=A1squez?= Date: Mon, 17 Jun 2019 11:03:04 -0400 Subject: [PATCH] Migrate Channel.GetByNames to sync by default (#11202) --- app/channel.go | 16 ++--- store/sqlstore/channel_store.go | 99 +++++++++++++-------------- store/store.go | 2 +- store/storetest/channel_store.go | 11 ++- store/storetest/mocks/ChannelStore.go | 19 +++-- 5 files changed, 76 insertions(+), 71 deletions(-) diff --git a/app/channel.go b/app/channel.go index f991110af8..388571be23 100644 --- a/app/channel.go +++ b/app/channel.go @@ -1190,16 +1190,16 @@ func (a *App) GetChannelByName(channelName, teamId string, includeDeleted bool) } func (a *App) GetChannelsByNames(channelNames []string, teamId string) ([]*model.Channel, *model.AppError) { - result := <-a.Srv.Store.Channel().GetByNames(teamId, channelNames, true) - if result.Err != nil { - if result.Err.Id == "store.sql_channel.get_by_name.missing.app_error" { - result.Err.StatusCode = http.StatusNotFound - return nil, result.Err + channels, err := a.Srv.Store.Channel().GetByNames(teamId, channelNames, true) + if err != nil { + if err.Id == "store.sql_channel.get_by_name.missing.app_error" { + err.StatusCode = http.StatusNotFound + return nil, err } - result.Err.StatusCode = http.StatusBadRequest - return nil, result.Err + err.StatusCode = http.StatusBadRequest + return nil, err } - return result.Data.([]*model.Channel), nil + return channels, nil } func (a *App) GetChannelByNameForTeamName(channelName, teamName string, includeDeleted bool) (*model.Channel, *model.AppError) { diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index f7ea8823a9..8b7b44c334 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1156,63 +1156,60 @@ func (s SqlChannelStore) GetByName(teamId string, name string, allowFromCache bo return s.getByName(teamId, name, false, allowFromCache) } -func (s SqlChannelStore) GetByNames(teamId string, names []string, allowFromCache bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - var channels []*model.Channel +func (s SqlChannelStore) GetByNames(teamId string, names []string, allowFromCache bool) ([]*model.Channel, *model.AppError) { + var channels []*model.Channel - if allowFromCache { - var misses []string - visited := make(map[string]struct{}) - for _, name := range names { - if _, ok := visited[name]; ok { - continue - } - visited[name] = struct{}{} - if cacheItem, ok := channelByNameCache.Get(teamId + name); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter("Channel By Name") - } - channels = append(channels, cacheItem.(*model.Channel)) - } else { - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter("Channel By Name") - } - misses = append(misses, name) - } + if allowFromCache { + var misses []string + visited := make(map[string]struct{}) + for _, name := range names { + if _, ok := visited[name]; ok { + continue } - names = misses - } - - if len(names) > 0 { - props := map[string]interface{}{} - var namePlaceholders []string - for _, name := range names { - key := fmt.Sprintf("Name%v", len(namePlaceholders)) - props[key] = name - namePlaceholders = append(namePlaceholders, ":"+key) - } - - var query string - if teamId == "" { - query = `SELECT * FROM Channels WHERE Name IN (` + strings.Join(namePlaceholders, ", ") + `) AND DeleteAt = 0` + visited[name] = struct{}{} + if cacheItem, ok := channelByNameCache.Get(teamId + name); ok { + if s.metrics != nil { + s.metrics.IncrementMemCacheHitCounter("Channel By Name") + } + channels = append(channels, cacheItem.(*model.Channel)) } else { - props["TeamId"] = teamId - query = `SELECT * FROM Channels WHERE Name IN (` + strings.Join(namePlaceholders, ", ") + `) AND TeamId = :TeamId AND DeleteAt = 0` - } - - var dbChannels []*model.Channel - if _, err := s.GetReplica().Select(&dbChannels, query, props); err != nil && err != sql.ErrNoRows { - result.Err = model.NewAppError("SqlChannelStore.GetByName", "store.sql_channel.get_by_name.existing.app_error", nil, "teamId="+teamId+", "+err.Error(), http.StatusInternalServerError) - return - } - for _, channel := range dbChannels { - channelByNameCache.AddWithExpiresInSecs(teamId+channel.Name, channel, CHANNEL_CACHE_SEC) - channels = append(channels, channel) + if s.metrics != nil { + s.metrics.IncrementMemCacheMissCounter("Channel By Name") + } + misses = append(misses, name) } } + names = misses + } - result.Data = channels - }) + if len(names) > 0 { + props := map[string]interface{}{} + var namePlaceholders []string + for _, name := range names { + key := fmt.Sprintf("Name%v", len(namePlaceholders)) + props[key] = name + namePlaceholders = append(namePlaceholders, ":"+key) + } + + var query string + if teamId == "" { + query = `SELECT * FROM Channels WHERE Name IN (` + strings.Join(namePlaceholders, ", ") + `) AND DeleteAt = 0` + } else { + props["TeamId"] = teamId + query = `SELECT * FROM Channels WHERE Name IN (` + strings.Join(namePlaceholders, ", ") + `) AND TeamId = :TeamId AND DeleteAt = 0` + } + + var dbChannels []*model.Channel + if _, err := s.GetReplica().Select(&dbChannels, query, props); err != nil && err != sql.ErrNoRows { + return nil, model.NewAppError("SqlChannelStore.GetByName", "store.sql_channel.get_by_name.existing.app_error", nil, "teamId="+teamId+", "+err.Error(), http.StatusInternalServerError) + } + for _, channel := range dbChannels { + channelByNameCache.AddWithExpiresInSecs(teamId+channel.Name, channel, CHANNEL_CACHE_SEC) + channels = append(channels, channel) + } + } + + return channels, nil } func (s SqlChannelStore) GetByNameIncludeDeleted(teamId string, name string, allowFromCache bool) store.StoreChannel { diff --git a/store/store.go b/store/store.go index 886231b708..891ce4361f 100644 --- a/store/store.go +++ b/store/store.go @@ -143,7 +143,7 @@ type ChannelStore interface { PermanentDeleteByTeam(teamId string) StoreChannel PermanentDelete(channelId string) StoreChannel GetByName(team_id string, name string, allowFromCache bool) StoreChannel - GetByNames(team_id string, names []string, allowFromCache bool) StoreChannel + GetByNames(team_id string, names []string, allowFromCache bool) ([]*model.Channel, *model.AppError) GetByNameIncludeDeleted(team_id string, name string, allowFromCache bool) StoreChannel GetDeletedByName(team_id string, name string) StoreChannel GetDeleted(team_id string, offset int, limit int) StoreChannel diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 7233378113..42ee91c1ce 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -716,9 +716,9 @@ func testChannelStoreGetByNames(t *testing.T, ss store.Store) { {"", []string{o1.Name, "foo", o2.Name, o2.Name}, []string{o1.Id, o2.Id}}, {"asd", []string{o1.Name, "foo", o2.Name, o2.Name}, nil}, } { - r := <-ss.Channel().GetByNames(tc.TeamId, tc.Names, true) - require.Nil(t, r.Err) - channels := r.Data.([]*model.Channel) + var channels []*model.Channel + channels, err = ss.Channel().GetByNames(tc.TeamId, tc.Names, true) + require.Nil(t, err) var ids []string for _, channel := range channels { ids = append(ids, channel.Id) @@ -734,9 +734,8 @@ func testChannelStoreGetByNames(t *testing.T, ss store.Store) { err = ss.Channel().Delete(o2.Id, model.GetMillis()) require.Nil(t, err, "channel should have been deleted") - r := <-ss.Channel().GetByNames(o1.TeamId, []string{o1.Name}, false) - require.Nil(t, r.Err) - channels := r.Data.([]*model.Channel) + channels, err := ss.Channel().GetByNames(o1.TeamId, []string{o1.Name}, false) + require.Nil(t, err) assert.Len(t, channels, 0) } diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index 83e38395cd..d476efa2f3 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -300,19 +300,28 @@ func (_m *ChannelStore) GetByNameIncludeDeleted(team_id string, name string, all } // GetByNames provides a mock function with given fields: team_id, names, allowFromCache -func (_m *ChannelStore) GetByNames(team_id string, names []string, allowFromCache bool) store.StoreChannel { +func (_m *ChannelStore) GetByNames(team_id string, names []string, allowFromCache bool) ([]*model.Channel, *model.AppError) { ret := _m.Called(team_id, names, allowFromCache) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, []string, bool) store.StoreChannel); ok { + var r0 []*model.Channel + if rf, ok := ret.Get(0).(func(string, []string, bool) []*model.Channel); ok { r0 = rf(team_id, names, allowFromCache) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).([]*model.Channel) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, []string, bool) *model.AppError); ok { + r1 = rf(team_id, names, allowFromCache) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // GetChannelCounts provides a mock function with given fields: teamId, userId