Model.save() commits transactions after every parent class save

[Anssi Kääriäinen]

The bug is that when running outside transaction management, Django does a save() after every update/insert. In multitable inheritance, it is possible (although rare) that this will result in half-saved objects. For example, using postgresql, and model inheritance I get these queries:

BEGIN
SELECT (1) AS "a" FROM "parent" WHERE "parent"."id" = 1  LIMIT 1
UPDATE "parent" SET "f1" = 1 WHERE "parent"."id" = 1 
COMMIT -- Why commit here?
BEGIN
INSERT INTO "child" ("parent_ptr_id", "pk2") VALUES (1, NULL)
ERROR:  null value in column "pk2" violates not-null constraint

Now I have committed a half-update. The attached patch gets rid of the COMMIT/BEGIN in the middle.

I found this while hacking on #17332. It has more cleanups for .save (for example proxy handling), but I thought this deserves its own ticket.

[Anssi Kääriäinen]

As #17332 was marked as DDN (fully agreed), I think the cleanup of .save() included in the patch attached to that ticket should be moved to this ticket.

There are two things needing cleanup:

• The position of commit_unless_managed, which this ticket addresses directly
• The logic about handling proxy models. The problem here is that it is hard to follow what is happening. So hard, that I am pretty sure the last if condition before the actual save

  if not meta.is_proxy:
      do the actual save
  

is actually not needed at all. A proxy model can never reach this stage, see the "if meta.is_proxy: return" above.

I can write a patch incorporating this change. If piggy-bagging the proxy change is not wanted into this ticket, I can open another ticket. Or just leave it as is if that is wanted (IMHO bad idea...).

[Aymeric Augustin]

Yes, this makes sense.

[Anssi Kääriäinen]

Ok, here is a patch. In my opinion it does make it easier to understand what is going on in the saving process. I have tried to make sure saving works just like before. The only behavioral change should be committing only once, after all tables have been saved. At least the tests agree I haven't broken anything.

There is one thing I don't like, and that is the parameter name "topmost_concrete". It is needed so that we know to send signals only once, after the last concrete (non-proxy) model's table is saved. Better names (or better logic) welcome. Before this was done using origin, which was self.__class__, until first concrete model was seen, and after that it was None. The information in origin was not needed for anything else than checking when to send signals.

[Anssi Kääriäinen]

Cleanup of base_save() + tests

[Anssi Kääriäinen]

I realized there is one more plain old bug hidden in there: force_insert & force_update are not passed down the inheritance chain. There might be some reason for not doing that for multitable inheritance (does it mean force_insert all the tables, or force_insert just the topmost table?). But for proxy models this is a clear bug.

I now think it might be better to write the cleanup as:

def save_base(self, ...):
    # Do state setup & some special pre & post things unique to first save_base call.
    self._save_base(...)

def _save_base(...):
    # Do just inheritance chain traveling + actual saves here. No signals, no transactions.

The reason for the additional method is that the first save_base call is special. The additional method makes it explicit. It is easy to see that we only send the signals for the topmost model. I could not easily see that in the current trunk version. Patch attached. Me likes this version, but maybe this is getting overly cute...

In the latest patch, actual behavioral changes are:

• commit_unless_managed is called only once, after all tables have been saved.
• force_update & force_insert do have an effect for proxy models. As before, these are not passed down to multitable inheritance chains.

These changes have tests in the patch. All tests passed on SQLite.

The rest is just cleanup. There should not be any changes in how the per-table saving is done (the large if not meta.proxy: branch in trunk code is just reindented).

I wonder if I am once again extending the scope of the ticket too much. Maybe rename this to cleanup of model.save_base()? If not, just say and I will create a separate ticket for the general cleanup and leave this ticket just for the transaction case. In that case, the first submitted patch is the way to go.

Last: is it intentional that force_insert & force_update are not passed down the inheritance chain in multitable inheritance case?

[Aymeric Augustin]

The patch needs to be rewritten with transaction.atomic.

Unfortunately it's moving a lot of code around and I can't see exactly where to inject the atomic wrapper.

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

In 6b4834952dcce0db5cbc1534635c00ff8573a6d8:

Fixed #16649 -- Refactored save_base logic

Model.save() will use UPDATE - if not updated - INSERT instead of
SELECT - if found UPDATE else INSERT. This should save a query when
updating, but will cost a little when inserting model with PK set.

Also fixed #17341 -- made sure .save() commits transactions only after
the whole model has been saved. This wasn't the case in model
inheritance situations.

The save_base implementation was refactored into multiple methods.
A typical chain for inherited save is:
save_base()

_save_parents(self)

for each parent:

_save_parents(parent)
_save_table(parent)

_save_table(self)