From 5c8e0051f22708b4a7f7b235a7666214e70f60ec Mon Sep 17 00:00:00 2001 From: Daniel Kavan Date: Wed, 23 Mar 2022 09:42:30 +0100 Subject: [PATCH] #1693 API v3: VersionedModelControllerV3 - put to yield no content -> IT updated --- .../controllers/RestExceptionHandler.scala | 5 ++ .../v3/VersionedModelControllerV3.scala | 30 ++++++---- .../DatasetControllerV3IntegrationSuite.scala | 55 ++++++++++++------- 3 files changed, 60 insertions(+), 30 deletions(-) diff --git a/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/RestExceptionHandler.scala b/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/RestExceptionHandler.scala index 5fa14a03b..5ef13e650 100644 --- a/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/RestExceptionHandler.scala +++ b/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/RestExceptionHandler.scala @@ -44,6 +44,11 @@ class RestExceptionHandler { private val logger = LoggerFactory.getLogger(this.getClass) + @ExceptionHandler(value = Array(classOf[IllegalArgumentException])) + def handleIllegalArgumentException(exception: IllegalArgumentException): ResponseEntity[Any] = { + ResponseEntity.badRequest().body(exception.getMessage) + } + @ExceptionHandler(value = Array(classOf[AsyncRequestTimeoutException])) def handleAsyncRequestTimeoutException(exception: AsyncRequestTimeoutException): ResponseEntity[Any] = { val message = Option(exception.getMessage).getOrElse("Request timeout expired.") diff --git a/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/VersionedModelControllerV3.scala b/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/VersionedModelControllerV3.scala index fee8584a9..a07872178 100644 --- a/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/VersionedModelControllerV3.scala +++ b/rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/VersionedModelControllerV3.scala @@ -32,6 +32,7 @@ import java.net.URI import java.util.Optional import java.util.concurrent.CompletableFuture import javax.servlet.http.HttpServletRequest +import scala.concurrent.Future abstract class VersionedModelControllerV3[C <: VersionedModel with Product with Auditable[C]](versionedModelService: VersionedModelService[C]) extends BaseController { @@ -71,7 +72,7 @@ abstract class VersionedModelControllerV3[C <: VersionedModel with Product def getLatestDetail(@PathVariable name: String): CompletableFuture[C] = { versionedModelService.getLatestVersion(name).map { case Some(entity) => entity - case None => throw NotFoundException() + case None => throw NotFoundException() } } @@ -84,7 +85,7 @@ abstract class VersionedModelControllerV3[C <: VersionedModel with Product @GetMapping(Array("/{name}/{version}/used-in")) @ResponseStatus(HttpStatus.OK) def usedIn(@PathVariable name: String, - @PathVariable version: Int): CompletableFuture[UsedIn] = { + @PathVariable version: Int): CompletableFuture[UsedIn] = { versionedModelService.getUsedIn(name, Some(version)) } @@ -108,7 +109,7 @@ abstract class VersionedModelControllerV3[C <: VersionedModel with Product // todo check that the name pathVar and object conform versionedModelService.importSingleItem(importObject.item, principal.getUsername, importObject.metadata).map { case Some(entity) => entity // todo redo to have header Location present - case None => throw notFound() + case None => throw notFound() } } @@ -136,20 +137,29 @@ abstract class VersionedModelControllerV3[C <: VersionedModel with Product } } - @PutMapping(Array("")) - @ResponseStatus(HttpStatus.OK) + @PutMapping(Array("/{name}/{version}")) + @ResponseStatus(HttpStatus.NO_CONTENT) def edit(@AuthenticationPrincipal user: UserDetails, - @RequestBody item: C): CompletableFuture[C] = { - versionedModelService.update(user.getUsername, item).map { - case Some(entity) => entity // todo change not to return content - case None => throw notFound() + @PathVariable name: String, + @PathVariable version: Int, + @RequestBody item: C): CompletableFuture[ResponseEntity[Nothing]] = { + + if (name != item.name) { + Future.failed(new IllegalArgumentException(s"URL and payload entity name mismatch: '$name' != '${item.name}'")) + } else if (version != item.version) { + Future.failed(new IllegalArgumentException(s"URL and payload version mismatch: ${version} != ${item.version}")) + } else { + versionedModelService.update(user.getUsername, item).map { + case Some(entity) => ResponseEntity.noContent().build() + case None => throw notFound() + } } } @DeleteMapping(Array("/{name}", "/{name}/{version}")) @ResponseStatus(HttpStatus.OK) def disable(@PathVariable name: String, - @PathVariable version: Optional[String]): CompletableFuture[UpdateResult] = { + @PathVariable version: Optional[String]): CompletableFuture[UpdateResult] = { val v = if (version.isPresent) { // For some reason Spring reads the Optional[Int] param as a Optional[String] and then throws ClassCastException Some(version.get.toInt) diff --git a/rest-api/src/test/scala/za/co/absa/enceladus/rest_api/integration/controllers/v3/DatasetControllerV3IntegrationSuite.scala b/rest-api/src/test/scala/za/co/absa/enceladus/rest_api/integration/controllers/v3/DatasetControllerV3IntegrationSuite.scala index a45fde450..eee12729e 100644 --- a/rest-api/src/test/scala/za/co/absa/enceladus/rest_api/integration/controllers/v3/DatasetControllerV3IntegrationSuite.scala +++ b/rest-api/src/test/scala/za/co/absa/enceladus/rest_api/integration/controllers/v3/DatasetControllerV3IntegrationSuite.scala @@ -20,6 +20,7 @@ import org.scalatest.BeforeAndAfterAll import org.scalatest.matchers.should.Matchers import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.test.context.SpringBootTest +import org.springframework.http.HttpStatus import org.springframework.test.context.ActiveProfiles import org.springframework.test.context.junit4.SpringRunner import za.co.absa.enceladus.model.conformanceRule.MappingConformanceRule @@ -109,7 +110,7 @@ class DatasetControllerV3IntegrationSuite extends BaseRestApiTestV3 with BeforeA datasetFixture.add(dataset1, dataset2) val dataset3 = DatasetFactory.getDummyDataset("dummyDs", version = 7) // version is ignored for create - val response = sendPost[Dataset, Dataset](apiUrl, bodyOpt = Some(dataset3)) + val response = sendPost[Dataset, String](apiUrl, bodyOpt = Some(dataset3)) assertCreated(response) val locationHeaders = response.getHeaders.get("location").asScala locationHeaders should have size 1 @@ -129,11 +130,14 @@ class DatasetControllerV3IntegrationSuite extends BaseRestApiTestV3 with BeforeA s"PUT $apiUrl" can { "return 200" when { - "a Schema with the given name and version is the latest that exists" should { - "return the updated Schema (with empty properties stripped)" in { + "a Dataset with the given name and version is the latest that exists" should { + "update the dataset (with empty properties stripped)" in { val datasetA1 = DatasetFactory.getDummyDataset("datasetA", description = Some("init version"), properties = Some(Map("keyA" -> "valA"))) - datasetFixture.add(datasetA1) + val datasetA2 = DatasetFactory.getDummyDataset("datasetA", + description = Some("second version"), properties = Some(Map("keyA" -> "valA")), version = 2) + + datasetFixture.add(datasetA1, datasetA2) val exampleMappingCr = MappingConformanceRule(0, controlCheckpoint = true, @@ -156,33 +160,44 @@ class DatasetControllerV3IntegrationSuite extends BaseRestApiTestV3 with BeforeA overrideMappingTableOwnFilter = Some(true) ) - val datasetA2 = DatasetFactory.getDummyDataset("datasetA", + val datasetA3 = DatasetFactory.getDummyDataset("datasetA", description = Some("updated"), properties = Some(Map("keyA" -> "valA", "keyB" -> "valB", "keyC" -> "")), - conformance = List(exampleMappingCr) + conformance = List(exampleMappingCr), + version = 2 // update references the last version ) - val response = sendPut[Dataset, Dataset](s"$apiUrl", bodyOpt = Some(datasetA2)) - assertOk(response) + val response = sendPut[Dataset, String](s"$apiUrl/datasetA/2", bodyOpt = Some(datasetA3)) + response.getStatusCode shouldBe HttpStatus.NO_CONTENT - // todo should be ok/no content and then actually no content -> run get again - val actual = response.getBody - val expectedDs = DatasetFactory.getDummyDataset( - name = "datasetA", - version = 2, - description = Some("updated"), - parent = Some(DatasetFactory.toParent(datasetA1)), - properties = Some(Map("keyA" -> "valA", "keyB" -> "valB")), - conformance = List(exampleMappingCr) - ) - val expected = toExpected(expectedDs, actual) + val response2 = sendGet[Dataset](s"$apiUrl/datasetA/3") // next version + assertOk(response2) + val actual = response2.getBody + val expected = toExpected(datasetA3.copy(version = 3, parent = Some(DatasetFactory.toParent(datasetA2)), properties = Some(Map("keyA" -> "valA", "keyB" -> "valB"))), actual) // blank property stripped assert(actual == expected) } } } - } + "return 405" when { + "a Dataset with the given name and version" should { + "fail when version/name in the URL and payload is mismatched" in { + val datasetA1 = DatasetFactory.getDummyDataset("datasetA", description = Some("init version")) + datasetFixture.add(datasetA1) + + val response = sendPut[Dataset, String](s"$apiUrl/datasetA/7", + bodyOpt = Some(DatasetFactory.getDummyDataset("datasetA", version = 5))) + response.getStatusCode shouldBe HttpStatus.BAD_REQUEST + response.getBody should include("version mismatch: 7 != 5") + val response2 = sendPut[Dataset, String](s"$apiUrl/datasetABC/4", + bodyOpt = Some(DatasetFactory.getDummyDataset("datasetXYZ", version = 4))) + response2.getStatusCode shouldBe HttpStatus.BAD_REQUEST + response2.getBody should include("name mismatch: 'datasetABC' != 'datasetXYZ'") + } + } + } + } s"GET $apiUrl/{name}/{version}/export" should { "return 404" when {