Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#31071 closed Bug (fixed)

Change in behaviour when saving a model instance with an explcit pk value if the pk field has a default

Reported by: Reupen Shah Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 3.0
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Reupen Shah)

Consider the following model:

from uuid import uuid4

from django.db import models

class Sample(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid4)
    name = models.CharField(blank=True, max_length=100)

In Django 2.2 and earlier, the following commands would result in an INSERT followed by an UPDATE:

s0 = Sample.objects.create()
s1 = Sample(pk=s0.pk, name='Test 1')
s1.save()

However, in Django 3.0, this results in two INSERTs (naturally the second one fails). The behaviour also changes if default=uuid4 is removed from the id field.

This seems related to https://code.djangoproject.com/ticket/29260.

The change in behaviour also has the side effect of changing the behaviour of the loaddata management command when the fixture contains explicit pk values and the objects already exist (e.g. when loading the fixture multiple times).

Perhaps the intention was to only change the behaviour if an explicit pk value was not set on the model instance being saved? (At least, that would be more backwards-compatible behaviour...)

Change History (11)

comment:1 by Reupen Shah, 5 years ago

Description: modified (diff)

comment:2 by Reupen Shah, 5 years ago

Description: modified (diff)

comment:3 by Simon Charette, 5 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

It looks like the logic in _save_table should not force an insert if an explicit pk value is provided.

The logic should likely take pk_set into account

if (
    not pk_set
    and self._state.adding
    and self._meta.pk.default
    and self._meta.pk.default is not NOT_PROVIDED
):
    force_insert = True

I'm surprised this was not caught by the suite if this breaks fixtures loading.

comment:4 by Baptiste Mispelon, 5 years ago

I bisected the regression down to 85458e94e38c20e57939947ee515a1a53689659f if that helps.

comment:5 by Simon Charette, 5 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:6 by Simon Charette, 5 years ago

I'm afraid we'll have to revert 85458e94e38c20e57939947ee515a1a53689659f if we want this pattern to work because we assign field defaults on Model.__init__ without tracking whether or not they were generated from field defaults or not which makes both Sample() and Sample(pk=s0.pk) have pk_set=True in _save_table.

Note that this limitation was mentioned in https://code.djangoproject.com/ticket/29260#comment:3 so another option could be to document that force_update must be used in this particular case. I feel like this would be good compromise. Regarding the fixture loading we should branch of raw which is passed by the serialization framework to disable the optimiation.

Happy to provide a patch for whatever solution we choose. I think it's worth adjusting the feature given it does reduce the number of queries significantly when using primary key defaults and documenting it as a backward incompatible change that can be worked around by passing force_update but simply reverting the feature and reopening #29260 to target the next release is likely less trouble.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:7 by Reupen Shah, 5 years ago

If it helps, I noticed this through the changed behaviour of loaddata in such cases (rather than the example code given). (I don't think we have any code that directly looks like the example.)

in reply to:  6 comment:8 by Mariusz Felisiak, 5 years ago

Replying to Simon Charette:

Note that this limitation was mentioned in https://code.djangoproject.com/ticket/29260#comment:3 so another option could be to document that force_update must be used in this particular case. I feel like this would be good compromise. Regarding the fixture loading we should branch of raw which is passed by the serialization framework to disable the optimiation.

Happy to provide a patch for whatever solution we choose. I think it's worth adjusting the feature given it does reduce the number of queries significantly when using primary key defaults and documenting it as a backward incompatible change that can be worked around by passing force_update...

I really like this proposition and I think it's fine to adjust the current fix and backport it to the Django 3.0.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 5779cc93:

Fixed #31071 -- Disabled insert optimization for primary keys with defaults when loading fixtures.

Model.save_base() is called directly when loading fixtures and assumes
existing rows will be updated. Branching of "raw" allows to maintain
the optimization introduced in #29260 while supporting this edge case.

Regression in 85458e94e38c20e57939947ee515a1a53689659f.

Thanks Reupen Shah for the report.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 7db4ab84:

[3.0.x] Fixed #31071 -- Disabled insert optimization for primary keys with defaults when loading fixtures.

Model.save_base() is called directly when loading fixtures and assumes
existing rows will be updated. Branching of "raw" allows to maintain
the optimization introduced in #29260 while supporting this edge case.

Regression in 85458e94e38c20e57939947ee515a1a53689659f.

Thanks Reupen Shah for the report.

Backport of 5779cc938a34eb96815c7a40ded2c8f6c8087c58 from master

Note: See TracTickets for help on using tickets.
Back to Top