Merge pull request #2050 from bagerard/change_custom_field_validation_raise

custom field validator is now expected to raise a ValidationError

docs/changelog.rst

@@ -5,6 +5,8 @@ Changelog
 Development
 ===========
 - Add support for MongoDB 3.6 and Python3.7 in travis
+- BREAKING CHANGE: Changed the custom field validator (i.e `validation` parameter of Field) so that it now requires:
+    the callable to raise a ValidationError (i.o return True/False).
 - Fix querying on List(EmbeddedDocument) subclasses fields #1961 #1492
 - Fix querying on (Generic)EmbeddedDocument subclasses fields #475
 - expose `mongoengine.connection.disconnect` and `mongoengine.connection.disconnect_all`

mongoengine/base/fields.py

@@ -11,8 +11,7 @@ from mongoengine.base.common import UPDATE_OPERATORS
 from mongoengine.base.datastructures import (BaseDict, BaseList,
                                              EmbeddedDocumentList)
 from mongoengine.common import _import_class
-from mongoengine.errors import ValidationError
+from mongoengine.errors import DeprecatedError, ValidationError
-
 
 __all__ = ('BaseField', 'ComplexBaseField', 'ObjectIdField',
            'GeoJsonBaseField')
@@ -53,8 +52,8 @@ class BaseField(object):
             unique with.
         :param primary_key: Mark this field as the primary key. Defaults to False.
         :param validation: (optional) A callable to validate the value of the
-            field.  Generally this is deprecated in favour of the
+            field.  The callable takes the value as parameter and should raise
-            `FIELD.validate` method
+            a ValidationError if validation fails
         :param choices: (optional) The valid choices
         :param null: (optional) If the field value can be null. If no and there is a default value
             then the default value is set
@@ -226,10 +225,18 @@ class BaseField(object):
         # check validation argument
         if self.validation is not None:
             if callable(self.validation):
-                if not self.validation(value):
+                try:
-                    self.error('Value does not match custom validation method')
+                    # breaking change of 0.18
+                    # Get rid of True/False-type return for the validation method
+                    # in favor of having validation raising a ValidationError
+                    ret = self.validation(value)
+                    if ret is not None:
+                        raise DeprecatedError('validation argument for `%s` must not return anything, '
+                                              'it should raise a ValidationError if validation fails' % self.name)
+                except ValidationError as ex:
+                    self.error(str(ex))
             else:
-                raise ValueError('validation argument for "%s" must be a '
+                raise ValueError('validation argument for `"%s"` must be a '
                                  'callable.' % self.name)
 
         self.validate(value, **kwargs)

mongoengine/errors.py

@@ -6,7 +6,7 @@ from six import iteritems
 __all__ = ('NotRegistered', 'InvalidDocumentError', 'LookUpError',
            'DoesNotExist', 'MultipleObjectsReturned', 'InvalidQueryError',
            'OperationError', 'NotUniqueError', 'FieldDoesNotExist',
-           'ValidationError', 'SaveConditionError')
+           'ValidationError', 'SaveConditionError', 'DeprecatedError')
 
 
 class NotRegistered(Exception):
@@ -142,3 +142,8 @@ class ValidationError(AssertionError):
         for k, v in iteritems(self.to_dict()):
             error_dict[generate_key(v)].append(k)
         return ' '.join(['%s: %s' % (k, v) for k, v in iteritems(error_dict)])
+
+
+class DeprecatedError(Exception):
+    """Raise when a user uses a feature that has been Deprecated"""
+    pass

tests/fields/fields.py

@@ -12,6 +12,7 @@ from mongoengine import Document, StringField, IntField, DateTimeField, DateFiel
     FieldDoesNotExist, EmbeddedDocumentListField, MultipleObjectsReturned, NotUniqueError, BooleanField,\
     ObjectIdField, SortedListField, GenericLazyReferenceField, LazyReferenceField, DynamicDocument
 from mongoengine.base import (BaseField, EmbeddedDocumentList, _document_registry)
+from mongoengine.errors import DeprecatedError
 
 from tests.utils import MongoDBTestCase
 
@@ -56,6 +57,48 @@ class FieldTest(MongoDBTestCase):
         self.assertEqual(
             data_to_be_saved, ['age', 'created', 'day', 'name', 'userid'])
 
+    def test_custom_field_validation_raise_deprecated_error_when_validation_return_something(self):
+        # Covers introduction of a breaking change in the validation parameter (0.18)
+        def _not_empty(z):
+            return bool(z)
+
+        class Person(Document):
+            name = StringField(validation=_not_empty)
+
+        Person.drop_collection()
+
+        error = ("validation argument for `name` must not return anything, "
+                 "it should raise a ValidationError if validation fails")
+
+        with self.assertRaises(DeprecatedError) as ctx_err:
+            Person(name="").validate()
+        self.assertEqual(str(ctx_err.exception), error)
+
+        with self.assertRaises(DeprecatedError) as ctx_err:
+            Person(name="").save()
+        self.assertEqual(str(ctx_err.exception), error)
+
+    def test_custom_field_validation_raise_validation_error(self):
+        def _not_empty(z):
+            if not z:
+                raise ValidationError('cantbeempty')
+
+        class Person(Document):
+            name = StringField(validation=_not_empty)
+
+        Person.drop_collection()
+
+        with self.assertRaises(ValidationError) as ctx_err:
+            Person(name="").validate()
+        self.assertEqual("ValidationError (Person:None) (cantbeempty: ['name'])", str(ctx_err.exception))
+
+        with self.assertRaises(ValidationError):
+            Person(name="").save()
+        self.assertEqual("ValidationError (Person:None) (cantbeempty: ['name'])", str(ctx_err.exception))
+
+        Person(name="garbage").validate()
+        Person(name="garbage").save()
+
     def test_default_values_set_to_None(self):
         """Ensure that default field values are used even when
         we explcitly initialize the doc with None values.