Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix get system setting bug when enabled redis cache (#22295) #22298

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions models/avatars/avatar.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,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 {
Expand All @@ -176,9 +175,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
Expand Down
15 changes: 8 additions & 7 deletions models/system/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,21 +93,22 @@ 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
})
}

// GetSettingBool return bool value of setting,
// 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
Expand Down Expand Up @@ -184,8 +185,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
Expand Down
4 changes: 1 addition & 3 deletions models/user/avatar.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,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:
Expand Down
10 changes: 5 additions & 5 deletions models/user/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,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
})
}

Expand Down Expand Up @@ -155,7 +155,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)
})

Expand Down
33 changes: 0 additions & 33 deletions modules/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,39 +46,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 {
Expand Down