Silent failure when saving non-concrete field with update_fields

[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.

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.

[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.

[Simon Charette]

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.

[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

[Pat Garcia]

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

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8954f255:

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

[Florian Apolloner]

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?