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 Carlton Gibson)

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 Carlton Gibson, 5 years ago

Has patch: set

comment:2 by Carlton Gibson, 5 years ago

Description: modified (diff)

comment:3 by Mariusz Felisiak, 5 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Carlton Gibson, 5 years ago

Description: modified (diff)

comment:5 by Mariusz Felisiak, 5 years ago

Triage Stage: AcceptedReady for checkin
Version: 3.0master

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 4bbe8261:

Fixed #31437 -- Corrected tests to show abstract multiple inheritance system check error.

Added minimal multiple inheritance test case showing error.
Removed obsolete diamond-inheritance case, originally added in
85ef98dc6ec565b1add417bd76808664e7318026.

Note: See TracTickets for help on using tickets.
Back to Top