MJG-MBP:django_on_delete_patch mjg$ svn diff Index: django/db/models/base.py =================================================================== --- django/db/models/base.py (revision 11620) +++ django/db/models/base.py (working copy) @@ -13,10 +13,11 @@ from django.db.models.fields import AutoField, FieldDoesNotExist from django.db.models.fields.related import OneToOneRel, ManyToOneRel, OneToOneField from django.db.models.query import delete_objects, Q -from django.db.models.query_utils import CollectedObjects, DeferredAttribute +from django.db.models.query_utils import CollectedFields, CollectedObjects, DeferredAttribute from django.db.models.options import Options -from django.db import connection, transaction, DatabaseError +from django.db import connection, transaction, DatabaseError, IntegrityError from django.db.models import signals +from django.db.models.fields.related import CASCADE, PROTECT, SET_NULL, SET_DEFAULT from django.db.models.loading import register_models, get_model from django.utils.functional import curry from django.utils.encoding import smart_str, force_unicode, smart_unicode @@ -507,7 +508,7 @@ save_base.alters_data = True - def _collect_sub_objects(self, seen_objs, parent=None, nullable=False): + def _collect_sub_objects(self, seen_objs, fields_to_set, parent=None, nullable=False): """ Recursively populates seen_objs with all objects related to this object. @@ -519,16 +520,65 @@ pk_val = self._get_pk_val() if seen_objs.add(self.__class__, pk_val, self, parent, nullable): return + + def _handle_sub_obj(related, sub_obj): + on_delete = related.field.rel.on_delete + if on_delete is None: + #If no explicit on_delete option is specified, use the old + #django behavior as the default: SET_NULL if the foreign + #key is nullable, otherwise CASCADE. + if related.field.null: + on_delete = SET_NULL + else: + on_delete = CASCADE + + if on_delete == CASCADE: + sub_obj._collect_sub_objects(seen_objs, fields_to_set, self.__class__) + elif on_delete == PROTECT: + msg = '[Django] Cannot delete a parent object: a foreign key constraint fails (ForeignKey `%s` (pk=`%s`) references `%s` (pk=`%s`))' % ( + sub_obj.__class__, + sub_obj._get_pk_val(), + self.__class__, + pk_val, + ) + raise IntegrityError(msg) + elif on_delete == SET_NULL: + if not related.field.null: + msg = '[Django] Cannot delete a parent object: foreign key constraint on_delete=SET_NULL is specified for a non-nullable foreign key (ForeignKey `%s` (pk=`%s`) references `%s` (pk=`%s`))' % ( + sub_obj.__class__, + sub_obj._get_pk_val(), + self.__class__, + pk_val, + ) + raise IntegrityError(msg) + fields_to_set.add(sub_obj.__class__, sub_obj._get_pk_val(), sub_obj, related.field.name, None) + elif on_delete == SET_DEFAULT: + if not related.field.has_default(): + msg = '[Django] Cannot delete a parent object: foreign key constraint on_delete=SET_DEFAULT is specified for a foreign key with no default value (ForeignKey `%s` (pk=`%s`) references `%s` (pk=`%s`))' % ( + sub_obj.__class__, + sub_obj._get_pk_val(), + self.__class__, + pk_val, + ) + raise IntegrityError(msg) + fields_to_set.add(sub_obj.__class__, sub_obj._get_pk_val(), sub_obj, related.field.name, related.field.get_default()) + else: + raise AttributeError('Unexpected value for on_delete') for related in self._meta.get_all_related_objects(): rel_opts_name = related.get_accessor_name() if isinstance(related.field.rel, OneToOneRel): try: + # delattr(self, rel_opts_name) #Delete first to clear any stale cache + #TODO: the above line is a bit of a hack + #It's one way (not a very good one) to work around stale cache data causing + #spurious RESTRICT errors, etc; it would be better to prevent the cache from + #becoming stale in the first place. sub_obj = getattr(self, rel_opts_name) except ObjectDoesNotExist: pass else: - sub_obj._collect_sub_objects(seen_objs, self.__class__, related.field.null) + _handle_sub_obj(related, sub_obj) else: # To make sure we can access all elements, we can't use the # normal manager on the related object. So we work directly @@ -541,7 +591,7 @@ raise AssertionError("Should never get here.") delete_qs = rel_descriptor.delete_manager(self).all() for sub_obj in delete_qs: - sub_obj._collect_sub_objects(seen_objs, self.__class__, related.field.null) + _handle_sub_obj(related, sub_obj) # Handle any ancestors (for the model-inheritance case). We do this by # traversing to the most remote parent classes -- those with no parents @@ -556,18 +606,18 @@ continue # At this point, parent_obj is base class (no ancestor models). So # delete it and all its descendents. - parent_obj._collect_sub_objects(seen_objs) + parent_obj._collect_sub_objects(seen_objs, fields_to_set) def delete(self): assert self._get_pk_val() is not None, "%s object can't be deleted because its %s attribute is set to None." % (self._meta.object_name, self._meta.pk.attname) # Find all the objects than need to be deleted. seen_objs = CollectedObjects() - self._collect_sub_objects(seen_objs) + fields_to_set = CollectedFields() + self._collect_sub_objects(seen_objs, fields_to_set) # Actually delete the objects. - delete_objects(seen_objs) - + delete_objects(seen_objs, fields_to_set) delete.alters_data = True def _get_FIELD_display(self, field): Index: django/db/models/fields/related.py =================================================================== --- django/db/models/fields/related.py (revision 11620) +++ django/db/models/fields/related.py (working copy) @@ -20,6 +20,16 @@ pending_lookups = {} +class CASCADE(object): + pass +class PROTECT(object): + pass +class SET_NULL(object): + pass +class SET_DEFAULT(object): + pass +ALLOWED_ON_DELETE_ACTION_TYPES = set([None, CASCADE, PROTECT, SET_NULL, SET_DEFAULT]) + def add_lazy_relation(cls, field, relation, operation): """ Adds a lookup on ``cls`` when a related field is defined using a string, @@ -218,6 +228,16 @@ # object you just set. setattr(instance, self.cache_name, value) setattr(value, self.related.field.get_cache_name(), instance) + + #TODO: the following function is a bit of a hack + #It's one way (not a very good one) to work around stale cache data causing + #spurious RESTRICT errors, etc; it would be better to prevent the cache from + #becoming stale in the first place. + # def __delete__(self, instance): + # try: + # return delattr(instance, self.cache_name) + # except AttributeError: + # pass class ReverseSingleRelatedObjectDescriptor(object): # This class provides the functionality that makes the related-object @@ -628,7 +648,8 @@ class ManyToOneRel(object): def __init__(self, to, field_name, related_name=None, - limit_choices_to=None, lookup_overrides=None, parent_link=False): + limit_choices_to=None, lookup_overrides=None, parent_link=False, + on_delete=None): try: to._meta except AttributeError: # to._meta doesn't exist, so it must be RECURSIVE_RELATIONSHIP_CONSTANT @@ -641,6 +662,7 @@ self.lookup_overrides = lookup_overrides or {} self.multiple = True self.parent_link = parent_link + self.on_delete = on_delete def get_related_field(self): """ @@ -655,10 +677,12 @@ class OneToOneRel(ManyToOneRel): def __init__(self, to, field_name, related_name=None, - limit_choices_to=None, lookup_overrides=None, parent_link=False): + limit_choices_to=None, lookup_overrides=None, parent_link=False, + on_delete=None): super(OneToOneRel, self).__init__(to, field_name, related_name=related_name, limit_choices_to=limit_choices_to, - lookup_overrides=lookup_overrides, parent_link=parent_link) + lookup_overrides=lookup_overrides, parent_link=parent_link, + on_delete=on_delete) self.multiple = False class ManyToManyRel(object): @@ -697,7 +721,8 @@ related_name=kwargs.pop('related_name', None), limit_choices_to=kwargs.pop('limit_choices_to', None), lookup_overrides=kwargs.pop('lookup_overrides', None), - parent_link=kwargs.pop('parent_link', False)) + parent_link=kwargs.pop('parent_link', False), + on_delete=kwargs.pop('on_delete', None)) Field.__init__(self, **kwargs) self.db_index = True @@ -742,6 +767,16 @@ target = self.rel.to._meta.db_table cls._meta.duplicate_targets[self.column] = (target, "o2m") + on_delete = self.rel.on_delete + if on_delete not in ALLOWED_ON_DELETE_ACTION_TYPES: + raise ValueError("Invalid value 'on_delete=%s' specified for %s %s.%s." % (on_delete, type(self).__name__, cls.__name__, name)) + if on_delete == SET_NULL and not self.null: + specification = "'on_delete=SET_NULL'" + raise ValueError("%s specified for %s '%s.%s', but the field is not nullable." % (specification, type(self).__name__, cls.__name__, name)) + if on_delete == SET_DEFAULT and not self.has_default(): + specification = "'on_delete=SET_DEFAULT'" + raise ValueError("%s specified for %s '%s.%s', but the field has no default value." % (specification, type(self).__name__, cls.__name__, name)) + def contribute_to_related_class(self, cls, related): setattr(cls, related.get_accessor_name(), ForeignRelatedObjectsDescriptor(related)) Index: django/db/models/__init__.py =================================================================== --- django/db/models/__init__.py (revision 11620) +++ django/db/models/__init__.py (working copy) @@ -11,6 +11,7 @@ from django.db.models.fields.subclassing import SubfieldBase from django.db.models.fields.files import FileField, ImageField from django.db.models.fields.related import ForeignKey, OneToOneField, ManyToManyField, ManyToOneRel, ManyToManyRel, OneToOneRel +from django.db.models.fields.related import CASCADE, PROTECT, SET_NULL, SET_DEFAULT from django.db.models import signals # Admin stages. Index: django/db/models/query.py =================================================================== --- django/db/models/query.py (revision 11620) +++ django/db/models/query.py (working copy) @@ -11,8 +11,9 @@ from django.db import connection, transaction, IntegrityError from django.db.models.aggregates import Aggregate +from django.db.models.sql.constants import GET_ITERATOR_CHUNK_SIZE from django.db.models.fields import DateField -from django.db.models.query_utils import Q, select_related_descend, CollectedObjects, CyclicDependency, deferred_class_factory +from django.db.models.query_utils import Q, select_related_descend, CollectedFields, CollectedObjects, CyclicDependency, deferred_class_factory from django.db.models import signals, sql @@ -391,12 +392,13 @@ # Collect all the objects to be deleted in this chunk, and all the # objects that are related to the objects that are to be deleted. seen_objs = CollectedObjects(seen_objs) + fields_to_set = CollectedFields() for object in del_query[:CHUNK_SIZE]: - object._collect_sub_objects(seen_objs) + object._collect_sub_objects(seen_objs, fields_to_set) if not seen_objs: break - delete_objects(seen_objs) + delete_objects(seen_objs, fields_to_set) # Clear the result cache, in case this QuerySet gets reused. self._result_cache = None @@ -1002,7 +1004,7 @@ setattr(obj, f.get_cache_name(), rel_obj) return obj, index_end -def delete_objects(seen_objs): +def delete_objects(seen_objs, fields_to_set): """ Iterate through a list of seen classes, and remove any instances that are referred to. @@ -1023,6 +1025,19 @@ obj_pairs = {} try: + for cls, cls_dct in fields_to_set.iteritems(): + #TODO: batch these, similar to UpdateQuery.clear_related? + #(Note that it may be harder to do here because the default value + #for a given field may be different for each instance, + #while UpdateQuery.clear_related always uses the value None). + query = sql.UpdateQuery(cls, connection) + for instance, field_names_and_values in cls_dct.itervalues(): + query.where = query.where_class() + pk = query.model._meta.pk + query.where.add((sql.where.Constraint(None, pk.column, pk), 'exact', instance.pk), sql.where.AND) + query.add_update_values(field_names_and_values) + query.execute_sql() + for cls in ordered_classes: items = seen_objs[cls].items() items.sort() @@ -1032,33 +1047,29 @@ for pk_val, instance in items: signals.pre_delete.send(sender=cls, instance=instance) + # Handle related GenericRelation and ManyToManyField instances pk_list = [pk for pk,instance in items] del_query = sql.DeleteQuery(cls, connection) del_query.delete_batch_related(pk_list) - update_query = sql.UpdateQuery(cls, connection) - for field, model in cls._meta.get_fields_with_model(): - if (field.rel and field.null and field.rel.to in seen_objs and - filter(lambda f: f.column == field.rel.get_related_field().column, - field.rel.to._meta.fields)): - if model: - sql.UpdateQuery(model, connection).clear_related(field, - pk_list) - else: - update_query.clear_related(field, pk_list) - - # Now delete the actual data. for cls in ordered_classes: items = obj_pairs[cls] items.reverse() - pk_list = [pk for pk,instance in items] del_query = sql.DeleteQuery(cls, connection) del_query.delete_batch(pk_list) - # Last cleanup; set NULLs where there once was a reference to the - # object, NULL the primary key of the found objects, and perform - # post-notification. + #Last cleanup; set NULLs and default values where there once was a + #reference to the object, NULL the primary key of the found objects, + #and perform post-notification. + for cls, cls_dct in fields_to_set.iteritems(): + for instance, field_names_and_values in cls_dct.itervalues(): + for field_name, field_value in field_names_and_values.iteritems(): + field = cls._meta.get_field_by_name(field_name)[0] + setattr(instance, field.attname, field_value) + for cls in ordered_classes: + items = obj_pairs[cls] + items.reverse() for pk_val, instance in items: for field in cls._meta.fields: if field.rel and field.null and field.rel.to in seen_objs: Index: django/db/models/query_utils.py =================================================================== --- django/db/models/query_utils.py (revision 11620) +++ django/db/models/query_utils.py (working copy) @@ -124,6 +124,56 @@ """ return self.data.keys() +class CollectedFields(object): + """ + A container that stores the model object and field name + for fields that need to be set to enforce on_delete=SET_NULL + and on_delete=SET_DEFAULT ForeigKey constraints. + """ + + def __init__(self): + self.data = {} + + def add(self, model, pk, obj, field_name, field_value): + """ + Adds an item. + model is the class of the object being added, + pk is the primary key, obj is the object itself, + field_name is the name of the field to be set, + field_value is the value it needs to be set to. + """ + d = self.data.setdefault(model, SortedDict()) + obj, field_names_and_values = d.setdefault(pk, (obj, dict())) + assert field_name not in field_names_and_values or field_names_and_values[field_name] == field_value + field_names_and_values[field_name] = field_value + + def __contains__(self, key): + return self.data.__contains__(key) + + def __getitem__(self, key): + return self.data[key] + + def __nonzero__(self): + return bool(self.data) + + def iteritems(self): + return self.data.iteritems() + + def iterkeys(self): + return self.data.iterkeys() + + def itervalues(self): + return self.data.itervalues() + + def items(self): + return self.data.items() + + def keys(self): + return self.data.keys() + + def values(self): + return self.data.values() + class QueryWrapper(object): """ A type that indicates the contents are an SQL fragment and the associate Index: tests/modeltests/delete/models.py =================================================================== --- tests/modeltests/delete/models.py (revision 11620) +++ tests/modeltests/delete/models.py (working copy) @@ -46,7 +46,7 @@ ## First, test the CollectedObjects data structure directly ->>> from django.db.models.query import CollectedObjects +>>> from django.db.models.query_utils import CollectedFields, CollectedObjects >>> g = CollectedObjects() >>> g.add("key1", 1, "item1", None) @@ -112,10 +112,12 @@ >>> d1 = D(c=c1, a=a1) >>> d1.save() ->>> o = CollectedObjects() ->>> a1._collect_sub_objects(o) +>>> o, f = CollectedObjects(), CollectedFields() +>>> a1._collect_sub_objects(o, f) >>> o.keys() [, , , ] +>>> f.keys() +[] >>> a1.delete() # Same again with a known bad order @@ -131,10 +133,12 @@ >>> d2 = D(c=c2, a=a2) >>> d2.save() ->>> o = CollectedObjects() ->>> a2._collect_sub_objects(o) +>>> o, f = CollectedObjects(), CollectedFields() +>>> a2._collect_sub_objects(o, f) >>> o.keys() [, , , ] +>>> f.keys() +[] >>> a2.delete() ### Tests for models E,F - nullable related fields ### @@ -163,21 +167,14 @@ # Since E.f is nullable, we should delete F first (after nulling out # the E.f field), then E. ->>> o = CollectedObjects() ->>> e1._collect_sub_objects(o) +>>> o, f = CollectedObjects(), CollectedFields() +>>> e1._collect_sub_objects(o, f) >>> o.keys() [, ] +>>> f.keys() +[] -# temporarily replace the UpdateQuery class to verify that E.f is actually nulled out first ->>> import django.db.models.sql ->>> class LoggingUpdateQuery(django.db.models.sql.UpdateQuery): -... def clear_related(self, related_field, pk_list): -... print "CLEARING FIELD",related_field.name -... return super(LoggingUpdateQuery, self).clear_related(related_field, pk_list) ->>> original_class = django.db.models.sql.UpdateQuery ->>> django.db.models.sql.UpdateQuery = LoggingUpdateQuery >>> e1.delete() -CLEARING FIELD f >>> e2 = E() >>> e2.save() @@ -188,15 +185,13 @@ # Same deal as before, though we are starting from the other object. ->>> o = CollectedObjects() ->>> f2._collect_sub_objects(o) +>>> o, f = CollectedObjects(), CollectedFields() +>>> f2._collect_sub_objects(o, f) >>> o.keys() -[, ] +[] +>>> f.keys() +[] >>> f2.delete() -CLEARING FIELD f - -# Put this back to normal ->>> django.db.models.sql.UpdateQuery = original_class """ } MJG-MBP:django_on_delete_patch mjg$