Opened 5 months ago

Last modified 3 weeks ago

#31382 assigned Bug

Silent failure when saving non-concrete field with update_fields

Reported by: Peter Law Owned by: Pat Garcia
Component: Database layer (models, ORM) Version: master
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

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 (5)

comment:1 Changed 5 months ago 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.

comment:2 Changed 5 months ago by Simon Charette

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.

comment:3 in reply to:  description Changed 4 months ago by Sanskar Jaiswal

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 Changed 5 weeks ago by Pat Garcia

Owner: changed from nobody to Pat Garcia
Status: newassigned

comment:5 Changed 3 weeks ago by Pat Garcia

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

Note: See TracTickets for help on using tickets.
Back to Top