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
```

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 {

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) {

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)
 }

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)
 

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])
 	}