Refactor backend and frontend cleanup paths
This commit is contained in:
+39
-153
@@ -1,22 +1,4 @@
|
||||
"""Document CRUD endpoints — list, get, patch, delete, and re-classify.
|
||||
|
||||
Endpoints:
|
||||
GET "" — list documents with sort, folder filter, and FTS (list_documents)
|
||||
GET /{doc_id} — get document metadata (get_document)
|
||||
PATCH /{doc_id} — update filename and/or folder_id (patch_document)
|
||||
DELETE /{doc_id} — delete document, decrement quota atomically (delete_document)
|
||||
POST /{doc_id}/classify — re-queue Celery classification (classify_document, D-08)
|
||||
|
||||
Sub-router carries NO prefix — prefix="/api/documents" lives in __init__.py (D-04).
|
||||
|
||||
Security:
|
||||
T-03-11: ownership assertion on every resource endpoint — cross-user access returns 404.
|
||||
T-05-09-01: get_regular_user dep rejects admins (403) and unauthenticated (401).
|
||||
T-05-09-02: response uses storage.get_metadata() whitelist — no credentials_enc, no password_hash.
|
||||
T-06.2-03-01: cloud documents skip MinIO quota decrement.
|
||||
T-06.2-03-02: cloud delete failure returns {success: false, cloud_delete_failed: true} (HTTP 200).
|
||||
T-07-10: classify endpoint — IDOR returns 404 per ownership assertion.
|
||||
"""
|
||||
"""Document CRUD endpoints."""
|
||||
from __future__ import annotations
|
||||
|
||||
import uuid
|
||||
@@ -41,14 +23,31 @@ from services.rate_limiting import account_limiter
|
||||
from storage import get_storage_backend_for_document as _get_storage_backend_for_document
|
||||
from tasks.document_tasks import extract_and_classify
|
||||
|
||||
from api.documents.shared import DocumentPatch, _CLOUD_PROVIDERS
|
||||
from api.documents.shared import DocumentPatch, get_accessible_document, get_owned_document
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
|
||||
# ── GET /api/documents ────────────────────────────────────────────────────────
|
||||
# Route registered on parent router in __init__.py (FastAPI 0.100+ disallows
|
||||
# include_router when both the include prefix and route path are empty strings).
|
||||
async def _shared_document_ids(session: AsyncSession, user_id: uuid.UUID) -> set[uuid.UUID]:
|
||||
result = await session.execute(select(Share.document_id).where(Share.owner_id == user_id))
|
||||
return {row[0] for row in result.fetchall()}
|
||||
|
||||
|
||||
async def _decorate_shared_flags(
|
||||
session: AsyncSession,
|
||||
user_id: uuid.UUID,
|
||||
items: list[dict],
|
||||
) -> list[dict]:
|
||||
shared_ids = await _shared_document_ids(session, user_id)
|
||||
for item in items:
|
||||
try:
|
||||
doc_id = uuid.UUID(item.get("id", ""))
|
||||
except (TypeError, ValueError):
|
||||
item["is_shared"] = False
|
||||
else:
|
||||
item["is_shared"] = doc_id in shared_ids
|
||||
return items
|
||||
|
||||
|
||||
@account_limiter.limit("100/minute")
|
||||
async def list_documents(
|
||||
@@ -63,40 +62,17 @@ async def list_documents(
|
||||
session: AsyncSession = Depends(get_db),
|
||||
current_user: User = Depends(get_regular_user),
|
||||
):
|
||||
"""List documents with optional sort, folder filter, and full-text search.
|
||||
|
||||
D-16: requires authenticated regular user (get_regular_user rejects admins).
|
||||
Returns only documents belonging to the current user.
|
||||
|
||||
FOLD-05: sort by name|date|size; order asc|desc; folder_id filter;
|
||||
q full-text search via plainto_tsquery (PostgreSQL only — silently skipped
|
||||
on SQLite when function is unavailable). FTS scope is always scoped to
|
||||
current_user.id (T-04-03-02).
|
||||
|
||||
Backward-compat: when sort/order/folder_id/q are not provided, behaviour
|
||||
is identical to the pre-Phase-4 implementation.
|
||||
"""
|
||||
"""List documents with optional sort, folder filter, and full-text search."""
|
||||
request.state.current_user = current_user
|
||||
# If no new params used, fall through to the legacy storage.list_metadata path
|
||||
# to preserve full backward compatibility with topic filtering.
|
||||
if folder_id is None and q is None and sort == "date" and order == "desc":
|
||||
docs = await storage.list_metadata(session, user_id=current_user.id, topic=topic)
|
||||
total = len(docs)
|
||||
start = (page - 1) * per_page
|
||||
# Add is_shared field (Phase 4 addition)
|
||||
shared_result = await session.execute(
|
||||
select(Share.document_id).where(Share.owner_id == current_user.id)
|
||||
items = await _decorate_shared_flags(
|
||||
session,
|
||||
current_user.id,
|
||||
docs[start : start + per_page],
|
||||
)
|
||||
shared_ids = {row[0] for row in shared_result.fetchall()}
|
||||
items = []
|
||||
for d in docs[start : start + per_page]:
|
||||
doc_id_str = d.get("id", "")
|
||||
try:
|
||||
doc_uuid = uuid.UUID(doc_id_str)
|
||||
except (ValueError, AttributeError):
|
||||
doc_uuid = None
|
||||
d["is_shared"] = doc_uuid in shared_ids if doc_uuid else False
|
||||
items.append(d)
|
||||
return {"items": items, "total": total, "page": page, "per_page": per_page}
|
||||
|
||||
from db.models import DocumentTopic, Topic # noqa: PLC0415 (avoid circular at module top)
|
||||
@@ -126,8 +102,6 @@ async def list_documents(
|
||||
order_fn = sort_col.asc if order == "asc" else sort_col.desc
|
||||
stmt = stmt.order_by(order_fn())
|
||||
|
||||
# Full-text search — plainto_tsquery on extracted_text (PostgreSQL only)
|
||||
# Falls back to unfiltered if the DB dialect doesn't support @@ (e.g. SQLite in test env)
|
||||
fts_requested = q is not None and len(q) >= 2
|
||||
if fts_requested:
|
||||
fts_stmt = stmt.where(
|
||||
@@ -143,14 +117,11 @@ async def list_documents(
|
||||
result = await session.execute(stmt)
|
||||
docs_orm = result.scalars().all()
|
||||
|
||||
shared_result = await session.execute(
|
||||
select(Share.document_id).where(Share.owner_id == current_user.id)
|
||||
)
|
||||
shared_ids = {row[0] for row in shared_result.fetchall()}
|
||||
|
||||
all_items = []
|
||||
shared_ids = await _shared_document_ids(session, current_user.id)
|
||||
for doc in docs_orm:
|
||||
from services.storage import _doc_to_dict, _load_topic_names # noqa: PLC0415
|
||||
|
||||
topic_names = await _load_topic_names(session, doc.id)
|
||||
d = _doc_to_dict(doc, topic_names)
|
||||
d["is_shared"] = doc.id in shared_ids
|
||||
@@ -166,8 +137,6 @@ async def list_documents(
|
||||
}
|
||||
|
||||
|
||||
# ── GET /api/documents/{doc_id} ───────────────────────────────────────────────
|
||||
|
||||
@router.get("/{doc_id}")
|
||||
@account_limiter.limit("100/minute")
|
||||
async def get_document(
|
||||
@@ -176,44 +145,18 @@ async def get_document(
|
||||
session: AsyncSession = Depends(get_db),
|
||||
current_user: User = Depends(get_regular_user),
|
||||
):
|
||||
"""Return document metadata by ID.
|
||||
|
||||
D-16: requires authenticated regular user. Asserts ownership — cross-user
|
||||
access returns 404 (not 403) to avoid information leakage (T-03-11).
|
||||
"""
|
||||
"""Return document metadata by ID."""
|
||||
request.state.current_user = current_user
|
||||
try:
|
||||
uid = uuid.UUID(doc_id)
|
||||
except ValueError:
|
||||
raise HTTPException(404, "Document not found")
|
||||
|
||||
doc = await session.get(Document, uid)
|
||||
if doc is None:
|
||||
raise HTTPException(404, "Document not found")
|
||||
|
||||
is_recipient = False
|
||||
if doc.user_id != current_user.id:
|
||||
share_result = await session.execute(
|
||||
select(Share).where(
|
||||
Share.document_id == uid,
|
||||
Share.recipient_id == current_user.id,
|
||||
)
|
||||
)
|
||||
if share_result.scalar_one_or_none() is None:
|
||||
raise HTTPException(404, "Document not found")
|
||||
is_recipient = True
|
||||
_, is_recipient = await get_accessible_document(session, doc_id, current_user.id)
|
||||
|
||||
meta = await storage.get_metadata(session, doc_id)
|
||||
if meta is None:
|
||||
raise HTTPException(404, "Document not found")
|
||||
# T-04-04-03: recipients get metadata only — extracted_text excluded (consistent with /shares/received)
|
||||
if is_recipient:
|
||||
meta.pop("extracted_text", None)
|
||||
return meta
|
||||
|
||||
|
||||
# ── PATCH /api/documents/{doc_id} ────────────────────────────────────────────
|
||||
|
||||
@router.patch("/{doc_id}")
|
||||
@account_limiter.limit("100/minute")
|
||||
async def patch_document(
|
||||
@@ -223,25 +166,9 @@ async def patch_document(
|
||||
session: AsyncSession = Depends(get_db),
|
||||
current_user: User = Depends(get_regular_user),
|
||||
):
|
||||
"""Update document metadata (filename and/or folder_id).
|
||||
|
||||
T-05-09-01: get_regular_user dep rejects admins (403) and unauthenticated (401).
|
||||
T-05-09-01: ownership check — non-owner gets 404 to avoid leaking document IDs (D-16).
|
||||
T-05-09-02: response uses storage.get_metadata() which excludes credentials_enc and
|
||||
password_hash via the _doc_to_dict whitelist.
|
||||
|
||||
At least one field must be provided — empty body returns 422.
|
||||
folder_id=null moves the document to the root (no folder).
|
||||
"""
|
||||
"""Update document metadata."""
|
||||
request.state.current_user = current_user
|
||||
try:
|
||||
uid = uuid.UUID(doc_id)
|
||||
except ValueError:
|
||||
raise HTTPException(404, "Document not found")
|
||||
|
||||
doc = await session.get(Document, uid)
|
||||
if doc is None or doc.user_id != current_user.id:
|
||||
raise HTTPException(404, "Document not found")
|
||||
doc = await get_owned_document(session, doc_id, current_user.id)
|
||||
|
||||
if not body.model_fields_set:
|
||||
raise HTTPException(422, "At least one field (filename, folder_id) must be provided")
|
||||
@@ -250,7 +177,6 @@ async def patch_document(
|
||||
doc.filename = body.filename
|
||||
|
||||
if "folder_id" in body.model_fields_set:
|
||||
# folder_id=null → move to root (no folder); folder_id=<uuid> → move to folder
|
||||
if body.folder_id is not None:
|
||||
target = await session.get(Folder, body.folder_id)
|
||||
if target is None or target.user_id != current_user.id:
|
||||
@@ -265,8 +191,6 @@ async def patch_document(
|
||||
return meta
|
||||
|
||||
|
||||
# ── DELETE /api/documents/{doc_id} ───────────────────────────────────────────
|
||||
|
||||
@router.delete("/{doc_id}")
|
||||
@account_limiter.limit("100/minute")
|
||||
async def delete_document(
|
||||
@@ -276,27 +200,9 @@ async def delete_document(
|
||||
session: AsyncSession = Depends(get_db),
|
||||
current_user: User = Depends(get_regular_user),
|
||||
):
|
||||
"""Delete a document and decrement quota atomically.
|
||||
|
||||
For cloud-stored documents:
|
||||
- Default path: attempt cloud provider delete first; on failure return
|
||||
{success: false, cloud_delete_failed: true} (HTTP 200) so the frontend
|
||||
can offer a "Remove from app" fallback (T-06.2-03-02).
|
||||
- remove_only=true: skip cloud delete, remove DB row only, skip quota decrement.
|
||||
- Cloud docs always use skip_quota=True (never charged MinIO quota, T-06.2-03-01).
|
||||
|
||||
D-16: requires authenticated regular user. Asserts ownership — cross-user
|
||||
delete returns 404 (not 403) to avoid information leakage (T-03-11).
|
||||
"""
|
||||
"""Delete a document and decrement quota when appropriate."""
|
||||
request.state.current_user = current_user
|
||||
try:
|
||||
uid = uuid.UUID(doc_id)
|
||||
except ValueError:
|
||||
raise HTTPException(404, "Document not found")
|
||||
|
||||
doc = await session.get(Document, uid)
|
||||
if doc is None or doc.user_id != current_user.id:
|
||||
raise HTTPException(404, "Document not found")
|
||||
doc = await get_owned_document(session, doc_id, current_user.id)
|
||||
|
||||
is_cloud = doc.storage_backend != "minio"
|
||||
_doc_size = doc.size_bytes
|
||||
@@ -305,7 +211,8 @@ async def delete_document(
|
||||
|
||||
if is_cloud and not remove_only:
|
||||
try:
|
||||
import api.documents as _doc_pkg # late import allows test monkeypatching via api.documents
|
||||
import api.documents as _doc_pkg
|
||||
|
||||
_gsb = _doc_pkg.get_storage_backend_for_document
|
||||
cloud_backend = await _gsb(doc, current_user, session)
|
||||
await cloud_backend.delete_object(doc.object_key)
|
||||
@@ -320,13 +227,10 @@ async def delete_document(
|
||||
},
|
||||
)
|
||||
|
||||
# auto_commit=False defers the commit so the audit log write below happens
|
||||
# in the same transaction — avoids the split-transaction gap (WR-08).
|
||||
ok = await storage.delete_document(session, doc_id, skip_quota=is_cloud, auto_commit=False)
|
||||
if not ok:
|
||||
raise HTTPException(404, "Document not found")
|
||||
|
||||
# D-13: document deleted event — written in the same transaction as the delete (WR-08).
|
||||
await write_audit_log(
|
||||
session,
|
||||
event_type="document.deleted",
|
||||
@@ -341,8 +245,6 @@ async def delete_document(
|
||||
return {"success": True}
|
||||
|
||||
|
||||
# ── POST /api/documents/{doc_id}/classify ────────────────────────────────────
|
||||
|
||||
@router.post("/{doc_id}/classify")
|
||||
@account_limiter.limit("100/minute")
|
||||
async def classify_document(
|
||||
@@ -351,25 +253,9 @@ async def classify_document(
|
||||
session: AsyncSession = Depends(get_db),
|
||||
current_user: User = Depends(get_regular_user),
|
||||
):
|
||||
"""Re-queue a document for classification via Celery (D-11).
|
||||
|
||||
Sets doc.status='processing', commits, dispatches extract_and_classify.delay(),
|
||||
and returns {'document_id': str, 'status': 'processing'}.
|
||||
|
||||
D-16: requires authenticated regular user. Asserts ownership — cross-user
|
||||
classify returns 404 (not 403) to avoid information leakage (T-03-11).
|
||||
T-07-10: ownership enforced here; IDOR returns 404 per STATE.md policy.
|
||||
Placed in crud.py per D-08: same ownership-check pattern as get/patch/delete.
|
||||
"""
|
||||
"""Re-queue a document for classification via Celery."""
|
||||
request.state.current_user = current_user
|
||||
try:
|
||||
uid = uuid.UUID(doc_id)
|
||||
except ValueError:
|
||||
raise HTTPException(404, "Document not found")
|
||||
|
||||
doc = await session.get(Document, uid)
|
||||
if doc is None or doc.user_id != current_user.id:
|
||||
raise HTTPException(404, "Document not found")
|
||||
doc = await get_owned_document(session, doc_id, current_user.id)
|
||||
|
||||
doc.status = "processing"
|
||||
await session.commit()
|
||||
|
||||
Reference in New Issue
Block a user