Opened 11 months ago

Closed 10 months ago

Last modified 10 months 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 Changed 11 months ago by Reupen Shah

Description: modified (diff)

comment:2 Changed 11 months ago by Reupen Shah

Description: modified (diff)

comment:3 Changed 11 months ago by Simon Charette

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 Changed 11 months ago by Baptiste Mispelon

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

comment:5 Changed 11 months ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:6 Changed 11 months ago by Simon Charette

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 11 months ago by Simon Charette (previous) (diff)

comment:7 Changed 11 months ago by Reupen Shah

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.)

comment:8 in reply to:  6 Changed 11 months ago by Mariusz Felisiak

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 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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