#31437: Invalid model structure used in tests.
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Carlton
| Gibson
Type: Bug | Status: assigned
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Carlton Gibson:
Old description:
> 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()`.
>
> 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.)
New description:
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()`.
([https://github.com/django/django/blob/94b32a20ef798768029651f0e8fcc07731a62a64/django/db/models/base.py#L273-L278
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.)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31437#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/071.4bd715d07e6e0d276ebafcbdef62885e%40djangoproject.com.
