From 3b9434faf48f99b73544ee71688be65ec713e008 Mon Sep 17 00:00:00 2001 From: Ali Zarinkolah Date: Sat, 22 Aug 2026 15:08:54 +0330 Subject: [PATCH] feat(points): add the /v1/points read paths Why: - Ingestion writes points in bulk but nothing could read one back. Plan 002 Phase 2 opens the read surface an admin frontend needs. Changes: - GET /v1/points/{point_id}, /v1/points?file_id=..., /v1/points/count, /v1/points/search, and /v1/files/{file_id}/points, all under points:read -- the scope follows the data, so an upload key does not become a way to read every chunk of every file. - The keyword query is Persian-normalized before matching, because ingestion letter-folds content at ingest and an unfolded query would return an empty result set silently rather than an error. - file_id is required on the listing: the cursor is an order_id value and order_id is only unique within one file. - PointNotFoundError maps to 404, never 403, so a cross-tenant point id is indistinguishable from a nonexistent one. - Route order is load-bearing: /count and /search precede /{point_id}, or "count" is parsed as a UUID and fails 422. Impact: - Requires the content/is_active/chunk_index payload indexes, so a deployed environment needs qdrant_bootstrap re-run before search works. - Keyword search returns no relevance score and no ranked order; callers must not read array position as relevance. Co-Authored-By: Claude Opus 5 --- src/api/errors.py | 4 + src/api/router.py | 2 + src/api/routers/files.py | 32 +++++++ src/api/routers/points.py | 134 ++++++++++++++++++++++++++++++ src/api/schemas/points.py | 20 +++++ src/application/points/queries.py | 115 +++++++++++++++++++++++++ 6 files changed, 307 insertions(+) create mode 100644 src/api/routers/points.py create mode 100644 src/application/points/queries.py diff --git a/src/api/errors.py b/src/api/errors.py index c6f8da8..7ec6fdc 100644 --- a/src/api/errors.py +++ b/src/api/errors.py @@ -27,6 +27,7 @@ from src.application.ingestion.errors import ( PointIndexingError, UnsupportedSourceTypeError, ) +from src.application.points.point import PointNotFoundError logger = structlog.get_logger(__name__) @@ -41,6 +42,9 @@ _MAPPING: tuple[tuple[type[Exception], int, str], ...] = ( (TenantInactiveError, status.HTTP_401_UNAUTHORIZED, "tenant_not_found"), (MissingScopeError, status.HTTP_403_FORBIDDEN, "missing_scope"), (InvalidUploadError, status.HTTP_400_BAD_REQUEST, "validation_error"), + # 404, never 403: a cross-tenant point id must be indistinguishable from a + # nonexistent one, or the API becomes an existence oracle (ADR-0016). + (PointNotFoundError, status.HTTP_404_NOT_FOUND, "not_found"), (UnknownDomainError, status.HTTP_400_BAD_REQUEST, "unknown_domain"), (DomainAlreadyExistsError, status.HTTP_409_CONFLICT, "conflict"), (DocumentParseError, status.HTTP_400_BAD_REQUEST, "validation_error"), diff --git a/src/api/router.py b/src/api/router.py index f806747..39d21ff 100644 --- a/src/api/router.py +++ b/src/api/router.py @@ -2,7 +2,9 @@ from fastapi import APIRouter from src.api.routers.domains import router as domains_router from src.api.routers.files import router as files_router +from src.api.routers.points import router as points_router router = APIRouter() router.include_router(domains_router) router.include_router(files_router) +router.include_router(points_router) diff --git a/src/api/routers/files.py b/src/api/routers/files.py index 4f6e971..d873dbd 100644 --- a/src/api/routers/files.py +++ b/src/api/routers/files.py @@ -14,17 +14,21 @@ from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from src.api.dependencies.auth import require_scope from src.api.schemas.files import FileStatusResponse, FileUploadResponse +from src.api.schemas.points import DEFAULT_PAGE_SIZE, LimitQuery, PointListResponse from src.application.auth.context import AuthContext from src.application.files.status import get_file_status from src.application.files.upload import upload_source_file +from src.application.points.queries import list_file_points from src.application.ports.embedding import DenseEmbedder, SparseEmbedder from src.application.ports.object_storage import ObjectStorage +from src.application.ports.point_repository import PointRepository from src.application.ports.point_storage import PointStorage from src.bootstrap.dependencies import ( get_dense_embedders, get_ingestion_concurrency_limiter, get_ingestion_limiter, get_object_storage, + get_point_repository, get_point_storage, get_sessionmaker, get_settings, @@ -35,9 +39,11 @@ from src.config import Settings router = APIRouter(prefix="/files", tags=["files"]) _RequireFilesWrite = Annotated[AuthContext, Depends(require_scope("files:write"))] +_RequirePointsRead = Annotated[AuthContext, Depends(require_scope("points:read"))] _SessionmakerDep = Annotated[async_sessionmaker[AsyncSession], Depends(get_sessionmaker)] _ObjectStorageDep = Annotated[ObjectStorage, Depends(get_object_storage)] _PointStorageDep = Annotated[PointStorage, Depends(get_point_storage)] +_PointRepositoryDep = Annotated[PointRepository, Depends(get_point_repository)] _SettingsDep = Annotated[Settings, Depends(get_settings)] _IngestionLimiterDep = Annotated[CapacityLimiter, Depends(get_ingestion_limiter)] _ConcurrencyLimiterDep = Annotated[Semaphore, Depends(get_ingestion_concurrency_limiter)] @@ -92,3 +98,29 @@ async def get_file( if result is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="file not found") return FileStatusResponse.from_result(result) + + +@router.get("/{file_id}/points") +async def list_points_for_file( + file_id: uuid.UUID, + auth: _RequirePointsRead, + repository: _PointRepositoryDep, + limit: LimitQuery = DEFAULT_PAGE_SIZE, + cursor: str | None = None, + include_inactive: bool = False, +) -> PointListResponse: + """The same listing as `GET /v1/points?file_id=...`, addressed by file. + + Gated on `points:read`, not `files:write`: the resource being read is the + file's chunks, so the scope follows the data rather than the URL prefix. An + upload-only key must not become a way to read every chunk of every file. + """ + page = await list_file_points( + repository, + tenant_id=auth.tenant_id, + file_id=file_id, + limit=limit, + cursor=cursor, + include_inactive=include_inactive, + ) + return PointListResponse.from_page(page) diff --git a/src/api/routers/points.py b/src/api/routers/points.py new file mode 100644 index 0000000..9b9530b --- /dev/null +++ b/src/api/routers/points.py @@ -0,0 +1,134 @@ +"""`/v1/points` read paths (ADR-0002, ADR-0008). + +Routes adapt HTTP to `application/points` calls. They build no Qdrant filters +and hold no CRUD semantics (ADR-0015), and they never read a tenant from the +request — `auth.tenant_id` is the only source, which is what makes ADR-0002's +isolation rule structural rather than a habit. + +**Route order is load-bearing.** `/count` and `/search` are declared before +`/{point_id}`. FastAPI matches in declaration order, so with `/{point_id}` first +a request for `/v1/points/count` would try to parse `"count"` as a UUID and +fail with `422` instead of counting anything. The failure is loud but confusing, +and it comes back the moment someone reorders these for tidiness. + +Gated on `points:read`, separately from `files:write`: a key that can upload +documents should not thereby be able to read every chunk of every file, and +plan 002's mutating paths will want `points:write` distinct again. +""" + +import uuid +from typing import Annotated + +from fastapi import APIRouter, Depends, Query + +from src.api.dependencies.auth import require_scope +from src.api.schemas.points import ( + DEFAULT_PAGE_SIZE, + LimitQuery, + PointCountResponse, + PointListResponse, + PointResponse, + PointSearchResponse, +) +from src.application.auth.context import AuthContext +from src.application.points.queries import ( + count_points, + get_point, + list_file_points, + search_points, +) +from src.application.ports.point_repository import PointRepository +from src.bootstrap.dependencies import get_point_repository + +router = APIRouter(prefix="/points", tags=["points"]) + +_RequirePointsRead = Annotated[AuthContext, Depends(require_scope("points:read"))] +_PointRepositoryDep = Annotated[PointRepository, Depends(get_point_repository)] + + +@router.get("/count") +async def count_tenant_points( + auth: _RequirePointsRead, + repository: _PointRepositoryDep, + domain: str | None = None, + file_id: uuid.UUID | None = None, + include_inactive: bool = False, +) -> PointCountResponse: + count = await count_points( + repository, + tenant_id=auth.tenant_id, + domain=domain, + file_id=file_id, + include_inactive=include_inactive, + ) + return PointCountResponse(count=count) + + +@router.get("/search") +async def search_tenant_points( + auth: _RequirePointsRead, + repository: _PointRepositoryDep, + q: Annotated[str, Query(min_length=1)], + limit: LimitQuery = DEFAULT_PAGE_SIZE, + cursor: str | None = None, + domain: str | None = None, + file_id: uuid.UUID | None = None, + include_inactive: bool = False, +) -> PointSearchResponse: + """Keyword search over point content — **not** semantic retrieval. + + Matches Qdrant's full-text payload index on `content`, combined with the + structured filters below. Results are unranked: the index filters rather + than scores, so there is no relevance order and no score to return. Callers + wanting ranked answers want the agent retrieval path (plan 003), not this. + """ + page = await search_points( + repository, + tenant_id=auth.tenant_id, + query=q, + limit=limit, + cursor=cursor, + domain=domain, + file_id=file_id, + include_inactive=include_inactive, + ) + return PointSearchResponse.from_search(page, query=q) + + +@router.get("") +async def list_tenant_points( + auth: _RequirePointsRead, + repository: _PointRepositoryDep, + file_id: uuid.UUID, + limit: LimitQuery = DEFAULT_PAGE_SIZE, + cursor: str | None = None, + include_inactive: bool = False, +) -> PointListResponse: + """A file's points in `order_id` order. + + `file_id` is required rather than optional: the pagination cursor is an + `order_id` value, and `order_id` is only unique within one file. Listing + across files would silently drop or repeat rows at every page boundary. + """ + page = await list_file_points( + repository, + tenant_id=auth.tenant_id, + file_id=file_id, + limit=limit, + cursor=cursor, + include_inactive=include_inactive, + ) + return PointListResponse.from_page(page) + + +@router.get("/{point_id}") +async def get_tenant_point( + point_id: uuid.UUID, + auth: _RequirePointsRead, + repository: _PointRepositoryDep, + with_vectors: bool = False, +) -> PointResponse: + point = await get_point( + repository, tenant_id=auth.tenant_id, point_id=point_id, with_vectors=with_vectors + ) + return PointResponse.from_point(point) diff --git a/src/api/schemas/points.py b/src/api/schemas/points.py index 5537e80..48e5a85 100644 --- a/src/api/schemas/points.py +++ b/src/api/schemas/points.py @@ -13,12 +13,24 @@ The shape callers see, kept separate from `application/points`' domain models import uuid from datetime import datetime +from typing import Annotated +from fastapi import Query from pydantic import BaseModel, ConfigDict, Field from src.application.points.point import Point from src.application.ports.point_repository import PointPage +# A page ceiling the caller cannot raise. Scroll pages are materialized in +# memory both here and in Qdrant, so an unbounded `limit` is a cheap way for one +# request to hurt every other tenant sharing the process. Declared once because +# two routers paginate points -- `/v1/points` and `/v1/files/{file_id}/points` -- +# and a ceiling that differs between them is a ceiling in only one of them. +DEFAULT_PAGE_SIZE = 50 +MAX_PAGE_SIZE = 200 + +LimitQuery = Annotated[int, Query(ge=1, le=MAX_PAGE_SIZE)] + class PointResponse(BaseModel): point_id: uuid.UUID @@ -91,6 +103,14 @@ class PointSearchResponse(PointListResponse): query: str + @classmethod + def from_search(cls, page: PointPage, *, query: str) -> "PointSearchResponse": + return cls( + query=query, + points=[PointResponse.from_point(point) for point in page.points], + next_cursor=page.next_cursor, + ) + class PointCreateRequest(BaseModel): """Create one point. The server assigns identity, ordering, and provenance. diff --git a/src/application/points/queries.py b/src/application/points/queries.py new file mode 100644 index 0000000..f914f54 --- /dev/null +++ b/src/application/points/queries.py @@ -0,0 +1,115 @@ +"""Read paths for `/v1/points` (ADR-0002, ADR-0008). + +The caller-facing entry points for point reads. Routers call these; they never +touch the `PointRepository` directly, and never build a filter. + +This module is thin on purpose but not empty, and the two things it does own are +exactly the ones a route would otherwise get wrong: + +- **`tenant_id` always comes from the caller's `AuthContext`.** Every function + takes it as a required keyword and hands it to the repository. Nothing here + reads a tenant from a query string or body. +- **A keyword query is Persian-normalized before it reaches the index.** + Ingestion letter-folds chunk content (`normalize_persian_text`, ADR-0018), so + stored text contains Persian yeh/keheh. A query typed on an Arabic keyboard + carries U+064A/U+0643 and would match nothing at all — a silent empty result, + not an error. Folding the query the same way is what makes the two comparable. + +Reads emit no log events. The request middleware already records every call, and +ADR-0011 reserves `INFO` for lifecycle events rather than per-read volume; +mutations get their own events when those paths land. +""" + +import uuid + +from src.application.ingestion.normalization import normalize_persian_text +from src.application.points.point import Point, PointNotFoundError +from src.application.ports.point_repository import PointPage, PointRepository + + +async def get_point( + repository: PointRepository, + *, + tenant_id: uuid.UUID, + point_id: uuid.UUID, + with_vectors: bool = False, +) -> Point: + """One point, or `PointNotFoundError` if it is not this tenant's. + + Raises rather than returning `None` so a route cannot forget the check and + serve `200 null`. "Absent" and "another tenant's" are the same outcome by + design (ADR-0016: cross-tenant access is `404`, never `403`). + """ + point = await repository.get(tenant_id=tenant_id, point_id=point_id, with_vectors=with_vectors) + if point is None: + raise PointNotFoundError(f"point {point_id} not found") + return point + + +async def list_file_points( + repository: PointRepository, + *, + tenant_id: uuid.UUID, + file_id: uuid.UUID, + limit: int, + cursor: str | None = None, + include_inactive: bool = False, +) -> PointPage: + """One file's points in display (`order_id`) order. + + An unknown or foreign `file_id` yields an empty page rather than an error: + the two are indistinguishable to the caller, which is the same + non-disclosure property `get_point` gets from raising. + """ + return await repository.list_by_file( + tenant_id=tenant_id, + file_id=file_id, + limit=limit, + cursor=cursor, + include_inactive=include_inactive, + ) + + +async def count_points( + repository: PointRepository, + *, + tenant_id: uuid.UUID, + domain: str | None = None, + file_id: uuid.UUID | None = None, + include_inactive: bool = False, +) -> int: + return await repository.count( + tenant_id=tenant_id, + domain=domain, + file_id=file_id, + include_inactive=include_inactive, + ) + + +async def search_points( + repository: PointRepository, + *, + tenant_id: uuid.UUID, + query: str, + limit: int, + cursor: str | None = None, + domain: str | None = None, + file_id: uuid.UUID | None = None, + include_inactive: bool = False, +) -> PointPage: + """Keyword match on `content`, within this tenant. + + **Not semantic retrieval.** Qdrant's full-text index filters rather than + scores, so results carry no relevance ranking and their order is + unspecified. Ranked retrieval is ADR-0003's hybrid path in plan 003; this + function must not grow a semantic mode (ADR-0002). + """ + return await repository.keyword_search( + tenant_id=tenant_id, + query=normalize_persian_text(query), + limit=limit, + cursor=cursor, + domain=domain, + file_id=file_id, + include_inactive=include_inactive, + )