diff --git a/models/avatars/avatar.go b/models/avatars/avatar.go index 265ee6428..e40aa3f54 100644 --- 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 6c960168f..30542bc82 100644 --- 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 6af759dc8..9fffa74e9 100644 --- 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 3d1e2ed20..7f996fa79 100644 --- 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 96844d5b1..ede60429a 100644 --- 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 b6ad967d4..093109259 100644 --- 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/routers/web/admin/users.go b/routers/web/admin/users.go index c83d652c3..47dff6e85 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -314,7 +314,9 @@ func EditUser(ctx *context.Context) { ctx.Data["DisableRegularOrgCreation"] = setting.Admin.DisableRegularOrgCreation ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() - ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) + ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar, + setting.GetDefaultDisableGravatar(), + ) prepareUserInfo(ctx) if ctx.Written() { @@ -331,7 +333,8 @@ func EditUserPost(ctx *context.Context) { ctx.Data["PageIsAdminUsers"] = true ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() - ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) + ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar, + setting.GetDefaultDisableGravatar()) u := prepareUserInfo(ctx) if ctx.Written() { diff --git a/routers/web/user/setting/profile.go b/routers/web/user/setting/profile.go index e1dc5680c..0321a5759 100644 --- a/routers/web/user/setting/profile.go +++ b/routers/web/user/setting/profile.go @@ -44,7 +44,9 @@ func Profile(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("settings.profile") ctx.Data["PageIsSettingsProfile"] = true ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() - ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) + ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar, + setting.GetDefaultDisableGravatar(), + ) ctx.HTML(http.StatusOK, tplSettingsProfile) } @@ -86,7 +88,9 @@ func ProfilePost(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("settings") ctx.Data["PageIsSettingsProfile"] = true ctx.Data["AllowedUserVisibilityModes"] = setting.Service.AllowedUserVisibilityModesSlice.ToVisibleTypeSlice() - ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar) + ctx.Data["DisableGravatar"] = system_model.GetSettingWithCacheBool(ctx, system_model.KeyPictureDisableGravatar, + setting.GetDefaultDisableGravatar(), + ) if ctx.HasError() { ctx.HTML(http.StatusOK, tplSettingsProfile)