fix(gcb): Return mutable lists from methods annoated with PostFilter (#…

…673)

* test(gcb): Update tests to run postFilter logic

The GCB integration tests currently don't run the postFilter logic
that is added as an annotation on controller methods. I believe this
is because we've been to selective in exactly the beans that should
be present in the test, and are not pulling in the required Spring
Security beans.

In order to fix this, add the same @componentscan that we have on
Main.class to the test so it pulls in the same beans.  (We can then
also remove the specific controller bean we'd been pulling in, but
will leave all the configuration beans in place.)

This causes the listAccountTest to fail because the listAccounts
function is currently broken; the next commit will fix the function
and the test.

* fix(gcb): Return mutable lists from methods annoated with PostFilter

Controller methods annotated with PostFilter cannot return immutable
lists because the filtering is done in-place on the returned array;
update all controller methods in GoogleCloudBuildController to return
a mutable list.

As we would like the lower levels of the stack to deal in immutable
collections as much as possible, just create a mutable collection
at the last step before returning from the controller method.

* fix(gcb): Dirty application context for GCB tests

As we are now fully loading the application context as part of
the GCB tests, we should mark the test with DirtiesContext so
that it does not interfere with other tests.

* fix(gcb): Use mock HystrixSpectatorPublisher bean

The tests were failing because we can only ever create a single
HystrixSpectatorPublisher bean, due to the fact that it mutates
a static field on the HystrixPlugins class, which causes a
failure if we try to mutate it again.

This was addressed in MainTest by clearing the Hystrix class after
every test, but let's just inject a mock bean instead to avoid
mutating the static class in the first place.

igor-web/src/main/java/com/netflix/spinnaker/igor/gcb/GoogleCloudBuildController.java

@@ -19,8 +19,9 @@
 import com.google.api.services.cloudbuild.v1.model.Build;
 import com.google.api.services.cloudbuild.v1.model.BuildTrigger;
 import com.google.api.services.cloudbuild.v1.model.RepoSource;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
+import java.util.List;
 import lombok.RequiredArgsConstructor;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.http.MediaType;
@@ -39,8 +40,8 @@ public class GoogleCloudBuildController {
 
   @RequestMapping(value = "/accounts", method = RequestMethod.GET)
   @PostFilter("hasPermission(filterObject, 'BUILD_SERVICE', 'READ')")
-  public ImmutableList<String> getAccounts() {
-    return googleCloudBuildAccountRepository.getAccounts();
+  public List<String> getAccounts() {
+    return Lists.newArrayList(googleCloudBuildAccountRepository.getAccounts());
   }
 
   @RequestMapping(
@@ -75,25 +76,27 @@ public Build getBuild(@PathVariable String account, @PathVariable String buildId
 
   @RequestMapping(value = "/builds/{account}/{buildId}/artifacts", method = RequestMethod.GET)
   @PreAuthorize("hasPermission(#account, 'BUILD_SERVICE', 'READ')")
-  public ImmutableList<Artifact> getArtifacts(
-      @PathVariable String account, @PathVariable String buildId) {
-    return googleCloudBuildAccountRepository.getGoogleCloudBuild(account).getArtifacts(buildId);
+  public List<Artifact> getArtifacts(@PathVariable String account, @PathVariable String buildId) {
+    return Lists.newArrayList(
+        googleCloudBuildAccountRepository.getGoogleCloudBuild(account).getArtifacts(buildId));
   }
 
   @RequestMapping(
       value = "/artifacts/extract/{account}",
       method = RequestMethod.PUT,
       consumes = MediaType.APPLICATION_JSON_VALUE)
-  public ImmutableList<Artifact> extractArtifacts(
+  public List<Artifact> extractArtifacts(
       @PathVariable String account, @RequestBody String serializedBuild) {
     Build build = googleCloudBuildParser.parse(serializedBuild, Build.class);
-    return googleCloudBuildAccountRepository.getGoogleCloudBuild(account).extractArtifacts(build);
+    return Lists.newArrayList(
+        googleCloudBuildAccountRepository.getGoogleCloudBuild(account).extractArtifacts(build));
   }
 
   @RequestMapping(value = "/triggers/{account}", method = RequestMethod.GET)
   @PreAuthorize("hasPermission(#account, 'BUILD_SERVICE', 'READ')")
-  public ImmutableList<BuildTrigger> listTriggers(@PathVariable String account) {
-    return googleCloudBuildAccountRepository.getGoogleCloudBuild(account).listTriggers();
+  public List<BuildTrigger> listTriggers(@PathVariable String account) {
+    return Lists.newArrayList(
+        googleCloudBuildAccountRepository.getGoogleCloudBuild(account).listTriggers());
   }
 
   @RequestMapping(

igor-web/src/test/groovy/com/netflix/spinnaker/igor/gcb/GoogleCloudBuildTest.java

@@ -37,9 +37,9 @@
 import com.google.api.services.cloudbuild.v1.model.ListBuildTriggersResponse;
 import com.google.api.services.cloudbuild.v1.model.Operation;
 import com.google.api.services.cloudbuild.v1.model.RepoSource;
+import com.netflix.spinnaker.hystrix.spectator.HystrixSpectatorPublisher;
 import com.netflix.spinnaker.igor.RedisConfig;
 import com.netflix.spinnaker.igor.config.LockManagerConfig;
-import com.netflix.spinnaker.kork.web.exceptions.GenericExceptionHandlers;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -54,6 +54,8 @@
 import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.boot.test.context.TestConfiguration;
+import org.springframework.boot.test.mock.mockito.MockBean;
+import org.springframework.context.annotation.ComponentScan;
 import org.springframework.core.annotation.Order;
 import org.springframework.http.MediaType;
 import org.springframework.security.config.annotation.web.builders.HttpSecurity;
@@ -67,19 +69,20 @@
 @RunWith(SpringRunner.class)
 @AutoConfigureMockMvc
 @EnableWebMvc
+@ComponentScan({"com.netflix.spinnaker.config", "com.netflix.spinnaker.igor"})
 @SpringBootTest(
     classes = {
       GoogleCloudBuildConfig.class,
-      GoogleCloudBuildController.class,
       RedisConfig.class,
       LockManagerConfig.class,
-      GenericExceptionHandlers.class,
       GoogleCloudBuildTestConfig.class
     })
 @TestPropertySource(properties = {"spring.config.location=classpath:gcb/gcb-test.yml"})
 public class GoogleCloudBuildTest {
   @Autowired private MockMvc mockMvc;
 
+  @MockBean HystrixSpectatorPublisher hystrixSpectatorPublisher;
+
   @Autowired
   @Qualifier("stubCloudBuildService")
   private WireMockServer stubCloudBuildService;

igor-web/src/test/java/com/netflix/spinnaker/igor/MainTest.java

@@ -16,28 +16,18 @@
 
 package com.netflix.spinnaker.igor;
 
-import com.netflix.hystrix.Hystrix;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import com.netflix.spinnaker.hystrix.spectator.HystrixSpectatorPublisher;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.mock.mockito.MockBean;
 import org.springframework.test.context.junit4.SpringRunner;
 
 @RunWith(SpringRunner.class)
 @SpringBootTest(classes = {RedisConfig.class, Main.class})
 public class MainTest {
-
-  @BeforeClass
-  public static void setUp() {
-    Hystrix.reset();
-  }
+  @MockBean HystrixSpectatorPublisher hystrixSpectatorPublisher;
 
   @Test
   public void startupTest() {}
-
-  @AfterClass
-  public static void tearDown() {
-    Hystrix.reset();
-  }
 }