[Django] #37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------
Reporter: Denny Biasiolli | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Normal
Keywords: compositeprimarykey | Triage Stage:
_is_pk_set | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
`Model.__eq__` still uses `pk is None` to decide whether an instance is
unsaved. That is wrong for composite primary keys (`pk` is a tuple) and
for `db_default` PKs (`DatabaseDefault` is not `None`).

```
>>> User() == User()
True
>>> User(tenant_id=1) == User(tenant_id=1)
True
```

Documented contract (and simple-PK behavior since #18864 / #18250) is that
unsaved instances are equal only to themselves. `__hash__` already uses
`_is_pk_set()` and raises for an unset PK.

The XML serializer has the same leftover (`if obj.pk is not None`) and
emits `pk='["None", "None"]'` for an unsaved composite instance instead of
omitting the attribute.

Proposed fix: use `_is_pk_set()` in both places, matching `__hash__` and
the rest of the #373 conversion.

Related: #18864, #18250, #373.
--
Ticket URL: <https://code.djangoproject.com/ticket/37309>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------

Reporter: Denny Biasiolli | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: compositeprimarykey | Triage Stage:
_is_pk_set | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Denny Biasiolli:

Old description:

> `Model.__eq__` still uses `pk is None` to decide whether an instance is
> unsaved. That is wrong for composite primary keys (`pk` is a tuple) and
> for `db_default` PKs (`DatabaseDefault` is not `None`).
>
> ```
> >>> User() == User()
> True
> >>> User(tenant_id=1) == User(tenant_id=1)
> True
> ```
>
> Documented contract (and simple-PK behavior since #18864 / #18250) is
> that unsaved instances are equal only to themselves. `__hash__` already
> uses `_is_pk_set()` and raises for an unset PK.
>
> The XML serializer has the same leftover (`if obj.pk is not None`) and
> emits `pk='["None", "None"]'` for an unsaved composite instance instead
> of omitting the attribute.
>
> Proposed fix: use `_is_pk_set()` in both places, matching `__hash__` and
> the rest of the #373 conversion.
>
> Related: #18864, #18250, #373.

New description:

{{{Model.__eq__}}} still uses {{{pk is None}}} to decide whether an

instance is unsaved. That is wrong for composite primary keys ({{{pk}}} is
a tuple) and for {{{db_default}}} PKs ({{{DatabaseDefault}}} is not
{{{None}}}).

{{{
>>> User() == User()
True
>>> User(tenant_id=1) == User(tenant_id=1)
True
}}}

Documented contract (and simple-PK behavior since #18864 / #18250) is that
unsaved instances are equal only to themselves. {{{__hash__}}} already
uses {{{_is_pk_set()}}} and raises for an unset PK.

The XML serializer has the same leftover ({{{if obj.pk is not None}}}) and
emits {{{pk='["None", "None"]'}}} for an unsaved composite instance
instead of omitting the attribute.

Proposed fix: use {{{_is_pk_set()}}} in both places, matching
{{{__hash__}}} and the rest of the #373 conversion.

Related: #18864, #18250, #373.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/37309#comment:1>

[Django]

PR here: https://github.com/django/django/pull/21861

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

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------
Reporter: Denny Biasiolli | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: compositeprimarykey | Triage Stage: Ready for
_is_pk_set | checkin

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

Changes (by Jacob Walls):

* stage: Unreviewed => Ready for checkin

Comment:

Small suggestion to move the release note and split into two commits,
otherwise looks g2g.
--
Ticket URL: <https://code.djangoproject.com/ticket/37309#comment:3>

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------

Reporter: Denny Biasiolli | Owner: Jacob
| Walls <jacobtylerwalls@…>
Type: Bug | Status: closed

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

Severity: Normal | Resolution: fixed

Keywords: compositeprimarykey | Triage Stage: Ready for
_is_pk_set | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls <jacobtylerwalls@…>):

In [changeset:"6126f5d2fe0f22c0d2d4888de8c94b91a9795bb6" 6126f5d]:
{{{#!CommitTicketReference repository=""
revision="6126f5d2fe0f22c0d2d4888de8c94b91a9795bb6"
Refs #37309 -- Used _is_pk_set() in the XML serializer.

Unsaved composite and db_default primary keys are not None, so the old
pk is None check emitted a dummy pk attribute in XML dumps.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37309#comment:5>

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------
Reporter: Denny Biasiolli | Owner: Jacob
| Walls <jacobtylerwalls@…>
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: compositeprimarykey | Triage Stage: Ready for
_is_pk_set | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls <jacobtylerwalls@…>):

* owner: (none) => Jacob Walls <jacobtylerwalls@…>
* resolution: => fixed
* status: new => closed

Comment:

In [changeset:"8d1c673466bcabff500edc6c7724e5a20f6df3de" 8d1c673]:
{{{#!CommitTicketReference repository=""
revision="8d1c673466bcabff500edc6c7724e5a20f6df3de"
Fixed #37309 -- Used _is_pk_set() in Model.__eq__.

Composite and db_default primary keys are not None when unset, so the
old pk is None check treated distinct unsaved instances as equal.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37309#comment:4>

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------
Reporter: Denny Biasiolli | Owner: Jacob
| Walls <jacobtylerwalls@…>

Type: Bug | Status: new

Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: compositeprimarykey | Triage Stage: Ready for
_is_pk_set | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

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

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

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------

Reporter: Denny Biasiolli | Owner: Denny
| Biasiolli
Type: Bug | Status: assigned

Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: compositeprimarykey | Triage Stage: Ready for
_is_pk_set | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* owner: Jacob Walls <jacobtylerwalls@…> => Denny Biasiolli
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37309#comment:7>

[Django]

#37309: `Model.__eq__` treats distinct unsaved composite-PK instances as equal
-------------------------------------+-------------------------------------
Reporter: Denny Biasiolli | Owner: Denny
| Biasiolli

Type: Bug | Status: closed

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

Severity: Normal | Resolution: fixed

Keywords: compositeprimarykey | Triage Stage: Ready for
_is_pk_set | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

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

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