Opened 18 months ago

Closed 17 months ago

Last modified 17 months ago

#34634 closed Bug (fixed)

Creating objects with nested MTI crashes.

Reported by: Mariusz Felisiak Owned by: Akash Kumar Sen
Component: Database layer (models, ORM) Version: 4.2
Severity: Normal Keywords:
Cc: Simon Charette, David Sanders, Lily Foote 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 Mariusz Felisiak)

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

  • tests/model_inheritance/models.py

    diff --git a/tests/model_inheritance/models.py b/tests/model_inheritance/models.py
    index dc0e238f7e..d75df0d533 100644
    a b class ItalianRestaurant(Restaurant):  
    106106    serves_gnocchi = models.BooleanField(default=False)
    107107
    108108
     109class ItalianRestaurantManyParents(ItalianRestaurant, Place):
     110    pass
     111
     112
    109113class Supplier(Place):
    110114    customers = models.ManyToManyField(Restaurant, related_name="provider")
    111115
  • tests/model_inheritance/tests.py

    diff --git a/tests/model_inheritance/tests.py b/tests/model_inheritance/tests.py
    index 4542e6c3cc..838d35b9d6 100644
    a b from .models import (  
    1414    GrandChild,
    1515    GrandParent,
    1616    ItalianRestaurant,
     17    ItalianRestaurantManyParents,
    1718    MixinModel,
    1819    Parent,
    1920    ParkingLot,
    class ModelInheritanceTests(TestCase):  
    149150        # accidentally found).
    150151        self.assertSequenceEqual(s.titles.all(), [])
    151152
     153    def test_create_diamond_mti_common_parents(self):
     154        with self.assertNumQueries(4):
     155            ItalianRestaurantManyParents.objects.create(
     156                name="Ristorante Miron",
     157                address="1234 W. Ash",
     158            )
     159
    152160    def test_update_parent_filtering(self):
    153161        """
    154162        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'

Attachments (1)

Screenshot_20230606_091439.png (12.2 KB ) - added by Mariusz Felisiak 18 months ago.

Download all attachments as: .zip

Change History (24)

by Mariusz Felisiak, 18 months ago

comment:1 by Mariusz Felisiak, 18 months ago

Description: modified (diff)

comment:2 by David Sanders, 18 months ago

Triage Stage: UnreviewedAccepted

comment:3 by Akash Kumar Sen, 18 months ago

Owner: changed from nobody to Akash Kumar Sen
Status: newassigned

comment:4 by Akash Kumar Sen, 18 months ago

Has patch: set
Last edited 18 months ago by Akash Kumar Sen (previous) (diff)

comment:5 by Akash Kumar Sen, 18 months ago

comment:6 by Mariusz Felisiak, 18 months ago

Patch needs improvement: set

Assertions fail.

comment:7 by Akash Kumar Sen, 18 months ago

comment:8 by Akash Kumar Sen, 18 months ago

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)
    

comment:9 by Akash Kumar Sen, 18 months ago

Patch needs improvement: unset

Updated the PR with possible solution 2 on comment

comment:10 by Simon Charette, 18 months ago

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

    diff --git a/tests/model_inheritance/models.py b/tests/model_inheritance/models.py
    index dc0e238f7e..9b25ac4b8a 100644
    a b class Supplier(Place):  
    110110    customers = models.ManyToManyField(Restaurant, related_name="provider")
    111111
    112112
     113class Foo(Supplier, Restaurant):
     114    pass
     115
    113116class CustomSupplier(Supplier):
    114117    pass
    115118
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

    diff --git a/tests/model_inheritance/models.py b/tests/model_inheritance/models.py
    index dc0e238f7e..c303a97d04 100644
    a b class ItalianRestaurant(Restaurant):  
    106106    serves_gnocchi = models.BooleanField(default=False)
    107107
    108108
     109class ItalianRestaurantManyParents(ItalianRestaurant, Place):
     110    place_two_ptr = models.OneToOneField(
     111        Place, on_delete=models.CASCADE, parent_link=True
     112    )
     113
     114
    109115class Supplier(Place):
    110116    customers = models.ManyToManyField(Restaurant, related_name="provider")
    111117
  • tests/model_inheritance/tests.py

    diff --git a/tests/model_inheritance/tests.py b/tests/model_inheritance/tests.py
    index 4542e6c3cc..0ed9d2f14e 100644
    a b  
    2525    Supplier,
    2626    Title,
    2727    Worker,
     28    ItalianRestaurantManyParents,
    2829)
    2930
    3031
    def test_custompk_m2m(self):  
    149150        # accidentally found).
    150151        self.assertSequenceEqual(s.titles.all(), [])
    151152
     153    def test_create_diamond_mti_common_parents(self):
     154        with self.assertNumQueries(5):
     155            ItalianRestaurantManyParents.objects.create(
     156                name="Ristorante Miron",
     157                address="1234 W. Ash",
     158            )
     159
    152160    def test_update_parent_filtering(self):
    153161        """
    154162        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)

in reply to:  10 comment:11 by Mariusz Felisiak, 18 months ago

Replying to Simon Charette:

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

Agreed

in reply to:  10 comment:12 by Akash Kumar Sen, 18 months ago

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

comment:13 by Simon Charette, 18 months ago

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.

in reply to:  10 comment:14 by Akash Kumar Sen, 18 months ago

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.

Last edited 18 months ago by Akash Kumar Sen (previous) (diff)

comment:15 by Simon Charette, 18 months ago

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

    diff --git a/django/db/models/options.py b/django/db/models/options.py
    index 00735e0de1..2f46df992e 100644
    a b def _get_fields(  
    864864        reverse=True,
    865865        include_parents=True,
    866866        include_hidden=False,
    867         seen_models=None,
     867        topmost_call=True,
    868868    ):
    869869        """
    870870        Internal helper function to return fields of the model.
    def _get_fields(  
    885885        # implementation and to provide a fast way for Django's internals to
    886886        # access specific subsets of fields.
    887887
    888         # We must keep track of which models we have already seen. Otherwise we
    889         # could include the same field multiple times from different models.
    890         topmost_call = seen_models is None
    891         if topmost_call:
    892             seen_models = set()
    893         seen_models.add(self.model)
    894 
    895888        # Creates a cache key composed of all arguments
    896889        cache_key = (forward, reverse, include_parents, include_hidden, topmost_call)
    897890
    def _get_fields(  
    906899        # Recursively call _get_fields() on each parent, with the same
    907900        # options provided in this call.
    908901        if include_parents is not False:
     902            # In diamond inheritance it is possible that we see the same
     903            # field from two different routes. In that case, avoid adding
     904            # fields from the same parent again.
     905            parent_fields = set()
    909906            for parent in self.parents:
    910                 # In diamond inheritance it is possible that we see the same
    911                 # model from two different routes. In that case, avoid adding
    912                 # fields from the same parent again.
    913                 if parent in seen_models:
    914                     continue
    915907                if (
    916908                    parent._meta.concrete_model != self.concrete_model
    917909                    and include_parents == PROXY_PARENTS
    def _get_fields(  
    922914                    reverse=reverse,
    923915                    include_parents=include_parents,
    924916                    include_hidden=include_hidden,
    925                     seen_models=seen_models,
     917                    topmost_call=False,
    926918                ):
    927919                    if (
    928920                        not getattr(obj, "parent_link", False)
    929921                        or obj.model == self.concrete_model
    930                     ):
     922                    ) and obj not in parent_fields:
    931923                        fields.append(obj)
     924                        parent_fields.add(obj)
    932925        if reverse and not self.proxy:
    933926            # Tree is computed once and cached until the app cache is expired.
    934927            # 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?

Last edited 18 months ago by Simon Charette (previous) (diff)

comment:16 by Mariusz Felisiak, 18 months ago

Patch needs improvement: set

Has for me.

Last edited 18 months ago by Mariusz Felisiak (previous) (diff)

comment:17 by Akash Kumar Sen, 17 months ago

Patch needs improvement: unset

That makes sense. Updated the patch accordingly.

comment:18 by Mariusz Felisiak, 17 months ago

Patch needs improvement: set

comment:19 by Akash Kumar Sen, 17 months ago

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

comment:20 by Akash Kumar Sen, 17 months ago

Patch needs improvement: unset

comment:21 by Mariusz Felisiak, 17 months ago

Triage Stage: AcceptedReady for checkin

comment:22 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

Resolution: fixed
Status: assignedclosed

In 82a588a6:

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

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

In 1754c2c:

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

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

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