-
-
Notifications
You must be signed in to change notification settings - Fork 73
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] Invalidate org membership cache when organization is_active flag is changed #357 #379
[fix] Invalidate org membership cache when organization is_active flag is changed #357 #379
Conversation
When the status ( is_active ) of an organization changes it now invalidates the cache of all the organizationsusers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kaushikaryan04 for contributing!
The code is not formatted according to our standards and the build will surely fail the QA checks. Please read here how to run QA checks locally and how to reformat the code with black
: https://openwisp.io/docs/developer/contributing.html#python-code-conventions.
We need to add an automated test for this which asserts that changing the is_active
field of an organization invalidates the cache. Look in the test suite for test code doing similar assertion for the invalidation of the user cache when user details are changed.
openwisp_users/apps.py
Outdated
@@ -98,11 +98,18 @@ def set_default_settings(self): | |||
def connect_receivers(self): | |||
OrganizationUser = load_model('openwisp_users', 'OrganizationUser') | |||
OrganizationOwner = load_model('openwisp_users', 'OrganizationOwner') | |||
Organization = load_model("openwisp_users" , "Organization") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use single quotes ('
), please maintain consistency, applies to all other changes.
openwisp_users/apps.py
Outdated
return | ||
if getattr(old_obj , "is_active") != getattr(instance , "is_active") : | ||
for user in OrganizationUsers.objects.filter(organization = instance): | ||
cls._invalidate_user_cache(user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the database is big enough, this task can take enough time to cause the webserver to time out.
We need to move this logic to a background celery task.
We have a number of examples in other modules, eg: https://github.com/openwisp/openwisp-controller/blob/master/openwisp_controller/config/tasks.py#L75-L80
Ideally the celery task calls a class method in the model which takes care of the rest.
openwisp_users/apps.py
Outdated
logger.exception(f"An error occurred while fetching the organization: {e}") | ||
return | ||
if getattr(old_obj , "is_active") != getattr(instance , "is_active") : | ||
for user in OrganizationUsers.objects.filter(organization = instance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must not load the entire set in memory as it can crash big installations, to do that we use the iterator()
method of the Django ORM, eg:
for user in OrganizationUsers.objects.filter(organization = instance): | |
for user in OrganizationUsers.objects.filter(organization = instance).iterator(): |
openwisp_users/apps.py
Outdated
except ObjectDoesNotExist: | ||
logger.warning(f"Organization with pk {instance.pk} does not exist.") | ||
return | ||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this block can be removed
openwisp_users/apps.py
Outdated
OrganizationUsers = load_model("openwisp_users" ,"OrganizationUser") | ||
try: | ||
old_obj = Organization.objects.get(pk=instance.pk) | ||
except ObjectDoesNotExist: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except ObjectDoesNotExist: | |
except Organization.DoesNotExist: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey sorry for all that inconsistency should have read the guidelines.
I make these changes and make another commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, ping me in the dev chat when it's ready for review again.
… 1 more than expected
I added the suggested changes but the some tests are failing as the no of running queries is one more than expected
And one testcase which is failing is this. I am working on this it is failing because of the line which is invalidating cache in tasks.py which I am working on but any suggestion are appreciated.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up @kaushikaryan04, you can increase the query count.
Please follow our commit message guidelines or the QA checks for the CI build will fail (please see the CI build results below).
I will need more time to look into the outstanding issue.
Okay I will change the query count and will follow the commit message guidelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I will change the query count and will follow the commit message guidelines. About the Issue I have tried a lot of things. If you find something out let me know.
Please resolve the issues I pointed out, I will look into to help on any outstanding issue once those are sorted out.
I made a pre_save signal that calls a celery task that would invalidate cache of all users in the organization if the status is changed. But it is failing a test case which i have mentioned. Fixes openwisp#357
I have made a commit with those issues solved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the test which is failing is slightly flawed, it seems to me the organization is disabled when it executes. I am still investigating. But your patch looks good, I left some more comments to keep improving it. I will keep you informed about my investigation on the failing test.
openwisp_users/apps.py
Outdated
@classmethod | ||
def handle_organization_update(cls, instance, **kwargs): | ||
try: | ||
old_instance = instance._meta.model.objects.get(pk=instance.pk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old_instance = instance._meta.model.objects.get(pk=instance.pk) | |
old_instance = Organization.objects.only('is_active').get(pk=instance.pk) |
Calling only
should be more efficient.
openwisp_users/apps.py
Outdated
def handle_organization_update(cls, instance, **kwargs): | ||
try: | ||
old_instance = instance._meta.model.objects.get(pk=instance.pk) | ||
except instance._meta.model.DoesNotExist: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except instance._meta.model.DoesNotExist: | |
except Organization.DoesNotExist: |
openwisp_users/tasks.py
Outdated
|
||
|
||
@shared_task | ||
def organization_update_task(organization_pk): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to invalidate_org_membership_cache
openwisp_users/tasks.py
Outdated
@shared_task | ||
def organization_update_task(organization_pk): | ||
""" | ||
Invalidates cache of users when organization become inactive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalidates cache of users when organization become inactive | |
Invalidates organization membership cache of all users of an | |
organization when organization.is_active changes | |
(organization is disabled or enabled again). |
openwisp_users/tasks.py
Outdated
""" | ||
Invalidates cache of users when organization become inactive | ||
""" | ||
OrganizationUsers = load_model('openwisp_users', 'OrganizationUser') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can place this on the top of the file, after User
.
Refactored the code, changed name of a function that describes it better and made a query more efficient.
Made the changes suggested by you. Thanks for guiding line by line. I will try to understand more about this project meanwhile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for following up and welcome @kaushikaryan04.
Upon inspection again with fresh eyes today I found a few more details that can be improved/simplified. See below.
openwisp_users/tasks.py
Outdated
""" | ||
OrganizationUsers = load_model('openwisp_users', 'OrganizationUser') | ||
qs = OrganizationUsers.objects.filter( | ||
qs = OrganizationUser.objects.filter( | ||
organization_id=organization_pk | ||
).select_related('user') | ||
User = get_user_model() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant, User
is already imported at the top of the file.
openwisp_users/apps.py
Outdated
pre_save.connect( | ||
self.handle_organization_update, | ||
sender=Organization, | ||
dispatch_uid='handle_organization_is_active_change', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatch_uid='handle_organization_is_active_change', | |
dispatch_uid='handle_org_is_active_change', |
openwisp_users/tasks.py
Outdated
if not isinstance(user, User): | ||
user = user.user | ||
user._invalidate_user_organizations_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is all this really necessary? Why do you feel the need of checking if user.user is an instance of User
? Could it be anything different? Let me know.
I think this could be simplified as following:
if not isinstance(user, User): | |
user = user.user | |
user._invalidate_user_organizations_dict() | |
user._invalidate_user_organizations_dict() |
If I am missing something let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the if condition can be removed but I cannot do this
user._invalidate_user_organizations_dict()
This will cause error as organization user does not have this method.
So we can do this
user.user._invalidate_user_organizations_dict()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the variable name user
is misleading here, this is an OrganizationUser instance, so let's call it org_user
.
Then you can call: org_user.user._invalidate_user_organizations_dict()
.
openwisp_users/apps.py
Outdated
@@ -130,6 +137,18 @@ def connect_receivers(self): | |||
dispatch_uid='make_first_org_user_org_owner', | |||
) | |||
|
|||
@classmethod | |||
def handle_organization_update(cls, instance, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this for consistency: handle_org_is_active_change
openwisp_users/tasks.py
Outdated
organization_id=organization_pk | ||
).select_related('user') | ||
User = get_user_model() | ||
for user in qs.iterator(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for user in qs.iterator(): | |
for org_user in qs.iterator(): |
openwisp_users/tasks.py
Outdated
if not isinstance(user, User): | ||
user = user.user | ||
user._invalidate_user_organizations_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the variable name user
is misleading here, this is an OrganizationUser instance, so let's call it org_user
.
Then you can call: org_user.user._invalidate_user_organizations_dict()
.
@kaushikaryan04 come in the OpenWISP dev chat to coordinate when you can 🙏 |
Added a condition that reduced query count in 2 testcases and imporved naming of variables and funtions.
834602a
to
2020fea
Compare
Added is_active parameter in post request to avoid disabling org.
…ations-users-visible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kaushikaryan04 👍 👏
When the status ( is_active ) of an organization changes it now invalidates the cache of all the organizationsusers.
Fixes #357.