fix(web): restore check for regenerateCronTriggerIds in PipelineContr…

…oller.save (#1452)

* refactor(web/test): construct Pipeline objects in the first place

instead of making Maps and casting

* fix(web): restore check for regenerateCronTriggerIds in PipelineController.save

https://github.com/spinnaker/front50/pull/1035/files#diff-9b514be177faf5444c86a88ab6bb9e6a0add032bfa67862bc8e33b17c4bb9cc9L159
removed it, but orca's SavePipelineTask still sets it.

* refactor(web): adjust pipeline triggers directly

The comment in https://github.com/spinnaker/front50/pull/987/files#r515405296 is no longer
true after #1035.  Pipeline.getTriggers no longer
makes a copy, so there's no need to get/modify/set.

---------

Co-authored-by: Jason <[email protected]>

front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/PipelineController.java

@@ -249,13 +249,12 @@ public synchronized Pipeline save(
     pipeline.setName(pipeline.getName().trim());
     pipeline = ensureCronTriggersHaveIdentifier(pipeline);
 
-    if (Strings.isNullOrEmpty(pipeline.getId())) {
+    if (Strings.isNullOrEmpty(pipeline.getId())
+        || (boolean) pipeline.getAny().getOrDefault("regenerateCronTriggerIds", false)) {
       // ensure that cron triggers are assigned a unique identifier for new pipelines
-      List<Trigger> triggers = pipeline.getTriggers();
-      triggers.stream()
+      pipeline.getTriggers().stream()
           .filter(it -> "cron".equals(it.getType()))
           .forEach(it -> it.put("id", UUID.randomUUID().toString()));
-      pipeline.setTriggers(triggers);
     }
 
     return pipelineDAO.create(pipeline.getId(), pipeline);
@@ -417,8 +416,7 @@ private void checkForDuplicatePipeline(String application, String name) {
 
   private static Pipeline ensureCronTriggersHaveIdentifier(Pipeline pipeline) {
     // ensure that all cron triggers have an assigned identifier
-    List<Trigger> triggers = pipeline.getTriggers();
-    triggers.stream()
+    pipeline.getTriggers().stream()
         .filter(it -> "cron".equalsIgnoreCase(it.getType()))
         .forEach(
             it -> {
@@ -429,7 +427,6 @@ private static Pipeline ensureCronTriggersHaveIdentifier(Pipeline pipeline) {
 
               it.put("id", triggerId);
             });
-    pipeline.setTriggers(triggers);
 
     return pipeline;
   }

front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/StrategyController.java

@@ -19,7 +19,6 @@
 
 import com.google.common.base.Strings;
 import com.netflix.spinnaker.front50.api.model.pipeline.Pipeline;
-import com.netflix.spinnaker.front50.api.model.pipeline.Trigger;
 import com.netflix.spinnaker.front50.exceptions.DuplicateEntityException;
 import com.netflix.spinnaker.front50.exceptions.InvalidRequestException;
 import com.netflix.spinnaker.front50.model.pipeline.PipelineStrategyDAO;
@@ -68,11 +67,9 @@ public void save(@RequestBody Pipeline strategy) {
 
       // ensure that cron triggers are assigned a unique identifier for new strategies
       if (strategy.getTriggers() != null) {
-        List<Trigger> triggers = strategy.getTriggers();
-        triggers.stream()
+        strategy.getTriggers().stream()
             .filter(it -> "cron".equals(it.getType()))
             .forEach(it -> it.put("id", UUID.randomUUID().toString()));
-        strategy.setTriggers(triggers);
       }
     }
 

front50-web/src/test/groovy/com/netflix/spinnaker/front50/controllers/PipelineControllerTck.groovy

@@ -188,17 +188,20 @@ abstract class PipelineControllerTck extends Specification {
   }
 
   @Unroll
-  void 'should only (re)generate cron trigger ids for new pipelines'() {
+  void '(re)generates cron trigger ids for new pipelines, or when explicitly specified: lookupPipelineId: #lookupPipelineId, regenerateCronTriggerIds: #regenerateCronTriggerIds'() {
     given:
-    def pipeline = [
+    def pipeline = new Pipeline([
       name       : "My Pipeline",
       application: "test",
       triggers   : [
         new Trigger([type: "cron", id: "original-id"])
       ]
-    ]
+    ])
+    if (regenerateCronTriggerIds != null) {
+      pipeline.setAny("regenerateCronTriggerIds", regenerateCronTriggerIds)
+    }
     if (lookupPipelineId) {
-      pipelineDAO.create(null, pipeline as Pipeline)
+      pipelineDAO.create(null, pipeline)
       pipeline.id = pipelineDAO.findById(
         pipelineDAO.getPipelineId("test", "My Pipeline")
       ).getId()
@@ -218,24 +221,26 @@ abstract class PipelineControllerTck extends Specification {
     expectedTriggerCheck.call(updatedPipeline)
 
     where:
-    lookupPipelineId || expectedTriggerCheck
-    false            || { Pipeline p -> p.triggers*.id != ["original-id"] }
-    true             || { Pipeline p -> p.triggers*.id == ["original-id"] }
+    lookupPipelineId | regenerateCronTriggerIds || expectedTriggerCheck
+    false            | null                     || { Pipeline p -> p.triggers*.id != ["original-id"] }
+    true             | null                     || { Pipeline p -> p.triggers*.id == ["original-id"] }
+    true             | false                    || { Pipeline p -> p.triggers*.id == ["original-id"] }
+    true             | true                     || { Pipeline p -> p.triggers*.id != ["original-id"] }
   }
 
   void 'should ensure that all cron triggers have an identifier'() {
     given:
-    def pipeline = [
+    def pipeline = new Pipeline([
       name       : "My Pipeline",
       application: "test",
       triggers   : [
         new Trigger([type: "cron", id: "original-id", expression: "1"]),
         new Trigger([type: "cron", expression: "2"]),
         new Trigger([type: "cron", id: "", expression: "3"])
       ]
-    ]
+    ])
 
-    pipelineDAO.create(null, pipeline as Pipeline)
+    pipelineDAO.create(null, pipeline)
     pipeline.id = pipelineDAO.findById(
       pipelineDAO.getPipelineId("test", "My Pipeline")
     ).getId()