From 6dd2b5468fa275d53aa60fdcaff8c28bdc5e9c25 Mon Sep 17 00:00:00 2001 From: Malcolm Tredinnick Date: Fri, 4 Jul 2008 06:42:58 +0000 Subject: [PATCH] Redo the changes in [7773] in a better way. This removes some of the leaky abstraction problems (lifting WhereNode internals into the Query class) from that commit and makes it possible for extensions to WhereNode to have access to the field instances. It's also backwards-compatible with pre-[7773] code, which is also better. git-svn-id: http://code.djangoproject.com/svn/django/trunk@7835 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/db/models/sql/query.py | 21 +++------ django/db/models/sql/subqueries.py | 12 ++--- django/db/models/sql/where.py | 71 ++++++++++++++++++++---------- django/utils/tree.py | 31 ++++++++++--- 4 files changed, 84 insertions(+), 51 deletions(-) diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index 6eccaf997ee..7944d0358fa 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -1104,19 +1104,7 @@ class Query(object): # that's harmless. self.promote_alias(table) - # To save memory and copying time, convert the value from the Python - # object to the actual value used in the SQL query. - if field: - params = field.get_db_prep_lookup(lookup_type, value) - else: - params = Field().get_db_prep_lookup(lookup_type, value) - if isinstance(value, datetime.datetime): - annotation = datetime.datetime - else: - annotation = bool(value) - - self.where.add((alias, col, field.db_type(), lookup_type, annotation, - params), connector) + self.where.add((alias, col, field, lookup_type, value), connector) if negate: for alias in join_list: @@ -1126,8 +1114,8 @@ class Query(object): for alias in join_list: if self.alias_map[alias][JOIN_TYPE] == self.LOUTER: j_col = self.alias_map[alias][RHS_JOIN_COL] - entry = Node([(alias, j_col, None, 'isnull', True, - [True])]) + entry = self.where_class() + entry.add((alias, j_col, None, 'isnull', True), AND) entry.negate() self.where.add(entry, AND) break @@ -1135,7 +1123,8 @@ class Query(object): # Leaky abstraction artifact: We have to specifically # exclude the "foo__in=[]" case from this handling, because # it's short-circuited in the Where class. - entry = Node([(alias, col, None, 'isnull', True, [True])]) + entry = self.where_class() + entry.add((alias, col, None, 'isnull', True), AND) entry.negate() self.where.add(entry, AND) diff --git a/django/db/models/sql/subqueries.py b/django/db/models/sql/subqueries.py index d8c5b074eca..0bb741d7067 100644 --- a/django/db/models/sql/subqueries.py +++ b/django/db/models/sql/subqueries.py @@ -49,7 +49,7 @@ class DeleteQuery(Query): for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE): where = self.where_class() where.add((None, related.field.m2m_reverse_name(), - related.field.db_type(), 'in', True, + related.field, 'in', pk_list[offset : offset+GET_ITERATOR_CHUNK_SIZE]), AND) self.do_query(related.field.m2m_db_table(), where) @@ -59,11 +59,11 @@ class DeleteQuery(Query): if isinstance(f, generic.GenericRelation): from django.contrib.contenttypes.models import ContentType field = f.rel.to._meta.get_field(f.content_type_field_name) - w1.add((None, field.column, field.db_type(), 'exact', True, - [ContentType.objects.get_for_model(cls).id]), AND) + w1.add((None, field.column, field, 'exact', + ContentType.objects.get_for_model(cls).id), AND) for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE): where = self.where_class() - where.add((None, f.m2m_column_name(), f.db_type(), 'in', True, + where.add((None, f.m2m_column_name(), f, 'in', pk_list[offset : offset + GET_ITERATOR_CHUNK_SIZE]), AND) if w1: @@ -81,7 +81,7 @@ class DeleteQuery(Query): for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE): where = self.where_class() field = self.model._meta.pk - where.add((None, field.column, field.db_type(), 'in', True, + where.add((None, field.column, field, 'in', pk_list[offset : offset + GET_ITERATOR_CHUNK_SIZE]), AND) self.do_query(self.model._meta.db_table, where) @@ -204,7 +204,7 @@ class UpdateQuery(Query): for offset in range(0, len(pk_list), GET_ITERATOR_CHUNK_SIZE): self.where = self.where_class() f = self.model._meta.pk - self.where.add((None, f.column, f.db_type(), 'in', True, + self.where.add((None, f.column, f, 'in', pk_list[offset : offset + GET_ITERATOR_CHUNK_SIZE]), AND) self.values = [(related_field.column, None, '%s')] diff --git a/django/db/models/sql/where.py b/django/db/models/sql/where.py index bf45bceb4ba..18e4bf2f7e9 100644 --- a/django/db/models/sql/where.py +++ b/django/db/models/sql/where.py @@ -27,7 +27,36 @@ class WhereNode(tree.Node): """ default = AND - def as_sql(self, node=None, qn=None): + def add(self, data, connector): + """ + Add a node to the where-tree. If the data is a list or tuple, it is + expected to be of the form (alias, col_name, field_obj, lookup_type, + value), which is then slightly munged before being stored (to avoid + storing any reference to field objects). Otherwise, the 'data' is + stored unchanged and can be anything with an 'as_sql()' method. + """ + if not isinstance(data, (list, tuple)): + super(WhereNode, self).add(data, connector) + return + + alias, col, field, lookup_type, value = data + if field: + params = field.get_db_prep_lookup(lookup_type, value) + db_type = field.db_type() + else: + # This is possible when we add a comparison to NULL sometimes (we + # don't really need to waste time looking up the associated field + # object). + params = Field().get_db_prep_lookup(lookup_type, value) + db_type = None + if isinstance(value, datetime.datetime): + annotation = datetime.datetime + else: + annotation = bool(value) + super(WhereNode, self).add((alias, col, db_type, lookup_type, + annotation, params), connector) + + def as_sql(self, qn=None): """ Returns the SQL version of the where clause and the value to be substituted in. Returns None, None if this node is empty. @@ -36,60 +65,56 @@ class WhereNode(tree.Node): (generally not needed except by the internal implementation for recursion). """ - if node is None: - node = self if not qn: qn = connection.ops.quote_name - if not node.children: + if not self.children: return None, [] result = [] result_params = [] empty = True - for child in node.children: + for child in self.children: try: if hasattr(child, 'as_sql'): sql, params = child.as_sql(qn=qn) - format = '(%s)' - elif isinstance(child, tree.Node): - sql, params = self.as_sql(child, qn) - if child.negated: - format = 'NOT (%s)' - elif len(child.children) == 1: - format = '%s' - else: - format = '(%s)' else: + # A leaf node in the tree. sql, params = self.make_atom(child, qn) - format = '%s' except EmptyResultSet: - if node.connector == AND and not node.negated: + if self.connector == AND and not self.negated: # We can bail out early in this particular case (only). raise - elif node.negated: + elif self.negated: empty = False continue except FullResultSet: if self.connector == OR: - if node.negated: + if self.negated: empty = True break # We match everything. No need for any constraints. return '', [] - if node.negated: + if self.negated: empty = True continue empty = False if sql: - result.append(format % sql) + result.append(sql) result_params.extend(params) if empty: raise EmptyResultSet - conn = ' %s ' % node.connector - return conn.join(result), result_params + + conn = ' %s ' % self.connector + sql_string = conn.join(result) + if sql_string: + if self.negated: + sql_string = 'NOT (%s)' % sql_string + elif len(self.children) != 1: + sql_string = '(%s)' % sql_string + return sql_string, result_params def make_atom(self, child, qn): """ - Turn a tuple (table_alias, field_name, db_type, lookup_type, + Turn a tuple (table_alias, column_name, db_type, lookup_type, value_annot, params) into valid SQL. Returns the string for the SQL fragment and the parameters to use for diff --git a/django/utils/tree.py b/django/utils/tree.py index a62a4ae6c3a..5dd9f567994 100644 --- a/django/utils/tree.py +++ b/django/utils/tree.py @@ -29,6 +29,22 @@ class Node(object): self.subtree_parents = [] self.negated = negated + # We need this because of django.db.models.query_utils.Q. Q. __init__() is + # problematic, but it is a natural Node subclass in all other respects. + def _new_instance(cls, children=None, connector=None, negated=False): + """ + This is called to create a new instance of this class when we need new + Nodes (or subclasses) in the internal code in this class. Normally, it + just shadows __init__(). However, subclasses with an __init__ signature + that is not an extension of Node.__init__ might need to implement this + method to allow a Node to create a new instance of them (if they have + any extra setting up to do). + """ + obj = Node(children, connector, negated) + obj.__class__ = cls + return obj + _new_instance = classmethod(_new_instance) + def __str__(self): if self.negated: return '(NOT (%s: %s))' % (self.connector, ', '.join([str(c) for c @@ -82,7 +98,8 @@ class Node(object): else: self.children.append(node) else: - obj = Node(self.children, self.connector, self.negated) + obj = self._new_instance(self.children, self.connector, + self.negated) self.connector = conn_type self.children = [obj, node] @@ -96,7 +113,8 @@ class Node(object): Interpreting the meaning of this negate is up to client code. This method is useful for implementing "not" arrangements. """ - self.children = [Node(self.children, self.connector, not self.negated)] + self.children = [self._new_instance(self.children, self.connector, + not self.negated)] self.connector = self.default def start_subtree(self, conn_type): @@ -108,12 +126,13 @@ class Node(object): if len(self.children) == 1: self.connector = conn_type elif self.connector != conn_type: - self.children = [Node(self.children, self.connector, self.negated)] + self.children = [self._new_instance(self.children, self.connector, + self.negated)] self.connector = conn_type self.negated = False - self.subtree_parents.append(Node(self.children, self.connector, - self.negated)) + self.subtree_parents.append(self.__class__(self.children, + self.connector, self.negated)) self.connector = self.default self.negated = False self.children = [] @@ -126,7 +145,7 @@ class Node(object): the current instances state to be the parent. """ obj = self.subtree_parents.pop() - node = Node(self.children, self.connector) + node = self.__class__(self.children, self.connector) self.connector = obj.connector self.negated = obj.negated self.children = obj.children