Overriding a Model's save() method does not allow proper creation of ManyToMany related objects
Dmitri Fedortchenko
def save(self):
regex = re.compile("\{\{([^\}]{2,60})\}\}")
words = regex.findall(self.body)
self.body = regex.sub("\\1",self.body)
super(Chapter,self).save()
for word in words:
if len(word.strip()) > 0:
try:
self.keywords.create(value=word.strip().title())
except Exception, e:
print e
print self.keywords.all() # This prints the correct keywords!
super(Chapter,self).save()
The outcome of this code is that new Keywords are created, but they
are not bound to this Chapter.
Simply calling chapter.keywords.create(value="Test") will indeed
create a new keyword with the value "Test" bound to the chapter. I am
running this from the django admin btw.
The problem seems to be in the
django.db.models.fields.related._add_items method, somewhere around
line 340.
The fact is that the insertion query is executed, but for some reason
it is not committed, perhaps it is a transaction issue?
It is simply broken when called from within the save() method of a
Model instance...
I haven't started a ticket because I am not sure if this is bug or a
feature ;-)
Collin Grady
actually at fault, since it hard-sets m2ms, which would clear anything
set in save().
Also, you don't need to call save() again after adding to an m2m, since
it does not edit the model instance itself, but the related table.
--
Collin Grady
Haste makes waste.
-- John Heywood
Malcolm Tredinnick
> Example code:
>
> def save(self):
> regex = re.compile("\{\{([^\}]{2,60})\}\}")
> words = regex.findall(self.body)
> self.body = regex.sub("\\1",self.body)
> super(Chapter,self).save()
> for word in words:
> if len(word.strip()) > 0:
> try:
> self.keywords.create(value=word.strip().title())
> except Exception, e:
> print e
> print self.keywords.all() # This prints the correct keywords!
> super(Chapter,self).save()
>
>
> The outcome of this code is that new Keywords are created, but they
> are not bound to this Chapter.
> Simply calling chapter.keywords.create(value="Test") will indeed
> create a new keyword with the value "Test" bound to the chapter. I am
> running this from the django admin btw.
Can you explain what is meant to be going on here?
If you are trying to save m2m relations for an object that has not been
saved yet, you're out of luck. We need to know the current object's pk
value before it can be used in a m2m table and that isn't always
available (with auto primary keys).
Normally, if you want to adjust the m2m relations for a model, hooking
into the post_save signal is the way to go.
>
> The problem seems to be in the
> django.db.models.fields.related._add_items method, somewhere around
> line 340.
> The fact is that the insertion query is executed, but for some reason
> it is not committed, perhaps it is a transaction issue?
Perhaps it is. Perhaps it isn't. How have you established the query was
executed? What does the commit_unless_managed() call end up doing in
your case? It might depend on your particular setup, too. Do some more
debugging and see what shows up if you think there's a problem there.
However, do keep in mind that it's generally impossible to do everything
with m2m's inside a model's save method because of the chicken-and-egg
problem with primary keys noted above.
Regards,
Malcolm
--
Tolkien is hobbit-forming.
http://www.pointy-stick.com/blog/
Dmitri Fedortchenko
Basically I have followed the call all the way to the execution of the
cursor.execute() on line 343 in models.fields.related.
I have not actually delved into the transaction method. However in my
setup I am using the django admin, I have not set up any automatic
transaction handling and the save method does is not transaction
managed. Also I am running this on an existing object, and even if I
was not, the primary key would have been created since the save method
is called before I add new m2ms.
Calling the super(Chapter,self).save() AFTER adding new m2ms is not
needed, nor does it work.
I also checked to make sure that the
django.models.fields.related._remove_items() method is not called
after the _add_items() method, to restore some previous state.
My only conclusion is that somehow the transaction of the save()
cancels out the transaction of the _add_items() method...
Maybe it's the
save.alters_data = True
flag?
or something to do with the dispatcher? I have not yet gone that deep
into the source...
But I can assure you that I am using as basic a setup as possible.
It is simply django admin, MySQL with InnoDB and that very save method
that you see in my initial post.
So repeating this should not be a problem...
I'll try to delve deeper into the transaction side of things, but any
insight you can give me would be appreciated.
//Dmitri
On Nov 7, 2:25 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
Dmitri Fedortchenko
_add_items() method is only called once with the correct objects
(meaning the existing keywords and the keywords that I added).
Are you telling me that the admin actually has it's own method for
setting the relationships that is outside the
django.models.fields.related module?
//Dmitri
Dmitri Fedortchenko
Collin was absolutely right.
After the new keywords are added, the related.clear() method is called
and the admin adds it's own keywords, thus erasing any changes.
So it all boils down to the
django.models.manipulators.AutomaticManipulator:128
I totally get it, I've solved m2ms in exactly the same way many many
times before, delete them all then add the existing ones. Solves the
problem of having to check which ones are to be deleted...
While I see now how this works, I am still curious as to how this can
be solved... a post_save hook?
I found an article that details this very problem:
http://www.progprog.com/articles/2007/03/03/django-when-save-is-not-safe
It seems that no simple solution exists yet...
//D
On Nov 7, 2:25 am, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
Dmitri Fedortchenko
admin.
The idea is that when you are writing an article, you might mark
certain words in that article to become keywords. When saving the
article, these keywords are created (if they do not exist) and then
added as m2ms to the article.
I placed this code into the model's save method, however there is
currently no way to do it with the django admin since all m2ms are
cleared after the save() method...
So I cheated, the admin list_display variable can call methods, so I
dumped the code into
a method called by the admin list_display. So whenever a list is
displayed this method is called and thus the body of the article is
scanned for keywords to be added. Not the most optimal solution, but
the only one I could think of.
Still, the most logical course is to override the save method.
//D
Collin Grady
> Still, the most logical course is to override the save method.
I'd say the more logical course is to stop using admin for this ;)
--
Collin Grady
QOTD:
"If I'm what I eat, I'm a chocolate chip cookie."
Dmitri Fedortchenko
Collin Grady
> if I stop using the admin for editing an article, then I would have to
> write a whole bunch of code just to edit a single article. Suddenly the
> benefit of a unified admin interface is lost and I have to deal with
> permissions, UI, validation, forms.. it seems like a nightmare for
> something should work in the first place... ( i.e. overriding the save()
> method to add extra functionality)
>
> From the django features list:
> A dynamic admin interface: it's not just scaffolding — it's the whole house
> [The philosophy here is that your site is edited by a staff, or a
> client, or maybe just you — and you don't want to have to deal with
> creating backend interfaces just to manage content.]
>
> The message seems clear to me. ;)
It's not meant to do absolutely everything though - trying to hack it to
do something it doesn't is usually harder than writing a really simple
custom view :)
--
Collin Grady
If I had any humility I would be perfect.
-- Ted Turner
Dmitri Fedortchenko
Joseph Kocherhans
>
> The Admin class should be able to define post_ and pre_save hooks that are
> called before or after all Manipulation of the model is done (there are
> post_save hooks for the save method, but they are still called before the
> Manipulator is done working with the model)
Something similar is in progress already.
http://code.djangoproject.com/wiki/NewformsAdminBranch
It is however, mostly undocumented and subject to change. These things
take time :)
Joseph
Dmitri Fedortchenko
Gary Wilson
> Are you seeing this behavior in admin? If so, I believe that is what is
> actually at fault, since it hard-sets m2ms, which would clear anything
> set in save().
I know Russ didn't take much of a liking to this idea last time I mentioned
it, but...
That's why I say this functionality shouldn't be part of the admin, but rather
built in to the manytomany field or the model. Let the manytomany field store
the state it needs to in order to know what needs to be saved, and have that
happen when you call the model instance's save().
So model instance save() would be something like:
def save(self, ...)
normal_saving_of_self()
for field in self.m2mfields:
# This could add or remove objects.
field.save_pending()
Dmitri's overridden save() would become:
def save(self):
regex = re.compile("\{\{([^\}]{2,60})\}\}")
words = regex.findall(self.body)
self.body = regex.sub("\\1",self.body)
for word in words:
if len(word.strip()) > 0:
try:
# This not actually saved do database yet, but stored
# in the keywords field.
self.keywords.create(value=word.strip().title())
except Exception, e:
print e
super(Chapter,self).save() # <-- saving of data and m2m data happens here
This allows manytomany fields to be treated like any of the other model fields
(even though it's a bit different in that the data is not stored in the same
table as the other fields).
As far as AutomaticManipulator.save(), it appears that there is follow and
edit_inline logic mixed in with the manytomany saving. Does anyone who is
more familiar with newforms-admin know if the situation would be any better there?
Gary
Russell Keith-Magee
>
> Collin Grady wrote:
> > Are you seeing this behavior in admin? If so, I believe that is what is
> > actually at fault, since it hard-sets m2ms, which would clear anything
> > set in save().
>
> I know Russ didn't take much of a liking to this idea last time I mentioned
> it, but...
Just to reiterate what I said last time:
Firstly, this would be _huge_ backwards incompatibility. If this
change were to be made, finding and fixing models that relied upon the
old behaviour would be a non-trivial exercise. In the absence of a
_very_ compelling reason to make this change, you'll find me coming
down -1 for this reason alone.
Secondly, although an m2m field is defined on a single model, isn't
associated with a single model. It's associated with _two_ models. To
me, this means that putting the save on the relation (i.e., when you
assign the m2m attribute) rather than associating it with a particular
model makes conceptual sense.
If we were to move the m2m saving as part of the model save, you need
to know which side of the m2m the field was defined upon, which is
unwieldy because it presupposed knowledge of the model design.
Alternatively, if you put the m2m saving on the save method of both
models, there would be two different ways to effect the same database
change.
Thirdly, I'm not completely convinced that your proposed scheme won't
end up with similar (or possibly even 0worse) race conditions. At
present, the rules are simple - save all your models, then add all
your m2m relations. If we moved saving m2m to the instances, then when
you save instance X, you need to be sure that all the instances with
which X has an m2m relation are also saved (say, instance Y). If
instance Y then has an m2m relation back onto X through some other
attribute, you've got yourself tied in a knot.
> As far as AutomaticManipulator.save(), it appears that there is follow and
> edit_inline logic mixed in with the manytomany saving. Does anyone who is
> more familiar with newforms-admin know if the situation would be any better there?
I certainly hope so - that's why we're building newforms :-)
Broadly, though, newforms has much better separation of concerns when
it comes to dealing with inline objects; each inline object is handled
as a discrete form, tied up in a formset wrapper that knows which
object the formset is related to. This means there are a lot more (and
a lot better isolated) points for customization of save behaviour.
Yours,
Russ Magee %-)