Use a signal handler instead of patching save.#130
Use a signal handler instead of patching save.#130treyhunner merged 1 commit intojazzband:masterfrom
Conversation
References jazzband#83. Instead of patching the save method of a tracked model class, we can use a signal handler on post_save, which means we can still pickle our model class. Note we can't just listen for the signal from the class we have, but instead listen for all post_save signals. This means we actually install a new signal handler for each tracked model class, which fires on all model save occurrences (and returns immediately if this handler doesn't care). We probably could improve this to have a registry of tracked models, or something, that allows us to just install one signal handler, and filter according to membership.
|
I'm fine with this approach. I'll leave it up to @treyhunner as author of FieldTracker to merge if it looks ok to him, too. Thanks for the PR! |
|
Thanks for working on this @schinckel! My only concern with this is the possibility of inconsistent results for signal handlers registered before our If we do find a failing test it could be useful for documenting that behavior and noting it in the docs. Thoughts on testing for incorrect behavior in very early I actually don't see any existing tests for has_changed/previous/changed behavior in |
|
Good point. The only way a signal handler could fire before is if it listened for Writing a test that shows this is possible: the signal handler just needs to be registered before this model is declared (since the declaration of the model will register the signal). Alternatively, someone else registering a signal handler in the same way we do (contribute_to_class) could beat us. Having said all of that, it would depend upon what that signal handler is doing. Most signal handlers may not interfere with what the FieldTracker is doing. |
|
I had a vague memory that this had already been discussed and there was a reason we didn't use a signal, but I didn't go looking for it; thanks @treyhunner. I think the main concern is not so much other signal handlers interfering with That said, it seems unlikely that such a handler would be registered before creation of the model class. So I could see accepting this limitation and documenting it. It does mean that we'd be dependent on the signal-handler-ordering behavior of Django -- is that behavior documented/guaranteed? If not that would be a negative for this approach. |
As far as I can tell that behavior is not documented. I doubt that functionality will ever change though. After thinking this over more I think this is rare enough that we can just document the behavior. Here's my proposal for accepting this solution:
If that plan sounds reasonable I can merge this and update the documentation. |
|
That sounds fine to me. |
|
Apologies @schinckel for having to revert your work, and for not catching the issue before merge. Appreciate the pull request. |
|
Ah, no worries. I’ll just have to look into a solution that works for everything. |
|
I was hoping you'd say that ;) |
References #83.
Instead of patching the save method of a tracked model class, we can use
a signal handler on post_save, which means we can still pickle our model
class.
Note we can't just listen for the signal from the class we have, but
instead listen for all post_save signals. This means we actually install
a new signal handler for each tracked model class, which fires on all
model save occurrences (and returns immediately if this handler doesn't care).
We probably could improve this to have a registry of tracked models, or
something, that allows us to just install one signal handler, and filter
according to membership.