From 9374ce97b1c3884a4f6dc6a4a45ef525b242b9bb Mon Sep 17 00:00:00 2001 From: anotherchrisberry Date: Wed, 11 May 2016 11:47:23 -0700 Subject: [PATCH] prevent creation of pipelines with duplicate names --- .../controllers/PipelineController.groovy | 16 ++++++ .../controllers/StrategyController.groovy | 17 +++++++ .../controllers/PipelineControllerTck.groovy | 49 +++++++++++++++++++ .../controllers/StrategyControllerTck.groovy | 49 +++++++++++++++++++ 4 files changed, 131 insertions(+) diff --git a/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/PipelineController.groovy b/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/PipelineController.groovy index 2ed61b10d..44b098435 100644 --- a/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/PipelineController.groovy +++ b/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/PipelineController.groovy @@ -19,10 +19,13 @@ package com.netflix.spinnaker.front50.controllers import com.netflix.spinnaker.front50.model.pipeline.Pipeline import com.netflix.spinnaker.front50.model.pipeline.PipelineDAO import org.springframework.beans.factory.annotation.Autowired +import org.springframework.http.HttpStatus +import org.springframework.web.bind.annotation.ExceptionHandler import org.springframework.web.bind.annotation.PathVariable import org.springframework.web.bind.annotation.RequestBody import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RequestMethod +import org.springframework.web.bind.annotation.ResponseStatus import org.springframework.web.bind.annotation.RestController /** @@ -53,6 +56,9 @@ class PipelineController { triggers.findAll { it.type == "cron" }.each { Map trigger -> trigger.id = UUID.randomUUID().toString() } + if (pipelineDAO.getPipelineId(pipeline.getApplication(), pipeline.getName())) { + throw new DuplicatePipelineNameException() + } } pipelineDAO.create(pipeline.id as String, pipeline) @@ -77,6 +83,9 @@ class PipelineController { @RequestMapping(value = 'move', method = RequestMethod.POST) void rename(@RequestBody RenameCommand command) { + if (pipelineDAO.getPipelineId(command.application, command.to)) { + throw new DuplicatePipelineNameException() + } def pipelineId = pipelineDAO.getPipelineId(command.application, command.from) def pipeline = pipelineDAO.findById(pipelineId) pipeline.setName(command.to) @@ -90,4 +99,11 @@ class PipelineController { String to } + @ExceptionHandler(DuplicatePipelineNameException) + @ResponseStatus(HttpStatus.BAD_REQUEST) + Map handleDuplicatePipelineNameException() { + return [error: "A pipeline with that name already exists in that application", status: HttpStatus.BAD_REQUEST] + } + + static class DuplicatePipelineNameException extends Exception {} } diff --git a/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/StrategyController.groovy b/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/StrategyController.groovy index b12646e1a..6440b8472 100644 --- a/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/StrategyController.groovy +++ b/front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/StrategyController.groovy @@ -19,10 +19,13 @@ package com.netflix.spinnaker.front50.controllers import com.netflix.spinnaker.front50.model.pipeline.Pipeline import com.netflix.spinnaker.front50.model.pipeline.PipelineStrategyDAO import org.springframework.beans.factory.annotation.Autowired +import org.springframework.http.HttpStatus +import org.springframework.web.bind.annotation.ExceptionHandler import org.springframework.web.bind.annotation.PathVariable import org.springframework.web.bind.annotation.RequestBody import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RequestMethod +import org.springframework.web.bind.annotation.ResponseStatus import org.springframework.web.bind.annotation.RestController /** @@ -53,6 +56,9 @@ class StrategyController { triggers.findAll { it.type == "cron" }.each { Map trigger -> trigger.id = UUID.randomUUID().toString() } + if (pipelineStrategyDAO.getPipelineId(strategy.getApplication(), strategy.getName())) { + throw new DuplicateStrategyException() + } } pipelineStrategyDAO.create(strategy.getId(), strategy) @@ -77,6 +83,9 @@ class StrategyController { @RequestMapping(value = 'move', method = RequestMethod.POST) void rename(@RequestBody RenameCommand command) { + if (pipelineStrategyDAO.getPipelineId(command.application, command.to)) { + throw new DuplicateStrategyException() + } def pipelineId = pipelineStrategyDAO.getPipelineId(command.application, command.from) def pipeline = pipelineStrategyDAO.findById(pipelineId) pipeline.setName(command.to) @@ -84,6 +93,14 @@ class StrategyController { pipelineStrategyDAO.update(pipelineId, pipeline) } + @ExceptionHandler(DuplicateStrategyException) + @ResponseStatus(HttpStatus.BAD_REQUEST) + Map handleDuplicateStrategyNameException() { + return [error: "A strategy with that name already exists in that application", status: HttpStatus.BAD_REQUEST] + } + + static class DuplicateStrategyException extends Exception {} + static class RenameCommand { String application String from diff --git a/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/PipelineControllerTck.groovy b/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/PipelineControllerTck.groovy index 634311286..220f608b0 100644 --- a/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/PipelineControllerTck.groovy +++ b/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/PipelineControllerTck.groovy @@ -44,6 +44,7 @@ import org.springframework.test.web.servlet.setup.MockMvcBuilders abstract class PipelineControllerTck extends Specification { static final int OK = 200 + static final int BAD_REQUEST = 400 MockMvc mockMvc @@ -137,6 +138,54 @@ abstract class PipelineControllerTck extends Specification { response.status == OK pipelineDAO.all()*.name == ["pipeline2"] } + + void 'should enforce unique names on save operations'() { + given: + pipelineDAO.create(null, new Pipeline([ + name: "pipeline1", application: "test" + ])) + pipelineDAO.create(null, new Pipeline([ + name: "pipeline2", application: "test" + ])) + + when: + def allPipelines = pipelineDAO.all() + def allPipelinesForApplication = pipelineDAO.getPipelinesByApplication("test") + + then: + allPipelines*.id.sort() == allPipelinesForApplication*.id.sort() + allPipelines.size() == 2 + + when: + def response = mockMvc.perform(post('/pipelines') + .contentType(MediaType.APPLICATION_JSON) + .content(new ObjectMapper().writeValueAsString([name: "pipeline1", application: "test"]))) + .andReturn().response + + then: + response.status == BAD_REQUEST + response.contentAsString == '{"error":"A pipeline with that name already exists in that application","status":"BAD_REQUEST"}' + } + + void 'should enforce unique names on rename operations'() { + given: + pipelineDAO.create(null, new Pipeline([ + name: "pipeline1", application: "test" + ])) + pipelineDAO.create(null, new Pipeline([ + name: "pipeline2", application: "test" + ])) + + when: + def response = mockMvc.perform(post('/pipelines/move') + .contentType(MediaType.APPLICATION_JSON) + .content(new ObjectMapper().writeValueAsString([from: "pipeline2", to: "pipeline1", application: "test"]))) + .andReturn().response + + then: + response.status == BAD_REQUEST + response.contentAsString == '{"error":"A pipeline with that name already exists in that application","status":"BAD_REQUEST"}' + } } class CassandraPipelineControllerTck extends PipelineControllerTck { diff --git a/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/StrategyControllerTck.groovy b/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/StrategyControllerTck.groovy index 5b7fbe6eb..bb4789859 100644 --- a/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/StrategyControllerTck.groovy +++ b/front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/StrategyControllerTck.groovy @@ -46,6 +46,7 @@ import org.springframework.test.web.servlet.setup.MockMvcBuilders abstract class StrategyControllerTck extends Specification { static final int OK = 200 + static final int BAD_REQUEST = 400 MockMvc mockMvc @@ -140,6 +141,54 @@ abstract class StrategyControllerTck extends Specification { response.status == OK pipelineStrategyDAO.all()*.name == ["pipeline2"] } + + void 'should enforce unique names on save operations'() { + given: + pipelineStrategyDAO.create(null, new Pipeline([ + name: "pipeline1", application: "test" + ])) + pipelineStrategyDAO.create(null, new Pipeline([ + name: "pipeline2", application: "test" + ])) + + when: + def allPipelines = pipelineStrategyDAO.all() + def allPipelinesForApplication = pipelineStrategyDAO.getPipelinesByApplication("test") + + then: + allPipelines*.id.sort() == allPipelinesForApplication*.id.sort() + allPipelines.size() == 2 + + when: + def response = mockMvc.perform(post('/strategies') + .contentType(MediaType.APPLICATION_JSON) + .content(new ObjectMapper().writeValueAsString([name: "pipeline1", application: "test"]))) + .andReturn().response + + then: + response.status == BAD_REQUEST + response.contentAsString == '{"error":"A strategy with that name already exists in that application","status":"BAD_REQUEST"}' + } + + void 'should enforce unique names on rename operations'() { + given: + pipelineStrategyDAO.create(null, new Pipeline([ + name: "pipeline1", application: "test" + ])) + pipelineStrategyDAO.create(null, new Pipeline([ + name: "pipeline2", application: "test" + ])) + + when: + def response = mockMvc.perform(post('/strategies/move') + .contentType(MediaType.APPLICATION_JSON) + .content(new ObjectMapper().writeValueAsString([from: "pipeline2", to: "pipeline1", application: "test"]))) + .andReturn().response + + then: + response.status == BAD_REQUEST + response.contentAsString == '{"error":"A strategy with that name already exists in that application","status":"BAD_REQUEST"}' + } } class CassandraStrategyControllerTck extends StrategyControllerTck {