[Django] #36074: Updating a model instance with a composite primary key through .save() results in unnecessary re-assignment of primary key fields

[Django]

#36074: Updating a model instance with a composite primary key through .save()
results in unnecessary re-assignment of primary key fields
-------------------------------------+-------------------------------------
Reporter: Simon | Owner: Simon Charette
Charette |
Type: Bug | Status: assigned
Component: Database | Version: dev
layer (models, ORM) |
Severity: Release | Keywords:
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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="f...@bar.org")
user.email = "some...@example.com"
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36074>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#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:

New description:

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]

#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: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* has_patch: 0 => 1
* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36074#comment:2>

[Django]

#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: 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):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36074#comment:3>

[Django]

#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: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Release blocker | Resolution: fixed

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@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"af6336f2c82fd6e5a0e42a7650a3a132c3362a0f" af6336f]:
{{{#!CommitTicketReference repository=""
revision="af6336f2c82fd6e5a0e42a7650a3a132c3362a0f"
Fixed #36074 -- Excluded composite primary key fields on save() updates.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36074#comment:4>