Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#34260 closed Cleanup/optimization (wontfix)

models.FloatField documentation doesn't mention that +inf, -inf, and NaN are database-dependent.

Reported by: Matt Cooper Owned by: nobody
Component: Documentation Version: 4.1
Severity: Normal Keywords: floatfield
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Matt Cooper)

The model field reference documentation doesn't mention that, in a FloatField, non-numeric values have database-dependent behavior. float('nan') on SQLite requires the field to be nullable, for example. I saw old discussion about why this is the case, but only after being bitten by it in a project. I'd like to submit a patch adding this info to the docs.

Change History (12)

comment:1 by Matt Cooper, 2 years ago

https://github.com/django/django/pull/16455 proposes a fix for the dev documentation. This change could (should?) also be backported to all supported Django versions.

comment:2 by Mariusz Felisiak, 2 years ago

Resolution: invalid
Status: newclosed
Summary: Field reference documentation doesn't mention that +inf, -inf, and NaN are invalidmodels.FloatField documentation doesn't mention that +inf, -inf, and NaN are invalid.

Storing inf, -inf, and nan in FloatField works for me, I don't see any protection against them in models.FloatField:

  • tests/model_fields/test_floatfield.py

    diff --git a/tests/model_fields/test_floatfield.py b/tests/model_fields/test_floatfield.py
    index 264b5677e9..e3b3fde380 100644
    a b  
     1import math
     2
    13from django.db import transaction
    24from django.test import TestCase
    35
    class TestFloatField(TestCase):  
    3133        with self.assertRaisesMessage(TypeError, msg):
    3234            obj.save()
    3335
     36    def test_save_inf(self):
     37        for value in [float("inf"), math.inf, "inf"]:
     38            FloatModel.objects.create(size=value)
     39        for value in [float("-inf"), -math.inf, "-inf"]:
     40             FloatModel.objects.create(size=value)
     41
     42    def test_save_nan(self):
     43        for value in [float("nan"), math.nan, "nan"]:
     44            FloatModel.objects.create(size=value)
     45
    3446    def test_invalid_value(self):
    3547        tests = [
    3648            (TypeError, ()),

comment:3 by Nick Pope, 2 years ago

I suppose it would make sense to add the above tests. It'd probably also make sense to round-trip the value to make sure that it is actually supported...

It's also curious that b92ffebb0cdc469baaf1b8f0e72dddb069eb2fb4 (#33954) explicitly made this unsupported where (at least) PostgreSQL does support nan, +inf, and -inf. Arguably I'd say that that was a backward incompatible change with no release note and people may already be depending on that support. Lack of support for these values is a database-specific thing.

comment:4 by Matt Cooper, 2 years ago

Resolution: invalid
Status: closednew

Ah, I made two mistakes in my research, covered below.

I believe a scoped-down version of this proposed change is valid, because float('nan') is considered "null" (at least on SQLite). This is not obvious, and much like the "empty string vs null" discussion in CharField, would have helped me. The infinities do seem to work properly, so as mentioned above, this must be database-dependent.

In that test case, is the field null=True? In a brand new Django 4.1 project with the following models.py pointing to a SQLite database:

from django.db import models

class Example(models.Model):
    nullable_float = models.FloatField(null=True)
    nonnullable_float = models.FloatField()

then I get the following in ./manage.py shell:

>>> from repro.models import Example
>>> e1 = Example(nullable_float=1.0, nonnullable_float=2.0)
>>> e2 = Example(nullable_float=float('nan'), nonnullable_float=2.0)
>>> e3 = Example(nullable_float=1.0, nonnullable_float=float('nan'))
>>> e1.save()
>>> e2.save()
>>> e3.save()
Traceback (most recent call last):
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/sqlite3/base.py", line 357, in execute
    return Database.Cursor.execute(self, query, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sqlite3.IntegrityError: NOT NULL constraint failed: repro_example.nonnullable_float

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/models/base.py", line 812, in save
    self.save_base(
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/models/base.py", line 863, in save_base
    updated = self._save_table(
              ^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/models/base.py", line 1006, in _save_table
    results = self._do_insert(
              ^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/models/base.py", line 1047, in _do_insert
    return manager._insert(
           ^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/models/query.py", line 1791, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/models/sql/compiler.py", line 1660, in execute_sql
    cursor.execute(sql, params)
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 103, in execute
    return super().execute(sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 84, in _execute
    with self.db.wrap_database_errors:
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/vtbassmatt/Library/Caches/pypoetry/virtualenvs/django34260-KHbD0UBT-py3.11/lib/python3.11/site-packages/django/db/backends/sqlite3/base.py", line 357, in execute
    return Database.Cursor.execute(self, query, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.IntegrityError: NOT NULL constraint failed: repro_example.nonnullable_float

And for completeness, my mistakes:

  1. I was looking at the FloatField code for forms, not models, because I searched FloatField and didn't pay close enough attention. See https://github.com/django/django/blob/0fbdb9784da915fce5dcc1fe82bac9b4785749e5/django/forms/fields.py#L370.
  2. I had only tested float('nan') myself. Once I made the above search error, I assumed that float('inf') would behave identically, but didn't test it since I wanted to get back to my task at hand.

comment:5 by Matt Cooper, 2 years ago

Description: modified (diff)
Summary: models.FloatField documentation doesn't mention that +inf, -inf, and NaN are invalid.models.FloatField documentation doesn't mention that +inf, -inf, and NaN are database-dependent.

comment:6 by Matt Cooper, 2 years ago

Fixed the title, body, and added a new PR. https://github.com/django/django/pull/16461

in reply to:  3 comment:7 by Mariusz Felisiak, 2 years ago

In that test case, is the field null=True? In a brand new Django 4.1 project with the following models.py pointing to a SQLite database:

As for me it's a database caveat, and we cannot document all caveats in Django docs.

It's also curious that b92ffebb0cdc469baaf1b8f0e72dddb069eb2fb4 (#33954) explicitly made this unsupported where (at least) PostgreSQL does support nan, +inf, and -inf. Arguably I'd say that that was a backward incompatible change with no release note and people may already be depending on that support. Lack of support for these values is a database-specific thing.

That's not exactly true. Folks were able to save objects with nan but they were not able to fetch them back from the database, so I don't see how someone can rely on this behavior.

comment:8 by Carlton Gibson, 2 years ago

Resolution: wontfix
Status: newclosed

So we have:

This is not obvious, and much like the "empty string vs null" discussion in CharField

vs:

As for me it's a database caveat, and we cannot document all caveats in Django docs.

I think I agree with the latter here.

The CharField blank/null issue is something that's going to hit every user, pretty much immediately as they start using Django. It's worth documenting, and it even merits a whole kwarg and special handling in the field.

In contrast (I think) storing inf and NaN are more specialised and advanced use cases. I look at the diff in the PR and think that it's just not relevant to the vast majority of users. I think the particular SQLite callout is misplaced there. Most though I expect folks storing these values (as more specialised and advanced) to be writing tests checking the round-trip behaviour, in a way that we can't expect of the beginner hitting the blank/null issue with CharField.

Floats are troublesome (granted). Maybe there's a cases for an addition to the Databases doc with the issue explained for each DB. With the proposed PR, what conclusion am I to take if I'm on PostgreSQL, or …? — write some tests and see?

Happy to review such if you wanted to take it on Matt. Pending such, I think it's a wontfix.

comment:9 by Matt Cooper, 2 years ago

Thanks Carlton, I agree that's a better spot for it. I don't have easy access to most of the supported databases, but I can at least skim their docs for mention of these special float values.

comment:10 by Carlton Gibson, 2 years ago

I don't have easy access to most of the supported databases...

If you can make a stab at SQLite, Postgres, and MySQL (if you have that) we arehere meaning Mariusz is :) quite accustomed to helping with the Oracle bit.

comment:11 by Tim Graham, 2 years ago

The proposed addition to the database docs looks unnecessary, particularly because the form field disallows these values. The documentation for the model FloatField begins "A floating-point number..." and gives no indication that NaN and inf (non-numbers) are supported.

comment:12 by Carlton Gibson, 2 years ago

Yes, sorry Matt — thanks for the hustle, but I have to agree. This is right into the woods, for something which most users will never hit.

A model and a form:

from django import forms
from django.db import models


class FloatModel(models.Model):
    number = models.FloatField(null=False)


class FloatForm(forms.ModelForm):
    class Meta:
        model = FloatModel
        fields = ['number']

Then:

>>> from ticket_34260.models import FloatModel, FloatForm
>>> form = FloatForm({"number":float("inf")})
>>> form.is_valid()
False
>>> form.errors
{'number': ['Enter a number.']}
>>> form = FloatForm({"number":float("nan")})
>>> form.is_valid()
False
>>> form.errors
{'number': ['Enter a number.']}
>>> form = FloatForm({"number":float("-inf")})
>>> form.is_valid()
False
>>> form.errors
{'number': ['Enter a number.']}

I appreciate you can work around the form layer but I think at that point we come back to Mariusz' we cannot document all caveats in Django docs.

Likely this ticket will serve folks searching this in the future, so it's not all for naught.

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