MM-21209: Use the LRU cache for UserStore.Get call too (#13456)

* MM-21209: Use the LRU cache for UserStore.Get call too

We already have userProfileByIdsCache to store the user profiles by Id.
It just wasn't being used for the (*UserStore).Get method. We add a wrapper
method in LocalCacheUserStore to intercept that call and check for the
presence of the user Id in the cache.

There is no need to add any code for invalidation as all of that is already
present.

* Fix nil check for rootstore

* Fix TestUserStore test

Added an invalidate call. The invalidation was being done
from the app level. Hence we have to do it manually here for the test.

app/server_app_adapters.go

@@ -61,7 +61,11 @@ func (s *Server) RunOldAppInitialization() error {
 
 	if s.FakeApp().Srv.newStore == nil {
 		s.FakeApp().Srv.newStore = func() store.Store {
-			return store.NewTimerLayer(localcachelayer.NewLocalCacheLayer(sqlstore.NewSqlSupplier(s.FakeApp().Config().SqlSettings, s.Metrics), s.Metrics, s.Cluster), s.Metrics)
+			return store.NewTimerLayer(
+				localcachelayer.NewLocalCacheLayer(
+					sqlstore.NewSqlSupplier(s.FakeApp().Config().SqlSettings, s.Metrics),
+					s.Metrics, s.Cluster),
+				s.Metrics)
 		}
 	}
 

store/localcachelayer/main_test.go

@@ -112,6 +112,7 @@ func getMockStore() *mocks.Store {
 	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)
+	mockUserStore.On("Get", "123").Return(fakeUser[0], nil)
 	mockStore.On("User").Return(&mockUserStore)
 
 	fakeUserTeamIds := []string{"1", "2", "3"}

store/localcachelayer/user_layer.go

@@ -80,8 +80,31 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us
 		for _, user := range remainingUsers {
 			s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, user.Id, user)
 		}
-
 	}
 
 	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, 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, model.NewAppError("SqlUserStore.Get", "store.sql_user.get.app_error", nil, err.Error(), http.StatusInternalServerError)
+	}
+	s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, id, user)
+	return user, nil
+}

store/localcachelayer/user_layer_test.go

@@ -19,7 +19,7 @@ func TestUserStore(t *testing.T) {
 	StoreTestWithSqlSupplier(t, storetest.TestUserStore)
 }
 
-func TestUserStoreCache(t *testing.T) {
+func TestUserStoreGetProfileByIdsCache(t *testing.T) {
 	fakeUserIds := []string{"123"}
 	fakeUser := []*model.User{{Id: "123"}}
 
@@ -64,3 +64,36 @@ func TestUserStoreCache(t *testing.T) {
 		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetProfileByIds", 2)
 	})
 }
+
+func TestUserStoreGetCache(t *testing.T) {
+	fakeUserId := "123"
+	fakeUser := &model.User{Id: "123"}
+
+	t.Run("first call not cached, second cached and returning same data", func(t *testing.T) {
+		mockStore := getMockStore()
+		cachedStore := NewLocalCacheLayer(mockStore, nil, nil)
+
+		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()
+		cachedStore := NewLocalCacheLayer(mockStore, nil, nil)
+
+		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().InvalidatProfileCacheForUser("123")
+
+		_, _ = cachedStore.User().Get(fakeUserId)
+		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 2)
+	})
+}

store/storetest/user_store.go

@@ -4949,6 +4949,8 @@ func testUserStoreResetLastPictureUpdate(t *testing.T, ss store.Store) {
 	err = ss.User().ResetLastPictureUpdate(u1.Id)
 	require.Nil(t, err)
 
+	ss.User().InvalidatProfileCacheForUser(u1.Id)
+
 	user2, err := ss.User().Get(u1.Id)
 	require.Nil(t, err)