avian2 / jsonmerge

Merge a series of JSON documents.
MIT License
214 stars 25 forks source link

If object uses additionalProperties: false and an input document contains a rogue key, obscure exception is raised #30

Closed terrisgit closed 6 years ago

terrisgit commented 6 years ago

I fed a schema that uses "additionalProperties": false everywhere to a Merger. I then sent a bad document (because a key wasn't defined in properties:) to the Merger and got an AttributeError exception. Partial trace is below. It took me about an hour to figure out that my document was invalid. Great that I got an exception; not so good that the exception made no sense.

    rv = self.call_descender(descender, schema, *args)
  File "/Users/terris/venv/edc/lib/python3.6/site-packages/jsonmerge/__init__.py", line 102, in call_descender
    return descender.descend_instance(self, schema, base, head, meta)
  File "/Users/terris/venv/edc/lib/python3.6/site-packages/jsonmerge/descenders.py", line 22, in descend_instance
    ref = schema.val.get("$ref")
AttributeError: 'bool' object has no attribute 'get'
avian2 commented 6 years ago

After the first look this seems like two separate problems:

terrisgit commented 6 years ago

I like the current behavior of not validating document fragments that are merged together. E.g., my schema has "requires" which is violated by many of the fragments. When I merge them together they produce a valid document.

avian2 commented 6 years ago

A small update regarding the issue of correctly supporting additionalProperties: false:

Boolean value as a valid schema was added in draft 6 (see changelog). jsonmerge is currently based on a draft 4 validator from jsonschema package, which is the most recent draft supported in that package. Hence supporting draft 6 features in jsonmerge depends on support for these features in jsonschema.

So until jsonschema is released with a draft 6 (or later) validator, I will not work on fixing this.

This is the relevant issue for jsonschema: https://github.com/Julian/jsonschema/issues/337

Julian commented 6 years ago

FWIW just glancing at this ticket because it's linked from that one now, additionalProperties: false was valid even in draft 4, what draft 6 did was just extend that possibility to other validators.

(Might not change your response here though obviously)

avian2 commented 6 years ago

The exception that's raised by jsonmerge when additionalProperties keyword is a boolean should now be fixed.

@terrisgit if there's still some draft 4-compliant case where you get an error, please provide a test case. Otherwise I will close this issue.