[Django] #21643: QuerySets that use F() + timedelta() crash when compiling their query more than once
Django
than once
----------------------------------------------+--------------------
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/21643>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: => 0
* needs_tests: => 1
* needs_docs: => 0
Comment:
Could you write a regression test that demonstrates the problem and proves
it's fixed?
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:1>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_tests: 1 => 0
* stage: Unreviewed => Ready for checkin
Comment:
Verified the test and the rest of the test suite on SQlite.
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:2>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by akaariai):
Wouldn't it be better to use just node.children[-1] instead of .pop()?
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:3>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by despawn@…):
I've thought about that, but then the datetime value gets put in the query
the second time (and incorrectly) in evaluate_node. There's probably a
better way to do this, but I'm not exactly knowledgeable enough about
Django internals to figure it out. The patch attached works as a quick-
and-easy fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:4>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by despawnerer):
Sorry, I meant the timedelta value.
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:5>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by akaariai):
OK, good enough for me. Is this a regression in 1.6 or has this bug
existed for a longer time (that is, does this need to be backpatched to
1.6)?
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:6>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by despawnerer):
Google seems to find reports of the error from way back in December 2011.
The code in question hasn't been touched since it appeared with the
introduction of support for F() + timedelta(). Looks like it's been this
way for years.
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:7>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Anssi Kääriäinen <akaariai@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"7f2485b4d180956aa556a88ed1d9754eeeeea054"]:
{{{
#!CommitTicketReference repository=""
revision="7f2485b4d180956aa556a88ed1d9754eeeeea054"
Fixed #21643 -- repeated execution of qs with F() + timedelta
Thanks Tim Graham for review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:8>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by mrmachine):
This is a regression in 1.6, I believe due to the deep copy changes when
cloning querysets. I confirmed that `F() + timedelta()` does work in 1.5.
Previously, the `DateModifierNode` expression would have been deep copied
every time a queryset was cloned, so it would never have been evaluated
twice.
So I think this should be backported.
Also #22101 was a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:9>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by mrmachine):
The test from the fix already applied also fails in 1.5, but the following
test passes in 1.5 and fails in 1.6 showing that this is a regression.
{{{
def test_query_clone(self):
# Ticket #21643
qs = Experiment.objects.filter(end__lt=F('start') +
datetime.timedelta(hours=1))
qs2 = qs.all()
list(qs)
list(qs2)
}}}
Is it enough to back port the existing commit, or should this new test
also be committed and back ported?
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:10>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"8137215973c8cf97f58f244021b1a4e75923ade8"]:
{{{
#!CommitTicketReference repository=""
revision="8137215973c8cf97f58f244021b1a4e75923ade8"
Added release note and regression test for refs #21643.
This will be backported to stable/1.6.x along with the original fix.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:11>
Django
than once
Reporter: despawn@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5cda1d27027ea74d8a1b53e43bef697cd4426e9a"]:
{{{
#!CommitTicketReference repository=""
revision="5cda1d27027ea74d8a1b53e43bef697cd4426e9a"
[1.6.x] Fixed #21643 -- repeated execution of qs with F() + timedelta
Thanks Tim Graham for review and Tai Lee for the additional test to prove
this was a regression in 1.6.
Backport of 7f2485b4d1 and 8137215973 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21643#comment:12>