[Django] #34099: update_or_create() not saving data assigned in a model's save() method
Django
-------------------------------------+-------------------------------------
Reporter: Phil | Owner: nobody
Gyford |
Type: Bug | Status: new
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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:
{{{#!python
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:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34099>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Florian Apolloner, Sarah Boyce (added)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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.
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
{{{#!python
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
{{{#!python
self.nickname = name
if update_fields is not None and "name" in update_fields:
update_fields = set(update_fields) | {"nickname"}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
I agree with Simon, we should revert the commit and closed #32095 as
"wontfix".
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
OK, I misunderstood Simon. It seems that doc adjustments are the best way
to move this forward.
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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".
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
> 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
[https://github.com/django/django/blob/de2c2127b66e77a034c01c81753c5c08e651a5b4/django/db/models/base.py#L793-L802
only the fetched fields will be updated] which
[https://docs.djangoproject.com/en/4.1/ref/models/querysets/#django.db.models.query.QuerySet.defer
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.
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Wobrock (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:13>
Django
-------------------------------------+-------------------------------------
| Boyce
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Sarah Boyce
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* status: new => assigned
Comment:
[https://github.com/django/django/pull/16186 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0678d657222dd667bcc7e4fc307ea2ab70d219d7" 0678d65]:
{{{
#!CommitTicketReference repository=""
revision="0678d657222dd667bcc7e4fc307ea2ab70d219d7"
Refs #34099 -- Doc'd that custom Model.save() should update update_fields
kwarg.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"cedf1be7e5448a951538eeadf73b463832ba1a35" cedf1be]:
{{{
#!CommitTicketReference repository=""
revision="cedf1be7e5448a951538eeadf73b463832ba1a35"
[4.1.x] Refs #34099 -- Doc'd that custom Model.save() should update
update_fields kwarg.
Backport of 0678d657222dd667bcc7e4fc307ea2ab70d219d7 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Phil Gyford | Owner: Sarah
| Boyce
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7a5307974ac18d1e9f35ac5754414479395f6a83" 7a53079]:
{{{
#!CommitTicketReference repository=""
revision="7a5307974ac18d1e9f35ac5754414479395f6a83"
Fixed #34099 -- Added release notes for QuerySet.update_or_create()
changes.
Follow up to 6cc0f22a73970dd7c0d29d4d8d2ff9e1cc862b30.
Thanks Phil Gyford for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34099#comment:18>