refactor(db): migrate `ChangeUsername` off `user.go` (#7261)

5 files changed, 205 insertions, 62 deletions

diff --git a/internal/db/pull.go b/internal/db/pull.go
index e2f55361..b0f723fa 100644
--- a/internal/db/pull.go
+++ b/internal/db/pull.go
@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strings"
 	"time"
 
 	"github.com/unknwon/com"
@@ -45,28 +44,28 @@ const (
 
 // PullRequest represents relation between pull request and repositories.
 type PullRequest struct {
-	ID     int64
+	ID     int64 `gorm:"primaryKey"`
 	Type   PullRequestType
 	Status PullRequestStatus
 
-	IssueID int64  `xorm:"INDEX"`
-	Issue   *Issue `xorm:"-" json:"-"`
+	IssueID int64  `xorm:"INDEX" gorm:"index"`
+	Issue   *Issue `xorm:"-" json:"-" gorm:"-"`
 	Index   int64
 
 	HeadRepoID   int64
-	HeadRepo     *Repository `xorm:"-" json:"-"`
+	HeadRepo     *Repository `xorm:"-" json:"-" gorm:"-"`
 	BaseRepoID   int64
-	BaseRepo     *Repository `xorm:"-" json:"-"`
+	BaseRepo     *Repository `xorm:"-" json:"-" gorm:"-"`
 	HeadUserName string
 	HeadBranch   string
 	BaseBranch   string
-	MergeBase    string `xorm:"VARCHAR(40)"`
+	MergeBase    string `xorm:"VARCHAR(40)" gorm:"type:VARCHAR(40)"`
 
 	HasMerged      bool
-	MergedCommitID string `xorm:"VARCHAR(40)"`
+	MergedCommitID string `xorm:"VARCHAR(40)" gorm:"type:VARCHAR(40)"`
 	MergerID       int64
-	Merger         *User     `xorm:"-" json:"-"`
-	Merged         time.Time `xorm:"-" json:"-"`
+	Merger         *User     `xorm:"-" json:"-" gorm:"-"`
+	Merged         time.Time `xorm:"-" json:"-" gorm:"-"`
 	MergedUnix     int64
 }
 
@@ -823,14 +822,6 @@ func AddTestPullRequestTask(doer *User, repoID int64, branch string, isSync bool
 	}
 }
 
-func ChangeUsernameInPullRequests(oldUserName, newUserName string) error {
-	pr := PullRequest{
-		HeadUserName: strings.ToLower(newUserName),
-	}
-	_, err := x.Cols("head_user_name").Where("head_user_name = ?", strings.ToLower(oldUserName)).Update(pr)
-	return err
-}
-
 // checkAndUpdateStatus checks if pull request is possible to leaving checking status,
 // and set to be either conflict or mergeable.
 func (pr *PullRequest) checkAndUpdateStatus() {
diff --git a/internal/db/repo.go b/internal/db/repo.go
index f9e19468..8abe47ed 100644
--- a/internal/db/repo.go
+++ b/internal/db/repo.go
@@ -1444,7 +1444,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 		return fmt.Errorf("rename repository directory: %v", err)
 	}
 
-	deleteRepoLocalCopy(repo)
+	deleteRepoLocalCopy(repo.ID)
 
 	// Rename remote wiki repository to new path and delete local copy.
 	wikiPath := WikiPath(owner.Name, repo.Name)
@@ -1458,10 +1458,10 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 	return sess.Commit()
 }
 
-func deleteRepoLocalCopy(repo *Repository) {
-	repoWorkingPool.CheckIn(com.ToStr(repo.ID))
-	defer repoWorkingPool.CheckOut(com.ToStr(repo.ID))
-	RemoveAllWithNotice("Delete repository local copy", repo.LocalCopyPath())
+func deleteRepoLocalCopy(repoID int64) {
+	repoWorkingPool.CheckIn(com.ToStr(repoID))
+	defer repoWorkingPool.CheckOut(com.ToStr(repoID))
+	RemoveAllWithNotice(fmt.Sprintf("Delete repository %d local copy", repoID), repoutil.RepositoryLocalPath(repoID))
 }
 
 // ChangeRepositoryName changes all corresponding setting from old repository name to new one.
@@ -1497,7 +1497,7 @@ func ChangeRepositoryName(u *User, oldRepoName, newRepoName string) (err error)
 		RemoveAllWithNotice("Delete repository wiki local copy", repo.LocalWikiPath())
 	}
 
-	deleteRepoLocalCopy(repo)
+	deleteRepoLocalCopy(repo.ID)
 	return nil
 }
 
diff --git a/internal/db/user.go b/internal/db/user.go
index 922a340a..5d75bfca 100644
--- a/internal/db/user.go
+++ b/internal/db/user.go
@@ -12,7 +12,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/unknwon/com"
 	log "unknwon.dev/clog/v2"
 	"xorm.io/xorm"
 
@@ -57,40 +56,6 @@ func (u *User) getOrganizationCount(e Engine) (int64, error) {
 	return e.Where("uid=?", u.ID).Count(new(OrgUser))
 }
 
-// ChangeUserName changes all corresponding setting from old user name to new one.
-func ChangeUserName(u *User, newUserName string) (err error) {
-	if err = isUsernameAllowed(newUserName); err != nil {
-		return err
-	}
-
-	if Users.IsUsernameUsed(context.TODO(), newUserName) {
-		return ErrUserAlreadyExist{args: errutil.Args{"name": newUserName}}
-	}
-
-	if err = ChangeUsernameInPullRequests(u.Name, newUserName); err != nil {
-		return fmt.Errorf("ChangeUsernameInPullRequests: %v", err)
-	}
-
-	// Delete all local copies of repositories and wikis the user owns.
-	if err = x.Where("owner_id=?", u.ID).Iterate(new(Repository), func(idx int, bean interface{}) error {
-		repo := bean.(*Repository)
-		deleteRepoLocalCopy(repo)
-		// TODO: By the same reasoning, shouldn't we also sync access to the local wiki path?
-		RemoveAllWithNotice("Delete repository wiki local copy", repo.LocalWikiPath())
-		return nil
-	}); err != nil {
-		return fmt.Errorf("delete repository and wiki local copy: %v", err)
-	}
-
-	// Rename or create user base directory
-	baseDir := repoutil.UserPath(u.Name)
-	newBaseDir := repoutil.UserPath(newUserName)
-	if com.IsExist(baseDir) {
-		return os.Rename(baseDir, newBaseDir)
-	}
-	return os.MkdirAll(newBaseDir, os.ModePerm)
-}
-
 func updateUser(e Engine, u *User) error {
 	// Organization does not need email
 	if !u.IsOrganization() {
diff --git a/internal/db/users.go b/internal/db/users.go
index f12eca09..12fa90a2 100644
--- a/internal/db/users.go
+++ b/internal/db/users.go
@@ -24,6 +24,7 @@ import (
 	"gogs.io/gogs/internal/dbutil"
 	"gogs.io/gogs/internal/errutil"
 	"gogs.io/gogs/internal/osutil"
+	"gogs.io/gogs/internal/repoutil"
 	"gogs.io/gogs/internal/strutil"
 	"gogs.io/gogs/internal/tool"
 	"gogs.io/gogs/internal/userutil"
@@ -45,11 +46,17 @@ type UsersStore interface {
 	// When the "loginSourceID" is positive, it tries to authenticate via given
 	// login source and creates a new user when not yet exists in the database.
 	Authenticate(ctx context.Context, username, password string, loginSourceID int64) (*User, error)
+	// ChangeUsername changes the username of the given user and updates all
+	// references to the old username. It returns ErrNameNotAllowed if the given
+	// name or pattern of the name is not allowed as a username, or
+	// ErrUserAlreadyExist when another user with same name already exists.
+	ChangeUsername(ctx context.Context, userID int64, newUsername string) error
 	// Count returns the total number of users.
 	Count(ctx context.Context) int64
 	// Create creates a new user and persists to database. It returns
-	// ErrUserAlreadyExist when a user with same name already exists, or
-	// ErrEmailAlreadyUsed if the email has been used by another user.
+	// ErrNameNotAllowed if the given name or pattern of the name is not allowed as
+	// a username, or ErrUserAlreadyExist when a user with same name already exists,
+	// or ErrEmailAlreadyUsed if the email has been used by another user.
 	Create(ctx context.Context, username, email string, opts CreateUserOptions) (*User, error)
 	// DeleteCustomAvatar deletes the current user custom avatar and falls back to
 	// use look up avatar by email.
@@ -188,6 +195,81 @@ func (db *users) Authenticate(ctx context.Context, login, password string, login
 	)
 }
 
+func (db *users) ChangeUsername(ctx context.Context, userID int64, newUsername string) error {
+	err := isUsernameAllowed(newUsername)
+	if err != nil {
+		return err
+	}
+
+	if db.IsUsernameUsed(ctx, newUsername) {
+		return ErrUserAlreadyExist{
+			args: errutil.Args{
+				"name": newUsername,
+			},
+		}
+	}
+
+	user, err := db.GetByID(ctx, userID)
+	if err != nil {
+		return errors.Wrap(err, "get user")
+	}
+
+	return db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
+		err := tx.Model(&User{}).
+			Where("id = ?", user.ID).
+			Updates(map[string]any{
+				"lower_name": strings.ToLower(newUsername),
+				"name":       newUsername,
+			}).Error
+		if err != nil {
+			return errors.Wrap(err, "update user name")
+		}
+
+		// Update all references to the user name in pull requests
+		err = tx.Model(&PullRequest{}).
+			Where("head_user_name = ?", user.LowerName).
+			Update("head_user_name", strings.ToLower(newUsername)).
+			Error
+		if err != nil {
+			return errors.Wrap(err, `update "pull_request.head_user_name"`)
+		}
+
+		// Delete local copies of repositories and their wikis that are owned by the user
+		rows, err := tx.Model(&Repository{}).Where("owner_id = ?", user.ID).Rows()
+		if err != nil {
+			return errors.Wrap(err, "iterate repositories")
+		}
+		defer func() { _ = rows.Close() }()
+
+		for rows.Next() {
+			var repo struct {
+				ID int64
+			}
+			err = tx.ScanRows(rows, &repo)
+			if err != nil {
+				return errors.Wrap(err, "scan rows")
+			}
+
+			deleteRepoLocalCopy(repo.ID)
+			RemoveAllWithNotice(fmt.Sprintf("Delete repository %d wiki local copy", repo.ID), repoutil.RepositoryLocalWikiPath(repo.ID))
+		}
+		if err = rows.Err(); err != nil {
+			return errors.Wrap(err, "check rows.Err")
+		}
+
+		// Rename user directory if exists
+		userPath := repoutil.UserPath(user.Name)
+		if osutil.IsExist(userPath) {
+			newUserPath := repoutil.UserPath(newUsername)
+			err = os.Rename(userPath, newUserPath)
+			if err != nil {
+				return errors.Wrap(err, "rename user directory")
+			}
+		}
+		return nil
+	})
+}
+
 func (db *users) Count(ctx context.Context) int64 {
 	var count int64
 	db.WithContext(ctx).Model(&User{}).Where("type = ?", UserTypeIndividual).Count(&count)
diff --git a/internal/db/users_test.go b/internal/db/users_test.go
index 40006330..697bad8f 100644
--- a/internal/db/users_test.go
+++ b/internal/db/users_test.go
@@ -8,6 +8,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -17,10 +18,12 @@ import (
 	"gorm.io/gorm"
 
 	"gogs.io/gogs/internal/auth"
+	"gogs.io/gogs/internal/conf"
 	"gogs.io/gogs/internal/dbtest"
 	"gogs.io/gogs/internal/dbutil"
 	"gogs.io/gogs/internal/errutil"
 	"gogs.io/gogs/internal/osutil"
+	"gogs.io/gogs/internal/repoutil"
 	"gogs.io/gogs/internal/userutil"
 	"gogs.io/gogs/public"
 )
@@ -79,7 +82,7 @@ func TestUsers(t *testing.T) {
 	}
 	t.Parallel()
 
-	tables := []interface{}{new(User), new(EmailAddress), new(Repository), new(Follow)}
+	tables := []interface{}{new(User), new(EmailAddress), new(Repository), new(Follow), new(PullRequest)}
 	db := &users{
 		DB: dbtest.NewDB(t, "users", tables...),
 	}
@@ -89,6 +92,7 @@ func TestUsers(t *testing.T) {
 		test func(t *testing.T, db *users)
 	}{
 		{"Authenticate", usersAuthenticate},
+		{"ChangeUsername", usersChangeUsername},
 		{"Count", usersCount},
 		{"Create", usersCreate},
 		{"DeleteCustomAvatar", usersDeleteCustomAvatar},
@@ -212,6 +216,107 @@ func usersAuthenticate(t *testing.T, db *users) {
 	})
 }
 
+func usersChangeUsername(t *testing.T, db *users) {
+	ctx := context.Background()
+
+	alice, err := db.Create(
+		ctx,
+		"alice",
+		"alice@example.com",
+		CreateUserOptions{
+			Activated: true,
+		},
+	)
+	require.NoError(t, err)
+
+	t.Run("name not allowed", func(t *testing.T) {
+		err := db.ChangeUsername(ctx, alice.ID, "-")
+		wantErr := ErrNameNotAllowed{
+			args: errutil.Args{
+				"reason": "reserved",
+				"name":   "-",
+			},
+		}
+		assert.Equal(t, wantErr, err)
+	})
+
+	t.Run("name already exists", func(t *testing.T) {
+		err := db.ChangeUsername(ctx, alice.ID, alice.Name)
+		wantErr := ErrUserAlreadyExist{
+			args: errutil.Args{
+				"name": alice.Name,
+			},
+		}
+		assert.Equal(t, wantErr, err)
+	})
+
+	tempRepositoryRoot := filepath.Join(os.TempDir(), "usersChangeUsername-tempRepositoryRoot")
+	conf.SetMockRepository(
+		t,
+		conf.RepositoryOpts{
+			Root: tempRepositoryRoot,
+		},
+	)
+	err = os.RemoveAll(tempRepositoryRoot)
+	require.NoError(t, err)
+	defer func() { _ = os.RemoveAll(tempRepositoryRoot) }()
+
+	tempServerAppDataPath := filepath.Join(os.TempDir(), "usersChangeUsername-tempServerAppDataPath")
+	conf.SetMockServer(
+		t,
+		conf.ServerOpts{
+			AppDataPath: tempServerAppDataPath,
+		},
+	)
+	err = os.RemoveAll(tempServerAppDataPath)
+	require.NoError(t, err)
+	defer func() { _ = os.RemoveAll(tempServerAppDataPath) }()
+
+	repo, err := NewReposStore(db.DB).Create(
+		ctx,
+		alice.ID,
+		CreateRepoOptions{
+			Name: "test-repo-1",
+		},
+	)
+	require.NoError(t, err)
+
+	// TODO: Use PullRequests.Create to replace SQL hack when the method is available.
+	err = db.Exec(`INSERT INTO pull_request (head_user_name) VALUES (?)`, alice.Name).Error
+	require.NoError(t, err)
+
+	err = os.MkdirAll(repoutil.UserPath(alice.Name), os.ModePerm)
+	require.NoError(t, err)
+	err = os.MkdirAll(repoutil.RepositoryLocalPath(repo.ID), os.ModePerm)
+	require.NoError(t, err)
+	err = os.MkdirAll(repoutil.RepositoryLocalWikiPath(repo.ID), os.ModePerm)
+	require.NoError(t, err)
+
+	// Make sure mock data is set up correctly
+	assert.True(t, osutil.IsExist(repoutil.UserPath(alice.Name)))
+	assert.True(t, osutil.IsExist(repoutil.RepositoryLocalPath(repo.ID)))
+	assert.True(t, osutil.IsExist(repoutil.RepositoryLocalWikiPath(repo.ID)))
+
+	const newUsername = "alice-new"
+	err = db.ChangeUsername(ctx, alice.ID, newUsername)
+	require.NoError(t, err)
+
+	// TODO: Use PullRequests.GetByID to replace SQL hack when the method is available.
+	var headUserName string
+	err = db.Select("head_user_name").Table("pull_request").Row().Scan(&headUserName)
+	require.NoError(t, err)
+	assert.Equal(t, headUserName, newUsername)
+
+	assert.True(t, osutil.IsExist(repoutil.UserPath(newUsername)))
+	assert.False(t, osutil.IsExist(repoutil.UserPath(alice.Name)))
+	assert.False(t, osutil.IsExist(repoutil.RepositoryLocalPath(repo.ID)))
+	assert.False(t, osutil.IsExist(repoutil.RepositoryLocalWikiPath(repo.ID)))
+
+	alice, err = db.GetByID(ctx, alice.ID)
+	require.NoError(t, err)
+	assert.Equal(t, newUsername, alice.Name)
+}
+
 func usersCount(t *testing.T, db *users) {
 	ctx := context.Background()