Model._do_update not always consistent with the precise meaning of returned value

[Jozef]

This is a bit obscure corner-case bug which might seem unimportant, but the solution is easy and actually only makes the code simpler (removes one nested if) and more consistent.

When select_on_save is set, Model._do_update() ignores the result of Model._update() if it learnt that the object exists in a previous DB call. That last thing is crucial, as the object could have already been deleted by concurrent worker just in-between these two ORM calls (exists and _update). In such case, the _update() call will fail, resulting in nothing being actually updated. However, _do_update() method will currently return True regardless of _update() result, which is inconsistent with the common (and expected) behavior, and also in contrast with the method's docs (saying: If the model was updated (in the sense that an update query was done and a matching row was found from the DB) the method will return True.)

As a consequence, object will not be saved into the database, because _save_table() won't call _do_insert(), being misled by _do_update(). Now you may oppose that this does not really matter that much, since it has been caused by concurrent modifications of the same data, which is considered non-deterministic anyway (so noone has the right to complain that the object is not there at the end). However, here's one example that vouches for this bug to be fixed: if you happen to have a trigger in your DB counting the number of times an object was changed (updated/inserted), you'd be missing one (which is no longer justifiable by concurrent save() and delete() operations).

As it sometimes happens, it is IMHO impossible to create a unit test for this bug, as it is too low level thing, exploitable only in a concurrent environment, and with some (bad) luck. I only found the bug when studying the code. So that's why I tried to provide a solid explanation, believing the fix I'm about to make should be obvious and mergable without worries.

[Jozef]

My fix is available in this ​Github branch

[Jozef]

I've modified the fix to count with the rare DB behavior when a successful UPDATE returns zero (as discussed in #20988).

More info in ​Github discussion.

[Tim Graham]

On the PR Anssi said at least docs and comments should be updated.

[Tim Graham]

Anssi will commit this.

[Anssi Kääriäinen <akaariai@…>]

In c56c42b5c01ed774cfa8e044cd372a984608536c:

Fixed #22967 -- Made Model._do_update consistent

Made _do_update behave more strictly according to its docs,
including a corner case when specific concurent updates are
executed and select_on_save is set.