[Django] #22023: .values() followed by defer() or only() results invalid data or crash
Django
----------------------------------------------+--------------------
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.5
Severity: Normal | Keywords: orm
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
Given model
{{{
class MyModel(models.Model):
field_a = models.CharField(max_length=100)
field_b = models.IntegerField()
field_c = models.TextField()
}}}
Query {{{ MyModel.objects.values().only('field_b') }}}
Returns totally incorrect set of fields
Query {{{ MyModel.objects.values().defer('field_b') }}}
Returns all fields but data is shifted starting from field_b
Query {{{ MyModel.objects.values('field_a').only('field_b') }}}
Results a crash due malformed SQL.
--
Ticket URL: <https://code.djangoproject.com/ticket/22023>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* needs_docs: => 0
* needs_better_patch: => 0
* version: 1.5 => master
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
I can reproduce this on sqlite and it seems to be an old problem (I tested
all versions of Django until 1.4 and they're all affected).
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:1>
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Lauxley):
I was curious so i tracked down the issue,
the iterator() method of the ValuesQuerySet doesn't use the
deferred_loading attribute of the query, even though the docstring of
Query.deferred_to_data says that :
"This [self.deferred_loading] is used to compute the columns to select
from the database and also by the QuerySet class to work out which fields
are being initialised on each model."
So a quick patch (django 1.4.5)
{{{#!python
@@ -956,7 +956,14 @@
def iterator(self):
# Purge any extra columns that haven't been explicitly asked for
extra_names = self.query.extra_select.keys()
- field_names = self.field_names
+ deferred_fields, defer = self.query.deferred_loading
+ if deferred_fields:
+ if not defer:
+ field_names = list(deferred_fields)
+ else:
+ field_names = list(set(self.field_names) -
deferred_fields)
+ else:
+ field_names = self.field_names
aggregate_names = self.query.aggregate_select.keys()
names = extra_names + field_names + aggregate_names
}}}
This is probably not the 'right' way to do it, only an hint.
I strongly feel that this should not be computed in the iterator method,
i'm not even sure why the QuerySet class would need to do it itself, but
i'm probably missing something.
Note:
.values('field_a').only('field_b') and .values('field_a').defer('field_a')
will still raise a DatabaseError, but i think it's fine in this case.
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:2>
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Lauxley):
woops
{{{
--- db/models/query.py
+++ db/models/query.py
@@ -956,7 +956,14 @@
def iterator(self):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:3>
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I wonder if we should just disallow .only() and .defer() after .values().
Deferred loading for .values() doesn't seem sensible to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:4>
Django
Reporter: jtiai | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Lauxley):
I guess it depends if you want to keep the same API for a QuerySet and a
ValuesQuerySet.
The thing is .only() before .values() does nothing at all today,
so raising a NotImplementedError in ValuesQuerySet's .only() and .defer()
would only create a weird case where
.values(...).only(...) != .only(...).values(...)
I know that not all query methods are commutable, but is there a good
reason for it in this case ?
Also, why does the QuerySet/ValuesQuerySet instance has to compute the
fields to set in the object/dict, in what case is it different from the
fields fetched by the Query instance ?
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:5>
Django
Reporter: jtiai | Owner: wiget
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => wiget
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:6>
Django
Reporter: jtiai | Owner: wiget
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by bmispelon):
For what it's worth, the current documentation [1] states that "[...] a
ValuesQuerySet is a subclass of QuerySet, so it has all methods of
QuerySet".
If we start changing the behavior of `ValuesQuerySet` significantly, we
may want to reorganize the documentation a bit and introduce proper
reference documentation for it. Currently, it's only documented inside the
reference for `Queryset.values()`.
[1] https://docs.djangoproject.com/en/1.6/ref/models/querysets/#values
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:7>
Django
Reporter: jtiai | Owner: wiget
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
I have to agree with @akaariai -- I don't understand what the use case for
using `values()` along with `only()` and `defer()` would be? The
[https://github.com/django/django/pull/2303 proposal] of having `only()`
override the fields in `values()` seems confusing. I would expect any
fields not already selected by `values()` not to be available later in the
`QuerySet`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:8>
Django
Reporter: jtiai | Owner: wiget
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by loic84):
IMO "deferring" only makes sense if you get actual objects from which you
can fetch deferred fields. While `only()` could conceptually modify the
list of fields used by `values()` (but shouldn't since we made it the
antonym of `defer()`), the "deferring" concept and therefore `defer()` are
completely at odd with `values()`/`values_list()`.
I recommend raising `NotImplementedError` for both
`ValuesQuerySet.defer()` and `ValuesQuerySet.only()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:9>
Django
Reporter: jtiai | Owner: wiget
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by wiget):
Please take a look at https://github.com/django/django/pull/2303
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:10>
Django
Reporter: jtiai | Owner: wiget
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: orm | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"faf6a911ad7357c8dd1defa3be0317a8418febf7"]:
{{{
#!CommitTicketReference repository=""
revision="faf6a911ad7357c8dd1defa3be0317a8418febf7"
Fixed #22023 -- Raised an error for values() followed by defer() or
only().
Previously, doing so resulted in invalid data or crash.
Thanks jtiai for the report and Karol Jochelson,
Jakub Nowak, Loic Bistuer, and Baptiste Mispelon for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22023#comment:11>