update_or_create() not saving data assigned in a model's save() method

[Phil Gyford]

In the current main branch (testing from commit 37c5b8c) it seems that a model's field assigned in its custom save() method isn't being saved to the database.

I have this model, which should set nickname to the value of name on save:

from django.db import models

class Person(models.Model):

    name = models.CharField(blank=True, max_length=255)

    nickname = models.CharField(blank=True, max_length=255)

    def save(self, *args, **kwargs):
        self.nickname = self.name
        super().save(*args, **kwargs)

And here's a test which should pass, and does pass with Django 4.1:

from django.test import TestCase
from .models import Person

class MyTestCase(TestCase):
    def test_name(self):
        person = Person.objects.create(name="Bob")

        self.assertEqual(person.nickname, "Bob")

        updated_person, created = Person.objects.update_or_create(
            id=person.id, defaults={"name": "Terry"}
        )
        updated_person.refresh_from_db()

        self.assertEqual(updated_person.name, "Terry")
        self.assertEqual(updated_person.nickname, "Terry")  # Fails here

But it fails:

➜  ./manage.py test
Found 1 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
F
======================================================================
FAIL: test_name (myapp.tests.MyTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/phil/Projects/personal/django-tester/myproject/myapp/tests.py", line 16, in test_name
    self.assertEqual(updated_person.nickname, "Terry")
AssertionError: 'Bob' != 'Terry'
- Bob
+ Terry

If the line updated_person.refresh_from_db() is removed in the test, the test passes.

So it seems that update_or_create() is returning the correctly updated object, but for some reason it's not being saved to the database.

I'm using SQLite for this test, although I first noticed the problem in a project using Postgresql.

[Mariusz Felisiak]

Thanks for the report! Unfortunately, this can be really tricky to fix. You can fix this locally by updating update_fields (if needed), e.g.

    def save(self, *args, **kwargs):
        if "update_fields" in kwargs:
            kwargs["update_fields"].add("nickname")
        self.nickname = self.name
        super().save(*args, **kwargs)

Regression in 6cc0f22a73970dd7c0d29d4d8d2ff9e1cc862b30.

[Simon Charette]

I would argue that this is the responsibility of the user to properly account for update_fields when doing field assignments in a save() override.

The issue is triggered by the change in the update_or_create implementation but it's also broken when update_fields is used directly

person = Person.objects.create(name="foo")
person.name = "bar"
person.save(update_fields={"name"}))
person.refresh_from_db()
assert person.nickname == "bar"  # AssertionError

I feels like the person.nickbar assignment should also augment update_fields if provided

self.nickname = name
if update_fields is not None and "name" in update_fields:
    update_fields = set(update_fields) | {"nickname"}

[Sarah Boyce]

I guess we would also have this problem if any fields are set on a pre_save signal.

Adding to the update_fields when overwriting a model save is good practise but I admit I have never thought about it, maybe we should add a note/example here: ​https://docs.djangoproject.com/en/4.1/topics/db/models/#overriding-predefined-model-methods

If we chose to keep the commit, maybe we add update_fields in the boiler plate code here: ​https://docs.djangoproject.com/en/4.1/ref/models/querysets/#update-or-create
I am also ok with dropping the commit.

[Phil Gyford]

I must admit that in all the years I've been overriding save() methods and using update_or_create() without any problems (and checking the docs a lot because my memory is terrible) I've never even been aware of update_fields.

I guess I'm not alone – I just looked through my first 30 google results for "django override save site:stackoverflow.com" and only one of them used update_fields, and that was a question explicitly about it. (Not all of the results were about updating the model's own fields but that seems the most common use case.)

So, at a bare minimum, updating the docs for those two methods would be great, as Sarah Boyce suggests.

[Sarah Boyce]

I gave updating the docs with an example a go as it might still be valuable even if we drop the commit: ​https://github.com/django/django/pull/16186

[Mariusz Felisiak]

I agree with Simon, we should revert the commit and closed #32095 as "wontfix".

[Florian Apolloner]

I agree with Simon, we should revert the commit and closed #32095 as "wontfix".

I also agree with Simon but am not really convinced we should revert the commit. I agree that the docs are somewhat unfortunate in the sense that the boiler plate code does not use update_fields to only update fields from defaults. But aside from that nothing in update_or_create suggest that all fields would (or should) get saved. It actually says:

The defaults is a dictionary of (field, value) pairs used to update the object.

This could in the end also be done via qs.update() instead of qs.save(). So the main question becomes: Is the behavior that people relied on an implementation detail on which the relied erroneously? Can we deal with the fallout? If the answer to the latter is no, can we somehow salvage this situation? Using update_fields is really preferred especially in cases with concurrent access.

[Mariusz Felisiak]

OK, I misunderstood Simon. It seems that doc adjustments are the best way to move this forward.

[Florian Apolloner]

Thinking about this more I am wondering if we have to go through a deprecation path somehow for this feature/fix. The issue here is that overriding save and setting attributes is not reflected in the database which can be argued to be a dataloss bug. To be 100% clear on this: I actually do think that the new code is correct and it is up to the user to properly implement save. But since update_fields was not so important in the past I can imagine users not implementing it leading to bugs that are not discovered for a while etc… So I think this should be (it already is) a release blocker and should not downgraded to a normal bug. The only backwards compat fix I can envision is to hide this fix behind a setting which users need to turn to True + a backwards incompatible note in the release notes. This should pick up everyone reading the release notes or at least running their tests with warnings enabled. Another option would be to detect if save is overridden or if signals are attached to the model and act accordingly during the "deprecation".

[Simon Charette]

But since update_fields was not so important in the past I can imagine users not implementing it leading to bugs that are not discovered for a while etc…

I would argue that it was always important even before the update_or_create changes reliance was introduced.

For example, when partially fetching model instance fields (e.g. using only) then ​only the fetched fields will be updated which ​is documented.

If we believe that documenting how to properly override Model.save or deal with post_save signal wrt/ to update_fields is not enough the only deprecation path I can think of that doesn't involve adding a setting is to add a temporary __save_update_fields=False kwarg to update_or_create and when __save_update_fields is False and save() is overridden or post_save signals are connected emit a deprecation warning and don't pass update_fields. That would allow callers to pass __save_update_fields=True to opt-in the new behaviour and silence the warning during the deprecation period.

This all seems like a lot of work for little benefit to me though given we didn't take this extra precautions when introducing update_fields in the first place in Django 1.5 and code that fails to account for this feature has been subtly broken since then.

[Florian Apolloner]

Good point about only, that makes me feel a bit better about a documentation only patch. I think it would still be worth to call out in the release notes as a possibly backwards incompatible change?

[Simon Charette]

I think it would still be worth to call out in the release notes as a possibly backwards incompatible change?

Agreed. Ideally we'd have examples on how to properly override save() as well but a release note and maybe an admonition to the update-or-create / versionchanged could be valuable?

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In cedf1be:

[4.1.x] Refs #34099 -- Doc'd that custom Model.save() should update update_fields kwarg.

Backport of 0678d657222dd667bcc7e4fc307ea2ab70d219d7 from main

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0678d65:

Refs #34099 -- Doc'd that custom Model.save() should update update_fields kwarg.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 7a53079:

Fixed #34099 -- Added release notes for QuerySet.update_or_create() changes.

Follow up to 6cc0f22a73970dd7c0d29d4d8d2ff9e1cc862b30.

Thanks Phil Gyford for the report.