Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#34590 closed Bug (fixed)

Querying for decimals larger than max_digits crashes on SQLite

Reported by: Marc Odermatt Owned by: David Sanders
Component: Database layer (models, ORM) Version: 4.2
Severity: Release blocker Keywords:
Cc: Florian Apolloner Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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'>]

Change History (12)

comment:1 by David Sanders, 14 months ago

Cc: Florian Apolloner added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Confirmed regression with 7990d254b0af158baf827fafbd90fe8e890f23bd

Crashes with sqlite, postgres ok

edit: mysql ok too

Last edited 14 months ago by David Sanders (previous) (diff)

comment:2 by David Sanders, 14 months ago

Summary: Regression in 4.2 when querying db for too large decimalsQuerying for decimals larger than max_digits crashes on SQLite

comment:3 by Simon Charette, 14 months ago

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

in reply to:  3 comment:4 by Mohit Singh Sinsniwal, 14 months ago

Owner: changed from nobody to Mohit Singh Sinsniwal
Status: newassigned

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

comment:5 by Florian Apolloner, 14 months ago

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?

comment:6 by David Sanders, 14 months ago

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)

Last edited 14 months ago by David Sanders (previous) (diff)

in reply to:  6 comment:7 by David Sanders, 14 months ago

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)

comment:8 by Florian Apolloner, 14 months ago

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

comment:9 by Simon Charette, 14 months ago

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.

comment:10 by David Sanders, 14 months ago

Has patch: set
Owner: changed from Mohit Singh Sinsniwal to David Sanders

comment:11 by GitHub <noreply@…>, 14 months ago

Resolution: fixed
Status: assignedclosed

In 0c1518e:

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

This reverts 7990d254b0af158baf827fafbd90fe8e890f23bd.

Thanks Marc Odermatt for the report.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 14 months ago

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

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