#23941: Aggregating over decimal field regression
-------------------------------------+-------------------------------------
Reporter: jarshwah | Owner: jarshwah
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
It seems this issue is maybe being presented as broader than it really is?
We don't ever do "validation" (in the Django sense) on annotation outputs,
we just convert from db value to Python value. We have one special case
with `DecimalField` where that conversion itself can actually raise an
exception. The problem is not with validation, the problem is that
`max_digits` is not just a validation, it is a hard limit on the value
that the field knows how to represent (due to its use in setting the
decimal context precision). There is no parallel problem with e.g.
`max_length` on a `CharField` (or with any other field with validation
constraints), because the `max_length` (or any other validation
constraint) isn't checked anywhere in the conversion from DB value to
Python value. So in a way, this is simply a bug in `DecimalField` (or
perhaps in the way an internal-type of `DecimalField` is special-cased in
`ExpressionNode.convert_value`), in that its "validation" constraints are
applied (and result in hard errors) in data conversion, where normally
field validation does not apply at all.
(As an aside, it also seems a little odd to me that
`ExpressionNode.convert_value` assumes that any field with internal-type
of `DecimalField` will have a `format_number` method, when
`DecimalField.format_number()` has a docstring indicating that it only
exists for legacy code.)
In the case where no explicit `output_field` is given for an annotation,
we should definitely not apply the `max_digits` or `decimal_places` values
from the source field to the result; we should simply convert to Decimal
and return.
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.
This does leave us in the odd situation where specifying `output_field`
sometimes requires that you provide field parameters which are effectively
ignored. I think the ideal API for this would be to allow (or require?)
`output_field` to be a field class, not a field instance. But that would
require making `get_internal_type` a classmethod rather than an instance
method on Fields. Which perhaps it should be, but that's also a major
invasive change for third-party field classes.
--
Ticket URL: <https://code.djangoproject.com/ticket/23941#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/066.f804ccf7ef7528ef6aab4ffceed8ccb7%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
