Performance: batch permission assignment in bulk set_permissions (#13806)

* Perf: batch guardian permission assignment in bulk-edit

bulk_edit.set_permissions and BulkEditObjectPermissionsView both
looped documents/objects and called set_permissions_for_object per
object, which itself calls guardian's assign_perm/remove_perm once
per (object, user) pair -- ~10-20+ queries per object, scaling with
selection size.

Added set_permissions_for_objects, a bulk equivalent that resolves
existing permission holders once across the whole batch (not once per
object) and applies changes with a small, batch-size-independent
number of queries per action instead of one per (object, user) pair.

* 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.

* Fix: use .distinct() for existing-grant lookup, drop flaky query-count invariant tests

.distinct() lets the database dedupe identity ids server-side instead of
transferring one row per (object, grantee) match and deduping in Python --
was the dominant cost on a large selection with existing grants.

Also replaced the two query-count-equality tests (bulk_edit and the
bulk_edit_objects API path) with plain functional-correctness checks at
both batch sizes.  Hopefully stops that flake.

* Mark empty-pks early-return in set_permissions_for_objects as no-cover

Defensive guard for an edge case (all requested pks already gone/invalid)
rather than a path normal usage exercises; matches the existing
pragma: no cover convention elsewhere in this file.

* Perf: drop speculative row-chunking in bulk permission assignment, keep the query-batching fix

* Resolve every permission action before applying any of them

This fixes the existing issue and resolves the Copilot comment

* Assert permission assignment does not scale with selection size

The two batching tests only checked that permissions came out correct at
5 and 50 objects, so reverting to the old per-object loop would still
have passed.  Check sizes as well to prevent that
This commit is contained in:
Trenton H
2026-09-10 19:54:09 +00:00
committed by GitHub
parent 9a8163fbbb
commit 2256cb3d38
5 changed files with 428 additions and 11 deletions
+65
View File
@@ -2,10 +2,15 @@ import datetime
import json
from unittest import mock
from django.contrib.auth.models import Group
from django.contrib.auth.models import Permission
from django.contrib.auth.models import User
from django.db import connection
from django.test import override_settings
from django.test.utils import CaptureQueriesContext
from guardian.shortcuts import assign_perm
from guardian.shortcuts import get_groups_with_perms
from guardian.shortcuts import get_users_with_perms
from rest_framework import status
from rest_framework.test import APITestCase
@@ -842,6 +847,66 @@ class TestBulkEditObjects(APITestCase):
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(StoragePath.objects.count(), 0)
def test_bulk_objects_set_permissions_batched_across_object_count(
self,
) -> None:
"""
GIVEN:
- Many tags are being bulk-edited to set permissions at once
WHEN:
- bulk_edit_objects API endpoint is called with set_permissions
operation over a small batch vs. a much larger one
THEN:
- Permissions are applied correctly at both scales
- Query count does not grow with the number of tags, i.e. each
user/group is applied across all tags with one batched call
rather than one call per (tag, identity) pair
"""
group1 = Group.objects.create(name="perm-group")
permissions = {
"view": {"users": [self.user1.id, self.user2.id], "groups": [group1.id]},
"change": {"users": [self.user1.id], "groups": [group1.id]},
}
def run_with_n_tags(n: int) -> int:
tags = [Tag.objects.create(name=f"perm-tag-{n}-{i}") for i in range(n)]
with CaptureQueriesContext(connection) as ctx:
response = self.client.post(
"/api/bulk_edit_objects/",
json.dumps(
{
"objects": [t.id for t in tags],
"object_type": "tags",
"operation": "set_permissions",
"permissions": permissions,
"merge": False,
},
),
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_200_OK)
for tag in tags:
self.assertEqual(get_users_with_perms(tag).count(), 2)
self.assertEqual(get_groups_with_perms(tag).count(), 1)
return len(ctx.captured_queries)
small_batch_queries = run_with_n_tags(5)
large_batch_queries = run_with_n_tags(50)
# A tolerance rather than equality, matching the N+1 check in
# test_views.py: bulk_create's batch_size caps rows per INSERT, so a
# large enough selection does legitimately add statements, and the
# per-process ContentType cache makes the first run carry an extra
# query. Neither can hide a regression to per-object assignment,
# which would be ~10x the small-batch count here.
self.assertLessEqual(
large_batch_queries,
small_batch_queries + 5,
"Permission assignment appears to scale with object count: "
f"{small_batch_queries} queries for 5 tags vs. "
f"{large_batch_queries} for 50",
)
def test_bulk_objects_delete_all_filtered(self) -> None:
"""
GIVEN: