[Django] #30022: Doc how to combine post_save signal with on_commit to alter a m2m relation when saving a model instance
Django
when saving a model instance
------------------------------------------------+------------------------
Reporter: George Tantiras | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Trying to alter a many to many relation when saving a model's instance is
a common use case.
[https://stackoverflow.com/a/53608043/2996101 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
[https://docs.djangoproject.com/en/dev/topics/db/transactions/#django.db.transaction.on_commit
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
[https://docs.djangoproject.com/en/dev/ref/signals/#django.db.models.signals.post_save
post_save] signal chapter?
--
Ticket URL: <https://code.djangoproject.com/ticket/30022>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
when saving a model instance
Reporter: George Tantiras | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* status: new => closed
* resolution: => wontfix
Comment:
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
{{{#!python
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30022#comment:1>
Django
when saving a model instance
Reporter: George Tantiras | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by George Tantiras):
I see the point, thank you for sharing that deeper point of view.
--
Ticket URL: <https://code.djangoproject.com/ticket/30022#comment:2>
Django
when saving a model instance
Reporter: George Tantiras | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by George Tantiras):
Replying to [comment:1 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 [https://github.com/raratiru/django-testing-on-commit 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 [https://github.com/raratiru/django-
testing-on-commit/blob/master/user/signals.py user/signals.py] and
[https://github.com/raratiru/django-testing-on-
commit/blob/master/user/test_user.py 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"
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30022#comment:3>
Django
when saving a model instance
Reporter: George Tantiras | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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
[https://docs.djangoproject.com/en/2.1/topics/forms/modelforms/#the-save-
method 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.
{{{#!python
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,
{{{#!python
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30022#comment:4>
Django
when saving a model instance
Reporter: George Tantiras | Owner: nobody
Cleanup/optimization |
Component: Documentation | Version: 2.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by George Tantiras):
Thank you, it is amazing what Django can do!
--
Ticket URL: <https://code.djangoproject.com/ticket/30022#comment:5>