[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 <mattermod@users.noreply.github.com>

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.

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)

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)

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
+}

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)
+	})
+}

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)