-
-
Notifications
You must be signed in to change notification settings - Fork 215
several misclassified tests using URNs in $id #630
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
Comments
They're there to test resolving of the reference in What we have now for classification is really only a "loose" one, there's a (single I believe) test in You may want to look at #629 though (which I haven't looked at yet) to opine on whether these tests are required at all -- obviously given I added them I believe I checked and read the spec as requiring them, but that issue has some discussion, so at some point I'll double check. |
@karenetheridge I'd say they are definitely required in the sense that any conforming implementation MUST support all 5 of the distinct URI grammars from RFC 3986, with the most important being what @Julian notes about same-document references working with any base URI. Technically these are RFC 3986 requirements rather than JSON Schema requirements, but a.) this is critically important functionality, unlike (say) correct handling of internationalized emails in So I'd argue that this is one of the very few times when pass-through requirements from another spec deserve to be treated as 1st-class required JSON Schema tests. The functionality is too important, and reasonable, well-intentioned programmers can accidentally end up not supporting them by trusting a seemingly-correct library or guide. We don't need to cover every case, but definitely the major less-common-but-still-important cases. As far as the relationship of this to #629, here we want to cover every major thing required by RFC 3986. But #629 is arguing about treating schemes differently based on behavior that is allowed by but goes beyond RFC 3986 requirements. I think that scheme/protocol-specific requirements are not required. On-demand retrieval is only a MAY in JSON Schema, and we are silent on URI normalization for comparison purposes (which is an implicit MAY in my book). So those would go in |
I'm not questioning the utility of testing URNs in |
@karenetheridge I see. I do think we should test fragment-only/same-document references with different sorts of base URIs as URI implementations are not required to treat them differently. But I agree with you that those are not the only or most interesting cases. Some possible interesting cases include:
I used If there is a rootless scheme that does not allow |
@karenetheridge did my answer answer your question? For the most part most of the tests in |
Not really. I've been trying to say that when using a json pointer-only $ref, it doesn't really matter what's in $id, and such a test is not likely to exercise the proper handlings of non-http URIs. |
I'm not quite sure what you mean by this, where the test lives of course has no bearing on how well it exercises the behavior, it's just a human aide for classification, and precisely as you say, the tests are there precisely to ensure the But all that being said -- there's no science here, I'm not saying I did a long think on which of the two it belonged in, so if you'd really like to compare and recategorize them, feel free to send a PR. |
I see your point here. RFC 3986 §5.1 notes that fragment-only references are usable without a base URI. So if the schema document is only one schema resource, the presence, absence, or scheme of the URI is irrelevant to implementations that handle fragment-only references this way. However, implementations are not required by RFC 3986 to handle fragment-only references this way, so there is a failure mode here where a JSON Schema implementation supplies the base URI to the RFC 3986 implementation anyway, and that implementation tries to use it and fails because it does not understand rootless-syntax schemes like This is, admittedly, a pretty darn obscure failure mode. With my test planning hat on it would be a low-priority test case, but since it's so easy I'd probably include it. A lot of people treat relative reference resolution as barely more complex than string concatenation (although they'd probably fail some other tests as well). However, other possible relative tests using @karenetheridge is right that if the most compliant approach to supporting this is used, the test covers nothing that's not already covered, but it might catch bugs for people who took a weird approach. idk how that fits into the suite. I'd be inclined to add a note that ideally it's irrelevant, but leave it. |
I'm afraid I can't easily see what each of your comments, including the last one, seem to have to do with the question here (something I think @karenetheridge also is having trouble seeing given the previous confused emoji and response). I don't see @karenetheridge questioning any behavior here, this is strictly a classification question. I'd missed this before by the way @karenetheridge:
This test I of course wanted to add -- I mentioned as much on the PR adding these:
Which as I said we don't really have an easy way to add today, given that all we tell implementers is "we expect all the files in |
@Julian my last comment was just explaining how I read RFC 3986 to allow both:
So, basically, the test isn't always useful, but could be. I don't have strong feelings on whether to keep it or where it should be, so don't worry too much if I'm not making sense. |
Ah got it -- sounds good, that one certainly makes sense to me now for background, thanks. |
I think I'm going to close this for cleanliness given as I mentioned above that our classification system is pretty loose and this isn't terribly clear cut but I think if someone feels quite strongly that there's a better way to categorize what we have, just send a PR with some concrete reasoning. |
There are several tests that were recently added to ref.json (multiple versions) using URNs in the
$id
keyword, but they don't do anything interesting with the$ref
keyword -- the$ref
s are all simply json pointers. What is the purpose of these tests? Shouldn't they be in id.json?The text was updated successfully, but these errors were encountered: