Fixed #36481 -- Fixed QuerySet.update concrete fields check. #19595

rpkilby · 2025-06-26T06:04:55Z

Trac ticket number

ticket-36481

Branch description

Fixed QuerySet.update concrete fields check.

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

nessita

Thank you @rpkilby for this PR, this is an interesting find! 🌟

I think this could be simplified further, since, assuming A = field.auto_created and B = field.concrete, using DeMorgan Law:

direct = not (A and not B) or not B
direct = not A or not (not B) or not B
direct = not A or B or not B
direct = not A or True
direct = True

So then:

if not direct or (field.is_relation and field.many_to_many):

Becomes if not True or... which is if False or... which is:

if field.is_relation and field.many_to_many:

Wanna push that change?

nessita · 2025-06-26T16:48:46Z

@charettes am I missing something??? 🤯

rpkilby · 2025-06-26T17:23:58Z

@nessita There is some context in the attached ticket, but I think it would be valuable to keep the not field.concrete check. In my case, I am subclassing ForeignObject to implement .select_related(...) joins against correlated subqueries, and this would ensure that a FieldError is appropriately raised when calling update against it. I assume this would be helpful for other uses of ForeignObject, where it would not be appropriate to allow an update.

nessita · 2025-06-26T17:41:10Z

@nessita There is some context in the attached ticket, but I think it would be valuable to keep the not field.concrete check. In my case, I am subclassing ForeignObject to implement .select_related(...) joins against correlated subqueries, and this would ensure that a FieldError is appropriately raised when calling update against it. I assume this would be helpful for other uses of ForeignObject, where it would not be appropriate to allow an update.

Thank you @rpkilby! I did read the ticket, but I still think there are two things in this PR:

Boolean logic simplification: direct can be reduced and removed, as mentioned in my previous comment, since we can prove it's equivalent using boolean algebra.
Maybe a new feature? The change in add_update_values, based on the new test, looks like it slightly changes the logic consequently changing the current behavior. So I think we need to consider that as well. Could this be seen as a breaking change? (honest question)

rpkilby · 2025-06-27T07:41:55Z

Hi @nessita. I'd argue the two items are related and that this is more of a bugfix. imo this change reflects the intent of the original code, and direct being reducable is only due to the faulty logic, which this would fix.

As to the changed test method, composite keys are a new feature as of 5.2, and ForeignObject is documented as being an internal API. I think raising a different exception is reasonably a bugfix. Additionally, .update() isn't tested against a saved instance, which fails with AttributeError: 'ForeignObjectRel' object has no attribute 'get_related_field'. Raising FieldError: Cannot update model field <django.db.models.fields.related.ForeignObject: user> (only non-relations and foreign keys permitted) seems more correct and consistent than the assertion/value errors. That said, I don't know if there are other non-concrete fields that this change would incorrectly impact.

Updated the test to include both saved and unsaved instances.

charettes

imo this change reflects the intent of the original code, and direct being reducable is only due to the faulty logic, which this would fix.

I agree with this conclusion. The logic in add_update_values is there to ensure that only fields backed by actual columns can be updated against as that's a requirement of UPDATE.

Since Field.concrete basically means column is not None only checking against it should be enough? That fact we need the or (field.is_relation and field.many_to_many) part in the first place is likely due to ManyToManyField lacking a proper get_attname_column override.

    def get_attname_column(self):
        attname, column = super().get_attname_column()
        return attname, None

rpkilby · 2025-06-27T19:21:10Z

@charettes should I commit an update with the following changes:

diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py
index bad71a5fd6..e97ecf8a0d 100644
--- a/django/db/models/fields/related.py
+++ b/django/db/models/fields/related.py
@@ -1844,6 +1844,10 @@ class ManyToManyField(RelatedField):
             )
         return name, path, args, kwargs
 
+    def get_attname_column(self):
+        attname, column = super().get_attname_column()
+        return attname, None
+
     def _get_path_info(self, direct=False, filtered_relation=None):
         """Called by both direct and indirect m2m traversal."""
         int_model = self.remote_field.through
diff --git a/django/db/models/sql/subqueries.py b/django/db/models/sql/subqueries.py
index d0e0f82cd5..86f12f95ea 100644
--- a/django/db/models/sql/subqueries.py
+++ b/django/db/models/sql/subqueries.py
@@ -91,10 +91,10 @@ class UpdateQuery(Query):
                 raise FieldError(
                     "Composite primary key fields must be updated individually."
                 )
-            if not field.concrete or (field.is_relation and field.many_to_many):
+            if not field.concrete:
                 raise FieldError(
-                    "Cannot update model field %r (only non-relations and "
-                    "foreign keys permitted)." % field
+                    "Cannot update model field %r (only concrete fields are permitted)."
+                    % field
                 )
             if model is not self.get_meta().concrete_model:
                 self.add_related_update(model, field, val)
diff --git a/tests/update/tests.py b/tests/update/tests.py
index bb83440008..e17d906c80 100644
--- a/tests/update/tests.py
+++ b/tests/update/tests.py
@@ -160,7 +160,7 @@ class AdvancedTests(TestCase):
         msg = (
             "Cannot update model field "
             "<django.db.models.fields.related.ManyToManyField: m2m_foo> "
-            "(only non-relations and foreign keys permitted)."
+            "(only concrete fields are permitted)."
         )
         with self.assertRaisesMessage(FieldError, msg):
             Bar.objects.update(m2m_foo="whatever")

charettes · 2025-06-27T19:23:10Z

@rpkilby no lets keep this work focused on dealing with ForeignObject please. It's possible that implementing ManyToManyField.get_attname_column cause other breakages so it should be investigate separately.

rpkilby · 2025-06-27T19:24:29Z

Thanks - let me know if there's anything else I can to do to help wrap this up.

rpkilby · 2025-06-27T19:48:39Z

It's possible that implementing ManyToManyField.get_attname_column cause other breakages so it should be investigate separately.

I've reopened https://code.djangoproject.com/ticket/35453, which discusses this question.

rpkilby · 2025-08-30T17:18:31Z

Hi @charettes @nessita - anything I can do to move this forward? Would be great to see this released as a bugfix in 5.2.x

charettes · 2025-09-02T01:44:33Z

@rpkilby you'll want to unset patch needs improvement on the ticket to make sure it appears in the review queue.

It's unlikely to be backported to 5.2.x as it doesn't qualify per our policy but it might make the cut for 6.0 feature freeze which is around the corner.

jacobtylerwalls

Thanks @rpkilby, this looks great.

I verified that in addition to ForeignObject, the reverse side of a OneToOneField behaves the same way, failing now with this better error rather than AttributeError.

Could you add a test case for OneToOneField? Something like:

diff --git a/tests/update/models.py b/tests/update/models.py
index d71fc887c7..83c289a5e7 100644
--- a/tests/update/models.py
+++ b/tests/update/models.py
@@ -50,4 +50,6 @@ class UniqueNumber(models.Model):
 
 
 class UniqueNumberChild(UniqueNumber):
-    pass
+    parent = models.OneToOneField(
+        null=True, to=UniqueNumber, on_delete=models.CASCADE, related_name="child"
+    )
diff --git a/tests/update/tests.py b/tests/update/tests.py
index bb83440008..6a85c992f0 100644
--- a/tests/update/tests.py
+++ b/tests/update/tests.py
@@ -165,6 +165,14 @@ class AdvancedTests(TestCase):
         with self.assertRaisesMessage(FieldError, msg):
             Bar.objects.update(m2m_foo="whatever")
 
+    def test_update_o2o_field(self):
+        msg = (
+            "Cannot update model field <OneToOneRel: update.uniquenumberchild>"
+            " (only non-relations and foreign keys permitted)."
+        )
+        with self.assertRaisesMessage(FieldError, msg):
+            UniqueNumber.objects.update(child="whatever")
+
     def test_update_transformed_field(self):
         A.objects.create(x=5)
         A.objects.create(x=-6)

jacobtylerwalls · 2025-09-02T20:36:33Z

(I'm also happy to push this myself in the next few days, as the PR is already ready for merge otherwise.)

rpkilby · 2025-09-03T15:32:04Z

Thanks for the review @jacobtylerwalls, I can add this test case later today.

rpkilby · 2025-09-03T22:08:32Z

Hi @jacobtylerwalls, I added an o2o field between update.Foo and update.Bar, then tested the reverse descriptors for the fk, o2o, and m2m fields, as well as the existing reverse parent_link descriptor between UniqueNumber and UniqueNumberChild. Does that all look good? idk if some of these would be considered redundant.

…relations.

FieldError is now emitted for invalid update calls involving reverse relations, where previously they failed with AttributeError.

jacobtylerwalls

Wonderful, thanks for the updates!

nessita reviewed Jun 26, 2025

View reviewed changes

rpkilby force-pushed the ticket-36481 branch from 420d86b to 2c252ca Compare June 27, 2025 06:35

charettes approved these changes Jun 27, 2025

View reviewed changes

rpkilby mentioned this pull request Jun 27, 2025

Fixed #35453 -- ManyToManyField no longer flagged as a concrete field. #19602

Draft

6 tasks

jacobtylerwalls reviewed Sep 2, 2025

View reviewed changes

rpkilby force-pushed the ticket-36481 branch from c9af049 to 96d4de2 Compare September 3, 2025 22:10

rpkilby added 2 commits September 4, 2025 09:28

Refs #36481 -- Improved test coverage for invalid updates on reverse …

54fac21

…relations.

Fixed #36481 -- Fixed QuerySet.update concrete fields check.

a69a9f1

FieldError is now emitted for invalid update calls involving reverse relations, where previously they failed with AttributeError.

jacobtylerwalls force-pushed the ticket-36481 branch from 96d4de2 to a69a9f1 Compare September 4, 2025 13:35

jacobtylerwalls approved these changes Sep 4, 2025

View reviewed changes

jacobtylerwalls changed the title ~~Refs #36481 -- Fixed QuerySet.update concrete fields check.~~ Fixed #36481 -- Fixed QuerySet.update concrete fields check. Sep 4, 2025

jacobtylerwalls merged commit bad03eb into django:main Sep 4, 2025
34 checks passed

rpkilby deleted the ticket-36481 branch September 5, 2025 01:55

Uh oh!

Fixed #36481 -- Fixed QuerySet.update concrete fields check. #19595

Fixed #36481 -- Fixed QuerySet.update concrete fields check. #19595

Conversation

rpkilby commented Jun 26, 2025 • edited by jacobtylerwalls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trac ticket number

Branch description

Checklist

Uh oh!

nessita left a comment

Choose a reason for hiding this comment

Uh oh!

nessita commented Jun 26, 2025

Uh oh!

rpkilby commented Jun 26, 2025

Uh oh!

nessita commented Jun 26, 2025

Uh oh!

rpkilby commented Jun 27, 2025

Uh oh!

charettes left a comment

Choose a reason for hiding this comment

Uh oh!

rpkilby commented Jun 27, 2025

Uh oh!

charettes commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpkilby commented Jun 27, 2025

Uh oh!

rpkilby commented Jun 27, 2025

Uh oh!

rpkilby commented Aug 30, 2025

Uh oh!

charettes commented Sep 2, 2025

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

jacobtylerwalls commented Sep 2, 2025

Uh oh!

rpkilby commented Sep 3, 2025

Uh oh!

rpkilby commented Sep 3, 2025

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rpkilby commented Jun 26, 2025 •

edited by jacobtylerwalls

Loading

charettes commented Jun 27, 2025 •

edited

Loading