[Django] #36064: save() behavior for new model instance with default primary key does not apply to CompositePrimaryKey

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Release
| blocker
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Based on the [https://docs.djangoproject.com/en/dev/ref/models/instances
/#how-django-knows-to-update-vs-insert description] of how `save()` is
different than `update_or_create()` if the model's primary key has a
default, i.e. that only an INSERT is attempted if the pk is not set by the
user, I would expect models with a CompositePrimaryKey to behave as if
they "have a default" if all of their composed fields have a default, or
else for this to be called out in the doc.

This test shows this is not the case. The first test showing 2 queries
instead of 1 is not such a serious problem, but the second test not
raising IntegrityError seems like it could bite a user who was expecting
to catch IntegrityError and do a custom update (based on how other models
behave since Django 3.0 and #29260).

{{{#!diff
diff --git a/tests/composite_pk/models/tenant.py
b/tests/composite_pk/models/tenant.py
index ac0b3d9715..2273908031 100644
--- a/tests/composite_pk/models/tenant.py
+++ b/tests/composite_pk/models/tenant.py
@@ -1,3 +1,4 @@
+import uuid
from django.db import models


@@ -46,5 +47,5 @@ class Comment(models.Model):

class Post(models.Model):
pk = models.CompositePrimaryKey("tenant_id", "id")
- tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE)
- id = models.UUIDField()
+ tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE,
default=1)
+ id = models.UUIDField(default=uuid.uuid4)
diff --git a/tests/composite_pk/test_create.py
b/tests/composite_pk/test_create.py
index 7c9925b946..af7ceb7df2 100644
--- a/tests/composite_pk/test_create.py
+++ b/tests/composite_pk/test_create.py
@@ -1,6 +1,7 @@
+from django.db import IntegrityError
from django.test import TestCase

-from .models import Tenant, User
+from .models import Post, Tenant, User


class CompositePKCreateTests(TestCase):
@@ -14,6 +15,16 @@ class CompositePKCreateTests(TestCase):
id=1,
email="user...@example.com",
)
+ cls.post = Post.objects.create()
+
+ def test_save_default_pk_not_set(self):
+ with self.assertNumQueries(1):
+ Post().save()
+
+ def test_save_default_pk_set(self):
+ with self.assertRaises(IntegrityError):
+ Post(tenant_id=1, id=self.post.id).save()
+

def test_create_user(self):
test_cases = (
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36064>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

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

Changes (by Sarah Boyce):

* cc: Csirmaz Bendegúz (added)
* stage: Unreviewed => Accepted

Comment:

Thank you Jacob! Agree this needs either a fix or some docs
--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:1>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by mxp-xc):

* Attachment "t.diff" added.

--
Ticket URL: <https://code.djangoproject.com/ticket/36064>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mxp-xc):

* Attachment "t.diff" removed.

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mxp-xc):

* Attachment "t.diff" added.

[Django]

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mxp-xc):

* Attachment "t.diff" removed.

[Django]

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mxp-xc):

* Attachment "t.diff" added.

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by mxp-xc):

This is my first time trying to submit code. Please see if it can meet the
requirements. If so, I will learn to submit PR. Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:2>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by mao xin chen):

* Attachment "t.diff" removed.

--
Ticket URL: <https://code.djangoproject.com/ticket/36064>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Csirmaz Bendegúz):

Replying to [comment:2 mao xin chen]:

> This is my first time trying to submit code. Please see if it can meet
the requirements. If so, I will learn to submit PR. Thanks

Thanks for the patch! Please check the
[https://docs.djangoproject.com/en/dev/internals/contributing/ docs] on
how to contribute. If you open a PR on GitHub we can review your code.
--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:3>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Csirmaz Bendegúz):

Thanks Jacob!

|| n || default || obj.save() || conflict ||
|| 1 || yes || insert || IntegrityError ||
|| 2 || no || update & insert || update ||

1. If pk has a ''default'' or ''db_default'', saving a new object with a
conflicting pk attempts an insert and raises an IntegrityError
2. If pk doesn't have a ''default'' or ''db_default'', saving a new object
with a conflicting pk attempts an update (if no rows affected attempts an
insert)

Is that correct?
--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:4>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
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 Csirmaz Bendegúz):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:5>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Csirmaz
| Bendegúz
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):

* owner: (none) => Csirmaz Bendegúz
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:6>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Csirmaz
| Bendegúz
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/36064#comment:7>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Csirmaz
| Bendegúz
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):

* stage: Ready for checkin => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:8>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Csirmaz
| Bendegúz
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
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"9fa4d07ce0729850661a31a6b37c6b48f13d2266" 9fa4d07]:
{{{#!CommitTicketReference repository=""
revision="9fa4d07ce0729850661a31a6b37c6b48f13d2266"
Refs #36064 -- Added Model.has_db_default() to encapsulate NOT_PROVIDED
checks.

This avoids many awkward checks against NOT_PROVIDED and provides symmetry
with Field.has_default() which is also the reason why it wasn't made a
property.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:9>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Csirmaz
| Bendegúz
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/36064#comment:10>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Csirmaz
| Bendegúz
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
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"8287fd49151b1b99045efbf3de8ef911d63c5f45" 8287fd49]:
{{{#!CommitTicketReference repository=""
revision="8287fd49151b1b99045efbf3de8ef911d63c5f45"
Refs #36064 -- Added test that falsey primary key default/db_default value
skips an update query on save.

This adds test coverage for logic change in
9fa4d07ce0729850661a31a6b37c6b48f13d2266.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:11>

[Django]

#36064: save() behavior for new model instance with default primary key does not
apply to CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Csirmaz
| Bendegúz

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:"585160586336f3bcd7b694f53cf10db73c56981c" 5851605]:
{{{#!CommitTicketReference repository=""
revision="585160586336f3bcd7b694f53cf10db73c56981c"
Fixed #36064 -- Skipped an UPDATE when adding a model instance with a
composite primary key with default values.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36064#comment:12>