[Environment API Spark]: Compute MergedEnvironmentConfig etag using Objects.hash (#5812)

* Making imlelmetaion consistent with ruby api implemetation

- Use `getEnvironments` method instead of `getAllMergedEnvironments` to
  get all environment for index call.

- Use `getEnvironmentConfig` instead of `getMergedEnvironmentforDisplay`
  to find environment by name.

* Calculate md5 using object hash.

- This is done as Environment coming from config repo represeted using
  MergedEnvironmentConfig object and caculating etag for none config
  entity is not supported by entity  hashing service.

- Here we have used `Objects.hash(...)` which generates identical hash
  for same valued objects.

* Fixed indentation of code.

* Sort the EnvironmentConfigs by name to guaranty the order in the response as well as for Etag computation

Author: bdpiprava

api/api-environments-v2/src/main/java/com/thoughtworks/go/apiv2/environments/EnvironmentsControllerV2.java

@@ -29,7 +29,9 @@
 import com.thoughtworks.go.apiv2.environments.representers.PatchEnvironmentRequestRepresenter;
 import com.thoughtworks.go.config.BasicEnvironmentConfig;
 import com.thoughtworks.go.config.EnvironmentConfig;
+import com.thoughtworks.go.config.exceptions.NoSuchEnvironmentException;
 import com.thoughtworks.go.config.exceptions.RecordNotFoundException;
+import com.thoughtworks.go.config.merge.MergeEnvironmentConfig;
 import com.thoughtworks.go.domain.ConfigElementForEdit;
 import com.thoughtworks.go.server.service.EntityHashingService;
 import com.thoughtworks.go.server.service.EnvironmentConfigService;
@@ -44,7 +46,10 @@
 import spark.Response;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Comparator;
 import java.util.List;
+import java.util.Objects;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
@@ -92,7 +97,8 @@ public void setupRoutes() {
     }
 
     public String index(Request request, Response response) throws IOException {
-        List<EnvironmentConfig> environmentViewModelList = environmentConfigService.getAllMergedEnvironments();
+        List<EnvironmentConfig> environmentViewModelList = environmentConfigService.getEnvironments().stream()
+                .sorted(Comparator.comparing(EnvironmentConfig::name)).collect(Collectors.toList());
 
         setEtagHeader(response, calculateEtag(environmentViewModelList));
 
@@ -179,14 +185,19 @@ public String remove(Request request, Response response) {
 
     @Override
     public String etagFor(EnvironmentConfig entityFromServer) {
+        if (entityFromServer instanceof MergeEnvironmentConfig) {
+            return DigestUtils.md5Hex(String.valueOf(Objects.hash(entityFromServer)));
+        }
         return entityHashingService.md5ForEntity(entityFromServer);
     }
 
     @Override
     public EnvironmentConfig doFetchEntityFromConfig(String name) {
-        ConfigElementForEdit<EnvironmentConfig> mergedEnvironmentforDisplay = environmentConfigService.getMergedEnvironmentforDisplay(name, new HttpLocalizedOperationResult());
-
-        return mergedEnvironmentforDisplay == null ? null : mergedEnvironmentforDisplay.getConfigElement();
+        try {
+            return environmentConfigService.getEnvironmentConfig(name);
+        } catch (NoSuchEnvironmentException e) {
+            throw new RecordNotFoundException(e);
+        }
     }
 
     @Override
@@ -212,7 +223,7 @@ private void haltIfEntityWithSameNameExists(EnvironmentConfig environmentConfig)
         throw haltBecauseEntityAlreadyExists(jsonWriter(environmentConfig), "environment", environmentConfig.name().toString());
     }
 
-    private String calculateEtag(List<EnvironmentConfig> environmentConfigs) {
+    private String calculateEtag(Collection<EnvironmentConfig> environmentConfigs) {
         final String environmentConfigSegment = environmentConfigs
                 .stream()
                 .map(this::etagFor)

api/api-environments-v2/src/main/java/com/thoughtworks/go/apiv2/environments/representers/EnvironmentRepresenter.java

@@ -60,11 +60,11 @@ public static EnvironmentConfig fromJSON(JsonReader jsonReader) {
 
         jsonReader.readArrayIfPresent("environment_variables",
                 array -> array.forEach(envVar -> {
-                        String name = envVar.getAsJsonObject().get("name").getAsString();
-                        String value = envVar.getAsJsonObject().get("value").getAsString();
-                        boolean secure = envVar.getAsJsonObject().get("secure").getAsBoolean();
-                        environmentConfig.addEnvironmentVariable(new EnvironmentVariableConfig(new GoCipher(), name, value, secure));
-                    }
+                            String name = envVar.getAsJsonObject().get("name").getAsString();
+                            String value = envVar.getAsJsonObject().get("value").getAsString();
+                            boolean secure = envVar.getAsJsonObject().get("secure").getAsBoolean();
+                            environmentConfig.addEnvironmentVariable(new EnvironmentVariableConfig(new GoCipher(), name, value, secure));
+                        }
                 )
         );
 

api/api-environments-v2/src/main/java/com/thoughtworks/go/apiv2/environments/representers/EnvironmentsRepresenter.java

@@ -20,18 +20,18 @@
 import com.thoughtworks.go.config.EnvironmentConfig;
 import com.thoughtworks.go.spark.Routes;
 
-import java.util.List;
+import java.util.Collection;
 
 public class EnvironmentsRepresenter {
-    public static void toJSON(OutputWriter writer, List<EnvironmentConfig> environmentViewModelList) {
-            writer.addLinks(
-                    outputLinkWriter -> outputLinkWriter
-                            .addLink("self", Routes.Environments.BASE)
-                            .addAbsoluteLink("doc", Routes.Environments.DOC))
-                    .addChild("_embedded",
-                            embeddedWriter -> embeddedWriter.addChildList("environments",
-                                    environmentsWriter -> environmentViewModelList.forEach(
-                                            environment -> environmentsWriter.addChild(
-                                                    environmentWriter -> EnvironmentRepresenter.toJSON(environmentWriter, environment)))));
+    public static void toJSON(OutputWriter writer, Collection<EnvironmentConfig> environmentViewModelList) {
+        writer.addLinks(
+                outputLinkWriter -> outputLinkWriter
+                        .addLink("self", Routes.Environments.BASE)
+                        .addAbsoluteLink("doc", Routes.Environments.DOC))
+                .addChild("_embedded",
+                        embeddedWriter -> embeddedWriter.addChildList("environments",
+                                environmentsWriter -> environmentViewModelList.forEach(
+                                        environment -> environmentsWriter.addChild(
+                                                environmentWriter -> EnvironmentRepresenter.toJSON(environmentWriter, environment)))));
     }
 }

api/api-environments-v2/src/main/java/com/thoughtworks/go/apiv2/environments/representers/PatchEnvironmentRequestRepresenter.java

@@ -50,9 +50,9 @@ public static PatchEnvironmentRequest fromJSON(JsonReader jsonReader) {
             envVariableToRemove.addAll(reader.readStringArrayIfPresent("remove").orElse(emptyList()));
 
             reader.readArrayIfPresent("add", array ->
-                            array.forEach(envVariable -> envVariablesToAdd
-                                    .add(EnvironmentVariableRepresenter.fromJSON(envVariable.getAsJsonObject()))
-                            ));
+                    array.forEach(envVariable -> envVariablesToAdd
+                            .add(EnvironmentVariableRepresenter.fromJSON(envVariable.getAsJsonObject()))
+                    ));
         });
 
         PatchEnvironmentRequest patchRequest = new PatchEnvironmentRequest(

api/api-environments-v2/src/main/java/com/thoughtworks/go/apiv2/environments/representers/PipelineRepresenter.java

@@ -32,6 +32,6 @@ private static void addLinks(OutputLinkWriter linksWriter, EnvironmentPipelineCo
         String pipelineName = pipelineConfig.getName().toString();
         linksWriter.addLink("self", Routes.Pipeline.history(pipelineName))
                 .addAbsoluteLink("doc", Routes.Pipeline.DOC)
-        .addAbsoluteLink("find", Routes.PipelineConfig.find());
+                .addAbsoluteLink("find", Routes.PipelineConfig.find());
     }
 }

api/api-environments-v2/src/test/groovy/com/thoughtworks/go/apiv2/environments/EnvironmentsControllerV2Test.groovy

@@ -94,7 +94,7 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
         env1.addPipeline(new CaseInsensitiveString("Pipeline2"))
 
         when(entityHashingService.md5ForEntity(env1)).thenReturn("md5-hash")
-        when(environmentConfigService.getMergedEnvironmentforDisplay(eq("env1"), any(HttpLocalizedOperationResult.class))).thenReturn(new ConfigElementForEdit(env1, "md5-hash"))
+        when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(env1)
 
         getWithApiHeader(controller.controllerPath("env1"))
 
@@ -154,7 +154,7 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
         env1.addPipeline(new CaseInsensitiveString("Pipeline1"))
         env1.addPipeline(new CaseInsensitiveString("Pipeline2"))
 
-        when(environmentConfigService.getMergedEnvironmentforDisplay(anyString(), any(HttpLocalizedOperationResult.class))).thenReturn(new ConfigElementForEdit(env1, "3123abcef"))
+        when(environmentConfigService.getEnvironmentConfig(anyString())).thenReturn(env1)
         when(environmentConfigService.deleteEnvironment(eq(env1), eq(currentUsername()), any(HttpLocalizedOperationResult))).then({
           InvocationOnMock invocation ->
             HttpLocalizedOperationResult result = (HttpLocalizedOperationResult) invocation.getArguments().last()
@@ -223,14 +223,11 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
         newConfig.addPipeline(new CaseInsensitiveString("Pipeline1"))
 
         when(entityHashingService.md5ForEntity(existingConfig)).thenReturn("ffff")
-        when(environmentConfigService.getMergedEnvironmentforDisplay(
-          eq("env1"),
-          any(HttpLocalizedOperationResult))
-        ).thenReturn(new ConfigElementForEdit<>(existingConfig, "ffff"))
+        when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(existingConfig)
 
         def json = toObjectString({ EnvironmentRepresenter.toJSON(it, newConfig) })
 
-        putWithApiHeader(controller.controllerPath("env1"),['if-match': 'ffff'], json)
+        putWithApiHeader(controller.controllerPath("env1"), ['if-match': 'ffff'], json)
 
         assertThatResponse()
           .isUnprocessableEntity()
@@ -245,16 +242,13 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
         env1.addPipeline(new CaseInsensitiveString("Pipeline1"))
 
 
-        when(environmentConfigService.getMergedEnvironmentforDisplay(
-          eq("env1"),
-          any(HttpLocalizedOperationResult))
-        ).thenReturn(new ConfigElementForEdit<>(env1, "ffff"))
+        when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(env1)
 
         when(entityHashingService.md5ForEntity(env1)).thenReturn("wrong-md5")
 
         def json = toObjectString({ EnvironmentRepresenter.toJSON(it, env1) })
 
-        putWithApiHeader(controller.controllerPath("env1"),['if-match': 'ffff'], json)
+        putWithApiHeader(controller.controllerPath("env1"), ['if-match': 'ffff'], json)
 
         assertThatResponse()
           .isPreconditionFailed()
@@ -272,10 +266,7 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
 
         when(entityHashingService.md5ForEntity(env1)).thenReturn("ffff")
 
-        when(
-            environmentConfigService.getMergedEnvironmentforDisplay(eq("env1"),
-            any(HttpLocalizedOperationResult))
-        ).thenReturn(new ConfigElementForEdit<>(env1, "ffff"))
+        when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(env1)
 
         when(environmentConfigService.updateEnvironment(eq("env1"), eq(env1), eq(currentUsername()), anyString(), any(HttpLocalizedOperationResult))).then({
           InvocationOnMock invocation ->
@@ -285,7 +276,7 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
 
         def json = toObjectString({ EnvironmentRepresenter.toJSON(it, env1) })
 
-        putWithApiHeader(controller.controllerPath("env1"),['if-match': 'ffff'], json)
+        putWithApiHeader(controller.controllerPath("env1"), ['if-match': 'ffff'], json)
 
         assertThatResponse()
           .isOk()
@@ -296,7 +287,7 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
       @Test
       void 'should error out if the environment does not exist'() {
         when(environmentConfigService.getMergedEnvironmentforDisplay(eq("env1"), any(HttpLocalizedOperationResult)))
-        .then({
+          .then({
           InvocationOnMock invocation ->
             def result = (HttpLocalizedOperationResult) invocation.arguments.last()
             result.badRequest("The environment does not exist")
@@ -352,12 +343,10 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
         updatedConfig.addPipeline(new CaseInsensitiveString("Pipeline2"))
 
         when(entityHashingService.md5ForEntity(updatedConfig)).thenReturn("md5-hash")
-        when(environmentConfigService.getMergedEnvironmentforDisplay(eq("env1"), any(HttpLocalizedOperationResult)))
-          .thenReturn(new ConfigElementForEdit<>(oldConfig, "old_md5_hash"))
-        when(environmentConfigService.getMergedEnvironmentforDisplay(eq("env1"), any(HttpLocalizedOperationResult)))
-          .thenReturn(new ConfigElementForEdit<>(updatedConfig, "new_md5_hash"))
+        when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(oldConfig)
+        when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(updatedConfig)
         when(environmentConfigService.patchEnvironment(
-          eq(oldConfig), anyList(), anyList(), anyList(), anyList(),anyList(), anyList(),eq(currentUsername()), any(HttpLocalizedOperationResult))
+          eq(oldConfig), anyList(), anyList(), anyList(), anyList(), anyList(), anyList(), eq(currentUsername()), any(HttpLocalizedOperationResult))
         ).then({
           InvocationOnMock invocation ->
             HttpLocalizedOperationResult result = (HttpLocalizedOperationResult) invocation.arguments.last()
@@ -366,26 +355,26 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
 
 
         patchWithApiHeader(controller.controllerPath("env1"), [
-          "pipelines": [
-            "add": [
+          "pipelines"            : [
+            "add"   : [
               "Pipeline1", "Pipeline2"
             ],
             "remove": [
               "Pipeline3"
             ]
           ],
-          "agents": [
-            "add": [
+          "agents"               : [
+            "add"   : [
               "agent1"
             ],
             "remove": [
               "agent2"
             ]
           ],
           "environment_variables": [
-            "add": [
+            "add"   : [
               [
-                "name": "JAVA_HOME",
+                "name" : "JAVA_HOME",
                 "value": "/bin/java"
               ]
             ],
@@ -516,24 +505,26 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
       }
 
       @Test
-      void 'should return all configured environments'() {
-        def env1 = new BasicEnvironmentConfig(new CaseInsensitiveString("env1"))
-        env1.addAgent("agent1")
-        env1.addAgent("agent2")
-        env1.addEnvironmentVariable("JAVA_HOME", "/bin/java")
-        env1.addPipeline(new CaseInsensitiveString("Pipeline1"))
-        env1.addPipeline(new CaseInsensitiveString("Pipeline2"))
+      void 'should sort environments by name and return all configured environments'() {
+        def prodEnv = new BasicEnvironmentConfig(new CaseInsensitiveString("prod"))
+        prodEnv.addAgent("agent1")
+        prodEnv.addAgent("agent2")
+        prodEnv.addEnvironmentVariable("JAVA_HOME", "/bin/java")
+        prodEnv.addPipeline(new CaseInsensitiveString("Pipeline1"))
+        prodEnv.addPipeline(new CaseInsensitiveString("Pipeline2"))
 
-        def env2 = new BasicEnvironmentConfig(new CaseInsensitiveString("env2"))
+        def devEnv = new BasicEnvironmentConfig(new CaseInsensitiveString("dev"))
+        def qaEnv = new BasicEnvironmentConfig(new CaseInsensitiveString("qa"))
 
-        def listOfEnvironmentConfigs = [env1, env2]
+        def listOfEnvironmentConfigs = new HashSet([qaEnv, devEnv, prodEnv])
 
-        when(environmentConfigService.getAllMergedEnvironments()).thenReturn(listOfEnvironmentConfigs)
+        when(environmentConfigService.getEnvironments()).thenReturn(listOfEnvironmentConfigs)
 
         getWithApiHeader(controller.controllerBasePath())
-
+        
+        def environmentsConfigSortedByName = new LinkedHashSet([devEnv, prodEnv, qaEnv])
         assertThatResponse()
-          .hasBodyWithJsonObject(listOfEnvironmentConfigs, EnvironmentsRepresenter)
+          .hasBodyWithJsonObject(environmentsConfigSortedByName, EnvironmentsRepresenter)
       }
 
 
@@ -553,7 +544,7 @@ class EnvironmentsControllerV2Test implements SecurityServiceTrait, ControllerTr
           ]
         ]
 
-        when(environmentConfigService.getAllMergedEnvironments()).thenReturn([])
+        when(environmentConfigService.getEnvironments()).thenReturn(new HashSet<>([]))
 
         getWithApiHeader(controller.controllerBasePath())
 

api/api-environments-v2/src/test/groovy/com/thoughtworks/go/apiv2/environments/representers/AgentRepresenterTest.groovy

@@ -33,7 +33,7 @@ class AgentRepresenterTest {
 
     assertThatJson(json).isEqualTo([
       "_links": [
-        "doc": [
+        "doc" : [
           "href": apiDocsUrl("#agents")
         ],
         "find": [
@@ -43,7 +43,7 @@ class AgentRepresenterTest {
           "href": "http://test.host/go/api/agents/agent1"
         ]
       ],
-      "uuid": "agent1"
+      "uuid"  : "agent1"
     ])
   }
 }