[Django] #27159: Pickling query from queryset causes unintended queryset evaluation
Django
-------------------------------+--------------------
Reporter: jtiai | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
According to documentation pickling queryset.query should be correct
approach and it should only pickle only relevant information to recreate
queryset.query again.
Given models:
{{{#!python
from django.db import models
# Create your models here.
class ParentModel(models.Model):
name = models.CharField(max_length=64)
class ChildModel(models.Model):
name = models.CharField(max_length=64)
parent = models.ForeignKey('ParentModel', related_name='children')
}}}
Following querys, when pickled causes unwanted evaluation and creating
quite large pickled result:
{{{#!python
for x in range(1,10000):
ParentModel(name='Parent {}'.format(x), ).save()
ChildModel(name='Child 1', parent=ParentModel.objects.all()[0]).save()
parents_1 = ParentModel.objects.all().values_list('pk', flat=True)
children_1 = ChildModel.objects.filter(parent__in=parents_1)
pickled_stuff_1 = pickle.dumps(children_1.query)
parents_2 = ParentModel.objects.all()
children_2 = ChildModel.objects.filter(parent__in=parents_2)
pickled_stuff_2 = pickle.dumps(children_2.query)
# First len is about 74 kilobytes, second len is about 2 megabytes.
print len(pickled_stuff_1), len(pickled_stuff_2)
}}}
When comparing sizes of pickled queries both are relatively big. When
inspecting latter pickle it can be seen that it actually contains all
instances from fully evaluated queryset. Same behavior exists in 1.8 and
1.10 as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* component: Uncategorized => Database layer (models, ORM)
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
I didn't attempt to reproduce but I trust the reporter. If you could
transform the reproduction information into a test case for
`queryset_pickle`, that's always helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:1>
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jtiai):
I added crude, initial test to {{{queryset_pickle}}} to
https://github.com/jtiai/django/tree/issue_27159
Interesting enough, if you check pickled output it's clearly visible that
it contains all instances from Parent-model.
If you compare query output as a string both are equal.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:2>
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jtiai):
After doing some debugging it looks like pickle goes to {{{WhereNode}}}
which in turn goes to {{{RelatedIn}}} query, which goes to
{{{QuerySet.__getstate__}}} which, by design populates full cache.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:3>
Django
Reporter: jtiai | Owner: DavidFozo
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by DavidFozo):
* owner: nobody => DavidFozo
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:4>
Django
Reporter: jtiai | Owner: DavidFozo
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by DavidFozo):
* Attachment "ticket_27159.diff" added.
Django
Reporter: jtiai | Owner: DavidFozo
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by DavidFozo):
I deleted `self._fetch_all()` from `django/db/models/query.py
QuerySet.__getstate__ `, so now it doesn't populate the cache before
pickling. Is it a valid solution? All tests are passing.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:5>
Django
Reporter: jtiai | Owner: DavidFozo
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jtiai):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:6>
Django
Reporter: jtiai | Owner: DavidFozo
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
I left some comments for improvement on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:7>
Django
Reporter: jtiai | Owner: DavidFozo
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:8>
Django
inner_qs
-------------------------------------+-------------------------------------
Reporter: jtiai | Owner: jtiai
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jtiai):
* owner: DavidFozo => jtiai
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:9>
Django
inner_qs
-------------------------------------+-------------------------------------
Reporter: jtiai | Owner: jtiai
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 0 => 1
Comment:
Left some comments on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:10>
Django
inner_qs
-------------------------------------+-------------------------------------
Reporter: jtiai | Owner: jtiai
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jtiai):
* needs_better_patch: 1 => 0
Comment:
Made all suggested changes and improved test cases.
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:11>
Django
inner_qs
-------------------------------------+-------------------------------------
| Tiainen
Type: Bug | Status: closed
Component: Database layer | Version: 1.10
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7a2c27112d1f804f75191e9bf45a96a89318a684" 7a2c271]:
{{{
#!CommitTicketReference repository=""
revision="7a2c27112d1f804f75191e9bf45a96a89318a684"
Fixed #27159 -- Prevented pickling a query with an __in=inner_qs lookup
from evaluating inner_qs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27159#comment:12>