From 58ea6d45618cb8b5bd4306c395d7bb8860f9a842 Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Mon, 16 Feb 2009 12:28:37 +0000 Subject: [PATCH] Fixed #10256 -- Corrected the interaction of extra(select=) with values() and values_list() where an explicit list of columns is requested. git-svn-id: http://code.djangoproject.com/svn/django/trunk@9837 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/db/models/query.py | 52 ++++++++----- .../aggregation_regress/models.py | 2 +- tests/regressiontests/extra_regress/models.py | 76 +++++++++++++++++++ 3 files changed, 111 insertions(+), 19 deletions(-) diff --git a/django/db/models/query.py b/django/db/models/query.py index c06a37897e..86fafc519d 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -727,12 +727,15 @@ class ValuesQuerySet(QuerySet): # names of the model fields to select. def iterator(self): - if (not self.extra_names and - len(self.field_names) != len(self.model._meta.fields)): + # Purge any extra columns that haven't been explicitly asked for + if self.extra_names is not None: self.query.trim_extra_select(self.extra_names) - names = self.query.extra_select.keys() + self.field_names - names.extend(self.query.aggregate_select.keys()) + extra_names = self.query.extra_select.keys() + field_names = self.field_names + aggregate_names = self.query.aggregate_select.keys() + + names = extra_names + field_names + aggregate_names for row in self.query.results_iter(): yield dict(zip(names, row)) @@ -745,29 +748,30 @@ class ValuesQuerySet(QuerySet): instance. """ self.query.clear_select_fields() - self.extra_names = [] - self.aggregate_names = [] if self._fields: + self.extra_names = [] + self.aggregate_names = [] if not self.query.extra_select and not self.query.aggregate_select: - field_names = list(self._fields) + self.field_names = list(self._fields) else: - field_names = [] + self.query.default_cols = False + self.field_names = [] for f in self._fields: if self.query.extra_select.has_key(f): self.extra_names.append(f) elif self.query.aggregate_select.has_key(f): self.aggregate_names.append(f) else: - field_names.append(f) + self.field_names.append(f) else: # Default to all fields. - field_names = [f.attname for f in self.model._meta.fields] + self.extra_names = None + self.field_names = [f.attname for f in self.model._meta.fields] + self.aggregate_names = None self.query.select = [] - self.query.add_fields(field_names, False) - self.query.default_cols = False - self.field_names = field_names + self.query.add_fields(self.field_names, False) def _clone(self, klass=None, setup=False, **kwargs): """ @@ -817,7 +821,8 @@ class ValuesQuerySet(QuerySet): class ValuesListQuerySet(ValuesQuerySet): def iterator(self): - self.query.trim_extra_select(self.extra_names) + if self.extra_names is not None: + self.query.trim_extra_select(self.extra_names) if self.flat and len(self._fields) == 1: for row in self.query.results_iter(): yield row[0] @@ -825,13 +830,24 @@ class ValuesListQuerySet(ValuesQuerySet): for row in self.query.results_iter(): yield tuple(row) else: - # When extra(select=...) is involved, the extra cols come are - # always at the start of the row, so we need to reorder the fields + # When extra(select=...) or an annotation is involved, the extra cols are + # always at the start of the row, and we need to reorder the fields # to match the order in self._fields. - names = self.query.extra_select.keys() + self.field_names + self.query.aggregate_select.keys() + extra_names = self.query.extra_select.keys() + field_names = self.field_names + aggregate_names = self.query.aggregate_select.keys() + names = extra_names + field_names + aggregate_names + + # If a field list has been specified, use it. Otherwise, use the + # full list of fields, including extras and aggregates. + if self._fields: + fields = self._fields + else: + fields = names + for row in self.query.results_iter(): data = dict(zip(names, row)) - yield tuple([data[f] for f in self._fields]) + yield tuple([data[f] for f in fields]) def _clone(self, *args, **kwargs): clone = super(ValuesListQuerySet, self)._clone(*args, **kwargs) diff --git a/tests/regressiontests/aggregation_regress/models.py b/tests/regressiontests/aggregation_regress/models.py index fc2c44b09e..8eac67a0fc 100644 --- a/tests/regressiontests/aggregation_regress/models.py +++ b/tests/regressiontests/aggregation_regress/models.py @@ -91,7 +91,7 @@ __test__ = {'API_TESTS': """ >>> sorted(Book.objects.all().annotate(mean_auth_age=Avg('authors__age')).extra(select={'manufacture_cost' : 'price * .5'}).values().get(pk=2).items()) [('contact_id', 3), ('id', 2), ('isbn', u'067232959'), ('manufacture_cost', ...11.545...), ('mean_auth_age', 45.0), ('name', u'Sams Teach Yourself Django in 24 Hours'), ('pages', 528), ('price', Decimal("23.09")), ('pubdate', datetime.date(2008, 3, 3)), ('publisher_id', 2), ('rating', 3.0)] -# The order of the values, annotate and extra clauses doesn't matter +# The order of the (empty) values, annotate and extra clauses doesn't matter >>> sorted(Book.objects.all().values().annotate(mean_auth_age=Avg('authors__age')).extra(select={'manufacture_cost' : 'price * .5'}).get(pk=2).items()) [('contact_id', 3), ('id', 2), ('isbn', u'067232959'), ('manufacture_cost', ...11.545...), ('mean_auth_age', 45.0), ('name', u'Sams Teach Yourself Django in 24 Hours'), ('pages', 528), ('price', Decimal("23.09")), ('pubdate', datetime.date(2008, 3, 3)), ('publisher_id', 2), ('rating', 3.0)] diff --git a/tests/regressiontests/extra_regress/models.py b/tests/regressiontests/extra_regress/models.py index 680917b8ae..fd34982c9a 100644 --- a/tests/regressiontests/extra_regress/models.py +++ b/tests/regressiontests/extra_regress/models.py @@ -30,6 +30,11 @@ class Order(models.Model): created_by = models.ForeignKey(User) text = models.TextField() +class TestObject(models.Model): + first = models.CharField(max_length=20) + second = models.CharField(max_length=20) + third = models.CharField(max_length=20) + __test__ = {"API_TESTS": """ # Regression tests for #7314 and #7372 @@ -115,4 +120,75 @@ True # cause incorrect SQL to be produced otherwise. >>> RevisionableModel.objects.extra(select={"the_answer": 'id'}).dates('when', 'month') [datetime.datetime(2008, 9, 1, 0, 0)] + +# Regression test for #10256... If there is a values() clause, Extra columns are +# only returned if they are explicitly mentioned. +>>> TestObject(first='first', second='second', third='third').save() + +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values() +[{'bar': u'second', 'third': u'third', 'second': u'second', 'whiz': u'third', 'foo': u'first', 'id': 1, 'first': u'first'}] + +# Extra clauses after an empty values clause are still included +>>> TestObject.objects.values().extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))) +[{'bar': u'second', 'third': u'third', 'second': u'second', 'whiz': u'third', 'foo': u'first', 'id': 1, 'first': u'first'}] + +# Extra columns are ignored if not mentioned in the values() clause +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values('first', 'second') +[{'second': u'second', 'first': u'first'}] + +# Extra columns after a non-empty values() clause are ignored +>>> TestObject.objects.values('first', 'second').extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))) +[{'second': u'second', 'first': u'first'}] + +# Extra columns can be partially returned +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values('first', 'second', 'foo') +[{'second': u'second', 'foo': u'first', 'first': u'first'}] + +# Also works if only extra columns are included +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values('foo', 'whiz') +[{'foo': u'first', 'whiz': u'third'}] + +# Values list works the same way +# All columns are returned for an empty values_list() +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list() +[(u'first', u'second', u'third', 1, u'first', u'second', u'third')] + +# Extra columns after an empty values_list() are still included +>>> TestObject.objects.values_list().extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))) +[(u'first', u'second', u'third', 1, u'first', u'second', u'third')] + +# Extra columns ignored completely if not mentioned in values_list() +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('first', 'second') +[(u'first', u'second')] + +# Extra columns after a non-empty values_list() clause are ignored completely +>>> TestObject.objects.values_list('first', 'second').extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))) +[(u'first', u'second')] + +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('second', flat=True) +[u'second'] + +# Only the extra columns specified in the values_list() are returned +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('first', 'second', 'whiz') +[(u'first', u'second', u'third')] + +# ...also works if only extra columns are included +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('foo','whiz') +[(u'first', u'third')] + +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('whiz', flat=True) +[u'third'] + +# ... and values are returned in the order they are specified +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('whiz','foo') +[(u'third', u'first')] + +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('first','id') +[(u'first', 1)] + +>>> TestObject.objects.extra(select=SortedDict((('foo','first'),('bar','second'),('whiz','third')))).values_list('whiz', 'first', 'bar', 'id') +[(u'third', u'first', u'second', 1)] + """} + +