-
Notifications
You must be signed in to change notification settings - Fork 9.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alternative: Clarify resolving implicit connections (3.1.1- alternative to #3823) #3856
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, with formulation proposal
I prefer this alternative over #3823. |
A good discussion about this in TSC this week. Examples might be findable in https://github.com/OAI/oascomply/tree/main/reports to illustrate this point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the text to clarify understanding and use cohesive terminology, throughout.
versions/3.1.1.md
Outdated
|
||
Several features of this specification require resolving a non-URI-based connection to some other part of the OpenAPI Description (OAD). | ||
|
||
These connections are easily resolved in single-document OADs, but the resolution process in multi-document OADs has never been spelled out, and is therefore _implementation-defined_, within the constraints described in this section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These connections are easily resolved in single-document OADs, but the resolution process in multi-document OADs has never been spelled out, and is therefore _implementation-defined_, within the constraints described in this section. | |
These connections are easily resolved in single-document OADs, but the resolution process is _undefined_ in multi-document OADs. Therefore, implementation is defined within the constraints described in this section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how we are using "undefined", which means "while some tools might do something here you cannot rely on it even being correct, except maybe by coincidence", while "implementation-defined" means "there are multiple correct alternatives, so you can't expect interoperability, but you can expect correct results."
versions/3.1.1.md
Outdated
[Operation Object](#operationObject) `tags` | [Tag Object](#tagObject) `name` (in the Components Object) | _n/a_ | ||
[Link Object](#linkObject) `operationId` | [Path Item Object](#pathItemObject) `operationId` | `operationRef` | ||
|
||
A fifth implicit connection, which involves appending the templated URL paths of the [Paths Object](#pathsObject) to the appropriate [Server Object](#serverObject)'s `url` field, is unambiguous because only the entry document's Paths Object contributes URLs to the described API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fifth implicit connection, which involves appending the templated URL paths of the [Paths Object](#pathsObject) to the appropriate [Server Object](#serverObject)'s `url` field, is unambiguous because only the entry document's Paths Object contributes URLs to the described API. | |
A fifth, implicit connection, involves appending the templated URL paths of the [Paths Object](#pathsObject) to the appropriate [Server Object](#serverObject)'s `url` field. This is unambiguous because only the entry document's Paths Object contributes URLs to the described API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremyfiel I feel like, the extra comma, reads like, a Shatner, comma. 😉 Is there a specific grammar rule you're applying here?
More seriously, all of this section is about implicit connections, so a "fifth implicit connection" is no different from a "fifth connection". "implicit" is not a modifier to be set off in contrast to other connections, but an inherent part of the concept being named (of which there are five).
However, "involves appending the... to the.. url
field" is a non-essential clause being set off from the sentence that otherwise stands on its own ("A fifth implicit connection is unambiguous because...") So it gets commas around it and a conjunction to introduce it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up taking your suggestion of splitting the sentence and removed all commas and "which" in the first sentence as I think that is correct and then we don't need to debate complex sentence structure.
versions/3.1.1.md
Outdated
A fifth implicit connection, which involves appending the templated URL paths of the [Paths Object](#pathsObject) to the appropriate [Server Object](#serverObject)'s `url` field, is unambiguous because only the entry document's Paths Object contributes URLs to the described API. | ||
|
||
It is RECOMMENDED to consider all Operation Objects from all parsed documents when resolving any Link Object `operationId`. | ||
This requires ensuring that all referenced documents have been parsed prior to determining an `operationId` to be unresolvable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires ensuring that all referenced documents have been parsed prior to determining an `operationId` to be unresolvable. | |
This requires all referenced documents to be parsed prior to determining an `operationId` unresolvable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still needs either a "that" before "all" or a "to" before "have". I'm fine with removing "ensuring", though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a native speaker, I don't know enough rules to argue for or against it. haha
I think this may sound better
versions/3.1.1.md
Outdated
|
||
The interface approach can also work for Discriminator Objects and Schema Objects, but it is also possible to keep the Discriminator Object's behavior within a single document using the relative URI-reference syntax of `mapping`. | ||
|
||
There currently are no URI-based alternatives for the Security Requirement Object or for the Operation Object's `tags` field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There currently are no URI-based alternatives for the Security Requirement Object or for the Operation Object's `tags` field. | |
Currently, there are no URI-based alternatives for the Security Requirement Object or the Operation Object `tags` field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I wonder how to best make the intent more clear here.
In this sentence, the structure is making a distinction between the Security Requirement Object (as a whole, or more precisely all top-level keys in the Object) and the specific tags
field in the Operation Object. Your rewrite reads to me like both the Operation Object and Security Requirement Object have tags
fields, which is not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, that does seem to infer both of them have the tags
. I added a the
in there.. Not sure if that is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs the repeated "for." I did remove the "currently", though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremyfiel I appreciate you going over this for consistency! We will need an effort like this on a larger scale before publishing, and I hope we can build up a style guide, which perhaps you could help with?
I've replied to a few where I interpret grammatical rules differently, or see other precedents in the existing text, or just think the sentence was unclear to start with which led you to fix the grammar in the "wrong" direction. But I appreciate this feedback and am merging several other comments.
versions/3.1.1.md
Outdated
|
||
Several features of this specification require resolving a non-URI-based connection to some other part of the OpenAPI Description (OAD). | ||
|
||
These connections are easily resolved in single-document OADs, but the resolution process in multi-document OADs has never been spelled out, and is therefore _implementation-defined_, within the constraints described in this section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how we are using "undefined", which means "while some tools might do something here you cannot rely on it even being correct, except maybe by coincidence", while "implementation-defined" means "there are multiple correct alternatives, so you can't expect interoperability, but you can expect correct results."
versions/3.1.1.md
Outdated
[Operation Object](#operationObject) `tags` | [Tag Object](#tagObject) `name` (in the Components Object) | _n/a_ | ||
[Link Object](#linkObject) `operationId` | [Path Item Object](#pathItemObject) `operationId` | `operationRef` | ||
|
||
A fifth implicit connection, which involves appending the templated URL paths of the [Paths Object](#pathsObject) to the appropriate [Server Object](#serverObject)'s `url` field, is unambiguous because only the entry document's Paths Object contributes URLs to the described API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremyfiel I feel like, the extra comma, reads like, a Shatner, comma. 😉 Is there a specific grammar rule you're applying here?
More seriously, all of this section is about implicit connections, so a "fifth implicit connection" is no different from a "fifth connection". "implicit" is not a modifier to be set off in contrast to other connections, but an inherent part of the concept being named (of which there are five).
However, "involves appending the... to the.. url
field" is a non-essential clause being set off from the sentence that otherwise stands on its own ("A fifth implicit connection is unambiguous because...") So it gets commas around it and a conjunction to introduce it.
versions/3.1.1.md
Outdated
A fifth implicit connection, which involves appending the templated URL paths of the [Paths Object](#pathsObject) to the appropriate [Server Object](#serverObject)'s `url` field, is unambiguous because only the entry document's Paths Object contributes URLs to the described API. | ||
|
||
It is RECOMMENDED to consider all Operation Objects from all parsed documents when resolving any Link Object `operationId`. | ||
This requires ensuring that all referenced documents have been parsed prior to determining an `operationId` to be unresolvable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still needs either a "that" before "all" or a "to" before "have". I'm fine with removing "ensuring", though.
versions/3.1.1.md
Outdated
|
||
The interface approach can also work for Discriminator Objects and Schema Objects, but it is also possible to keep the Discriminator Object's behavior within a single document using the relative URI-reference syntax of `mapping`. | ||
|
||
There currently are no URI-based alternatives for the Security Requirement Object or for the Operation Object's `tags` field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I wonder how to best make the intent more clear here.
In this sentence, the structure is making a distinction between the Security Requirement Object (as a whole, or more precisely all top-level keys in the Object) and the specific tags
field in the Operation Object. Your rewrite reads to me like both the Operation Object and Security Requirement Object have tags
fields, which is not the case.
a88d108
to
66e4d93
Compare
@jeremyfiel I've updated several areas that you commented on with changes that I think address your concerns in slightly different ways. There are a few I have not (yet) changed, pending your replies. I've also rebased the branch to resolve merge conflicts - while I had to force-push that, my latest changes are a separate commit if you want to look at them specifically. |
Amazing, the things I learn here 😁 |
This clarifies how to handle resolving implicit (non-URI-based) connections in multi-document OpenAPI Descriptions. While the behavior is implementation-defined overall, this RECOMMENDS a single approach based on how things behaved going back to the 2.0 referencing model. This allows Security Schemes and Tags to (like the top-level Server Objects) define a deployment-specific interface for referenced documents to access. This entry document interface approach makes less sense for the Discriminator Object, but it can use the URI syntax of `mapping` to keep things within the local document. This also aligns the search for matching `operationId`s with 3.1's full-document parsing requirements. Note that the term "complete OpenAPI document" has been defined in another change pending approval on the 3.0.4 branch.
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
Co-authored-by: Jeremy Fiel <32110157+jeremyfiel@users.noreply.github.com>
66e4d93
to
59148a2
Compare
I have now added an example of resolving a Security Requirement in a referenced document. I am hoping that is sufficient and we don't need examples of everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, minor nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the example works well, but must we provide that in JSON and YAML?
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
@lornajane we usually do but sometimes do not. I suppose I can add it. |
@lornajane I remember why I didn't want to add the JSON It annoys me every time I see a I really want either both references to be to just plain Maybe I could frame it as HTTP content negotiation? This is the version you get with |
I like that! |
@lornajane @ralfhandl I've expanded the examples with both JSON and YAML showing the HTTP requests with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple nits on the language.
I'm not a huge proponent of the conversational text, especially for a spec. But I won't rewrite the whole thing. :)
Co-authored-by: Jeremy Fiel <32110157+jeremyfiel@users.noreply.github.com>
That's an editing thing rather than a writing thing. With these PRs the point is to get the information in the spec. In an ideal world someone like you who has a good eye for editing would go over everything and do wordsmithing. I do not care if my writing style is retained, as long as we first get the content in that I can provide (and no one else is currently both willing and able to do), and then worry about the style. And preferably make a style guide! It would make it so much easier to add text if we had one. But for just getting the content in- whoever does the work gets to determine how it's written up, especially when there's no style guide. And if someone wants to do an editing pass before publishing- I'm all for it. |
@jeremyfiel also, I tend to write more conversationally for examples, and less so for definitions. My preference would be to integrate the spec and the Learn site more closely and move most examples to the Learn site, which would have a different tone. But for now, people expect to find the examples in the spec, and (up to a point) we need to put some there. I struggle with where to draw the line on what goes in or doesn't – for now it's been "apparently I'm the one writing the PRs so the line is where I say it is", but that's not a viable policy long-term and leads to questions like in #3874. |
Not merging, waiting for @jeremyfiel to approve the latest changes |
@lornajane Please confirm whether the text now has sufficient examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting into the weeds to get this under control!
Resolving implicit connections (3.0.4 port of #3856)
This is an alternative approach to the one in PR #3823
The other PR pushes users more towards document-oriented resolution, which is what I personally prefer. But it only works if the documents are self-contained, and the way people organize multi-document OADs based on 2.0 conventions does not work with it. So the other PR provides two different approaches depending on how the OAD is structured. That's a bit complicated.
This PR advises one approach: resolve implicit connections from the entry document, and use URI-based alternatives to resolve locally.
This works because the ones where you are most likely to want local connections both have URI-based alternatives, while the ones without URI-based alternatives arguably make more sense associated with the API deployment, which is handled by the entry document:
mapping
supports URIsoperationId
collisions across multiple documents, andoperationRef
is a URI-based alternative that does thatI think we would want to still provide the missing URI-based alternatives in 3.2 because sometimes you will want to package up a set of Tag Objects or Security Schemes+Requirements. We in fact have issues open for both of those things.
But this is a more straightforward option, even if I'd prefer the self-contained document approach.
This clarifies how to handle resolving implicit (non-URI-based) connections in multi-document OpenAPI Descriptions.
While the behavior is implementation-defined overall, this RECOMMENDS a single approach based on how things behaved going back to the 2.0 referencing model. This allows Security Schemes and Tags to (like the top-level Server Objects) define a deployment-specific interface for referenced documents to access.
This entry document interface approach makes less sense for the Discriminator Object, but it can use the URI syntax of
mapping
to keep things within the local document.This also aligns the search for matching
operationId
s with 3.1's full-document parsing requirements.Note that the term "complete OpenAPI document" has been defined in another change pending approval on the 3.0.4 branch.