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

Have purge_unknown and allow_unknown play nice together #324

Closed
audricschiltknecht opened this issue Jul 12, 2017 · 7 comments
Closed

Have purge_unknown and allow_unknown play nice together #324

audricschiltknecht opened this issue Jul 12, 2017 · 7 comments

Comments

@audricschiltknecht
Copy link
Contributor

@audricschiltknecht audricschiltknecht commented Jul 12, 2017

Issue

(Unless I am missing something)
Setting purge_unknown on Validator (or top-level schemas) will ignore specific allow_unknown properties on fields.

The use-case is that while undeclared fields should be purged from the document, there are some of the sub-documents that have been explicitly tagged as permitting unknown fields, and these should not be purged.

Reproduce

  • First example
>>> validator = Validator(purge_unknown=True)
>>> schema = {'foo': {'type': 'dict', 'allow_unknown': True}}
>>> document = {'foo': {'bar': True}, 'bar': 'foo'}
>>> validator.validated(document, schema)
{'foo': {}}

Expected:

{'foo': {'bar': True}}
  • Second example
validator = Validator(purge_unknown=True)
schema = {'foo': {'type': 'dict', 'schema': {'bar': {'type': 'string'}}, 'allow_unknown': True}}
document = {'foo': {'bar': 'baz', 'corge': False}, 'thud': 'xyzzy'}
>>> validator.validated(document, schema)
{'foo': {'bar': 'baz'}}

Expected:

{'foo': {'bar': 'baz', 'corge': False}}
@audricschiltknecht
Copy link
Contributor Author

@audricschiltknecht audricschiltknecht commented Jul 12, 2017

I have implemented a small patch audricschiltknecht@07b61ed
If there is interest, I can create a pull-request for this.

@funkyfuture
Copy link
Member

@funkyfuture funkyfuture commented Jul 13, 2017

imo, it's reasonable to address and merge that.

this means that allow_unknown and purge_unknown should be declared as mutually exclusive, both in the validator of schemas and the configuration of a validator instance.

@audricschiltknecht
Copy link
Contributor Author

@audricschiltknecht audricschiltknecht commented Jul 13, 2017

@funkyfuture thanks for your comment.
However, I do not think that they should be mutually exclusive, but that allow_unknown will have precedence on purge_unknown where defined (this is our use-case). Am I making sense?

@funkyfuture
Copy link
Member

@funkyfuture funkyfuture commented Jul 18, 2017

yes, it makes sense. my intent is to avoid questions from users who are not aware of that behaviour by telling them upfront what they shouldn't expect to function as they may expect that.

@nicolaiarocci
Copy link
Member

@nicolaiarocci nicolaiarocci commented Jul 22, 2017

@audricschiltknecht please submit a PR for this. Make sure the docs are updated, maybe with the very same example you posted here, so users know what the intended behavior is.

@audricschiltknecht
Copy link
Contributor Author

@audricschiltknecht audricschiltknecht commented Jul 25, 2017

Ok, will do

@audricschiltknecht
Copy link
Contributor Author

@audricschiltknecht audricschiltknecht commented Jul 25, 2017

I've just created a PR. However, I could not run doc creation nor doctests because of an error raised by sphinx (neither using my virtualenv nor by running the docker script).

nicolaiarocci added a commit that referenced this issue Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants