Compare commits

...
Author SHA1 Message Date
stumpylog 5865e96a06 Handles a bad client sending malformed JSON or non-int primary keys 2026-08-25 14:26:33 -07:00
Trenton Holmes 172af8362d 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-25 14:26:33 -07:00
Trenton Holmes e46163bfdf 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-25 14:26:33 -07:00
Trenton Holmes f8e5aea8dd 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-25 14:26:33 -07:00
shamoon e24db7023a Tweak: adjust brand leaf opacity 2026-08-25 10:44:49 -07:00
shamoon f5ddc14588 Fix: also check global change_mailaccount with test 2026-08-25 10:44:49 -07:00
8 changed files with 377 additions and 5 deletions
@@ -417,7 +417,7 @@ main {
:host ::ng-deep .navbar-official-logo { :host ::ng-deep .navbar-official-logo {
.leaf { .leaf {
fill: color-mix(in srgb, var(--pngx-primary-text-contrast) 70%, var(--bs-primary)) !important; fill: color-mix(in srgb, var(--pngx-primary-text-contrast) 85%, var(--bs-primary)) !important;
} }
.text { .text {
+1 -1
View File
@@ -129,7 +129,7 @@ class DocumentMetadataOverrides:
) )
overrides.custom_fields = { overrides.custom_fields = {
custom_field.field.id: custom_field.value 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( groups_with_perms = get_groups_with_perms(
+101 -1
View File
@@ -3,6 +3,7 @@ from __future__ import annotations
import logging import logging
import math import math
import re import re
from collections.abc import Iterable
from datetime import datetime from datetime import datetime
from datetime import timedelta from datetime import timedelta
from decimal import Decimal from decimal import Decimal
@@ -876,8 +877,106 @@ def validate_documentlink_targets(user, doc_ids):
) )
# drf-writable-nested revalidates a document's custom_fields more than once
# per request: once as the ordinary nested list, then again per-item while
# matching existing vs. new CustomFieldInstance rows during save() -- and
# that second pass builds a brand new serializer (and field) instance per
# item (see its update_or_create_reverse_relations / _get_serializer_for_field),
# so a cache on the field instance alone only helps the first pass. It does,
# however, explicitly pass `context=self.context` to every one of those
# fresh serializers -- the *same* dict object the outer DocumentSerializer
# is using, not a copy. 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 later pass 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]): class CustomFieldInstanceSerializer(serializers.ModelSerializer[CustomFieldInstance]):
field = serializers.PrimaryKeyRelatedField(queryset=CustomField.objects.all()) field = _CachingCustomFieldPrimaryKeyField(queryset=CustomField.objects.all())
value = ReadWriteSerializerMethodField(allow_null=True) value = ReadWriteSerializerMethodField(allow_null=True)
def create(self, validated_data): def create(self, validated_data):
@@ -978,6 +1077,7 @@ class CustomFieldInstanceSerializer(serializers.ModelSerializer[CustomFieldInsta
class Meta: class Meta:
model = CustomFieldInstance model = CustomFieldInstance
list_serializer_class = CustomFieldInstanceListSerializer
fields = [ fields = [
"value", "value",
"field", "field",
@@ -5,7 +5,9 @@ from unittest.mock import ANY
from django.contrib.auth.models import Permission from django.contrib.auth.models import Permission
from django.contrib.auth.models import User from django.contrib.auth.models import User
from django.db import connection
from django.test import override_settings from django.test import override_settings
from django.test.utils import CaptureQueriesContext
from guardian.shortcuts import assign_perm from guardian.shortcuts import assign_perm
from rest_framework import status from rest_framework import status
from rest_framework.test import APITestCase 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 CustomField
from documents.models import CustomFieldInstance from documents.models import CustomFieldInstance
from documents.models import Document 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 from documents.tests.utils import DirectoriesMixin
@@ -530,6 +535,136 @@ class TestCustomFieldsAPI(DirectoriesMixin, APITestCase):
doc.refresh_from_db() doc.refresh_from_db()
self.assertEqual(len(doc.custom_fields.all()), 10) 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 is what drf-writable-nested does: it rebuilds a fresh
serializer -- and fresh field instances -- per item while
matching existing vs. new instances during save())
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: def test_change_custom_field_instance_value(self) -> None:
""" """
GIVEN: GIVEN:
+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}",
)
+75
View File
@@ -253,6 +253,81 @@ class TestAPIMailAccounts(DirectoriesMixin, APITestCase):
self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(response.data["success"], True) 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: def test_mail_account_test_existing_nonexistent_id_forbidden(self) -> None:
response = self.client.post( response = self.client.post(
f"{self.ENDPOINT}test/", f"{self.ENDPOINT}test/",
+3 -1
View File
@@ -2195,7 +2195,9 @@ class TestMailAccountTestView(APITestCase):
password="testpassword", password="testpassword",
) )
self.user.user_permissions.add( self.user.user_permissions.add(
*Permission.objects.filter(codename__in=["add_mailaccount"]), *Permission.objects.filter(
codename__in=["add_mailaccount", "change_mailaccount"],
),
) )
self.user.save() self.user.save()
self.client.force_authenticate(user=self.user) self.client.force_authenticate(user=self.user)
+3 -1
View File
@@ -106,7 +106,9 @@ class MailAccountViewSet(PassUserMixin, ModelViewSet[MailAccount]):
except (TypeError, ValueError, MailAccount.DoesNotExist): except (TypeError, ValueError, MailAccount.DoesNotExist):
return HttpResponseForbidden("Insufficient permissions") return HttpResponseForbidden("Insufficient permissions")
if not has_perms_owner_aware( if not request.user.has_perms(
["paperless_mail.change_mailaccount"],
) or not has_perms_owner_aware(
request.user, request.user,
"change_mailaccount", "change_mailaccount",
existing_account, existing_account,