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).
The permission filter OR'd three querysets together on top of a
queryset that could already carry two independent tags__id__all
joins, letting a document that matched more than one branch (e.g.
unowned + group-permissioned) come back twice. Replaced it with a
single id__in filter against the existing permitted_document_ids
helper, which is join-free and can't hit this.
* Fix: selection_data re-derives the filtered document set 5 times over
_get_selection_data_for_queryset() (powers ?include_selection_data=true on
the document list and search endpoints) computed document_count for
Correspondent/DocumentType/StoragePath/Tag/CustomField by embedding the
caller's full filtered queryset -- filters plus the permission check -- as
a subquery inside 5 separate Count(filter=Q(documents__in=queryset), ...)
calls. Each one re-evaluates that whole queryset from scratch.
Resolve the document ids once into a concrete list and reuse it across all
five annotations instead. For Tag/CustomField specifically (M2M via a
through-model table), also route through annotate_document_count_by_ids()
-- extracted from the tag/custom-field document_count fix (#13203) -- to
avoid the same Count(filter=Q(id__in=...), distinct=True)-on-an-M2M-relation
anti-pattern diagnosed there.
On a 400k-document/1,000-tag corpus, the correspondents portion alone
previously didn't finish within several minutes (killed twice while
investigating, including one run that left a zombie query still consuming
a CPU 30+ minutes later). All five queries together now complete in
~30-35s. Root-caused from a real report (paperless-ngx#13201) via a
different, already-fixed query (#13205) -- this one hasn't been reported
in the wild yet, found by auditing the same call path.
Depends on #13203 for annotate_document_count_by_ids().
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* Address review feedback: drop unnecessary ordering before collecting ids
queryset passed into _get_selection_data_for_queryset() carries the
default/user-specified ordering, which is irrelevant once we're only
collecting a flat id list. Clearing it removes a pointless sort.
No measurable change in benchmarking at 400k documents -- the id-list
materialization/IN-clause cost still dominates -- but it's a free,
strictly-correct cleanup, not just noise-neutral in the other direction.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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>
* Updates code to use a FileResponse for streaming and unlink the file, but keep a handle to it
* Transitions the rest of the code to use FileResponse instead of a basic response, fixes up tests which assumed .content exists
* While here, let's add schema for it
stumpylog
shamoonandGitHub
Gaëtan GOUZIandGitHub
Sebastian SteinbeißerandGitHub
Antoine MérinoandGitHub
