Re: [Django] #34590: Querying for decimals larger than max_digits crashes on SQLite
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Singh Sinsniwal
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Mohit Singh Sinsniwal
* status: new => assigned
Comment:
Replying to [comment:3 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
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: Mohit
| Singh Sinsniwal
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: Mohit
| Singh Sinsniwal
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: Mohit
| Singh Sinsniwal
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
Replying to [comment:6 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)
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: Mohit
| Singh Sinsniwal
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: Mohit
| Singh Sinsniwal
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:9>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Mohit Singh Sinsniwal => David Sanders
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: David
| Sanders
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0c1518ee429b01c145cf5b34eab01b0b92f8c246" 0c1518e]:
{{{
#!CommitTicketReference repository=""
revision="0c1518ee429b01c145cf5b34eab01b0b92f8c246"
Fixed #34590 -- Reverted "Refs #33308 -- Improved adapting DecimalField
values to decimal."
This reverts 7990d254b0af158baf827fafbd90fe8e890f23bd.
Thanks Marc Odermatt for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Marc Odermatt | Owner: David
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"91f8df5c2e711ff4b3d10602181d8a6daa62a93a" 91f8df5c]:
{{{
#!CommitTicketReference repository=""
revision="91f8df5c2e711ff4b3d10602181d8a6daa62a93a"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34590#comment:12>