mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-07-28 14:54:55 +00:00
Fix (beta): remove unnecessary .distinct() dominating document list queries (#13205)
DocumentViewSet.get_queryset() carried a blanket .distinct() left over from an older query shape (predating the version-file feature and the correlated-subquery rewrite of num_notes/effective_content). Nothing in the current base queryset can produce duplicate document rows: select_related is all FK-to-PK, and the permission filter is a boolean id__in predicate, not a join. id (the PK) is also always selected and inherently unique, so .distinct() was a structural no-op for correctness. It was not a no-op for cost. EXPLAIN showed it forcing a full sort-then- dedupe over the entire permission-visible document set before LIMIT could apply -- 340k rows for a 25-row page, with the effective_content/num_notes correlated subqueries re-executed once per row as a result. This was the dominant cost behind the original report's document overview slowness, well beyond the get_user_can_change N+1 fixed separately. Removing it isn't safe on its own, though -- auditing found two existing filters that rely on an M2M join *without* deduping themselves, previously papered over by the blanket .distinct(): - InboxFilter: a document with two tags both flagged is_inbox_tag=True (nothing prevents that) would be returned twice for ?is_in_inbox=true. - CustomFieldsFilter: a document with multiple custom field instances matching different OR-ed branches would be returned once per match. Fixed both locally, matching the pattern ObjectFilter already used for tags__id__in/custom_fields__id__in. Added regression tests for both -- confirmed they fail without the local .distinct() calls. Benchmarked against the same 400k-document/1,000-tag corpus: document list wall-clock for a permission-restricted user drops from ~15.8s to ~2.25s (~7x), closing most of the previous gap to a superuser's ~1.8s. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
be40356d2e
commit
02c6b5f3e4
@@ -161,7 +161,9 @@ class ObjectFilter(Filter):
|
||||
class InboxFilter(Filter):
|
||||
def filter(self, qs, value):
|
||||
if value == "true":
|
||||
return qs.filter(tags__is_inbox_tag=True)
|
||||
# A document can have more than one tag flagged as an inbox tag
|
||||
# (nothing enforces uniqueness), so this join can multiply rows.
|
||||
return qs.filter(tags__is_inbox_tag=True).distinct()
|
||||
elif value == "false":
|
||||
return qs.exclude(tags__is_inbox_tag=True)
|
||||
else:
|
||||
@@ -268,6 +270,10 @@ class CustomFieldsFilter(Filter):
|
||||
for _, option in enumerate(options):
|
||||
if option.get("label").lower().find(value.lower()) != -1:
|
||||
option_ids.extend([option.get("id")])
|
||||
# A document with multiple custom field instances can match more
|
||||
# than one of these OR-ed branches (or the same branch via
|
||||
# different fields), each via its own join to custom_fields --
|
||||
# dedupe explicitly rather than relying on the caller to.
|
||||
return (
|
||||
qs.filter(custom_fields__field__name__icontains=value)
|
||||
| qs.filter(custom_fields__value_text__icontains=value)
|
||||
@@ -280,7 +286,7 @@ class CustomFieldsFilter(Filter):
|
||||
| qs.filter(custom_fields__value_document_ids__icontains=value)
|
||||
| qs.filter(custom_fields__value_select__in=option_ids)
|
||||
| qs.filter(custom_fields__value_long_text__icontains=value)
|
||||
)
|
||||
).distinct()
|
||||
else:
|
||||
return qs
|
||||
|
||||
|
||||
@@ -883,6 +883,62 @@ class TestDocumentApi(DirectoriesMixin, ConsumeTaskMixin, APITestCase):
|
||||
results = response.data["results"]
|
||||
self.assertEqual(len(results), 3)
|
||||
|
||||
def test_is_in_inbox_filter_no_duplicates_with_multiple_inbox_tags(self) -> None:
|
||||
"""
|
||||
GIVEN:
|
||||
- A document tagged with two different inbox tags
|
||||
WHEN:
|
||||
- The document list is filtered by is_in_inbox=true
|
||||
THEN:
|
||||
- The document appears exactly once, not once per matching tag
|
||||
"""
|
||||
doc = Document.objects.create(title="doc", checksum="c1")
|
||||
inbox_1 = Tag.objects.create(name="inbox1", is_inbox_tag=True)
|
||||
inbox_2 = Tag.objects.create(name="inbox2", is_inbox_tag=True)
|
||||
doc.tags.add(inbox_1, inbox_2)
|
||||
|
||||
response = self.client.get("/api/documents/?is_in_inbox=true")
|
||||
self.assertEqual(response.status_code, status.HTTP_200_OK)
|
||||
results = response.data["results"]
|
||||
self.assertEqual(len(results), 1)
|
||||
self.assertEqual(results[0]["id"], doc.id)
|
||||
|
||||
def test_custom_fields_icontains_filter_no_duplicates(self) -> None:
|
||||
"""
|
||||
GIVEN:
|
||||
- A document with two custom field instances that both match the
|
||||
same custom_fields__icontains search term
|
||||
WHEN:
|
||||
- The document list is filtered by custom_fields__icontains
|
||||
THEN:
|
||||
- The document appears exactly once, not once per matching field
|
||||
"""
|
||||
doc = Document.objects.create(title="doc", checksum="c1")
|
||||
field_1 = CustomField.objects.create(
|
||||
name="apple",
|
||||
data_type=CustomField.FieldDataType.STRING,
|
||||
)
|
||||
field_2 = CustomField.objects.create(
|
||||
name="apricot",
|
||||
data_type=CustomField.FieldDataType.STRING,
|
||||
)
|
||||
CustomFieldInstance.objects.create(
|
||||
document=doc,
|
||||
field=field_1,
|
||||
value_text="something",
|
||||
)
|
||||
CustomFieldInstance.objects.create(
|
||||
document=doc,
|
||||
field=field_2,
|
||||
value_text="something else",
|
||||
)
|
||||
|
||||
response = self.client.get("/api/documents/?custom_fields__icontains=ap")
|
||||
self.assertEqual(response.status_code, status.HTTP_200_OK)
|
||||
results = response.data["results"]
|
||||
self.assertEqual(len(results), 1)
|
||||
self.assertEqual(results[0]["id"], doc.id)
|
||||
|
||||
def test_custom_field_select_filter(self) -> None:
|
||||
"""
|
||||
GIVEN:
|
||||
|
||||
@@ -1063,9 +1063,16 @@ class DocumentViewSet(
|
||||
.values("count"),
|
||||
output_field=IntegerField(),
|
||||
)
|
||||
# No .distinct() here: nothing in this base queryset can produce
|
||||
# duplicate document rows (select_related below is all FK-to-PK;
|
||||
# permission filtering is a boolean id__in predicate, not a join).
|
||||
# M2M-based filters that *do* introduce a join (e.g. tags__id__in)
|
||||
# already call .distinct() themselves where they need it -- see
|
||||
# ObjectFilter.filter(). A blanket .distinct() here forces the
|
||||
# database to fully sort and dedupe every visible document before
|
||||
# it can apply LIMIT, which is disastrous at scale.
|
||||
return (
|
||||
Document.objects.filter(root_document__isnull=True)
|
||||
.distinct()
|
||||
.order_by("-created", "-id")
|
||||
.annotate(effective_content=Coalesce(latest_version_content, F("content")))
|
||||
.annotate(num_notes=Coalesce(note_count, 0))
|
||||
|
||||
Reference in New Issue
Block a user