core: actually omit users from the groups API response when include_users=false#21690
Open
SAY-5 wants to merge 1 commit intogoauthentik:mainfrom
Open
core: actually omit users from the groups API response when include_users=false#21690SAY-5 wants to merge 1 commit intogoauthentik:mainfrom
SAY-5 wants to merge 1 commit intogoauthentik:mainfrom
Conversation
…sers=false GroupSerializer's `include_users=false` query flag only controlled the `users_obj` SerializerMethodField, which correctly returned None. The companion `users` field (implicit PrimaryKeyRelatedField(many=True) from the Meta.fields list) was untouched, so every list response still walked the group.users M2M to collect user PKs. On tenants with large groups this makes /api/v3/core/groups/?include_users=false indistinguishable from include_users=true in response time, which is the opposite of what the caller asked for and what the OpenAPI doc implies. The viewset already skips the `users` prefetch when _should_include_users is false, but that only means the M2M walk is now a cold DB round-trip per group rather than a cached one, so the N+1 pattern is actually worse than with include_users=true in that mode. Override GroupSerializer.get_fields to drop `users` when _should_include_users is false. GET /core/groups/?include_users=false now returns neither `users` nor `users_obj`. Write paths (POST / PATCH) do not set the query param so _should_include_users stays at its True default and `users` remains writable exactly as before. Fixes goauthentik#21278 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21690 +/- ##
==========================================
- Coverage 92.21% 92.17% -0.05%
==========================================
Files 1033 1033
Lines 59947 59952 +5
Branches 2695 2695
==========================================
- Hits 55283 55259 -24
- Misses 4593 4620 +27
- Partials 71 73 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
GroupSerializerinauthentik/core/api/groups.pygates theusers_objSerializerMethodField on_should_include_userscorrectly (returnsNonewhen?include_users=false). The siblingusersfield is not a SerializerMethodField — it's the implicitPrimaryKeyRelatedField(many=True)that DRF derives from the Meta.fields list — and nothing turned it off. So every list response still serialises the group-users M2M as an array of PKs.Per #21278, on tenants with large groups this makes:
indistinguishable from
include_users=truein wall-clock time, which is the opposite of what the caller asked for and what the OpenAPI parameter doc implies. The viewset already drops theusersprefetch when_should_include_usersis false (see theprefetch_related("users", ...)guard), so each group now takes a cold round-trip to walk the M2M. That makes the N+1 shape in this mode arguably worse than withinclude_users=true.Fix
Override
GroupSerializer.get_fieldsto drop theusersbinding when the include flag is off:After this:
GET /core/groups/?include_users=falsereturns neitherusersnorusers_obj. The M2M is never walked.GET /core/groups/(default) still returns both./core/groups/, PATCH/core/groups/{uuid}/) do not setinclude_usersin their query string, so_should_include_usersstays at itsTruedefault andusersremains a writable field. Create/update semantics for assigning users are unaffected.Test plan
GET /core/groups/?include_users=false&page_size=20on a tenant with large groups. Response body still containsusers: [pk, pk, ...]per group; p95 latency unchanged vsinclude_users=true.usersorusers_objkeys. p95 drops back to the "no user walks" curve.GET /core/groups/(no query param) unchanged: both fields present.POST /core/groups/ {"name": "...", "users": [1, 2]}still assigns users correctly (test_groups_api covers this).Fixes #21278