[Django] #30195: RawSQL ignores decimal_places property of DecimalField
Django
-------------------------------------+-------------------------------------
Reporter: alakae | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 2.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 |
-------------------------------------+-------------------------------------
When updating Django in our environment from 2.0.6 to 2.1.7 we have
noticed that {{{RawSQL}}} does not take into account the
{{{decimal_places}}} property of a {{{DecimalField}}} passed as
{{{output_field}}}. Please consider the following example:
{{{#!python
def currency_field(*, decimal_places=2, **kwargs):
return models.DecimalField(max_digits=19,
decimal_places=decimal_places, **kwargs)
class A(models.Model):
x = currency_field()
def test_demo():
A.objects.create(x=Decimal('1.3333333333333'))
raw_sql = RawSQL(2.3333333333333, (), currency_field())
qs = A.objects.all()
res = qs.annotate(val=raw_sql).get()
assert res.x == Decimal('1.33')
assert res.val == Decimal('2.33')
}}}
On Django 2.0.6 the test passes. When using Django 2.1.7 the following
error is returned:
{{{#!python
> assert res.val == Decimal('2.33')
E AssertionError: assert Decimal('2.33333333333330') ==
Decimal('2.33')
E + where Decimal('2.33333333333330') = <A: A object (1)>.val
E + and Decimal('2.33') = Decimal('2.33')
}}}
Since this change does not seem to be mentioned in
[https://docs.djangoproject.com/en/2.1/releases/2.1/] we suspect that this
might be a bug. I am using Python 3.6.5 and sqlite 3.27.1.
Related to: https://code.djangoproject.com/ticket/23941
--
Ticket URL: <https://code.djangoproject.com/ticket/30195>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: alakae | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Dave Halter (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30195#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: alakae | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: alakae (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30195#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: alakae | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Bisected to ebc4ee3369694e6dca5cf216d4176bdefd930fd6
> Refs #23941 -- Prevented incorrect rounding of DecimalField annotations
on SQLite.
--
Ticket URL: <https://code.djangoproject.com/ticket/30195#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: alakae | Owner: nobody
Component: Database layer | Version: 2.1
(models, ORM) |
Keywords: sqlite | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* keywords: => sqlite
* resolution: => wontfix
Comment:
So there's an easy-enough adjustment for the test case, at the point where
the behaviour changed, adjusting `convert_decimalfield_value()` like so:
{{{
def convert_decimalfield_value(self, value, expression, connection):
if value is not None:
if not isinstance(expression, Col):
# SQLite stores only 15 significant digits. Digits coming
from
# float inaccuracy must be removed.
value =
decimal.Context(prec=15).create_decimal_from_float(value) # This
`returns` in actual code.
value = expression.output_field.format_number(value)
return decimal.Decimal(value)
return value
}}}
Here we continue to pass `value` through the output field's
`format_number()`, as before the change.
Doing so, though, introduces two failures elsewhere in the test suite:
{{{
======================================================================
ERROR: test_decimal_max_digits_has_no_effect
(aggregation.tests.AggregateTestCase)
----------------------------------------------------------------------
...
decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]
======================================================================
FAIL: test_decimal_annotation
(annotations.tests.NonAggregateAnnotationTestCase)
----------------------------------------------------------------------
...
AssertionError: Decimal('0.00') != Decimal('0.001')
}}}
The first of these was introduced in the main patch for #23941 "Removed
implicit decimal formatting from expressions."
Looking at that commit message and the discussion on #23941, it seems the
behaviour here has explicitly unsupported since Django 1.8.
In particular, [https://code.djangoproject.com/ticket/23941#comment:5
comment 5]:
> The trickier case is where an explicit output_field was given. I hear
Anssi's argument that we should respect the max_digits and decimal_places
in that case, otherwise we are "lying" to the user, but I'm not convinced.
Why should DecimalField be special? If you specify a CharField as
output_field, its max_length will not be respected in converting from db
to Python. If you specify a PositiveIntegerField as output_field for an
expression, you can get a negative result with no error, etc. Conversion
is not the same thing as validation.
>
> It seems to me that the idea of applying field validation to the output
of an expression would be a major new feature addition that would need
quite a lot of separate discussion to figure out how it would work, or if
its even a good idea (there is currently no such thing as getting
ValidationError from attempting to do a database query in Django). In the
meantime, it's simply a bug (and a regression) that such validation is
sort-of applied in the case of DecimalField, and the correct fix is to
just stop doing that, as the current pull request does.
I think we have to say this is `wontfix` on that basis. Happy though if
you want to go to the DevelopersMailingList about that.
(Note in master the relevant code has moved to
[https://github.com/django/django/blob/7feddd878cd0d4ad6cc98f0da2b597d603a211a7/django/db/backends/sqlite3/operations.py#L276-L290
`get_decimalfield_converter()`].)
--
Ticket URL: <https://code.djangoproject.com/ticket/30195#comment:4>