Opened 3 years ago

Closed 3 years ago

Last modified 3 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 3 years ago.
0001-Add-a-regression-test-for-21643.patch (1.0 KB) - added by despawn@… 3 years ago.
Regression test.

Download all attachments as: .zip

Change History (14)

Changed 3 years ago by anonymous

comment:1 Changed 3 years ago by Tim Graham

Needs documentation: unset
Needs tests: set
Patch needs improvement: unset

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

Changed 3 years ago by despawn@…

Regression test.

comment:2 Changed 3 years ago by Tim Graham

Needs tests: unset
Triage Stage: UnreviewedReady for checkin

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

comment:3 Changed 3 years ago by Anssi Kääriäinen

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

comment:4 Changed 3 years ago 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.

comment:5 Changed 3 years ago by Alexey Voronov

Sorry, I meant the timedelta value.

comment:6 Changed 3 years ago by Anssi Kääriäinen

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 Changed 3 years ago by Alexey Voronov

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 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: newclosed

In 7f2485b4d180956aa556a88ed1d9754eeeeea054:

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

Thanks Tim Graham for review.

comment:9 Changed 3 years ago by Tai Lee

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 Changed 3 years ago by Tai Lee

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 Changed 3 years ago by Tim Graham <timograham@…>

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 Changed 3 years ago by Tim Graham <timograham@…>

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