Add support for draft-2019-09 #131
Conversation
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
You're trying to do too much. The changes to these keywords from 2019-09 to 2020-12 were almost entirely renaming. You just need to make copies of the existing items and prefixItems handlers and make minor edits. If your handlers look significantly different than the 2020-12 versions of these keywords, you're on the wrong track.
One test isn't close to enough. Like, I said before, go through the existing tests, translate them to 2019-09, and remove any duplicates. There are six tests that use the prefixItems keyword not counting unevaluatedItems tests. I expect at least that many new tests.
ProTip: Always make an effort to match the code style used in the repo you're contributing to. Install the ESLint plugin in your editor to get code style hints as you work.
ProTip: Always run the test, lint, and type-check scripts before submitting a PR (see "Contributing" in the README). The automation is there to double check us because we all forget things sometimes, but it looks unprofessional when you submit something that isn't ready.
| */ | ||
|
|
||
| /** @type NormalizationHandler */ | ||
| const itemsDraft04 = { |
There was a problem hiding this comment.
Let's stick with the naming convention used in all the other normalization handlers.
| const itemsDraft04 = { | |
| const itemsDraft04NormalizationHandler = { |
| // If items is not a tuple, additionalItems is ignored | ||
| const items = | ||
| /** @type {{ schema?: { items?: unknown } }} */ | ||
| (context).schema?.items; | ||
|
|
||
|
|
||
| if (!Array.isArray(items)) { | ||
| return outputs; | ||
| } | ||
|
|
||
| const startIndex = items.length; | ||
|
|
||
| // additionalItems: true → always valid | ||
| if (additionalItems === true) { | ||
| return outputs; | ||
| } | ||
|
|
||
| let index = startIndex; | ||
|
|
||
| while (true) { | ||
| const itemNode = Instance.step(String(index), instance); | ||
| if (!itemNode) break; | ||
|
|
||
| // additionalItems: false → validate against false schema | ||
| if (additionalItems === false) { | ||
| outputs.push( | ||
| evaluateSchema( | ||
| /** @type {string} */ ("false"), | ||
| itemNode, | ||
| context | ||
| ) | ||
| ); | ||
|
|
||
|
|
||
| } else { | ||
| // additionalItems is a schema | ||
| outputs.push( | ||
| evaluateSchema( | ||
| /** @type {string} */ (additionalItems), | ||
| itemNode, | ||
| context | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| index++; | ||
| } |
There was a problem hiding this comment.
The current items keyword is literally no more than a rename of additionalItems. This handler should look a lot more like the current items normalization handler.
src/test-suite/tests/items.json
Outdated
| { | ||
| "description": "items (tuple validation) - draft 2019-09", | ||
| "compatibility": "<=2019", | ||
| "schema": { | ||
| "type": "array", | ||
| "items": [ | ||
| { "type": "number" } | ||
| ] | ||
| }, | ||
| "instance": ["foo"], | ||
| "errors": [ | ||
| { | ||
| "messageId": "type-message", | ||
| "messageParams": { | ||
| "expectedTypes": { "or": ["number"] } | ||
| }, | ||
| "instanceLocation": "#/0", | ||
| "schemaLocations": ["#/items/0/type"] | ||
| } | ||
| ] | ||
| } | ||
| , |
There was a problem hiding this comment.
This needs to be indented properly.
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
This looks great! It's almost ready. My only comments are minor code style things.
I liked @srivastava-diya's approach in #130 of putting draft-specific keywords in separate folder. Could you please do that here too. That will make it easier to merge her PR in after yours.
Also, please revert any formatting changes in the JSON files for lines you didn't change. I noticed that in items.json and oneOf.json which didn't have anything to do with this PR.
| }, | ||
| { | ||
| "description": "items (as schema) applies to all - draft 2019-09", | ||
| "compatibility": "<=2019", |
There was a problem hiding this comment.
This test and the next one should work for all versions, so it's ok to leave off compatibility.
| "compatibility": "<=2019", |
| @@ -1,50 +1,370 @@ | |||
| { | |||
| "$schema": "../test-suite.schema.json", | |||
|
|
|||
There was a problem hiding this comment.
I'd like to keep this empty space in the test files. It's a separation of meta-data from the actual data that I think improves readability.
|
|
||
| setNormalizationHandler( | ||
| "https://json-schema.org/keyword/draft-04/items", | ||
| itemsDraft04NormalizationHandler | ||
| ); | ||
|
|
||
| setNormalizationHandler( | ||
| "https://json-schema.org/keyword/draft-04/additionalItems", | ||
| additionalItemsDraft04NormalizationHandler | ||
| ); |
There was a problem hiding this comment.
Let's register these last instead of first. And please make these one function call per line like the rest so the code style is consistent.
2 participants
@jdesrosiers
What’s included
New normalization handlers
items-draft04.js— implements pre-2020 tuple validation semanticsadditionalItems-draft04.js— implements legacy handling for items beyond the tupleCorrect keyword registration
https://json-schema.org/keyword/draft-04/itemshttps://json-schema.org/keyword/draft-04/additionalItemsNew test coverage
compatibility: "<=2019"prefixItems/itemscases where behavior differsNo behavior changes for draft-2020-12 or newer drafts
Implementation details
items-draft04mirrors the behavior ofprefixItems:evaluateSchemaper array positionNormalizedOutput[](nonullvalues)additionalItems-draft04mirrors the draft-2020-12itemshandler:itemsis an array (tuple form)items.lengthtrue,false, and schema valuesBoth handlers are implemented as simple applicators.
Test status
All existing tests pass
New draft-2019-09 tests pass
Total: 200 / 200 tests passing
Notes
This implementation follows the guidance discussed in the issue:
Happy to adjust anything if you’d like a different structure or naming.