polling last operation with throwing ServiceInstanceDoesNotExistException returns 422 instead of 404

andrejb-dev commented 4 years ago

I have implemented the ServiceInstanceService interface. My getLastOperation pseudocode looks like this (of course wrapped in mono):

return getDeployment(serviceId, planId, instanceId)
        .map(deployment ->
                taskClient.getTask(deployment.operationId())
        )
        .map(taskMapper::mapTaskToLastServiceOperationResponse)
        .orElseThrow(() -> new ServiceInstanceDoesNotExistException(instanceId));

If the instance, of wich last operation is questioned, does not exists (failed to create when provisioning), I throw an ServiceInstanceDoesNotExistException according to guidance in comments inside interface for this method. Unfortunately, the spring osb returns in such case 422 http status and the platform does not cease polling getLastOperation. By the OSB API specification, 404 or 410 should be returned. I was unsuccesfull to find the reason for that status, but I think somewhere below these lines: https://github.com/spring-cloud/spring-cloud-open-service-broker/blob/fd1a6db9717d35bf4df851db998866064d86ab22/spring-cloud-open-service-broker-core/src/main/java/org/springframework/cloud/servicebroker/controller/ServiceInstanceController.java#L243-L247

Or did I understand it wrongly?

royclarkson commented 4 years ago

Hi @Zavael, thanks for the interest in the project. If I understand correctly, you are polling the last operation for a delete operation. It appears that we aren't handling ServiceInstanceDoesNotExistException properly in that case. Good catch!

For more context, we have an exception handler that processes all thrown exceptions within the application. You can see at the following link that we default to HTTP 422 Unprocessable Entity for a ServiceInstanceDoesNotExistException. The reason for this is that depending on which API you are exercising, the spec defines more general behavior:

https://github.com/spring-cloud/spring-cloud-open-service-broker/blob/master/spring-cloud-open-service-broker-core/src/main/java/org/springframework/cloud/servicebroker/controller/ServiceBrokerExceptionHandler.java#L92-L102

royclarkson commented 4 years ago

This is an interesting situation because the 2.15 spec does not define how to handle this scenario well. An HTTP 410 Gone status isn't really correct if the service instance Id never existed. Additionally the last operation API requires a status of failed or succeeded, which also don't apply in this case.

The other reason this is interesting is that this scenario is fixed in the 2.16 spec, where they added a response of HTTP 404 not found, which "MUST be returned if the Service Instance being polled does not exist".

The current version of Spring Cloud Open Service Broker is supporting v2.15 of the OSB API spec, so we'll work around it for now and return a 400 if we encounter a ServiceInstanceDoesNotExistException. The spec says HTTP 400 bad request, "MUST be returned if the request is malformed or missing mandatory data.

andrejb-dev commented 4 years ago

@royclarkson thank you for the explanation and quick fix

spring-cloud / spring-cloud-open-service-broker

polling last operation with throwing ServiceInstanceDoesNotExistException returns 422 instead of 404 #306