Fix context cache bug & enable context cache for dashabord commits' authors(#26991...

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 .

diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go
index 265ee6428e361ebc00f25f0efa7a787793600d46..e40aa3f542432cbdb43f8c8cfc92c1ba00abdc72 100644 (file)
--- a/models/avatars/avatar.go
+++ b/models/avatars/avatar.go
@@ -153,7 +153,12 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final
                return DefaultAvatarLink()
        }
 
-       enableFederatedAvatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureEnableFederatedAvatar)
+       disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
+               setting.GetDefaultDisableGravatar(),
+       )
+
+       enableFederatedAvatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureEnableFederatedAvatar,
+               setting.GetDefaultEnableFederatedAvatar(disableGravatar))
 
        var err error
        if enableFederatedAvatar && system_model.LibravatarService != nil {
@@ -174,7 +179,6 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final
                return urlStr
        }
 
-       disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar)
        if !disableGravatar {
                // copy GravatarSourceURL, because we will modify its Path.
                avatarURLCopy := *system_model.GravatarSourceURL

diff --git a/models/fixtures/system_setting.yml b/models/fixtures/system_setting.yml
index 6c960168fcb81bf91bc3b81e1a8fce93df50579e..30542bc82a92c79115566ad6937049be53c6986c 100644 (file)
--- a/models/fixtures/system_setting.yml
+++ b/models/fixtures/system_setting.yml
@@ -1,6 +1,6 @@
 -
   id: 1
-  setting_key: 'disable_gravatar'
+  setting_key: 'picture.disable_gravatar'
   setting_value: 'false'
   version: 1
   created: 1653533198
@@ -8,7 +8,7 @@
 
 -
   id: 2
-  setting_key: 'enable_federated_avatar'
+  setting_key: 'picture.enable_federated_avatar'
   setting_value: 'false'
   version: 1
   created: 1653533198

diff --git a/models/system/setting.go b/models/system/setting.go
index 6af759dc8a1c0a45cc5f33692052a5cf0327d6f4..9fffa74e94afa5460300d746a32eceb080899e42 100644 (file)
--- a/models/system/setting.go
+++ b/models/system/setting.go
@@ -94,11 +94,14 @@ func GetSetting(ctx context.Context, key string) (*Setting, error) {
 const contextCacheKey = "system_setting"
 
 // GetSettingWithCache returns the setting value via the key
-func GetSettingWithCache(ctx context.Context, key string) (string, error) {
+func GetSettingWithCache(ctx context.Context, key, defaultVal string) (string, error) {
        return cache.GetWithContextCache(ctx, contextCacheKey, key, func() (string, error) {
                return cache.GetString(genSettingCacheKey(key), func() (string, error) {
                        res, err := GetSetting(ctx, key)
                        if err != nil {
+                               if IsErrSettingIsNotExist(err) {
+                                       return defaultVal, nil
+                               }
                                return "", err
                        }
                        return res.SettingValue, nil
@@ -108,17 +111,21 @@ func GetSettingWithCache(ctx context.Context, key string) (string, error) {
 
 // GetSettingBool return bool value of setting,
 // none existing keys and errors are ignored and result in false
-func GetSettingBool(ctx context.Context, key string) bool {
-       s, _ := GetSetting(ctx, key)
-       if s == nil {
-               return false
+func GetSettingBool(ctx context.Context, key string, defaultVal bool) (bool, error) {
+       s, err := GetSetting(ctx, key)
+       switch {
+       case err == nil:
+               v, _ := strconv.ParseBool(s.SettingValue)
+               return v, nil
+       case IsErrSettingIsNotExist(err):
+               return defaultVal, nil
+       default:
+               return false, err
        }
-       v, _ := strconv.ParseBool(s.SettingValue)
-       return v
 }
 
-func GetSettingWithCacheBool(ctx context.Context, key string) bool {
-       s, _ := GetSettingWithCache(ctx, key)
+func GetSettingWithCacheBool(ctx context.Context, key string, defaultVal bool) bool {
+       s, _ := GetSettingWithCache(ctx, key, strconv.FormatBool(defaultVal))
        v, _ := strconv.ParseBool(s)
        return v
 }
@@ -259,52 +266,41 @@ var (
 )
 
 func Init(ctx context.Context) error {
-       var disableGravatar bool
-       disableGravatarSetting, err := GetSetting(ctx, KeyPictureDisableGravatar)
-       if IsErrSettingIsNotExist(err) {
-               disableGravatar = setting_module.GetDefaultDisableGravatar()
-               disableGravatarSetting = &Setting{SettingValue: strconv.FormatBool(disableGravatar)}
-       } else if err != nil {
+       disableGravatar, err := GetSettingBool(ctx, KeyPictureDisableGravatar, setting_module.GetDefaultDisableGravatar())
+       if err != nil {
                return err
-       } else {
-               disableGravatar = disableGravatarSetting.GetValueBool()
        }
 
-       var enableFederatedAvatar bool
-       enableFederatedAvatarSetting, err := GetSetting(ctx, KeyPictureEnableFederatedAvatar)
-       if IsErrSettingIsNotExist(err) {
-               enableFederatedAvatar = setting_module.GetDefaultEnableFederatedAvatar(disableGravatar)
-               enableFederatedAvatarSetting = &Setting{SettingValue: strconv.FormatBool(enableFederatedAvatar)}
-       } else if err != nil {
+       enableFederatedAvatar, err := GetSettingBool(ctx, KeyPictureEnableFederatedAvatar, setting_module.GetDefaultEnableFederatedAvatar(disableGravatar))
+       if err != nil {
                return err
-       } else {
-               enableFederatedAvatar = disableGravatarSetting.GetValueBool()
        }
 
        if setting_module.OfflineMode {
-               disableGravatar = true
-               enableFederatedAvatar = false
-               if !GetSettingBool(ctx, KeyPictureDisableGravatar) {
+               if !disableGravatar {
                        if err := SetSettingNoVersion(ctx, KeyPictureDisableGravatar, "true"); err != nil {
-                               return fmt.Errorf("Failed to set setting %q: %w", KeyPictureDisableGravatar, err)
+                               return fmt.Errorf("failed to set setting %q: %w", KeyPictureDisableGravatar, err)
                        }
                }
-               if GetSettingBool(ctx, KeyPictureEnableFederatedAvatar) {
+               disableGravatar = true
+
+               if enableFederatedAvatar {
                        if err := SetSettingNoVersion(ctx, KeyPictureEnableFederatedAvatar, "false"); err != nil {
-                               return fmt.Errorf("Failed to set setting %q: %w", KeyPictureEnableFederatedAvatar, err)
+                               return fmt.Errorf("failed to set setting %q: %w", KeyPictureEnableFederatedAvatar, err)
                        }
                }
+               enableFederatedAvatar = false
        }
 
        if enableFederatedAvatar || !disableGravatar {
                var err error
                GravatarSourceURL, err = url.Parse(setting_module.GravatarSource)
                if err != nil {
-                       return fmt.Errorf("Failed to parse Gravatar URL(%s): %w", setting_module.GravatarSource, err)
+                       return fmt.Errorf("failed to parse Gravatar URL(%s): %w", setting_module.GravatarSource, err)
                }
        }
 
-       if GravatarSourceURL != nil && enableFederatedAvatarSetting.GetValueBool() {
+       if GravatarSourceURL != nil && enableFederatedAvatar {
                LibravatarService = libravatar.New()
                if GravatarSourceURL.Scheme == "https" {
                        LibravatarService.SetUseHTTPS(true)

diff --git a/models/user/avatar.go b/models/user/avatar.go
index 3d1e2ed20b910b1da04a4bc5be93087f5ca83631..7f996fa79ab7f0b2cd7fe16cf5de83ebdc1e3bc3 100644 (file)
--- a/models/user/avatar.go
+++ b/models/user/avatar.go
@@ -67,7 +67,9 @@ func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string {
        useLocalAvatar := false
        autoGenerateAvatar := false
 
-       disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar)
+       disableGravatar := system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar,
+               setting.GetDefaultDisableGravatar(),
+       )
 
        switch {
        case u.UseCustomAvatar:

diff --git a/modules/repository/commits.go b/modules/repository/commits.go
index 96844d5b1da0413644c3c7c78e703d7b5d118400..ede60429a13fb92be16b8c6d5b0ff52c1463ba9a 100644 (file)
--- a/modules/repository/commits.go
+++ b/modules/repository/commits.go
@@ -11,6 +11,7 @@ import (
 
        "code.gitea.io/gitea/models/avatars"
        user_model "code.gitea.io/gitea/models/user"
+       "code.gitea.io/gitea/modules/cache"
        "code.gitea.io/gitea/modules/git"
        "code.gitea.io/gitea/modules/log"
        "code.gitea.io/gitea/modules/setting"
@@ -34,42 +35,36 @@ type PushCommits struct {
        HeadCommit *PushCommit
        CompareURL string
        Len        int
-
-       avatars    map[string]string
-       emailUsers map[string]*user_model.User
 }
 
 // NewPushCommits creates a new PushCommits object.
 func NewPushCommits() *PushCommits {
-       return &PushCommits{
-               avatars:    make(map[string]string),
-               emailUsers: make(map[string]*user_model.User),
-       }
+       return &PushCommits{}
 }
 
 // toAPIPayloadCommit converts a single PushCommit to an api.PayloadCommit object.
-func (pc *PushCommits) toAPIPayloadCommit(ctx context.Context, repoPath, repoLink string, commit *PushCommit) (*api.PayloadCommit, error) {
+func (pc *PushCommits) toAPIPayloadCommit(ctx context.Context, emailUsers map[string]*user_model.User, repoPath, repoLink string, commit *PushCommit) (*api.PayloadCommit, error) {
        var err error
        authorUsername := ""
-       author, ok := pc.emailUsers[commit.AuthorEmail]
+       author, ok := emailUsers[commit.AuthorEmail]
        if !ok {
                author, err = user_model.GetUserByEmail(ctx, commit.AuthorEmail)
                if err == nil {
                        authorUsername = author.Name
-                       pc.emailUsers[commit.AuthorEmail] = author
+                       emailUsers[commit.AuthorEmail] = author
                }
        } else {
                authorUsername = author.Name
        }
 
        committerUsername := ""
-       committer, ok := pc.emailUsers[commit.CommitterEmail]
+       committer, ok := emailUsers[commit.CommitterEmail]
        if !ok {
                committer, err = user_model.GetUserByEmail(ctx, commit.CommitterEmail)
                if err == nil {
                        // TODO: check errors other than email not found.
                        committerUsername = committer.Name
-                       pc.emailUsers[commit.CommitterEmail] = committer
+                       emailUsers[commit.CommitterEmail] = committer
                }
        } else {
                committerUsername = committer.Name
@@ -107,11 +102,10 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
        commits := make([]*api.PayloadCommit, len(pc.Commits))
        var headCommit *api.PayloadCommit
 
-       if pc.emailUsers == nil {
-               pc.emailUsers = make(map[string]*user_model.User)
-       }
+       emailUsers := make(map[string]*user_model.User)
+
        for i, commit := range pc.Commits {
-               apiCommit, err := pc.toAPIPayloadCommit(ctx, repoPath, repoLink, commit)
+               apiCommit, err := pc.toAPIPayloadCommit(ctx, emailUsers, repoPath, repoLink, commit)
                if err != nil {
                        return nil, nil, err
                }
@@ -123,7 +117,7 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
        }
        if pc.HeadCommit != nil && headCommit == nil {
                var err error
-               headCommit, err = pc.toAPIPayloadCommit(ctx, repoPath, repoLink, pc.HeadCommit)
+               headCommit, err = pc.toAPIPayloadCommit(ctx, emailUsers, repoPath, repoLink, pc.HeadCommit)
                if err != nil {
                        return nil, nil, err
                }
@@ -134,35 +128,21 @@ func (pc *PushCommits) ToAPIPayloadCommits(ctx context.Context, repoPath, repoLi
 // AvatarLink tries to match user in database with e-mail
 // in order to show custom avatar, and falls back to general avatar link.
 func (pc *PushCommits) AvatarLink(ctx context.Context, email string) string {
-       if pc.avatars == nil {
-               pc.avatars = make(map[string]string)
-       }
-       avatar, ok := pc.avatars[email]
-       if ok {
-               return avatar
-       }
-
        size := avatars.DefaultAvatarPixelSize * setting.Avatar.RenderedSizeFactor
 
-       u, ok := pc.emailUsers[email]
-       if !ok {
-               var err error
-               u, err = user_model.GetUserByEmail(ctx, email)
+       v, _ := cache.GetWithContextCache(ctx, "push_commits", email, func() (string, error) {
+               u, err := user_model.GetUserByEmail(ctx, email)
                if err != nil {
-                       pc.avatars[email] = avatars.GenerateEmailAvatarFastLink(ctx, email, size)
                        if !user_model.IsErrUserNotExist(err) {
                                log.Error("GetUserByEmail: %v", err)
-                               return ""
+                               return "", err
                        }
-               } else {
-                       pc.emailUsers[email] = u
+                       return avatars.GenerateEmailAvatarFastLink(ctx, email, size), nil
                }
-       }
-       if u != nil {
-               pc.avatars[email] = u.AvatarLinkWithSize(ctx, size)
-       }
+               return u.AvatarLinkWithSize(ctx, size), nil
+       })
 
-       return pc.avatars[email]
+       return v
 }
 
 // CommitToPushCommit transforms a git.Commit to PushCommit type.
@@ -189,7 +169,5 @@ func GitToPushCommits(gitCommits []*git.Commit) *PushCommits {
                HeadCommit: nil,
                CompareURL: "",
                Len:        len(commits),
-               avatars:    make(map[string]string),
-               emailUsers: make(map[string]*user_model.User),
        }
 }

diff --git a/modules/repository/commits_test.go b/modules/repository/commits_test.go
index b6ad967d4ce558ee8c40380d39a7079380bc4017..0931092597101b78a84cd4d20bf928914af5baa0 100644 (file)
--- a/modules/repository/commits_test.go
+++ b/modules/repository/commits_test.go
@@ -103,11 +103,9 @@ func TestPushCommits_ToAPIPayloadCommits(t *testing.T) {
        assert.EqualValues(t, []string{"readme.md"}, headCommit.Modified)
 }
 
-func enableGravatar(t *testing.T) {
-       err := system_model.SetSettingNoVersion(db.DefaultContext, system_model.KeyPictureDisableGravatar, "false")
-       assert.NoError(t, err)
+func initGravatarSource(t *testing.T) {
        setting.GravatarSource = "https://secure.gravatar.com/avatar"
-       err = system_model.Init(db.DefaultContext)
+       err := system_model.Init(db.DefaultContext)
        assert.NoError(t, err)
 }
 
@@ -134,7 +132,7 @@ func TestPushCommits_AvatarLink(t *testing.T) {
                },
        }
 
-       enableGravatar(t)
+       initGravatarSource(t)
 
        assert.Equal(t,
                "https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?d=identicon&s="+strconv.Itoa(28*setting.Avatar.RenderedSizeFactor),

diff --git a/modules/templates/helper.go b/modules/templates/helper.go
index 2b918f42c099e545e3dade5980cd5e6c733a7d47..ff60054c403ce325966be4ab3b3b13c51ed2f7fc 100644 (file)
--- a/modules/templates/helper.go
+++ b/modules/templates/helper.go
@@ -104,7 +104,7 @@ func NewFuncMap() template.FuncMap {
                        return setting.AssetVersion
                },
                "DisableGravatar": func(ctx context.Context) bool {
-                       return system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar)
+                       return system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar, setting.GetDefaultDisableGravatar())
                },
                "DefaultShowFullName": func() bool {
                        return setting.UI.DefaultShowFullName