-
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
Improve explanation of Reference Object #388
Comments
On further review, the REUSE guidelines should be updated too: |
@jasonh-n-austin - are you referring specifically to how relative references are resolved? |
Right, so perhaps examples of what One example:
Also, deep links such as:
All of this is really just how JSON Reference works, but some practical examples would probably help devs implementing the Swagger 2 spec get it right more consistently. |
👍 been burning a lot of cycles reverse engineering what references are supported. REUSE.md seems to indicate only absolute references. |
@jasonh-n-austin Hi, I'm refactoring my python client for external file reference, and feel confused with these: swagger.yaml
$ref: 'models/Thing.yaml'
Thing.yaml
$ref: 'models/AnotherThing.yaml' # note path is relative to swagger.yaml, not Thing.yaml In another discussion, one of your comment:
In the first comment, the base URI used to resolve 'models/AnotherThing.yaml' should be derived from swagger.yaml, although the $ref resides in Thing.yaml. But the comment in another issue states that the base URI should be derived from Thing.yaml. Which one of my understanding is correct? Thanks for any feedback in advance. 👍 |
I would expect relative references to be based on the location of the document containing the reference. |
I tend to agree with you @whitlockjc (clearly contradicting my earlier statements). I think I was basing the examples on the way that @BigstickCarpet 's swagger-parser resolves JSON references (which could be incorrect here). Per the RFC for JSON Reference (https://tools.ietf.org/html/draft-pbryan-zyp-json-ref-03):
|
When working on swagger-tools and sway, I created json-refs just for this. While I'm not saying it's perfect or should be the guide, it's been well received. Ever since supporting relative references in json-refs, relative references have always been resolved as I mentioned above and I'm yet to have anyone complain about it or suggest otherwise. While the sample set is small, this tells me that the intuitive approach matches what I suggested above as well. |
FWIW, I put together a sample spec which has separated files (https://github.com/OAI/OpenAPI-Specification/tree/master/examples/v2.0/json/petstore-separate). IIRC I ran swagger-tools across this to validate that everything was resolving right. This should serve as a working example for other implementers for the time being. |
I'm still confused with that RFC :(
In the example above, should the referring document be swagger.yaml or Thing.yaml when resolving 'models/AnotherThing.yaml'? @BigstickCarpet very clear, thanks. |
I agree with @jasonh-n-austin and @whitlockjc that relative references should be based on the location of the document containing the reference. Swagger Parser 1.x and 2.x both resolved relative paths based on the main document rather than the containing document, but there were at least three problems with that:
Swagger Parser 3.0 was a complete rewrite, and one of the main goals was to make it fully compliant with all of the specifications involved (Swagger 2.0, JSON Schema, JSON Hyper-Schema, JSON Reference, and JSON Pointer). As a result, relative references are now resolved relative to the containing document. |
I'll see what I can do about finally getting around to writing something up in |
I'm not sure there was ambiguity other than at some point, we have to call these what they are: JSON References. Thankfully, if we can do that the JSON Reference specification is very clear on things. I've had no problem understanding the spec when working on my tooling and I'm sure @BigstickCarpet has had similar success writing his. At the same time, I doubt anyone would complain with more documentation if it meant making things simpler to understand. |
👍 to that |
Used part of my open dev day and did some writing for PR #535, I'd love feedback @whitlockjc & @BigstickCarpet |
@jasonh-n-austin Thanks for PR! Overall it looks good to me. I would reorganize the guide a bit, but I'd suggest we'll merge your PR first, and then I'll submit another PR reorganizing it for everyone here to review. Basically, I'd split it into cases, and change some headers and topic order, just to create a clearer flow, not changing the content itself. Per your request, I'll wait for the feedback from @whitlockjc and @BigstickCarpet before merging. |
@jasonh-n-austin - All of your edits look good to me. But there are a few other parts of the document that I feel are a bit unclear: 1. I'm not sure exactly what this line under "Operations" means:
What exactly is it referring to? It doesn't seem to pertain to anything in the code sample immediately preceding it. Maybe it just needs to be reworded? 2. In the "Responses" section, there's an example file {
"NotFoundError": {
"description": "Entity not found",
"schema": {
"$ref": "#/definitions/ErrorModel"
}
}
} The reference to 3. At the very end, in the "Constraints" section, it says:
I assume it means that OAI 2.0 only supports references to JSON files, not to YAML files? If so, why? The format supports YAML for the main file, so why not support it for external files as well? This seems like an arbitrary limitation, especially since YAML is simply an alternate syntax for JSON, not something more advanced that requires special consideration or handling. |
@BigstickCarpet In Swagger/OpenAPI 2.0 we made an unfortunate mistake by allowing $ref inside the Path Item Object where we allowed |
Sorry, should have replied to everything. For point 2, that's not @jasonh-n-austin's fault, it's mine :) It can definitely be expanded. As for 3, that's a constraint that may no longer relevant and is related to the official Swagger tools around the spec. Now that the spec has migrated to the OAI and the tools are separate, that constraint should be removed (it may also be irrelevant now, I'd need to check). |
I provided my responses to the PR. Long story short, I think we really should avoid adding too much to this. Something like "Swagger reuse is enabled through JSON References." would be a good start. After that, some example of the different types of JSON Reference locations (local, relative, remote, relative or remote with a hash, ...). I also think we should remember that JSON Reference processors will not necessarily be Swagger aware and will gladly resolve references to locations within the Swagger document regardless of whether or not the JSON Reference points to a collection of definitions or not. (Collection of definitions like we Swagger supports for |
@whitlockjc reuse is also enabled with internal references to |
Perhaps it would be better to split the REUSE into two docs? One explaining what can be reused, and one giving more info about how to use JSON References within the current spec (first doc pointing to the other)? |
@jasonh-n-austin Yep...using JSON References to get to them. ;) |
lol. What @whitlockjc said. |
My personal opinion is that it makes no sense to say "We use JSON References but we only allow them to point to certain locations when they are local references." Over the last two years, my work and open source life has been Swagger tooling. I have read every shred of Swagger specification documentation and their JSON Schema files so many times I can't even begin to count. I never once have made the connection that just because these containers were created that it implied JSON References to locations within the same document could only point to those containers. Not only that but never once has anyone brought it up when working on any of the Swagger tooling I've been involved with. But that is probably a topic for a different issue since I guess the proposed verbiage is fine based on the unwritten rules that I've just become privy to. I just thought you should know. |
I was thinking that the internal references were JSON Pointer, and external was JSON Reference (which uses JSON Pointer for fragments). Maybe I just didn't realize JSON reference would do the local resolution stuff too. Now that I think about it, the I'll work on rewording this a bit. @webron are you concerned this is too long already, re: 2 docs? |
Just to avoid confusion for anyone reading this later, the JSON Reference spec says that the To make things simpler, I think mentioning that JSON References are used to enable reuse should be good enough as long as we have good examples. The reason being is that the differences between the different "types" of JSON References used in Swagger documents are just URI semantics. For example, see the three "main" JSON Definition definition types:
Are we over complicating things? |
I hope this is reading right. There is no emotion or attitude in any of this. I'm just trying to help. |
Oh the joys of text-based communication. :-/ It reads fine to me. Very informative and clarifying too. Thanks for elaborating! |
Any time I communicate via text, I always err on the side of caution just in case. Thanks for the feedback. |
Okay, as @whitlockjc mentioned we always use JSON References inside the spec. JSON References use JSON Pointers in some cases, as defined in the JSON Reference RFC. I really don't think we need to get into the subtleties of how and when JSON Pointers are used inside JSON References - if anyone wants to know, they should read the RFC. I know @whitlockjc have done so, I've done so, and I'm sure @BigstickCarpet and @jasonh-n-austin have done so as well. Since we use JSON References, we just need to mention where we deviate from it or what additional restrictions we add to it. In most cases, we use it as-is. Overloading the spec or our docs with information that can be easily attained from reading other specs seems wasteful to me. @jasonh-n-austin - If it's ok with you, please keep it in one doc and concentrate on the content. I'll rework the docs afterwards and submit a PR for the contributors here to review. @whitlockjc - I assume I annoy people by default with my comments (even though it's not my intent), and am ready to be hit by rotten tomatoes at any given time (virtually and in real life). |
Excellent stuff @whitlockjc. No negative emotion read...remember I work with anti-social developers all day...I read that you care ;) Totally agree it's all just JSON Reference...my apologies for confusing the matter. |
@webron PR should be ready for merge. This was a fun one, with all of the collaborative input! Glad to finally have that topic covered. |
Thanks everyone for the great work on this. The PR has been merged. I'd like to keep the issue open for a bit more, if you don't mind. Please wait for an additional comment to come soon. |
So following the recent changes, I'd propose splitting the guides into two as shown in #547. Just a proposal, can be scrapped as quickly as it was created. @jasonh-n-austin, @whitlockjc, @BigstickCarpet - thoughts? |
@webron @darrelmiller what is the status of this one? |
@philsturgeon It is closed |
With the addition of relative file references (notably in swagger-js, and samples), there should be an explanation of how that resolves (based on JSON Reference).
Samples/petstore-separate
provides a reference point that could be utilized.The text was updated successfully, but these errors were encountered: