From 5a511a14ee419dd8e7786de42d433766fc26b19a Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Wed, 25 Sep 2024 13:54:57 +0530 Subject: [PATCH] MM-60606: Respect allowFromCache flag in Channelstore.GetMany (#28290) Setting the flag to false never worked, and it was never caught because there wasn't an instance when this method was called with allowFromCache=false. https://mattermost.atlassian.net/browse/MM-60606 ```release-note NONE ``` --- .../store/localcachelayer/channel_layer.go | 33 ++++++++++--------- .../localcachelayer/channel_layer_test.go | 13 ++++++++ .../store/localcachelayer/main_test.go | 1 + 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/server/channels/store/localcachelayer/channel_layer.go b/server/channels/store/localcachelayer/channel_layer.go index 2a58f51405..9523dbdd66 100644 --- a/server/channels/store/localcachelayer/channel_layer.go +++ b/server/channels/store/localcachelayer/channel_layer.go @@ -211,7 +211,6 @@ func (s LocalCacheChannelStore) GetPinnedPostCount(channelId string, allowFromCa } count, err := s.ChannelStore.GetPinnedPostCount(channelId, allowFromCache) - if err != nil { return 0, err } @@ -244,22 +243,24 @@ func (s LocalCacheChannelStore) GetMany(ids []string, allowFromCache bool) (mode var foundChannels []*model.Channel var channelsToQuery []string - if allowFromCache { - toPass := allocateCacheTargets[*model.Channel](len(ids)) - errs := s.rootStore.doMultiReadCache(s.rootStore.roleCache, ids, toPass) - for i, err := range errs { - if err != nil { - if err != cache.ErrKeyNotFound { - s.rootStore.logger.Warn("Error in Channelstore.GetMany: ", mlog.Err(err)) - } - channelsToQuery = append(channelsToQuery, ids[i]) + if !allowFromCache { + return s.ChannelStore.GetMany(ids, allowFromCache) + } + + toPass := allocateCacheTargets[*model.Channel](len(ids)) + errs := s.rootStore.doMultiReadCache(s.rootStore.roleCache, ids, toPass) + for i, err := range errs { + if err != nil { + if err != cache.ErrKeyNotFound { + s.rootStore.logger.Warn("Error in Channelstore.GetMany: ", mlog.Err(err)) + } + channelsToQuery = append(channelsToQuery, ids[i]) + } else { + gotChannel := *(toPass[i].(**model.Channel)) + if gotChannel != nil { + foundChannels = append(foundChannels, gotChannel) } else { - gotChannel := *(toPass[i].(**model.Channel)) - if gotChannel != nil { - foundChannels = append(foundChannels, gotChannel) - } else { - s.rootStore.logger.Warn("Found nil channel in GetMany. This is not expected") - } + s.rootStore.logger.Warn("Found nil channel in GetMany. This is not expected") } } } diff --git a/server/channels/store/localcachelayer/channel_layer_test.go b/server/channels/store/localcachelayer/channel_layer_test.go index 3ad69b693b..91094fe9d9 100644 --- a/server/channels/store/localcachelayer/channel_layer_test.go +++ b/server/channels/store/localcachelayer/channel_layer_test.go @@ -391,6 +391,19 @@ func TestChannelStoreGetManyCache(t *testing.T) { assert.ElementsMatch(t, model.ChannelList{&fakeChannel, &fakeChannel2}, channels) mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetMany", 2) }) + + t.Run("passing allowCache=false should bypass cache", func(t *testing.T) { + mockStore := getMockStore(t) + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider, logger) + require.NoError(t, err) + + fakeChannel := model.Channel{Id: "channel1", Name: "channel1-name"} + channels, err := cachedStore.Channel().GetMany([]string{fakeChannel.Id}, false) + require.NoError(t, err) + assert.ElementsMatch(t, model.ChannelList{&fakeChannel}, channels) + mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetMany", 1) + }) } func TestChannelStoreGetByNamesCache(t *testing.T) { diff --git a/server/channels/store/localcachelayer/main_test.go b/server/channels/store/localcachelayer/main_test.go index ca7d25e8f2..bafb2278a7 100644 --- a/server/channels/store/localcachelayer/main_test.go +++ b/server/channels/store/localcachelayer/main_test.go @@ -104,6 +104,7 @@ func getMockStore(t *testing.T) *mocks.Store { mockChannelStore.On("Get", channelId, true).Return(&fakeChannel1, nil) mockChannelStore.On("Get", channelId, false).Return(&fakeChannel1, nil) mockChannelStore.On("GetMany", []string{channelId}, true).Return(model.ChannelList{&fakeChannel1}, nil) + mockChannelStore.On("GetMany", []string{channelId}, false).Return(model.ChannelList{&fakeChannel1}, nil) mockChannelStore.On("GetMany", []string{fakeChannel2.Id}, true).Return(model.ChannelList{&fakeChannel2}, nil) mockChannelStore.On("GetByNames", "team1", []string{fakeChannel1.Name}, true).Return([]*model.Channel{&fakeChannel1}, nil) mockChannelStore.On("GetByNames", "team1", []string{fakeChannel2.Name}, true).Return([]*model.Channel{&fakeChannel2}, nil)