From 225d96533a8e05debd402a2bfe566487cc27d95f Mon Sep 17 00:00:00 2001 From: Carlton Gibson Date: Wed, 9 Jun 2021 16:55:22 +0200 Subject: [PATCH] Fixed #30427, Fixed #16176 -- Corrected setting descriptor in Field.contribute_to_class(). Co-authored-by: Jarek Glowacki --- django/db/models/fields/__init__.py | 6 +- docs/topics/db/models.txt | 7 +++ tests/check_framework/tests.py | 6 ++ tests/defer/models.py | 13 +++++ tests/defer/tests.py | 6 ++ tests/invalid_models_tests/test_models.py | 5 +- .../test_abstract_inheritance.py | 55 ++++++++++++++++--- tests/model_inheritance/tests.py | 31 +++++++++++ 8 files changed, 112 insertions(+), 17 deletions(-) diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 51155f5776..d7e19ad25b 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -782,11 +782,7 @@ class Field(RegisterLookupMixin): self.model = cls cls._meta.add_field(self, private=private_only) if self.column: - # Don't override classmethods with the descriptor. This means that - # if you have a classmethod and a field with the same name, then - # such fields can't be deferred (we don't have a check for this). - if not getattr(cls, self.attname, None): - setattr(cls, self.attname, self.descriptor_class(self)) + setattr(cls, self.attname, self.descriptor_class(self)) if self.choices is not None: # Don't override a get_FOO_display() method defined explicitly on # this class, but don't check methods derived from inheritance, to diff --git a/docs/topics/db/models.txt b/docs/topics/db/models.txt index 26a6d7dc58..1c4e99817f 100644 --- a/docs/topics/db/models.txt +++ b/docs/topics/db/models.txt @@ -1457,6 +1457,13 @@ different database tables). Django will raise a :exc:`~django.core.exceptions.FieldError` if you override any model field in any ancestor model. +Note that because of the way fields are resolved during class definition, model +fields inherited from multiple abstract parent models are resolved in a strict +depth-first order. This contrasts with standard Python MRO, which is resolved +breadth-first in cases of diamond shaped inheritance. This difference only +affects complex model hierarchies, which (as per the advice above) you should +try to avoid. + Organizing models in a package ============================== diff --git a/tests/check_framework/tests.py b/tests/check_framework/tests.py index 9e461c5040..f43abaca12 100644 --- a/tests/check_framework/tests.py +++ b/tests/check_framework/tests.py @@ -314,6 +314,12 @@ class CheckFrameworkReservedNamesTests(SimpleTestCase): obj=ModelWithAttributeCalledCheck, id='models.E020' ), + Error( + "The 'ModelWithFieldCalledCheck.check()' class method is " + "currently overridden by %r." % ModelWithFieldCalledCheck.check, + obj=ModelWithFieldCalledCheck, + id='models.E020' + ), Error( "The 'ModelWithRelatedManagerCalledCheck.check()' class method is " "currently overridden by %r." % ModelWithRelatedManagerCalledCheck.check, diff --git a/tests/defer/models.py b/tests/defer/models.py index fc14f43393..fea673643b 100644 --- a/tests/defer/models.py +++ b/tests/defer/models.py @@ -44,3 +44,16 @@ class RefreshPrimaryProxy(Primary): if fields.intersection(deferred_fields): fields = fields.union(deferred_fields) super().refresh_from_db(using, fields, **kwargs) + + +class ShadowParent(models.Model): + """ + ShadowParent declares a scalar, rather than a field. When this is + overridden, the field value, rather than the scalar value must still be + used when the field is deferred. + """ + name = 'aphrodite' + + +class ShadowChild(ShadowParent): + name = models.CharField(default='adonis', max_length=6) diff --git a/tests/defer/tests.py b/tests/defer/tests.py index b85475f74f..4058fadde9 100644 --- a/tests/defer/tests.py +++ b/tests/defer/tests.py @@ -3,6 +3,7 @@ from django.test import TestCase from .models import ( BigChild, Child, ChildProxy, Primary, RefreshPrimaryProxy, Secondary, + ShadowChild, ) @@ -165,6 +166,11 @@ class DeferTests(AssertionMixin, TestCase): self.assertEqual(obj.name, "c1") self.assertEqual(obj.value, "foo") + def test_defer_of_overridden_scalar(self): + ShadowChild.objects.create() + obj = ShadowChild.objects.defer('name').get() + self.assertEqual(obj.name, 'adonis') + class BigChildDeferTests(AssertionMixin, TestCase): @classmethod diff --git a/tests/invalid_models_tests/test_models.py b/tests/invalid_models_tests/test_models.py index 958ad120ca..66657211b2 100644 --- a/tests/invalid_models_tests/test_models.py +++ b/tests/invalid_models_tests/test_models.py @@ -1212,9 +1212,8 @@ class OtherModelTests(SimpleTestCase): class Model(models.Model): fk = models.ForeignKey('self', models.CASCADE) - @property - def fk_id(self): - pass + # Override related field accessor. + Model.fk_id = property(lambda self: 'ERROR') self.assertEqual(Model.check(), [ Error( diff --git a/tests/model_inheritance/test_abstract_inheritance.py b/tests/model_inheritance/test_abstract_inheritance.py index 27b390d9f6..f2dab05b8b 100644 --- a/tests/model_inheritance/test_abstract_inheritance.py +++ b/tests/model_inheritance/test_abstract_inheritance.py @@ -34,7 +34,12 @@ class AbstractInheritanceTests(SimpleTestCase): self.assertEqual(DerivedChild._meta.get_field('name').max_length, 50) self.assertEqual(DerivedGrandChild._meta.get_field('name').max_length, 50) - def test_multiple_inheritance_cannot_shadow_inherited_field(self): + def test_multiple_inheritance_allows_inherited_field(self): + """ + Single layer multiple inheritance is as expected, deriving the + inherited field from the first base. + """ + class ParentA(models.Model): name = models.CharField(max_length=255) @@ -50,14 +55,46 @@ class AbstractInheritanceTests(SimpleTestCase): class Child(ParentA, ParentB): pass - self.assertEqual(Child.check(), [ - Error( - "The field 'name' clashes with the field 'name' from model " - "'model_inheritance.child'.", - obj=Child._meta.get_field('name'), - id='models.E006', - ), - ]) + self.assertEqual(Child.check(), []) + inherited_field = Child._meta.get_field('name') + self.assertTrue(isinstance(inherited_field, models.CharField)) + self.assertEqual(inherited_field.max_length, 255) + + def test_diamond_shaped_multiple_inheritance_is_depth_first(self): + """ + In contrast to standard Python MRO, resolution of inherited fields is + strictly depth-first, rather than breadth-first in diamond-shaped cases. + + This is because a copy of the parent field descriptor is placed onto + the model class in ModelBase.__new__(), rather than the attribute + lookup going via bases. (It only **looks** like inheritance.) + + Here, Child inherits name from Root, rather than ParentB. + """ + + class Root(models.Model): + name = models.CharField(max_length=255) + + class Meta: + abstract = True + + class ParentA(Root): + class Meta: + abstract = True + + class ParentB(Root): + name = models.IntegerField() + + class Meta: + abstract = True + + class Child(ParentA, ParentB): + pass + + self.assertEqual(Child.check(), []) + inherited_field = Child._meta.get_field('name') + self.assertTrue(isinstance(inherited_field, models.CharField)) + self.assertEqual(inherited_field.max_length, 255) def test_target_field_may_be_pushed_down(self): """ diff --git a/tests/model_inheritance/tests.py b/tests/model_inheritance/tests.py index 1ab0e15eee..d3508c23af 100644 --- a/tests/model_inheritance/tests.py +++ b/tests/model_inheritance/tests.py @@ -2,6 +2,7 @@ from operator import attrgetter from django.core.exceptions import FieldError, ValidationError from django.db import connection, models +from django.db.models.query_utils import DeferredAttribute from django.test import SimpleTestCase, TestCase from django.test.utils import CaptureQueriesContext, isolate_apps @@ -222,6 +223,36 @@ class ModelInheritanceTests(TestCase): self.assertIs(models.QuerySet[Post, Post], models.QuerySet) self.assertIs(models.QuerySet[Post, int, str], models.QuerySet) + def test_shadow_parent_attribute_with_field(self): + class ScalarParent(models.Model): + foo = 1 + + class ScalarOverride(ScalarParent): + foo = models.IntegerField() + + self.assertEqual(type(ScalarOverride.foo), DeferredAttribute) + + def test_shadow_parent_property_with_field(self): + class PropertyParent(models.Model): + @property + def foo(self): + pass + + class PropertyOverride(PropertyParent): + foo = models.IntegerField() + + self.assertEqual(type(PropertyOverride.foo), DeferredAttribute) + + def test_shadow_parent_method_with_field(self): + class MethodParent(models.Model): + def foo(self): + pass + + class MethodOverride(MethodParent): + foo = models.IntegerField() + + self.assertEqual(type(MethodOverride.foo), DeferredAttribute) + class ModelInheritanceDataTests(TestCase): @classmethod