From ee9fcb1672ddf5910ed8c45c37a00f32ebbe2bb1 Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Fri, 7 Feb 2014 22:34:56 +0100 Subject: [PATCH] Fixed #17673 -- Forbid field shadowing. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks Anssi Kääriäinen for the suggestion. --- django/db/models/base.py | 63 ++++++++- django/db/models/fields/__init__.py | 23 +++- docs/releases/1.7.txt | 6 + tests/invalid_models_tests/test_models.py | 158 ++++++++++++++++++---- tests/model_inheritance/models.py | 4 - tests/model_inheritance/tests.py | 34 +---- 6 files changed, 223 insertions(+), 65 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index 4b8aededa9..6270e35b37 100644 --- a/django/db/models/base.py +++ b/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()) - errors.extend(cls._check_column_name_clashes()) + clash_errors = cls._check_id_field() + cls._check_field_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. diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 4454429997..77a48d0723 100644 --- a/django/db/models/fields/__init__.py +++ b/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 [] diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index b22890c301..bda84e847f 100644 --- a/docs/releases/1.7.txt +++ b/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 diff --git a/tests/invalid_models_tests/test_models.py b/tests/invalid_models_tests/test_models.py index 8515cc8070..9820aea969 100644 --- a/tests/invalid_models_tests/test_models.py +++ b/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): - 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): + def test_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: diff --git a/tests/model_inheritance/models.py b/tests/model_inheritance/models.py index 7f5702da59..3245c71232 100644 --- a/tests/model_inheritance/models.py +++ b/tests/model_inheritance/models.py @@ -45,10 +45,6 @@ class Student(CommonInfo): pass -class StudentWorker(Student, Worker): - pass - - # # Abstract base classes with related models # diff --git a/tests/model_inheritance/tests.py b/tests/model_inheritance/tests.py index dab3088a41..ebcfe1154e 100644 --- a/tests/model_inheritance/tests.py +++ b/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.