[Django] #33397: Arithmetic operations on DateField/DateTimeField/DurationField expressions are buggy.
Django
are buggy.
-------------------------------------+-------------------------------------
Reporter: Luke | Owner: Luke Plant
Plant |
Type: Bug | Status: assigned
Component: Database | Version: 4.0
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 |
-------------------------------------+-------------------------------------
There are two main sets of bugs here:
1. Nonsensical operations involving `DateField` or `DateTimeField`
expressions (such as adding two dates) do not raise the expected
`FieldError` exceptions. They usually raise exceptions later that vary
depending on the database backend.
2. Well-defined operations, which work in SQL, such as ’date + duration’,
require using `ExpressionWrapper(output_field=…)` when this could be
inferred.
Although we could technically split this into two bugs, I’m filing as one
since the two parts are closely related and fixing part 2 (which is the
real reason I’m
here) will require some changes that impinge on part 1.
== Part 1
Test case
{{{#!python
# tests/experiments/tests.py
class FTimeDeltaTests(TestCase):
def test_nonsensical_date_operations(self):
queryset = Experiment.objects.annotate(nonsense=F('name') +
F('assigned'))
with self.assertRaises(FieldError):
list(queryset)
queryset = Experiment.objects.annotate(nonsense=F('assigned') +
F('completed'))
with self.assertRaises(FieldError):
list(queryset)
}}}
The first part works as expected (it makes no sense to add a string to a
date, and a `FieldError` is raised), but the second doesn’t.
Expected behaviour: `FieldError` should be raised
Actual behaviour:
With Postgres:
{{{
File "/home/luke/devel/django/main/django/db/models/query.py", line 51,
in __iter__
results = compiler.execute_sql(chunked_fetch=self.chunked_fetch,
chunk_size=self.chunk_size)
File "/home/luke/devel/django/main/django/db/models/sql/compiler.py",
line 1211, in execute_sql
cursor.execute(sql, params)
File "/home/luke/devel/django/main/django/db/backends/utils.py", line
67, in execute
return self._execute_with_wrappers(sql, params, many=False,
executor=self._execute)
File "/home/luke/devel/django/main/django/db/backends/utils.py", line
76, in _execute_with_wrappers
return executor(sql, params, many, context)
File "/home/luke/devel/django/main/django/db/backends/utils.py", line
85, in _execute
return self.cursor.execute(sql, params)
File "/home/luke/devel/django/main/django/db/utils.py", line 90, in
__exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File "/home/luke/devel/django/main/django/db/backends/utils.py", line
85, in _execute
return self.cursor.execute(sql, params)
psycopg2.errors.UndefinedFunction: operator does not exist: date + date
LINE 1: ...t"."scalar", ("expressions_ExPeRiMeNt"."assigned" + "express...
^
HINT: No operator matches the given name and argument types. You might
need to add explicit type casts.
}}}
With SQLite:
{{{
File "/home/luke/devel/django/main/django/db/models/query.py", line 68,
in __iter__
for row in compiler.results_iter(results):
File "/home/luke/devel/django/main/django/db/models/sql/compiler.py",
line 1158, in apply_converters
value = converter(value, expression, connection)
File
"/home/luke/devel/django/main/django/db/backends/sqlite3/operations.py",
line 305, in convert_datefield_value
value = parse_date(value)
File "/home/luke/devel/django/main/django/utils/dateparse.py", line 76,
in parse_date
return datetime.date.fromisoformat(value)
TypeError: fromisoformat: argument must be str
}}}
I have not tested on other databases.
=== Additional notes
There is a related bug in some context. For example, in contrast to the
above test case `Experiment.objects.filter(name=F('name') +
F('assigned'))` does not raise `FieldError`, despite the attempt to add a
date to a string. Instead you get backend dependent results - SQLite
silently does some type coercion, Postgres fails with `UndefinedFunction`.
Tackling this may need to be done separately - there are different code
paths involved when using `QuerySet.annotate()` compared to
`QuerySet.filter()`
== Part 2
Test case:
{{{#!python
# tests/experiments/tests.py
class FTimeDeltaTests(TestCase):
def
test_datetime_and_duration_field_addition_without_output_field(self):
test_set = Experiment.objects.annotate(estimated_end=F('start') +
F('estimated_time'))
self.assertEqual(
[e.estimated_end for e in test_set],
[e.start + e.estimated_time for e in test_set]
)
}}}
Expected behaviour: Django should infer the output type, like it does for
other expressions such as integer field addition.
Actual behaviour: Django raises `django.core.exceptions.FieldError:
Expression contains mixed types: DateTimeField, DurationField. You must
set output_field.`
=== Additional motivation
If we have this code:
{{{#!python
Experiment.objects.filter(end__gt=F('start') + F('estimated_time'))
}}}
we should be able to refactor to:
{{{#!python
Experiment.objects.alias(estimated_end=F('start') +
F('estimated_time')).filter(end__gt=F('estimated_end'))
}}}
But the latter fails with `FieldError`. The former succeeds because in
that context the ORM doesn't need to do any type inference.
== Notes
* Above tests have been run against `main` branch.
* There are a bunch of other cases, like "date multiplied by date" etc.
that don't work as expected.
--
Ticket URL: <https://code.djangoproject.com/ticket/33397>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(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
-------------------------------------+-------------------------------------
Comment (by Luke Plant):
For anyone else looking at this:
The underlying behaviour of `BaseExpression._resolve_output_field` is an
enormous can of worms, especially once you dig into database differences,
tickets #26650, #31506 and preserving compatibility with existing
behaviour.
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:1>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:2>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Egor R (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:3>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Hi Luke — thanks for the ticket. Sounds good. I'm going to ''Accept'',
given that Simon's initial review is generally positive.
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:4>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15271 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:5>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
#33464 was a duplicate for `DecimalField` and `MOD`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:6>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"04ad0f26ba4b8c79dc311e1789457e0c4d1b8832" 04ad0f26]:
{{{
#!CommitTicketReference repository=""
revision="04ad0f26ba4b8c79dc311e1789457e0c4d1b8832"
Refs #33397 -- Added extra tests for resolving an output_field of
CombinedExpression.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:7>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:8>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Note that currently `output_field` has limited scope in what it affects:
- it does not cause a database level `CAST` to be done (see #26650)
- it controls conversion to Python data types, but that doesn't
necessarily include validation, so some invalid values for the field may
be returned.
Correct behavior for `NULL` was difficult to decide, due to different
database handling of `NULL`. The current approach is based on lowest
common denominator behavior i.e. if one of the supported databases is
raising an error (rather than return `NULL`) for `val <op> NULL`, then
Django raises `FieldError`.
See also [https://github.com/django/django/pull/15271#issue-1091887189
Luke's description] for more details.
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:9>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"1efea11808f7b8a3b31445e0c1c7d270f832d965" 1efea118]:
{{{
#!CommitTicketReference repository=""
revision="1efea11808f7b8a3b31445e0c1c7d270f832d965"
Refs #33397 -- Added register_combinable_fields().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:10>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Component: Database layer | Version: 4.0
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"40b8a6174f001a310aa33f7880db0efeeb04d4c4" 40b8a617]:
{{{
#!CommitTicketReference repository=""
revision="40b8a6174f001a310aa33f7880db0efeeb04d4c4"
Fixed #33397 -- Corrected resolving output_field for
DateField/DateTimeField/TimeField/DurationFields.
This includes refactoring of CombinedExpression._resolve_output_field()
so it no longer uses the behavior inherited from Expression of guessing
same output type if argument types match, and instead we explicitly
define the output type of all supported operations.
This also makes nonsensical operations involving dates
(e.g. date + date) raise a FieldError, and adds support for
automatically inferring output_field for cases such as:
* date - date
* date + duration
* date - duration
* time + duration
* time - time
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:11>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: closed
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"e8dcef155c1848ef49e54f787a7d20faf3bf9296" e8dcef15]:
{{{
#!CommitTicketReference repository=""
revision="e8dcef155c1848ef49e54f787a7d20faf3bf9296"
Refs #33397, Refs #34160 -- Added release note for resolving output_field
changes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:12>
Django
are buggy.
-------------------------------------+-------------------------------------
| Plant
Type: Bug | Status: closed
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"58156f4ed761b9f7650d5e7a12f3f20be3d60c53" 58156f4e]:
{{{
#!CommitTicketReference repository=""
revision="58156f4ed761b9f7650d5e7a12f3f20be3d60c53"
[4.1.x] Refs #33397, Refs #34160 -- Added release note for resolving
output_field changes.
Backport of e8dcef155c1848ef49e54f787a7d20faf3bf9296 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33397#comment:13>