fix(core): handle null when invoking pipelines with stages that are m…

…issing a "type" (#4655)

* test(core): demonstrate the current behavior when invoking some invalid pipelines

* 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.  #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.  #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-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;
   }
 

orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright 2023 Salesforce, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.orca.pipeline.model;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+class PipelineBuilderTest {
+
+  @Test
+  void withStagesChecksForNull() {
+    PipelineBuilder pipelineBuilder = new PipelineBuilder("my-application");
+    assertThrows(IllegalArgumentException.class, () -> pipelineBuilder.withStages(null));
+  }
+
+  @Test
+  void withStagesChecksForType() {
+    PipelineBuilder pipelineBuilder = new PipelineBuilder("my-application");
+    Map<String, Object> stageWithoutType = new HashMap<>();
+    stageWithoutType.put("name", "my-pipeline-stage");
+    assertThrows(
+        IllegalArgumentException.class,
+        () -> pipelineBuilder.withStages(List.of(stageWithoutType)));
+  }
+}

orca-web/src/test/java/com/netflix/spinnaker/orca/controllers/OperationsControllerTest.java

@@ -0,0 +1,130 @@
+/*
+ * Copyright 2023 Salesforce, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.orca.controllers;
+
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
+import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
+import static org.springframework.test.web.servlet.setup.MockMvcBuilders.webAppContextSetup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.netflix.spinnaker.orca.Main;
+import com.netflix.spinnaker.orca.notifications.NotificationClusterLock;
+import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository;
+import com.netflix.spinnaker.orca.q.pending.PendingExecutionService;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.Map;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.mock.mockito.MockBean;
+import org.springframework.http.MediaType;
+import org.springframework.test.context.TestPropertySource;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.web.context.WebApplicationContext;
+
+@SpringBootTest(classes = Main.class)
+@TestPropertySource(
+    properties = {
+      "spring.config.location=classpath:orca-test.yml",
+      "keiko.queue.redis.enabled = false"
+    })
+class OperationsControllerTest {
+
+  private MockMvc webAppMockMvc;
+
+  @Autowired private WebApplicationContext webApplicationContext;
+
+  @Autowired ObjectMapper objectMapper;
+
+  @MockBean ExecutionRepository executionRepository;
+
+  @MockBean PendingExecutionService pendingExecutionService;
+
+  @MockBean NotificationClusterLock notificationClusterLock;
+
+  @BeforeEach
+  void init(TestInfo testInfo) {
+    System.out.println("--------------- Test " + testInfo.getDisplayName());
+    webAppMockMvc = webAppContextSetup(webApplicationContext).build();
+  }
+
+  @Test
+  void orchestrateWithEmptyRequestBody() throws Exception {
+    webAppMockMvc
+        .perform(
+            post("/orchestrate")
+                .contentType(MediaType.APPLICATION_JSON_VALUE)
+                .characterEncoding(StandardCharsets.UTF_8.toString()))
+        .andDo(print())
+        .andExpect(status().isBadRequest());
+  }
+
+  @Test
+  void orchestrateWithEmptyPipelineMap() throws Exception {
+    webAppMockMvc
+        .perform(
+            post("/orchestrate")
+                .contentType(MediaType.APPLICATION_JSON_VALUE)
+                .characterEncoding(StandardCharsets.UTF_8.toString())
+                .content(objectMapper.writeValueAsString(Map.of())))
+        .andDo(print())
+        // It's a little marginal to accept an empty pipeline since it's likely
+        // a user error to try to run a pipeline that doesn't do anything.
+        .andExpect(status().isOk());
+  }
+
+  @Test
+  void orchestrateWithoutStages() throws Exception {
+    Map<String, Object> pipelineWithoutStages =
+        Map.of("application", "my-application", "name", "my-pipeline-name");
+    webAppMockMvc
+        .perform(
+            post("/orchestrate")
+                .contentType(MediaType.APPLICATION_JSON_VALUE)
+                .characterEncoding(StandardCharsets.UTF_8.toString())
+                .content(objectMapper.writeValueAsString(pipelineWithoutStages)))
+        .andDo(print())
+        // It's a little marginal to accept a pipeline without stages since it's
+        // likely a user error to try to run a pipeline that doesn't do
+        // anything.
+        .andExpect(status().isOk());
+  }
+
+  @Test
+  void orchestrateTreatsMissingTypeAsBadRequest() throws Exception {
+    Map<String, Object> pipelineWithStageWithoutType =
+        Map.of(
+            "application",
+            "my-application",
+            "name",
+            "my-pipeline-name",
+            "stages",
+            List.of(Map.of("name", "my-pipeline-stage-name")));
+    webAppMockMvc
+        .perform(
+            post("/orchestrate")
+                .contentType(MediaType.APPLICATION_JSON_VALUE)
+                .characterEncoding(StandardCharsets.UTF_8.toString())
+                .content(objectMapper.writeValueAsString(pipelineWithStageWithoutType)))
+        .andDo(print())
+        .andExpect(status().isBadRequest());
+  }
+}