diff --git a/backend/app/api/docs/documents/register_v2.md b/backend/app/api/docs/documents/register_v2.md new file mode 100644 index 000000000..5008b3697 --- /dev/null +++ b/backend/app/api/docs/documents/register_v2.md @@ -0,0 +1,7 @@ +Register a document that was uploaded to the pre-signed URL from `POST /api/v2/documents/upload-url`. + +Final step of the v2 upload flow. Pass the `document_id` returned by the upload URL endpoint together with the filename; the document row is created and the response carries a fresh signed URL for reading the file back. + +Errors: `400` if no file was uploaded for that `document_id` (or the extension is unsupported), `413` if the uploaded file exceeds 25 MB (the object is deleted), `409` if the `document_id` was already registered — request a new upload URL in that case. + +Document transformation is not available on v2. Use `POST /api/v1/documents` if you need a `target_format` conversion. diff --git a/backend/app/api/docs/documents/upload_url_v2.md b/backend/app/api/docs/documents/upload_url_v2.md new file mode 100644 index 000000000..37353741f --- /dev/null +++ b/backend/app/api/docs/documents/upload_url_v2.md @@ -0,0 +1,9 @@ +Request a pre-signed URL to upload a document straight to Kaapi's object storage. + +Step 1 of the v2 upload flow: + +1. `POST /api/v2/documents/upload-url` with the filename — returns a `document_id` and an `upload_url`. +2. `PUT` the raw file bytes to `upload_url` (no auth header, no form encoding). +3. `POST /api/v2/documents` with the same `document_id` and `filename` to register the document. + +The filename extension is validated here, so an unsupported file type fails before any upload. Nothing is persisted at this step: the `document_id` only becomes a document once you register it. `upload_url` is valid for `expires_in` seconds; request a new one if it lapses. Maximum file size is 25 MB, enforced at registration. diff --git a/backend/app/api/main.py b/backend/app/api/main.py index 6fd97fd1f..c7807ac19 100644 --- a/backend/app/api/main.py +++ b/backend/app/api/main.py @@ -12,6 +12,7 @@ cron, doc_transformation_job, documents, + documents_v2, evaluations, features, fine_tuning, @@ -89,8 +90,10 @@ # v2 API surface (mounted at settings.API_V2_STR). Only the endpoints that differ -# from v1 live here — currently the judged run trigger. Everything else stays v1. +# from v1 live here — currently the judged run trigger and the pre-signed document +# upload flow. Everything else stays v1. api_v2_router = APIRouter() +api_v2_router.include_router(documents_v2.router) api_v2_router.include_router(evaluations_v2_router) api_v2_router.include_router(evaluations_dataset_v2_router) api_v2_router.include_router(evaluations_prompt_improvement_v2_router) diff --git a/backend/app/api/routes/documents_v2.py b/backend/app/api/routes/documents_v2.py new file mode 100644 index 000000000..aa4d18574 --- /dev/null +++ b/backend/app/api/routes/documents_v2.py @@ -0,0 +1,125 @@ +"""v2 document upload: pre-signed PUT to storage, then registration. No transformation.""" + +import logging +from pathlib import Path +from uuid import uuid4 + +from fastapi import APIRouter, Depends, HTTPException + +from app.api.deps import AuthContextDep, SessionDep +from app.api.permissions import Permission, require_permission +from app.core.cloud import get_cloud_storage +from app.core.cloud.storage import SimpleStorageName +from app.crud import DocumentCrud +from app.models import ( + Document, + DocumentPublic, + DocumentRegisterRequest, + DocumentUploadResponse, + DocumentUploadURLRequest, + DocumentUploadURLResponse, +) +from app.services.documents.helpers import ( + validate_filename_format, + verify_uploaded_object, +) +from app.utils import APIResponse, load_description + +logger = logging.getLogger(__name__) + +router = APIRouter(prefix="/documents", tags=["Documents v2"]) + +UPLOAD_URL_EXPIRY_SECONDS = 3600 + + +@router.post( + "/upload-url", + description=load_description("documents/upload_url_v2.md"), + response_model=APIResponse[DocumentUploadURLResponse], + dependencies=[Depends(require_permission(Permission.REQUIRE_PROJECT))], +) +def create_upload_url( + session: SessionDep, + current_user: AuthContextDep, + request: DocumentUploadURLRequest, +) -> APIResponse[DocumentUploadURLResponse]: + validate_filename_format(request.filename) + + storage = get_cloud_storage(session=session, project_id=current_user.project_.id) + document_id = uuid4() + key = Path(storage.storage_path) / str(document_id) + upload_url = storage.get_signed_upload_url( + key.as_posix(), + expires_in=UPLOAD_URL_EXPIRY_SECONDS, + ) + + logger.info( + f"[create_upload_url] Upload URL issued | " + f"document_id: {document_id}, project_id: {current_user.project_.id}" + ) + + return APIResponse[DocumentUploadURLResponse].success_response( + DocumentUploadURLResponse( + document_id=document_id, + upload_url=upload_url, + expires_in=UPLOAD_URL_EXPIRY_SECONDS, + ) + ) + + +@router.post( + "", + description=load_description("documents/register_v2.md"), + status_code=201, + response_model=APIResponse[DocumentUploadResponse], + dependencies=[Depends(require_permission(Permission.REQUIRE_PROJECT))], +) +def register_document( + session: SessionDep, + current_user: AuthContextDep, + request: DocumentRegisterRequest, +) -> APIResponse[DocumentUploadResponse]: + validate_filename_format(request.filename) + + crud = DocumentCrud(session, current_user.project_.id) + if crud.exists(request.document_id): + logger.warning( + f"[register_document] Document already registered | " + f"document_id: {request.document_id}, project_id: {current_user.project_.id}" + ) + raise HTTPException( + status_code=409, + detail="This document_id is already registered. Request a new upload URL.", + ) + + storage = get_cloud_storage(session=session, project_id=current_user.project_.id) + key = Path(storage.storage_path) / str(request.document_id) + object_store_url = str(SimpleStorageName(key.as_posix())) + + file_size_kb = verify_uploaded_object( + storage=storage, + object_store_url=object_store_url, + document_id=request.document_id, + ) + + document = crud.update( + Document( + id=request.document_id, + fname=request.filename, + file_size_kb=file_size_kb, + object_store_url=object_store_url, + project_id=current_user.project_.id, + ) + ) + + document_schema = DocumentPublic.model_validate(document, from_attributes=True) + document_schema.signed_url = storage.get_signed_url(document.object_store_url) + + logger.info( + f"[register_document] Document registered | " + f"document_id: {document.id}, project_id: {current_user.project_.id}, size_kb: {file_size_kb}" + ) + + return APIResponse[DocumentUploadResponse].success_response( + DocumentUploadResponse(**document_schema.model_dump()) + ) diff --git a/backend/app/core/cloud/storage.py b/backend/app/core/cloud/storage.py index a0b451edd..33815f2ea 100644 --- a/backend/app/core/cloud/storage.py +++ b/backend/app/core/cloud/storage.py @@ -155,6 +155,16 @@ def get_signed_url(self, url: str, expires_in: int = 3600) -> str: """Generate a signed URL with an optional expiry""" pass + @abstractmethod + def get_signed_upload_url( + self, + key: str, + content_type: str | None = None, + expires_in: int = 3600, + ) -> str: + """Generate a signed URL the client can upload (PUT) to directly""" + pass + @abstractmethod def delete(self, url: str) -> None: """Delete a file from storage""" @@ -285,6 +295,42 @@ def get_signed_url(self, url: str, expires_in: int = 3600) -> str: ) raise CloudStorageError(f'AWS Error: "{err}" ({url})') from err + def get_signed_upload_url( + self, + key: str, + content_type: str | None = None, + expires_in: int = 3600, + ) -> str: + """ + Generate a signed S3 URL the client can PUT a file to. + content_type, when set, is enforced: the client must send a matching header. + """ + expires_in = min(expires_in, self.MAX_SIGNED_URL_EXPIRY) + + name = SimpleStorageName(key) + params: dict[str, str] = asdict(name) + if content_type: + params["ContentType"] = content_type + + try: + signed_url = self.aws.client.generate_presigned_url( + "put_object", + Params=params, + ExpiresIn=expires_in, + ) + logger.info( + f"[AmazonCloudStorage.get_signed_upload_url] Signed upload URL generated | " + f"{{'project_id': '{self.project_id}', 'bucket': '{_mask(name.Bucket)}', 'key': '{_mask(name.Key)}', 'expires_in': {expires_in}}}" + ) + return signed_url + except ClientError as err: + logger.error( + f"[AmazonCloudStorage.get_signed_upload_url] AWS presign error | " + f"{{'project_id': '{self.project_id}', 'bucket': '{_mask(name.Bucket)}', 'key': '{_mask(name.Key)}', 'error': '{str(err)}'}}", + exc_info=True, + ) + raise CloudStorageError(f'AWS Error: "{err}" ({key})') from err + def delete(self, url: str) -> None: name = SimpleStorageName.from_url(url) kwargs = asdict(name) diff --git a/backend/app/crud/document/document.py b/backend/app/crud/document/document.py index 1acf6cda3..7790624b5 100644 --- a/backend/app/crud/document/document.py +++ b/backend/app/crud/document/document.py @@ -33,6 +33,10 @@ def read_one(self, doc_id: UUID) -> Document: return result + def exists(self, doc_id: UUID) -> bool: + # Ignores deleted_at and project scope: the PK stays taken after a soft delete. + return self.session.get(Document, doc_id) is not None + def read_many( self, skip: int | None = None, diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index 863506047..94adeb270 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -86,7 +86,10 @@ DocTransformationJobsPublic, Document, DocumentPublic, + DocumentRegisterRequest, DocumentUploadResponse, + DocumentUploadURLRequest, + DocumentUploadURLResponse, TransformationJobInfo, TransformedDocumentPublic, ) diff --git a/backend/app/models/document.py b/backend/app/models/document.py index 3f3c80996..6009a8762 100644 --- a/backend/app/models/document.py +++ b/backend/app/models/document.py @@ -117,6 +117,33 @@ class DocumentUploadResponse(DocumentPublic): transformation_job: TransformationJobInfo | None = None +class DocumentUploadURLRequest(SQLModel): + filename: str = Field( + min_length=1, + max_length=255, + description="Original filename including its extension, e.g. report.pdf", + ) + + +class DocumentUploadURLResponse(SQLModel): + document_id: UUID = Field( + description="Identifier to register the document with once the upload completes" + ) + upload_url: str = Field(description="Pre-signed URL to PUT the file contents to") + expires_in: int = Field(description="Lifetime of the upload URL in seconds") + + +class DocumentRegisterRequest(SQLModel): + document_id: UUID = Field( + description="The document_id returned by the upload URL endpoint" + ) + filename: str = Field( + min_length=1, + max_length=255, + description="Original filename including its extension, e.g. report.pdf", + ) + + class DocTransformationJobPublic(SQLModel): job_id: UUID source_document_id: UUID diff --git a/backend/app/services/documents/helpers.py b/backend/app/services/documents/helpers.py index ffbb1096b..5868267d4 100644 --- a/backend/app/services/documents/helpers.py +++ b/backend/app/services/documents/helpers.py @@ -2,11 +2,13 @@ from typing import Optional, Tuple, Iterable, Union from uuid import UUID +from botocore.exceptions import ClientError from fastapi import HTTPException, UploadFile from sqlmodel import Session -from app.core.cloud.storage import CloudStorage +from app.core.cloud.storage import CloudStorage, CloudStorageError +from app.services.collections.helpers import MAX_DOC_SIZE_MB from app.services.doctransform.registry import ( get_available_transformers, get_file_format, @@ -87,6 +89,59 @@ def calculate_file_size(file: UploadFile) -> float: return round(size_bytes / 1024) +def validate_filename_format(filename: str) -> str: + """Resolve document format from the extension; HTTPException(400) if unsupported.""" + try: + return get_file_format(filename) + except ValueError as e: + logger.warning( + f"[validate_filename_format] Unsupported file extension | filename: {filename}" + ) + raise HTTPException(status_code=400, detail=str(e)) + + +def verify_uploaded_object( + *, + storage: CloudStorage, + object_store_url: str, + document_id: UUID, +) -> float: + """Confirm the uploaded object exists and fits the size budget; return size in KB.""" + try: + file_size_kb = storage.get_file_size_kb(object_store_url) + except CloudStorageError as e: + # Only a missing object is the client's fault; other S3 failures stay 500. + cause = e.__cause__ + if not ( + isinstance(cause, ClientError) + and cause.response.get("Error", {}).get("Code") in ("404", "NoSuchKey") + ): + raise + logger.warning( + f"[verify_uploaded_object] No object found at expected key | document_id: {document_id}" + ) + raise HTTPException( + status_code=400, + detail="No uploaded file found for this document_id. Upload the file to the " + "pre-signed URL before registering it.", + ) + + file_size_mb = file_size_kb / 1024 + if file_size_mb > MAX_DOC_SIZE_MB: + storage.delete(object_store_url) + logger.warning( + f"[verify_uploaded_object] Document size exceeds limit | " + f"document_id: {document_id}, size_mb: {round(file_size_mb, 2)}, max_size_mb: {MAX_DOC_SIZE_MB}" + ) + raise HTTPException( + status_code=413, + detail=f"Document size ({round(file_size_mb, 2)} MB) exceeds the maximum allowed size of {MAX_DOC_SIZE_MB} MB. " + f"Please upload a smaller file.", + ) + + return file_size_kb + + def pre_transform_validation( *, src_filename: str, @@ -102,10 +157,7 @@ def pre_transform_validation( Returns: (source_format, actual_transformer_or_none) Raises: HTTPException(400) on client errors. """ - try: - source_format = get_file_format(src_filename) - except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + source_format = validate_filename_format(src_filename) actual_transformer: Optional[str] = None if target_format: diff --git a/backend/app/tests/api/routes/documents/conftest.py b/backend/app/tests/api/routes/documents/conftest.py index d36dc181c..03db1a6e8 100644 --- a/backend/app/tests/api/routes/documents/conftest.py +++ b/backend/app/tests/api/routes/documents/conftest.py @@ -1,6 +1,9 @@ +import os + import pytest from starlette.testclient import TestClient +from app.core.config import settings from app.tests.utils.auth import TestAuthContext from app.tests.utils.document import WebCrawler @@ -9,3 +12,12 @@ def crawler(client: TestClient, user_api_key: TestAuthContext) -> WebCrawler: """Provides a WebCrawler instance for document API testing.""" return WebCrawler(client, user_api_key=user_api_key) + + +@pytest.fixture(scope="class") +def aws_credentials() -> None: + os.environ["AWS_ACCESS_KEY_ID"] = "testing" + os.environ["AWS_SECRET_ACCESS_KEY"] = "testing" + os.environ["AWS_SECURITY_TOKEN"] = "testing" + os.environ["AWS_SESSION_TOKEN"] = "testing" + os.environ["AWS_DEFAULT_REGION"] = settings.AWS_DEFAULT_REGION diff --git a/backend/app/tests/api/routes/documents/test_route_document_register_v2.py b/backend/app/tests/api/routes/documents/test_route_document_register_v2.py new file mode 100644 index 000000000..d3a40a0d6 --- /dev/null +++ b/backend/app/tests/api/routes/documents/test_route_document_register_v2.py @@ -0,0 +1,212 @@ +from unittest.mock import patch +from uuid import UUID, uuid4 + +import pytest +import requests +from botocore.exceptions import ClientError +from fastapi.testclient import TestClient +from httpx import Response +from moto import mock_aws +from sqlmodel import Session + +from app.core.cloud import AmazonCloudStorageClient +from app.core.cloud.storage import AmazonCloudStorage +from app.core.config import settings +from app.core.util import now +from app.models import Document +from app.services.collections.helpers import MAX_DOC_SIZE_MB +from app.tests.utils.auth import TestAuthContext +from app.tests.utils.document import DocumentMaker + +REGISTER_ROUTE = f"{settings.API_V2_STR}/documents" +UPLOAD_URL_ROUTE = f"{settings.API_V2_STR}/documents/upload-url" + + +def object_key(auth: TestAuthContext, document_id: UUID) -> str: + return f"{auth.project.storage_path}/{document_id}" + + +def put_object(key: str, body: bytes) -> None: + AmazonCloudStorageClient().client.put_object( + Bucket=settings.AWS_S3_BUCKET, Key=key, Body=body + ) + + +def register( + client: TestClient, auth: TestAuthContext, document_id: UUID, filename: str +) -> Response: + return client.post( + REGISTER_ROUTE, + headers={"X-API-KEY": auth.key}, + json={"document_id": str(document_id), "filename": filename}, + ) + + +@mock_aws +@pytest.mark.usefixtures("aws_credentials") +class TestDocumentRegisterV2: + def test_registers_uploaded_object( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + document_id = uuid4() + key = object_key(user_api_key, document_id) + put_object(key, b"x" * 2048) + + response = register(client, user_api_key, document_id, "report.pdf") + + assert response.status_code == 201 + data = response.json()["data"] + assert data["id"] == str(document_id) + assert data["fname"] == "report.pdf" + assert data["transformation_job"] is None + assert data["signed_url"] + + document = db.get(Document, document_id) + assert document is not None + assert document.fname == "report.pdf" + assert document.file_size_kb == 2.0 + assert document.object_store_url == f"s3://{settings.AWS_S3_BUCKET}/{key}" + assert document.project_id == user_api_key.project_id + + def test_missing_object_is_rejected( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + document_id = uuid4() + + response = register(client, user_api_key, document_id, "report.pdf") + + assert response.status_code == 400 + assert "No uploaded file found" in response.json()["error"] + assert db.get(Document, document_id) is None + + def test_oversized_object_is_rejected_and_deleted( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + aws = AmazonCloudStorageClient() + aws.create() + document_id = uuid4() + key = object_key(user_api_key, document_id) + put_object(key, b"x" * 1024) + + # Faking the reported size keeps a >25 MB body out of the test. + oversized_kb = (MAX_DOC_SIZE_MB + 1) * 1024 + with patch.object( + AmazonCloudStorage, "get_file_size_kb", return_value=oversized_kb + ): + response = register(client, user_api_key, document_id, "report.pdf") + + assert response.status_code == 413 + assert "exceeds the maximum allowed size" in response.json()["error"] + assert db.get(Document, document_id) is None + + with pytest.raises(ClientError) as excinfo: + aws.client.head_object(Bucket=settings.AWS_S3_BUCKET, Key=key) + assert excinfo.value.response["Error"]["Code"] == "404" + + def test_duplicate_document_id_is_rejected( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + existing = next(DocumentMaker(project_id=user_api_key.project_id, session=db)) + db.add(existing) + db.commit() + put_object(object_key(user_api_key, existing.id), b"x" * 1024) + + response = register(client, user_api_key, existing.id, "report.pdf") + + assert response.status_code == 409 + assert "already registered" in response.json()["error"] + + db.refresh(existing) + assert existing.fname != "report.pdf" + + def test_soft_deleted_document_id_is_rejected( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + deleted = next(DocumentMaker(project_id=user_api_key.project_id, session=db)) + deleted.deleted_at = now() + db.add(deleted) + db.commit() + + response = register(client, user_api_key, deleted.id, "report.pdf") + + assert response.status_code == 409 + assert "already registered" in response.json()["error"] + + def test_unsupported_extension_is_rejected( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + document_id = uuid4() + put_object(object_key(user_api_key, document_id), b"x" * 1024) + + response = register(client, user_api_key, document_id, "report.xyz") + + assert response.status_code == 400 + assert "Unsupported file extension: .xyz" in response.json()["error"] + assert db.get(Document, document_id) is None + + def test_missing_api_key_is_unauthorized(self, client: TestClient) -> None: + response = client.post( + REGISTER_ROUTE, + json={"document_id": str(uuid4()), "filename": "report.pdf"}, + ) + + assert response.status_code == 401 + + +@mock_aws +@pytest.mark.usefixtures("aws_credentials") +class TestDocumentUploadRoundTripV2: + def test_upload_url_then_put_then_register( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + + url_response = client.post( + UPLOAD_URL_ROUTE, + headers={"X-API-KEY": user_api_key.key}, + json={"filename": "handbook.pdf"}, + ) + assert url_response.status_code == 200 + upload_url = url_response.json()["data"]["upload_url"] + document_id = UUID(url_response.json()["data"]["document_id"]) + + put_response = requests.put(upload_url, data=b"y" * 3072) + assert put_response.status_code == 200 + + response = register(client, user_api_key, document_id, "handbook.pdf") + + assert response.status_code == 201 + assert response.json()["data"]["id"] == str(document_id) + + document = db.get(Document, document_id) + assert document is not None + assert document.file_size_kb == 3.0 + assert document.object_store_url == ( + f"s3://{settings.AWS_S3_BUCKET}/{object_key(user_api_key, document_id)}" + ) diff --git a/backend/app/tests/api/routes/documents/test_route_document_upload_url_v2.py b/backend/app/tests/api/routes/documents/test_route_document_upload_url_v2.py new file mode 100644 index 000000000..314169312 --- /dev/null +++ b/backend/app/tests/api/routes/documents/test_route_document_upload_url_v2.py @@ -0,0 +1,98 @@ +from uuid import UUID + +import pytest +from fastapi.testclient import TestClient +from moto import mock_aws +from sqlmodel import Session + +from app.core.cloud import AmazonCloudStorageClient +from app.core.config import settings +from app.models import Document +from app.tests.utils.auth import TestAuthContext + +UPLOAD_URL_ROUTE = f"{settings.API_V2_STR}/documents/upload-url" + + +@mock_aws +@pytest.mark.usefixtures("aws_credentials") +class TestDocumentUploadURLV2: + def test_returns_presigned_put_url_for_new_document_id( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + + response = client.post( + UPLOAD_URL_ROUTE, + headers={"X-API-KEY": user_api_key.key}, + json={"filename": "quarterly-report.pdf"}, + ) + + assert response.status_code == 200 + data = response.json()["data"] + assert data["expires_in"] == 3600 + + document_id = UUID(data["document_id"]) + key = f"{user_api_key.project.storage_path}/{document_id}" + assert key in data["upload_url"] + assert "X-Amz-Signature" in data["upload_url"] + + def test_does_not_create_document_row( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + + response = client.post( + UPLOAD_URL_ROUTE, + headers={"X-API-KEY": user_api_key.key}, + json={"filename": "notes.txt"}, + ) + + document_id = UUID(response.json()["data"]["document_id"]) + assert db.get(Document, document_id) is None + + def test_unsupported_extension_is_rejected( + self, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + response = client.post( + UPLOAD_URL_ROUTE, + headers={"X-API-KEY": user_api_key.key}, + json={"filename": "notes.xyz"}, + ) + + assert response.status_code == 400 + assert "Unsupported file extension: .xyz" in response.json()["error"] + + def test_blank_filename_is_rejected( + self, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + response = client.post( + UPLOAD_URL_ROUTE, + headers={"X-API-KEY": user_api_key.key}, + json={"filename": ""}, + ) + + assert response.status_code == 422 + + def test_missing_api_key_is_unauthorized(self, client: TestClient) -> None: + response = client.post(UPLOAD_URL_ROUTE, json={"filename": "notes.txt"}) + + assert response.status_code == 401 + + def test_invalid_api_key_is_unauthorized(self, client: TestClient) -> None: + response = client.post( + UPLOAD_URL_ROUTE, + headers={"X-API-KEY": "ApiKey not-a-real-key"}, + json={"filename": "notes.txt"}, + ) + + assert response.status_code == 401 diff --git a/backend/app/tests/core/cloud/test_storage.py b/backend/app/tests/core/cloud/test_storage.py index 95e9ff7ff..563e2604e 100644 --- a/backend/app/tests/core/cloud/test_storage.py +++ b/backend/app/tests/core/cloud/test_storage.py @@ -1,8 +1,21 @@ """Tests for app.core.cloud.storage helpers.""" +import os from unittest.mock import patch +from urllib.parse import parse_qs, urlparse +from uuid import uuid4 -from app.core.cloud.storage import GCS_SCOPES, build_gcp_sa_credentials +import pytest +from botocore.exceptions import ClientError +from moto import mock_aws + +from app.core.cloud.storage import ( + GCS_SCOPES, + AmazonCloudStorage, + CloudStorageError, + build_gcp_sa_credentials, +) +from app.core.config import settings def test_build_gcp_sa_credentials_passes_key_and_scopes(): @@ -14,3 +27,62 @@ def test_build_gcp_sa_credentials_passes_key_and_scopes(): mock_from_info.assert_called_once_with(sa_key, scopes=list(GCS_SCOPES)) assert creds is mock_from_info.return_value + + +@pytest.fixture(scope="class") +def aws_credentials(): + os.environ["AWS_ACCESS_KEY_ID"] = "testing" + os.environ["AWS_SECRET_ACCESS_KEY"] = "testing" + os.environ["AWS_SECURITY_TOKEN"] = "testing" + os.environ["AWS_SESSION_TOKEN"] = "testing" + os.environ["AWS_DEFAULT_REGION"] = settings.AWS_DEFAULT_REGION + + +@mock_aws +@pytest.mark.usefixtures("aws_credentials") +class TestGetSignedUploadURL: + def test_url_targets_the_requested_key(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + key = f"{storage.storage_path}/{uuid4()}" + + url = storage.get_signed_upload_url(key) + + parsed = urlparse(url) + assert parsed.path.endswith(key) + assert settings.AWS_S3_BUCKET in f"{parsed.netloc}{parsed.path}" + assert "X-Amz-Signature" in parse_qs(parsed.query) + + def test_content_type_is_signed_when_given(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + + url = storage.get_signed_upload_url("key.pdf", content_type="application/pdf") + + signed_headers = parse_qs(urlparse(url).query)["X-Amz-SignedHeaders"][0] + assert "content-type" in signed_headers + + def test_expiry_is_capped_at_one_day(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + + url = storage.get_signed_upload_url("key.pdf", expires_in=7 * 24 * 3600) + + assert parse_qs(urlparse(url).query)["X-Amz-Expires"] == ["86400"] + + def test_shorter_expiry_is_preserved(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + + url = storage.get_signed_upload_url("key.pdf", expires_in=600) + + assert parse_qs(urlparse(url).query)["X-Amz-Expires"] == ["600"] + + def test_aws_error_is_wrapped(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + error = ClientError( + {"Error": {"Code": "AccessDenied", "Message": "denied"}}, + "PutObject", + ) + + with patch.object( + storage.aws.client, "generate_presigned_url", side_effect=error + ): + with pytest.raises(CloudStorageError, match="AccessDenied"): + storage.get_signed_upload_url("key.pdf") diff --git a/docs/wiki/modules/knowledge-base.md b/docs/wiki/modules/knowledge-base.md index c6b8462df..92a66ef5a 100644 --- a/docs/wiki/modules/knowledge-base.md +++ b/docs/wiki/modules/knowledge-base.md @@ -6,7 +6,8 @@ Deep dive: `docs/architecture/kaapi-knowledge-base-ARCHITECTURE.md` (§3 upload, All paths relative to `backend/app/`. ## Routes -- `api/routes/documents.py` — upload/list +- `api/routes/documents.py` — upload/list (v1 multipart upload, the only path that transforms) +- `api/routes/documents_v2.py` — v2 pre-signed upload: `POST /upload-url` (issues a PUT URL) then `POST ""` (registers the uploaded object); no transformation - `api/routes/collections.py`, `api/routes/collection_job.py` — collection CRUD + job status - `api/routes/doc_transformation_job.py` — transform job status @@ -34,6 +35,7 @@ All paths relative to `backend/app/`. ## Gotchas - Uploads de-duplicate by provider file ID (see deep dive §7). +- v2 register trusts only the extension and the object's size — bytes never reach the backend, so `validate_document_content` sniffing (v1 only) is skipped. Register verifies the object exists at `{storage_path}/{document_id}` and deletes it when oversized. - Collections are immutable-ish: deletion semantics in deep dive §10. - OpenAI file-batch id: the SDK's `file_batches.poll()` / `upload_and_poll()` final return deserializes a vector-store body, so its `.id` is the `vs_` id, not the `vsfb_` batch id. `crud/rag/open_ai.py` captures the batch id from `create()` before polling and uses it for `list_files`. Any failed file is a hard failure (whole vector store rolled back); partial indexing needs an add-documents endpoint first. - The SDK's `file_batches.poll()` never times out. `_poll_file_batch` polls `retrieve` in a loop with no internal deadline — the Celery soft time limit bounds it, and its `SoftTimeLimitExceeded` aborts the task. An earlier version took a deadline from the caller via a `task_budget` `ContextVar` (and a fixed `BATCH_POLL_TIMEOUT_SECONDS`); both were deleted. Don't reintroduce caller coupling here. diff --git a/features/documents-v2-presigned-upload/PLAN.md b/features/documents-v2-presigned-upload/PLAN.md new file mode 100644 index 000000000..a95629ee1 --- /dev/null +++ b/features/documents-v2-presigned-upload/PLAN.md @@ -0,0 +1,94 @@ +# Documents v2 Presigned Upload — Implementation Plan + +Source spec: GitHub issue #1169 (verbal description, see Open Questions). Related: closed issue #1143. + +## Summary + +Add a v2 documents surface that replaces multipart upload through the backend with a two-step presigned-URL flow: the client requests a presigned PUT URL (JSON), uploads the file directly to S3, then registers the document (JSON) which creates the `document` row. Document transformation is not part of v2 upload; it stays a v1-only concern. v1 endpoints stay as they are, not deprecated (user decision). No schema change, no new tables. + +## Blast Radius + +Primary entities: Document (new write path only, same row shape). + +| Surface | Hop | Impact | Decision | +|---|---|---|---| +| Document (table) | 0 | New rows created via v2 register endpoint; identical columns and key layout (`{storage_path}/{document_id}`) | in scope | +| DocumentCollection / Collection | 1 | None, consumes Document rows which keep the same shape | out of scope | +| DocTransformationJob | 1 | Untouched; v2 register does not schedule transformations (user decision) | out of scope | +| FineTuning / ModelEvaluation | 1 | None, read Document rows unchanged | out of scope | +| Object storage (`core/cloud/storage.py`) | ext | New `get_signed_upload_url` method on `CloudStorage` + `AmazonCloudStorage` (presigned `put_object`) | in scope | +| kaapi-frontend console | ext | Keeps using v1 unchanged; migration is a later frontend task | deferred | +| Glific | ext | v2 endpoint docs describe the new flow; v1 untouched | in scope (docs only) | +| Langfuse | ext | Unaffected, no LLM call path touched | out of scope | +| Provider batch APIs | ext | Unaffected | out of scope | + +## Steps + +### 1. Core: presigned PUT support in storage +- Files: `backend/app/core/cloud/storage.py` (change) +- Add abstract `get_signed_upload_url(self, key: str, content_type: str | None = None, expires_in: int = 3600) -> str` to `CloudStorage`; implement in `AmazonCloudStorage` via `generate_presigned_url("put_object", ...)`, capping `expires_in` at `MAX_SIGNED_URL_EXPIRY`, logging per convention. +- Depends on: nothing + +### 2. Model: v2 request/response schemas +- Files: `backend/app/models/document.py` (change), `backend/app/models/__init__.py` (change) +- Add non-table SQLModel schemas: + - `DocumentUploadURLRequest` (`filename: str`) + - `DocumentUploadURLResponse` (`document_id: UUID`, `upload_url: str`, `expires_in: int`) + - `DocumentRegisterRequest` (`document_id: UUID`, `filename: str`) +- Response for register reuses existing `DocumentUploadResponse` (its `transformation_job` stays `None` in v2). +- Export new names from `models/__init__.py`. +- Depends on: nothing + +### 3. Route: v2 documents endpoints +- Files: `backend/app/api/routes/documents_v2.py` (new), `backend/app/api/docs/documents/upload_url_v2.md` (new), `backend/app/api/docs/documents/register_v2.md` (new) +- Router: `APIRouter(prefix="/documents", tags=["Documents v2"])`, mounted under `/api/v2` (step 4). Both endpoints `application/json`, `require_permission(Permission.REQUIRE_PROJECT)`. +- `POST /documents/upload-url`: + - Validate filename via `get_file_format` (rejects unsupported extensions early). + - `document_id = uuid4()`; key `{project.storage_path}/{document_id}` via `SimpleStorageName`, matching v1 key layout. + - Return `DocumentUploadURLResponse` in `APIResponse`. +- `POST /documents` (register): + - Validate filename extension via `get_file_format`. + - Verify object exists at the expected key with `storage.get_file_size_kb` (404 → 400 "file not uploaded"); enforce `MAX_DOC_SIZE_MB` (413, delete oversized object). + - Reject a `document_id` that already exists in `document` (409) to keep register idempotent-safe. + - Create `Document` row via `DocumentCrud.update`, return `DocumentUploadResponse` with fresh `get_signed_url`. No transformation scheduling in v2. +- Depends on: steps 1, 2 + +### 4. Wiring: mount v2 router +- Files: `backend/app/api/main.py` (change) +- Import `documents_v2` router, `api_v2_router.include_router(documents_v2.router)`. +- Depends on: step 3 + +### 5. v1 endpoints unchanged +- v1 upload is NOT deprecated (user decision); no change to `backend/app/api/routes/documents.py` or `upload.md`. + +### 6. Wiki update +- Files: `docs/wiki/modules/knowledge-base.md` (change) +- Routes section gains `api/routes/documents_v2.py` (v2 presigned flow) and notes v1 upload deprecated. No `domain-map.md` change (no entity or edge change). +- Depends on: step 3 + +### 7. Tests +- Files: `backend/app/tests/api/routes/documents/test_route_document_upload_url_v2.py` (new), `backend/app/tests/api/routes/documents/test_route_document_register_v2.py` (new), `backend/app/tests/core/test_storage.py` or nearest existing storage test (change, presign method) +- See Tests section. +- Depends on: steps 1-4 + +## Migration + +None. No table or column changes. + +## Tests + +Moto (`mock_aws`) for S3, matching `app/tests/api/routes/documents/` fixtures: + +- upload-url: happy path returns `document_id` + `upload_url` + `expires_in`; unsupported extension → 400; missing project permission → 403. +- register: happy path (object pre-put into moto bucket) creates Document row, returns signed URL; object absent → 400; oversized object → 413 and object deleted; duplicate `document_id` → 409; unsupported extension → 400. +- storage: `get_signed_upload_url` returns URL containing the key, expiry capped at `MAX_SIGNED_URL_EXPIRY`. + +## Open Questions + +Assumptions made (issue is short; all inferred, flagged here): + +- Two-endpoint flow (upload-url then register) chosen over one endpoint returning a presigned URL plus a pending DB row, to avoid adding an upload-status column and a migration. Register verifies the object server-side instead. +- Content-type sniffing (`validate_document_content`) is skipped in v2; only extension validation and size enforcement run, since bytes never pass through the backend. Downstream transformers already fail cleanly on malformed content. If sniffing is required, register would stream the first bytes from S3. +- No v1 endpoint is deprecated (user decision); v2 exists alongside v1. +- v2 upload excludes document transformation entirely (user decision); clients needing transforms keep using v1 until a v2 transform story exists. +- `expires_in` for the presigned PUT fixed at 3600s (matches existing signed GET default).