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

Nested schemas requires all validations to pass #269

Closed
jomag opened this Issue Sep 1, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@jomag

jomag commented Sep 1, 2015

If the validation of a single field in a nested schema fails, then values of the entire nested schema are thrown away. Example:

import marshmallow as ma

class Child(ma. Schema):
    a = ma.fields.Integer()
    b = ma.fields.Integer()

class Parent(ma.Schema):
    x = ma.fields.Integer()
    y = ma.fields.Integer()
    z = ma.fields.Nested(Child)

p = Parent()

If I call p.load() with a dict that has 'x' set to a valid integer, and 'y' set to None, the returned data has one value, and the error list contains on error message for 'y'

But if I do the same thing with the child instance ('z') and set for example 'a' to None, and 'b' to a valid value, then the entire nested schema ('z') is absent from the data returned by p.load()

I'm trying to use marshmallow for saving settings to disk. I do not want the loading method to skip an entire nested object just because one of the values of it are invalid.

Any ideas?

@jomag

This comment has been minimized.

jomag commented Sep 2, 2015

I wrote a small test program:

import marshmallow as ma
from pprint import pprint

class SchemaA(ma.Schema):
    x = ma.fields.Integer()
    y = ma.fields.Integer()
    z = ma.fields.Integer()

class SchemaB(ma.Schema):
    w = ma.fields.Integer()
    n = ma.fields.Nested(SchemaA)

def test(schema_class, input_data):
    print('-' * 80)
    print("Input data:")
    pprint(input_data)

    sch = schema_class()
    data, errors = sch.load(input_data)

    print("Validated data:")
    pprint(data)

    print("Validation errors:")
    pprint(errors)

test(SchemaA, { 'x': 50, 'y': 60, 'z': None })
test(SchemaB, { 'w': 90, 'n': { 'x': 90, 'y': 89, 'z': None} })
print('-' * 80)

Output:

--------------------------------------------------------------------------------
Input data:
{'x': 50, 'y': 60, 'z': None}
Validated data:
{'x': 50, 'y': 60}
Validation errors:
{'z': [u'Field may not be null.']}
--------------------------------------------------------------------------------
Input data:
{'n': {'x': 90, 'y': 89, 'z': None}, 'w': 90}
Validated data:
{'w': 90}
Validation errors:
{'n': {'z': [u'Field may not be null.']}}
--------------------------------------------------------------------------------

As for the second test I would want the validated data to be:

{ 'w': 90, 'n': { 'x': 90, 'y': 89 } }

And the errors:

{ 'n': { 'z': [ u'Field may not be null' ] } }
@jomag

This comment has been minimized.

jomag commented Sep 2, 2015

The following patch fixes the problem, but I'm not familiar with the code base so it is a quite ugly hack ... It applies cleanly to tag 2.0.0b5.

diff --git a/marshmallow/exceptions.py b/marshmallow/exceptions.py
index 706b28f..4ad51bd 100644
--- a/marshmallow/exceptions.py
+++ b/marshmallow/exceptions.py
@@ -39,6 +39,12 @@ class ValidationError(MarshmallowError):
         MarshmallowError.__init__(self, message)


+class NestedValidationError(ValidationError):
+    def __init__(self, message, value=None, field_names=None, fields=None):
+        super(NestedValidationError, self).__init__(message, field_names, fields)
+        self.value = value
+
+
 class RegistryError(NameError):
     """Raised when an invalid operation is performed on the serializer
     class registry.
diff --git a/marshmallow/fields.py b/marshmallow/fields.py
index 0990f11..5f7a63a 100755
--- a/marshmallow/fields.py
+++ b/marshmallow/fields.py
@@ -13,7 +13,7 @@ from marshmallow import validate, utils, class_registry
 from marshmallow.base import FieldABC, SchemaABC
 from marshmallow.utils import missing as missing_
 from marshmallow.compat import text_type, basestring
-from marshmallow.exceptions import ValidationError
+from marshmallow.exceptions import ValidationError, NestedValidationError

 __all__ = [
     'Field',
@@ -385,7 +385,7 @@ class Nested(Field):

         data, errors = self.schema.load(value)
         if errors:
-            raise ValidationError(errors)
+            raise NestedValidationError(errors, data)
         return data

     def _validate_missing(self, value):
diff --git a/marshmallow/marshalling.py b/marshmallow/marshalling.py
index 8bbb42d..b531a74 100644
--- a/marshmallow/marshalling.py
+++ b/marshmallow/marshalling.py
@@ -14,7 +14,7 @@ from marshmallow import utils
 from marshmallow.utils import missing
 from marshmallow.compat import text_type, iteritems
 from marshmallow.exceptions import (
-    ValidationError,
+    ValidationError, NestedValidationError
 )

 __all__ = [
@@ -71,7 +71,10 @@ class ErrorStore(object):
                 errors[field_name] = err.messages
             else:
                 errors.setdefault(field_name, []).extend(err.messages)
-            value = missing
+            if isinstance(err, NestedValidationError):
+                value = err.value
+            else:
+                value = missing
         return value

 class Marshaller(ErrorStore):

@sloria sloria added this to the 2.0.0 (final) milestone Sep 6, 2015

@sloria sloria closed this in 02de91b Sep 11, 2015

@sloria

This comment has been minimized.

Member

sloria commented Sep 11, 2015

This is now fixed. I used a similar solution to the one above, without adding a new Exception class. Thanks @jomag for the suggestion and your work on finding a solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment