#34634: Creating objects with nested MTI crashes.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(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
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
The empty string insertions are due to a bug in `Options._get_fields` due
to how its caching strategy doesn't take `seen_models` into accounts even
if it can greatly influence the value stashed in the cache.
Because of that fields inherited from `Place` are present twice in
`ItalianRestaurantManyParents.meta.fields` which breaks `Model.__init__`
{{{#!python
class ItalianRestaurantManyParents(ItalianRestaurant, Place):
place_two_ptr = models.OneToOneField(
Place, on_delete=models.CASCADE, parent_link=True
)
>>> ItalianRestaurantManyParents._meta.fields
(<django.db.models.fields.AutoField: id>,
<django.db.models.fields.CharField: name>,
<django.db.models.fields.CharField: address>,
<django.db.models.fields.related.OneToOneField: place_ptr>,
<django.db.models.fields.IntegerField: rating>,
<django.db.models.fields.BooleanField: serves_hot_dogs>,
<django.db.models.fields.BooleanField: serves_pizza>,
<django.db.models.fields.related.ForeignKey: chef>,
<django.db.models.fields.related.OneToOneField: restaurant_ptr>,
<django.db.models.fields.BooleanField: serves_gnocchi>,
<django.db.models.fields.AutoField: id>, # dupe, already inherited from
Place
<django.db.models.fields.CharField: name>, # dupe, already inherited
from Place
<django.db.models.fields.CharField: address>, # dupe, already inherited
from Place
<django.db.models.fields.related.OneToOneField: italianrestaurant_ptr>,
<django.db.models.fields.related.OneToOneField: place_two_ptr>)
}}}
But if you clear the options cache
{{{#!python
>>> ItalianRestaurantManyParents._meta._expire_cache()
>>> ItalianRestaurant._meta._expire_cache()
>>> Restaurant._meta._expire_cache()
>>> Rating._meta._expire_cache()
>>> Place._meta._expire_cache()
>>> ItalianRestaurantManyParents._meta.fields
(<django.db.models.fields.AutoField: id>,
<django.db.models.fields.CharField: name>,
<django.db.models.fields.CharField: address>,
<django.db.models.fields.related.OneToOneField: place_ptr>,
<django.db.models.fields.IntegerField: rating>,
<django.db.models.fields.BooleanField: serves_hot_dogs>,
<django.db.models.fields.BooleanField: serves_pizza>,
<django.db.models.fields.related.ForeignKey: chef>,
<django.db.models.fields.related.OneToOneField: restaurant_ptr>,
<django.db.models.fields.BooleanField: serves_gnocchi>,
<django.db.models.fields.related.OneToOneField: italianrestaurant_ptr>,
<django.db.models.fields.related.OneToOneField: place_two_ptr>)
}}}
Things are right again!
My initial attempt at solving the issue
{{{#!diff
diff --git a/django/db/models/options.py b/django/db/models/options.py
index 00735e0de1..2f46df992e 100644
--- a/django/db/models/options.py
+++ b/django/db/models/options.py
@@ -864,7 +864,7 @@ def _get_fields(
reverse=True,
include_parents=True,
include_hidden=False,
- seen_models=None,
+ topmost_call=True,
):
"""
Internal helper function to return fields of the model.
@@ -885,13 +885,6 @@ def _get_fields(
# implementation and to provide a fast way for Django's internals
to
# access specific subsets of fields.
- # We must keep track of which models we have already seen.
Otherwise we
- # could include the same field multiple times from different
models.
- topmost_call = seen_models is None
- if topmost_call:
- seen_models = set()
- seen_models.add(self.model)
-
# Creates a cache key composed of all arguments
cache_key = (forward, reverse, include_parents, include_hidden,
topmost_call)
@@ -906,12 +899,11 @@ def _get_fields(
# Recursively call _get_fields() on each parent, with the same
# options provided in this call.
if include_parents is not False:
+ # In diamond inheritance it is possible that we see the same
+ # field from two different routes. In that case, avoid adding
+ # fields from the same parent again.
+ parent_fields = set()
for parent in self.parents:
- # In diamond inheritance it is possible that we see the
same
- # model from two different routes. In that case, avoid
adding
- # fields from the same parent again.
- if parent in seen_models:
- continue
if (
parent._meta.concrete_model != self.concrete_model
and include_parents == PROXY_PARENTS
@@ -922,13 +914,14 @@ def _get_fields(
reverse=reverse,
include_parents=include_parents,
include_hidden=include_hidden,
- seen_models=seen_models,
+ topmost_call=False,
):
if (
not getattr(obj, "parent_link", False)
or obj.model == self.concrete_model
- ):
+ ) and obj not in parent_fields:
fields.append(obj)
+ parent_fields.add(obj)
if reverse and not self.proxy:
# Tree is computed once and cached until the app cache is
expired.
# It is composed of a list of fields pointing to the current
model
}}}
So in order to address I issue I think a plan would be
1. Wait for #33414 to be merged as it affects the number of queries
performed on creation
2. Adjust the system check that detects field collisions to catch the case
in the initial report
3. Merge the changes to `Options._get_fields` with the test added by
Mariusz with the small adjustment to the model mentioned in comment:10 to
make sure it passes the adjusted test added in 2.
Does that make sense to you?
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:15>
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/01070188bd8dd80d-21fd9f99-3366-43b4-aeb1-ab2b14073653-000000%40eu-central-1.amazonses.com.
