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 Xabier Bello)

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)

comment:1 by Claude Paroz, 2 years ago

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.

in reply to:  1 ; comment:2 by Mariusz Felisiak, 2 years ago

Resolution: invalid
Status: newclosed

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.

in reply to:  2 comment:3 by Xabier Bello, 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 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 Mariusz Felisiak, 2 years ago

Resolution: invalid
Status: closednew
Summary: NaN can be stored in DecimalField but cannot be retrieved"NaN" can be stored in DecimalField but cannot be retrieved
Triage Stage: UnreviewedAccepted
Type: BugCleanup/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 Mohamed Karam, 2 years ago

Owner: changed from nobody to Mohamed Karam
Status: newassigned

comment:6 by Mohamed Karam, 2 years ago

Has patch: set

comment:7 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In b92ffebb:

Fixed #33954 -- Prevented models.DecimalField from accepting NaN, Inf, and -Inf values.

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