Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add JSON pointer support to $data reference. #289

Merged
merged 2 commits into from
Aug 29, 2016

Conversation

HotelDon
Copy link
Contributor

What issue does this pull request resolve?
Issue #285
What changes did you make?
I added support for regular JSON pointers to the $data reference
Is there anything that requires more attention while reviewing?
Make sure I didn't accidentally create the regex from hell when I used and modified the existing relative-json-pointer regex.

@HotelDon
Copy link
Contributor Author

Not sure where the test code for $data is located.

"format": "relative-json-pointer"
"format": {
"anyOf": ["relative-json-pointer", "json-pointer"]
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not valid - should be anyOf with two format keywords inside.

@epoberezkin
Copy link
Member

epoberezkin commented Aug 25, 2016

Thank you for the PR!
Could you please:

  • determine whether it's a relative or an absolute pointer by checking the first character $data[0] == '/', matching regexp twice is wasteful.
  • fix meta schema (see comment in code)
  • squash existing commits (with the change above) in your branch
  • add tests here (as a separate commit). I suggest creating a separate file absolute_ref.json to test several different keywords (constant and a couple of others). There should also be tests of how it works inside $ref (to make sure it can go to a higher level than the root of $ref, because relative refs can't do it on purpose (I remember so at least :), but absolute ones should be able to come to a higher level. If at the moment they can't, please skip those tests, I will think how to improve it (you can use skip: true in json test file, both on a suite level and a test level).

@HotelDon
Copy link
Contributor Author

I can say definitively from my own manual testing, absolute pointers cannot go higher than the root of a $ref ($data: "/" will always resolve to the root of the $ref, not the entire document), which was an issue I was going to bring up once I got this commit fixed up. It looks like this limitation is handled on the $ref side of the code, not the $data side (or at least, not in any of the code I've touched so far). At least I know this was a side effect of something done intentionally, I was going a little crazy wondering some of my test schemas weren't working the way they should have.

As for everything else, I'll get those fixes and tests written up in a day or two. Thanks!

@HotelDon
Copy link
Contributor Author

HotelDon commented Aug 27, 2016

Alright, I finished putting together the tests. The last two tests in the file I added are set to skip: true, as they are testing the interaction between $ref and $data, which currently fails. The rest of the tests were copied over from other files and modified to use regular JSON pointers, and removing the tests that don't apply (the # sign has a completely different use in non-relative JSON pointers). If there's anything else I need to fix, let me know!

@epoberezkin
Copy link
Member

Thank you! I will try to fix this issue - I hoped that even if it doesn't go outside of $ref at least the value would be correct based on the absolute reference. From what you say it's even worse - it just points to the wrong value (in case the root for the ref is different from the data root).

Docs need to be updated too, I will do it.

@epoberezkin
Copy link
Member

@HotelDon I understand how to resolve the issue - the validation function should be passed an additional 5th argument - rootData, then the code generated by getData function can use this variable instead of data (that is the top level data at the current $ref'd schema).

I don't think it will be a breaking change for anyone - It would only be breaking if somebody were passing additional arg(s) to compiled validating function (they could only be used in inline custom keywords).

@@ -214,20 +214,33 @@ function getPath(currentPath, prop, jsonPointers) {
}


var JSON_POINTER = /^#|\/(?:[^~]|~0|~1)*?$/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need "#" in the pattern? If you mean it as the pointer to the root it should be "" (an empty string) instead. "/" is the pointer to the empty property at the top level. "#" in relative json pointer is a special thing resolving to the name of the property/index rather than to data. Or am I missing something?

Copy link
Contributor Author

@HotelDon HotelDon Aug 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I forgot to take it out. The # sign at the start of a pointer DOES have a specific meaning for regular JSON pointers, but it's one that has no relevance to JSON schema, and I should have taken it out of the regex.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really: https://tools.ietf.org/html/rfc6901#section-3
It's when JSON pointer is in URI (in $ref)

Copy link
Contributor Author

@HotelDon HotelDon Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh jeeze, I had never actually noticed that $refs are also JSON pointers. Now that you've pointed that out, I have noticed something. A $data reference formatted like a URI ("$data": "#/foo") will be considered valid in the schema, but the data won't validate. Formatting $ref as a non-URI pointer has a similar effct: "$ref": "/definitions/foo" won't throw an error in the schema, but the data won't validate.

According to the JSON-schema spec, a $ref has to be formatted as a URI, and the $data proposal doesn't mention URI support at all (while I can think of some uses for it, it sounds like a potential security nightmare).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/defenitions/foo is a valid relative URI (definitions is a folder and foo is a file), schema compilation fails because it cannot find schema with such uri.

But #/foo is not a valid JSON pointer, it passes json-pointer format validation because I allowed (maybe mistakenly? Not sure, it's not precisely specified) URI encoded JSON-pointers in this format.

I don't think it's such a big deal to worry about, as long as you are aware that $data only takes normal JSON pointers it's fine. Allowing uri encoded JSON pointers would be a bit more effort that just stripping #, you'd need to decode as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I understand.

Thanks for all your help, by the way. This pull request solved a lot of headaches I was having. If you're ever near San Jose, I'll buy you a beer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha :) Thanks!

@epoberezkin
Copy link
Member

In 4.6.0

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants