From aed3a80287241a4d09e62469074b5e1ee37977b4 Mon Sep 17 00:00:00 2001 From: "Joonhyeok Ahn (Joon)" <92326584+bitethecode@users.noreply.github.com> Date: Mon, 31 Oct 2022 10:08:57 -0500 Subject: [PATCH] Migrate sql querys to xorm query builder (#1356) Co-authored-by: 6543 <6543@obermui.de> --- server/model/feed.go | 38 +++---- server/store/datastore/config.go | 29 ++---- server/store/datastore/feed.go | 145 ++++++++++----------------- server/store/datastore/feed_test.go | 92 +++++++++++++++++ server/store/datastore/permission.go | 9 ++ server/store/datastore/user.go | 18 +++- 6 files changed, 195 insertions(+), 136 deletions(-) diff --git a/server/model/feed.go b/server/model/feed.go index f510bf7df..7e3ede09c 100644 --- a/server/model/feed.go +++ b/server/model/feed.go @@ -19,24 +19,24 @@ package model // // swagger:model feed type Feed struct { - Owner string `json:"owner" xorm:"repo_owner"` - Name string `json:"name" xorm:"repo_name"` - FullName string `json:"full_name" xorm:"repo_full_name"` + Owner string `json:"owner" xorm:"feed_repo_owner"` + Name string `json:"name" xorm:"feed_repo_name"` + FullName string `json:"full_name" xorm:"feed_repo_full_name"` - Number int64 `json:"number,omitempty" xorm:"pipeline_number"` - Event string `json:"event,omitempty" xorm:"pipeline_event"` - Status string `json:"status,omitempty" xorm:"pipeline_status"` - Created int64 `json:"created_at,omitempty" xorm:"pipeline_created"` - Started int64 `json:"started_at,omitempty" xorm:"pipeline_started"` - Finished int64 `json:"finished_at,omitempty" xorm:"pipeline_finished"` - Commit string `json:"commit,omitempty" xorm:"pipeline_commit"` - Branch string `json:"branch,omitempty" xorm:"pipeline_branch"` - Ref string `json:"ref,omitempty" xorm:"pipeline_ref"` - Refspec string `json:"refspec,omitempty" xorm:"pipeline_refspec"` - Remote string `json:"remote,omitempty" xorm:"pipeline_remote"` - Title string `json:"title,omitempty" xorm:"pipeline_title"` - Message string `json:"message,omitempty" xorm:"pipeline_message"` - Author string `json:"author,omitempty" xorm:"pipeline_author"` - Avatar string `json:"author_avatar,omitempty" xorm:"pipeline_avatar"` - Email string `json:"author_email,omitempty" xorm:"pipeline_email"` + Number int64 `json:"number,omitempty" xorm:"feed_pipeline_number"` + Event string `json:"event,omitempty" xorm:"feed_pipeline_event"` + Status string `json:"status,omitempty" xorm:"feed_pipeline_status"` + Created int64 `json:"created_at,omitempty" xorm:"feed_pipeline_created"` + Started int64 `json:"started_at,omitempty" xorm:"feed_pipeline_started"` + Finished int64 `json:"finished_at,omitempty" xorm:"feed_pipeline_finished"` + Commit string `json:"commit,omitempty" xorm:"feed_pipeline_commit"` + Branch string `json:"branch,omitempty" xorm:"feed_pipeline_branch"` + Ref string `json:"ref,omitempty" xorm:"feed_pipeline_ref"` + Refspec string `json:"refspec,omitempty" xorm:"feed_pipeline_refspec"` + Remote string `json:"remote,omitempty" xorm:"feed_pipeline_remote"` + Title string `json:"title,omitempty" xorm:"feed_pipeline_title"` + Message string `json:"message,omitempty" xorm:"feed_pipeline_message"` + Author string `json:"author,omitempty" xorm:"feed_pipeline_author"` + Avatar string `json:"author_avatar,omitempty" xorm:"feed_pipeline_avatar"` + Email string `json:"author_email,omitempty" xorm:"feed_pipeline_email"` } diff --git a/server/store/datastore/config.go b/server/store/datastore/config.go index 08156b6da..8d81cfd87 100644 --- a/server/store/datastore/config.go +++ b/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(new(model.Pipeline)). - Join("INNER", "pipeline_config", "pipelines.pipeline_id = pipeline_config.pipeline_id" ). - Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID}). - And(builder.Eq{"pipeline_config.config_id": config.ID}). - And(builder.In("pipelines.pipeline_status", "blocked", "pending")). - 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 + return s.engine.Table("pipelines").Select("pipelines.pipeline_id"). + Join("INNER", "pipeline_config", "pipelines.pipeline_id = pipeline_config.pipeline_id"). + Where(builder.Eq{"pipelines.pipeline_repo_id": config.RepoID}. + And(builder.Eq{"pipeline_config.config_id": config.ID}). + And(builder.NotIn("pipelines.pipeline_status", model.StatusBlocked, model.StatusPending))). + Exist() } func (s storage) ConfigCreate(config *model.Config) error { diff --git a/server/store/datastore/feed.go b/server/store/datastore/feed.go index 5225a8c25..c99992a0d 100644 --- a/server/store/datastore/feed.go +++ b/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.SQL(` -SELECT - repo_owner -,repo_name -,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) + err := s.engine.Table("pipelines"). + Select(feedItemSelect). + Join("INNER", "repos", "pipelines.pipeline_repo_id = repos.repo_id"). + In("pipelines.pipeline_status", model.StatusPending, model.StatusRunning). + 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) - return feed, s.engine.SQL(` -SELECT - repo_owner -,repo_name -,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 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) + err := s.engine.Table("repos"). + Select(feedItemSelect). + Join("INNER", "perms", "repos.repo_id = perms.perm_repo_id"). + Join("INNER", "pipelines", "repos.repo_id = pipelines.pipeline_repo_id"). + Where(userPushOrAdminCondition(user.ID)). + Desc("pipelines.pipeline_id"). + Limit(perPage). + Find(&feed) + + return feed, err } 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(` -SELECT - repo_owner -,repo_name -,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 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). + + err := s.engine.Table("repos"). + Select(feedItemSelect). + Join("INNER", "perms", "repos.repo_id = perms.perm_repo_id"). + Join("LEFT", "pipelines", "pipelines.pipeline_id = "+`( + SELECT pipelines.pipeline_id FROM pipelines + WHERE pipelines.pipeline_repo_id = repos.repo_id + ORDER BY pipelines.pipeline_id DESC + LIMIT 1 + )`). + Where(userPushOrAdminCondition(user.ID)). + And(builder.Eq{"repos.repo_active": true}). + Asc("repos.repo_full_name"). Find(&feed) + + return feed, err } diff --git a/server/store/datastore/feed_test.go b/server/store/datastore/feed_test.go index 1669786cf..446e9e5a7 100644 --- a/server/store/datastore/feed_test.go +++ b/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() diff --git a/server/store/datastore/permission.go b/server/store/datastore/permission.go index 5b97fc327..8a20d8800 100644 --- a/server/store/datastore/permission.go +++ b/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})) +} diff --git a/server/store/datastore/user.go b/server/store/datastore/user.go index bfa57eb14..230dc443e 100644 --- a/server/store/datastore/user.go +++ b/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)) - // TODO: delete related content that need this user to work - return err + sess := s.engine.NewSession() + defer sess.Close() + 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() }