Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#31382 closed Bug (fixed)

Silent failure when saving non-concrete field with update_fields

Reported by: Peter Law Owned by: Pat Garcia
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (8)

comment:1 by Peter Law, 4 years ago

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.

comment:2 by Simon Charette, 4 years ago

Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug
Version: 2.2master

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.

in reply to:  description comment:3 by Sanskar Jaiswal, 4 years ago

Replying to 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

comment:4 by Pat Garcia, 4 years ago

Owner: changed from nobody to Pat Garcia
Status: newassigned

comment:5 by Pat Garcia, 4 years ago

Started working on a fix, should have a PR ready soonish

comment:6 by Mariusz Felisiak, 4 years ago

Has patch: set

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 8954f255:

Fixed #31382 -- Made Model.save(update_fields=...) raise ValueError on non-concrete fields.

comment:8 by Florian Apolloner, 3 years ago

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.

Did anyone create a ticket for the complex approach?

Last edited 3 years ago by Florian Apolloner (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top