Querying for decimals larger than max_digits crashes on SQLite

[Marc Odermatt]

Introduced in: Refs #33308 -- Improved adapting DecimalField values to decimal.

Description: I have DecimalField with max_digits=4. Previously, querying for something larger (e.g. 12345) would execute the sql and return ObjectNotFound. Now, in 4.2, it throws a decimal.InvalidOperation error, as it tries to quantize the value to have 4 digits.

I understand that it doesn't make sense to query for a larger number, but the error that occurs was pretty confusing to me. Also, it is not as easy to check in my application, because I don't have easy access to the max_digits parameter of the field.

In my opinion, the backend should either accept larger values and always return "not found", or the error should be more descriptive, so that it can be caught specifically.

Testcase: placed in tests/backends folder and used for git bisect

import decimal

from django.db import models
from django.test import TestCase


class DecimalModel(models.Model):
    dec_field = models.DecimalField(decimal_places=0, max_digits=4)

class InvalidDecimalQuery(TestCase):
    def test_invalid_decimal_query(self):
        try:
            DecimalModel.objects.get(dec_field='12345')
        except decimal.InvalidOperation:
            self.fail("Too large decimal query caused exception.")
        except DecimalModel.DoesNotExist:
            pass

Stacktrace:

Traceback (most recent call last):
  File "lib/python3.10/site-packages/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "lib/python3.10/site-packages/django/db/models/query.py", line 633, in get
    num = len(clone)
  File "lib/python3.10/site-packages/django/db/models/query.py", line 380, in __len__
    self._fetch_all()
  File "lib/python3.10/site-packages/django/db/models/query.py", line 1881, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "lib/python3.10/site-packages/django/db/models/query.py", line 91, in __iter__
    results = compiler.execute_sql(
  File "lib/python3.10/site-packages/django/db/models/sql/compiler.py", line 1547, in execute_sql
    sql, params = self.as_sql()
  File "lib/python3.10/site-packages/django/db/models/sql/compiler.py", line 762, in as_sql
    self.compile(self.where) if self.where is not None else ("", [])
  File "lib/python3.10/site-packages/django/db/models/sql/compiler.py", line 544, in compile
    sql, params = node.as_sql(self, self.connection)
  File "lib/python3.10/site-packages/django/db/models/sql/where.py", line 145, in as_sql
    sql, params = compiler.compile(child)
  File "lib/python3.10/site-packages/django/db/models/sql/compiler.py", line 544, in compile
    sql, params = node.as_sql(self, self.connection)
  File "lib/python3.10/site-packages/django/db/models/lookups.py", line 357, in as_sql
    return super().as_sql(compiler, connection)
  File "lib/python3.10/site-packages/django/db/models/lookups.py", line 225, in as_sql
    rhs_sql, rhs_params = self.process_rhs(compiler, connection)
  File "lib/python3.10/site-packages/django/db/models/lookups.py", line 126, in process_rhs
    return self.get_db_prep_lookup(value, connection)
  File "lib/python3.10/site-packages/django/db/models/lookups.py", line 254, in get_db_prep_lookup
    else [get_db_prep_value(value, connection, prepared=True)],
  File "lib/python3.10/site-packages/django/db/models/fields/__init__.py", line 1761, in get_db_prep_value
    return connection.ops.adapt_decimalfield_value(
  File "lib/python3.10/site-packages/django/db/backends/base/operations.py", line 574, in adapt_decimalfield_value
    return utils.format_number(value, max_digits, decimal_places)
  File "lib/python3.10/site-packages/django/db/backends/utils.py", line 304, in format_number
    value = value.quantize(
decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]

[David Sanders]

Confirmed regression with 7990d254b0af158baf827fafbd90fe8e890f23bd

Crashes with sqlite, postgres ok

edit: mysql ok too

[Simon Charette]

The most straightforward way to address this is likely to have DecimalField.get_db_prep_value catch decimal.InvalidOperation and raise EmptyResultSet instead.

[Mohit Singh Sinsniwal]

Replying to Simon Charette:

The most straightforward way to address this is likely to have DecimalField.get_db_prep_value catch decimal.InvalidOperation and raise EmptyResultSet instead.

Hi Simon 👋 , I want to implement this patch. I will send the pr within 6hrs

[Florian Apolloner]

Mhm this is a tricky one. Raising EmptyResultSet feels wrong though. There is no reason why this code should not be able to properly prep the value (there will certainly be dragons somewhere down the line but it feels like we are doing one transformation to many). I'd be interested to know what the actual code was that led to this behavior. If you pass .filter(pk='abc') you also get an exception that this is not a valid integer. Wouldn't form validation where the validator is derived from the model field would have detected that this is out of range already?

[David Sanders]

Yup raising EmptyResultSet would break doing DecimalModel.objects.filter(dec_field__lte=Decimal('12345')) which seems valid if there are values <= 12,345 🤔 (which is currently crashing as per above)

[David Sanders]

Replying to David Sanders:

Yup raising EmptyResultSet would break doing DecimalModel.objects.filter(dec_field__lte=Decimal('12345')) which seems valid if there are values <= 12,345 🤔 (which is currently crashing as per above)

(Also just confirming that DecimalModel.objects.filter(dec_field__lte=Decimal('12345')) did work ok prior to 7990d254b0af158baf827fafbd90fe8e890f23bd)

[Florian Apolloner]

Yeah, so a query for "less than" seems valid to me and tells me our conversion code is not as lenient as it should be. So maybe let's just revert ​https://github.com/django/django/commit/7990d254b0af158baf827fafbd90fe8e890f23bd locally and rerun the testsuite to maybe see a reason for introducing it? To be honest I am not that sure anymore what I did why for the psycopg3 branch :D

[Simon Charette]

You're right, the EmptyResultSet approach failed to account for lte and friends that would have to be adjusted as well like we did with IntegerField.

What would be good to confirm if we can't revert this change is what the previous logic did for cases where an extra decimal was provided. Was it a non-match on all backends? Were some of them performing rounding?

For example, given field = DecimalField(max_digits=4, decimal_places=2) what does filter(Q(field=Decimal('42.425')) | Q(field=Decimal('42.424')) | Q(field=Decimal('42.426'))) yielded on all backends against existing rows values of 42.42 and 42.43. I agree that we don't want to start erroring out here without deprecation period though so this is more of exploration work but I could see why we could want to error out immediately when providing invalid lookup values.

[GitHub <noreply@…>]

In 0c1518e:

Fixed #34590 -- Reverted "Refs #33308 -- Improved adapting DecimalField values to decimal."

This reverts 7990d254b0af158baf827fafbd90fe8e890f23bd.

Thanks Marc Odermatt for the report.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 91f8df5c:

[4.2.x] Fixed #34590 -- Reverted "Refs #33308 -- Improved adapting DecimalField values to decimal."

This reverts 7990d254b0af158baf827fafbd90fe8e890f23bd.

Thanks Marc Odermatt for the report.
Backport of 0c1518ee429b01c145cf5b34eab01b0b92f8c246 from main