#36074: Updating a model instance with a composite primary key through .save()
results in unnecessary re-assignment of primary key fields
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | 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 Simon Charette:
Old description:
> Looking at the code that gets involved and the generated SQL It appears
> that every `save` that results in an `UPDATE` for a model with a
> composite primary key also includes all the primary key field in the
> updated fields.
>
> For example, for a model of the form
>
> {{{#!python
> class User(models.Model):
> tenant = models.ForeignKey(Tenant)
> id = models.UUIDField(default=uuid.uuid4)
> username = models.EmailField()
> }}}
>
> Then doing
>
> {{{#!python
> user = User.objects.create(tenant=tenant, id=1, email="[email protected]")
> user.email = "[email protected]"
> user.save()
> # Will result in UPDATE user SET tenant_id = %s, id = %s, email = %s
> WHERE tenant_id = %s AND id = %s
> # ^^^^^^^^^^^^^^^^^^^^^^^ <- tenant_id
> and id should not be marked for UPDATE
> }}}
>
> Will result in the following SQL
>
> {{{#!sql
> INSERT INTO user(tenant_id, id, email) VALUES (%s, %s, %s)
> UPDATE user SET tenant_id = %s, id = %s, email = %s WHERE tenant_id = %s
> AND id = %s
> }}}
>
> But the `UPDATE` statement should '''not''' set `tenant_id` and `id` as
> they are part of the primary key lookup. The `UPDATE` should be
>
> {{{#!sql
> UPDATE user SET email = %s WHERE tenant_id = %s AND id = %s
> }}}
>
> which is due to a non-audited usage of `.primary_key` in `save_tables
>
> {{{#!diff
> diff --git a/django/db/models/base.py b/django/db/models/base.py
> index a7a26b405c..6d66080c20 100644
> --- a/django/db/models/base.py
> +++ b/django/db/models/base.py
> @@ -1091,10 +1091,11 @@ def _save_table(
> for a single table.
> """
> meta = cls._meta
> + pk_fields = meta.pk_fields
> non_pks_non_generated = [
> f
> for f in meta.local_concrete_fields
> - if not f.primary_key and not f.generated
> + if f not in pk_fields and not f.generated
> ]
>
> if update_fields:
> }}}
>
> and makes me fear there might be others lying around that went unnoticed.
New description:
Looking at the code that gets involved and the generated SQL It appears
that every `save` that results in an `UPDATE` for a model with a composite
primary key also includes all the primary key field in the updated fields.
For example, for a model of the form
{{{#!python
class User(models.Model):
tenant = models.ForeignKey(Tenant)
id = models.UUIDField(default=uuid.uuid4)
username = models.EmailField()
}}}
Then doing
{{{#!python
user = User.objects.create(tenant=tenant, id=1, email="[email protected]")
user.email = "[email protected]"
user.save()
# Will result in UPDATE user SET tenant_id = %s, id = %s, email = %s WHERE
tenant_id = %s AND id = %s
# ^^^^^^^^^^^^^^^^^^^^^^^ <- tenant_id and
id should not be marked for UPDATE
}}}
Will result in the following SQL
{{{#!sql
INSERT INTO user(tenant_id, id, email) VALUES (%s, %s, %s)
UPDATE user SET tenant_id = %s, id = %s, email = %s WHERE tenant_id = %s
AND id = %s
}}}
But the `UPDATE` statement should '''not''' set `tenant_id` and `id` as
they are part of the primary key lookup. The `UPDATE` should be
{{{#!sql
UPDATE user SET email = %s WHERE tenant_id = %s AND id = %s
}}}
which is due to a non-audited usage of `.primary_key` in `save_tables
{{{#!diff
diff --git a/django/db/models/base.py b/django/db/models/base.py
index a7a26b405c..6d66080c20 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -1091,10 +1091,11 @@ def _save_table(
for a single table.
"""
meta = cls._meta
+ pk_fields = meta.pk_fields
non_pks_non_generated = [
f
for f in meta.local_concrete_fields
- if not f.primary_key and not f.generated
+ if f not in pk_fields and not f.generated
]
if update_fields:
}}}
and makes me fear there might be others lying around that went unnoticed.
If you grep for `.primary_key` you'll notice that most of the checks
against this field need to be adjusted to use `field in opts.pk_fields`
instead which makes me wonder if we took the right decision by not setting
this flag to `True` when included as a member of `CompositePrimaryKey`.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/36074#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 [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/010701944994799c-9db00914-6fc4-465d-8e1a-84389794f523-000000%40eu-central-1.amazonses.com.
