A FieldError should be raised when trying to update MTI inherited field with F reference to child field

[Shai Berger]

This is the converse of #30044.

In that bug, we made sure a proper error was raised when we do

ChildModel.objects.update(child_field=F('parent_field'))

We should similarly make sure a proper error is raised when we do

ChildModel.objects.update(parent_field=F('child_field'))

With current main, if we add the obvious test

tests/expressions/tests.py

@@ -318 +318 @@
         with self.assertRaisesMessage(FieldError, msg):
             RemoteEmployee.objects.update(adjusted_salary=F('salary') * 5)
 
+    def test_update_set_inherited_field_from_child(self):
+        msg = 'Joined field references are not permitted in this query'
+        with self.assertRaisesMessage(FieldError, msg):
+            RemoteEmployee.objects.update(salary=F('adjusted_salary') / 5)
+
     def test_object_update_unsaved_objects(self):
         # F expressions cannot be used to update attributes on objects which do
         # not yet exist in the database

it fails

======================================================================
FAIL: test_update_set_inherited_field_from_child (expressions.tests.BasicExpressionsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib/python3.9/unittest/case.py", line 592, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/home/django/django/tests/expressions/tests.py", line 324, in test_update_set_inherited_field_from_child
    RemoteEmployee.objects.update(salary=F('adjusted_salary') / 5)
  File "/usr/lib/python3.9/contextlib.py", line 137, in __exit__
    self.gen.throw(typ, value, traceback)
  File "/home/django/django/django/test/testcases.py", line 687, in _assert_raises_or_warns_cm
    self.assertIn(expected_message, str(getattr(cm, cm_attr)))
  File "/usr/lib/python3.9/unittest/case.py", line 1096, in assertIn
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib/python3.9/unittest/case.py", line 668, in fail
    raise self.failureException(msg)
AssertionError: 'Joined field references are not permitted in this query' not found in "Cannot resolve keyword 'adjusted_salary' into field. Choices are: company_ceo_set, company_point_of_contact_set, firstname, id, lastname, manager, manager_id, remoteemployee, salary"

----------------------------------------------------------------------

[Abhijith Ganesh]

Was this bug patched, I would like to work on this bug and patch it. Would that be possible @Shai Berger

[Mariusz Felisiak]

Shai, thanks for the report.

Abhijith, feel-free to prepare a patch.

[Abhijith Ganesh]

This is my first attempt of patching an actual django bug, now once i have made the patch and pushed it to my repository, I should be creating a PR. Is that all?

[Abhijith Ganesh]

I am self assigning myself for this issue to avoid confusion.

[Abhijith Ganesh]

For fixing this I should be adding a conditional statement on query.py file right? and can I get some elaboration as to which method I should be resolving, I am under the assumption the same resolve method (function) will be getting a new conditional statement

[Abhijith Ganesh]

if annotation and not allow_joins:
          raise FieldError('Parent field references are not allowed in this query.')

Would this be a good fix?
This is my first fix and I am looking forward to learn, thanks in advance.

[Shai Berger]

Hi Abhijith, sorry for failing to respond earlier.

The best answer is, try: I've included in the ticket description a failing test for it. Add this test, and check if your suggested fix makes it pass (and doesn't break other tests).

[Bhuvnesh]

I think there is no direct way to get child field from opts of parent model in inheritance. For the testcase provided by shai the value of field is None at ​this part ,throwing different field error ​here. Is it the expected behaviour? or we are supposed to get the child field and later raise field error Joined field references are not permitted in this query.

[Shai Berger]

Replying to Bhuvnesh:

I think there is no direct way to get child field from opts of parent model in inheritance.

That makes sense

For the testcase provided by shai the value of field is None at ​this part ,throwing different field error ​here. Is it the expected behaviour?

No, that's the bug...

or we are supposed to get the child field and later raise field error Joined field references are not permitted in this query.

Yes, basically. Or, putting it another way (given your input): The update() logic uses the parent model opts, because the fields to be updated are all from that model; but then it uses the parent model to resolve all references, and that is wrong if there are expressions referencing the child model. Or, more succinctly: The move to the parent model is too early.

[Clifford Gama]

I wonder if it would be acceptable to validate the expression references in UpdateQuery.add_update_values() with something like:

django/db/models/options.py

@@ -1018 +1018 @@
                     names.append(field.attname)
         return frozenset(names)
 
+    @cached_property
+    def _local_concrete_field_names(self):
+        """
+        Return a set of the local concrete field names defined on the model.
+        """
+        names = []
+        for f in self.local_concrete_fields:
+            names.append(f.name)
+            if f.name != f.attname:
+                names.append(f.attname)
+        return frozenset(names)
+
     @cached_property
     def _reverse_one_to_one_field_names(self):
         """

django/db/models/sql/subqueries.py

@@ -99 +99 @@
                     "Cannot update model field %r (only non-relations and "
                     "foreign keys permitted)." % field
                 )
+            for field_name, *lookups in model._get_expr_references(val):
+                if field_name not in model._meta._local_concrete_field_names:
+                    raise FieldError(
+                        "Cannot update model field %r using the non-local field "
+                        "reference %r." % (field, field_name)
+                    )
             if model is not self.get_meta().concrete_model:
                 self.add_related_update(model, field, val)
                 continue

this would handle the case in this ticket and the one in #30044 without the need for special-casing either. This would make this ticket easier, by simplifying the logic since we don't have to check whether a descendant's reference is actually a field in the model originating the query update.

The downside is that this would also raise for invalid references, without listing the available field choices.

[Clifford Gama]

remote_employee.salary = F("incentive") * 5
remote_employee.save()

could also raise the same error. Currently it's FieldError: Cannot resolve ... into field ... The choices are ...

[Shai Berger]

Replying to Clifford Gama:

django/db/models/sql/subqueries.py

@@ -99 +99 @@
                     "Cannot update model field %r (only non-relations and "
                     "foreign keys permitted)." % field
                 )
+            for field_name, *lookups in model._get_expr_references(val):
+                if field_name not in model._meta._local_concrete_field_names:
+                    raise FieldError(
+                        "Cannot update model field %r using the non-local field "
+                        "reference %r." % (field, field_name)
+                    )
             if model is not self.get_meta().concrete_model:
                 self.add_related_update(model, field, val)
                 continue

I find the language "non-local field reference" a bit cryptic. It's only clear when you think in terms of _meta properties and functions, and it's too easy to code the error when not thinking about those at all.

It makes sense to me, to use something like your suggestion to easily find if there was an error. But in the case that an error is found, I'd prefer if we start looking for the specific cases, so we can give a more friendly error message.

[Clifford Gama]

Thank you. I will prepare a patch in the following few days