Raise a specialized exception when Model.save(update_fields) does not affect any rows

[Simon Charette]

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 ​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 ​both instances should be switched to this pattern (force_update and update_fields).

[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 ​PEP 249.
Not sure if we should raise a ProgrammingError or InterfaceError

[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 ​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.

[Simon Charette]

Another alternative we would be to simply raise a cls.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 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.

[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

[Simon Charette]

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.

[Enrique Soria]

I've done all request changes, and now I think now it's ready to be merged

[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.

[Mariusz Felisiak]

Replying to 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 😉, we don't raise any exception on save() without update_fields.

[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. Any save() call can raise an IntegrityError (and other flavors of DatabaseError) so even if we removed that particular instance it would still be best practice to handle them on save calls.

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 ​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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In ab148c0:

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.