mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-08-19 09:13:24 +00:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMsn6DgzbvSqh1pwy66VVF
This commit is contained in:
co-authored by
Claude Fable 5
parent
a418487f3c
commit
1cb07030b0
@@ -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",
|
||||
]
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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:
|
||||
"""
|
||||
|
||||
+31
-17
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user