Possible mishandling in allOf keyword canonicalisation

[Stranger6667]

Consider the following schema:

SCHEMA = {
    "allOf": [
        {"$ref": "#/definitions/ref"},
        {"required": ["foo"]}
    ],
    "properties": {
        "foo": {},
    },
    "definitions": {
        "ref": {"maxProperties": 1}
    },
    "type": "object"
}

If we call jsonschema.validate({}, SCHEMA) then it will complain that 'foo' is a required property which is expected. But, if we canonicalise the schema, then $ref keyword will be placed to the schema's root and will cancel the validation of other keywords as implemented in jsonschema. I can't find the exact place in the spec itself, but there are tests in the JSON Schema suite that ensure that other keywords are ignored.

So, with the canonicalised version of the schema an empty object passes validation:

>>> CANONICALISED = canonicalish(SCHEMA)
>>> jsonschema.validate({}, CANONICALISED)

What would be the best way to solve it? Should we check if there are schemas with $ref keywords, then keep them inside allOf and move everything else to the previous level?

P.S. I found the issue via Webpack bootstrap-loader configuration file schema in the testing catalog

[Zac-HD]

Canonicalising should remove the $ref keyword for non-recursive references, but we will probably need to add special handling for recursive references.

[Stranger6667]

Canonicalising should remove the $ref keyword for non-recursive references

Aha, I missed that :) I found it in my experiment branch, where I didn't inline references at all.

but we will probably need to add special handling for recursive references.

Would it make sense if I submit a PR that will handle it? I.e. to be future-compatible with recursive references? Or is it better to add it as a part of that recursive references PR?

[Zac-HD]

I'd be happy either way, but I don't think we can really test it separately so let's keep them together.

[Zac-HD]

Having checked on master, this no longer happens 🙂