Minimal recursive references support#69
Draft
Stranger6667 wants to merge 4 commits intopython-jsonschema:masterfrom
Draft
Minimal recursive references support#69Stranger6667 wants to merge 4 commits intopython-jsonschema:masterfrom
Stranger6667 wants to merge 4 commits intopython-jsonschema:masterfrom
Conversation
4896ec3 to
2f1dbeb
Compare
This was referenced Nov 20, 2020
Closed
2f1dbeb to
4af9e0b
Compare
This was referenced Dec 23, 2020
d701fa5 to
ee40445
Compare
82f874a to
8071952
Compare
|
@Stranger6667 This seems like a useful feature and we also have some use cases where this could come in handy. How complete is this PR? Just curious, do you (or anyone) plan to work on this or are there any other updates that make this unnecessary? |
Contributor
Author
|
The plan is to refactor the canonicalization process and carry some context around as it is needed to detect the used draft as well. So, this PR is not that relevant structurally, but likely will be useful for such refactoring. It is still needed for recursive reference support though. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #61 but doesn't include explicit data generation for recursive references.
I will update this comment with the results of different commits to track the progress in a more structured way. Also, I will use the
test_can_generate_for_real_large_schematest schemas fromRECURSIVE_REFSto check how many tests are working or not (and why). There are 39 schemas with recursive references in this test, excluding onexfailabout Draft 3.Step 1. Skipping recursive references in
resolve_all_refsBased on your comment in #33 and discussions in #61, I made
resolve_all_refsto stop resolving recursive references. At this point, it still inlines schemas up to the point where a recursive reference occurred. Tests breakdown:11/39 - passed
15/39 - failed due to reference resolving (
Unresolvable JSON pointer:)2/39 - recursion error inside the strategy
11/39 - stuck (I waited for a couple of minutes this time)
The recursion error lead me to the following
jsonschemabehavior:Which occurred in the
Meta-validation schema for JSON Schema Draft 4. Here is a minimized version to illustrate this behavior:{ "properties": { "additionalItems": { "anyOf": [{"type": "boolean"}, {"$ref": "#"}], } }, "type": "object" }Still, it goes to the category where we can't resolve a reference properly - here, it should point to the root schema, but in
merged_as_strategieswe have subschemas and validators inside this function don't see the parent schema, and this error happens. Having a proper reference resolver there should solve the problem.I also not sure if it should be somehow addressed on the
jsonschemalevel, the behavior seems undefined for that simplistic case (I still will handle it in my Rust library, since it causes a segfault on the Python bindings level, maybe will catch recursion level and return some error)Step 2. Reduce inlining in
resolve_all_refsBased on your comment,
resolve_all_refsstops inlining the whole branch, but at the moment, it doesn't remove the inlined reference (will take a look at it later). Tests breakdown:18/39 - passed
1/39 - an example is generated but fails validation (
Avro Schema Avsc file)20/39 - failed due to reference resolving (
Unresolvable JSON pointer:)And no schemas stuck!
Step 3. Pass resolver everywhere where
jsonschemavalidators are usedMost of the previous step errors are related to reference resolving - validators contain sub-schemas that are referring to their ancestors and can't find them. This change adds a resolver that contains the top-level schema, and these validators can resolve references in most cases. There are few exceptions, though - tests from the JSON Schema test suite - "remote ref, containing refs itself" and "valid definition" for Drafts 4 & 7.
There are only 6 failing tests:
Avro Schema Avsc file- there is a canonicalisation error, that leads to a wrong type (oneOf+ recursive reference)Web component file- seems like a missing type as well for{'$ref': '#'}kind of referencesSome schemas are quite slow, but almost everything is working now :)