openservicebrokerapi / servicebroker

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

Additional fields for schema in Catalog (/v2/catalog) #484

Closed DevasiaThomas closed 5 years ago

DevasiaThomas commented 6 years ago

Can we have additional fields that can help extend schema interpretation? Fields like additionalProperties(bool), optional(bool), label(string). I have given an example usage below.

The reason i as this to be included in the examples that you provide, is because platforms blindly implement what is in the example that is provided in the OSB specification. additonalProperties is something that is part of the JSON Schema specification, but is not honored by Cloud Foundry(I don't know about others) implementations. The other two (optional and label) are useful keywords if put in the example can be picked up by platforms.

"schemas": {
        "service_instance": {
          "create": {
            "parameters": {
              "$schema": "http://json-schema.org/draft-04/schema#",
              "type": "object",
              **"additonalProperties": "true",**
              "properties": {
                "billing-account": {
                  **"label": "Billing Account",**
                  "description": "Billing account number used to charge use of shared fake server.",
                  "type": "string",
                  **"optional": "true"**
                }
              }
            }
          }
      }
    }
n3wscott commented 6 years ago

Context: The additionalProperties keyword is used to control the handling of properties whose names are not listed in the properties keyword. For json schema, any additional properties are allowed by default. (see the JSON Schema Validation Spec - 6.5.6. additionalProperties)

I think including additionalProperties is a good suggestion, and in-fact the broker should use this field as it is inline with the requirement to ignore unknown fields. At the moment the validation will fail for a broker that does not have "additionalProperties": true and the platform is sending additional fields.

label and optional are not in JSON Schema spec from what I can see. Please link the section. They might be an implementation detail of some popular library.

DevasiaThomas commented 6 years ago

Hi @n3wscott (Sorry for the late reply),

I have a broker written that is being used in Cloud Foundry (the Pivotal offering of it).

additonalProperties: Thank you for clarifying the usage and I understand it. But as you said, by default this is true in the schema although at the moment validation(on behalf of the platform) will fail if it is not explicitly specified. I don't wish to suggest a change to the current explicit usage. I just request this field to be documented in the example. My reason for it - if the user is using a cli to communicate with the platform and eventually the broker, the user will provide whatever params they wish to; but in cases where there is a UI provided by the platform, they pretty much just treat the schema as rulebook and do not allow additional params to be sent as they usually provide a form with keys pre-populated.

label: You are right, it is not in the spec. It is meta-information about the property itself, like description. I am requesting for it to be included in the documented examples for a similar reason as the above - When there is a UI, it could pick up the label instead of the keyword (hence if a property has a long dash/underscore separated name, it could have a pretty name to show).

optional: Please ignore this request. I interpreted the schema incorrectly.

mattmcneeney commented 5 years ago

Hey @DevasiaThomas are you still interested in this issue? I'm not sure if the JSON Schema spec has developed since last April. Thanks!

DevasiaThomas commented 5 years ago

@mattmcneeney Yes I still am :)

mattmcneeney commented 5 years ago

@DevasiaThomas Last time I tried this with PCF Apps Manager, the additionalParameters field seemed to render correctly. Is this still a problem for you? Could you let me know which version you are using?

DevasiaThomas commented 5 years ago

@mattmcneeney I'm on the latest PCF (I have access to the PIE env's through work). Also did you mean additionalProperties ? When I checked the entire flow, the additional stuff never made it to me, because I think the middle man (cloud controller maybe?) only passes what specified by the schema in the catalog. Do you have a sample broker that you tested this with? Maybe I can see what I'm doing wrong.

mattmcneeney commented 5 years ago

Yes, I did mean additionalProperties, and you're right - I just tried that and it doesn't seem to support this workflow. It doesn't allow for arbitrary key/value pairs, only the fields that are described under the properties field.

This is from a broker that has some properties defined, but also has additionalProperties set to true:

Screenshot 2019-07-25 at 17 47 16

Since this is actually a feature request for PCF Apps Manager, I'd recommend you ask the Pivotal folks you're working with to raise this feature request for you!

DevasiaThomas commented 5 years ago

Cool