diff --git a/store/localcachelayer/user_layer.go b/store/localcachelayer/user_layer.go index 32d1c55321..ce9a08a012 100644 --- a/store/localcachelayer/user_layer.go +++ b/store/localcachelayer/user_layer.go @@ -105,11 +105,10 @@ 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 := &model.User{} - *u = *cacheItem.(*model.User) + u := cacheItem.(*model.User) if options.Since == 0 || u.UpdateAt > options.Since { - users = append(users, u) + users = append(users, u.DeepCopy()) } } else { remainingUserIds = append(remainingUserIds, userId) @@ -127,9 +126,8 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us return nil, model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError) } - users = append(users, remainingUsers...) - for _, user := range remainingUsers { + users = append(users, user.DeepCopy()) s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, user.Id, user) } } @@ -147,8 +145,8 @@ func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) { if s.rootStore.metrics != nil { s.rootStore.metrics.AddMemCacheHitCounter("Profile By Id", float64(1)) } - u := *cacheItem.(*model.User) - return &u, nil + u := cacheItem.(*model.User) + return u.DeepCopy(), nil } if s.rootStore.metrics != nil { s.rootStore.metrics.AddMemCacheMissCounter("Profile By Id", float64(1)) @@ -158,5 +156,5 @@ func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) { 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 + return user.DeepCopy(), nil } diff --git a/store/localcachelayer/user_layer_test.go b/store/localcachelayer/user_layer_test.go index 9e9f193787..8507acf3c6 100644 --- a/store/localcachelayer/user_layer_test.go +++ b/store/localcachelayer/user_layer_test.go @@ -66,6 +66,50 @@ func TestUserStoreGetProfileByIdsCache(t *testing.T) { _, _ = cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true) mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetProfileByIds", 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) + + storedUsers, err := mockStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, false) + require.Nil(t, err) + + originalProps := make([]model.StringMap, len(storedUsers)) + + for i := 0; i < len(storedUsers); i++ { + originalProps[i] = storedUsers[i].NotifyProps + storedUsers[i].NotifyProps = map[string]string{} + storedUsers[i].NotifyProps["key"] = "somevalue" + } + + cachedUsers, err := cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true) + require.Nil(t, err) + + for i := 0; i < len(storedUsers); i++ { + assert.Equal(t, storedUsers[i], cachedUsers[i]) + if storedUsers[i] == cachedUsers[i] { + assert.Fail(t, "should be different pointers") + } + cachedUsers[i].NotifyProps["key"] = "othervalue" + assert.NotEqual(t, storedUsers[i], cachedUsers[i]) + } + + cachedUsers, err = cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true) + require.Nil(t, err) + for i := 0; i < len(storedUsers); i++ { + assert.Equal(t, storedUsers[i], cachedUsers[i]) + if storedUsers[i] == cachedUsers[i] { + assert.Fail(t, "should be different pointers") + } + cachedUsers[i].NotifyProps["key"] = "othervalue" + assert.NotEqual(t, storedUsers[i], cachedUsers[i]) + } + + for i := 0; i < len(storedUsers); i++ { + storedUsers[i].NotifyProps = originalProps[i] + } + }) } func TestUserStoreProfilesInChannelCache(t *testing.T) { @@ -167,4 +211,37 @@ func TestUserStoreGetCache(t *testing.T) { _, _ = 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 + }) }