Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Recursively validate documents and complex fields #328

Closed
n1k0 opened this Issue · 22 comments

4 participants

@n1k0

As far as I can see, validating a document or a ComplexBaseField won't give all the validation errors but only the first one encountered. Also, errors on EmbeddedDocumentField will just give something like 'Invalid EmbeddedDocumentField item (FooBaz)'which is hard to debug, to say the least.

What would be cool is to have access to some property in the ValidationError instance, eg. an errors one which could give something like:

def print_errors(errors):
    for field_name, error in errors.iteritems():
        if isinstance(error, dict):
            print_errors(error)
        else:
            print 'error for %s: %s' % (field_name, error, )

try:
    obj.validate()
except ValidationError, err:
    print_errors(err.errors)

The errors property would basically contain a ditionnary with the validation errors:

errors = {
    'field1': {
        'subfield1': 'this field is required',
        'subfield2': 'wrong type provided',
    },
    'field2': 'Email is invalid',
}

I can eventually work on a patch in my fork, but I would love to hear your thoughts and feedback on this proposition first.

@linuxnow

It would be great if you could add an atribute like in mongokit. I enclose it's description, just a like a model we could adopt

Extract from MongoKit's documentation:

Quiet validation detection
By default, when validation is on each error raises an Exception. Sometime, you just want to collect errors in one place. This is possible by setting the raise_validation_errors to False. At this moment, all errors are store in the validation_errors attribute:

class MyDoc(Document):
... raise_validation_errors = False
... structure={
... "foo":set,
... }
con.register([MyDoc])
doc = tutorial.MyDoc()
doc.validate()
doc.validation_errors
{'foo': [StructureError(" is not an authorized type",), RequireFieldError('foo is required',)]}
validation_errors is a dictionary which take the field name as key and the python exception as value. Here foo has two issues : a structure one (set is not an authorized type) and is required.

doc.validation_errors['foo'][0].message
" is not an authorized type"

@n1k0

Yes, that's quite exactly what I had in mind actually. I would love to start working on the feature, on which branch should I implement it? I'm thinking about dev, but I could also create a new feature branch named validation-schema. Thoughts?

Also, I'm wondering about the current test suite, as the one in dev fail (tests/fields.py):

$ python tests/fields.py 
.....F123
.........'abc'
...................'abc'
..1
'a'
<Comment: Comment object>
<User: User object>
1
<User: User object>
<Comment: Comment object>
.'abc'
...............E
======================================================================
ERROR: test_uuid_validation (__main__.FieldTest)
Ensure that invalid values cannot be assigned to UUID fields.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/fields.py", line 179, in test_uuid_validation
    class Person(Document):
  File "tests/fields.py", line 180, in Person
    api_key = UUIDField()
NameError: name 'UUIDField' is not defined

======================================================================
FAIL: test_complex_field_required (__main__.FieldTest)
Ensure required cant be None / Empty
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/fields.py", line 557, in test_complex_field_required
    self.assertRaises(ValidationError, e.save)
AssertionError: ValidationError not raised

----------------------------------------------------------------------
Ran 53 tests in 0.689s

FAILED (failures=1, errors=1)

Should I start a new branch based on master instead?

Any hint much appreciated.

@n1k0

Hmm, my bad, messed up with my virtualenvs. I'll start to work from the dev branch.

@linuxnow

You should fork a new branch form dev, as you suggest, so that the merge is easy and only for one feature.
Please go for it, if needed we can make a few changes afterwards, but this is definately needed in ordfer to do batch validation of all the fields.

@n1k0
@n1k0

Here's a first draft of the feature, with tests: https://github.com/n1k0/mongoengine/tree/validation-schema

I would like to hear some feedback before sending any PR :)

PS: basically the diff is here n1k0@3d7b30d

@ultramiraculous

Thanks for doing this work n1k0! This has been sorely needed in multiple cases in our project, but we haven't had time to sit down and code up a fix.

Edit: Read through the diffs and I'm really excited. Thanks again!

@linuxnow

I've added a comment o the patch. Thanks for your work!

@n1k0

Indeed I didn't added any option flag but as you can see all of this is entirely backward compatible (run the tests), so as it doesn't have any significative impact on performances I wonder why we should add an option?

By default it works as before the patch, but if you want you can optionally retrieve an errors attribute of the raised ValidationError to fetch the whole validation result schema, recursively. You'll keep having to catch and get a ValidationError, any case :)

@n1k0

I added a ValidationError.schema property to get directly a standard Python dict containing the whole validation error schema: n1k0@62480fe

If you're okay, I'll now work on updating the documentation to reflect these new features (I'm still hearing your comments in the meanwhile). Then, I'll send a PR.

@linuxnow

A small question: do we need both errors and schema? Why have both? Again excuse me if I don't get it, I'm just reading the code.

@n1k0

A small question: do we need both errors and schema? Why have both?

Well schema is just another representation of errors. Basically:

>>> from mongoengine import *
>>> v = ValidationError('plop', errors={
...     'foo': ValidationError('bar'),
...     'bar': ValidationError('baz', errors={
...         'baz': ValidationError('inception'),
...     }),
... })
>>> print v.errors
{'bar': ValidationError(baz,), 'foo': ValidationError(bar,)}
>>> print v.schema
{'bar': {'baz': u'inception'}, 'foo': u'bar'}

Here calling schema is actually the simplest way to retrieve the inception error directly :)

@linuxnow

IMO (and I'm not the mantainer) I'd only keep errors. We can add a helper function to extract a schema-like structure.
For the rest this is joy to have, I hope that rozza finds some time to pull it soon.

@n1k0

We can add a helper function to extract a schema-like structure.

Actually the schema property is probably the helper you're talking about: https://github.com/n1k0/mongoengine/blob/validation-schema/mongoengine/base.py#L46

I can eventually move the get_schema local function elsewhere, but I have no idea where… hints?

Though I'd love keeping the schema property accessor because it's easy and straightforward to use and don't require the developer to import a dedicated external utility :)

Another idea would be to create a dedicated ErrorSchema class so we could have something like this in the end:

>>> from mongoengine import *
>>> v = ValidationError('plop', errors={
...     'foo': ValidationError('bar'),
...     'bar': ValidationError('baz', errors={
...         'baz': ValidationError('inception'),
...     }),
... })
>>> print v.errors
{'bar': ValidationError(baz,), 'foo': ValidationError(bar,)}
>>> print v.errors.schema
{'bar': {'baz': u'inception'}, 'foo': u'bar'}

IMO (and I'm not the mantainer)

I'd love hearing his feedback :)

@linuxnow

I can eventually move the get_schema local function elsewhere, but I have no idea where… hints?

I wouldn't call it schema, as it is confusing, so get _schema should be something else like get_validation_errors.
As the code is now, I'd put in under get_document in mongoengine/base.py, but TBH I don't like it much.

I'd rather arrange the code a bit, creating a utils.py (or a dir if there are a few distinct categories), and and exceptions.py where I'd add all my exceptions and related functions. In this case this would be its place, but this is IMHO.

I'd love hearing his feedback :)

He told me he had some crazy work days, don't worry, he's usually super responsive. Once he has the time to review it, hell probably pull it soon.

@n1k0

I wouldn't call it schema, as it is confusing, so get _schema should be something else like get_validation_errors.

Yes, you're right, I don't like schema neither. Naming things's definitely the hardest part of the work, as usual. In fact the more I think about it, the more I feel like validation_error.errors.to_dict() or to_python() is a good compromise.

As the code is now, I'd put in under get_document in mongoengine/base.py, but TBH I don't like it much.

Neither do I…

I'd rather arrange the code a bit, creating a utils.py (or a dir if there are a few distinct categories), and and exceptions.py where I'd add all my exceptions and related functions. In this case this would be its place, but this is IMHO.

I agree but this could break backward compatibility… I'll wait a bit for maintainer's feedback.

@linuxnow

I think you should make a pull resquest!
And, IMO, extend this behaviour to whole document validation too.

@n1k0

And, IMO, extend this behaviour to whole document validation too.

Well it is the case atm, or am I misunderstanding what you say? :)

Look at the test: https://github.com/n1k0/mongoengine/blob/validation-schema/tests/fields.py#L1565

@linuxnow

For a moment I was confused by the title "Retrieve all validation errors for ComplexBaseField, recursively" and was too lazy to check :p

Update the title!!!

@n1k0

Update the title!!!

Sir, yes Sir! :-)

@linuxnow

Doesn't it look cute? :)

@rozza rozza referenced this issue from a commit
@rozza rozza Renamed schema for errors
Now is `to_dict()` as is more explicit
[refs #344 #328]
3ee60af
@rozza

See #344 - this has been merged with a slight change schema is now to_dict() thanks for the code and the conversation guys :)

@rozza rozza closed this
@drudim drudim referenced this issue from a commit
@rozza rozza Documentation cleanup (#328) c96a1b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.