[Django] #35425: .save(force_update=True) not respected for model instances with default primary keys
11 views
Skip to first unread message
Django
May 3, 2024, 4:22:36β―AMMay 3
to django-...@googlegroups.com
#35425: .save(force_update=True) not respected for model instances with default
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Jacob Walls
Walls |
Type: Bug | Status: assigned
Component: Database | Version: 4.2
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 |
-------------------------------------+-------------------------------------
With this model,
{{{
class WithDefault(models.Model):
id = models.UUIDField(primary_key=True, default=uuid.uuid4)
message = models.CharField(null=True)
}}}
the first IntegrityError at line 5 is expected as of
https://code.djangoproject.com/ticket/29260#comment:3, but the second one
at line 6, I suggest, is not.
{{{
In [1]: from models import WithDefault
In [2]: import uuid
In [3]: known_uuid = uuid.uuid4()
In [4]: WithDefault.objects.create(pk=known_uuid)
Out[4]: <WithDefault: WithDefault object (0ccbf1df-6296-4efe-8f5d-
8f9091d9ebdc)>
In [5]: WithDefault(pk=known_uuid, message="overwritten").save()
---------------------------------------------------------------------------
UniqueViolation Traceback (most recent call
last)
UniqueViolation: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
...
IntegrityError: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
In [6]: WithDefault(pk=known_uuid,
message="overwritten").save(force_update=True)
---------------------------------------------------------------------------
UniqueViolation Traceback (most recent call
last)
File ~/django/django/db/backends/utils.py:105, in
CursorWrapper._execute(self, sql, params, *ignored_wrapper_args)
104 else:
--> 105 return self.cursor.execute(sql, params)
UniqueViolation: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
The above exception was the direct cause of the following exception:
IntegrityError Traceback (most recent call
last)
Cell In[6], line 1
----> 1 WithDefault(pk=known_uuid,
message="overwritten").save(force_update=True)
File ~/django/django/db/models/base.py:1185, in Model._do_insert(self,
manager, using, fields, returning_fields, raw)
1180 def _do_insert(self, manager, using, fields, returning_fields,
raw):
1181 """
1182 Do an INSERT. If returning_fields is defined then this method
should
1183 return the newly created data for the model.
1184 """
-> 1185 return manager._insert(
1186 [self],
1187 fields=fields,
1188 returning_fields=returning_fields,
1189 using=using,
1190 raw=raw,
1191 )
...
IntegrityError: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
}}}
----
I had an illuminating [https://forum.djangoproject.com/t/save-behavior-
when-updating-model-with-default-primary-keys/30778 conversation on the
forum] regarding my surprise at the behavior of `save()` when dealing with
the first failure on line 5. Stating what I learned from it in case
helpful.
Until yesterday, I thought that the following two calls were equivalent,
other than perhaps one being faster for updates and the other faster for
inserts. In fact, I thought this equivalence would have been a nice
clarification of the value prop of the ORM that
β[https://docs.djangoproject.com/en/5.0/ref/models/instances/#how-django-
knows-to-update-vs-insert Django abstracts] the need to use INSERT or
UPDATE SQL statements.β In other words, what does `save()` do? ''It
updates or creates.''
{{{
def overwrite_1(known_uuid):
MyModel.objects.update_or_create(
pk=known_uuid,
defaults={
"other_field": 1,
},
)
def overwrite_2(known_uuid):
MyModel(
pk=known_uuid,
other_field=1,
).save()
}}}
So, at least when I'm in an overwriting posture--and Ken brings up a good
point that [https://forum.djangoproject.com/t/save-behavior-when-updating-
model-with-default-primary-keys/30778/9 forcing the user to opt in to this
potential for data loss] for existing primary keys is worth something--I
prefer `save()`. It's simpler and doesn't involve "defaults". (''What's a
"default" got to do with updating one row's values?'') It's also faster
for the UPDATE case, which is my hot path. I might be in the minority, and
could be convinced otherwise. (A third variation is possible,
`.filter(pk=known).update(...)`, but not unless you know the objects
necessarily exist.) But everything I just said is skating on pretty
rarefied ice -- the basic point is that ''huh, I have to be careful with
save() depending on the details of my field definitions?''
This was acknowledged as an acceptable wart in #29260 and then documented
further in #31070 with a fleshed out comment in the 3.0 release notes.
At all of those points, though, it was assumed that this would still
succeed:
{{{
def overwrite_2(known_uuid):
MyModel(
pk=known_uuid,
other_field=1,
).save(force_update=True)
}}}
This ticket is for that bug, the failure on line 6 in my REPL. (had a
looksee, likely to be a one-line fix, happy to PR it π€).
---
But perhaps in another ticket, or on the forum, we could consider the
question of whether we should drive at the solution discussed several
times by the participants on those tickets (~"if only there were a way to
determine whether a field's value came from a default..."). On the thread
I suggest we could do that by [https://forum.djangoproject.com/t/save-
behavior-when-updating-model-with-default-primary-keys/30778/8 adjusting]
`._state.adding = False` to agree with the admonition in the docs to be
prepared for explicitly-specified primary keys to be treated as updates
(here, at [https://forum.djangoproject.com/t/save-behavior-when-updating-
model-with-default-primary-keys/30778/8 "the one gotcha..."]).
To paraphrase Carlton, who
[https://github.com/django/django/pull/12209#pullrequestreview-331284409
suggested reverting] #29260, that change traded one user's ''desire'' to
optimize out one query, which can already be had with `force_insert=True`,
for another person's ''obligation'' to start using `force_update=True`
(once we fix it), which we
[https://docs.djangoproject.com/en/5.0/ref/models/instances/#forcing-an-
insert-or-update discourage], or to avoid `save()` altogether in favor of
the QuerySet API, which seems like a loss for feature parity /
understandability.
I understand this might cause churn in the "release story" if we revisit
it, but it's churn that wouldn't require user action. Appreciate all the
hard work that went into developing and reviewing those changes--I'm
totally fine with a decision to let it be. π
---
I'll volunteer some small edits to the docs. I re-read the save() docs and
didn't grok what "existing model instance" meant, since "exist" is also
used in the sentence to refer to existing database rows. (We could say
"fetched.") And Ken pointed me to the [https://forum.djangoproject.com/t
/save-behavior-when-updating-model-with-default-primary-
keys/30778/4#:~:text=Backward%20incompatible%20changes 3.0 release notes],
which I wouldn't have discovered on my own developing a feature against
4.2. Suggest surfacing up the recipes in that 3.0 release note somewhere
more permanent.
--
Ticket URL: <https://code.djangoproject.com/ticket/35425>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Jacob Walls
Walls |
Type: Bug | Status: assigned
Component: Database | Version: 4.2
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 |
-------------------------------------+-------------------------------------
With this model,
{{{
class WithDefault(models.Model):
id = models.UUIDField(primary_key=True, default=uuid.uuid4)
message = models.CharField(null=True)
}}}
the first IntegrityError at line 5 is expected as of
https://code.djangoproject.com/ticket/29260#comment:3, but the second one
at line 6, I suggest, is not.
{{{
In [1]: from models import WithDefault
In [2]: import uuid
In [3]: known_uuid = uuid.uuid4()
In [4]: WithDefault.objects.create(pk=known_uuid)
Out[4]: <WithDefault: WithDefault object (0ccbf1df-6296-4efe-8f5d-
8f9091d9ebdc)>
In [5]: WithDefault(pk=known_uuid, message="overwritten").save()
---------------------------------------------------------------------------
UniqueViolation Traceback (most recent call
last)
UniqueViolation: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
...
IntegrityError: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
In [6]: WithDefault(pk=known_uuid,
message="overwritten").save(force_update=True)
---------------------------------------------------------------------------
UniqueViolation Traceback (most recent call
last)
File ~/django/django/db/backends/utils.py:105, in
CursorWrapper._execute(self, sql, params, *ignored_wrapper_args)
104 else:
--> 105 return self.cursor.execute(sql, params)
UniqueViolation: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
The above exception was the direct cause of the following exception:
IntegrityError Traceback (most recent call
last)
Cell In[6], line 1
----> 1 WithDefault(pk=known_uuid,
message="overwritten").save(force_update=True)
File ~/django/django/db/models/base.py:1185, in Model._do_insert(self,
manager, using, fields, returning_fields, raw)
1180 def _do_insert(self, manager, using, fields, returning_fields,
raw):
1181 """
1182 Do an INSERT. If returning_fields is defined then this method
should
1183 return the newly created data for the model.
1184 """
-> 1185 return manager._insert(
1186 [self],
1187 fields=fields,
1188 returning_fields=returning_fields,
1189 using=using,
1190 raw=raw,
1191 )
...
IntegrityError: duplicate key value violates unique constraint
"models_withdefault_pkey"
DETAIL: Key (id)=(0ccbf1df-6296-4efe-8f5d-8f9091d9ebdc) already exists.
}}}
----
I had an illuminating [https://forum.djangoproject.com/t/save-behavior-
when-updating-model-with-default-primary-keys/30778 conversation on the
forum] regarding my surprise at the behavior of `save()` when dealing with
the first failure on line 5. Stating what I learned from it in case
helpful.
Until yesterday, I thought that the following two calls were equivalent,
other than perhaps one being faster for updates and the other faster for
inserts. In fact, I thought this equivalence would have been a nice
clarification of the value prop of the ORM that
β[https://docs.djangoproject.com/en/5.0/ref/models/instances/#how-django-
knows-to-update-vs-insert Django abstracts] the need to use INSERT or
UPDATE SQL statements.β In other words, what does `save()` do? ''It
updates or creates.''
{{{
def overwrite_1(known_uuid):
MyModel.objects.update_or_create(
pk=known_uuid,
defaults={
"other_field": 1,
},
)
def overwrite_2(known_uuid):
MyModel(
pk=known_uuid,
other_field=1,
).save()
}}}
So, at least when I'm in an overwriting posture--and Ken brings up a good
point that [https://forum.djangoproject.com/t/save-behavior-when-updating-
model-with-default-primary-keys/30778/9 forcing the user to opt in to this
potential for data loss] for existing primary keys is worth something--I
prefer `save()`. It's simpler and doesn't involve "defaults". (''What's a
"default" got to do with updating one row's values?'') It's also faster
for the UPDATE case, which is my hot path. I might be in the minority, and
could be convinced otherwise. (A third variation is possible,
`.filter(pk=known).update(...)`, but not unless you know the objects
necessarily exist.) But everything I just said is skating on pretty
rarefied ice -- the basic point is that ''huh, I have to be careful with
save() depending on the details of my field definitions?''
This was acknowledged as an acceptable wart in #29260 and then documented
further in #31070 with a fleshed out comment in the 3.0 release notes.
At all of those points, though, it was assumed that this would still
succeed:
{{{
def overwrite_2(known_uuid):
MyModel(
pk=known_uuid,
other_field=1,
).save(force_update=True)
}}}
This ticket is for that bug, the failure on line 6 in my REPL. (had a
looksee, likely to be a one-line fix, happy to PR it π€).
---
But perhaps in another ticket, or on the forum, we could consider the
question of whether we should drive at the solution discussed several
times by the participants on those tickets (~"if only there were a way to
determine whether a field's value came from a default..."). On the thread
I suggest we could do that by [https://forum.djangoproject.com/t/save-
behavior-when-updating-model-with-default-primary-keys/30778/8 adjusting]
`._state.adding = False` to agree with the admonition in the docs to be
prepared for explicitly-specified primary keys to be treated as updates
(here, at [https://forum.djangoproject.com/t/save-behavior-when-updating-
model-with-default-primary-keys/30778/8 "the one gotcha..."]).
To paraphrase Carlton, who
[https://github.com/django/django/pull/12209#pullrequestreview-331284409
suggested reverting] #29260, that change traded one user's ''desire'' to
optimize out one query, which can already be had with `force_insert=True`,
for another person's ''obligation'' to start using `force_update=True`
(once we fix it), which we
[https://docs.djangoproject.com/en/5.0/ref/models/instances/#forcing-an-
insert-or-update discourage], or to avoid `save()` altogether in favor of
the QuerySet API, which seems like a loss for feature parity /
understandability.
I understand this might cause churn in the "release story" if we revisit
it, but it's churn that wouldn't require user action. Appreciate all the
hard work that went into developing and reviewing those changes--I'm
totally fine with a decision to let it be. π
---
I'll volunteer some small edits to the docs. I re-read the save() docs and
didn't grok what "existing model instance" meant, since "exist" is also
used in the sentence to refer to existing database rows. (We could say
"fetched.") And Ken pointed me to the [https://forum.djangoproject.com/t
/save-behavior-when-updating-model-with-default-primary-
keys/30778/4#:~:text=Backward%20incompatible%20changes 3.0 release notes],
which I wouldn't have discovered on my own developing a feature against
4.2. Suggest surfacing up the recipes in that 3.0 release note somewhere
more permanent.
--
Ticket URL: <https://code.djangoproject.com/ticket/35425>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
May 3, 2024, 4:44:43β―AMMay 3
to django-...@googlegroups.com
#35425: .save(force_update=True) not respected for model instances with default
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
primary keys
-------------------------------------+-------------------------------------
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Jacob Walls:
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
Ticket URL: <https://code.djangoproject.com/ticket/35425#comment:1>
Django
May 3, 2024, 9:41:23β―AMMay 3
to django-...@googlegroups.com
we should drive at the solution discussed several times by the
participants on those tickets (~"if only there were a way to determine
whether a field's value came from a default..."). On the thread I suggest
we could do that by [https://forum.djangoproject.com/t/save-behavior-when-
updating-model-with-default-primary-keys/30778/8 adjusting]
`._state.adding = False` to agree with the
[https://docs.djangoproject.com/en/5.0/ref/models/instances/#explicitly-
participants on those tickets (~"if only there were a way to determine
whether a field's value came from a default..."). On the thread I suggest
we could do that by [https://forum.djangoproject.com/t/save-behavior-when-
updating-model-with-default-primary-keys/30778/8 adjusting]
`._state.adding = False` to agree with the
specifying-auto-primary-key-admonition documentation] stating that when
explicitly specifying primary keys, "Django will assume youβre changing
the existing record rather than creating a new one."
Django
May 4, 2024, 5:22:59β―PMMay 4
to django-...@googlegroups.com
#35425: .save(force_update=True) not respected for model instances with default
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/18131 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35425#comment:3>
Django
May 5, 2024, 11:57:36β―AMMay 5
to django-...@googlegroups.com
#35425: .save(force_update=True) not respected for model instances with default
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
* stage: Unreviewed => Accepted
Comment:
Having `save()` always do the expected thing when called on a newly
created instance of a model with a primary key configured with a Python
level default is a complex situation as you've pointed out.
Given the ORM has historically expected newly created model instances to
be used for ''additions'' and uses `_state.adding` to take a decision in
the face of ambiguity I still believe that #29260 was the right thing to
do.
With that being said, when being explicitly told what to do via
`force_update=True` there is no `save` ambiguity and thus we should not
fallback to `force_insert`. In this sense I think this represents a valid
bug and the provided patch seems adequate.
--
Ticket URL: <https://code.djangoproject.com/ticket/35425#comment:4>
Django
May 6, 2024, 5:47:08β―AMMay 6
to django-...@googlegroups.com
#35425: .save(force_update=True) not respected for model instances with default
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/35425#comment:5>
Django
May 7, 2024, 6:15:38β―AMMay 7
to django-...@googlegroups.com
#35425: .save(force_update=True) not respected for model instances with default
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: closed
primary keys
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution: fixed
(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
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce <42296566+sarahboyce@β¦>):
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"ceea86baa36b91d0002911770340a2d7bd4f64b7" ceea86b]:
{{{#!CommitTicketReference repository=""
revision="ceea86baa36b91d0002911770340a2d7bd4f64b7"
Fixed #35425 -- Avoided INSERT with force_update and explicit pk.
Affected models where the primary key field is defined with a
default or db_default, such as UUIDField.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35425#comment:6>
Reply all
Reply to author
Forward
0 new messages