-
-
Notifications
You must be signed in to change notification settings - Fork 591
Fix relative references #306
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
Fix relative references #306
Conversation
Awesome, really appreciate a patch here, will try to have a look and leave some initial comments -- will try to read this a bit more carefully as well in a bit. |
@@ -771,6 +772,28 @@ class TestRefResolver(unittest.TestCase): | |||
stored_uri = "foo://stored" | |||
stored_schema = {"stored" : "schema"} | |||
|
|||
def prev_resolve_fragment(self, document, fragment): | |||
fragment = fragment.lstrip(u"/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like quite a lot of not-testing-specific-looking code making an appearance in a test. I don't have a specific recommendation yet on how to remove it, but it doesn't look right to me, especially not in combination with being patched into place (I'd like to sometime soon remove the mock
dependency entirely, but alas).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry about that, I can remove some of the unnecessary parts of that method. I only put them in there to replicate the original method as closely as possible.
@@ -898,6 +921,77 @@ def test_helpful_error_message_on_failed_pop_scope(self): | |||
resolver.pop_scope() | |||
self.assertIn("Failed to pop the scope", str(exc.exception)) | |||
|
|||
def test_prev_resolve_fragment_raises_exception(self): | |||
schema = { | |||
"definitions": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This schema seems like it should be easy to simplify.
@@ -1,5 +1,6 @@ | |||
/jsonschema/_version.py | |||
/version.txt | |||
.idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this in your own personal global gitignore
} | ||
} | ||
resolver = RefResolver.from_schema(schema) | ||
self.assertEqual(resolver.resolve_fragment(schema, "#/definitions/Category"), schema['definitions']['Category']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that what this is trying to fix is probably validation related, so would be nice to have at least one actual integration test (that uses a relative reference in a schema for validation) -- especially so, since if there's a bug here, it means we're missing a test in the official test suite
@@ -356,7 +356,7 @@ def resolve_fragment(self, document, fragment): | |||
|
|||
""" | |||
|
|||
fragment = fragment.lstrip(u"/") | |||
fragment = fragment.lstrip(u"#").lstrip(u"/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems likely to be misplaced overall to me (which maybe is another reason why an integration test would be good).
This method is about resolving fragments -- fragments don't start with #
, #
delimits a fragment, so whoever called this should have removed the #
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, my mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you want functionality, perhaps in another method, to convert a pointer into a fragment, by stripping the #
, etc, or do you want to close the request?
Currently, calling the resolve_fragment method of the RefResolver class to return the JSON referenced by the pointer of a $ref key fails if the pointer is preceded by a
#
character. The JSON schema specs specify this format, e.g. { "$ref": "#/definitions/address" }. Specifically, the following exception is raised: jsonschema.exceptions.RefResolutionError: Unresolvable JSON pointer. This happens even if the pointer is valid.I made changes to the resolve_fragment method of the RefResolver class in jsonschema/validators.py to strip away this character, and now the pointer resolves correctly. I also added two tests to jsonschema/tests/test_validators.py to demonstrate this behavior of the old method and to show the results of the revised method. Let me know if you have any questions.