From 1cb07030b09593be0db667ca23a362082b6267bd Mon Sep 17 00:00:00 2001 From: Trenton Holmes <797416+stumpylog@users.noreply.github.com> Date: Tue, 18 Aug 2026 10:15:25 -0700 Subject: [PATCH] fix(search): complete the query error surface across every endpoint Four pieces of the same surface: whoosh-compat's emit() documents a two-part host contract: both a parse diagnostic and the QueryEmitError/UnsupportedQueryError pair are user-input errors. Only the latter half was caught; QueryEmitError now maps to SearchQueryError too. Messages pass through a cleanup that strips the library's DIVERGENCES.md references and replaces the fast=True host-configuration advice with user language, so no library-internal vocabulary reaches a searching user. The bulk selection paths (bulk edit, the legacy bulk endpoint, bulk download) reached the backend with no SearchQueryError handler, so a bad date or number in a selection filter raised straight to a DRF 500. They now share the search list endpoint's exact mapping (a new search_query_error_messages helper flattens MultipleSearchQueryErrors in one place), returning the same 400 body for the same bad query. QueryParserError means a whoosh-compat parser bug, not user-fixable input, per its own contract; the list endpoint's blanket handler was converting it to a generic 400. It now re-raises and surfaces as a 500 that monitoring can see. All behavior is pinned test-first: bulk edit and bulk download API tests assert 400s naming the bad value (previously unhandled exceptions), a unit test pins the QueryEmitError mapping, three parametrized checks assert no internal vocabulary leaks for the unsupported query shapes, and a mocked parser-bug test asserts the 500. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WMsn6DgzbvSqh1pwy66VVF --- src/documents/search/__init__.py | 2 + src/documents/search/_query.py | 36 ++++++++++++- src/documents/tests/search/test_query.py | 53 +++++++++++++++++++ src/documents/tests/test_api_bulk_download.py | 18 +++++++ src/documents/tests/test_api_bulk_edit.py | 19 +++++++ src/documents/tests/test_api_search.py | 23 ++++++++ src/documents/views.py | 48 +++++++++++------ 7 files changed, 180 insertions(+), 19 deletions(-) diff --git a/src/documents/search/__init__.py b/src/documents/search/__init__.py index d7a4db786..fa5512cbd 100644 --- a/src/documents/search/__init__.py +++ b/src/documents/search/__init__.py @@ -10,6 +10,7 @@ from documents.search._query import InvalidDateQuery from documents.search._query import InvalidNumberQuery from documents.search._query import MultipleSearchQueryErrors from documents.search._query import SearchQueryError +from documents.search._query import search_query_error_messages from documents.search._schema import needs_rebuild from documents.search._schema import wipe_index @@ -27,5 +28,6 @@ __all__ = [ "get_backend", "needs_rebuild", "reset_backend", + "search_query_error_messages", "wipe_index", ] diff --git a/src/documents/search/_query.py b/src/documents/search/_query.py index 37137ce91..b5acfc37c 100644 --- a/src/documents/search/_query.py +++ b/src/documents/search/_query.py @@ -11,6 +11,7 @@ from django.conf import settings from whoosh_compat.emitters.tantivy_ import emit as tantivy_emit from whoosh_compat.errors import Diagnostic from whoosh_compat.errors import DiagnosticKind +from whoosh_compat.errors import QueryEmitError from whoosh_compat.errors import UnsupportedQueryError from documents.search._fields import PUBLIC_FIELDS @@ -61,6 +62,18 @@ class MultipleSearchQueryErrors(SearchQueryError): super().__init__("; ".join(str(e) for e in self.errors)) +def search_query_error_messages(e: SearchQueryError) -> list[str]: + """The user-facing message list for a SearchQueryError. + + Every offending value's message, not just the first, so the user can + fix them all in one round-trip. Shared by every view that maps + SearchQueryError to an HTTP 400. + """ + if isinstance(e, MultipleSearchQueryErrors): + return [str(sub) for sub in e.errors] + return [str(e)] + + logger = logging.getLogger("paperless.search") # Maximum seconds any single regex substitution may run. @@ -153,6 +166,23 @@ def _rewrite_bare_json_field_prefixes(raw_query: str) -> str: return raw_query +# whoosh-compat's emit() error messages are written for the HOST: they +# cite the library's own divergence ledger and give registry-configuration +# advice. Neither belongs in a message shown to a searching user. +_DIVERGENCE_REF_RE: Final = regex.compile(r"\s*\(DIVERGENCES\.md entry \d+\)") + + +def _user_facing_emit_message(exc: Exception) -> str: + """A user-safe message for a QueryEmitError/UnsupportedQueryError.""" + message = _DIVERGENCE_REF_RE.sub("", str(exc)) + if "fast=True" in message: + # The exists-check message advises marking the field fast=True, a + # host configuration action; the user just needs to know the + # search form is unsupported here. + return "existence searches (field:*) are not supported for this field" + return message + + def _has_cjk(text: str) -> bool: """Return True if text contains any CJK characters.""" return bool(_CJK_RE.search(text)) @@ -404,8 +434,10 @@ def parse_user_query( try: exact = tantivy_emit(result.ast, index=index, registry=registry) - except UnsupportedQueryError as e: - raise SearchQueryError(str(e)) from e + except (QueryEmitError, UnsupportedQueryError) as e: + # emit()'s documented host contract: BOTH of these are user-input + # errors, exactly like a parse diagnostic, and both map to a 400. + raise SearchQueryError(_user_facing_emit_message(e)) from e cjk_query = ( _build_cjk_query(index, raw_query, _CJK_ALL_FIELDS) diff --git a/src/documents/tests/search/test_query.py b/src/documents/tests/search/test_query.py index 0060456ef..36a5f46ba 100644 --- a/src/documents/tests/search/test_query.py +++ b/src/documents/tests/search/test_query.py @@ -11,6 +11,7 @@ import time_machine from documents.search._query import InvalidDateQuery from documents.search._query import InvalidNumberQuery from documents.search._query import MultipleSearchQueryErrors +from documents.search._query import SearchQueryError from documents.search._query import build_permission_filter from documents.search._query import parse_simple_text_highlight_query from documents.search._query import parse_user_query @@ -333,3 +334,55 @@ class TestSearchQueryErrors: assert err.errors == tuple(sub_errors) assert "created" in str(err) assert "asn" in str(err) + + +class TestEmitErrorContract: + """whoosh-compat's emit() documents a two-part host contract: BOTH a + non-empty diagnostics list AND the QueryEmitError/UnsupportedQueryError + pair raised by emit() itself are user-input errors. Every one must + surface as SearchQueryError (HTTP 400), with library-internal + vocabulary (DIVERGENCES.md references, fast=True host advice) kept out + of the user-facing message.""" + + @pytest.fixture + def query_index(self) -> tantivy.Index: + schema = build_schema() + idx = tantivy.Index(schema, path=None) + register_tokenizers(idx, "") + return idx + + def test_query_emit_error_maps_to_search_query_error( + self, + query_index: tantivy.Index, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + from whoosh_compat.errors import QueryEmitError + + import documents.search._query as query_mod + + def raise_emit_error(*args: object, **kwargs: object) -> None: + raise QueryEmitError("synthetic emit failure") + + monkeypatch.setattr(query_mod, "tantivy_emit", raise_emit_error) + with pytest.raises(SearchQueryError): + parse_user_query(query_index, "invoice", UTC) + + @pytest.mark.parametrize( + ("query", "leaked_fragment"), + [ + pytest.param("title:[a TO b]", "DIVERGENCES", id="text-range-doc-ref"), + pytest.param("notes.note:wild*", "DIVERGENCES", id="json-wildcard-doc-ref"), + pytest.param("notes.user:*", "fast=True", id="exists-host-advice"), + ], + ) + def test_unsupported_messages_carry_no_internal_vocabulary( + self, + query_index: tantivy.Index, + query: str, + leaked_fragment: str, + ) -> None: + with pytest.raises(SearchQueryError) as exc_info: + parse_user_query(query_index, query, UTC) + assert leaked_fragment not in str(exc_info.value) + # The message must still say something useful, not be blanked. + assert str(exc_info.value).strip() diff --git a/src/documents/tests/test_api_bulk_download.py b/src/documents/tests/test_api_bulk_download.py index eae03a3ed..9a8531244 100644 --- a/src/documents/tests/test_api_bulk_download.py +++ b/src/documents/tests/test_api_bulk_download.py @@ -339,3 +339,21 @@ class TestBulkDownload(DirectoriesMixin, SampleDirMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) self.assertEqual(response.content, b"Insufficient permissions") + + def test_bad_search_query_returns_400(self) -> None: + response = self.client.post( + self.ENDPOINT, + json.dumps( + { + "all": True, + "filters": {"query": "added:notadate"}, + "content": "originals", + }, + ), + content_type="application/json", + ) + + # A user-fixable query error must surface as a 400 naming the bad + # value, exactly like the search list endpoint, never a 500. + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn(b"notadate", response.content) diff --git a/src/documents/tests/test_api_bulk_edit.py b/src/documents/tests/test_api_bulk_edit.py index fd202a04a..3387ee9d7 100644 --- a/src/documents/tests/test_api_bulk_edit.py +++ b/src/documents/tests/test_api_bulk_edit.py @@ -1976,3 +1976,22 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(LogEntry.objects.filter(object_pk=self.doc1.id).count(), 2) + + def test_api_bulk_edit_with_bad_search_query_returns_400(self) -> None: + response = self.client.post( + "/api/documents/bulk_edit/", + json.dumps( + { + "all": True, + "filters": {"query": "added:notadate"}, + "method": "set_storage_path", + "parameters": {"storage_path": self.sp1.id}, + }, + ), + content_type="application/json", + ) + + # A user-fixable query error must surface as a 400 naming the bad + # value, exactly like the search list endpoint, never a 500. + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn(b"notadate", response.content) diff --git a/src/documents/tests/test_api_search.py b/src/documents/tests/test_api_search.py index 9d585855c..bc4b6d374 100644 --- a/src/documents/tests/test_api_search.py +++ b/src/documents/tests/test_api_search.py @@ -855,6 +855,29 @@ class TestDocumentSearchApi(DirectoriesMixin, APITestCase): results = response.data["results"] self.assertEqual({r["id"] for r in results}, {1, 2}) + @mock.patch("documents.search._backend.parse_user_query") + def test_search_parser_bug_surfaces_as_500_not_400(self, m) -> None: + """ + GIVEN: + - The query parser itself fails (a whoosh-compat bug, per + QueryParserError's own contract: not user-fixable input) + WHEN: + - Any search request runs + THEN: + - The error surfaces as a 500 monitoring can see, never a 400 + blaming the user for a library defect + """ + from whoosh_compat.errors import QueryParserError + + m.side_effect = QueryParserError("synthetic parser bug") + + self.client.raise_request_exception = False + response = self.client.get("/api/documents/?query=anything") + self.assertEqual( + response.status_code, + status.HTTP_500_INTERNAL_SERVER_ERROR, + ) + @mock.patch("documents.search._backend.TantivyBackend.autocomplete") def test_search_autocomplete_limits(self, m) -> None: """ diff --git a/src/documents/views.py b/src/documents/views.py index 4f4cdbe99..71b2004e8 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -2415,12 +2415,14 @@ class UnifiedSearchViewSet(DocumentViewSet): if not self._is_search_request(): return super().list(request) - from documents.search import MultipleSearchQueryErrors + from whoosh_compat.errors import QueryParserError + from documents.search import SearchHit from documents.search import SearchQueryError from documents.search import TantivyBackend from documents.search import TantivyRelevanceList from documents.search import get_backend + from documents.search import search_query_error_messages def parse_search_params() -> SearchParams: """Extract query string, search mode, and ordering from request.""" @@ -2614,12 +2616,12 @@ class UnifiedSearchViewSet(DocumentViewSet): # User-fixable query error(s) (e.g. unparsable dates/numbers): # surface every offending field's message, not just the first, # so the user can fix them all in one round-trip. - messages = ( - [str(sub) for sub in e.errors] - if isinstance(e, MultipleSearchQueryErrors) - else [str(e)] - ) - raise ValidationError({"query": messages}) from e + raise ValidationError({"query": search_query_error_messages(e)}) from e + except QueryParserError: + # A whoosh-compat parser BUG (its own contract: not user-fixable + # input). Let it surface as a 500 monitoring can see instead of + # a 400 blaming the user for a library defect. + raise except Exception as e: logger.warning(f"An error occurred listing search results: {e!s}") return HttpResponseBadRequest( @@ -2766,17 +2768,29 @@ class DocumentSelectionMixin: backend = get_backend() search_user = None if user.is_superuser else user - if filter_name == "more_like_id": - more_like_doc_id = _get_more_like_id(filters, user) + from documents.search import SearchQueryError + from documents.search import search_query_error_messages - search_ids = backend.more_like_this_ids(more_like_doc_id, user=search_user) - else: - query_str, search_mode = _get_tantivy_query_and_mode(filters) - search_ids = backend.search_ids( - query_str, - user=search_user, - search_mode=search_mode, - ) + try: + if filter_name == "more_like_id": + more_like_doc_id = _get_more_like_id(filters, user) + + search_ids = backend.more_like_this_ids( + more_like_doc_id, + user=search_user, + ) + else: + query_str, search_mode = _get_tantivy_query_and_mode(filters) + search_ids = backend.search_ids( + query_str, + user=search_user, + search_mode=search_mode, + ) + except SearchQueryError as e: + # Same user-fixable-query mapping as the search list endpoint: + # a bad date/number in a bulk selection filter is a 400 naming + # the value, never a 500. + raise ValidationError({"query": search_query_error_messages(e)}) from e return search_ids