add_to_revision() in other middleware.py does not save

[pybean]

Hello,
I created my own django middleware.py, and simply called "add_to_revision(instance)" in its process_request() method. I see that my middleware works without any exceptions, but I do not see the new Version instance created by the "add_to_revision" call in database. This middleware worked fine when I was using reversion 2.0.8, and then it stops storing version instance after upgrading the reversion module to 2.0.10.

Please let me know if you need more information. Thanks!

[etianen]

1. Are you also using RevisionMiddleware? 2. Is your middleware old-style (class-based), or new-style (functional)? 3. Did you upgrade Django at the same time as upgrading reversion? It would be useful to see your actual middleware code, if possible.

…

On 12 September 2017 at 17:18, pybean ***@***.***> wrote: Hello, I created my own django middleware.py, and simply called "add_to_revision(instance)" in its process_request() method. I see that my middleware works without any exceptions, but I do not see the new Version instance created by the "add_to_revision" call in database. This middleware worked fine when I was using reversion 2.0.8, and then it stops storing version instance after upgrading the reversion module to 2.0.10. Please let me know if you need more information. Thanks! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#664>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJFCOopVRIKXxfb_gujFeGUwAT52plNks5shq4qgaJpZM4PU53l> .

[pybean]

Thanks for the reply.

1. Yes, I am using RevisionMiddleware, so I could simply call add_to_revision without using additional create_revision() block and handle the rest.
2. my middleware is old-style.
3. No, I am still using django 1.8.

Please check my code below. The purpose is to backup the latest snapshot before the instance deletion. Thanks again!

def backup_pre_delete_info(sender, instance, **kwargs):
    if reversion.is_registered(sender):
        reversion.add_to_revision(instance)

class MyMiddleware(object):
    def process_request(self, request):
        if not request.method in ('GET', 'HEAD', 'OPTIONS', 'TRACE'):
            reversion.set_user(request.user)
            signals.pre_delete.connect(backup_pre_delete_info, weak=False)

[etianen]

This doesn't look like a very good fit for middleware. Why not register the signal handler in your models.py file?

…

On 12 September 2017 at 18:39, pybean ***@***.***> wrote: Thanks for the reply. 1. Yes, I am using RevisionMiddleware, so I could simply call add_to_revision without using additional create_revision() block and handle the rest. 2. my middleware is old-style. 3. No, I am still using django 1.8. Please check my code below. The purpose is to backup the latest snapshot before the instance deletion. Thanks again! def backup_pre_delete_info(sender, instance, **kwargs): if reversion.is_registered(sender): reversion.add_to_revision(instance) class MyMiddleware(object): def process_request(self, request): if not request.method in ('GET', 'HEAD', 'OPTIONS', 'TRACE'): reversion.set_user(request.user) signals.pre_delete.connect(backup_pre_delete_info, weak=False) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#664 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJFCBYCYYlicTerCTI4UTXW477xmMTcks5shsGpgaJpZM4PU53l> .

[etianen]

I've dug deeper. It looks like this commit is to blame: 71ad022 Reversion now includes a check that a model still exists in the database before adding it to the revision. This is to fix cases of models being updated and deleted in the same revision that can crop up in the admin interface. There isn't much point in saving a backup copy before instance deletion, since every time the instance is saved it will be saved by reversion anyway. All you're doing is saving a duplicate copy in the reversion database.

…

On 12 September 2017 at 18:44, Dave Hall ***@***.***> wrote: This doesn't look like a very good fit for middleware. Why not register the signal handler in your models.py file? On 12 September 2017 at 18:39, pybean ***@***.***> wrote: > Thanks for the reply. > > 1. Yes, I am using RevisionMiddleware, so I could simply call > add_to_revision without using additional create_revision() block and handle > the rest. > 2. my middleware is old-style. > 3. No, I am still using django 1.8. > > Please check my code below. The purpose is to backup the latest snapshot > before the instance deletion. Thanks again! > > def backup_pre_delete_info(sender, instance, **kwargs): > if reversion.is_registered(sender): > reversion.add_to_revision(instance) > > class MyMiddleware(object): > def process_request(self, request): > if not request.method in ('GET', 'HEAD', 'OPTIONS', 'TRACE'): > reversion.set_user(request.user) > signals.pre_delete.connect(backup_pre_delete_info, weak=False) > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#664 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAJFCBYCYYlicTerCTI4UTXW477xmMTcks5shsGpgaJpZM4PU53l> > . >

[pybean]

Thank you so much for this clear explanation.
I understand that in general we don't have to save a backup copy before instance deletion. But for my case:

1. My reversion_* tables are growing pretty big, so I need to archive old reversion data.
2. Able to trace who, when, why the instance was deleted
3. Grouping all related instances from cascade delete, and/or other modified instances in a single revision (my favorite part)

I am trying to avoid using signal handler in model.py, because I have to handle create_revision block and transaction.atomic for different situations, and your RevisionMiddleware already takes care about this.

Do you think is it okay to stick with 2.0.8? Please advise if it has any limitations, or I shouldn't use it at all. Thanks!

[etianen]

2.0.8 is fine. Stick with that if you need this functionality. There seems to be no point registering the signal handler in your middleware, though. Once a hander is registered, it stays registered for the whole app. You're just re-registering it once per request. I still don't think you need to save a revision on delete, though. In terms of your points: 1. If you need to archive old revision data, consider using `./manage.py deleterevisions --keep=1 --days=30`. That will always keep at least one revision per object, purging ones older than 30 days. That way, you have no reason to save a revision on delete, since each model will keep at least one entry. 2. Consider using a dedicated table for tracking who deleted something. Or, use the admin.LogEntry model for this, with a signal handler. 3. Won't all related instances already be grouped in a revision when any of them are modified? Thing is, reversion used to have the behaviour you've added, but it was removed in 1.8 for a number of good reasons: #164 It's your project, though. Just be aware of some possible side-effects.

…

On 12 September 2017 at 19:33, pybean ***@***.***> wrote: Thank you so much for this clear explanation. I understand that in general we don't have to save a backup copy before instance deletion. But for my case: 1. My reversion_* table is growing pretty big, so I need to archive old reversion data. 2. Able to trace who, when, why the instance was deleted 3. Grouping all related instances from cascade delete, and/or other modified instances in a single revision (my favorite part) I am trying to avoid using signal handler in model.py, because I have to handle create_revision block and transaction.atomic for different situations, and your RevisionMiddleware already takes care about this. Do you think is it okay to stick with 2.0.8? Please advise if it has any limitations, or I shouldn't use it at all. Thanks! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#664 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJFCII8N7-IgV_QWuFFAB364ZCrXpa4ks5shs6WgaJpZM4PU53l> .

[pybean]

Thank you for the fast reply, then I will stick with 2.0.8 for the time being.

What I pasted was the partial code. I disconnect those signals in process_response() with unique identifiers so it works as expected. Still thanks for your kind reminder.

There are complicated models in my project. Sometimes purging an top level instance will remove more than 100 related instances triggered by cascade delete. So I needed a complete snapshot for all deleted instances triggered by a single delete call, and logging (when/who/why) is just a bonus because the Revision model already has all those fields.

I wonder if you could add an option for create_revision block, to ignore the new check that a model still exists in the database before adding it to the revision. Please consider. Thanks!