Reduce the number of SQL queries in updates of groups #255
Conversation
- only one query instead of one for each group when no MIRROR_GROUPS - the use of iterator() inside a list() creates a useless SQL cursor - avoid to create Django instances of groups by using values_list() - replace a frozenset by a tuple
Codecov Report
@@ Coverage Diff @@
## master #255 +/- ##
========================================
+ Coverage 85.4% 86.3% +0.8%
========================================
Files 11 8 -3
Lines 556 497 -59
========================================
- Hits 475 429 -46
+ Misses 81 68 -13
|
sondrelg
requested review from
JonasKs and
tim-schilling
and removed request for
JonasKs
|
Hi! Thank you so much for this. It all seems valid to me, and code looks good. I’d like @tim-schilling to say his opinion before I merge though 😊 |
django_auth_adfs/backend.py
Outdated
| new_claimed_group_names = (name for name in claim_groups if name not in existing_claimed_group_names) | ||
| if settings.MIRROR_GROUPS: | ||
| new_groups = [ | ||
| new_claimed_groups = [ | ||
| Group.objects.get_or_create(name=name)[0] | ||
| for name in claim_groups | ||
| if name not in existing_group_names | ||
| for name in new_claimed_group_names | ||
| ] | ||
| else: | ||
| for name in claim_groups: | ||
| if name not in existing_group_names: | ||
| try: | ||
| group = Group.objects.get(name=name) | ||
| new_groups.append(group) | ||
| except ObjectDoesNotExist: | ||
| pass | ||
| user.groups.set(existing_groups + new_groups) | ||
| new_claimed_groups = Group.objects.filter(name__in=new_claimed_group_names) |
There was a problem hiding this comment.
When settings.MIRROR_GROUPS is false, the only groups that should be set for the user are existing_claimed_groups. You can likely move the list comprehension of 335 into the if settings.MIRROR_GROUPS branch and define new_claimed_groups as an empty list in the else branch.
There was a problem hiding this comment.
Yes, indeed!
It does't make sense to filter again on new_claimed_group_names because the result won't change.
It avoids one more query, great!
I prefer to move user.groups.set() in the if branches instead of define new_claimed_groups as an empty list.
stephane
force-pushed
the
speedup-group-creation
branch
from
22fabce
to
e440441
Compare
|
Strange crash of GithHub Actions: Let's cool down the servers before testing again... ;) |
|
I don't find the button to re-run jobs BTW... |
|
Trying to re-run pipeline, but GitHub is giving me errors.. I’ll try later again. |
|
Pipelines seem to have completed now 😊 |
|
@tim-schilling , If you'd like to look over again? 😊 |
|
Thank you, @stephane! 😊 I'll get this released after work today. |
|
Thank you all for the review and the release. |
After having read the PR #230, my initial intention was to focus on SQL queries. I think it's more efficient to reduce the number of SQL queries to speed up the group creation (round-trip latency could be significant with DB).
So my initial patch leveraged
create_bulkof Django ORM but I found out about the signal issues in #220 and I changed my mind to provide something less intrusive.I'll be happy to provide a
create_bulkversion if you want.Changes:
MIRROR_GROUPSI had to disable Black in my editor to not mess the code ;)