#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 )
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): 106 106 serves_gnocchi = models.BooleanField(default=False) 107 107 108 108 109 class ItalianRestaurantManyParents(ItalianRestaurant, Place): 110 pass 111 112 109 113 class Supplier(Place): 110 114 customers = models.ManyToManyField(Restaurant, related_name="provider") 111 115 -
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 ( 14 14 GrandChild, 15 15 GrandParent, 16 16 ItalianRestaurant, 17 ItalianRestaurantManyParents, 17 18 MixinModel, 18 19 Parent, 19 20 ParkingLot, … … class ModelInheritanceTests(TestCase): 149 150 # accidentally found). 150 151 self.assertSequenceEqual(s.titles.all(), []) 151 152 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 152 160 def test_update_parent_filtering(self): 153 161 """ 154 162 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)
Change History (24)
by , 17 months ago
Attachment: | Screenshot_20230606_091439.png added |
---|
comment:1 by , 17 months ago
Description: | modified (diff) |
---|
comment:2 by , 17 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 17 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 17 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 thePlace
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 ofPlace
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 , 17 months ago
Patch needs improvement: | unset |
---|
Updated the PR with possible solution 2 on comment
follow-ups: 11 12 14 comment:10 by , 17 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): 110 110 customers = models.ManyToManyField(Restaurant, related_name="provider") 111 111 112 112 113 class Foo(Supplier, Restaurant): 114 pass 115 113 116 class CustomSupplier(Supplier): 114 117 pass 115 118
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): 106 106 serves_gnocchi = models.BooleanField(default=False) 107 107 108 108 109 class ItalianRestaurantManyParents(ItalianRestaurant, Place): 110 place_two_ptr = models.OneToOneField( 111 Place, on_delete=models.CASCADE, parent_link=True 112 ) 113 114 109 115 class Supplier(Place): 110 116 customers = models.ManyToManyField(Restaurant, related_name="provider") 111 117 -
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 25 25 Supplier, 26 26 Title, 27 27 Worker, 28 ItalianRestaurantManyParents, 28 29 ) 29 30 30 31 … … def test_custompk_m2m(self): 149 150 # accidentally found). 150 151 self.assertSequenceEqual(s.titles.all(), []) 151 152 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 152 160 def test_update_parent_filtering(self): 153 161 """ 154 162 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)
comment:11 by , 17 months ago
Replying to Simon Charette:
It feels like
ItalianRestaurantManyParents
should not be allowed to be defined in the first place sinceplace_ptr
conflicts.
Agreed
comment:12 by , 17 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 , 17 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.
comment:14 by , 17 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.
comment:15 by , 17 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( 864 864 reverse=True, 865 865 include_parents=True, 866 866 include_hidden=False, 867 seen_models=None,867 topmost_call=True, 868 868 ): 869 869 """ 870 870 Internal helper function to return fields of the model. … … def _get_fields( 885 885 # implementation and to provide a fast way for Django's internals to 886 886 # access specific subsets of fields. 887 887 888 # We must keep track of which models we have already seen. Otherwise we889 # could include the same field multiple times from different models.890 topmost_call = seen_models is None891 if topmost_call:892 seen_models = set()893 seen_models.add(self.model)894 895 888 # Creates a cache key composed of all arguments 896 889 cache_key = (forward, reverse, include_parents, include_hidden, topmost_call) 897 890 … … def _get_fields( 906 899 # Recursively call _get_fields() on each parent, with the same 907 900 # options provided in this call. 908 901 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() 909 906 for parent in self.parents: 910 # In diamond inheritance it is possible that we see the same911 # model from two different routes. In that case, avoid adding912 # fields from the same parent again.913 if parent in seen_models:914 continue915 907 if ( 916 908 parent._meta.concrete_model != self.concrete_model 917 909 and include_parents == PROXY_PARENTS … … def _get_fields( 922 914 reverse=reverse, 923 915 include_parents=include_parents, 924 916 include_hidden=include_hidden, 925 seen_models=seen_models,917 topmost_call=False, 926 918 ): 927 919 if ( 928 920 not getattr(obj, "parent_link", False) 929 921 or obj.model == self.concrete_model 930 ) :922 ) and obj not in parent_fields: 931 923 fields.append(obj) 924 parent_fields.add(obj) 932 925 if reverse and not self.proxy: 933 926 # Tree is computed once and cached until the app cache is expired. 934 927 # 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
- Wait for #33414 to be merged as it affects the number of queries performed on creation
- Adjust the system check that detects field collisions to catch the case in the initial report (first commit)
- 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?
comment:17 by , 17 months ago
Patch needs improvement: | unset |
---|
That makes sense. Updated the patch accordingly.
comment:18 by , 17 months ago
Patch needs improvement: | set |
---|
comment:19 by , 17 months ago
Please share your thoughts about the following comment. I will update the patch accordingly.
comment:20 by , 17 months ago
Patch needs improvement: | unset |
---|
comment:21 by , 17 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR