From ba7921dd4ebd3fe088eaa4f51ee05863b275f8c8 Mon Sep 17 00:00:00 2001 From: Ali Zarinkolah Date: Sat, 22 Aug 2026 15:09:31 +0330 Subject: [PATCH] test(points): cover the read paths and the query service Why: - The isolation and pagination guarantees are the ones that fail silently, so they need tests that would actually notice. Changes: - API tests over real Postgres and Qdrant together, because the invariant worth testing spans both: the tenant Postgres derived is the only one Qdrant is ever queried with. - Pagination holds when a point is inserted behind the cursor mid-listing -- the defect an offset cursor would have. - A route-order guard, since /{point_id} declared first turns count into a 422 and nothing else in the suite would catch it. - Unit tests for the query service against the fake, including the Arabic to Persian letterform fold and raising rather than returning None. Impact: - Suite goes to 325 passed, 3 skipped. Co-Authored-By: Claude Opus 5 --- tests/integration/qdrant/test_points_api.py | 395 ++++++++++++++++++ tests/unit/application/points/test_queries.py | 103 +++++ 2 files changed, 498 insertions(+) create mode 100644 tests/integration/qdrant/test_points_api.py create mode 100644 tests/unit/application/points/test_queries.py diff --git a/tests/integration/qdrant/test_points_api.py b/tests/integration/qdrant/test_points_api.py new file mode 100644 index 0000000..03235f8 --- /dev/null +++ b/tests/integration/qdrant/test_points_api.py @@ -0,0 +1,395 @@ +"""`/v1/points` read paths over HTTP (ADR-0002, ADR-0008). + +Needs both containers: Postgres authenticates the key and derives the tenant, +Qdrant holds the points. Testing them together is the point — the invariant this +file exists for is that the tenant Postgres derived is the only one Qdrant is +ever queried with. + +Everything below goes through the real app: real routing, real `require_scope`, +real error envelope. Only the session factory is overridden, so a route that +forgot its scope or read a tenant from the query string fails here. +""" + +import uuid +from collections.abc import AsyncIterator + +import pytest +import pytest_asyncio +from asgi_lifespan import LifespanManager +from httpx import ASGITransport, AsyncClient +from qdrant_client import AsyncQdrantClient +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from src.bootstrap.dependencies import get_sessionmaker +from src.config import QdrantSettings, Settings +from src.infrastructure.qdrant.collection import ensure_chunks_collection +from src.infrastructure.qdrant.points import QdrantPointStorage +from src.main import create_app +from tests.support.factories import create_api_key, create_tenant +from tests.support.point_contract import SeedSpec, chunk_point_for, point_id_for + +pytestmark = [ + pytest.mark.integration, + pytest.mark.qdrant, + pytest.mark.postgres, + pytest.mark.asyncio(loop_scope="session"), +] + +FILE_ID = uuid.UUID("aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa") +OTHER_FILE_ID = uuid.UUID("bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb") + + +@pytest.fixture +def api_settings(settings: Settings, qdrant_settings: QdrantSettings) -> Settings: + """The unit-test settings with Qdrant pointed at the real container. + + Everything else keeps its closed-port default, so this exercises the point + paths without the lifespan reaching a colleague's Ollama box or OpenAI. + """ + return settings.model_copy(update={"qdrant": qdrant_settings}) + + +@pytest_asyncio.fixture(loop_scope="session") +async def api_client( + api_settings: Settings, db_sessionmaker: async_sessionmaker[AsyncSession] +) -> AsyncIterator[AsyncClient]: + app = create_app(api_settings) + app.dependency_overrides[get_sessionmaker] = lambda: db_sessionmaker + async with ( + LifespanManager(app) as manager, + AsyncClient(transport=ASGITransport(app=manager.app), base_url="http://test") as client, + ): + yield client + + +def _auth(token: str) -> dict[str, str]: + return {"Authorization": f"Bearer {token}"} + + +async def _seed( + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, + specs: tuple[SeedSpec, ...], +) -> None: + await ensure_chunks_collection(qdrant_client, collection=qdrant_settings.collection) + storage = QdrantPointStorage(qdrant_client, collection=qdrant_settings.collection) + await storage.upsert_points([chunk_point_for(spec) for spec in specs]) + + +async def _tenant_with_key( + db_session: AsyncSession, *, scopes: list[str] | None = None +) -> tuple[uuid.UUID, str]: + tenant = await create_tenant(db_session) + _, token = await create_api_key( + db_session, tenant=tenant, scopes=scopes if scopes is not None else ["points:read"] + ) + await db_session.commit() + return tenant.id, token + + +async def test_list_points_returns_a_files_points_in_order_id_order( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + tenant_id, token = await _tenant_with_key(db_session) + await _seed( + qdrant_client, + qdrant_settings, + ( + SeedSpec(tenant_id, FILE_ID, 0, "first", 1.0), + SeedSpec(tenant_id, FILE_ID, 2, "third", 3.0), + SeedSpec(tenant_id, FILE_ID, 1, "second", 2.0), + ), + ) + + response = await api_client.get( + "/v1/points", params={"file_id": str(FILE_ID)}, headers=_auth(token) + ) + + assert response.status_code == 200 + assert [point["content"] for point in response.json()["points"]] == [ + "first", + "second", + "third", + ] + + +async def test_list_points_omits_the_tenant_id_from_the_response( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + """Echoing `tenant_id` back invites clients to start sending it.""" + tenant_id, token = await _tenant_with_key(db_session) + await _seed(qdrant_client, qdrant_settings, (SeedSpec(tenant_id, FILE_ID, 0, "only", 1.0),)) + + response = await api_client.get( + "/v1/points", params={"file_id": str(FILE_ID)}, headers=_auth(token) + ) + + assert "tenant_id" not in response.json()["points"][0] + + +async def test_get_point_returns_404_for_another_tenants_point( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + """`404`, not `403`: the API must not confirm the id exists (ADR-0016).""" + owner_id, _ = await _tenant_with_key(db_session) + _, intruder_token = await _tenant_with_key(db_session) + await _seed(qdrant_client, qdrant_settings, (SeedSpec(owner_id, FILE_ID, 0, "secret", 1.0),)) + + response = await api_client.get( + f"/v1/points/{point_id_for(FILE_ID, 0)}", headers=_auth(intruder_token) + ) + + assert response.status_code == 404 + assert response.json()["error"]["code"] == "not_found" + + +async def test_list_points_returns_nothing_for_another_tenants_file( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + owner_id, _ = await _tenant_with_key(db_session) + _, intruder_token = await _tenant_with_key(db_session) + await _seed(qdrant_client, qdrant_settings, (SeedSpec(owner_id, FILE_ID, 0, "secret", 1.0),)) + + response = await api_client.get( + "/v1/points", params={"file_id": str(FILE_ID)}, headers=_auth(intruder_token) + ) + + assert response.status_code == 200 + assert response.json()["points"] == [] + + +async def test_list_points_excludes_inactive_points_unless_asked( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + tenant_id, token = await _tenant_with_key(db_session) + await _seed( + qdrant_client, + qdrant_settings, + ( + SeedSpec(tenant_id, FILE_ID, 0, "live", 1.0), + SeedSpec(tenant_id, FILE_ID, 1, "deleted", 2.0, is_active=False), + ), + ) + + default = await api_client.get( + "/v1/points", params={"file_id": str(FILE_ID)}, headers=_auth(token) + ) + opted_in = await api_client.get( + "/v1/points", + params={"file_id": str(FILE_ID), "include_inactive": True}, + headers=_auth(token), + ) + + assert [point["content"] for point in default.json()["points"]] == ["live"] + assert [point["content"] for point in opted_in.json()["points"]] == ["live", "deleted"] + + +async def test_list_points_pagination_does_not_skip_or_repeat_under_a_concurrent_insert( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + """The defect an offset cursor would have. + + Between page one and page two, a point is inserted *behind* the cursor + (`order_id` 0.5, ahead of everything already returned). An offset-based + cursor would shift every later row down by one and hand back `b` a second + time. A value-based cursor is unaffected: the reader has passed 2.0 and the + new row is behind it. + """ + tenant_id, token = await _tenant_with_key(db_session) + await _seed( + qdrant_client, + qdrant_settings, + ( + SeedSpec(tenant_id, FILE_ID, 0, "a", 1.0), + SeedSpec(tenant_id, FILE_ID, 1, "b", 2.0), + SeedSpec(tenant_id, FILE_ID, 2, "c", 3.0), + SeedSpec(tenant_id, FILE_ID, 3, "d", 4.0), + ), + ) + + first = await api_client.get( + "/v1/points", + params={"file_id": str(FILE_ID), "limit": 2}, + headers=_auth(token), + ) + await _seed(qdrant_client, qdrant_settings, (SeedSpec(tenant_id, FILE_ID, 9, "inserted", 0.5),)) + second = await api_client.get( + "/v1/points", + params={ + "file_id": str(FILE_ID), + "limit": 2, + "cursor": first.json()["next_cursor"], + }, + headers=_auth(token), + ) + + assert [point["content"] for point in first.json()["points"]] == ["a", "b"] + assert [point["content"] for point in second.json()["points"]] == ["c", "d"] + + +async def test_count_points_respects_tenant_and_domain_filters( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + tenant_id, token = await _tenant_with_key(db_session) + other_id, _ = await _tenant_with_key(db_session) + await _seed( + qdrant_client, + qdrant_settings, + ( + SeedSpec(tenant_id, FILE_ID, 0, "one", 1.0), + SeedSpec(tenant_id, OTHER_FILE_ID, 0, "two", 1.0, domain="ops"), + SeedSpec(other_id, FILE_ID, 5, "theirs", 1.0), + ), + ) + + everything = await api_client.get("/v1/points/count", headers=_auth(token)) + scoped = await api_client.get( + "/v1/points/count", params={"domain": "ops"}, headers=_auth(token) + ) + + assert everything.json()["count"] == 2 + assert scoped.json()["count"] == 1 + + +async def test_search_points_matches_content_within_the_tenant_only( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + tenant_id, token = await _tenant_with_key(db_session) + other_id, _ = await _tenant_with_key(db_session) + await _seed( + qdrant_client, + qdrant_settings, + ( + SeedSpec(tenant_id, FILE_ID, 0, "annual leave policy", 1.0), + SeedSpec(tenant_id, FILE_ID, 1, "expense policy", 2.0), + SeedSpec(tenant_id, FILE_ID, 2, "office address", 3.0), + SeedSpec(other_id, OTHER_FILE_ID, 0, "their leave policy", 1.0), + ), + ) + + response = await api_client.get( + "/v1/points/search", params={"q": "policy"}, headers=_auth(token) + ) + + body = response.json() + assert body["query"] == "policy" + assert {point["content"] for point in body["points"]} == { + "annual leave policy", + "expense policy", + } + + +async def test_search_points_folds_arabic_letterforms_in_the_query( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + """A query typed on an Arabic keyboard must match Persian-folded content. + + Ingestion folds U+064A/U+0643 to U+06CC/U+06A9 before storing, so an + unfolded query would match nothing — an empty result rather than an error, + which is exactly the kind of silent failure nobody reports as a bug. + Codepoints, not literals: the two forms are visually identical here. + """ + persian = "مدیریت" # stored, Persian yeh + arabic = "مديريت" # queried, Arabic yeh + assert persian != arabic + + tenant_id, token = await _tenant_with_key(db_session) + await _seed(qdrant_client, qdrant_settings, (SeedSpec(tenant_id, FILE_ID, 0, persian, 1.0),)) + + response = await api_client.get("/v1/points/search", params={"q": arabic}, headers=_auth(token)) + + assert [point["content"] for point in response.json()["points"]] == [persian] + + +async def test_point_read_routes_require_the_points_read_scope( + api_client: AsyncClient, db_session: AsyncSession +) -> None: + """An upload key must not double as a way to read every chunk.""" + _, token = await _tenant_with_key(db_session, scopes=["files:write"]) + + response = await api_client.get( + "/v1/points", params={"file_id": str(FILE_ID)}, headers=_auth(token) + ) + + assert response.status_code == 403 + assert response.json()["error"]["code"] == "missing_scope" + + +async def test_count_and_search_paths_are_not_parsed_as_point_ids( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + """Route order regression guard. + + `/{point_id}` declared before `/count` would make this `422` — Pydantic + failing to read "count" as a UUID — rather than a count. The bug is one + tidy-up reorder away, and nothing else in the suite would catch it. + + Seeds nothing but still bootstraps the collection: querying a collection + that does not exist is a `500`, which `/readyz` is responsible for + preventing, and which would mask the `422` this test is watching for. + """ + _, token = await _tenant_with_key(db_session) + await _seed(qdrant_client, qdrant_settings, ()) + + count = await api_client.get("/v1/points/count", headers=_auth(token)) + search = await api_client.get( + "/v1/points/search", params={"q": "anything"}, headers=_auth(token) + ) + + assert count.status_code == 200 + assert search.status_code == 200 + + +async def test_file_points_route_lists_the_same_points_as_the_query_form( + api_client: AsyncClient, + db_session: AsyncSession, + qdrant_client: AsyncQdrantClient, + qdrant_settings: QdrantSettings, +) -> None: + tenant_id, token = await _tenant_with_key(db_session) + await _seed( + qdrant_client, + qdrant_settings, + ( + SeedSpec(tenant_id, FILE_ID, 0, "first", 1.0), + SeedSpec(tenant_id, FILE_ID, 1, "second", 2.0), + ), + ) + + by_query = await api_client.get( + "/v1/points", params={"file_id": str(FILE_ID)}, headers=_auth(token) + ) + by_path = await api_client.get(f"/v1/files/{FILE_ID}/points", headers=_auth(token)) + + assert by_path.status_code == 200 + assert by_path.json() == by_query.json() diff --git a/tests/unit/application/points/test_queries.py b/tests/unit/application/points/test_queries.py new file mode 100644 index 0000000..cf71ba9 --- /dev/null +++ b/tests/unit/application/points/test_queries.py @@ -0,0 +1,103 @@ +"""`application/points/queries.py` against the fake repository (ADR-0016). + +The HTTP-level behaviour is covered in `tests/integration/qdrant/`. What is +worth isolating here is the small amount of logic the service actually owns — +raising instead of returning `None`, and folding a query's Arabic letterforms — +because both are cheap to test directly and expensive to notice when broken. +""" + +import uuid + +import pytest + +from src.application.points.point import PointNotFoundError +from src.application.points.queries import ( + count_points, + get_point, + list_file_points, + search_points, +) +from tests.fakes import FakePointRepository +from tests.support.point_contract import SeedSpec, build_point, point_id_for + +pytestmark = [pytest.mark.unit, pytest.mark.asyncio] + +TENANT = uuid.UUID("11111111-1111-4111-8111-111111111111") +OTHER_TENANT = uuid.UUID("22222222-2222-4222-8222-222222222222") +FILE = uuid.UUID("33333333-3333-4333-8333-333333333333") + + +def _repository(*specs: SeedSpec) -> FakePointRepository: + repository = FakePointRepository() + for spec in specs: + repository.add(build_point(spec)) + return repository + + +async def test_get_point_returns_the_point_for_its_owner() -> None: + repository = _repository(SeedSpec(TENANT, FILE, 0, "hello", 1.0)) + + point = await get_point(repository, tenant_id=TENANT, point_id=point_id_for(FILE, 0)) + + assert point.content == "hello" + + +async def test_get_point_raises_for_another_tenants_point() -> None: + """Raising, not returning `None`, so a route cannot serve `200 null`.""" + repository = _repository(SeedSpec(TENANT, FILE, 0, "hello", 1.0)) + + with pytest.raises(PointNotFoundError): + await get_point(repository, tenant_id=OTHER_TENANT, point_id=point_id_for(FILE, 0)) + + +async def test_get_point_raises_for_an_unknown_point() -> None: + repository = _repository() + + with pytest.raises(PointNotFoundError): + await get_point(repository, tenant_id=TENANT, point_id=uuid.uuid4()) + + +async def test_search_points_folds_arabic_letterforms_before_matching() -> None: + """The query is normalized the same way ingestion normalized the content. + + Stored content carries Persian yeh (U+06CC) because `normalize_persian_text` + folded it at ingest. A query typed with Arabic yeh (U+064A) is a different + codepoint and would match nothing — silently, with no error to notice. + """ + persian = "مدیریت" + arabic = "مديريت" + assert persian != arabic + + repository = _repository(SeedSpec(TENANT, FILE, 0, persian, 1.0)) + + found = await search_points(repository, tenant_id=TENANT, query=arabic, limit=10) + + assert [point.content for point in found.points] == [persian] + + +async def test_search_points_does_not_reach_another_tenant() -> None: + repository = _repository(SeedSpec(OTHER_TENANT, FILE, 0, "secret", 1.0)) + + found = await search_points(repository, tenant_id=TENANT, query="secret", limit=10) + + assert found.points == () + + +async def test_list_file_points_returns_an_empty_page_for_a_foreign_file() -> None: + """Indistinguishable from an unknown file — the same non-disclosure rule.""" + repository = _repository(SeedSpec(OTHER_TENANT, FILE, 0, "secret", 1.0)) + + page = await list_file_points(repository, tenant_id=TENANT, file_id=FILE, limit=10) + + assert page.points == () + assert page.next_cursor is None + + +async def test_count_points_excludes_inactive_unless_asked() -> None: + repository = _repository( + SeedSpec(TENANT, FILE, 0, "live", 1.0), + SeedSpec(TENANT, FILE, 1, "gone", 2.0, is_active=False), + ) + + assert await count_points(repository, tenant_id=TENANT) == 1 + assert await count_points(repository, tenant_id=TENANT, include_inactive=True) == 2