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

Reduce queries in bulk permission assignment api #5087

Open
wants to merge 11 commits into
base: beta
Choose a base branch
from

Conversation

RuthShryock
Copy link
Member

@RuthShryock RuthShryock commented Aug 29, 2024

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Description

Refactored assign_perm() and remove_perm() to support bulk assignment and removal of permissions instead of processing them individually

Notes

Some background

This was the original task: #3869 added more queries to the endpoint. We should review and either reduce the number of queries or prove why this is necessary. Of course we should also inspect average load times - the goal is to make it faster, not just reduce queries.
After some clarification, the issue is that we call assign_perm() and remove_perm() for each permission assignment, one by one. This is costly and we want to reduce the number of queries and speed-up the endpoint so we should rewrite assign_perm() and remove_perm() to support bulk assignment.

Testing

Test cases have been added but if you would like to test manually:

  1. Pick a test in asset_permission_assignment.py such as test_partial_permission_grants_implied_view_asset
  2. Add self.assertNumQueries
  3. Compare before and after the PR

@RuthShryock RuthShryock marked this pull request as draft August 29, 2024 12:48
@RuthShryock RuthShryock changed the title Reduce queries bulk permission assignment api Reduce queries in bulk permission assignment api Aug 29, 2024
Copy link

@RuthShryock RuthShryock marked this pull request as ready for review August 29, 2024 15:29
Copy link

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

Mostly style comments and a thought about testing

kpi/mixins/object_permission.py Outdated Show resolved Hide resolved
kpi/mixins/object_permission.py Outdated Show resolved Hide resolved
kpi/mixins/object_permission.py Outdated Show resolved Hide resolved
kpi/mixins/object_permission.py Show resolved Hide resolved
kpi/tests/api/v2/test_api_asset_permission_assignment.py Outdated Show resolved Hide resolved
kpi/tests/api/v2/test_api_asset_permission_assignment.py Outdated Show resolved Hide resolved
@noliveleger noliveleger self-assigned this Sep 5, 2024
@@ -442,118 +442,158 @@ def get_all_implied_perms(cls, for_instance=None):

@transaction.atomic
@kc_transaction_atomic
def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False,
def assign_perms(self, user_obj, perm, deny=False, defer_recalc=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

if we are renaming assign_perm to assign_perms, we may need to rename perm param to perms.
Let's use string annotations too. i.e.:

assign_perms(self, user_obj: 'User', perms: Union[str, list[str]], deny: bool = False, etc...)

kpi/mixins/object_permission.py Outdated Show resolved Hide resolved
remove_applicable_kc_permissions(
self, user_obj, contradictory_codenames
)
# Create the new permission and add to the list of permissions
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not create new permission yet. It is just added to the list.

)

if new_permissions:
ObjectPermission.objects.bulk_create(new_permissions)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use ignore_conflicts=True in case race conditions and two concurrent requests try to add same permissions at the same time.

Comment on lines 567 to 568
if len(new_permissions) == 1:
new_permissions = new_permissions[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this condition needed?

Copy link
Member Author

@RuthShryock RuthShryock Sep 20, 2024

Choose a reason for hiding this comment

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

This was needed because a lot of code calling assign_perm() was expecting a single string in return. So I wanted to make sure that if we just assigned one permission, it would return one string as expected. But now, since we are keeping the original assign_perm() function and that will still be used when assigning a single permission, it is no longer needed.

Comment on lines 583 to 586
for p in perm:
self._update_partial_permissions(
user_obj, p, partial_perms=partial_perms
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This may call clean_up_table() (within _update_partial_permissions ) several times in a row.
It would be great to avoid that if the list of permissions contains conflicting permissions.

post_assign_perm.send(
sender=self.__class__,
instance=self,
user=user_obj,
codename=codename,
codename=', '.join(perm),
Copy link
Contributor

Choose a reason for hiding this comment

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

post_assign_asset_perm expects the (the receiver method in kpi/signals.py) codename to be a string.
You need to adapt the code to be sure it works. I think a test is missing there. We need to be sure that the signal is called and the enketo server URL is updated when anonymous is granted add_submissions.

Copy link
Contributor

@noliveleger noliveleger left a comment

Choose a reason for hiding this comment

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

Do we have a test that checks expected permission assignments are there when we use assign_perms() with many multiple permissions at a time.

I would like to be sure that assigning permissions with collections for example we still work the way we expect.

By the way, I would keepassign_perm() (singular) and it would call your new assign_perms behind the scene. We would avoid changing assign_perm everywhere.
We can use typing annotation to know which type each method is expecting.

post_assign_perm.send(
sender=self.__class__,
instance=self,
user=user_obj,
codename=codename,
codename=', '.join(perm),
)

# Recalculate all descendants
self.recalculate_descendants_perms()
Copy link
Contributor

Choose a reason for hiding this comment

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

If there a way to improve this method too?

@RuthShryock RuthShryock force-pushed the reduce-queries-bulk-permission-assignment-api branch from 65bca19 to adeda44 Compare September 13, 2024 19:47
@RuthShryock RuthShryock force-pushed the reduce-queries-bulk-permission-assignment-api branch from d8cfa8f to 872ee18 Compare September 20, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants