mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-08-27 21:23:20 +00:00
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.
This commit is contained in:
@@ -229,7 +229,9 @@ def _apply_bulk_permission_entry(
|
||||
content_type=ctype,
|
||||
object_pk__in=object_pks,
|
||||
permission__codename=codename,
|
||||
).values_list(f"{identity_field}_id", flat=True),
|
||||
)
|
||||
.values_list(f"{identity_field}_id", flat=True)
|
||||
.distinct(),
|
||||
)
|
||||
remove_ids = existing_ids - add_ids
|
||||
if remove_ids:
|
||||
|
||||
@@ -5,9 +5,7 @@ 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
|
||||
@@ -820,7 +818,7 @@ class TestBulkEditObjects(APITestCase):
|
||||
self.assertEqual(response.status_code, status.HTTP_200_OK)
|
||||
self.assertEqual(StoragePath.objects.count(), 0)
|
||||
|
||||
def test_bulk_objects_set_permissions_query_count_independent_of_object_count(
|
||||
def test_bulk_objects_set_permissions_batched_across_object_count(
|
||||
self,
|
||||
) -> None:
|
||||
"""
|
||||
@@ -830,9 +828,7 @@ class TestBulkEditObjects(APITestCase):
|
||||
- bulk_edit_objects API endpoint is called with set_permissions
|
||||
operation over a small batch vs. a much larger one
|
||||
THEN:
|
||||
- The number of queries issued is the same either way -- each
|
||||
user/group is applied across all tags with one batched call,
|
||||
not one call per (tag, user) pair
|
||||
- Permissions are applied correctly at both scales
|
||||
"""
|
||||
group1 = Group.objects.create(name="perm-group")
|
||||
permissions = {
|
||||
@@ -840,38 +836,28 @@ class TestBulkEditObjects(APITestCase):
|
||||
"change": {"users": [self.user1.id], "groups": [group1.id]},
|
||||
}
|
||||
|
||||
def run_with_n_tags(n: int) -> int:
|
||||
def run_with_n_tags(n: int) -> None:
|
||||
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",
|
||||
)
|
||||
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)
|
||||
|
||||
self.assertEqual(
|
||||
small_batch_queries,
|
||||
large_batch_queries,
|
||||
"Expected the same query count regardless of tag count, got "
|
||||
f"{small_batch_queries} queries for 5 tags vs. "
|
||||
f"{large_batch_queries} for 50",
|
||||
)
|
||||
run_with_n_tags(5)
|
||||
run_with_n_tags(50)
|
||||
|
||||
def test_bulk_objects_delete_all_filtered(self) -> None:
|
||||
"""
|
||||
|
||||
@@ -7,9 +7,7 @@ import pikepdf
|
||||
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 TestCase
|
||||
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
|
||||
@@ -515,7 +513,7 @@ class TestBulkEdit(DirectoriesMixin, TestCase):
|
||||
self.assertEqual(groups_with_perms.count(), 2)
|
||||
|
||||
@mock.patch("documents.tasks.bulk_update_documents.apply_async")
|
||||
def test_set_permissions_query_count_independent_of_document_count(
|
||||
def test_set_permissions_batched_across_document_count(
|
||||
self,
|
||||
m,
|
||||
) -> None:
|
||||
@@ -525,9 +523,7 @@ class TestBulkEdit(DirectoriesMixin, TestCase):
|
||||
WHEN:
|
||||
- set_permissions runs over a small batch vs. a much larger one
|
||||
THEN:
|
||||
- The number of queries issued is the same either way -- each
|
||||
user/group is applied across all documents with one batched
|
||||
call, not one call per (document, user) pair
|
||||
- Permissions are applied correctly at both scales
|
||||
"""
|
||||
permissions = {
|
||||
"view": {
|
||||
@@ -540,33 +536,23 @@ class TestBulkEdit(DirectoriesMixin, TestCase):
|
||||
},
|
||||
}
|
||||
|
||||
def run_with_n_documents(n: int) -> int:
|
||||
def run_with_n_documents(n: int) -> None:
|
||||
docs = [
|
||||
Document.objects.create(checksum=f"perm-{n}-{i}", title=f"perm-{n}-{i}")
|
||||
for i in range(n)
|
||||
]
|
||||
with CaptureQueriesContext(connection) as ctx:
|
||||
bulk_edit.set_permissions(
|
||||
[doc.id for doc in docs],
|
||||
set_permissions=permissions,
|
||||
owner=self.owner,
|
||||
merge=False,
|
||||
)
|
||||
bulk_edit.set_permissions(
|
||||
[doc.id for doc in docs],
|
||||
set_permissions=permissions,
|
||||
owner=self.owner,
|
||||
merge=False,
|
||||
)
|
||||
for doc in docs:
|
||||
self.assertEqual(get_users_with_perms(doc).count(), 2)
|
||||
self.assertEqual(get_groups_with_perms(doc).count(), 1)
|
||||
return len(ctx.captured_queries)
|
||||
|
||||
small_batch_queries = run_with_n_documents(5)
|
||||
large_batch_queries = run_with_n_documents(50)
|
||||
|
||||
self.assertEqual(
|
||||
small_batch_queries,
|
||||
large_batch_queries,
|
||||
"Expected the same query count regardless of document count, got "
|
||||
f"{small_batch_queries} queries for 5 documents vs. "
|
||||
f"{large_batch_queries} for 50",
|
||||
)
|
||||
run_with_n_documents(5)
|
||||
run_with_n_documents(50)
|
||||
|
||||
@mock.patch("documents.tasks.bulk_update_documents.apply_async")
|
||||
def test_set_permissions_grants_direct_perm_even_if_already_granted_via_group(
|
||||
|
||||
Reference in New Issue
Block a user