[Django] #29557: add on_commit decorator

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #29557: add on_commit decorator

Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
               Reporter:  obayemi    |          Owner:  nobody
                   Type:  New        |         Status:  new
  feature                            |
              Component:             |        Version:  master
  Uncategorized                      |       Keywords:  transaction
               Severity:  Normal     |  on_commit decorator
           Triage Stage:             |      Has patch:  0
  Unreviewed                         |
    Needs documentation:  0          |    Needs tests:  0
Patch needs improvement:  0          |  Easy pickings:  0
                  UI/UX:  0          |
-------------------------------------+-------------------------------------
 I recently had to add on_commit hooks with lambdas to some functions
 (mainly email notifications in post_save signals handler)
 That was a lot of boilerplate so I wrote a simple decorator for making a
 function call only trigger on transaction commit

 {{{
 def call_on_commit(f):
     """
     only call the decorated function at transaction commit
     warning the return value will be ignored
     """
     def handle(*args, **kwargs):
         transaction.on_commit(lambda: f(*args, **kwargs))
     return handle
 }}}

 leading to

 {{{
 @call_on_commit
 def send_message(user, context):
     # send message
 }}}
 instead of
 {{{
 def send_message(user, context):
     def _send_message():
         # send message
     on_commit(do_stuff)
 }}}

 I made a PR on github to implement this feature
 https://github.com/django/django/pull/10167

--
Ticket URL: <https://code.djangoproject.com/ticket/29557>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/050.2e5916aa653c4f85994d445150726066%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Uncategorized        |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  0                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  0                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Description changed by obayemi:

Old description:

> I recently had to add on_commit hooks with lambdas to some functions
> (mainly email notifications in post_save signals handler)
> That was a lot of boilerplate so I wrote a simple decorator for making a
> function call only trigger on transaction commit
>
> {{{
> def call_on_commit(f):
>     """
>     only call the decorated function at transaction commit
>     warning the return value will be ignored
>     """
>     def handle(*args, **kwargs):
>         transaction.on_commit(lambda: f(*args, **kwargs))
>     return handle
> }}}
>
> leading to
>
> {{{
> @call_on_commit
> def send_message(user, context):
>     # send message
> }}}
> instead of
> {{{
> def send_message(user, context):
>     def _send_message():
>         # send message
>     on_commit(do_stuff)
> }}}
>
> I made a PR on github to implement this feature
> https://github.com/django/django/pull/10167
New description:

 I recently had to add on_commit hooks with lambdas to some functions
 (mainly email notifications in post_save signals handler)
 That was a lot of boilerplate so I wrote a simple decorator for making a
 function call only trigger on transaction commit

 {{{
 def call_on_commit(f):
     """
     only call the decorated function at transaction commit
     warning the return value will be ignored
     """
     def handle(*args, **kwargs):
         transaction.on_commit(lambda: f(*args, **kwargs))
     return handle
 }}}

 leading to

 {{{
 @call_on_commit
 def send_message(user, context):
     # send message
 }}}
 instead of
 {{{
 def send_message(user, context):
     def _send_message():
         # send message
     on_commit(do_stuff)
 }}}

 I made a PR on github to implement this feature if it seems relevant
 https://github.com/django/django/pull/10167

--

--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.23946bd42f38247afa98ade7b323a703%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Uncategorized        |                  Version:  master
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by obayemi):

 * has_patch:  0 => 1
 * easy:  0 => 1


--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.f10bb1108d1bee1aef7a9f8f0826ed2d%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  1
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Sergei Maertens):

 * needs_docs:  0 => 1
 * component:  Uncategorized => Database layer (models, ORM)
 * needs_tests:  0 => 1


--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.a3e5bbd748ea36fa8c278bbec992787e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  1
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):

 * cc: Carlton Gibson (added)


Comment:

 I think you need to explain the usage more fully here.

 I'm expecting this sort of thing:

 {{{
 def send_mail()
     ...

 with transaction.atomic():
     Thing.objects.create(num=num)
     transaction.on_commit(send_mail)
 }}}

 You need to explain how the decorator would work.

 From the docs:

 > If you call on_commit() while there isn’t an active transaction, the
 callback will be executed immediately.

 So you can't just decorate `send_mail()` at the point of declaration here.
 So what would your code look like?

--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.50f595131c6d7fc91dcea4fe41ba39d5%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  1
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by obayemi):

 Replying to [comment:4 Carlton Gibson]:
 > I think you need to explain the usage more fully here.
 >
 > I'm expecting this sort of thing:
 >
 > {{{
 > def send_mail()
 >     ...
 >
 > with transaction.atomic():
 >     Thing.objects.create(...)
 >     transaction.on_commit(send_mail)
 > }}}
 >
 > You need to explain how the decorator would work.
 >
 > From the docs:
 >
 > > If you call on_commit() while there isn’t an active transaction, the
 callback will be executed immediately.
 >
 > So you can't just decorate `send_mail()` at the point of declaration
 here. So what would your code look like?

 The decorator don't run the function at the declaration, it only makes
 every usage run at transaction commit, and it would be completely
 transparent for the user as even if no transaction is active it would
 onlybe run synchronously as you stated from the doc
 Manually calling on_commit(function) can work, but it adds some
 boilerplate if your function should always be run at the end of
 transaction, and it is easy to forget

 The decorator would be usefull in two cases
 1) when you have to automatically add every invocation of the function to
 the commit hook instead of running it instantly
 2) when your function is ran by django e.g. post_save on creation when you
 did not finshed filling all the items in a m2m field

 for example, one my actual use case is that I have callbacks that get
 called in post_save in a complex objects that needs a lot of data in
 related objects that with foreignkeys to the objects that was created
 like if you wanted to send a message at every user in a group at creation
 with a post_save signal
 {{{
 with transaction.atomic():
     group = Group.objects.create()
     group.users.add(Users.objects.all()
 }}}
 You would have to do
 {{{
 @receiver(post_save, sender=Group)
 def notify_users(sender, instance, created, *args, **kwargs):
     def _notify_users():  # defining inner function to remove the need to
 pass parameters from context since they can't be used in on_commit hook
         if created:
             for user in group.users.all():
                 user.notify_group_creation(group)

     # without on_commit, there wouldn't be any users in the group yet
     on_commit(_notify_users)
 }}}

 where the goal of my decorator is to remove the need to declare an inner
 function and call it in the on_commit hook explicitely which is useless
 boilerplate and indent the whole function one step more if the whole
 function should be ran after the commit
 using this decorator would allow only writing the receiver as
 {{{
 @receiver(post_save, sender=Group)
 @call_on_commit
 def notify_users(sender, instance, created, *args, **kwargs):
     if created:
         for user in group.users.all():
             user.notify_group_creation(group)
 }}}

--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.ed458d60f94d393305b5e2f0fa6ae46b%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  1
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Carlton Gibson):

 Hmmm. OK. I see. My immediate thought there is that you would be MUCH
 better avoiding the post save signal and just using the on_commit hook
 straight-forwardly. The control flow would be significantly clearer.

 I'm inclined towards closing this as `wontfix` on that basis. I'll leave
 some time for others to comment though.

--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.a08312a475ffc3212859f9aaaecb4e55%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  1
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by obayemi):

 I agree that the workflow could be better without the post_save, but it
 ease the dev a lot IMHO
 that way we can't forget to call the handler everywhere the Models can be
 created and it is simpler to think, (these will be always be called at
 creation, don't need to think about it (isn't this why signals are
 implemented anyway?))

--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.736617032ff558c2c1d06de744cf26bf%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  new
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  1
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Carlton Gibson):

 > isn't this why signals are implemented anyway?

 Not really. There's a good article on it here:
 [https://lincolnloop.com/blog/django-anti-patterns-signals/ Django Anti-
 Patterns: Signals]

 You really are better off explicitly calling the handler in the places you
 need it. (They'll be few in reality.)

 However, that's an off-topic discussion for here... 🙂

--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.6026713722ceb3c5258a934b4edee399%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29557: add on_commit decorator

Django
In reply to this post by Django
#29557: add on_commit decorator
-------------------------------------+-------------------------------------
     Reporter:  obayemi              |                    Owner:  nobody
         Type:  New feature          |                   Status:  closed
    Component:  Database layer       |                  Version:  master
  (models, ORM)                      |
     Severity:  Normal               |               Resolution:  wontfix
     Keywords:  transaction          |             Triage Stage:
  on_commit decorator                |  Unreviewed
    Has patch:  1                    |      Needs documentation:  1
  Needs tests:  1                    |  Patch needs improvement:  0
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):

 * status:  new => closed
 * resolution:   => wontfix


Comment:

 Hi @obayemi.

 Having considered this more I'm going to go with `wontfix`.

 You're obviously welcome to use a decorator such as this in your own
 project but, using `on_commit()` directly is going to result in
 significantly clearer code.

 In your example, do this:


 {{{
 with transaction.atomic():
     group = Group.objects.create()
     group.users.add(Users.objects.all()
     transaction.on_commit(notify_users)
 }}}

 By shipping a call_on_commit decorator we'd be guiding people towards
 writing less clear code. I think we're not doing anyone any favours there.

 Thanks for the input!

--
Ticket URL: <https://code.djangoproject.com/ticket/29557#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/065.e21380270e293506f962c07b546c8e97%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.