mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-10-14 20:17:13 +00:00
Delete these
This commit is contained in:
1 parent
0ab69e9f4d
commit
efea218563
5 files changed
+1641
-1191
No files matched your search
@@ -1,790 +0,0 @@
|
||||
# Search Error Shapes Follow-up Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Replace the generic advanced-search HTTP 400 ("Error listing search results, check logs for more detail.") with three specific, user-fixable `SearchQueryError` subclasses (`UnknownFieldError`, `InvalidFieldValueError`, `MalformedQueryError`).
|
||||
|
||||
**Architecture:** Two detection layers feeding the _existing_ `except SearchQueryError` handler in `UnifiedSearchViewSet.list` (no view change). (1) A **proactive** numeric-value validator inside `translate_query`'s render pass (`_translate.py`) raises `InvalidFieldValueError` before the query reaches Tantivy. (2) A **backstop** wrapper around `index.parse_query` in `parse_user_query` (`_query.py`) maps residual Tantivy `ValueError` message prefixes (`Field does not exist:`, `Syntax Error:`, `Expected a valid integer:`) into the right subclass, so nothing leaks Rust internals or hits the generic 400.
|
||||
|
||||
**Tech Stack:** Python 3.11+, Django, `tantivy` (tantivy-py 0.26.0), `regex`, stdlib `difflib`, pytest + pytest-django. All commands run via `uv run` from `src/`.
|
||||
|
||||
**Spec:** `docs/superpowers/specs/2026-06-15-search-error-shapes-followup-design.md` (read it first).
|
||||
|
||||
**Reference facts (empirically verified 2026-06-15):**
|
||||
|
||||
- Tantivy `index.parse_query` raises `ValueError` with exactly these prefixes: `Field does not exist: '<X>'`, `Syntax Error: <echo>`, `Expected a valid integer: 'ParseIntError { kind: InvalidDigit }'`.
|
||||
- `page_count:>5`, `asn:<10`, `page_count:>=5`, `asn:[1 TO 10]`, `tag_id:1,2,3` parse OK (comparison operators produce correct `RangeQuery`).
|
||||
- `asn:[1 TO]` / `asn:[TO 10]` are a **Syntax Error** (open numeric ranges unsupported; only open _date_ ranges work via sentinels).
|
||||
- `scan()` only tokenizes fields in `KNOWN_FIELDS`; unknown `foobar:hello` stays a `Passthrough` and only fails at `parse_query` -> detected by the backstop, not proactively.
|
||||
- `difflib.get_close_matches("correspondent", pool)` -> `["correspondent"]`; `has_tags`/`http`/`12` -> `[]` (bare message).
|
||||
- `tantivy.Schema` exposes no field-name list, so the drift guard is parse-based.
|
||||
|
||||
## File Structure
|
||||
|
||||
| File | Responsibility | Change |
|
||||
| ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------ | --------------- |
|
||||
| `src/documents/search/_translate.py` | Error classes, field-set constants, proactive numeric validation in `_render`, Tantivy-error mapper + hint helpers | Modify |
|
||||
| `src/documents/search/_query.py` | Backstop wrapper around `index.parse_query` in `parse_user_query` | Modify |
|
||||
| `src/documents/search/__init__.py` | Re-export new error classes for the view import | Modify (verify) |
|
||||
| `src/documents/tests/search/test_error_shapes.py` | All unit tests for the new behavior (dedicated file per subject) | Create |
|
||||
| `src/documents/tests/test_api_search.py` | One view-level 400 integration test (mirrors existing `test_search_added_invalid_date`) | Modify |
|
||||
|
||||
**Test command convention:** single-file runs disable xdist:
|
||||
`cd src && uv run pytest documents/tests/search/test_error_shapes.py --override-ini="addopts=" -v`
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Error classes and field-set constants
|
||||
|
||||
**Files:**
|
||||
|
||||
- Modify: `src/documents/search/_translate.py` (add `import difflib`; add constants and classes after the existing `InvalidDateQuery` class, around line 337)
|
||||
- Test: `src/documents/tests/search/test_error_shapes.py` (create)
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Create `src/documents/tests/search/test_error_shapes.py`:
|
||||
|
||||
```python
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from documents.search._translate import FIELD_ALIASES
|
||||
from documents.search._translate import KNOWN_FIELDS
|
||||
from documents.search._translate import NUMERIC_FIELDS
|
||||
from documents.search._translate import SEARCHABLE_FIELDS
|
||||
from documents.search._translate import InvalidFieldValueError
|
||||
from documents.search._translate import MalformedQueryError
|
||||
from documents.search._translate import SearchQueryError
|
||||
from documents.search._translate import UnknownFieldError
|
||||
|
||||
|
||||
@pytest.mark.search
|
||||
class TestErrorClasses:
|
||||
def test_all_subclass_search_query_error(self):
|
||||
assert issubclass(UnknownFieldError, SearchQueryError)
|
||||
assert issubclass(InvalidFieldValueError, SearchQueryError)
|
||||
assert issubclass(MalformedQueryError, SearchQueryError)
|
||||
|
||||
def test_unknown_field_message_without_suggestion(self):
|
||||
err = UnknownFieldError("has_tags")
|
||||
assert err.field == "has_tags"
|
||||
assert err.suggestion is None
|
||||
assert str(err) == "Unknown search field 'has_tags'."
|
||||
|
||||
def test_unknown_field_message_with_suggestion(self):
|
||||
err = UnknownFieldError("correspondent", suggestion="correspondent")
|
||||
assert err.suggestion == "correspondent"
|
||||
assert str(err) == (
|
||||
"Unknown search field 'corespondent'. Did you mean 'correspondent'?"
|
||||
)
|
||||
|
||||
def test_invalid_field_value_message_with_field(self):
|
||||
err = InvalidFieldValueError("asn", "notanumber")
|
||||
assert err.field == "asn"
|
||||
assert err.value == "notanumber"
|
||||
assert str(err) == "Field 'asn' expects a number, got 'notanumber'."
|
||||
|
||||
def test_invalid_field_value_generic_message(self):
|
||||
err = InvalidFieldValueError()
|
||||
assert "number" in str(err).lower()
|
||||
assert "ParseIntError" not in str(err)
|
||||
|
||||
def test_malformed_query_message(self):
|
||||
err = MalformedQueryError("Unbalanced quote in search query.")
|
||||
assert str(err) == "Unbalanced quote in search query."
|
||||
|
||||
|
||||
@pytest.mark.search
|
||||
class TestFieldSets:
|
||||
def test_numeric_fields_are_known(self):
|
||||
assert NUMERIC_FIELDS <= KNOWN_FIELDS
|
||||
|
||||
def test_searchable_excludes_aliases(self):
|
||||
assert SEARCHABLE_FIELDS == KNOWN_FIELDS - set(FIELD_ALIASES)
|
||||
# aliases must NOT be suggestable
|
||||
for alias in FIELD_ALIASES:
|
||||
assert alias not in SEARCHABLE_FIELDS
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `cd src && uv run pytest documents/tests/search/test_error_shapes.py --override-ini="addopts=" -v`
|
||||
Expected: FAIL with `ImportError: cannot import name 'NUMERIC_FIELDS'` (and the other new names).
|
||||
|
||||
- [ ] **Step 3: Write minimal implementation**
|
||||
|
||||
In `src/documents/search/_translate.py`, add `import difflib` to the stdlib import group (after line 2, before `from dataclasses import dataclass`):
|
||||
|
||||
```python
|
||||
import difflib
|
||||
```
|
||||
|
||||
Then, immediately after the `InvalidDateQuery` class (after line 336), add:
|
||||
|
||||
```python
|
||||
class UnknownFieldError(SearchQueryError):
|
||||
"""Raised when a query scopes on a field name that does not exist."""
|
||||
|
||||
def __init__(self, field: str, suggestion: str | None = None) -> None:
|
||||
self.field = field
|
||||
self.suggestion = suggestion
|
||||
message = f"Unknown search field {field!r}."
|
||||
if suggestion:
|
||||
message += f" Did you mean {suggestion!r}?"
|
||||
super().__init__(message)
|
||||
|
||||
|
||||
class InvalidFieldValueError(SearchQueryError):
|
||||
"""Raised when a numeric field receives a non-numeric value."""
|
||||
|
||||
def __init__(self, field: str | None = None, value: str | None = None) -> None:
|
||||
self.field = field
|
||||
self.value = value
|
||||
if field is not None and value is not None:
|
||||
message = f"Field {field!r} expects a number, got {value!r}."
|
||||
else:
|
||||
message = "A numeric field in the search query received a non-numeric value."
|
||||
super().__init__(message)
|
||||
|
||||
|
||||
class MalformedQueryError(SearchQueryError):
|
||||
"""Raised for structural syntax errors (unbalanced quotes/brackets, etc.)."""
|
||||
```
|
||||
|
||||
Add the field-set constants next to `KNOWN_FIELDS` (after line 92, after the `KNOWN_FIELDS` definition):
|
||||
|
||||
```python
|
||||
# Numeric (unsigned-int) fields. Values must be integers, optionally prefixed by
|
||||
# a comparison operator (>, <, >=, <=). Validated proactively in _render.
|
||||
NUMERIC_FIELDS = frozenset(
|
||||
{
|
||||
"asn",
|
||||
"page_count",
|
||||
"num_notes",
|
||||
"correspondent_id",
|
||||
"document_type_id",
|
||||
"storage_path_id",
|
||||
"tag_id",
|
||||
"owner_id",
|
||||
"viewer_id",
|
||||
},
|
||||
)
|
||||
|
||||
# Canonical user-facing field names for validation and did-you-mean suggestions.
|
||||
# Aliases are excluded so a typo is never "corrected" to a deprecated alias.
|
||||
SEARCHABLE_FIELDS = KNOWN_FIELDS - frozenset(FIELD_ALIASES)
|
||||
```
|
||||
|
||||
Note: `SEARCHABLE_FIELDS` references `FIELD_ALIASES`, which is defined above `KNOWN_FIELDS` (line 54), so this ordering is valid.
|
||||
|
||||
- [ ] **Step 4: Run test to verify it passes**
|
||||
|
||||
Run: `cd src && uv run pytest documents/tests/search/test_error_shapes.py --override-ini="addopts=" -v`
|
||||
Expected: PASS (all `TestErrorClasses` and `TestFieldSets` cases green).
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add src/documents/search/_translate.py src/documents/tests/search/test_error_shapes.py
|
||||
git commit -m "feat(search): add error-shape classes and field-set constants"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Proactive numeric-value validation in `translate_query`
|
||||
|
||||
**Files:**
|
||||
|
||||
- Modify: `src/documents/search/_translate.py` (add `_validate_numeric`; hook into `_render` at lines 484-503)
|
||||
- Test: `src/documents/tests/search/test_error_shapes.py`
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Append to `src/documents/tests/search/test_error_shapes.py`:
|
||||
|
||||
```python
|
||||
from datetime import UTC
|
||||
|
||||
from documents.search._translate import translate_query
|
||||
|
||||
|
||||
@pytest.mark.search
|
||||
class TestProactiveNumericValidation:
|
||||
@pytest.mark.parametrize(
|
||||
("query", "field", "value"),
|
||||
[
|
||||
("asn:notanumber", "asn", "notanumber"),
|
||||
("num_notes:abc", "num_notes", "abc"),
|
||||
("page_count:[foo TO bar]", "page_count", "foo"),
|
||||
("tag_id:1,foo", "tag_id", "foo"),
|
||||
],
|
||||
)
|
||||
def test_non_numeric_value_raises(self, query, field, value):
|
||||
with pytest.raises(InvalidFieldValueError) as exc_info:
|
||||
translate_query(query, UTC)
|
||||
assert exc_info.value.field == field
|
||||
assert exc_info.value.value == value
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"query",
|
||||
[
|
||||
"asn:5",
|
||||
"asn:>5",
|
||||
"asn:<10",
|
||||
"page_count:>=5",
|
||||
"page_count:<=5",
|
||||
"asn:[1 TO 10]",
|
||||
"tag_id:1,2,3",
|
||||
"viewer_id:1,2",
|
||||
"asn:[1 TO]", # open numeric range: passes the integer check here
|
||||
"asn:[TO 10]",
|
||||
],
|
||||
)
|
||||
def test_valid_numeric_values_do_not_raise(self, query):
|
||||
# Should not raise InvalidFieldValueError. (Open numeric ranges still fail
|
||||
# later at parse_query as a Syntax Error -> MalformedQueryError, but NOT
|
||||
# here in the value validator.)
|
||||
translate_query(query, UTC)
|
||||
|
||||
def test_alias_numeric_field_validated(self):
|
||||
# type_id is a numeric alias -> document_type_id; must still validate.
|
||||
with pytest.raises(InvalidFieldValueError):
|
||||
translate_query("type_id:abc", UTC)
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/search/test_error_shapes.py::TestProactiveNumericValidation" --override-ini="addopts=" -v`
|
||||
Expected: FAIL — `test_non_numeric_value_raises` cases do not raise (values currently pass through to Tantivy unvalidated).
|
||||
|
||||
- [ ] **Step 3: Write minimal implementation**
|
||||
|
||||
In `src/documents/search/_translate.py`, add a module-level regex near the other operator patterns (after line 510, near `_SPACED_OP_RE`):
|
||||
|
||||
```python
|
||||
# Leading comparison operator on a numeric value (asn:>5, page_count:<=10).
|
||||
_COMPARISON_PREFIX_RE = regex.compile(r"^(>=|<=|>|<)")
|
||||
```
|
||||
|
||||
Add the validator helper (place it just above `_render`, around line 483):
|
||||
|
||||
```python
|
||||
def _validate_numeric(field: str, value: str) -> None:
|
||||
"""Raise InvalidFieldValueError if a numeric-field value is not an integer.
|
||||
|
||||
Strips a single leading comparison operator (>, <, >=, <=) and surrounding
|
||||
quotes first so comparison queries pass. An empty value (open range bound)
|
||||
is accepted here; an open numeric bracket-range still fails downstream at
|
||||
parse_query as a Syntax Error, surfaced as MalformedQueryError.
|
||||
"""
|
||||
candidate = _COMPARISON_PREFIX_RE.sub("", value.strip().strip("\"'")).strip()
|
||||
if candidate == "":
|
||||
return
|
||||
if not candidate.isdigit():
|
||||
raise InvalidFieldValueError(field, value)
|
||||
```
|
||||
|
||||
Modify `_render` (lines 490-502) to validate numeric fields. Replace the `FieldValueList`, `FieldValue`, and `FieldRange` branches with:
|
||||
|
||||
```python
|
||||
if isinstance(tok, FieldValueList):
|
||||
field = FIELD_ALIASES.get(tok.field, tok.field)
|
||||
if field in NUMERIC_FIELDS:
|
||||
for v in tok.values:
|
||||
_validate_numeric(field, v)
|
||||
return " AND ".join(f"{field}:{v}" for v in tok.values)
|
||||
if isinstance(tok, FieldValue):
|
||||
field = FIELD_ALIASES.get(tok.field, tok.field)
|
||||
if field in DATE_FIELDS:
|
||||
return translate_scalar(field, tok.value, tz)
|
||||
if field in NUMERIC_FIELDS:
|
||||
_validate_numeric(field, tok.value)
|
||||
return f"{field}:{tok.value}"
|
||||
if isinstance(tok, FieldRange):
|
||||
field = FIELD_ALIASES.get(tok.field, tok.field)
|
||||
if field in DATE_FIELDS:
|
||||
return translate_range(field, tok.lo, tok.hi, tz)
|
||||
if field in NUMERIC_FIELDS:
|
||||
_validate_numeric(field, tok.lo)
|
||||
_validate_numeric(field, tok.hi)
|
||||
return f"{field}:{tok.open}{tok.lo} TO {tok.hi}{tok.close}"
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run test to verify it passes**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/search/test_error_shapes.py::TestProactiveNumericValidation" --override-ini="addopts=" -v`
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 5: Run the full translate test file to check for regressions**
|
||||
|
||||
Run: `cd src && uv run pytest documents/tests/search/test_translate.py --override-ini="addopts=" -q`
|
||||
Expected: PASS (no existing translate behavior broken).
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add src/documents/search/_translate.py src/documents/tests/search/test_error_shapes.py
|
||||
git commit -m "feat(search): proactively validate numeric field values"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Tantivy-error mapper and malformed-query hint
|
||||
|
||||
**Files:**
|
||||
|
||||
- Modify: `src/documents/search/_translate.py` (add `_suggest_field`, `_malformed_hint`, `map_tantivy_error`)
|
||||
- Test: `src/documents/tests/search/test_error_shapes.py`
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Append to `src/documents/tests/search/test_error_shapes.py`:
|
||||
|
||||
```python
|
||||
from documents.search._translate import map_tantivy_error
|
||||
|
||||
|
||||
@pytest.mark.search
|
||||
class TestMapTantivyError:
|
||||
def test_unknown_field_maps_with_suggestion(self):
|
||||
exc = ValueError("Field does not exist: 'corespondent'")
|
||||
mapped = map_tantivy_error(exc, "correspondent:foo")
|
||||
assert isinstance(mapped, UnknownFieldError)
|
||||
assert mapped.field == "correspondent"
|
||||
assert mapped.suggestion == "correspondent"
|
||||
|
||||
def test_unknown_field_maps_without_suggestion(self):
|
||||
exc = ValueError("Field does not exist: 'has_tags'")
|
||||
mapped = map_tantivy_error(exc, "has_tags:true")
|
||||
assert isinstance(mapped, UnknownFieldError)
|
||||
assert mapped.field == "has_tags"
|
||||
assert mapped.suggestion is None
|
||||
|
||||
def test_integer_error_maps_to_invalid_value(self):
|
||||
exc = ValueError("Expected a valid integer: 'ParseIntError { kind: InvalidDigit }'")
|
||||
mapped = map_tantivy_error(exc, "asn:x")
|
||||
assert isinstance(mapped, InvalidFieldValueError)
|
||||
assert "ParseIntError" not in str(mapped)
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("raw", "fragment"),
|
||||
[
|
||||
('title:"abc', "quote"),
|
||||
("(invoice OR bill", "parenthes"),
|
||||
("created:[2020 TO 2021", "bracket"),
|
||||
("invoice AND", "AND/OR/NOT"),
|
||||
("OR invoice", "AND/OR/NOT"),
|
||||
],
|
||||
)
|
||||
def test_syntax_error_maps_to_specific_hint(self, raw, fragment):
|
||||
exc = ValueError(f"Syntax Error: {raw}")
|
||||
mapped = map_tantivy_error(exc, raw)
|
||||
assert isinstance(mapped, MalformedQueryError)
|
||||
assert fragment.lower() in str(mapped).lower()
|
||||
assert raw not in str(mapped) # never echo the raw query verbatim
|
||||
|
||||
def test_balanced_open_numeric_range_gets_generic_hint(self):
|
||||
# asn:[1 TO] is a Syntax Error but brackets ARE balanced: must NOT claim
|
||||
# "unbalanced bracket".
|
||||
exc = ValueError("Syntax Error: asn:[1 TO ]")
|
||||
mapped = map_tantivy_error(exc, "asn:[1 TO]")
|
||||
assert isinstance(mapped, MalformedQueryError)
|
||||
assert "unbalanced" not in str(mapped).lower()
|
||||
|
||||
def test_unrecognized_message_returns_none(self):
|
||||
exc = ValueError("Some brand new tantivy error")
|
||||
assert map_tantivy_error(exc, "whatever") is None
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/search/test_error_shapes.py::TestMapTantivyError" --override-ini="addopts=" -v`
|
||||
Expected: FAIL with `ImportError: cannot import name 'map_tantivy_error'`.
|
||||
|
||||
- [ ] **Step 3: Write minimal implementation**
|
||||
|
||||
In `src/documents/search/_translate.py`, add near the other error helpers (after the `MalformedQueryError` class is fine; place all three together at the end of the error-class section):
|
||||
|
||||
```python
|
||||
_FIELD_MISSING_RE = regex.compile(r"^Field does not exist: '(?P<field>[^']*)'")
|
||||
|
||||
_GENERIC_MALFORMED = (
|
||||
"Could not parse the search query. Check for unbalanced quotes, brackets, "
|
||||
"or parentheses, or a misplaced AND/OR/NOT operator."
|
||||
)
|
||||
|
||||
|
||||
def _suggest_field(field: str) -> str | None:
|
||||
"""Return the closest valid field name to ``field``, or None."""
|
||||
matches = difflib.get_close_matches(field, SEARCHABLE_FIELDS, n=1)
|
||||
return matches[0] if matches else None
|
||||
|
||||
|
||||
def _malformed_hint(raw_query: str) -> str:
|
||||
"""Best-effort specific hint for a structural error; generic fallback.
|
||||
|
||||
Only claims a specific cause when it is structurally evident (unbalanced
|
||||
delimiters or a clearly misplaced boolean operator); otherwise returns the
|
||||
generic message so we never assert a wrong-but-confident cause.
|
||||
"""
|
||||
if raw_query.count('"') % 2 != 0:
|
||||
return "Unbalanced quote in the search query."
|
||||
if raw_query.count("(") != raw_query.count(")"):
|
||||
return "Unbalanced parenthesis in the search query."
|
||||
if (
|
||||
raw_query.count("[") != raw_query.count("]")
|
||||
or raw_query.count("{") != raw_query.count("}")
|
||||
):
|
||||
return "Unbalanced bracket in the search query."
|
||||
upper = raw_query.strip().upper()
|
||||
if upper.startswith(("AND ", "OR ")) or upper.endswith((" AND", " OR", " NOT")):
|
||||
return "Misplaced AND/OR/NOT operator in the search query."
|
||||
return _GENERIC_MALFORMED
|
||||
|
||||
|
||||
def map_tantivy_error(exc: ValueError, raw_query: str) -> SearchQueryError | None:
|
||||
"""Map a tantivy parse_query ValueError to a user-safe SearchQueryError.
|
||||
|
||||
Returns None when the message is not a recognised family, so the caller can
|
||||
re-raise the original (preserving today's generic 400 for truly unknown
|
||||
errors rather than inventing a misleading message).
|
||||
"""
|
||||
message = str(exc)
|
||||
m = _FIELD_MISSING_RE.match(message)
|
||||
if m is not None:
|
||||
field = m.group("field")
|
||||
return UnknownFieldError(field, _suggest_field(field))
|
||||
if message.startswith("Expected a valid integer"):
|
||||
return InvalidFieldValueError()
|
||||
if message.startswith("Syntax Error"):
|
||||
return MalformedQueryError(_malformed_hint(raw_query))
|
||||
return None
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run test to verify it passes**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/search/test_error_shapes.py::TestMapTantivyError" --override-ini="addopts=" -v`
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add src/documents/search/_translate.py src/documents/tests/search/test_error_shapes.py
|
||||
git commit -m "feat(search): map tantivy parse errors to user-safe messages"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Backstop wrapper wired into `parse_user_query`
|
||||
|
||||
**Files:**
|
||||
|
||||
- Modify: `src/documents/search/_query.py` (import `map_tantivy_error`; add `_parse_query_friendly`; use it at lines 216-220 and 238-244)
|
||||
- Test: `src/documents/tests/search/test_error_shapes.py`
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Append to `src/documents/tests/search/test_error_shapes.py`:
|
||||
|
||||
```python
|
||||
import tantivy
|
||||
|
||||
from documents.search._query import parse_user_query
|
||||
from documents.search._translate import SearchQueryError as _SQE # noqa: F401
|
||||
|
||||
|
||||
@pytest.mark.search
|
||||
class TestBackstopViaParseUserQuery:
|
||||
"""Uses the module-scope ``index`` fixture from conftest.py."""
|
||||
|
||||
def test_unknown_field_raises_unknown_field_error(self, index: tantivy.Index):
|
||||
with pytest.raises(UnknownFieldError) as exc_info:
|
||||
parse_user_query(index, "foobar:hello", UTC)
|
||||
assert exc_info.value.field == "foobar"
|
||||
|
||||
def test_unknown_field_suggestion(self, index: tantivy.Index):
|
||||
with pytest.raises(UnknownFieldError) as exc_info:
|
||||
parse_user_query(index, "correspondent:bob", UTC)
|
||||
assert exc_info.value.suggestion == "correspondent"
|
||||
|
||||
def test_legacy_backend_field_is_unknown(self, index: tantivy.Index):
|
||||
with pytest.raises(UnknownFieldError) as exc_info:
|
||||
parse_user_query(index, "has_tags:true", UTC)
|
||||
assert exc_info.value.field == "has_tags"
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"query",
|
||||
["(invoice OR bill", "invoice AND", "OR invoice", 'title:"abc'],
|
||||
)
|
||||
def test_syntax_error_raises_malformed(self, index: tantivy.Index, query):
|
||||
with pytest.raises(MalformedQueryError):
|
||||
parse_user_query(index, query, UTC)
|
||||
|
||||
def test_open_numeric_range_is_malformed_not_unbalanced(self, index: tantivy.Index):
|
||||
with pytest.raises(MalformedQueryError) as exc_info:
|
||||
parse_user_query(index, "asn:[1 TO]", UTC)
|
||||
assert "unbalanced" not in str(exc_info.value).lower()
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"query",
|
||||
["page_count:>5", "asn:<10", "page_count:>=5", "asn:[1 TO 10]", "tag_id:1,2,3"],
|
||||
)
|
||||
def test_comparison_and_range_queries_succeed(self, index: tantivy.Index, query):
|
||||
assert isinstance(parse_user_query(index, query, UTC), tantivy.Query)
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"query",
|
||||
["notes.user:alice", "custom_fields.name:invoice"],
|
||||
)
|
||||
def test_dotted_json_subfields_not_flagged(self, index: tantivy.Index, query):
|
||||
assert isinstance(parse_user_query(index, query, UTC), tantivy.Query)
|
||||
|
||||
def test_numeric_mismatch_raises_invalid_value(self, index: tantivy.Index):
|
||||
# Proactive pass fires inside translate_query before parse_query.
|
||||
with pytest.raises(InvalidFieldValueError) as exc_info:
|
||||
parse_user_query(index, "asn:notanumber", UTC)
|
||||
assert exc_info.value.field == "asn"
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/search/test_error_shapes.py::TestBackstopViaParseUserQuery" --override-ini="addopts=" -v`
|
||||
Expected: FAIL — unknown-field/syntax cases currently raise the bare Tantivy `ValueError`, not the new subclasses (the `index.parse_query` calls are unwrapped). The numeric-mismatch and success cases may already pass.
|
||||
|
||||
- [ ] **Step 3: Write minimal implementation**
|
||||
|
||||
In `src/documents/search/_query.py`, add the import alongside the existing translate imports (after line 13):
|
||||
|
||||
```python
|
||||
from documents.search._translate import map_tantivy_error
|
||||
```
|
||||
|
||||
Add a module-level helper (place it just above `parse_user_query`, before line 176):
|
||||
|
||||
```python
|
||||
def _parse_query_friendly(
|
||||
index: tantivy.Index,
|
||||
query_str: str,
|
||||
raw_query: str,
|
||||
default_fields: list[str],
|
||||
**kwargs,
|
||||
) -> tantivy.Query:
|
||||
"""Call index.parse_query, translating Tantivy ValueErrors into user-safe
|
||||
SearchQueryError subclasses. Unrecognised errors are re-raised unchanged."""
|
||||
try:
|
||||
return index.parse_query(query_str, default_fields, **kwargs)
|
||||
except SearchQueryError:
|
||||
raise
|
||||
except ValueError as exc:
|
||||
mapped = map_tantivy_error(exc, raw_query)
|
||||
if mapped is not None:
|
||||
raise mapped from exc
|
||||
raise
|
||||
```
|
||||
|
||||
In `parse_user_query`, replace the exact-query parse (lines 216-220):
|
||||
|
||||
```python
|
||||
exact = _parse_query_friendly(
|
||||
index,
|
||||
query_str,
|
||||
raw_query,
|
||||
DEFAULT_SEARCH_FIELDS,
|
||||
field_boosts=_FIELD_BOOSTS,
|
||||
)
|
||||
```
|
||||
|
||||
and the fuzzy parse (lines 238-244):
|
||||
|
||||
```python
|
||||
fuzzy = _parse_query_friendly(
|
||||
index,
|
||||
query_str,
|
||||
raw_query,
|
||||
DEFAULT_SEARCH_FIELDS,
|
||||
field_boosts=_FIELD_BOOSTS,
|
||||
# (prefix=True, distance=1, transposition_cost_one=True) — edit-distance fuzziness
|
||||
fuzzy_fields={f: (True, 1, True) for f in DEFAULT_SEARCH_FIELDS},
|
||||
)
|
||||
```
|
||||
|
||||
(`SearchQueryError` is already imported in `_query.py` at line 12.)
|
||||
|
||||
- [ ] **Step 4: Run test to verify it passes**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/search/test_error_shapes.py::TestBackstopViaParseUserQuery" --override-ini="addopts=" -v`
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 5: Run the full query test file for regressions**
|
||||
|
||||
Run: `cd src && uv run pytest documents/tests/search/test_query.py --override-ini="addopts=" -q`
|
||||
Expected: PASS (existing `parse_user_query` behavior, including `InvalidDateQuery` propagation, intact).
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add src/documents/search/_query.py src/documents/tests/search/test_error_shapes.py
|
||||
git commit -m "feat(search): wrap parse_query to surface friendly error shapes"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Guard tests (pin prefixes + drift) and view-level 400
|
||||
|
||||
**Files:**
|
||||
|
||||
- Modify: `src/documents/search/__init__.py` (verify the new error classes are exported; add if missing)
|
||||
- Test: `src/documents/tests/search/test_error_shapes.py` (pin + drift guards)
|
||||
- Test: `src/documents/tests/test_api_search.py` (one view-level integration test)
|
||||
|
||||
- [ ] **Step 1: Verify the search package exports the new classes**
|
||||
|
||||
Run: `cd src && rg -n "SearchQueryError|InvalidDateQuery|__all__" documents/search/__init__.py`
|
||||
|
||||
If `SearchQueryError` is re-exported there (the view imports `from documents.search import SearchQueryError`), add the three new classes the same way. Example edit — add to the existing `from documents.search._translate import ...` block and to `__all__` if present:
|
||||
|
||||
```python
|
||||
from documents.search._translate import InvalidFieldValueError
|
||||
from documents.search._translate import MalformedQueryError
|
||||
from documents.search._translate import UnknownFieldError
|
||||
```
|
||||
|
||||
(The subclasses route through the existing `except SearchQueryError` handler regardless, so exporting is for discoverability/consumers. Skip if the package does not re-export error classes.)
|
||||
|
||||
- [ ] **Step 2: Write the failing pin + drift guard tests**
|
||||
|
||||
Append to `src/documents/tests/search/test_error_shapes.py`:
|
||||
|
||||
```python
|
||||
from documents.search._query import DEFAULT_SEARCH_FIELDS
|
||||
from documents.search._query import _FIELD_BOOSTS
|
||||
from documents.search._translate import SEARCHABLE_FIELDS as _SEARCHABLE
|
||||
|
||||
|
||||
@pytest.mark.search
|
||||
class TestTantivyPinnedPrefixes:
|
||||
"""If a tantivy-py upgrade changes these prefixes, the backstop silently
|
||||
regresses to the generic 400. Pin them so the upgrade fails loudly."""
|
||||
|
||||
def _err(self, index: tantivy.Index, raw: str) -> str:
|
||||
with pytest.raises(ValueError) as exc_info:
|
||||
index.parse_query(raw, DEFAULT_SEARCH_FIELDS, field_boosts=_FIELD_BOOSTS)
|
||||
return str(exc_info.value)
|
||||
|
||||
def test_unknown_field_prefix(self, index: tantivy.Index):
|
||||
assert self._err(index, "foobar:hello").startswith("Field does not exist:")
|
||||
|
||||
def test_syntax_error_prefix(self, index: tantivy.Index):
|
||||
assert self._err(index, "(invoice OR bill").startswith("Syntax Error")
|
||||
|
||||
def test_integer_error_prefix(self, index: tantivy.Index):
|
||||
assert self._err(index, "asn:notanumber").startswith("Expected a valid integer")
|
||||
|
||||
|
||||
@pytest.mark.search
|
||||
class TestFieldDriftGuard:
|
||||
"""Every user-facing searchable field must be a real schema field. tantivy
|
||||
exposes no field-name list, so we assert via parse: a real field never raises
|
||||
'Field does not exist'."""
|
||||
|
||||
@pytest.mark.parametrize("field", sorted(_SEARCHABLE))
|
||||
def test_searchable_field_exists_in_schema(self, index: tantivy.Index, field):
|
||||
try:
|
||||
index.parse_query(
|
||||
f"{field}:1",
|
||||
DEFAULT_SEARCH_FIELDS,
|
||||
field_boosts=_FIELD_BOOSTS,
|
||||
)
|
||||
except ValueError as exc:
|
||||
# A type/syntax error proves the field EXISTS; only "does not exist"
|
||||
# is a drift failure.
|
||||
assert "Field does not exist" not in str(exc), (
|
||||
f"{field!r} is in SEARCHABLE_FIELDS but missing from the schema"
|
||||
)
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Run the guard tests to verify they pass**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/search/test_error_shapes.py::TestTantivyPinnedPrefixes" "documents/tests/search/test_error_shapes.py::TestFieldDriftGuard" --override-ini="addopts=" -v`
|
||||
Expected: PASS. (These assert current truth; they guard against future drift. If `TestFieldDriftGuard` fails now, `SEARCHABLE_FIELDS` lists a name not in the schema — fix `KNOWN_FIELDS`/`NUMERIC_FIELDS`, not the test.)
|
||||
|
||||
- [ ] **Step 4: Write the failing view-level test**
|
||||
|
||||
In `src/documents/tests/test_api_search.py`, locate `test_search_added_invalid_date` (around line 765) and add this test directly after it, inside the same `TestDocumentSearchApi` class (mirrors that test's structure):
|
||||
|
||||
```python
|
||||
def test_search_unknown_field_returns_400(self) -> None:
|
||||
"""
|
||||
GIVEN:
|
||||
- A query scoping on a non-existent field
|
||||
WHEN:
|
||||
- The search API is called
|
||||
THEN:
|
||||
- HTTP 400 with the unknown-field message under the "query" key
|
||||
"""
|
||||
response = self.client.get("/api/documents/?query=foobar:hello")
|
||||
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
|
||||
self.assertIn("foobar", str(response.data["query"]))
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Run the view-level test to verify it passes**
|
||||
|
||||
Run: `cd src && uv run pytest "documents/tests/test_api_search.py::TestDocumentSearchApi::test_search_unknown_field_returns_400" --override-ini="addopts=" -v`
|
||||
Expected: PASS (the existing `except SearchQueryError` handler converts `UnknownFieldError` to `ValidationError({"query": [...]})`).
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add src/documents/search/__init__.py src/documents/tests/search/test_error_shapes.py src/documents/tests/test_api_search.py
|
||||
git commit -m "test(search): pin tantivy error prefixes, guard field drift, view 400"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 6: Full suite + lint
|
||||
|
||||
**Files:** none (verification only)
|
||||
|
||||
- [ ] **Step 1: Run the whole search test directory**
|
||||
|
||||
Run: `cd src && uv run pytest documents/tests/search/ --override-ini="addopts=" -q`
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 2: Run the API search tests**
|
||||
|
||||
Run: `cd src && uv run pytest documents/tests/test_api_search.py --override-ini="addopts=" -q`
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 3: Lint the changed files**
|
||||
|
||||
Run: `cd src && uv run ruff check documents/search/_translate.py documents/search/_query.py documents/tests/search/test_error_shapes.py`
|
||||
Expected: no errors (fix any import-ordering/formatting issues ruff reports; run `uv run ruff format` on the same files if needed).
|
||||
|
||||
- [ ] **Step 4: Final commit (only if lint produced changes)**
|
||||
|
||||
```bash
|
||||
git add -A
|
||||
git commit -m "chore(search): lint error-shapes follow-up"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Self-Review
|
||||
|
||||
**Spec coverage:**
|
||||
|
||||
- `UnknownFieldError` (+ did-you-mean, legacy backend fields as unknown) -> Tasks 1, 3, 4.
|
||||
- `InvalidFieldValueError` (proactive + backstop) -> Tasks 1, 2, 4.
|
||||
- `MalformedQueryError` (balance-check, no verbatim echo, open-range caveat) -> Tasks 1, 3, 4.
|
||||
- Hybrid detection (proactive scanner + backstop wrapper) -> Tasks 2, 4.
|
||||
- `>`/`<` left working + validator allows operators -> Task 2 (`test_valid_numeric_values_do_not_raise`), Task 4 (`test_comparison_and_range_queries_succeed`).
|
||||
- Single source of truth + drift guard -> Task 1 (`SEARCHABLE_FIELDS`), Task 5 (`TestFieldDriftGuard`).
|
||||
- Message-prefix pin test -> Task 5 (`TestTantivyPinnedPrefixes`).
|
||||
- Dotted-JSON / open-numeric-range / view-400 -> Tasks 4 and 5.
|
||||
- Out of scope (frontend, URL search) -> correctly untouched.
|
||||
|
||||
**Placeholder scan:** none — every code step shows full code and exact commands.
|
||||
|
||||
**Type/name consistency:** `UnknownFieldError(field, suggestion=)`, `InvalidFieldValueError(field=None, value=None)`, `MalformedQueryError(message)`, `NUMERIC_FIELDS`, `SEARCHABLE_FIELDS`, `_validate_numeric(field, value)`, `_suggest_field(field)`, `_malformed_hint(raw_query)`, `map_tantivy_error(exc, raw_query)`, `_parse_query_friendly(index, query_str, raw_query, default_fields, **kwargs)` are used identically across all tasks.
|
||||
File diff suppressed because it is too large.
Load diff
@@ -1,262 +0,0 @@
|
||||
# Friendlier advanced-search error shapes
|
||||
|
||||
**Status:** design / ready for implementation plan.
|
||||
**Follow-up to:** the `InvalidDateQuery` work on branch `fix/search-query-translation`
|
||||
(PR #13010), itself specced in
|
||||
`docs/superpowers/done/specs/2026-06-14-search-query-translation-design.md`.
|
||||
**Builds on:** the `SearchQueryError(ValueError)` base in
|
||||
`documents/search/_translate.py` and the single `except SearchQueryError` handler
|
||||
in `UnifiedSearchViewSet.list` (`documents/views.py:2612`), which re-raises as DRF
|
||||
`ValidationError({"query": [msg]})`. Any new subclass surfaces through that one
|
||||
handler automatically, so this work is purely additive.
|
||||
|
||||
## Problem
|
||||
|
||||
Every advanced-search failure other than the now-handled invalid date lands in
|
||||
the view's generic `except Exception` and returns
|
||||
`HttpResponseBadRequest("Error listing search results, check logs for more
|
||||
detail.")` (`views.py:2617-2621`). `index.parse_query(...)` runs _outside_ the
|
||||
`translate_query` try/except in `parse_user_query` (`_query.py:220-235`), so
|
||||
anything Tantivy rejects bypasses `SearchQueryError` entirely and gets the
|
||||
unhelpful generic 400. Some Tantivy errors also leak Rust internals (e.g.
|
||||
`ParseIntError { kind: InvalidDigit }`) if surfaced verbatim.
|
||||
|
||||
## Ground truth: what Tantivy raises (empirically re-verified 2026-06-15)
|
||||
|
||||
Probed against a real index built from `documents.search._schema.build_schema` +
|
||||
`_tokenizer.register_tokenizers`, running each query through `translate_query`
|
||||
then `index.parse_query(..., DEFAULT_SEARCH_FIELDS, field_boosts=_FIELD_BOOSTS)`.
|
||||
`index.parse_query` raises `ValueError` with three distinguishable message
|
||||
families:
|
||||
|
||||
| Family | Example inputs | Tantivy message |
|
||||
| ---------------- | ---------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------ |
|
||||
| Unknown field | `foobar:hello`, `owner:5`, `has_tags:true`, `is_shared:true` | `Field does not exist: 'foobar'` |
|
||||
| Syntax error | `(invoice OR bill`, `created:[2020 TO 2021`, `invoice AND`, `OR invoice`, `title:"abc`, `:value`, `AND OR` | `Syntax Error: <echoes the whole query, no location>` |
|
||||
| Wrong value type | `asn:notanumber`, `page_count:[foo TO bar]` | `Expected a valid integer: 'ParseIntError { kind: InvalidDigit }'` |
|
||||
|
||||
**Parses OK (NOT errors):** `page_count:>5`, `asn:<10`, `page_count:>=5` (these
|
||||
produce _correct_ Tantivy `RangeQuery` objects — see the `>`/`<` decision below),
|
||||
`page_count:5` (TermQuery), `title:[a TO b]` (Str range on a text field),
|
||||
`title:~~~` (EmptyQuery), `` (empty query).
|
||||
|
||||
## The `>`/`<`/`>=`/`<=` decision (investigated)
|
||||
|
||||
The stub flagged these as "parses OK but possibly silently wrong vs Whoosh." That
|
||||
is **incorrect** — verified empirically:
|
||||
|
||||
- `page_count:>5` -> `RangeQuery { lower_bound: Excluded(5), upper_bound: Unbounded }`
|
||||
- `page_count:>=5` -> `RangeQuery { lower_bound: Included(5), ... }`
|
||||
- `asn:<10` -> `RangeQuery { lower_bound: Unbounded, upper_bound: Excluded(10) }`
|
||||
|
||||
Tantivy's query parser supports comparison operators natively and produces
|
||||
correct range queries. They were _never_ supported in paperless-Whoosh (no
|
||||
`GtLtPlugin`; confirmed in the base design §3 and against the old schema on
|
||||
`main`), so there is no prior behavior to "match" and nothing is silently wrong.
|
||||
|
||||
**Decision: leave them as-is.** They work correctly and are effectively a free
|
||||
capability gain over Whoosh. Do not reject them. The numeric value validator
|
||||
(below) must explicitly _allow_ a leading `>`, `<`, `>=`, or `<=` so it does not
|
||||
wrongly reject working comparison queries. Document them as supported.
|
||||
|
||||
Note the asymmetry (verified): the _comparison_ forms work, but _open
|
||||
bracket-ranges_ on numeric fields do NOT. `asn:[1 TO]` and `asn:[TO 10]`
|
||||
translate verbatim and Tantivy raises `Syntax Error` on them — only open _date_
|
||||
ranges are rewritten to bounded sentinels (`OPEN_LO`/`OPEN_HI`) by
|
||||
`translate_range`. So `asn:>1` works but `asn:[1 TO]` is malformed; readers must
|
||||
not assume bracket-ranges behave like the comparison operators.
|
||||
|
||||
## Clarification: the old "Whoosh-only" fields were backend internals
|
||||
|
||||
The stub proposed targeted "this field isn't available in full-text search; use
|
||||
the filter sidebar" messages for `owner`, `has_tags`, `is_shared`, etc. Checking
|
||||
the old Whoosh code still present on `main` (`src/documents/index.py`,
|
||||
`make_schema`) shows these were **backend filter / permission fields, not
|
||||
user-facing search syntax**:
|
||||
|
||||
- `has_correspondent`, `has_tag`, `has_type`, `has_path`, `has_owner`,
|
||||
`has_custom_fields`, `is_shared`, `custom_field_count`, and `owner` (username
|
||||
text) were populated at index time so the permission and list-filter machinery
|
||||
could query them. `get_permissions_criterias()` builds
|
||||
`query.Term("has_owner", ...)`, `query.Term("owner_id", ...)`,
|
||||
`query.Term("viewer_id", ...)` programmatically.
|
||||
- The user-facing full-text parser (`DelayedFullTextQuery`) advertised only
|
||||
`["content", "title", "correspondent", "tag", "type", "notes",
|
||||
"custom_fields"]`. (Whoosh's generic `FieldsPlugin` would technically parse a
|
||||
typed `has_tag:true`, but that was never a designed or documented feature.)
|
||||
|
||||
**Consequence:** no curated per-field messages. These names are simply _unknown
|
||||
fields_ and flow through `UnknownFieldError` + did-you-mean like any typo. Note
|
||||
that `owner_id` and `viewer_id` legitimately remain queryable (present in
|
||||
`KNOWN_FIELDS` and the new schema, still used for permission filtering), so they
|
||||
are correctly _not_ unknown; `owner`, the `has_*` booleans, `is_shared`, and
|
||||
`custom_field_count` are correctly absent and read as plain unknown fields.
|
||||
|
||||
## Proposed error shapes (all `SearchQueryError` subclasses)
|
||||
|
||||
All three live next to `InvalidDateQuery` in `_translate.py` and inherit the
|
||||
"message is safe to surface" contract.
|
||||
|
||||
### 1. `UnknownFieldError`
|
||||
|
||||
`Unknown search field 'corespondent'.` plus a `Did you mean 'correspondent'?`
|
||||
suggestion via `difflib.get_close_matches(field, suggestion_pool, n=1)`. The
|
||||
suggestion pool is the user-facing field set derived from `KNOWN_FIELDS` (see
|
||||
single-source-of-truth below). Typos get a suggestion; names with no close match
|
||||
(e.g. `has_tags`) get the bare unknown-field message.
|
||||
|
||||
### 2. `InvalidFieldValueError`
|
||||
|
||||
Sibling of `InvalidDateQuery`. For numeric fields: `Field 'asn' expects a number,
|
||||
got 'notanumber'.` Carries `field` + `value` attributes like `InvalidDateQuery`.
|
||||
Removes the `ParseIntError { ... }` Rust leak.
|
||||
|
||||
### 3. `MalformedQueryError`
|
||||
|
||||
Structural syntax errors. A cheap balance-check pass gives specific hints for the
|
||||
common cases (unbalanced `"`, `[`, `(`, dangling/leading `AND`/`OR`) before
|
||||
falling back to a clean generic "check for unbalanced quotes, brackets, or
|
||||
parentheses." Tantivy's message has no location and echoes the whole query, so it
|
||||
is **never** surfaced verbatim.
|
||||
|
||||
Caveat: an open numeric bracket-range (`asn:[1 TO]`) reaches this path as a
|
||||
`Syntax Error` even though its brackets are _balanced_. The balance-check must not
|
||||
confidently assert "unbalanced brackets" for it — when brackets/quotes/parens are
|
||||
balanced, fall back to the generic hint rather than a wrong-but-specific one.
|
||||
|
||||
## Detection strategy: hybrid, split by what each layer can see
|
||||
|
||||
The scanner and the parse-wrapper see different things; assigning each error
|
||||
shape to the layer that can detect it cleanly avoids false positives.
|
||||
|
||||
### Proactive numeric validation in the scanner (`_translate.py`)
|
||||
|
||||
`scan()` already tokenizes recognized `field:value` and `field:[range]` clauses
|
||||
into `FieldValue` / `FieldRange` for fields in `KNOWN_FIELDS`. Add a
|
||||
`NUMERIC_FIELDS` set (`asn`, `page_count`, `num_notes`, and the `*_id` fields)
|
||||
and validate those tokens' values during translation, raising
|
||||
`InvalidFieldValueError` before the string ever reaches `index.parse_query`.
|
||||
|
||||
Validation rules:
|
||||
|
||||
- Strip a single leading comparison operator (`>=`, `<=`, `>`, `<`) before the
|
||||
integer check, so comparison queries pass.
|
||||
- For ranges, validate each present bound (`lo`, `hi`) as an integer. An empty
|
||||
bound passes the _integer_ check (an empty string is not a bad integer), but be
|
||||
aware that an open numeric bracket-range (`asn:[1 TO]`) is still rejected
|
||||
downstream by Tantivy as a `Syntax Error` (see the `>`/`<` note above) — i.e.
|
||||
the validator does not make it succeed, it falls through to the malformed path.
|
||||
Do not emit a "bad number" message for an empty bound; let the structural error
|
||||
surface as `MalformedQueryError`.
|
||||
- For multi-value numeric fields after comma expansion (`tag_id`, `viewer_id`),
|
||||
validate each expanded value (`tag_id:1,foo` -> `InvalidFieldValueError`).
|
||||
|
||||
This path owns `InvalidFieldValueError` exclusively: messages are rich,
|
||||
context-aware, and independent of Tantivy's English strings.
|
||||
|
||||
### Why unknown-field detection is NOT proactive
|
||||
|
||||
`_match_field_token` returns `None` for any field not in `KNOWN_FIELDS`
|
||||
(`_translate.py:193`) — an unknown `foobar:hello` is intentionally left as a
|
||||
`Passthrough` (the existing `http:`-misfire guard). Detecting unknown fields in
|
||||
the scanner would require a separate `\w+:` pass that re-introduces exactly the
|
||||
false positives that guard exists to prevent:
|
||||
|
||||
- URLs: `http://example.com/a` (`http:`)
|
||||
- Dotted JSON subfields, which are valid: `notes.user:alice`,
|
||||
`custom_fields.invoice_no:123` (the `\w+:` regex would see `user:` / `invoice_no:`)
|
||||
- Time-like literals: `12:30` (note: a bare `12:30` is already a parse failure
|
||||
today — Tantivy raises `Field does not exist: '12'` — so under this design it
|
||||
reshapes into `UnknownFieldError("12")` with no close match, i.e. the bare
|
||||
unknown-field message. It is _not_ a clean passthrough; the point here is only
|
||||
that a proactive `\w+:` pass would mis-flag it even more aggressively.)
|
||||
|
||||
So unknown fields are detected in the backstop instead, where Tantivy has already
|
||||
confirmed the token is a real field reference.
|
||||
|
||||
### Catch-and-sanitize backstop around `index.parse_query` (`_query.py`)
|
||||
|
||||
Wrap the `index.parse_query` call(s) in `parse_user_query`. Map residual Tantivy
|
||||
`ValueError` messages by prefix:
|
||||
|
||||
- `Field does not exist: 'X'` -> extract `X`, build `UnknownFieldError(X)` with
|
||||
did-you-mean.
|
||||
- `Syntax Error: ...` -> `MalformedQueryError` (run the balance-check for a
|
||||
specific hint; never echo the Tantivy text).
|
||||
- `Expected a valid integer: ...` -> `InvalidFieldValueError`. This backstop is
|
||||
effectively _unreachable today_: every query that produces this Tantivy error
|
||||
goes through a recognized numeric field token (`asn`, `page_count`, `num_notes`,
|
||||
`*_id`) that `scan()` already models, so the proactive pass catches it first
|
||||
(verified — no query reaches this branch without the proactive pass firing).
|
||||
Its real value is forward-safety: if a future numeric field is added to the
|
||||
schema but not to `NUMERIC_FIELDS`, this branch guarantees the raw
|
||||
`ParseIntError { ... }` Rust struct never leaks. Keep it; the generic "expects a
|
||||
number" message stands in when `field`/`value` cannot be recovered from the
|
||||
Tantivy text.
|
||||
- Anything unrecognized -> re-raise, preserving today's generic-500/400 path
|
||||
rather than inventing a misleading message.
|
||||
|
||||
Both the fuzzy and exact `parse_query` calls go through the same wrapper.
|
||||
|
||||
### Single source of truth for fields
|
||||
|
||||
`KNOWN_FIELDS` (`_translate.py:63`) is the canonical set of field names a user may
|
||||
validly scope on; it already includes the v2 aliases (`type`, `path`, `type_id`,
|
||||
`path_id`) that `translate_query` rewrites to real schema names. Use it (with
|
||||
aliases optionally excluded from the _suggestion_ pool to avoid suggesting a
|
||||
deprecated alias) for both validation and did-you-mean.
|
||||
|
||||
Add a drift-guard test asserting `KNOWN_FIELDS` minus the alias set is a subset of
|
||||
the schema field names produced by `build_schema()`, so the two definitions cannot
|
||||
silently diverge as the schema evolves. The backend-only Whoosh names (`owner`,
|
||||
`has_*`, `is_shared`, `custom_field_count`) are correctly excluded from both.
|
||||
|
||||
## Testing
|
||||
|
||||
New dedicated test file (per project convention), e.g.
|
||||
`src/documents/tests/test_search_error_shapes.py`:
|
||||
|
||||
- One case per error family asserting the `SearchQueryError` subclass and a
|
||||
user-safe message (no paths, no Rust structs, no verbatim Tantivy echo).
|
||||
- `UnknownFieldError`: typo yields a did-you-mean suggestion; a no-close-match
|
||||
name (e.g. `has_tags`) yields the bare message.
|
||||
- `InvalidFieldValueError`: `asn:notanumber`, `page_count:[foo TO bar]`, and a
|
||||
bad multi-value `tag_id:1,foo`.
|
||||
- **`>`/`<` working case**: `page_count:>5` / `asn:<10` / `page_count:>=5` parse
|
||||
successfully and are NOT raised as errors (guards the numeric validator's
|
||||
operator allowance).
|
||||
- **Open numeric range**: `asn:[1 TO]` / `asn:[TO 10]` surface as
|
||||
`MalformedQueryError` (Tantivy `Syntax Error`, brackets balanced) and the hint
|
||||
is the generic one, NOT a false "unbalanced brackets" claim.
|
||||
- **Dotted JSON non-regression**: `notes.user:alice`,
|
||||
`custom_fields.name:invoice` are not flagged as unknown fields.
|
||||
- **URL behavior**: a query containing `http://...` is unchanged from today's
|
||||
behavior — Tantivy treats `http` as a field, so it still 400s; under this design
|
||||
the message becomes the clearer `UnknownFieldError('http')` (no close match) and
|
||||
the proactive numeric pass does not touch it. This is a clarity gain, not a
|
||||
regression (it already 400'd generically). Out of scope to make URL substrings
|
||||
searchable.
|
||||
- **Message-prefix pin**: a test that asserts the exact Tantivy prefixes
|
||||
(`Field does not exist:`, `Syntax Error:`, `Expected a valid integer:`) the
|
||||
backstop depends on, so a `tantivy-py` upgrade that changes them fails loudly
|
||||
instead of silently regressing to the generic 400.
|
||||
- **Drift guard**: `KNOWN_FIELDS` (minus aliases) ⊆ `build_schema()` field names.
|
||||
- View-level: each subclass surfaces as HTTP 400 with `{"query": [msg]}` through
|
||||
the existing handler.
|
||||
|
||||
## Risks / notes
|
||||
|
||||
- The backstop depends on Tantivy's error _message-string prefixes_, which are
|
||||
brittle across `tantivy-py` upgrades. The pin test above is the mitigation.
|
||||
- Keep all messages safe to surface: they may echo user input but must never
|
||||
include internal paths, stack details, or Rust error structs.
|
||||
- The balance-check for `MalformedQueryError` is a heuristic for _hints_ only; its
|
||||
failure mode is the clean generic message, never a wrong-but-confident one.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Frontend rendering of the structured `{"query": [...]}` 400 (the inline search
|
||||
error UI). Only relevant if the messages should render differently from the
|
||||
current generic banner; the current banner already displays the message.
|
||||
- Adding or changing `>`/`<` semantics. They work; this spec only ensures the
|
||||
numeric validator does not break them.
|
||||
@@ -1,139 +0,0 @@
|
||||
# Whoosh→Tantivy Advanced-Query Translation — Design
|
||||
|
||||
**Date:** 2026-06-15
|
||||
**Branch base:** `dev` (Phase 1 implemented on `fix/search-query-translation`)
|
||||
**Status:** Phase 1 implemented; Phase 2 pending an upstream `tantivy-py` release (see below)
|
||||
|
||||
## Problem
|
||||
|
||||
The Tantivy search migration changed the advanced-query (`?query=`) syntax contract.
|
||||
A class of queries that worked under the old Whoosh backend now return an opaque
|
||||
**HTTP 400**, because the old query string is handed to `tantivy.Index.parse_query`
|
||||
which rejects forms Whoosh accepted. There is no fallback: a parse error propagates
|
||||
through `documents/search/_backend.py` and is caught by the generic handler in
|
||||
`documents/views.py` → `HttpResponseBadRequest`, with the real error only in logs.
|
||||
|
||||
Affected query forms (all verified against real Whoosh and real Tantivy):
|
||||
|
||||
- Bare dates on a date field: `created:2020`, `created:202003`.
|
||||
- Bracketed absolute/partial/ISO ranges: `created:[20200101 TO 20201231]`,
|
||||
`created:[2020-01-01 TO 2020-12-31]`, `created:[202001 TO 202006]`.
|
||||
- Open-ended ranges: `created:[2020 to]`, `created:[to 2020]`.
|
||||
- Relative ranges: `added:[-1 week to now]`, `added:[now-7d TO now]`.
|
||||
- Comma-joined clauses and value lists: `created:[r],added:[r]`, `tag:foo,bar`,
|
||||
the malformed `title:x,created:[…]`.
|
||||
- Renamed fields: `type:`/`path:` (Whoosh names) vs `document_type:`/`storage_path:`
|
||||
(Tantivy names).
|
||||
- Invalid dates (`created:202023`) — Whoosh matched nothing (NullQuery); Tantivy 400s.
|
||||
|
||||
The original compatibility layer was a stack of order-dependent regex substitutions,
|
||||
which had no structural awareness (could not tell a comma inside `[...]` from a clause
|
||||
separator) and required a new regex per form. This design replaces it.
|
||||
|
||||
## Approach
|
||||
|
||||
A structural, context-aware translation pass that intercepts only the forms Tantivy
|
||||
parses differently (dates, commas, renamed fields) and passes everything else
|
||||
(booleans, grouping, phrases, wildcards) straight through to Tantivy's own parser.
|
||||
|
||||
Pipeline (`documents/search/_translate.py`):
|
||||
|
||||
```
|
||||
parse_user_query(raw, tz)
|
||||
→ translate_query(raw, tz) # wrapped in a safety net: on any
|
||||
→ scan(raw) depth-aware tokenizer (quotes / [] depth)
|
||||
→ resolve_commas value-list (tag/tag_id/viewer_id) vs clause separator
|
||||
→ _render date tokens → translate_scalar / translate_range,
|
||||
field aliasing, comma → AND
|
||||
→ operator normalization (spaced/trailing -/+ cleanup)
|
||||
→ index.parse_query(translated, …) # exception → fall back to raw query
|
||||
```
|
||||
|
||||
Date math lives in `documents/search/_dates.py` (no Django deps). The two date-field
|
||||
semantics are preserved: `created` is date-only (UTC-midnight boundaries);
|
||||
`added`/`modified` are datetimes (local-tz-midnight → UTC).
|
||||
|
||||
### Verified compatibility contract (from running both engines)
|
||||
|
||||
- Comma between clauses = AND, both sides preserved; comma within a `KEYWORD(commas=True)`
|
||||
field value = value list. Multi-value fields are exactly `{tag, tag_id, viewer_id}`.
|
||||
- Invalid/unparsable dates → a no-match clause (never a 400), matching Whoosh's NullQuery.
|
||||
- Field renames to alias: `type`→`document_type`, `type_id`→`document_type_id`,
|
||||
`path`→`storage_path`, `path_id`→`storage_path_id`. Both old and new names work.
|
||||
- Partial-date ranges floor the low bound and ceil the high bound; reversed ranges swap.
|
||||
|
||||
## Phase 1 (implemented)
|
||||
|
||||
Branch `fix/search-query-translation`. The full pipeline above, output as a Tantivy
|
||||
query **string**, with these workarounds for things the string parser cannot express
|
||||
on date fields:
|
||||
|
||||
- **Open-ended ranges** use far-past / far-future string sentinels
|
||||
(`0001-01-01T00:00:00Z` / `9999-12-31T23:59:59Z`).
|
||||
- **No-match** (unparsable date) uses a degenerate equal-bound date range.
|
||||
|
||||
Status: complete; the full `-m search` suite passes (date forms, comma clauses, field
|
||||
aliasing, relative ranges, operator normalization, and the existing search tests now
|
||||
validating the new pipeline). The old `_rewrite_*` regex helpers were left in place as
|
||||
delegation shims during the transition.
|
||||
|
||||
Landed in 15 commits on `fix/search-query-translation` (`f1b92a493`..`816a078a4`):
|
||||
`_dates.py` extraction → partial-date helpers → scanner → comma resolution →
|
||||
scalar/range translation → field aliasing → relative ranges + operator normalization →
|
||||
`parse_user_query` wiring + delegation + tests. Key symbols in `_translate.py`:
|
||||
`scan`, `resolve_commas`, `translate_scalar`, `translate_range`, `translate_query`;
|
||||
constants `MULTI_VALUE_FIELDS`, `DATE_FIELDS`, `KNOWN_FIELDS`, `FIELD_ALIASES`,
|
||||
`NO_MATCH`, `OPEN_LO`, `OPEN_HI`.
|
||||
|
||||
## Phase 2 (pending — the thing being tracked)
|
||||
|
||||
Replace the Phase-1 string workarounds with real `tantivy.Query` objects for date
|
||||
clauses, which removes the sentinel/degenerate-range hacks entirely:
|
||||
|
||||
1. **Open bounds** via `Query.range_query(field, FieldType.Date, lower_bound=…,
|
||||
upper_bound=None)` (and vice-versa) instead of `OPEN_LO`/`OPEN_HI` sentinels.
|
||||
2. **No-match** via `Query.empty_query()` instead of the degenerate range. This also
|
||||
fixes the cosmetic issue that the no-match sentinel always names the `created` field.
|
||||
3. **Retire the dead code**: remove the now-unused `_rewrite_*` helpers and the
|
||||
`rewrite_natural_date_keywords` / `normalize_query` delegation shims in `_query.py`
|
||||
(~160 lines left from the Phase-1 transition), and migrate their string-output tests
|
||||
in `test_query.py` (replace the direct `_rewrite_compact_date` test with a
|
||||
`translate_scalar` test).
|
||||
|
||||
### Blocker
|
||||
|
||||
Phase 2 is **gated on a published `tantivy-py` release**, not on any further code
|
||||
contribution. In `tantivy-py 0.26.0` (our current pin: `tantivy~=0.26.0` in
|
||||
`pyproject.toml`, released 2026-04-29), `range_query`/`term_query` **reject Python
|
||||
`datetime` values on `Date` fields** (`Expected DateTime type for field …`), so date
|
||||
Query objects cannot be built from Python. The fix is already merged on `tantivy-py`
|
||||
`master` across two PRs:
|
||||
|
||||
- **#655** — `feat: support unbounded range queries via None bounds`.
|
||||
- **#666** — `fix: add_date loses tzinfo` (adds the `PyDateTime → tantivy DateTime`
|
||||
converter and routes `range_query`/`term_query` through it).
|
||||
|
||||
Both postdate the `0.26.0` wheel.
|
||||
|
||||
- **Trigger:** a `tantivy-py` release `> 0.26.0` containing #655 + #666 is published to PyPI.
|
||||
- **Action:** bump the `tantivy-py` pin, then do items 1–3 above.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Unknown-field handling: Whoosh degraded an unknown `field:` to text; Tantivy 400s
|
||||
(`http://x/a,b` → `Field does not exist: 'http'`). Not a date/comma/rename issue.
|
||||
- Whoosh fields with no Tantivy equivalent: `owner` (text), the `has_*` presence
|
||||
booleans, `is_shared`, `custom_field_count`, `custom_fields_id`.
|
||||
- `>`/`<`/`>=`/`<=` comparisons — never supported in paperless-Whoosh (no `GtLtPlugin`).
|
||||
|
||||
## Reference / how to re-verify
|
||||
|
||||
- Tantivy side (does a translated string parse?): build a real index via
|
||||
`documents.search._schema.build_schema` + `register_tokenizers`, then
|
||||
`index.parse_query(translate_query(q, tz), DEFAULT_SEARCH_FIELDS, field_boosts=…)`.
|
||||
- Whoosh side (what did v2 do?): `src/documents/index.py` (the old `get_schema()` +
|
||||
`MultifieldParser([...]) + DateParserPlugin(...)`) was deleted from `main` in
|
||||
`aed9abe48` (#12471, 2026-04-02); check it out at `git show aed9abe48^:src/documents/index.py`
|
||||
(or `git checkout aed9abe48^ -- src/documents/index.py`) and run a query through it
|
||||
to get the ground-truth `Query`.
|
||||
- A fuller empirical gap matrix lives in `SEARCH_TANTIVY_WHOOSH_COMPAT.md`.
|
||||
@@ -0,0 +1,355 @@
|
||||
# Structured advanced-search errors from whoosh-compat diagnostics
|
||||
|
||||
**Status:** design / ready for implementation plan.
|
||||
**Builds on:** advanced search parsed by whoosh-compat (#14072) and the search
|
||||
field registry (#14071). Line numbers below refer to `dev` at `34a6c718f`.
|
||||
**Scope:** backend. The frontend work lands on the same branch from a
|
||||
co-maintainer; see "Frontend compatibility" and "Consumer notes".
|
||||
|
||||
## Current behavior
|
||||
|
||||
Advanced search does not use Tantivy's query-string parser.
|
||||
`parse_user_query` (`documents/search/_query.py`) calls `wc.parse()` against the
|
||||
`FieldRegistry` built from `PUBLIC_FIELDS`, maps any `Diagnostic`s to
|
||||
`SearchQueryError` subclasses, then emits a `tantivy.Query` directly from the
|
||||
AST. Probing the real `parse_user_query` on `dev` (whoosh-compat 0.3, tantivy
|
||||
0.26) gives:
|
||||
|
||||
| Input | Result today |
|
||||
| --------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ |
|
||||
| `foobar:hello`, `correspondent:bob`, `has_tags:true`, `owner_id:1`, `http://x`, `12:30` | No error. Unknown field prefixes are demoted to text and searched literally (documented in `docs/usage.md`). |
|
||||
| `(invoice OR bill`, `invoice AND`, `OR invoice`, `title:"abc` | No error. The whoosh grammar is lenient and never fails on structure. |
|
||||
| `asn:notanumber`, `page_count:[foo TO bar]`, `asn:1,2` | 400, `InvalidNumberQuery`. |
|
||||
| `page_count:>5`, `asn:<10` | 400, `InvalidNumberQuery` (comparison operators are not whoosh syntax). |
|
||||
| `asn:[1 TO]`, `asn:[TO 10]` | Works (open numeric ranges). |
|
||||
| `created:notadate`, `created:[2020 TO 2021` | 400, `InvalidDateQuery`. |
|
||||
| `title:[a TO b]`, `notes:*`, `asn:*x` | 400, plain `SearchQueryError` with a kind-specific message. |
|
||||
| `asn:5 created:bad page_count:x` | 400, `MultipleSearchQueryErrors`, one message per bad value. |
|
||||
|
||||
Every user-fixable failure already arrives as a `SearchQueryError` with a safe
|
||||
message.
|
||||
|
||||
## Problem
|
||||
|
||||
whoosh-compat's `Diagnostic` carries more than paperless uses. Besides `kind`
|
||||
and `raw_value`, each record has:
|
||||
|
||||
- `field`: set on parse-time diagnostics and on every emit-time one, but
|
||||
paperless only keeps it on `InvalidDateQuery`/`InvalidNumberQuery`.
|
||||
- `startchar` / `endchar`: where in the query string the problem is. Set on
|
||||
parse-time diagnostics and on emit-time ones (the emitter copies the node's
|
||||
span, `emitters/tantivy_.py:732-733`; `_widen_leaf`, `_cjk_alternative` and
|
||||
`_fuzzy_alternative` keep the original leaf span via `_leaf_span`).
|
||||
- `suggestion`: replacement text for that span when exactly one rewrite of the
|
||||
query would work. The library's documented host contract is to splice it in
|
||||
rather than re-derive the rule:
|
||||
`fixed = q[:startchar] + suggestion + q[endchar:]`.
|
||||
In 0.3 the only case that carries one is an unquoted multi-word date
|
||||
(`created:december 2019` suggests `"december 2019"`); `created:20231340`,
|
||||
number errors and every emit-time diagnostic carry none.
|
||||
|
||||
Paperless throws most of that away. `_single_diagnostic_to_error` and
|
||||
`_map_emit_error` keep only a message, and both views raise
|
||||
`ValidationError({"query": [msg, ...]})`. A client can tell the user _what_ is
|
||||
wrong but not _where_, and cannot offer the fix the library already computed.
|
||||
|
||||
Probed spans for reference:
|
||||
|
||||
```
|
||||
created:december 2019 BAD_DATE 8..21 suggestion '"december 2019"'
|
||||
created:december 2019 asn:x BAD_DATE 8..21 suggestion '"december 2019"'
|
||||
BAD_NUMBER 26..27 suggestion None
|
||||
created:20231340 BAD_DATE 8..16 suggestion None
|
||||
😀 created:december 2019 BAD_DATE 10..23 (code points; 11..24 in UTF-16)
|
||||
```
|
||||
|
||||
## Goals
|
||||
|
||||
- Every search-query 400 also carries, per error, its field, location, the
|
||||
offending text and the suggested replacement, in a typed shape a client can
|
||||
rely on.
|
||||
- When suggestions exist, the server returns one corrected query string, so no
|
||||
client has to do span arithmetic.
|
||||
- Additive only, with no new API version: the `query` key keeps exactly today's
|
||||
strings.
|
||||
- whoosh-compat types stay private to `_query.py`. The public payload is
|
||||
paperless's own contract, not the library's enum names.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- New suggestions. Fixes come only from `Diagnostic.suggestion`; paperless adds
|
||||
no rewrite rules of its own. New library suggestions (for example for `>5`)
|
||||
appear in the payload with a whoosh-compat upgrade and no paperless change.
|
||||
- Changes to whoosh-compat.
|
||||
- Hints for demoted unknown fields (`correspondent:bob`). `Diagnostic` is
|
||||
fatal-only by library contract, and the documented paperless behavior is to
|
||||
search such text literally.
|
||||
- Frontend changes. They are done on the same branch by a co-maintainer (see
|
||||
"Frontend compatibility" and "Consumer notes").
|
||||
- A live query-validation endpoint (see "Future work").
|
||||
- The `corrected_query: null` key on successful search responses
|
||||
(`views.py:2744`), a leftover of Whoosh spelling correction. Untouched.
|
||||
- An OpenAPI schema for the 400 body. The search list endpoint documents none
|
||||
today.
|
||||
|
||||
## Design
|
||||
|
||||
### Error model (`documents/search/_errors.py`)
|
||||
|
||||
`SearchQueryError` gains four keyword-only, optional attributes:
|
||||
|
||||
```python
|
||||
class SearchQueryError(ValueError):
|
||||
def __init__(
|
||||
self,
|
||||
message: str,
|
||||
*,
|
||||
field: str | None = None,
|
||||
start: int | None = None,
|
||||
end: int | None = None,
|
||||
suggestion: str | None = None,
|
||||
) -> None:
|
||||
self.field = field
|
||||
self.start = start
|
||||
self.end = end
|
||||
self.suggestion = suggestion
|
||||
super().__init__(message)
|
||||
```
|
||||
|
||||
`SearchQueryError("msg")` keeps working. `InvalidDateQuery` and
|
||||
`InvalidNumberQuery` keep their positional `(field, value)` signature, pass
|
||||
`field` up to the base, and accept and forward `start`, `end` and `suggestion`.
|
||||
`QueryTooLongError` and `MultipleSearchQueryErrors` leave all four `None`: the
|
||||
first has no span, and the second holds sub-errors that each carry their own.
|
||||
|
||||
Messages are unchanged byte for byte. The fix is never folded into `str(e)`.
|
||||
|
||||
### Diagnostic plumbing (`documents/search/_query.py`)
|
||||
|
||||
- `_single_diagnostic_to_error(d)` passes `field=str(d.field)` (when set),
|
||||
`start=d.startchar`, `end=d.endchar` and `suggestion=d.suggestion` to
|
||||
whichever error it builds, including the plain `SearchQueryError` branches
|
||||
(TOO_DEEP, PATTERN_ON_*, SINGLE_CHAR_BRACKET_RANGE) and the unmapped
|
||||
fallback.
|
||||
- `_map_emit_error(e)` does the same for the 400 it returns. Emit-time
|
||||
diagnostics never carry a suggestion in 0.3 (the emitter's `_fail` takes
|
||||
none); it is copied anyway so a future one flows through. The paths that
|
||||
re-raise (INTERNAL, and MISCONFIGURED other than EXISTS_REQUIRES_FAST) are
|
||||
unchanged.
|
||||
|
||||
`field` is the canonical dotted name `str(FieldRef)` produces, as messages
|
||||
already use (`type:` reports `document_type`, `notes.user:` reports
|
||||
`notes.user`).
|
||||
|
||||
### Detail helper (`documents/search/_errors.py`)
|
||||
|
||||
Next to `search_query_error_messages`, and exported from `documents.search`:
|
||||
|
||||
```python
|
||||
def search_query_error_details(
|
||||
e: SearchQueryError,
|
||||
query: str,
|
||||
) -> tuple[list[dict[str, object]], str | None]:
|
||||
"""(errors, suggested_query) for a SearchQueryError raised parsing ``query``."""
|
||||
```
|
||||
|
||||
- `errors` has one entry per leaf error, flattening `MultipleSearchQueryErrors`
|
||||
in the same order as the messages helper:
|
||||
|
||||
```
|
||||
{"message": str, "field": str | None, "start": int | None,
|
||||
"end": int | None, "text": str | None, "suggestion": str | None}
|
||||
```
|
||||
|
||||
`start`/`end` are code-point offsets into the normalized query (see
|
||||
"Which string the spans index"). `text` is `query[start:end]`, so a client
|
||||
never has to slice by offset itself; JavaScript strings index in UTF-16 units
|
||||
and would get astral characters (emoji, CJK Extension B) wrong. `text` is
|
||||
`None` when the error has no span or the span falls outside `query`.
|
||||
|
||||
- `suggested_query` applies every non-`None` suggestion to `query` right to
|
||||
left, so earlier spans stay valid. It is `None` when no error has a
|
||||
suggestion, or when any suggestion lacks a span, has a span outside `query`,
|
||||
or overlaps another suggestion's span. A wrong fix is worse than none.
|
||||
- Errors without a suggestion do not block the others: in
|
||||
`created:december 2019 asn:x` the date is quoted and `asn:x` is left as typed.
|
||||
|
||||
`query` must be the exact string that was parsed. See the next section.
|
||||
|
||||
### Which string the spans index
|
||||
|
||||
`wc.parse` has one call site (`_query.py`, in `parse_user_query`), reached only
|
||||
through `TantivyBackend._parse_query`. In every search mode the only string it
|
||||
parses is the `query` request parameter: `_get_tantivy_query_and_mode`
|
||||
(`views.py:324`) uses it as the search itself in `QUERY` mode and as
|
||||
`filter_query` alongside `text`/`title_search`. The simple modes never produce
|
||||
diagnostics. The backend NFC-normalizes before parsing (`_backend.py:485`), so
|
||||
spans index `normalize_search_text(str(params["query"]))`, written with `str()`
|
||||
to match `_get_tantivy_query_and_mode` (bulk `filters` values are not guaranteed
|
||||
to be strings). The views pass that string to the helper. For the NFC input
|
||||
browsers normally send, this is identical to what the user typed.
|
||||
|
||||
### Response and views (`documents/views.py`)
|
||||
|
||||
A search-query 400 becomes:
|
||||
|
||||
```json
|
||||
{
|
||||
"query": [
|
||||
"Invalid date value 'december 2019' for field 'created'.",
|
||||
"Invalid numeric value 'x' for field 'asn'."
|
||||
],
|
||||
"query_errors": [
|
||||
{
|
||||
"message": "Invalid date value 'december 2019' for field 'created'.",
|
||||
"field": "created",
|
||||
"start": 8,
|
||||
"end": 21,
|
||||
"text": "december 2019",
|
||||
"suggestion": "\"december 2019\""
|
||||
},
|
||||
{
|
||||
"message": "Invalid numeric value 'x' for field 'asn'.",
|
||||
"field": "asn",
|
||||
"start": 26,
|
||||
"end": 27,
|
||||
"text": "x",
|
||||
"suggestion": null
|
||||
}
|
||||
],
|
||||
"suggested_query": "created:\"december 2019\" asn:x"
|
||||
}
|
||||
```
|
||||
|
||||
- `query`: unchanged.
|
||||
- `query_errors`: always present on a search-query 400.
|
||||
- `suggested_query`: string or `null`, always present.
|
||||
- `QueryTooLongError` (raised in `_get_tantivy_query_and_mode` before parsing,
|
||||
for an over-long `text`, `title_search` or `query`): one entry with `field`,
|
||||
`start`, `end`, `text` and `suggestion` all `null`, and
|
||||
`suggested_query: null`.
|
||||
- A `SearchQueryError` on the `more_like_id` path (both try blocks catch it
|
||||
there too) gets the same shape with `query` treated as `""`, so every entry
|
||||
has `text: null`.
|
||||
|
||||
DRF's `ValidationError` cannot carry this. `_get_error_details` runs every leaf
|
||||
through `force_str` (`rest_framework/exceptions.py:39`), so `start: 8` would
|
||||
serialize as `"8"` and `None` as `"None"`. Add a small `APIException` subclass:
|
||||
|
||||
```python
|
||||
class SearchQueryValidationError(APIException):
|
||||
status_code = status.HTTP_400_BAD_REQUEST
|
||||
|
||||
def __init__(self, e: SearchQueryError, query: str) -> None:
|
||||
errors, suggested = search_query_error_details(e, query)
|
||||
detail: dict[str, object] = {
|
||||
"query": search_query_error_messages(e),
|
||||
"query_errors": errors,
|
||||
"suggested_query": suggested,
|
||||
}
|
||||
# Assigned directly, not via super().__init__(detail), so DRF does not
|
||||
# stringify the ints and nulls.
|
||||
self.detail = detail
|
||||
```
|
||||
|
||||
DRF's default `exception_handler` returns a dict `detail` as the response body
|
||||
unchanged, and paperless configures no custom `EXCEPTION_HANDLER`. The `query`
|
||||
list serializes to the same JSON as today's `ValidationError` (`ErrorDetail` is
|
||||
a `str` subclass). Where the class lives (`documents/views.py` beside its two
|
||||
users, or a small module the views import) is a plan-level choice.
|
||||
|
||||
Both call sites switch to it, each a one-line change with no signature changes:
|
||||
|
||||
- the document search list endpoint (`views.py:2771`);
|
||||
- `_get_search_document_ids` in `DocumentSelectionMixin` (`views.py:2930`),
|
||||
reached through `_resolve_document_ids` by bulk edit, bulk download and the
|
||||
other selection endpoints. It raises from inside a helper, which is why this
|
||||
is an exception rather than a returned `Response`. It already has `filters`,
|
||||
the params dict to read `query` from.
|
||||
|
||||
### Frontend compatibility
|
||||
|
||||
The current frontend does not read `query` specifically. Its error handler
|
||||
(`document-list-view.service.ts:391-403`) loops over every key of a 400 body and
|
||||
prints `key: value[0]`. Against the new body it would print
|
||||
`query_errors: [object Object]` and the first character of `suggested_query`,
|
||||
and on `suggested_query: null` it throws a `TypeError` inside the handler, so
|
||||
the banner never appears.
|
||||
|
||||
The backend change is therefore not merged on its own. The branch is shared
|
||||
with a co-maintainer who adapts that handler (at minimum, stop treating the two
|
||||
new keys as generic field errors) and builds the UI from "Consumer notes"
|
||||
before the branch merges.
|
||||
|
||||
## Consumer notes (for the frontend, not implemented here)
|
||||
|
||||
What the payload enables, so the UI work can be scheduled with its owner:
|
||||
|
||||
- With `MultipleSearchQueryErrors` only the first message reaches the user
|
||||
today (`fieldError[0]`). The UI can list every `query_errors[].message`.
|
||||
- Show the offending part with `query_errors[].text`. Do not slice the query
|
||||
with `start`/`end` in JavaScript: they are code-point offsets and JS strings
|
||||
index in UTF-16 units.
|
||||
- When `suggested_query` is set, offer "Search for `<suggested_query>`
|
||||
instead", which replaces the `FILTER_FULLTEXT_QUERY` rule value and calls
|
||||
`setFilterRules(rules, true)`.
|
||||
- Global search (`/api/search/`) runs `SearchMode.TEXT` (`views.py:3829`),
|
||||
never produces diagnostics, and is unaffected.
|
||||
|
||||
## Testing
|
||||
|
||||
Pytest-style, grouped in classes, with GIVEN/WHEN/THEN docstrings and
|
||||
`pytest.param` ids, marked `search` (and `django_db` where needed).
|
||||
|
||||
- **Helper unit tests** (new `src/documents/tests/search/test_error_details.py`),
|
||||
built from hand-constructed errors:
|
||||
- single error with a suggestion: entry fields, `text`, and spliced
|
||||
`suggested_query`;
|
||||
- two suggestions in one query: both applied, right to left;
|
||||
- one fixable and one unfixable error: only the fixable span changes;
|
||||
- missing span, out-of-range span, overlapping spans: `suggested_query` is
|
||||
`None` and out-of-range `text` is `None`;
|
||||
- no suggestions: `suggested_query` is `None`;
|
||||
- non-ASCII before the span (an astral character): `text` is the right
|
||||
substring;
|
||||
- `QueryTooLongError`: one entry, all-null location;
|
||||
- `MultipleSearchQueryErrors` order matches `search_query_error_messages`.
|
||||
- **Plumbing** (extend `src/documents/tests/search/test_error_routing.py`):
|
||||
- `_single_diagnostic_to_error` and `_map_emit_error` copy `field`,
|
||||
`startchar`, `endchar` and `suggestion` onto the error for each branch;
|
||||
- end to end through `parse_user_query` with the `query_index` fixture:
|
||||
`created:december 2019 asn:x` yields the spans and single suggestion probed
|
||||
above, and `title:[a TO b]` yields an emit-time error with `field` and a
|
||||
span. These pin the whoosh-compat contracts paperless depends on, so an
|
||||
upgrade that changes them fails loudly.
|
||||
- **API** (extend `src/documents/tests/test_api_search_errors.py`, which already
|
||||
covers the list endpoint and the selection endpoints pytest-style):
|
||||
- list endpoint: 400 with `query` identical to today, `query_errors` entries
|
||||
with integer (not string) `start`/`end`, `text`, JSON `null`s, and the
|
||||
expected `suggested_query`;
|
||||
- a query with no possible fix: `suggested_query` is `null`;
|
||||
- `query` sent alongside `text=`: spans index the `query` param;
|
||||
- over-long query: all-null entry;
|
||||
- a selection endpoint (bulk edit): same shape.
|
||||
|
||||
## Risks / notes
|
||||
|
||||
- Span correctness depends on passing the exact parsed string. The helper's
|
||||
bounds check and the end-to-end tests guard against drift; a mismatch yields
|
||||
`text: null` / `suggested_query: null`, never a corrupted query.
|
||||
- `suggestion` and the emit-time span are whoosh-compat contracts (`Diagnostic`
|
||||
docstring, README host contract). The end-to-end tests are the upgrade
|
||||
tripwire.
|
||||
- Until the frontend half lands on the branch, the document list's search error
|
||||
banner is broken there (see "Frontend compatibility"). Expected on a shared
|
||||
branch; it must not merge in that state.
|
||||
- Third-party clients that iterate every key of a 400 body the way paperless's
|
||||
own frontend does would see the new keys. Clients that read `query` are
|
||||
unaffected. Note the new keys in the release notes.
|
||||
|
||||
## Future work
|
||||
|
||||
- A live `/api/search/validate/` endpoint so the filter editor can mark spans
|
||||
and offer fixes while the user types.
|
||||
- More suggestions in whoosh-compat (for example comparison operators to range
|
||||
syntax), which this payload surfaces without paperless changes.
|
||||
Reference in new issue
Block a user