[Django] #33579: Raise a specialized exception when Model.save(update_fields) does not affect any rows

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon | Owner: nobody
Charette |
Type: New | Status: new
feature |
Component: Database | Version: 4.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When `Model.save(update_fields)` is used to update an instance of a
previously fetched model and the resulting `UPDATE` doesn't affect any row
a `DatabaseError`
[https://github.com/django/django/blob/3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc/django/db/models/base.py#L1000
is raised].

Since the resulting exception cannot be differentiated by its `type` from
any other exception raised during `save` (e.g. a failed `UPDATE` also
results in a `DatabaseError`) the only pattern to gracefully handle this
rare edge case to catch all `DatabaseError` and compare its `.args[0]` (or
`str` representation) to a string that doesn't offer any stability
guarantees.

In order to make it easier for advanced users that rely on the
`update_fields` feature to handle this case differently from others where
a `DatabaseError` is raised I propose that that we introduced a new
`DatabaseError` subclass that would be raised instead when an unexpected
empty update occurs.

I believe
[https://github.com/django/django/blob/3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc/django/db/models/base.py#L997-L1000
both instances] should be switched to this pattern (`force_update` and
`update_fields`).

--
Ticket URL: <https://code.djangoproject.com/ticket/33579>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------

Reporter: Simon Charette | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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:

> When `Model.save(update_fields)` is used to update an instance of a
> previously fetched model and the resulting `UPDATE` doesn't affect any
> row a `DatabaseError`
> [https://github.com/django/django/blob/3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc/django/db/models/base.py#L1000
> is raised].
>
> Since the resulting exception cannot be differentiated by its `type` from
> any other exception raised during `save` (e.g. a failed `UPDATE` also
> results in a `DatabaseError`) the only pattern to gracefully handle this
> rare edge case to catch all `DatabaseError` and compare its `.args[0]`
> (or `str` representation) to a string that doesn't offer any stability
> guarantees.
>
> In order to make it easier for advanced users that rely on the
> `update_fields` feature to handle this case differently from others where
> a `DatabaseError` is raised I propose that that we introduced a new
> `DatabaseError` subclass that would be raised instead when an unexpected
> empty update occurs.
>
> I believe
> [https://github.com/django/django/blob/3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc/django/db/models/base.py#L997-L1000
> both instances] should be switched to this pattern (`force_update` and
> `update_fields`).

New description:

When `Model.save(update_fields)` is used to update an instance of a
previously fetched model and the resulting `UPDATE` doesn't affect any row
a `DatabaseError`
[https://github.com/django/django/blob/3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc/django/db/models/base.py#L1000
is raised].

Since the resulting exception cannot be differentiated by its `type` from

any other dababase errors occuring during `save` (e.g. a failed `UPDATE`

also results in a `DatabaseError`) the only pattern to gracefully handle
this rare edge case to catch all `DatabaseError` and compare its
`.args[0]` (or `str` representation) to a string that doesn't offer any
stability guarantees.

In order to make it easier for advanced users that rely on the
`update_fields` feature to handle this case differently from others where

a `DatabaseError` is raised I propose that that we introduce a new

`DatabaseError` subclass that would be raised instead when an unexpected
empty update occurs.

I believe
[https://github.com/django/django/blob/3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc/django/db/models/base.py#L997-L1000
both instances] should be switched to this pattern (`force_update` and
`update_fields`).

--

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

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Carlton Gibson):

* stage: Unreviewed => Accepted

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

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Mohamed Nabil Rady):

Is it the best the idea to introduce a new subclass rather than using one
of the other ones ? Because I think we should stick with the ones
specified in [https://peps.python.org/pep-0249 PEP 249].
Not sure if we should raise a {{{ ProgrammingError }}} or {{{
InterfaceError }}}

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

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Simon Charette):

I feel like a `django.db.models.Model.save` forced update failure has
little to do with PEP-249; it's an exception that occurs at the ORM layer
and not at the backend one.

> Not sure if we should raise a ProgrammingError or InterfaceError

I would argue that we should not subclass either of them as it will result
in the same effect as raising `DatabaseError` from a callers perspective
(no way to differentiate between a database level error and an ORM level
one without a `str(exc)` comparison).

The reason for proposing to subclass `DatabaseError` is solely for
backward compatibility purposes which I believed would make the proposed
change less controversial.

If it was do all over again I think we should have a dedicated
`NoopUpdate` (better name welcome) type like we do with `DoesNotExist` and
`MultipleObjectsReturned` but [https://bugs.python.org/issue12029 since
Python doesn't allow for virtual subclass overrides in except clauses]
there's no way to provide a deprecation path for `except DatabaseError`
clauses in the wild that might already be handling this case.

--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:4>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Sardorbek Imomaliev):

* cc: Sardorbek Imomaliev (added)

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

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------

Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned

Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Dulalet):

* owner: nobody => Dulalet
* status: new => assigned

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

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Simon Charette):

Another alternative we would be to simply raise a `base.DoesNotExists`
error instead.

It would be backward incompatible in the sense that a different exception
type would be raised but it seems it would fit elegantly with the message
it's meant to convey; the object that was attempted to be updated was not
found. It would also make code that handles generic `DoesNotExist` and
turn them into HTTP 404 automatically pick up errors of this form without
any form of special casing.

Another argument for this approach is that the affected ''base'' is
already available from `Model._save_table` via the `cls` kwarg so it would
also allow to differentiate which model row was missing when dealing with
concrete model inheritance that require update against multiple tables.

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

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Enrique Soria):

* has_patch: 0 => 1

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

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Enrique Soria):

I've made a PR for this which simply creates a new exception by inheriting
from DatabaseError. This will be compatible with people who already was
catching DatabaseError, and will make new developments easier by having a
separate exception class.

https://github.com/django/django/pull/18244
--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:9>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

Comment:

Left some comments on the PR about possible improvement to make the raised
exception more coherent with `ObjectDoesNotExist` and
`MultipleObjectsReturned` while still inheriting from `DatabaseError` for
backward compatiblity reasons.
--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:10>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Enrique Soria):

* needs_better_patch: 1 => 0

Comment:

I've done all request changes, and now I think now it's ready to be merged
--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:11>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

* needs_docs: 0 => 1
* needs_tests: 0 => 1

Comment:

Left some comments on the PR. It still needs tests tweaking, some code
adjustments, and documentation about
`django.core.exceptions.ObjectNotUpdated` and `NotUpdated`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:12>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* cc: Mariusz Felisiak (added)

Comment:

Replying to [comment:12 Simon Charette]:

> Left some comments on the PR. It still needs tests tweaking, some code
adjustments, and documentation about
`django.core.exceptions.ObjectNotUpdated` and `NotUpdated`.

TBH, this error is extremely annoying to me. It forces users to catch
`DatabaseError` on every `save()` call when you process things
asynchronously and don't really care if someone was deleted in the
meantime. I'd vote to remove it totally 😉, but realistically we should
raise `DoesNotExist` like on `save()` without `update_fields`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:13>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Dulalet
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

> I'd vote to remove it totally 😉, we don't raise any exception on save()
without update_fields.

As annoying as this exception can be I don't think we should entirely
silence it. Passing `update_fields` is synonymous with `force_update` and
just like when `force_insert` is provided erroring out to let callers know
that the forced operation failed seems like the right thing to do as doing
otherwise could result in unexpected data loses.

At least with a specialized exception, something less generic than
`DatabaseError`, callers can more adequately react to such errors. For
example, raised `ObjectNotUpdated` can be turned into a 404, silenced, or
turn into a subsequent `save` call without update fields depending on the
situation.

If you look at the ''core'' usages of `save(update_fields)` there seems to
be cases where we'd want to do one over the other. For example,
`reverse_many_to_one_manager._clear` might want to silence such exceptions
[https://github.com/django/django/blob/de1117ea8eabe0ee0aa048e5a4e249eab7c4245e/django/db/models/fields/related_descriptors.py#L909
when assigning] `None` while in `RenameContentType._rename` you'd most
like would want to refetch the row by natural key and update it again.

Once the exception lands and is documented it could allow third party
tools like DRF to catch them on `PUT` / `PATCH` and turn them into 404s.

Given the patch has not been improved for over 6 months I'll assign it to
me and try to bring it through the finish line.
--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:14>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------

Reporter: Simon Charette | Owner: Simon
| Charette

Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* owner: Dulalet => Simon Charette

--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:15>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Simon Charette):

* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:16>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Type: New feature | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | 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 Mariusz Felisiak):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:17>

[Django]

#33579: Raise a specialized exception when Model.save(update_fields) does not
affect any rows
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette

Type: New feature | Status: closed

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

Severity: Normal | 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 Mariusz Felisiak <felisiak.mariusz@…>):

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

Comment:

In [changeset:"ab148c02cedbac492f29930dcd5346e1af052635" ab148c0]:
{{{#!CommitTicketReference repository=""
revision="ab148c02cedbac492f29930dcd5346e1af052635"
Fixed #33579 -- Specialized exception raised on forced update failures.

Raising DatabaseError directly made it harder than it should to
differentiate between IntegrityError when a forced update resulted in no
affected rows.

Introducing a specialized exception allows for callers to more easily
silence, log, or turn them update failures into user facing exceptions
(e.g. 404s).

Thanks Mariusz for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33579#comment:18>