Perf: avoid unnecessary full-row fetches in batch permission assignment

set_permissions_for_objects now takes a model + pks instead of instances,
and identity filtering resolves straight to ids, so bulk-editing
permissions no longer materializes full Document/User/Group rows just to
read their pk/id. Row construction for bulk_create is also chunked to
bound peak memory for very large "apply to all" operations.
This commit is contained in:
stumpylog
2026-08-26 14:48:06 -07:00
parent 345220733a
commit fdb0828344
4 changed files with 45 additions and 19 deletions
+7 -4
View File
@@ -424,10 +424,13 @@ def set_permissions(
else:
qs.update(owner=owner)
docs = list(qs)
set_permissions_for_objects(permissions=set_permissions, objects=docs, merge=merge)
affected_docs = [doc.pk for doc in docs]
affected_docs = list(qs.values_list("pk", flat=True))
set_permissions_for_objects(
permissions=set_permissions,
model=Document,
pks=affected_docs,
merge=merge,
)
bulk_update_documents.apply_async(
kwargs={"document_ids": affected_docs},
+34 -13
View File
@@ -197,6 +197,13 @@ def _resolve_permissions(codenames: set[str], ctype: ContentType) -> list[Permis
return permission_objs
# Target number of permission rows to build in Python before handing them to
# bulk_create -- keeps peak memory bounded for a large "apply to all" call,
# independent of bulk_create's own batch_size (which only caps the size of
# each INSERT statement, not how many row objects exist in memory at once).
_PERMISSION_ROW_CHUNK_SIZE = 5000
def _apply_bulk_permission_entry(
*,
perm_model: type[UserObjectPermission] | type[GroupObjectPermission],
@@ -209,10 +216,14 @@ def _apply_bulk_permission_entry(
object_pks: list[str],
merge: bool,
) -> None:
identities_to_add = list(identity_model.objects.filter(id__in=ids))
# Only the ids are needed to build permission rows (via `<field>_id=`),
# so avoid fetching full User/Group rows for identities that may not
# even end up being granted anything new.
add_ids = set(
identity_model.objects.filter(id__in=ids).values_list("id", flat=True),
)
if not merge:
add_ids = {identity.id for identity in identities_to_add}
existing_ids = set(
perm_model.objects.filter(
content_type=ctype,
@@ -229,17 +240,23 @@ def _apply_bulk_permission_entry(
**{f"{identity_field}_id__in": remove_ids},
).delete()
if identities_to_add:
if not add_ids:
return
rows_per_pk = len(permission_objs) * len(add_ids)
pks_per_chunk = max(1, _PERMISSION_ROW_CHUNK_SIZE // rows_per_pk)
for start in range(0, len(object_pks), pks_per_chunk):
pk_chunk = object_pks[start : start + pks_per_chunk]
rows = [
perm_model(
content_type=ctype,
object_pk=pk,
permission=permission_obj,
**{identity_field: identity},
**{f"{identity_field}_id": identity_id},
)
for permission_obj in permission_objs
for pk in object_pks
for identity in identities_to_add
for pk in pk_chunk
for identity_id in add_ids
]
# ignore_conflicts skips only rows that already exist as an exact
# (identity, permission, object) match -- the same de-dup the
@@ -247,19 +264,25 @@ def _apply_bulk_permission_entry(
# already enforces for the single-object assign_perm() this
# replaces, so it doesn't change what counts as "already granted".
# batch_size caps how many rows go into a single INSERT so a huge
# "apply to all" call doesn't build one enormous statement.
# chunk doesn't build one enormous statement.
perm_model.objects.bulk_create(rows, ignore_conflicts=True, batch_size=1000)
def set_permissions_for_objects(
permissions: dict,
objects: QuerySet | list,
model: type[Model],
pks: QuerySet | list,
*,
merge: bool = False,
) -> None:
"""
Bulk equivalent of set_permissions_for_object: applies the same
permission changes to every object in `objects` at once.
permission changes to every object identified by `pks` at once.
Takes a model + pks (rather than model instances) deliberately -- the
permission rows built below only ever need `pk`, `content_type`, and
identity ids, so callers shouldn't have to fetch full rows (with every
other field) just to hand them to this function.
Deliberately does not use guardian's queryset/list-aware assign_perm:
passing a list as the object routes to bulk_assign_perm, which skips
@@ -276,14 +299,12 @@ def set_permissions_for_objects(
and every identity into one query per action, rather than one query per
(object, user) pair.
"""
objects = list(objects)
if not objects:
object_pks = [str(pk) for pk in pks]
if not object_pks:
return
model = objects[0].__class__
model_name = model.__name__.lower()
ctype = ContentType.objects.get_for_model(model)
object_pks = [str(obj.pk) for obj in objects]
for action, entry in permissions.items():
codename = f"{action}_{model_name}"
+2 -1
View File
@@ -636,7 +636,8 @@ class TestBulkEdit(DirectoriesMixin, TestCase):
with self.assertRaises(Permission.DoesNotExist):
set_permissions_for_objects(
{"not_a_real_action": {"users": [self.user1.id], "groups": []}},
[self.doc1],
Document,
[self.doc1.pk],
)
@mock.patch("documents.models.Document.delete")
+2 -1
View File
@@ -4912,7 +4912,8 @@ class BulkEditObjectsView(PassUserMixin):
if "permissions" in serializer.validated_data:
set_permissions_for_objects(
permissions=permissions,
objects=qs,
model=object_class,
pks=qs.values_list("pk", flat=True),
merge=merge,
)