From ade4a1246c5e65ed544a990a720cf1c953ec604b Mon Sep 17 00:00:00 2001 From: Malcolm Tredinnick Date: Fri, 27 Jun 2008 03:27:20 +0000 Subject: [PATCH] Reorganised the internals of the Where node a bit to fix some copying problems. We no longer store any reference to Django field instances or models in the Where node. This should improve cloning speed, fix some pickling difficulties, reduce memory usage and remove some infinite loop possibilities in odd cases. Slightly backwards incompatible if you're writing custom filters. See the BackwardsIncompatibleChanges wiki page for details. Fixed #7128, #7204, #7506. git-svn-id: http://code.djangoproject.com/svn/django/trunk@7773 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/db/models/sql/query.py | 20 +++++++++++++++--- django/db/models/sql/subqueries.py | 12 +++++------ django/db/models/sql/where.py | 27 +++++++++++-------------- tests/regressiontests/queries/models.py | 5 +++++ 4 files changed, 40 insertions(+), 24 deletions(-) diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index e8d10bc55b..fc2a6b2b5d 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -7,6 +7,7 @@ databases). The abstraction barrier only works one way: this module has to know all about the internals of models in order to get the information it needs. """ +import datetime from copy import deepcopy from django.utils.tree import Node @@ -1048,7 +1049,19 @@ class Query(object): # that's harmless. self.promote_alias(table) - self.where.add((alias, col, field, lookup_type, value), connector) + # 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) if negate: for alias in join_list: @@ -1058,7 +1071,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)]) + entry = Node([(alias, j_col, None, 'isnull', True, + [True])]) entry.negate() self.where.add(entry, AND) break @@ -1066,7 +1080,7 @@ 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, field, 'isnull', True)]) + entry = Node([(alias, col, None, 'isnull', True, [True])]) entry.negate() self.where.add(entry, AND) diff --git a/django/db/models/sql/subqueries.py b/django/db/models/sql/subqueries.py index 0bb741d706..d8c5b074ec 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, 'in', + related.field.db_type(), 'in', True, 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, 'exact', - ContentType.objects.get_for_model(cls).id), AND) + w1.add((None, field.column, field.db_type(), 'exact', True, + [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, 'in', + where.add((None, f.m2m_column_name(), f.db_type(), 'in', True, 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, 'in', + where.add((None, field.column, field.db_type(), 'in', True, 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, 'in', + self.where.add((None, f.column, f.db_type(), 'in', True, 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 14e54487a3..eed3bf0e21 100644 --- a/django/db/models/sql/where.py +++ b/django/db/models/sql/where.py @@ -21,8 +21,9 @@ class WhereNode(tree.Node): the correct SQL). The children in this tree are usually either Q-like objects or lists of - [table_alias, field_name, field_class, lookup_type, value]. However, a - child could also be any class with as_sql() and relabel_aliases() methods. + [table_alias, field_name, db_type, lookup_type, value_annotation, + params]. However, a child could also be any class with as_sql() and + relabel_aliases() methods. """ default = AND @@ -88,29 +89,25 @@ class WhereNode(tree.Node): def make_atom(self, child, qn): """ - Turn a tuple (table_alias, field_name, field_class, lookup_type, value) - into valid SQL. + Turn a tuple (table_alias, field_name, db_type, lookup_type, + value_annot, params) into valid SQL. Returns the string for the SQL fragment and the parameters to use for it. """ - table_alias, name, field, lookup_type, value = child + table_alias, name, db_type, lookup_type, value_annot, params = child if table_alias: lhs = '%s.%s' % (qn(table_alias), qn(name)) else: lhs = qn(name) - db_type = field and field.db_type() or None + ##db_type = field and field.db_type() or None field_sql = connection.ops.field_cast_sql(db_type) % lhs - if isinstance(value, datetime.datetime): + if value_annot is datetime.datetime: cast_sql = connection.ops.datetime_cast_sql() else: cast_sql = '%s' - if field: - params = field.get_db_prep_lookup(lookup_type, value) - else: - params = Field().get_db_prep_lookup(lookup_type, value) if isinstance(params, QueryWrapper): extra, params = params.data else: @@ -123,11 +120,11 @@ class WhereNode(tree.Node): connection.operators[lookup_type] % cast_sql), params) if lookup_type == 'in': - if not value: + if not value_annot: raise EmptyResultSet if extra: return ('%s IN %s' % (field_sql, extra), params) - return ('%s IN (%s)' % (field_sql, ', '.join(['%s'] * len(value))), + return ('%s IN (%s)' % (field_sql, ', '.join(['%s'] * len(params))), params) elif lookup_type in ('range', 'year'): return ('%s BETWEEN %%s and %%s' % field_sql, params) @@ -135,8 +132,8 @@ class WhereNode(tree.Node): return ('%s = %%s' % connection.ops.date_extract_sql(lookup_type, field_sql), params) elif lookup_type == 'isnull': - return ('%s IS %sNULL' % (field_sql, (not value and 'NOT ' or '')), - params) + return ('%s IS %sNULL' % (field_sql, + (not value_annot and 'NOT ' or '')), ()) elif lookup_type == 'search': return (connection.ops.fulltext_search_sql(field_sql), params) elif lookup_type in ('regex', 'iregex'): diff --git a/tests/regressiontests/queries/models.py b/tests/regressiontests/queries/models.py index c02ad73998..18cea705e5 100644 --- a/tests/regressiontests/queries/models.py +++ b/tests/regressiontests/queries/models.py @@ -3,6 +3,7 @@ Various complex queries that have been problematic in the past. """ import datetime +import pickle from django.db import models from django.db.models.query import Q @@ -791,5 +792,9 @@ Empty querysets can be merged with others. >>> Note.objects.all() & Note.objects.none() [] +Bug #7204, #7506 -- make sure querysets with related fields can be pickled. If +this doesn't crash, it's a Good Thing. +>>> out = pickle.dumps(Item.objects.all()) + """}