"atomic"-ness of hooks should be configureable or removed

[amcclosky]

First of all, thanks for this library. The API is really well done.

However, today I discovered that in #85 the change was made to force hooks to run inside of a transaction, which for many cases is desirable behavior, however one of my uses for lifecycle hooks is to queue background jobs in AFTER_SAVE assuming any calls to the model's save() will either observe the default django orm autocommit behavior or abide by whatever the behavior set by its current context will be.

Forcing a transaction / savepoint that wraps all model hooks using the atomic decorator means you can't safely make a call to an external service(or queue a celery / rq job) and assume the model changes will be visible. For example, it is not unusual for a background job to begin execution before the transaction that queued the job commits.

I'd be happy to open a PR that either reverts #85 or makes the current behavior configureable in some way depending no your preference if you are open to it.

[rsinger86]

Thanks, and understood that this behavior isn't always desired. Does it looks easy to make it configurable? I think it would makes sense to have atomic hooks by default, with the option to disable.

…

On Sun, Jan 30, 2022, 2:38 PM Anthony McClosky ***@***.***> wrote: First of all, thanks for this library. The API is really well done. However, today I discovered that in #85 <#85> the change was made to force hooks to run inside of a transaction, which for many cases is desirable behavior, however one of my uses for lifecycle hooks is to queue background jobs in POST_SAVE assuming any calls to the model's save() will either observe the default django orm autocommit behavior or abide by whatever the behavior set by its current context will be. Forcing a transaction / savepoint that wraps all model hooks using the atomic decorator means you can't safely make a call to an external service(or queue a celery / rq job) and assume the model changes will be visible. For example, it is not unusual for a background job to begin execution before the transaction that queued the job commits. I'd be happy to open a PR that either reverts #85 <#85> or makes the current behavior configureable in some way depending no your preference if you are open to it. — Reply to this email directly, view it on GitHub <#97>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB32I5WFHE6WFIFLE7EQT5TUYWOVZANCNFSM5NEWIORQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[amcclosky]

I'm looking at it right now.

Making it globally configureable looks straightforward which is probably what I will open a PR for.

The "better" solution I may experiment with later would be to add a new set of hooks, POST_*, that complements the existing AFTER_* hooks which execute only after a transaction succeeds.

[rsinger86]

o I like the "post transact" hook idea too, if that seems simple to fit into the code base, we could go that route

…

On Sun, Jan 30, 2022, 2:50 PM Anthony McClosky ***@***.***> wrote: I'm looking at it right now. Making it globally configureable looks straightforward which is probably what I will open a PR for. The "better" solution may experiment with would be to add a new set of hooks, POST_*, that complements the existing AFTER_* hooks which execute only after a transaction succeeds. — Reply to this email directly, view it on GitHub <#97 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB32I5WKYV72Y4EQKA2WAVDUYWP7VANCNFSM5NEWIORQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[rsinger86]

Or instead of a new hook, a new decorator arg for whether it should run during the transaction?

…

On Sun, Jan 30, 2022, 2:52 PM Robert Singer ***@***.***> wrote: o I like the "post transact" hook idea too, if that seems simple to fit into the code base, we could go that route On Sun, Jan 30, 2022, 2:50 PM Anthony McClosky ***@***.***> wrote: > I'm looking at it right now. > > Making it globally configureable looks straightforward which is probably > what I will open a PR for. > > The "better" solution may experiment with would be to add a new set of > hooks, POST_*, that complements the existing AFTER_* hooks which execute > only after a transaction succeeds. > > — > Reply to this email directly, view it on GitHub > <#97 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AB32I5WKYV72Y4EQKA2WAVDUYWP7VANCNFSM5NEWIORQ> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > You are receiving this because you commented.Message ID: > ***@***.***> >

[amcclosky]

I had similar thoughts on the decorator arg. That would be cleaner IMO. I'll see what I can come up with. This is my first time back in Django orm-land in awhile so I am reacquainting myself with how the internals work these days.

[reedjones]

Does this mean that django 4.x support is working now?