Opened 31 hours ago
Closed 8 hours ago
#36093 closed Bug (fixed)
Calling full_clean() on an existing child model instance in a multi-table inheritance should not execute a database query.
Reported by: | Sage Abdullah | Owned by: | Simon Charette |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | 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 )
Regression in bf7b17d16d3978b2e1cee4a0f7ce8840bd1a8dc4.
The following test now fails after the above commit:
diff --git a/tests/model_inheritance/tests.py b/tests/model_inheritance/tests.py index 6b005fcef0..28d03e1687 100644 --- a/tests/model_inheritance/tests.py +++ b/tests/model_inheritance/tests.py @@ -343,6 +343,12 @@ class ModelInheritanceTests(TestCase): self.assertEqual(type(MethodOverride.foo), DeferredAttribute) + def test_full_clean(self): + restaurant = Restaurant.objects.create() + with self.assertNumQueries(0): + with self.assertRaises(ValidationError): + restaurant.full_clean() + class ModelInheritanceDataTests(TestCase): @classmethod
This is because the primary key of a child model is a OneToOneField
to the parent model (e.g. place_ptr
), and it's not in pk_fields
, so the changes to the check for skipping primary key when editing no longer passes for child models.
The exact query can be seen in my repro example.
Change History (10)
comment:1 by , 31 hours ago
comment:2 by , 31 hours ago
Description: | modified (diff) |
---|---|
Summary: | Calling `full_clean()` on an existing child model instance in a multi-table inheritance should not execute a database query. → Calling full_clean() on an existing child model instance in a multi-table inheritance should not execute a database query. |
follow-up: 4 comment:3 by , 31 hours ago
This is because the primary key of a child model is a
OneToOneField
to the parent model (e.g.place_ptr
), and it's not inpk_fields
If that's truly the case it is a bug in pk_fields
itself.
The one-to-one parent link seems to be in pk_fields
though?
>>> Restaurant._meta.pk_fields [<django.db.models.fields.related.OneToOneField: place_ptr>]
comment:4 by , 31 hours ago
Replying to Simon Charette:
If that's truly the case it is a bug in
pk_fields
itself.
The one-to-one parent link seems to be in
pk_fields
though?
>>> Restaurant._meta.pk_fields [<django.db.models.fields.related.OneToOneField: place_ptr>]
Oops, yep, sorry – I was in a rush!
The OneToOneField
is there, but the real PK in the parent model isn't. Not sure how it was before, but _get_unique_checks
returns both the OneToOneField
and the parent pk.
comment:5 by , 31 hours ago
Triage Stage: | Unreviewed → Accepted |
---|
Ahh I see. We should retrieve the pk_fields
of all bases involved.
follow-up: 7 comment:6 by , 31 hours ago
Does the following resolves the issue?
-
django/db/models/base.py
diff --git a/django/db/models/base.py b/django/db/models/base.py index a4d5a0d553..0f07d235a9 100644
a b def _perform_unique_checks(self, unique_checks): 1493 1493 ): 1494 1494 # no value, skip the lookup 1495 1495 continue 1496 if f in self._meta.pk_fields and not self._state.adding:1496 if f in model_class._meta.pk_fields and not self._state.adding: 1497 1497 # no need to check for unique primary key when editing 1498 1498 continue 1499 1499 lookup_kwargs[str(field_name)] = lookup_value
comment:7 by , 31 hours ago
comment:8 by , 28 hours ago
Has patch: | set |
---|---|
Owner: | set to |
Status: | new → assigned |
comment:9 by , 8 hours ago
Triage Stage: | Accepted → Ready for checkin |
---|
I have a sneaking suspicion that the problem may also manifest in the other areas affected by the commit, e.g.
bulk_update
. I have yet to confirm it, though.If handling this requires some additional logic to check whether a field is a primary key beyond checking its existence in
_meta.pk_fields
, we might need to have a dedicated method for it and use it everywhere.