Closed ChALkeR closed 1 year ago
Merging #162 (3167a72) into master (287f3a8) will increase coverage by
0.00%
. The diff coverage is100.00%
.
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Files Changed | Coverage Δ | |
---|---|---|
src/compile.js | 99.26% <100.00%> (+<0.01%) |
:arrow_up: |
@RichAyotte this should be ready now
I've checkout the branch and ran a few tests and observed a few new behaviours.
When a $ref is specified without unevaluatedProperties
, I get the following error: [requireValidation] additionalProperties or unevaluatedProperties should be specified at #/properties/bh1
"properties": {
"kind": {
"const": "double_baking_evidence"
},
"bh1": {
"$ref": "#/definitions/TezosBlockHeader"
},
"bh2": {
"$ref": "#/definitions/TezosBlockHeader"
}
Adding unevaluatedProperties
to each item resolves the issue.
"properties": {
"kind": {
"const": "double_baking_evidence"
},
"bh1": {
"unevaluatedProperties": false,
"$ref": "#/definitions/TezosBlockHeader"
},
"bh2": {
"unevaluatedProperties": false,
"$ref": "#/definitions/TezosBlockHeader"
}
}
The second new behaviour is with items. The errors are [requireValidation] items rule should be specified at #/oneOf/3/items
and [requireValidation] items rule should be specified at #/oneOf/3/properties/args/items
for the following:
"MichelineMichelsonV1Expression": {
"type": ["array", "object"],
"oneOf": [
...// removed 3 irrelevant items here
{
"type": "array",
"items": {
"$ref": "#/definitions/MichelineMichelsonV1Expression"
}
},
{
"type": "object",
"properties": {
"prim": {
"type": "string",
"maxLength": 256,
"format": "anyString"
},
"args": {
"type": "array",
"items": {
"$ref": "#/definitions/MichelineMichelsonV1Expression"
}
},
"annots": {
"type": "array",
"items": {
"type": "string",
"maxLength": 256,
"format": "anyString"
}
}
}
}
]
}
I forgot to add, the changes here resolve issue https://github.com/ExodusMovement/schemasafe/issues/161 :rocket:
Re: refs -- thanks!
Yes, this did change the behavior -- previously, all $ref
s were checked to be fully validating everything on their own, and now that check was removed, but together with corresponding assumption that $ref
s fully validate everything`.
I'll take a closer look at/investigate schemas that were previously compiling and now don't due to this.
Yes, I see a regression on
validator(
{
$schema: 'http://json-schema.org/draft-07/schema#',
$defs: {
A: {
type: ['array', 'object'],
oneOf: [
{
type: 'array',
items: {
$ref: '#/$defs/A'
}
},
{
type: 'object',
required: [],
additionalProperties: false,
properties: {}
}
]
}
},
$ref: '#/$defs/A'
},
{ mode: 'strong' }
)
Looks like we need actual propagation for cyclic refs.
I assume that bh1
/bh2
in the first example are also cyclic? Because there should be proper tracing for non-cyclic refs that works independent on the change here.
I'm not sure what cyclic means here but here's the bh1
/bh2
sample with more context.
"TezosDoubleBakingEvidenceOperation": {
"type": "object",
"allOf": [{ "$ref": "#/definitions/TezosBaseOperation" }],
"required": ["kind", "bh1", "bh2"],
"properties": {
"kind": {
"const": "double_baking_evidence"
},
"bh1": {
"unevaluatedProperties": false,
"$ref": "#/definitions/TezosBlockHeader"
},
"bh2": {
"unevaluatedProperties": false,
"$ref": "#/definitions/TezosBlockHeader"
}
}
},
That doesn't look like a regression.
Perhaps we could enable the new behavior as an opt-in or only on non-cyclic refs to avoid regressions
@RichAyotte updated
Everything looks really good now and all my tests pass. Thanks for putting this patch together so quickly :pray:
@RichAyotte I added more tests/safeguards and fixed an issue that detected. This should be ready/final now.
(lint fixes / code formatting only in the last pushes)
Everything still looks good :+1:
Merged and published as @exodus/schemasafe@1.2.0
.
Upd: use @exodus/schemasafe@1.2.1
instead 🤦🏻 (I should have tested the unrelated commit before including it in the release)
I'm glad you found it and I just tested with v1.2.2
and it works well.
Refs: https://github.com/ExodusMovement/schemasafe/issues/161#issuecomment-1670111982
This will allow e.g. this to compile in strong mode: