Performance updates in self.update_user_groups method

[theognis1002]

Made some very slight performance updates in the self.update_user_groups method. values_list() retrieves a subset of data without the overhead of creating model instances

[sondrelg]

The values_list changes look good, but can you explain why it makes sense to replace set with add? I'm not that familiar with this code, so just want to know your reasoning before diving in further 🙂

[JonasKs]

Group tests fails, so I'm not sure if this works as intended?

[theognis1002]

@JonasKs ahh thx! didn't see that they failed at the time. I reverted it back to set.

[codecov]

Codecov Report

Merging #230 (e94bb5a) into master (5067f8f) will not change coverage.
The diff coverage is 100.0%.

@@          Coverage Diff           @@
##           master    #230   +/-   ##
======================================
  Coverage    86.0%   86.0%           
======================================
  Files          11      11           
  Lines         515     515           
======================================
  Hits          443     443           
  Misses         72      72           

Impacted Files	Coverage Δ
django_auth_adfs/backend.py	85.8% <100.0%> (ø)

[JonasKs]

This looks good to me.

@sondrelg, @tim-schilling , opinions? 😊

[sondrelg]

Could you please explain how

list(existing_groups.iterator()) + new_groups

is more performant than this?

existing_groups + new_groups

I'm sure it could be, but to me it just looks like extra steps 🙂

[theognis1002]

@sondrelg list(existing_groups.iterator()) was just taken from L213 and moved down. This was so that the queryset would not be evaluated before it was needed

[sondrelg]

Ah yeah I see 👍

That being said, I'm not sure the iterator() call is worth keeping. Doing a little testing, it seems like the list call is all that's needed.

from django.contrib.auth.models import Group

g = Group.objects.all()

print(list(g) == list(g.iterator()))

>> True

I could be wrong though 🙂

Another alternative to combining lists here might be to merge querysets (if new_groups's queryset is available), using the pipe (|) operator.

Otherwise, looks good to me!

[tim-schilling]

This looks good to me. It reminds me that I need to find time to do that benchmarking on #220

[JonasKs]

Hey @theognis1002 , could you address @sondrelg's comment so we can merge? 😊

[stephane]

This values_list call will generate a SQL query and the lazyexisting_groups is casted to list later (one query), the new code adds a new SQL query.

[tim-schilling]

I'm closing this since we merged #255. If you feel that this still improves the situation, please rebase on main and re-open the PR.