Doc how to combine post_save signal with on_commit to alter a m2m relation when saving a model instance

[George Tantiras]

Trying to alter a many to many relation when saving a model's instance is a common use case.

​For example, when creating a new user or altering an already existing user, programmatically add or remove groups he should(n't) belong to.

This can be achieved by catching a post save signal and creating a decorator that uses ​on_commit:

def on_transaction_commit(func):
    ''' Create the decorator '''
    def inner(*args, **kwargs):
        transaction.on_commit(lambda: func(*args, **kwargs))

    return inner


@receiver(
    post_save,
    sender=settings.AUTH_USER_MODEL,
)
@on_transaction_commit
def group_delegation(instance, raw, **kwargs):
    do stuff()

Is it relevant to doc it as an example in the ​post_save signal chapter?

[Simon Charette]

Thank you for the suggestion but it feels like a pretty specific example to document as this is not something you want to do in all cases.

If I understand where you are coming from you want to make sure changes performed in save() are committed to the database in the same transaction as the m2m alterations.

In your "group" case your suggested approach will work fine in cases where save() in not called in within a transaction.atomic() block will have surprising results if this doesn't hold true.

For example in

@transaction.atomic
def accept_group_invite(request, group_id):
    validate_and_add_to_group(request.user, group_id)
    # The below line would always fail in your case because the on_commit
    # receiver wouldn't be called until exiting this function.
    if request.user.has_perm('group_permission'):
        do_something()
    ...

do_something() would never be called because validate_and_add_to_group()'s save() calls that would trigger the post_save receiver would only queue the group additions to be performed on_commit which wouldn't happen until the accept_group_invite function exits.

If you want to make sure both save() and its pre_save and post_save side effects are performed in as an atomic operation (either succeed or fail) then you should simply wrap your save() calls in an atomic block.

[George Tantiras]

I see the point, thank you for sharing that deeper point of view.

[George Tantiras]

Replying to Simon Charette:

do_something() would never be called because validate_and_add_to_group()'s save() calls that would trigger the post_save receiver would only queue the group additions to be performed on_commit which wouldn't happen until the accept_group_invite function exits.

The above is very clear and understood.

I made ​an effort to replicate the below quote:

If you want to make sure both save() and its pre_save and post_save side effects are performed in as an atomic operation (either succeed or fail) then you should simply wrap your save() calls in an atomic block.

In my bare django project I plugged ​user/signals.py and ​user/test_user.py to test under which circumstances the signal will successfully add the saved user instance to the 'Superuser' group.

The result is that both transaction_atomic() and transaction.on_commit() are needed.

I report this, just in case it is an unexpected behavior.

def test_user(group, user_data, superuser_data):
    '''
    If transaction.on_commit() is not used in the receiver, this test will fail.
    '''
    # Add a user
    creation_form = UserCreationForm(data=user_data)
    user = creation_form.save()
    # Make the new user a Superuser
    change_form = UserChangeForm(instance=user, data=superuser_data)
    with transaction.atomic():
        superuser = change_form.save()

    assert group in superuser.groups.all(), "Although is_superuser is True, the user is not in the Superuser group"


def test_user_non_atomic(group, user_data, superuser_data):
    '''
    This will fail because transaction.atomic() is not used.
    '''
    creation_form = UserCreationForm(data=user_data)
    user = creation_form.save()
    change_form = UserChangeForm(instance=user, data=superuser_data)
    superuser = change_form.save()

    assert group in superuser.groups.all(), "Although is_superuser is True, the user is not in the Superuser group"

[Simon Charette]

Hello George,

This is getting in into django-user territory but the test_user test happens to fail if you remove the on_transaction_commit decorator on the post_save receiver because of ​how model forms m2m saving works.

ModelForm.save() start by saving their attached instance, which triggers post_save signals, and then save their m2m values. In your case since superuser_data doesn't include any value for groups then the following chain of actions happen on change_form.save() if you remove the on_transaction_commit decorator.

1. user get assigned superuser_data and its save() method is invoked.
2. The post_save signal is fired and the group gets added.
3. The form saves the groups m2m to an empty set because groups is missing from superuser_data.

You can confirm this is the case by changing your test_user test to the following.

def test_user(group, user_data, superuser_data):
    '''
    This test will fail if on_commit is not used in the signal.
    '''
    creation_form = UserCreationForm(data=user_data)
    user = creation_form.save()
    change_form = UserChangeForm(instance=user, data=superuser_data)
    assert change_form.is_valid()
    with transaction.atomic():
        superuser = change_form.save(commit=False)
        superuser.save()
        assert group in superuser.groups.all()
        superuser.save_m2m()
    assert group not in superuser.groups.all()

When your post_save receiver is decorated with on_transaction_commit the group addition happens to be performed after save_m2m is called which happens to make the test pass.

What you should do to assert user.is_superuser == user.groups.filter(name='superusers').exists() consistency is hook into the m2m_changed signal as well instead of relying on unrelated on_commit behaviour. For example,

@receiver(
    m2m_changed,
    sender=User.groups.through
)
def ensure_groups_consistency(signal, sender, instance, action, pk_set, **kwargs):
    if instance.is_superuser and action in ('pre_remove', 'post_clear'):
        group = Group.objects.get(name='Superusers')
        if action == 'pre_remove' and group.pk in pk_set:
            pk_set.remove(group.pk)
        elif action == 'post_clear':
            instance.groups.add(group)

As I said earlier this is probably better discussed on support channels that here as this is expected behaviour.

[George Tantiras]

Thank you, it is amazing what Django can do!