diff --git a/.claude/skills/whoosh-compat-transition/SKILL.md b/.claude/skills/whoosh-compat-transition/SKILL.md index 1430225dc..aebabe168 100644 --- a/.claude/skills/whoosh-compat-transition/SKILL.md +++ b/.claude/skills/whoosh-compat-transition/SKILL.md @@ -17,8 +17,10 @@ whoosh-compat (github.com/stumpylog/whoosh-compat; local checkout usually at `.. - **Diagnostics before emit:** `whoosh_compat.parse()` never raises on bad input. Check `ParseResult.diagnostics` and map to `SearchQueryError`/`InvalidDateQuery` (HTTP 400) BEFORE calling `emit()`; also catch the emitter's `UnsupportedQueryError` into a 400. Never carry forward the legacy raw-string fallback (`except Exception: query_str = raw_query`) into the new path; it masks integration bugs. - **Build typed errors from structured diagnostic data, never by parsing `message`.** Each `Diagnostic` carries `kind`, `startchar`/`endchar`, and `field`/`raw_value`. `message` is human-readable text whose wording can change. For a range that fails on one bound, `raw_value` is the bound that actually failed. - `diagnostic.field` is a **`FieldRef`**, not a string: use `str(diagnostic.field)` for the canonical dotted name (`created`, `notes.user`) or `diagnostic.field.name` for the field alone. Note the name is canonical, so an aliased query (`type:`) reports the field it resolves to (`document_type`), and the diagnostic span covers the offending value rather than the field name, so the text the user typed for the field is not recoverable. -- **The registry has one resolver.** `registry.make_ref(raw)` turns a raw field string into a `FieldRef` or `None` for an unknown field, and `registry.resolve(ref)` returns the spec. There is no `resolve_json()`; a dotted name is interpreted only inside `make_ref`. +- **The registry has one resolver.** `registry.make_ref(raw)` turns a raw field string into a `FieldRef` or `None` for an unknown field, and `registry.resolve(ref)` returns a `ResolvedField | None`, not a bare spec: read `.spec` for the `FieldSpec`, `.json_path` for the subpath (or `None`), `.is_subpath` and `.dotted_name` are convenience properties. There is no `resolve_json()`; a dotted name is interpreted only inside `make_ref`. Write `resolved = registry.resolve(ref)` then `resolved.spec.kind`, not `spec = registry.resolve(ref)` then `spec.kind`. - **`notes` and `custom_fields` are JSON fields** with fixed subpaths (`notes.user`/`notes.note`, `custom_fields.name`/`custom_fields.value`); the registry stays a static, language-keyed singleton, never per-request. +- **`emit()`'s signature is `emit(node, *, index, registry)`, with no `schema` parameter.** Do not write a call site passing `schema=`. `emit()` calls the library's own `analyze()` pipeline stage internally (token analysis, multitoken resolution, zero-token drop), so paperless-ngx never needs to call `analyze()` itself. +- **A wildcard/prefix pattern on a JSON subpath reports a parse-time diagnostic**, not a silent whole-field query: `custom_fields.value:abc*` reports `DiagnosticKind.UNSUPPORTED_PATTERN` (the same kind used for a wildcard on a numeric or BOOLEAN_EXISTS field) instead of matching against the wrong encoded bytes. Relevant here because `custom_fields.value` is exactly the kind of JSON subpath a user might expect to pattern-match; the error-mapping code needs a case for `UNSUPPORTED_PATTERN`, not just `BAD_DATE`/`BAD_NUMBER`. ## Mandatory before deleting old code @@ -42,15 +44,17 @@ Added: - One `Multitoken` case nested inside a top-level `OR` (whoosh-compat DIVERGENCES entry on Multitoken.DEFAULT) to prove it does not matter for paperless's data. - If acceptance work surfaces a new whoosh-compat divergence, that is a whoosh-compat-repo change (its `differential-triage` skill applies), not a silent paperless workaround. -## Do not mark the JSON fields fast +## Fast JSON field existence checks -`notes` and `custom_fields` must stay non-fast for now. Existence checks against a **fast** JSON field currently return inverted results (a document that has the field is reported as not having it), because the underlying call does not check subpath columns by default. The unsupported-configuration error raised for a non-fast JSON field advises marking it fast, which walks directly into that bug. Ignore that advice until the upstream issue is fixed, then re-check. +Existence checks against a fast JSON field work correctly, both whole-field (`notes:*`, which internally requires `json_subpaths=True`) and subpath-scoped (`custom_fields.value:*`, which checks only that subpath's own fast column). Both are covered by whoosh-compat's own test suite; see `DIVERGENCES.md` entry 20 for the exists-strategy design and its subpath-scoping note. + +Whether to mark `notes`/`custom_fields` fast is a paperless-ngx-side tradeoff (fast fields cost index size/build time for cheaper existence/range queries) independent of whoosh-compat's correctness — worth a maintainer decision, not assumed by this document. ## Coordination - whoosh-compat is pre-1.0: pin an exact version or git SHA; upgrades are deliberate, reviewed changes. - JSON subpath emission depends on the installed tantivy-py version (fallback until quickwit-oss/tantivy-py#716 ships). The whoosh-compat repo has a `carve-out-retirement` skill; coordinate tantivy pin bumps with it, in a separate PR from the parser migration. -- Rollout: settings flag defaulting to the legacy path plus shadow-compare logging (log when old and new paths return different ID sets; sample if cost matters) for one release; delete `_translate.py`/`_dates.py` only after the flag defaults to the new path with no material reports. +- Rollout: no feature flag, no shadow-compare period. Safety comes from the date-grammar parity audit and the result-level acceptance corpus instead; `_translate.py`/`_dates.py` are deleted once those are green. ## Common mistakes diff --git a/docs/superpowers/plans/2026-08-07-whoosh-compat-transition.md b/docs/superpowers/plans/2026-08-07-whoosh-compat-transition.md index 734834e1b..61117eacd 100644 --- a/docs/superpowers/plans/2026-08-07-whoosh-compat-transition.md +++ b/docs/superpowers/plans/2026-08-07-whoosh-compat-transition.md @@ -2,21 +2,21 @@ > **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. -> **Parts of this plan call an API that has moved.** Read the status banner at -> the top of the spec before starting; it lists what changed, what is decided -> but not yet implemented, and the one question still open. Known-stale code -> in this plan, to fix on sight rather than copy: -> -> - `registry.resolve_json(...)` no longer exists. Tasks asserting on it (the -> registry JSON-subpath tests) must use `registry.make_ref(raw)`, which -> returns a `FieldRef` or `None`, and `registry.resolve(ref)` for the spec. -> A `None` from `make_ref` is how an unknown field or unknown subpath now -> reports itself. -> - `InvalidDateQuery(d.field, ...)` passes a `FieldRef` where a name is -> expected. Use `str(d.field)` or `d.field.name`. -> - `emit(..., schema=...)` may lose its `schema` parameter; check -> [#27](https://github.com/stumpylog/whoosh-compat/issues/27) before writing -> those call sites. +> **API reference used by this plan.** The design spec's API reference +> (top of `docs/superpowers/specs/2026-08-07-whoosh-compat-transition-design.md`) +> has the full shape; the points that matter most while executing the tasks +> below: the one resolver is `registry.make_ref(raw) -> FieldRef | None` (a +> `None` return is how an unknown field or unknown subpath reports itself) +> plus `registry.resolve(ref) -> ResolvedField | None` — read `.spec` off the +> result for the `FieldSpec`, `.json_path` for the subpath. `Diagnostic.field` +> is a `FieldRef`, not a string: call `str(d.field)` (or `d.field.name`) +> before passing it to `InvalidDateQuery`/`InvalidNumberQuery`, whose own +> constructors expect `str | None`. `emit()`'s signature is `emit(node, *, +index, registry)`, with no `schema` parameter: every `tantivy_emit(...)` +> call site in this plan drops `schema=...`. `DiagnosticKind` has four +> members: `BAD_DATE`, `BAD_NUMBER`, `TOO_DEEP`, `UNSUPPORTED_PATTERN` (the +> last fires for a wildcard/prefix pattern on a numeric field, a +> `BOOLEAN_EXISTS` field, or a JSON subpath, e.g. `custom_fields.value:abc*`). > > Verify against the whoosh-compat checkout rather than this plan wherever the > two disagree. The library is the source of truth. @@ -451,74 +451,97 @@ class TestFieldRegistry: def test_type_alias_resolves_to_document_type(self) -> None: registry = get_field_registry(None) - spec = registry.resolve("type") - assert spec is not None - assert spec.name == "document_type" + ref = registry.make_ref("type") + assert ref is not None + resolved = registry.resolve(ref) + assert resolved is not None + assert resolved.spec.name == "document_type" def test_path_alias_resolves_to_storage_path(self) -> None: registry = get_field_registry(None) - spec = registry.resolve("path") - assert spec is not None - assert spec.name == "storage_path" + ref = registry.make_ref("path") + assert ref is not None + resolved = registry.resolve(ref) + assert resolved is not None + assert resolved.spec.name == "storage_path" def test_notes_json_subpaths_resolve(self) -> None: registry = get_field_registry(None) - resolved = registry.resolve_json("notes.user") + ref = registry.make_ref("notes.user") + assert ref is not None + resolved = registry.resolve(ref) assert resolved is not None - spec, subpath = resolved - assert spec.name == "notes" - assert subpath == "user" + assert resolved.spec.name == "notes" + assert resolved.json_path == "user" + assert resolved.is_subpath is True def test_custom_fields_json_subpaths_resolve(self) -> None: registry = get_field_registry(None) - assert registry.resolve_json("custom_fields.name") is not None - assert registry.resolve_json("custom_fields.value") is not None + for raw in ("custom_fields.name", "custom_fields.value"): + ref = registry.make_ref(raw) + assert ref is not None + assert registry.resolve(ref) is not None def test_unregistered_json_subpath_does_not_resolve(self) -> None: registry = get_field_registry(None) - assert registry.resolve_json("notes.bogus") is None + # An unregistered subpath is not even a valid FieldRef: make_ref + # returns None for a dotted name whose subpath isn't registered + # (it doesn't produce a ref for resolve() to then reject). + assert registry.make_ref("notes.bogus") is None def test_tag_is_comma_values(self) -> None: registry = get_field_registry(None) - spec = registry.resolve("tag") - assert spec is not None - assert spec.comma_values is True + ref = registry.make_ref("tag") + assert ref is not None + resolved = registry.resolve(ref) + assert resolved is not None + assert resolved.spec.comma_values is True def test_created_is_date_kind(self) -> None: registry = get_field_registry(None) - spec = registry.resolve("created") - assert spec is not None - assert spec.kind is FieldKind.DATE - assert spec.date_only is True + ref = registry.make_ref("created") + assert ref is not None + resolved = registry.resolve(ref) + assert resolved is not None + assert resolved.spec.kind is FieldKind.DATE + assert resolved.spec.date_only is True def test_analyzer_lowercases_and_ascii_folds(self) -> None: # title uses the paperless_text analyzer: simple -> remove_long -> # lowercase -> ascii_fold [-> stemmer]. With no language configured # (None), no stemmer runs, so "Café" folds to the single token "cafe". registry = get_field_registry(None) - spec = registry.resolve("title") - assert spec is not None - assert spec.analyzer is not None - assert spec.analyzer("Café") == ["cafe"] + ref = registry.make_ref("title") + assert ref is not None + resolved = registry.resolve(ref) + assert resolved is not None + assert resolved.spec.analyzer is not None + assert resolved.spec.analyzer("Café") == ["cafe"] def test_checksum_analyzer_is_identity_single_token(self) -> None: # checksum uses the raw tokenizer at index time (no splitting). registry = get_field_registry(None) - spec = registry.resolve("checksum") - assert spec is not None - assert spec.analyzer("ABC-123") == ["ABC-123"] + ref = registry.make_ref("checksum") + assert ref is not None + resolved = registry.resolve(ref) + assert resolved is not None + assert resolved.spec.analyzer("ABC-123") == ["ABC-123"] def test_pattern_normalizer_is_ascii_fold_only_no_stemming(self) -> None: registry = get_field_registry(None) - spec = registry.resolve("title") - assert spec is not None - assert spec.pattern_normalizer is not None + ref = registry.make_ref("title") + assert ref is not None + resolved = registry.resolve(ref) + assert resolved is not None + assert resolved.spec.pattern_normalizer is not None # "running" must NOT be stemmed to "run" by the pattern normalizer, # only case/accent-folded — even with English stemming configured. registry_en = get_field_registry("en") - spec_en = registry_en.resolve("title") - assert spec_en is not None - assert spec_en.pattern_normalizer("Running") == "running" + ref_en = registry_en.make_ref("title") + assert ref_en is not None + resolved_en = registry_en.resolve(ref_en) + assert resolved_en is not None + assert resolved_en.spec.pattern_normalizer("Running") == "running" def test_registry_is_cached_per_language(self) -> None: a = get_field_registry("en") @@ -685,7 +708,7 @@ git commit -m "test(search): guard JSON subpath/dict-key coupling between _field ### Task 6: Write `test_date_grammar_parity.py` -**Suggested executor:** `agentType: python-pro`, `model: sonnet` — correctness-critical, comparing two independent date-computation code paths; needs care transcribing every keyword/unit exactly. +**Suggested executor:** `agentType: python-pro`, `model: sonnet` — needs care transcribing every keyword/unit exactly, and reading the grammar closely enough to phrase each case correctly. **Files:** @@ -693,18 +716,18 @@ git commit -m "test(search): guard JSON subpath/dict-key coupling between _field **Interfaces:** -- Consumes: `_DATE_KEYWORDS` from `documents.search._dates`; `_UNIT_ALIASES`, `_bound_datetimes`, `translate_scalar`, `translate_range` from `documents.search._translate` (both still present at this point in the plan — deleted only in Task 14); `get_field_registry` from `documents.search._registry` (Task 4); `wc.parse` from `whoosh_compat`. +- Consumes: `_DATE_KEYWORDS` from `documents.search._dates`; `_UNIT_ALIASES` from `documents.search._translate` (both still present at this point in the plan — deleted only in Task 14); `get_field_registry` from `documents.search._registry` (Task 4); `wc.parse` from `whoosh_compat`. - [ ] **Step 1: Write the test** -This test has no "make it pass" implementation step of its own — it's a differential test against two systems that already exist. Read `src/documents/search/_dates.py` and `src/documents/search/_translate.py` in full before writing this (both already read earlier in this session) to transcribe every accepted keyword/unit exactly — do not paraphrase or abbreviate the list. +This test has no "make it pass" implementation step of its own — it's a coverage audit against a system that already exists. Read `src/documents/search/_dates.py` and `src/documents/search/_translate.py` in full before writing this (both already read earlier in this session) to transcribe every accepted keyword/unit exactly — do not paraphrase or abbreviate the list. Every case here asks only "does whoosh-compat accept this input at all" (the real migration-safety question — an existing saved view must not start failing to parse); it never asserts on the bounds or AST shape whoosh-compat parses a keyword to, since that's whoosh-compat's own differential-testing responsibility against a real whoosh oracle, not paperless's to re-verify against the legacy code being deleted. ```python # src/documents/tests/search/test_date_grammar_parity.py -"""Transitional differential test: every date keyword/unit _dates.py and -_translate.py accept today must parse cleanly through whoosh-compat with -matching bounds, before those modules are deleted (Task 14). This test is -deleted in the same task as its oracle — superseded by the permanent +"""Transitional coverage audit: every date keyword/unit _dates.py and +_translate.py accept today must still parse cleanly (no diagnostics) +through whoosh-compat, before those modules are deleted (Task 14). This +test is deleted in the same task as the legacy code it audits — superseded by the permanent result-level acceptance corpus (Task 12). """ @@ -720,7 +743,6 @@ import whoosh_compat as wc from documents.search._dates import _DATE_KEYWORDS from documents.search._registry import get_field_registry from documents.search._translate import _UNIT_ALIASES -from documents.search._translate import _bound_datetimes FROZEN_NOW = datetime(2026, 6, 15, 12, 0, tzinfo=UTC) @@ -832,48 +854,20 @@ def test_range_forms_parse_without_diagnostics(range_query, registry) -> None: tz=UTC, ) assert result.diagnostics == (), f"{range_query!r}: {result.diagnostics}" - - -@pytest.mark.parametrize("keyword", sorted(_DATE_KEYWORDS)) -def test_keyword_bounds_match_legacy_translate_scalar(keyword, registry) -> None: - """Cross-check computed bounds, not just successful parse. - - Compares whoosh-compat's resolved DateRange against what - _translate.py's translate_scalar() computes for the same keyword today — - using the legacy code as the oracle while it's still present. - """ - from documents.search import _translate - - with time_machine.travel(FROZEN_NOW, tick=False): - legacy_str = _translate.translate_scalar("created", keyword, UTC) - result = wc.parse( - f'created:"{keyword}"' if " " in keyword else f"created:{keyword}", - registry=registry, - default_fields=["content"], - tz=UTC, - ) - assert result.diagnostics == () - # A single fielded date term parses straight to a top-level DateRange — - # confirmed directly: `wc.parse("created:today", ...).ast` returns - # `DateRange(startchar=8, endchar=13, field='created', lo=..., hi=..., - # incl_lo=True, incl_hi=False)` with no wrapping node, and the same - # holds for a quoted multi-word keyword like "previous week". - date_range = result.ast - # Extract the ISO bounds the legacy string encodes, e.g. - # "created:[2026-06-15T00:00:00Z TO 2026-06-16T00:00:00Z}" -> both ends. - # Confirmed format directly: translate_scalar("created", "today", UTC) - # returns 'created:[2026-08-09T00:00:00Z TO 2026-08-10T00:00:00Z}' (the - # half-open '}' close, not ']' - the regex below already handles both). - import re - - m = re.search(r"\[(\S+) TO (\S+)[\]}]", legacy_str) - assert m is not None, f"unexpected legacy format: {legacy_str!r}" - legacy_lo = datetime.fromisoformat(m.group(1).replace("Z", "+00:00")) - legacy_hi = datetime.fromisoformat(m.group(2).replace("Z", "+00:00")) - assert date_range.lo == legacy_lo, (keyword, date_range.lo, legacy_lo) - assert date_range.hi == legacy_hi, (keyword, date_range.hi, legacy_hi) ``` +Coverage only, deliberately: each test above asks "does whoosh-compat +accept this keyword/form at all" (the real migration-safety question — +does an existing saved view stop parsing), never "does it compute the +same bounds/AST shape whoosh-compat would compute on its own." The +latter is whoosh-compat's own differential-testing responsibility +against a real whoosh oracle (`tests/differential/`, +`tests/test_parser_dates.py` in that repo), not something to re-verify +here against `_translate.py` as a second, weaker oracle. If a specific +keyword's actual search _behavior_ needs confidence beyond "it parses," +express that as a real-document, matched-ID case in the Task 12 +acceptance corpus instead. + - [ ] **Step 2: Run the test suite and record results** Run: `cd src && uv run pytest documents/tests/search/test_date_grammar_parity.py -v --override-ini="addopts="` @@ -1102,40 +1096,35 @@ git commit -m "refactor(search): move SearchQueryError family to _query.py, add --- -### Task 9: Verify whoosh-compat issue #1 has landed +### Task 9: Confirm `Diagnostic.field`/`raw_value` shape before wiring error mapping -**Suggested executor:** `agentType: general-purpose`, `model: haiku` — a status check, not implementation. +**Suggested executor:** `agentType: general-purpose`, `model: haiku` — a status/shape check, not implementation. -**Files:** none in paperless-ngx; read-only check against `/tank/users/trenton/projects/paperless/whoosh-compat` +**Files:** none in paperless-ngx; read-only check against the whoosh-compat checkout. **Interfaces:** N/A — gate for Task 10. -- [ ] **Step 1: Check whether `Diagnostic` has `field`/`raw_value`** +Task 10's error-mapping code needs to match `Diagnostic`'s actual field +shapes exactly, since `field` is a `FieldRef`, not a plain `str | None`. +Confirm both `Diagnostic`'s fields and `DiagnosticKind`'s members directly +against the checkout before writing that code. -Run: `rg -n "class Diagnostic" -A 10 /tank/users/trenton/projects/paperless/whoosh-compat/src/whoosh_compat/errors.py` - -- [ ] **Step 2: If not landed, implement it now (this is [stumpylog/whoosh-compat#1](https://github.com/stumpylog/whoosh-compat/issues/1))** - -Working directory: `/tank/users/trenton/projects/paperless/whoosh-compat`. Add `field: str | None = None` and `raw_value: str | None = None` to the `Diagnostic` dataclass in `src/whoosh_compat/errors.py`. Thread them through at the three construction sites documented in the issue: - -1. `src/whoosh_compat/parser/dateparse.py`'s `_error()` method — add a `field: str` parameter, pass `raw_value=text`; update its one call site in `text_to_node()` (`self._error(node, text)` → `self._error(node, text, field=spec.name)`). -2. `src/whoosh_compat/parser/default.py`'s `term_query()` BAD_NUMBER `Diagnostic(...)` construction — add `field=fieldname, raw_value=text`. -3. `src/whoosh_compat/parser/default.py`'s `_coerce_range_bound()` BAD_NUMBER `Diagnostic(...)` construction — add `field=fieldname, raw_value=text`. - -Follow that repo's own test conventions for this change (add/update unit tests asserting the new fields are populated at each site). Commit there, following that repo's commit style. Close issue #1 with a reference to the commit once merged. - -- [ ] **Step 3: Confirm from the paperless-ngx side** +- [ ] **Step 1: Confirm `Diagnostic`'s fields and `field`'s type** Run: `cd src && uv run python -c " from whoosh_compat.errors import Diagnostic import dataclasses -fields = {f.name for f in dataclasses.fields(Diagnostic)} -assert 'field' in fields and 'raw_value' in fields, fields -print('OK:', fields) +fields = {f.name: f.type for f in dataclasses.fields(Diagnostic)} +print(fields) "` -Expected: prints `OK: {'message', 'kind', 'startchar', 'endchar', 'field', 'raw_value'}` (order may vary) +Expected: a `field` key present. `Diagnostic.field` is typed `FieldRef | None`, not `str | None` — Task 10's `_single_diagnostic_to_error` must call `str(d.field)` (or `d.field.name`) to get a name, never pass the `FieldRef` straight into `InvalidDateQuery`/`InvalidNumberQuery`, whose own constructors expect `str | None`. -No commit in paperless-ngx for this task — it's a prerequisite check/implementation entirely in the other repo. +- [ ] **Step 2: Confirm `DiagnosticKind`'s members** + +Run: `cd src && uv run python -c "from whoosh_compat.errors import DiagnosticKind; print(list(DiagnosticKind))"` +Expected: `BAD_DATE`, `BAD_NUMBER`, `TOO_DEEP`, `UNSUPPORTED_PATTERN`. Task 10's `_single_diagnostic_to_error` branches on `BAD_DATE`/`BAD_NUMBER` and falls through to a generic `SearchQueryError(d.message)` for the other two — confirm that fallthrough is still adequate (or add typed handling) now that `UNSUPPORTED_PATTERN` is reachable for a wildcard on `asn`/`page_count`/`num_notes` or on `custom_fields.value`/`notes.user`-shaped subpaths. + +No commit in paperless-ngx for this task — it's a read-only confirmation gating Task 10. --- @@ -1279,7 +1268,7 @@ def parse_user_query( raise _diagnostics_to_error(result.diagnostics) try: - exact = tantivy_emit(result.ast, index=index, schema=index.schema, registry=registry) + exact = tantivy_emit(result.ast, index=index, registry=registry) except UnsupportedQueryError as e: raise SearchQueryError(str(e)) from e @@ -1317,10 +1306,17 @@ def _diagnostics_to_error(diagnostics: tuple[Diagnostic, ...]) -> SearchQueryErr def _single_diagnostic_to_error(d: Diagnostic) -> SearchQueryError: + # d.field is a FieldRef, not a str: str(d.field) gives the canonical + # dotted name (an aliased query, e.g. type:, reports document_type). + field_name = str(d.field) if d.field is not None else None if d.kind is DiagnosticKind.BAD_DATE: - return InvalidDateQuery(d.field, d.raw_value) + return InvalidDateQuery(field_name, d.raw_value) if d.kind is DiagnosticKind.BAD_NUMBER: - return InvalidNumberQuery(d.field, d.raw_value) + return InvalidNumberQuery(field_name, d.raw_value) + # TOO_DEEP and UNSUPPORTED_PATTERN (e.g. a wildcard on asn/page_count/ + # num_notes, or on a custom_fields.*/notes.* subpath) fall through to + # the generic message; see Task 9's note on whether either warrants its + # own typed subclass. return SearchQueryError(d.message) ``` @@ -1466,7 +1462,6 @@ from datetime import UTC from datetime import datetime import pytest -import tantivy import time_machine from documents.models import CustomField @@ -1475,8 +1470,6 @@ from documents.models import Document from documents.models import Note from documents.search._backend import TantivyBackend from documents.search._query import parse_user_query -from documents.search._schema import build_schema -from documents.search._tokenizer import register_tokenizers pytestmark = [pytest.mark.search, pytest.mark.django_db] @@ -1666,45 +1659,11 @@ class TestMultitokenInNestedOr: backend.add_or_update(doc_b) matched = _matched_ids(backend, 'tag:"multi word tag" OR title:B') assert matched == {doc_a.pk, doc_b.pk} - - -class TestParseUserQueryResultLevel: - """Ported from test_query.py's TestParseUserQuery — see Task 10.""" - - @pytest.fixture - def query_index(self) -> tantivy.Index: - schema = build_schema() - idx = tantivy.Index(schema, path=None) - register_tokenizers(idx, "") - return idx - - @pytest.mark.parametrize( - "raw_query", - [ - pytest.param("created:2020", id="created_year_scalar"), - pytest.param( - "created:[20200101 TO 20201231]", - id="created_8digit_bracket_range", - ), - pytest.param("title:x,created:[2020 TO 2021]", id="title_comma_created_range"), - pytest.param("type:invoice", id="type_alias"), - pytest.param("created:previous week", id="created_previous_week"), - pytest.param( - "created:[2026-01-01T00:00:00Z TO 2026-06-01T00:00:00Z]", - id="created_iso_range", - ), - ], - ) - def test_advanced_search_queries_do_not_raise( - self, - query_index: tantivy.Index, - raw_query: str, - ) -> None: - with time_machine.travel(FROZEN_NOW, tick=False): - assert isinstance(parse_user_query(query_index, raw_query, UTC), tantivy.Query) ``` -The module-level `pytestmark = [pytest.mark.search, pytest.mark.django_db]` (confirmed as the real convention against `test_backend.py`'s identical line) covers every class in the file, including `TestParseUserQueryResultLevel` even though it doesn't touch the ORM — harmless, `django_db` only makes database access available, it doesn't force any given test to use it. +Not ported forward: `test_query.py`'s existing `TestParseUserQuery.test_advanced_search_queries_do_not_raise` (a parametrized `isinstance(..., tantivy.Query)` check over a handful of advanced query shapes) is dropped rather than carried into this module. It never indexes a document or asserts a matched-ID set, so keeping it here would be the one class in this file testing nothing paperless-specific — the underlying guarantee it's checking (a diagnostics-free parse never raises anything but `UnsupportedQueryError` at emit) is whoosh-compat's own tested contract, not paperless's to re-verify with generic query strings. Task 10's kept tests already cover paperless's own specific "does not raise" behaviors (fuzzy mode, dash-query robustness); every other query shape either becomes a real result-level case above or is trusted to whoosh-compat's own suite. + +The module-level `pytestmark = [pytest.mark.search, pytest.mark.django_db]` (confirmed as the real convention against `test_backend.py`'s identical line) covers every class in the file. - [ ] **Step 2: Run the acceptance suite** @@ -1713,7 +1672,7 @@ Expected: PASS, all cases. If `TestIssue13568BracketWildcard` fails, that's the - [ ] **Step 3: Remove the internals-only classes from `test_query.py`** -Per the design spec's test-migration table, delete these classes entirely from `src/documents/tests/search/test_query.py` (they test `_translate.py`/`_dates.py` internals or intermediate query strings, both gone once Task 14 runs): `TestCreatedDateField`, `TestDateTimeFields`, `TestWhooshQueryRewriting`, `TestYearRangeRewriting`, `TestNonDateFieldsNotRewritten`, `TestPassthrough`, `TestNormalizeQuery`. Also remove `TestParseUserQuery`'s `test_advanced_search_queries_do_not_raise` (now duplicated in `test_acceptance.py`'s `TestParseUserQueryResultLevel`) — keep the rest of `TestParseUserQuery` (the exception-path tests from Task 10, `test_returns_tantivy_query`, `test_fuzzy_mode_does_not_raise`, `test_date_rewriting_applied_before_tantivy_parse`, `test_spaced_dash_queries_do_not_raise`, `test_invalid_date_propagates_not_swallowed`) — those aren't duplicated, they cover different behavior (fuzzy mode, dash-query robustness, exception propagation). +Per the design spec's test-migration table, delete these classes entirely from `src/documents/tests/search/test_query.py` (they test `_translate.py`/`_dates.py` internals or intermediate query strings, both gone once Task 14 runs): `TestCreatedDateField`, `TestDateTimeFields`, `TestWhooshQueryRewriting`, `TestYearRangeRewriting`, `TestNonDateFieldsNotRewritten`, `TestPassthrough`, `TestNormalizeQuery`. Also remove `TestParseUserQuery`'s `test_advanced_search_queries_do_not_raise` outright — it never indexed a document or asserted a matched-ID set, checking only that a diagnostics-free parse doesn't raise, which is whoosh-compat's own tested contract, not something paperless needs to re-verify with generic query strings. Keep the rest of `TestParseUserQuery` (the exception-path tests from Task 10, `test_returns_tantivy_query`, `test_fuzzy_mode_does_not_raise`, `test_date_rewriting_applied_before_tantivy_parse`, `test_spaced_dash_queries_do_not_raise`, `test_invalid_date_propagates_not_swallowed`) — those cover paperless-specific behavior (fuzzy mode, dash-query robustness, exception propagation), not generic library coverage. Keep `TestParseSimpleTextHighlightQuery` and `TestPermissionFilter` in `test_query.py` entirely unchanged — neither ever touched `translate_query`. diff --git a/docs/superpowers/specs/2026-08-07-whoosh-compat-transition-design.md b/docs/superpowers/specs/2026-08-07-whoosh-compat-transition-design.md index 7fc2db52f..19848a806 100644 --- a/docs/superpowers/specs/2026-08-07-whoosh-compat-transition-design.md +++ b/docs/superpowers/specs/2026-08-07-whoosh-compat-transition-design.md @@ -1,48 +1,61 @@ # whoosh-compat transition design Date: 2026-08-07 -Status: approved, pending spec review +Status: approved Related skill: `whoosh-compat-transition` -Related issue: [stumpylog/whoosh-compat#1](https://github.com/stumpylog/whoosh-compat/issues/1) -> ## Read this before implementing: parts of this spec describe an API that has moved +> **API reference used by this design.** `FieldRegistry` exposes one +> resolution path: `registry.make_ref(raw) -> FieldRef | None` interprets a +> raw, possibly dotted field string (an unknown field or an unknown subpath +> both return `None`), and `registry.resolve(ref) -> ResolvedField | None` +> looks up the resolved ref. `ResolvedField` carries `.spec` (the +> `FieldSpec`), `.json_path` (the subpath, or `None`), `.is_subpath`, and +> `.dotted_name` — read `resolved.spec.kind`, not `spec.kind` off a bare +> `FieldSpec`. `Diagnostic.field` is a `FieldRef`, not a string: use +> `str(d.field)` for the canonical dotted name, or `d.field.name` for the +> field alone (the name is canonical, so an aliased query like `type:` +> reports `document_type`). Every field-carrying AST leaf holds a `FieldRef`. +> `emit()`'s signature is `emit(node, *, index, registry) -> tantivy.Query`, +> with no `schema` parameter; it calls the library's own `analyze()` pipeline +> stage internally (token analysis, multitoken resolution, zero-token drop) +> before visiting the tree, so this design's call sites never invoke +> `analyze()` themselves. `FieldSpec.subpaths` is stored internally as +> `Mapping[str, SubpathSpec]`, though construction still accepts a plain +> `tuple[str, ...]` as sugar and normalizes it automatically — this design's +> own `PublicField.subpaths: tuple[str, ...]` (below) passes a tuple into +> `FieldSpec(..., subpaths=...)` and needs nothing further. +> `DiagnosticKind` has four members: `BAD_DATE`, `BAD_NUMBER`, `TOO_DEEP`, and +> `UNSUPPORTED_PATTERN`; the error-mapping code below needs cases for all +> four. > -> whoosh-compat changed after this spec was written, and more changes are -> already decided. Re-check anything below against the library's own source -> and `ARCHITECTURE.md` before writing code from it. The library repo is the -> source of truth; this document is not. +> A few library behaviors worth knowing before writing code against it: +> `parse()` validates its own configuration eagerly — an empty or unknown +> `default_fields`, or a `field_boosts` key that resolves to neither a known +> field nor an alias, raises `ValueError` at the `parse()` call itself, and an +> alias in either argument resolves normally. A naive `basedate` is rejected +> (`ValueError`) rather than silently read in the host machine's local +> timezone; pass an aware datetime. A wildcard/prefix pattern on a numeric +> (`U64`) field, a `BOOLEAN_EXISTS` field, or a JSON subpath produces a +> parse-time `Diagnostic(kind=UNSUPPORTED_PATTERN)` instead of silently +> mangling to an exact-match term or matching the wrong encoded bytes — this +> is directly relevant to `custom_fields.value`: a user typing +> `custom_fields.value:abc*` gets a diagnostic, not a query that silently +> matches the wrong documents. A bare JSON field name with no subpath +> (`notes:foo`) demotes to an ordinary text search for the literal string, +> the same treatment an unknown field or unknown subpath gets. Registry +> construction validates its input eagerly: exists-target cycles, empty +> field/alias names, duplicate aliases, dotted canonical names, +> invalid-character or empty JSON subpath strings, and a subpath that would +> shadow a registered plain field are all rejected at `FieldRegistry.__init__` +> with an actionable message, not deferred to query time. > -> **Wrong today, fix on sight:** -> -> | This spec says | Reality now | -> | ---------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -> | `FieldRegistry.resolve_json(dotted)` (§ JSON subpath resolution) | Gone. There is one resolver: `registry.make_ref(raw) -> FieldRef \| None` interprets a dotted name, and `registry.resolve(ref) -> FieldSpec \| None` looks it up. A dot is interpreted only inside `make_ref`. | -> | `InvalidDateQuery(d.field, d.raw_value)` | `Diagnostic.field` is now a `FieldRef`, not a string. Use `str(d.field)` for the canonical dotted name, or `d.field.name`. The name is canonical, so an aliased query (`type:`) reports `document_type`. | -> | AST leaves carrying a field name string | Every field-carrying AST leaf now holds a `FieldRef`. | -> -> **Decided upstream, not yet implemented. Write toward these, they will land before this migration executes:** -> -> | Behavior | Issue | -> | ---------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------- | -> | An empty or unknown `default_fields` raises at `parse()`; a `field_boosts` key naming an alias resolves, one naming nothing raises | [#20](https://github.com/stumpylog/whoosh-compat/issues/20) | -> | A naive `basedate` is rejected rather than read in the host machine's timezone | [#19](https://github.com/stumpylog/whoosh-compat/issues/19) | -> | A wildcard on a numeric field produces a diagnostic instead of failing at search time, so the error mapping gains a case | [#17](https://github.com/stumpylog/whoosh-compat/issues/17) | -> | A bare JSON field name (`notes:foo`) demotes to a text search rather than raising at emit | [#11](https://github.com/stumpylog/whoosh-compat/issues/11) | -> | Registry construction rejects exists-target cycles, empty names, duplicate aliases within a spec, and dotted canonical names | [#21](https://github.com/stumpylog/whoosh-compat/issues/21) | -> -> **Still undecided, do not guess:** whether `emit()` keeps its `schema` -> parameter ([#27](https://github.com/stumpylog/whoosh-compat/issues/27)). -> This spec calls `emit(ast, index=index, schema=schema, registry=...)` in two -> places. Check the issue before writing either call site. -> -> **Trap:** do not add `fast=True` to the `notes` or `custom_fields` JSON -> specs. Existence checks against a fast JSON field currently return inverted -> results ([#7](https://github.com/stumpylog/whoosh-compat/issues/7)), and the -> error raised for a non-fast JSON field advises marking it fast, which walks -> straight into that bug. The field table below correctly leaves them non-fast. -> -> [#28](https://github.com/stumpylog/whoosh-compat/issues/28) tracks all open -> upstream work ordered by effort. +> Fast-field existence checks against a JSON field are correct, both for +> whole-field existence (`notes:*`) and the per-subpath case +> (`custom_fields.value:*`, which checks only that subpath's own fast +> column). Marking `notes`/`custom_fields` fast is therefore a plain +> paperless-ngx-side index-size/query-cost tradeoff, independent of +> whoosh-compat correctness — worth a maintainer decision, not something this +> document settles. ## Summary @@ -78,8 +91,9 @@ ParseResult(ast, diagnostics) │ subclass(es) → HTTP 400 (never just the first diagnostic) │ ▼ -emit(ast, index=index, schema=schema, registry=FIELD_REGISTRY) - │ (raises UnsupportedQueryError → mapped to SearchQueryError → 400, +emit(ast, index=index, registry=FIELD_REGISTRY) + │ (calls whoosh-compat's own analyze() pipeline stage internally, then + │ raises UnsupportedQueryError → mapped to SearchQueryError → 400, │ for constructs that parse but can't execute against tantivy) ▼ tantivy.Query @@ -123,13 +137,12 @@ already proven correct in isolation. `test_translate.py` and the internals-testing classes in `test_query.py`, update `docs/usage.md` and changelog. -**Prerequisite, whoosh-compat repo** (tracked as -[stumpylog/whoosh-compat#1](https://github.com/stumpylog/whoosh-compat/issues/1), -lands before PR 4 starts its diagnostics-mapping work): add `field: str | -None` and `raw_value: str | None` to `Diagnostic`, threaded through at its -three construction sites (`dateparse.py`'s `_error()`, `default.py`'s -`term_query()` and `_coerce_range_bound()`), so paperless can build typed -exceptions without parsing whoosh-compat's human-readable `message` text. +`Diagnostic` carries `field: FieldRef | None` and `raw_value: str | None`, +populated at its construction sites (`dateparse.py`'s `_error()`, +`default.py`'s `BAD_NUMBER` sites), so paperless can build typed exceptions +without parsing whoosh-compat's human-readable `message` text. `field` is a +`FieldRef`, not a plain string; see the API reference at the top of this +document. ## Field surface @@ -220,16 +233,22 @@ knob for DATE fields the way it might look; it only matters in the sense that `created` sets it explicitly for clarity, while `modified`/`added` use `FieldKind.DATETIME` instead of relying on that override. -**`subpaths` stays `tuple[str, ...]`, not a nested structure.** Confirmed -against whoosh-compat's own `FieldRegistry.resolve_json()`: it splits a +**`PublicField.subpaths` stays `tuple[str, ...]`, not a nested structure.** +Confirmed against whoosh-compat's own `FieldRegistry.make_ref()`: it splits a dotted query term on the _first_ dot only and matches the remainder as an exact string against `spec.subpaths` — even the docstring's own `"metadata.author.name"` example is a single opaque string in the tuple, -not a recursive tree. A tuple of strings is exactly as expressive as the -library it feeds; inventing richer structure in `PublicField` now would -just get flattened back to strings at the registry-construction boundary. -Real recursive nesting, if ever needed, is new whoosh-compat capability -first. +not a recursive tree. (`FieldSpec.subpaths` itself now stores a `Mapping[str, +SubpathSpec]` internally, normalized from whatever tuple is passed at +construction; that's an implementation detail of `FieldSpec.__post_init__`, +not something `PublicField`'s own table needs to mirror — passing a plain +tuple into `FieldSpec(..., subpaths=...)` still works exactly as written +here.) A tuple of strings is exactly as expressive as the library it feeds; +inventing richer structure in `PublicField` now would just get flattened +back to strings at the registry-construction boundary. Real recursive +nesting, if ever needed, is new whoosh-compat capability first (the +per-subpath `SubpathSpec` container exists specifically to make that a +later, additive change). **JSON document population stays separate from `subpaths`.** `subpaths` is query-side only — it declares which dotted names are legal to type and @@ -263,7 +282,21 @@ carve-out is self-retiring on whoosh-compat's side once tantivy-py catches up — but the acceptance corpus's `notes.user:`/`custom_fields.name:` cases (PR 4) are exercising that fallback escaping path specifically, not the programmatic path every other field goes through, and that's worth knowing -if one of those cases ever behaves oddly around quoting/escaping. +if one of those cases ever behaves oddly around quoting/escaping. A +multi-token JSON subpath value with `Multitoken.AND`/`OR` now gets correct +combinator semantics through this fallback (each token becomes its own +`index.parse_query()`-backed leaf, `Must`/`Should`-combined normally, +instead of collapsing into one space-joined phrase-shaped query); a genuine +quoted phrase on a JSON subpath still cannot carry an explicit slop through +this fallback (silently ignored, `~N` has no effect) until the carve-out +retires. Also worth knowing given `custom_fields.value` is JSON: this +fallback's `index.parse_query()` call gives a JSON subpath term free +numeric/boolean type inference tantivy's own query grammar provides (a +query like `custom_fields.value:100` matches both a stored JSON number `100` +and a stored JSON string `"100"`); the future programmatic path (once +tantivy-py#716 ships) has no equivalent union and would need this +re-evaluated for numeric/boolean custom field values specifically +(whoosh-compat's `DIVERGENCES.md` entry 22 tracks this open question). **Analyzer wiring**: `FieldSpec.analyzer` reuses the same `tantivy .TextAnalyzer` objects `_tokenizer.py` already builds (`_paperless_text @@ -308,7 +341,7 @@ def parse_user_query(index, raw_query, tz): raise _diagnostics_to_error(result.diagnostics) # ALL diagnostics, not [0] try: - exact = tantivy_emit(result.ast, index=index, schema=index.schema, registry=registry) + exact = tantivy_emit(result.ast, index=index, registry=registry) except UnsupportedQueryError as e: raise SearchQueryError(str(e)) from e @@ -338,10 +371,18 @@ def _diagnostics_to_error(diagnostics: tuple[Diagnostic, ...]) -> SearchQueryErr def _single_diagnostic_to_error(d: Diagnostic) -> SearchQueryError: + # d.field is a FieldRef, not a string: str(d.field) gives the canonical + # dotted name (e.g. "created", "custom_fields.value"); an aliased query + # (type:) reports the field it resolves to (document_type). + field_name = str(d.field) if d.field is not None else None if d.kind is DiagnosticKind.BAD_DATE: - return InvalidDateQuery(d.field, d.raw_value) + return InvalidDateQuery(field_name, d.raw_value) if d.kind is DiagnosticKind.BAD_NUMBER: - return InvalidNumberQuery(d.field, d.raw_value) + return InvalidNumberQuery(field_name, d.raw_value) + # TOO_DEEP (pathological paren nesting) and UNSUPPORTED_PATTERN (a + # wildcard/prefix pattern on a numeric, BOOLEAN_EXISTS, or JSON-subpath + # field) both fall through to the generic message; a typed subclass for + # either isn't warranted unless a caller needs to branch on it. return SearchQueryError(d.message) ``` @@ -359,10 +400,10 @@ except SearchQueryError as e: raise ValidationError({"query": messages}) from e ``` -`d.field`/`d.raw_value` depend on the whoosh-compat prerequisite change -(issue #1) landing first; until then (or if `field`/`raw_value` are `None` -for a given diagnostic kind not yet covered), `_single_diagnostic_to_error` -falls back to `SearchQueryError(d.message)`. +`d.field`/`d.raw_value` are populated for `BAD_DATE` and `BAD_NUMBER` +diagnostics; if either is `None` for a diagnostic kind that doesn't populate +them, `_single_diagnostic_to_error`'s fallthrough to +`SearchQueryError(d.message)` still applies. Deferred, explicitly out of scope for this PR stack: any frontend use of `startchar`/`endchar` (already present on `Diagnostic` today) to highlight @@ -394,12 +435,18 @@ New tests per PR: (`_DATE_KEYWORDS`, all of `_UNIT_ALIASES`'s Whoosh-era abbreviations — `yrs`/`mos`/`wks`/`hrs`/`mins`/`secs` etc. — digit-precision forms, ISO dash forms, `now-7d`/`now+1h`/`now-30m` compact offsets, open/reversed - ranges). Each case parses through - `wc.parse()` against a DATE-kind `FieldRegistry` and asserts no - diagnostics _and_ bounds matching what `_dates.py`/`_translate.py` - compute today, using the still-present legacy code as the oracle. - Deleted again in PR 4 along with that oracle, superseded by the - permanent acceptance corpus. This audit is scoped to _parity_ only — + ranges). Each case parses through `wc.parse()` against a DATE-kind + `FieldRegistry` and asserts no diagnostics come back — a coverage check + only (does whoosh-compat accept this input at all), not a check on the + bounds or AST shape it parses to, which is whoosh-compat's own + differential-testing responsibility against a real whoosh oracle, not + something to re-verify here against `_translate.py` as a second, weaker + oracle. If the team wants confidence that actual search _behavior_ at a + given keyword didn't change, that belongs in the PR 4 result-level + acceptance corpus (real indexed documents at date boundaries, matched-ID + assertions), not an AST/bounds comparison. + Deleted again in PR 4 along with the legacy code it audits, superseded by + the permanent acceptance corpus. This audit is scoped to _parity_ only — whoosh-compat's date grammar is a strict superset of what `_dates.py` accepts today (e.g. `tomorrow`, `now`, `midnight`, `noon`, weekday names like `next monday`), so the migration also grants new date vocabulary for @@ -431,9 +478,9 @@ PR stack — both repos are being actively co-developed. The final swap happens at PR 4: - **Primary plan**: whoosh-compat is released to PyPI around PR 3 (per - your stated intent), assuming the parity audit and issue #1 don't turn - up anything else needing a second round. PR 4 switches to a pinned PyPI - version (`whoosh-compat[tantivy]==X.Y.Z` in `dependencies`, the + your stated intent), assuming the parity audit doesn't turn up anything + needing a second round. PR 4 switches to a pinned PyPI version + (`whoosh-compat[tantivy]==X.Y.Z` in `dependencies`, the `[tool.uv.sources]` override removed entirely). - **Fallback**: if the PyPI release slips past PR 4's start, pin an exact git commit SHA instead (`whoosh-compat[tantivy] @ git+https://github.com/