From b57eef508410668377a1dd4d89b3c8f03d00872e Mon Sep 17 00:00:00 2001 From: Roy Clarkson Date: Wed, 11 Nov 2020 17:33:54 -0500 Subject: [PATCH] Return HTTP 400 Bad Request for unknown instance in last operation v2.15 of the OSB API spec is a little unclear about how to handle the scenario where a request is made for the last operation of an instance id that does not exist and never existed. It does specify a 400 Bad Request for "malformed or missing mandatory data". In this case an unknown instance id is provided, so it's the best option for response code. The spec is improved in v2.16. There, it adds a 404 Not Found for this scenario. We'll change this when we move to the newer spec. Resolves #306 --- ...tServiceInstanceControllerIntegrationTest.java | 5 +++++ .../ServiceInstanceControllerIntegrationTest.java | 15 +++++++++++++++ .../ServiceInstanceControllerIntegrationTest.java | 15 +++++++++++++++ .../controller/ServiceInstanceController.java | 13 ++++++++++++- ...ServiceInstanceControllerResponseCodeTest.java | 14 ++++++++++++++ 5 files changed, 61 insertions(+), 1 deletion(-) diff --git a/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/AbstractServiceInstanceControllerIntegrationTest.java b/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/AbstractServiceInstanceControllerIntegrationTest.java index 426b38ed..548b0ac6 100644 --- a/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/AbstractServiceInstanceControllerIntegrationTest.java +++ b/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/AbstractServiceInstanceControllerIntegrationTest.java @@ -125,6 +125,11 @@ protected void setupServiceInstanceService(ServiceInstanceDoesNotExistException .willReturn(Mono.error(exception)); } + protected void setupServiceInstanceServiceLastOperation(ServiceInstanceDoesNotExistException exception) { + given(serviceInstanceService.getLastOperation(any(GetLastServiceOperationRequest.class))) + .willReturn(Mono.error(exception)); + } + protected void setupServiceInstanceService(UpdateServiceInstanceResponse response) { given(serviceInstanceService.updateServiceInstance(any(UpdateServiceInstanceRequest.class))) .willReturn(Mono.just(response)); diff --git a/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/reactive/ServiceInstanceControllerIntegrationTest.java b/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/reactive/ServiceInstanceControllerIntegrationTest.java index b5445a43..853f10a4 100644 --- a/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/reactive/ServiceInstanceControllerIntegrationTest.java +++ b/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/reactive/ServiceInstanceControllerIntegrationTest.java @@ -53,6 +53,7 @@ import org.springframework.test.web.reactive.server.WebTestClient; import static org.assertj.core.api.Assertions.assertThat; +import static org.hamcrest.core.StringContains.containsString; import static org.springframework.cloud.servicebroker.exception.ServiceBrokerAsyncRequiredException.ASYNC_REQUIRED_ERROR; import static org.springframework.cloud.servicebroker.exception.ServiceBrokerMaintenanceInfoConflictException.MAINTENANCE_INFO_CONFLICT_ERROR; import static org.springframework.cloud.servicebroker.exception.ServiceBrokerMaintenanceInfoConflictException.MAINTENANCE_INFO_CONFLICT_MESSAGE; @@ -787,6 +788,20 @@ void lastOperationHasSucceededStatusWithDeletionComplete() { .jsonPath("$.description").isEqualTo("all gone"); } + @Test + void lastOperationWithUnknownInstanceBadRequest() { + setupServiceInstanceServiceLastOperation(new ServiceInstanceDoesNotExistException("nonexistent-instance-id")); + + client.get().uri(buildLastOperationUrl()) + .accept(MediaType.APPLICATION_JSON) + .exchange() + .expectStatus().is4xxClientError() + .expectStatus().isEqualTo(HttpStatus.BAD_REQUEST) + .expectBody() + .jsonPath("$.state").doesNotExist() + .jsonPath("$.description").value(containsString("The requested Service Instance does not exist")); + } + @Test void lastOperationHasFailedStatus() { setupServiceInstanceService(GetLastServiceOperationResponse.builder() diff --git a/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/servlet/ServiceInstanceControllerIntegrationTest.java b/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/servlet/ServiceInstanceControllerIntegrationTest.java index c619e65a..e38e5bf7 100644 --- a/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/servlet/ServiceInstanceControllerIntegrationTest.java +++ b/spring-cloud-open-service-broker-autoconfigure/src/test/java/org/springframework/cloud/servicebroker/autoconfigure/web/servlet/ServiceInstanceControllerIntegrationTest.java @@ -861,6 +861,21 @@ void lastOperationHasSucceededStatusWithDeletionComplete() throws Exception { .andExpect(jsonPath("$.description", is("all gone"))); } + @Test + void lastOperationWithUnknownInstanceBadRequest() throws Exception { + setupServiceInstanceServiceLastOperation(new ServiceInstanceDoesNotExistException("nonexistent-instance-id")); + + MvcResult mvcResult = mockMvc.perform(get(buildLastOperationUrl())) + .andExpect(request().asyncStarted()) + .andReturn(); + + mockMvc.perform(asyncDispatch(mvcResult)) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.state").doesNotExist()) + .andExpect(jsonPath("$.description", + containsString("The requested Service Instance does not exist"))); + } + @Test void lastOperationHasFailedStatus() throws Exception { setupServiceInstanceService(GetLastServiceOperationResponse.builder() diff --git a/spring-cloud-open-service-broker-core/src/main/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceController.java b/spring-cloud-open-service-broker-core/src/main/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceController.java index 53e32d33..a77143bd 100644 --- a/spring-cloud-open-service-broker-core/src/main/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceController.java +++ b/spring-cloud-open-service-broker-core/src/main/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceController.java @@ -241,9 +241,20 @@ public Mono> getServiceInstanceL .doOnError(e -> LOG.error("Error getting service instance last operation. error=" + e.getMessage(), e))) .map(response -> { - boolean isSuccessfulDelete = response.getState().equals(OperationState.SUCCEEDED) && response + boolean isSuccessfulDelete = OperationState.SUCCEEDED.equals(response.getState()) && response .isDeleteOperation(); return new ResponseEntity<>(response, isSuccessfulDelete ? HttpStatus.GONE : HttpStatus.OK); + }) + .onErrorResume(e -> { + if (e instanceof ServiceInstanceDoesNotExistException) { + // TODO: v2.16 of the OSB API spec changes this to an HTTP 404 + return Mono.just(new ResponseEntity<>(GetLastServiceOperationResponse.builder() + .description("The requested Service Instance does not exist") + .build(), HttpStatus.BAD_REQUEST)); + } + else { + return Mono.error(e); + } }); } diff --git a/spring-cloud-open-service-broker-core/src/test/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceControllerResponseCodeTest.java b/spring-cloud-open-service-broker-core/src/test/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceControllerResponseCodeTest.java index 5de184b3..be591450 100755 --- a/spring-cloud-open-service-broker-core/src/test/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceControllerResponseCodeTest.java +++ b/spring-cloud-open-service-broker-core/src/test/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceControllerResponseCodeTest.java @@ -313,6 +313,20 @@ void getLastOperationWithDeleteSucceededResponseGivesExpectedStatus() { .build(), HttpStatus.GONE); } + @Test + void getLastOperationWithUnknownInstanceBadRequest() { + given(serviceInstanceService.getLastOperation(any(GetLastServiceOperationRequest.class))) + .willReturn(Mono.error(new ServiceInstanceDoesNotExistException("instance never existed"))); + + ResponseEntity responseEntity = controller + .getServiceInstanceLastOperation(pathVariables, null, "service-definition-id", + "service-definition-plan-id", null, null, null, null) + .block(); + + assertThat(responseEntity).isNotNull(); + assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); + } + @Test void getLastOperationWithFailedResponseGivesExpectedStatus() { validateGetLastOperationWithResponseStatus(GetLastServiceOperationResponse.builder()