feat(search): ADR-027 Phase 1 — modified-date range filter
Add a modified_after/modified_before date-range filter to semantic search, honoured on both the MCP tool path (BM25HybridSearchAlgorithm) and the dense-only visualization/API path (SemanticSearchAlgorithm) through one shared contract. - Promote modified_after/modified_before to explicit keyword params on the SearchAlgorithm ABC and both concrete algorithms; factor the shared placeholder+ownership+doc_type+date filter into access_filter.build_base_filter_conditions so new filters land in one place. - nc_semantic_search: accept RFC 3339 / ISO 8601 (or Unix seconds) bounds via utils.validation.parse_modified_timestamp; Annotated/Field constraints on the numeric args; explicit McpError guard for after > before. Thread the parsed bounds through the cross-app and per-doc_type dispatch. - /api/v1 search endpoints + viz route parse the same formats and 400 on bad or inverted ranges. - Add a modified_at INTEGER payload index to _PAYLOAD_INDEX_FIELDS; the idempotent _ensure_payload_indexes() startup path migrates existing collections with no content re-index. - Update ADR-027 to resolve the review feedback (validation placement, shared algorithm contract, deferral of nc_semantic_search_answer, payload index, RFC-3339-at-the-boundary rationale). Add unit tests. Refs ADR-027. Deck #177. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
f6ab04b2d9
commit
c2c8dc1a08
@@ -5,9 +5,11 @@ from __future__ import annotations
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
from qdrant_client.models import FieldCondition, Range
|
||||
|
||||
from nextcloud_mcp_server.search import access_filter
|
||||
from nextcloud_mcp_server.search.access_filter import (
|
||||
build_base_filter_conditions,
|
||||
build_ownership_filter,
|
||||
clear_accessible_owners_cache,
|
||||
list_accessible_owners,
|
||||
@@ -186,3 +188,75 @@ class TestBuildOwnershipFilter:
|
||||
(user_branch,) = flt.should
|
||||
assert user_branch.key == "user_id"
|
||||
assert user_branch.match.value == "alice"
|
||||
|
||||
|
||||
class TestBuildBaseFilterConditions:
|
||||
"""The shared ADR-027 filter contract used by both search algorithms."""
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_minimal_is_placeholder_plus_ownership(self) -> None:
|
||||
# No doc_type, no date bounds -> exactly placeholder + ownership.
|
||||
conditions = build_base_filter_conditions("alice", None)
|
||||
assert len(conditions) == 2
|
||||
# No modified_at Range condition present.
|
||||
assert not any(
|
||||
isinstance(c, FieldCondition) and c.key == "modified_at" for c in conditions
|
||||
)
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_doc_type_appends_match_condition(self) -> None:
|
||||
conditions = build_base_filter_conditions("alice", None, doc_type="note")
|
||||
doc_type_conds = [
|
||||
c
|
||||
for c in conditions
|
||||
if isinstance(c, FieldCondition) and c.key == "doc_type"
|
||||
]
|
||||
assert len(doc_type_conds) == 1
|
||||
assert doc_type_conds[0].match.value == "note"
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.parametrize(
|
||||
"after,before,expected_gte,expected_lte",
|
||||
[
|
||||
(100, 200, 100, 200),
|
||||
(100, None, 100, None), # after-only
|
||||
(None, 200, None, 200), # before-only
|
||||
],
|
||||
)
|
||||
def test_modified_at_range_appended(
|
||||
self, after, before, expected_gte, expected_lte
|
||||
) -> None:
|
||||
conditions = build_base_filter_conditions(
|
||||
"alice", None, modified_after=after, modified_before=before
|
||||
)
|
||||
range_conds = [
|
||||
c
|
||||
for c in conditions
|
||||
if isinstance(c, FieldCondition) and c.key == "modified_at"
|
||||
]
|
||||
assert len(range_conds) == 1
|
||||
rng = range_conds[0].range
|
||||
assert isinstance(rng, Range)
|
||||
assert rng.gte == expected_gte
|
||||
assert rng.lte == expected_lte
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_no_range_when_both_bounds_none(self) -> None:
|
||||
conditions = build_base_filter_conditions(
|
||||
"alice", None, modified_after=None, modified_before=None
|
||||
)
|
||||
assert not any(
|
||||
isinstance(c, FieldCondition) and c.key == "modified_at" for c in conditions
|
||||
)
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_all_filters_compose(self) -> None:
|
||||
# placeholder + ownership + doc_type + modified_at range = 4 conditions.
|
||||
conditions = build_base_filter_conditions(
|
||||
"alice",
|
||||
["alice", "bob"],
|
||||
doc_type="file",
|
||||
modified_after=100,
|
||||
modified_before=200,
|
||||
)
|
||||
assert len(conditions) == 4
|
||||
|
||||
@@ -1,8 +1,13 @@
|
||||
"""Unit tests for shared boundary validators."""
|
||||
|
||||
from datetime import datetime, timezone
|
||||
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id
|
||||
from nextcloud_mcp_server.utils.validation import (
|
||||
is_valid_nextcloud_doc_id,
|
||||
parse_modified_timestamp,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -52,3 +57,62 @@ def test_accepts_positive_ascii_integers(value):
|
||||
def test_rejects_invalid_doc_ids(value, reason):
|
||||
"""Reject empty/zero/leading-zero/non-ASCII/non-digit inputs."""
|
||||
assert is_valid_nextcloud_doc_id(value) is False, f"should reject: {reason}"
|
||||
|
||||
|
||||
# parse_modified_timestamp (ADR-027) ---------------------------------------
|
||||
|
||||
_JAN_2026_UTC = int(datetime(2026, 1, 1, tzinfo=timezone.utc).timestamp())
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.parametrize(
|
||||
"value,expected",
|
||||
[
|
||||
(None, None),
|
||||
("", None),
|
||||
(" ", None),
|
||||
# RFC 3339 / ISO 8601
|
||||
("2026-01-01T00:00:00Z", _JAN_2026_UTC),
|
||||
("2026-01-01T00:00:00+00:00", _JAN_2026_UTC),
|
||||
# +02:00 offset is two hours earlier in UTC
|
||||
("2026-01-01T02:00:00+02:00", _JAN_2026_UTC),
|
||||
# Naive datetime is assumed UTC
|
||||
("2026-01-01T00:00:00", _JAN_2026_UTC),
|
||||
# Date-only ISO form
|
||||
("2026-01-01", _JAN_2026_UTC),
|
||||
# Bare Unix seconds (string and int) pass through
|
||||
(str(_JAN_2026_UTC), _JAN_2026_UTC),
|
||||
(_JAN_2026_UTC, _JAN_2026_UTC),
|
||||
(0, 0),
|
||||
(1767225600.9, 1767225600), # float truncates
|
||||
],
|
||||
)
|
||||
def test_parse_modified_timestamp_accepts(value, expected):
|
||||
"""RFC 3339 strings, Unix seconds, and None normalize to int seconds (UTC)."""
|
||||
assert parse_modified_timestamp(value) == expected
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.parametrize(
|
||||
"value,reason",
|
||||
[
|
||||
("not-a-date", "unparseable string"),
|
||||
("2026-13-01T00:00:00Z", "invalid month"),
|
||||
(-1, "negative int"),
|
||||
(-5.0, "negative float"),
|
||||
(True, "bool is not a timestamp"),
|
||||
(False, "bool is not a timestamp"),
|
||||
([], "wrong type"),
|
||||
],
|
||||
)
|
||||
def test_parse_modified_timestamp_rejects(value, reason):
|
||||
"""Bad formats / negatives / bools raise ValueError (→ McpError or HTTP 400)."""
|
||||
with pytest.raises(ValueError):
|
||||
parse_modified_timestamp(value)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_parse_modified_timestamp_error_names_param():
|
||||
"""The param_name is surfaced in the error for caller-friendly messages."""
|
||||
with pytest.raises(ValueError, match="modified_after"):
|
||||
parse_modified_timestamp("nope", param_name="modified_after")
|
||||
|
||||
@@ -73,6 +73,21 @@ def _record(point_id: int | str, doc_id: int | str | None) -> SimpleNamespace:
|
||||
return SimpleNamespace(id=point_id, payload=payload)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _PAYLOAD_INDEX_FIELDS contract
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_modified_at_indexed_as_integer():
|
||||
"""ADR-027: the date-range filter needs a numeric index on modified_at.
|
||||
|
||||
INTEGER (not FLOAT/DATETIME) because the payload stores an int Unix-second
|
||||
timestamp; a numeric Range filters it without a content re-index.
|
||||
"""
|
||||
assert _PAYLOAD_INDEX_FIELDS.get("modified_at") == PayloadSchemaType.INTEGER
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _ensure_payload_indexes
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user