From b31b2d4da3772463d007c9d986c4bdd1392b09e7 Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Sat, 27 Mar 2010 15:16:27 +0000 Subject: [PATCH] Fixed #13227 -- Modified ForeignKeys to fully honor the db_prep/prep separation introduced by multidb. This was required to ensure that model instances aren't deepcopied as a result of being involved in a filter clause. Thanks to claudep for the report, and Alex Gaynor for the help on the patch. git-svn-id: http://code.djangoproject.com/svn/django/trunk@12865 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/db/models/fields/related.py | 80 ++++++++++++++++--------- django/db/models/query_utils.py | 6 +- tests/regressiontests/queries/models.py | 8 +++ 3 files changed, 64 insertions(+), 30 deletions(-) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 8329792f17..c6723c6826 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -121,32 +121,24 @@ class RelatedField(object): if not cls._meta.abstract: self.contribute_to_related_class(other, self.related) + def get_prep_lookup(self, lookup_type, value): + if hasattr(value, 'prepare'): + return value.prepare() + if hasattr(value, '_prepare'): + return value._prepare() + # FIXME: lt and gt are explicitly allowed to make + # get_(next/prev)_by_date work; other lookups are not allowed since that + # gets messy pretty quick. This is a good candidate for some refactoring + # in the future. + if lookup_type in ['exact', 'gt', 'lt', 'gte', 'lte']: + return self._pk_trace(value, 'get_prep_lookup', lookup_type) + if lookup_type in ('range', 'in'): + return [self._pk_trace(v, 'get_prep_lookup', lookup_type) for v in value] + elif lookup_type == 'isnull': + return [] + raise TypeError("Related Field has invalid lookup: %s" % lookup_type) + def get_db_prep_lookup(self, lookup_type, value, connection, prepared=False): - # If we are doing a lookup on a Related Field, we must be - # comparing object instances. The value should be the PK of value, - # not value itself. - def pk_trace(value): - # Value may be a primary key, or an object held in a relation. - # If it is an object, then we need to get the primary key value for - # that object. In certain conditions (especially one-to-one relations), - # the primary key may itself be an object - so we need to keep drilling - # down until we hit a value that can be used for a comparison. - v, field = value, None - try: - while True: - v, field = getattr(v, v._meta.pk.name), v._meta.pk - except AttributeError: - pass - - if field: - if lookup_type in ('range', 'in'): - v = [v] - v = field.get_db_prep_lookup(lookup_type, v, - connection=connection, prepared=prepared) - if isinstance(v, list): - v = v[0] - return v - if not prepared: value = self.get_prep_lookup(lookup_type, value) if hasattr(value, 'get_compiler'): @@ -162,18 +154,50 @@ class RelatedField(object): sql, params = value._as_sql(connection=connection) return QueryWrapper(('(%s)' % sql), params) - # FIXME: lt and gt are explicitally allowed to make + # FIXME: lt and gt are explicitly allowed to make # get_(next/prev)_by_date work; other lookups are not allowed since that # gets messy pretty quick. This is a good candidate for some refactoring # in the future. if lookup_type in ['exact', 'gt', 'lt', 'gte', 'lte']: - return [pk_trace(value)] + return [self._pk_trace(value, 'get_db_prep_lookup', lookup_type, + connection=connection, prepared=prepared)] if lookup_type in ('range', 'in'): - return [pk_trace(v) for v in value] + return [self._pk_trace(v, 'get_db_prep_lookup', lookup_type, + connection=connection, prepared=prepared) + for v in value] elif lookup_type == 'isnull': return [] raise TypeError("Related Field has invalid lookup: %s" % lookup_type) + def _pk_trace(self, value, prep_func, lookup_type, **kwargs): + # Value may be a primary key, or an object held in a relation. + # If it is an object, then we need to get the primary key value for + # that object. In certain conditions (especially one-to-one relations), + # the primary key may itself be an object - so we need to keep drilling + # down until we hit a value that can be used for a comparison. + v = value + try: + while True: + v = getattr(v, v._meta.pk.name) + except AttributeError: + pass + except exceptions.ObjectDoesNotExist: + v = None + + field = self + while field.rel: + if hasattr(field.rel, 'field_name'): + field = field.rel.to._meta.get_field(field.rel.field_name) + else: + field = field.rel.to._meta.pk + + if lookup_type in ('range', 'in'): + v = [v] + v = getattr(field, prep_func)(lookup_type, v, **kwargs) + if isinstance(v, list): + v = v[0] + return v + def _get_related_query_name(self, opts): # This method defines the name that can be used to identify this # related object in a table-spanning query. It uses the lower-cased diff --git a/django/db/models/query_utils.py b/django/db/models/query_utils.py index c899a846e3..f75b1555ab 100644 --- a/django/db/models/query_utils.py +++ b/django/db/models/query_utils.py @@ -155,7 +155,8 @@ class Q(tree.Node): def _combine(self, other, conn): if not isinstance(other, Q): raise TypeError(other) - obj = deepcopy(self) + obj = type(self)() + obj.add(self, conn) obj.add(other, conn) return obj @@ -166,7 +167,8 @@ class Q(tree.Node): return self._combine(other, self.AND) def __invert__(self): - obj = deepcopy(self) + obj = type(self)() + obj.add(self, self.AND) obj.negate() return obj diff --git a/tests/regressiontests/queries/models.py b/tests/regressiontests/queries/models.py index 69d990eb7f..65b0be419d 100644 --- a/tests/regressiontests/queries/models.py +++ b/tests/regressiontests/queries/models.py @@ -5,6 +5,7 @@ Various complex queries that have been problematic in the past. import datetime import pickle import sys +import threading from django.conf import settings from django.db import models, DEFAULT_DB_ALIAS @@ -45,6 +46,13 @@ class Note(models.Model): def __unicode__(self): return self.note + def __init__(self, *args, **kwargs): + super(Note, self).__init__(*args, **kwargs) + # Regression for #13227 -- having an attribute that + # is unpickleable doesn't stop you from cloning queries + # that use objects of that type as an argument. + self.lock = threading.Lock() + class Annotation(models.Model): name = models.CharField(max_length=10) tag = models.ForeignKey(Tag)