Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
terrisgit opened this issue Mar 15, 2018 · 5 comments

Comments

@terrisgit
Copy link

terrisgit commented Mar 15, 2018

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'
@terrisgit terrisgit changed the title If schema additionalProperties: false and an input document contains a rogue key, error message is esoteric If schema additionalProperties: false and an input document contains a rogue key, the exception is esoteric Mar 15, 2018
@terrisgit terrisgit changed the title If schema additionalProperties: false and an input document contains a rogue key, the exception is esoteric If schema additionalProperties: false and an input document contains a rogue key, obscure exception is thrown Mar 15, 2018
@terrisgit terrisgit changed the title If schema additionalProperties: false and an input document contains a rogue key, obscure exception is thrown If schema additionalProperties: false and an input document contains a rogue key, obscure exception is raised Mar 15, 2018
@terrisgit terrisgit changed the title If schema additionalProperties: false and an input document contains a rogue key, obscure exception is raised If object uses additionalProperties: false and an input document contains a rogue key, obscure exception is raised Mar 15, 2018
@avian2
Copy link
Owner

avian2 commented Mar 15, 2018

After the first look this seems like two separate problems:

  • jsonmerge assumes that the document is valid for the schema. This has always been the case, but isn't mentioned explicitly the documentation. It might be a good idea for jsonmerge to validate it in merge() call before running the merge.

  • jsonmerge support for schemas that have boolean values for subschemas is buggy (like in your additionalProperties case) and needs better test coverage. RFC section about boolean schema values for future reference.

@terrisgit
Copy link
Author

terrisgit commented Mar 15, 2018

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
Copy link
Owner

avian2 commented Mar 23, 2018

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: python-jsonschema/jsonschema#337

@Julian
Copy link

Julian commented Apr 3, 2018

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 added a commit that referenced this issue Apr 21, 2018
jsonschema draft 4 allows additionalProperties to be true and false, in
addition to containing a subschema.

Related to: #30
@avian2
Copy link
Owner

avian2 commented Apr 21, 2018

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.

@avian2 avian2 closed this as completed May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants