[Django] #31382: Silent failure when saving non-concrete field with update_fields
Django
-----------------------------------------+------------------------
Reporter: Peter Law | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
If you have a non-concrete field which wraps a couple of concrete fields
(a bit like `GenericForeignKey`, though my codebase doesn't uses this for
compound data types rather than relations), then when you pass the name of
that non-concrete field to
`save(update_fields=('my_non_concrete_field',))`, the model will be saved
'''without''' saving that field and yet no error will be emitted.
I think that this could be because the check for the validity of the
`update_fields` is done against `meta.fields`
(https://github.com/django/django/blob/5c8441a0b8787c14b45fb761550571baea48604e/django/db/models/base.py#L714-L737)
while the later check for which fields to actually save is done using
`meta.local_concrete_fields`
(https://github.com/django/django/blob/5c8441a0b8787c14b45fb761550571baea48604e/django/db/models/base.py#L838-L844).
Ideally, I would like a way for a non-concrete field to be able to specify
the underlying concrete fields which should be saved on its behalf. That's
clearly rather complex though (and would likely have impacts beyond just
`update_fields`) -- a simpler solution would be to error about this case
so that the developer knows something is amis.
--
Ticket URL: <https://code.djangoproject.com/ticket/31382>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Peter Law | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
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 Peter Law):
Just to add for anyone who has a similar issue -- I'm currently working
around this by having my field listen for the `pre_save` signal as part of
its `contribute_to_class` method and having the signal emit an exception
which prevents the save going through.
--
Ticket URL: <https://code.djangoproject.com/ticket/31382#comment:1>
Django
Reporter: Peter Law | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* type: Uncategorized => Bug
* version: 2.2 => master
* component: Uncategorized => Database layer (models, ORM)
* stage: Unreviewed => Accepted
Comment:
> Ideally, I would like a way for a non-concrete field to be able to
specify the underlying concrete fields which should be saved on its
behalf. That's clearly rather complex though (and would likely have
impacts beyond just update_fields) -- a simpler solution would be to error
about this case so that the developer knows something is amis.
Completely agree with this premise, let's focus this ticket on addressing
the concrete field discrepancy. A later improvement could be to allow
specifying ''virtual'' update fields as aliases to the field they manage
but that should be tackled in another ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/31382#comment:2>
Django
Reporter: Peter Law | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
Replying to [ticket:31382 Peter Law]:
> If you have a non-concrete field which wraps a couple of concrete fields
(a bit like `GenericForeignKey`, though my codebase doesn't uses this for
compound data types rather than relations), then when you pass the name of
that non-concrete field to
`save(update_fields=('my_non_concrete_field',))`, the model will be saved
'''without''' saving that field and yet no error will be emitted.
Hey Peter,
Could you kindly provide a minimal code sample so that I can successfully
reproduce this error on my machine?
Thanks
Sanskar
--
Ticket URL: <https://code.djangoproject.com/ticket/31382#comment:3>
Django
Reporter: Peter Law | Owner: patgarcia
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by patgarcia):
* owner: nobody => patgarcia
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31382#comment:4>
Django
Reporter: Peter Law | Owner: Pat
| Garcia
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Pat Garcia):
Started working on a fix, should have a PR ready soonish
--
Ticket URL: <https://code.djangoproject.com/ticket/31382#comment:5>
Django
Reporter: Peter Law | Owner: Pat
| Garcia
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/13295 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31382#comment:6>
Django
Reporter: Peter Law | Owner: Pat
| Garcia
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8954f255bbf5f4ee997fd6de62cb50fc9b5dd697" 8954f255]:
{{{
#!CommitTicketReference repository=""
revision="8954f255bbf5f4ee997fd6de62cb50fc9b5dd697"
Fixed #31382 -- Made Model.save(update_fields=...) raise ValueError on
non-concrete fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31382#comment:7>