From 63de40c54eb3c613667bdb16044d6c0f80fee6c9 Mon Sep 17 00:00:00 2001 From: stumpylog <797416+stumpylog@users.noreply.github.com> Date: Thu, 20 Aug 2026 06:52:21 -0700 Subject: [PATCH] fix(search): route emit diagnostics by Cause, own the user-facing wording The except QueryError arm converted every kind to a 400 on the strength of a comment asserting the INTERNAL kinds could not occur. SCHEMA_FIELD_MISSING fires on registry/schema drift, which deriving both from PUBLIC_FIELDS newly makes possible, so a defect in our own wiring was reported to the user as a bad query and never reached monitoring. Diagnostics now route on Cause: INVALID_INPUT/UNSUPPORTED are a 400, MISCONFIGURED is logged at error level naming the field and then a 400 (the registry and the schema disagree, which only an operator can fix, but a request is still waiting and the query cannot run either way), and INTERNAL is re-raised rather than converted. Messages, parse-time as well as emit-time, are built from the Diagnostic's structured fields; d.message is documented as unstable developer output and PATTERN_TOO_COMPLEX embedded raw backend error text in the 400 body. Co-Authored-By: Claude Opus 5 --- src/documents/search/_query.py | 88 +++++-- .../tests/search/test_error_routing.py | 234 ++++++++++++++++++ src/documents/tests/search/test_query.py | 40 +-- 3 files changed, 308 insertions(+), 54 deletions(-) create mode 100644 src/documents/tests/search/test_error_routing.py diff --git a/src/documents/search/_query.py b/src/documents/search/_query.py index 65b358a92..8a3c67b70 100644 --- a/src/documents/search/_query.py +++ b/src/documents/search/_query.py @@ -9,6 +9,7 @@ import tantivy import whoosh_compat as wc from django.conf import settings from whoosh_compat.emitters.tantivy_ import emit as tantivy_emit +from whoosh_compat.errors import Cause from whoosh_compat.errors import Diagnostic from whoosh_compat.errors import DiagnosticKind from whoosh_compat.errors import QueryError @@ -114,16 +115,47 @@ def _rewrite_bare_json_field_prefixes(raw_query: str) -> str: def _user_facing_emit_message(d: Diagnostic) -> str: """A user-safe message for an emit-time QueryError's Diagnostic. - whoosh-compat's own Diagnostic.message is developer/log output with no - stability guarantee (branch on kind, never parse the message). Only - EXISTS_REQUIRES_FAST needs a distinct user-facing rewrite: its message - advises marking the field fast=True, a host configuration action the - user can't act on; the user just needs to know the search form is - unsupported here. + Built from the Diagnostic's structured fields (kind, field), never from + d.message: whoosh-compat documents that as developer/log output with no + stability guarantee, and PATTERN_TOO_COMPLEX embeds the raw backend + error text in it. """ + field = str(d.field) if d.field is not None else None if d.kind is DiagnosticKind.EXISTS_REQUIRES_FAST: - return "existence searches (field:*) are not supported for this field" - return d.message + return f"Existence searches (field:*) are not supported for field {field!r}." + if d.kind is DiagnosticKind.TEXT_RANGE: + return f"Range searches are not supported for field {field!r}." + if d.kind is DiagnosticKind.PATTERN_TOO_COMPLEX: + return f"The wildcard pattern for field {field!r} is too complex." + if d.kind is DiagnosticKind.SCHEMA_FIELD_MISSING: + return f"Field {field!r} is not available in the search index." + logger.warning("Unmapped emit diagnostic %s: %s", d.kind, d.message) + return "The search query could not be executed." + + +def _map_emit_error(e: QueryError) -> SearchQueryError: + """Route an emit-time QueryError by its Diagnostic's Cause. + + INVALID_INPUT/UNSUPPORTED are user-input errors, exactly like a parse + diagnostic, and map to a 400. INTERNAL means a defect in whoosh-compat + or in our own AST handling, never the user's query, so the QueryError is + re-raised to surface the same way views.py already lets QueryParserError + surface. MISCONFIGURED is deliberately both: the registry and the index + schema disagree, which only an operator can fix, so it is logged as an + error, but a request is still waiting and the query cannot run either + way, so it also returns a 400. + """ + d = e.diagnostic + if d.cause is Cause.INTERNAL: + raise e + if d.cause is Cause.MISCONFIGURED: + logger.error( + "Search index misconfiguration for field %s (%s): %s", + d.field, + d.kind.name, + d.message, + ) + return SearchQueryError(_user_facing_emit_message(d)) def _has_cjk(text: str) -> bool: @@ -318,8 +350,10 @@ def parse_user_query( and raise — the view returns HTTP 400 with every offending field listed, not just the first. 3. emit() turns the AST into a tantivy.Query directly (no string - round-trip). QueryError (a construct that parses but can't execute - against tantivy, e.g. a text-field range) also maps to a 400. + round-trip). A QueryError is routed by its Diagnostic's Cause + (_map_emit_error): a construct that parses but can't execute against + tantivy (e.g. a text-field range) is a 400, a registry/schema + mismatch is logged and a 400, and an INTERNAL defect is re-raised. 4. Optional fuzzy blend (ADVANCED_FUZZY_SEARCH_THRESHOLD) builds a plain word string from the parsed AST's free-text tokens (whoosh_compat.free_text_tokens) and feeds THAT to @@ -346,12 +380,7 @@ def parse_user_query( try: exact = tantivy_emit(result.ast, index=index, registry=registry) except QueryError as e: - # emit()'s documented host contract: every reachable-from-query-text - # kind here (TEXT_RANGE, PATTERN_TOO_COMPLEX, EXISTS_REQUIRES_FAST) - # is a user-input error, exactly like a parse diagnostic, and maps - # to a 400. The AST_*/BACKEND_REJECTED backstop kinds cannot occur - # here: whoosh-compat only ever hands us the AST it parsed itself. - raise SearchQueryError(_user_facing_emit_message(e.diagnostic)) from e + raise _map_emit_error(e) from e cjk_query = ( _build_cjk_query(index, raw_query, _CJK_ALL_FIELDS) @@ -377,6 +406,18 @@ def parse_user_query( return _any_of(clauses) +# The three whoosh-compat kinds for a wildcard on a field that cannot +# carry one. d.field_kind supplies the discriminator, so naming the field's +# type needs no second trip through the registry. +_PATTERN_ON_KINDS: Final = frozenset( + { + DiagnosticKind.PATTERN_ON_NUMERIC, + DiagnosticKind.PATTERN_ON_BOOLEAN_EXISTS, + DiagnosticKind.PATTERN_ON_SUBPATH, + }, +) + + def _diagnostics_to_error(diagnostics: tuple[Diagnostic, ...]) -> SearchQueryError: errors = [_single_diagnostic_to_error(d) for d in diagnostics] return errors[0] if len(errors) == 1 else MultipleSearchQueryErrors(errors) @@ -390,11 +431,16 @@ def _single_diagnostic_to_error(d: Diagnostic) -> SearchQueryError: return InvalidDateQuery(field_name, d.raw_value) if d.kind is DiagnosticKind.BAD_NUMBER: return InvalidNumberQuery(field_name, d.raw_value) - # TOO_DEEP and UNSUPPORTED_PATTERN (e.g. a wildcard on asn/page_count/ - # num_notes, or on a custom_fields.*/notes.* subpath) fall through to - # the generic message; consider whether either warrants its own typed - # subclass if callers ever need to distinguish them programmatically. - return SearchQueryError(d.message) + if d.kind is DiagnosticKind.TOO_DEEP: + return SearchQueryError("The search query is nested too deeply.") + if d.kind in _PATTERN_ON_KINDS: + kind_label = f" ({d.field_kind.name.lower()})" if d.field_kind else "" + return SearchQueryError( + f"Wildcard patterns are not supported for field " + f"{field_name!r}{kind_label}.", + ) + logger.warning("Unmapped parse diagnostic %s: %s", d.kind, d.message) + return SearchQueryError("The search query could not be executed.") def parse_simple_query( diff --git a/src/documents/tests/search/test_error_routing.py b/src/documents/tests/search/test_error_routing.py new file mode 100644 index 000000000..7864a089c --- /dev/null +++ b/src/documents/tests/search/test_error_routing.py @@ -0,0 +1,234 @@ +"""Diagnostics route by Cause, and user-facing messages are host-owned. + +whoosh-compat documents ``Diagnostic.message`` as developer output with no +stability guarantee, so it must never reach an HTTP response body. +""" + +from __future__ import annotations + +import logging +from datetime import UTC + +import pytest +import tantivy +from whoosh_compat.errors import Diagnostic +from whoosh_compat.errors import DiagnosticKind +from whoosh_compat.errors import QueryError +from whoosh_compat.errors import cause_for +from whoosh_compat.fields import FieldKind +from whoosh_compat.fields import FieldRef + +from documents.search._errors import SearchQueryError +from documents.search._query import _map_emit_error +from documents.search._query import _single_diagnostic_to_error +from documents.search._query import parse_user_query +from documents.search._schema import build_schema +from documents.search._tokenizer import register_tokenizers + +pytestmark = pytest.mark.search + +_LIBRARY_PROSE = "INTERNAL LIBRARY WORDING WITH raw tantivy detail" + + +@pytest.fixture(scope="module") +def query_index() -> tantivy.Index: + """An in-memory, unstemmed index; these tests only parse, never index.""" + idx = tantivy.Index(build_schema(), path=None) + register_tokenizers(idx, "") + return idx + + +def _diagnostic( + kind: DiagnosticKind, + *, + field: FieldRef | None = FieldRef("title"), + field_kind: FieldKind | None = FieldKind.TEXT, +) -> Diagnostic: + """A Diagnostic shaped like the emitter's, with the library's own + kind -> cause mapping rather than a hand-picked cause.""" + return Diagnostic( + kind=kind, + cause=cause_for(kind), + message=_LIBRARY_PROSE, + field=field, + field_kind=field_kind, + ) + + +class TestEmitErrorRouting: + """Every Cause gets a distinguishable treatment, not just "a 400".""" + + @pytest.mark.parametrize( + "kind", + [ + DiagnosticKind.BACKEND_REJECTED, + DiagnosticKind.AST_INVALID_SHAPE, + DiagnosticKind.AST_UNKNOWN_FIELD, + ], + ) + def test_internal_cause_is_not_converted(self, kind: DiagnosticKind) -> None: + """A library defect must surface as a 500 monitoring can see, not a + 400 blaming the user.""" + error = QueryError(_diagnostic(kind)) + with pytest.raises(QueryError) as excinfo: + _map_emit_error(error) + assert excinfo.value is error + + @pytest.mark.parametrize( + "kind", + [ + DiagnosticKind.SCHEMA_FIELD_MISSING, + DiagnosticKind.EXISTS_REQUIRES_FAST, + ], + ) + def test_misconfigured_cause_is_logged_and_becomes_a_400( + self, + kind: DiagnosticKind, + caplog: pytest.LogCaptureFixture, + ) -> None: + with caplog.at_level(logging.ERROR, logger="paperless.search"): + error = _map_emit_error( + QueryError(_diagnostic(kind, field=FieldRef("asn"))), + ) + assert isinstance(error, SearchQueryError) + errors = [r for r in caplog.records if r.levelno == logging.ERROR] + assert len(errors) == 1 + assert "asn" in errors[0].getMessage() + assert kind.name in errors[0].getMessage() + + @pytest.mark.parametrize( + "kind", + [ + DiagnosticKind.TEXT_RANGE, + DiagnosticKind.PATTERN_TOO_COMPLEX, + ], + ) + def test_unsupported_cause_is_a_400_with_no_operator_log( + self, + kind: DiagnosticKind, + caplog: pytest.LogCaptureFixture, + ) -> None: + """A query tantivy cannot run is the user's to fix; it must not page + an operator the way a registry/schema mismatch does.""" + with caplog.at_level(logging.WARNING, logger="paperless.search"): + error = _map_emit_error(QueryError(_diagnostic(kind))) + assert isinstance(error, SearchQueryError) + assert caplog.records == [] + + @pytest.mark.parametrize( + "kind", + [ + DiagnosticKind.TEXT_RANGE, + DiagnosticKind.PATTERN_TOO_COMPLEX, + DiagnosticKind.EXISTS_REQUIRES_FAST, + DiagnosticKind.SCHEMA_FIELD_MISSING, + ], + ) + def test_user_facing_message_never_echoes_library_prose( + self, + kind: DiagnosticKind, + ) -> None: + error = _map_emit_error(QueryError(_diagnostic(kind))) + assert _LIBRARY_PROSE not in str(error) + + @pytest.mark.parametrize( + "kind", + [ + DiagnosticKind.TEXT_RANGE, + DiagnosticKind.PATTERN_TOO_COMPLEX, + DiagnosticKind.EXISTS_REQUIRES_FAST, + DiagnosticKind.SCHEMA_FIELD_MISSING, + ], + ) + def test_user_facing_message_names_the_field( + self, + kind: DiagnosticKind, + ) -> None: + """FieldRef.__str__ yields the canonical dotted name, including a + JSON subpath, so every user-reachable emit kind can name it.""" + diagnostic = _diagnostic( + kind, + field=FieldRef("custom_fields", "value"), + field_kind=FieldKind.JSON, + ) + error = _map_emit_error(QueryError(diagnostic)) + assert "custom_fields.value" in str(error) + + +class TestParseDiagnosticMessages: + """Parse-time diagnostics are host-worded too, off field_kind.""" + + def test_too_deep_is_a_400_without_library_prose(self) -> None: + error = _single_diagnostic_to_error( + _diagnostic(DiagnosticKind.TOO_DEEP, field=None, field_kind=None), + ) + assert isinstance(error, SearchQueryError) + assert _LIBRARY_PROSE not in str(error) + + @pytest.mark.parametrize( + ("kind", "field_kind"), + [ + (DiagnosticKind.PATTERN_ON_NUMERIC, FieldKind.U64), + (DiagnosticKind.PATTERN_ON_BOOLEAN_EXISTS, FieldKind.BOOLEAN_EXISTS), + (DiagnosticKind.PATTERN_ON_SUBPATH, FieldKind.JSON), + ], + ) + def test_pattern_on_kinds_name_the_field_and_its_kind( + self, + kind: DiagnosticKind, + field_kind: FieldKind, + ) -> None: + error = _single_diagnostic_to_error( + _diagnostic(kind, field=FieldRef("asn"), field_kind=field_kind), + ) + message = str(error) + assert _LIBRARY_PROSE not in message + assert "asn" in message + assert field_kind.name.lower() in message + + +class TestRealQueriesRouteCorrectly: + """The routing table against diagnostics emit() really produces.""" + + def test_text_range_is_a_400_naming_the_field( + self, + query_index: tantivy.Index, + ) -> None: + with pytest.raises(SearchQueryError) as excinfo: + parse_user_query(query_index, "title:[a to b]", UTC) + assert "title" in str(excinfo.value) + + def test_exists_on_a_non_fast_json_subpath_logs_and_400s( + self, + query_index: tantivy.Index, + caplog: pytest.LogCaptureFixture, + ) -> None: + with caplog.at_level(logging.ERROR, logger="paperless.search"): + with pytest.raises(SearchQueryError) as excinfo: + parse_user_query(query_index, "notes.user:*", UTC) + assert "notes.user" in str(excinfo.value) + assert any(r.levelno == logging.ERROR for r in caplog.records) + + def test_wildcard_on_a_numeric_field_is_a_400_naming_the_field( + self, + query_index: tantivy.Index, + ) -> None: + with pytest.raises(SearchQueryError) as excinfo: + parse_user_query(query_index, "asn:12*", UTC) + assert "asn" in str(excinfo.value) + + def test_internal_diagnostic_escapes_as_a_query_error( + self, + query_index: tantivy.Index, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The one case with no query text that reaches it: emit() reporting + a defect in itself must not be converted to a user-facing 400.""" + import documents.search._query as query_mod + + def raise_internal(*args: object, **kwargs: object) -> None: + raise QueryError(_diagnostic(DiagnosticKind.BACKEND_REJECTED)) + + monkeypatch.setattr(query_mod, "tantivy_emit", raise_internal) + with pytest.raises(QueryError): + parse_user_query(query_index, "invoice", UTC) diff --git a/src/documents/tests/search/test_query.py b/src/documents/tests/search/test_query.py index f0eba61f8..f3975bbe9 100644 --- a/src/documents/tests/search/test_query.py +++ b/src/documents/tests/search/test_query.py @@ -313,46 +313,20 @@ class TestSearchQueryErrors: class TestEmitErrorContract: - """A diagnostics list, or a QueryError from emit(), are both user-input - errors and must surface as SearchQueryError (HTTP 400).""" + """A QueryError from emit() surfaces as a SearchQueryError (HTTP 400). - def test_query_emit_error_maps_to_search_query_error( - self, - query_index: tantivy.Index, - monkeypatch: pytest.MonkeyPatch, - ) -> None: - from whoosh_compat.errors import Cause - from whoosh_compat.errors import Diagnostic - from whoosh_compat.errors import DiagnosticKind - from whoosh_compat.errors import QueryError - - import documents.search._query as query_mod - - def raise_emit_error(*args: object, **kwargs: object) -> None: - raise QueryError( - Diagnostic( - kind=DiagnosticKind.TEXT_RANGE, - cause=Cause.UNSUPPORTED, - message="synthetic emit failure", - ), - ) - - monkeypatch.setattr(query_mod, "tantivy_emit", raise_emit_error) - with pytest.raises(SearchQueryError): - parse_user_query(query_index, "invoice", UTC) + The Cause-based routing table itself is covered in test_error_routing.py. + """ def test_exists_requires_fast_gets_the_user_facing_rewrite( self, query_index: tantivy.Index, ) -> None: - # The only emit-time diagnostic kind paperless rewrites itself - # (_user_facing_emit_message): whoosh-compat's own message advises - # a host-side fast=True config change, which the user can't act - # on, so this checks OUR rewrite, not whoosh-compat's wording - # (that's whoosh-compat's own tests/emitter/test_kind_matrix.py's - # job now). + # whoosh-compat's own message advises a host-side fast=True config + # change the user can't act on, so this checks OUR wording, not + # whoosh-compat's (that's its own test suite's job now). with pytest.raises(SearchQueryError) as exc_info: parse_user_query(query_index, "notes.user:*", UTC) assert str(exc_info.value) == ( - "existence searches (field:*) are not supported for this field" + "Existence searches (field:*) are not supported for field 'notes.user'." )