Migrate sql querys to xorm query builder (#1356)

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

server/model/feed.go

@@ -19,24 +19,24 @@ package model
 //
 // swagger:model feed
 type Feed struct {
-	Owner    string `json:"owner"                   xorm:"repo_owner"`
+	Owner    string `json:"owner"                   xorm:"feed_repo_owner"`
-	Name     string `json:"name"                    xorm:"repo_name"`
+	Name     string `json:"name"                    xorm:"feed_repo_name"`
-	FullName string `json:"full_name"               xorm:"repo_full_name"`
+	FullName string `json:"full_name"               xorm:"feed_repo_full_name"`
 
-	Number   int64  `json:"number,omitempty"        xorm:"pipeline_number"`
+	Number   int64  `json:"number,omitempty"        xorm:"feed_pipeline_number"`
-	Event    string `json:"event,omitempty"         xorm:"pipeline_event"`
+	Event    string `json:"event,omitempty"         xorm:"feed_pipeline_event"`
-	Status   string `json:"status,omitempty"        xorm:"pipeline_status"`
+	Status   string `json:"status,omitempty"        xorm:"feed_pipeline_status"`
-	Created  int64  `json:"created_at,omitempty"    xorm:"pipeline_created"`
+	Created  int64  `json:"created_at,omitempty"    xorm:"feed_pipeline_created"`
-	Started  int64  `json:"started_at,omitempty"    xorm:"pipeline_started"`
+	Started  int64  `json:"started_at,omitempty"    xorm:"feed_pipeline_started"`
-	Finished int64  `json:"finished_at,omitempty"   xorm:"pipeline_finished"`
+	Finished int64  `json:"finished_at,omitempty"   xorm:"feed_pipeline_finished"`
-	Commit   string `json:"commit,omitempty"        xorm:"pipeline_commit"`
+	Commit   string `json:"commit,omitempty"        xorm:"feed_pipeline_commit"`
-	Branch   string `json:"branch,omitempty"        xorm:"pipeline_branch"`
+	Branch   string `json:"branch,omitempty"        xorm:"feed_pipeline_branch"`
-	Ref      string `json:"ref,omitempty"           xorm:"pipeline_ref"`
+	Ref      string `json:"ref,omitempty"           xorm:"feed_pipeline_ref"`
-	Refspec  string `json:"refspec,omitempty"       xorm:"pipeline_refspec"`
+	Refspec  string `json:"refspec,omitempty"       xorm:"feed_pipeline_refspec"`
-	Remote   string `json:"remote,omitempty"        xorm:"pipeline_remote"`
+	Remote   string `json:"remote,omitempty"        xorm:"feed_pipeline_remote"`
-	Title    string `json:"title,omitempty"         xorm:"pipeline_title"`
+	Title    string `json:"title,omitempty"         xorm:"feed_pipeline_title"`
-	Message  string `json:"message,omitempty"       xorm:"pipeline_message"`
+	Message  string `json:"message,omitempty"       xorm:"feed_pipeline_message"`
-	Author   string `json:"author,omitempty"        xorm:"pipeline_author"`
+	Author   string `json:"author,omitempty"        xorm:"feed_pipeline_author"`
-	Avatar   string `json:"author_avatar,omitempty" xorm:"pipeline_avatar"`
+	Avatar   string `json:"author_avatar,omitempty" xorm:"feed_pipeline_avatar"`
-	Email    string `json:"author_email,omitempty"  xorm:"pipeline_email"`
+	Email    string `json:"author_email,omitempty"  xorm:"feed_pipeline_email"`
 }

server/store/datastore/config.go

@@ -15,6 +15,8 @@
 package datastore
 
 import (
+	"xorm.io/builder"
+
 	"github.com/woodpecker-ci/woodpecker/server/model"
 )
 
@@ -39,27 +41,12 @@ func (s storage) ConfigFindIdentical(repoID int64, hash string) (*model.Config,
 }
 
 func (s storage) ConfigFindApproved(config *model.Config) (bool, error) {
-	/* TODO: use builder (do not behave same as pure sql, fix that)
+	return s.engine.Table("pipelines").Select("pipelines.pipeline_id").
-	return s.engine.Table(new(model.Pipeline)).
+		Join("INNER", "pipeline_config", "pipelines.pipeline_id = pipeline_config.pipeline_id").
-		Join("INNER", "pipeline_config", "pipelines.pipeline_id = pipeline_config.pipeline_id" ).
+		Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID}.
-		Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID}).
+			And(builder.Eq{"pipeline_config.config_id": config.ID}).
-		And(builder.Eq{"pipeline_config.config_id": config.ID}).
+			And(builder.NotIn("pipelines.pipeline_status", model.StatusBlocked, model.StatusPending))).
-		And(builder.In("pipelines.pipeline_status", "blocked", "pending")).
+		Exist()
-		Exist(new(model.Pipeline))
-	*/
-
-	c, err := s.engine.SQL(`
-SELECT pipeline_id FROM pipelines
-WHERE pipeline_repo_id = ?
-AND pipeline_id in (
-SELECT pipeline_id
-FROM pipeline_config
-WHERE pipeline_config.config_id = ?
-)
-AND pipeline_status NOT IN ('blocked', 'pending')
-LIMIT 1
-`, config.RepoID, config.ID).Count()
-	return c > 0, err
 }
 
 func (s storage) ConfigCreate(config *model.Config) error {

server/store/datastore/feed.go

@@ -15,112 +15,71 @@
 package datastore
 
 import (
+	"xorm.io/builder"
+
 	"github.com/woodpecker-ci/woodpecker/server/model"
 )
 
-// TODO: need tests before converting to builder statement
+var feedItemSelect = `repos.repo_owner as feed_repo_owner,
+repos.repo_name as feed_repo_name,
+repos.repo_full_name as feed_repo_full_name,
+pipelines.pipeline_number as feed_pipeline_number,
+pipelines.pipeline_event as feed_pipeline_event,
+pipelines.pipeline_status as feed_pipeline_status,
+pipelines.pipeline_created as feed_pipeline_created,
+pipelines.pipeline_started as feed_pipeline_started,
+pipelines.pipeline_finished as feed_pipeline_finished,
+pipelines.pipeline_commit as feed_pipeline_commit,
+pipelines.pipeline_branch as feed_pipeline_branch,
+pipelines.pipeline_ref as feed_pipeline_ref,
+pipelines.pipeline_refspec as feed_pipeline_refspec,
+pipelines.pipeline_remote as feed_pipeline_remote,
+pipelines.pipeline_title as feed_pipeline_title,
+pipelines.pipeline_message as feed_pipeline_message,
+pipelines.pipeline_author as feed_pipeline_author,
+pipelines.pipeline_email as feed_pipeline_email,
+pipelines.pipeline_avatar as feed_pipeline_avatar`
+
 func (s storage) GetPipelineQueue() ([]*model.Feed, error) {
 	feed := make([]*model.Feed, 0, perPage)
-	// TODO: use builder (do not behave same as pure sql, fix that)
+	err := s.engine.Table("pipelines").
-	err := s.engine.SQL(`
+		Select(feedItemSelect).
-SELECT
+		Join("INNER", "repos", "pipelines.pipeline_repo_id = repos.repo_id").
- repo_owner
+		In("pipelines.pipeline_status", model.StatusPending, model.StatusRunning).
-,repo_name
+		Find(&feed)
-,repo_full_name
-,pipeline_number
-,pipeline_event
-,pipeline_status
-,pipeline_created
-,pipeline_started
-,pipeline_finished
-,pipeline_commit
-,pipeline_branch
-,pipeline_ref
-,pipeline_refspec
-,pipeline_remote
-,pipeline_title
-,pipeline_message
-,pipeline_author
-,pipeline_email
-,pipeline_avatar
-FROM
- pipelines p
-,repos r
-WHERE p.pipeline_repo_id = r.repo_id
-  AND p.pipeline_status IN ('pending','running')
-`).Find(&feed)
 	return feed, err
 }
 
 func (s storage) UserFeed(user *model.User) ([]*model.Feed, error) {
 	feed := make([]*model.Feed, 0, perPage)
-	// TODO: use builder (do not behave same as pure sql, fix that)
+	err := s.engine.Table("repos").
-	return feed, s.engine.SQL(`
+		Select(feedItemSelect).
-SELECT
+		Join("INNER", "perms", "repos.repo_id = perms.perm_repo_id").
- repo_owner
+		Join("INNER", "pipelines", "repos.repo_id = pipelines.pipeline_repo_id").
-,repo_name
+		Where(userPushOrAdminCondition(user.ID)).
-,repo_full_name
+		Desc("pipelines.pipeline_id").
-,pipeline_number
+		Limit(perPage).
-,pipeline_event
+		Find(&feed)
-,pipeline_status
+
-,pipeline_created
+	return feed, err
-,pipeline_started
-,pipeline_finished
-,pipeline_commit
-,pipeline_branch
-,pipeline_ref
-,pipeline_refspec
-,pipeline_remote
-,pipeline_title
-,pipeline_message
-,pipeline_author
-,pipeline_email
-,pipeline_avatar
-FROM repos
-INNER JOIN perms  ON perms.perm_repo_id   = repos.repo_id
-INNER JOIN pipelines ON pipelines.pipeline_repo_id = repos.repo_id
-WHERE perms.perm_user_id = ?
-  AND (perms.perm_push = ? OR perms.perm_admin = ?)
-ORDER BY pipeline_id DESC
-LIMIT 50
-`, user.ID, true, true).Find(&feed)
 }
 
 func (s storage) RepoListLatest(user *model.User) ([]*model.Feed, error) {
 	feed := make([]*model.Feed, 0, perPage)
-	// TODO: use builder (do not behave same as pure sql, fix that)
+
-	return feed, s.engine.SQL(`
+	err := s.engine.Table("repos").
-SELECT
+		Select(feedItemSelect).
- repo_owner
+		Join("INNER", "perms", "repos.repo_id = perms.perm_repo_id").
-,repo_name
+		Join("LEFT", "pipelines", "pipelines.pipeline_id = "+`(
-,repo_full_name
+			SELECT pipelines.pipeline_id FROM pipelines
-,pipeline_number
+			WHERE pipelines.pipeline_repo_id = repos.repo_id
-,pipeline_event
+			ORDER BY pipelines.pipeline_id DESC
-,pipeline_status
+			LIMIT 1
-,pipeline_created
+			)`).
-,pipeline_started
+		Where(userPushOrAdminCondition(user.ID)).
-,pipeline_finished
+		And(builder.Eq{"repos.repo_active": true}).
-,pipeline_commit
+		Asc("repos.repo_full_name").
-,pipeline_branch
-,pipeline_ref
-,pipeline_refspec
-,pipeline_remote
-,pipeline_title
-,pipeline_message
-,pipeline_author
-,pipeline_email
-,pipeline_avatar
-FROM repos LEFT OUTER JOIN pipelines ON pipeline_id = (
-	SELECT pipeline_id FROM pipelines
-	WHERE pipelines.pipeline_repo_id = repos.repo_id
-	ORDER BY pipeline_id DESC
-	LIMIT 1
-)
-INNER JOIN perms ON perms.perm_repo_id = repos.repo_id
-WHERE perms.perm_user_id = ?
-  AND (perms.perm_push = ? OR perms.perm_admin = ?)
-  AND repos.repo_active = ?
-ORDER BY repo_full_name ASC;
-`, user.ID, true, true, true).
 		Find(&feed)
+
+	return feed, err
 }

server/store/datastore/feed_test.go

@@ -22,6 +22,98 @@ import (
 	"github.com/woodpecker-ci/woodpecker/server/model"
 )
 
+func TestGetPipelineQueue(t *testing.T) {
+	store, closer := newTestStore(t, new(model.Repo), new(model.User), new(model.Perm), new(model.Pipeline))
+	defer closer()
+
+	user := &model.User{
+		Login: "joe",
+		Email: "foo@bar.com",
+		Token: "e42080dddf012c718e476da161d21ad5",
+	}
+	assert.NoError(t, store.CreateUser(user))
+
+	repo1 := &model.Repo{
+		Owner:    "bradrydzewski",
+		Name:     "test",
+		FullName: "bradrydzewski/test",
+		RemoteID: "1",
+		IsActive: true,
+	}
+
+	assert.NoError(t, store.CreateRepo(repo1))
+	for _, perm := range []*model.Perm{
+		{UserID: user.ID, Repo: repo1, Push: true, Admin: false},
+	} {
+		assert.NoError(t, store.PermUpsert(perm))
+	}
+	pipeline1 := &model.Pipeline{
+		RepoID: repo1.ID,
+		Status: model.StatusPending,
+	}
+	assert.NoError(t, store.CreatePipeline(pipeline1))
+
+	feed, err := store.GetPipelineQueue()
+	if err != nil {
+		t.Errorf("Unexpected error: repository list with latest pipeline: %s", err)
+		return
+	}
+	if got, want := len(feed), 1; got != want {
+		t.Errorf("Want %d repositories, got %d", want, got)
+	}
+}
+
+func TestUserFeed(t *testing.T) {
+	store, closer := newTestStore(t, new(model.Repo), new(model.User), new(model.Perm), new(model.Pipeline))
+	defer closer()
+
+	user := &model.User{
+		Login: "joe",
+		Email: "foo@bar.com",
+		Token: "e42080dddf012c718e476da161d21ad5",
+	}
+	assert.NoError(t, store.CreateUser(user))
+
+	repo1 := &model.Repo{
+		Owner:    "bradrydzewski",
+		Name:     "test1",
+		FullName: "bradrydzewski/test1",
+		RemoteID: "1",
+		IsActive: true,
+	}
+	repo2 := &model.Repo{
+		Owner:    "johndoe",
+		Name:     "test",
+		FullName: "johndoe/test2",
+		RemoteID: "2",
+		IsActive: true,
+	}
+
+	assert.NoError(t, store.CreateRepo(repo1))
+	assert.NoError(t, store.CreateRepo(repo2))
+
+	for _, perm := range []*model.Perm{
+		{UserID: user.ID, Repo: repo1, Push: true, Admin: false},
+	} {
+		assert.NoError(t, store.PermUpsert(perm))
+	}
+
+	pipeline1 := &model.Pipeline{
+		RepoID: repo1.ID,
+		Status: model.StatusFailure,
+	}
+
+	assert.NoError(t, store.CreatePipeline(pipeline1))
+	feed, err := store.UserFeed(user)
+	if err != nil {
+		t.Errorf("Unexpected error: repository list with latest pipeline: %s", err)
+		return
+	}
+	if got, want := len(feed), 1; got != want {
+		t.Errorf("Want %d repositories, got %d", want, got)
+	}
+}
+
 func TestRepoListLatest(t *testing.T) {
 	store, closer := newTestStore(t, new(model.Repo), new(model.User), new(model.Perm), new(model.Pipeline))
 	defer closer()

server/store/datastore/permission.go

@@ -17,6 +17,7 @@ package datastore
 import (
 	"fmt"
 
+	"xorm.io/builder"
 	"xorm.io/xorm"
 
 	"github.com/woodpecker-ci/woodpecker/server/model"
@@ -87,3 +88,11 @@ func (s storage) PermFlush(user *model.User, before int64) error {
 		Delete(new(model.Perm))
 	return err
 }
+
+// userPushOrAdminCondition return condition where user must have push or admin rights
+// if used make sure to have permission table ("perms") joined
+func userPushOrAdminCondition(userID int64) builder.Cond {
+	return builder.Eq{"perms.perm_user_id": userID}.
+		And(builder.Eq{"perms.perm_push": true}.
+			Or(builder.Eq{"perms.perm_admin": true}))
+}

server/store/datastore/user.go

@@ -49,7 +49,19 @@ func (s storage) UpdateUser(user *model.User) error {
 }
 
 func (s storage) DeleteUser(user *model.User) error {
-	_, err := s.engine.ID(user.ID).Delete(new(model.User))
+	sess := s.engine.NewSession()
-	// TODO: delete related content that need this user to work
+	defer sess.Close()
-	return err
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+
+	if _, err := sess.ID(user.ID).Delete(new(model.User)); err != nil {
+		return err
+	}
+
+	if _, err := sess.Where("perm_user_id = ?", user.ID).Delete(new(model.Perm)); err != nil {
+		return err
+	}
+
+	return sess.Commit()
 }