Opened 3 years ago
Closed 3 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 , 3 years ago
follow-up: 3 comment:2 by , 3 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 NaNs. I don't think we should do more than that.
comment:3 by , 3 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
NaNs. 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 , 3 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 , 3 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:6 by , 3 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.