Opened 11 years ago

Closed 11 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: dev
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

Description

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: https://github.com/gavinwahl/django-optimistic-lock/blob/4472b73b3eeb7ce2ea19164c75d70aa39249aa34/tests/tests/tests.py#L72

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 https://github.com/gavinwahl/django-optimistic-lock/commit/90be77cb356a7b65410df3f52407cc38da2f8e22.

My patch: https://github.com/gavinwahl/django/commit/cfebca2f11655b64b21cab554371c2696d4faa36/

Change History (5)

comment:1 by Anssi Kääriäinen, 11 years ago

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 by gavinwahl@…, 11 years ago

comment:3 by Tim Graham, 11 years ago

Type: UncategorizedCleanup/optimization

comment:4 by Claude Paroz, 11 years ago

Triage Stage: AcceptedReady for checkin

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

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