Added tests for update package definition call.

Incooperated review comments.

api/api-packages-v1/src/main/java/com/thoughtworks/go/apiv1/packages/PackagesControllerV1.java

@@ -74,10 +74,10 @@ public void setupRoutes() {
             before("/*", mimeType, this.apiAuthenticationHelper::checkAdminUserOrGroupAdminUserAnd403);
 
             get("", mimeType, this::index);
-            get(Routes.Packages.SHOW, mimeType, this::show);
+            get(Routes.Packages.PACKAGE_ID, mimeType, this::show);
             post("", mimeType, this::create);
-            put(Routes.Packages.SHOW, mimeType, this::update);
-            delete(Routes.Packages.SHOW, mimeType, this::remove);
+            put(Routes.Packages.PACKAGE_ID, mimeType, this::update);
+            delete(Routes.Packages.PACKAGE_ID, mimeType, this::remove);
         });
     }
 

api/api-packages-v1/src/test/groovy/com/thoughtworks/go/apiv1/packages/PackagesControllerV1Test.groovy

@@ -36,6 +36,7 @@ import org.mockito.invocation.InvocationOnMock
 
 import static com.thoughtworks.go.api.base.JsonUtils.toObjectString
 import static org.mockito.ArgumentMatchers.any
+import static org.mockito.ArgumentMatchers.anyString
 import static org.mockito.ArgumentMatchers.eq
 import static org.mockito.Mockito.when
 import static org.mockito.MockitoAnnotations.initMocks
@@ -283,6 +284,141 @@ class PackagesControllerV1Test implements SecurityServiceTrait, ControllerTrait<
     }
   }
 
+  @Nested
+  class Update {
+
+    @Nested
+    class Security implements SecurityTestTrait, GroupAdminUserSecurity {
+
+      @Override
+      String getControllerMethodUnderTest() {
+        return "update"
+      }
+
+      @Override
+      void makeHttpCall() {
+        putWithApiHeader(controller.controllerPath('foo'), [])
+      }
+    }
+
+    @Nested
+    class AsAdmin {
+      @BeforeEach
+      void setUp() {
+        enableSecurity()
+        loginAsAdmin()
+      }
+
+      @Test
+      void 'should update the package definition as part of update call'() {
+        def configuration = new Configuration(ConfigurationPropertyMother.create('key', 'value'))
+        def packageDefinition = PackageDefinitionMother.create('id', 'name', configuration, PackageRepositoryMother.create('repo-id'))
+
+        def updatedConfiguration = new Configuration(ConfigurationPropertyMother.create('updated-key', 'updated-value'))
+        def updatedPackageDefinition = PackageDefinitionMother.create('id', 'updated-name', updatedConfiguration, PackageRepositoryMother.create('updated-repo-id'))
+
+        def json = toObjectString({ PackageDefinitionRepresenter.toJSON(it, updatedPackageDefinition) })
+
+        when(entityHashingService.md5ForEntity(packageDefinition)).thenReturn('etag')
+        when(entityHashingService.md5ForEntity(updatedPackageDefinition)).thenReturn('updated-etag')
+        when(packageDefinitionService.find('id')).thenReturn(packageDefinition)
+        when(packageDefinitionService.updatePackage(eq('id'), eq(updatedPackageDefinition), eq('etag'), eq(currentUsername()), any(HttpLocalizedOperationResult.class))).then({
+          InvocationOnMock invocation ->
+            HttpLocalizedOperationResult result = (HttpLocalizedOperationResult) invocation.arguments.last()
+            result.setMessage("Package definition was updated successfully")
+        })
+
+        putWithApiHeader(controller.controllerPath('id'), ['if-match': 'etag'], json)
+
+        assertThatResponse()
+          .isOk()
+          .hasEtag('"updated-etag"')
+          .hasBodyWithJson(json)
+      }
+
+      @Test
+      void 'should not update the package definition if etag does not match'() {
+        def configuration = new Configuration(ConfigurationPropertyMother.create('key', 'value'))
+        def packageDefinition = PackageDefinitionMother.create('id', 'name', configuration, PackageRepositoryMother.create('repo-id'))
+
+        def updatedConfiguration = new Configuration(ConfigurationPropertyMother.create('updated-key', 'updated-value'))
+        def updatedPackageDefinition = PackageDefinitionMother.create('id', 'updated-name', updatedConfiguration, PackageRepositoryMother.create('updated-repo-id'))
+
+        def json = toObjectString({ PackageDefinitionRepresenter.toJSON(it, updatedPackageDefinition) })
+
+        when(entityHashingService.md5ForEntity(packageDefinition)).thenReturn('etag')
+        when(entityHashingService.md5ForEntity(updatedPackageDefinition)).thenReturn('updated-etag')
+        when(packageDefinitionService.find('id')).thenReturn(packageDefinition)
+
+        putWithApiHeader(controller.controllerPath('id'), ['if-match': 'unknown-etag'], json)
+
+        assertThatResponse()
+          .isPreconditionFailed()
+          .hasJsonMessage("Someone has modified the configuration for packageDefinition 'id'. Please update your copy of the config with the changes and try again.")
+      }
+
+
+      @Test
+      void 'should error out if there are errors in parsing package definition config'() {
+        def expectedResponse = [
+          "message": "Validation error.",
+          "data"   : [
+            "name"         : "updated-name",
+            "id"           : "id",
+            "auto_update"  : true,
+            "package_repo" : [
+              "id"  : "updated-repo-id",
+              "name": "repo-updated-repo-id"
+            ],
+            "configuration": [
+              [
+                "key"  : "updated-key",
+                "value": "updated-value"
+              ]
+            ]
+          ]
+        ]
+
+        def configuration = new Configuration(ConfigurationPropertyMother.create('key', 'value'))
+        def packageDefinition = PackageDefinitionMother.create('id', 'name', configuration, PackageRepositoryMother.create('repo-id'))
+
+        def updatedConfiguration = new Configuration(ConfigurationPropertyMother.create('updated-key', 'updated-value'))
+        def updatedPackageDefinition = PackageDefinitionMother.create('id', 'updated-name', updatedConfiguration, PackageRepositoryMother.create('updated-repo-id'))
+
+        def json = toObjectString({ PackageDefinitionRepresenter.toJSON(it, updatedPackageDefinition) })
+
+        when(packageDefinitionService.find('id')).thenReturn(packageDefinition)
+        when(entityHashingService.md5ForEntity(packageDefinition)).thenReturn('etag')
+        when(entityHashingService.md5ForEntity(updatedPackageDefinition)).thenReturn('updated-etag')
+        when(packageDefinitionService.updatePackage(anyString(), any(), any(), any(), any())).then({
+          InvocationOnMock invocation ->
+            HttpLocalizedOperationResult result = (HttpLocalizedOperationResult) invocation.arguments.last()
+            result.unprocessableEntity("Validation error.")
+        })
+
+        putWithApiHeader(controller.controllerPath('id'), ['if-match': 'etag'], json)
+
+        assertThatResponse()
+          .isUnprocessableEntity()
+          .hasJsonBody(expectedResponse)
+      }
+
+      @Test
+      void 'should return 404 if entity not found'() {
+        def configuration = new Configuration(ConfigurationPropertyMother.create('key', 'value'))
+        def packageDefinition = PackageDefinitionMother.create('id', 'name', configuration, PackageRepositoryMother.create('repo-id'))
+
+        def json = toObjectString({ PackageDefinitionRepresenter.toJSON(it, packageDefinition) })
+
+        putWithApiHeader(controller.controllerPath('id'), ['if-match': 'etag'], json)
+
+        assertThatResponse()
+          .isNotFound()
+          .hasJsonMessage(controller.entityType.notFoundMessage("id"))
+      }
+    }
+  }
+
   @Nested
   class Remove {
 

api/api-packages-v1/src/test/groovy/com/thoughtworks/go/apiv1/packages/representers/PackageDefinitionRepresenterTest.groovy

@@ -16,20 +16,20 @@
 
 package com.thoughtworks.go.apiv1.packages.representers
 
-import com.thoughtworks.go.api.base.OutputWriter
 import com.thoughtworks.go.api.util.GsonTransformer
 import com.thoughtworks.go.domain.config.Configuration
 import com.thoughtworks.go.domain.config.ConfigurationKey
 import com.thoughtworks.go.domain.config.ConfigurationProperty
 import com.thoughtworks.go.domain.config.ConfigurationValue
+import com.thoughtworks.go.domain.config.PluginConfiguration
 import com.thoughtworks.go.domain.packagerepository.PackageDefinition
 import com.thoughtworks.go.domain.packagerepository.PackageRepository
 import org.junit.jupiter.api.Test
 
 import static com.thoughtworks.go.CurrentGoCDVersion.apiDocsUrl
 import static com.thoughtworks.go.api.base.JsonUtils.toObjectString
 import static net.javacrumbs.jsonunit.fluent.JsonFluentAssert.assertThatJson
-import static org.junit.jupiter.api.Assertions.*
+import static org.junit.jupiter.api.Assertions.assertEquals
 
 class PackageDefinitionRepresenterTest {
 
@@ -72,23 +72,21 @@ class PackageDefinitionRepresenterTest {
   ]
 
   @Test
-  void 'should serialize package definition'() {
+  void 'should deserialize package definition'() {
     def jsonReader = GsonTransformer.instance.jsonReaderFrom(expectedJson)
     def packageDefinition = PackageDefinitionRepresenter.fromJSON(jsonReader)
 
     def configurationProperties = new Configuration(new ConfigurationProperty(new ConfigurationKey('PACKAGE_NAME'), new ConfigurationValue('foo')))
     def expectedPackageDefinition = new PackageDefinition('package-id-1', 'package-1', configurationProperties)
-    expectedPackageDefinition.setRepository(new PackageRepository('package-repo-id-1', 'package-repo-name-1', null, null))
+    expectedPackageDefinition.setRepository(new PackageRepository('package-repo-id-1', 'package-repo-name-1', new PluginConfiguration(), new Configuration()))
 
-    assertEquals(expectedPackageDefinition.name, packageDefinition.name)
-    assertEquals(expectedPackageDefinition.id, packageDefinition.id)
-    assertEquals(expectedPackageDefinition.configuration, packageDefinition.configuration)
-    assertEquals(expectedPackageDefinition.repository.id, packageDefinition.repository.id)
-    assertEquals(expectedPackageDefinition.repository.name, packageDefinition.repository.name)
+    assertEquals(expectedPackageDefinition, packageDefinition)
+    // separate assert is required as PackageDefinition.equals does not compare 'repository'
+    assertEquals(expectedPackageDefinition.repository, packageDefinition.repository)
   }
 
   @Test
-  void 'should deserialize package definition'() {
+  void 'should serialize package definition'() {
     def configurationProperties = new Configuration(new ConfigurationProperty(new ConfigurationKey('PACKAGE_NAME'), new ConfigurationValue('foo')))
     def packageDefinition = new PackageDefinition('package-id-1', 'package-1', configurationProperties)
     packageDefinition.setRepository(new PackageRepository('package-repo-id-1', 'package-repo-name-1', null, null))

api/api-packages-v1/src/test/groovy/com/thoughtworks/go/apiv1/packages/representers/PackageRepositoryRepresenterTest.groovy

@@ -44,7 +44,7 @@ class PackageRepositoryRepresenterTest {
   ]
 
   @Test
-  void 'should serialize package repository'() {
+  void 'should deserialize package repository'() {
     def jsonReader = GsonTransformer.instance.jsonReaderFrom(expectedJson)
     def packageRepository = PackageRepositoryRepresenter.fromJSON(jsonReader)
 
@@ -53,7 +53,7 @@ class PackageRepositoryRepresenterTest {
   }
 
   @Test
-  void 'should deserialize package repository'() {
+  void 'should serialize package repository'() {
     def packageRepository = new PackageRepository('package-repo-id-1', 'package-repo-name-1', null, null)
     def actualJson = toObjectString({ PackageRepositoryRepresenter.toJSON(it, packageRepository) })
 

spark/spark-base/src/main/java/com/thoughtworks/go/spark/Routes.java

@@ -739,8 +739,8 @@ public static class CompareAPI {
     public static class Packages {
         public static final String BASE = "/api/admin/packages";
         public static final String DOC = apiDocsUrl("packages");
-        public static final String SHOW = "/:package_id";
-        public static final String FIND = BASE + SHOW;
+        public static final String PACKAGE_ID = "/:package_id";
+        public static final String FIND = BASE + PACKAGE_ID;
 
         public static String self(String id) {
             return BASE + "/" + id;