Skip to content

Commit 4ffc54f

Browse files
wxiaoguangwxiaoguang
authored
Refactor user & avatar (#33433)
1. better GetPossibleUserByID logic 2. fix some function name & comment typos 3. do not re-generate avatar if one exists
1 parent a89c735 commit 4ffc54f

File tree

13 files changed

+139
-66
lines changed

13 files changed

+139
-66
lines changed

‎models/user/avatar.go‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,27 +38,30 @@ func GenerateRandomAvatar(ctx context.Context, u *User) error {
3838

3939
u.Avatar = avatars.HashEmail(seed)
4040

41-
// Don't share the images so that we can delete them easily
42-
if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error {
43-
if err := png.Encode(w, img); err != nil {
44-
log.Error("Encode: %v", err)
41+
_, err = storage.Avatars.Stat(u.CustomAvatarRelativePath())
42+
if err != nil {
43+
// If unable to Stat the avatar file (usually it means non-existing), then try to save a new one
44+
// Don't share the images so that we can delete them easily
45+
if err := storage.SaveFrom(storage.Avatars, u.CustomAvatarRelativePath(), func(w io.Writer) error {
46+
if err := png.Encode(w, img); err != nil {
47+
log.Error("Encode: %v", err)
48+
}
49+
return nil
50+
}); err != nil {
51+
return fmt.Errorf("failed to save avatar %s: %w", u.CustomAvatarRelativePath(), err)
4552
}
46-
return err
47-
}); err != nil {
48-
return fmt.Errorf("Failed to create dir %s: %w", u.CustomAvatarRelativePath(), err)
4953
}
5054

5155
if _, err := db.GetEngine(ctx).ID(u.ID).Cols("avatar").Update(u); err != nil {
5256
return err
5357
}
5458

55-
log.Info("New random avatar created: %d", u.ID)
5659
return nil
5760
}
5861

5962
// AvatarLinkWithSize returns a link to the user's avatar with size. size <= 0 means default size
6063
func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string {
61-
if u.IsGhost() {
64+
if u.IsGhost() || u.IsGiteaActions() {
6265
return avatars.DefaultAvatarLink()
6366
}
6467

‎models/user/avatar_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,19 @@
44
package user
55

66
import (
7+
"context"
8+
"io"
9+
"strings"
710
"testing"
811

912
"code.gitea.io/gitea/models/db"
13+
"code.gitea.io/gitea/models/unittest"
1014
"code.gitea.io/gitea/modules/setting"
15+
"code.gitea.io/gitea/modules/storage"
1116
"code.gitea.io/gitea/modules/test"
1217

1318
"github.com/stretchr/testify/assert"
19+
"github.com/stretchr/testify/require"
1420
)
1521

1622
func TestUserAvatarLink(t *testing.T) {
@@ -26,3 +32,37 @@ func TestUserAvatarLink(t *testing.T) {
2632
link = u.AvatarLink(db.DefaultContext)
2733
assert.Equal(t, "https://localhost/sub-path/avatars/avatar.png", link)
2834
}
35+
36+
func TestUserAvatarGenerate(t *testing.T) {
37+
assert.NoError(t, unittest.PrepareTestDatabase())
38+
var err error
39+
tmpDir := t.TempDir()
40+
storage.Avatars, err = storage.NewLocalStorage(context.Background(), &setting.Storage{Path: tmpDir})
41+
require.NoError(t, err)
42+
43+
u := unittest.AssertExistsAndLoadBean(t, &User{ID: 2})
44+
45+
// there was no avatar, generate a new one
46+
assert.Empty(t, u.Avatar)
47+
err = GenerateRandomAvatar(db.DefaultContext, u)
48+
require.NoError(t, err)
49+
assert.NotEmpty(t, u.Avatar)
50+
51+
// make sure the generated one exists
52+
oldAvatarPath := u.CustomAvatarRelativePath()
53+
_, err = storage.Avatars.Stat(u.CustomAvatarRelativePath())
54+
require.NoError(t, err)
55+
// and try to change its content
56+
_, err = storage.Avatars.Save(u.CustomAvatarRelativePath(), strings.NewReader("abcd"), 4)
57+
require.NoError(t, err)
58+
59+
// try to generate again
60+
err = GenerateRandomAvatar(db.DefaultContext, u)
61+
require.NoError(t, err)
62+
assert.Equal(t, oldAvatarPath, u.CustomAvatarRelativePath())
63+
f, err := storage.Avatars.Open(u.CustomAvatarRelativePath())
64+
require.NoError(t, err)
65+
defer f.Close()
66+
content, _ := io.ReadAll(f)
67+
assert.Equal(t, "abcd", string(content))
68+
}

‎models/user/user.go‎

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,7 @@ type globalVarsStruct struct {
528528
replaceCharsHyphenRE *regexp.Regexp
529529
emailToReplacer *strings.Replacer
530530
emailRegexp *regexp.Regexp
531+
systemUserNewFuncs map[int64]func() *User
531532
}
532533

533534
var globalVars = sync.OnceValue(func() *globalVarsStruct {
@@ -550,6 +551,11 @@ var globalVars = sync.OnceValue(func() *globalVarsStruct {
550551
";", "",
551552
),
552553
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])?)*$"),
554+
555+
systemUserNewFuncs: map[int64]func() *User{
556+
GhostUserID: NewGhostUser,
557+
ActionsUserID: NewActionsUser,
558+
},
553559
}
554560
})
555561

@@ -978,30 +984,28 @@ func GetUserByIDs(ctx context.Context, ids []int64) ([]*User, error) {
978984
return users, err
979985
}
980986

981-
// GetPossibleUserByID returns the user if id > 0 or return system usrs if id < 0
987+
// GetPossibleUserByID returns the user if id > 0 or returns system user if id < 0
982988
func GetPossibleUserByID(ctx context.Context, id int64) (*User, error) {
983-
switch id {
984-
case GhostUserID:
985-
return NewGhostUser(), nil
986-
case ActionsUserID:
987-
return NewActionsUser(), nil
988-
case 0:
989+
if id < 0 {
990+
if newFunc, ok := globalVars().systemUserNewFuncs[id]; ok {
991+
return newFunc(), nil
992+
}
993+
return nil, ErrUserNotExist{UID: id}
994+
} else if id == 0 {
989995
return nil, ErrUserNotExist{}
990-
default:
991-
return GetUserByID(ctx, id)
992996
}
997+
return GetUserByID(ctx, id)
993998
}
994999

995-
// GetPossibleUserByIDs returns the users if id > 0 or return system users if id < 0
1000+
// GetPossibleUserByIDs returns the users if id > 0 or returns system users if id < 0
9961001
func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) {
9971002
uniqueIDs := container.SetOf(ids...)
9981003
users := make([]*User, 0, len(ids))
9991004
_ = uniqueIDs.Remove(0)
1000-
if uniqueIDs.Remove(GhostUserID) {
1001-
users = append(users, NewGhostUser())
1002-
}
1003-
if uniqueIDs.Remove(ActionsUserID) {
1004-
users = append(users, NewActionsUser())
1005+
for systemUID, newFunc := range globalVars().systemUserNewFuncs {
1006+
if uniqueIDs.Remove(systemUID) {
1007+
users = append(users, newFunc())
1008+
}
10051009
}
10061010
res, err := GetUserByIDs(ctx, uniqueIDs.Values())
10071011
if err != nil {
@@ -1011,7 +1015,7 @@ func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) {
10111015
return users, nil
10121016
}
10131017

1014-
// GetUserByNameCtx returns user by given name.
1018+
// GetUserByName returns user by given name.
10151019
func GetUserByName(ctx context.Context, name string) (*User, error) {
10161020
if len(name) == 0 {
10171021
return nil, ErrUserNotExist{Name: name}
@@ -1042,8 +1046,8 @@ func GetUserEmailsByNames(ctx context.Context, names []string) []string {
10421046
return mails
10431047
}
10441048

1045-
// GetMaileableUsersByIDs gets users from ids, but only if they can receive mails
1046-
func GetMaileableUsersByIDs(ctx context.Context, ids []int64, isMention bool) ([]*User, error) {
1049+
// GetMailableUsersByIDs gets users from ids, but only if they can receive mails
1050+
func GetMailableUsersByIDs(ctx context.Context, ids []int64, isMention bool) ([]*User, error) {
10471051
if len(ids) == 0 {
10481052
return nil, nil
10491053
}
@@ -1068,17 +1072,6 @@ func GetMaileableUsersByIDs(ctx context.Context, ids []int64, isMention bool) ([
10681072
Find(&ous)
10691073
}
10701074

1071-
// GetUserNamesByIDs returns usernames for all resolved users from a list of Ids.
1072-
func GetUserNamesByIDs(ctx context.Context, ids []int64) ([]string, error) {
1073-
unames := make([]string, 0, len(ids))
1074-
err := db.GetEngine(ctx).In("id", ids).
1075-
Table("user").
1076-
Asc("name").
1077-
Cols("name").
1078-
Find(&unames)
1079-
return unames, err
1080-
}
1081-
10821075
// GetUserNameByID returns username for the id
10831076
func GetUserNameByID(ctx context.Context, id int64) (string, error) {
10841077
var name string

‎models/user/user_system.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ const (
4141
ActionsUserEmail = "[email protected]"
4242
)
4343

44+
func IsGiteaActionsUserName(name string) bool {
45+
return strings.EqualFold(name, ActionsUserName)
46+
}
47+
4448
// NewActionsUser creates and returns a fake user for running the actions.
4549
func NewActionsUser() *User {
4650
return &User{
@@ -58,6 +62,16 @@ func NewActionsUser() *User {
5862
}
5963
}
6064

61-
func (u *User) IsActions() bool {
65+
func (u *User) IsGiteaActions() bool {
6266
return u != nil && u.ID == ActionsUserID
6367
}
68+
69+
func GetSystemUserByName(name string) *User {
70+
if IsGhostUserName(name) {
71+
return NewGhostUser()
72+
}
73+
if IsGiteaActionsUserName(name) {
74+
return NewActionsUser()
75+
}
76+
return nil
77+
}

‎models/user/user_system_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// Copyright 2025 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package user
5+
6+
import (
7+
"testing"
8+
9+
"code.gitea.io/gitea/models/db"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
func TestSystemUser(t *testing.T) {
16+
u, err := GetPossibleUserByID(db.DefaultContext, -1)
17+
require.NoError(t, err)
18+
assert.Equal(t, "Ghost", u.Name)
19+
assert.Equal(t, "ghost", u.LowerName)
20+
assert.True(t, u.IsGhost())
21+
assert.True(t, IsGhostUserName("gHost"))
22+
23+
u, err = GetPossibleUserByID(db.DefaultContext, -2)
24+
require.NoError(t, err)
25+
assert.Equal(t, "gitea-actions", u.Name)
26+
assert.Equal(t, "gitea-actions", u.LowerName)
27+
assert.True(t, u.IsGiteaActions())
28+
assert.True(t, IsGiteaActionsUserName("Gitea-actionS"))
29+
30+
_, err = GetPossibleUserByID(db.DefaultContext, -3)
31+
require.Error(t, err)
32+
}

‎models/user/user_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,14 +333,14 @@ func TestGetUserIDsByNames(t *testing.T) {
333333
func TestGetMaileableUsersByIDs(t *testing.T) {
334334
assert.NoError(t, unittest.PrepareTestDatabase())
335335

336-
results, err := user_model.GetMaileableUsersByIDs(db.DefaultContext, []int64{1, 4}, false)
336+
results, err := user_model.GetMailableUsersByIDs(db.DefaultContext, []int64{1, 4}, false)
337337
assert.NoError(t, err)
338338
assert.Len(t, results, 1)
339339
if len(results) > 1 {
340340
assert.Equal(t, 1, results[0].ID)
341341
}
342342

343-
results, err = user_model.GetMaileableUsersByIDs(db.DefaultContext, []int64{1, 4}, true)
343+
results, err = user_model.GetMailableUsersByIDs(db.DefaultContext, []int64{1, 4}, true)
344344
assert.NoError(t, err)
345345
assert.Len(t, results, 2)
346346
if len(results) > 2 {

‎modules/storage/storage.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func Clean(storage ObjectStorage) error {
9393
}
9494

9595
// SaveFrom saves data to the ObjectStorage with path p from the callback
96-
func SaveFrom(objStorage ObjectStorage, p string, callback func(w io.Writer) error) error {
96+
func SaveFrom(objStorage ObjectStorage, path string, callback func(w io.Writer) error) error {
9797
pr, pw := io.Pipe()
9898
defer pr.Close()
9999
go func() {
@@ -103,7 +103,7 @@ func SaveFrom(objStorage ObjectStorage, p string, callback func(w io.Writer) err
103103
}
104104
}()
105105

106-
_, err := objStorage.Save(p, pr, -1)
106+
_, err := objStorage.Save(path, pr, -1)
107107
return err
108108
}
109109

‎routers/web/user/avatar.go‎

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,27 +20,18 @@ func cacheableRedirect(ctx *context.Context, location string) {
2020
ctx.Redirect(location)
2121
}
2222

23-
// AvatarByUserName redirect browser to user avatar of requested size
24-
func AvatarByUserName(ctx *context.Context) {
25-
userName := ctx.PathParam("username")
26-
size := int(ctx.PathParamInt64("size"))
27-
28-
var user *user_model.User
29-
if !user_model.IsGhostUserName(userName) {
23+
// AvatarByUsernameSize redirect browser to user avatar of requested size
24+
func AvatarByUsernameSize(ctx *context.Context) {
25+
username := ctx.PathParam("username")
26+
user := user_model.GetSystemUserByName(username)
27+
if user == nil {
3028
var err error
31-
if user, err = user_model.GetUserByName(ctx, userName); err != nil {
32-
if user_model.IsErrUserNotExist(err) {
33-
ctx.NotFound("GetUserByName", err)
34-
return
35-
}
36-
ctx.ServerError("Invalid user: "+userName, err)
29+
if user, err = user_model.GetUserByName(ctx, username); err != nil {
30+
ctx.NotFoundOrServerError("GetUserByName", user_model.IsErrUserNotExist, err)
3731
return
3832
}
39-
} else {
40-
user = user_model.NewGhostUser()
4133
}
42-
43-
cacheableRedirect(ctx, user.AvatarLinkWithSize(ctx, size))
34+
cacheableRedirect(ctx, user.AvatarLinkWithSize(ctx, int(ctx.PathParamInt64("size"))))
4435
}
4536

4637
// AvatarByEmailHash redirects the browser to the email avatar link

‎routers/web/user/home.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -732,7 +732,7 @@ func UsernameSubRoute(ctx *context.Context) {
732732
switch {
733733
case strings.HasSuffix(username, ".png"):
734734
if reloadParam(".png") {
735-
AvatarByUserName(ctx)
735+
AvatarByUsernameSize(ctx)
736736
}
737737
case strings.HasSuffix(username, ".keys"):
738738
if reloadParam(".keys") {

‎routers/web/web.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -681,7 +681,7 @@ func registerRoutes(m *web.Router) {
681681
m.Get("/activate", auth.Activate)
682682
m.Post("/activate", auth.ActivatePost)
683683
m.Any("/activate_email", auth.ActivateEmail)
684-
m.Get("/avatar/{username}/{size}", user.AvatarByUserName)
684+
m.Get("/avatar/{username}/{size}", user.AvatarByUsernameSize)
685685
m.Get("/recover_account", auth.ResetPasswd)
686686
m.Post("/recover_account", auth.ResetPasswdPost)
687687
m.Get("/forgot_password", auth.ForgotPasswd)

0 commit comments

Comments
 (0)