Creating objects with nested MTI crashes.

[Mariusz Felisiak]

Checking ​PR I noticed that creating objects with more complicated multi-table inheritance crashes. For example:

tests/model_inheritance/models.py

@@ -106 +106 @@
     serves_gnocchi = models.BooleanField(default=False)
 
 
+class ItalianRestaurantManyParents(ItalianRestaurant, Place):
+    pass
+
+
 class Supplier(Place):
     customers = models.ManyToManyField(Restaurant, related_name="provider")
 

tests/model_inheritance/tests.py

@@ -14 +14 @@
     GrandChild,
     GrandParent,
     ItalianRestaurant,
+    ItalianRestaurantManyParents,
     MixinModel,
     Parent,
     ParkingLot,
@@ -149 +150 @@
         # 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'

[Akash Kumar Sen]

​PR

[Akash Kumar Sen]

​Separate PR

[Mariusz Felisiak]

Assertions fail.

[Akash Kumar Sen]

Opened a ​google group discussion

[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)
  

[Akash Kumar Sen]

Updated the PR with possible solution 2 on comment

[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

tests/model_inheritance/models.py

@@ -110 +110 @@
     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

tests/model_inheritance/models.py

@@ -106 +106 @@
     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")
 

tests/model_inheritance/tests.py

@@ -25 +25 @@
     Supplier,
     Title,
     Worker,
+    ItalianRestaurantManyParents,
 )
 
 
@@ -149 +150 @@
         # 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)

[Mariusz Felisiak]

Replying to Simon Charette:

It feels like ItalianRestaurantManyParents should not be allowed to be defined in the first place since place_ptr conflicts.

Agreed

[Akash Kumar Sen]

Replying to 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

[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.

[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 the place_ptr field on checks.

[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__

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

>>> 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

django/db/models/options.py

@@ -864 +864 @@
         reverse=True,
         include_parents=True,
         include_hidden=False,
-        seen_models=None,
+        topmost_call=True,
     ):
         """
         Internal helper function to return fields of the model.
@@ -885 +885 @@
         # 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 +899 @@
         # 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 +914 @@
                     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 (first commit)
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 system check added adjusted in 2 (second commit)

Does that make sense to you?

[Akash Kumar Sen]

That makes sense. Updated the ​patch accordingly.

[Akash Kumar Sen]

Please share your thoughts about the following ​comment. I will update the patch accordingly.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 82a588a6:

Fixed #34634 -- Adjusted system check for clashing fields to warn about links to common parent for MTI models.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 1754c2c:

Refs #34634 -- Fixed creating diamond-shaped MTI objects with ancestors inherited from different paths.

Co-authored-by: Simon Charette <charette.s@…>