From 385462d36c75e809ee082d3432941f938cbdffc9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 10 Nov 2022 14:43:53 +0800 Subject: [PATCH] Fix dashboard ignored system setting cache (#21621) This is a performance regression from #18058 Signed-off-by: Andrew Thornton Co-authored-by: Andrew Thornton --- models/avatars/avatar.go | 11 ++- models/system/setting.go | 40 ++++++-- models/system/setting_key.go | 5 + models/user/avatar.go | 6 +- models/user/setting.go | 36 +++++++- models/user/setting_test.go | 2 +- modules/activitypub/user_settings.go | 2 +- modules/cache/cache.go | 131 +++++++++++++++++---------- modules/system/setting.go | 46 ---------- modules/system/user_setting.go | 34 ------- modules/templates/helper.go | 3 +- routers/web/admin/config.go | 7 +- 12 files changed, 172 insertions(+), 151 deletions(-) delete mode 100644 modules/system/setting.go delete mode 100644 modules/system/user_setting.go diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 418e9b9cc..ec3b61131 100644 --- a/models/avatars/avatar.go +++ b/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)) diff --git a/models/system/setting.go b/models/system/setting.go index 9711d38f3..b4011b1b3 100644 --- a/models/system/setting.go +++ b/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)} diff --git a/models/system/setting_key.go b/models/system/setting_key.go index 5a6ea6ed7..14105b89d 100644 --- a/models/system/setting_key.go +++ b/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 +} diff --git a/models/user/avatar.go b/models/user/avatar.go index f73ac56c5..102206f3a 100644 --- a/models/user/avatar.go +++ b/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: diff --git a/models/user/setting.go b/models/user/setting.go index 5fe7c2ec2..896f3c8da 100644 --- a/models/user/setting.go +++ b/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 { diff --git a/models/user/setting_test.go b/models/user/setting_test.go index f0083038d..5a772a8ce 100644 --- a/models/user/setting_test.go +++ b/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) diff --git a/modules/activitypub/user_settings.go b/modules/activitypub/user_settings.go index fc9775b0f..d192b9cdb 100644 --- a/modules/activitypub/user_settings.go +++ b/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 { diff --git a/modules/cache/cache.go b/modules/cache/cache.go index 21d0cd0a0..d98b0a0ce 100644 --- a/modules/cache/cache.go +++ b/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 { + + cached := conn.Get(key) + + if cached == nil { + value, err := getFunc() + if err != nil { return value, err } - err = conn.Put(key, value, setting.CacheService.TTLSeconds()) - if err != nil { - return "", 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 { + + cached := conn.Get(key) + + if cached == nil { + value, err := getFunc() + if err != nil { return value, err } - err = conn.Put(key, value, setting.CacheService.TTLSeconds()) - if err != nil { - return 0, 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 { + + cached := conn.Get(key) + + if cached == nil { + value, err := getFunc() + if err != nil { return value, err } - err = conn.Put(key, value, setting.CacheService.TTLSeconds()) - if err != nil { - return 0, 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()) } } diff --git a/modules/system/setting.go b/modules/system/setting.go deleted file mode 100644 index aebf24a50..000000000 --- a/modules/system/setting.go +++ /dev/null @@ -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, - }) -} diff --git a/modules/system/user_setting.go b/modules/system/user_setting.go deleted file mode 100644 index eaf146c08..000000000 --- a/modules/system/user_setting.go +++ /dev/null @@ -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) -} diff --git a/modules/templates/helper.go b/modules/templates/helper.go index c5434b7c6..d0866d3e2 100644 --- a/modules/templates/helper.go +++ b/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 diff --git a/routers/web/admin/config.go b/routers/web/admin/config.go index 614d3d4f6..792eec8d5 100644 --- a/routers/web/admin/config.go +++ b/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),