Call out that extra fields in requests and responses are allowed #436
Conversation
|
Hey duglin! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
|
I'm happy with this, but we could keep the spec shorter by having this in some generic section at the top somewhere? |
|
I thought about that and about how we've done this in other specs. Normally what I've seen is that the spec would put "..." in the normative pseudo schema that shows what the messages look like on the wire and where extensions can go. And then in a single spot near the top of the spec it says that "... means 'extensions go here'". However, we don't have pseudo schema - we just have a table listing the properties and then examples. So, I felt kind of stuck. As an alternative, we could just add something like: For each operation defined in this specification, there are a set of properties defined that make up the request and response messages. Implementations MAY choose to include additional properties, as extensions, in those messages. Those extension property names MUST NOT conflict with properties defined by this specification. Receivers of these messages MAY ignore any extensions unless some out of band agreement has been made. The only issue with this is that we don't have the ability to say exactly where extensions can go - it basically says any JSON object can have them. That might be ok, we'd just need to check each one. |
|
@mattmcneeney WDYT? |
|
@duglin I prefer having something at the top, but I don't love the term "extensions" here, especially if we're going to use it as part of the "actions" work that is going on. Can't we just say |
spec.md
Outdated
| @@ -349,6 +349,8 @@ It is therefore RECOMMENDED that implementations avoid such strings. | |||
|
|
|||
| \* Fields with an asterisk are REQUIRED. | |||
|
|
|||
| Service Brokers MAY include additional fields within the response. | |||
There was a problem hiding this comment.
In order to make clients that are generally useful in the presence of extensions, we will have to put an extensions object into each request. If we allow extension fields to go directly into the namespace of the first-class spec fields, it will be much harder to write clients for the API.
There was a problem hiding this comment.
Why would it be harder to create a client w/o a wrapper? Granted, some languages (like go) don't handle extensions by default but it is possible to support them. I'm not necessarily pushing back on an "extension" wrapper, but I'm not in favor of having some language's limitations dictate what our spec-wise solution should be, so I'd like to better understand why this impacts clients.
There was a problem hiding this comment.
We should at least make sure that there will be no conflicts in the future. For error responses we have this sentence: When adding new fields, Service Brokers SHOULD use a unique prefix for the field names to reduce the chances of conflict with future specification defined fields.
We could make a more general statement regarding additional fields. For example, if we agree never to use certain characters (e.g. . or :) in field names in the spec then using field names with these characters in extension field names is safe.
There was a problem hiding this comment.
I added the guidance as you suggested. Not sure about the special char things yet...
|
Been thinking about this and regardless of whether we want to put additional (unknown) properties in an "extensions" wrapper, I don't think we can ban the use of extensions outside of the wrapper - at least not w/o breaking backwards compatibility. So, rather than having two ways of doing the same thing, I'm thinking that we should: WDYT? |
|
I agree with @mattmcneeney for just a all encompassing mention that this is fine to do. Adding this language in each object definition seems redundant. |
|
ok - put it in one spot- see whatcha think |
|
Reviews needed |
|
Thanks for that change, it looks awesome now. LGTM |
|
LGTM |
|
@pmorie see if this aligns with what we talked about earlier today |
spec.md
Outdated
| Senders of messages defined by this specification MAY include additional | ||
| fields within the JSON objects as extensions. For backwards compatibility | ||
| reasons, new fields MAY be added at any location within the JSON objects, | ||
| however, they SHOULD be placed within the `extensions` property of the JSON |
There was a problem hiding this comment.
I don't think the extensions property is a good addition. If this becomes an emergent pattern I think you push it into the spec at a later time but this change really limits the integration with existing broker-like solutions. The OSBAPI applied as a mix-in has way more power than forcing the implementor to make OSB the most important part of the API.
There was a problem hiding this comment.
I would prefer them to go anywhere without formal wrapper, yes. I would be fine with
they MAY be placed within the
extensionsproperty of the JSON
There was a problem hiding this comment.
I take it back. I am now in support of something like a extensions property after talking/thinking about the future of the API.
duglin
force-pushed
the
exts
branch
2 times, most recently
from
202410d
to
078314e
Compare
spec.md
Outdated
| ## Extending Resources | ||
|
|
||
| Senders of messages defined by this specification MAY include additional | ||
| fields within the JSON objects as extensions. For backwards compatibility |
There was a problem hiding this comment.
I really disagree with the application of backward compatibility as a concept here, and suggest that we think the approach.
Here's what I suggest:
- There should be a first-class place, that is 100% explicit on every single message, where extension data goes on payloads that people can reason about as users and authors of the OSB spec
- You can reason about these specific places and know you're reasoning about the right scope for what OSB is
- We should make extremely specific guarantees about what the OSB spec says can happen for those specific places
- If you choose to add random extra fields to requests where the extension points are, that's fine, and you can do that, but the first-class place to put extra data for OSB payloads is in the designated spots
- If you implement a client of this API, you only need to implement the designated, explicit places in the API
- If you program to this API, you only need to understand the designated, explicit places for extensions
My concerns with the specific change of saying that brokers can add extensions anywhere:
- It makes the API much harder to reason about as a platform:
- What data will be present on any particular response from a broker?
- What expectations will platforms have about what the platform will do with this data?
- What guarantees does the platform provide to brokers about this random extension data?
- It makes the spec harder to reason about as a broker author, for the same reasons as above
- It complicates platform implementations: now a platform has to deal with the problem of processing unknown fields on every response from a broker
- Makes future linting of brokers impossibly difficult: how do you determine if a broker is using these random additions to the spec in a way that is compliant or a good practice?
- I worry about platforms gradually accreting business logic around specific extensions, which provide an uneven experience for broker implementors
On a more general level, I really worry that if we enshrine things that people decided to do without feedback into the WG and going through the normal process, that we are setting a precedent that will constrain our ability to make progress and changes to the spec without breaking someone. We already have confusing areas of the spec around accommodating stateless brokers; I worry that we are potentially opening the door here to even more confusing semantics.
In any event, it's in our best interest to clearly define:
- What the guarantees around extension fields are - for example, if you put extra fields on a broker response, do you ever expect to have them sent back to you?
- What kind of semantics platforms and broker authors can assign to extensions - is the user ever guaranteed to see them? Should platforms ignore them? etc
I really want to foster innovation in this spec and use of extensions. I also don't want to make things that people are doing 'wrong' and out of compliance with the spec. However, I don't think we should say that you can do whatever you want to any request, potentially ascribing any meaning you want to it, and that it's in scope for OSB.
As a practical matter, let me close this with an example. Say that a broker adds some new ID to provision, update, and deprovision requests as a sibling to other top-level fields in the request body. When first-class extension points are adopted into the spec, if this broker does not change at all, they do not suddenly lose compliance with the spec, they are simply not sending OSB extensions. If they want to send that data as OSB extensions, they should change their broker, or otherwise find a way, possibly through a proxy, to also send that data in the write places to be considered extensions to OSB. Nothing will have changed for the broker - I know that Kubernetes does not in any way support additional fields beyond the official ones, and I have no indication that CF does either - they are no more wrong or right in their use of OSB as a spec than before official extentions existed, because they aren't sending official extensions.
There was a problem hiding this comment.
@pmorie I think you're mixing a lot of topics here. And its way too long to hit all of them, so let me just make a few points:
- regardless of which version of this PR you look at, no one is suggesting that any of these extensions be required for a receiver to understand. The sender has no guarantee that the receiver will understand any of them. In that respect they are all 100% optional and its "sender beware". That's just the nature of extensions.
- extensions, by definition, will not go thru the WG - if they did then they wouldn't be extensions (unless we get into defining "well known extensions", but that's not really the point of this PR).
- tooling on either side of the wire is free to support, or not, the idea of randomly placed extensions. It would then be up to the users of those tools to pester the tool authors to add support if they needed it.
- I don't understand your example but let me add that I don't think the location of the extension has any impact on the semantics behind it - its just a syntax difference.
To me this comes down to a very simple question: what do we want extensions to look like in v3 (or if we were starting from scratch)?
If the answer is: they MAY go anywhere, then that's what we should do now for v2. And I can extract the old version of this PR that just adds a paragraph to say so.
If the answer is: they MUST go into well defined locations in the JSON, then we should not break existing implementations (by allowing them anywhere) but strongly push people towards the v3 model - meaning MAY go anywhere but SHOULD be put into the well defined spots. The current version of this PR.
|
I would like to avoid having this merged via additional LGTM before we can discuss this as a group; let's discuss this on the call tomorrow. |
There was a problem hiding this comment.
I think there is a misunderstanding between the future work of things like Generic Extensions, and this PR asking for what sounds like a place to have Extensions returned from the broker. Are they related? If so, then I am in agreement with Paul that there should be a sanctioned location that the client can process for extensions.
If this is just trying to allow the broker to send random data that a platform can ignore, I would avoid calling it an extension, because there can be no expectation for a platform to process or act on such data. The only case I can think of is broker debug or tight coupling of the broker to the platform. In the case of tight coupling, there will be no expectation that that broker is going to connect to other platforms and the use-case is no longer OSB.
spec.md
Outdated
| Senders of messages defined by this specification MAY include additional | ||
| fields within the JSON objects as extensions. For backwards compatibility | ||
| reasons, new fields MAY be added at any location within the JSON objects, | ||
| however, they SHOULD be placed within the `extensions` property of the JSON |
There was a problem hiding this comment.
I take it back. I am now in support of something like a extensions property after talking/thinking about the future of the API.
spec.md
Outdated
| fields within the JSON objects as extensions. For backwards compatibility | ||
| reasons, new fields MAY be added at any location within the JSON objects, | ||
| however, they SHOULD be placed within the `extensions` property of the JSON | ||
| object. |
There was a problem hiding this comment.
The wording I would like to see is more the the following:
Senders of messages defined by this specification MAY include additional
fields within the JSON objects. Only fields found within theextensionslocation
are considered extensions. For backwards compatibility reasons, new fields MAY
be added at any location within the JSON objects, but these MAY be ignored by
the Platform. Extensions MUST be placed within theextensionsproperty of the JSON
object.
Basically I don't want to limit the data returned from the broker, but I can't see a future compatible way to make this work if we don't direct that output.
spec.md
Outdated
| however, they SHOULD be placed within the `extensions` property of the JSON | ||
| object. | ||
|
|
||
| When adding new fields, unique prefixes SHOULD be used for the field names to |
There was a problem hiding this comment.
I would strike this, if the OEM is putting prefixes, they might as well put the data inside the extensions location.
|
ok- updated per today's call. |
spec.md
Outdated
| @@ -151,6 +151,18 @@ Service Broker MAY reject the request with `412 Precondition Failed` and | |||
| provide a message that informs the operator of the API version that is to be | |||
| used instead. | |||
|
|
|||
| ## Extending Resources | |||
There was a problem hiding this comment.
I am fine with the updated text body but I think the section header reads as if the additional fields have some OSB support. How about a second title like:
Unknown Fields
or
Ignored Fields
or
Out of Scope JSON Payloads
or
Ignored JSON Fields
or
Out of Spec Fields
or
Non-OSB Communications
my vote is on Unknown Fields
|
I didn't want to people get confused because the comment was old and based on old text. I'll skip doing that next time
… On Mar 21, 2018, at 12:24 PM, Scott Nichols ***@***.***> wrote:
I don't think you should be deleting comments @duglin <https://github.com/duglin>. I feel like that is an overstep of the github admin power. @pmorie <https://github.com/pmorie>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#436 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB2sXyPVAKnGQUsVNlH8to6ibbeKtL7Lks5tgn7SgaJpZM4R02kc>.
|
|
updated section title |
|
Reviews needed |
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
|
hmm @n3wscott isn't in the PA list of people for some reason - very odd |
|
I take that back - he is. No idea what's going on. I think we may need to manually count them on this one. I count 3 so far. |
This change will make it clear that request and responses that have HTTP bodies MAY have additional fields specified by the sender. Signed-off-by: Doug Davis <dug@us.ibm.com>
Signed-off-by: Doug Davis <dug@us.ibm.com>
Signed-off-by: Doug Davis <dug@us.ibm.com>
Signed-off-by: Doug Davis <dug@us.ibm.com>
|
rebased to see if that helps |
|
LGTM |
|
Hey it is working again! I think pullapprove was having some issues. |
|
LGTM |
1 similar comment
|
LGTM |
This change will make it clear that request and responses that
have HTTP bodies MAY have additional fields specified by the
sender.
Signed-off-by: Doug Davis dug@us.ibm.com