Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flush with reverse m2m changed signal #386

Merged

Conversation

k4rl85
Copy link
Contributor

@k4rl85 k4rl85 commented Jan 14, 2021

Fix issue #377

try:
instance.flush()
except AttributeError:
for flag in get_waffle_flag_model().objects.filter(pk__in=kwargs['pk_set']):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this is necessary, please? Is there anyway to do this without a database call?

Copy link
Contributor Author

@k4rl85 k4rl85 Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Clinton, first of all thx for the review on this patch.

The problem here is m2m_changed handling from Django.

If i execute the following code

from my_app.models import MyUserModel
from waffle.models import Flag

flag = Flag.objects.first()
user = MyUserModel.objects.first()

flag.users.add(user) # all fine, cache is flushed without errors

user.flag_set.add(flag) # crash

the last line will crash because the instance received by flag_membership_changed is a MyUserModel instance instead of a Flag instance.

The only solution that i can see when instance is of type user/group is to retrieve all the flag involved and flush them like i do here

The reason why i query the db is that is not possible to guarantee that the flag involved are still accessible from instance when flag_membership_changed is executed.

Let me know if you want some of this explanation in the code.

I have added 2 tests to ensure that the m2m_changed signal is tested when the change are performed from the User/Group instance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the code to use reverse instead of try/except to be more explicit of what is happening here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Would you mind adding a comment with a version of this explanation, too?

Also, instead of checking for a kwarg, what if you checked: if isinstance(instance, get_waffle_flag_model())? That seems a bit safer than relying on the kwarg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have applied the required changes.

@k4rl85 k4rl85 force-pushed the fix-flush-with-reverse-m2m-changed-signal branch from 0fcf22c to 917f573 Compare January 18, 2021 14:49
try:
instance.flush()
except AttributeError:
for flag in get_waffle_flag_model().objects.filter(pk__in=kwargs['pk_set']):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Would you mind adding a comment with a version of this explanation, too?

Also, instead of checking for a kwarg, what if you checked: if isinstance(instance, get_waffle_flag_model())? That seems a bit safer than relying on the kwarg.

@k4rl85 k4rl85 force-pushed the fix-flush-with-reverse-m2m-changed-signal branch from 598992a to da67d2e Compare January 19, 2021 12:12
@clintonb clintonb merged commit 9c0c285 into jazzband:master Jan 19, 2021
@k4rl85
Copy link
Contributor Author

k4rl85 commented Jan 20, 2021

Thx @clintonb for the merge.

What are the policy for this repository to have a new release?

Unfortunately this bug impact my production code and i would avoid if possible to install django-waffle from github.

@clintonb
Copy link
Collaborator

@k4rl85 new release is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants