#17341: Model.save() commits transactions after every parent class save
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.3
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* needs_tests: 1 => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/17341#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
