-
Notifications
You must be signed in to change notification settings - Fork 14
mrc-2476: Upgrade ajv to version 8.5.0 and include draft-04 extension #41
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
Conversation
richfitz
left a comment
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.
Nice, this is fantastic. A few minor changes below, most important is to check that we didn't break any of the other packages (if we did we'll need to leave imjv as the default, perhaps unless draft >= 4 is detected)
R/validate.R
Outdated
| error = FALSE, engine = "imjv", reference = NULL, | ||
| query = NULL) { | ||
| tmp <- json_validator(schema, engine, reference = reference) | ||
| error = FALSE, engine = "ajv", reference = NULL, |
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.
If we make this change we might break other packages, can you check please?
https://cran.r-project.org/web/packages/jsonvalidate/index.html - see devtools::revdep to automate
R/validate.R
Outdated
| tmp <- json_validator(schema, engine, reference = reference) | ||
| error = FALSE, engine = "ajv", reference = NULL, | ||
| query = NULL, strict = FALSE) { | ||
| tmp <- json_validator(schema, engine, reference = reference, strict = strict) |
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.
While we're here, could you rename tmp to something better (validator, perhaps?)
js/in.js
Outdated
| if (meta_schema_version === "draft-06") { | ||
| ret.addMetaSchema(AjvSchema6); | ||
| } | ||
| addFormats(ret); |
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.
Aha, is that how you do this?
| switch(engine, | ||
| imjv = json_validator_imjv(schema, v8, reference), | ||
| ajv = json_validator_ajv(schema, v8, reference), | ||
| ajv = json_validator_ajv(schema, v8, reference, strict), |
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.
What happens if we try and use imjv with a meta schema version of draft-6/7? Still throws an error? We should probably document that. Also the docstring for engine needs updating to reflect the new default
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.
It still throws an error yeah - this referenced issue from the code mafintosh/is-my-json-valid#160 still a problem
Co-authored-by: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Co-authored-by: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Alternative approach to compatibility
Co-authored-by: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Co-authored-by: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
See https://ajv.js.org/json-schema.html#draft-04
stricttojson_validatorandjson_validateFALSE by default which allows permissive handling of schema validation see https://ajv.js.org/strict-mode.htmldataPathproperty toverboseerrors, for backwards compatibility as this has been renamed toinstancePathin ajv version 8+Checked this is working with biocompute, and with hintr. Checked this is working with dependencies using
revdepcheck::revdep_check()too - failing on TKCatIssues encountered (which I think I have fixed)
TKCatuses new2019-09schema -"$schema": "https://json-schema.org/draft/2019-09/schema",https://github.com/patzaw/TKCat/blob/master/inst/Collections/Collection-Schema.json#L2 . Ajv supports this see https://ajv.js.org/v6-to-v8-migration.html - tried to work around that by updating the version parsing and making 2019-09 and 2020-12 availabledccvalidatorfailing as it usesdataPathreturned in table of errors which has been renamed ininstancePathin updated ajv, see https://github.com/Sage-Bionetworks/dccvalidator/blob/5dc13f7f77d6444226e3031c1210190937a78682/R/check-schema-json.R#L58 - tried to work around this by dupingdataPathtoinstancePathtableschema.rfails if run with ajv because ajv returns 4 errors where imjv returns 1 (we can work around this if we continue to use imjv for draft-04 by default`biocomputeusesreferenceswhich we don't want to fail on e.g. https://github.com/sbg/biocompute/blob/master/inst/schemas/1.3.0-alpha/biocomputeobject.json#L86Remaining issue
TKCatuses"$schema": {"enum": ["TKCat_collections_1.0"]},which throwsWith cran version they are validating against
imjv. But with updated version currently on master this won't work as it tries to parse the version of the schema so it errors early if not usingdraft-04. This didn't error previously so I assume validated permissively. Now updated to support2019-09this is failing validation on ajv. I don't really know what this is - some kind of metaschema? Not sure if"$schema": {"enum": ["TKCat_collections_1.0"]}is ever valid?