Update database in one transaction on syncing user repositorys (#513)

* userSyncer Sync commit changes in one transaction
* cover new cases with unit tests

server/model/perm.go

@@ -19,7 +19,6 @@ package model
 type PermStore interface {
 	PermFind(user *User, repo *Repo) (*Perm, error)
 	PermUpsert(perm *Perm) error
-	PermBatch(perms []*Perm) error
 	PermDelete(perm *Perm) error
 	PermFlush(user *User, before int64) error
 }

server/shared/userSyncer.go

@@ -16,6 +16,7 @@ package shared
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/woodpecker-ci/woodpecker/server/model"
@@ -70,24 +71,24 @@ func (s *Syncer) Sync(ctx context.Context, user *model.User) error {
 		return err
 	}
 
-	var remoteRepos []*model.Repo
+	remoteRepos := make([]*model.Repo, 0, len(repos))
-	var perms []*model.Perm
-
 	for _, repo := range repos {
 		if s.Match(repo) {
-			remoteRepos = append(remoteRepos, repo)
+			repo.Perm = &model.Perm{
-			perm := model.Perm{
 				UserID: user.ID,
+				RepoID: repo.ID,
 				Repo:   repo.FullName,
-				Pull:   true,
 				Synced: unix,
 			}
 			remotePerm, err := s.Remote.Perm(ctx, user, repo.Owner, repo.Name)
-			if err == nil && remotePerm != nil {
+			if err != nil {
-				perm.Push = remotePerm.Push
+				return fmt.Errorf("could not fetch permission of repo '%s': %v", repo.FullName, err)
-				perm.Admin = remotePerm.Admin
 			}
-			perms = append(perms, &perm)
+			repo.Perm.Pull = remotePerm.Pull
+			repo.Perm.Push = remotePerm.Push
+			repo.Perm.Admin = remotePerm.Admin
+
+			remoteRepos = append(remoteRepos, repo)
 		}
 	}
 
@@ -96,11 +97,6 @@ func (s *Syncer) Sync(ctx context.Context, user *model.User) error {
 		return err
 	}
 
-	err = s.Store.PermBatch(perms)
-	if err != nil {
-		return err
-	}
-
 	// this is here as a precaution. I want to make sure that if an api
 	// call to the version control system fails and (for some reason) returns
 	// an empty list, we don't wipe out the user repository permissions.

server/store/datastore/feed_test.go

@@ -55,10 +55,12 @@ func TestRepoListLatest(t *testing.T) {
 	assert.NoError(t, store.CreateRepo(repo2))
 	assert.NoError(t, store.CreateRepo(repo3))
 
-	assert.NoError(t, store.PermBatch([]*model.Perm{
+	for _, perm := range []*model.Perm{
 		{UserID: user.ID, Repo: repo1.FullName, Push: true, Admin: false},
 		{UserID: user.ID, Repo: repo2.FullName, Push: true, Admin: true},
-	}))
+	} {
+		assert.NoError(t, store.PermUpsert(perm))
+	}
 
 	build1 := &model.Build{
 		RepoID: repo1.ID,

server/store/datastore/permission.go

@@ -17,6 +17,8 @@ package datastore
 import (
 	"fmt"
 
+	"xorm.io/xorm"
+
 	"github.com/woodpecker-ci/woodpecker/server/model"
 )
 
@@ -29,19 +31,26 @@ func (s storage) PermFind(user *model.User, repo *model.Repo) (*model.Perm, erro
 }
 
 func (s storage) PermUpsert(perm *model.Perm) error {
-	if len(perm.Repo) == 0 && perm.RepoID == 0 {
-		return fmt.Errorf("could not determine repo for permission: %v", perm)
-	}
-
 	sess := s.engine.NewSession()
 	defer sess.Close()
 	if err := sess.Begin(); err != nil {
 		return err
 	}
 
-	// lookup repo based on name if possible (preserve old behaviour)
+	if err := s.permUpsert(sess, perm); err != nil {
-	// TODO: check if needed
+		return err
-	if len(perm.Repo) != 0 {
+	}
+
+	return sess.Commit()
+}
+
+func (s storage) permUpsert(sess *xorm.Session, perm *model.Perm) error {
+	if perm.RepoID == 0 && len(perm.Repo) == 0 {
+		return fmt.Errorf("could not determine repo for permission: %v", perm)
+	}
+
+	// lookup repo based on name if possible
+	if perm.RepoID == 0 && len(perm.Repo) != 0 {
 		if r, err := s.getRepoName(sess, perm.Repo); err != nil {
 			return err
 		} else {
@@ -62,20 +71,7 @@ func (s storage) PermUpsert(perm *model.Perm) error {
 		// only Insert set auto created ID back to object
 		_, err = sess.Insert(perm)
 	}
-	if err != nil {
+	return err
-		return err
-	}
-
-	return sess.Commit()
-}
-
-func (s storage) PermBatch(perms []*model.Perm) error {
-	for i := range perms {
-		if err := s.PermUpsert(perms[i]); err != nil {
-			return err
-		}
-	}
-	return nil
 }
 
 func (s storage) PermDelete(perm *model.Perm) error {

server/store/datastore/repo.go

@@ -74,6 +74,8 @@ func (s storage) RepoList(user *model.User, owned bool) ([]*model.Repo, error) {
 		Find(&repos)
 }
 
+// RepoBatch Sync batch of repos from SCM (with permissions) to store (create if not exist else update)
+// TODO: only store activated repos ...
 func (s storage) RepoBatch(repos []*model.Repo) error {
 	sess := s.engine.NewSession()
 	defer sess.Close()
@@ -86,12 +88,14 @@ func (s storage) RepoBatch(repos []*model.Repo) error {
 			log.Debug().Msgf("skip insert/update repo: %#v", repos[i])
 			continue
 		}
+
 		exist, err := sess.
 			Where("repo_owner = ? AND repo_name = ?", repos[i].Owner, repos[i].Name).
 			Exist(new(model.Repo))
 		if err != nil {
 			return err
 		}
+
 		if exist {
 			if _, err := sess.
 				Where("repo_owner = ? AND repo_name = ?", repos[i].Owner, repos[i].Name).
@@ -112,6 +116,14 @@ func (s storage) RepoBatch(repos []*model.Repo) error {
 				return err
 			}
 		}
+
+		if repos[i].Perm != nil {
+			repos[i].Perm.RepoID = repos[i].ID
+			repos[i].Perm.Repo = repos[i].FullName
+			if err := s.permUpsert(sess, repos[i].Perm); err != nil {
+				return err
+			}
+		}
 	}
 
 	return sess.Commit()

server/store/datastore/repo_test.go

@@ -16,6 +16,7 @@ package datastore
 
 import (
 	"testing"
+	"time"
 
 	"github.com/franela/goblin"
 	"github.com/stretchr/testify/assert"
@@ -290,6 +291,13 @@ func TestRepoBatch(t *testing.T) {
 			Owner:    "foo",
 			Name:     "bar",
 			IsActive: true,
+			Perm: &model.Perm{
+				UserID: 1,
+				Pull:   true,
+				Push:   true,
+				Admin:  true,
+				Synced: time.Now().Unix(),
+			},
 		},
 		{
 			UserID:   1,
@@ -317,16 +325,33 @@ func TestRepoBatch(t *testing.T) {
 		return
 	}
 
+	// check DB state
+	perm, err := store.PermFind(&model.User{ID: 1}, repos[0])
+	assert.NoError(t, err)
+	assert.True(t, perm.Admin)
+
 	repo := &model.Repo{
 		FullName: "foo/bar",
 		Owner:    "foo",
 		Name:     "bar",
+		Perm: &model.Perm{
+			UserID: 1,
+			Pull:   true,
+			Push:   true,
+			Admin:  false,
+			Synced: time.Now().Unix(),
+		},
 	}
 	assert.NoError(t, store.RepoBatch([]*model.Repo{repo}))
 	assert.EqualValues(t, repos[0].ID, repo.ID)
-	_, err := store.engine.ID(repo.ID).Get(repo)
+
+	// check current DB state
+	_, err = store.engine.ID(repo.ID).Get(repo)
 	assert.NoError(t, err)
 	assert.True(t, repo.IsActive)
+	perm, err = store.PermFind(&model.User{ID: 1}, repos[0])
+	assert.NoError(t, err)
+	assert.False(t, perm.Admin)
 
 	allRepos := make([]*model.Repo, 0, 4)
 	assert.NoError(t, store.engine.Find(&allRepos))

server/store/datastore/users_test.go

@@ -213,10 +213,12 @@ func TestUsers(t *testing.T) {
 			g.Assert(store.CreateRepo(repo2)).IsNil()
 			g.Assert(store.CreateRepo(repo3)).IsNil()
 
-			g.Assert(store.PermBatch([]*model.Perm{
+			for _, perm := range []*model.Perm{
 				{UserID: user.ID, Repo: repo1.FullName, Push: true, Admin: false},
 				{UserID: user.ID, Repo: repo2.FullName, Push: false, Admin: true},
-			})).IsNil()
+			} {
+				g.Assert(store.PermUpsert(perm)).IsNil()
+			}
 
 			build1 := &model.Build{
 				RepoID: repo1.ID,

server/store/store.go

@@ -106,12 +106,11 @@ type Store interface {
 	// RepoList TODO: paginate
 	RepoList(user *model.User, owned bool) ([]*model.Repo, error)
 	RepoListLatest(*model.User) ([]*model.Feed, error)
-	// RepoBatch TODO: only store activated repos ...
+	// RepoBatch Sync batch of repos from SCM (with permissions) to store (create if not exist else update)
 	RepoBatch([]*model.Repo) error
 
 	PermFind(user *model.User, repo *model.Repo) (*model.Perm, error)
 	PermUpsert(perm *model.Perm) error
-	PermBatch(perms []*model.Perm) error
 	PermDelete(perm *model.Perm) error
 	PermFlush(user *model.User, before int64) error