diff --git a/store/localcachelayer/main_test.go b/store/localcachelayer/main_test.go index 39c6655106..294622b3de 100644 --- a/store/localcachelayer/main_test.go +++ b/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"} diff --git a/store/localcachelayer/user_layer.go b/store/localcachelayer/user_layer.go index 2224f56a7f..88586c1ef8 100644 --- a/store/localcachelayer/user_layer.go +++ b/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 +} diff --git a/store/localcachelayer/user_layer_test.go b/store/localcachelayer/user_layer_test.go index 90cd8e5d3c..a627a24539 100644 --- a/store/localcachelayer/user_layer_test.go +++ b/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 + }) +} diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index f8d4e5ac85..0ef485bde4 100644 --- a/store/sqlstore/user_store.go +++ b/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 } diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index b1a2be39ba..a6b5e2f53b 100644 --- a/store/storetest/user_store.go +++ b/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)