Merge pull request #2055 from bradrydzewski/fix/build-seq

Store build number counter in repository table

model/repo.go

@@ -31,6 +31,7 @@ type Repo struct {
 	AllowPush   bool   `json:"allow_push"               meddler:"repo_allow_push"`
 	AllowDeploy bool   `json:"allow_deploys"            meddler:"repo_allow_deploys"`
 	AllowTag    bool   `json:"allow_tags"               meddler:"repo_allow_tags"`
+	Counter     int    `json:"last_build"               meddler:"repo_counter"`
 	Config      string `json:"config_file"              meddler:"repo_config_path"`
 	Hash        string `json:"-"                        meddler:"repo_hash"`
 }

store/datastore/builds.go

@@ -1,9 +1,11 @@
 package datastore
 
 import (
+	"fmt"
 	"time"
 
 	"github.com/drone/drone/model"
+	"github.com/drone/drone/store/datastore/sql"
 	"github.com/russross/meddler"
 )
 
@@ -56,12 +58,14 @@ func (db *datastore) GetBuildQueue() ([]*model.Feed, error) {
 }
 
 func (db *datastore) CreateBuild(build *model.Build, procs ...*model.Proc) error {
-	var number int
+	id, err := db.incrementRepoRetry(build.RepoID)
-	db.QueryRow(rebind(buildNumberLast), build.RepoID).Scan(&number)
+	if err != nil {
-	build.Number = number + 1
+		return err
+	}
+	build.Number = id
 	build.Created = time.Now().UTC().Unix()
 	build.Enqueued = build.Created
-	err := meddler.Insert(db, buildTable, build)
+	err = meddler.Insert(db, buildTable, build)
 	if err != nil {
 		return err
 	}
@@ -75,6 +79,39 @@ func (db *datastore) CreateBuild(build *model.Build, procs ...*model.Proc) error
 	return nil
 }
 
+func (db *datastore) incrementRepoRetry(id int64) (int, error) {
+	repo, err := db.GetRepo(id)
+	if err != nil {
+		return 0, fmt.Errorf("database: cannot fetch repository: %s", err)
+	}
+	for i := 0; i < 10; i++ {
+		seq, err := db.incrementRepo(repo.ID, repo.Counter+i, repo.Counter+i+1)
+		if err != nil {
+			return 0, err
+		}
+		if seq == 0 {
+			continue
+		}
+		return seq, nil
+	}
+	return 0, fmt.Errorf("cannot increment next build number")
+}
+
+func (db *datastore) incrementRepo(id int64, old, new int) (int, error) {
+	results, err := db.Exec(sql.Lookup(db.driver, "repo-update-counter"), new, old, id)
+	if err != nil {
+		return 0, fmt.Errorf("database: update repository counter: %s", err)
+	}
+	updated, err := results.RowsAffected()
+	if err != nil {
+		return 0, fmt.Errorf("database: update repository counter: %s", err)
+	}
+	if updated == 0 {
+		return 0, nil
+	}
+	return new, nil
+}
+
 func (db *datastore) UpdateBuild(build *model.Build) error {
 	return meddler.Update(db, buildTable, build)
 }

store/datastore/builds_test.go

@@ -8,27 +8,40 @@ import (
 )
 
 func TestBuilds(t *testing.T) {
-	db := openTest()
+	repo := &model.Repo{
-	defer db.Close()
+		UserID:   1,
+		FullName: "bradrydzewski/drone",
+		Owner:    "bradrydzewski",
+		Name:     "drone",
+	}
+
+	s := newTest()
+	defer s.Close()
 
-	s := From(db)
 	g := goblin.Goblin(t)
 	g.Describe("Builds", func() {
+		g.Before(func() {
+			s.Exec("DELETE FROM repos")
+			s.CreateRepo(repo)
+		})
+		g.After(func() {
+			s.Exec("DELETE FROM repos")
+		})
 
 		// before each test be sure to purge the package
 		// table data from the database.
 		g.BeforeEach(func() {
-			db.Exec("DELETE FROM builds")
+			s.Exec("DELETE FROM builds")
-			db.Exec("DELETE FROM jobs")
+			s.Exec("DELETE FROM jobs")
 		})
 
 		g.It("Should Post a Build", func() {
 			build := model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusSuccess,
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac",
 			}
-			err := s.CreateBuild(&build, []*model.Proc{}...)
+			err := s.CreateBuild(&build)
 			g.Assert(err == nil).IsTrue()
 			g.Assert(build.ID != 0).IsTrue()
 			g.Assert(build.Number).Equal(1)
@@ -37,12 +50,12 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Put a Build", func() {
 			build := model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Number: 5,
 				Status: model.StatusSuccess,
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac",
 			}
-			s.CreateBuild(&build, []*model.Proc{}...)
+			s.CreateBuild(&build)
 			build.Status = model.StatusRunning
 			err1 := s.UpdateBuild(&build)
 			getbuild, err2 := s.GetBuild(build.ID)
@@ -56,7 +69,7 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Get a Build", func() {
 			build := model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusSuccess,
 			}
 			s.CreateBuild(&build, []*model.Proc{}...)
@@ -69,11 +82,11 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Get a Build by Number", func() {
 			build1 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 			}
 			build2 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 			}
 			err1 := s.CreateBuild(build1, []*model.Proc{}...)
@@ -89,12 +102,12 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Get a Build by Ref", func() {
 			build1 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 				Ref:    "refs/pull/5",
 			}
 			build2 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 				Ref:    "refs/pull/6",
 			}
@@ -112,12 +125,12 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Get a Build by Ref", func() {
 			build1 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 				Ref:    "refs/pull/5",
 			}
 			build2 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 				Ref:    "refs/pull/6",
 			}
@@ -135,13 +148,13 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Get a Build by Commit", func() {
 			build1 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 				Branch: "master",
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac",
 			}
 			build2 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusPending,
 				Branch: "dev",
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144aa",
@@ -161,14 +174,14 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Get the last Build", func() {
 			build1 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusFailure,
 				Branch: "master",
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac",
 				Event:  model.EventPush,
 			}
 			build2 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusSuccess,
 				Branch: "master",
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144aa",
@@ -190,19 +203,19 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should Get the last Build Before Build N", func() {
 			build1 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusFailure,
 				Branch: "master",
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac",
 			}
 			build2 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusSuccess,
 				Branch: "master",
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144aa",
 			}
 			build3 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusRunning,
 				Branch: "master",
 				Commit: "85f8c029b902ed9400bc600bac301a0aadb144aa",
@@ -225,11 +238,11 @@ func TestBuilds(t *testing.T) {
 
 		g.It("Should get recent Builds", func() {
 			build1 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusFailure,
 			}
 			build2 := &model.Build{
-				RepoID: 1,
+				RepoID: repo.ID,
 				Status: model.StatusSuccess,
 			}
 			s.CreateBuild(build1, []*model.Proc{}...)
@@ -243,3 +256,58 @@ func TestBuilds(t *testing.T) {
 		})
 	})
 }
+
+func TestBuildIncrement(t *testing.T) {
+	s := newTest()
+	defer func() {
+		s.Exec("delete from repos")
+		s.Exec("delete from builds")
+		s.Close()
+	}()
+
+	repo := &model.Repo{
+		UserID:   1,
+		FullName: "bradrydzewski/drone",
+		Owner:    "bradrydzewski",
+		Name:     "drone",
+	}
+	if err := s.CreateRepo(repo); err != nil {
+		t.Error(err)
+	}
+
+	num, err := s.incrementRepo(repo.ID, 0, 1)
+	if err != nil {
+		t.Error(err)
+	}
+
+	if got, want := num, 1; got != want {
+		t.Errorf("Want repository counter incremented to %d, got %d", want, got)
+	}
+
+	num, err = s.incrementRepo(repo.ID, 1, 2)
+	if err != nil {
+		t.Error(err)
+	}
+
+	if got, want := num, 2; got != want {
+		t.Errorf("Want repository counter incremented to %d, got %d", want, got)
+	}
+
+	// this block tests incrementing the repository counter
+	// should fail when attempting to increment the counter
+	// from a stale base.
+	num, _ = s.incrementRepo(repo.ID, 1, 2)
+	if num != 0 {
+		t.Errorf("Want error when trying to increment stale number")
+	}
+
+	// this block tests incrementing the repository counter
+	// using the given repository id with backoff.
+	num, err = s.incrementRepoRetry(repo.ID)
+	if err != nil {
+		t.Error(err)
+	}
+	if got, want := num, 3; got != want {
+		t.Errorf("Want repository counter incremented to %d, got %d", want, got)
+	}
+}

store/datastore/config_test.go

@@ -62,14 +62,23 @@ func TestConfigApproved(t *testing.T) {
 	defer func() {
 		s.Exec("delete from config")
 		s.Exec("delete from builds")
+		s.Exec("delete from repos")
 		s.Close()
 	}()
 
+	repo := &model.Repo{
+		UserID:   1,
+		FullName: "bradrydzewski/drone",
+		Owner:    "bradrydzewski",
+		Name:     "drone",
+	}
+	s.CreateRepo(repo)
+
 	var (
 		data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]"
 		hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26"
 		conf = &model.Config{
-			RepoID: 1,
+			RepoID: repo.ID,
 			Data:   data,
 			Hash:   hash,
 		}
@@ -79,15 +88,14 @@ func TestConfigApproved(t *testing.T) {
 		t.Errorf("Unexpected error: insert config: %s", err)
 		return
 	}
-
 	s.CreateBuild(&model.Build{
-		RepoID:   1,
+		RepoID:   repo.ID,
 		ConfigID: conf.ID,
 		Status:   model.StatusBlocked,
 		Commit:   "85f8c029b902ed9400bc600bac301a0aadb144ac",
 	})
 	s.CreateBuild(&model.Build{
-		RepoID:   1,
+		RepoID:   repo.ID,
 		ConfigID: conf.ID,
 		Status:   model.StatusPending,
 		Commit:   "85f8c029b902ed9400bc600bac301a0aadb144ac",
@@ -99,7 +107,7 @@ func TestConfigApproved(t *testing.T) {
 	}
 
 	s.CreateBuild(&model.Build{
-		RepoID:   1,
+		RepoID:   repo.ID,
 		ConfigID: conf.ID,
 		Status:   model.StatusRunning,
 		Commit:   "85f8c029b902ed9400bc600bac301a0aadb144ac",

store/datastore/ddl/mysql/ddl_gen.go

@@ -96,6 +96,14 @@ var migrations = []struct {
 		name: "update-table-set-repo-visibility",
 		stmt: updateTableSetRepoVisibility,
 	},
+	{
+		name: "alter-table-add-repo-seq",
+		stmt: alterTableAddRepoSeq,
+	},
+	{
+		name: "update-table-set-repo-seq",
+		stmt: updateTableSetRepoSeq,
+	},
 }
 
 // Migrate performs the database migration. If the migration fails
@@ -466,3 +474,19 @@ SET repo_visibility = CASE
   ELSE 'private'
   END
 `
+
+//
+// 014_add_column_repo_seq.sql
+//
+
+var alterTableAddRepoSeq = `
+ALTER TABLE repos ADD COLUMN repo_counter INTEGER;
+`
+
+var updateTableSetRepoSeq = `
+UPDATE repos SET repo_counter = (
+  SELECT max(build_number)
+  FROM builds
+  WHERE builds.build_repo_id = repos.repo_id
+)
+`

store/datastore/ddl/mysql/files/014_add_column_repo_seq.sql

@@ -0,0 +1,11 @@
+-- name: alter-table-add-repo-seq
+
+ALTER TABLE repos ADD COLUMN repo_counter INTEGER;
+
+-- name: update-table-set-repo-seq
+
+UPDATE repos SET repo_counter = (
+  SELECT max(build_number)
+  FROM builds
+  WHERE builds.build_repo_id = repos.repo_id
+)

store/datastore/ddl/postgres/ddl_gen.go

@@ -96,6 +96,14 @@ var migrations = []struct {
 		name: "update-table-set-repo-visibility",
 		stmt: updateTableSetRepoVisibility,
 	},
+	{
+		name: "alter-table-add-repo-seq",
+		stmt: alterTableAddRepoSeq,
+	},
+	{
+		name: "update-table-set-repo-seq",
+		stmt: updateTableSetRepoSeq,
+	},
 }
 
 // Migrate performs the database migration. If the migration fails
@@ -466,3 +474,19 @@ SET repo_visibility = (CASE
   ELSE 'private'
   END)
 `
+
+//
+// 014_add_column_repo_seq.sql
+//
+
+var alterTableAddRepoSeq = `
+ALTER TABLE repos ADD COLUMN repo_counter INTEGER;
+`
+
+var updateTableSetRepoSeq = `
+UPDATE repos SET repo_counter = (
+  SELECT max(build_number)
+  FROM builds
+  WHERE builds.build_repo_id = repos.repo_id
+)
+`

store/datastore/ddl/postgres/files/014_add_column_repo_seq.sql

@@ -0,0 +1,11 @@
+-- name: alter-table-add-repo-seq
+
+ALTER TABLE repos ADD COLUMN repo_counter INTEGER;
+
+-- name: update-table-set-repo-seq
+
+UPDATE repos SET repo_counter = (
+  SELECT max(build_number)
+  FROM builds
+  WHERE builds.build_repo_id = repos.repo_id
+)

store/datastore/ddl/sqlite/ddl_gen.go

@@ -100,6 +100,14 @@ var migrations = []struct {
 		name: "update-table-set-repo-visibility",
 		stmt: updateTableSetRepoVisibility,
 	},
+	{
+		name: "alter-table-add-repo-seq",
+		stmt: alterTableAddRepoSeq,
+	},
+	{
+		name: "update-table-set-repo-seq",
+		stmt: updateTableSetRepoSeq,
+	},
 }
 
 // Migrate performs the database migration. If the migration fails
@@ -467,3 +475,19 @@ SET repo_visibility = CASE
   ELSE 'private'
   END
 `
+
+//
+// 014_add_column_repo_seq.sql
+//
+
+var alterTableAddRepoSeq = `
+ALTER TABLE repos ADD COLUMN repo_counter INTEGER;
+`
+
+var updateTableSetRepoSeq = `
+UPDATE repos SET repo_counter = (
+  SELECT max(build_number)
+  FROM builds
+  WHERE builds.build_repo_id = repos.repo_id
+)
+`

store/datastore/ddl/sqlite/files/014_add_column_repo_seq.sql

@@ -0,0 +1,11 @@
+-- name: alter-table-add-repo-seq
+
+ALTER TABLE repos ADD COLUMN repo_counter INTEGER;
+
+-- name: update-table-set-repo-seq
+
+UPDATE repos SET repo_counter = (
+  SELECT max(build_number)
+  FROM builds
+  WHERE builds.build_repo_id = repos.repo_id
+)

store/datastore/sql/postgres/files/repos.sql

@@ -0,0 +1,5 @@
+-- name: repo-update-counter
+
+UPDATE repos SET repo_counter = $1
+WHERE repo_counter = $2
+  AND repo_id = $3

store/datastore/sql/postgres/sql_gen.go

@@ -25,6 +25,7 @@ var index = map[string]string{
 	"registry-find-repo-addr":   registryFindRepoAddr,
 	"registry-delete-repo":      registryDeleteRepo,
 	"registry-delete":           registryDelete,
+	"repo-update-counter":       repoUpdateCounter,
 	"secret-find-repo":          secretFindRepo,
 	"secret-find-repo-name":     secretFindRepoName,
 	"secret-delete":             secretDelete,
@@ -249,6 +250,12 @@ var registryDelete = `
 DELETE FROM registry WHERE registry_id = $1
 `
 
+var repoUpdateCounter = `
+UPDATE repos SET repo_counter = $1
+WHERE repo_counter = $2
+  AND repo_id = $3
+`
+
 var secretFindRepo = `
 SELECT
  secret_id

store/datastore/sql/sqlite/files/repos.sql

@@ -0,0 +1,5 @@
+-- name: repo-update-counter
+
+UPDATE repos SET repo_counter = ?
+WHERE repo_counter = ?
+  AND repo_id = ?

store/datastore/sql/sqlite/sql_gen.go

@@ -25,6 +25,7 @@ var index = map[string]string{
 	"registry-find-repo-addr":   registryFindRepoAddr,
 	"registry-delete-repo":      registryDeleteRepo,
 	"registry-delete":           registryDelete,
+	"repo-update-counter":       repoUpdateCounter,
 	"secret-find-repo":          secretFindRepo,
 	"secret-find-repo-name":     secretFindRepoName,
 	"secret-delete":             secretDelete,
@@ -249,6 +250,12 @@ var registryDelete = `
 DELETE FROM registry WHERE registry_id = ?
 `
 
+var repoUpdateCounter = `
+UPDATE repos SET repo_counter = ?
+WHERE repo_counter = ?
+  AND repo_id = ?
+`
+
 var secretFindRepo = `
 SELECT
  secret_id

store/datastore/users_test.go

@@ -8,9 +8,8 @@ import (
 )
 
 func TestUsers(t *testing.T) {
-	db := openTest()
+	s := newTest()
-	defer db.Close()
+	defer s.Close()
-	s := From(db)
 
 	g := goblin.Goblin(t)
 	g.Describe("User", func() {
@@ -18,10 +17,10 @@ func TestUsers(t *testing.T) {
 		// before each test be sure to purge the package
 		// table data from the database.
 		g.BeforeEach(func() {
-			db.Exec("DELETE FROM users")
+			s.Exec("DELETE FROM users")
-			db.Exec("DELETE FROM repos")
+			s.Exec("DELETE FROM repos")
-			db.Exec("DELETE FROM builds")
+			s.Exec("DELETE FROM builds")
-			db.Exec("DELETE FROM jobs")
+			s.Exec("DELETE FROM jobs")
 		})
 
 		g.It("Should Update a User", func() {
@@ -138,7 +137,11 @@ func TestUsers(t *testing.T) {
 			s.CreateUser(&user2)
 			count, err := s.GetUserCount()
 			g.Assert(err == nil).IsTrue()
-			g.Assert(count).Equal(2)
+			if s.driver != "postgres" {
+				// we have to skip this check for postgres because it uses
+				// an estimate which may not be updated.
+				g.Assert(count).Equal(2)
+			}
 		})
 
 		g.It("Should Get a User Count Zero", func() {