Skip to content

Allow using URN and not just URLs #166

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

Closed
flozano opened this issue Jun 18, 2019 · 26 comments
Closed

Allow using URN and not just URLs #166

flozano opened this issue Jun 18, 2019 · 26 comments

Comments

@flozano
Copy link
Contributor

flozano commented Jun 18, 2019

(From #141)

Right now, if we understand correctly, references to external schemas are forced to be URLs.

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.

@flozano
Copy link
Contributor Author

flozano commented Jun 18, 2019

related to #100

@stevehu
Copy link
Contributor

stevehu commented Jun 18, 2019

@flozano I agree that we need to support both. For now, most users are using external references as they want to share some comment schemas between the specifications of services. Eventually, users would like to refer to some common schemas.

@flozano
Copy link
Contributor Author

flozano commented Jun 18, 2019

In our case, it's not so much about sharing with 3rd parties, but about our own private schemas. We have many schemas which use URNs that refer to a few well-known schemas.

If we could use URI instead of URL it would be fixed. I could attempt to do a pull-request, but I'm afraid URL is scattered all over the code, so it'd be very big.

@stevehu
Copy link
Contributor

stevehu commented Jun 18, 2019

@flozano Please give it a try. We have sufficient test cases to ensure nothing is going to break.

@stevehu
Copy link
Contributor

stevehu commented Jun 19, 2019

@flozano Could you please provide several examples of URNs that are publicly accessible? Also, any documents or links that might help us to understand the requirement in your case.

@flozano
Copy link
Contributor Author

flozano commented Jun 19, 2019

You can see one here:
https://api.flightstats.com/flex/airports/rest/v1/schema/json

in this example, the ref refers to a URN inside the same document, but it could be externally defined. Basically I think schema validator should not dictate what references are and how they are fetched for everyone.

By using URL you enforce the references to be URLs, by using URI you can let the "fetcher" implementator decide how to resolve those.

@flozano
Copy link
Contributor Author

flozano commented Jun 19, 2019

Please note that, as per https://json-schema.org/understanding-json-schema/structuring.html, $ref and $id are URIs and not URLs

@stevehu
Copy link
Contributor

stevehu commented Jun 19, 2019

For people (including me) who are confused about URL, URI, and URN, please check this link

https://stackoverflow.com/questions/176264/what-is-the-difference-between-a-uri-a-url-and-a-urn

I think we should support all of them as they do have a place in the JSON schema.

@jawaff
Copy link
Contributor

jawaff commented Jun 19, 2019

I'm starting to understand what you're looking for. We already have a means for injecting a URLFetcher, but the main problem is combining the ids and $ref values together. If the ids and $refs are for a URN, then they probably just get concatenated (that's my guess). However, a URL component has some special rules (which are handled by the URL constructor).

I could make the update and allow some sort of injectable fetcher for custom schemes.

@jawaff
Copy link
Contributor

jawaff commented Jun 20, 2019

There only seems to be 7 files that need updated at first glance. It looks like the URI constructor and URI#resolve method do what we need. We just need to make some sort of URIFetcher that allows custom schemes to be fetched. By default, the current URLFetcher can be used for all of the URL schemes.

@jawaff
Copy link
Contributor

jawaff commented Jun 20, 2019

I've got something almost done, but I'm having problems with combining ids and $refs. The URI#resolve method doesn't work as expected for some things and broke some of the tests.

One of the failing unit tests had to do with the custom classpath: URI scheme. The id was 'classpath:/tests/relativeRefRemote.json' and the $ref was 'integer.json'. I would have expected that to be recognized as a relative path and to end up with 'classpath:/tests/integer.json'. However, the result was 'integer.json' as the combined URI.

The only builtin Java tool I've found for combining these things correctly so far has been the URL constructor. Maybe we need to allow injectable behavior for combining pieces of URIs based on the scheme. We can use the URL constructor for the URL schemes (which would include our custom classpath scheme) and expect a custom 'combiner' for other URIs.

@flozano
Copy link
Contributor Author

flozano commented Jun 20, 2019

wow that was fast. If you upload what you have I can try to help... as you know better the code-base, there's no point in me trying to duplicate the effort. I was still exploring how to do it. I agree using different strategies for URL-looking URIs and for non-URL-looking URIs should be doable.

@jawaff
Copy link
Contributor

jawaff commented Jun 24, 2019 via email

@jawaff
Copy link
Contributor

jawaff commented Jun 24, 2019

I think I got something worked out on a spare computer. I'm going to submit a pull request.

@stevehu
Copy link
Contributor

stevehu commented Jun 24, 2019

@jawaff I am sorry to hear the hard drive problem. I have sent you an invite to join light-4j so that you can create a branch and constantly check in your code. I am doing the same although my hard drive is backing up daily.

@jawaff
Copy link
Contributor

jawaff commented Jun 24, 2019

@stevehu Thanks for the invite. I've got a fork of this project. I could have easily checked in my changes to the fork, but I was still experimenting and didn't really want to yet. It's not a big deal. I've got everything rewritten and ready to review now. Hopefully you like my solution.

stevehu added a commit that referenced this issue Jun 24, 2019
@stevehu
Copy link
Contributor

stevehu commented Jun 24, 2019

1 similar comment
@stevehu
Copy link
Contributor

stevehu commented Jun 24, 2019

@stevehu
Copy link
Contributor

stevehu commented Jun 24, 2019

@flozano I have reviewed and merged the PR @jawaff submitted. Could you please review and let us know if it covers all your scenarios. If possible, please add more test cases if you see some of the scenarios are not covered.

@jawaff Thanks a lot for your help.

@jawaff
Copy link
Contributor

jawaff commented Jun 25, 2019

@stevehu Thanks for checking out my updates so quickly. I hope I didn't miss anything in my updates.

stevehu added a commit that referenced this issue Jun 25, 2019
@flozano
Copy link
Contributor Author

flozano commented Jun 26, 2019

This covers all our scenarios, very little glue code was needed.

We're just ditched our own fork of the "everit" validator and already enjoying this validator.

Thanks a lot @jawaff and @stevehu !!!

@jawaff
Copy link
Contributor

jawaff commented Jun 26, 2019 via email

@jawaff
Copy link
Contributor

jawaff commented Jun 26, 2019 via email

@stevehu
Copy link
Contributor

stevehu commented Jun 26, 2019

@flozano @jawaff This is a perfect scenario that we work together. Thanks a lot.

@stevehu stevehu closed this as completed Jul 29, 2019
@jenschiavone
Copy link

Hello. I've been reading through various threads about supporting nested schemas and am still unable to tell if that is supported in networknt json-schema-validator. I have my schema defined locally in my project. When I write my definitions in my main schema file and reference them, everything works. But I really prefer to have my definitions in other files within the same directory. I cannot get my refs to those definitions to work though. I need to understand if I'm doing something wrong or if this capability is simply not supported by this library. If it IS supported, could you please provide a very basic example of referencing a definition in another, local file? Thanks.

@stevehu
Copy link
Contributor

stevehu commented Oct 7, 2022

In the specification, local files are not supported. We do have some ways to support local files, but it is not recommended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants