Update rename logic and add testflight test

Signed-off-by: Ka Hin Ng <kahinnng@gmail.com>

atc/db/team.go

@@ -508,14 +508,6 @@ func (t *team) SavePipeline(
 		}
 	}
 
-	_, err = psql.Delete("jobs").
-		Where(sq.Eq{"pipeline_id": pipelineID, "active": false}).
-		RunWith(tx).
-		Exec()
-	if err != nil {
-		return nil, false, err
-	}
-
 	err = removeUnusedWorkerTaskCaches(tx, pipelineID, config.Jobs)
 	if err != nil {
 		return nil, false, err
@@ -905,25 +897,59 @@ func (t *team) FindCheckContainers(logger lager.Logger, pipelineName string, res
 type UpdateName struct {
 	OldName  string
 	NewName  string
-	TempName int
 }
 
 func (t *team) updateName(tx Tx, jobs []atc.JobConfig, pipelineID int) error {
 	jobsToUpdate := []UpdateName{}
 
-	for counter, job := range jobs {
+	for _, job := range jobs {
 		if job.OldName != "" {
-			jobsToUpdate = append(jobsToUpdate, UpdateName{
-				OldName:  job.OldName,
-				NewName:  job.Name,
-				TempName: counter,
-			})
+			var count int
+			err := psql.Select("COUNT(*) as count").
+				From("jobs").
+				Where(sq.Eq{
+					"name": job.OldName,
+					"pipeline_id": pipelineID}).
+				RunWith(tx).
+				QueryRow().
+				Scan(&count)
+			if err != nil {
+				return err
+			}
+
+			if count != 0 {
+				jobsToUpdate = append(jobsToUpdate, UpdateName{
+					OldName: job.OldName,
+					NewName: job.Name,
+				})
+			}
 		}
 	}
 
+	newMap := make(map[int]bool)
+	for _, updateNames := range jobsToUpdate {
+		isCyclic := checkCyclic(jobsToUpdate, updateNames.OldName, newMap)
+		if isCyclic {
+			return errors.New("job name swapping is not supported at this time")
+		}
+	}
+
+	jobsToUpdate = sortUpdateNames(jobsToUpdate)
+
 	for _, updateName := range jobsToUpdate {
-		_, err := psql.Update("jobs").
-			Set("name", updateName.TempName).
+		_, err := psql.Delete("jobs").
+			Where(sq.Eq{
+				"name": updateName.NewName,
+				"pipeline_id": pipelineID,
+				"active": false}).
+			RunWith(tx).
+			Exec()
+		if err != nil {
+			return err
+		}
+
+		_, err = psql.Update("jobs").
+			Set("name", updateName.NewName).
 			Where(sq.Eq{"name": updateName.OldName, "pipeline_id": pipelineID}).
 			RunWith(tx).
 			Exec()
@@ -932,18 +958,39 @@ func (t *team) updateName(tx Tx, jobs []atc.JobConfig, pipelineID int) error {
 		}
 	}
 
-	for _, updateName := range jobsToUpdate {
-		_, err := psql.Update("jobs").
-			Set("name", updateName.NewName).
-			Where(sq.Eq{"name": updateName.TempName, "pipeline_id": pipelineID}).
-			RunWith(tx).
-			Exec()
-		if err != nil {
-			return err
+	return nil
+}
+
+func checkCyclic(jobNames []UpdateName, curr string, visited map[int]bool) bool {
+	for i, job := range jobNames {
+		if job.NewName == curr && !visited[i] {
+			visited[i] = true
+			checkCyclic(jobNames, job.OldName, visited)
+		} else if job.NewName == curr && visited[i] && curr != job.OldName {
+			return true
 		}
 	}
 
-	return nil
+	return false
+}
+
+func sortUpdateNames(jobNames []UpdateName) []UpdateName {
+	newMap := make(map[string]int)
+	for i, job := range jobNames {
+		newMap[job.NewName] = i+1
+
+		if newMap[job.OldName] != 0 {
+			index := newMap[job.OldName]-1
+
+			tempJob := jobNames[index]
+			jobNames[index] = job
+			jobNames[i] = tempJob
+
+			return sortUpdateNames(jobNames)
+		}
+	}
+
+	return jobNames
 }
 
 func (t *team) saveJob(tx Tx, job atc.JobConfig, pipelineID int, groups []string) error {

atc/db/team_test.go

@@ -1964,29 +1964,6 @@ var _ = Describe("Team", func() {
 			Expect(found).To(BeFalse())
 		})
 
-		It("removes jobs that are inactive", func() {
-			pipeline, _, err := team.SavePipeline(pipelineName, config, 0, db.PipelineNoChange)
-			Expect(err).ToNot(HaveOccurred())
-
-			job, _, err := pipeline.Job("some-job")
-			Expect(err).ToNot(HaveOccurred())
-
-			jobs := config.Jobs
-			config.Jobs = []atc.JobConfig{}
-
-			_, _, err = team.SavePipeline(pipelineName, config, pipeline.ConfigVersion(), db.PipelineNoChange)
-			Expect(err).ToNot(HaveOccurred())
-
-			config.Jobs = jobs
-
-			updatedAgainPipeline, _, err := team.SavePipeline(pipelineName, config, pipeline.ConfigVersion()+1, db.PipelineNoChange)
-			Expect(err).ToNot(HaveOccurred())
-
-			newJob, _, err := updatedAgainPipeline.Job("some-job")
-			Expect(err).ToNot(HaveOccurred())
-			Expect(job.ID()).ToNot(Equal(newJob.ID()))
-		})
-
 		Context("update job names but keeps history", func() {
 			BeforeEach(func() {
 				newJobConfig := atc.JobConfig{
@@ -2027,14 +2004,14 @@ var _ = Describe("Team", func() {
 				config.Jobs = append(config.Jobs, newJobConfig)
 			})
 
-			It("should handle when multiple there are multiple name changes", func() {
+			It("should handle when there are multiple name changes", func() {
 				pipeline, _, err := team.SavePipeline(pipelineName, config, 0, db.PipelineNoChange)
 				Expect(err).ToNot(HaveOccurred())
 
 				job, _, _ := pipeline.Job("some-job")
 				otherJob, _, _ := pipeline.Job("new-job")
 
-				config.Jobs[0].Name = "some-other-job"
+				config.Jobs[0].Name = "new-job"
 				config.Jobs[0].OldName = "some-job"
 
 				config.Jobs[1].Name = "new-other-job"
@@ -2043,46 +2020,44 @@ var _ = Describe("Team", func() {
 				updatedPipeline, _, err := team.SavePipeline(pipelineName, config, pipeline.ConfigVersion(), db.PipelineNoChange)
 				Expect(err).ToNot(HaveOccurred())
 
-				updatedJob, _, _ := updatedPipeline.Job("some-other-job")
+				updatedJob, _, _ := updatedPipeline.Job("new-job")
 				Expect(updatedJob.ID()).To(Equal(job.ID()))
 
 				otherUpdatedJob, _, _ := updatedPipeline.Job("new-other-job")
 				Expect(otherUpdatedJob.ID()).To(Equal(otherJob.ID()))
 			})
 
-			It("should handle when there is a swap with job name", func() {
+			It("should return an error when there is a swap with job name", func() {
 				pipeline, _, err := team.SavePipeline(pipelineName, config, 0, db.PipelineNoChange)
 				Expect(err).ToNot(HaveOccurred())
 
-				job, _, _ := pipeline.Job("some-job")
-				otherJob, _, _ := pipeline.Job("new-job")
-
 				config.Jobs[0].Name = "new-job"
 				config.Jobs[0].OldName = "some-job"
 
 				config.Jobs[1].Name = "some-job"
 				config.Jobs[1].OldName = "new-job"
 
-				updatedPipeline, _, err := team.SavePipeline(pipelineName, config, pipeline.ConfigVersion(), db.PipelineNoChange)
-				Expect(err).ToNot(HaveOccurred())
-
-				updatedJob, _, _ := updatedPipeline.Job("new-job")
-				Expect(updatedJob.ID()).To(Equal(job.ID()))
-
-				otherUpdatedJob, _, _ := updatedPipeline.Job("some-job")
-				Expect(otherUpdatedJob.ID()).To(Equal(otherJob.ID()))
-			})
-
-			It("should error out when new name conflicts with other job names", func() {
-				pipeline, _, err := team.SavePipeline(pipelineName, config, 0, db.PipelineNoChange)
-				Expect(err).ToNot(HaveOccurred())
-
-				config.Jobs[0].Name = "new-job"
-				config.Jobs[0].OldName = "some-job"
-
 				_, _, err = team.SavePipeline(pipelineName, config, pipeline.ConfigVersion(), db.PipelineNoChange)
 				Expect(err).To(HaveOccurred())
 			})
+
+			Context("when new job name is in database but is inactive", func() {
+				It("should successfully update job name", func() {
+					pipeline, _, err := team.SavePipeline(pipelineName, config, 0, db.PipelineNoChange)
+					Expect(err).ToNot(HaveOccurred())
+
+					config.Jobs = config.Jobs[:len(config.Jobs)-1]
+
+					_, _, err = team.SavePipeline(pipelineName, config, pipeline.ConfigVersion(), db.PipelineNoChange)
+					Expect(err).ToNot(HaveOccurred())
+
+					config.Jobs[0].Name = "new-job"
+					config.Jobs[0].OldName = "some-job"
+
+					_, _, err = team.SavePipeline(pipelineName, config, pipeline.ConfigVersion()+1, db.PipelineNoChange)
+					Expect(err).ToNot(HaveOccurred())
+				})
+			})
 		})
 
 		It("removes worker task caches for jobs that are no longer in pipeline", func() {

testflight/fixtures/rename-simple.yml

@@ -0,0 +1,14 @@
+---
+jobs:
+- name: rename-simple
+  old_name: simple
+  plan:
+  - task: simple-task
+    config:
+      platform: linux
+      image_resource:
+        type: mock
+        source: {mirror_self: true}
+      run:
+        path: echo
+        args: ["Hello, world!"]

testflight/rename_job_test.go

@@ -0,0 +1,24 @@
+package testflight_test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/gbytes"
+)
+
+var _ = Describe("Renaming a job", func() {
+	BeforeEach(func() {
+		setAndUnpausePipeline("fixtures/simple.yml")
+	})
+
+	It("retains job history", func() {
+		fly("trigger-job", "-j", inPipeline("simple"), "-w")
+		build := fly("builds", "-p", pipelineName)
+		Expect(build).To(gbytes.Say(pipelineName+"/simple"))
+
+		setPipeline("fixtures/rename-simple.yml")
+
+		build = fly("builds", "-p", pipelineName)
+		Expect(build).To(gbytes.Say(pipelineName+"/rename-simple"))
+	})
+})