From f8f213bbb45993464068d43b4fd1384d8426ffef Mon Sep 17 00:00:00 2001
From: David Byron <dbyron@salesforce.com>
Date: Sat, 20 May 2023 11:31:20 -0700
Subject: [PATCH] fix(core): handle null when invoking pipelines with stages
 that are missing a "type"

so callers see 400 with a helpful mesage instead of 500.

There's also code here in PipelineBuilder.withStages to throw an IllegalArgumentException
for null stages.  Due to [this change](https://github.com/spinnaker/orca/pull/4102/files#diff-88a2bb8f8eddf44a88866f3d27f096d67577c6c5f651acd88fdb3da32ed88d0aR211),
PipelineBuilder.withStages isn't called with null even if stages is null in the incoming
pipeline.  https://github.com/spinnaker/orca/pull/4182 (Nov 30, 2021, version 1.28.0) was
a behavior change from a NullPointerException/500 to succeed (but do nothing) for a
pipeline with null stages.  https://github.com/spinnaker/orca/pull/4102 (Jan 3, 2022) was
also part of version 1.28.0.  Changing the behavior to explictly reject null stages /
respond with 400 instead of accepting them and doing nothing feels better, but I'll leave
that for a separate PR as it's potentially more controversial.
---
 .../orca/pipeline/model/PipelineBuilder.java  | 30 ++++++++++++++-----
 .../pipeline/model/PipelineBuilderTest.java   |  9 ++----
 .../controllers/OperationsControllerTest.java |  3 +-
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilder.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilder.java
index 84235d378c..f618b47b26 100644
--- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilder.java
+++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilder.java
@@ -84,14 +84,30 @@ public PipelineBuilder withStage(String type) {
   }
 
   public PipelineBuilder withStages(List<Map<String, Object>> stages) {
-    if (stages != null) {
-      stages.forEach(
-          it -> {
-            String type = it.remove("type").toString();
-            String name = it.containsKey("name") ? it.remove("name").toString() : null;
-            withStage(type, name != null ? name : type, it);
-          });
+    if (stages == null) {
+      throw new IllegalArgumentException(
+          "null stages in pipeline '"
+              + pipeline.getName()
+              + "' in application '"
+              + pipeline.getApplication()
+              + "'");
     }
+    stages.forEach(
+        it -> {
+          String name = it.containsKey("name") ? it.remove("name").toString() : null;
+          if (!it.containsKey("type")) {
+            throw new IllegalArgumentException(
+                "type missing from pipeline '"
+                    + pipeline.getName()
+                    + "' in application '"
+                    + pipeline.getApplication()
+                    + "', stage name: '"
+                    + name
+                    + "'");
+          }
+          String type = it.remove("type").toString();
+          withStage(type, name != null ? name : type, it);
+        });
     return this;
   }
 
diff --git a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java
index e97f675f31..e678d43636 100644
--- a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java
+++ b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java
@@ -28,9 +28,7 @@ class PipelineBuilderTest {
   @Test
   void withStagesChecksForNull() {
     PipelineBuilder pipelineBuilder = new PipelineBuilder("my-application");
-    // There's no exception, but also no indication that the caller is likely
-    // doing something wrong by passing null.
-    pipelineBuilder.withStages(null);
+    assertThrows(IllegalArgumentException.class, () -> pipelineBuilder.withStages(null));
   }
 
   @Test
@@ -38,9 +36,8 @@ void withStagesChecksForType() {
     PipelineBuilder pipelineBuilder = new PipelineBuilder("my-application");
     Map<String, Object> stageWithoutType = new HashMap<>();
     stageWithoutType.put("name", "my-pipeline-stage");
-    // NullPointerException is unfortunate since the input is bad.  It
-    // translates to a 500 http response code, so callers retry.
     assertThrows(
-        NullPointerException.class, () -> pipelineBuilder.withStages(List.of(stageWithoutType)));
+        IllegalArgumentException.class,
+        () -> pipelineBuilder.withStages(List.of(stageWithoutType)));
   }
 }
diff --git a/orca-web/src/test/java/com/netflix/spinnaker/orca/controllers/OperationsControllerTest.java b/orca-web/src/test/java/com/netflix/spinnaker/orca/controllers/OperationsControllerTest.java
index ed6613ff0d..d94450575f 100644
--- a/orca-web/src/test/java/com/netflix/spinnaker/orca/controllers/OperationsControllerTest.java
+++ b/orca-web/src/test/java/com/netflix/spinnaker/orca/controllers/OperationsControllerTest.java
@@ -125,7 +125,6 @@ void orchestrateTreatsMissingTypeAsBadRequest() throws Exception {
                 .characterEncoding(StandardCharsets.UTF_8.toString())
                 .content(objectMapper.writeValueAsString(pipelineWithStageWithoutType)))
         .andDo(print())
-        // NB: this is a bug.  Expect a bad request so callers don't retry.
-        .andExpect(status().isInternalServerError());
+        .andExpect(status().isBadRequest());
   }
 }