From 7fd88326e2ce9c2915b79517a6c0c61faf4ecf17 Mon Sep 17 00:00:00 2001 From: stumpylog <797416+stumpylog@users.noreply.github.com> Date: Sat, 12 Sep 2026 13:26:30 -0700 Subject: [PATCH] Route SCHEMA_FIELD_MISSING into an error, not an HTTP 400 --- src/documents/search/_query.py | 19 +++++++----- .../tests/search/test_error_routing.py | 29 ++++++++++++------- .../search/test_exists_on_json_fields.py | 26 ++++++++++++----- 3 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/documents/search/_query.py b/src/documents/search/_query.py index 5f51d9591..f28135e5f 100644 --- a/src/documents/search/_query.py +++ b/src/documents/search/_query.py @@ -43,7 +43,8 @@ def _user_facing_emit_message(d: Diagnostic) -> str: 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. + error text in it. SCHEMA_FIELD_MISSING never reaches here: _map_emit_error + re-raises it before calling this function, the same as INTERNAL. """ field = str(d.field) if d.field is not None else None if d.kind is DiagnosticKind.EXISTS_REQUIRES_FAST: @@ -52,8 +53,6 @@ def _user_facing_emit_message(d: Diagnostic) -> str: 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, @@ -69,10 +68,15 @@ def _map_emit_error(e: QueryError) -> SearchQueryError: 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 rather than converted, reaching the generic 500 handler instead - of blaming the query. 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. + of blaming the query. MISCONFIGURED other than EXISTS_REQUIRES_FAST is + treated the same way as INTERNAL: the registry and the index schema + disagree, which only an operator can fix, and the exact same query would + succeed on its own once the index is rebuilt. That makes it a transient + server-side condition, not a permanently bad request, so it is logged as + an error and re-raised rather than converted to a 400: telling the + client their query is invalid would be wrong, it would work once the + index catches up, and a 400 also hides the condition from monitoring + that only watches 5xx rates. EXISTS_REQUIRES_FAST is the one MISCONFIGURED kind that is not a disagreement. whoosh-compat derives it from the registry's own FieldSpec @@ -96,6 +100,7 @@ def _map_emit_error(e: QueryError) -> SearchQueryError: d.kind.name, d.message, ) + raise e return SearchQueryError(_user_facing_emit_message(d)) diff --git a/src/documents/tests/search/test_error_routing.py b/src/documents/tests/search/test_error_routing.py index 223a89b77..cd8822560 100644 --- a/src/documents/tests/search/test_error_routing.py +++ b/src/documents/tests/search/test_error_routing.py @@ -82,7 +82,7 @@ class TestEmitErrorRouting: _map_emit_error(error) assert excinfo.value is error - def test_misconfigured_cause_is_logged_and_becomes_a_400( + def test_misconfigured_cause_is_logged_and_reraised( self, caplog: pytest.LogCaptureFixture, ) -> None: @@ -92,15 +92,21 @@ class TestEmitErrorRouting: WHEN: - _map_emit_error processes it THEN: - - It becomes a SearchQueryError, and exactly one ERROR log - record is emitted naming the field and the diagnostic kind + - Exactly one ERROR log record is emitted naming the field and + the diagnostic kind, and the original QueryError propagates + unchanged: a registry/schema disagreement is transient (the + exact same query succeeds once the index is rebuilt), so it + surfaces as a 500 an operator can see rather than a 400 + telling the client their query is permanently invalid """ kind = DiagnosticKind.SCHEMA_FIELD_MISSING - with caplog.at_level(logging.ERROR, logger="paperless.search"): - error = _map_emit_error( - QueryError(_diagnostic(kind, field=FieldRef("asn"))), - ) - assert isinstance(error, SearchQueryError) + error = QueryError(_diagnostic(kind, field=FieldRef("asn"))) + with ( + caplog.at_level(logging.ERROR, logger="paperless.search"), + pytest.raises(QueryError) as excinfo, + ): + _map_emit_error(error) + assert excinfo.value is error errors = [r for r in caplog.records if r.levelno == logging.ERROR] assert len(errors) == 1 assert "asn" in errors[0].getMessage() @@ -144,7 +150,6 @@ class TestEmitErrorRouting: DiagnosticKind.TEXT_RANGE, DiagnosticKind.PATTERN_TOO_COMPLEX, DiagnosticKind.EXISTS_REQUIRES_FAST, - DiagnosticKind.SCHEMA_FIELD_MISSING, ], ) def test_user_facing_message_never_echoes_library_prose( @@ -154,7 +159,10 @@ class TestEmitErrorRouting: """ GIVEN: - A QueryError carrying whoosh-compat's own developer-facing - message text + message text (SCHEMA_FIELD_MISSING excluded: it is now + re-raised rather than converted, so it never produces a + user-facing message at all, see + test_misconfigured_cause_is_logged_and_reraised) WHEN: - _map_emit_error processes it THEN: @@ -170,7 +178,6 @@ class TestEmitErrorRouting: DiagnosticKind.TEXT_RANGE, DiagnosticKind.PATTERN_TOO_COMPLEX, DiagnosticKind.EXISTS_REQUIRES_FAST, - DiagnosticKind.SCHEMA_FIELD_MISSING, ], ) def test_user_facing_message_names_the_field( diff --git a/src/documents/tests/search/test_exists_on_json_fields.py b/src/documents/tests/search/test_exists_on_json_fields.py index 33b86cc2b..875087439 100644 --- a/src/documents/tests/search/test_exists_on_json_fields.py +++ b/src/documents/tests/search/test_exists_on_json_fields.py @@ -9,7 +9,10 @@ action can clear the condition. Any authenticated user could otherwise emit ERROR lines in a loop by repeating ``notes:*``. SCHEMA_FIELD_MISSING, the other MISCONFIGURED kind, does compare the registry -against the live schema, so it stays an ERROR. +against the live schema, so it stays an ERROR log. But it is not a 400 +either: the exact same query would succeed once the index is rebuilt, so it +is a transient server-side condition, not a permanently bad request, and is +re-raised the same way an INTERNAL cause is. """ from __future__ import annotations @@ -81,7 +84,7 @@ class TestJsonExistsIsUserError: class TestGenuineMisconfigurationStillLogs: - def test_schema_field_missing_is_an_error_log( + def test_schema_field_missing_is_an_error_log_and_reraised( self, caplog: pytest.LogCaptureFixture, ) -> None: @@ -92,9 +95,13 @@ class TestGenuineMisconfigurationStillLogs: WHEN: - _map_emit_error processes it THEN: - - It becomes a SearchQueryError and logs exactly one ERROR - record naming the diagnostic kind, since this is a real - mismatch an operator can fix and keeps the alert + - It logs exactly one ERROR record naming the diagnostic kind + (a real mismatch an operator can fix, so it keeps the + alert), and the original QueryError propagates unchanged + rather than becoming a SearchQueryError: the exact same + query would succeed once the index is rebuilt, so this is a + transient server-side condition, not a permanently bad + request, and surfaces as a 500 rather than a 400 """ kind = DiagnosticKind.SCHEMA_FIELD_MISSING error = QueryError( @@ -106,9 +113,12 @@ class TestGenuineMisconfigurationStillLogs: field_kind=FieldKind.U64, ), ) - with caplog.at_level(logging.ERROR, logger="paperless.search"): - mapped = _map_emit_error(error) - assert isinstance(mapped, SearchQueryError) + with ( + caplog.at_level(logging.ERROR, logger="paperless.search"), + pytest.raises(QueryError) as excinfo, + ): + _map_emit_error(error) + assert excinfo.value is error records = [r for r in caplog.records if r.levelno == logging.ERROR] assert len(records) == 1 assert kind.name in records[0].getMessage()