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

Setter does not behave as it should. #1059

Open
cenouro opened this issue Jul 3, 2015 · 3 comments
Open

Setter does not behave as it should. #1059

cenouro opened this issue Jul 3, 2015 · 3 comments
Labels

Comments

@cenouro
Copy link

cenouro commented Jul 3, 2015

import mongoengine as me

class Product(me.Document):
    price = me.DecimalField(force_string=True, precision=2)

p = Product(price=10.50)
p.to_mongo()
# >> SON([('price', '10.50')])

p = Product()
p.price = 10.50
p.to_mongo()
# >> SON([('price', '10.5')])

Am I doing something wrong?

@touilleMan touilleMan added the Bug label Jul 10, 2015
@touilleMan
Copy link
Member

Hi,
I've created an unittest that show the bug (see touilleMan@bc7b4a9)

From my point of view, the trouble is when price is passed during document creation, it is altered by Product.price.to_python before being set.
On the other hand, using p.price = 10.50 means p.price is a regular float

>>> p = Product(price=10.50).save()
>>> type(p.btc)
<class 'decimal.Decimal'>
>>> p.price = 10.50
>>> p.save()
>>> type(p.btc)
builtins.float

I guess we should overload Document setter to call to_python

@touilleMan
Copy link
Member

I've pushed a first quick fix to use to_python in BaseDocument.__setattr__ (see touilleMan@a3919ef)

Plenty of tests are broken with this, but I'm wondering if this doesn't hide a bigger refactoring need, see for example:

511         def test_boolean_validation(self):
512             """Ensure that invalid values cannot be assigned to boolean fields.
513             """
514             class Person(Document):
515                 admin = BooleanField()
516     
517             person = Person()
518             person.admin = True
519             person.validate()
520     
521             person.admin = 2
522  ->         self.assertRaises(ValidationError, person.validate)  # My fix makes the test break here
523             person.admin = 'Yes'
524             self.assertRaises(ValidationError, person.validate)
(Pdb) person.admin  # admin field has been converted by my `to_python` from an `int` to a `boolean`...
True
(Pdb) Person(admin=2).admin  # ...but it's already the behaviour when giving a field at creation time !
True

@cenouro
Copy link
Author

cenouro commented Jul 16, 2015

Could you add this to your test case?

    Person(admin=[]).admin   
    # if this returns False, then the problem is python's duck-typing

If the problem is indeed Python's duck-typing, then a good feature would be to implement a more restrictive 'type casting' on the BooleanField class (and other *Field classes for that matter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants