[Django] #29557: add on_commit decorator

Django

unread,

Jul 10, 2018, 7:49:49 AM7/10/18

to django-...@googlegroups.com

#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.

Django

unread,

Jul 10, 2018, 7:53:31 AM7/10/18

to django-...@googlegroups.com

#29557: add on_commit decorator
-------------------------------------+-------------------------------------
Reporter: obayemi | Owner: nobody

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

unread,

Jul 10, 2018, 7:54:22 AM7/10/18

to django-...@googlegroups.com

#29557: add on_commit decorator
-------------------------------------+-------------------------------------
Reporter: obayemi | Owner: nobody

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

unread,

Jul 10, 2018, 8:08:22 AM7/10/18

to django-...@googlegroups.com

#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

unread,

Jul 10, 2018, 8:53:18 AM7/10/18

to django-...@googlegroups.com

#29557: add on_commit decorator
-------------------------------------+-------------------------------------
Reporter: obayemi | Owner: nobody

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

unread,

Jul 10, 2018, 9:44:30 AM7/10/18

to django-...@googlegroups.com

#29557: add on_commit decorator
-------------------------------------+-------------------------------------
Reporter: obayemi | Owner: nobody

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

unread,

Jul 10, 2018, 9:50:56 AM7/10/18

to django-...@googlegroups.com

#29557: add on_commit decorator
-------------------------------------+-------------------------------------
Reporter: obayemi | Owner: nobody

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

unread,

Jul 10, 2018, 10:11:04 AM7/10/18

to django-...@googlegroups.com

#29557: add on_commit decorator
-------------------------------------+-------------------------------------
Reporter: obayemi | Owner: nobody

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

unread,

Jul 10, 2018, 10:15:37 AM7/10/18

to django-...@googlegroups.com

#29557: add on_commit decorator
-------------------------------------+-------------------------------------
Reporter: obayemi | Owner: nobody

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

unread,

Jul 11, 2018, 3:58:59 AM7/11/18

to django-...@googlegroups.com

#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.