queryset-refactor: Fixed up extra(select=...) calls with parameters so that the

parameters are substituted in correctly in all cases. This introduces an extra
argument to extra() for this purpose; no alternative there.

Also fixed values() to work if you don't specify *all* the extra select aliases
in the values() call.

Refs #3141.


git-svn-id: http://code.djangoproject.com/svn/django/branches/queryset-refactor@7340 bcc190cf-cafb-0310-a4f2-bffc1f526a37

2 files changed, 43 insertions, 24 deletions

diff --git a/django/db/models/query.py b/django/db/models/query.py
index 5cbaf9d5ff..388a54dc85 100644
--- a/django/db/models/query.py
+++ b/django/db/models/query.py
@@ -142,16 +142,16 @@ class _QuerySet(object):
         else:
             requested = None
         max_depth = self.query.max_depth
-        index_end = len(self.model._meta.fields)
         extra_select = self.query.extra_select.keys()
+        index_start = len(extra_select)
         for row in self.query.results_iter():
             if fill_cache:
-                obj, index_end = get_cached_row(self.model, row, 0, max_depth,
-                        requested=requested)
+                obj, _ = get_cached_row(self.model, row, index_start,
+                        max_depth, requested=requested)
             else:
-                obj = self.model(*row[:index_end])
+                obj = self.model(*row[index_start:])
             for i, k in enumerate(extra_select):
-                setattr(obj, k, row[index_end + i])
+                setattr(obj, k, row[i])
             yield obj
 
     def count(self):
@@ -413,14 +413,14 @@ class _QuerySet(object):
         return obj
 
     def extra(self, select=None, where=None, params=None, tables=None,
-            order_by=None):
+            order_by=None, select_params=None):
         """
         Add extra SQL fragments to the query.
         """
         assert self.query.can_filter(), \
                 "Cannot change a query once a slice has been taken"
         clone = self._clone()
-        clone.query.add_extra(select, where, params, tables, order_by)
+        clone.query.add_extra(select, select_params, where, params, tables, order_by)
         return clone
 
     def reverse(self):
@@ -475,9 +475,10 @@ class ValuesQuerySet(QuerySet):
         return self.iterator()
 
     def iterator(self):
-        self.field_names.extend([f for f in self.query.extra_select.keys()])
+        self.query.trim_extra_select(self.extra_names)
+        names = self.query.extra_select.keys() + self.field_names
         for row in self.query.results_iter():
-            yield dict(zip(self.field_names, row))
+            yield dict(zip(names, row))
 
     def _setup_query(self):
         """
@@ -487,6 +488,7 @@ class ValuesQuerySet(QuerySet):
         Called by the _clone() method after initialising the rest of the
         instance.
         """
+        self.extra_names = []
         if self._fields:
             if not self.query.extra_select:
                 field_names = list(self._fields)
@@ -496,7 +498,9 @@ class ValuesQuerySet(QuerySet):
                 for f in self._fields:
                     if f in names:
                         field_names.append(f)
-                    elif not self.query.extra_select.has_key(f):
+                    elif self.query.extra_select.has_key(f):
+                        self.extra_names.append(f)
+                    else:
                         raise FieldDoesNotExist('%s has no field named %r'
                                 % (self.model._meta.object_name, f))
         else:
@@ -513,7 +517,8 @@ class ValuesQuerySet(QuerySet):
         """
         c = super(ValuesQuerySet, self)._clone(klass, **kwargs)
         c._fields = self._fields[:]
-        c.field_names = self.field_names[:]
+        c.field_names = self.field_names
+        c.extra_names = self.extra_names
         if setup and hasattr(c, '_setup_query'):
             c._setup_query()
         return c
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index b67bfae699..6a68448bf1 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -73,6 +73,7 @@ class Query(object):
         # These are for extensions. The contents are more or less appended
         # verbatim to the appropriate clause.
         self.extra_select = {}  # Maps col_alias -> col_sql.
+        self.extra_select_params = ()
         self.extra_tables = ()
         self.extra_where = ()
         self.extra_params = ()
@@ -150,6 +151,7 @@ class Query(object):
         obj.select_related = self.select_related
         obj.max_depth = self.max_depth
         obj.extra_select = self.extra_select.copy()
+        obj.extra_select_params = self.extra_select_params
         obj.extra_tables = self.extra_tables
         obj.extra_where = self.extra_where
         obj.extra_params = self.extra_params
@@ -214,6 +216,7 @@ class Query(object):
         # get_from_clause() for details.
         from_, f_params = self.get_from_clause()
         where, w_params = self.where.as_sql(qn=self.quote_name_unless_alias)
+        params = list(self.extra_select_params)
 
         result = ['SELECT']
         if self.distinct:
@@ -222,7 +225,7 @@ class Query(object):
 
         result.append('FROM')
         result.extend(from_)
-        params = list(f_params)
+        params.extend(f_params)
 
         if where:
             result.append('WHERE %s' % where)
@@ -351,8 +354,8 @@ class Query(object):
         the model.
         """
         qn = self.quote_name_unless_alias
-        result = []
-        aliases = []
+        result = ['(%s) AS %s' % (col, alias) for alias, col in self.extra_select.items()]
+        aliases = self.extra_select.keys()
         if self.select:
             for col in self.select:
                 if isinstance(col, (list, tuple)):
@@ -364,12 +367,9 @@ class Query(object):
                     if hasattr(col, 'alias'):
                         aliases.append(col.alias)
         elif self.default_cols:
-            result = self.get_default_columns(True)
-            aliases = result[:]
-
-        result.extend(['(%s) AS %s' % (col, alias)
-                for alias, col in self.extra_select.items()])
-        aliases.extend(self.extra_select.keys())
+            cols = self.get_default_columns(True)
+            result.extend(cols)
+            aliases.extend(cols)
 
         self._select_aliases = set(aliases)
         return result
@@ -403,9 +403,9 @@ class Query(object):
     def get_from_clause(self):
         """
         Returns a list of strings that are joined together to go after the
-        "FROM" part of the query, as well as any extra parameters that need to
-        be included. Sub-classes, can override this to create a from-clause via
-        a "select", for example (e.g. CountQuery).
+        "FROM" part of the query, as well as a list any extra parameters that
+        need to be included. Sub-classes, can override this to create a
+        from-clause via a "select", for example (e.g. CountQuery).
 
         This should only be called after any SQL construction methods that
         might change the tables we need. This means the select columns and
@@ -1253,6 +1253,7 @@ class Query(object):
             self.distinct = False
         self.select = [select]
         self.extra_select = {}
+        self.extra_select_params = ()
 
     def add_select_related(self, fields):
         """
@@ -1267,7 +1268,7 @@ class Query(object):
                 d = d.setdefault(part, {})
         self.select_related = field_dict
 
-    def add_extra(self, select, where, params, tables, order_by):
+    def add_extra(self, select, select_params, where, params, tables, order_by):
         """
         Adds data to the various extra_* attributes for user-created additions
         to the query.
@@ -1279,6 +1280,8 @@ class Query(object):
                     not isinstance(self.extra_select, SortedDict)):
                 self.extra_select = SortedDict(self.extra_select)
             self.extra_select.update(select)
+        if select_params:
+            self.extra_select_params += tuple(select_params)
         if where:
             self.extra_where += tuple(where)
         if params:
@@ -1288,6 +1291,17 @@ class Query(object):
         if order_by:
             self.extra_order_by = order_by
 
+    def trim_extra_select(self, names):
+        """
+        Removes any aliases in the extra_select dictionary that aren't in
+        'names'.
+
+        This is needed if we are selecting certain values that don't incldue
+        all of the extra_select names.
+        """
+        for key in set(self.extra_select).difference(set(names)):
+            del self.extra_select[key]
+
     def set_start(self, start):
         """
         Sets the table from which to start joining. The start position is