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

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

models/fixtures/system_setting.yml

@@ -1,14 +1,14 @@
 -
   id: 1
-  setting_key: 'disable_gravatar'
+  setting_key: 'picture.disable_gravatar'
   setting_value: 'false'
   version: 1
   created: 1653533198
   updated: 1653533198
 
 -
   id: 2
-  setting_key: 'enable_federated_avatar'
+  setting_key: 'picture.enable_federated_avatar'
   setting_value: 'false'
   version: 1
   created: 1653533198

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)

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:

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),
 	}
 }

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),

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