Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Fixed #17673 -- forbidding field shadowing #556

Closed
wants to merge 1 commit into from

2 participants

chrismedrela Tim Graham
chrismedrela

This is updated version of a patch written 7 months ago; it applies
clearly.

Fixed #17673 -- forbidding field shadowing
This is updated version of a patch written 7 months ago; it applies
clearly.
d9d4998
Tim Graham
Owner

Patch has gone stale again. Please re-open or open a new request when updated.

Tim Graham timgraham closed this May 31, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Nov 25, 2012
Fixed #17673 -- forbidding field shadowing
This is updated version of a patch written 7 months ago; it applies
clearly.
d9d4998
This page is out of date. Refresh to see the latest.
42  django/core/management/validation.py
@@ -3,6 +3,7 @@
3 3
 
4 4
 from django.conf import settings
5 5
 from django.core.management.color import color_style
  6
+from django.db.models.constants import LOOKUP_SEP
6 7
 from django.utils.encoding import force_str
7 8
 from django.utils.itercompat import is_iterable
8 9
 from django.utils import six
@@ -37,6 +38,27 @@ def get_validation_errors(outfile, app=None):
37 38
 
38 39
     for cls in models.get_models(app, include_swapped=True):
39 40
         opts = cls._meta
  41
+        # Keep record of used field names. Clashing names inside a single model
  42
+        # (example: `book` and `book_id`) and shadowing field names in inheritance
  43
+        # cases is not allowed as this will lead to buggy behavior. See ticket
  44
+        # #17673 for details.
  45
+        used_fields = {} # name or attname -> field
  46
+
  47
+        # Check that multi-inheritance doesn't cause field name
  48
+        # shadowing.
  49
+        for parent in opts.parents:
  50
+            for f in parent._meta.local_fields:
  51
+                clashing_field = (used_fields.get(f.name) or
  52
+                                  used_fields.get(f.attname) or
  53
+                                  None)
  54
+                if clashing_field:
  55
+                    e.add(opts, 'The field "%s" from parent model "%s" '
  56
+                        'clashes with the field "%s" '
  57
+                        'from another parent model "%s"' % (
  58
+                            f.name, f.model._meta, clashing_field.name,
  59
+                            clashing_field.model._meta))
  60
+                used_fields[f.name] = f
  61
+                used_fields[f.attname] = f
40 62
 
41 63
         # Check swappable attribute.
42 64
         if opts.swapped:
@@ -50,7 +72,8 @@ def get_validation_errors(outfile, app=None):
50 72
             # No need to perform any other validation checks on a swapped model.
51 73
             continue
52 74
 
53  
-        # This is the current User model. Check known validation problems with User models
  75
+        # This is the current User model. Check known validation problems with
  76
+        # User models
54 77
         if settings.AUTH_USER_MODEL == '%s.%s' % (opts.app_label, opts.object_name):
55 78
             # Check that the USERNAME FIELD isn't included in REQUIRED_FIELDS.
56 79
             if cls.USERNAME_FIELD in cls.REQUIRED_FIELDS:
@@ -58,6 +81,23 @@ def get_validation_errors(outfile, app=None):
58 81
 
59 82
         # Model isn't swapped; do field-specific validation.
60 83
         for f in opts.local_fields:
  84
+            # check clashing
  85
+            clashing_field = (used_fields.get(f.name) or
  86
+                              used_fields.get(f.attname) or
  87
+                              None)
  88
+            if clashing_field:
  89
+                e.add(opts, '"%s": This field clashes '
  90
+                    'with field "%s" from "%s"' % (
  91
+                    f.name, clashing_field.name, clashing_field.model._meta))
  92
+            used_fields[f.name] = f
  93
+            used_fields[f.attname] = f
  94
+            if f.name == 'pk':
  95
+                e.add(opts, '"%s": You can\'t use "pk" as a field name. '
  96
+                            'It is a reserved name.' % f.name)
  97
+            if LOOKUP_SEP in f.name:
  98
+                e.add(opts, '"%s": Field\'s name must not contain "%s".' % (
  99
+                                f.name, LOOKUP_SEP))
  100
+
61 101
             if f.name == 'id' and not f.primary_key and opts.pk.name == 'id':
62 102
                 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)
63 103
             if f.name.endswith('_'):
35  tests/modeltests/invalid_models/invalid_models/models.py
@@ -363,6 +363,35 @@ class Meta:
363 363
         ]
364 364
 
365 365
 
  366
+class InvalidFieldNames(models.Model):
  367
+    pk = models.IntegerField()
  368
+    some__field = models.IntegerField()
  369
+
  370
+
  371
+class FirstParent(models.Model):
  372
+    somef_id = models.IntegerField()
  373
+    someotherf = models.IntegerField()
  374
+
  375
+
  376
+class SecondParent(models.Model):
  377
+    somef_id = models.IntegerField()
  378
+
  379
+
  380
+class ChildShadowingField(FirstParent):
  381
+    somef = models.ForeignKey(SecondParent)
  382
+
  383
+
  384
+class MultiInheritanceClash(FirstParent, SecondParent):
  385
+    # Here we have two clashed: id (automatic field) and somef, because
  386
+    # both parents define these fields.
  387
+    pass
  388
+
  389
+
  390
+class InternalClashingNames(models.Model):
  391
+    # fk.attname must not clash with fk_id.name
  392
+    fk = models.ForeignKey(FirstParent)
  393
+    fk_id = models.IntegerField()
  394
+
366 395
 model_errors = """invalid_models.fielderrors: "charfield": CharFields require a "max_length" attribute that is a positive integer.
367 396
 invalid_models.fielderrors: "charfield2": CharFields require a "max_length" attribute that is a positive integer.
368 397
 invalid_models.fielderrors: "charfield3": CharFields require a "max_length" attribute that is a positive integer.
@@ -478,6 +507,12 @@ class Meta:
478 507
 invalid_models.badswappablevalue: TEST_SWAPPED_MODEL_BAD_VALUE is not of the form 'app_label.app_name'.
479 508
 invalid_models.badswappablemodel: Model has been swapped out for 'not_an_app.Target' which has not been installed or is abstract.
480 509
 invalid_models.badindextogether1: "index_together" refers to field_that_does_not_exist, a field that doesn't exist.
  510
+invalid_models.invalidfieldnames: "pk": You can't use "pk" as a field name. It is a reserved name.
  511
+invalid_models.invalidfieldnames: "some__field": Field's name must not contain "__".
  512
+invalid_models.childshadowingfield: "somef": This field clashes with field "somef_id" from "invalid_models.firstparent"
  513
+invalid_models.multiinheritanceclash: The field "id" from parent model "invalid_models.secondparent" clashes with the field "id" from another parent model "invalid_models.firstparent"
  514
+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"
  515
+invalid_models.internalclashingnames: "fk_id": This field clashes with field "fk" from "invalid_models.internalclashingnames"
481 516
 """
482 517
 
483 518
 if not connection.features.interprets_empty_strings_as_nulls:
3  tests/modeltests/model_inheritance/models.py
@@ -41,9 +41,6 @@ class Student(CommonInfo):
41 41
     class Meta:
42 42
         pass
43 43
 
44  
-class StudentWorker(Student, Worker):
45  
-    pass
46  
-
47 44
 #
48 45
 # Abstract base classes with related models
49 46
 #
30  tests/modeltests/model_inheritance/tests.py
@@ -7,7 +7,7 @@
7 7
 from django.utils import six
8 8
 
9 9
 from .models import (Chef, CommonInfo, ItalianRestaurant, ParkingLot, Place,
10  
-    Post, Restaurant, Student, StudentWorker, Supplier, Worker, MixinModel)
  10
+    Post, Restaurant, Student, Supplier, Worker, MixinModel)
11 11
 
12 12
 
13 13
 class ModelInheritanceTests(TestCase):
@@ -44,34 +44,6 @@ def test_abstract(self):
44 44
         # doesn't exist as a model).
45 45
         self.assertRaises(AttributeError, lambda: CommonInfo.objects.all())
46 46
 
47  
-        # A StudentWorker which does not exist is both a Student and Worker
48  
-        # which does not exist.
49  
-        self.assertRaises(Student.DoesNotExist,
50  
-            StudentWorker.objects.get, pk=12321321
51  
-        )
52  
-        self.assertRaises(Worker.DoesNotExist,
53  
-            StudentWorker.objects.get, pk=12321321
54  
-        )
55  
-
56  
-        # MultipleObjectsReturned is also inherited.
57  
-        # This is written out "long form", rather than using __init__/create()
58  
-        # because of a bug with diamond inheritance (#10808)
59  
-        sw1 = StudentWorker()
60  
-        sw1.name = "Wilma"
61  
-        sw1.age = 35
62  
-        sw1.save()
63  
-        sw2 = StudentWorker()
64  
-        sw2.name = "Betty"
65  
-        sw2.age = 24
66  
-        sw2.save()
67  
-
68  
-        self.assertRaises(Student.MultipleObjectsReturned,
69  
-            StudentWorker.objects.get, pk__lt=sw2.pk + 100
70  
-        )
71  
-        self.assertRaises(Worker.MultipleObjectsReturned,
72  
-            StudentWorker.objects.get, pk__lt=sw2.pk + 100
73  
-        )
74  
-
75 47
     def test_multiple_table(self):
76 48
         post = Post.objects.create(title="Lorem Ipsum")
77 49
         # The Post model has distinct accessors for the Comment and Link models.
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.