The input schema draft version should be used in all validators

[Stranger6667]

Hi Zac!

During work on the "recursive references" branch I found a problem.

For example, the input schema is Draft 4 and uses exclusiveMaximum as a boolean value and has keywords that involve subschema validation and are not canonicalised.

The problem is that those subschemas will use Draft 7 (the default from jsonschema) and will be rejected as invalid. For example, the following schema is valid Draft 4 schema:

SCHEMA = {
    "$schema": "http://json-schema.org/draft-04/schema#",
    "not": {
        "allOf": [
            {"exclusiveMinimum": True, "minimum": 0},
            {"exclusiveMaximum": True, "maximum": 10},
        ]
    }
}

And canonicalise call fails:

~/code/hypothesis-jsonschema/src/hypothesis_jsonschema/_canonicalise.py in merged(schemas, resolver)
    922             return FALSEY
    923     assert isinstance(out, dict)
--> 924     jsonschema.validators.validator_for(out).check_schema(out)
    925     return out
    926 

~/.virtualenvs/hypothesis-jsonschema/lib/python3.8/site-packages/jsonschema/validators.py in check_schema(cls, schema)
    292         def check_schema(cls, schema):
    293             for error in cls(cls.META_SCHEMA).iter_errors(schema):
--> 294                 raise exceptions.SchemaError.create_from(error)
    295 
    296         def iter_errors(self, instance, _schema=None):

SchemaError: True is not of type 'number'

Failed validating 'type' in metaschema['properties']['exclusiveMaximum']:
    {'type': 'number'}

On schema['exclusiveMaximum']:
    True

The real-life example is the Open API schema, which I found failing for this reason in my "recursive references" branch.

To me, it seems like we need to pass the schema version to places where those validators are created to use proper validators for the schema's draft. I could try to push a PR with that approach.

What do you think?

Cheers

[Zac-HD]

Huh, I'm surprised this didn't come up earlier. Rather than threading the validator we need through everywhere, how about we rip off this trick:

https://github.com/Zac-HD/hypothesis-jsonschema/blob/c1a12bb034679751a802bbb03efcbcab00d09e18/tests/test_from_schema.py#L268-L271

and just define a get_validator_for(schema) function which tries the upstream get_validator, and falls back to the Draft4Validator if .check_schema() raises? This is might be conceptually pretty dodgy, but the code will look a lot nicer!

[Stranger6667]

It seems like a feasible approach, at least short-term. But I am concerned that it might cause potential problems in the future - for example, when Draft 2019-09 will be more common, and it will be the default validator, then it might be ambiguous what fallback should be used. Luckily, there are no backward-incompatible changes between Draft 6 & 7, so using Draft 4 should be ok.

Another thing is that for recursive schemas, there could be some more pieces to pass along - e.g., ref resolver. I was wondering if it might make sense to create a "context" object that could contain everything needed instead of passing dependencies as separate arguments. In this case, there could be a resolver and a validator, and extending it might be easier than adding new args across the codebase.

However, I feel perfectly fine with fallback to Draft 4 now - it still can be redesigned later if needed.

[Zac-HD]

From memory all draft7 schemas are also valid in draft 2019-09, so let's stick with the simple solution for now. As you say, we can always change it over later if it stops working!

[Stranger6667]

I found another small issue - the test suite run becomes consistently ~25% slower :( I will push the new version in a separate commit - maybe there is something with the implementation itself

[Stranger6667]

Update. I had a slight problem in my code - now there are no perf regressions :) Will push the new version soon