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

Price (and other Number based) field bug/question #86

Closed
philtay opened this Issue Dec 7, 2014 · 9 comments

Comments

Projects
None yet
2 participants
@philtay
Contributor

philtay commented Dec 7, 2014

Below the steps to reproduce the bug:

from marshmallow.fields import Price

p = Price()
p.deserialize('23.545')
p.deserialize('23.555')

assert p.deserialize('23.545') != p.deserialize('23.555')

The problem is ROUND_HALF_EVEN used in utils.decimal_to_fixed.
In general, it's false that ROUND_HALF_EVEN is the right rounding method for an amount of money.
For instance, in Europe the law prescribes the use of ROUND_HALF_UP to round amounts in EUR currency.

Moreover the use of float for money is very dangerous. Something we are going to regret.
More info here: http://spin.atomicobject.com/2014/08/14/currency-rounding-errors/
(a quick search on google will show a lot of horror stories about this)

Last but not least a question. Why it deserializes to a string and not to a Decimal?

@sloria

This comment has been minimized.

Member

sloria commented Dec 8, 2014

Yes, the Fixed and Price implementations are indeed incorrect. These were ported from Flask-RESTful in the early days of marshmallow and haven't gone under much scrutiny since, so thanks for catching the bug.

How about we take the following actions for release 1.2.0:

  • Add a fields.Decimal field which deserializes to a Python Decimal object and serializes to a string (basically the str of the Decimal object)
  • Mark fields.Fixed for PendingDeprecation
  • Add a FutureWarning to the fields.Price.deserialize method, warning that it will be changed to return a Decimal in version 2.0.

Then, in 2.0:

  • Deprecate or completely remove fields.Fixed.
  • Make fields.Price a subclass of fields.Decimal so that it correctly deserializes to a Decimal.

Thoughts?

@sloria

This comment has been minimized.

Member

sloria commented Dec 8, 2014

Oh, and also change the rounding to ROUND_HALF_UP to fix the bug. This could happen in 1.2.0, but it is technically a breaking change, so this may need to go into 2.0.

We could also add a rounding parameter to fields.Fixed as a stopgap (default would be the current value,--ROUND_HALF_EVEN--but the user could override this).

@philtay

This comment has been minimized.

Contributor

philtay commented Dec 8, 2014

Add a fields.Decimal field which deserializes to a Python Decimal object and serializes to a string (basically the str of the Decimal object)

You already have my implementation of it. #72
There are 3 new fields in this pull request. The third is called Numeric. Please review it and I'll open a new pull request.
The only difference is that serializes to a Decimal and not to a string. This because you want it to be a number in your JSON and not a string. json.dump / load understands Decimal.

Mark fields.Fixed for PendingDeprecation

Sure.

Add a FutureWarning to the fields.Price.deserialize method, warning that it will be changed to return a Decimal in version 2.0.

Uhm, no. Mark it for PendingDeprecation as well. It doesn't make any sense as a field when you have Numeric. They will do the same thing.

Oh, and also change the rounding to ROUND_HALF_UP to fix the bug.

No, don't do this. For instance, ROUND_HALF_EVEN is good for USD (but not for EUR). Just mark the field for PendingDeprecation without any further change.

@sloria sloria added this to the 1.2.0 milestone Dec 8, 2014

@sloria

This comment has been minimized.

Member

sloria commented Dec 9, 2014

The third is called Numeric. Please review it and I'll open a new pull request.

Looks good after a quick pass. Made a few suggestions for minor changes. Will review again when you send another PR.

json.dump / load understands Decimal.

I wasn't able to serialize a Decimal object:

>>> import json
>>> from decimal import Decimal
>>> json.dumps(Decimal('12.34'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/Cellar/python/2.7.8_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/__init__.py", line 243, in dumps
    return _default_encoder.encode(obj)
  File "/usr/local/Cellar/python/2.7.8_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 207, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/local/Cellar/python/2.7.8_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 270, in iterencode
    return _iterencode(o, 0)
  File "/usr/local/Cellar/python/2.7.8_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 184, in default
    raise TypeError(repr(o) + " is not JSON serializable")
TypeError: Decimal('12.34') is not JSON serializable

Uhm, no. Mark it for PendingDeprecation as well. It doesn't make any sense as a field when you have Numeric. They will do the same thing.

My thought was to include the Price as a convenient shorthand, similar to colander's Money: https://github.com/Pylons/colander/blob/1.0-branch/colander/__init__.py#L1242-L1256 . But one could make a strong argument that the a Price field makes too many assumptions about locality for only a minor convenience gain.

So sure, I think it can be marked for PendingDeprecation.

@philtay

This comment has been minimized.

Contributor

philtay commented Dec 9, 2014

@sloria

I wasn't able to serialize a Decimal object

import simplejson as json
from decimal import Decimal

json.dumps({'foo':Decimal('12.34')})

The result is: '{"foo": 12.34}'. Instead, having it in this way: '{"foo": "12.34"}' is very wrong (note that now "foo" is a string and not a number anymore).

@philtay

This comment has been minimized.

Contributor

philtay commented Dec 9, 2014

Before you say it. Yes, simplejson IS a requirement!

@sloria

This comment has been minimized.

Member

sloria commented Dec 9, 2014

We can't assume the user is using any custom JSON encoder. Marshmallow fields are meant to serialize to primitive types.

Number fields already have an as_string parameter. Let's use that.

@philtay

This comment has been minimized.

Contributor

philtay commented Dec 9, 2014

Done. It's here #89 with the class based validators.

@sloria

This comment has been minimized.

Member

sloria commented Dec 20, 2014

Closed by #103

@sloria sloria closed this Dec 20, 2014

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