feat: dedup shared-file parsing/embedding across users in vector sync
A file shared across many users — directly, or via a group folder shared to a group — was parsed and embedded once per user. Chunk point IDs are user-agnostic (uuid5(tenant_id, doc_id=fileid, chunk_index)), but the per-user freshness gate filtered Qdrant by user_id, so two readers ping-ponged: each overwrote the other's points and each kept seeing "not indexed for me", reprocessing every scan. Production telemetry (note 386945, finding #5) measured identical docs re-processed every few hours at 7-13s each, with PDF parse ~62% of per-doc cost. Layer 1 — tenant-wide dedup: - Thread the scanner's tag-REPORT etag into the file DocumentTask and the chunk payload; index `etag` as a KEYWORD field. - vector/sharing_state.find_indexed_content scrolls tenant-wide (no user_id filter) for a non-placeholder point matching (doc_id, doc_type, etag), gated on embedding_identity in Python so a model switch correctly forces a re-embed. - Scanner skips enqueue and the processor skips fetch/parse/embed when a match exists (cross-worker race-guard before WebDAV read). Dedup is fail-safe: a Qdrant error degrades to "process normally". Layer 2 — observed-access ACL (no admin / GroupFolders API needed): - Each point carries `acl_principals` = the set of user:<uid> whose scanner has observed (hence can read) the file. The per-user tag REPORT is the access oracle; group membership/GroupFolders enumeration is admin-only and unavailable in multi-user modes. - build_ownership_filter ORs MatchAny(acl_principals, ["user:<me>"]) so a deduplicated shared/group-folder point surfaces to every reader; verify-on-read (_verify_files) remains the precise ACL gate. - Deletion/eviction become "release one user": drop the principal and delete the points only when the set empties, so one user untagging a shared file doesn't evict it for the others. Legacy points without the field keep the original per-user delete. 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
0919513f21
commit
1c93e7286d
@@ -2,6 +2,7 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
@@ -157,26 +158,32 @@ class TestOwnersCacheBehavior:
|
||||
|
||||
|
||||
class TestBuildOwnershipFilter:
|
||||
@staticmethod
|
||||
def _by_key(flt: Filter) -> dict[str, Any]:
|
||||
assert flt.should is not None
|
||||
return {cond.key: cond for cond in flt.should}
|
||||
|
||||
def test_defaults_to_self_only_when_owners_omitted(self) -> None:
|
||||
flt = build_ownership_filter("alice")
|
||||
|
||||
# Self-only: just the user_id branch. Self is NOT duplicated into an
|
||||
# owner_id branch (the user_id branch already covers self-owned content).
|
||||
assert flt.should is not None
|
||||
assert len(flt.should) == 1
|
||||
(user_branch,) = flt.should
|
||||
assert user_branch.key == "user_id"
|
||||
assert user_branch.match.value == "alice"
|
||||
# Self-only: the user_id branch plus the observed-access acl_principals
|
||||
# branch (so a deduplicated shared file the user has claimed is still
|
||||
# findable). No owner_id branch — self is covered by user_id.
|
||||
branches = self._by_key(flt)
|
||||
assert set(branches) == {"user_id", "acl_principals"}
|
||||
assert branches["user_id"].match.value == "alice"
|
||||
assert branches["acl_principals"].match.any == ["user:alice"]
|
||||
|
||||
def test_expands_owner_branch_with_accessible_owners(self) -> None:
|
||||
flt = build_ownership_filter("alice", ["alice", "bob", "carol"])
|
||||
|
||||
owner_branch, user_branch = flt.should
|
||||
branches = self._by_key(flt)
|
||||
assert set(branches) == {"owner_id", "user_id", "acl_principals"}
|
||||
# Owner branch holds only the OTHER owners — self ("alice") is excluded
|
||||
# because the user_id branch already matches self-owned content.
|
||||
assert set(owner_branch.match.any) == {"bob", "carol"}
|
||||
assert user_branch.key == "user_id"
|
||||
assert user_branch.match.value == "alice"
|
||||
assert set(branches["owner_id"].match.any) == {"bob", "carol"}
|
||||
assert branches["user_id"].match.value == "alice"
|
||||
assert branches["acl_principals"].match.any == ["user:alice"]
|
||||
|
||||
def test_explicit_empty_list_omits_owner_branch_keeps_legacy(self) -> None:
|
||||
# Edge case: caller passed an explicit empty list. The owner_id branch
|
||||
@@ -185,11 +192,9 @@ class TestBuildOwnershipFilter:
|
||||
# user still finds their own content from before the migration.
|
||||
flt = build_ownership_filter("alice", [])
|
||||
|
||||
assert flt.should is not None
|
||||
assert len(flt.should) == 1
|
||||
(user_branch,) = flt.should
|
||||
assert user_branch.key == "user_id"
|
||||
assert user_branch.match.value == "alice"
|
||||
branches = self._by_key(flt)
|
||||
assert set(branches) == {"user_id", "acl_principals"}
|
||||
assert branches["user_id"].match.value == "alice"
|
||||
|
||||
|
||||
class TestBuildBaseFilterConditions:
|
||||
|
||||
@@ -312,8 +312,12 @@ class TestProcessDocumentMetricCounting:
|
||||
)
|
||||
|
||||
qmock = MagicMock()
|
||||
qmock.delete = AsyncMock()
|
||||
with patch.object(proc, "get_qdrant_client", new=AsyncMock(return_value=qmock)):
|
||||
# Deletion now delegates to release_document_for_user (release-one-user
|
||||
# semantics); stub it so the test exercises only the metric accounting.
|
||||
with (
|
||||
patch.object(proc, "get_qdrant_client", new=AsyncMock(return_value=qmock)),
|
||||
patch.object(proc, "release_document_for_user", new=AsyncMock()),
|
||||
):
|
||||
await proc.process_document(task, MagicMock())
|
||||
|
||||
assert metric_sample(
|
||||
@@ -339,8 +343,16 @@ class TestProcessDocumentMetricCounting:
|
||||
)
|
||||
|
||||
qmock = MagicMock()
|
||||
qmock.delete = AsyncMock(side_effect=RuntimeError("boom"))
|
||||
with patch.object(proc, "get_qdrant_client", new=AsyncMock(return_value=qmock)):
|
||||
# A failed release still counts as a processed (error) delete and must
|
||||
# not touch the indexed counter.
|
||||
with (
|
||||
patch.object(proc, "get_qdrant_client", new=AsyncMock(return_value=qmock)),
|
||||
patch.object(
|
||||
proc,
|
||||
"release_document_for_user",
|
||||
new=AsyncMock(side_effect=RuntimeError("boom")),
|
||||
),
|
||||
):
|
||||
with pytest.raises(RuntimeError):
|
||||
await proc.process_document(task, MagicMock())
|
||||
|
||||
|
||||
@@ -0,0 +1,229 @@
|
||||
"""Unit tests for tenant-wide content dedup + observed-access ACL state.
|
||||
|
||||
Covers vector/sharing_state.py: the tenant-wide content lookup that lets a
|
||||
shared/group-folder file be parsed+embedded once per tenant instead of once per
|
||||
user, and the ``acl_principals`` maintenance (grant/release) that keeps a
|
||||
deduplicated point findable by every reader without re-indexing.
|
||||
|
||||
All functions reach Qdrant via ``get_qdrant_client`` and resolve the collection
|
||||
via ``get_settings``; both are monkeypatched here so the logic is exercised
|
||||
without a live Qdrant.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.vector import payload_keys
|
||||
from nextcloud_mcp_server.vector import sharing_state as ss
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
_COLLECTION = "test_collection"
|
||||
_MODEL = "model-x"
|
||||
|
||||
|
||||
class _Settings:
|
||||
def get_collection_name(self) -> str:
|
||||
return _COLLECTION
|
||||
|
||||
def get_embedding_model_name(self) -> str:
|
||||
return _MODEL
|
||||
|
||||
|
||||
def _point(payload: dict) -> SimpleNamespace:
|
||||
"""Stand-in for a qdrant_client Record (only id/payload are read)."""
|
||||
return SimpleNamespace(id="pt", payload=payload)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def client(monkeypatch) -> AsyncMock:
|
||||
"""An AsyncMock Qdrant client wired into sharing_state, with a stub Settings.
|
||||
|
||||
``scroll`` defaults to "no points"; individual tests override
|
||||
``client.scroll.return_value``/``side_effect``.
|
||||
"""
|
||||
qc = AsyncMock()
|
||||
qc.scroll.return_value = ([], None)
|
||||
monkeypatch.setattr(ss, "get_qdrant_client", AsyncMock(return_value=qc))
|
||||
monkeypatch.setattr(ss, "get_settings", lambda: _Settings())
|
||||
return qc
|
||||
|
||||
|
||||
def _must_keys(flt) -> list[str | None]:
|
||||
"""Collect the FieldCondition keys in a Filter's ``must`` clause."""
|
||||
return [getattr(c, "key", None) for c in (flt.must or [])]
|
||||
|
||||
|
||||
class TestFindIndexedContent:
|
||||
async def test_returns_payload_on_etag_and_model_match(self, client) -> None:
|
||||
payload = {
|
||||
"doc_id": "42",
|
||||
"etag": "abc",
|
||||
payload_keys.EMBEDDING_IDENTITY: _MODEL,
|
||||
ss.ACL_PRINCIPALS_KEY: ["user:alice"],
|
||||
}
|
||||
client.scroll.return_value = ([_point(payload)], None)
|
||||
|
||||
result = await ss.find_indexed_content("42", "file", "abc", _MODEL)
|
||||
assert result == payload
|
||||
|
||||
async def test_none_when_no_points(self, client) -> None:
|
||||
client.scroll.return_value = ([], None)
|
||||
assert await ss.find_indexed_content("42", "file", "abc", _MODEL) is None
|
||||
|
||||
async def test_none_on_embedding_model_mismatch(self, client) -> None:
|
||||
# A model switch overwrites the same point IDs; existing vectors made by
|
||||
# a different model must be re-embedded, so this reports "not indexed".
|
||||
client.scroll.return_value = (
|
||||
[_point({payload_keys.EMBEDDING_IDENTITY: "other-model"})],
|
||||
None,
|
||||
)
|
||||
assert await ss.find_indexed_content("42", "file", "abc", _MODEL) is None
|
||||
|
||||
async def test_empty_etag_short_circuits_without_query(self, client) -> None:
|
||||
assert await ss.find_indexed_content("42", "file", "", _MODEL) is None
|
||||
client.scroll.assert_not_called()
|
||||
|
||||
|
||||
class TestAddPrincipal:
|
||||
async def test_noop_when_principal_already_present(self, client) -> None:
|
||||
added = await ss.add_principal("42", "file", "alice", ["user:alice"])
|
||||
assert added is False
|
||||
client.set_payload.assert_not_called()
|
||||
|
||||
async def test_unions_principal_when_absent(self, client) -> None:
|
||||
added = await ss.add_principal("42", "file", "bob", ["user:alice"])
|
||||
assert added is True
|
||||
client.set_payload.assert_awaited_once()
|
||||
kwargs = client.set_payload.await_args.kwargs
|
||||
assert kwargs["payload"][ss.ACL_PRINCIPALS_KEY] == ["user:alice", "user:bob"]
|
||||
# Updates only real (non-placeholder) chunks of this document.
|
||||
assert _must_keys(kwargs["points"]) == ["doc_id", "doc_type", "is_placeholder"]
|
||||
|
||||
async def test_handles_none_current_principals(self, client) -> None:
|
||||
added = await ss.add_principal("42", "file", "alice", None)
|
||||
assert added is True
|
||||
kwargs = client.set_payload.await_args.kwargs
|
||||
assert kwargs["payload"][ss.ACL_PRINCIPALS_KEY] == ["user:alice"]
|
||||
|
||||
|
||||
class TestClaimExistingIndex:
|
||||
async def test_true_and_grants_principal_on_hit(self, client) -> None:
|
||||
client.scroll.return_value = (
|
||||
[
|
||||
_point(
|
||||
{
|
||||
payload_keys.EMBEDDING_IDENTITY: _MODEL,
|
||||
ss.ACL_PRINCIPALS_KEY: ["user:alice"],
|
||||
}
|
||||
)
|
||||
],
|
||||
None,
|
||||
)
|
||||
claimed = await ss.claim_existing_index("42", "file", "abc", "bob")
|
||||
assert claimed is True
|
||||
# bob was added to the existing point's principals.
|
||||
client.set_payload.assert_awaited_once()
|
||||
assert client.set_payload.await_args.kwargs["payload"][
|
||||
ss.ACL_PRINCIPALS_KEY
|
||||
] == ["user:alice", "user:bob"]
|
||||
|
||||
async def test_false_when_not_indexed(self, client) -> None:
|
||||
client.scroll.return_value = ([], None)
|
||||
assert await ss.claim_existing_index("42", "file", "abc", "bob") is False
|
||||
client.set_payload.assert_not_called()
|
||||
|
||||
async def test_hit_for_already_listed_user_writes_nothing(self, client) -> None:
|
||||
client.scroll.return_value = (
|
||||
[
|
||||
_point(
|
||||
{
|
||||
payload_keys.EMBEDDING_IDENTITY: _MODEL,
|
||||
ss.ACL_PRINCIPALS_KEY: ["user:alice"],
|
||||
}
|
||||
)
|
||||
],
|
||||
None,
|
||||
)
|
||||
# alice already present -> claim still True (skip reprocess) but no write.
|
||||
assert await ss.claim_existing_index("42", "file", "abc", "alice") is True
|
||||
client.set_payload.assert_not_called()
|
||||
|
||||
async def test_lookup_error_degrades_to_process_normally(self, client) -> None:
|
||||
# A Qdrant hiccup during dedup must not abort the scan — fall back to
|
||||
# processing the document (return False), not raise.
|
||||
client.scroll.side_effect = RuntimeError("qdrant down")
|
||||
assert await ss.claim_existing_index("42", "file", "abc", "bob") is False
|
||||
|
||||
async def test_principal_grant_failure_after_hit_is_non_fatal(self, client) -> None:
|
||||
# The content IS indexed (skip reprocess), so a failure to record the
|
||||
# principal still returns True; verify-on-read + next scan reconcile.
|
||||
client.scroll.return_value = (
|
||||
[_point({payload_keys.EMBEDDING_IDENTITY: _MODEL})],
|
||||
None,
|
||||
)
|
||||
client.set_payload.side_effect = RuntimeError("set_payload failed")
|
||||
assert await ss.claim_existing_index("42", "file", "abc", "bob") is True
|
||||
|
||||
|
||||
class TestExistingPrincipals:
|
||||
async def test_returns_recorded_principals(self, client) -> None:
|
||||
client.scroll.return_value = (
|
||||
[_point({ss.ACL_PRINCIPALS_KEY: ["user:alice", "user:bob"]})],
|
||||
None,
|
||||
)
|
||||
assert await ss.existing_principals("42", "file") == ["user:alice", "user:bob"]
|
||||
|
||||
async def test_empty_when_no_points(self, client) -> None:
|
||||
client.scroll.return_value = ([], None)
|
||||
assert await ss.existing_principals("42", "file") == []
|
||||
|
||||
|
||||
class TestReleaseDocumentForUser:
|
||||
async def test_keeps_points_and_trims_principals_when_readers_remain(
|
||||
self, client
|
||||
) -> None:
|
||||
client.scroll.return_value = (
|
||||
[_point({ss.ACL_PRINCIPALS_KEY: ["user:alice", "user:bob"]})],
|
||||
None,
|
||||
)
|
||||
await ss.release_document_for_user("42", "file", "alice")
|
||||
|
||||
client.delete.assert_not_called()
|
||||
kwargs = client.set_payload.await_args.kwargs
|
||||
assert kwargs["payload"][ss.ACL_PRINCIPALS_KEY] == ["user:bob"]
|
||||
|
||||
async def test_deletes_all_points_when_last_reader_released(self, client) -> None:
|
||||
client.scroll.return_value = (
|
||||
[_point({ss.ACL_PRINCIPALS_KEY: ["user:alice"]})],
|
||||
None,
|
||||
)
|
||||
await ss.release_document_for_user("42", "file", "alice")
|
||||
|
||||
client.set_payload.assert_not_called()
|
||||
client.delete.assert_awaited_once()
|
||||
selector = client.delete.await_args.kwargs["points_selector"]
|
||||
# Whole document removed: doc_id + doc_type, no user_id, no placeholder gate.
|
||||
assert _must_keys(selector) == ["doc_id", "doc_type"]
|
||||
|
||||
async def test_legacy_points_without_principals_delete_by_user(
|
||||
self, client
|
||||
) -> None:
|
||||
# Pre-acl_principals points: preserve the original per-user delete.
|
||||
client.scroll.return_value = ([_point({"doc_id": "42"})], None)
|
||||
await ss.release_document_for_user("42", "file", "alice")
|
||||
|
||||
client.set_payload.assert_not_called()
|
||||
selector = client.delete.await_args.kwargs["points_selector"]
|
||||
assert _must_keys(selector) == ["user_id", "doc_id", "doc_type"]
|
||||
|
||||
async def test_no_points_falls_back_to_per_user_delete(self, client) -> None:
|
||||
client.scroll.return_value = ([], None)
|
||||
await ss.release_document_for_user("42", "file", "alice")
|
||||
|
||||
selector = client.delete.await_args.kwargs["points_selector"]
|
||||
assert _must_keys(selector) == ["user_id", "doc_id", "doc_type"]
|
||||
Reference in New Issue
Block a user