Make GetPossibleUserByID can handle deleted user (#37430)

Make sure deleted user won't cause 500 error, simplify the caller's code

models/actions/run.go

@@ -120,7 +120,7 @@ func (run *ActionRun) RefTooltip() string {
 }
 
 // LoadAttributes load Repo TriggerUser if not loaded
-func (run *ActionRun) LoadAttributes(ctx context.Context) error {
+func (run *ActionRun) LoadAttributes(ctx context.Context) (err error) {
 	if run == nil {
 		return nil
 	}
@@ -134,11 +134,10 @@ func (run *ActionRun) LoadAttributes(ctx context.Context) error {
 	}
 
 	if run.TriggerUser == nil {
-		u, err := user_model.GetPossibleUserByID(ctx, run.TriggerUserID)
+		run.TriggerUserID, run.TriggerUser, err = user_model.GetPossibleUserByID(ctx, run.TriggerUserID)
 		if err != nil {
 			return err
 		}
-		run.TriggerUser = u
 	}
 
 	return nil

models/actions/run_attempt.go

@@ -50,7 +50,7 @@ func (attempt *ActionRunAttempt) Duration() time.Duration {
 	return calculateDuration(attempt.Started, attempt.Stopped, attempt.Status, attempt.Updated)
 }
 
-func (attempt *ActionRunAttempt) LoadAttributes(ctx context.Context) error {
+func (attempt *ActionRunAttempt) LoadAttributes(ctx context.Context) (err error) {
 	if attempt == nil {
 		return nil
 	}
@@ -67,11 +67,10 @@ func (attempt *ActionRunAttempt) LoadAttributes(ctx context.Context) error {
 	}
 
 	if attempt.TriggerUser == nil {
-		u, err := user_model.GetPossibleUserByID(ctx, attempt.TriggerUserID)
+		attempt.TriggerUserID, attempt.TriggerUser, err = user_model.GetPossibleUserByID(ctx, attempt.TriggerUserID)
 		if err != nil {
 			return err
 		}
-		attempt.TriggerUser = u
 	}
 
 	return nil

models/activities/action.go

@@ -186,15 +186,7 @@ func (a *Action) LoadActUser(ctx context.Context) {
 	if a.ActUser != nil {
 		return
 	}
-	var err error
-	a.ActUser, err = user_model.GetPossibleUserByID(ctx, a.ActUserID)
-	if err == nil {
-		return
-	} else if user_model.IsErrUserNotExist(err) {
-		a.ActUser = user_model.NewGhostUser()
-	} else {
-		log.Error("GetUserByID(%d): %v", a.ActUserID, err)
-	}
+	a.ActUserID, a.ActUser, _ = user_model.GetPossibleUserByID(ctx, a.ActUserID)
 }
 
 func (a *Action) LoadRepo(ctx context.Context) error {

models/issues/comment.go

@@ -400,16 +400,7 @@ func (c *Comment) LoadPoster(ctx context.Context) (err error) {
 	if c.Poster != nil {
 		return nil
 	}
-
-	c.Poster, err = user_model.GetPossibleUserByID(ctx, c.PosterID)
-	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
-			c.PosterID = user_model.GhostUserID
-			c.Poster = user_model.NewGhostUser()
-		} else {
-			log.Error("getUserByID[%d]: %v", c.ID, err)
-		}
-	}
+	c.PosterID, c.Poster, err = user_model.GetPossibleUserByID(ctx, c.PosterID)
 	return err
 }
 

models/issues/issue.go

@@ -175,17 +175,10 @@ func (issue *Issue) IsTimetrackerEnabled(ctx context.Context) bool {
 
 // LoadPoster loads poster
 func (issue *Issue) LoadPoster(ctx context.Context) (err error) {
-	if issue.Poster == nil && issue.PosterID != 0 {
-		issue.Poster, err = user_model.GetPossibleUserByID(ctx, issue.PosterID)
-		if err != nil {
-			issue.PosterID = user_model.GhostUserID
-			issue.Poster = user_model.NewGhostUser()
-			if !user_model.IsErrUserNotExist(err) {
-				return fmt.Errorf("getUserByID.(poster) [%d]: %w", issue.PosterID, err)
-			}
-			return nil
-		}
+	if issue.Poster != nil {
+		return nil
 	}
+	issue.PosterID, issue.Poster, err = user_model.GetPossibleUserByID(ctx, issue.PosterID)
 	return err
 }
 

models/issues/review.go

@@ -176,15 +176,7 @@ func (r *Review) LoadReviewer(ctx context.Context) (err error) {
 	if r.ReviewerID == 0 || r.Reviewer != nil {
 		return err
 	}
-	r.Reviewer, err = user_model.GetPossibleUserByID(ctx, r.ReviewerID)
-	if err != nil {
-		if !user_model.IsErrUserNotExist(err) {
-			return fmt.Errorf("GetPossibleUserByID [%d]: %w", r.ReviewerID, err)
-		}
-		r.ReviewerID = user_model.GhostUserID
-		r.Reviewer = user_model.NewGhostUser()
-		return nil
-	}
+	r.ReviewerID, r.Reviewer, err = user_model.GetPossibleUserByID(ctx, r.ReviewerID)
 	return err
 }
 

models/pull/automerge.go

@@ -5,14 +5,12 @@ package pull
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/timeutil"
-	"code.gitea.io/gitea/modules/util"
 )
 
 // AutoMerge represents a pull request scheduled for merging when checks succeed
@@ -78,16 +76,8 @@ func GetScheduledMergeByPullID(ctx context.Context, pullID int64) (bool, *AutoMe
 		return false, nil, err
 	}
 
-	doer, err := user_model.GetPossibleUserByID(ctx, scheduledPRM.DoerID)
-	if errors.Is(err, util.ErrNotExist) {
-		doer, err = user_model.NewGhostUser(), nil
-	}
-	if err != nil {
-		return false, nil, err
-	}
-
-	scheduledPRM.Doer = doer
-	return true, scheduledPRM, nil
+	scheduledPRM.DoerID, scheduledPRM.Doer, err = user_model.GetPossibleUserByID(ctx, scheduledPRM.DoerID)
+	return true, scheduledPRM, err
 }
 
 // DeleteScheduledAutoMerge delete a scheduled pull request

models/user/user.go

@@ -7,6 +7,7 @@ package user
 import (
 	"context"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"html/template"
 	"mime"
@@ -1013,17 +1014,22 @@ func GetUserByIDs(ctx context.Context, ids []int64) ([]*User, error) {
 	return users, err
 }
 
-// GetPossibleUserByID returns the user if id > 0 or returns system user if id < 0
-func GetPossibleUserByID(ctx context.Context, id int64) (*User, error) {
+// GetPossibleUserByID returns the possible user and its ID. If the user  doesn't exist, it returns Ghost user
+func GetPossibleUserByID(ctx context.Context, id int64) (_ int64, u *User, err error) {
 	if id < 0 {
 		if newFunc, ok := globalVars().systemUserNewFuncs[id]; ok {
-			return newFunc(), nil
+			u = newFunc()
 		}
-		return nil, ErrUserNotExist{UID: id}
-	} else if id == 0 {
-		return nil, ErrUserNotExist{}
 	}
-	return GetUserByID(ctx, id)
+	if u == nil {
+		u, err = GetUserByID(ctx, id)
+		if errors.Is(err, util.ErrNotExist) {
+			u = NewGhostUser()
+		} else if err != nil {
+			return 0, nil, err
+		}
+	}
+	return u.ID, u, nil
 }
 
 // GetPossibleUserByIDs returns the users if id > 0 or returns system users if id < 0

models/user/user_system_test.go

@@ -11,8 +11,9 @@ import (
 )
 
 func TestSystemUser(t *testing.T) {
-	u, err := GetPossibleUserByID(t.Context(), -1)
+	uid, u, err := GetPossibleUserByID(t.Context(), -1)
 	require.NoError(t, err)
+	assert.Equal(t, int64(-1), uid)
 	assert.Equal(t, "Ghost", u.Name)
 	assert.Equal(t, "ghost", u.LowerName)
 	assert.True(t, u.IsGhost())
@@ -21,8 +22,9 @@ func TestSystemUser(t *testing.T) {
 	require.NotNil(t, u)
 	assert.Equal(t, "Ghost", u.Name)
 
-	u, err = GetPossibleUserByID(t.Context(), -2)
+	uid, u, err = GetPossibleUserByID(t.Context(), -2)
 	require.NoError(t, err)
+	assert.Equal(t, int64(-2), uid)
 	assert.Equal(t, "gitea-actions", u.Name)
 	assert.Equal(t, "gitea-actions", u.LowerName)
 	assert.True(t, u.IsGiteaActions())
@@ -31,6 +33,8 @@ func TestSystemUser(t *testing.T) {
 	require.NotNil(t, u)
 	assert.Equal(t, "Gitea Actions", u.FullName)
 
-	_, err = GetPossibleUserByID(t.Context(), -3)
-	require.Error(t, err)
+	uid, u, err = GetPossibleUserByID(t.Context(), 999999)
+	require.NoError(t, err)
+	assert.Equal(t, int64(-1), uid)
+	assert.Equal(t, "Ghost", u.Name)
 }

routers/web/repo/release.go

@@ -104,13 +104,9 @@ func getReleaseInfos(ctx *context.Context, opts *repo_model.FindReleasesOptions)
 	releaseInfos := make([]*ReleaseInfo, 0, len(releases))
 	for _, r := range releases {
 		if r.Publisher, ok = cacheUsers[r.PublisherID]; !ok {
-			r.Publisher, err = user_model.GetPossibleUserByID(ctx, r.PublisherID)
+			r.PublisherID, r.Publisher, err = user_model.GetPossibleUserByID(ctx, r.PublisherID)
 			if err != nil {
-				if user_model.IsErrUserNotExist(err) {
-					r.Publisher = user_model.NewGhostUser()
-				} else {
-					return nil, err
-				}
+				return nil, err
 			}
 			cacheUsers[r.PublisherID] = r.Publisher
 		}

services/convert/convert.go

@@ -831,7 +831,7 @@ func ToOAuth2Application(app *auth.OAuth2Application) *api.OAuth2Application {
 
 // ToLFSLock convert a LFSLock to api.LFSLock
 func ToLFSLock(ctx context.Context, l *git_model.LFSLock) *api.LFSLock {
-	u, err := user_model.GetPossibleUserByID(ctx, l.OwnerID)
+	_, u, err := user_model.GetPossibleUserByID(ctx, l.OwnerID)
 	if err != nil {
 		return nil
 	}