Model post_save doesn't play well with Model.save overriding

[Jeremy Dunck]

I have a few places that override Model.save to do additional work
after the normal functionality.

I needed to add a post_save handler to do even more stuff after that
additional work, but ran into the obvious problem: post_save is
called at the end of Model.save, and that's before the additional work
in the override was done.

Example:
============
class MyModel(Model):
def save(self):
super(MyModel, self).save()
#extra work here

def even_more(sender,instance):
#stuff that depends on extra work having already occurred

dispatcher.connect(post_save, even_more_stuff)
==============

I feel OK about having to manually call post_save when I override
Model.save, but adding it now just results in post_save being called
twice.

Perhaps Model.save(raw=False) should become Model.save(raw=False,
fire_signals=True), leaving it up to the overrider to decide the
policy?

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Collin Grady]

Post by Jeremy Dunck
I feel OK about having to manually call post_save when I override
Model.save, but adding it now just results in post_save being called
twice.

Couldn't you just fire your own signal? :)

[James Bennett]

Post by Jeremy Dunck
Perhaps Model.save(raw=False) should become Model.save(raw=False,
fire_signals=True), leaving it up to the overrider to decide the
policy?

Personally, I'm comfortable with documenting the fact that if
Model.save() is never called, the pre_save and post_save signals will
never fire (same is true of overridden __init__() and the post_init
signal, and of overridden delete() and the pre_delete and post_delete
signals).

[Jeremy Dunck]

Post by James Bennett

Post by Jeremy Dunck
Perhaps Model.save(raw=False) should become Model.save(raw=False,
fire_signals=True), leaving it up to the overrider to decide the
policy?

Personally, I'm comfortable with documenting the fact that if
Model.save() is never called, the pre_save and post_save signals will
never fire (same is true of overridden __init__() and the post_init
signal, and of overridden delete() and the pre_delete and post_delete
signals).

Oh, I do want to call Model.save, I just don't want post_save to fire
until MyModel.save is done.

But Collin's point about just making a new signal is fair.

I'm just concerned about composability (C inherits B inherits A), but
I can live without it for now. If we get model subclassing working,
then this will definitely be an issue, but I doubt it'll be the only
one to work through. ;-)

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Gary Wilson]

Post by Jeremy Dunck

Post by James Bennett
Personally, I'm comfortable with documenting the fact that if
Model.save() is never called, the pre_save and post_save signals will
never fire (same is true of overridden __init__() and the post_init
signal, and of overridden delete() and the pre_delete and post_delete
signals).

Oh, I do want to call Model.save, I just don't want post_save to fire
until MyModel.save is done.
But Collin's point about just making a new signal is fair.
I'm just concerned about composability (C inherits B inherits A), but
I can live without it for now. If we get model subclassing working,
then this will definitely be an issue, but I doubt it'll be the only
one to work through. ;-)

How about introducing pre_save and/or post_save methods. The signal firing
happens in pre_save and post_save. You can override whatever you want -
pre_save, save, or post_save depending on your needs. This would also allow
you to disable the firing of signals by overriding pre/post_save with a no-op.

Gary

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[James Bennett]

Post by Gary Wilson
How about introducing pre_save and/or post_save methods. The signal firing
happens in pre_save and post_save. You can override whatever you want -
pre_save, save, or post_save depending on your needs. This would also allow
you to disable the firing of signals by overriding pre/post_save with a no-op.

We had that, once upon a time, and it was probably more complex than
it needed to be. Plus, you'd probably need to have save() call
pre_save() and post_save() in order to make sure they all get called,
which gets back into the problem of what happens when you override
save().

[Gary Wilson]

Post by James Bennett

Post by Gary Wilson
How about introducing pre_save and/or post_save methods. The signal firing
happens in pre_save and post_save. You can override whatever you want -
pre_save, save, or post_save depending on your needs. This would also allow
you to disable the firing of signals by overriding pre/post_save with a no-op.

We had that, once upon a time, and it was probably more complex than
it needed to be. Plus, you'd probably need to have save() call
pre_save() and post_save() in order to make sure they all get called,
which gets back into the problem of what happens when you override
save().

I would think most use cases for overriding save() involve doing something
before or after the actual saving of the object. In other words, having a
pre_save() and post_save() means that few people would need to override save()
at all.

Gary

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Malcolm Tredinnick]

On Tue, 2007-11-13 at 22:58 -0600, Gary Wilson wrote:
[...]

Post by Gary Wilson
I would think most use cases for overriding save() involve doing something
before or after the actual saving of the object. In other words, having a
pre_save() and post_save() means that few people would need to override save()
at all.

Your assessment of common use-cases is quite possibly correct, but it's
already supported by the current code. Reintroducing pre_save and
post_save doesn't remove the requirement that a developer can override
save() -- now that the raw parameter exists -- so it's adding two extra
methods that must always be called (or checked to see if they exist)
without simplifying the root problem.

Jeremy's still got a valid problem, though, but I'm not sure
reintroducing pre_save() and post_save() is a necessary step yet.
Hopefully there's something else we can do.

Malcolm
--
I've got a mind like a... a... what's that thing called?
http://www.pointy-stick.com/blog/


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Marty Alchin]

Post by Malcolm Tredinnick
Jeremy's still got a valid problem, though, but I'm not sure
reintroducing pre_save() and post_save() is a necessary step yet.
Hopefully there's something else we can do.

I wonder if there's a way to modify PyDispatcher (since we're
distributing our own copy anyway) so that it allows dispatching of a
particular signal to be suspended temporarily. That way, dispatched
signals might go into a queue, perhaps and get sent in order when the
signal is reactivated. Then, Jeremy's code might look something like
this:

from django.dispatch import dispatcher
from django.db.models import signals

class MyModel(Model):
def save(self):
queue = dispatcher.queue(signals.post_save)
super(MyModel, self).save()

#extra work here

queue.dispatch()

def even_more(sender,instance):
#stuff that depends on extra work having already occurred

dispatcher.connect(signals.post_save, even_more_stuff)

I'm not sure if this would work as well as I'm thinking, and the
semantics I listed there are just a rough guess at how it could be
done, but it seems reasonable to me. In the above example, when
queue.dispatch() is called, all queued dispatches are sent as
originally intended, and future dispatches are no longer routed to the
queue. So basically it would do everything necessary to pretend that
it had never been queued in the first place.

Another approach would be to have a pair of module-level functions,
rather than a queue object (again, names are not important):

dispatcher.suspend(signals.post_save)
dispatcher.activate(signals.post_save)

But those semantics seem like they'd require the signal to be
dispatched manually after reactivating it, and that would be bad if
the dispatch itself ever changed inside Model.save(). Fragile
base-class problem and all.

Plus, having a queue object allows for potentially more interesting
usage, such as being able to inspect what dispatches were sent, before
they actually get called. I don't know if that would do anybody any
good, but it's an added bonus that it'd be possible.

I dunno, that's all just off the top of my head, but maybe it could work.

-Gul

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Marty Alchin]

And, of course, adding a queue to the dispatcher would decouple this
solution from save() and make it usable in any signal handling code.

-Gul

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Forest Bond]

Post by Marty Alchin

Post by Malcolm Tredinnick
Jeremy's still got a valid problem, though, but I'm not sure
reintroducing pre_save() and post_save() is a necessary step yet.
Hopefully there's something else we can do.

I wonder if there's a way to modify PyDispatcher (since we're
distributing our own copy anyway) so that it allows dispatching of a
particular signal to be suspended temporarily. That way, dispatched
signals might go into a queue, perhaps and get sent in order when the
signal is reactivated. Then, Jeremy's code might look something like
from django.dispatch import dispatcher
from django.db.models import signals
queue = dispatcher.queue(signals.post_save)
super(MyModel, self).save()
#extra work here
queue.dispatch()
#stuff that depends on extra work having already occurred
dispatcher.connect(signals.post_save, even_more_stuff)

This is neat and all, but I don't think at actually solves the problem at hand.
If save is not overridden, when does queue.dispatch() get called?

The only way I can think of overcoming this is to dynamically modify self.save
when it is accessed (by overriding __getattr__). The signals are fired from
code that is dynamically appended to the save method. This guarrantees that the
signal is fired at the very end of the save call:

--------------------------------------------------------------------------------
class Model(object):
def save(self, ...):
...

def __getattr__(self, name):
value = super(Model, self).__getattr__(name)
if name == 'save':
def save(self, *args, **kwargs):
retval = value(self, *args, **kwargs)
send_postsave_signal()
return retval
return save
retun value
--------------------------------------------------------------------------------

This adds some overhead, of course, but it does do (roughly) what is being
asked.

-Forest

[Marty Alchin]

Post by Forest Bond
This is neat and all, but I don't think at actually solves the problem at hand.
If save is not overridden, when does queue.dispatch() get called?

If save isn't overridden, queue.dispatch() wouldn't be called, because
the queue wouldn't even exist. Signals would just be dispatched the
same way they normally are. Even most overrides of save() still
wouldn't need to deal with the queue, it'd only be necessary if
someone actively needed to delay dispatches in code they don't
control.

Post by Forest Bond
The only way I can think of overcoming this is to dynamically modify self.save
when it is accessed (by overriding __getattr__). The signals are fired from
code that is dynamically appended to the save method. This guarrantees that the

You're right, this adds extra overhead, and I don't think it's
necessary, to be honest. Yeah, using the queue does imply some risk
that somebody might set up a queue and not dispatch it, or that they
might forget to use the queue in the first place if they need it, but
it's not the responsibility of the framework to prevent people from
making coding mistakes. Down that path is Java.

-Gul

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Forest Bond]

Hi,

Post by Marty Alchin

Post by Forest Bond
This is neat and all, but I don't think at actually solves the problem at hand.
If save is not overridden, when does queue.dispatch() get called?

If save isn't overridden, queue.dispatch() wouldn't be called, because
the queue wouldn't even exist. Signals would just be dispatched the
same way they normally are. Even most overrides of save() still
wouldn't need to deal with the queue, it'd only be necessary if
someone actively needed to delay dispatches in code they don't
control.

Ah, I wasn't following that signals are only queued if the queue is explicitly
engaged. Now I get it.

Post by Marty Alchin

Post by Forest Bond
The only way I can think of overcoming this is to dynamically modify self.save
when it is accessed (by overriding __getattr__). The signals are fired from
code that is dynamically appended to the save method. This guarrantees that the

You're right, this adds extra overhead, and I don't think it's
necessary, to be honest. Yeah, using the queue does imply some risk
that somebody might set up a queue and not dispatch it, or that they
might forget to use the queue in the first place if they need it, but
it's not the responsibility of the framework to prevent people from
making coding mistakes. Down that path is Java.

Nah, I'm not after protecting programmers from themselves, I just missed a bit
of sophistication in your queue.

I'm not entirely convinced the overhead is all that terrible, especially given
that:

* Database writes are less frequent than reads.
* Anybody overriding save is probably adding more overhead than this anyway.

Maybe we could only engage this slight trickery if we detect that save has been
overridden by a sub-class of Model? Then, if save hasn't been touched, there is
no additional overhead.

-Forest

[Marty Alchin]

Post by Forest Bond
I'm not entirely convinced the overhead is all that terrible, especially given
* Database writes are less frequent than reads.
* Anybody overriding save is probably adding more overhead than this anyway.
Maybe we could only engage this slight trickery if we detect that save has been
overridden by a sub-class of Model? Then, if save hasn't been touched, there is
no additional overhead.

It still seems unnecessary to engage in this activity in core itself.
Like anything else, *if* someone actually finds the need for this (as
Jeremy has), they're free to do it themselves. I see no reason to make
assumptions about what users will be doing with their overridden
save() methods.

I expect most cases won't even care when post_save is called, and
still more will actively expect it to be sent when they call
super(MyModel, self).save(), and I think it's best not to break that.
So, it's probably the minority of cases where this is necessary
anyway, and people in those situations will know they need it. As long
as we make it available and document it, it should suffice, I would
think.

-Gul

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Jeremy Dunck]

On Nov 14, 2007 8:37 AM, Forest Bond <forest-B/***@public.gmane.org> wrote:
...

Post by Forest Bond
This is neat and all, but I don't think at actually solves the problem at hand.
If save is not overridden, when does queue.dispatch() get called?

The queue.dispatch is only necessary if dispatcher.queue has been
invoked; if save is not overridden, dispatcher.queue is not invoked.
Right?

Even so, it seems to me that the queue approach would need to be thread-local.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Marty Alchin]

Post by Jeremy Dunck
The queue.dispatch is only necessary if dispatcher.queue has been
invoked; if save is not overridden, dispatcher.queue is not invoked.
Right?

Correct.

Post by Jeremy Dunck
Even so, it seems to me that the queue approach would need to be thread-local.

I admit, I don't have the necessary threading knowledge to know the
answer to this, but I would assume so. I also don't know what all it
would take to make sure it is.

-Gul

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Marty Alchin]

Post by Marty Alchin
I dunno, that's all just off the top of my head, but maybe it could work.

Well, I did a little digging and put together something that seems to
work. And when I say "seems to work", I mean that there are almost 100
lines of doctests for about 30 lines of code, and it all works as
expected. I expect there are some corner cases the tests don't cover
yet, but that's why I'm sending it around here instead of just
throwing it up on the wiki.

And no, I'm not making a ticket for it yet, because it occurred to me
while working on it, that it doesn't require any changes to
PyDispatcher itself. That means, Jeremy, that you can just drop this
somewhere and use it as I described earlier. Of course, if the rest of
you think it might be useful for other people, I'll see if I can track
down any corner cases that might break, and put it up on
djangosnippets or something.

Of course, it'd feel a bit cleaner if it were part of our PyDispatcher
distribution, since it reaches in and modifies a module-level variable
to make things work right. But, for the few who need it, I think it'd
be fine to have it as an external file.

Also, I apologize if Google Groups doesn't play nice with attachments.
I'll put it somewhere if that's a problem.

-Gul

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---

[Jeremy Dunck]

On Nov 14, 2007 3:00 PM, Marty Alchin <gulopine-yBTmmr1ai6q+***@public.gmane.org> wrote:
...

Post by Marty Alchin
And no, I'm not making a ticket for it yet, because it occurred to me
while working on it, that it doesn't require any changes to
PyDispatcher itself. That means, Jeremy, that you can just drop this
somewhere and use it as I described earlier. Of course, if the rest of
you think it might be useful for other people, I'll see if I can track
down any corner cases that might break, and put it up on
djangosnippets or something.

Neat. Definitely not threadsafe, but we're prefork for now anyway. I
don't even know if pydispatcher is threadsafe to begin with. :)
Anyway, thanks for the work.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-developers-/***@public.gmane.org
To unsubscribe from this group, send email to django-developers-unsubscribe-/***@public.gmane.org
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---