From de9a0197e256a07705c345824a6007389f231276 Mon Sep 17 00:00:00 2001 From: "Sanele T. Mahlalela" Date: Fri, 20 Dec 2019 14:51:54 +0200 Subject: [PATCH] [GH-13074] Migrate TermsOfServiceCache from store/sqlstore/terms_of_service.go to the new store/localcachelayer (#13205) * migrated termsOfServiceCache from sqlstore to localcachelayer, and mocked store tests * revert gitignore local change * fixed caching in termsOfService Get, and added basic tests for termsOfServiceCache * added a test for cache save, and fixed call to Store tests * fixed GeLatest termsOfService from cache test * added license headers to terms_of_service cache files * using doStandardReadFromCache and doStandardAddToCache when reading and writing to cache * removed unused variable, termsOfServiceCacheName * added special key for the latest termsOfService value in termsOfServiceCache * updated license information on localcachelayer termsOfServiceCache files * fixed not updating latest termsOfServiceCache on Get by ID, and invalidating cache cluster on termsOfServiceCache save Co-authored-by: mattermod --- model/cluster_message.go | 1 + store/localcachelayer/layer.go | 16 ++- store/localcachelayer/main_test.go | 10 ++ .../localcachelayer/terms_of_service_layer.go | 79 +++++++++++ .../terms_of_service_layer_test.go | 127 ++++++++++++++++++ store/sqlstore/terms_of_service_store.go | 43 ------ 6 files changed, 231 insertions(+), 45 deletions(-) create mode 100644 store/localcachelayer/terms_of_service_layer.go create mode 100644 store/localcachelayer/terms_of_service_layer_test.go diff --git a/model/cluster_message.go b/model/cluster_message.go index bf51d3ee1c..ba9ff2beac 100644 --- a/model/cluster_message.go +++ b/model/cluster_message.go @@ -37,6 +37,7 @@ const ( CLUSTER_EVENT_CLEAR_SESSION_CACHE_FOR_ALL_USERS = "inv_all_user_sessions" CLUSTER_EVENT_INSTALL_PLUGIN = "install_plugin" CLUSTER_EVENT_REMOVE_PLUGIN = "remove_plugin" + CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TERMS_OF_SERVICE = "inv_terms_of_service" CLUSTER_EVENT_BUSY_STATE_CHANGED = "busy_state_change" // SendTypes for ClusterMessage. diff --git a/store/localcachelayer/layer.go b/store/localcachelayer/layer.go index 6706671c60..7d8e81fe9e 100644 --- a/store/localcachelayer/layer.go +++ b/store/localcachelayer/layer.go @@ -38,8 +38,10 @@ const ( LAST_POSTS_CACHE_SIZE = 20000 LAST_POSTS_CACHE_SEC = 30 * 60 - LAST_POST_TIME_CACHE_SIZE = 25000 - LAST_POST_TIME_CACHE_SEC = 15 * 60 + TERMS_OF_SERVICE_CACHE_SIZE = 20000 + TERMS_OF_SERVICE_CACHE_SEC = 30 * 60 + LAST_POST_TIME_CACHE_SIZE = 25000 + LAST_POST_TIME_CACHE_SEC = 15 * 60 USER_PROFILE_BY_ID_CACHE_SIZE = 20000 USER_PROFILE_BY_ID_SEC = 30 * 60 @@ -79,6 +81,8 @@ type LocalCacheStore struct { userProfileByIdsCache *utils.Cache team LocalCacheTeamStore teamAllTeamIdsForUserCache *utils.Cache + termsOfService LocalCacheTermsOfServiceStore + termsOfServiceCache *utils.Cache } func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterface, cluster einterfaces.ClusterInterface) LocalCacheStore { @@ -106,6 +110,8 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf localCacheStore.lastPostTimeCache = utils.NewLruWithParams(LAST_POST_TIME_CACHE_SIZE, "LastPostTime", LAST_POST_TIME_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POST_TIME) localCacheStore.postLastPostsCache = utils.NewLruWithParams(LAST_POSTS_CACHE_SIZE, "LastPost", LAST_POSTS_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POSTS) localCacheStore.post = LocalCachePostStore{PostStore: baseStore.Post(), rootStore: &localCacheStore} + localCacheStore.termsOfServiceCache = utils.NewLruWithParams(TERMS_OF_SERVICE_CACHE_SIZE, "TermsOfService", TERMS_OF_SERVICE_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TERMS_OF_SERVICE) + localCacheStore.termsOfService = LocalCacheTermsOfServiceStore{TermsOfServiceStore: baseStore.TermsOfService(), rootStore: &localCacheStore} localCacheStore.userProfileByIdsCache = utils.NewLruWithParams(USER_PROFILE_BY_ID_CACHE_SIZE, "UserProfileByIds", USER_PROFILE_BY_ID_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_PROFILE_BY_IDS) localCacheStore.user = LocalCacheUserStore{UserStore: baseStore.User(), rootStore: &localCacheStore} localCacheStore.teamAllTeamIdsForUserCache = utils.NewLruWithParams(TEAM_CACHE_SIZE, "Team", TEAM_CACHE_SEC, model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TEAMS) @@ -124,6 +130,7 @@ func NewLocalCacheLayer(baseStore store.Store, metrics einterfaces.MetricsInterf cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL_GUEST_COUNT, localCacheStore.channel.handleClusterInvalidateChannelGuestCounts) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_CHANNEL, localCacheStore.channel.handleClusterInvalidateChannelById) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_LAST_POSTS, localCacheStore.post.handleClusterInvalidateLastPosts) + cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TERMS_OF_SERVICE, localCacheStore.termsOfService.handleClusterInvalidateTermsOfService) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_PROFILE_BY_IDS, localCacheStore.user.handleClusterInvalidateScheme) cluster.RegisterClusterMessageHandler(model.CLUSTER_EVENT_INVALIDATE_CACHE_FOR_TEAMS, localCacheStore.team.handleClusterInvalidateTeam) } @@ -158,6 +165,10 @@ func (s LocalCacheStore) Post() store.PostStore { return s.post } +func (s LocalCacheStore) TermsOfService() store.TermsOfServiceStore { + return s.termsOfService +} + func (s LocalCacheStore) User() store.UserStore { return s.user } @@ -224,6 +235,7 @@ func (s *LocalCacheStore) Invalidate() { s.doClearCacheCluster(s.channelGuestCountCache) s.doClearCacheCluster(s.channelByIdCache) s.doClearCacheCluster(s.postLastPostsCache) + s.doClearCacheCluster(s.termsOfServiceCache) s.doClearCacheCluster(s.lastPostTimeCache) s.doClearCacheCluster(s.userProfileByIdsCache) s.doClearCacheCluster(s.teamAllTeamIdsForUserCache) diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index 0249e09bff..15514facec 100644 --- a/store/localcachelayer/main_test.go +++ b/store/localcachelayer/main_test.go @@ -98,6 +98,16 @@ func getMockStore() *mocks.Store { mockPostStore.On("GetPostsSince", mockPostStoreOptions, false).Return(model.NewPostList(), nil) mockStore.On("Post").Return(&mockPostStore) + fakeTermsOfService := model.TermsOfService{Id: "123", CreateAt: 11111, UserId: "321", Text: "Terms of service test"} + mockTermsOfServiceStore := mocks.TermsOfServiceStore{} + mockTermsOfServiceStore.On("InvalidateTermsOfService", "123") + mockTermsOfServiceStore.On("Save", &fakeTermsOfService).Return(&fakeTermsOfService, nil) + mockTermsOfServiceStore.On("GetLatest", true).Return(&fakeTermsOfService, nil) + mockTermsOfServiceStore.On("GetLatest", false).Return(&fakeTermsOfService, nil) + mockTermsOfServiceStore.On("Get", "123", true).Return(&fakeTermsOfService, nil) + mockTermsOfServiceStore.On("Get", "123", false).Return(&fakeTermsOfService, nil) + mockStore.On("TermsOfService").Return(&mockTermsOfServiceStore) + fakeUser := []*model.User{{Id: "123"}} mockUserStore := mocks.UserStore{} mockUserStore.On("GetProfileByIds", []string{"123"}, &store.UserGetByIdsOpts{}, true).Return(fakeUser, nil) diff --git a/store/localcachelayer/terms_of_service_layer.go b/store/localcachelayer/terms_of_service_layer.go new file mode 100644 index 0000000000..41a798b48c --- /dev/null +++ b/store/localcachelayer/terms_of_service_layer.go @@ -0,0 +1,79 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package localcachelayer + +import ( + "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/store" +) + +const ( + LATEST_KEY = "latest" +) + +type LocalCacheTermsOfServiceStore struct { + store.TermsOfServiceStore + rootStore *LocalCacheStore +} + +func (s *LocalCacheTermsOfServiceStore) handleClusterInvalidateTermsOfService(msg *model.ClusterMessage) { + if msg.Data == CLEAR_CACHE_MESSAGE_DATA { + s.rootStore.termsOfServiceCache.Purge() + } else { + s.rootStore.termsOfServiceCache.Remove(msg.Data) + } +} + +func (s LocalCacheTermsOfServiceStore) ClearCaches() { + s.rootStore.doClearCacheCluster(s.rootStore.termsOfServiceCache) + + if s.rootStore.metrics != nil { + s.rootStore.metrics.IncrementMemCacheInvalidationCounter("Terms Of Service - Purge") + } +} + +func (s LocalCacheTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*model.TermsOfService, *model.AppError) { + tos, err := s.TermsOfServiceStore.Save(termsOfService) + + if err == nil { + s.rootStore.doStandardAddToCache(s.rootStore.termsOfServiceCache, tos.Id, tos) + s.rootStore.doInvalidateCacheCluster(s.rootStore.termsOfServiceCache, LATEST_KEY) + } + return tos, err +} + +func (s LocalCacheTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) { + if allowFromCache { + if s.rootStore.termsOfServiceCache.Len() != 0 { + if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.termsOfServiceCache, LATEST_KEY); cacheItem != nil { + return cacheItem.(*model.TermsOfService), nil + } + } + } + + termsOfService, err := s.TermsOfServiceStore.GetLatest(allowFromCache) + + if allowFromCache && err == nil { + s.rootStore.doStandardAddToCache(s.rootStore.termsOfServiceCache, termsOfService.Id, termsOfService) + s.rootStore.doStandardAddToCache(s.rootStore.termsOfServiceCache, LATEST_KEY, termsOfService) + } + + return termsOfService, err +} + +func (s LocalCacheTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) { + if allowFromCache { + if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.termsOfServiceCache, id); cacheItem != nil { + return cacheItem.(*model.TermsOfService), nil + } + } + + termsOfService, err := s.TermsOfServiceStore.Get(id, allowFromCache) + + if allowFromCache && err == nil { + s.rootStore.doStandardAddToCache(s.rootStore.termsOfServiceCache, termsOfService.Id, termsOfService) + } + + return termsOfService, err +} diff --git a/store/localcachelayer/terms_of_service_layer_test.go b/store/localcachelayer/terms_of_service_layer_test.go new file mode 100644 index 0000000000..9036c6b4ee --- /dev/null +++ b/store/localcachelayer/terms_of_service_layer_test.go @@ -0,0 +1,127 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package localcachelayer + +import ( + "testing" + + "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/store/storetest" + "github.com/mattermost/mattermost-server/v5/store/storetest/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTermsOfServiceStore(t *testing.T) { + StoreTest(t, storetest.TestTermsOfServiceStore) +} + +func TestTermsOfServiceStoreTermsOfServiceCache(t *testing.T) { + + fakeTermsOfService := model.TermsOfService{Id: "123", CreateAt: 11111, UserId: "321", Text: "Terms of service test"} + + t.Run("first call by latest not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + termsOfService, err := cachedStore.TermsOfService().GetLatest(true) + require.Nil(t, err) + assert.Equal(t, termsOfService, &fakeTermsOfService) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 1) + termsOfService, err = cachedStore.TermsOfService().GetLatest(true) + require.Nil(t, err) + assert.Equal(t, termsOfService, &fakeTermsOfService) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 1) + }) + + t.Run("first call by id not cached, second cached and returning same data", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + termsOfService, err := cachedStore.TermsOfService().Get("123", true) + require.Nil(t, err) + assert.Equal(t, termsOfService, &fakeTermsOfService) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 1) + termsOfService, err = cachedStore.TermsOfService().Get("123", true) + require.Nil(t, err) + assert.Equal(t, termsOfService, &fakeTermsOfService) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 1) + }) + + t.Run("first call by id not cached, second force no cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.TermsOfService().Get("123", true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 1) + cachedStore.TermsOfService().Get("123", false) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 2) + }) + + t.Run("first call latest not cached, second force no cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.TermsOfService().GetLatest(true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 1) + cachedStore.TermsOfService().GetLatest(false) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 2) + }) + + t.Run("first call by id force no cached, second not cached, third cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.TermsOfService().Get("123", false) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 1) + cachedStore.TermsOfService().Get("123", true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 2) + cachedStore.TermsOfService().Get("123", true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 2) + }) + + t.Run("first call latest force no cached, second not cached, third cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.TermsOfService().GetLatest(false) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 1) + cachedStore.TermsOfService().GetLatest(true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 2) + cachedStore.TermsOfService().GetLatest(true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 2) + }) + + t.Run("first call latest, second call by id cached", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.TermsOfService().GetLatest(true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 1) + cachedStore.TermsOfService().Get("123", true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 0) + }) + + t.Run("first call by id not cached, save, and then not cached again", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.TermsOfService().Get("123", false) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 1) + cachedStore.TermsOfService().Save(&fakeTermsOfService) + cachedStore.TermsOfService().Get("123", false) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "Get", 2) + }) + + t.Run("first get latest not cached, save new, then get latest, returning different data", func(t *testing.T) { + mockStore := getMockStore() + cachedStore := NewLocalCacheLayer(mockStore, nil, nil) + + cachedStore.TermsOfService().GetLatest(true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 1) + cachedStore.TermsOfService().Save(&fakeTermsOfService) + cachedStore.TermsOfService().GetLatest(true) + mockStore.TermsOfService().(*mocks.TermsOfServiceStore).AssertNumberOfCalls(t, "GetLatest", 2) + }) +} diff --git a/store/sqlstore/terms_of_service_store.go b/store/sqlstore/terms_of_service_store.go index 73ee31648e..e3a32f66e7 100644 --- a/store/sqlstore/terms_of_service_store.go +++ b/store/sqlstore/terms_of_service_store.go @@ -10,7 +10,6 @@ import ( "github.com/mattermost/mattermost-server/v5/einterfaces" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" - "github.com/mattermost/mattermost-server/v5/utils" ) type SqlTermsOfServiceStore struct { @@ -18,12 +17,6 @@ type SqlTermsOfServiceStore struct { metrics einterfaces.MetricsInterface } -var termsOfServiceCache = utils.NewLru(model.TERMS_OF_SERVICE_CACHE_SIZE) - -const ( - termsOfServiceCacheName = "TermsOfServiceStore" -) - func NewSqlTermsOfServiceStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) store.TermsOfServiceStore { s := SqlTermsOfServiceStore{sqlStore, metrics} @@ -55,28 +48,10 @@ func (s SqlTermsOfServiceStore) Save(termsOfService *model.TermsOfService) (*mod return nil, model.NewAppError("SqlTermsOfServiceStore.Save", "store.sql_terms_of_service.save.app_error", nil, "terms_of_service_id="+termsOfService.Id+",err="+err.Error(), http.StatusInternalServerError) } - termsOfServiceCache.AddWithDefaultExpires(termsOfService.Id, termsOfService) - return termsOfService, nil } func (s SqlTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfService, *model.AppError) { - if allowFromCache { - if termsOfServiceCache.Len() != 0 { - if cacheItem, ok := termsOfServiceCache.Get(termsOfServiceCache.Keys()[0]); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter(termsOfServiceCacheName) - } - - return cacheItem.(*model.TermsOfService), nil - } - } - } - - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter(termsOfServiceCacheName) - } - var termsOfService *model.TermsOfService err := s.GetReplica().SelectOne(&termsOfService, "SELECT * FROM TermsOfService ORDER BY CreateAt DESC LIMIT 1") @@ -87,28 +62,10 @@ func (s SqlTermsOfServiceStore) GetLatest(allowFromCache bool) (*model.TermsOfSe return nil, model.NewAppError("SqlTermsOfServiceStore.GetLatest", "store.sql_terms_of_service_store.get.app_error", nil, "err="+err.Error(), http.StatusInternalServerError) } - if allowFromCache { - termsOfServiceCache.AddWithDefaultExpires(termsOfService.Id, termsOfService) - } return termsOfService, nil } func (s SqlTermsOfServiceStore) Get(id string, allowFromCache bool) (*model.TermsOfService, *model.AppError) { - if allowFromCache { - if termsOfServiceCache.Len() != 0 { - if cacheItem, ok := termsOfServiceCache.Get(id); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter(termsOfServiceCacheName) - } - - return cacheItem.(*model.TermsOfService), nil - } - } - } - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter(termsOfServiceCacheName) - } - obj, err := s.GetReplica().Get(model.TermsOfService{}, id) if err != nil { return nil, model.NewAppError("SqlTermsOfServiceStore.Get", "store.sql_terms_of_service_store.get.app_error", nil, "err="+err.Error(), http.StatusInternalServerError)