Opened 3 years ago

Last modified 3 months ago

#33579 assigned New feature

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

Reported by: Simon Charette Owned by: Dulalet
Component: Database layer (models, ORM) Version: 4.0
Severity: Normal Keywords:
Cc: Sardorbek Imomaliev Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by 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).

Change History (12)

comment:1 by Simon Charette, 3 years ago

Description: modified (diff)

comment:2 by Carlton Gibson, 3 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Mohamed Nabil Rady, 3 years ago

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

comment:4 by Simon Charette, 3 years ago

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.

comment:5 by Sardorbek Imomaliev, 3 years ago

Cc: Sardorbek Imomaliev added

comment:6 by Dulalet, 3 years ago

Owner: changed from nobody to Dulalet
Status: newassigned

comment:7 by Simon Charette, 2 years ago

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.

Last edited 2 years ago by Simon Charette (previous) (diff)

comment:8 by Enrique Soria, 6 months ago

Has patch: set

comment:9 by Enrique Soria, 6 months ago

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

comment:10 by Simon Charette, 6 months ago

Patch needs improvement: set

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.

comment:11 by Enrique Soria, 3 months ago

Patch needs improvement: unset

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

comment:12 by Simon Charette, 3 months ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

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

Note: See TracTickets for help on using tickets.
Back to Top