-
-
Notifications
You must be signed in to change notification settings - Fork 8
Add Support for JSON Schema Draft-04 Keywords #130
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
base: main
Are you sure you want to change the base?
Conversation
|
@jdesrosiers please have a look whenever you're free,because i feel a lot of it needs to be re iterated over |
jdesrosiers
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.
This is a quick review. I didn't have enough time to look over everything in much detail.
Make sure you're working with @Adityakumar37 on the items/additionalItems keywords. I'm hoping to merge that PR first, then you can rebase and add the rest of the draft-04 implementation on top of that.
Anyway, your approach for those keywords isn't right. Those are simple applicators. Simple applicators don't need error handlers. Your normalization handlers should look a lot like the keywords from draft-2020-12 that serve the same purpose.
dependencies is a weird one because it's a simple applicator in some cases (schemas) and a validation keyword in other cases (string array). It can even do both at the same time. I need to go through that handler closer later, but it seems like it should be a problem.
|
hey @jdesrosiers , |
jdesrosiers
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.
Looks pretty close now. The only thing I haven't looked at yet is the tests.
| const parentLocation = schemaLocation.replace(/\/maximum$/, ""); | ||
| const parent = await getSchema(parentLocation); | ||
| const exclusiveNode = await Schema.step("exclusiveMaximum", parent); | ||
| const exclusive = /** @type boolean */ (Schema.value(exclusiveNode) ?? false); |
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 won't always work. You have to find the value using the keyword URI, not the keyword name. See the contains error handler for an example.
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.
Actually, contains isn't quite right either because it considers values from other schemas applied to the same location. You're going to have find a solution that doesn't hardcode the keyword name, but only considers keywords in the same subschema.
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.
should i use the Regex approach?
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.
Definitely not. You can't depend on the keyword names being consistent. You can only depend on keyword URIs. JSON Schema vocabularies allow for a lot of a lot of things including changing the names of keywords. For example, someone might create a vocabulary that includes all the standard keyword, but translated to another language. So, the actual keyword name might be máximo (spanish) instead of maximum, but it would still have the same keyword URI and be able to be handled by this handler.
So, you need to find a solution that's based on the keyword URI. That's mostly not a problem because the normalized output will include the keyword based on its URI (see the contains handler), but you'll need to add something to make sure you're only considering keywords in the same schema. You should be able to do that with some pointer manipulation in this case, but it won't work for contains when we get the draft-06 support because of references. But, don't worry about that for now.
- Add normalization handlers - Add error handlers - Fix type handling and numeric count/matchCount constraints - Update draft-04 test suite
feat(draft-04): add support for JSON Schema Draft-04 keywords - Add normalization handlers - Add error handlers - Fix type handling and numeric count/matchCount constraints - Update draft-04 test suite A fix normalization and error handlers Signed-off-by: Diya <[email protected]> fix: normalization and error handlers for draft-04 fix type checks revert minimum and maximum error handler
|
hey @jdesrosiers |
We want to merge #131 first, so let's keep this in draft until that's merged. Other than that, yes. I'll review now, but I think it's mostly ready with exception of that one part. |
jdesrosiers
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.
I added a couple more comments in the dependencies error handler. I also took a look at the tests this time. I skipped the items/additionalItems tests because I expect those to come from the other PR, but the rest of the tests look good enough.
| continue; | ||
| } | ||
|
|
||
| if (normalizedErrors["https://json-schema.org/keyword/draft-04/dependencies"][schemaLocation]) { |
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 if isn't necessary. In this case, the value could be true or NormalizedOutput[]. We already checked if it's true, so the only thing left that it could be is NormalizedOutput[].
You could change the ... === true check above to typeof ... === "boolean". That would accomplish the same thing and you wouldn't have cast on the next line here.
| const compiled = await getSchema(schemaLocation); | ||
| const dependencies = /** @type {Record<string, string | string[]>} */ (Schema.value(compiled)); | ||
|
|
||
| for (const property in dependencies) { |
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's better to use the AST functions here especially because there could be schemas with references in this value.
| const compiled = await getSchema(schemaLocation); | |
| const dependencies = /** @type {Record<string, string | string[]>} */ (Schema.value(compiled)); | |
| for (const property in dependencies) { | |
| const dependencies = await getSchema(schemaLocation); | |
| for await (const [property, dependency] of Schema.entries(dependencies)) { |
| if (Array.isArray(dependencies[property])) { | ||
| const missing = dependencies[property].filter((required) => !Instance.has(required, instance)); |
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.
Once we know it's an array, it's reasonable to convert it to a value.
| if (Array.isArray(dependencies[property])) { | |
| const missing = dependencies[property].filter((required) => !Instance.has(required, instance)); | |
| if (Schema.typeOf(dependency) === "array") { | |
| const dependentRequired = /** @type string[] */ (Schema.value(dependency)); | |
| const missing = dependentRequired.filter((required) => !Instance.has(required, instance)); |
| "tests": [ | ||
| { | ||
| "description": "dependencies with schema", | ||
| "compatibility": "=4", |
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 version of dependencies is used through draft-07.
| "compatibility": "=4", | |
| "compatibility": "<=7", |
| "errors": [ | ||
| { | ||
| "messageId": "maximum-message", | ||
| "messageParams": {"maximum": "3" }, |
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.
Formatting
| "messageParams": {"maximum": "3" }, | |
| "messageParams": { "maximum": "3" }, |
| "errors": [ | ||
| { | ||
| "messageId": "exclusiveMaximum-message", | ||
| "messageParams": {"exclusiveMaximum": "3" }, |
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.
Formatting.
| "messageParams": {"exclusiveMaximum": "3" }, | |
| "messageParams": { "exclusiveMaximum": "3" }, |
| } | ||
| ] | ||
| } | ||
| } |
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.
All files should end with a newline. I suggest changing your editor settings to add these automatically.
Added Support for JSON Schema Draft-04 Keywords
I have implemented full support for the requested Draft-04 keywords in
@hyperjump/json-schema-errors.Issue : #119
Changes made
itemsandadditionalItemsDraft-04 treats
itemsandadditionalItemsas siblings whereadditionalItemsvalidation depends onitems(ifitemsis an array).itemsnormalization handler in items.js.itemshandler infers the URI of the siblingadditionalItemskeyword (by string manipulation of theitemsURI) and retrieves the schema from the AST. It then acts as the validator for the "tail" items that fall underadditionalItemscriteria.dependenciesImplemented
dependencies.jsto handle both:Numeric Keywords (
maximum,minimum)Implemented handlers
maximum.js,minimum.js.exclusiveMaximum(boolean).exclusiveMaximum) to construct the correct error message ("less than" vs "less than or equal to").Also Added test cases