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

[BUG] Nested document validation is broken #107

Closed
vinilios opened this issue Jun 9, 2015 · 12 comments
Closed

[BUG] Nested document validation is broken #107

vinilios opened this issue Jun 9, 2015 · 12 comments

Comments

@vinilios
Copy link

vinilios commented Jun 9, 2015

Nested document validation seems to be broken in master branch. After a bit of debugging it seems that this is due to the fact that self.document attribute of subdocument validators gets assigned with a copy of the contents of the parent document (context) causing further validation steps to fail.

To reproduce

import cerberus

schema = {
    'info': {
        'type': 'dict',
        'schema': {
            'name': {'type': 'string', 'required': True}
        }
    }
}

validator = cerberus.Validator(schema)
res = validator.validate({'info': {'name': 'my name'}})
if not res:
    print validator._errors
funkyfuture added a commit to funkyfuture/cerberus that referenced this issue Jun 9, 2015
@funkyfuture
Copy link
Member

hm, this doesn't fail.

what's actually the output of your code above?

@vinilios
Copy link
Author

The _errors include the following,

{'info': {'name': 'required field'}}

This seems to got introduced with f66d8bd. This commit is not in your fork though and i guess that's why you don't get an error.

@CD3
Copy link
Contributor

CD3 commented Jun 10, 2015

I'm seeing the same problem.

I think the problem is in the __validate_required_fields function. There are two lines that get the set of missing fields.

required = list(field for field, definition in self.schema.items()
                      if definition.get('required') is True)
missing = set(required) - set(key for key in document.keys()
                                      if document.get(key) is not None or
                                      not self.ignore_none_values)

self.schema is the subdocument schema, but document is always coming in as the full dict being validated. So, the list of required keys is generated correctly for each level in a document, but then the keys in the top level of the document are checked instead of the keys at the correct level.

@CD3
Copy link
Contributor

CD3 commented Jun 10, 2015

If I revert self.document back to document, undoing commit f66d8bd, it fixes the error.

@funkyfuture
Copy link
Member

well, i was the one that authored the commit in question. so, yes i'm actually using this code. can you please cherry-pick this commit and run the tests? it should fail. if not, please investigate.

i'm, too tired to hack around now.
but as @CD3 points at some interesting piece of code, my guess is that it could be rather caused because self.document is not referenced, but document.

here's my git log for comparison:

6f68def Adds a test for #107
e680644 Merge branch 'use-str.format-for-error-messages'
3213498 Changelog for #106
4936621 flake8
65e0093 use str.format
8c31395 Merge branch 'fix-usages-of-document-to-self.document'
3df80e9 Changelog for #103
20bdff7 Merge branch 'add-propertyschema-validation'
9752c8d Adds 'propertyschema'-validation rule.
f66d8bd Fixes usages of document to self.document in _validate  <- here it is.
7c37825 Merge branch 'add-wrapper-method-validate'
2ebc8e5 Improve 'validated' method visibility in the docs.
2c19edf Adds wrapper-method `validated`

@CD3
Copy link
Contributor

CD3 commented Jun 12, 2015

do you know why you made this change? the commit log doesn't say.

funkyfuture added a commit to funkyfuture/cerberus that referenced this issue Jun 12, 2015
@funkyfuture
Copy link
Member

yes, because with the introduction of coercion Validator_validate began working on a copy of the document. these were missing changes, as they referred still to the object that was passed to the method initially. and i still suspect more that has been overlooked like in __validate_required_fields.

but as long as i can't reproduce a fail, i won't investigate further. has anyone tried the test i added, and how does it behave?

@CD3
Copy link
Contributor

CD3 commented Jun 12, 2015

Now it does fail.

...
======================================================================
FAIL: test_issue_107 (cerberus.tests.tests.TestValidator)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cclark/build/cerberus/cerberus/tests/tests.py", line 861, in test_issue_107
    raise AssertionError("Document did not validate")
AssertionError: Document did not validate
----------------------------------------------------------------------
Ran 91 tests in 0.048s

Using self.assertSuccess works, creating a validator and giving a document to the validate function does not.

@funkyfuture
Copy link
Member

thanks, i will look into it.

have you already figured out the reason why self.assertSuccess doesn't fail as it should? this is the first issue to tackle down.

@CD3
Copy link
Contributor

CD3 commented Jun 12, 2015

no, I have not been able to look it yet. it creates and used the validator differently,

    def assertSuccess(self, document, schema=None, validator=None):
        if validator is None:
            validator = self.validator
        self.assertTrue(validator.validate(document, schema, update=True),
                        validator.errors)

as opposed to just

        validator = Validator(schema)
        res = validator.validate(document)

which fails. There must be some difference in the setup between these two cases.

@isaulv
Copy link

isaulv commented Jun 12, 2015

I too am affected by this issue.
I have

{"mx": {"type": "list", "required": True,
                      "schema": {"type": "dict",
                                 "schema":{
                                    "from": {"type": "string", "required": True},
                                    "ip": {"type": "string", "required": True},
                                    "host": {"type": "string", "required": True},
                                    "procs": {"type": "integer", "required": True},
                                    "mta": {"type": "string", "required": True},
                                    "port": {"type": "string", "required": True}}}}}

as my schema and I get a similar error as the poster above.

@funkyfuture funkyfuture mentioned this issue Jun 13, 2015
@funkyfuture
Copy link
Member

that's a nasty one, see #111 for some progress.

funkyfuture added a commit to funkyfuture/cerberus that referenced this issue Jun 18, 2015
funkyfuture added a commit to funkyfuture/cerberus that referenced this issue Jun 18, 2015
funkyfuture added a commit to funkyfuture/cerberus that referenced this issue Jun 19, 2015
nicolaiarocci pushed a commit that referenced this issue Jun 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants