From a1c30740bb04ed55905415dcd195ee9fb97547de Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 2 Jan 2023 00:06:52 +0800 Subject: [PATCH] Fix get system setting bug when enabled redis cache (#22295) Fix #22281 In #21621 , `Get[V]` and `Set[V]` has been introduced, so that cache value will be `*Setting`. For memory cache it's OK. But for redis cache, it can only store `string` for the current implementation. This PR revert some of changes of that and just store or return a `string` for system setting. --- models/avatars/avatar.go | 7 ++----- models/system/setting.go | 15 ++++++++------- models/user/avatar.go | 4 +--- models/user/setting.go | 10 +++++----- modules/cache/cache.go | 33 --------------------------------- 5 files changed, 16 insertions(+), 53 deletions(-) diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 7ccb782fa..438614286 100644 --- a/models/avatars/avatar.go +++ b/models/avatars/avatar.go @@ -153,8 +153,7 @@ func generateEmailAvatarLink(email string, size int, final bool) string { return DefaultAvatarLink() } - enableFederatedAvatarSetting, _ := system_model.GetSetting(system_model.KeyPictureEnableFederatedAvatar) - enableFederatedAvatar := enableFederatedAvatarSetting.GetValueBool() + enableFederatedAvatar := system_model.GetSettingBool(system_model.KeyPictureEnableFederatedAvatar) var err error if enableFederatedAvatar && system_model.LibravatarService != nil { @@ -175,9 +174,7 @@ func generateEmailAvatarLink(email string, size int, final bool) string { return urlStr } - disableGravatarSetting, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar) - - disableGravatar := disableGravatarSetting.GetValueBool() + disableGravatar := system_model.GetSettingBool(system_model.KeyPictureDisableGravatar) if !disableGravatar { // copy GravatarSourceURL, because we will modify its Path. avatarURLCopy := *system_model.GravatarSourceURL diff --git a/models/system/setting.go b/models/system/setting.go index 6f333d6fd..8e16547d9 100644 --- a/models/system/setting.go +++ b/models/system/setting.go @@ -92,13 +92,13 @@ func GetSettingNoCache(key string) (*Setting, error) { } // GetSetting returns the setting value via the key -func GetSetting(key string) (*Setting, error) { - return cache.Get(genSettingCacheKey(key), func() (*Setting, error) { +func GetSetting(key string) (string, error) { + return cache.GetString(genSettingCacheKey(key), func() (string, error) { res, err := GetSettingNoCache(key) if err != nil { - return nil, err + return "", err } - return res, nil + return res.SettingValue, nil }) } @@ -106,7 +106,8 @@ func GetSetting(key string) (*Setting, error) { // none existing keys and errors are ignored and result in false func GetSettingBool(key string) bool { s, _ := GetSetting(key) - return s.GetValueBool() + v, _ := strconv.ParseBool(s) + return v } // GetSettings returns specific settings @@ -183,8 +184,8 @@ func SetSettingNoVersion(key, value string) error { // SetSetting updates a users' setting for a specific key func SetSetting(setting *Setting) error { - _, err := cache.Set(genSettingCacheKey(setting.SettingKey), func() (*Setting, error) { - return setting, upsertSettingValue(strings.ToLower(setting.SettingKey), setting.SettingValue, setting.Version) + _, err := cache.GetString(genSettingCacheKey(setting.SettingKey), func() (string, error) { + return setting.SettingValue, upsertSettingValue(strings.ToLower(setting.SettingKey), setting.SettingValue, setting.Version) }) if err != nil { return err diff --git a/models/user/avatar.go b/models/user/avatar.go index ce25139b5..e6ca49efd 100644 --- a/models/user/avatar.go +++ b/models/user/avatar.go @@ -67,9 +67,7 @@ func (u *User) AvatarLinkWithSize(size int) string { useLocalAvatar := false autoGenerateAvatar := false - disableGravatarSetting, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar) - - disableGravatar := disableGravatarSetting.GetValueBool() + disableGravatar := system_model.GetSettingBool(system_model.KeyPictureDisableGravatar) switch { case u.UseCustomAvatar: diff --git a/models/user/setting.go b/models/user/setting.go index a17483f9a..f5cfef5b3 100644 --- a/models/user/setting.go +++ b/models/user/setting.go @@ -53,13 +53,13 @@ func genSettingCacheKey(userID int64, key string) string { } // GetSetting returns the setting value via the key -func GetSetting(uid int64, key string) (*Setting, error) { - return cache.Get(genSettingCacheKey(uid, key), func() (*Setting, error) { +func GetSetting(uid int64, key string) (string, error) { + return cache.GetString(genSettingCacheKey(uid, key), func() (string, error) { res, err := GetSettingNoCache(uid, key) if err != nil { - return nil, err + return "", err } - return res, nil + return res.SettingValue, nil }) } @@ -154,7 +154,7 @@ func SetUserSetting(userID int64, key, value string) error { return err } - _, err := cache.Set(genSettingCacheKey(userID, key), func() (string, error) { + _, err := cache.GetString(genSettingCacheKey(userID, key), func() (string, error) { return value, upsertUserSettingValue(userID, key, value) }) diff --git a/modules/cache/cache.go b/modules/cache/cache.go index 2e7d5bb60..edaf48313 100644 --- a/modules/cache/cache.go +++ b/modules/cache/cache.go @@ -45,39 +45,6 @@ func GetCache() mc.Cache { return conn } -// Get returns the key value from cache with callback when no key exists in cache -func Get[V interface{}](key string, getFunc func() (V, error)) (V, error) { - if conn == nil || setting.CacheService.TTL == 0 { - return getFunc() - } - - cached := conn.Get(key) - if value, ok := cached.(V); ok { - return value, nil - } - - value, err := getFunc() - if err != nil { - return value, err - } - - return value, conn.Put(key, value, setting.CacheService.TTLSeconds()) -} - -// Set updates and returns the key value in the cache with callback. The old value is only removed if the updateFunc() is successful -func Set[V interface{}](key string, valueFunc func() (V, error)) (V, error) { - if conn == nil || setting.CacheService.TTL == 0 { - return valueFunc() - } - - value, err := valueFunc() - if err != nil { - return value, err - } - - return value, conn.Put(key, value, setting.CacheService.TTLSeconds()) -} - // GetString returns the key value from cache with callback when no key exists in cache func GetString(key string, getFunc func() (string, error)) (string, error) { if conn == nil || setting.CacheService.TTL == 0 {