diff --git a/AUTHORS b/AUTHORS index 82d83ae4e1..026406d985 100644 --- a/AUTHORS +++ b/AUTHORS @@ -69,6 +69,7 @@ answer newbie questions, and generally made Django that much better: Aram Dulyan arien Armin Ronacher + Aron Podrigal Artem Gnilov Arthur Arthur Koziel diff --git a/django/db/models/base.py b/django/db/models/base.py index 2a6fa081a4..19dca3926d 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -212,26 +212,35 @@ class ModelBase(type): if isinstance(field, OneToOneField): related = resolve_relation(new_class, field.remote_field.model) parent_links[make_model_tuple(related)] = field + + # Track fields inherited from base models. + inherited_attributes = set() # Do the appropriate setup for any model parents. - for base in parents: + for base in new_class.mro(): original_base = base - if not hasattr(base, '_meta'): + if base not in parents or not hasattr(base, '_meta'): # Things without _meta aren't functional models, so they're # uninteresting parents. + inherited_attributes |= set(base.__dict__.keys()) continue parent_fields = base._meta.local_fields + base._meta.local_many_to_many - # Check for clashes between locally declared fields and those - # on the base classes (we cannot handle shadowed fields at the - # moment). - for field in parent_fields: - if field.name in field_names: - raise FieldError( - 'Local field %r in class %r clashes ' - 'with field of similar name from ' - 'base class %r' % (field.name, name, base.__name__) - ) if not base._meta.abstract: + # Check for clashes between locally declared fields and those + # on the base classes. + for field in parent_fields: + if field.name in field_names: + raise FieldError( + 'Local field %r in class %r clashes with field of ' + 'the same name from base class %r.' % ( + field.name, + name, + base.__name__, + ) + ) + else: + inherited_attributes.add(field.name) + # Concrete classes... base = base._meta.concrete_model base_key = make_model_tuple(base) @@ -246,6 +255,18 @@ class ModelBase(type): auto_created=True, parent_link=True, ) + + if attr_name in field_names: + raise FieldError( + "Auto-generated field '%s' in class %r for " + "parent_link to base class %r clashes with " + "declared field of the same name." % ( + attr_name, + name, + base.__name__, + ) + ) + # Only add the ptr field if it's not already present; # e.g. migrations will already have it specified if not hasattr(new_class, attr_name): @@ -256,16 +277,19 @@ class ModelBase(type): else: base_parents = base._meta.parents.copy() - # .. and abstract ones. + # Add fields from abstract base class if it wasn't overridden. for field in parent_fields: - new_field = copy.deepcopy(field) - new_class.add_to_class(field.name, new_field) - # Replace parent links defined on this base by the new - # field as it will be appropriately resolved if required. - if field.one_to_one: - for parent, parent_link in base_parents.items(): - if field == parent_link: - base_parents[parent] = new_field + if (field.name not in field_names and + field.name not in new_class.__dict__ and + field.name not in inherited_attributes): + new_field = copy.deepcopy(field) + new_class.add_to_class(field.name, new_field) + # Replace parent links defined on this base by the new + # field. It will be appropriately resolved if required. + if field.one_to_one: + for parent, parent_link in base_parents.items(): + if field == parent_link: + base_parents[parent] = new_field # Pass any non-abstract parent classes onto child. new_class._meta.parents.update(base_parents) @@ -281,13 +305,18 @@ class ModelBase(type): # Inherit private fields (like GenericForeignKey) from the parent # class for field in base._meta.private_fields: - if base._meta.abstract and field.name in field_names: - raise FieldError( - 'Local field %r in class %r clashes ' - 'with field of similar name from ' - 'abstract base class %r' % (field.name, name, base.__name__) - ) - new_class.add_to_class(field.name, copy.deepcopy(field)) + if field.name in field_names: + if not base._meta.abstract: + raise FieldError( + 'Local field %r in class %r clashes with field of ' + 'the same name from base class %r.' % ( + field.name, + name, + base.__name__, + ) + ) + else: + new_class.add_to_class(field.name, copy.deepcopy(field)) if abstract: # Abstract base models can't be instantiated and don't appear in diff --git a/docs/releases/1.10.txt b/docs/releases/1.10.txt index 9e24225173..35cf1d42c3 100644 --- a/docs/releases/1.10.txt +++ b/docs/releases/1.10.txt @@ -400,6 +400,8 @@ Models app label and class interpolation using the ``'%(app_label)s'`` and ``'%(class)s'`` strings. +* Allowed overriding model fields inherited from abstract base classes. + * The :func:`~django.db.models.prefetch_related_objects` function is now a public API. diff --git a/docs/topics/db/models.txt b/docs/topics/db/models.txt index 2850294581..1dc1b17202 100644 --- a/docs/topics/db/models.txt +++ b/docs/topics/db/models.txt @@ -1376,11 +1376,35 @@ Field name "hiding" is not permitted ------------------------------------- In normal Python class inheritance, it is permissible for a child class to -override any attribute from the parent class. In Django, this is not permitted -for attributes that are :class:`~django.db.models.Field` instances (at -least, not at the moment). If a base class has a field called ``author``, you -cannot create another model field called ``author`` in any class that inherits -from that base class. +override any attribute from the parent class. In Django, this isn't usually +permitted for model fields. If a non-abstract model base class has a field +called ``author``, you can't create another model field or define +an attribute called ``author`` in any class that inherits from that base class. + +This restriction doesn't apply to model fields inherited from an abstract +model. Such fields may be overridden with another field or value, or be removed +by setting ``field_name = None``. + +.. versionchanged:: 1.10 + + The ability to override abstract fields was added. + +.. warning:: + + Model managers are inherited from abstract base classes. Overriding an + inherited field which is referenced by an inherited + :class:`~django.db.models.Manager` may cause subtle bugs. See :ref:`custom + managers and model inheritance `. + +.. note:: + + Some fields define extra attributes on the model, e.g. a + :class:`~django.db.models.ForeignKey` defines an extra attribute with + ``_id`` appended to the field name, as well as ``related_name`` and + ``related_query_name`` on the foreign model. + + These extra attributes cannot be overridden unless the field that defines + it is changed or removed so that it no longer defines the extra attribute. Overriding fields in a parent model leads to difficulties in areas such as initializing new instances (specifying which field is being initialized in diff --git a/tests/model_inheritance/test_abstract_inheritance.py b/tests/model_inheritance/test_abstract_inheritance.py new file mode 100644 index 0000000000..71b8513a57 --- /dev/null +++ b/tests/model_inheritance/test_abstract_inheritance.py @@ -0,0 +1,354 @@ +from __future__ import unicode_literals + +from django.contrib.contenttypes.fields import ( + GenericForeignKey, GenericRelation, +) +from django.contrib.contenttypes.models import ContentType +from django.core.checks import Error +from django.core.exceptions import FieldDoesNotExist, FieldError +from django.db import models +from django.test import TestCase +from django.test.utils import isolate_apps + + +@isolate_apps('model_inheritance') +class AbstractInheritanceTests(TestCase): + def test_single_parent(self): + class AbstractBase(models.Model): + name = models.CharField(max_length=30) + + class Meta: + abstract = True + + class AbstractDescendant(AbstractBase): + name = models.CharField(max_length=50) + + class Meta: + abstract = True + + class DerivedChild(AbstractBase): + name = models.CharField(max_length=50) + + class DerivedGrandChild(AbstractDescendant): + pass + + self.assertEqual(AbstractDescendant._meta.get_field('name').max_length, 50) + self.assertEqual(DerivedChild._meta.get_field('name').max_length, 50) + self.assertEqual(DerivedGrandChild._meta.get_field('name').max_length, 50) + + def test_multiple_parents_mro(self): + class AbstractBaseOne(models.Model): + class Meta: + abstract = True + + class AbstractBaseTwo(models.Model): + name = models.CharField(max_length=30) + + class Meta: + abstract = True + + class DescendantOne(AbstractBaseOne, AbstractBaseTwo): + class Meta: + abstract = True + + class DescendantTwo(AbstractBaseOne, AbstractBaseTwo): + name = models.CharField(max_length=50) + + class Meta: + abstract = True + + class Derived(DescendantOne, DescendantTwo): + pass + + self.assertEqual(DescendantOne._meta.get_field('name').max_length, 30) + self.assertEqual(DescendantTwo._meta.get_field('name').max_length, 50) + self.assertEqual(Derived._meta.get_field('name').max_length, 50) + + def test_multiple_inheritance_cannot_shadow_concrete_inherited_field(self): + class ConcreteParent(models.Model): + name = models.CharField(max_length=255) + + class AbstractParent(models.Model): + name = models.IntegerField() + + class Meta: + abstract = True + + class FirstChild(ConcreteParent, AbstractParent): + pass + + class AnotherChild(AbstractParent, ConcreteParent): + pass + + self.assertIsInstance(FirstChild._meta.get_field('name'), models.CharField) + self.assertEqual( + AnotherChild.check(), + [Error( + "The field 'name' clashes with the field 'name' " + "from model 'model_inheritance.concreteparent'.", + obj=AnotherChild._meta.get_field('name'), + id="models.E006", + )] + ) + + def test_virtual_field(self): + class RelationModel(models.Model): + content_type = models.ForeignKey(ContentType, models.CASCADE) + object_id = models.PositiveIntegerField() + content_object = GenericForeignKey('content_type', 'object_id') + + class RelatedModelAbstract(models.Model): + field = GenericRelation(RelationModel) + + class Meta: + abstract = True + + class ModelAbstract(models.Model): + field = models.CharField(max_length=100) + + class Meta: + abstract = True + + class OverrideRelatedModelAbstract(RelatedModelAbstract): + field = models.CharField(max_length=100) + + class ExtendModelAbstract(ModelAbstract): + field = GenericRelation(RelationModel) + + self.assertIsInstance(OverrideRelatedModelAbstract._meta.get_field('field'), models.CharField) + self.assertIsInstance(ExtendModelAbstract._meta.get_field('field'), GenericRelation) + + def test_cannot_override_indirect_abstract_field(self): + class AbstractBase(models.Model): + name = models.CharField(max_length=30) + + class Meta: + abstract = True + + class ConcreteDescendant(AbstractBase): + pass + + msg = ( + "Local field 'name' in class 'Descendant' clashes with field of " + "the same name from base class 'ConcreteDescendant'." + ) + with self.assertRaisesMessage(FieldError, msg): + class Descendant(ConcreteDescendant): + name = models.IntegerField() + + def test_override_field_with_attr(self): + class AbstractBase(models.Model): + first_name = models.CharField(max_length=50) + last_name = models.CharField(max_length=50) + middle_name = models.CharField(max_length=30) + full_name = models.CharField(max_length=150) + + class Meta: + abstract = True + + class Descendant(AbstractBase): + middle_name = None + + def full_name(self): + return self.first_name + self.last_name + + with self.assertRaises(FieldDoesNotExist): + Descendant._meta.get_field('middle_name') + + with self.assertRaises(FieldDoesNotExist): + Descendant._meta.get_field('full_name') + + def test_overriding_field_removed_by_concrete_model(self): + class AbstractModel(models.Model): + foo = models.CharField(max_length=30) + + class Meta: + abstract = True + + class RemovedAbstractModelField(AbstractModel): + foo = None + + class OverrideRemovedFieldByConcreteModel(RemovedAbstractModelField): + foo = models.CharField(max_length=50) + + self.assertEqual(OverrideRemovedFieldByConcreteModel._meta.get_field('foo').max_length, 50) + + def test_shadowed_fkey_id(self): + class Foo(models.Model): + pass + + class AbstractBase(models.Model): + foo = models.ForeignKey(Foo, models.CASCADE) + + class Meta: + abstract = True + + class Descendant(AbstractBase): + foo_id = models.IntegerField() + + self.assertEqual( + Descendant.check(), + [Error( + "The field 'foo_id' clashes with the field 'foo' " + "from model 'model_inheritance.descendant'.", + obj=Descendant._meta.get_field('foo_id'), + id='models.E006', + )] + ) + + def test_shadow_related_name_when_set_to_none(self): + class AbstractBase(models.Model): + bar = models.IntegerField() + + class Meta: + abstract = True + + class Foo(AbstractBase): + bar = None + foo = models.IntegerField() + + class Bar(models.Model): + bar = models.ForeignKey(Foo, models.CASCADE, related_name='bar') + + self.assertEqual(Bar.check(), []) + + def test_reverse_foreign_key(self): + class AbstractBase(models.Model): + foo = models.CharField(max_length=100) + + class Meta: + abstract = True + + class Descendant(AbstractBase): + pass + + class Foo(models.Model): + foo = models.ForeignKey(Descendant, models.CASCADE, related_name='foo') + + self.assertEqual( + Foo._meta.get_field('foo').check(), + [ + Error( + "Reverse accessor for 'Foo.foo' clashes with field name 'Descendant.foo'.", + hint=( + "Rename field 'Descendant.foo', or add/change a related_name " + "argument to the definition for field 'Foo.foo'." + ), + obj=Foo._meta.get_field('foo'), + id='fields.E302', + ), + Error( + "Reverse query name for 'Foo.foo' clashes with field name 'Descendant.foo'.", + hint=( + "Rename field 'Descendant.foo', or add/change a related_name " + "argument to the definition for field 'Foo.foo'." + ), + obj=Foo._meta.get_field('foo'), + id='fields.E303', + ), + ] + ) + + def test_multi_inheritance_field_clashes(self): + class AbstractBase(models.Model): + name = models.CharField(max_length=30) + + class Meta: + abstract = True + + class ConcreteBase(AbstractBase): + pass + + class AbstractDescendant(ConcreteBase): + class Meta: + abstract = True + + class ConcreteDescendant(AbstractDescendant): + name = models.CharField(max_length=100) + + self.assertEqual( + ConcreteDescendant.check(), + [Error( + "The field 'name' clashes with the field 'name' from " + "model 'model_inheritance.concretebase'.", + obj=ConcreteDescendant._meta.get_field('name'), + id="models.E006", + )] + ) + + def test_override_one2one_relation_auto_field_clashes(self): + class ConcreteParent(models.Model): + name = models.CharField(max_length=255) + + class AbstractParent(models.Model): + name = models.IntegerField() + + class Meta: + abstract = True + + msg = ( + "Auto-generated field 'concreteparent_ptr' in class 'Descendant' " + "for parent_link to base class 'ConcreteParent' clashes with " + "declared field of the same name." + ) + with self.assertRaisesMessage(FieldError, msg): + class Descendant(ConcreteParent, AbstractParent): + concreteparent_ptr = models.CharField(max_length=30) + + def test_abstract_model_with_regular_python_mixin_mro(self): + class AbstractModel(models.Model): + name = models.CharField(max_length=255) + age = models.IntegerField() + + class Meta: + abstract = True + + class Mixin(object): + age = None + + class Mixin2(object): + age = 2 + + class DescendantMixin(Mixin): + pass + + class ConcreteModel(models.Model): + foo = models.IntegerField() + + class ConcreteModel2(ConcreteModel): + age = models.SmallIntegerField() + + def fields(model): + if not hasattr(model, '_meta'): + return list() + return list((f.name, f.__class__) for f in model._meta.get_fields()) + + model_dict = {'__module__': 'model_inheritance'} + model1 = type(str('Model1'), (AbstractModel, Mixin), model_dict.copy()) + model2 = type(str('Model2'), (Mixin2, AbstractModel), model_dict.copy()) + model3 = type(str('Model3'), (DescendantMixin, AbstractModel), model_dict.copy()) + model4 = type(str('Model4'), (Mixin2, Mixin, AbstractModel), model_dict.copy()) + model5 = type(str('Model5'), (Mixin2, ConcreteModel2, Mixin, AbstractModel), model_dict.copy()) + + self.assertEqual( + fields(model1), + [('id', models.AutoField), ('name', models.CharField), ('age', models.IntegerField)] + ) + + self.assertEqual(fields(model2), [('id', models.AutoField), ('name', models.CharField)]) + self.assertEqual(getattr(model2, 'age'), 2) + + self.assertEqual(fields(model3), [('id', models.AutoField), ('name', models.CharField)]) + + self.assertEqual(fields(model4), [('id', models.AutoField), ('name', models.CharField)]) + self.assertEqual(getattr(model4, 'age'), 2) + + self.assertEqual( + fields(model5), + [ + ('id', models.AutoField), ('foo', models.IntegerField), + ('concretemodel_ptr', models.OneToOneField), + ('age', models.SmallIntegerField), ('concretemodel2_ptr', models.OneToOneField), + ('name', models.CharField), + ] + )