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

@@ -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()
 	}
 

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

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

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
+}

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

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 {

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
 }
 

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

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") {

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)

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.

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
 	}

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
 	}