Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fixed #17673 -- Forbid field shadowing.

Thanks Anssi Kääriäinen for the suggestion.
  • Loading branch information...
commit ee9fcb1672ddf5910ed8c45c37a00f32ebbe2bb1 1 parent f5123c7
Christopher Medrela authored timgraham committed
View
63 django/db/models/base.py
@@ -1055,8 +1055,12 @@ def check(cls, **kwargs):
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())
@@ -1176,6 +1180,61 @@ def _check_id_field(cls):
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.
used_column_names = []
View
23 django/db/models/fields/__init__.py
@@ -191,8 +191,9 @@ def check(self, **kwargs):
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 @@ def _check_field_name(self):
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 []
View
6 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
View
150 tests/invalid_models_tests/test_models.py
@@ -191,48 +191,158 @@ class Meta:
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)
+ field_ = models.CharField(max_length=10)
+ m2m_ = models.ManyToManyField('self')
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',
+ 'Field names must not end with underscores.',
+ hint=None,
+ obj=Model._meta.get_field('field_'),
+ id='E001',
),
Error(
- 'Field "id" has column name "id" that is already used.',
+ 'Field names must not end with underscores.',
hint=None,
- obj=Model,
+ obj=Model._meta.get_field('m2m_'),
+ id='E001',
+ ),
+ ]
+ 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_field_names_ending_with_underscore(self):
+ def test_pk(self):
class Model(models.Model):
- field_ = models.CharField(max_length=10)
- m2m_ = models.ManyToManyField('self')
+ pk = models.IntegerField()
errors = Model.check()
expected = [
Error(
- 'Field names must not end with underscores.',
+ 'Cannot use "pk" as a field name since it is a reserved name.',
hint=None,
- obj=Model._meta.get_field('field_'),
- id='E001',
+ 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(
- 'Field names must not end with underscores.',
+ ('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=Model._meta.get_field('m2m_'),
- id='E001',
+ 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)
View
4 tests/model_inheritance/models.py
@@ -45,10 +45,6 @@ class Meta:
pass
-class StudentWorker(Student, Worker):
- pass
-
-
#
# Abstract base classes with related models
#
View
34 tests/model_inheritance/tests.py
@@ -10,7 +10,7 @@
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 @@ def test_abstract(self):
# 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.
Please sign in to comment.
Something went wrong with that request. Please try again.