From 12716794dbcf2e1bf7a07bd18dfc0ae24be4e6f8 Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Mon, 9 Jun 2008 14:03:35 +0000 Subject: [PATCH] Fixed #7350, #7202 -- Fixed serialization for multi-model inheritance, which had multiple problems: * Serializers were including all superclass fields in their output. Now only local fields are included. * Implicit OneToOne primary keys were not correctly added to the metamodel, so they were always marked to be serialized, even though they were primary * Model saving was too aggressive about creating new parent class instances during deserialization. Raw save on a model now skips saving of the parent class. git-svn-id: http://code.djangoproject.com/svn/django/trunk@7600 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/core/serializers/base.py | 2 +- django/db/models/base.py | 13 +- django/db/models/options.py | 2 +- docs/serialization.txt | 35 +++++ tests/modeltests/model_inheritance/models.py | 5 + .../model_inheritance_regress/__init__.py | 0 .../model_inheritance_regress/models.py | 120 ++++++++++++++++++ .../serializers_regress/models.py | 20 +++ .../serializers_regress/tests.py | 48 ++++++- 9 files changed, 232 insertions(+), 13 deletions(-) create mode 100644 tests/regressiontests/model_inheritance_regress/__init__.py create mode 100644 tests/regressiontests/model_inheritance_regress/models.py diff --git a/django/core/serializers/base.py b/django/core/serializers/base.py index a79497ecec..e22a35815b 100644 --- a/django/core/serializers/base.py +++ b/django/core/serializers/base.py @@ -38,7 +38,7 @@ class Serializer(object): self.start_serialization() for obj in queryset: self.start_object(obj) - for field in obj._meta.fields: + for field in obj._meta.local_fields: if field.serialize: if field.rel is None: if self.selected_fields is None or field.attname in self.selected_fields: diff --git a/django/db/models/base.py b/django/db/models/base.py index 0ee225675a..5dd11a9d83 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -290,12 +290,17 @@ class Model(object): meta = cls._meta signal = False - for parent, field in meta.parents.items(): - self.save_base(raw, parent) - setattr(self, field.attname, self._get_pk_val(parent._meta)) + # If we are in a raw save, save the object exactly as presented. + # That means that we don't try to be smart about saving attributes + # that might have come from the parent class - we just save the + # attributes we have been given to the class we have been given. + if not raw: + for parent, field in meta.parents.items(): + self.save_base(raw, parent) + setattr(self, field.attname, self._get_pk_val(parent._meta)) non_pks = [f for f in meta.local_fields if not f.primary_key] - + # First, try an UPDATE. If that doesn't update anything, do an INSERT. pk_val = self._get_pk_val(meta) # Note: the comparison with '' is required for compatibility with diff --git a/django/db/models/options.py b/django/db/models/options.py index 4036bfb36e..bc1aec62c1 100644 --- a/django/db/models/options.py +++ b/django/db/models/options.py @@ -102,7 +102,7 @@ class Options(object): # field. field = self.parents.value_for_index(0) field.primary_key = True - self.pk = field + self.setup_pk(field) else: auto = AutoField(verbose_name='ID', primary_key=True, auto_created=True) diff --git a/docs/serialization.txt b/docs/serialization.txt index 8a672d8b8a..2a3e7038da 100644 --- a/docs/serialization.txt +++ b/docs/serialization.txt @@ -63,6 +63,41 @@ be serialized. doesn't specify all the fields that are required by a model, the deserializer will not be able to save deserialized instances. +Inherited Models +~~~~~~~~~~~~~~~~ + +If you have a model that is defined using an `abstract base class`_, you don't +have to do anything special to serialize that model. Just call the serializer +on the object (or objects) that you want to serialize, and the output will be +a complete representation of the serialized object. + +However, if you have a model that uses `multi-table inheritance`_, you also +need to serialize all of the base classes for the model. This is because only +the fields that are locally defined on the model will be serialized. For +example, consider the following models:: + + class Place(models.Model): + name = models.CharField(max_length=50) + + class Restaurant(Place): + serves_hot_dogs = models.BooleanField() + +If you only serialize the Restaurant model:: + + data = serializers.serialize('xml', Restaurant.objects.all()) + +the fields on the serialized output will only contain the `serves_hot_dogs` +attribute. The `name` attribute of the base class will be ignored. + +In order to fully serialize your Restaurant instances, you will need to +serialize the Place models as well:: + + all_objects = list(Restaurant.objects.all()) + list(Place.objects.all()) + data = serializers.serialize('xml', all_objects) + +.. _abstract base class: http://www.djangoproject.com/documentation/model-api/#abstract-base-classes +.. _multi-table inheritance: http://www.djangoproject.com/documentation/model-api/#multi-table-inheritance + Deserializing data ------------------ diff --git a/tests/modeltests/model_inheritance/models.py b/tests/modeltests/model_inheritance/models.py index b1a751f5e8..7c737b6bd1 100644 --- a/tests/modeltests/model_inheritance/models.py +++ b/tests/modeltests/model_inheritance/models.py @@ -147,8 +147,13 @@ Test constructor for Restaurant. >>> c.save() >>> ir = ItalianRestaurant(name='Ristorante Miron', address='1234 W. Ash', serves_hot_dogs=False, serves_pizza=False, serves_gnocchi=True, rating=4, chef=c) >>> ir.save() +>>> ItalianRestaurant.objects.filter(address='1234 W. Ash') +[] + >>> ir.address = '1234 W. Elm' >>> ir.save() +>>> ItalianRestaurant.objects.filter(address='1234 W. Elm') +[] # Make sure Restaurant and ItalianRestaurant have the right fields in the right # order. diff --git a/tests/regressiontests/model_inheritance_regress/__init__.py b/tests/regressiontests/model_inheritance_regress/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/regressiontests/model_inheritance_regress/models.py b/tests/regressiontests/model_inheritance_regress/models.py new file mode 100644 index 0000000000..8801715a0c --- /dev/null +++ b/tests/regressiontests/model_inheritance_regress/models.py @@ -0,0 +1,120 @@ +""" +Regression tests for Model inheritance behaviour. +""" + +from django.db import models + +class Place(models.Model): + name = models.CharField(max_length=50) + address = models.CharField(max_length=80) + + class Meta: + ordering = ('name',) + + def __unicode__(self): + return u"%s the place" % self.name + +class Restaurant(Place): + serves_hot_dogs = models.BooleanField() + serves_pizza = models.BooleanField() + + def __unicode__(self): + return u"%s the restaurant" % self.name + +class ItalianRestaurant(Restaurant): + serves_gnocchi = models.BooleanField() + + def __unicode__(self): + return u"%s the italian restaurant" % self.name + +class ParkingLot(Place): + # An explicit link to the parent (we can control the attribute name). + parent = models.OneToOneField(Place, primary_key=True, parent_link=True) + capacity = models.IntegerField() + + def __unicode__(self): + return u"%s the parking lot" % self.name + +__test__ = {'API_TESTS':""" +# Regression for #7350, #7202 +# Check that when you create a Parent object with a specific reference to an existent +# child instance, saving the Parent doesn't duplicate the child. +# This behaviour is only activated during a raw save - it is mostly relevant to +# deserialization, but any sort of CORBA style 'narrow()' API would require a +# similar approach. + +# Create a child-parent-grandparent chain +>>> place1 = Place(name="Guido's House of Pasta", address='944 W. Fullerton') +>>> place1.save_base(raw=True) +>>> restaurant = Restaurant(place_ptr=place1, serves_hot_dogs=True, serves_pizza=False) +>>> restaurant.save_base(raw=True) +>>> italian_restaurant = ItalianRestaurant(restaurant_ptr=restaurant, serves_gnocchi=True) +>>> italian_restaurant.save_base(raw=True) + +# Create a child-parent chain with an explicit parent link +>>> place2 = Place(name='Main St', address='111 Main St') +>>> place2.save_base(raw=True) +>>> park = ParkingLot(parent=place2, capacity=100) +>>> park.save_base(raw=True) + +# Check that no extra parent objects have been created. +>>> Place.objects.all() +[, ] + +>>> dicts = Restaurant.objects.values('name','serves_hot_dogs') +>>> [sorted(d.items()) for d in dicts] +[[('name', u"Guido's House of Pasta"), ('serves_hot_dogs', True)]] + +>>> dicts = ItalianRestaurant.objects.values('name','serves_hot_dogs','serves_gnocchi') +>>> [sorted(d.items()) for d in dicts] +[[('name', u"Guido's House of Pasta"), ('serves_gnocchi', True), ('serves_hot_dogs', True)]] + +>>> dicts = ParkingLot.objects.values('name','capacity') +>>> [sorted(d.items()) for d in dicts] +[[('capacity', 100), ('name', u'Main St')]] + +# You can also update objects when using a raw save. +>>> place1.name = "Guido's All New House of Pasta" +>>> place1.save_base(raw=True) + +>>> restaurant.serves_hot_dogs = False +>>> restaurant.save_base(raw=True) + +>>> italian_restaurant.serves_gnocchi = False +>>> italian_restaurant.save_base(raw=True) + +>>> place2.name='Derelict lot' +>>> place2.save_base(raw=True) + +>>> park.capacity = 50 +>>> park.save_base(raw=True) + +# No extra parent objects after an update, either. +>>> Place.objects.all() +[, ] + +>>> dicts = Restaurant.objects.values('name','serves_hot_dogs') +>>> [sorted(d.items()) for d in dicts] +[[('name', u"Guido's All New House of Pasta"), ('serves_hot_dogs', False)]] + +>>> dicts = ItalianRestaurant.objects.values('name','serves_hot_dogs','serves_gnocchi') +>>> [sorted(d.items()) for d in dicts] +[[('name', u"Guido's All New House of Pasta"), ('serves_gnocchi', False), ('serves_hot_dogs', False)]] + +>>> dicts = ParkingLot.objects.values('name','capacity') +>>> [sorted(d.items()) for d in dicts] +[[('capacity', 50), ('name', u'Derelict lot')]] + +# If you try to raw_save a parent attribute onto a child object, +# the attribute will be ignored. + +>>> italian_restaurant.name = "Lorenzo's Pasta Hut" +>>> italian_restaurant.save_base(raw=True) + +# Note that the name has not changed +# - name is an attribute of Place, not ItalianRestaurant +>>> dicts = ItalianRestaurant.objects.values('name','serves_hot_dogs','serves_gnocchi') +>>> [sorted(d.items()) for d in dicts] +[[('name', u"Guido's All New House of Pasta"), ('serves_gnocchi', False), ('serves_hot_dogs', False)]] + +"""} diff --git a/tests/regressiontests/serializers_regress/models.py b/tests/regressiontests/serializers_regress/models.py index 593e61ecc7..7d3f9d3b1d 100644 --- a/tests/regressiontests/serializers_regress/models.py +++ b/tests/regressiontests/serializers_regress/models.py @@ -223,3 +223,23 @@ class ModifyingSaveData(models.Model): "A save method that modifies the data in the object" self.data = 666 super(ModifyingSaveData, self).save(raw) + +# Tests for serialization of models using inheritance. +# Regression for #7202, #7350 +class AbstractBaseModel(models.Model): + parent_data = models.IntegerField() + class Meta: + abstract = True + +class InheritAbstractModel(AbstractBaseModel): + child_data = models.IntegerField() + +class BaseModel(models.Model): + parent_data = models.IntegerField() + +class InheritBaseModel(BaseModel): + child_data = models.IntegerField() + +class ExplicitInheritBaseModel(BaseModel): + parent = models.OneToOneField(BaseModel) + child_data = models.IntegerField() diff --git a/tests/regressiontests/serializers_regress/tests.py b/tests/regressiontests/serializers_regress/tests.py index db34f8cf77..9bc5eec1eb 100644 --- a/tests/regressiontests/serializers_regress/tests.py +++ b/tests/regressiontests/serializers_regress/tests.py @@ -32,7 +32,7 @@ def data_create(pk, klass, data): instance = klass(id=pk) instance.data = data models.Model.save_base(instance, raw=True) - return instance + return [instance] def generic_create(pk, klass, data): instance = klass(id=pk) @@ -40,32 +40,45 @@ def generic_create(pk, klass, data): models.Model.save_base(instance, raw=True) for tag in data[1:]: instance.tags.create(data=tag) - return instance + return [instance] def fk_create(pk, klass, data): instance = klass(id=pk) setattr(instance, 'data_id', data) models.Model.save_base(instance, raw=True) - return instance + return [instance] def m2m_create(pk, klass, data): instance = klass(id=pk) models.Model.save_base(instance, raw=True) instance.data = data - return instance + return [instance] def o2o_create(pk, klass, data): instance = klass() instance.data_id = data models.Model.save_base(instance, raw=True) - return instance + return [instance] def pk_create(pk, klass, data): instance = klass() instance.data = data models.Model.save_base(instance, raw=True) - return instance + return [instance] +def inherited_create(pk, klass, data): + instance = klass(id=pk,**data) + # This isn't a raw save because: + # 1) we're testing inheritance, not field behaviour, so none + # of the field values need to be protected. + # 2) saving the child class and having the parent created + # automatically is easier than manually creating both. + models.Model.save(instance) + created = [instance] + for klass,field in instance._meta.parents.items(): + created.append(klass.objects.get(id=pk)) + return created + # A set of functions that can be used to compare # test data objects of various kinds def data_compare(testcase, pk, klass, data): @@ -94,6 +107,11 @@ def pk_compare(testcase, pk, klass, data): instance = klass.objects.get(data=data) testcase.assertEqual(data, instance.data) +def inherited_compare(testcase, pk, klass, data): + instance = klass.objects.get(id=pk) + for key,value in data.items(): + testcase.assertEqual(value, getattr(instance,key)) + # Define some data types. Each data type is # actually a pair of functions; one to create # and one to compare objects of that type @@ -103,6 +121,7 @@ fk_obj = (fk_create, fk_compare) m2m_obj = (m2m_create, m2m_compare) o2o_obj = (o2o_create, o2o_compare) pk_obj = (pk_create, pk_compare) +inherited_obj = (inherited_create, inherited_compare) test_data = [ # Format: (data type, PK value, Model Class, data) @@ -255,6 +274,10 @@ The end."""), (data_obj, 800, AutoNowDateTimeData, datetime.datetime(2006,6,16,10,42,37)), (data_obj, 810, ModifyingSaveData, 42), + + (inherited_obj, 900, InheritAbstractModel, {'child_data':37,'parent_data':42}), + (inherited_obj, 910, ExplicitInheritBaseModel, {'child_data':37,'parent_data':42}), + (inherited_obj, 920, InheritBaseModel, {'child_data':37,'parent_data':42}), ] # Because Oracle treats the empty string as NULL, Oracle is expected to fail @@ -277,13 +300,19 @@ def serializerTest(format, self): # Create all the objects defined in the test data objects = [] + instance_count = {} transaction.enter_transaction_management() transaction.managed(True) for (func, pk, klass, datum) in test_data: - objects.append(func[0](pk, klass, datum)) + objects.extend(func[0](pk, klass, datum)) + instance_count[klass] = 0 transaction.commit() transaction.leave_transaction_management() + # Get a count of the number of objects created for each class + for klass in instance_count: + instance_count[klass] = klass.objects.count() + # Add the generic tagged objects to the object list objects.extend(Tag.objects.all()) @@ -304,6 +333,11 @@ def serializerTest(format, self): for (func, pk, klass, datum) in test_data: func[1](self, pk, klass, datum) + # Assert that the number of objects deserialized is the + # same as the number that was serialized. + for klass, count in instance_count.items(): + self.assertEquals(count, klass.objects.count()) + def fieldsTest(format, self): # Clear the database first management.call_command('flush', verbosity=0, interactive=False)