Fixed #17673 -- Forbid field shadowing.

Thanks Anssi Kääriäinen for the suggestion.

django/db/models/base.py

@@ -1055,8 +1055,12 @@ class Model(six.with_metaclass(ModelBase)):
         if not cls._meta.swapped:
             errors.extend(cls._check_fields(**kwargs))
             errors.extend(cls._check_m2m_through_same_relationship())
-            errors.extend(cls._check_id_field())
+            clash_errors = cls._check_id_field() + cls._check_field_name_clashes()
-            errors.extend(cls._check_column_name_clashes())
+            errors.extend(clash_errors)
+            # If there are field name clashes, hide consequent column name
+            # clashes.
+            if not clash_errors:
+                errors.extend(cls._check_column_name_clashes())
             errors.extend(cls._check_index_together())
             errors.extend(cls._check_unique_together())
             errors.extend(cls._check_ordering())
@@ -1175,6 +1179,61 @@ class Model(six.with_metaclass(ModelBase)):
         else:
             return []
 
+    @classmethod
+    def _check_field_name_clashes(cls):
+        """ Ref #17673. """
+
+        errors = []
+        used_fields = {}  # name or attname -> field
+
+        # Check that multi-inheritance doesn't cause field name shadowing.
+        for parent in cls._meta.parents:
+            for f in parent._meta.local_fields:
+                clash = used_fields.get(f.name) or used_fields.get(f.attname) or None
+                if clash:
+                    errors.append(
+                        checks.Error(
+                            ('The field "%s" from parent model '
+                             '%s clashes with the field "%s" '
+                             'from parent model %s.') % (
+                                clash.name, clash.model._meta,
+                                f.name, f.model._meta
+                            ),
+                            hint=None,
+                            obj=cls,
+                            id='E053',
+                        )
+                    )
+                used_fields[f.name] = f
+                used_fields[f.attname] = f
+
+        # Check that fields defined in the model don't clash with fields from
+        # parents.
+        for f in cls._meta.local_fields:
+            clash = used_fields.get(f.name) or used_fields.get(f.attname) or None
+            # Note that we may detect clash between user-defined non-unique
+            # field "id" and automatically added unique field "id", both
+            # defined at the same model. This special case is considered in
+            # _check_id_field and here we ignore it.
+            id_conflict = (f.name == "id" and
+                clash and clash.name == "id" and clash.model == cls)
+            if clash and not id_conflict:
+                errors.append(
+                    checks.Error(
+                        ('The field clashes with the field "%s" '
+                         'from model %s.') % (
+                            clash.name, clash.model._meta
+                        ),
+                        hint=None,
+                        obj=f,
+                        id='E054',
+                    )
+                )
+            used_fields[f.name] = f
+            used_fields[f.attname] = f
+
+        return errors
+
     @classmethod
     def _check_column_name_clashes(cls):
         # Store a list of column names which have already been used by other fields.

django/db/models/fields/__init__.py

@@ -191,8 +191,9 @@ class Field(RegisterLookupMixin):
         return errors
 
     def _check_field_name(self):
-        """ Check if field name is valid (i. e. not ending with an underscore).
+        """ Check if field name is valid, i.e. 1) does not end with an
-        """
+        underscore, 2) does not contain "__" and 3) is not "pk". """
+
         if self.name.endswith('_'):
             return [
                 checks.Error(
@@ -202,6 +203,24 @@ class Field(RegisterLookupMixin):
                     id='E001',
                 )
             ]
+        elif '__' in self.name:
+            return [
+                checks.Error(
+                    'Field names must not contain "__".',
+                    hint=None,
+                    obj=self,
+                    id='E052',
+                )
+            ]
+        elif self.name == 'pk':
+            return [
+                checks.Error(
+                    'Cannot use "pk" as a field name since it is a reserved name.',
+                    hint=None,
+                    obj=self,
+                    id='E051',
+                )
+            ]
         else:
             return []
 

docs/releases/1.7.txt

@@ -1068,6 +1068,12 @@ Miscellaneous
   which does allow primary keys with value 0. It only forbids *autoincrement*
   primary keys with value 0.
 
+* Shadowing model fields defined in a parent model has been forbidden as this
+  creates amiguity in the expected model behavior. In addition, any clashing
+  fields in the model inheritance hierarchy results in a system check error.
+  For example, if you use multi-inheritance, you need to define custom primary
+  key fields on parent models, otherwise the default ``id`` fields will clash.
+
 .. _deprecated-features-1.7:
 
 Features deprecated in 1.7

tests/invalid_models_tests/test_models.py

@@ -191,31 +191,9 @@ class UniqueTogetherTests(IsolatedModelsTestCase):
         self.assertEqual(errors, expected)
 
 
-class OtherModelTests(IsolatedModelsTestCase):
+class FieldNamesTests(IsolatedModelsTestCase):
 
-    def test_unique_primary_key(self):
+    def test_ending_with_underscore(self):
-        class Model(models.Model):
-            id = models.IntegerField(primary_key=False)
-
-        errors = Model.check()
-        expected = [
-            Error(
-                ('You cannot use "id" as a field name, because each model '
-                 'automatically gets an "id" field if none of the fields '
-                 'have primary_key=True.'),
-                hint='Remove or rename "id" field or add primary_key=True to a field.',
-                obj=Model,
-                id='E005',
-            ),
-            Error(
-                'Field "id" has column name "id" that is already used.',
-                hint=None,
-                obj=Model,
-            )
-        ]
-        self.assertEqual(errors, expected)
-
-    def test_field_names_ending_with_underscore(self):
         class Model(models.Model):
             field_ = models.CharField(max_length=10)
             m2m_ = models.ManyToManyField('self')
@@ -237,6 +215,138 @@ class OtherModelTests(IsolatedModelsTestCase):
         ]
         self.assertEqual(errors, expected)
 
+    def test_including_separator(self):
+        class Model(models.Model):
+            some__field = models.IntegerField()
+
+        errors = Model.check()
+        expected = [
+            Error(
+                'Field names must not contain "__".',
+                hint=None,
+                obj=Model._meta.get_field('some__field'),
+                id='E052',
+            )
+        ]
+        self.assertEqual(errors, expected)
+
+    def test_pk(self):
+        class Model(models.Model):
+            pk = models.IntegerField()
+
+        errors = Model.check()
+        expected = [
+            Error(
+                'Cannot use "pk" as a field name since it is a reserved name.',
+                hint=None,
+                obj=Model._meta.get_field('pk'),
+                id='E051',
+            )
+        ]
+        self.assertEqual(errors, expected)
+
+
+class ShadowingFieldsTests(IsolatedModelsTestCase):
+
+    def test_multiinheritance_clash(self):
+        class Mother(models.Model):
+            clash = models.IntegerField()
+
+        class Father(models.Model):
+            clash = models.IntegerField()
+
+        class Child(Mother, Father):
+            # Here we have two clashed: id (automatic field) and clash, because
+            # both parents define these fields.
+            pass
+
+        errors = Child.check()
+        expected = [
+            Error(
+                ('The field "id" from parent model '
+                 'invalid_models_tests.mother clashes with the field "id" '
+                 'from parent model invalid_models_tests.father.'),
+                hint=None,
+                obj=Child,
+                id='E053',
+            ),
+            Error(
+                ('The field "clash" from parent model '
+                 'invalid_models_tests.mother clashes with the field "clash" '
+                 'from parent model invalid_models_tests.father.'),
+                hint=None,
+                obj=Child,
+                id='E053',
+            )
+        ]
+        self.assertEqual(errors, expected)
+
+    def test_inheritance_clash(self):
+        class Parent(models.Model):
+            f_id = models.IntegerField()
+
+        class Target(models.Model):
+            # This field doesn't result in a clash.
+            f_id = models.IntegerField()
+
+        class Child(Parent):
+            # This field clashes with parent "f_id" field.
+            f = models.ForeignKey(Target)
+
+        errors = Child.check()
+        expected = [
+            Error(
+                ('The field clashes with the field "f_id" '
+                 'from model invalid_models_tests.parent.'),
+                hint=None,
+                obj=Child._meta.get_field('f'),
+                id='E054',
+            )
+        ]
+        self.assertEqual(errors, expected)
+
+    def test_id_clash(self):
+        class Target(models.Model):
+            pass
+
+        class Model(models.Model):
+            fk = models.ForeignKey(Target)
+            fk_id = models.IntegerField()
+
+        errors = Model.check()
+        expected = [
+            Error(
+                ('The field clashes with the field "fk" from model '
+                 'invalid_models_tests.model.'),
+                hint=None,
+                obj=Model._meta.get_field('fk_id'),
+                id='E054',
+            )
+        ]
+        self.assertEqual(errors, expected)
+
+
+class OtherModelTests(IsolatedModelsTestCase):
+
+    def test_unique_primary_key(self):
+        invalid_id = models.IntegerField(primary_key=False)
+
+        class Model(models.Model):
+            id = invalid_id
+
+        errors = Model.check()
+        expected = [
+            Error(
+                ('You cannot use "id" as a field name, because each model '
+                 'automatically gets an "id" field if none of the fields '
+                 'have primary_key=True.'),
+                hint='Remove or rename "id" field or add primary_key=True to a field.',
+                obj=Model,
+                id='E005',
+            ),
+        ]
+        self.assertEqual(errors, expected)
+
     def test_ordering_non_iterable(self):
         class Model(models.Model):
             class Meta:

tests/model_inheritance/models.py

@@ -45,10 +45,6 @@ class Student(CommonInfo):
         pass
 
 
-class StudentWorker(Student, Worker):
-    pass
-
-
 #
 # Abstract base classes with related models
 #

tests/model_inheritance/tests.py

@@ -10,7 +10,7 @@ from django.utils import six
 
 from .models import (
     Chef, CommonInfo, ItalianRestaurant, ParkingLot, Place, Post,
-    Restaurant, Student, StudentWorker, Supplier, Worker, MixinModel,
+    Restaurant, Student, Supplier, Worker, MixinModel,
     Title, Base, SubBase)
 
 
@@ -48,38 +48,6 @@ class ModelInheritanceTests(TestCase):
         # doesn't exist as a model).
         self.assertRaises(AttributeError, lambda: CommonInfo.objects.all())
 
-        # A StudentWorker which does not exist is both a Student and Worker
-        # which does not exist.
-        self.assertRaises(
-            Student.DoesNotExist,
-            StudentWorker.objects.get, pk=12321321
-        )
-        self.assertRaises(
-            Worker.DoesNotExist,
-            StudentWorker.objects.get, pk=12321321
-        )
-
-        # MultipleObjectsReturned is also inherited.
-        # This is written out "long form", rather than using __init__/create()
-        # because of a bug with diamond inheritance (#10808)
-        sw1 = StudentWorker()
-        sw1.name = "Wilma"
-        sw1.age = 35
-        sw1.save()
-        sw2 = StudentWorker()
-        sw2.name = "Betty"
-        sw2.age = 24
-        sw2.save()
-
-        self.assertRaises(
-            Student.MultipleObjectsReturned,
-            StudentWorker.objects.get, pk__lt=sw2.pk + 100
-        )
-        self.assertRaises(
-            Worker.MultipleObjectsReturned,
-            StudentWorker.objects.get, pk__lt=sw2.pk + 100
-        )
-
     def test_multiple_table(self):
         post = Post.objects.create(title="Lorem Ipsum")
         # The Post model has distinct accessors for the Comment and Link models.