From a190fe8503f6ed4d2bfac1149e433ef798670ab1 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Mon, 14 Oct 2024 21:03:01 +0530 Subject: [PATCH] MM-59947: Remove the remaining **model.User special casing (#28707) We remove the remaining special casing for **model.User and add unit tests to lock in the behavior. Additional load tests were done locally to confirm there are no hidden code paths left out. https://mattermost.atlassian.net/browse/MM-59947 ```release-note NONE ``` --- .../store/localcachelayer/user_layer.go | 12 +++--- .../store/localcachelayer/user_layer_test.go | 39 ++++++++++++++++++- server/platform/services/cache/lru.go | 16 -------- server/platform/services/cache/lru_test.go | 6 +-- server/platform/services/cache/redis.go | 25 ------------ 5 files changed, 47 insertions(+), 51 deletions(-) diff --git a/server/channels/store/localcachelayer/user_layer.go b/server/channels/store/localcachelayer/user_layer.go index 85744995c3..bb130b2bf6 100644 --- a/server/channels/store/localcachelayer/user_layer.go +++ b/server/channels/store/localcachelayer/user_layer.go @@ -173,7 +173,7 @@ func (s *LocalCacheUserStore) GetProfileByIds(ctx context.Context, userIds []str remainingUserIds := make([]string, 0) fromMaster := false - toPass := allocateCacheTargets[*model.User](len(userIds)) + toPass := allocateCacheTargets[model.User](len(userIds)) errs := s.rootStore.doMultiReadCache(s.rootStore.userProfileByIdsCache, userIds, toPass) for i, err := range errs { if err != nil { @@ -190,7 +190,7 @@ func (s *LocalCacheUserStore) GetProfileByIds(ctx context.Context, userIds []str s.userProfileByIdsMut.Unlock() remainingUserIds = append(remainingUserIds, userIds[i]) } else { - gotUser := *(toPass[i].(**model.User)) + gotUser := toPass[i].(*model.User) if (gotUser != nil) && (options.Since == 0 || gotUser.UpdateAt > options.Since) { users = append(users, gotUser) } else if gotUser == nil { @@ -221,9 +221,9 @@ func (s *LocalCacheUserStore) GetProfileByIds(ctx context.Context, userIds []str // if it is present. Otherwise, it fetches the entry from the store and stores it in the // cache. func (s *LocalCacheUserStore) Get(ctx context.Context, id string) (*model.User, error) { - var cacheItem *model.User + var cacheItem model.User if err := s.rootStore.doStandardReadCache(s.rootStore.userProfileByIdsCache, id, &cacheItem); err == nil { - return cacheItem, nil + return &cacheItem, nil } // If it was invalidated, then we need to query master. @@ -255,7 +255,7 @@ func (s *LocalCacheUserStore) GetMany(ctx context.Context, ids []string) ([]*mod uniqIDs := dedup(ids) fromMaster := false - toPass := allocateCacheTargets[*model.User](len(uniqIDs)) + toPass := allocateCacheTargets[model.User](len(uniqIDs)) errs := s.rootStore.doMultiReadCache(s.rootStore.userProfileByIdsCache, uniqIDs, toPass) for i, err := range errs { if err != nil { @@ -272,7 +272,7 @@ func (s *LocalCacheUserStore) GetMany(ctx context.Context, ids []string) ([]*mod s.userProfileByIdsMut.Unlock() notCachedUserIds = append(notCachedUserIds, uniqIDs[i]) } else { - gotUser := *(toPass[i].(**model.User)) + gotUser := toPass[i].(*model.User) if gotUser != nil { cachedUsers = append(cachedUsers, gotUser) } else { diff --git a/server/channels/store/localcachelayer/user_layer_test.go b/server/channels/store/localcachelayer/user_layer_test.go index 5521f3c1b8..9ad82bb33d 100644 --- a/server/channels/store/localcachelayer/user_layer_test.go +++ b/server/channels/store/localcachelayer/user_layer_test.go @@ -8,14 +8,15 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" - "github.com/mattermost/mattermost/server/public/plugin/plugintest/mock" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/v8/channels/store" "github.com/mattermost/mattermost/server/v8/channels/store/storetest" "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks" + cmocks "github.com/mattermost/mattermost/server/v8/platform/services/cache/mocks" ) func TestUserStore(t *testing.T) { @@ -118,6 +119,27 @@ func TestUserStoreCache(t *testing.T) { storedUsers[i].NotifyProps = originalProps[i] } }) + + t.Run("assert **model.User not passed", func(t *testing.T) { + mockStore := getMockStore(t) + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider, logger) + require.NoError(t, err) + + cmock := cmocks.NewCache(t) + cmock.On("GetMulti", []string{"123"}, mock.MatchedBy(func(values []any) bool { + if len(values) != 1 { + return false + } + _, ok := values[0].(*model.User) + return ok + })).Return(nil) + + cachedStore.user.rootStore.userProfileByIdsCache = cmock + + _, err = cachedStore.User().GetProfileByIds(context.Background(), fakeUserIds, &store.UserGetByIdsOpts{}, true) + require.NoError(t, err) + }) } func TestUserStoreGetAllProfiles(t *testing.T) { @@ -301,6 +323,21 @@ func TestUserStoreGetCache(t *testing.T) { storedUser.NotifyProps = originalProps }) + + t.Run("assert **model.User not passed", func(t *testing.T) { + mockStore := getMockStore(t) + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider, logger) + require.NoError(t, err) + + cmock := cmocks.NewCache(t) + cmock.On("Get", "123", mock.AnythingOfType("*model.User")).Return(nil) + + cachedStore.user.rootStore.userProfileByIdsCache = cmock + + _, err = cachedStore.User().Get(context.Background(), fakeUserId) + require.NoError(t, err) + }) } func TestUserStoreGetManyCache(t *testing.T) { diff --git a/server/platform/services/cache/lru.go b/server/platform/services/cache/lru.go index a23faef5af..2d80a31e36 100644 --- a/server/platform/services/cache/lru.go +++ b/server/platform/services/cache/lru.go @@ -206,22 +206,6 @@ func (l *LRU) get(key string, value any) error { return err } - // This is ugly and makes the cache package aware of the model package. - // But this is due to 2 things. - // 1. The msgp package works on methods on structs rather than functions. - // 2. Our cache interface passes pointers to empty pointers, and not pointers - // to values. This is mainly how all our model structs are passed around. - // It might be technically possible to use values _just_ for hot structs - // like these and then return a pointer while returning from the cache function, - // but it will make the codebase inconsistent, and has some edge-cases to take care of. - switch v := value.(type) { - case **model.User: - var u model.User - _, err := u.UnmarshalMsg(val) - *v = &u - return err - } - // Slow path for other structs. return msgpack.Unmarshal(val, value) } diff --git a/server/platform/services/cache/lru_test.go b/server/platform/services/cache/lru_test.go index 620082b6a1..bf551c50d0 100644 --- a/server/platform/services/cache/lru_test.go +++ b/server/platform/services/cache/lru_test.go @@ -286,16 +286,16 @@ func TestLRUMarshalUnMarshal(t *testing.T) { err = l.SetWithDefaultExpiry("user", user) require.NoError(t, err) - var u *model.User + var u model.User err = l.Get("user", &u) require.NoError(t, err) // msgp returns an empty map instead of a nil map. // This does not make an actual difference in terms of functionality. u.Timezone = nil - require.Equal(t, user, u) + require.Equal(t, user, &u) tt := make(model.UserMap) - tt["1"] = u + tt["1"] = &u err = l.SetWithDefaultExpiry("mm", tt) require.NoError(t, err) diff --git a/server/platform/services/cache/redis.go b/server/platform/services/cache/redis.go index ba6a50992c..f366b974d3 100644 --- a/server/platform/services/cache/redis.go +++ b/server/platform/services/cache/redis.go @@ -169,22 +169,6 @@ func (r *Redis) Get(key string, value any) error { return err } - // This is ugly and makes the cache package aware of the model package. - // But this is due to 2 things. - // 1. The msgp package works on methods on structs rather than functions. - // 2. Our cache interface passes pointers to empty pointers, and not pointers - // to values. This is mainly how all our model structs are passed around. - // It might be technically possible to use values _just_ for hot structs - // like these and then return a pointer while returning from the cache function, - // but it will make the codebase inconsistent, and has some edge-cases to take care of. - switch v := value.(type) { - case **model.User: - var u model.User - _, err := u.UnmarshalMsg(bytesVal) - *v = &u - return err - } - // Slow path for other structs. return msgpack.Unmarshal(bytesVal, value) } @@ -256,15 +240,6 @@ func (r *Redis) GetMulti(keys []string, values []any) []error { continue } - switch v := values[i].(type) { - case **model.User: - var u model.User - _, err := u.UnmarshalMsg(bytesVal) - *v = &u - errs[i] = err - continue - } - // Slow path for other structs. errs[i] = msgpack.Unmarshal(bytesVal, values[i]) }