[Django] #35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim | Owner: Tim Graham
Graham |
Type: Bug | Status: assigned
Component: Database | Version: 5.1
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Some built-in fields have corresponding
`DatabaseOperations.adapt_<field_name>_value()` methods. For all of these
fields except `DecimalField`, `get_db_prep_value()` calls the
corresponding "adapt" method.

`DecimalField`, however, was changed in
e9814029f570bd0866dc859147bca90340bcc913 to call
`adapt_decimalfield_value()` in `get_db_prep_save()`. This causes decimal
values not to be converted when needed for query lookups (see
[https://github.com/mongodb-labs/django-
mongodb/blob/36c57187de430ab4313e8db5c3ce8ab2ab569f5b/django_mongodb/operations.py#L182-L202
hacky workaround in django-mongodb)].

The issue also manifested when adapting `ArrayField` for MongoDB. The lack
of a proper `DecimalField.get_db_prep_value()` means
[https://github.com/django/django/blob/c075d4c2c8cef3c9b28180c749d421c63544a9dd/django/contrib/postgres/fields/array.py#L126-L132
Decimals aren't prepared properly].
--
Ticket URL: <https://code.djangoproject.com/ticket/35982>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------

Reporter: Tim Graham | Owner: Tim

| Graham
Type: Bug | Status: assigned

Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/18895 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:1>

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted

Comment:

The report makes sense but just like any regression its warrants a test
demonstrating it.
--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:2>

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

I considered whether an additional test would be useful, but I'm not sure
one could be created for the built-in database backends without resorting
to mocking that checks that `get_db_prep_value()` calls
`adapt_decimalfield_value()`. There are already many failures on django-
mongodb without this patch (and removing the hacky workaround I mentioned
in the description). Example:
{{{
======================================================================
ERROR: test_gt (lookup.test_decimalfield.DecimalFieldLookupTests.test_gt)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tim/code/django/tests/lookup/test_decimalfield.py", line 25,
in test_gt
self.assertCountEqual(qs, [self.p3])
File "/opt/python3.12.0/lib/python3.12/unittest/case.py", line 1198, in
assertCountEqual
first_seq, second_seq = list(first), list(second)
^^^^^^^^^^^
File "/home/tim/code/django/django/db/models/query.py", line 400, in
__iter__
self._fetch_all()
File "/home/tim/code/django/django/db/models/query.py", line 1928, in
_fetch_all
self._result_cache = list(self._iterable_class(self))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/code/django/django/db/models/query.py", line 91, in
__iter__
results = compiler.execute_sql(
^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/code/django-mongodb/django_mongodb/compiler.py", line
255, in execute_sql
cursor = query.get_cursor()
^^^^^^^^^^^^^^^^^^
File "/home/tim/code/django-mongodb/django_mongodb/query.py", line 19,
in wrapper
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/code/django-mongodb/django_mongodb/query.py", line 74,
in get_cursor
return self.compiler.collection.aggregate(self.get_pipeline())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/collection.py", line 2958, in aggregate
return self._aggregate(
^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/_csot.py", line 119, in csot_wrapper
return func(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/collection.py", line 2866, in _aggregate
return self._database.client._retryable_read(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/mongo_client.py", line 1863, in
_retryable_read
return self._retry_internal(
^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/_csot.py", line 119, in csot_wrapper
return func(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/mongo_client.py", line 1830, in
_retry_internal
).run()
^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/mongo_client.py", line 2554, in run
return self._read() if self._is_read else self._write()
^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/mongo_client.py", line 2697, in _read
return self._func(self._session, self._server, conn, read_pref) #
type: ignore
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/aggregation.py", line 164, in get_cursor
result = conn.command(
^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/helpers.py", line 45, in inner
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/pool.py", line 566, in command
self._raise_connection_failure(error)
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/pool.py", line 538, in command
return command(
^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/synchronous/network.py", line 158, in command
request_id, msg, size, max_doc_size = message._op_msg(
^^^^^^^^^^^^^^^^
File "/home/tim/.virtualenvs/django312/lib/python3.12/site-
packages/pymongo/message.py", line 415, in _op_msg
return _op_msg_uncompressed(flags, command, identifier, docs, opts)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
bson.errors.InvalidDocument: cannot encode object: Decimal('0'), of type:
<class 'decimal.Decimal'>
}}}

It raises the question of whether built-in database backends get special
testing treatment. If this bug manifested on Oracle, for example, I don't
think we would require a regression test for SQLite. I'm comfortable with
running the existing tests with django-mongodb to ensure this issue does
not regress.
--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:3>

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Ok so basically this was never an issue because the core backends PEP249
drivers (`sqlite3`, `psycopg`, `mysqlclient`, and `oracledb`) all support
`decimal.Decimal` natively? If this is the case then it means the
`utils.format_number` call is likely unnecessary at this point (I'll give
the suite a run by removing it) otherwise it means a regression test can
be written for querying as well without relying on mocking.
--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:4>

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 1 => 0
* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:5>

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:6>

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham

Type: Bug | Status: closed

Component: Database layer | Version: 5.1
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"1860a1afc9ac20750f932e8e0a94b32d096f2848" 1860a1a]:
{{{#!CommitTicketReference repository=""
revision="1860a1afc9ac20750f932e8e0a94b32d096f2848"
Fixed #35982 -- Made DecimalField.get_db_prep_value() call
DatabaseOperations.adapt_decimalfield_value().

Regression in e9814029f570bd0866dc859147bca90340bcc913.

Thanks Simon Charette for advice and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:8>

[Django]

#35982: DecimalField.get_db_prep_value() doesn't call adapt_decimalfield_value()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham

Type: Bug | Status: assigned

Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"b0b30247204aea8096b3c5456d71c2df9bc4f4ae" b0b3024]:
{{{#!CommitTicketReference repository=""
revision="b0b30247204aea8096b3c5456d71c2df9bc4f4ae"
Refs #35982 -- Made BaseDatabaseOperations.adapt_decimalfield_value() a
no-op.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35982#comment:7>