Ticket #17673: 17673_v2.diff

django/core/management/validation.py

@@ -1 +1 @@
 import sys
 
 from django.core.management.color import color_style
+from django.db.models.sql.constants import LOOKUP_SEP
 from django.utils.itercompat import is_iterable
 
+
 class ModelErrorCollection:
     def __init__(self, outfile=sys.stdout):
         self.errors = []
@@ -32 +34 @@
 
     for cls in models.get_models(app):
         opts = cls._meta
+        # Keep record of used field names. Clashing names inside a single model
+        # (example: `book` and `book_id`) and shadowing field names in inheritance
+        # cases is not allowed as this will lead to buggy behavior. See ticket
+        # #17673 for details.
+        used_fields = {} # name or attname -> field
+
+        # Check that multi-inheritance doesn't cause field name
+        # shadowing.
+        for parent in opts.parents:
+            for f in parent._meta.local_fields:
+                clashing_field = (used_fields.get(f.name) or
+                                  used_fields.get(f.attname) or
+                                  None)
+                if clashing_field:
+                    e.add(opts, 'The field "%s" from parent model "%s" clashes with the field '
+                                '"%s" from another parent model "%s"' % (
+                                    f.name, f.model._meta, clashing_field.name,
+                                    clashing_field.model._meta))
+                used_fields[f.name] = f
+                used_fields[f.attname] = f
 
         # Do field-specific validation.
         for f in opts.local_fields:
+            clashing_field = (used_fields.get(f.name) or
+                              used_fields.get(f.attname) or
+                              None)
+            if clashing_field:
+                e.add(opts, '"%s": This field clashes with field "%s" from "%s"' % (
+                                f.name, clashing_field.name, clashing_field.model._meta))
+            used_fields[f.name] = f
+            used_fields[f.attname] = f
+            if f.name == 'pk':
+                e.add(opts, '"%s": You can\'t use "pk" as a field name. '
+                            'It is a reserved name.' % f.name)
+            if LOOKUP_SEP in f.name:
+                e.add(opts, '"%s": Field\'s name must not contain "%s".' % (
+                                f.name, LOOKUP_SEP))
             if f.name == 'id' and not f.primary_key and opts.pk.name == 'id':
                 e.add(opts, '"%s": You can\'t use "id" as a field name, because each model automatically gets an "id" field if none of the fields have primary_key=True. You need to either remove/rename your "id" field or add primary_key=True to a field.' % f.name)
             if f.name.endswith('_'):

tests/modeltests/invalid_models/invalid_models/models.py

@@ -243 +243 @@
     class Meta:
         ordering = ('pk',)
 
+class InvalidFieldNames(models.Model):
+    pk = models.IntegerField()
+    some__field = models.IntegerField()
+
+class FirstParent(models.Model):
+    somef_id = models.IntegerField()
+    someotherf = models.IntegerField()
+
+class SecondParent(models.Model):
+    somef_id = models.IntegerField()
+
+class ChildShadowingField(FirstParent):
+    somef = models.ForeignKey(SecondParent)
+
+class MultiInheritanceClash(FirstParent, SecondParent):
+    # Here we have two clashed: id (automatic field) and somef, because
+    # both parents define these fields.
+    pass
+
+class InternalClashingNames(models.Model):
+    # fk.attname must not clash with fk_id.name
+    fk = models.ForeignKey(FirstParent)
+    fk_id = models.IntegerField()
+
 model_errors = """invalid_models.fielderrors: "charfield": CharFields require a "max_length" attribute that is a positive integer.
 invalid_models.fielderrors: "charfield2": CharFields require a "max_length" attribute that is a positive integer.
 invalid_models.fielderrors: "charfield3": CharFields require a "max_length" attribute that is a positive integer.
@@ -351 +375 @@
 invalid_models.nonexistingorderingwithsingleunderscore: "ordering" refers to "does_not_exist", a field that doesn't exist.
 invalid_models.invalidsetnull: 'fk' specifies on_delete=SET_NULL, but cannot be null.
 invalid_models.invalidsetdefault: 'fk' specifies on_delete=SET_DEFAULT, but has no default value.
+invalid_models.invalidfieldnames: "pk": You can't use "pk" as a field name. It is a reserved name.
+invalid_models.invalidfieldnames: "some__field": Field's name must not contain "__".
+invalid_models.childshadowingfield: "somef": This field clashes with field "somef_id" from "invalid_models.firstparent"
+invalid_models.multiinheritanceclash: The field "id" from parent model "invalid_models.secondparent" clashes with the field "id" from another parent model "invalid_models.firstparent"
+invalid_models.multiinheritanceclash: The field "somef_id" from parent model "invalid_models.secondparent" clashes with the field "somef_id" from another parent model "invalid_models.firstparent"
+invalid_models.internalclashingnames: "fk_id": This field clashes with field "fk" from "invalid_models.internalclashingnames"
 """
 
 if not connection.features.interprets_empty_strings_as_nulls:

tests/modeltests/model_inheritance/models.py

@@ -38 +38 @@
     class Meta:
         pass
 
-class StudentWorker(Student, Worker):
-    pass
-
 #
 # Abstract base classes with related models
 #

tests/modeltests/model_inheritance/tests.py

@@ -6 +6 @@
 from django.test import TestCase
 
 from .models import (Chef, CommonInfo, ItalianRestaurant, ParkingLot, Place,
-    Post, Restaurant, Student, StudentWorker, Supplier, Worker, MixinModel)
+    Post, Restaurant, Student, Supplier, Worker, MixinModel)
 
 
 class ModelInheritanceTests(TestCase):
@@ -43 +43 @@
         # 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.