From 4ffc54f59a7723eb5aef21955129bdd329ee1d4f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 30 Jan 2025 07:33:50 +0800 Subject: [PATCH] Refactor user & avatar (#33433) 1. better GetPossibleUserByID logic 2. fix some function name & comment typos 3. do not re-generate avatar if one exists --- models/user/avatar.go | 21 +++++++----- models/user/avatar_test.go | 40 ++++++++++++++++++++++ models/user/user.go | 51 +++++++++++++---------------- models/user/user_system.go | 16 ++++++++- models/user/user_system_test.go | 32 ++++++++++++++++++ models/user/user_test.go | 4 +-- modules/storage/storage.go | 4 +-- routers/web/user/avatar.go | 25 +++++--------- routers/web/user/home.go | 2 +- routers/web/web.go | 2 +- services/actions/notifier_helper.go | 2 +- services/mailer/mail_issue.go | 2 +- services/mailer/mail_release.go | 4 +-- 13 files changed, 139 insertions(+), 66 deletions(-) create mode 100644 models/user/user_system_test.go diff --git a/models/user/avatar.go b/models/user/avatar.go index 5453c78fc61..2a41b991292 100644 --- a/models/user/avatar.go +++ b/models/user/avatar.go @@ -38,27 +38,30 @@ func GenerateRandomAvatar(ctx context.Context, u *User) error { u.Avatar = avatars.HashEmail(seed) - // Don't share the images so that we can delete them easily - if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error { - if err := png.Encode(w, img); err != nil { - log.Error("Encode: %v", err) + _, err = storage.Avatars.Stat(u.CustomAvatarRelativePath()) + if err != nil { + // If unable to Stat the avatar file (usually it means non-existing), then try to save a new one + // Don't share the images so that we can delete them easily + if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error { + if err := png.Encode(w, img); err != nil { + log.Error("Encode: %v", err) + } + return nil + }); err != nil { + return fmt.Errorf("failed to save avatar %s: %w", u.CustomAvatarRelativePath(), err) } - return err - }); err != nil { - return fmt.Errorf("Failed to create dir %s: %w", u.CustomAvatarRelativePath(), err) } if _, err := db.GetEngine(ctx).ID(u.ID).Cols("avatar").Update(u); err != nil { return err } - log.Info("New random avatar created: %d", u.ID) return nil } // AvatarLinkWithSize returns a link to the user's avatar with size. size <= 0 means default size func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string { - if u.IsGhost() { + if u.IsGhost() || u.IsGiteaActions() { return avatars.DefaultAvatarLink() } diff --git a/models/user/avatar_test.go b/models/user/avatar_test.go index 1078875ee16..a1cc01316f3 100644 --- a/models/user/avatar_test.go +++ b/models/user/avatar_test.go @@ -4,13 +4,19 @@ package user import ( + "context" + "io" + "strings" "testing" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestUserAvatarLink(t *testing.T) { @@ -26,3 +32,37 @@ func TestUserAvatarLink(t *testing.T) { link = u.AvatarLink(db.DefaultContext) assert.Equal(t, "https://localhost/sub-path/avatars/avatar.png", link) } + +func TestUserAvatarGenerate(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + var err error + tmpDir := t.TempDir() + storage.Avatars, err = storage.NewLocalStorage(context.Background(), &setting.Storage{Path: tmpDir}) + require.NoError(t, err) + + u := unittest.AssertExistsAndLoadBean(t, &User{ID: 2}) + + // there was no avatar, generate a new one + assert.Empty(t, u.Avatar) + err = GenerateRandomAvatar(db.DefaultContext, u) + require.NoError(t, err) + assert.NotEmpty(t, u.Avatar) + + // make sure the generated one exists + oldAvatarPath := u.CustomAvatarRelativePath() + _, err = storage.Avatars.Stat(u.CustomAvatarRelativePath()) + require.NoError(t, err) + // and try to change its content + _, err = storage.Avatars.Save(u.CustomAvatarRelativePath(), strings.NewReader("abcd"), 4) + require.NoError(t, err) + + // try to generate again + err = GenerateRandomAvatar(db.DefaultContext, u) + require.NoError(t, err) + assert.Equal(t, oldAvatarPath, u.CustomAvatarRelativePath()) + f, err := storage.Avatars.Open(u.CustomAvatarRelativePath()) + require.NoError(t, err) + defer f.Close() + content, _ := io.ReadAll(f) + assert.Equal(t, "abcd", string(content)) +} diff --git a/models/user/user.go b/models/user/user.go index e93ad41ade8..75ed7ece8bb 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -528,6 +528,7 @@ type globalVarsStruct struct { replaceCharsHyphenRE *regexp.Regexp emailToReplacer *strings.Replacer emailRegexp *regexp.Regexp + systemUserNewFuncs map[int64]func() *User } var globalVars = sync.OnceValue(func() *globalVarsStruct { @@ -550,6 +551,11 @@ var globalVars = sync.OnceValue(func() *globalVarsStruct { ";", "", ), emailRegexp: regexp.MustCompile("^[a-zA-Z0-9.!#$%&'*+-/=?^_`{|}~]*@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$"), + + systemUserNewFuncs: map[int64]func() *User{ + GhostUserID: NewGhostUser, + ActionsUserID: NewActionsUser, + }, } }) @@ -978,30 +984,28 @@ func GetUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { return users, err } -// GetPossibleUserByID returns the user if id > 0 or return system usrs if id < 0 +// GetPossibleUserByID returns the user if id > 0 or returns system user if id < 0 func GetPossibleUserByID(ctx context.Context, id int64) (*User, error) { - switch id { - case GhostUserID: - return NewGhostUser(), nil - case ActionsUserID: - return NewActionsUser(), nil - case 0: + if id < 0 { + if newFunc, ok := globalVars().systemUserNewFuncs[id]; ok { + return newFunc(), nil + } + return nil, ErrUserNotExist{UID: id} + } else if id == 0 { return nil, ErrUserNotExist{} - default: - return GetUserByID(ctx, id) } + return GetUserByID(ctx, id) } -// GetPossibleUserByIDs returns the users if id > 0 or return system users if id < 0 +// GetPossibleUserByIDs returns the users if id > 0 or returns system users if id < 0 func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { uniqueIDs := container.SetOf(ids...) users := make([]*User, 0, len(ids)) _ = uniqueIDs.Remove(0) - if uniqueIDs.Remove(GhostUserID) { - users = append(users, NewGhostUser()) - } - if uniqueIDs.Remove(ActionsUserID) { - users = append(users, NewActionsUser()) + for systemUID, newFunc := range globalVars().systemUserNewFuncs { + if uniqueIDs.Remove(systemUID) { + users = append(users, newFunc()) + } } res, err := GetUserByIDs(ctx, uniqueIDs.Values()) if err != nil { @@ -1011,7 +1015,7 @@ func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { return users, nil } -// GetUserByNameCtx returns user by given name. +// GetUserByName returns user by given name. func GetUserByName(ctx context.Context, name string) (*User, error) { if len(name) == 0 { return nil, ErrUserNotExist{Name: name} @@ -1042,8 +1046,8 @@ func GetUserEmailsByNames(ctx context.Context, names []string) []string { return mails } -// GetMaileableUsersByIDs gets users from ids, but only if they can receive mails -func GetMaileableUsersByIDs(ctx context.Context, ids []int64, isMention bool) ([]*User, error) { +// GetMailableUsersByIDs gets users from ids, but only if they can receive mails +func GetMailableUsersByIDs(ctx context.Context, ids []int64, isMention bool) ([]*User, error) { if len(ids) == 0 { return nil, nil } @@ -1068,17 +1072,6 @@ func GetMaileableUsersByIDs(ctx context.Context, ids []int64, isMention bool) ([ Find(&ous) } -// GetUserNamesByIDs returns usernames for all resolved users from a list of Ids. -func GetUserNamesByIDs(ctx context.Context, ids []int64) ([]string, error) { - unames := make([]string, 0, len(ids)) - err := db.GetEngine(ctx).In("id", ids). - Table("user"). - Asc("name"). - Cols("name"). - Find(&unames) - return unames, err -} - // GetUserNameByID returns username for the id func GetUserNameByID(ctx context.Context, id int64) (string, error) { var name string diff --git a/models/user/user_system.go b/models/user/user_system.go index 6e7638377ee..e54973dc8e5 100644 --- a/models/user/user_system.go +++ b/models/user/user_system.go @@ -41,6 +41,10 @@ const ( ActionsUserEmail = "teabot@gitea.io" ) +func IsGiteaActionsUserName(name string) bool { + return strings.EqualFold(name, ActionsUserName) +} + // NewActionsUser creates and returns a fake user for running the actions. func NewActionsUser() *User { return &User{ @@ -58,6 +62,16 @@ func NewActionsUser() *User { } } -func (u *User) IsActions() bool { +func (u *User) IsGiteaActions() bool { return u != nil && u.ID == ActionsUserID } + +func GetSystemUserByName(name string) *User { + if IsGhostUserName(name) { + return NewGhostUser() + } + if IsGiteaActionsUserName(name) { + return NewActionsUser() + } + return nil +} diff --git a/models/user/user_system_test.go b/models/user/user_system_test.go new file mode 100644 index 00000000000..97768b509be --- /dev/null +++ b/models/user/user_system_test.go @@ -0,0 +1,32 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package user + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSystemUser(t *testing.T) { + u, err := GetPossibleUserByID(db.DefaultContext, -1) + require.NoError(t, err) + assert.Equal(t, "Ghost", u.Name) + assert.Equal(t, "ghost", u.LowerName) + assert.True(t, u.IsGhost()) + assert.True(t, IsGhostUserName("gHost")) + + u, err = GetPossibleUserByID(db.DefaultContext, -2) + require.NoError(t, err) + assert.Equal(t, "gitea-actions", u.Name) + assert.Equal(t, "gitea-actions", u.LowerName) + assert.True(t, u.IsGiteaActions()) + assert.True(t, IsGiteaActionsUserName("Gitea-actionS")) + + _, err = GetPossibleUserByID(db.DefaultContext, -3) + require.Error(t, err) +} diff --git a/models/user/user_test.go b/models/user/user_test.go index cad1a64d6e8..51098417e6d 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -333,14 +333,14 @@ func TestGetUserIDsByNames(t *testing.T) { func TestGetMaileableUsersByIDs(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - results, err := user_model.GetMaileableUsersByIDs(db.DefaultContext, []int64{1, 4}, false) + results, err := user_model.GetMailableUsersByIDs(db.DefaultContext, []int64{1, 4}, false) assert.NoError(t, err) assert.Len(t, results, 1) if len(results) > 1 { assert.Equal(t, 1, results[0].ID) } - results, err = user_model.GetMaileableUsersByIDs(db.DefaultContext, []int64{1, 4}, true) + results, err = user_model.GetMailableUsersByIDs(db.DefaultContext, []int64{1, 4}, true) assert.NoError(t, err) assert.Len(t, results, 2) if len(results) > 2 { diff --git a/modules/storage/storage.go b/modules/storage/storage.go index 750ecdfe0db..b0529941e7d 100644 --- a/modules/storage/storage.go +++ b/modules/storage/storage.go @@ -93,7 +93,7 @@ func Clean(storage ObjectStorage) error { } // SaveFrom saves data to the ObjectStorage with path p from the callback -func SaveFrom(objStorage ObjectStorage, p string, callback func(w io.Writer) error) error { +func SaveFrom(objStorage ObjectStorage, path string, callback func(w io.Writer) error) error { pr, pw := io.Pipe() defer pr.Close() go func() { @@ -103,7 +103,7 @@ func SaveFrom(objStorage ObjectStorage, p string, callback func(w io.Writer) err } }() - _, err := objStorage.Save(p, pr, -1) + _, err := objStorage.Save(path, pr, -1) return err } diff --git a/routers/web/user/avatar.go b/routers/web/user/avatar.go index f70b2803770..81c00b3bd42 100644 --- a/routers/web/user/avatar.go +++ b/routers/web/user/avatar.go @@ -20,27 +20,18 @@ func cacheableRedirect(ctx *context.Context, location string) { ctx.Redirect(location) } -// AvatarByUserName redirect browser to user avatar of requested size -func AvatarByUserName(ctx *context.Context) { - userName := ctx.PathParam("username") - size := int(ctx.PathParamInt64("size")) - - var user *user_model.User - if !user_model.IsGhostUserName(userName) { +// AvatarByUsernameSize redirect browser to user avatar of requested size +func AvatarByUsernameSize(ctx *context.Context) { + username := ctx.PathParam("username") + user := user_model.GetSystemUserByName(username) + if user == nil { var err error - if user, err = user_model.GetUserByName(ctx, userName); err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", err) - return - } - ctx.ServerError("Invalid user: "+userName, err) + if user, err = user_model.GetUserByName(ctx, username); err != nil { + ctx.NotFoundOrServerError("GetUserByName", user_model.IsErrUserNotExist, err) return } - } else { - user = user_model.NewGhostUser() } - - cacheableRedirect(ctx, user.AvatarLinkWithSize(ctx, size)) + cacheableRedirect(ctx, user.AvatarLinkWithSize(ctx, int(ctx.PathParamInt64("size")))) } // AvatarByEmailHash redirects the browser to the email avatar link diff --git a/routers/web/user/home.go b/routers/web/user/home.go index c4ed242f71b..ff9334da6e2 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -732,7 +732,7 @@ func UsernameSubRoute(ctx *context.Context) { switch { case strings.HasSuffix(username, ".png"): if reloadParam(".png") { - AvatarByUserName(ctx) + AvatarByUsernameSize(ctx) } case strings.HasSuffix(username, ".keys"): if reloadParam(".keys") { diff --git a/routers/web/web.go b/routers/web/web.go index 5330b0f3c1b..fc01d81b509 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -681,7 +681,7 @@ func registerRoutes(m *web.Router) { m.Get("/activate", auth.Activate) m.Post("/activate", auth.ActivatePost) m.Any("/activate_email", auth.ActivateEmail) - m.Get("/avatar/{username}/{size}", user.AvatarByUserName) + m.Get("/avatar/{username}/{size}", user.AvatarByUsernameSize) m.Get("/recover_account", auth.ResetPasswd) m.Post("/recover_account", auth.ResetPasswdPost) m.Get("/forgot_password", auth.ForgotPasswd) diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 323c6a76e42..2d8885dc323 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -117,7 +117,7 @@ func (input *notifyInput) Notify(ctx context.Context) { func notify(ctx context.Context, input *notifyInput) error { shouldDetectSchedules := input.Event == webhook_module.HookEventPush && input.Ref.BranchName() == input.Repo.DefaultBranch - if input.Doer.IsActions() { + if input.Doer.IsGiteaActions() { // avoiding triggering cyclically, for example: // a comment of an issue will trigger the runner to add a new comment as reply, // and the new comment will trigger the runner again. diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index fab3315be21..e269b1ca1e2 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -109,7 +109,7 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*user_mo } visited.AddMultiple(ids...) - unfilteredUsers, err := user_model.GetMaileableUsersByIDs(ctx, unfiltered, false) + unfilteredUsers, err := user_model.GetMailableUsersByIDs(ctx, unfiltered, false) if err != nil { return err } diff --git a/services/mailer/mail_release.go b/services/mailer/mail_release.go index 796d63d27ae..31316b0053b 100644 --- a/services/mailer/mail_release.go +++ b/services/mailer/mail_release.go @@ -35,9 +35,9 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) { return } - recipients, err := user_model.GetMaileableUsersByIDs(ctx, watcherIDList, false) + recipients, err := user_model.GetMailableUsersByIDs(ctx, watcherIDList, false) if err != nil { - log.Error("user_model.GetMaileableUsersByIDs: %v", err) + log.Error("user_model.GetMailableUsersByIDs: %v", err) return }