Compare commits

..
Author SHA1 Message Date
stumpylog 74abdf8ff8 Docs: remove stale drf-writable-nested references, DRY up audit-actor update()
Final-review cleanup on refactor/remove-drf-writable-nested: several
comments/docstrings still described drf-writable-nested's removed
NestedUpdateMixin behavior in the present tense. Rewrote them to explain
the shared context-cache and delete-query-count guarantees in terms of
the current code (bulk-edit's per-field CustomFieldInstanceSerializer
construction, and _sync_custom_fields' single hard-delete query).

Also collapsed the duplicated `if custom_fields_data is not None:
self._sync_custom_fields(...)` line in DocumentSerializer.update()'s
audit-log branches into a single code path using
contextlib.nullcontext(), removing the drift risk that caused the
earlier audit-actor bug.

No behavior change.
2026-08-25 11:53:42 -07:00
stumpylogandClaude Sonnet 5 ce5d5c33b0 chore: remove drf-writable-nested dependency
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-08-25 11:07:11 -07:00
stumpylog d9d44520bd test: add query-count regression coverage for custom_fields sync on update 2026-08-25 10:59:47 -07:00
stumpylog 500ff563cf refactor: replace drf-writable-nested's NestedUpdateMixin with explicit custom_fields sync 2026-08-25 10:38:51 -07:00
stumpylog 7f314b5149 test: characterize custom field instance delete-on-omit as a hard delete 2026-08-25 10:28:24 -07:00
stumpylog 132918821d Merge remote-tracking branch 'origin/perf/batch-custom-field-lookup' into tmp/perf-integration 2026-08-25 10:15:52 -07:00
stumpylog 512a3fe196 Merge remote-tracking branch 'origin/perf/batch-modify-custom-fields' into tmp/perf-integration 2026-08-25 10:15:48 -07:00
stumpylog b01e0368b7 Merge remote-tracking branch 'origin/perf/batch-tags-field-lookup' into tmp/perf-integration 2026-08-25 10:15:44 -07:00
stumpylog 1c82bd15c5 Merge branch 'perf/batch-set-permissions' into tmp/perf-integration 2026-08-25 10:15:39 -07:00
stumpylog cbac71c165 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.
2026-08-25 09:27:24 -07:00
stumpylog 8780bcd5c7 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.

Deliberately does not use guardian's queryset-aware assign_perm:
passing a list as the target routes to bulk_assign_perm, which skips
creating a direct permission row for anyone who already has the
permission via ANY group membership (checked via
ObjectPermissionChecker.has_perm, which is group-inheritance-aware) --
unlike the single-object assign_perm this replaces, which always
ensures a direct row via get_or_create. Losing that guarantee would
mean a later revocation of the group's grant silently strips access
an admin explicitly asked to be direct. Bulk-creates rows straight
against UserObjectPermission/GroupObjectPermission instead
(ignore_conflicts=True, relying on the existing (identity, permission,
object_pk) unique constraint), which preserves the original semantics
exactly while still batching every object and identity into one query
per action. Also raises Permission.DoesNotExist for an unrecognized
action name instead of silently no-op-ing, matching the original
per-object path -- BulkEditObjectsSerializer never actually validates
action keys against the raw client-supplied permissions dict, so this
is reachable from client input, not just internal callers.

Verified via CaptureQueriesContext: query count is now identical at 5
vs. 50 documents/objects (was 1,123 queries for 20 documents on the
Document path, 2,806 for 50 tags on the BulkEditObjectPermissionsView
path, both now flat). Full documents test suite green (2,148 passed,
1 skipped).
2026-08-24 20:04:58 -07:00
stumpylog 9d2416c435 Perf: batch id resolution for TagsField and friends
TagsField/CorrespondentField/DocumentTypeField/StoragePathField were
plain PrimaryKeyRelatedField subclasses with no batching. When used
with many=True (only tags today: DocumentSerializer.tags,
WorkflowActionSerializer.assign_tags), DRF's ManyRelatedField resolves
each submitted id with its own query -- one query per tag on every
PATCH/PUT that sets tags.

Added BatchResolvingPrimaryKeyRelatedField as the shared base for all
four field classes and overrode many_init so the many=True form
(_BatchingManyRelatedField) resolves the whole id list with one
pk__in query, falling back to the child relation's normal per-item
validation for anything not found in that batch. Only TagsField uses
many=True today, but the fix isn't tag-specific -- if a future PR puts
many=True on one of the others, it inherits the same batching instead
of reintroducing this as a new bug.

Independent review caught a real regression: Django's IntegerFieldOverflow
guard (out-of-range int -> EmptyResultSet) only covers exact/gt/gte/lt/lte
lookups, not `in`, so an absurdly large tag id reached the batched
pk__in= query as-is and raised an unhandled OverflowError (SQLite) /
DataError (Postgres) instead of the normal 400 the original per-item
`exact` lookup produced. Guarded the batch query and fall through to
per-item resolution (which goes through the protected `exact` lookup)
on failure.

Verified via CaptureQueriesContext against a real API PATCH: 20 tags
dropped from 54 to 35 queries per request (exactly the 19 saved by
collapsing 20 individual lookups into one batched query). Full
documents/workflows/bulk-edit/retagger/custom-fields suites green
(443 passed).
2026-08-24 15:15:00 -07:00
stumpylog a98d0669e4 Perf: batch CustomField/Document lookups in modify_custom_fields
modify_custom_fields looped documents x fields, re-.get()-ing the
CustomField queryset per iteration and Document.objects.get() per doc
for DOCUMENTLINK fields -- same shape as the earlier custom_fields
serializer N+1 (#13779), just nested one level deeper. Resolve both
into dicts once up front instead. Also pass the resolved objects
(not bare ids) to update_or_create so newly-created CustomFieldInstance
rows cache their field/document FK, avoiding a re-fetch when auditlog's
post_save receiver calls str(instance) (which touches .field.name).

docs_by_id defers `content` (the one field guaranteed both large and
unused by this function or its receivers) rather than using .only(),
since .only() would just turn the filename-generation signal's other
field access into a deferred-reload N+1.

Verified via CaptureQueriesContext: 6 docs x 4 fields dropped from 48
CustomField queries to 1; DOCUMENTLINK per-doc Document lookups dropped
from N to 0 (single batched query instead).
2026-08-24 14:21:59 -07:00
stumpylog bda506968b Handles a bad client sending malformed JSON or non-int primary keys 2026-08-24 12:35:06 -07:00
Trenton Holmes 4ccb34a70b Perf: avoid per-instance CustomField reload in DocumentMetadataOverrides
send_websocket_document_updated calls document.refresh_from_db()
before building overrides, which drops the custom_fields prefetch
(and its select_related("field")) set up by the view's queryset.
DocumentMetadataOverrides.from_document() then lazily reloads field
once per custom field instance. Since from_document() can't rely on
the caller having a prefetched document, select_related explicitly at
the point of use instead.
2026-08-23 17:33:05 -07:00
Trenton Holmes 0a466c9fcf Perf: reuse resolved CustomField objects across drf-writable-nested's per-item revalidation
drf-writable-nested's update_or_create_reverse_relations rebuilds a
fresh serializer -- and fresh field instances -- per custom_fields item
while matching existing vs. new instances during save(), so the
per-instance lookup cache alone only helped the first validation pass.
It passes the same context dict (by reference) to every one of those
serializers, so stash resolved CustomField objects there instead:
later passes reuse them for free rather than re-querying.
2026-08-23 17:12:25 -07:00
Trenton Holmes c9cc4f427d Perf: batch CustomField lookups when validating a document's custom_fields
DocumentSerializer.custom_fields validates each item's field id via a
plain PrimaryKeyRelatedField, which issues one SELECT per custom field
per validation pass (discussion #13690). Batch-resolve all field ids in
one query and cache them on the field instance so per-item validation
is free instead of re-querying.
2026-08-23 14:53:46 -07:00
17 changed files with 1169 additions and 185 deletions
-1
View File
@@ -40,7 +40,6 @@ dependencies = [
"djangorestframework~=3.16",
"drf-spectacular~=0.30",
"drf-spectacular-sidecar~=2026.7.1",
"drf-writable-nested~=0.7.1",
"filelock~=3.32.0",
"flower~=2.0.1",
"gotenberg-client~=0.14.0",
@@ -417,7 +417,7 @@ main {
:host ::ng-deep .navbar-official-logo {
.leaf {
fill: color-mix(in srgb, var(--pngx-primary-text-contrast) 85%, var(--bs-primary)) !important;
fill: color-mix(in srgb, var(--pngx-primary-text-contrast) 70%, var(--bs-primary)) !important;
}
.text {
+46 -29
View File
@@ -27,7 +27,7 @@ from documents.models import DocumentType
from documents.models import PaperlessTask
from documents.models import StoragePath
from documents.models import Tag
from documents.permissions import set_permissions_for_object
from documents.permissions import set_permissions_for_objects
from documents.plugins.helpers import DocumentsStatusManager
from documents.tasks import bulk_update_documents
from documents.tasks import consume_file
@@ -305,33 +305,49 @@ def modify_custom_fields(
else [(field, None) for field in add_custom_fields]
)
custom_fields = CustomField.objects.filter(
id__in=[int(field) for field, _ in add_custom_fields],
).distinct()
custom_fields_by_id: dict[int, CustomField] = {
cf.id: cf
for cf in CustomField.objects.filter(
id__in=[int(field) for field, _ in add_custom_fields],
)
}
# Deferred, not `.only()`: these objects get cached onto the FK
# descriptor of newly-created CustomFieldInstance rows below, and
# downstream post_save receivers (e.g. the filename-generation signal)
# touch other Document fields -- `.only("pk")` would just turn that into
# a deferred-field reload per document, trading one N+1 for another.
# `content` is the one field guaranteed to be both large (full OCR text)
# and unused by anything this function or its receivers touch.
docs_by_id: dict[int, Document] = {
doc.id: doc
for doc in Document.objects.filter(id__in=affected_docs).defer("content")
}
for field_id, value in add_custom_fields:
custom_field = custom_fields_by_id[field_id]
value_field = CustomFieldInstance.TYPE_TO_DATA_STORE_NAME_MAP[
custom_field.data_type
]
for doc_id in affected_docs:
defaults = {}
custom_field = custom_fields.get(id=field_id)
if custom_field:
value_field = CustomFieldInstance.TYPE_TO_DATA_STORE_NAME_MAP[
custom_field.data_type
]
defaults[value_field] = value
if (
custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK
and value
and doc_id in value
):
# Prevent self-linking
continue
defaults = {value_field: value}
if (
custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK
and value
and doc_id in value
):
# Prevent self-linking
continue
# Pass the already-resolved objects, not bare ids: this caches
# them on the FK descriptor of any newly-created instance, so a
# later `.field`/`.document` access (e.g. auditlog's post_save
# receiver calling `str(instance)`, which touches `.field.name`)
# doesn't trigger its own per-instance re-fetch.
CustomFieldInstance.objects.update_or_create(
document_id=doc_id,
field_id=field_id,
document=docs_by_id[doc_id],
field=custom_field,
defaults=defaults,
)
if custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK:
doc = Document.objects.get(id=doc_id)
reflect_doclinks(doc, custom_field, value)
reflect_doclinks(docs_by_id[doc_id], custom_field, value)
# For doc link fields that are being removed, remove symmetrical links
for doclink_being_removed_instance in CustomFieldInstance.objects.filter(
@@ -339,12 +355,10 @@ def modify_custom_fields(
field__id__in=remove_custom_fields,
field__data_type=CustomField.FieldDataType.DOCUMENTLINK,
value_document_ids__isnull=False,
):
).select_related("field"):
for target_doc_id in doclink_being_removed_instance.value:
remove_doclink(
document=Document.objects.get(
id=doclink_being_removed_instance.document.id,
),
document=docs_by_id[doclink_being_removed_instance.document_id],
field=doclink_being_removed_instance.field,
target_doc_id=target_doc_id,
)
@@ -430,10 +444,13 @@ def set_permissions(
else:
qs.update(owner=owner)
for doc in qs:
set_permissions_for_object(permissions=set_permissions, object=doc, merge=merge)
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},
+1 -1
View File
@@ -129,7 +129,7 @@ class DocumentMetadataOverrides:
)
overrides.custom_fields = {
custom_field.field.id: custom_field.value
for custom_field in doc.custom_fields.all()
for custom_field in doc.custom_fields.select_related("field").all()
}
groups_with_perms = get_groups_with_perms(
+176
View File
@@ -173,6 +173,182 @@ def set_permissions_for_object(
)
def _resolve_permissions(codenames: set[str], ctype: ContentType) -> list[Permission]:
"""
Resolves `codenames` to Permission rows, raising like the single-object
assign_perm() this bulk path replaces does (via a `.get()` internally)
if any codename doesn't exist -- e.g. a client-supplied action name that
was never validated (BulkEditObjectsSerializer._validate_permissions
calls validate_set_permissions() only for its side-effecting id checks
and discards the filtered dict it returns, so an unrecognized action key
reaches this function as-is). A plain `.filter()` with no existence
check would otherwise silently build zero rows and no-op instead of
reporting the bad input.
"""
permission_objs = list(
Permission.objects.filter(content_type=ctype, codename__in=codenames),
)
missing = codenames - {p.codename for p in permission_objs}
if missing:
raise Permission.DoesNotExist(
f"Permission matching query does not exist for codename(s): "
f"{', '.join(sorted(missing))}",
)
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],
identity_model: type[User] | type[Group],
identity_field: str,
ids: list[int],
codename: str,
permission_objs: list[Permission],
ctype: ContentType,
object_pks: list[str],
merge: bool,
) -> None:
# 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:
existing_ids = set(
perm_model.objects.filter(
content_type=ctype,
object_pk__in=object_pks,
permission__codename=codename,
).values_list(f"{identity_field}_id", flat=True),
)
remove_ids = existing_ids - add_ids
if remove_ids:
perm_model.objects.filter(
content_type=ctype,
object_pk__in=object_pks,
permission__codename=codename,
**{f"{identity_field}_id__in": remove_ids},
).delete()
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,
**{f"{identity_field}_id": identity_id},
)
for permission_obj in permission_objs
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
# underlying (user|group, permission, object_pk) unique constraint
# 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
# 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,
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 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
creating a direct permission row for anyone who already has the
permission via ANY group membership (it checks
ObjectPermissionChecker.has_perm, which is group-inheritance-aware) --
unlike the single-object assign_perm this replaces, which always
ensures a direct row via get_or_create regardless of group-derived
access. Losing that guarantee would mean a later revocation of the
group's grant silently strips access an admin explicitly asked to be
direct. Bulk-creating rows straight against the permission models
instead (see _apply_bulk_permission_entry) preserves the original
always-create-a-direct-row semantics while still batching every object
and every identity into one query per action, rather than one query per
(object, user) pair.
"""
object_pks = [str(pk) for pk in pks]
if not object_pks:
return
model_name = model.__name__.lower()
ctype = ContentType.objects.get_for_model(model)
for action, entry in permissions.items():
codename = f"{action}_{model_name}"
implied_codenames = {codename}
if action == "change":
# change gives view too
implied_codenames.add(f"view_{model_name}")
# Resolved once per action (not once per users/groups branch) and
# shared between both below -- also where an unrecognized action
# name (see _resolve_permissions) is caught.
permission_objs = (
_resolve_permissions(implied_codenames, ctype)
if "users" in entry or "groups" in entry
else []
)
if "users" in entry:
_apply_bulk_permission_entry(
perm_model=UserObjectPermission,
identity_model=User,
identity_field="user",
ids=entry["users"],
codename=codename,
permission_objs=permission_objs,
ctype=ctype,
object_pks=object_pks,
merge=merge,
)
if "groups" in entry:
_apply_bulk_permission_entry(
perm_model=GroupObjectPermission,
identity_model=Group,
identity_field="group",
ids=entry["groups"],
codename=codename,
permission_objs=permission_objs,
ctype=ctype,
object_pks=object_pks,
merge=merge,
)
def permitted_object_ids(
user: User | None,
model: type[Model],
+236 -13
View File
@@ -1,8 +1,10 @@
from __future__ import annotations
import contextlib
import logging
import math
import re
from collections.abc import Iterable
from datetime import datetime
from datetime import timedelta
from decimal import Decimal
@@ -24,6 +26,7 @@ from django.core.validators import MaxValueValidator
from django.core.validators import MinValueValidator
from django.core.validators import RegexValidator
from django.core.validators import integer_validator
from django.db import DataError
from django.db.models import Count
from django.db.models import Q
from django.db.models.functions import Lower
@@ -37,12 +40,12 @@ from django.utils.timezone import make_aware
from django.utils.translation import gettext as _
from drf_spectacular.utils import extend_schema_field
from drf_spectacular.utils import extend_schema_serializer
from drf_writable_nested.serializers import NestedUpdateMixin
from guardian.core import ObjectPermissionChecker
from guardian.shortcuts import get_users_with_perms
from guardian.utils import get_group_obj_perms_model
from guardian.utils import get_user_obj_perms_model
from rest_framework import fields
from rest_framework import relations
from rest_framework import serializers
from rest_framework.exceptions import PermissionDenied
from rest_framework.fields import SerializerMethodField
@@ -742,22 +745,100 @@ class TagSerializer(MatchingModelSerializer, OwnedObjectSerializer):
return super().validate(attrs)
class CorrespondentField(serializers.PrimaryKeyRelatedField[Correspondent]):
class _BatchingManyRelatedField(serializers.ManyRelatedField):
"""
`ManyRelatedField.to_internal_value` resolves each id in the submitted
list with its own `child_relation.to_internal_value()` call -- one query
per item on every PATCH/PUT that sets a `many=True` relation field.
Batch-resolve them instead, falling back to the child relation's normal
(query-per-item) validation for anything that isn't a plausible int pk,
so bad input still gets the usual DRF validation error rather than being
silently dropped.
"""
@staticmethod
def _normalize_pk(item) -> int | None:
# Excludes bool: DRF's own PrimaryKeyRelatedField rejects it too
# (True == 1 would otherwise silently match pk 1).
if isinstance(item, bool):
return None
try:
return int(item)
except (TypeError, ValueError):
return None
def to_internal_value(self, data):
if isinstance(data, str) or not hasattr(data, "__iter__"):
self.fail("not_a_list", input_type=type(data).__name__)
if not self.allow_empty and len(data) == 0:
self.fail("empty")
item_pks = [(item, self._normalize_pk(item)) for item in data]
candidate_pks = {pk for _, pk in item_pks if pk is not None}
# Django's IntegerFieldOverflow guard (-> EmptyResultSet, i.e. no
# match) only covers exact/gt/gte/lt/lte lookups, not `in` -- an
# out-of-range int in `pk__in=` reaches the DB driver as-is and
# raises OverflowError (SQLite) / DataError (Postgres) instead of
# cleanly matching nothing. The per-item `exact`-lookup fallback
# below IS covered, so on that failure just skip the batch and let
# every item resolve individually -- each still costs one query,
# but reports the normal validation error instead of a raw 500.
try:
resolved_by_pk = {
obj.pk: obj
for obj in self.child_relation.get_queryset().filter(
pk__in=candidate_pks,
)
}
except (OverflowError, DataError):
resolved_by_pk = {}
result = []
for item, pk in item_pks:
obj = resolved_by_pk.get(pk) if pk is not None else None
result.append(
obj if obj is not None else self.child_relation.to_internal_value(item),
)
return result
class BatchResolvingPrimaryKeyRelatedField(serializers.PrimaryKeyRelatedField):
"""
A PrimaryKeyRelatedField whose `many=True` form (a DRF ManyRelatedField)
resolves all submitted ids with one batched query instead of one query
per id. Subclasses only need to implement `get_queryset()` as usual --
only `TagsField` is used with `many=True` today, but this is the base
for all four so the fix isn't tag-specific: if a future PR puts
`many=True` on correspondent/document_type/storage_path, it inherits the
same batching instead of reintroducing this as a new bug to rediscover.
"""
@classmethod
def many_init(cls, *args, **kwargs):
list_kwargs = {"child_relation": cls(*args, **kwargs)}
for key, value in kwargs.items():
if key in relations.MANY_RELATION_KWARGS:
list_kwargs[key] = value
return _BatchingManyRelatedField(**list_kwargs)
class CorrespondentField(BatchResolvingPrimaryKeyRelatedField[Correspondent]):
def get_queryset(self):
return Correspondent.objects.all()
class TagsField(serializers.PrimaryKeyRelatedField[Tag]):
class TagsField(BatchResolvingPrimaryKeyRelatedField[Tag]):
def get_queryset(self):
return Tag.objects.all()
class DocumentTypeField(serializers.PrimaryKeyRelatedField[DocumentType]):
class DocumentTypeField(BatchResolvingPrimaryKeyRelatedField[DocumentType]):
def get_queryset(self):
return DocumentType.objects.all()
class StoragePathField(serializers.PrimaryKeyRelatedField[StoragePath]):
class StoragePathField(BatchResolvingPrimaryKeyRelatedField[StoragePath]):
def get_queryset(self):
return StoragePath.objects.all()
@@ -876,8 +957,106 @@ def validate_documentlink_targets(user, doc_ids):
)
# A CustomField lookup cache scoped to a single field/serializer instance
# only helps within that one instance's own validation pass. Several call
# sites, though, build more than one CustomFieldInstanceSerializer (or its
# CustomFieldInstanceListSerializer/field) for the same request and pass
# each of them `context=self.context` -- the *same* dict object, not a
# copy -- e.g. bulk-edit's _validate_custom_field_values() constructing a
# fresh CustomFieldInstanceSerializer per submitted field. That context
# dict is already request-scoped (DRF builds it fresh per request via
# get_serializer_context()), so stashing the resolved CustomField objects
# there -- rather than in some new global/thread-local cache -- lets every
# one of those separately-instantiated serializers reuse them for free
# while staying entirely within DRF's existing, already-request-scoped
# machinery.
_CUSTOM_FIELD_CONTEXT_CACHE_KEY = "_custom_field_lookup_cache"
class _CachingCustomFieldPrimaryKeyField(serializers.PrimaryKeyRelatedField):
"""
Resolves CustomField ids with as few queries as possible: a per-instance
cache for repeat lookups on this exact field instance, backed by a
shared cache on the serializer context (see _CUSTOM_FIELD_CONTEXT_CACHE_KEY
above) so later, separately-instantiated fields for the same request
reuse what was already resolved instead of re-querying.
"""
def __init__(self, **kwargs: Any) -> None:
super().__init__(**kwargs)
self._cache: dict[int, CustomField] = {}
def _shared_cache(self) -> dict[int, CustomField]:
return self.context.setdefault(_CUSTOM_FIELD_CONTEXT_CACHE_KEY, {})
@staticmethod
def _normalize_pk(data: Any) -> int | None:
"""
Returns `data` coerced to the int a valid CustomField pk would be,
or None if `data` isn't a plausible pk (wrong type, unhashable,
non-numeric, or a bool -- DRF itself rejects bools as pks since
`True == 1` would otherwise silently match). None tells callers to
leave `data` alone and let `super().to_internal_value()` report the
normal validation error instead of touching the cache/queryset with
it directly.
"""
if isinstance(data, bool):
return None
try:
return int(data)
except (TypeError, ValueError):
return None
def prefetch(self, ids: Iterable[Any]) -> None:
shared_cache = self._shared_cache()
candidates = {pk for i in ids if (pk := self._normalize_pk(i)) is not None}
missing = {
i for i in candidates if i not in self._cache and i not in shared_cache
}
if missing:
for obj in self.get_queryset().filter(pk__in=missing):
shared_cache[obj.pk] = obj
for i in candidates:
obj = shared_cache.get(i)
if obj is not None:
self._cache[i] = obj
def to_internal_value(self, data: Any) -> CustomField:
pk = self._normalize_pk(data)
if pk is None:
return super().to_internal_value(data)
if pk in self._cache:
return self._cache[pk]
shared_cache = self._shared_cache()
if pk in shared_cache:
obj = shared_cache[pk]
self._cache[pk] = obj
return obj
obj: CustomField = super().to_internal_value(data)
self._cache[obj.pk] = obj
shared_cache[obj.pk] = obj
return obj
class CustomFieldInstanceListSerializer(serializers.ListSerializer):
def to_internal_value(self, data: Any) -> list[Any]:
if isinstance(data, list):
field_ids = []
for item in data:
if not isinstance(item, dict) or "field" not in item:
continue
try:
hash(item["field"])
except TypeError:
continue
field_ids.append(item["field"])
if field_ids:
self.child.fields["field"].prefetch(field_ids)
return super().to_internal_value(data)
class CustomFieldInstanceSerializer(serializers.ModelSerializer[CustomFieldInstance]):
field = serializers.PrimaryKeyRelatedField(queryset=CustomField.objects.all())
field = _CachingCustomFieldPrimaryKeyField(queryset=CustomField.objects.all())
value = ReadWriteSerializerMethodField(allow_null=True)
def create(self, validated_data):
@@ -978,6 +1157,7 @@ class CustomFieldInstanceSerializer(serializers.ModelSerializer[CustomFieldInsta
class Meta:
model = CustomFieldInstance
list_serializer_class = CustomFieldInstanceListSerializer
fields = [
"value",
"field",
@@ -1043,7 +1223,6 @@ class DocumentVersionInfoSerializer(serializers.Serializer[_DocumentVersionInfo]
)
class DocumentSerializer(
OwnedObjectSerializer,
NestedUpdateMixin,
DocumentUpdateFieldsModelSerializer,
):
correspondent = CorrespondentField(allow_null=True)
@@ -1258,16 +1437,60 @@ class DocumentSerializer(
if tag not in inbox_tags_not_being_added
]
if settings.AUDIT_LOG_ENABLED:
with set_actor(self.user):
super().update(instance, validated_data)
else:
custom_fields_data = validated_data.pop("custom_fields", None)
actor_context = (
set_actor(self.user)
if settings.AUDIT_LOG_ENABLED
else contextlib.nullcontext()
)
with actor_context:
super().update(instance, validated_data)
if custom_fields_data is not None:
self._sync_custom_fields(instance, custom_fields_data)
# hard delete custom field instances that were soft deleted
CustomFieldInstance.deleted_objects.filter(document=instance).delete()
return instance
def _sync_custom_fields(
self,
instance: Document,
custom_fields_data: list[dict],
) -> None:
"""
Create/update a CustomFieldInstance for every (field, value) pair in
custom_fields_data, then hard-delete any of the document's existing
instances whose field wasn't included.
Replaces drf-writable-nested's generic
update_or_create_reverse_relations()/delete_reverse_relations_if_need():
that machinery always matched submitted items by an instance "id"
this client payload never sends, so its own pk-matching never did
anything for this field -- the real upsert semantics were always
CustomFieldInstanceSerializer.create()'s update_or_create() below.
On a partial (PATCH) update, DRF skips the "value" field's required
check on the first validation pass because it's absent entirely from
the payload item, not merely null. drf-writable-nested happened to
re-enforce that check itself, by re-validating each item against a
freshly built, non-partial child serializer before saving. Reproduce
that specific guarantee explicitly here, since CustomFieldInstance's
"value" is not optional.
"""
for item in custom_fields_data:
if "value" not in item:
raise serializers.ValidationError(
{"custom_fields": [{"value": ["This field is required."]}]},
)
kept_field_ids: set[int] = set()
serializer = CustomFieldInstanceSerializer()
for item in custom_fields_data:
kept_field_ids.add(item["field"].pk)
serializer.create({**item, "document": instance})
CustomFieldInstance.objects.filter(document=instance).exclude(
field_id__in=kept_field_ids,
).hard_delete()
def __init__(self, *args, **kwargs) -> None:
self.truncate_content = kwargs.pop("truncate_content", False)
@@ -5,7 +5,9 @@ from unittest.mock import ANY
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 rest_framework import status
from rest_framework.test import APITestCase
@@ -13,6 +15,9 @@ from rest_framework.test import APITestCase
from documents.models import CustomField
from documents.models import CustomFieldInstance
from documents.models import Document
from documents.serialisers import CustomFieldInstanceSerializer
from documents.serialisers import DocumentSerializer
from documents.tests.factories import DocumentFactory
from documents.tests.utils import DirectoriesMixin
@@ -530,6 +535,137 @@ class TestCustomFieldsAPI(DirectoriesMixin, APITestCase):
doc.refresh_from_db()
self.assertEqual(len(doc.custom_fields.all()), 10)
def test_document_serializer_custom_fields_validation_batches_field_lookup(
self,
) -> None:
"""
GIVEN:
- A document is being validated with several custom field values
at once (as happens on every PATCH/PUT/POST)
WHEN:
- The serializer is validated
THEN:
- The referenced CustomField objects are resolved with a single
query, not one query per custom field
"""
doc = DocumentFactory(mime_type="application/pdf")
custom_fields = [
CustomField.objects.create(
name=f"Test Custom Field {i}",
data_type=CustomField.FieldDataType.STRING,
)
for i in range(5)
]
serializer = DocumentSerializer(
doc,
data={
"custom_fields": [
{"field": custom_field.id, "value": "test value"}
for custom_field in custom_fields
],
},
partial=True,
)
with CaptureQueriesContext(connection) as ctx:
self.assertTrue(serializer.is_valid(), serializer.errors)
custom_field_lookups = [
query
for query in ctx.captured_queries
if 'FROM "documents_customfield" WHERE "documents_customfield"."id"'
in query["sql"]
]
self.assertEqual(
len(custom_field_lookups),
1,
"Expected a single batched query to resolve the custom fields, "
f"got {len(custom_field_lookups)}: {custom_field_lookups}",
)
def test_custom_field_lookup_reuses_shared_context_cache(self) -> None:
"""
GIVEN:
- A CustomField has already been resolved once, by a serializer
sharing a given `context` dict
WHEN:
- A second, separately-instantiated CustomFieldInstanceSerializer
validates the same field id, sharing that same context
(this mirrors what real call sites do, e.g. bulk-edit's
validate_custom_fields()/_validate_custom_field_values()
constructing a fresh CustomFieldInstanceSerializer per
submitted field, all sharing the outer serializer's context)
THEN:
- No additional query is issued to resolve the CustomField
"""
custom_field = CustomField.objects.create(
name="Test Custom Field",
data_type=CustomField.FieldDataType.STRING,
)
context: dict = {}
first_pass = CustomFieldInstanceSerializer(
data={"field": custom_field.id, "value": "a"},
context=context,
)
self.assertTrue(first_pass.is_valid(), first_pass.errors)
second_pass = CustomFieldInstanceSerializer(
data={"field": custom_field.id, "value": "b"},
context=context,
)
with CaptureQueriesContext(connection) as ctx:
self.assertTrue(second_pass.is_valid(), second_pass.errors)
custom_field_lookups = [
query
for query in ctx.captured_queries
if 'FROM "documents_customfield" WHERE "documents_customfield"."id"'
in query["sql"]
]
self.assertEqual(
len(custom_field_lookups),
0,
"Expected the second, separately-instantiated serializer to reuse "
f"the already-resolved CustomField, got: {custom_field_lookups}",
)
def test_custom_field_validation_rejects_malformed_field_value(self) -> None:
"""
GIVEN:
- A document is being validated with a malformed custom_fields
entry whose "field" value is neither a valid CustomField id
nor a type DRF's own PrimaryKeyRelatedField can safely reject
on its own (unhashable, or a non-numeric scalar)
WHEN:
- The serializer is validated
THEN:
- A normal validation error is raised, not an unhandled
TypeError/ValueError escaping past DRF's validation layer
"""
doc = DocumentFactory(mime_type="application/pdf")
bad_field_values = {
"unhashable-list": [],
"unhashable-dict": {},
"non-numeric-scalar": "abc",
}
for case_id, bad_field_value in bad_field_values.items():
with self.subTest(case_id):
serializer = DocumentSerializer(
doc,
data={
"custom_fields": [
{"field": bad_field_value, "value": "test value"},
],
},
partial=True,
)
self.assertFalse(serializer.is_valid())
self.assertIn("custom_fields", serializer.errors)
def test_change_custom_field_instance_value(self) -> None:
"""
GIVEN:
@@ -660,6 +796,72 @@ class TestCustomFieldsAPI(DirectoriesMixin, APITestCase):
assert _cf_4 is not None
self.assertEqual(_cf_4.value, date_value)
def test_delete_custom_field_instance_is_hard_deleted_not_soft_deleted(
self,
) -> None:
"""
GIVEN:
- A document has two custom field instances
WHEN:
- A PATCH request updates custom_fields to omit one of them
THEN:
- The omitted instance is immediately hard-deleted: it is gone
from both the default manager and the soft-deleted manager,
not left in a soft-deleted, not-yet-purged state
"""
doc = Document.objects.create(
title="WOW",
content="the content",
checksum="123-hard-delete",
mime_type="application/pdf",
)
kept_field = CustomField.objects.create(
name="Kept Field",
data_type=CustomField.FieldDataType.STRING,
)
removed_field = CustomField.objects.create(
name="Removed Field",
data_type=CustomField.FieldDataType.STRING,
)
resp = self.client.patch(
f"/api/documents/{doc.id}/",
data={
"custom_fields": [
{"field": kept_field.id, "value": "keep me"},
{"field": removed_field.id, "value": "remove me"},
],
},
format="json",
)
self.assertEqual(resp.status_code, status.HTTP_200_OK)
self.assertEqual(CustomFieldInstance.objects.count(), 2)
resp = self.client.patch(
f"/api/documents/{doc.id}/",
data={
"custom_fields": [
{"field": kept_field.id, "value": "keep me"},
],
},
format="json",
)
self.assertEqual(resp.status_code, status.HTTP_200_OK)
self.assertEqual(CustomFieldInstance.objects.count(), 1)
self.assertEqual(
CustomFieldInstance.deleted_objects.filter(field=removed_field).count(),
0,
"Removed custom field instance should be hard-deleted, not left "
"soft-deleted",
)
self.assertEqual(
CustomFieldInstance.global_objects.filter(field=removed_field).count(),
0,
"Removed custom field instance should not exist at all, even in "
"the all-rows manager",
)
def test_custom_field_validation(self) -> None:
"""
GIVEN:
@@ -1351,6 +1553,81 @@ class TestCustomFieldsAPI(DirectoriesMixin, APITestCase):
results = response.data["results"]
self.assertEqual(results[0]["document_count"], 0)
def test_document_update_custom_fields_sync_query_count(self) -> None:
"""
GIVEN:
- A document already has 3 custom field instances attached
WHEN:
- A PATCH request updates 2 of them, adds 1 new one, and omits
the 3rd (which should be deleted)
THEN:
- The omitted custom field is removed via exactly one
delete-shaped query, not drf-writable-nested's old two-step
soft-delete-then-hard-delete-after pattern (two delete
passes)
"""
doc = Document.objects.create(
title="WOW",
content="the content",
checksum="123-sync-count",
mime_type="application/pdf",
)
fields = [
CustomField.objects.create(
name=f"Sync Field {i}",
data_type=CustomField.FieldDataType.STRING,
)
for i in range(4)
]
# Attach the first 3 up front; the 4th is added in the PATCH below,
# and the 3rd is omitted (so it should be deleted).
for field in fields[:3]:
CustomFieldInstance.objects.create(
document=doc,
field=field,
value_text="initial",
)
self.assertEqual(CustomFieldInstance.objects.count(), 3)
with CaptureQueriesContext(connection) as ctx:
resp = self.client.patch(
f"/api/documents/{doc.id}/",
data={
"custom_fields": [
{"field": fields[0].id, "value": "updated 0"},
{"field": fields[1].id, "value": "updated 1"},
{"field": fields[3].id, "value": "new 3"},
],
},
format="json",
)
self.assertEqual(resp.status_code, status.HTTP_200_OK)
delete_queries = [
q
for q in ctx.captured_queries
if 'DELETE FROM "documents_customfieldinstance"' in q["sql"]
or (
'UPDATE "documents_customfieldinstance"' in q["sql"]
and '"deleted_at"' in q["sql"]
and '"deleted_at" = NULL' not in q["sql"]
)
]
self.assertEqual(
len(delete_queries),
1,
"Expected exactly one delete/soft-delete-marking query for the "
f"omitted custom field, got {len(delete_queries)}: {delete_queries}",
)
self.assertEqual(CustomFieldInstance.objects.count(), 3)
doc.refresh_from_db()
values = {cfi.field_id: cfi.value for cfi in doc.custom_fields.all()}
self.assertEqual(values[fields[0].id], "updated 0")
self.assertEqual(values[fields[1].id], "updated 1")
self.assertEqual(values[fields[3].id], "new 3")
self.assertNotIn(fields[2].id, values)
def test_patch_document_invalid_date_custom_field_returns_validation_error(
self,
) -> None:
@@ -11,7 +11,6 @@ from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import FieldError
from django.core.files.uploadedfile import SimpleUploadedFile
from django.test import TestCase as DjangoTestCase
from django.utils import timezone
from rest_framework import status
from rest_framework.test import APITestCase
@@ -22,7 +21,6 @@ from documents.filters import TitleContentFilter
from documents.models import Document
from documents.tests.utils import DirectoriesMixin
from documents.tests.utils import read_streaming_response
from documents.views import DocumentSelectionMixin
if TYPE_CHECKING:
from pathlib import Path
@@ -925,36 +923,3 @@ class TestVersionAwareFilters(TestCase):
self.assertIs(result, queryset)
queryset.filter.assert_not_called()
class TestBulkSelectionExcludesVersions(DjangoTestCase):
def test_select_all_matching_does_not_select_version_documents(self) -> None:
"""
"Select all matching" reconstructs the document list, which never
contains version documents as rows of their own.
"""
user = User.objects.create_superuser(username="bulk_versions")
root = Document.objects.create(
title="shared-title root",
checksum="bulk-root",
mime_type="application/pdf",
content="root",
)
Document.objects.create(
title="shared-title version",
checksum="bulk-version",
mime_type="application/pdf",
root_document=root,
version_index=1,
content="version",
)
selected = DocumentSelectionMixin()._resolve_document_ids(
user=user,
validated_data={
"all": True,
"filters": {"title__icontains": "shared-title"},
},
)
self.assertEqual(selected, [root.id])
+79
View File
@@ -1,5 +1,6 @@
import datetime
import json
import re
import shutil
import tempfile
import uuid
@@ -21,7 +22,9 @@ from django.core import mail
from django.core.cache import cache
from django.core.files.uploadedfile import SimpleUploadedFile
from django.db import DataError
from django.db import connection
from django.test import override_settings
from django.test.utils import CaptureQueriesContext
from django.utils import timezone
from guardian.shortcuts import assign_perm
from rest_framework import status
@@ -252,6 +255,82 @@ class TestDocumentApi(DirectoriesMixin, ConsumeTaskMixin, APITestCase):
doc.refresh_from_db()
self.assertEqual(doc.created, date(2023, 6, 28))
def test_document_update_tags_batches_tag_lookup(self) -> None:
"""
GIVEN:
- A document is being updated with several tags at once
WHEN:
- API PATCH request is made setting the document's tags
THEN:
- The referenced Tag objects are resolved with a single batched
query, not one query per tag
"""
doc = Document.objects.create(
title="none",
checksum="123",
mime_type="application/pdf",
)
tags = [TagFactory() for _ in range(8)]
with CaptureQueriesContext(connection) as ctx:
response = self.client.patch(
f"/api/documents/{doc.pk}/",
{"tags": [t.id for t in tags]},
format="json",
)
self.assertEqual(response.status_code, status.HTTP_200_OK)
# Match `"documents_tag"."id" = <literal>` (a single-row WHERE lookup)
# but not the same substring appearing as a JOIN's ON condition
# (`"documents_tag"."id" = "documents_document_tags"."tag_id"`),
# which is a legitimate, unrelated response-serialization query.
single_tag_lookup_re = re.compile(r'"documents_tag"\."id" = \d')
single_tag_lookups = [
q for q in ctx.captured_queries if single_tag_lookup_re.search(q["sql"])
]
self.assertEqual(
len(single_tag_lookups),
0,
"Expected tags to be resolved with a batched query, not "
f"per-tag lookups, got: {single_tag_lookups}",
)
doc.refresh_from_db()
self.assertCountEqual(
doc.tags.values_list("id", flat=True),
[t.id for t in tags],
)
def test_document_update_tags_rejects_out_of_range_id(self) -> None:
"""
GIVEN:
- A document is being updated with a tag id too large for the
database's integer column
WHEN:
- API PATCH request is made setting the document's tags
THEN:
- A normal 400 validation error is returned, not an unhandled
OverflowError/DataError escaping as a 500
Django's IntegerFieldOverflow guard converts an out-of-range int
into a clean "no match" for exact/gt/gte/lt/lte lookups, but not for
`in` -- the batched tag resolution uses `pk__in=`, so this has to be
guarded explicitly rather than relying on Django to do it.
"""
doc = Document.objects.create(
title="none",
checksum="123",
mime_type="application/pdf",
)
response = self.client.patch(
f"/api/documents/{doc.pk}/",
{"tags": [99999999999999999999999999999]},
format="json",
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
def test_document_update_legacy_created_format(self) -> None:
"""
GIVEN:
+58
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
@@ -815,6 +820,59 @@ 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(
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:
- 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
"""
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)
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",
)
def test_bulk_objects_delete_all_filtered(self) -> None:
"""
GIVEN:
+224
View File
@@ -5,8 +5,11 @@ from unittest import mock
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
@@ -19,6 +22,7 @@ from documents.models import Document
from documents.models import DocumentType
from documents.models import StoragePath
from documents.models import Tag
from documents.permissions import set_permissions_for_objects
from documents.tests.utils import DirectoriesMixin
@@ -344,6 +348,100 @@ class TestBulkEdit(DirectoriesMixin, TestCase):
assert _cf_3 is not None
self.assertNotIn(self.doc3.id, _cf_3.value)
def test_modify_custom_fields_batches_field_lookup(self) -> None:
"""
GIVEN:
- Several documents are being bulk-edited to add several custom
fields at once
WHEN:
- modify_custom_fields runs
THEN:
- Each CustomField is resolved with one batched query total, not
once per (field, document) pair
"""
docs = [
Document.objects.create(checksum=f"batch-{i}", title=f"batch-{i}")
for i in range(6)
]
fields = [
CustomField.objects.create(
name=f"Batch Field {i}",
data_type=CustomField.FieldDataType.STRING,
)
for i in range(4)
]
with CaptureQueriesContext(connection) as ctx:
bulk_edit.modify_custom_fields(
[doc.id for doc in docs],
add_custom_fields=[field.id for field in fields],
remove_custom_fields=[],
)
field_lookups = [
q
for q in ctx.captured_queries
if 'FROM "documents_customfield"' in q["sql"]
]
self.assertEqual(
len(field_lookups),
1,
"Expected a single batched query to resolve the custom fields, "
f"got {len(field_lookups)}: {field_lookups}",
)
for doc in docs:
self.assertEqual(doc.custom_fields.count(), len(fields))
def test_modify_custom_fields_batches_document_lookup_for_documentlink(
self,
) -> None:
"""
GIVEN:
- Several documents are being bulk-edited to add a DOCUMENTLINK
custom field at once
WHEN:
- modify_custom_fields runs
THEN:
- The Document rows needed to reflect the symmetrical links are
resolved with one batched query total, not once per document
"""
docs = [
Document.objects.create(checksum=f"link-{i}", title=f"link-{i}")
for i in range(6)
]
target = Document.objects.create(checksum="link-target", title="link-target")
doclink_field = CustomField.objects.create(
name="Related",
data_type=CustomField.FieldDataType.DOCUMENTLINK,
)
with CaptureQueriesContext(connection) as ctx:
bulk_edit.modify_custom_fields(
[doc.id for doc in docs],
add_custom_fields={doclink_field.id: [target.id]},
remove_custom_fields=[],
)
single_document_lookups = [
q
for q in ctx.captured_queries
if 'FROM "documents_document"' in q["sql"]
and '"documents_document"."id" = ' in q["sql"]
]
self.assertEqual(
len(single_document_lookups),
0,
"Expected document rows to come from a batched query, not "
f"per-document lookups, got: {single_document_lookups}",
)
for doc in docs:
self.assertEqual(
doc.custom_fields.get(field=doclink_field).value,
[target.id],
)
def test_modify_custom_fields_doclink_self_link(self) -> None:
"""
GIVEN:
@@ -510,6 +608,132 @@ 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(
self,
m,
) -> None:
"""
GIVEN:
- Many documents are being bulk-edited to set permissions at once
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 = {
"view": {
"users": [self.user1.id, self.user2.id],
"groups": [self.group2.id],
},
"change": {
"users": [self.user1.id],
"groups": [self.group2.id],
},
}
def run_with_n_documents(n: int) -> int:
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,
)
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",
)
@mock.patch("documents.tasks.bulk_update_documents.apply_async")
def test_set_permissions_grants_direct_perm_even_if_already_granted_via_group(
self,
m,
) -> None:
"""
GIVEN:
- A user already has view access to a document via group
membership, with no direct grant of their own
WHEN:
- set_permissions explicitly grants that same user direct view
access via bulk_edit
THEN:
- A direct permission grant is created for the user, not skipped
because they already have equivalent access via the group
Regression test: guardian's queryset-aware assign_perm() (routed to
when the target is a list/queryset) skips creating a direct row for
anyone whose ObjectPermissionChecker.has_perm() already returns True
-- which includes group-derived access. The single-object assign_perm
this bulk path replaces has no such check; it always ensures a
direct row via get_or_create. Losing that guarantee would mean
revoking the group's grant later silently strips access that was
supposed to be explicit.
"""
self.doc1.owner = self.user1
self.doc1.save()
self.user1.groups.add(self.group1)
assign_perm("view_document", self.group1, self.doc1)
bulk_edit.set_permissions(
[self.doc1.id],
set_permissions={
"view": {"users": [self.user1.id], "groups": []},
},
merge=True,
)
direct_users = get_users_with_perms(
self.doc1,
only_with_perms_in=["view_document"],
with_group_users=False,
)
self.assertIn(self.user1, direct_users)
def test_set_permissions_for_objects_raises_for_unknown_action(self) -> None:
"""
GIVEN:
- An unrecognized permission action name with users to grant it
to
WHEN:
- set_permissions_for_objects is called
THEN:
- Permission.DoesNotExist is raised, not a silent no-op
Regression test: the endpoint that calls this
(BulkEditObjectPermissionsView) never actually validates action
names against the raw client-supplied permissions dict --
BulkEditObjectsSerializer._validate_permissions calls
validate_set_permissions() only for its side-effecting user/group id
checks and discards the filtered dict it returns -- so a bogus
action key reaches this function as-is. Resolving the Permission via
a bare `.filter()` (which returns empty instead of raising) would
silently drop the grant and report success.
"""
with self.assertRaises(Permission.DoesNotExist):
set_permissions_for_objects(
{"not_a_real_action": {"users": [self.user1.id], "groups": []}},
Document,
[self.doc1.pk],
)
@mock.patch("documents.models.Document.delete")
def test_delete_documents_old_uuid_field(self, m) -> None:
m.side_effect = Exception("Data too long for column 'transaction_id' at row 1")
+58
View File
@@ -0,0 +1,58 @@
from django.db import connection
from django.test import TestCase
from django.test.utils import CaptureQueriesContext
from documents.data_models import DocumentMetadataOverrides
from documents.models import CustomField
from documents.models import CustomFieldInstance
from documents.tests.factories import DocumentFactory
from documents.tests.utils import DirectoriesMixin
class TestDocumentMetadataOverridesFromDocument(DirectoriesMixin, TestCase):
def test_from_document_batches_custom_field_lookup_after_refresh_from_db(
self,
) -> None:
"""
GIVEN:
- A document has several custom field values
- The document instance has just been refreshed from the database,
which drops any prefetched related objects (as
send_websocket_document_updated does before building overrides)
WHEN:
- DocumentMetadataOverrides.from_document() reads the document's
custom field values
THEN:
- The referenced CustomField objects are resolved with a single
query, not one query per custom field
"""
doc = DocumentFactory(mime_type="application/pdf")
for i in range(5):
CustomFieldInstance.objects.create(
document=doc,
field=CustomField.objects.create(
name=f"Test Custom Field {i}",
data_type=CustomField.FieldDataType.STRING,
),
value_text="value",
)
doc.refresh_from_db()
with CaptureQueriesContext(connection) as ctx:
overrides = DocumentMetadataOverrides.from_document(doc)
self.assertEqual(len(overrides.custom_fields), 5)
unbatched_field_lookups = [
query
for query in ctx.captured_queries
if 'FROM "documents_customfield" WHERE "documents_customfield"."id"'
in query["sql"]
]
self.assertEqual(
unbatched_field_lookups,
[],
"Expected CustomField data to come from the CustomFieldInstance "
"join, not a separate per-instance lookup, "
f"got: {unbatched_field_lookups}",
)
+7 -10
View File
@@ -178,7 +178,7 @@ from documents.permissions import has_perms_owner_aware
from documents.permissions import has_system_status_permission
from documents.permissions import permitted_document_ids
from documents.permissions import permitted_object_ids
from documents.permissions import set_permissions_for_object
from documents.permissions import set_permissions_for_objects
from documents.plugins.date_parsing import get_date_parser
from documents.schema import generate_object_with_permissions_schema
from documents.search import SearchHit
@@ -2794,11 +2794,8 @@ class DocumentSelectionMixin:
for key, value in filters.items()
if key not in _TANTIVY_SEARCH_PARAM_NAMES
}
# Operations are addressed to roots, a caller that wants
# to act on a specific version passes its id explicitly instead
permitted_documents = Document.objects.filter(
id__in=permitted_document_ids(user),
root_document__isnull=True,
)
# orm-filtered docs
filtered_documents = DocumentFilterSet(
@@ -4920,12 +4917,12 @@ class BulkEditObjectsView(PassUserMixin):
qs_owner_update.update(owner=owner)
if "permissions" in serializer.validated_data:
for obj in qs:
set_permissions_for_object(
permissions=permissions,
object=obj,
merge=merge,
)
set_permissions_for_objects(
permissions=permissions,
model=object_class,
pks=qs.values_list("pk", flat=True),
merge=merge,
)
except Exception as e:
logger.warning(
-75
View File
@@ -253,81 +253,6 @@ class TestAPIMailAccounts(DirectoriesMixin, APITestCase):
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(response.data["success"], True)
def test_mail_account_test_existing_no_global_perms(self) -> None:
"""
GIVEN:
- Existing account without an owner
- User without any mail account permissions
WHEN:
- API call is made to test the account by id
THEN:
- API returns forbidden
"""
account = MailAccountFactory(
username="admin",
password="secret",
imap_server="server.example.com",
imap_port=443,
owner=None,
)
user = User.objects.create_user(username="no_perms")
self.client.force_authenticate(user=user)
response = self.client.post(
f"{self.ENDPOINT}test/",
json.dumps(
{
"id": account.pk,
"imap_server": "server.example.com",
"imap_port": 443,
"imap_security": MailAccount.ImapSecurity.SSL,
"username": "admin",
"password": "******",
},
),
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
self.assertEqual(response.content.decode(), "Insufficient permissions")
def test_mail_account_test_existing_object_perms_only(self) -> None:
"""
GIVEN:
- Existing account owned by another user
- User with an object level grant but no global change permission
WHEN:
- API call is made to test the account by id
THEN:
- API returns forbidden
"""
owner = User.objects.create_user(username="account_owner")
account = MailAccountFactory(
username="admin",
password="secret",
imap_server="server.example.com",
imap_port=443,
owner=owner,
)
user = User.objects.create_user(username="object_perms_only")
assign_perm("change_mailaccount", user, account)
self.client.force_authenticate(user=user)
response = self.client.post(
f"{self.ENDPOINT}test/",
json.dumps(
{
"id": account.pk,
"imap_server": "server.example.com",
"imap_port": 443,
"imap_security": MailAccount.ImapSecurity.SSL,
"username": "admin",
"password": "******",
},
),
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
def test_mail_account_test_existing_nonexistent_id_forbidden(self) -> None:
response = self.client.post(
f"{self.ENDPOINT}test/",
+1 -3
View File
@@ -2195,9 +2195,7 @@ class TestMailAccountTestView(APITestCase):
password="testpassword",
)
self.user.user_permissions.add(
*Permission.objects.filter(
codename__in=["add_mailaccount", "change_mailaccount"],
),
*Permission.objects.filter(codename__in=["add_mailaccount"]),
)
self.user.save()
self.client.force_authenticate(user=self.user)
+1 -3
View File
@@ -106,9 +106,7 @@ class MailAccountViewSet(PassUserMixin, ModelViewSet[MailAccount]):
except (TypeError, ValueError, MailAccount.DoesNotExist):
return HttpResponseForbidden("Insufficient permissions")
if not request.user.has_perms(
["paperless_mail.change_mailaccount"],
) or not has_perms_owner_aware(
if not has_perms_owner_aware(
request.user,
"change_mailaccount",
existing_account,
Generated
+4 -14
View File
@@ -4,11 +4,11 @@ requires-python = ">=3.11"
resolution-markers = [
"python_full_version >= '3.15' and sys_platform == 'darwin'",
"python_full_version >= '3.15' and sys_platform == 'linux'",
"python_full_version == '3.14.*' and platform_machine == 'x86_64' and sys_platform == 'linux'",
"python_full_version == '3.14.*' and platform_machine == 'aarch64' and sys_platform == 'linux'",
"python_full_version >= '3.12' and python_full_version < '3.15' and sys_platform == 'darwin'",
"python_full_version == '3.12.*' and platform_machine == 'x86_64' and sys_platform == 'linux'",
"python_full_version == '3.12.*' and platform_machine == 'aarch64' and sys_platform == 'linux'",
"python_full_version == '3.14.*' and platform_machine == 'x86_64' and sys_platform == 'linux'",
"python_full_version == '3.14.*' and platform_machine == 'aarch64' and sys_platform == 'linux'",
"(python_full_version >= '3.12' and python_full_version < '3.15' and platform_machine != 'aarch64' and platform_machine != 'x86_64' and sys_platform == 'linux') or (python_full_version == '3.13.*' and platform_machine == 'aarch64' and sys_platform == 'linux') or (python_full_version == '3.13.*' and platform_machine == 'x86_64' and sys_platform == 'linux')",
"python_full_version < '3.12' and sys_platform == 'darwin'",
"python_full_version < '3.12' and sys_platform == 'linux'",
@@ -1175,14 +1175,6 @@ wheels = [
{ url = "https://files.pythonhosted.org/packages/9d/76/d08f5c79f7643dbff4512605c28b75481966ed6c8cc9b397c9dd2ee91cd1/drf_spectacular_sidecar-2026.7.1-py3-none-any.whl", hash = "sha256:bc6d50c9b64660e45e09296d39553b3e759eedd825fc41d631ee5b3f88e0c5de", size = 2617384, upload-time = "2026-07-01T13:39:03.787Z" },
]
[[package]]
name = "drf-writable-nested"
version = "0.7.2"
source = { registry = "https://pypi.org/simple" }
wheels = [
{ url = "https://files.pythonhosted.org/packages/e8/57/df87d92fbfc3f0f2ef1a49c47f2a83389a4a13b7acf62b8bf7b223627d82/drf_writable_nested-0.7.2-py3-none-any.whl", hash = "sha256:4a3d2737c1cbfafa690e30236b169112e5b23cfe3d288f3992b0651a1b828c4d", size = 10570, upload-time = "2025-03-10T19:59:05.482Z" },
]
[[package]]
name = "execnet"
version = "2.1.2"
@@ -2895,7 +2887,6 @@ dependencies = [
{ name = "djangorestframework" },
{ name = "drf-spectacular" },
{ name = "drf-spectacular-sidecar" },
{ name = "drf-writable-nested" },
{ name = "filelock" },
{ name = "flower" },
{ name = "gotenberg-client" },
@@ -3045,7 +3036,6 @@ requires-dist = [
{ name = "djangorestframework", specifier = "~=3.16" },
{ name = "drf-spectacular", specifier = "~=0.30" },
{ name = "drf-spectacular-sidecar", specifier = "~=2026.7.1" },
{ name = "drf-writable-nested", specifier = "~=0.7.1" },
{ name = "filelock", specifier = "~=3.32.0" },
{ name = "flower", specifier = "~=2.0.1" },
{ name = "gotenberg-client", specifier = "~=0.14.0" },
@@ -5014,10 +5004,10 @@ version = "2.13.0+cpu"
source = { registry = "https://download.pytorch.org/whl/cpu" }
resolution-markers = [
"python_full_version >= '3.15' and sys_platform == 'linux'",
"python_full_version == '3.12.*' and platform_machine == 'x86_64' and sys_platform == 'linux'",
"python_full_version == '3.12.*' and platform_machine == 'aarch64' and sys_platform == 'linux'",
"python_full_version == '3.14.*' and platform_machine == 'x86_64' and sys_platform == 'linux'",
"python_full_version == '3.14.*' and platform_machine == 'aarch64' and sys_platform == 'linux'",
"python_full_version == '3.12.*' and platform_machine == 'x86_64' and sys_platform == 'linux'",
"python_full_version == '3.12.*' and platform_machine == 'aarch64' and sys_platform == 'linux'",
"(python_full_version >= '3.12' and python_full_version < '3.15' and platform_machine != 'aarch64' and platform_machine != 'x86_64' and sys_platform == 'linux') or (python_full_version == '3.13.*' and platform_machine == 'aarch64' and sys_platform == 'linux') or (python_full_version == '3.13.*' and platform_machine == 'x86_64' and sys_platform == 'linux')",
"python_full_version < '3.12' and sys_platform == 'linux'",
]