Add support for draft 2020-12 #817
Conversation
|
Hello there! Thanks very much for this, at first glance it looks like it's great, definitely in the right ballpark. The first thing I think we'll need to address are the changes to the If there are gaps you noticed there, we need to basically take them upstream (and then we'll be able to pull those changes into that folder, which is a Could you perhaps consider pulling those out as PRs to that repository? Ideally in self-contained chunks for each piece. I'll have to look more carefully at the changes to give more specific comments, but from my first skim it seemed exactly like what I'd expect. Let's see whether CI passes as well, which I just manually enabled. And thanks again! |
|
Thanks 🙂, good to know it's going the right way. Sure, I can remove that commit and create a dedicated PR if this fits better. The implementation is still in progress, the curent state is of the I added isodate new dependency for the |
That's already the case here functionally, that directory is a
Cool! That sounds promising, and yeah matches my experience when implementing earlier drafts.
I'll have to do some research myself. If we can't find something, the alternative is to possibly just not support the format. I don't really want to maintain a ton of format-specific code internally in the library. But let me see what I can find myself. |
Not quite sure, but could isoduration do the job? |
Already tried |
|
Inside The package was latter removed due to Zac-HD/js-regex#4 and now the repository is archived. Are there any plans to cover this? |
|
Some tests are skiped in |
If I remember correctly that is due to the new rules/bahavior for Format Vocabulary (see the description in 2019-09). |
Tried isoduration and it seems to work well, maybe I defined the wrong exepction in the |
It's not a priority for me personally at the minute -- if a library comes along in the Python ecosystem that provides JS regexes, it'll be considered, but yeah there are way more important things to address (including this PR!) |
There was a problem hiding this comment.
I gave your branch a spin (using https://github.com/devicetree-org/dt-schema) and found a few issues.
Ultimately, 202012 is too incompatible for me to test this further easily. I'm primarily interested in 'unevaluatedProperties'. The big issue is 'items' lists have been replaced with prefixItems and that's used everywhere. So I need 201909 support. Is that something you plan?
|
@nezhar I pulled in the subtree changes from the test suite. If you rebase or merge your branch, it should now remove those changes from this PR, so any outstanding changes to the |
|
@nezhar Hello, I'm the author of Since there's not been much activity recently, I'd like to mention I'm currently working on these two issues:
The first of those issues already has a PR attached, adding a few tests. No functional changes. The second one will have a PR soon (most likely tomorrow). I've reserved a healthy chunk of time in the next few weeks to solve all other issues, particularly the one related to supporting repeating intervals. I also understand there were some teething troubles while integrating the library, maybe due to unexpected interfaces? If so, please do let me know, and we can see if the situation can be improved. |
Hello @bolsote, thanks for creating and maintaining the library 🥇 As mentioned in #817 (comment) it works well and covers all test cases specified in https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft2020-12/optional/format/duration.json and it is already part of this PR. |
There is something maybe that I miss here. Having this schema: This test should pass (format.json) But this test should fail (optional/format/duration.json) Same schema and same data, and this applies for all format tests. Is this suposed to be controled with an extra param or something? |
Codecov Report
@@ Coverage Diff @@
## main #817 +/- ##
==========================================
+ Coverage 96.41% 97.01% +0.60%
==========================================
Files 18 18
Lines 2730 3011 +281
Branches 308 412 +104
==========================================
+ Hits 2632 2921 +289
+ Misses 78 73 -5
+ Partials 20 17 -3
Continue to review full report at Codecov.
|
It's a bit clunky, but the It may be we need a small tweak to the way we load the test suite to accommodate that. |
Splitting the tests seems to be working fine: 2bb7f52#diff-0953f1b9ffe16c7d4aa18ae8bf21287c552dcd9b241f66fb51e099724af8b722R493 |
jsonschema/validators.py
Outdated
| (id, validator.META_SCHEMA) | ||
| for id, validator in meta_schemas.items() | ||
| ) | ||
| self.store = get_store() |
There was a problem hiding this comment.
Why are we turning what was previously local state to mutable global state?
There was a problem hiding this comment.
Since draft2020-12 relies on references in takes some time to initialize the validator as it fetches all this URLs form remote. The tests for draft2020-12 require over 30 min without this approach and takes several hours with the current pipeline configuration as there is a limit on paralel github actions.
There was a problem hiding this comment.
That sounds like there are some URLs missing from Draft202012Validator's own schema store.
IIRC from the last time I did this myself, basically we need to be caching the vocabulary schemas as well (globally), not just the metaschema.
But yeah we shouldn't move to global state here for all URLs.
There was a problem hiding this comment.
Yes, the meta schema is loaded form the local file located in draft2020-12.json. All references are loaded afterwards:
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://json-schema.org/draft/2020-12/schema",
...
"allOf": [
{"$ref": "meta/core"},
{"$ref": "meta/applicator"},
{"$ref": "meta/unevaluated"},
{"$ref": "meta/validation"},
{"$ref": "meta/meta-data"},
{"$ref": "meta/format-annotation"},
{"$ref": "meta/content"}
],
...
}
In this case I will try to load only the meta schemas in a global store, everything else will be loaded in the validator store.
There was a problem hiding this comment.
Now the vocabulary schemas are also stored in the schemas directory and loaded additionally to the store by adding vocabulary_schemas when creating a validator.
.github/workflows/coverage.yml
Outdated
| @@ -14,14 +14,14 @@ jobs: | |||
| - name: Set up Python | |||
| uses: actions/setup-python@v2 | |||
| with: | |||
| python-version: pypy3 | |||
| python-version: 3.9 | |||
There was a problem hiding this comment.
Hi! Sorry for the delay on this one. I'll get you a full review soon, but this change (moving coverage to run on 3.9) is another good one to extract into its own tiny PR please! Appreciated.
|
I made another contribution on the tests repostiory to cover some cases on unevaluated properties json-schema-org/JSON-Schema-Test-Suite#500 The other contribution I made is kind of reverted, as the checks where moved to format.json - json-schema-org/JSON-Schema-Test-Suite@ba1f1a7. This means that the coverage will drop as there are no more non string tests for the functions inside |
|
Thanks @nezhar! This is likely getting close to merging, I'll have another look and perhaps leave one more round of comments but then likely merge and move forward from there. There are definitely some changes I want to make (specifically around vocabulary support), but we can do so after things are merged, and perhaps after a beta release. |
| narrow_unicode_build(test) | ||
| or ecmascript_regex_validation(test) | ||
| or skip( | ||
| message="ToDo: Extend validation", |
There was a problem hiding this comment.
I'm ok if we are missing support for something here, but can you change the message or add a ticket to indicate what we are missing and what'd be required to get it?
There was a problem hiding this comment.
There are two additional cases that fail that where introduced with the latest update of the tests. I'm kind of having a hard time understanding how the dynamic scopes are suposed to work, maybe they need to be split when loading anschors and dynamicAnchors. I'm still lokking into this, for now I fixed the message.
|
@nezhar I've merged this just now -- apologies for this taking so long, and quite grateful for the work! There definitely still are things I want to fix, you'll see I've just sent a PR upstream because the UUID formatting validation here isn't correct, and I think the relative pointer validation may not be either. But I want to get this merged and fix things on the main branch going forward, so we at least aren't sitting here with the PR waiting. I'll follow up perhaps here if there are any major updates/changes that need doing, just in case you have comments on them, but thanks again! |
| yield error | ||
|
|
||
|
|
||
| def defs(validator, defs, instance, schema): |
There was a problem hiding this comment.
In playing around myself here the past few days, I'm pretty sure something is wrong here, but I don't know what yet.
$defs doesn't really have any behavior, it's just a rename of definitions (and thereby just a standard place to put definitions used elsewhere in the schema), so there's no validation that should happen when encountering it.
But I see tests fail if I remove this -- I think that's an issue with the dynamicRef support (which is what should be generating the error in that test), not with $defs itself, the test that fails is one that validates an invalid schema against the metaschema.
If anyone sees where the issue is, let me know, otherwise will keep playing.
These indeed can be improved, as mentioned in #817 (comment) but it's a bit less clear exactly how yet -- rather than putting $ref in the schema path, instead using relative_schema_path to only refer to the schema post-$ref lookup is a bit more consistent with the current norms, wherein what's in schema_path should be lookup-able via indexing. But for now, they're distinguishable via .schema, which shows only the $ref'ed schema for the second error.
No description provided.