[Django] #33091: A FieldError should be raised when trying to update MTI inherited field with F reference to child field
Django
with F reference to child field
-------------------------------------+-------------------------------------
Reporter: Shai | Owner: nobody
Berger |
Type: Bug | Status: new
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This is the converse of #30044.
In that bug, we made sure a proper error was raised when we do
{{{#!python
ChildModel.objects.update(child_field=F('parent_field'))
}}}
We should similarly make sure a proper error is raised when we do
{{{#!python
ChildModel.objects.update(parent_field=F('child_field'))
}}}
With current main, if we add the obvious test
{{{#!diff
diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py
index 33c5189390..0a97d537fe 100644
--- a/tests/expressions/tests.py
+++ b/tests/expressions/tests.py
@@ -318,6 +318,11 @@ class BasicExpressionsTests(TestCase):
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"
----------------------------------------------------------------------
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33091>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Abhijith Ganesh):
Was this bug patched, I would like to work on this bug and patch it. Would
that be possible @Shai Berger
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:1>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Shai, thanks for the report.
Abhijith, feel-free to prepare a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:2>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:3>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Ganesh
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Abhijith Ganesh
* status: new => assigned
Comment:
I am self assigning myself for this issue to avoid confusion.
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:4>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Ganesh
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:5>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Ganesh
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Abhijith Ganesh):
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:6>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Ganesh
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:7>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Ganesh
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Bhuvnesh):
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
[https://github.com/django/django/blob/9da2210f121085e998dc7cedb783bc4568390b92/django/db/models/sql/query.py#L1631
this part] ,throwing different field error
[https://github.com/django/django/blob/9da2210f121085e998dc7cedb783bc4568390b92/django/db/models/sql/query.py#L1714
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.}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:8>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Ganesh
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Shai Berger):
Replying to [comment:8 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
[https://github.com/django/django/blob/9da2210f121085e998dc7cedb783bc4568390b92/django/db/models/sql/query.py#L1631
this part] ,throwing different field error
[https://github.com/django/django/blob/9da2210f121085e998dc7cedb783bc4568390b92/django/db/models/sql/query.py#L1714
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:9>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Gama
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Abhijith Ganesh => Clifford Gama
Comment:
I wonder if it would be acceptable to validate the expression references
in `UpdateQuery.add_update_values()` with something like:
{{{#!diff
diff --git a/django/db/models/options.py b/django/db/models/options.py
index 11b2742f7d..8457d74ff9 100644
--- a/django/db/models/options.py
+++ b/django/db/models/options.py
@@ -1018,6 +1018,18 @@ class Options:
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):
"""
diff --git a/django/db/models/sql/subqueries.py
b/django/db/models/sql/subqueries.py
index 9cb971b38f..7f6d06ee1f 100644
--- a/django/db/models/sql/subqueries.py
+++ b/django/db/models/sql/subqueries.py
@@ -99,6 +99,12 @@ class UpdateQuery(Query):
"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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:10>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Gama
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
{{{#!python
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 ...`
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:11>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Gama
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Replying to [comment:10 Clifford Gama]:
> {{{#!diff
b/django/db/models/sql/subqueries.py
> index 9cb971b38f..7f6d06ee1f 100644
> --- a/django/db/models/sql/subqueries.py
> +++ b/django/db/models/sql/subqueries.py
> @@ -99,6 +99,12 @@ class UpdateQuery(Query):
> "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
> }}}
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:12>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Gama
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Thank you. I will prepare a patch in the following few days
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:13>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Gama
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I have opened a [https://github.com/django/django/pull/19363 PR], but I am
a bit worried about the performance implications.
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:14>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Gama
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:15>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
* owner: Clifford Gama => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:16>
Django
with F reference to child field
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Clifford Gama (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33091#comment:17>