Opened 31 hours ago

Closed 7 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 Sage Abdullah)

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 Sage Abdullah, 30 hours ago

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.

comment:2 by Sage Abdullah, 30 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.

comment:3 by Simon Charette, 30 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 in pk_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>]
Last edited 30 hours ago by Simon Charette (previous) (diff)

in reply to:  3 comment:4 by Sage Abdullah, 30 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.

Version 0, edited 30 hours ago by Sage Abdullah (next)

comment:5 by Simon Charette, 30 hours ago

Triage Stage: UnreviewedAccepted

Ahh I see. We should retrieve the pk_fields of all bases involved.

comment:6 by Simon Charette, 30 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):  
    14931493                ):
    14941494                    # no value, skip the lookup
    14951495                    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:
    14971497                    # no need to check for unique primary key when editing
    14981498                    continue
    14991499                lookup_kwargs[str(field_name)] = lookup_value

in reply to:  6 comment:7 by Sage Abdullah, 30 hours ago

Replying to Simon Charette:

Does the following resolves the issue?

It does! Thanks!

comment:8 by Simon Charette, 28 hours ago

Has patch: set
Owner: set to Simon Charette
Status: newassigned

comment:9 by Sarah Boyce, 7 hours ago

Triage Stage: AcceptedReady for checkin

comment:10 by Sarah Boyce <42296566+sarahboyce@…>, 7 hours ago

Resolution: fixed
Status: assignedclosed

In 4bfec242:

Fixed #36093 -- Adjusted unique checks to account for inherited primary keys.

Regression in bf7b17d16d3978b2e1cee4a0f7ce8840bd1a8dc4 refs #36075.

Thanks Sage Abdullah for the report and tests.

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