heynemann / motorengine

Motorengine is a port of MongoEngine for Tornado.
http://motorengine.readthedocs.org
204 stars 67 forks source link

Can IntField, FloatField, UUIDField be optional ? #75

Closed partyzan closed 10 years ago

partyzan commented 10 years ago

Currently validation of the document fails if one of these fields is not present even if it is marked as optional. Is that a design decision or a bug ?

    @gen_test
    def test_empty_field(self):

        class A(Document):

            int_field = IntField(required=False)
            float_field = FloatField(required=False)
            UUIDField = UUIDField(required=False)

        a = yield A.objects.create(int_field=1)

        self.assertIsNotNone(a)
        self.assertIsNone(a.float_field)
        self.assertIsNone(a.UUIDField)

        a = yield A.objects.create(float_field=1.0)

        self.assertIsNotNone(a)
        self.assertIsNone(a.int_field)
        self.assertIsNone(a.UUIDField)

        a = yield A.objects.create(uuid_field=uuid4())

        self.assertIsNotNone(a)
        self.assertIsNone(a.float_field)
        self.assertIsNone(a.int_field)

The test fails on the first with the following:

motorengine.errors.InvalidDocumentError: Field 'UUIDField' must be valid.

According to what I see in the code, for StringField validate checks if it is None and returns true, while for int for example it just tries to do int(value) which obviously fails.

class StringField(BaseField):
...
    def validate(self, value):
        if value is None:
            return True
...
class IntField(BaseField):
...
     def validate(self, value):
        try:
            value = int(value)
        except:
            return False
...

Thanks for the help :)

partyzan commented 10 years ago

Looking at the Document.validate_fields makes me think this is a bug and the validate methods in the fields should be changed to support None.

 def validate_fields(self):
        for name, field in list(self._fields.items()):

            value = self.get_field_value(name)

            if field.required and field.is_empty(value):
                raise InvalidDocumentError("Field '%s' is required." % name)
            if not field.validate(value):
                raise InvalidDocumentError("Field '%s' must be valid." % name)

        return True
partyzan commented 10 years ago

If this is indeed a bug, I created a pull request with a simple fix - #76

partyzan commented 10 years ago

Hey, I see the change was merged. Any estimate on when it will be released ?

Thanks

heynemann commented 10 years ago

Released under 0.8.10. thanks a lot for your contribution @partyzan.

partyzan commented 10 years ago

Unfortunately my previous fix was not complete, it failed to serialize and deserialize empty values. I have fixed that and added unit tests that test empty fields. See pull request #77.

Thanks.