preserve users if restoring a repository on the same Gitea instance (#18604)

When calling DumpRepository and RestoreRepository on the same Gitea
instance, the users are preserved: all labels, issues etc. belong to
the external user who is, in this particular case, the local user.

Dead code verifying g.gitServiceType.Name() == "" (i.e. plain git) is
removed. The function is never called because the plain git downloader
does not migrate anything that is associated to a user, by definition.

Errors returned by GetUserIDByExternalUserID are no longer ignored.

The userMap is used when the external user is not kown, which is the
most common case. It was only used when the external user exists
which happens less often and, as a result, every occurence of an
unknown external user required a SQL query.

Signed-off-by: Loïc Dachary <loic@dachary.org>
Co-authored-by: Loïc Dachary <loic@dachary.org>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

diff --git a/integrations/dump_restore_test.go b/integrations/dump_restore_test.go
index c0e583293c93a3d8f2c84548b8b41b3b3ca0a026..57968618bc3220d43e7c9da9ea2bcea5917cf041 100644 (file)
--- a/integrations/dump_restore_test.go
+++ b/integrations/dump_restore_test.go
@@ -129,6 +129,7 @@ func TestDumpRestore(t *testing.T) {
                                assert.EqualValues(t, before[i].Created, after[i].Created)
                                assert.EqualValues(t, before[i].Updated, after[i].Updated)
                                assert.EqualValues(t, before[i].Labels, after[i].Labels)
+                               assert.EqualValues(t, before[i].Reactions, after[i].Reactions)
                        }
                }
        })

diff --git a/models/user/user.go b/models/user/user.go
index 38352fe5e24c3a4f54d987f67341e8b6aacf21d8..5ed850bdfa83d86156a6b45d34a8afaed8e742cb 100644 (file)
--- a/models/user/user.go
+++ b/models/user/user.go
@@ -1025,6 +1025,19 @@ func GetUserNamesByIDs(ids []int64) ([]string, error) {
        return unames, err
 }
 
+// GetUserNameByID returns username for the id
+func GetUserNameByID(ctx context.Context, id int64) (string, error) {
+       var name string
+       has, err := db.GetEngine(db.DefaultContext).Table("user").Where("id = ?", id).Cols("name").Get(&name)
+       if err != nil {
+               return "", err
+       }
+       if has {
+               return name, nil
+       }
+       return "", nil
+}
+
 // GetUserIDsByNames returns a slice of ids corresponds to names.
 func GetUserIDsByNames(names []string, ignoreNonExistent bool) ([]int64, error) {
        ids := make([]int64, 0, len(names))

diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go
index da085c0d8db07a25ac30c962931aa2ba5df1d095..8f0505afc5d0b8555fab5d3ca41fe2d266b84267 100644 (file)
--- a/services/migrations/gitea_uploader.go
+++ b/services/migrations/gitea_uploader.go
@@ -46,6 +46,7 @@ type GiteaLocalUploader struct {
        issues         map[int64]*models.Issue
        gitRepo        *git.Repository
        prHeadCache    map[string]struct{}
+       sameApp        bool
        userMap        map[int64]int64 // external user id mapping to user id
        prCache        map[int64]*models.PullRequest
        gitServiceType structs.GitServiceType
@@ -128,6 +129,7 @@ func (g *GiteaLocalUploader) CreateRepo(repo *base.Repository, opts base.Migrate
                MirrorInterval: opts.MirrorInterval,
        }, NewMigrationHTTPTransport())
 
+       g.sameApp = strings.HasPrefix(repo.OriginalURL, setting.AppURL)
        g.repo = r
        if err != nil {
                return err
@@ -256,7 +258,7 @@ func (g *GiteaLocalUploader) CreateReleases(releases ...*base.Release) error {
                        CreatedUnix:  timeutil.TimeStamp(release.Created.Unix()),
                }
 
-               if err := g.remapExternalUser(release, &rel); err != nil {
+               if err := g.remapUser(release, &rel); err != nil {
                        return err
                }
 
@@ -373,7 +375,7 @@ func (g *GiteaLocalUploader) CreateIssues(issues ...*base.Issue) error {
                        UpdatedUnix: timeutil.TimeStamp(issue.Updated.Unix()),
                }
 
-               if err := g.remapExternalUser(issue, &is); err != nil {
+               if err := g.remapUser(issue, &is); err != nil {
                        return err
                }
 
@@ -386,7 +388,7 @@ func (g *GiteaLocalUploader) CreateIssues(issues ...*base.Issue) error {
                                Type:        reaction.Content,
                                CreatedUnix: timeutil.TimeStampNow(),
                        }
-                       if err := g.remapExternalUser(reaction, &res); err != nil {
+                       if err := g.remapUser(reaction, &res); err != nil {
                                return err
                        }
                        is.Reactions = append(is.Reactions, &res)
@@ -437,7 +439,7 @@ func (g *GiteaLocalUploader) CreateComments(comments ...*base.Comment) error {
                        UpdatedUnix: timeutil.TimeStamp(comment.Updated.Unix()),
                }
 
-               if err := g.remapExternalUser(comment, &cm); err != nil {
+               if err := g.remapUser(comment, &cm); err != nil {
                        return err
                }
 
@@ -447,7 +449,7 @@ func (g *GiteaLocalUploader) CreateComments(comments ...*base.Comment) error {
                                Type:        reaction.Content,
                                CreatedUnix: timeutil.TimeStampNow(),
                        }
-                       if err := g.remapExternalUser(reaction, &res); err != nil {
+                       if err := g.remapUser(reaction, &res); err != nil {
                                return err
                        }
                        cm.Reactions = append(cm.Reactions, &res)
@@ -471,7 +473,7 @@ func (g *GiteaLocalUploader) CreatePullRequests(prs ...*base.PullRequest) error
                        return err
                }
 
-               if err := g.remapExternalUser(pr, gpr.Issue); err != nil {
+               if err := g.remapUser(pr, gpr.Issue); err != nil {
                        return err
                }
 
@@ -626,7 +628,7 @@ func (g *GiteaLocalUploader) newPullRequest(pr *base.PullRequest) (*models.PullR
                UpdatedUnix: timeutil.TimeStamp(pr.Updated.Unix()),
        }
 
-       if err := g.remapExternalUser(pr, &issue); err != nil {
+       if err := g.remapUser(pr, &issue); err != nil {
                return nil, err
        }
 
@@ -636,7 +638,7 @@ func (g *GiteaLocalUploader) newPullRequest(pr *base.PullRequest) (*models.PullR
                        Type:        reaction.Content,
                        CreatedUnix: timeutil.TimeStampNow(),
                }
-               if err := g.remapExternalUser(reaction, &res); err != nil {
+               if err := g.remapUser(reaction, &res); err != nil {
                        return nil, err
                }
                issue.Reactions = append(issue.Reactions, &res)
@@ -710,7 +712,7 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {
                        UpdatedUnix: timeutil.TimeStamp(review.CreatedAt.Unix()),
                }
 
-               if err := g.remapExternalUser(review, &cm); err != nil {
+               if err := g.remapUser(review, &cm); err != nil {
                        return err
                }
 
@@ -773,7 +775,7 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {
                                UpdatedUnix: timeutil.TimeStamp(comment.UpdatedAt.Unix()),
                        }
 
-                       if err := g.remapExternalUser(review, &c); err != nil {
+                       if err := g.remapUser(review, &c); err != nil {
                                return err
                        }
 
@@ -816,23 +818,52 @@ func (g *GiteaLocalUploader) Finish() error {
        return repo_model.UpdateRepositoryCols(g.repo, "status")
 }
 
-func (g *GiteaLocalUploader) remapExternalUser(source user_model.ExternalUserMigrated, target user_model.ExternalUserRemappable) (err error) {
+func (g *GiteaLocalUploader) remapUser(source user_model.ExternalUserMigrated, target user_model.ExternalUserRemappable) error {
+       var userid int64
+       var err error
+       if g.sameApp {
+               userid, err = g.remapLocalUser(source, target)
+       } else {
+               userid, err = g.remapExternalUser(source, target)
+       }
+
+       if err != nil {
+               return err
+       }
+
+       if userid > 0 {
+               return target.RemapExternalUser("", 0, userid)
+       }
+       return target.RemapExternalUser(source.GetExternalName(), source.GetExternalID(), g.doer.ID)
+}
+
+func (g *GiteaLocalUploader) remapLocalUser(source user_model.ExternalUserMigrated, target user_model.ExternalUserRemappable) (int64, error) {
        userid, ok := g.userMap[source.GetExternalID()]
-       tp := g.gitServiceType.Name()
-       if !ok && tp != "" {
-               userid, err = user_model.GetUserIDByExternalUserID(tp, fmt.Sprintf("%d", source.GetExternalID()))
+       if !ok {
+               name, err := user_model.GetUserNameByID(g.ctx, source.GetExternalID())
                if err != nil {
-                       log.Error("GetUserIDByExternalUserID: %v", err)
+                       return 0, err
                }
-               if userid > 0 {
-                       g.userMap[source.GetExternalID()] = userid
+               // let's not reuse an ID when the user was deleted or has a different user name
+               if name != source.GetExternalName() {
+                       userid = 0
+               } else {
+                       userid = source.GetExternalID()
                }
+               g.userMap[source.GetExternalID()] = userid
        }
+       return userid, nil
+}
 
-       if userid > 0 {
-               err = target.RemapExternalUser("", 0, userid)
-       } else {
-               err = target.RemapExternalUser(source.GetExternalName(), source.GetExternalID(), g.doer.ID)
+func (g *GiteaLocalUploader) remapExternalUser(source user_model.ExternalUserMigrated, target user_model.ExternalUserRemappable) (userid int64, err error) {
+       userid, ok := g.userMap[source.GetExternalID()]
+       if !ok {
+               userid, err = user_model.GetUserIDByExternalUserID(g.gitServiceType.Name(), fmt.Sprintf("%d", source.GetExternalID()))
+               if err != nil {
+                       log.Error("GetUserIDByExternalUserID: %v", err)
+                       return 0, err
+               }
+               g.userMap[source.GetExternalID()] = userid
        }
-       return
+       return userid, nil
 }

diff --git a/services/migrations/gitea_uploader_test.go b/services/migrations/gitea_uploader_test.go
index 7552245d74463c4e2df0b840335bd5f611b716f1..0a35b5a632d4d426927fc8dc33b2b9c8f849cbfe 100644 (file)
--- a/services/migrations/gitea_uploader_test.go
+++ b/services/migrations/gitea_uploader_test.go
@@ -117,6 +117,56 @@ func TestGiteaUploadRepo(t *testing.T) {
        assert.Len(t, pulls[0].Issue.Comments, 2)
 }
 
+func TestGiteaUploadRemapLocalUser(t *testing.T) {
+       unittest.PrepareTestEnv(t)
+       doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}).(*user_model.User)
+       user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User)
+
+       repoName := "migrated"
+       uploader := NewGiteaLocalUploader(context.Background(), doer, doer.Name, repoName)
+       // call remapLocalUser
+       uploader.sameApp = true
+
+       externalID := int64(1234567)
+       externalName := "username"
+       source := base.Release{
+               PublisherID:   externalID,
+               PublisherName: externalName,
+       }
+
+       //
+       // The externalID does not match any existing user, everything
+       // belongs to the doer
+       //
+       target := models.Release{}
+       uploader.userMap = make(map[int64]int64)
+       err := uploader.remapUser(&source, &target)
+       assert.NoError(t, err)
+       assert.EqualValues(t, doer.ID, target.GetUserID())
+
+       //
+       // The externalID matches a known user but the name does not match,
+       // everything belongs to the doer
+       //
+       source.PublisherID = user.ID
+       target = models.Release{}
+       uploader.userMap = make(map[int64]int64)
+       err = uploader.remapUser(&source, &target)
+       assert.NoError(t, err)
+       assert.EqualValues(t, doer.ID, target.GetUserID())
+
+       //
+       // The externalID and externalName match an existing user, everything
+       // belongs to the existing user
+       //
+       source.PublisherName = user.Name
+       target = models.Release{}
+       uploader.userMap = make(map[int64]int64)
+       err = uploader.remapUser(&source, &target)
+       assert.NoError(t, err)
+       assert.EqualValues(t, user.ID, target.GetUserID())
+}
+
 func TestGiteaUploadRemapExternalUser(t *testing.T) {
        unittest.PrepareTestEnv(t)
        doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}).(*user_model.User)
@@ -124,9 +174,11 @@ func TestGiteaUploadRemapExternalUser(t *testing.T) {
        repoName := "migrated"
        uploader := NewGiteaLocalUploader(context.Background(), doer, doer.Name, repoName)
        uploader.gitServiceType = structs.GiteaService
+       // call remapExternalUser
+       uploader.sameApp = false
 
        externalID := int64(1234567)
-       externalName := "url.or.something"
+       externalName := "username"
        source := base.Release{
                PublisherID:   externalID,
                PublisherName: externalName,
@@ -136,8 +188,9 @@ func TestGiteaUploadRemapExternalUser(t *testing.T) {
        // When there is no user linked to the external ID, the migrated data is authored
        // by the doer
        //
+       uploader.userMap = make(map[int64]int64)
        target := models.Release{}
-       err := uploader.remapExternalUser(&source, &target)
+       err := uploader.remapUser(&source, &target)
        assert.NoError(t, err)
        assert.EqualValues(t, doer.ID, target.GetUserID())
 
@@ -158,8 +211,9 @@ func TestGiteaUploadRemapExternalUser(t *testing.T) {
        // When a user is linked to the external ID, it becomes the author of
        // the migrated data
        //
+       uploader.userMap = make(map[int64]int64)
        target = models.Release{}
-       err = uploader.remapExternalUser(&source, &target)
+       err = uploader.remapUser(&source, &target)
        assert.NoError(t, err)
-       assert.EqualValues(t, target.GetUserID(), linkedUser.ID)
+       assert.EqualValues(t, linkedUser.ID, target.GetUserID())
 }