Improved Ref Validator

[jawaff]

This pull request's aim is to eliminate some of the limitations with the RefValidator class.

Here are list of some of the limitations:

• Relative $ref required an absolute id to exist.
• The RefValidator#isRelativePath() check only took into account the 'http' protocol. (Relevant to RefValidator on non-http (classpath & other custom protocol) URL breaks schema loading #98 Schema id does not declare a base URL for relative $ref URLs #26 Issue 26 #37).
• A $ref was only able to build off of a single id.
• A relative domain $ref only worked for absolute urls that followed a : pattern. (Relevant to RefValidator baseSchemaUrl Port Ignored #86).
• An id previously was required to have at least one "/" character. (Relevant to Support for $ref external schema references? #55).

In order to solve these issues, I made it so that JsonSchemas are able to optionally store a 'currentUrl'. This currentUrl is effected by 'id' keywords, urls that are provided to the JsonSchemaFactory. The JsonSchemaFactory is operated by the user of this library as well as RefValidators that point to a remote schema.

All of this then allows the RefValidator to easily deal with a relative $ref. We deal with the relative $ref by simply using the constructor of the URL class. The URL constructor is able to automatically handle domain and subdirectory relative references. The URL constructor just requires a parent URL (which is provided by the JsonSchema's currentUrl) and some relative/absolute string path. If the parent URL is null, then the constructor requires the string path to be an absolute url. If the parent URL is nonnull, then the string path can be either an absolute url or a relative path (absolute urls in the string path are able to override the parent URL).

Looking at my changes though, the URL constructor isn't actually being used directly. I chose to use the existing URLFactory class in this project and added to it so that it can combine a parent URL with some string path. Doing this allows the RefValidator to support the 'classpath:' and 'resource:' protocols that were already working with this library.

Another thing to note is that I updated the interface for the JsonSchemaFactory. I essentially just duplicated the existing factory methods so that users can optionally provide the URL of the schema that they're dealing with. The JsonSchemaFactory is still backwards compatible and doesn't require the URL of the schema, but it can be supplied so that '$ref' and 'id' values are able to be relative to the schema's URL.

So in short, I basically updated the library so that it can rely on the URL constructor for the complicated logic of combining 'id' and '$ref' values with the url of a schema. The URL constructor basically handles all of what this validator needs to do (we even are able to add support for custom protocols) and that bit of code is builtin to Java, so I think this creates a robust and flexible solution.

Note1: I saw that unit tests (in the JsonSchemaTest file) with invalid schemas were succeeding, so I ended up making it so that those tests fail. There was one existing unit test that was silently failing and it has probably been silently failing in this project for a while. It had a reference to the src/test/resources/tests/folder/folderInteger.json file (which didn't exist). I figure that if a test schema isn't able to be loaded, then the unit test or the project probably needs to be fixed.

Note2: I fixed a unit test relating to issue #12 (i.e. SelfRefTest.java). It was loading a schema as a JsonNode (which was succeeding), but the test seemed like it should have loaded it as a JsonSchema. Since I fixed this unit test and it now correctly reproduces the problem in issue #12, the unit test now fails (as expected with a StackOverflow). I just added an @ignore annotation until someone provides a fix.

[jawaff]

My development environment has a proxy, so some of the unit tests cause connection timeouts. I had some problems with a slight bug that I introduced into a schema, but I eventually figured out what the problem was. I initially thought that it had something to do with my Java version (I'm using OpenJDK 11 and Travis is using Oracle JDK 1.8), but it was just a simple typo in the end.

[jawaff]

On a positive note, if I didn't add in that exception for the invalid schemas, then my previous commits would have passed the unit tests. Silently failing tests can be a bad thing potentially.

[jawaff]

Anyway, I'm ready for this pull request to be reviewed. I'm done adding in the improvements that I was interested in. There's still some features that this library doesn't support regarding $refs, but I think it supports the main use cases and that seems fine to me.

[jawaff]

I updated the JsonSchemaFactory a bit. I previously thought that I should add method overloads for everything so that users can associate a URL with some given schema String/InputStream. However, the user can just pass in a URL and then implement a custom URLFetcher to do whatever they need.

I did decide to keep the overload that our JsonSchemaTest currently is using. This particular use case isn't easily fixed by a URLFetcher because the schema can potentially exist within a larger json file, but may still need a URL to be associated with.

[jawaff]

This project is once again up to date and ready to merge. I ran into an issue with the URL mapping while I was fixing a merge conflict in the RefValidator, but that is all worked out now.

[stevehu]

@jawaff I have reviewed the PR and want another developer to review it before merging. The idea is to skip this weekend's release as well due to some emergency bug fixes in this library.

[stevehu]

@jawaff We have got the emergency fix 1.0.10 out and going back to this PR. Could you please resolve the conflicts. Thanks a lot for your help.

[stevehu]

@jawaff I have merged the PR and released version 1.0.14. Thanks a lot for your help. It is a huge PR and it takes some time to review.

https://github.com/networknt/json-schema-validator/releases/tag/1.0.14

[jawaff]

Hey Steve, I thought I needed to make some updates. I'm on vacation right now, but I can make any updates that are needed in a couple of weeks. Thanks for checking out my updates.

…

On Thu, Jun 6, 2019, 7:55 PM Steve Hu ***@***.***> wrote: @jawaff <https://github.com/jawaff> I have merged the PR and released version 1.0.14. Thanks a lot for your help. It is a huge PR and it takes some time to review. https://github.com/networknt/json-schema-validator/releases/tag/1.0.14 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#141?email_source=notifications&email_token=AAWC3XEM7SBMZTQ55YK4BUTPZHE23A5CNFSM4HKXUCY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXEW2ZA#issuecomment-499740004>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWC3XB52BQHY6GYUPMYQIDPZHE23ANCNFSM4HKXUCYQ> .

[stevehu]

No worry. I made a little adjustment after the merging and it works perfectly. Again, thanks for the help as this PR resolves a lot of pending issues.

[flozano]

One Q, why using URL everywhere instead of URI? URL doesn't allow using URN values.

We'd like to extend the validator to register some well-known URNs for some internal schemas, without publishing them in a valid URL, but we don't think it's possible in a clean way with current code-base, so if we want to use this library we need to replace those URNs to URLs in a pre-processing which shouldn't be required.

[jawaff]

Hello, a URI may define something like a book's ISBN and that makes no sense in the context of defining the location of a file. A URL identifies the resource and provides the information for accessing it. I mostly went off of what the library was doing initially anyway. They already had a classpath URL handler defined, so it was just convenient to keep using URLs. URLs just make sense for this json schema validators because they follow the same rules as the $ref. As far as I can tell URLs meet all of the requirements and prevent things that shouldn't be allowed (such as book ISBNs). URIs don't seem necessary and aadditional URL schemes can be supported if need be.

…

On Tue, Jun 18, 2019, 2:44 AM Francisco A. Lozano ***@***.***> wrote: One Q, why using URL everywhere instead of URI? URL doesn't allow using URN values — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#141?email_source=notifications&email_token=AAWC3XGYZXHIXOGZIA2J7IDP3CU7FA5CNFSM4HKXUCY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX52KNQ#issuecomment-503031094>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWC3XG3T32XTOFGO7HD4GLP3CU7FANCNFSM4HKXUCYQ> .

[flozano]

Why do you want to dictate that schemas should be resolv'able thru URLs? Some people can have schemas ( I do :) ) that may have different URIs resolvable thru different means. An URN is a more generic form that allows me to plug whatever I want to retrieve those. With some pluggability, it'd be very convenient to say that URIs in the form of "http://..." are resolved with your URL resolver, whereas URIs in the form of "urn:mycompany:...." are resolved in the way I want

[flozano]

(discussion ongoing in #166)