-
Notifications
You must be signed in to change notification settings - Fork 11
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
Validate references #59
Comments
Note that these can be external references, pointing to other files or definitions residing in other files. The validator should work with those definitions as well. |
@tfesenko, Marking this one as high priority, as it will empower users to quickly resolve a significant number of issues without in-depth diagnosis. @andylowry , note that this may be preferable to fixing ZEN-2633 |
This commit adds the capability to the validator to check for wrong references ($ref keyword).
I started to work on this issue, the validator will return warnings for the links ($ref values) that are invalid or point to no files. Here's a demo of what it currently looks like There is still some clean up to do as I will try to reuse some code from the JsonReferenceHyperlinkDetector as their logic is quite similar. |
@ghillairet , @tfesenko , we have an important project called Swagger-Nomalizer that resolves and inlines Swagger references (among other things). It's important that we keep reference resolution consistent across Swagger Normalizer, hyperlinking ("go to definition"), and reference validation. Swagger-Normalizer also caches external files, for efficiency. Maybe reference validation can make use of this cache. I wish I had thought of this before. @andylowry , can we move Swagger-Normalizer into our open source RepreZen organization on GitHub? And do you have any suggestions to save Guillaume some time in understanding how Swagger-Normalizer resolves references? I don't think transitive reference resolution (and the contextual interpretation issues around that) are important here. SwagEdit only deals with one context at a time; i.e., the file loaded in the editor. |
@tedepstein wrote: @andylowry , can we move Swagger-Normalizer into our open source RepreZen organization on GitHub? And do you have any suggestions to save Guillaume some time in understanding how Swagger-Normalizer resolves references? I could, but I'm not sure how much value it will bring, especially since resolution in the normalizer is not done correctly at the moment. Very little of the normalizer code actually does reference validation or resolution, and the rules are pretty simple:
Paths are the biggest pain in the neck in this whole referencing scheme. To be absolutely consistent with the JSONRef spec, the reference value itself must conform with URI syntax. But URI syntax specifically excludes many punctuation chars, including the braces that occur regularly in swagger paths - and hence as the names of properties in the The alternative that we're adopting in our recommended multi-file strategy is to use simple names for paths separated out into subordinate files - e.g. use So I think in summary, the following two not-quite-kosher compromises are required by our approach:
Not pretty - not in the least. |
OK, so reading what I just wrote, my statement that the rules are fairly simple seems absurd. But I stand by it. The reasoning behind the compromises and how they affect the rules are complicated, but the rules themselves are fairly straightfoward, and once the URL and pointer are split apart, standard libraries support them well (e.g. using the Java URL class to parse and resolve the URL, and using the Jackson |
@andylowry wrote:
Actually, given all of what you just explained, I think this is quite complex. And unless we're careful, it seems quite likely that we will have inconsistencies between SwagEdit's reference validatation, Swagger-Normalizer's reference resolution. This can lead to situations where the SwagEdit allows something that doesn't parse and display correctly in the live views. It's better for SwagEdit to err the conservative side, being more restrictive than Swagger-Normalizer. But it's best not to err at all, and have both libraries share reference resolution code. |
@ghillairet , I just talked about this with Andy. We agreed that, while there's not a lot of code involved, this is still complex enough cause inconsistencies across components. So we suggest that you extract this logic from the SwaggerPreResolver class in Swagger-Normalizer into a utility class. When Andy gets back to working on Swagger-Normalizer, he will try to use the utility class, so you're both using the same logic. We also think it would be helpful if you took Andy's explanation above, and incorporated that as JavaDoc. |
There are certainly some parts that can be extracted from SwaggerPreResolver and share with SwagEdit, for example I agree that the logic for validation of references should be the same. Although SwaggerPreResolver does a lot more than that, for example I see that it does What we could do is extract the validation logic in some class, SwagEdit does not really need more than that. It only needs to know that a reference can be open, and which file does it point to. To give you an idea, this is how the validation of references is currently done in SwagEdit. It does not involve a complex logic:
|
Hi Guillaume. The part about "fixing" references applies to so-called Simple Refs. That's e.g. where you see something like This is a part that I forgot about up above, and it would be good to have that baked into our common library for as long as we retain it. Other than that, what you've suggested below sounds good. The Normalizer does not currently correctly handle local refs, and it also doesn't split the ref into URL + pointer, as I suggested in my earlier comment, so it will barf on path refs with braces. |
Glad we've converged on a way forward here. One more question: What do we do about caching? Loading external files can be expensive, especially when internet/intranet URLs. Should we coordinate a shared caching strategy, or even a shared cache, used by the validator and the normalizer? |
This commit includes a preliminary API for dealing with JSON references, notably to validate and resolve JSON references inside a JSON/YAML document. The API can be found in package com.reprezen.swagedit.json.references.
@tedepstein @andylowry @tfesenko I started to put together a preliminary API for dealing with JSON references. The purpose of it is to be able to validate and resolve JSON references found in a JSON or YAML document. This API could be used to replace the SwaggerPreResolver class once the code that fixes references is added in this new API from SwaggerPreResolver. The code can be found here https://github.com/RepreZen/SwagEdit/tree/task/59/com.reprezen.swagedit/src/com/reprezen/swagedit/json/references The class that could replace SwaggerPreResolver is JsonReferenceResolver that is meant to be used to resolve all references inside a JSON document. The code that fixes Simple Refs in SwaggerPreResolver could be place in a specialized JsonReferenceFactory that would be used by the resolver to obtain the JSON references. Currently the JsonReferenceResolver is not needed by SwagEdit but could be used to replace some code used for code completion, where we need to traverse the Swagger JSON schema. It would be used to have a fully resolved JSON schema when SwagEdit starts, instead of having to continuously resolve references when traversing the schema. |
Looks like good progress, @ghillairet. Thanks for the update. |
@ghillairet , please note work on SwaggerResolver in https://github.com/ModelSolv/RepreZen/pull/971 |
@tedepstein @andylowry @tfesenko This project could also be used to handle resolution of references inside a JSON document https://github.com/adjohnson916/jackson-json-reference |
@ghillairet Thanks - looks like an interesting library. I'm not sure we can adopt it as-is for a few reasons (take with a grain of salt - I've only read through most of the code in github - haven't experimented, and I could easily have misunderstood some stuff):
All of this could, of course, be addressed by enhancing the framework - and ideally contributing it back. And the simple refs hacks are, hopefully, temporary and maybe could be addressed outside the library by introducing a preprocessing hook to be run whenever a JSON structure is obtained from a file or URI. That might be a good way to go - I do think the overall design is superior to mine. But I'm not sure whether it's important to do now. @tedepstein, executive decision needed. It's probably strategically smart to move toward extending and adopting this library, assuming it has or is likely to get traction in the community. But I'm not sure it's what we should be focusing on at the moment. |
@andylowry, @ghillairet, I took a look at the stats on that project: 4 forks, 3 contributors (but mostly adjohnson916 himself). Started October 2014, and not much recent activity. Given the amount of work it would take to adapt this to our needs, I don't think it's worth the effort. |
as the reference) Original code works well for cases when referenced files were in the default workspace location. It did not work for other cases, e.g. when a project is imported. This fixes it and also simplifies the code by using standard library.
@tfesenko , I think this is done. If you agree, please close it. |
Code assist helps to choose a valid definition. But if the reference is edited, or the referenced definition removed or renamed, the validator should detect this and flag an error. Apparently there is no validation of JSON Pointers on
$ref
elements in the current build.The text was updated successfully, but these errors were encountered: