#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: 0 | Needs documentation: 0 Needs tests: 0 | Patch needs improvement: 0 Easy pickings: 0 | UI/UX: 0 -------------------------------------+------------------------------------- Description changed by Jacob Walls:
Old description: > 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. New description: 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 #31071 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#comment:1> Django <https://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-updates+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/0107018f3da0788f-2b83bcc7-5b1f-447e-8a7e-e4419f0b62b9-000000%40eu-central-1.amazonses.com.