mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-08-26 04:33:20 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
stumpylog
co-authored by
Claude Opus 5
Claude Opus 5
parent
48b36d4f16
commit
63de40c54e
@@ -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(
|
||||
|
||||
@@ -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)
|
||||
@@ -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'."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user