openservicebrokerapi / servicebroker

Open Service Broker API Specification
https://openservicebrokerapi.org/
Apache License 2.0
1.19k stars 434 forks source link

Updating a service instance should allow the "parameters" to be updated to "nil" #352

Closed maqiuyujoyce closed 5 years ago

maqiuyujoyce commented 7 years ago

In a service instance, if the parameters are optional, the "parameters" field can either be existent (a valid JSON object) or not (nil/null). So we should be able to update the "parameters" field to nil/null. However, the spec for the update request body said that:

Request field Type Description
parameters JSON object Configuration options for the service instance. Brokers SHOULD ensure that the client has provided valid configuration parameters and values for the operation. If this field is not present in the request message, then the broker MUST NOT change the parameters of the instance as a result of this request.

Basically, we can't clean up the "parameters" field. This looks like a problem. The request should be able to represent three states of the "parameters" update: no change, change to a JSON object, change to nil/null.

kibbles-n-bytes commented 7 years ago

I think we should define providing no parameters to a provision call as being functionally equivalent to passing the following:

{
  ...
  "parameters": {}
  ...
}

That way, we get the normal default provision behavior when leaving them out, but during an update we can be explicit and pass the above payload to clean out all parameters.

duglin commented 7 years ago

Right, for provision there's only two states for params: 1 - no params at all (no parameters property, or parameters property w/o any nested properties) 2 - one or more params (parameters) property with at least one nest property)

But for update we have 3: 1 - no params property == no update to existing params 2 - params property w/o any nested properties == clear all existing params 3 - params property with at least one nested property == update the nested properties but erase all others. Note that 2 and 3 are the same, just being a bit more explicit about it.

And I think this is backwards compatible.

mattmcneeney commented 7 years ago

I agree with this! Does someone want to draft a PR?

duglin commented 7 years ago

Let's discuss this on the call today to see if everyone is in agreement on the desired semantics.

duglin commented 7 years ago

Related to https://github.com/openservicebrokerapi/servicebroker/issues/350

If we try to define what it means to only include a subset of the parameters then we may break existing brokers. Even though I agree the spec really should be clear on this point.

Perhaps we could query the existing CF broker community to see how they interpret it?

duglin commented 6 years ago

If we adopt #365 then perhaps we can solve this issue by adding something like this after the proposed text in that PR: = = = = = = It is RECOMMENDED that Service Brokers support the ability to "unset" parameters via a JSON value of null. If supported, specifying a value of null for a specific parameter indicates that that parameter MUST be reset to its default value. Specifying a value of null for the parameters field itself MUST be semantically equivalent to specifying null for all possible parameters. If a Service Broker does not support this feature then it MUST reject any request that contains a null value with a 422 Unprocessable Entity response. = = = = = =

duglin commented 6 years ago

Round 2 based on last weeks' chat: = = = = = = It is RECOMMENDED that Service Brokers support the ability to "unset" parameters via a JSON value of null. If supported, specifying a value of null for a specific parameter indicates that that parameter MUST be reset to its default value. Specifying a value of null for the parameters field itself MUST be semantically equivalent to specifying null for all possible parameters. If a Service Broker does not support this feature then it MUST reject any request that contains a null value with a 422 Unprocessable Entity response.

This specification makes no statement as to how a Service Broker is to interpret a parameter that is not fully specified in the request. In other words, whether it treats a partially specified parameter as a complete replacement or an update to just those values specified, is out of scope of this specification. = = = = = = What do people think?

fmui commented 5 years ago

@maqiuyujoyce , are you still interested in this topic or can we close it?

maqiuyujoyce commented 5 years ago

Feel free to close it.