From cf02cd7ba0c94165743660cf30f0cbb5a73a385e Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Sat, 20 May 2017 04:48:22 -0400 Subject: [PATCH] Fix and test for delete user (#1713) * Fix and test for delete user * Run updates in batches * Unit test --- integrations/delete_user_test.go | 41 +++++++++++++++++ .../{consistency_test.go => consistency.go} | 26 +++++------ models/fixtures/follow.yml | 10 +++++ models/fixtures/user.yml | 9 ++-- models/main_test.go | 26 +++++++++++ models/{setup_for_test.go => unit_tests.go} | 23 +--------- models/user.go | 44 ++++++++++--------- models/user_test.go | 33 +++++++++++++- vendor/github.com/go-xorm/builder/cond_in.go | 6 +-- .../github.com/go-xorm/builder/cond_notin.go | 6 +-- vendor/vendor.json | 6 +-- 11 files changed, 161 insertions(+), 69 deletions(-) create mode 100644 integrations/delete_user_test.go rename models/{consistency_test.go => consistency.go} (89%) create mode 100644 models/main_test.go rename models/{setup_for_test.go => unit_tests.go} (84%) diff --git a/integrations/delete_user_test.go b/integrations/delete_user_test.go new file mode 100644 index 0000000000..bf38eeae2b --- /dev/null +++ b/integrations/delete_user_test.go @@ -0,0 +1,41 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "bytes" + "net/http" + "net/url" + "testing" + + "code.gitea.io/gitea/models" + + "github.com/stretchr/testify/assert" +) + +func TestDeleteUser(t *testing.T) { + prepareTestEnv(t) + + session := loginUser(t, "user1", "password") + + req, err := http.NewRequest("GET", "/admin/users/8", nil) + assert.NoError(t, err) + resp := session.MakeRequest(t, req) + assert.EqualValues(t, http.StatusOK, resp.HeaderCode) + + doc, err := NewHtmlParser(resp.Body) + assert.NoError(t, err) + req, err = http.NewRequest("POST", "/admin/users/8/delete", + bytes.NewBufferString(url.Values{ + "_csrf": []string{doc.GetInputValueByName("_csrf")}, + }.Encode())) + assert.NoError(t, err) + req.Header.Add("Content-Type", "application/x-www-form-urlencoded") + resp = session.MakeRequest(t, req) + assert.EqualValues(t, http.StatusOK, resp.HeaderCode) + + models.AssertNotExistsBean(t, &models.User{ID: 8}) + models.CheckConsistencyFor(t, &models.User{}) +} diff --git a/models/consistency_test.go b/models/consistency.go similarity index 89% rename from models/consistency_test.go rename to models/consistency.go index a8de8e3c2b..77308e538e 100644 --- a/models/consistency_test.go +++ b/models/consistency.go @@ -12,9 +12,9 @@ import ( "github.com/stretchr/testify/assert" ) -// ConsistencyCheckable a type that can be tested for database consistency -type ConsistencyCheckable interface { - CheckForConsistency(t *testing.T) +// consistencyCheckable a type that can be tested for database consistency +type consistencyCheckable interface { + checkForConsistency(t *testing.T) } // CheckConsistencyForAll test that the entire database is consistent @@ -44,12 +44,12 @@ func CheckConsistencyFor(t *testing.T, beansToCheck ...interface{}) { for i := 0; i < sliceValue.Len(); i++ { entity := sliceValue.Index(i).Interface() - checkable, ok := entity.(ConsistencyCheckable) + checkable, ok := entity.(consistencyCheckable) if !ok { t.Errorf("Expected %+v (of type %T) to be checkable for consistency", entity, entity) } else { - checkable.CheckForConsistency(t) + checkable.checkForConsistency(t) } } } @@ -68,7 +68,7 @@ func assertCount(t *testing.T, bean interface{}, expected int) { "Failed consistency test, the counted bean (of type %T) was %+v", bean, bean) } -func (user *User) CheckForConsistency(t *testing.T) { +func (user *User) checkForConsistency(t *testing.T) { assertCount(t, &Repository{OwnerID: user.ID}, user.NumRepos) assertCount(t, &Star{UID: user.ID}, user.NumStars) assertCount(t, &OrgUser{OrgID: user.ID}, user.NumMembers) @@ -81,7 +81,7 @@ func (user *User) CheckForConsistency(t *testing.T) { } } -func (repo *Repository) CheckForConsistency(t *testing.T) { +func (repo *Repository) checkForConsistency(t *testing.T) { assert.Equal(t, repo.LowerName, strings.ToLower(repo.Name), "repo: %+v", repo) assertCount(t, &Star{RepoID: repo.ID}, repo.NumStars) assertCount(t, &Watch{RepoID: repo.ID}, repo.NumWatches) @@ -112,7 +112,7 @@ func (repo *Repository) CheckForConsistency(t *testing.T) { "Unexpected number of closed milestones for repo %+v", repo) } -func (issue *Issue) CheckForConsistency(t *testing.T) { +func (issue *Issue) checkForConsistency(t *testing.T) { actual := getCount(t, x.Where("type=?", CommentTypeComment), &Comment{IssueID: issue.ID}) assert.EqualValues(t, issue.NumComments, actual, "Unexpected number of comments for issue %+v", issue) @@ -122,13 +122,13 @@ func (issue *Issue) CheckForConsistency(t *testing.T) { } } -func (pr *PullRequest) CheckForConsistency(t *testing.T) { +func (pr *PullRequest) checkForConsistency(t *testing.T) { issue := AssertExistsAndLoadBean(t, &Issue{ID: pr.IssueID}).(*Issue) assert.True(t, issue.IsPull) assert.EqualValues(t, issue.Index, pr.Index) } -func (milestone *Milestone) CheckForConsistency(t *testing.T) { +func (milestone *Milestone) checkForConsistency(t *testing.T) { assertCount(t, &Issue{MilestoneID: milestone.ID}, milestone.NumIssues) actual := getCount(t, x.Where("is_closed=?", true), &Issue{MilestoneID: milestone.ID}) @@ -136,7 +136,7 @@ func (milestone *Milestone) CheckForConsistency(t *testing.T) { "Unexpected number of closed issues for milestone %+v", milestone) } -func (label *Label) CheckForConsistency(t *testing.T) { +func (label *Label) checkForConsistency(t *testing.T) { issueLabels := make([]*IssueLabel, 0, 10) assert.NoError(t, x.Find(&issueLabels, &IssueLabel{LabelID: label.ID})) assert.EqualValues(t, label.NumIssues, len(issueLabels), @@ -155,12 +155,12 @@ func (label *Label) CheckForConsistency(t *testing.T) { "Unexpected number of closed issues for label %+v", label) } -func (team *Team) CheckForConsistency(t *testing.T) { +func (team *Team) checkForConsistency(t *testing.T) { assertCount(t, &TeamUser{TeamID: team.ID}, team.NumMembers) assertCount(t, &TeamRepo{TeamID: team.ID}, team.NumRepos) } -func (action *Action) CheckForConsistency(t *testing.T) { +func (action *Action) checkForConsistency(t *testing.T) { repo := AssertExistsAndLoadBean(t, &Repository{ID: action.RepoID}).(*Repository) owner := AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User) actor := AssertExistsAndLoadBean(t, &User{ID: action.ActUserID}).(*User) diff --git a/models/fixtures/follow.yml b/models/fixtures/follow.yml index 53db1e88b1..480fa065c7 100644 --- a/models/fixtures/follow.yml +++ b/models/fixtures/follow.yml @@ -2,3 +2,13 @@ id: 1 user_id: 4 follow_id: 2 + +- + id: 2 + user_id: 8 + follow_id: 2 + +- + id: 3 + user_id: 2 + follow_id: 8 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index dd8554c58b..1777cff77e 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -4,9 +4,9 @@ name: user1 full_name: User One email: user1@example.com - passwd: password + passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password type: 0 # individual - salt: salt + salt: ZogKvWdyEx is_admin: true avatar: avatar1 avatar_email: user1@example.com @@ -26,7 +26,8 @@ avatar_email: user2@example.com num_repos: 2 num_stars: 2 - num_followers: 1 + num_followers: 2 + num_following: 1 is_active: true - @@ -123,6 +124,8 @@ avatar_email: user8@example.com num_repos: 0 is_active: true + num_followers: 1 + num_following: 1 - id: 9 diff --git a/models/main_test.go b/models/main_test.go new file mode 100644 index 0000000000..304bbccaa9 --- /dev/null +++ b/models/main_test.go @@ -0,0 +1,26 @@ +package models + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "code.gitea.io/gitea/modules/setting" +) + +func TestMain(m *testing.M) { + if err := CreateTestEngine(); err != nil { + fmt.Printf("Error creating test engine: %v\n", err) + os.Exit(1) + } + + setting.AppURL = "https://try.gitea.io/" + setting.RunUser = "runuser" + setting.SSH.Port = 3000 + setting.SSH.Domain = "try.gitea.io" + setting.RepoRootPath = filepath.Join(os.TempDir(), "repos") + setting.AppDataPath = filepath.Join(os.TempDir(), "appdata") + + os.Exit(m.Run()) +} diff --git a/models/setup_for_test.go b/models/unit_tests.go similarity index 84% rename from models/setup_for_test.go rename to models/unit_tests.go index 9fb83f15c4..44b2e2c637 100644 --- a/models/setup_for_test.go +++ b/models/unit_tests.go @@ -5,13 +5,8 @@ package models import ( - "fmt" - "os" - "path/filepath" "testing" - "code.gitea.io/gitea/modules/setting" - "github.com/go-xorm/core" "github.com/go-xorm/xorm" _ "github.com/mattn/go-sqlite3" // for the test engine @@ -19,24 +14,9 @@ import ( "gopkg.in/testfixtures.v2" ) +// NonexistentID an ID that will never exist const NonexistentID = 9223372036854775807 -func TestMain(m *testing.M) { - if err := CreateTestEngine(); err != nil { - fmt.Printf("Error creating test engine: %v\n", err) - os.Exit(1) - } - - setting.AppURL = "https://try.gitea.io/" - setting.RunUser = "runuser" - setting.SSH.Port = 3000 - setting.SSH.Domain = "try.gitea.io" - setting.RepoRootPath = filepath.Join(os.TempDir(), "repos") - setting.AppDataPath = filepath.Join(os.TempDir(), "appdata") - - os.Exit(m.Run()) -} - // CreateTestEngine create an xorm engine for testing func CreateTestEngine() error { var err error @@ -52,6 +32,7 @@ func CreateTestEngine() error { return InitFixtures(&testfixtures.SQLite{}, "fixtures/") } +// TestFixturesAreConsistent assert that test fixtures are consistent func TestFixturesAreConsistent(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) CheckConsistencyForAll(t) diff --git a/models/user.go b/models/user.go index 75329d8ff0..5fe72f923a 100644 --- a/models/user.go +++ b/models/user.go @@ -924,38 +924,41 @@ func deleteUser(e *xorm.Session, u *User) error { } // ***** START: Watch ***** - watches := make([]*Watch, 0, 10) - if err = e.Find(&watches, &Watch{UserID: u.ID}); err != nil { + watchedRepoIDs := make([]int64, 0, 10) + if err = e.Table("watch").Cols("watch.repo_id"). + Where("watch.user_id = ?", u.ID).Find(&watchedRepoIDs); err != nil { return fmt.Errorf("get all watches: %v", err) } - for i := range watches { - if _, err = e.Exec("UPDATE `repository` SET num_watches=num_watches-1 WHERE id=?", watches[i].RepoID); err != nil { - return fmt.Errorf("decrease repository watch number[%d]: %v", watches[i].RepoID, err) - } + if _, err = e.Decr("num_watches").In("id", watchedRepoIDs).Update(new(Repository)); err != nil { + return fmt.Errorf("decrease repository num_watches: %v", err) } // ***** END: Watch ***** // ***** START: Star ***** - stars := make([]*Star, 0, 10) - if err = e.Find(&stars, &Star{UID: u.ID}); err != nil { + starredRepoIDs := make([]int64, 0, 10) + if err = e.Table("star").Cols("star.repo_id"). + Where("star.uid = ?", u.ID).Find(&starredRepoIDs); err != nil { return fmt.Errorf("get all stars: %v", err) - } - for i := range stars { - if _, err = e.Exec("UPDATE `repository` SET num_stars=num_stars-1 WHERE id=?", stars[i].RepoID); err != nil { - return fmt.Errorf("decrease repository star number[%d]: %v", stars[i].RepoID, err) - } + } else if _, err = e.Decr("num_watches").In("id", starredRepoIDs).Update(new(Repository)); err != nil { + return fmt.Errorf("decrease repository num_stars: %v", err) } // ***** END: Star ***** // ***** START: Follow ***** - followers := make([]*Follow, 0, 10) - if err = e.Find(&followers, &Follow{UserID: u.ID}); err != nil { - return fmt.Errorf("get all followers: %v", err) + followeeIDs := make([]int64, 0, 10) + if err = e.Table("follow").Cols("follow.follow_id"). + Where("follow.user_id = ?", u.ID).Find(&followeeIDs); err != nil { + return fmt.Errorf("get all followees: %v", err) + } else if _, err = e.Decr("num_followers").In("id", followeeIDs).Update(new(User)); err != nil { + return fmt.Errorf("decrease user num_followers: %v", err) } - for i := range followers { - if _, err = e.Exec("UPDATE `user` SET num_followers=num_followers-1 WHERE id=?", followers[i].UserID); err != nil { - return fmt.Errorf("decrease user follower number[%d]: %v", followers[i].UserID, err) - } + + followerIDs := make([]int64, 0, 10) + if err = e.Table("follow").Cols("follow.user_id"). + Where("follow.follow_id = ?", u.ID).Find(&followerIDs); err != nil { + return fmt.Errorf("get all followers: %v", err) + } else if _, err = e.Decr("num_following").In("id", followerIDs).Update(new(User)); err != nil { + return fmt.Errorf("decrease user num_following: %v", err) } // ***** END: Follow ***** @@ -965,6 +968,7 @@ func deleteUser(e *xorm.Session, u *User) error { &Access{UserID: u.ID}, &Watch{UserID: u.ID}, &Star{UID: u.ID}, + &Follow{UserID: u.ID}, &Follow{FollowID: u.ID}, &Action{UserID: u.ID}, &IssueUser{UID: u.ID}, diff --git a/models/user_test.go b/models/user_test.go index b10ed9dcba..a16f979530 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -36,5 +36,36 @@ func TestCanCreateOrganization(t *testing.T) { user.AllowCreateOrganization = true assert.True(t, admin.CanCreateOrganization()) assert.False(t, user.CanCreateOrganization()) - +} + +func TestDeleteUser(t *testing.T) { + test := func(userID int64) { + assert.NoError(t, PrepareTestDatabase()) + user := AssertExistsAndLoadBean(t, &User{ID: userID}).(*User) + + ownedRepos := make([]*Repository, 0, 10) + assert.NoError(t, x.Find(&ownedRepos, &Repository{OwnerID: userID})) + if len(ownedRepos) > 0 { + err := DeleteUser(user) + assert.Error(t, err) + assert.True(t, IsErrUserOwnRepos(err)) + return + } + + orgUsers := make([]*OrgUser, 0, 10) + assert.NoError(t, x.Find(&orgUsers, &OrgUser{UID: userID})) + for _, orgUser := range orgUsers { + if err := RemoveOrgUser(orgUser.OrgID, orgUser.UID); err != nil { + assert.True(t, IsErrLastOrgOwner(err)) + return + } + } + assert.NoError(t, DeleteUser(user)) + AssertNotExistsBean(t, &User{ID: userID}) + CheckConsistencyFor(t, &User{}, &Repository{}) + } + test(2) + test(4) + test(8) + test(11) } diff --git a/vendor/github.com/go-xorm/builder/cond_in.go b/vendor/github.com/go-xorm/builder/cond_in.go index 71093e4b49..f6366d35c2 100644 --- a/vendor/github.com/go-xorm/builder/cond_in.go +++ b/vendor/github.com/go-xorm/builder/cond_in.go @@ -23,10 +23,8 @@ func In(col string, values ...interface{}) Cond { } func (condIn condIn) handleBlank(w Writer) error { - if _, err := fmt.Fprintf(w, "%s IN ()", condIn.col); err != nil { - return err - } - return nil + _, err := fmt.Fprint(w, "0=1") + return err } func (condIn condIn) WriteTo(w Writer) error { diff --git a/vendor/github.com/go-xorm/builder/cond_notin.go b/vendor/github.com/go-xorm/builder/cond_notin.go index 9be44cb2af..dc3ac49a35 100644 --- a/vendor/github.com/go-xorm/builder/cond_notin.go +++ b/vendor/github.com/go-xorm/builder/cond_notin.go @@ -20,10 +20,8 @@ func NotIn(col string, values ...interface{}) Cond { } func (condNotIn condNotIn) handleBlank(w Writer) error { - if _, err := fmt.Fprintf(w, "%s NOT IN ()", condNotIn.col); err != nil { - return err - } - return nil + _, err := fmt.Fprint(w, "0=0") + return err } func (condNotIn condNotIn) WriteTo(w Writer) error { diff --git a/vendor/vendor.json b/vendor/vendor.json index 93e8fa9d8b..109f67b998 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -450,10 +450,10 @@ "revisionTime": "2016-11-01T11:13:14Z" }, { - "checksumSHA1": "HHB+Jna1wv0cXLxtCyOnQqFwvn4=", + "checksumSHA1": "9SXbj96wb1PgppBZzxMIN0axbFQ=", "path": "github.com/go-xorm/builder", - "revision": "c6e604e9c7b7461715091e14ad0c242ec44c26e4", - "revisionTime": "2017-02-24T04:30:50Z" + "revision": "043186300e9b2c22abdfc83567a979e3af04d9ae", + "revisionTime": "2017-05-18T21:58:56Z" }, { "checksumSHA1": "vt2CGANHLNXPAZ01ve3UlsgQ0uU=",