Opened 3 years ago
Last modified 2 years ago
#33091 assigned Cleanup/optimization
A FieldError should be raised when trying to update MTI inherited field with F reference to child field
Reported by: | Shai Berger | Owned by: | Abhijith Ganesh |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py index 33c5189390..0a97d537fe 100644
a b class BasicExpressionsTests(TestCase): 318 318 with self.assertRaisesMessage(FieldError, msg): 319 319 RemoteEmployee.objects.update(adjusted_salary=F('salary') * 5) 320 320 321 def test_update_set_inherited_field_from_child(self): 322 msg = 'Joined field references are not permitted in this query' 323 with self.assertRaisesMessage(FieldError, msg): 324 RemoteEmployee.objects.update(salary=F('adjusted_salary') / 5) 325 321 326 def test_object_update_unsaved_objects(self): 322 327 # F expressions cannot be used to update attributes on objects which do 323 328 # 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" ----------------------------------------------------------------------
Change History (9)
comment:1 by , 3 years ago
comment:2 by , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Bug → Cleanup/optimization |
Shai, thanks for the report.
Abhijith, feel-free to prepare a patch.
comment:3 by , 3 years ago
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?
comment:4 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I am self assigning myself for this issue to avoid confusion.
comment:5 by , 3 years ago
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
comment:6 by , 3 years ago
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.
comment:7 by , 3 years ago
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).
follow-up: 9 comment:8 by , 2 years ago
I think there is no direct way to get child field from opts of parent model in inheritance. 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.
comment:9 by , 2 years ago
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.
Was this bug patched, I would like to work on this bug and patch it. Would that be possible @Shai Berger