Opened 2 years ago
Closed 2 years ago
#33954 closed Cleanup/optimization (fixed)
"NaN" can be stored in DecimalField but cannot be retrieved
Reported by: | Xabier Bello | Owned by: | Mohamed Karam |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.1 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Same as ticket https://code.djangoproject.com/ticket/33033, but I managed to trigger it anyway:
Steps to reproduce
- Create a brand new project using python 3.10 and django 4.1 with the default sqlite3 backend.
- Create a model with a DecimalField:
class MyModel(models.Model): value = models.DecimalField(max_digits=10, decimal_places=5)
- Programmatically create a model instance with value="nan",
obj = MyModel.objects.create(value="nan") obj.save()
- Then try to retrieve the object from the database (or refresh from database):
MyModel.objects.get(pk=1)
Traceback
Traceback (most recent call last): File "/sandbox/dj/bug/dec/views.py", line 9, in <module> MyModel.objects.get(pk=1) File "/lib64/python3.10/site-packages/django/db/models/manager.py", line 85, in manager_method return getattr(self.get_queryset(), name)(*args, **kwargs) File "/lib64/python3.10/site-packages/django/db/models/query.py", line 646, in get num = len(clone) File "/lib64/python3.10/site-packages/django/db/models/query.py", line 376, in __len__ self._fetch_all() File "/lib64/python3.10/site-packages/django/db/models/query.py", line 1866, in _fetch_all self._result_cache = list(self._iterable_class(self)) File "/lib64/python3.10/site-packages/django/db/models/query.py", line 117, in __iter__ for row in compiler.results_iter(results): File "/lib64/python3.10/site-packages/django/db/models/sql/compiler.py", line 1333, in apply_converters value = converter(value, expression, connection) File "/lib64/python3.10/site-packages/django/db/backends/sqlite3/operations.py", line 344, in converter return create_decimal(value).quantize( TypeError: argument must be int or float
The value "nan" (and maybe "inf" also) skip the validation in DecimalField.to_python
, because is not None
, and is not instance of float. But decimal.Decimal("nan")
works without triggering the exception, so NaN
gets stored in the DB.
Change History (8)
follow-up: 2 comment:1 by , 2 years ago
follow-up: 3 comment:2 by , 2 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
Replying to Claude Paroz:
Django offers both model et form validation. If you insert values without using either one, you are on your own and responsible for what you insert in the database. I dont' think Django should do more here. To be confirmed.
Yes, we have both model and form validation for NaN
s. I don't think we should do more than that.
comment:3 by , 2 years ago
Description: | modified (diff) |
---|
Replying to Mariusz Felisiak:
Replying to Claude Paroz:
Django offers both model et form validation. If you insert values without using either one, you are on your own and responsible for what you insert in the database. I dont' think Django should do more here. To be confirmed.
Yes, we have both model and form validation for
NaN
s. I don't think we should do more than that.
What made me report as a bug is that in db.models.fields.DecimalField
, method to_python
, there are two shields against invalid values, one of them explicit against nan
. Both "nan" and "inf" can be weeded out early if the check is if not math.isfinite
instead of if math.isnan
, as it's actually implemented in the form validation. Currently float("inf")
fails to store in the DB as "Infinite" because it can't be quantize
'd at db/backends/utils.py::format_number
(decimal.InvalidOperation
), while "nan" can.
Notice that I'm using the Model validation, if I'm not mistaken and it refers to creating objects with MyModel.objects.create
. E.g. this works and happily stores "NaN" in the DB:
obj = MyModel.objects.create(value="nan")
But this fails with a ValidationError
raised from DecimalField.to_python
:
obj = MyModel.objects.create(value="invalid")
django.core.exceptions.ValidationError: ['“invalid” value must be a decimal number.']
And this fails with decimal.InvalidOperation
:
obj = MyModel.objects.create(value="inf")
decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]
IMHO it would be better interface to fail before saving, than to store the value and then fail on retrieving. The form validation does this, creating a valid Decimal
even if the value entered are the strings "nan" or "inf" (in to_python()
), and then check that only Decimal.is_finite()
are valid values (in validate()
).
This would be my refactor to DecimalField.to_python
, without adding any code, to consistently raise a ValidationError for all "inf", "nan" and invalid inputs, instead of one for each.
1703 def to_python(self, value): 1704 if value is None: 1705 return value 1706 if isinstance(value, float): 1707 result = self.context.create_decimal_from_float(value) 1708 try: 1709 result = decimal.Decimal(value) 1710 except (decimal.InvalidOperation, TypeError, ValueError): # Catches everything except "nan" and "inf" 1711 raise exceptions.ValidationError( 1712 self.error_messages["invalid"], 1713 code="invalid", 1714 params={"value": value}, 1715 ) 1716 if not result.is_finite(): # Catches both "nan" and "inf" 1717 raise exceptions.ValidationError( 1718 self.error_messages["invalid"], 1719 code="invalid", 1720 params={"value": value}, 1721 ) 1722 return result
Thanks for your time!
comment:4 by , 2 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
Summary: | NaN can be stored in DecimalField but cannot be retrieved → "NaN" can be stored in DecimalField but cannot be retrieved |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
Notice that I'm using the Model validation, if I'm not mistaken and it refers to creating objects with
MyModel.objects.create
Not really, .create()
and .save()
don't call full_clean()
(see docs). We added extra guards in #33033 because on some databases (SQLite and PostgreSQL) NaN
values are accepted without raising any database-level errors.
I agree that we should add the same checks for "nan"
strings.
comment:5 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 2 years ago
Has patch: | set |
---|
Django offers both model et form validation. If you insert values without using either one, you are on your own and responsible for what you insert in the database. I dont' think Django should do more here. To be confirmed.