Opened 4 years ago

Closed 4 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 4 years ago.
Decimal validation patch, DEMO ONLY -- do not apply, pending design decision

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 4 years ago by aaugustin

#14659 requested the opposite behavior and was rejected.

#13916 is slightly related.

comment:3 Changed 4 years ago by ShawnMilo

  • Owner changed from nobody to ShawnMilo

Changed 4 years ago by ShawnMilo

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

comment:4 follow-up: Changed 4 years ago by ShawnMilo

  • Triage Stage changed from Accepted to Design 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.

comment:5 in reply to: ↑ 4 Changed 4 years ago by russellm

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 Changed 4 years ago by ShawnMilo

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 Changed 4 years ago by aaugustin

  • Resolution set to wontfix
  • Status changed from new to closed
  • 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