Opened 20 months ago

Closed 20 months ago

Last modified 17 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

Description


Attachments (2)

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

Download all attachments as: .zip

Change History (14)

Changed 20 months ago by anonymous

comment:1 Changed 20 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 20 months ago by despawn@…

Regression test.

comment:2 Changed 20 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 20 months ago by akaariai

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

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

Sorry, I meant the timedelta value.

comment:6 Changed 20 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 20 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 20 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 18 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 18 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()
        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 17 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 17 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

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