[MM-21209] Add LRU cache for userstore.get (#13838)

* store/cache: add cache to user layer

* store/localcache: fix cache data access

* app/login: add invalidate for user

* store/user: move user sanitization to cache

* store/user: remove sanitize checks form the tests

* store/user: remove sanitization from store and cache layer

* store/cache: remove unnecessary error wraps

Co-authored-by: mattermod <mattermod@users.noreply.github.com>

store/localcachelayer/main_test.go

@@ -230,7 +230,7 @@ func getMockStore() *mocks.Store {
 	mockTermsOfServiceStore.On("Get", "123", false).Return(&fakeTermsOfService, nil)
 	mockStore.On("TermsOfService").Return(&mockTermsOfServiceStore)
 
-	fakeUser := []*model.User{{Id: "123"}}
+	fakeUser := []*model.User{{Id: "123", AuthData: model.NewString("")}}
 	mockUserStore := mocks.UserStore{}
 	mockUserStore.On("GetProfileByIds", []string{"123"}, &store.UserGetByIdsOpts{}, true).Return(fakeUser, nil)
 	mockUserStore.On("GetProfileByIds", []string{"123"}, &store.UserGetByIdsOpts{}, false).Return(fakeUser, nil)
@@ -241,6 +241,7 @@ func getMockStore() *mocks.Store {
 	mockUserStore.On("GetAllProfilesInChannel", "123", true).Return(fakeProfilesInChannelMap, nil)
 	mockUserStore.On("GetAllProfilesInChannel", "123", false).Return(fakeProfilesInChannelMap, nil)
 
+	mockUserStore.On("Get", "123").Return(fakeUser[0], nil)
 	mockStore.On("User").Return(&mockUserStore)
 
 	fakeUserTeamIds := []string{"1", "2", "3"}

store/localcachelayer/user_layer.go

@@ -4,8 +4,6 @@
 package localcachelayer
 
 import (
-	"net/http"
-
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/store"
 )
@@ -75,7 +73,8 @@ func (s LocalCacheUserStore) InvalidateProfilesInChannelCache(channelId string)
 func (s LocalCacheUserStore) GetAllProfilesInChannel(channelId string, allowFromCache bool) (map[string]*model.User, *model.AppError) {
 	if allowFromCache {
 		if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.profilesInChannelCache, channelId); cacheItem != nil {
-			return cacheItem.(map[string]*model.User), nil
+			cachedMap := cacheItem.(map[string]*model.User)
+			return deepCopyUserMap(cachedMap), nil
 		}
 	}
 
@@ -85,7 +84,7 @@ func (s LocalCacheUserStore) GetAllProfilesInChannel(channelId string, allowFrom
 	}
 
 	if allowFromCache {
-		s.rootStore.doStandardAddToCache(s.rootStore.profilesInChannelCache, channelId, userMap)
+		s.rootStore.doStandardAddToCache(s.rootStore.profilesInChannelCache, channelId, deepCopyUserMap(userMap))
 	}
 
 	return userMap, nil
@@ -106,7 +105,6 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us
 	for _, userId := range userIds {
 		if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.userProfileByIdsCache, userId); cacheItem != nil {
 			u := cacheItem.(*model.User)
-
 			if options.Since == 0 || u.UpdateAt > options.Since {
 				users = append(users, u.DeepCopy())
 			}
@@ -123,15 +121,48 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us
 	if len(remainingUserIds) > 0 {
 		remainingUsers, err := s.UserStore.GetProfileByIds(remainingUserIds, options, false)
 		if err != nil {
-			return nil, model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
+			return nil, err
 		}
-
 		for _, user := range remainingUsers {
-			users = append(users, user.DeepCopy())
 			s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, user.Id, user)
+			users = append(users, user.DeepCopy())
 		}
-
 	}
 
 	return users, nil
 }
+
+// Get is a cache wrapper around the SqlStore method to get a user profile by id.
+// It checks if the user entry is present in the cache, returning the entry from cache
+// if it is present. Otherwise, it fetches the entry from the store and stores it in the
+// cache.
+func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) {
+	cacheItem := s.rootStore.doStandardReadCache(s.rootStore.userProfileByIdsCache, id)
+	if cacheItem != nil {
+		if s.rootStore.metrics != nil {
+			s.rootStore.metrics.AddMemCacheHitCounter("Profile By Id", float64(1))
+		}
+		u := cacheItem.(*model.User)
+		return u.DeepCopy(), nil
+	}
+	if s.rootStore.metrics != nil {
+		s.rootStore.metrics.AddMemCacheMissCounter("Profile By Id", float64(1))
+	}
+	user, err := s.UserStore.Get(id)
+	if err != nil {
+		return nil, err
+	}
+	u := user.DeepCopy()
+	s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, id, u)
+	return user.DeepCopy(), nil
+}
+
+func deepCopyUserMap(users map[string]*model.User) map[string]*model.User {
+	copyOfUsers := make(map[string]*model.User)
+
+	for id, user := range users {
+		copyOfUsers[id] = user.DeepCopy()
+	}
+
+	return copyOfUsers
+}

store/localcachelayer/user_layer_test.go

@@ -21,7 +21,7 @@ func TestUserStore(t *testing.T) {
 
 func TestUserStoreCache(t *testing.T) {
 	fakeUserIds := []string{"123"}
-	fakeUser := []*model.User{{Id: "123"}}
+	fakeUser := []*model.User{{Id: "123", AuthData: model.NewString("")}}
 
 	t.Run("first call not cached, second cached and returning same data", func(t *testing.T) {
 		mockStore := getMockStore()
@@ -59,7 +59,6 @@ func TestUserStoreCache(t *testing.T) {
 		gotUser, err := cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true)
 		require.Nil(t, err)
 		assert.Equal(t, fakeUser, gotUser)
-		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetProfileByIds", 1)
 
 		cachedStore.User().InvalidateProfileCacheForUser("123")
 
@@ -87,7 +86,7 @@ func TestUserStoreCache(t *testing.T) {
 		require.Nil(t, err)
 
 		for i := 0; i < len(storedUsers); i++ {
-			assert.Equal(t, storedUsers[i], cachedUsers[i])
+			assert.Equal(t, storedUsers[i].Id, cachedUsers[i].Id)
 			if storedUsers[i] == cachedUsers[i] {
 				assert.Fail(t, "should be different pointers")
 			}
@@ -179,3 +178,70 @@ func TestUserStoreProfilesInChannelCache(t *testing.T) {
 		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetAllProfilesInChannel", 2)
 	})
 }
+
+func TestUserStoreGetCache(t *testing.T) {
+	fakeUserId := "123"
+	fakeUser := &model.User{Id: "123", AuthData: model.NewString("")}
+	t.Run("first call not cached, second cached and returning same data", func(t *testing.T) {
+		mockStore := getMockStore()
+		mockCacheProvider := getMockCacheProvider()
+		cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider)
+
+		gotUser, err := cachedStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		assert.Equal(t, fakeUser, gotUser)
+		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 1)
+
+		_, _ = cachedStore.User().Get(fakeUserId)
+		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 1)
+	})
+
+	t.Run("first call not cached, invalidate, and then not cached again", func(t *testing.T) {
+		mockStore := getMockStore()
+		mockCacheProvider := getMockCacheProvider()
+		cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider)
+
+		gotUser, err := cachedStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		assert.Equal(t, fakeUser, gotUser)
+		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 1)
+
+		cachedStore.User().InvalidateProfileCacheForUser("123")
+
+		_, _ = cachedStore.User().Get(fakeUserId)
+		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 2)
+	})
+
+	t.Run("should always return a copy of the stored data", func(t *testing.T) {
+		mockStore := getMockStore()
+		mockCacheProvider := getMockCacheProvider()
+		cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider)
+
+		storedUser, err := mockStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		originalProps := storedUser.NotifyProps
+
+		storedUser.NotifyProps = map[string]string{}
+		storedUser.NotifyProps["key"] = "somevalue"
+
+		cachedUser, err := cachedStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		assert.Equal(t, storedUser, cachedUser)
+		if storedUser == cachedUser {
+			assert.Fail(t, "should be different pointers")
+		}
+		cachedUser.NotifyProps["key"] = "othervalue"
+		assert.NotEqual(t, storedUser, cachedUser)
+
+		cachedUser, err = cachedStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		assert.Equal(t, storedUser, cachedUser)
+		if storedUser == cachedUser {
+			assert.Fail(t, "should be different pointers")
+		}
+		cachedUser.NotifyProps["key"] = "othervalue"
+		assert.NotEqual(t, storedUser, cachedUser)
+
+		storedUser.NotifyProps = originalProps
+	})
+}

store/sqlstore/user_store.go

@@ -825,10 +825,6 @@ func (us SqlUserStore) GetProfileByIds(userIds []string, options *store.UserGetB
 		return nil, model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
-	for _, u := range users {
-		u.Sanitize(map[string]bool{})
-	}
-
 	return users, nil
 }
 

store/storetest/user_store.go

@@ -351,10 +351,8 @@ func testGetAllUsingAuthService(t *testing.T, ss store.Store) {
 }
 
 func sanitized(user *model.User) *model.User {
-	clonedUser := model.UserFromJson(strings.NewReader(user.ToJson()))
-	clonedUser.AuthData = new(string)
-	*clonedUser.AuthData = ""
-	clonedUser.Props = model.StringMap{}
+	clonedUser := user.DeepCopy()
+	clonedUser.Sanitize(map[string]bool{})
 
 	return clonedUser
 }
@@ -1276,25 +1274,25 @@ func testUserStoreGetProfilesByIds(t *testing.T, ss store.Store) {
 	t.Run("get u1 by id, no caching", func(t *testing.T) {
 		users, err := ss.User().GetProfileByIds([]string{u1.Id}, nil, false)
 		require.Nil(t, err)
-		assert.Equal(t, []*model.User{sanitized(u1)}, users)
+		assert.Equal(t, []*model.User{u1}, users)
 	})
 
 	t.Run("get u1 by id, caching", func(t *testing.T) {
 		users, err := ss.User().GetProfileByIds([]string{u1.Id}, nil, true)
 		require.Nil(t, err)
-		assert.Equal(t, []*model.User{sanitized(u1)}, users)
+		assert.Equal(t, []*model.User{u1}, users)
 	})
 
 	t.Run("get u1, u2, u3 by id, no caching", func(t *testing.T) {
 		users, err := ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, false)
 		require.Nil(t, err)
-		assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, users)
+		assert.Equal(t, []*model.User{u1, u2, u3}, users)
 	})
 
 	t.Run("get u1, u2, u3 by id, caching", func(t *testing.T) {
 		users, err := ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, true)
 		require.Nil(t, err)
-		assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, users)
+		assert.Equal(t, []*model.User{u1, u2, u3}, users)
 	})
 
 	t.Run("get unknown id, caching", func(t *testing.T) {
@@ -1310,7 +1308,7 @@ func testUserStoreGetProfilesByIds(t *testing.T, ss store.Store) {
 		require.Nil(t, err)
 
 		// u3 comes from the cache, and u4 does not
-		assert.Equal(t, []*model.User{sanitized(u3), sanitized(u4)}, users)
+		assert.Equal(t, []*model.User{u3, u4}, users)
 	})
 }
 
@@ -4581,6 +4579,8 @@ func testUserStoreResetLastPictureUpdate(t *testing.T, ss store.Store) {
 	err = ss.User().ResetLastPictureUpdate(u1.Id)
 	require.Nil(t, err)
 
+	ss.User().InvalidateProfileCacheForUser(u1.Id)
+
 	user2, err := ss.User().Get(u1.Id)
 	require.Nil(t, err)