Opened 5 years ago
Closed 5 years ago
#31437 closed Bug (fixed)
Invalid model structure used in tests.
Reported by: | Carlton Gibson | Owned by: | Carlton Gibson |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Whilst researching the fix for #30427, #16176, it becomes apparent that an abstract multiple inheritance structure introduced for #24305 in 85ef98dc6ec565b1add417bd76808664e7318026 was invalid.
Checking out 85ef98dc6ec565b1add417bd76808664e7318026 and adding a model check call on the defined models, results in a failure:
(django) .../django/tests ((85ef98dc6e...))$ git diff diff --git a/tests/model_inheritance/test_abstract_inheritance.py b/tests/model_inheritance/test_abstract_inheritance.py index 71b8513a57..3964ff9858 100644 --- a/tests/model_inheritance/test_abstract_inheritance.py +++ b/tests/model_inheritance/test_abstract_inheritance.py @@ -60,6 +60,8 @@ class AbstractInheritanceTests(TestCase): class Derived(DescendantOne, DescendantTwo): pass + + self.assertEqual(Derived.check(), []) self.assertEqual(DescendantOne._meta.get_field('name').max_length, 30) self.assertEqual(DescendantTwo._meta.get_field('name').max_length, 50) self.assertEqual(Derived._meta.get_field('name').max_length, 50)
Here's the failure:
FAIL: test_multiple_parents_mro (model_inheritance.test_abstract_inheritance.AbstractInheritanceTests) First list contains 1 additional elements. First extra element 0: <Error: level=40, msg="The field 'name' clashes with the field 'name' from model 'model_inheritance.derived'.", hint=None, obj=<django.db.models.fields.CharField: name>, id='models.E006'> - [<Error: level=40, msg="The field 'name' clashes with the field 'name' from model 'model_inheritance.derived'.", hint=None, obj=<django.db.models.fields.CharField: name>, id='models.E006'>] + []
This issue occurs because the doubly inherited fields are not present in
local_fields
before iterating the bases in ModelBase.__new__()
, the list of local_fields
is not
updated whilst so iterating (which would not be a realistic option), and each
field adds itself to the model in contribute_to_class()
.
(Relevant bit of `ModelBase.__new__()`)
As a result overriding multiply inherited fields has never been supported. That
it is not supported is in line with the situation with MTI. (Thus no great hardship.)
The tests should be adjusted to communicate the actual situation.
What is more, the system check message is misleading:
"The field 'name' clashes with the field 'name' from model 'model_inheritance.derived'."
It's pointing to the child model, rather than the parent(s) that we hoped for.
Ideally the system check error message would be improved to point to the parent models in this case,
but it is not at all obvious how we can identify the relevant fields reliably. Yes, we can walk the bases
again, but all kinds of structures may be possible, such that the effort is probably not worth it.
(This has gone unnoticed for a number of years after all.)
Change History (6)
comment:1 by , 5 years ago
Has patch: | set |
---|
comment:2 by , 5 years ago
Description: | modified (diff) |
---|
comment:3 by , 5 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 5 years ago
Description: | modified (diff) |
---|
comment:5 by , 5 years ago
Triage Stage: | Accepted → Ready for checkin |
---|---|
Version: | 3.0 → master |
PR