Fetch repositories with remote ID if possible (#1078)

Use IDs of the forge to fetch repositories instead of their names and owner names. This improves handling of renamed and transferred repos.

TODO

- [ ] try to support as many forges as possible
    - [x] Gogs (no API)
    - [ ] Bitbucket Server
    - [x] Coding (no API?)
- [x] update repo every time it is fetched or received from the forge
- [x] if repo remote IDs are not available, use owner / name to get it
- [x] handle redirections (redirect a renamed repo to its new path)
- [x] ~~pull all repos once during migration to update ID (?)~~ issue fixed by on-demand loading of remote IDs
- [x] handle redirections in web UI
- [ ] improve handling of hooks after a repo was renamed (currently it checks for a redirection to the repo)
- [x] tests
- [x] `UNIQUE` constraint for remote IDs after migration shouldn't work (all repos have an empty string as remote ID)

close #854
close #648 partial
close https://codeberg.org/Codeberg-CI/feedback/issues/46

Possible follow-up PRs
- apply the same scheme on everything fetched from the remote (currently only users)

Co-authored-by: 6543 <6543@obermui.de>

Makefile

@@ -57,8 +57,8 @@ vendor:
 	go mod tidy
 	go mod vendor
 
-format:
-	@gofmt -s -w ${GOFILES_NOVENDOR}
+format: install-tools
+	@gofumpt -extra -w ${GOFILES_NOVENDOR}
 
 .PHONY: docs
 docs:
@@ -140,6 +140,9 @@ install-tools:
 	fi ; \
 	hash lint > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
 		go install github.com/rs/zerolog/cmd/lint@latest; \
+	fi ; \
+	hash gofumpt > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
+		go install mvdan.cc/gofumpt@latest; \
 	fi
 
 cross-compile-server:

server/api/hook.go

@@ -69,8 +69,9 @@ func BlockTilQueueHasRunningItem(c *gin.Context) {
 // PostHook start a pipeline triggered by a forges post webhook
 func PostHook(c *gin.Context) {
 	_store := store.FromContext(c)
+	remote := server.Config.Services.Remote
 
-	tmpRepo, tmpBuild, err := server.Config.Services.Remote.Hook(c, c.Request)
+	tmpRepo, tmpBuild, err := remote.Hook(c, c.Request)
 	if err != nil {
 		msg := "failure to parse hook"
 		log.Debug().Err(err).Msg(msg)
@@ -100,7 +101,7 @@ func PostHook(c *gin.Context) {
 		return
 	}
 
-	repo, err := _store.GetRepoName(tmpRepo.Owner + "/" + tmpRepo.Name)
+	repo, err := _store.GetRepoNameFallback(tmpRepo.RemoteID, tmpRepo.FullName)
 	if err != nil {
 		msg := fmt.Sprintf("failure to get repo %s from store", tmpRepo.FullName)
 		log.Error().Err(err).Msg(msg)
@@ -114,6 +115,23 @@ func PostHook(c *gin.Context) {
 		return
 	}
 
+	oldFullName := repo.FullName
+	if oldFullName != tmpRepo.FullName {
+		// create a redirection
+		err = _store.CreateRedirection(&model.Redirection{RepoID: repo.ID, FullName: repo.FullName})
+		if err != nil {
+			_ = c.AbortWithError(http.StatusInternalServerError, err)
+			return
+		}
+	}
+
+	repo.Update(tmpRepo)
+	err = _store.UpdateRepo(repo)
+	if err != nil {
+		c.String(http.StatusInternalServerError, err.Error())
+		return
+	}
+
 	// get the token and verify the hook is authorized
 	parsed, err := token.ParseRequest(c.Request, func(_ *token.Token) (string, error) {
 		return repo.Hash, nil
@@ -124,7 +142,18 @@ func PostHook(c *gin.Context) {
 		c.String(http.StatusBadRequest, msg)
 		return
 	}
-	if parsed.Text != repo.FullName {
+	verifiedKey := parsed.Text == oldFullName
+	if !verifiedKey {
+		verifiedKey, err = _store.HasRedirectionForRepo(repo.ID, repo.FullName)
+		if err != nil {
+			msg := "failure to verify token from hook. Could not check for redirections of the repo"
+			log.Error().Err(err).Msg(msg)
+			c.String(http.StatusInternalServerError, msg)
+			return
+		}
+	}
+
+	if !verifiedKey {
 		msg := fmt.Sprintf("failure to verify token from hook. Expected %s, got %s", repo.FullName, parsed.Text)
 		log.Debug().Msg(msg)
 		c.String(http.StatusForbidden, msg)

server/api/repo.go

@@ -86,17 +86,25 @@ func PostRepo(c *gin.Context) {
 		sig,
 	)
 
+	from, err := remote.Repo(c, user, repo.RemoteID, repo.Owner, repo.Name)
+	if err == nil {
+		if repo.FullName != from.FullName {
+			// create a redirection
+			err = _store.CreateRedirection(&model.Redirection{RepoID: repo.ID, FullName: repo.FullName})
+			if err != nil {
+				_ = c.AbortWithError(http.StatusInternalServerError, err)
+				return
+			}
+		}
+		repo.Update(from)
+	}
+
 	err = remote.Activate(c, user, repo, link)
 	if err != nil {
 		c.String(http.StatusInternalServerError, err.Error())
 		return
 	}
 
-	from, err := remote.Repo(c, user, repo.Owner, repo.Name)
-	if err == nil {
-		repo.Update(from)
-	}
-
 	err = _store.UpdateRepo(repo)
 	if err != nil {
 		c.String(http.StatusInternalServerError, err.Error())
@@ -251,35 +259,36 @@ func RepairRepo(c *gin.Context) {
 		sig,
 	)
 
-	if err := remote.Deactivate(c, user, repo, host); err != nil {
-		log.Trace().Err(err).Msgf("deactivate repo '%s' to repair failed", repo.FullName)
-	}
-	if err := remote.Activate(c, user, repo, link); err != nil {
-		c.String(500, err.Error())
-		return
-	}
-
-	from, err := remote.Repo(c, user, repo.Owner, repo.Name)
+	from, err := remote.Repo(c, user, repo.RemoteID, repo.Owner, repo.Name)
 	if err != nil {
 		log.Error().Err(err).Msgf("get repo '%s/%s' from remote", repo.Owner, repo.Name)
 		c.AbortWithStatus(http.StatusInternalServerError)
 		return
 	}
-	repo.Name = from.Name
-	repo.Owner = from.Owner
-	repo.FullName = from.FullName
-	repo.Avatar = from.Avatar
-	repo.Link = from.Link
-	repo.Clone = from.Clone
-	repo.IsSCMPrivate = from.IsSCMPrivate
-	if repo.IsSCMPrivate != from.IsSCMPrivate {
-		repo.ResetVisibility()
+
+	if repo.FullName != from.FullName {
+		// create a redirection
+		err = _store.CreateRedirection(&model.Redirection{RepoID: repo.ID, FullName: repo.FullName})
+		if err != nil {
+			_ = c.AbortWithError(http.StatusInternalServerError, err)
+			return
+		}
 	}
+
+	repo.Update(from)
 	if err := _store.UpdateRepo(repo); err != nil {
 		_ = c.AbortWithError(http.StatusInternalServerError, err)
 		return
 	}
 
+	if err := remote.Deactivate(c, user, repo, host); err != nil {
+		log.Trace().Err(err).Msgf("deactivate repo '%s' to repair failed", repo.FullName)
+	}
+	if err := remote.Activate(c, user, repo, link); err != nil {
+		c.String(500, err.Error())
+		return
+	}
+
 	c.Writer.WriteHeader(http.StatusOK)
 }
 
@@ -302,7 +311,7 @@ func MoveRepo(c *gin.Context) {
 		return
 	}
 
-	from, err := remote.Repo(c, user, owner, name)
+	from, err := remote.Repo(c, user, "", owner, name)
 	if err != nil {
 		_ = c.AbortWithError(http.StatusInternalServerError, err)
 		return
@@ -312,17 +321,14 @@ func MoveRepo(c *gin.Context) {
 		return
 	}
 
-	repo.Name = from.Name
-	repo.Owner = from.Owner
-	repo.FullName = from.FullName
-	repo.Avatar = from.Avatar
-	repo.Link = from.Link
-	repo.Clone = from.Clone
-	repo.IsSCMPrivate = from.IsSCMPrivate
-	if repo.IsSCMPrivate != from.IsSCMPrivate {
-		repo.ResetVisibility()
+	err = _store.CreateRedirection(&model.Redirection{RepoID: repo.ID, FullName: repo.FullName})
+	if err != nil {
+		_ = c.AbortWithError(http.StatusInternalServerError, err)
+		return
 	}
 
+	repo.Update(from)
+
 	errStore := _store.UpdateRepo(repo)
 	if errStore != nil {
 		_ = c.AbortWithError(http.StatusInternalServerError, errStore)

server/model/perm.go

@@ -25,15 +25,15 @@ type PermStore interface {
 
 // Perm defines a repository permission for an individual user.
 type Perm struct {
-	UserID  int64  `json:"-"       xorm:"UNIQUE(s) INDEX NOT NULL 'perm_user_id'"`
-	RepoID  int64  `json:"-"       xorm:"UNIQUE(s) INDEX NOT NULL 'perm_repo_id'"`
-	Repo    string `json:"-"       xorm:"-"` // TODO: better caching (use type *Repo)
-	Pull    bool   `json:"pull"    xorm:"perm_pull"`
-	Push    bool   `json:"push"    xorm:"perm_push"`
-	Admin   bool   `json:"admin"   xorm:"perm_admin"`
-	Synced  int64  `json:"synced"  xorm:"perm_synced"`
-	Created int64  `json:"created" xorm:"created"`
-	Updated int64  `json:"updated" xorm:"updated"`
+	UserID  int64 `json:"-"       xorm:"UNIQUE(s) INDEX NOT NULL 'perm_user_id'"`
+	RepoID  int64 `json:"-"       xorm:"UNIQUE(s) INDEX NOT NULL 'perm_repo_id'"`
+	Repo    *Repo `json:"-"       xorm:"-"`
+	Pull    bool  `json:"pull"    xorm:"perm_pull"`
+	Push    bool  `json:"push"    xorm:"perm_push"`
+	Admin   bool  `json:"admin"   xorm:"perm_admin"`
+	Synced  int64 `json:"synced"  xorm:"perm_synced"`
+	Created int64 `json:"created" xorm:"created"`
+	Updated int64 `json:"updated" xorm:"updated"`
 }
 
 // TableName return database table name for xorm

server/model/redirection.go

@@ -0,0 +1,11 @@
+package model
+
+type Redirection struct {
+	ID       int64  `xorm:"pk autoincr 'redirection_id'"`
+	RepoID   int64  `xorm:"'repo_id'"`
+	FullName string `xorm:"UNIQUE INDEX 'repo_full_name'"`
+}
+
+func (r Redirection) TableName() string {
+	return "redirections"
+}

server/model/repo.go

@@ -26,6 +26,7 @@ import (
 type Repo struct {
 	ID                           int64          `json:"id,omitempty"                    xorm:"pk autoincr 'repo_id'"`
 	UserID                       int64          `json:"-"                               xorm:"repo_user_id"`
+	RemoteID                     RemoteID       `json:"-"                               xorm:"'remote_id'"`
 	Owner                        string         `json:"owner"                           xorm:"UNIQUE(name) 'repo_owner'"`
 	Name                         string         `json:"name"                            xorm:"UNIQUE(name) 'repo_name'"`
 	FullName                     string         `json:"full_name"                       xorm:"UNIQUE 'repo_full_name'"`
@@ -74,6 +75,10 @@ func ParseRepo(str string) (user, repo string, err error) {
 
 // Update updates the repository with values from the given Repo.
 func (r *Repo) Update(from *Repo) {
+	r.RemoteID = from.RemoteID
+	r.Owner = from.Owner
+	r.Name = from.Name
+	r.FullName = from.FullName
 	r.Avatar = from.Avatar
 	r.Link = from.Link
 	r.SCMKind = from.SCMKind
@@ -99,3 +104,9 @@ type RepoPatch struct {
 	AllowPull                    *bool           `json:"allow_pr,omitempty"`
 	CancelPreviousPipelineEvents *[]WebhookEvent `json:"cancel_previous_pipeline_events"`
 }
+
+type RemoteID string
+
+func (r RemoteID) IsValid() bool {
+	return r != "" && r != "0"
+}

server/remote/bitbucket/bitbucket.go

@@ -142,7 +142,10 @@ func (c *config) Teams(ctx context.Context, u *model.User) ([]*model.Team, error
 }
 
 // Repo returns the named Bitbucket repository.
-func (c *config) Repo(ctx context.Context, u *model.User, owner, name string) (*model.Repo, error) {
+func (c *config) Repo(ctx context.Context, u *model.User, id model.RemoteID, owner, name string) (*model.Repo, error) {
+	if id.IsValid() {
+		name = string(id)
+	}
 	repo, err := c.newClient(ctx, u).FindRepo(owner, name)
 	if err != nil {
 		return nil, err

server/remote/bitbucket/bitbucket_test.go

@@ -126,12 +126,12 @@ func Test_bitbucket(t *testing.T) {
 
 		g.Describe("When requesting a repository", func() {
 			g.It("Should return the details", func() {
-				repo, err := c.Repo(ctx, fakeUser, fakeRepo.Owner, fakeRepo.Name)
+				repo, err := c.Repo(ctx, fakeUser, "", fakeRepo.Owner, fakeRepo.Name)
 				g.Assert(err).IsNil()
 				g.Assert(repo.FullName).Equal(fakeRepo.FullName)
 			})
 			g.It("Should handle not found errors", func() {
-				_, err := c.Repo(ctx, fakeUser, fakeRepoNotFound.Owner, fakeRepoNotFound.Name)
+				_, err := c.Repo(ctx, fakeUser, "", fakeRepoNotFound.Owner, fakeRepoNotFound.Name)
 				g.Assert(err).IsNotNil()
 			})
 		})

server/remote/bitbucket/convert.go

@@ -49,6 +49,7 @@ func convertStatus(status model.StatusValue) string {
 // structure to the common Woodpecker repository structure.
 func convertRepo(from *internal.Repo) *model.Repo {
 	repo := model.Repo{
+		RemoteID:     model.RemoteID(from.UUID),
 		Clone:        cloneLink(from),
 		Owner:        strings.Split(from.FullName, "/")[0],
 		Name:         strings.Split(from.FullName, "/")[1],

server/remote/bitbucket/internal/types.go

@@ -89,6 +89,7 @@ type LinkClone struct {
 }
 
 type Repo struct {
+	UUID      string  `json:"uuid"`
 	Owner     Account `json:"owner"`
 	Name      string  `json:"name"`
 	FullName  string  `json:"full_name"`

server/remote/bitbucketserver/bitbucketserver.go

@@ -151,7 +151,7 @@ func (*Config) TeamPerm(u *model.User, org string) (*model.Perm, error) {
 	return nil, nil
 }
 
-func (c *Config) Repo(ctx context.Context, u *model.User, owner, name string) (*model.Repo, error) {
+func (c *Config) Repo(ctx context.Context, u *model.User, _ model.RemoteID, owner, name string) (*model.Repo, error) {
 	repo, err := internal.NewClientWithToken(ctx, c.URL, c.Consumer, u.Token).FindRepo(owner, name)
 	if err != nil {
 		return nil, err

server/remote/bitbucketserver/convert.go

@@ -51,6 +51,7 @@ func convertStatus(status model.StatusValue) string {
 // structure to the common Woodpecker repository structure.
 func convertRepo(from *internal.Repo) *model.Repo {
 	repo := model.Repo{
+		RemoteID:     model.RemoteID(fmt.Sprint(from.ID)),
 		Name:         from.Slug,
 		Owner:        from.Project.Key,
 		Branch:       "master",

server/remote/bitbucketserver/parse.go

@@ -32,6 +32,7 @@ func parseHook(r *http.Request, baseURL string) (*model.Repo, *model.Build, erro
 	}
 	build := convertPushHook(hook, baseURL)
 	repo := &model.Repo{
+		RemoteID: model.RemoteID(fmt.Sprint(hook.Repository.ID)),
 		Name:     hook.Repository.Slug,
 		Owner:    hook.Repository.Project.Key,
 		FullName: fmt.Sprintf("%s/%s", hook.Repository.Project.Key, hook.Repository.Slug),

server/remote/coding/coding.go

@@ -160,8 +160,8 @@ func (c *Coding) TeamPerm(u *model.User, org string) (*model.Perm, error) {
 	return nil, nil
 }
 
-// Repo fetches the named repository from the remote system.
-func (c *Coding) Repo(ctx context.Context, u *model.User, owner, name string) (*model.Repo, error) {
+// Repo fetches the repository from the remote system.
+func (c *Coding) Repo(ctx context.Context, u *model.User, _ model.RemoteID, owner, name string) (*model.Repo, error) {
 	client := c.newClient(ctx, u)
 	project, err := client.GetProject(owner, name)
 	if err != nil {
@@ -172,6 +172,7 @@ func (c *Coding) Repo(ctx context.Context, u *model.User, owner, name string) (*
 		return nil, err
 	}
 	return &model.Repo{
+		// TODO(1138) RemoteID:     project.ID,
 		Owner:        project.Owner,
 		Name:         project.Name,
 		FullName:     projectFullName(project.Owner, project.Name),
@@ -199,6 +200,7 @@ func (c *Coding) Repos(ctx context.Context, u *model.User) ([]*model.Repo, error
 			return nil, err
 		}
 		repo := &model.Repo{
+			// TODO(1138) RemoteID:     project.ID,
 			Owner:        project.Owner,
 			Name:         project.Name,
 			FullName:     projectFullName(project.Owner, project.Name),

server/remote/coding/coding_test.go

@@ -108,7 +108,7 @@ func Test_coding(t *testing.T) {
 
 		g.Describe("When requesting a repository", func() {
 			g.It("Should return the details", func() {
-				repo, err := c.Repo(ctx, fakeUser, fakeRepo.Owner, fakeRepo.Name)
+				repo, err := c.Repo(ctx, fakeUser, "", fakeRepo.Owner, fakeRepo.Name)
 				g.Assert(err).IsNil()
 				g.Assert(repo.FullName).Equal(fakeRepo.FullName)
 				g.Assert(repo.Avatar).Equal(s.URL + fakeRepo.Avatar)
@@ -119,7 +119,7 @@ func Test_coding(t *testing.T) {
 				g.Assert(repo.IsSCMPrivate).Equal(fakeRepo.IsSCMPrivate)
 			})
 			g.It("Should handle not found errors", func() {
-				_, err := c.Repo(ctx, fakeUser, fakeRepoNotFound.Owner, fakeRepoNotFound.Name)
+				_, err := c.Repo(ctx, fakeUser, "", fakeRepoNotFound.Owner, fakeRepoNotFound.Name)
 				g.Assert(err).IsNotNil()
 			})
 		})

server/remote/coding/hook.go

@@ -137,6 +137,7 @@ func convertRepository(repo *Repository) (*model.Repo, error) {
 	}
 
 	return &model.Repo{
+		// TODO RemoteID: repo.ID,
 		Owner:    matches[1],
 		Name:     repo.Name,
 		FullName: projectFullName(repo.Owner.GlobalKey, repo.Name),