Skip to content

Commit 9df573b

Browse files
lunnylunny
authored
Fix context cache bug & enable context cache for dashabord commits' authors(#26991) (#27017)
backport #26991 Unfortunately, when a system setting hasn't been stored in the database, it cannot be cached. Meanwhile, this PR also uses context cache for push email avatar display which should avoid to read user table via email address again and again. According to my local test, this should reduce dashboard elapsed time from 150ms -> 80ms .
1 parent b0a405c commit 9df573b

File tree

7 files changed

+62
-84
lines changed

7 files changed

+62
-84
lines changed

‎models/avatars/avatar.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,12 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final
153153
return DefaultAvatarLink()
154154
}
155155

156-
enableFederatedAvatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureEnableFederatedAvatar)
156+
disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
157+
setting.GetDefaultDisableGravatar(),
158+
)
159+
160+
enableFederatedAvatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureEnableFederatedAvatar,
161+
setting.GetDefaultEnableFederatedAvatar(disableGravatar))
157162

158163
var err error
159164
if enableFederatedAvatar && system_model.LibravatarService != nil {
@@ -174,7 +179,6 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final
174179
return urlStr
175180
}
176181

177-
disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar)
178182
if !disableGravatar {
179183
// copy GravatarSourceURL, because we will modify its Path.
180184
avatarURLCopy := *system_model.GravatarSourceURL

‎models/fixtures/system_setting.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
-
22
id: 1
3-
setting_key: 'disable_gravatar'
3+
setting_key: 'picture.disable_gravatar'
44
setting_value: 'false'
55
version: 1
66
created: 1653533198
77
updated: 1653533198
88

99
-
1010
id: 2
11-
setting_key: 'enable_federated_avatar'
11+
setting_key: 'picture.enable_federated_avatar'
1212
setting_value: 'false'
1313
version: 1
1414
created: 1653533198

‎models/system/setting.go

Lines changed: 29 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,14 @@ func GetSetting(ctx context.Context, key string) (*Setting, error) {
9494
const contextCacheKey = "system_setting"
9595

9696
// GetSettingWithCache returns the setting value via the key
97-
func GetSettingWithCache(ctx context.Context, key string) (string, error) {
97+
func GetSettingWithCache(ctx context.Context, key, defaultVal string) (string, error) {
9898
return cache.GetWithContextCache(ctx, contextCacheKey, key, func() (string, error) {
9999
return cache.GetString(genSettingCacheKey(key), func() (string, error) {
100100
res, err := GetSetting(ctx, key)
101101
if err != nil {
102+
if IsErrSettingIsNotExist(err) {
103+
return defaultVal, nil
104+
}
102105
return "", err
103106
}
104107
return res.SettingValue, nil
@@ -108,17 +111,21 @@ func GetSettingWithCache(ctx context.Context, key string) (string, error) {
108111

109112
// GetSettingBool return bool value of setting,
110113
// none existing keys and errors are ignored and result in false
111-
func GetSettingBool(ctx context.Context, key string) bool {
112-
s, _ := GetSetting(ctx, key)
113-
if s == nil {
114-
return false
114+
func GetSettingBool(ctx context.Context, key string, defaultVal bool) (bool, error) {
115+
s, err := GetSetting(ctx, key)
116+
switch {
117+
case err == nil:
118+
v, _ := strconv.ParseBool(s.SettingValue)
119+
return v, nil
120+
case IsErrSettingIsNotExist(err):
121+
return defaultVal, nil
122+
default:
123+
return false, err
115124
}
116-
v, _ := strconv.ParseBool(s.SettingValue)
117-
return v
118125
}
119126

120-
func GetSettingWithCacheBool(ctx context.Context, key string) bool {
121-
s, _ := GetSettingWithCache(ctx, key)
127+
func GetSettingWithCacheBool(ctx context.Context, key string, defaultVal bool) bool {
128+
s, _ := GetSettingWithCache(ctx, key, strconv.FormatBool(defaultVal))
122129
v, _ := strconv.ParseBool(s)
123130
return v
124131
}
@@ -259,52 +266,41 @@ var (
259266
)
260267

261268
func Init(ctx context.Context) error {
262-
var disableGravatar bool
263-
disableGravatarSetting, err := GetSetting(ctx, KeyPictureDisableGravatar)
264-
if IsErrSettingIsNotExist(err) {
265-
disableGravatar = setting_module.GetDefaultDisableGravatar()
266-
disableGravatarSetting = &Setting{SettingValue: strconv.FormatBool(disableGravatar)}
267-
} else if err != nil {
269+
disableGravatar, err := GetSettingBool(ctx, KeyPictureDisableGravatar, setting_module.GetDefaultDisableGravatar())
270+
if err != nil {
268271
return err
269-
} else {
270-
disableGravatar = disableGravatarSetting.GetValueBool()
271272
}
272273

273-
var enableFederatedAvatar bool
274-
enableFederatedAvatarSetting, err := GetSetting(ctx, KeyPictureEnableFederatedAvatar)
275-
if IsErrSettingIsNotExist(err) {
276-
enableFederatedAvatar = setting_module.GetDefaultEnableFederatedAvatar(disableGravatar)
277-
enableFederatedAvatarSetting = &Setting{SettingValue: strconv.FormatBool(enableFederatedAvatar)}
278-
} else if err != nil {
274+
enableFederatedAvatar, err := GetSettingBool(ctx, KeyPictureEnableFederatedAvatar, setting_module.GetDefaultEnableFederatedAvatar(disableGravatar))
275+
if err != nil {
279276
return err
280-
} else {
281-
enableFederatedAvatar = disableGravatarSetting.GetValueBool()
282277
}
283278

284279
if setting_module.OfflineMode {
285-
disableGravatar = true
286-
enableFederatedAvatar = false
287-
if !GetSettingBool(ctx, KeyPictureDisableGravatar) {
280+
if !disableGravatar {
288281
if err := SetSettingNoVersion(ctx, KeyPictureDisableGravatar, "true"); err != nil {
289-
return fmt.Errorf("Failed to set setting %q: %w", KeyPictureDisableGravatar, err)
282+
return fmt.Errorf("failed to set setting %q: %w", KeyPictureDisableGravatar, err)
290283
}
291284
}
292-
if GetSettingBool(ctx, KeyPictureEnableFederatedAvatar) {
285+
disableGravatar = true
286+
287+
if enableFederatedAvatar {
293288
if err := SetSettingNoVersion(ctx, KeyPictureEnableFederatedAvatar, "false"); err != nil {
294-
return fmt.Errorf("Failed to set setting %q: %w", KeyPictureEnableFederatedAvatar, err)
289+
return fmt.Errorf("failed to set setting %q: %w", KeyPictureEnableFederatedAvatar, err)
295290
}
296291
}
292+
enableFederatedAvatar = false
297293
}
298294

299295
if enableFederatedAvatar || !disableGravatar {
300296
var err error
301297
GravatarSourceURL, err = url.Parse(setting_module.GravatarSource)
302298
if err != nil {
303-
return fmt.Errorf("Failed to parse Gravatar URL(%s): %w", setting_module.GravatarSource, err)
299+
return fmt.Errorf("failed to parse Gravatar URL(%s): %w", setting_module.GravatarSource, err)
304300
}
305301
}
306302

307-
if GravatarSourceURL != nil && enableFederatedAvatarSetting.GetValueBool() {
303+
if GravatarSourceURL != nil && enableFederatedAvatar {
308304
LibravatarService = libravatar.New()
309305
if GravatarSourceURL.Scheme == "https" {
310306
LibravatarService.SetUseHTTPS(true)

‎models/user/avatar.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,9 @@ func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string {
6767
useLocalAvatar := false
6868
autoGenerateAvatar := false
6969

70-
disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar)
70+
disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
71+
setting.GetDefaultDisableGravatar(),
72+
)
7173

7274
switch {
7375
case u.UseCustomAvatar:

‎modules/repository/commits.go

Lines changed: 18 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"code.gitea.io/gitea/models/avatars"
1313
user_model "code.gitea.io/gitea/models/user"
14+
"code.gitea.io/gitea/modules/cache"
1415
"code.gitea.io/gitea/modules/git"
1516
"code.gitea.io/gitea/modules/log"
1617
"code.gitea.io/gitea/modules/setting"
@@ -34,42 +35,36 @@ type PushCommits struct {
3435
HeadCommit *PushCommit
3536
CompareURL string
3637
Len int
37-
38-
avatars map[string]string
39-
emailUsers map[string]*user_model.User
4038
}
4139

4240
// NewPushCommits creates a new PushCommits object.
4341
func NewPushCommits() *PushCommits {
44-
return &PushCommits{
45-
avatars: make(map[string]string),
46-
emailUsers: make(map[string]*user_model.User),
47-
}
42+
return &PushCommits{}
4843
}
4944

5045
// toAPIPayloadCommit converts a single PushCommit to an api.PayloadCommit object.
51-
func (pc *PushCommits) toAPIPayloadCommit(ctx context.Context, repoPath, repoLink string, commit *PushCommit) (*api.PayloadCommit, error) {
46+
func (pc *PushCommits) toAPIPayloadCommit(ctx context.Context, emailUsers map[string]*user_model.User, repoPath, repoLink string, commit *PushCommit) (*api.PayloadCommit, error) {
5247
var err error
5348
authorUsername := ""
54-
author, ok := pc.emailUsers[commit.AuthorEmail]
49+
author, ok := emailUsers[commit.AuthorEmail]
5550
if !ok {
5651
author, err = user_model.GetUserByEmail(ctx, commit.AuthorEmail)
5752
if err == nil {
5853
authorUsername = author.Name
59-
pc.emailUsers[commit.AuthorEmail] = author
54+
emailUsers[commit.AuthorEmail] = author
6055
}
6156
} else {
6257
authorUsername = author.Name
6358
}
6459

6560
committerUsername := ""
66-
committer, ok := pc.emailUsers[commit.CommitterEmail]
61+
committer, ok := emailUsers[commit.CommitterEmail]
6762
if !ok {
6863
committer, err = user_model.GetUserByEmail(ctx, commit.CommitterEmail)
6964
if err == nil {
7065
// TODO: check errors other than email not found.
7166
committerUsername = committer.Name
72-
pc.emailUsers[commit.CommitterEmail] = committer
67+
emailUsers[commit.CommitterEmail] = committer
7368
}
7469
} else {
7570
committerUsername = committer.Name
@@ -107,11 +102,10 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
107102
commits := make([]*api.PayloadCommit, len(pc.Commits))
108103
var headCommit *api.PayloadCommit
109104

110-
if pc.emailUsers == nil {
111-
pc.emailUsers = make(map[string]*user_model.User)
112-
}
105+
emailUsers := make(map[string]*user_model.User)
106+
113107
for i, commit := range pc.Commits {
114-
apiCommit, err := pc.toAPIPayloadCommit(ctx, repoPath, repoLink, commit)
108+
apiCommit, err := pc.toAPIPayloadCommit(ctx, emailUsers, repoPath, repoLink, commit)
115109
if err != nil {
116110
return nil, nil, err
117111
}
@@ -123,7 +117,7 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
123117
}
124118
if pc.HeadCommit != nil && headCommit == nil {
125119
var err error
126-
headCommit, err = pc.toAPIPayloadCommit(ctx, repoPath, repoLink, pc.HeadCommit)
120+
headCommit, err = pc.toAPIPayloadCommit(ctx, emailUsers, repoPath, repoLink, pc.HeadCommit)
127121
if err != nil {
128122
return nil, nil, err
129123
}
@@ -134,35 +128,21 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
134128
// AvatarLink tries to match user in database with e-mail
135129
// in order to show custom avatar, and falls back to general avatar link.
136130
func (pc *PushCommits) AvatarLink(ctx context.Context, email string) string {
137-
if pc.avatars == nil {
138-
pc.avatars = make(map[string]string)
139-
}
140-
avatar, ok := pc.avatars[email]
141-
if ok {
142-
return avatar
143-
}
144-
145131
size := avatars.DefaultAvatarPixelSize * setting.Avatar.RenderedSizeFactor
146132

147-
u, ok := pc.emailUsers[email]
148-
if !ok {
149-
var err error
150-
u, err = user_model.GetUserByEmail(ctx, email)
133+
v, _ := cache.GetWithContextCache(ctx, "push_commits", email, func() (string, error) {
134+
u, err := user_model.GetUserByEmail(ctx, email)
151135
if err != nil {
152-
pc.avatars[email] = avatars.GenerateEmailAvatarFastLink(ctx, email, size)
153136
if !user_model.IsErrUserNotExist(err) {
154137
log.Error("GetUserByEmail: %v", err)
155-
return ""
138+
return "", err
156139
}
157-
} else {
158-
pc.emailUsers[email] = u
140+
return avatars.GenerateEmailAvatarFastLink(ctx, email, size), nil
159141
}
160-
}
161-
if u != nil {
162-
pc.avatars[email] = u.AvatarLinkWithSize(ctx, size)
163-
}
142+
return u.AvatarLinkWithSize(ctx, size), nil
143+
})
164144

165-
return pc.avatars[email]
145+
return v
166146
}
167147

168148
// CommitToPushCommit transforms a git.Commit to PushCommit type.
@@ -189,7 +169,5 @@ func GitToPushCommits(gitCommits []*git.Commit) *PushCommits {
189169
HeadCommit: nil,
190170
CompareURL: "",
191171
Len: len(commits),
192-
avatars: make(map[string]string),
193-
emailUsers: make(map[string]*user_model.User),
194172
}
195173
}

‎modules/repository/commits_test.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,9 @@ func TestPushCommits_ToAPIPayloadCommits(t *testing.T) {
103103
assert.EqualValues(t, []string{"readme.md"}, headCommit.Modified)
104104
}
105105

106-
func enableGravatar(t *testing.T) {
107-
err := system_model.SetSettingNoVersion(db.DefaultContext, system_model.KeyPictureDisableGravatar, "false")
108-
assert.NoError(t, err)
106+
func initGravatarSource(t *testing.T) {
109107
setting.GravatarSource = "https://secure.gravatar.com/avatar"
110-
err = system_model.Init(db.DefaultContext)
108+
err := system_model.Init(db.DefaultContext)
111109
assert.NoError(t, err)
112110
}
113111

@@ -134,7 +132,7 @@ func TestPushCommits_AvatarLink(t *testing.T) {
134132
},
135133
}
136134

137-
enableGravatar(t)
135+
initGravatarSource(t)
138136

139137
assert.Equal(t,
140138
"https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?d=identicon&s="+strconv.Itoa(28*setting.Avatar.RenderedSizeFactor),

‎modules/templates/helper.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func NewFuncMap() template.FuncMap {
104104
return setting.AssetVersion
105105
},
106106
"DisableGravatar": func(ctx context.Context) bool {
107-
return system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar)
107+
return system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar, setting.GetDefaultDisableGravatar())
108108
},
109109
"DefaultShowFullName": func() bool {
110110
return setting.UI.DefaultShowFullName

0 commit comments

Comments
 (0)