[Django] #34634: Creating objects with nested MTI crashes.
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz | Owner: nobody
Felisiak |
Type: Bug | Status: new
Component: Database | Version: 4.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Checking [https://github.com/django/django/pull/16903 PR] I noticed that
creating objects with more complicated multi-table inheritance crashes.
For example:
{{{#!diff
diff --git a/tests/model_inheritance/models.py
b/tests/model_inheritance/models.py
index dc0e238f7e..d75df0d533 100644
--- a/tests/model_inheritance/models.py
+++ b/tests/model_inheritance/models.py
@@ -106,6 +106,10 @@ class ItalianRestaurant(Restaurant):
serves_gnocchi = models.BooleanField(default=False)
+class ItalianRestaurantManyParents(ItalianRestaurant, Place):
+ pass
+
+
class Supplier(Place):
customers = models.ManyToManyField(Restaurant,
related_name="provider")
diff --git a/tests/model_inheritance/tests.py
b/tests/model_inheritance/tests.py
index 4542e6c3cc..838d35b9d6 100644
--- a/tests/model_inheritance/tests.py
+++ b/tests/model_inheritance/tests.py
@@ -14,6 +14,7 @@ from .models import (
GrandChild,
GrandParent,
ItalianRestaurant,
+ ItalianRestaurantManyParents,
MixinModel,
Parent,
ParkingLot,
@@ -149,6 +150,13 @@ class ModelInheritanceTests(TestCase):
# accidentally found).
self.assertSequenceEqual(s.titles.all(), [])
+ def test_create_diamond_mti_common_parents(self):
+ with self.assertNumQueries(4):
+ ItalianRestaurantManyParents.objects.create(
+ name="Ristorante Miron",
+ address="1234 W. Ash",
+ )
+
def test_update_parent_filtering(self):
"""
Updating a field of a model subclass doesn't issue an UPDATE
}}}
crashes with:
{{{
File "/django/tests/model_inheritance/tests.py", line 155, in
test_create_diamond_mti_common_parents
ItalianRestaurantManyParents.objects.create(
File "/django/django/db/models/manager.py", line 87, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/django/django/db/models/query.py", line 650, in create
obj.save(force_insert=True, using=self.db)
File "/django/django/db/models/base.py", line 814, in save
self.save_base(
File "/django/django/db/models/base.py", line 876, in save_base
parent_inserted = self._save_parents(cls, using, update_fields)
File "/django/django/db/models/base.py", line 928, in _save_parents
setattr(self, field.attname, self._get_pk_val(parent._meta))
AttributeError: 'OneToOneField' object has no attribute 'attname'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34634>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "Screenshot_20230606_091200.png" added.
schema
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* Attachment "Screenshot_20230606_091200.png" removed.
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* Attachment "Screenshot_20230606_091439.png" added.
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
Checking [https://github.com/django/django/pull/16903 PR] I noticed that
creating objects with more complicated multi-table inheritance crashes.
For example:
[[Image(Screenshot_20230606_091439.png)]]
crashes with:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:2>
Django
-------------------------------------+-------------------------------------
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Akash Kumar Sen
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:3>
Django
-------------------------------------+-------------------------------------
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
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Akash Kumar Sen):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/16903 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:4>
Django
-------------------------------------+-------------------------------------
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 Akash Kumar Sen):
[https://github.com/django/django/pull/16952 Separate PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:5>
Django
-------------------------------------+-------------------------------------
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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Assertions fail.
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:6>
Django
-------------------------------------+-------------------------------------
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: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Akash Kumar Sen):
Opened a [https://groups.google.com/g/django-developers/c/0oF_2yDekzQ
google group discussion]
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:7>
Django
-------------------------------------+-------------------------------------
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: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Akash Kumar Sen):
Possible MTI Scenarios with two bases in Django
**Example Model:**
{{{
class CommonChild(FirstParent, SecondParent):
pass
}}}
**Case 1 -- FirstParent and Secondparent are does not have a common
ancestor**
- This scenario shows no regression
**Case 2 -- FirstParent and Secondparent have a common ancestor**
- This scenario shows regression only with primary key with default, as
mentioned in #33414
**Case 3 -- FirstParent is the ancestor of SecondParent**
- This shows the following {{{TypeError}}}. (I think this is the expected
scenario)
{{{
TypeError: Cannot create a consistent method resolution
order (MRO) for bases FirstParent, Secondparent
}}}
**Case 4 -- Secondparent is the ancestor of FirstParent(Case mentioned
here)**
- I tried to print {{{ItalianRestaurantManyParents._meta.get_fields()}}} ,
it contained all the fields of the {{{Place}}} model twice, that means we
are having conflicts passing all the checks here.
- As the fields in {{{ItalianRestaurantManyParents}}} is already a
superset of all the fields of {{{Place}}} I don't think it makes any real
world use case scenario for allowing such this kind of MTI scenario.
- **Possible solution 1 :** would be to put a check and show a similar
kind of error like case 3.
- **Possible solution 2 :** would be to let the child eat the parent
during initialization of the model, like the following. (This seems to be
passing all the tests, just have one problem. When the user defines a
custom {{{OneToOneField}}} pointer to the SecondParent)
{{{
class ModelBase(type):
"""Metaclass for all models."""
def __new__(cls, name, bases, attrs, **kwargs):
super_new = super().__new__
# Also ensure initialization is only performed for subclasses of
Model
# (excluding Model class itself).
parents = [b for b in bases if isinstance(b, ModelBase)]
for parent in parents:
if any(
issubclass(other_parent, parent)
for other_parent in parents
if not other_parent == parent
):
parents.remove(parent)
if not parents:
return super_new(cls, name, bases, attrs)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:8>
Django
-------------------------------------+-------------------------------------
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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
Updated the PR with possible solution 2 on
[https://code.djangoproject.com/ticket/34634#comment:8 comment]
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:9>
Django
-------------------------------------+-------------------------------------
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):
It feels like `ItalianRestaurantManyParents` should not be allowed to be
defined in the first place since `place_ptr` conflicts.
For example the following model cannot be defined
{{{#!diff
diff --git a/tests/model_inheritance/models.py
b/tests/model_inheritance/models.py
index dc0e238f7e..9b25ac4b8a 100644
--- a/tests/model_inheritance/models.py
+++ b/tests/model_inheritance/models.py
@@ -110,6 +110,9 @@ class Supplier(Place):
customers = models.ManyToManyField(Restaurant,
related_name="provider")
+class Foo(Supplier, Restaurant):
+ pass
+
class CustomSupplier(Supplier):
pass
}}}
{{{
model_inheritance.Bob: (models.E005) The field 'place_ptr' from parent
model 'model_inheritance.supplier' clashes with the field 'place_ptr' from
parent model 'model_inheritance.restaurant'.
}}}
I would expect a similar crash when defining
`ItalianRestaurantManyParents` about a conflicting `place_ptr`.
Once the model is appropriately defined to avoid the conflicting parent
link the creation succeeds with an extra query
{{{#!diff
diff --git a/tests/model_inheritance/models.py
b/tests/model_inheritance/models.py
index dc0e238f7e..c303a97d04 100644
--- a/tests/model_inheritance/models.py
+++ b/tests/model_inheritance/models.py
@@ -106,6 +106,12 @@ class ItalianRestaurant(Restaurant):
serves_gnocchi = models.BooleanField(default=False)
+class ItalianRestaurantManyParents(ItalianRestaurant, Place):
+ place_two_ptr = models.OneToOneField(
+ Place, on_delete=models.CASCADE, parent_link=True
+ )
+
+
class Supplier(Place):
customers = models.ManyToManyField(Restaurant,
related_name="provider")
diff --git a/tests/model_inheritance/tests.py
b/tests/model_inheritance/tests.py
index 4542e6c3cc..0ed9d2f14e 100644
--- a/tests/model_inheritance/tests.py
+++ b/tests/model_inheritance/tests.py
@@ -25,6 +25,7 @@
Supplier,
Title,
Worker,
+ ItalianRestaurantManyParents,
)
@@ -149,6 +150,13 @@ def test_custompk_m2m(self):
# accidentally found).
self.assertSequenceEqual(s.titles.all(), [])
+ def test_create_diamond_mti_common_parents(self):
+ with self.assertNumQueries(5):
+ ItalianRestaurantManyParents.objects.create(
+ name="Ristorante Miron",
+ address="1234 W. Ash",
+ )
+
def test_update_parent_filtering(self):
"""
Updating a field of a model subclass doesn't issue an UPDATE
}}}
{{{
1. INSERT INTO "model_inheritance_place" ("name", "address") VALUES ('',
'') RETURNING "model_inheritance_place"."id"
2. INSERT INTO "my_restaurant" ("place_ptr_id", "rating",
"serves_hot_dogs", "serves_pizza", "chef_id") VALUES (1, NULL, 0, 0, NULL)
3. INSERT INTO "model_inheritance_italianrestaurant" ("restaurant_ptr_id",
"serves_gnocchi") VALUES (1, 0)
4. UPDATE "model_inheritance_place" SET "name" = '', "address" = '' WHERE
"model_inheritance_place"."id" = 1
5. INSERT INTO "model_inheritance_italianrestaurantmanyparents"
("italianrestaurant_ptr_id", "place_two_ptr_id") VALUES (1, 1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:10>
Django
-------------------------------------+-------------------------------------
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 Mariusz Felisiak):
Replying to [comment:10 Simon Charette]:
> It feels like `ItalianRestaurantManyParents` should not be allowed to be
defined in the first place since `place_ptr` conflicts.
Agreed
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:11>
Django
-------------------------------------+-------------------------------------
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 Akash Kumar Sen):
Replying to [comment:10 Simon Charette]:
> {{{
> 1. INSERT INTO "model_inheritance_place" ("name", "address") VALUES ('',
'') RETURNING "model_inheritance_place"."id"
> }}}
The values are empty strings in the query 1, So I don't think this gives
the expected results. The name and address should have their respective
values instead of empty strings.
>It feels like ItalianRestaurantManyParents should not be allowed to be
defined in the first place since place_ptr conflicts.
Along with the field {{{place_ptr}}} , the following fields, i.e all the
fields of {{{Place}}} model seems to be
{{{<django.db.models.fields.AutoField: id>,
<django.db.models.fields.CharField: name>,
<django.db.models.fields.CharField: address>}}} present twice without any
conflicts or errors. So I think it would make sense if we disallow the
case-4 according to comment:8
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:12>
Django
-------------------------------------+-------------------------------------
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 values are empty strings in the query 1, So I don't think this gives
the expected results. The name and address should have their respective
values instead of empty strings.
Right I missed that. So no more crash but wrong behaviour.
> Along with the field place_ptr , the following fields, i.e all the
fields of Place model seems to be <django.db.models.fields.AutoField: id>,
<django.db.models.fields.CharField: name>,
<django.db.models.fields.CharField: address> present twice without any
conflicts or errors. So I think it would make sense if we disallow the
case-4 according to comment:8
I'm not sure I understand why this is the case. When
`ItalianRestaurantManyParents(ItalianRestaurant, Place)` is defined it
requires creating two parent links, `italianrestaurant_ptr ->
ItalianRestaurant` and `place_ptr -> Place`. The only attribute conflict
should be between `ItalianRestaurantManyParents.place_ptr` and
`Restaurant.place_ptr` in this case, all the other fields are only defined
once on the `Place` model and nowhere else.
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:13>
Django
-------------------------------------+-------------------------------------
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 Akash Kumar Sen):
I tried to do the following:
{{{
class ItalianRestaurantManyParents(ItalianRestaurant, Place):
place_ptr = models.OneToOneField(
Place, on_delete=models.CASCADE, parent_link=True
)
}}}
This is producing the expected conflict of field on checks.
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:14>
Django
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:16>
Django
-------------------------------------+-------------------------------------
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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
That makes sense. Updated the [https://github.com/django/django/pull/16952
patch] accordingly.
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:17>
Django
-------------------------------------+-------------------------------------
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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:18>
Django
-------------------------------------+-------------------------------------
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: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Akash Kumar Sen):
Please share your thoughts about the following
[https://github.com/django/django/pull/16952#discussion_r1233827427
comment]. I will update the patch accordingly.
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:19>
Django
-------------------------------------+-------------------------------------
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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Akash
| Kumar Sen
Component: Database layer | Version: 4.2
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"82a588a6bce37e1fe0a1d266282034136460b37a" 82a588a6]:
{{{
#!CommitTicketReference repository=""
revision="82a588a6bce37e1fe0a1d266282034136460b37a"
Fixed #34634 -- Adjusted system check for clashing fields to warn about
links to common parent for MTI models.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Akash
| Kumar Sen
Type: Bug | Status: closed
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"1754c2c8026ab592183821c3933972a8aa7acc66" 1754c2c]:
{{{
#!CommitTicketReference repository=""
revision="1754c2c8026ab592183821c3933972a8aa7acc66"
Refs #34634 -- Fixed creating diamond-shaped MTI objects with ancestors
inherited from different paths.
Co-authored-by: Simon Charette <chare...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34634#comment:23>