-
Notifications
You must be signed in to change notification settings - Fork 9.1k
v3.2: $self
field (Alternative Approach)
#4556
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
base: v3.2-dev
Are you sure you want to change the base?
Conversation
After staring at the giant examples section a lot, I decided it works better as an appendix. (force push is just because I accidentally picked up a stray file in the commit and had to edit it to pull that out). |
This is the most readable of the lot, and I think I "get" the whole thing a lot more now. Thank you for doing this work and all the iterations! Minor points/suggestions from me (I'm not ready to approve but I've only got small questions):
|
Good discussion in the meeting:
|
I am in agreement with the shuffling of the examples to the learn site rather than in the main spec, but not with the change from relative to absolute uri. |
@handrews — A big +1 from me for including examples in the spec. I would prefer them to be in-line so a reader does not have to jump around to follow them, but if those examples need to be in an appendix that is better than not having any or having a link to someplace that might eventually 404. My biggest pet peeve about specs in general is that they do not include examples which make most specs all-but-inaccessible to those like me who need examples to understand the prose. P.S. Moving to the learn site only works IMO if there is a guarantee that the learn site will never change those examples nor take them down which, I believe, is impossible to guarantee. FTW. |
Relative vs. absolute
Seems the only feature added by What am I missing? |
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 there may be some errors in the examples of URI resolution in Appendix G. I pointed out two and I think the rest should probably be checked for accuracy before we merge this.
src/oas.md
Outdated
For the relative `$ref` in the first document, it is resolved against `$self` to produce `https://example.com/shared/foo#/components/requestBodies/Foo`. | ||
The portion of that URI before the '#' matches the `$self` of the second document, so the reference target is resolved to `#/components/requestBodies/Foo` in that second document. | ||
|
||
In that document, the `$ref` in the Request Body Object is resolved using that document's `$self` as the base URI, producing `https://example.com/schemas/foo`. |
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 too does not jive with the resolver that Copilot built. It says that the target URI is actually "https://example.com/api/schemas/foo".
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.
@mikekistler should be fixed. Really, "these look wrong" would have been sufficient here. The errors were just because I reworked the various paths at some point to make the examles cover more cases, and some things didn't get updated post-rework. Nothing obscure was going on.
Although it sounds like you produced something useful beyond this which is cool!
Another reason to allow I see no gain from forcing |
The main feature Using |
Is there an example (for the learn site, appendix or wherever) that you can construct that would demonstrate this issue, as a way of cautioning against it? |
After many discussions about relative vs absolute, we agreed in TDC this week that supporting both would make a more open feature and since the retrieval URL is already used in other places, the additional burden for tools maintainers is not huge. |
I have updated this with the outcome of today's TDC call. I have also:
The new commits were added in one push, and then I force-pushed the rebased commits unchanged to pick up the latest syntax highlighting that supports this change building without warnings. |
hmm... I thought I was only adding two new commits, looks like maybe I had not pushed some other local fixes? My apologies for any confusion, just be aware that I have never edited commits once pushed, so if a commit looks familiar it's the same as it was. |
This adds `$self` as a way for a document to define its own URI for use in reference targets, and as the base URI for relative URI references in the document. This does not impact the resolution of relative API URLs.
Including fixing a bug in one of the URIs.
Never change your directory structure halfway through writing examples...
Fix the $self resolution examples to consistently use /api/, which is used in all of the relevant base URIs and is not impacted by any of the relative paths.
This also further clarifies the need to use `$self` in reference targets for interoperability even if other URIs might work in some cases.
One more new bugfix commit plus an unmodified rebase force-push. |
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 to the content, suggestions for its representation
Co-authored-by: Ralf Handl <[email protected]>
Co-authored-by: Ralf Handl <[email protected]>
@ralfhandl I have tried to address everything that I did not accept as a commit.
Regarding the |
...because the YYYY-MM-DD in the URI makes it more trouble than it's worth.
@ralfhandl went ahead and removed the parameter because of the YYYY-MM-DD problem. Which makes me sad because I would rather promote the long-established correct use of the media type. |
Fixes:
This is an alternative to PR #4389.
The only functional difference is that$self
is required to be absolute (a change that @karenetheridge and @mikekistler are debating).The main difference is that I drastically reduced the normative specification text and instead gave examples of all of the different ways to establish a base URI. [EDIT: and put the examples in an appendix] This is probably more understandable than trying to explain the options in prose.
@notEthan I also dumped a lot of the use cases as I think I figured out a more concise way to get the point across.
This adds
$self
as a way for a document to define its own URI for use in reference targets, and as the base URI for relative URI references in the document.This does not impact the resolution of relative API URLs.