Skip to content

Commit 385462d

Browse files
lunnyzeripath
lunny
and
zeripath
authoredNov 10, 2022
Fix dashboard ignored system setting cache (#21621)
This is a performance regression from #18058 Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: Andrew Thornton <[email protected]>
·
v1.25.0-devv1.19.0-rc0
1 parent ce5aafb commit 385462d

File tree

12 files changed

+172
-151
lines changed

12 files changed

+172
-151
lines changed
 

‎models/avatars/avatar.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,11 @@ func generateEmailAvatarLink(email string, size int, final bool) string {
150150
return DefaultAvatarLink()
151151
}
152152

153-
enableFederatedAvatar, _ := system_model.GetSetting(system_model.KeyPictureEnableFederatedAvatar)
153+
enableFederatedAvatarSetting, _ := system_model.GetSetting(system_model.KeyPictureEnableFederatedAvatar)
154+
enableFederatedAvatar := enableFederatedAvatarSetting.GetValueBool()
154155

155156
var err error
156-
if enableFederatedAvatar != nil && enableFederatedAvatar.GetValueBool() && system_model.LibravatarService != nil {
157+
if enableFederatedAvatar && system_model.LibravatarService != nil {
157158
emailHash := saveEmailHash(email)
158159
if final {
159160
// 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 {
171172
return urlStr
172173
}
173174

174-
disableGravatar, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar)
175-
if disableGravatar != nil && !disableGravatar.GetValueBool() {
175+
disableGravatarSetting, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar)
176+
177+
disableGravatar := disableGravatarSetting.GetValueBool()
178+
if !disableGravatar {
176179
// copy GravatarSourceURL, because we will modify its Path.
177180
avatarURLCopy := *system_model.GravatarSourceURL
178181
avatarURLCopy.Path = path.Join(avatarURLCopy.Path, HashEmail(email))

‎models/system/setting.go

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"strings"
1313

1414
"code.gitea.io/gitea/models/db"
15+
"code.gitea.io/gitea/modules/cache"
1516
"code.gitea.io/gitea/modules/setting"
1617
"code.gitea.io/gitea/modules/timeutil"
1718

@@ -35,6 +36,10 @@ func (s *Setting) TableName() string {
3536
}
3637

3738
func (s *Setting) GetValueBool() bool {
39+
if s == nil {
40+
return false
41+
}
42+
3843
b, _ := strconv.ParseBool(s.SettingValue)
3944
return b
4045
}
@@ -75,8 +80,8 @@ func IsErrDataExpired(err error) bool {
7580
return ok
7681
}
7782

78-
// GetSetting returns specific setting
79-
func GetSetting(key string) (*Setting, error) {
83+
// GetSettingNoCache returns specific setting without using the cache
84+
func GetSettingNoCache(key string) (*Setting, error) {
8085
v, err := GetSettings([]string{key})
8186
if err != nil {
8287
return nil, err
@@ -87,6 +92,24 @@ func GetSetting(key string) (*Setting, error) {
8792
return v[key], nil
8893
}
8994

95+
// GetSetting returns the setting value via the key
96+
func GetSetting(key string) (*Setting, error) {
97+
return cache.Get(genSettingCacheKey(key), func() (*Setting, error) {
98+
res, err := GetSettingNoCache(key)
99+
if err != nil {
100+
return nil, err
101+
}
102+
return res, nil
103+
})
104+
}
105+
106+
// GetSettingBool return bool value of setting,
107+
// none existing keys and errors are ignored and result in false
108+
func GetSettingBool(key string) bool {
109+
s, _ := GetSetting(key)
110+
return s.GetValueBool()
111+
}
112+
90113
// GetSettings returns specific settings
91114
func GetSettings(keys []string) (map[string]*Setting, error) {
92115
for i := 0; i < len(keys); i++ {
@@ -139,12 +162,13 @@ func GetAllSettings() (AllSettings, error) {
139162

140163
// DeleteSetting deletes a specific setting for a user
141164
func DeleteSetting(setting *Setting) error {
165+
cache.Remove(genSettingCacheKey(setting.SettingKey))
142166
_, err := db.GetEngine(db.DefaultContext).Delete(setting)
143167
return err
144168
}
145169

146170
func SetSettingNoVersion(key, value string) error {
147-
s, err := GetSetting(key)
171+
s, err := GetSettingNoCache(key)
148172
if IsErrSettingIsNotExist(err) {
149173
return SetSetting(&Setting{
150174
SettingKey: key,
@@ -160,9 +184,13 @@ func SetSettingNoVersion(key, value string) error {
160184

161185
// SetSetting updates a users' setting for a specific key
162186
func SetSetting(setting *Setting) error {
163-
if err := upsertSettingValue(strings.ToLower(setting.SettingKey), setting.SettingValue, setting.Version); err != nil {
187+
_, err := cache.Set(genSettingCacheKey(setting.SettingKey), func() (*Setting, error) {
188+
return setting, upsertSettingValue(strings.ToLower(setting.SettingKey), setting.SettingValue, setting.Version)
189+
})
190+
if err != nil {
164191
return err
165192
}
193+
166194
setting.Version++
167195
return nil
168196
}
@@ -213,7 +241,7 @@ var (
213241

214242
func Init() error {
215243
var disableGravatar bool
216-
disableGravatarSetting, err := GetSetting(KeyPictureDisableGravatar)
244+
disableGravatarSetting, err := GetSettingNoCache(KeyPictureDisableGravatar)
217245
if IsErrSettingIsNotExist(err) {
218246
disableGravatar = setting.GetDefaultDisableGravatar()
219247
disableGravatarSetting = &Setting{SettingValue: strconv.FormatBool(disableGravatar)}
@@ -224,7 +252,7 @@ func Init() error {
224252
}
225253

226254
var enableFederatedAvatar bool
227-
enableFederatedAvatarSetting, err := GetSetting(KeyPictureEnableFederatedAvatar)
255+
enableFederatedAvatarSetting, err := GetSettingNoCache(KeyPictureEnableFederatedAvatar)
228256
if IsErrSettingIsNotExist(err) {
229257
enableFederatedAvatar = setting.GetDefaultEnableFederatedAvatar(disableGravatar)
230258
enableFederatedAvatarSetting = &Setting{SettingValue: strconv.FormatBool(enableFederatedAvatar)}

‎models/system/setting_key.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,8 @@ const (
99
KeyPictureDisableGravatar = "picture.disable_gravatar"
1010
KeyPictureEnableFederatedAvatar = "picture.enable_federated_avatar"
1111
)
12+
13+
// genSettingCacheKey returns the cache key for some configuration
14+
func genSettingCacheKey(key string) string {
15+
return "system.setting." + key
16+
}

‎models/user/avatar.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,9 @@ func (u *User) AvatarLinkWithSize(size int) string {
6868
useLocalAvatar := false
6969
autoGenerateAvatar := false
7070

71-
var disableGravatar bool
7271
disableGravatarSetting, _ := system_model.GetSetting(system_model.KeyPictureDisableGravatar)
73-
if disableGravatarSetting != nil {
74-
disableGravatar = disableGravatarSetting.GetValueBool()
75-
}
72+
73+
disableGravatar := disableGravatarSetting.GetValueBool()
7674

7775
switch {
7876
case u.UseCustomAvatar:

‎models/user/setting.go

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"strings"
1111

1212
"code.gitea.io/gitea/models/db"
13+
"code.gitea.io/gitea/modules/cache"
1314

1415
"xorm.io/builder"
1516
)
@@ -47,9 +48,25 @@ func IsErrUserSettingIsNotExist(err error) bool {
4748
return ok
4849
}
4950

50-
// GetSetting returns specific setting
51+
// genSettingCacheKey returns the cache key for some configuration
52+
func genSettingCacheKey(userID int64, key string) string {
53+
return fmt.Sprintf("user_%d.setting.%s", userID, key)
54+
}
55+
56+
// GetSetting returns the setting value via the key
5157
func GetSetting(uid int64, key string) (*Setting, error) {
52-
v, err := GetUserSettings(uid, []string{key})
58+
return cache.Get(genSettingCacheKey(uid, key), func() (*Setting, error) {
59+
res, err := GetSettingNoCache(uid, key)
60+
if err != nil {
61+
return nil, err
62+
}
63+
return res, nil
64+
})
65+
}
66+
67+
// GetSettingNoCache returns specific setting without using the cache
68+
func GetSettingNoCache(uid int64, key string) (*Setting, error) {
69+
v, err := GetSettings(uid, []string{key})
5370
if err != nil {
5471
return nil, err
5572
}
@@ -59,8 +76,8 @@ func GetSetting(uid int64, key string) (*Setting, error) {
5976
return v[key], nil
6077
}
6178

62-
// GetUserSettings returns specific settings from user
63-
func GetUserSettings(uid int64, keys []string) (map[string]*Setting, error) {
79+
// GetSettings returns specific settings from user
80+
func GetSettings(uid int64, keys []string) (map[string]*Setting, error) {
6481
settings := make([]*Setting, 0, len(keys))
6582
if err := db.GetEngine(db.DefaultContext).
6683
Where("user_id=?", uid).
@@ -105,6 +122,7 @@ func GetUserSetting(userID int64, key string, def ...string) (string, error) {
105122
if err := validateUserSettingKey(key); err != nil {
106123
return "", err
107124
}
125+
108126
setting := &Setting{UserID: userID, SettingKey: key}
109127
has, err := db.GetEngine(db.DefaultContext).Get(setting)
110128
if err != nil {
@@ -124,7 +142,10 @@ func DeleteUserSetting(userID int64, key string) error {
124142
if err := validateUserSettingKey(key); err != nil {
125143
return err
126144
}
145+
146+
cache.Remove(genSettingCacheKey(userID, key))
127147
_, err := db.GetEngine(db.DefaultContext).Delete(&Setting{UserID: userID, SettingKey: key})
148+
128149
return err
129150
}
130151

@@ -133,7 +154,12 @@ func SetUserSetting(userID int64, key, value string) error {
133154
if err := validateUserSettingKey(key); err != nil {
134155
return err
135156
}
136-
return upsertUserSettingValue(userID, key, value)
157+
158+
_, err := cache.Set(genSettingCacheKey(userID, key), func() (string, error) {
159+
return value, upsertUserSettingValue(userID, key, value)
160+
})
161+
162+
return err
137163
}
138164

139165
func upsertUserSettingValue(userID int64, key, value string) error {

‎models/user/setting_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func TestSettings(t *testing.T) {
2727
assert.NoError(t, err)
2828

2929
// get specific setting
30-
settings, err := user_model.GetUserSettings(99, []string{keyName})
30+
settings, err := user_model.GetSettings(99, []string{keyName})
3131
assert.NoError(t, err)
3232
assert.Len(t, settings, 1)
3333
assert.EqualValues(t, newSetting.SettingValue, settings[keyName].SettingValue)

‎modules/activitypub/user_settings.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import (
1111
// GetKeyPair function returns a user's private and public keys
1212
func GetKeyPair(user *user_model.User) (pub, priv string, err error) {
1313
var settings map[string]*user_model.Setting
14-
settings, err = user_model.GetUserSettings(user.ID, []string{user_model.UserActivityPubPrivPem, user_model.UserActivityPubPubPem})
14+
settings, err = user_model.GetSettings(user.ID, []string{user_model.UserActivityPubPrivPem, user_model.UserActivityPubPubPem})
1515
if err != nil {
1616
return
1717
} else if len(settings) == 0 {

‎modules/cache/cache.go

Lines changed: 85 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -46,63 +46,98 @@ func GetCache() mc.Cache {
4646
return conn
4747
}
4848

49+
// Get returns the key value from cache with callback when no key exists in cache
50+
func Get[V interface{}](key string, getFunc func() (V, error)) (V, error) {
51+
if conn == nil || setting.CacheService.TTL == 0 {
52+
return getFunc()
53+
}
54+
55+
cached := conn.Get(key)
56+
if value, ok := cached.(V); ok {
57+
return value, nil
58+
}
59+
60+
value, err := getFunc()
61+
if err != nil {
62+
return value, err
63+
}
64+
65+
return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
66+
}
67+
68+
// Set updates and returns the key value in the cache with callback. The old value is only removed if the updateFunc() is successful
69+
func Set[V interface{}](key string, valueFunc func() (V, error)) (V, error) {
70+
if conn == nil || setting.CacheService.TTL == 0 {
71+
return valueFunc()
72+
}
73+
74+
value, err := valueFunc()
75+
if err != nil {
76+
return value, err
77+
}
78+
79+
return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
80+
}
81+
4982
// GetString returns the key value from cache with callback when no key exists in cache
5083
func GetString(key string, getFunc func() (string, error)) (string, error) {
5184
if conn == nil || setting.CacheService.TTL == 0 {
5285
return getFunc()
5386
}
54-
if !conn.IsExist(key) {
55-
var (
56-
value string
57-
err error
58-
)
59-
if value, err = getFunc(); err != nil {
60-
return value, err
61-
}
62-
err = conn.Put(key, value, setting.CacheService.TTLSeconds())
87+
88+
cached := conn.Get(key)
89+
90+
if cached == nil {
91+
value, err := getFunc()
6392
if err != nil {
64-
return "", err
93+
return value, err
6594
}
95+
return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
6696
}
67-
value := conn.Get(key)
68-
if v, ok := value.(string); ok {
69-
return v, nil
97+
98+
if value, ok := cached.(string); ok {
99+
return value, nil
70100
}
71-
if v, ok := value.(fmt.Stringer); ok {
72-
return v.String(), nil
101+
102+
if stringer, ok := cached.(fmt.Stringer); ok {
103+
return stringer.String(), nil
73104
}
74-
return fmt.Sprintf("%s", conn.Get(key)), nil
105+
106+
return fmt.Sprintf("%s", cached), nil
75107
}
76108

77109
// GetInt returns key value from cache with callback when no key exists in cache
78110
func GetInt(key string, getFunc func() (int, error)) (int, error) {
79111
if conn == nil || setting.CacheService.TTL == 0 {
80112
return getFunc()
81113
}
82-
if !conn.IsExist(key) {
83-
var (
84-
value int
85-
err error
86-
)
87-
if value, err = getFunc(); err != nil {
88-
return value, err
89-
}
90-
err = conn.Put(key, value, setting.CacheService.TTLSeconds())
114+
115+
cached := conn.Get(key)
116+
117+
if cached == nil {
118+
value, err := getFunc()
91119
if err != nil {
92-
return 0, err
120+
return value, err
93121
}
122+
123+
return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
94124
}
95-
switch value := conn.Get(key).(type) {
125+
126+
switch v := cached.(type) {
96127
case int:
97-
return value, nil
128+
return v, nil
98129
case string:
99-
v, err := strconv.Atoi(value)
130+
value, err := strconv.Atoi(v)
100131
if err != nil {
101132
return 0, err
102133
}
103-
return v, nil
134+
return value, nil
104135
default:
105-
return 0, fmt.Errorf("Unsupported cached value type: %v", value)
136+
value, err := getFunc()
137+
if err != nil {
138+
return value, err
139+
}
140+
return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
106141
}
107142
}
108143

@@ -111,30 +146,34 @@ func GetInt64(key string, getFunc func() (int64, error)) (int64, error) {
111146
if conn == nil || setting.CacheService.TTL == 0 {
112147
return getFunc()
113148
}
114-
if !conn.IsExist(key) {
115-
var (
116-
value int64
117-
err error
118-
)
119-
if value, err = getFunc(); err != nil {
120-
return value, err
121-
}
122-
err = conn.Put(key, value, setting.CacheService.TTLSeconds())
149+
150+
cached := conn.Get(key)
151+
152+
if cached == nil {
153+
value, err := getFunc()
123154
if err != nil {
124-
return 0, err
155+
return value, err
125156
}
157+
158+
return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
126159
}
127-
switch value := conn.Get(key).(type) {
160+
161+
switch v := conn.Get(key).(type) {
128162
case int64:
129-
return value, nil
163+
return v, nil
130164
case string:
131-
v, err := strconv.ParseInt(value, 10, 64)
165+
value, err := strconv.ParseInt(v, 10, 64)
132166
if err != nil {
133167
return 0, err
134168
}
135-
return v, nil
169+
return value, nil
136170
default:
137-
return 0, fmt.Errorf("Unsupported cached value type: %v", value)
171+
value, err := getFunc()
172+
if err != nil {
173+
return value, err
174+
}
175+
176+
return value, conn.Put(key, value, setting.CacheService.TTLSeconds())
138177
}
139178
}
140179

‎modules/system/setting.go

Lines changed: 0 additions & 46 deletions
This file was deleted.

‎modules/system/user_setting.go

Lines changed: 0 additions & 34 deletions
This file was deleted.

‎modules/templates/helper.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ import (
4343
"code.gitea.io/gitea/modules/repository"
4444
"code.gitea.io/gitea/modules/setting"
4545
"code.gitea.io/gitea/modules/svg"
46-
system_module "code.gitea.io/gitea/modules/system"
4746
"code.gitea.io/gitea/modules/timeutil"
4847
"code.gitea.io/gitea/modules/util"
4948
"code.gitea.io/gitea/services/gitdiff"
@@ -88,7 +87,7 @@ func NewFuncMap() []template.FuncMap {
8887
return setting.AssetVersion
8988
},
9089
"DisableGravatar": func() bool {
91-
return system_module.GetSettingBool(system_model.KeyPictureDisableGravatar)
90+
return system_model.GetSettingBool(system_model.KeyPictureDisableGravatar)
9291
},
9392
"DefaultShowFullName": func() bool {
9493
return setting.UI.DefaultShowFullName

‎routers/web/admin/config.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"code.gitea.io/gitea/modules/json"
1919
"code.gitea.io/gitea/modules/log"
2020
"code.gitea.io/gitea/modules/setting"
21-
system_module "code.gitea.io/gitea/modules/system"
2221
"code.gitea.io/gitea/modules/util"
2322
"code.gitea.io/gitea/services/mailer"
2423

@@ -203,7 +202,11 @@ func ChangeConfig(ctx *context.Context) {
203202
value := ctx.FormString("value")
204203
version := ctx.FormInt("version")
205204

206-
if err := system_module.SetSetting(key, value, version); err != nil {
205+
if err := system_model.SetSetting(&system_model.Setting{
206+
SettingKey: key,
207+
SettingValue: value,
208+
Version: version,
209+
}); err != nil {
207210
log.Error("set setting failed: %v", err)
208211
ctx.JSON(http.StatusOK, map[string]string{
209212
"err": ctx.Tr("admin.config.set_setting_failed", key),

0 commit comments

Comments
 (0)
Please sign in to comment.