From bba6eb333ee9861ff0c918f8355bf45a2aa533ce Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Wed, 27 Sep 2023 09:35:52 -0300 Subject: [PATCH] Add db indexes to user table (#3067) Add composite indexes based on existing queries/usage, ensuring partial index prefixes are useful too. - `is_active` filtering is set in the default `User` manager - most of our user queries are per `organization` - multiple cases filter by `username` or `email` (most notably schedule related queries, given the low-level backend ical representation) Also rework how users are fetched from DB when getting users from schedules ical representation (which was particularly slow when regex filtering by required permission). Related to https://github.com/grafana/oncall-private/issues/2163 --- CHANGELOG.md | 4 +++ engine/apps/schedules/ical_utils.py | 25 ++++++++++--------- .../migrations/0015_auto_20230926_2203.py | 21 ++++++++++++++++ engine/apps/user_management/models/user.py | 4 +++ 4 files changed, 42 insertions(+), 12 deletions(-) create mode 100644 engine/apps/user_management/migrations/0015_auto_20230926_2203.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 55151194..c59e1908 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix regression in public actions endpoint handling user field by @mderynck ([#3053](https://github.com/grafana/oncall/pull/3053)) +### Changed + +- Rework how users are fetched from DB when getting users from schedules ical representation ([#3067](https://github.com/grafana/oncall/pull/3067)) + ## v1.3.38 (2023-09-19) ### Fixed diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index cdd02ffc..285ad176 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -67,11 +67,10 @@ IcalEvents = typing.List[IcalEvent] def users_in_ical( usernames_from_ical: typing.List[str], organization: "Organization", -) -> "UserQuerySet": +) -> typing.List["User"]: """ This method returns a sequence of `User` objects, filtered by users whose username, or case-insensitive e-mail, - is present in `usernames_from_ical`. If `include_viewers` is set to `True`, users are further filtered down - based on their granted permissions. + is present in `usernames_from_ical`. Parameters ---------- @@ -80,24 +79,26 @@ def users_in_ical( organization : apps.user_management.models.organization.Organization The organization in question """ - from apps.user_management.models import User + required_permission = RBACPermission.Permissions.SCHEDULES_WRITE emails_from_ical = [username.lower() for username in usernames_from_ical] - # users_found_in_ical = organization.users users_found_in_ical = organization.users.filter( - **User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization) - ) - - users_found_in_ical = users_found_in_ical.filter( (Q(username__in=usernames_from_ical) | Q(email__lower__in=emails_from_ical)) ).distinct() - return users_found_in_ical + if organization.is_rbac_permissions_enabled: + # it is more efficient to check permissions on the subset of users filtered above + # than performing a regex query for the required permission + users_found_in_ical = [u for u in users_found_in_ical if {"action": required_permission.value} in u.permissions] + else: + users_found_in_ical = users_found_in_ical.filter(role__lte=required_permission.fallback_role.value) + + return list(users_found_in_ical) @timed_lru_cache(timeout=100) -def memoized_users_in_ical(usernames_from_ical: typing.List[str], organization: "Organization") -> UserQuerySet: +def memoized_users_in_ical(usernames_from_ical: typing.List[str], organization: "Organization") -> typing.List["User"]: # using in-memory cache instead of redis to avoid pickling python objects return users_in_ical(usernames_from_ical, organization) @@ -354,7 +355,7 @@ def list_users_to_notify_from_ical_for_period( schedule: "OnCallSchedule", start_datetime: datetime.datetime, end_datetime: datetime.datetime, -) -> UserQuerySet: +) -> typing.List["User"]: users_found_in_ical: typing.Sequence["User"] = [] events = schedule.final_events(start_datetime, end_datetime) usernames = [] diff --git a/engine/apps/user_management/migrations/0015_auto_20230926_2203.py b/engine/apps/user_management/migrations/0015_auto_20230926_2203.py new file mode 100644 index 00000000..15ce31e5 --- /dev/null +++ b/engine/apps/user_management/migrations/0015_auto_20230926_2203.py @@ -0,0 +1,21 @@ +# Generated by Django 3.2.20 on 2023-09-26 22:03 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('user_management', '0014_auto_20230728_0802'), + ] + + operations = [ + migrations.AddIndex( + model_name='user', + index=models.Index(fields=['is_active', 'organization', 'username'], name='user_manage_is_acti_385fc4_idx'), + ), + migrations.AddIndex( + model_name='user', + index=models.Index(fields=['is_active', 'organization', 'email'], name='user_manage_is_acti_7e930d_idx'), + ), + ] diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index 2043f0f9..8c05bb62 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -169,6 +169,10 @@ class User(models.Model): # Including is_active to unique_together and setting is_active to None allows to # have multiple deleted users with the same user_id, but user_id is unique among active users unique_together = ("user_id", "organization", "is_active") + indexes = [ + models.Index(fields=["is_active", "organization", "username"]), + models.Index(fields=["is_active", "organization", "email"]), + ] public_primary_key = models.CharField( max_length=20,