Fix dashboard ignored system setting cache (#21621)

This is a performance regression from #18058

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Andrew Thornton <art27@cantab.net>

models/avatars/avatar.go

@@ -150,10 +150,11 @@ func generateEmailAvatarLink(email string, size int, final bool) string {
 		return DefaultAvatarLink()
 	}
 
-	enableFederatedAvatar, _ := system_model.GetSetting(system_model.KeyPictureEnableFederatedAvatar)
+	enableFederatedAvatarSetting, _ := system_model.GetSetting(system_model.KeyPictureEnableFederatedAvatar)
+	enableFederatedAvatar := enableFederatedAvatarSetting.GetValueBool()
 
 	var err error
-	if enableFederatedAvatar != nil && enableFederatedAvatar.GetValueBool() && system_model.LibravatarService != nil {
+	if enableFederatedAvatar && system_model.LibravatarService != nil {
 		emailHash := saveEmailHash(email)
 		if final {
 			// for final link, we can spend more time on slow external query
@@ -171,8 +172,10 @@ func generateEmailAvatarLink(email string, size int, final bool) string {
 		return urlStr
 	}
 
-	disableGravatar, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar)
-	if disableGravatar != nil && !disableGravatar.GetValueBool() {
+	disableGravatarSetting, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar)
+
+	disableGravatar := disableGravatarSetting.GetValueBool()
+	if !disableGravatar {
 		// copy GravatarSourceURL, because we will modify its Path.
 		avatarURLCopy := *system_model.GravatarSourceURL
 		avatarURLCopy.Path = path.Join(avatarURLCopy.Path, HashEmail(email))

models/system/setting.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/cache"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
 
@@ -35,6 +36,10 @@ func (s *Setting) TableName() string {
 }
 
 func (s *Setting) GetValueBool() bool {
+	if s == nil {
+		return false
+	}
+
 	b, _ := strconv.ParseBool(s.SettingValue)
 	return b
 }
@@ -75,8 +80,8 @@ func IsErrDataExpired(err error) bool {
 	return ok
 }
 
-// GetSetting returns specific setting
-func GetSetting(key string) (*Setting, error) {
+// GetSettingNoCache returns specific setting without using the cache
+func GetSettingNoCache(key string) (*Setting, error) {
 	v, err := GetSettings([]string{key})
 	if err != nil {
 		return nil, err
@@ -87,6 +92,24 @@ func GetSetting(key string) (*Setting, error) {
 	return v[key], nil
 }
 
+// GetSetting returns the setting value via the key
+func GetSetting(key string) (*Setting, error) {
+	return cache.Get(genSettingCacheKey(key), func() (*Setting, error) {
+		res, err := GetSettingNoCache(key)
+		if err != nil {
+			return nil, err
+		}
+		return res, 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()
+}
+
 // GetSettings returns specific settings
 func GetSettings(keys []string) (map[string]*Setting, error) {
 	for i := 0; i < len(keys); i++ {
@@ -139,12 +162,13 @@ func GetAllSettings() (AllSettings, error) {
 
 // DeleteSetting deletes a specific setting for a user
 func DeleteSetting(setting *Setting) error {
+	cache.Remove(genSettingCacheKey(setting.SettingKey))
 	_, err := db.GetEngine(db.DefaultContext).Delete(setting)
 	return err
 }
 
 func SetSettingNoVersion(key, value string) error {
-	s, err := GetSetting(key)
+	s, err := GetSettingNoCache(key)
 	if IsErrSettingIsNotExist(err) {
 		return SetSetting(&Setting{
 			SettingKey:   key,
@@ -160,9 +184,13 @@ func SetSettingNoVersion(key, value string) error {
 
 // SetSetting updates a users' setting for a specific key
 func SetSetting(setting *Setting) error {
-	if err := upsertSettingValue(strings.ToLower(setting.SettingKey), setting.SettingValue, setting.Version); err != nil {
+	_, err := cache.Set(genSettingCacheKey(setting.SettingKey), func() (*Setting, error) {
+		return setting, upsertSettingValue(strings.ToLower(setting.SettingKey), setting.SettingValue, setting.Version)
+	})
+	if err != nil {
 		return err
 	}
+
 	setting.Version++
 	return nil
 }
@@ -213,7 +241,7 @@ var (
 
 func Init() error {
 	var disableGravatar bool
-	disableGravatarSetting, err := GetSetting(KeyPictureDisableGravatar)
+	disableGravatarSetting, err := GetSettingNoCache(KeyPictureDisableGravatar)
 	if IsErrSettingIsNotExist(err) {
 		disableGravatar = setting.GetDefaultDisableGravatar()
 		disableGravatarSetting = &Setting{SettingValue: strconv.FormatBool(disableGravatar)}
@@ -224,7 +252,7 @@ func Init() error {
 	}
 
 	var enableFederatedAvatar bool
-	enableFederatedAvatarSetting, err := GetSetting(KeyPictureEnableFederatedAvatar)
+	enableFederatedAvatarSetting, err := GetSettingNoCache(KeyPictureEnableFederatedAvatar)
 	if IsErrSettingIsNotExist(err) {
 		enableFederatedAvatar = setting.GetDefaultEnableFederatedAvatar(disableGravatar)
 		enableFederatedAvatarSetting = &Setting{SettingValue: strconv.FormatBool(enableFederatedAvatar)}

models/system/setting_key.go

@@ -9,3 +9,8 @@ const (
 	KeyPictureDisableGravatar       = "picture.disable_gravatar"
 	KeyPictureEnableFederatedAvatar = "picture.enable_federated_avatar"
 )
+
+// genSettingCacheKey returns the cache key for some configuration
+func genSettingCacheKey(key string) string {
+	return "system.setting." + key
+}

models/user/avatar.go

@@ -68,11 +68,9 @@ func (u *User) AvatarLinkWithSize(size int) string {
 	useLocalAvatar := false
 	autoGenerateAvatar := false
 
-	var disableGravatar bool
 	disableGravatarSetting, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar)
-	if disableGravatarSetting != nil {
-		disableGravatar = disableGravatarSetting.GetValueBool()
-	}
+
+	disableGravatar := disableGravatarSetting.GetValueBool()
 
 	switch {
 	case u.UseCustomAvatar:

models/user/setting.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/cache"
 
 	"xorm.io/builder"
 )
@@ -47,9 +48,25 @@ func IsErrUserSettingIsNotExist(err error) bool {
 	return ok
 }
 
-// GetSetting returns specific setting
+// genSettingCacheKey returns the cache key for some configuration
+func genSettingCacheKey(userID int64, key string) string {
+	return fmt.Sprintf("user_%d.setting.%s", userID, key)
+}
+
+// GetSetting returns the setting value via the key
 func GetSetting(uid int64, key string) (*Setting, error) {
-	v, err := GetUserSettings(uid, []string{key})
+	return cache.Get(genSettingCacheKey(uid, key), func() (*Setting, error) {
+		res, err := GetSettingNoCache(uid, key)
+		if err != nil {
+			return nil, err
+		}
+		return res, nil
+	})
+}
+
+// GetSettingNoCache returns specific setting without using the cache
+func GetSettingNoCache(uid int64, key string) (*Setting, error) {
+	v, err := GetSettings(uid, []string{key})
 	if err != nil {
 		return nil, err
 	}
@@ -59,8 +76,8 @@ func GetSetting(uid int64, key string) (*Setting, error) {
 	return v[key], nil
 }
 
-// GetUserSettings returns specific settings from user
-func GetUserSettings(uid int64, keys []string) (map[string]*Setting, error) {
+// GetSettings returns specific settings from user
+func GetSettings(uid int64, keys []string) (map[string]*Setting, error) {
 	settings := make([]*Setting, 0, len(keys))
 	if err := db.GetEngine(db.DefaultContext).
 		Where("user_id=?", uid).
@@ -105,6 +122,7 @@ func GetUserSetting(userID int64, key string, def ...string) (string, error) {
 	if err := validateUserSettingKey(key); err != nil {
 		return "", err
 	}
+
 	setting := &Setting{UserID: userID, SettingKey: key}
 	has, err := db.GetEngine(db.DefaultContext).Get(setting)
 	if err != nil {
@@ -124,7 +142,10 @@ func DeleteUserSetting(userID int64, key string) error {
 	if err := validateUserSettingKey(key); err != nil {
 		return err
 	}
+
+	cache.Remove(genSettingCacheKey(userID, key))
 	_, err := db.GetEngine(db.DefaultContext).Delete(&Setting{UserID: userID, SettingKey: key})
+
 	return err
 }
 
@@ -133,7 +154,12 @@ func SetUserSetting(userID int64, key, value string) error {
 	if err := validateUserSettingKey(key); err != nil {
 		return err
 	}
-	return upsertUserSettingValue(userID, key, value)
+
+	_, err := cache.Set(genSettingCacheKey(userID, key), func() (string, error) {
+		return value, upsertUserSettingValue(userID, key, value)
+	})
+
+	return err
 }
 
 func upsertUserSettingValue(userID int64, key, value string) error {

models/user/setting_test.go

@@ -27,7 +27,7 @@ func TestSettings(t *testing.T) {
 	assert.NoError(t, err)
 
 	// get specific setting
-	settings, err := user_model.GetUserSettings(99, []string{keyName})
+	settings, err := user_model.GetSettings(99, []string{keyName})
 	assert.NoError(t, err)
 	assert.Len(t, settings, 1)
 	assert.EqualValues(t, newSetting.SettingValue, settings[keyName].SettingValue)

modules/activitypub/user_settings.go

@@ -11,7 +11,7 @@ import (
 // GetKeyPair function returns a user's private and public keys
 func GetKeyPair(user *user_model.User) (pub, priv string, err error) {
 	var settings map[string]*user_model.Setting
-	settings, err = user_model.GetUserSettings(user.ID, []string{user_model.UserActivityPubPrivPem, user_model.UserActivityPubPubPem})
+	settings, err = user_model.GetSettings(user.ID, []string{user_model.UserActivityPubPrivPem, user_model.UserActivityPubPubPem})
 	if err != nil {
 		return
 	} else if len(settings) == 0 {

modules/cache/cache.go

@@ -46,32 +46,64 @@ 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 {
 		return getFunc()
 	}
-	if !conn.IsExist(key) {
-		var (
-			value string
-			err   error
-		)
-		if value, err = getFunc(); err != nil {
-			return value, err
-		}
-		err = conn.Put(key, value, setting.CacheService.TTLSeconds())
+
+	cached := conn.Get(key)
+
+	if cached == nil {
+		value, err := getFunc()
 		if err != nil {
-			return "", err
+			return value, err
 		}
+		return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
 	}
-	value := conn.Get(key)
-	if v, ok := value.(string); ok {
-		return v, nil
+
+	if value, ok := cached.(string); ok {
+		return value, nil
 	}
-	if v, ok := value.(fmt.Stringer); ok {
-		return v.String(), nil
+
+	if stringer, ok := cached.(fmt.Stringer); ok {
+		return stringer.String(), nil
 	}
-	return fmt.Sprintf("%s", conn.Get(key)), nil
+
+	return fmt.Sprintf("%s", cached), nil
 }
 
 // GetInt returns key value from cache with callback when no key exists in cache
@@ -79,30 +111,33 @@ func GetInt(key string, getFunc func() (int, error)) (int, error) {
 	if conn == nil || setting.CacheService.TTL == 0 {
 		return getFunc()
 	}
-	if !conn.IsExist(key) {
-		var (
-			value int
-			err   error
-		)
-		if value, err = getFunc(); err != nil {
-			return value, err
-		}
-		err = conn.Put(key, value, setting.CacheService.TTLSeconds())
+
+	cached := conn.Get(key)
+
+	if cached == nil {
+		value, err := getFunc()
 		if err != nil {
-			return 0, err
+			return value, err
 		}
+
+		return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
 	}
-	switch value := conn.Get(key).(type) {
+
+	switch v := cached.(type) {
 	case int:
-		return value, nil
+		return v, nil
 	case string:
-		v, err := strconv.Atoi(value)
+		value, err := strconv.Atoi(v)
 		if err != nil {
 			return 0, err
 		}
-		return v, nil
+		return value, nil
 	default:
-		return 0, fmt.Errorf("Unsupported cached value type: %v", value)
+		value, err := getFunc()
+		if err != nil {
+			return value, err
+		}
+		return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
 	}
 }
 
@@ -111,30 +146,34 @@ func GetInt64(key string, getFunc func() (int64, error)) (int64, error) {
 	if conn == nil || setting.CacheService.TTL == 0 {
 		return getFunc()
 	}
-	if !conn.IsExist(key) {
-		var (
-			value int64
-			err   error
-		)
-		if value, err = getFunc(); err != nil {
-			return value, err
-		}
-		err = conn.Put(key, value, setting.CacheService.TTLSeconds())
+
+	cached := conn.Get(key)
+
+	if cached == nil {
+		value, err := getFunc()
 		if err != nil {
-			return 0, err
+			return value, err
 		}
+
+		return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
 	}
-	switch value := conn.Get(key).(type) {
+
+	switch v := conn.Get(key).(type) {
 	case int64:
-		return value, nil
+		return v, nil
 	case string:
-		v, err := strconv.ParseInt(value, 10, 64)
+		value, err := strconv.ParseInt(v, 10, 64)
 		if err != nil {
 			return 0, err
 		}
-		return v, nil
+		return value, nil
 	default:
-		return 0, fmt.Errorf("Unsupported cached value type: %v", value)
+		value, err := getFunc()
+		if err != nil {
+			return value, err
+		}
+
+		return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
 	}
 }
 

modules/system/setting.go

@@ -1,46 +0,0 @@
-// Copyright 2021 The Gitea Authors. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
-package system
-
-import (
-	"strconv"
-
-	"code.gitea.io/gitea/models/system"
-	"code.gitea.io/gitea/modules/cache"
-)
-
-func genKey(key string) string {
-	return "system.setting." + key
-}
-
-// GetSetting returns the setting value via the key
-func GetSetting(key string) (string, error) {
-	return cache.GetString(genKey(key), func() (string, error) {
-		res, err := system.GetSetting(key)
-		if err != nil {
-			return "", err
-		}
-		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)
-	b, _ := strconv.ParseBool(s)
-	return b
-}
-
-// SetSetting sets the setting value
-func SetSetting(key, value string, version int) error {
-	cache.Remove(genKey(key))
-
-	return system.SetSetting(&system.Setting{
-		SettingKey:   key,
-		SettingValue: value,
-		Version:      version,
-	})
-}

modules/system/user_setting.go

@@ -1,34 +0,0 @@
-// Copyright 2021 The Gitea Authors. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
-package system
-
-import (
-	"fmt"
-
-	"code.gitea.io/gitea/models/user"
-	"code.gitea.io/gitea/modules/cache"
-)
-
-func genUserKey(userID int64, key string) string {
-	return fmt.Sprintf("user_%d.setting.%s", userID, key)
-}
-
-// GetUserSetting returns the user setting value via the key
-func GetUserSetting(userID int64, key string) (string, error) {
-	return cache.GetString(genUserKey(userID, key), func() (string, error) {
-		res, err := user.GetSetting(userID, key)
-		if err != nil {
-			return "", err
-		}
-		return res.SettingValue, nil
-	})
-}
-
-// SetUserSetting sets the user setting value
-func SetUserSetting(userID int64, key, value string) error {
-	cache.Remove(genUserKey(userID, key))
-
-	return user.SetUserSetting(userID, key, value)
-}

modules/templates/helper.go

@@ -43,7 +43,6 @@ import (
 	"code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/svg"
-	system_module "code.gitea.io/gitea/modules/system"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/gitdiff"
@@ -88,7 +87,7 @@ func NewFuncMap() []template.FuncMap {
 			return setting.AssetVersion
 		},
 		"DisableGravatar": func() bool {
-			return system_module.GetSettingBool(system_model.KeyPictureDisableGravatar)
+			return system_model.GetSettingBool(system_model.KeyPictureDisableGravatar)
 		},
 		"DefaultShowFullName": func() bool {
 			return setting.UI.DefaultShowFullName

routers/web/admin/config.go

@@ -18,7 +18,6 @@ import (
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
-	system_module "code.gitea.io/gitea/modules/system"
 	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/mailer"
 
@@ -203,7 +202,11 @@ func ChangeConfig(ctx *context.Context) {
 	value := ctx.FormString("value")
 	version := ctx.FormInt("version")
 
-	if err := system_module.SetSetting(key, value, version); err != nil {
+	if err := system_model.SetSetting(&system_model.Setting{
+		SettingKey:   key,
+		SettingValue: value,
+		Version:      version,
+	}); err != nil {
 		log.Error("set setting failed: %v", err)
 		ctx.JSON(http.StatusOK, map[string]string{
 			"err": ctx.Tr("admin.config.set_setting_failed", key),