Opened 5 years ago

Closed 5 years ago

#20272 closed Cleanup/optimization (fixed)

Model._do_update isn't as useful as it could be

Reported by: gavinwahl@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Ticket #16649 refactored model saving and added a _do_update method that does the actual database UPDATE query. However, a similar database query that checks that a row with the pk still exists under certain conditions is still in _save_base, and as such can't be overridden by a subclass.

_save_base shortcuts _do_update when (from the comment):

We can end up here when saving a model in inheritance chain where update_fields doesn't target any field in current model. In that case we just say the update succeeded. Another case ending up here is a model with just PK - in that case check that the PK still exists.

I am using _do_update to implement optimistic concurrency control in django-optimistic-lock. In order to implement it correctly for inherited models, I need to add my own condition to the EXISTS query that _save_base does. If this query were pushed down to _do_update, I would be able to do this. Otherwise, I can't add my logic because it is deep inside the _save_base code.

I have a failing test in my library that demonstrates:

I modified Django slightly to move the existence query into _do_update, and I'm able to implement the behavior I need for my library. See

My patch:

Change History (5)

comment:1 Changed 5 years ago by Anssi Kääriäinen

Triage Stage: UnreviewedAccepted

_do_update() is private API but making private API easier to use by third party apps is a good idea. So, seems like a good change to me.

comment:2 Changed 5 years ago by gavinwahl@…

comment:3 Changed 5 years ago by Tim Graham

Type: UncategorizedCleanup/optimization

comment:4 Changed 5 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:5 Changed 5 years ago by Tim Graham <timgraham@…>

Resolution: fixed
Status: newclosed

In 616f3c4a795ba6d74c5f28d200d1921e998232f4:

Fixed #20272 - Moved update_fields existence check into Model._do_update.

Thanks Gavin Wahl.

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