Opened 13 years ago

Closed 13 years ago

#16015 closed Bug (wontfix)

DecimalField allows invalid default values.

Reported by: ShawnMilo Owned by: ShawnMilo
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

For example, a float may be passed in a DecimalField instantiation. This "works" because get_default coerces to unicode.

Attachments (1)

16015_DecimalField_invalid_defaults.diff (2.9 KB ) - added by ShawnMilo 13 years ago.
Decimal validation patch, DEMO ONLY -- do not apply, pending design decision

Download all attachments as: .zip

Change History (8)

comment:1 by Aymeric Augustin, 13 years ago

Triage Stage: UnreviewedAccepted

Accepting ticket based on Russell's comment on the mailing list:
http://groups.google.com/group/django-developers/browse_thread/thread/637426c493f490d8

comment:2 by Aymeric Augustin, 13 years ago

#14659 requested the opposite behavior and was rejected.

#13916 is slightly related.

comment:3 by ShawnMilo, 13 years ago

Owner: changed from nobody to ShawnMilo

by ShawnMilo, 13 years ago

Decimal validation patch, DEMO ONLY -- do not apply, pending design decision

comment:4 by ShawnMilo, 13 years ago

Triage Stage: AcceptedDesign decision needed

I've created a patch that works, but there's a problem. As of Python 2.7, you're suddenly allowed to instantiate a decimal.Decimal object with a float value.

http://docs.python.org/library/decimal.html

With Python2.6 my patch will result in a passing test. With Python 2.7, it will fail:

======================================================================
FAIL: runTest (__main__.InvalidModelTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./runtests.py", line 93, in runTest
    self.assertTrue(not missing, "Missing Errors: " + '\n'.join(missing))
AssertionError: False is not True : Missing Errors: invalid_models.fielderrors: "decimalfield6": DecimalFields require a "default" attribute that is of type Decimal.

--------------------------------"--------------------------------------

So, is it better to keep the currently technically-wrong-but-working state? I contacted the user who originally reported the confusion and they said they were using Python 2.6.1, so everything is "fine" unless they then try to create a queryset using a float.

in reply to:  4 comment:5 by Russell Keith-Magee, 13 years ago

Replying to ShawnMilo:

I've created a patch that works, but there's a problem. As of Python 2.7, you're suddenly allowed to instantiate a decimal.Decimal object with a float value.

It's hard to believe someone looked at the following example in the Python docs:

>>> Decimal(3.14)
Decimal('3.140000000000000124344978758017532527446746826171875')

and thought "that's a really good idea". And yet...

So, is it better to keep the currently technically-wrong-but-working state? I contacted the user who originally reported the confusion and they said they were using Python 2.6.1, so everything is "fine" unless they then try to create a queryset using a float.

I'm torn on this one. On the one hand, it is desirable to be consistent across Django versions. It is also desirable to compensate for the inconsistencies across versions in Python as a language. However, in this case, the inconsistency appears to have been deliberately introduced, and will presumably be the "one true way" going forward, which is in conflict with what would seem to be to be the right behavior.

My personal preference would be to retain the purity of the "float != Decimal" distinction; but that would put Django at conflict the declared direction of Python as a language, and I don't think it's Django's place to change the expectations of the underlying language. My inclination is that the best approach here will be to catch the error under Python 2.6, and use the float_to_decimal implementation described in the Decimal FAQ.

comment:6 by ShawnMilo, 13 years ago

Russell,

Since there's currently no error as-is in Python 2.6 or 2.7 (unless we introduce the patch above), where are you suggesting that we implement the float_to_decimal functionality?

Part of my confusion with this ticket is that get_default always returns a unicode object anyway -- for all field types. That could either be seen as a design flaw or as a "failsafe" because database backends all know how to coerce a string to the required type.

If get_default is as it should be, then it appears that this ticket should be closed with 'wontfix' unless I'm missing something.

comment:7 by Aymeric Augustin, 13 years ago

Resolution: wontfix
Status: newclosed
UI/UX: unset

I confirm that Python >= 2.7 allows instantiating a Decimal from a float:

% python2.6
>>> from decimal import Decimal
>>> Decimal(0.5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/decimal.py", line 649, in __new__
    "First convert the float to a string")
TypeError: Cannot convert float to Decimal.  First convert the float to a string

% python2.7
>>> from decimal import Decimal
>>> Decimal(0.5)
Decimal('0.5')

% python3.2
>>> from decimal import Decimal
>>> Decimal(0.5)
Decimal('0.5')

I'm a bit surprised by this change, but I understand the reasons, and I think it's better to follow Python's lead here.

Also, a developer who uses DecimalField instead of FloatField is probably conscious of the difference between a Decimal and a float, and will know why DecimalField(default=0.3) isn't a good idea.

Note: See TracTickets for help on using tickets.
Back to Top