mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-07-25 21:34:54 +00:00
Simplify: dedup HttpParams building, fire selection-data concurrently
Cleanups from a code-simplification pass over the prior commit: - Extract the repeated "build HttpParams from a plain object, skipping null/undefined" loop (previously duplicated in list(), getFew(), and the new getFilterSelectionData()) into a single shared AbstractPaperlessService.withParams() helper. - document-list-view.service.ts: fire the filter_selection_data request concurrently with the primary list request instead of nesting it inside the list request's success callback. They're independent (the follow-up only needs filterRules, not the list response), so there's no reason to wait on one before starting the other. Extracted into loadFilterSelectionData() for readability, and guarded the error path so a failed list request cancels the concurrent follow-up too rather than letting it resolve afterward and clobber the error-state reset. - views.py: give filter_selection_data a lean base queryset (_base_document_queryset) instead of the full get_queryset(), which carries select_related/prefetch_related meant for serializing full Document objects. Benchmarked this specifically: no measured query-time impact in this path, since those calls were never iterating the queryset directly and were already inert here -- keeping it for clarity, not as a performance claim. - Test fallout: since filter_selection_data now fires unconditionally on every non-search reload() rather than only after a successful primary flush, added a generic drain step to both spec files' afterEach so individual tests don't each need to know about this follow-up request unless they specifically assert on its response. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
eb5bf53476
commit
bb77e65d52
@@ -191,6 +191,14 @@ describe('BulkEditorComponent', () => {
|
||||
})
|
||||
|
||||
afterEach(async () => {
|
||||
// A filter_selection_data request now fires concurrently with every
|
||||
// non-search reload(), independent of whether a given test flushes or
|
||||
// even inspects the primary list response. Drain any left unclaimed.
|
||||
httpTestingController.match(
|
||||
(request) =>
|
||||
request.url ===
|
||||
`${environment.apiBaseUrl}documents/filter_selection_data/`
|
||||
)
|
||||
httpTestingController.verify()
|
||||
})
|
||||
|
||||
|
||||
@@ -137,6 +137,16 @@ describe('DocumentListViewService', () => {
|
||||
|
||||
afterEach(() => {
|
||||
documentListViewService.cancelPending()
|
||||
// A filter_selection_data request now fires concurrently with every
|
||||
// non-search reload(), independent of whether the test cares about or
|
||||
// flushes the primary list response. Drain any that a test didn't
|
||||
// explicitly claim via flushSelectionDataRequest, so unrelated tests
|
||||
// don't have to know about this follow-up request to pass verify().
|
||||
httpTestingController.match(
|
||||
(request) =>
|
||||
request.url ===
|
||||
`${environment.apiBaseUrl}documents/filter_selection_data/`
|
||||
)
|
||||
httpTestingController.verify()
|
||||
sessionStorage.clear()
|
||||
localStorage.clear()
|
||||
@@ -434,7 +444,10 @@ describe('DocumentListViewService', () => {
|
||||
count: 3,
|
||||
results: documents.slice(0, 3),
|
||||
})
|
||||
flushSelectionDataRequest(httpTestingController)
|
||||
// two reload()s ran above (setFilterRules, then pageSize), each firing
|
||||
// its own concurrent filter_selection_data request with an identical
|
||||
// (unfiltered) URL; this test doesn't assert on selectionData, so let
|
||||
// afterEach's drain step clean both up rather than disambiguating here.
|
||||
expect(documentListViewService.hasNext(documents[0].id)).toBeTruthy()
|
||||
expect(documentListViewService.hasPrevious(documents[0].id)).toBeFalsy()
|
||||
documentListViewService.getNext(documents[0].id).subscribe((docId) => {
|
||||
|
||||
@@ -314,6 +314,22 @@ export class DocumentListViewService {
|
||||
}
|
||||
}
|
||||
|
||||
private loadFilterSelectionData(filterRules: FilterRule[]) {
|
||||
this.documentService
|
||||
.getFilterSelectionData(filterRules)
|
||||
.pipe(takeUntil(this.unsubscribeNotifier))
|
||||
.subscribe({
|
||||
next: (selectionData) => {
|
||||
this.selectionData = selectionData
|
||||
this.markChanged()
|
||||
},
|
||||
error: () => {
|
||||
this.selectionData = null
|
||||
this.markChanged()
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
reload(onFinish?, updateQueryParams: boolean = true) {
|
||||
this.cancelPending()
|
||||
this.isReloading = true
|
||||
@@ -323,10 +339,14 @@ export class DocumentListViewService {
|
||||
// Full-text search results are already narrowed by the search backend, so
|
||||
// computing selection data inline there is cheap. A plain (unfiltered or
|
||||
// ORM-filtered) browse can span the entire document set, so its selection
|
||||
// data is fetched separately below instead of blocking the list response.
|
||||
// data is fetched separately -- concurrently with the list itself, rather
|
||||
// than blocking or waiting on it.
|
||||
const isFullTextSearch = isFullTextFilterRule(
|
||||
activeListViewState.filterRules
|
||||
)
|
||||
if (!isFullTextSearch) {
|
||||
this.loadFilterSelectionData(activeListViewState.filterRules)
|
||||
}
|
||||
this.documentService
|
||||
.listFiltered(
|
||||
activeListViewState.currentPage,
|
||||
@@ -342,31 +362,17 @@ export class DocumentListViewService {
|
||||
.pipe(takeUntil(this.unsubscribeNotifier))
|
||||
.subscribe({
|
||||
next: (result) => {
|
||||
const resultWithSelectionData = result as DocumentResults
|
||||
this.initialized = true
|
||||
this.isReloading = false
|
||||
activeListViewState.collectionSize = result.count
|
||||
activeListViewState.documents = result.results
|
||||
this.selectionData = resultWithSelectionData.selection_data ?? null
|
||||
if (isFullTextSearch) {
|
||||
this.selectionData =
|
||||
(result as DocumentResults).selection_data ?? null
|
||||
}
|
||||
this.syncSelectedToCurrentPage()
|
||||
this.markChanged()
|
||||
|
||||
if (!isFullTextSearch) {
|
||||
this.documentService
|
||||
.getFilterSelectionData(activeListViewState.filterRules)
|
||||
.pipe(takeUntil(this.unsubscribeNotifier))
|
||||
.subscribe({
|
||||
next: (selectionData) => {
|
||||
this.selectionData = selectionData
|
||||
this.markChanged()
|
||||
},
|
||||
error: () => {
|
||||
this.selectionData = null
|
||||
this.markChanged()
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
if (updateQueryParams && !this._activeSavedViewId) {
|
||||
let base = ['/documents']
|
||||
this.router.navigate(base, {
|
||||
@@ -402,6 +408,9 @@ export class DocumentListViewService {
|
||||
// e.g. field was deleted
|
||||
this.sortField = 'created'
|
||||
} else {
|
||||
// cancel the concurrently-fired selection-data request too, so it
|
||||
// can't resolve afterward and clobber this reset with stale data
|
||||
this.cancelPending()
|
||||
this.selectionData = null
|
||||
let errorMessage
|
||||
if (
|
||||
|
||||
@@ -41,6 +41,24 @@ export abstract class AbstractPaperlessService<T extends ObjectWithId> {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Merges a plain params object into an HttpParams instance, skipping
|
||||
* null/undefined values so they aren't serialized as literal "null" /
|
||||
* "undefined" query string entries.
|
||||
*/
|
||||
protected withParams(
|
||||
params,
|
||||
base: HttpParams = new HttpParams()
|
||||
): HttpParams {
|
||||
let httpParams = base
|
||||
for (let key in params) {
|
||||
if (params[key] != null) {
|
||||
httpParams = httpParams.set(key, params[key])
|
||||
}
|
||||
}
|
||||
return httpParams
|
||||
}
|
||||
|
||||
list(
|
||||
page?: number,
|
||||
pageSize?: number,
|
||||
@@ -60,11 +78,7 @@ export abstract class AbstractPaperlessService<T extends ObjectWithId> {
|
||||
if (ordering) {
|
||||
httpParams = httpParams.set('ordering', ordering)
|
||||
}
|
||||
for (let extraParamKey in extraParams) {
|
||||
if (extraParams[extraParamKey] != null) {
|
||||
httpParams = httpParams.set(extraParamKey, extraParams[extraParamKey])
|
||||
}
|
||||
}
|
||||
httpParams = this.withParams(extraParams, httpParams)
|
||||
return this.http
|
||||
.get<Results<T>>(this.getResourceUrl(), {
|
||||
params: httpParams,
|
||||
@@ -113,11 +127,7 @@ export abstract class AbstractPaperlessService<T extends ObjectWithId> {
|
||||
httpParams = httpParams.set('id__in', ids.join(','))
|
||||
httpParams = httpParams.set('ordering', '-id')
|
||||
httpParams = httpParams.set('page_size', 1000)
|
||||
for (let extraParamKey in extraParams) {
|
||||
if (extraParams[extraParamKey] != null) {
|
||||
httpParams = httpParams.set(extraParamKey, extraParams[extraParamKey])
|
||||
}
|
||||
}
|
||||
httpParams = this.withParams(extraParams, httpParams)
|
||||
return this.http
|
||||
.get<Results<T>>(this.getResourceUrl(), {
|
||||
params: httpParams,
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
import { HttpParams } from '@angular/common/http'
|
||||
import { Injectable, inject } from '@angular/core'
|
||||
import { Observable } from 'rxjs'
|
||||
import { map } from 'rxjs/operators'
|
||||
@@ -400,16 +399,9 @@ export class DocumentService extends AbstractPaperlessService<Document> {
|
||||
}
|
||||
|
||||
getFilterSelectionData(filterRules: FilterRule[]): Observable<SelectionData> {
|
||||
let httpParams = new HttpParams()
|
||||
const filterParams = queryParamsFromFilterRules(filterRules)
|
||||
for (let key in filterParams) {
|
||||
if (filterParams[key] != null) {
|
||||
httpParams = httpParams.set(key, filterParams[key])
|
||||
}
|
||||
}
|
||||
return this.http.get<SelectionData>(
|
||||
this.getResourceUrl(null, 'filter_selection_data'),
|
||||
{ params: httpParams }
|
||||
{ params: this.withParams(queryParamsFromFilterRules(filterRules)) }
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
+11
-2
@@ -1034,7 +1034,11 @@ class DocumentViewSet(
|
||||
],
|
||||
}
|
||||
|
||||
def get_queryset(self):
|
||||
def _base_document_queryset(self):
|
||||
# Root documents only, with the annotations that filter_backends rely
|
||||
# on (effective_content for SearchFilter, num_notes for ordering) --
|
||||
# but no select_related/prefetch_related, since those only matter for
|
||||
# serializing documents, not for filtering, ordering, or aggregating.
|
||||
latest_version_content = Subquery(
|
||||
Document.objects.filter(root_document=OuterRef("pk"))
|
||||
.order_by("-id")
|
||||
@@ -1046,6 +1050,11 @@ class DocumentViewSet(
|
||||
.order_by("-created", "-id")
|
||||
.annotate(effective_content=Coalesce(latest_version_content, F("content")))
|
||||
.annotate(num_notes=Count("notes"))
|
||||
)
|
||||
|
||||
def get_queryset(self):
|
||||
return (
|
||||
self._base_document_queryset()
|
||||
.select_related("correspondent", "storage_path", "document_type", "owner")
|
||||
.prefetch_related(
|
||||
Prefetch(
|
||||
@@ -1197,7 +1206,7 @@ class DocumentViewSet(
|
||||
)
|
||||
@action(detail=False, methods=["get"], url_path="filter_selection_data")
|
||||
def filter_selection_data(self, request, *args, **kwargs):
|
||||
queryset = self.filter_queryset(self.get_queryset())
|
||||
queryset = self.filter_queryset(self._base_document_queryset())
|
||||
return Response(self._get_selection_data_for_queryset(queryset))
|
||||
|
||||
def destroy(self, request, *args, **kwargs):
|
||||
|
||||
Reference in New Issue
Block a user