Opened 7 months ago

Closed 7 months ago

Last modified 4 months 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


Attachments (2)

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

Download all attachments as: .zip

Change History (14)

Changed 7 months ago by anonymous

comment:1 Changed 7 months ago by timo

  • 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 7 months ago by despawn@…

Regression test.

comment:2 Changed 7 months ago by timo

  • Needs tests unset
  • Triage Stage changed from Unreviewed to Ready for checkin

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

comment:3 Changed 7 months ago by akaariai

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

comment:4 Changed 7 months 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 7 months ago by despawnerer

Sorry, I meant the timedelta value.

comment:6 Changed 7 months ago 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)?

comment:7 Changed 7 months ago 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.

comment:8 Changed 7 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 7f2485b4d180956aa556a88ed1d9754eeeeea054:

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

Thanks Tim Graham for review.

comment:9 Changed 5 months ago 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.

comment:10 Changed 5 months ago 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()

Is it enough to back port the existing commit, or should this new test also be committed and back ported?

comment:11 Changed 4 months 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 4 months 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

Add Comment

Modify Ticket

Change Properties
<Author field>
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.