Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#21643 closed Bug (fixed)

QuerySets that use F() + timedelta() crash when compiling their query more than once

Reported by: despawn@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description


Attachments (2)

fix_ftimedelta_querysets.patch (916 bytes ) - added by anonymous 11 years ago.
0001-Add-a-regression-test-for-21643.patch (1.0 KB ) - added by despawn@… 11 years ago.
Regression test.

Download all attachments as: .zip

Change History (14)

by anonymous, 11 years ago

comment:1 by Tim Graham, 11 years ago

Needs tests: set

Could you write a regression test that demonstrates the problem and proves it's fixed?

by despawn@…, 11 years ago

Regression test.

comment:2 by Tim Graham, 11 years ago

Needs tests: unset
Triage Stage: UnreviewedReady for checkin

Verified the test and the rest of the test suite on SQlite.

comment:3 by Anssi Kääriäinen, 11 years ago

Wouldn't it be better to use just node.children[-1] instead of .pop()?

comment:4 by despawn@…, 11 years ago

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.

comment:5 by Alexey Voronov, 11 years ago

Sorry, I meant the timedelta value.

comment:6 by Anssi Kääriäinen, 11 years ago

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)?

comment:7 by Alexey Voronov, 11 years ago

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.

comment:8 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 7f2485b4d180956aa556a88ed1d9754eeeeea054:

Fixed #21643 -- repeated execution of qs with F() + timedelta

Thanks Tim Graham for review.

comment:9 by Tai Lee, 11 years ago

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.

comment:10 by Tai Lee, 11 years ago

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?

comment:11 by Tim Graham <timograham@…>, 11 years ago

In 8137215973c8cf97f58f244021b1a4e75923ade8:

Added release note and regression test for refs #21643.

This will be backported to stable/1.6.x along with the original fix.

comment:12 by Tim Graham <timograham@…>, 11 years ago

In 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

Note: See TracTickets for help on using tickets.
Back to Top