From 0fb36dae43d82c2bb3e43d9167c0b8a423c3f666 Mon Sep 17 00:00:00 2001 From: stumpylog <797416+stumpylog@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:13:29 -0700 Subject: [PATCH] fix(search): delete regex bare-JSON-prefix rewrite, use default subpaths The regex rewrite that turned bare notes:/custom_fields: prefixes into their subpath spelling was blind to quoting: content:"payment notes: none" was silently rewritten mid-phrase into a notes-field search and matched zero documents. whoosh-compat's FieldSpec now supports a default subpath per JSON field (SubpathSpec(default=True)), which resolves during parsing where quoting is already understood, so the pre-parse string rewrite is no longer needed. Co-Authored-By: Claude Sonnet 5 --- src/documents/search/_fields.py | 13 +- src/documents/search/_query.py | 38 +---- src/documents/tests/search/test_acceptance.py | 71 --------- .../tests/search/test_json_field_prefixes.py | 148 ++++++++++++++++++ 4 files changed, 165 insertions(+), 105 deletions(-) create mode 100644 src/documents/tests/search/test_json_field_prefixes.py diff --git a/src/documents/search/_fields.py b/src/documents/search/_fields.py index cf489a724..709f198e6 100644 --- a/src/documents/search/_fields.py +++ b/src/documents/search/_fields.py @@ -2,6 +2,7 @@ from __future__ import annotations from whoosh_compat import FieldKind from whoosh_compat import FieldSpec +from whoosh_compat import SubpathSpec # Internal-only schema fields with no query-syntax meaning of their own # (sort shadow fields, bigram CJK fields, simple_title/simple_content, @@ -28,6 +29,14 @@ PUBLIC_FIELDS: tuple[FieldSpec, ...] = ( FieldSpec("created", FieldKind.DATE, date_only=True, fast=True), FieldSpec("modified", FieldKind.DATETIME, fast=True), FieldSpec("added", FieldKind.DATETIME, fast=True), - FieldSpec("notes", FieldKind.JSON, subpaths=("user", "note")), - FieldSpec("custom_fields", FieldKind.JSON, subpaths=("name", "value")), + FieldSpec( + "notes", + FieldKind.JSON, + subpaths={"user": SubpathSpec(), "note": SubpathSpec(default=True)}, + ), + FieldSpec( + "custom_fields", + FieldKind.JSON, + subpaths={"name": SubpathSpec(), "value": SubpathSpec(default=True)}, + ), ) diff --git a/src/documents/search/_query.py b/src/documents/search/_query.py index 8a3c67b70..b6809ca65 100644 --- a/src/documents/search/_query.py +++ b/src/documents/search/_query.py @@ -86,32 +86,6 @@ def _quote_date_keyword_phrases(raw_query: str) -> str: ) -# notes:/custom_fields: were valid fielded searches before this migration. -# whoosh-compat's registry only exposes them as JSON subpaths, so a bare -# prefix would demote to an unfielded text search. Rewrite live to the -# equivalent subpath (notes: -> notes.note:, custom_fields: -> -# custom_fields.value:); custom_fields.name: remains available separately. -# Not preceded by a word character or dot, so subpath spellings and words -# merely ending in the prefix are untouched. -_BARE_JSON_PREFIX_RES: Final = ( - (regex.compile(r"(? str: - """Rewrite bare ``notes:``/``custom_fields:`` prefixes to their - subpath equivalents. Prefix substitution only, values untouched. - - Not quote-aware, same accepted trade-off as - _quote_date_keyword_phrases: a literal ``notes:`` inside an existing - quoted phrase on an unrelated field would also get rewritten. - """ - for pattern, replacement in _BARE_JSON_PREFIX_RES: - raw_query = pattern.sub(replacement, raw_query, timeout=_REGEX_TIMEOUT) - return raw_query - - def _user_facing_emit_message(d: Diagnostic) -> str: """A user-safe message for an emit-time QueryError's Diagnostic. @@ -340,12 +314,13 @@ def parse_user_query( """ Parse user query through whoosh-compat, then blend in fuzzy/CJK clauses. - 1. Two small pre-parse rewrites keep historically honored spellings + 1. A small pre-parse rewrite keeps a historically honored spelling working: unquoted multi-word date keyword phrases on date fields - are quoted (_quote_date_keyword_phrases), and bare - notes:/custom_fields: prefixes become their subpath equivalents - (_rewrite_bare_json_field_prefixes). Then wc.parse() against the - shared FieldRegistry (whoosh grammar -> AST). + are quoted (_quote_date_keyword_phrases). Then wc.parse() against + the shared FieldRegistry (whoosh grammar -> AST). Bare + notes:/custom_fields: prefixes resolve to their default subpath + (notes.note:/custom_fields.value:) directly in the registry, via + each JSON field's SubpathSpec(default=True). 2. Any diagnostics (bad dates/numbers) map to SearchQueryError subclasses and raise — the view returns HTTP 400 with every offending field listed, not just the first. @@ -366,7 +341,6 @@ def parse_user_query( """ registry = get_field_registry(settings.SEARCH_LANGUAGE) raw_query = _quote_date_keyword_phrases(raw_query) - raw_query = _rewrite_bare_json_field_prefixes(raw_query) result = wc.parse( raw_query, registry=registry, diff --git a/src/documents/tests/search/test_acceptance.py b/src/documents/tests/search/test_acceptance.py index 625b7914f..22e500504 100644 --- a/src/documents/tests/search/test_acceptance.py +++ b/src/documents/tests/search/test_acceptance.py @@ -354,77 +354,6 @@ class TestUnquotedDateKeywordPhrases: assert _matched_ids(backend, "title:previous month") == {wordy.pk} -class TestBareJsonFieldPrefixes: - """ "notes:foo"/"custom_fields:foo" were valid fielded searches before - this migration. whoosh-compat's registry only exposes them as JSON - subpaths, so parse_user_query rewrites the bare prefixes live: notes: - -> notes.note:, custom_fields: -> custom_fields.value:.""" - - def test_bare_notes_prefix_searches_note_text( - self, - backend: TantivyBackend, - ) -> None: - alice = User.objects.create_user(username="alice") - with_note = Document.objects.create( - title="Has note", - content="x", - checksum="bare-notes-with", - ) - Note.objects.create(document=with_note, user=alice, note="crocodile") - backend.add_or_update(with_note) - # This document's CONTENT contains the words a demoted text search - # would match; it must NOT match once the prefix addresses notes. - _index( - backend, - title="Notes about things", - content="notes crocodile mention", - checksum="bare-notes-decoy", - ) - assert _matched_ids(backend, "notes:crocodile") == {with_note.pk} - - def test_bare_custom_fields_prefix_searches_values( - self, - backend: TantivyBackend, - ) -> None: - field = CustomField.objects.create( - name="Policy Number", - data_type=CustomField.FieldDataType.STRING, - ) - with_value = Document.objects.create( - title="Has field", - content="x", - checksum="bare-cf-with", - ) - CustomFieldInstance.objects.create( - document=with_value, - field=field, - value_text="crocodile", - ) - backend.add_or_update(with_value) - _index( - backend, - title="Custom things", - content="custom fields crocodile", - checksum="bare-cf-decoy", - ) - assert _matched_ids(backend, "custom_fields:crocodile") == {with_value.pk} - - def test_subpath_spellings_are_untouched( - self, - backend: TantivyBackend, - ) -> None: - bob = User.objects.create_user(username="bob") - doc = Document.objects.create( - title="Bob note", - content="x", - checksum="bare-subpath", - ) - Note.objects.create(document=doc, user=bob, note="remark") - backend.add_or_update(doc) - assert _matched_ids(backend, "notes.user:bob") == {doc.pk} - assert _matched_ids(backend, "notes.note:remark") == {doc.pk} - - class TestFieldAliases: """type:/path: are registry aliases for document_type:/storage_path:. The only other alias coverage is parse-shape; these prove resolution diff --git a/src/documents/tests/search/test_json_field_prefixes.py b/src/documents/tests/search/test_json_field_prefixes.py new file mode 100644 index 000000000..e8a62cf6c --- /dev/null +++ b/src/documents/tests/search/test_json_field_prefixes.py @@ -0,0 +1,148 @@ +"""Bare notes:/custom_fields: prefix resolution. + +"notes:foo"/"custom_fields:foo" were valid fielded searches before the +whoosh-compat migration. The registry only exposes them as JSON subpaths, so +each JSON FieldSpec declares a default subpath (SubpathSpec(default=True)): +notes: resolves to notes.note:, custom_fields: resolves to +custom_fields.value:. This replaced an earlier regex-based rewrite +(_rewrite_bare_json_field_prefixes) that ran on the raw query string before +parsing and was blind to quoting, so a phrase like +content:"payment notes: none" was silently corrupted into a notes-field +search and matched nothing. Resolving the default subpath inside the parser +instead means quoting is already understood by the time it happens. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest +from django.contrib.auth.models import User + +from documents.models import CustomField +from documents.models import CustomFieldInstance +from documents.models import Document +from documents.models import Note + +if TYPE_CHECKING: + from documents.search._backend import TantivyBackend + +pytestmark = [pytest.mark.search, pytest.mark.django_db] + + +def _matched_ids(backend: TantivyBackend, query: str) -> set[int]: + return set(backend.search_ids(query, user=None)) + + +def _index(backend: TantivyBackend, **kwargs: object) -> Document: + doc = Document.objects.create(**kwargs) + backend.add_or_update(doc) + return doc + + +class TestBareJsonFieldPrefixes: + def test_bare_notes_prefix_searches_note_text( + self, + backend: TantivyBackend, + ) -> None: + alice = User.objects.create_user(username="alice") + with_note = Document.objects.create( + title="Has note", + content="x", + checksum="bare-notes-with", + ) + Note.objects.create(document=with_note, user=alice, note="crocodile") + backend.add_or_update(with_note) + # This document's CONTENT contains the words a demoted text search + # would match; it must NOT match once the prefix addresses notes. + _index( + backend, + title="Notes about things", + content="notes crocodile mention", + checksum="bare-notes-decoy", + ) + assert _matched_ids(backend, "notes:crocodile") == {with_note.pk} + + def test_bare_custom_fields_prefix_searches_values( + self, + backend: TantivyBackend, + ) -> None: + field = CustomField.objects.create( + name="Policy Number", + data_type=CustomField.FieldDataType.STRING, + ) + with_value = Document.objects.create( + title="Has field", + content="x", + checksum="bare-cf-with", + ) + CustomFieldInstance.objects.create( + document=with_value, + field=field, + value_text="crocodile", + ) + backend.add_or_update(with_value) + _index( + backend, + title="Custom things", + content="custom fields crocodile", + checksum="bare-cf-decoy", + ) + assert _matched_ids(backend, "custom_fields:crocodile") == {with_value.pk} + + def test_subpath_spellings_are_untouched( + self, + backend: TantivyBackend, + ) -> None: + bob = User.objects.create_user(username="bob") + doc = Document.objects.create( + title="Bob note", + content="x", + checksum="bare-subpath", + ) + Note.objects.create(document=doc, user=bob, note="remark") + backend.add_or_update(doc) + assert _matched_ids(backend, "notes.user:bob") == {doc.pk} + assert _matched_ids(backend, "notes.note:remark") == {doc.pk} + + +class TestQuotedPhraseContainingNotesColonIsNotCorrupted: + """The regex rewrite this migration removes was blind to quoting: it + matched "notes:" anywhere in the raw query string, including inside an + already-quoted phrase on an unrelated field, silently turning + content:"payment notes: none" into a notes-field search that matched + nothing. Resolving the default subpath during parsing (which is + quote-aware) fixes this.""" + + def test_quoted_phrase_with_notes_colon_matches_by_content( + self, + backend: TantivyBackend, + ) -> None: + target = _index( + backend, + title="Statement", + content="payment notes: none", + checksum="quoted-phrase-notes-colon", + ) + assert _matched_ids( + backend, + 'content:"payment notes: none"', + ) == {target.pk} + + def test_quoted_phrase_matches_the_same_document_unquoted( + self, + backend: TantivyBackend, + ) -> None: + # Same document, phrasing without the colon: this proves the fix is + # about quote-awareness, not about the words themselves being + # unsearchable. + target = _index( + backend, + title="Statement", + content="payment notes none", + checksum="quoted-phrase-no-colon", + ) + assert _matched_ids( + backend, + 'content:"payment notes none"', + ) == {target.pk}