From 25c4b3961dc2fd6990148e4782666f26ef9a12a6 Mon Sep 17 00:00:00 2001 From: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com> Date: Thu, 3 Sep 2026 19:59:26 +0530 Subject: [PATCH 1/6] feat(documents): Add v2 presigned-URL upload endpoints Two-step JSON flow replacing multipart upload through the backend: POST /api/v2/documents/upload-url issues a presigned PUT URL, the client uploads directly to S3, then POST /api/v2/documents registers the document. Register verifies the object exists, enforces the 25 MB limit (deleting oversized objects), and rejects duplicate document ids. Transformation stays v1-only. --- backend/app/api/docs/documents/register_v2.md | 7 + .../app/api/docs/documents/upload_url_v2.md | 9 + backend/app/api/main.py | 5 +- backend/app/api/routes/documents_v2.py | 125 +++++++++++ backend/app/core/cloud/storage.py | 46 ++++ backend/app/crud/document/document.py | 4 + backend/app/models/__init__.py | 3 + backend/app/models/document.py | 27 +++ backend/app/services/documents/helpers.py | 62 ++++- .../tests/api/routes/documents/conftest.py | 12 + .../test_route_document_register_v2.py | 212 ++++++++++++++++++ .../test_route_document_upload_url_v2.py | 98 ++++++++ backend/app/tests/core/cloud/test_storage.py | 74 +++++- docs/wiki/modules/knowledge-base.md | 4 +- .../documents-v2-presigned-upload/PLAN.md | 94 ++++++++ 15 files changed, 774 insertions(+), 8 deletions(-) create mode 100644 backend/app/api/docs/documents/register_v2.md create mode 100644 backend/app/api/docs/documents/upload_url_v2.md create mode 100644 backend/app/api/routes/documents_v2.py create mode 100644 backend/app/tests/api/routes/documents/test_route_document_register_v2.py create mode 100644 backend/app/tests/api/routes/documents/test_route_document_upload_url_v2.py create mode 100644 features/documents-v2-presigned-upload/PLAN.md 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). From 783c2aff19d2866f83abcd0f118d6220626c7722 Mon Sep 17 00:00:00 2001 From: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:50:49 +0530 Subject: [PATCH 2/6] refactor(documents): Reshape v2 upload into resource-style endpoints Review follow-up on the v2 presigned upload. Endpoints are now verb-free and document_id moves out of the request body: POST /api/v2/documents/uploads -> 200 {document_id, upload_url, expires_in} PUT /api/v2/documents/{document_id} -> 201 DocumentPublic Uploads stage at pending/{storage_path}/{document_id}{ext} and are copied to the final {storage_path}/{document_id} on registration. The final key is unchanged from v1, and the key served by get_signed_url was never presigned for PUT, so a stale upload URL can no longer overwrite a registered document. The staging prefix leads the key because S3 lifecycle filters are literal prefixes: with the per-project segment in front, no single rule could reap abandoned uploads. A rule expiring pending/ after one day is live on the staging and production buckets. Baking the extension into the staging key binds the two calls - registering under a different extension misses the key and returns the ordinary 400 - so no filename comparison is needed. Also: - ObjectNotFoundError lets callers stop unwrapping botocore error codes across the service boundary - url_for / staging_url_for are the only places an object key is built - SignedUpload reports the effective expiry rather than the requested one - IntegrityError on registration returns 409 instead of 500 when two registrations race past the exists() check - register responds with DocumentPublic; DocumentUploadResponse carries a transformation_job that is always null on v2 - the v2 upload policy moves out of the route into services/documents/registration.py --- backend/app/api/docs/documents/initiate_v2.md | 11 ++ backend/app/api/docs/documents/register_v2.md | 8 +- .../app/api/docs/documents/upload_url_v2.md | 9 -- backend/app/api/routes/documents_v2.py | 114 +++++----------- backend/app/core/cloud/__init__.py | 2 + backend/app/core/cloud/storage.py | 123 ++++++++++++----- backend/app/models/__init__.py | 5 +- backend/app/models/document.py | 21 +-- backend/app/services/documents/helpers.py | 49 +------ .../app/services/documents/registration.py | 93 +++++++++++++ .../tests/api/routes/documents/conftest.py | 12 -- .../test_route_document_register_v2.py | 126 ++++++++++++++---- ...2.py => test_route_document_uploads_v2.py} | 74 ++++++++-- backend/app/tests/conftest.py | 10 ++ backend/app/tests/core/cloud/test_storage.py | 126 +++++++++++++----- docs/wiki/modules/knowledge-base.md | 13 +- .../documents-v2-presigned-upload/PLAN.md | 94 ------------- 17 files changed, 524 insertions(+), 366 deletions(-) create mode 100644 backend/app/api/docs/documents/initiate_v2.md delete mode 100644 backend/app/api/docs/documents/upload_url_v2.md create mode 100644 backend/app/services/documents/registration.py rename backend/app/tests/api/routes/documents/{test_route_document_upload_url_v2.py => test_route_document_uploads_v2.py} (51%) delete mode 100644 features/documents-v2-presigned-upload/PLAN.md diff --git a/backend/app/api/docs/documents/initiate_v2.md b/backend/app/api/docs/documents/initiate_v2.md new file mode 100644 index 000000000..ba70fbaa1 --- /dev/null +++ b/backend/app/api/docs/documents/initiate_v2.md @@ -0,0 +1,11 @@ +Open a v2 upload session: get a pre-signed URL to send a document straight to Kaapi's object storage. + +Step 1 of the three-step flow: + +1. `POST /api/v2/documents/uploads` with the filename — returns a `document_id` and an `upload_url`. +2. `PUT` the raw file bytes to `upload_url` — the body is the file itself, with no auth header, no form encoding, and no extra headers. +3. `PUT /api/v2/documents/{document_id}` with the same filename to create the document. + +The filename extension is validated here, so an unsupported file type fails before any upload. Nothing is persisted at this step — hence the `200` rather than a `201` — the `document_id` only becomes a document once you register it in step 3. The filename sent in step 3 must be the same one this URL was issued for: its extension determines where the bytes are staged, so a different extension will find nothing to register. + +`upload_url` is valid for `expires_in` seconds (the effective value after server-side capping, which may be shorter than requested); open a new upload session if it lapses. Maximum file size is 25 MB, enforced at registration rather than at upload time — a pre-signed PUT cannot enforce a content-length range (that would require a pre-signed POST, which would break the raw-PUT contract). diff --git a/backend/app/api/docs/documents/register_v2.md b/backend/app/api/docs/documents/register_v2.md index 5008b3697..e31c7b1ae 100644 --- a/backend/app/api/docs/documents/register_v2.md +++ b/backend/app/api/docs/documents/register_v2.md @@ -1,7 +1,9 @@ -Register a document that was uploaded to the pre-signed URL from `POST /api/v2/documents/upload-url`. +Register a document at the `document_id` issued by `POST /api/v2/documents/uploads`, from the bytes staged at its pre-signed 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. +Final step of the v2 upload flow. Send the filename the upload URL was issued for — its extension determines where the bytes were staged. The staged object is moved to its permanent location, 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. +Errors: `400` if nothing is staged for that `document_id` and filename (the upload never happened, lapsed, or the filename differs from the one the URL was issued for) or the extension is unsupported; `413` if the uploaded file exceeds 25 MB, in which case the staged object is deleted; `409` if the `document_id` was already registered — open a new upload session in that case. + +The 25 MB cap is enforced here rather than at upload time: a pre-signed PUT cannot enforce a content-length range (that would require a pre-signed POST, which would break the raw-PUT contract). 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 deleted file mode 100644 index 37353741f..000000000 --- a/backend/app/api/docs/documents/upload_url_v2.md +++ /dev/null @@ -1,9 +0,0 @@ -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/routes/documents_v2.py b/backend/app/api/routes/documents_v2.py index aa4d18574..007ddf3a7 100644 --- a/backend/app/api/routes/documents_v2.py +++ b/backend/app/api/routes/documents_v2.py @@ -1,125 +1,83 @@ -"""v2 document upload: pre-signed PUT to storage, then registration. No transformation.""" +"""v2 document upload: pre-signed PUT to a staging key, then registration.""" -import logging from pathlib import Path -from uuid import uuid4 +from uuid import UUID, uuid4 -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends +from fastapi import Path as FastPath 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, + DocumentUploadInitiateResponse, + DocumentUploadRequest, ) +from app.services.documents.helpers import validate_filename_format +from app.services.documents.registration import register_uploaded_document 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], + "/uploads", + description=load_description("documents/initiate_v2.md"), + response_model=APIResponse[DocumentUploadInitiateResponse], dependencies=[Depends(require_permission(Permission.REQUIRE_PROJECT))], ) def create_upload_url( session: SessionDep, current_user: AuthContextDep, - request: DocumentUploadURLRequest, -) -> APIResponse[DocumentUploadURLResponse]: + request: DocumentUploadRequest, +) -> APIResponse[DocumentUploadInitiateResponse]: 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(), + extension = Path(request.filename).suffix.lower() + # Extension baked into the staging key: a changed filename at registration simply misses it. + signed = storage.get_signed_upload_url( + Path(f"{document_id}{extension}"), 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( + return APIResponse[DocumentUploadInitiateResponse].success_response( + DocumentUploadInitiateResponse( document_id=document_id, - upload_url=upload_url, - expires_in=UPLOAD_URL_EXPIRY_SECONDS, + upload_url=signed.url, + expires_in=signed.expires_in, ) ) -@router.post( - "", +@router.put( + "/{document_id}", description=load_description("documents/register_v2.md"), status_code=201, - response_model=APIResponse[DocumentUploadResponse], + response_model=APIResponse[DocumentPublic], 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, - ) + request: DocumentUploadRequest, + document_id: UUID = FastPath( + description="Document id issued by the upload session" + ), +) -> APIResponse[DocumentPublic]: + document = register_uploaded_document( + session=session, + project_id=current_user.project_.id, + document_id=document_id, + filename=request.filename, ) + storage = get_cloud_storage(session=session, 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()) - ) + return APIResponse[DocumentPublic].success_response(document_schema) diff --git a/backend/app/core/cloud/__init__.py b/backend/app/core/cloud/__init__.py index b6b0b08ec..1e68e7389 100644 --- a/backend/app/core/cloud/__init__.py +++ b/backend/app/core/cloud/__init__.py @@ -3,6 +3,8 @@ AmazonCloudStorageClient, CloudStorage, CloudStorageError, + ObjectNotFoundError, + SignedUpload, get_cloud_storage, upload_audio_to_gcs, ) diff --git a/backend/app/core/cloud/storage.py b/backend/app/core/cloud/storage.py index 33815f2ea..d7c3146e8 100644 --- a/backend/app/core/cloud/storage.py +++ b/backend/app/core/cloud/storage.py @@ -10,7 +10,7 @@ from urllib.parse import ParseResult, urlparse, urlunparse from abc import ABC, abstractmethod -from typing import Any +from typing import Any, NamedTuple import boto3 from fastapi import UploadFile from botocore.exceptions import ClientError @@ -36,6 +36,28 @@ class CloudStorageError(Exception): pass +class ObjectNotFoundError(CloudStorageError): + pass + + +MISSING_OBJECT_CODES = ("404", "NoSuchKey") + + +def _to_storage_error(err: ClientError, url: str) -> CloudStorageError: + """Map a botocore ClientError onto the storage exception hierarchy.""" + message = f'AWS Error: "{err}" ({url})' + code = err.response.get("Error", {}).get("Code") + if code in MISSING_OBJECT_CODES: + return ObjectNotFoundError(message) + return CloudStorageError(message) + + +class SignedUpload(NamedTuple): + url: str + # Effective expiry after capping, which may be shorter than what the caller asked for. + expires_in: int + + class AmazonCloudStorageClient: @ft.cached_property def client(self): @@ -125,11 +147,34 @@ def from_url(cls, url: str): return cls(Bucket=url.netloc, Key=str(path)) +# Leads the key rather than following storage_path: S3 lifecycle filters are literal +# prefixes with no wildcard, so a per-project segment in front makes abandoned uploads +# unreapable by a single rule. +# WARNING: a live S3 lifecycle rule deletes everything under this prefix after 1 day +# (staging + production buckets). Never write anything here that must outlive a day. +STAGING_PREFIX = "pending" + + class CloudStorage(ABC): def __init__(self, project_id: int, storage_path: UUID): self.project_id = project_id self.storage_path = str(storage_path) + def url_for(self, file_path: Path) -> SimpleStorageName: + """Resolve a project-relative path into a fully qualified storage name. + + The single place storage_path is joined — callers never build keys themselves. + """ + if file_path.is_absolute(): + raise ValueError("file_path must be relative to the project's storage root") + key = Path(self.storage_path) / file_path + return SimpleStorageName(key.as_posix()) + + def staging_url_for(self, file_path: Path) -> SimpleStorageName: + """Resolve a pre-registration staging key, under the bucket-wide staging prefix.""" + name = self.url_for(file_path) + return SimpleStorageName(f"{STAGING_PREFIX}/{name.Key}", name.Bucket) + @abstractmethod def put(self, source: UploadFile, filepath: Path) -> SimpleStorageName: """Upload a file to storage""" @@ -157,12 +202,18 @@ def get_signed_url(self, url: str, expires_in: int = 3600) -> str: @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""" + self, file_path: Path, expires_in: int = 3600 + ) -> SignedUpload: + """Generate a signed URL the client can upload (PUT) to directly. + + Always resolves through the staging prefix: nothing is ever presigned to a + final key, or an abandoned upload would be indistinguishable from a document. + """ + pass + + @abstractmethod + def copy(self, source_url: str, destination: Path) -> SimpleStorageName: + """Server-side copy of an existing object to a project-relative path""" pass @abstractmethod @@ -177,10 +228,7 @@ def __init__(self, project_id: int, storage_path: UUID): self.aws = AmazonCloudStorageClient() def put(self, source: UploadFile, file_path: Path) -> SimpleStorageName: - if file_path.is_absolute(): - raise ValueError("file_path must be relative to the project's storage root") - key = Path(self.storage_path) / file_path - destination = SimpleStorageName(key.as_posix()) + destination = self.url_for(file_path) kwargs = asdict(destination) try: @@ -221,7 +269,7 @@ def stream(self, url: str) -> StreamingBody: 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}" ({url})') from err + raise _to_storage_error(err, url) from err def get(self, url: str) -> bytes: name = SimpleStorageName.from_url(url) @@ -240,7 +288,7 @@ def get(self, url: str) -> bytes: 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}" ({url})') from err + raise _to_storage_error(err, url) from err def get_file_size_kb(self, url: str) -> float: name = SimpleStorageName.from_url(url) @@ -260,7 +308,7 @@ def get_file_size_kb(self, url: str) -> float: 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}" ({url})') from err + raise _to_storage_error(err, url) from err # Maximum allowed expiry for signed URLs (24 hours) MAX_SIGNED_URL_EXPIRY = 86400 @@ -296,40 +344,47 @@ 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: + self, file_path: Path, expires_in: int = 3600 + ) -> SignedUpload: """ - 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. + Generate a signed S3 URL the client can PUT raw bytes to, under the staging prefix. + No content type is signed, so the client sends no headers beyond the body. """ 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 - + name = self.staging_url_for(file_path) try: signed_url = self.aws.client.generate_presigned_url( "put_object", - Params=params, + Params=asdict(name), 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 + return SignedUpload(url=signed_url, expires_in=expires_in) 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 + raise _to_storage_error(err, str(name)) from err + + def copy(self, source_url: str, destination: Path) -> SimpleStorageName: + source = SimpleStorageName.from_url(source_url) + target = self.url_for(destination) + try: + self.aws.client.copy_object( + Bucket=target.Bucket, + Key=target.Key, + CopySource={"Bucket": source.Bucket, "Key": source.Key}, + ) + return target + except ClientError as err: + logger.error( + f"[AmazonCloudStorage.copy] AWS copy error | " + f"{{'project_id': '{self.project_id}', 'bucket': '{_mask(target.Bucket)}', 'source_key': '{_mask(source.Key)}', 'key': '{_mask(target.Key)}', 'error': '{str(err)}'}}", + exc_info=True, + ) + raise _to_storage_error(err, source_url) from err def delete(self, url: str) -> None: name = SimpleStorageName.from_url(url) @@ -346,7 +401,7 @@ def delete(self, url: str) -> None: 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}" ({url})') from err + raise _to_storage_error(err, url) from err def get_cloud_storage(session: Session, project_id: int) -> CloudStorage: diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index 94adeb270..8a1559573 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -86,10 +86,9 @@ DocTransformationJobsPublic, Document, DocumentPublic, - DocumentRegisterRequest, + DocumentUploadInitiateResponse, + DocumentUploadRequest, DocumentUploadResponse, - DocumentUploadURLRequest, - DocumentUploadURLResponse, TransformationJobInfo, TransformedDocumentPublic, ) diff --git a/backend/app/models/document.py b/backend/app/models/document.py index 6009a8762..d487d12c0 100644 --- a/backend/app/models/document.py +++ b/backend/app/models/document.py @@ -117,7 +117,7 @@ class DocumentUploadResponse(DocumentPublic): transformation_job: TransformationJobInfo | None = None -class DocumentUploadURLRequest(SQLModel): +class DocumentUploadRequest(SQLModel): filename: str = Field( min_length=1, max_length=255, @@ -125,22 +125,15 @@ class DocumentUploadURLRequest(SQLModel): ) -class DocumentUploadURLResponse(SQLModel): +class DocumentUploadInitiateResponse(SQLModel): document_id: UUID = Field( - description="Identifier to register the document with once the upload completes" + description="Identifier of the document to be; the completion endpoint takes it as a path parameter" ) - 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" + upload_url: str = Field( + description="Pre-signed URL to PUT the raw file bytes to, with no auth header and no form encoding" ) - filename: str = Field( - min_length=1, - max_length=255, - description="Original filename including its extension, e.g. report.pdf", + expires_in: int = Field( + description="Effective lifetime of the upload URL in seconds, after server-side capping" ) diff --git a/backend/app/services/documents/helpers.py b/backend/app/services/documents/helpers.py index 5868267d4..5e002d9ea 100644 --- a/backend/app/services/documents/helpers.py +++ b/backend/app/services/documents/helpers.py @@ -2,13 +2,11 @@ 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, CloudStorageError +from app.core.cloud.storage import CloudStorage -from app.services.collections.helpers import MAX_DOC_SIZE_MB from app.services.doctransform.registry import ( get_available_transformers, get_file_format, @@ -94,54 +92,9 @@ def validate_filename_format(filename: str) -> str: 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, diff --git a/backend/app/services/documents/registration.py b/backend/app/services/documents/registration.py new file mode 100644 index 000000000..286a964ac --- /dev/null +++ b/backend/app/services/documents/registration.py @@ -0,0 +1,93 @@ +"""v2 upload policy: verify what the client staged, then promote it to a document row.""" + +from pathlib import Path +from uuid import UUID + +from fastapi import HTTPException +from sqlalchemy.exc import IntegrityError +from sqlmodel import Session + +from app.core.cloud import get_cloud_storage +from app.core.cloud.storage import CloudStorage, ObjectNotFoundError +from app.crud import DocumentCrud +from app.models import Document +from app.services.collections.helpers import MAX_DOC_SIZE_MB +from app.services.documents.helpers import validate_filename_format + +DUPLICATE_DOCUMENT_DETAIL = ( + "This document_id is already registered. Request a new upload URL." +) + + +def verify_staged_object( + *, + storage: CloudStorage, + staged_url: str, + document_id: UUID, +) -> float: + """Confirm the staged object exists and fits the size budget; return size in KB.""" + try: + file_size_kb = storage.get_file_size_kb(staged_url) + except ObjectNotFoundError: + raise HTTPException( + status_code=400, + detail="No uploaded file found for this document_id. Upload the file to the " + "pre-signed URL first, and pass the same filename the upload URL was issued " + "for — its extension determines where the bytes were staged.", + ) + + file_size_mb = file_size_kb / 1024 + if file_size_mb > MAX_DOC_SIZE_MB: + storage.delete(staged_url) + 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 register_uploaded_document( + *, + session: Session, + project_id: int, + document_id: UUID, + filename: str, +) -> Document: + """Promote a staged upload into a document row, moving the object to its final key.""" + validate_filename_format(filename) + + document_crud = DocumentCrud(session, project_id) + if document_crud.exists(document_id): + raise HTTPException(status_code=409, detail=DUPLICATE_DOCUMENT_DETAIL) + + storage = get_cloud_storage(session=session, project_id=project_id) + extension = Path(filename).suffix.lower() + staged_url = str(storage.staging_url_for(Path(f"{document_id}{extension}"))) + + file_size_kb = verify_staged_object( + storage=storage, + staged_url=staged_url, + document_id=document_id, + ) + + object_store_url = storage.copy(staged_url, Path(str(document_id))) + storage.delete(staged_url) + + try: + document = document_crud.update( + Document( + id=document_id, + fname=filename, + file_size_kb=file_size_kb, + object_store_url=str(object_store_url), + project_id=project_id, + ) + ) + except IntegrityError: + # Two completions can both clear exists(); the PK decides, and the object is the winner's. + session.rollback() + raise HTTPException(status_code=409, detail=DUPLICATE_DOCUMENT_DETAIL) + + return document diff --git a/backend/app/tests/api/routes/documents/conftest.py b/backend/app/tests/api/routes/documents/conftest.py index 03db1a6e8..d36dc181c 100644 --- a/backend/app/tests/api/routes/documents/conftest.py +++ b/backend/app/tests/api/routes/documents/conftest.py @@ -1,9 +1,6 @@ -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 @@ -12,12 +9,3 @@ 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 index d3a40a0d6..a64d395fb 100644 --- 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 @@ -12,17 +12,24 @@ from app.core.cloud import AmazonCloudStorageClient from app.core.cloud.storage import AmazonCloudStorage from app.core.config import settings +from app.core.db import engine from app.core.util import now +from app.crud import DocumentCrud 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" +DOCUMENTS_ROUTE = f"{settings.API_V2_STR}/documents" +UPLOADS_ROUTE = f"{DOCUMENTS_ROUTE}/uploads" -def object_key(auth: TestAuthContext, document_id: UUID) -> str: +def staged_key(auth: TestAuthContext, document_id: UUID, extension: str) -> str: + # The staging prefix leads the key so one S3 lifecycle rule covers every project. + return f"pending/{auth.project.storage_path}/{document_id}{extension}" + + +def final_key(auth: TestAuthContext, document_id: UUID) -> str: return f"{auth.project.storage_path}/{document_id}" @@ -32,20 +39,28 @@ def put_object(key: str, body: bytes) -> None: ) +def assert_absent(key: str) -> None: + with pytest.raises(ClientError) as excinfo: + AmazonCloudStorageClient().client.head_object( + Bucket=settings.AWS_S3_BUCKET, Key=key + ) + assert excinfo.value.response["Error"]["Code"] == "404" + + def register( client: TestClient, auth: TestAuthContext, document_id: UUID, filename: str ) -> Response: - return client.post( - REGISTER_ROUTE, + return client.put( + f"{DOCUMENTS_ROUTE}/{document_id}", headers={"X-API-KEY": auth.key}, - json={"document_id": str(document_id), "filename": filename}, + json={"filename": filename}, ) @mock_aws @pytest.mark.usefixtures("aws_credentials") class TestDocumentRegisterV2: - def test_registers_uploaded_object( + def test_registers_staged_object_under_its_final_key( self, db: Session, client: TestClient, @@ -53,8 +68,8 @@ def test_registers_uploaded_object( ) -> None: AmazonCloudStorageClient().create() document_id = uuid4() - key = object_key(user_api_key, document_id) - put_object(key, b"x" * 2048) + staged = staged_key(user_api_key, document_id, ".pdf") + put_object(staged, b"x" * 2048) response = register(client, user_api_key, document_id, "report.pdf") @@ -62,16 +77,41 @@ def test_registers_uploaded_object( 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.object_store_url == ( + f"s3://{settings.AWS_S3_BUCKET}/{final_key(user_api_key, document_id)}" + ) assert document.project_id == user_api_key.project_id + assert_absent(staged) + + def test_extension_other_than_the_presigned_one_is_rejected( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + document_id = uuid4() + staged = staged_key(user_api_key, document_id, ".pdf") + put_object(staged, b"x" * 1024) + + response = register(client, user_api_key, document_id, "report.txt") + + assert response.status_code == 400 + assert "No uploaded file found" in response.json()["error"] + assert db.get(Document, document_id) is None + # The bytes the client actually staged are untouched, so a retry with the + # right filename still works. + assert AmazonCloudStorageClient().client.head_object( + Bucket=settings.AWS_S3_BUCKET, Key=staged + ) + def test_missing_object_is_rejected( self, db: Session, @@ -93,11 +133,10 @@ def test_oversized_object_is_rejected_and_deleted( client: TestClient, user_api_key: TestAuthContext, ) -> None: - aws = AmazonCloudStorageClient() - aws.create() + AmazonCloudStorageClient().create() document_id = uuid4() - key = object_key(user_api_key, document_id) - put_object(key, b"x" * 1024) + staged = staged_key(user_api_key, document_id, ".pdf") + put_object(staged, b"x" * 1024) # Faking the reported size keeps a >25 MB body out of the test. oversized_kb = (MAX_DOC_SIZE_MB + 1) * 1024 @@ -109,10 +148,8 @@ def test_oversized_object_is_rejected_and_deleted( 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" + assert_absent(staged) + assert_absent(final_key(user_api_key, document_id)) def test_duplicate_document_id_is_rejected( self, @@ -124,7 +161,7 @@ def test_duplicate_document_id_is_rejected( 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) + put_object(staged_key(user_api_key, existing.id, ".pdf"), b"x" * 1024) response = register(client, user_api_key, existing.id, "report.pdf") @@ -134,6 +171,40 @@ def test_duplicate_document_id_is_rejected( db.refresh(existing) assert existing.fname != "report.pdf" + def test_concurrent_registration_loses_the_insert_race( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + winner = next(DocumentMaker(project_id=user_api_key.project_id, session=db)) + document_id, original_fname = winner.id, winner.fname + # Committed outside the test transaction: the winner of the race is another + # request's session, so the request under test can only hit the PK constraint. + with Session(engine) as outside: + outside.add(winner) + outside.commit() + put_object(staged_key(user_api_key, document_id, ".pdf"), b"x" * 1024) + + try: + # exists() returning False simulates the racing request that also saw no row. + with patch.object(DocumentCrud, "exists", return_value=False): + response = register(client, user_api_key, document_id, "report.pdf") + + assert response.status_code == 409 + assert "already registered" in response.json()["error"] + + survivor = db.get(Document, document_id) + assert survivor is not None + assert survivor.fname == original_fname + finally: + with Session(engine) as cleanup: + row = cleanup.get(Document, document_id) + if row is not None: + cleanup.delete(row) + cleanup.commit() + def test_soft_deleted_document_id_is_rejected( self, db: Session, @@ -159,7 +230,7 @@ def test_unsupported_extension_is_rejected( ) -> None: AmazonCloudStorageClient().create() document_id = uuid4() - put_object(object_key(user_api_key, document_id), b"x" * 1024) + put_object(staged_key(user_api_key, document_id, ".xyz"), b"x" * 1024) response = register(client, user_api_key, document_id, "report.xyz") @@ -168,9 +239,9 @@ def test_unsupported_extension_is_rejected( 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"}, + response = client.put( + f"{DOCUMENTS_ROUTE}/{uuid4()}", + json={"filename": "report.pdf"}, ) assert response.status_code == 401 @@ -179,7 +250,7 @@ def test_missing_api_key_is_unauthorized(self, client: TestClient) -> None: @mock_aws @pytest.mark.usefixtures("aws_credentials") class TestDocumentUploadRoundTripV2: - def test_upload_url_then_put_then_register( + def test_uploads_then_put_then_register( self, db: Session, client: TestClient, @@ -188,7 +259,7 @@ def test_upload_url_then_put_then_register( AmazonCloudStorageClient().create() url_response = client.post( - UPLOAD_URL_ROUTE, + UPLOADS_ROUTE, headers={"X-API-KEY": user_api_key.key}, json={"filename": "handbook.pdf"}, ) @@ -208,5 +279,6 @@ def test_upload_url_then_put_then_register( 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)}" + f"s3://{settings.AWS_S3_BUCKET}/{final_key(user_api_key, document_id)}" ) + assert_absent(staged_key(user_api_key, document_id, ".pdf")) 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_uploads_v2.py similarity index 51% rename from backend/app/tests/api/routes/documents/test_route_document_upload_url_v2.py rename to backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py index 314169312..1da1eb64f 100644 --- a/backend/app/tests/api/routes/documents/test_route_document_upload_url_v2.py +++ b/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py @@ -1,3 +1,4 @@ +from urllib.parse import urlparse from uuid import UUID import pytest @@ -10,13 +11,24 @@ from app.models import Document from app.tests.utils.auth import TestAuthContext -UPLOAD_URL_ROUTE = f"{settings.API_V2_STR}/documents/upload-url" +UPLOADS_ROUTE = f"{settings.API_V2_STR}/documents/uploads" + + +def signed_key(upload_url: str) -> str: + """The object key a pre-signed URL points at, with host and bucket stripped.""" + path = urlparse(upload_url).path.lstrip("/") + return path.removeprefix(f"{settings.AWS_S3_BUCKET}/") + + +def staged_key(auth: TestAuthContext, document_id: str, extension: str) -> str: + # The staging prefix leads the key so one S3 lifecycle rule covers every project. + return f"pending/{auth.project.storage_path}/{document_id}{extension}" @mock_aws @pytest.mark.usefixtures("aws_credentials") -class TestDocumentUploadURLV2: - def test_returns_presigned_put_url_for_new_document_id( +class TestDocumentUploadsV2: + def test_returns_presigned_put_url_for_staging_key( self, db: Session, client: TestClient, @@ -25,7 +37,7 @@ def test_returns_presigned_put_url_for_new_document_id( AmazonCloudStorageClient().create() response = client.post( - UPLOAD_URL_ROUTE, + UPLOADS_ROUTE, headers={"X-API-KEY": user_api_key.key}, json={"filename": "quarterly-report.pdf"}, ) @@ -35,10 +47,50 @@ def test_returns_presigned_put_url_for_new_document_id( 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"] + staging_key = f"pending/{user_api_key.project.storage_path}/{document_id}.pdf" + assert staging_key in data["upload_url"] assert "X-Amz-Signature" in data["upload_url"] + def test_upload_url_does_not_target_the_final_key( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + + response = client.post( + UPLOADS_ROUTE, + headers={"X-API-KEY": user_api_key.key}, + json={"filename": "quarterly-report.pdf"}, + ) + + data = response.json()["data"] + # Compared whole: the final key is a substring of the staged one, so `not in` never holds. + key = signed_key(data["upload_url"]) + assert key == staged_key(user_api_key, data["document_id"], ".pdf") + assert key != f"{user_api_key.project.storage_path}/{data['document_id']}" + + def test_extension_is_lowercased_in_the_staging_key( + self, + db: Session, + client: TestClient, + user_api_key: TestAuthContext, + ) -> None: + AmazonCloudStorageClient().create() + + response = client.post( + UPLOADS_ROUTE, + headers={"X-API-KEY": user_api_key.key}, + json={"filename": "Quarterly-Report.PDF"}, + ) + + data = response.json()["data"] + staging_key = ( + f"pending/{user_api_key.project.storage_path}/{data['document_id']}.pdf" + ) + assert staging_key in data["upload_url"] + def test_does_not_create_document_row( self, db: Session, @@ -48,7 +100,7 @@ def test_does_not_create_document_row( AmazonCloudStorageClient().create() response = client.post( - UPLOAD_URL_ROUTE, + UPLOADS_ROUTE, headers={"X-API-KEY": user_api_key.key}, json={"filename": "notes.txt"}, ) @@ -62,7 +114,7 @@ def test_unsupported_extension_is_rejected( user_api_key: TestAuthContext, ) -> None: response = client.post( - UPLOAD_URL_ROUTE, + UPLOADS_ROUTE, headers={"X-API-KEY": user_api_key.key}, json={"filename": "notes.xyz"}, ) @@ -76,7 +128,7 @@ def test_blank_filename_is_rejected( user_api_key: TestAuthContext, ) -> None: response = client.post( - UPLOAD_URL_ROUTE, + UPLOADS_ROUTE, headers={"X-API-KEY": user_api_key.key}, json={"filename": ""}, ) @@ -84,13 +136,13 @@ def test_blank_filename_is_rejected( 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"}) + response = client.post(UPLOADS_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, + UPLOADS_ROUTE, headers={"X-API-KEY": "ApiKey not-a-real-key"}, json={"filename": "notes.txt"}, ) diff --git a/backend/app/tests/conftest.py b/backend/app/tests/conftest.py index dfa6fa18c..4a4ce9f41 100644 --- a/backend/app/tests/conftest.py +++ b/backend/app/tests/conftest.py @@ -78,6 +78,16 @@ def seed_baseline( yield +@pytest.fixture +def aws_credentials(monkeypatch: pytest.MonkeyPatch) -> None: + """Dummy AWS creds for moto. setenv (not os.environ) so they unwind after the test.""" + monkeypatch.setenv("AWS_ACCESS_KEY_ID", "testing") + monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "testing") + monkeypatch.setenv("AWS_SECURITY_TOKEN", "testing") + monkeypatch.setenv("AWS_SESSION_TOKEN", "testing") + monkeypatch.setenv("AWS_DEFAULT_REGION", settings.AWS_DEFAULT_REGION) + + @pytest.fixture(scope="function") def client(db: Session) -> Generator[TestClient, None, None]: app.dependency_overrides[get_db] = lambda: db diff --git a/backend/app/tests/core/cloud/test_storage.py b/backend/app/tests/core/cloud/test_storage.py index 563e2604e..1610ebe9c 100644 --- a/backend/app/tests/core/cloud/test_storage.py +++ b/backend/app/tests/core/cloud/test_storage.py @@ -1,6 +1,6 @@ """Tests for app.core.cloud.storage helpers.""" -import os +from pathlib import Path from unittest.mock import patch from urllib.parse import parse_qs, urlparse from uuid import uuid4 @@ -12,7 +12,11 @@ from app.core.cloud.storage import ( GCS_SCOPES, AmazonCloudStorage, + AmazonCloudStorageClient, CloudStorageError, + ObjectNotFoundError, + SimpleStorageName, + _to_storage_error, build_gcp_sa_credentials, ) from app.core.config import settings @@ -29,13 +33,39 @@ def test_build_gcp_sa_credentials_passes_key_and_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 +def client_error(code: str, operation: str = "HeadObject") -> ClientError: + return ClientError({"Error": {"Code": code, "Message": code}}, operation) + + +class TestUrlFor: + def test_joins_the_projects_storage_path(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + + name = storage.url_for(Path("pending") / "report.pdf") + + assert name.Key == f"{storage.storage_path}/pending/report.pdf" + assert name.Bucket == settings.AWS_S3_BUCKET + + def test_absolute_path_is_rejected(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + + with pytest.raises(ValueError, match="must be relative"): + storage.url_for(Path("/etc/passwd")) + + +class TestToStorageError: + @pytest.mark.parametrize("code", ["404", "NoSuchKey"]) + def test_missing_object_codes_become_object_not_found(self, code: str) -> None: + error = _to_storage_error(client_error(code), "s3://bucket/key") + + assert isinstance(error, ObjectNotFoundError) + assert "s3://bucket/key" in str(error) + + def test_other_codes_stay_generic(self) -> None: + error = _to_storage_error(client_error("AccessDenied"), "s3://bucket/key") + + assert isinstance(error, CloudStorageError) + assert not isinstance(error, ObjectNotFoundError) @mock_aws @@ -43,46 +73,82 @@ def 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()}" + file_path = Path("pending") / f"{uuid4()}.pdf" - url = storage.get_signed_upload_url(key) + signed = storage.get_signed_upload_url(file_path) - parsed = urlparse(url) - assert parsed.path.endswith(key) + parsed = urlparse(signed.url) + assert parsed.path.endswith(f"{storage.storage_path}/{file_path}") 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) + signed = storage.get_signed_upload_url( + Path("key.pdf"), expires_in=7 * 24 * 3600 + ) - assert parse_qs(urlparse(url).query)["X-Amz-Expires"] == ["86400"] + assert signed.expires_in == 86400 + assert parse_qs(urlparse(signed.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) + signed = storage.get_signed_upload_url(Path("key.pdf"), expires_in=600) - assert parse_qs(urlparse(url).query)["X-Amz-Expires"] == ["600"] + assert signed.expires_in == 600 + assert parse_qs(urlparse(signed.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 + storage.aws.client, + "generate_presigned_url", + side_effect=client_error("AccessDenied", "PutObject"), ): with pytest.raises(CloudStorageError, match="AccessDenied"): - storage.get_signed_upload_url("key.pdf") + storage.get_signed_upload_url(Path("key.pdf")) + + +@mock_aws +@pytest.mark.usefixtures("aws_credentials") +class TestGetFileSizeKB: + def test_missing_key_raises_object_not_found(self) -> None: + AmazonCloudStorageClient().create() + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + url = str(storage.url_for(Path(f"{uuid4()}.pdf"))) + + with pytest.raises(ObjectNotFoundError): + storage.get_file_size_kb(url) + + +@mock_aws +@pytest.mark.usefixtures("aws_credentials") +class TestCopy: + def test_copies_the_source_bytes_to_the_destination_key(self) -> None: + aws = AmazonCloudStorageClient() + aws.create() + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + source = storage.url_for(Path("pending") / "staged.pdf") + aws.client.put_object( + Bucket=source.Bucket, Key=source.Key, Body=b"staged bytes" + ) + destination = Path("final.pdf") + + target = storage.copy(str(source), destination) + + assert target == SimpleStorageName( + Key=f"{storage.storage_path}/final.pdf", Bucket=settings.AWS_S3_BUCKET + ) + copied = aws.client.get_object(Bucket=target.Bucket, Key=target.Key) + assert copied["Body"].read() == b"staged bytes" + + def test_missing_source_raises_object_not_found(self) -> None: + AmazonCloudStorageClient().create() + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + source = str(storage.url_for(Path("pending") / "absent.pdf")) + + with pytest.raises(ObjectNotFoundError): + storage.copy(source, Path("final.pdf")) diff --git a/docs/wiki/modules/knowledge-base.md b/docs/wiki/modules/knowledge-base.md index 92a66ef5a..e39ce3190 100644 --- a/docs/wiki/modules/knowledge-base.md +++ b/docs/wiki/modules/knowledge-base.md @@ -7,7 +7,7 @@ All paths relative to `backend/app/`. ## Routes - `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/documents_v2.py` — v2 pre-signed upload: `POST /documents/uploads` → 200 (issues a PUT URL to a staging key; nothing persisted) then `PUT /documents/{document_id}` → 201 (registers the staged 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 @@ -23,7 +23,7 @@ All paths relative to `backend/app/`. ## Services / CRUD - `services/collections/` — `create_collection.py`, `delete_collection.py`, `providers/`, `helpers.py` -- `services/documents/` — upload path +- `services/documents/` — `helpers.py` (v1 upload path), `registration.py` (v2 registration policy), `constants.py`, `validator.py` - `services/doctransform/` — `job.py`, `registry.py`, `transformer.py`, `zerox_transformer.py` - `crud/collection/`, `crud/document/`, `crud/document_collection.py`, `crud/rag/`, `crud/file.py` @@ -35,7 +35,14 @@ 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. +- v2 registration trusts only the extension and the object's size — bytes never reach the backend, so `validate_document_content` sniffing (v1 only) is skipped. Uploads are staged at `pending/{storage_path}/{document_id}{ext}`; registration verifies that key, copies to the final `{storage_path}/{document_id}` (same shape as v1), deletes the staged copy, and deletes the staged object instead when oversized. Because the extension is baked into the staging key, a filename mismatch between the two calls needs no explicit check — the key simply misses and the normal 400 fires. +- The staging prefix leads the key (`pending/{storage_path}/…`, not `{storage_path}/pending/…`) because **S3 lifecycle filters are literal prefixes with no wildcard support**. With the per-project `storage_path` in front, no single rule could match every project's staging area. `CloudStorage.staging_url_for` owns this layout and `get_signed_upload_url` always routes through it — never presign to a final key. +- **`pending/` has a 1-day TTL. Do not write anything else under it.** An S3 lifecycle rule (`expire-pending-uploads`, `Prefix: pending/`, `Expiration: 1 day`) is live on `ai-platform-documents-staging` and `-production`, and reaps abandoned v2 uploads — nothing in application code does. Consequences to know before touching this prefix: + - Any object written under `pending/`, by any code path, is **deleted within ~24-48h** (lifecycle sweeps run about once a day, so expiry is not exact). Never park anything there you expect to keep. + - If you do add a new writer under `pending/`, say so in this gotcha and in the PR — a reviewer cannot see the bucket config from the diff. + - Anything that must outlive a day belongs at a different prefix, with its own lifecycle rule. + - The rule is **not** applied to `ai-platform-documents-development`, so local/dev orphans accumulate until someone clears them by hand. + - The rule lives only in the bucket config, not in this repo or in Terraform. Re-creating a bucket does not re-create it. - 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 deleted file mode 100644 index a95629ee1..000000000 --- a/features/documents-v2-presigned-upload/PLAN.md +++ /dev/null @@ -1,94 +0,0 @@ -# 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). From bfa656286a5cff15bed538a7c443bb70fdd0ded8 Mon Sep 17 00:00:00 2001 From: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com> Date: Sun, 6 Sep 2026 22:10:44 +0530 Subject: [PATCH 3/6] refactor(documents): Address review feedback on the v2 upload Naming and scope cleanups from review. - STAGING_PREFIX -> PENDING_PREFIX and url_for's flag -> is_pending. "Staging" already means the deploy environment here, and the two appeared side by side in the same sentence; this prefix only ever meant "uploaded, not registered". The prefix value is unchanged, so keys and the lifecycle rule are unaffected. - Document is_pending in url_for's docstring, including the one-day TTL that governs anything written under the prefix. - Fold staging_url_for back into url_for rather than keeping a second function. - upload_url -> upload_signed_url, and finish its truncated field description. - Move validate_filename_format into registration.py so helpers.py and pre_transform_validation return to their state on main. The transformation path is out of scope for this PR and now carries no diff. - Drop the logger lines that sat immediately before a raise: the exception already carries the same facts, and Sentry sees it either way. - Rename crud to document_crud. --- backend/app/api/docs/documents/initiate_v2.md | 6 +-- backend/app/api/routes/documents_v2.py | 11 ++--- backend/app/core/cloud/storage.py | 41 ++++++++++--------- backend/app/models/document.py | 4 +- backend/app/services/documents/helpers.py | 13 ++---- .../app/services/documents/registration.py | 32 ++++++++++----- .../test_route_document_register_v2.py | 36 ++++++++-------- .../test_route_document_uploads_v2.py | 28 +++++++------ backend/app/tests/core/cloud/test_storage.py | 20 ++++++--- docs/wiki/modules/knowledge-base.md | 8 ++-- 10 files changed, 109 insertions(+), 90 deletions(-) diff --git a/backend/app/api/docs/documents/initiate_v2.md b/backend/app/api/docs/documents/initiate_v2.md index ba70fbaa1..07553c698 100644 --- a/backend/app/api/docs/documents/initiate_v2.md +++ b/backend/app/api/docs/documents/initiate_v2.md @@ -2,10 +2,10 @@ Open a v2 upload session: get a pre-signed URL to send a document straight to Ka Step 1 of the three-step flow: -1. `POST /api/v2/documents/uploads` with the filename — returns a `document_id` and an `upload_url`. -2. `PUT` the raw file bytes to `upload_url` — the body is the file itself, with no auth header, no form encoding, and no extra headers. +1. `POST /api/v2/documents/uploads` with the filename — returns a `document_id` and an `upload_signed_url`. +2. `PUT` the raw file bytes to `upload_signed_url` — the body is the file itself, with no auth header, no form encoding, and no extra headers. 3. `PUT /api/v2/documents/{document_id}` with the same filename to create the document. The filename extension is validated here, so an unsupported file type fails before any upload. Nothing is persisted at this step — hence the `200` rather than a `201` — the `document_id` only becomes a document once you register it in step 3. The filename sent in step 3 must be the same one this URL was issued for: its extension determines where the bytes are staged, so a different extension will find nothing to register. -`upload_url` is valid for `expires_in` seconds (the effective value after server-side capping, which may be shorter than requested); open a new upload session if it lapses. Maximum file size is 25 MB, enforced at registration rather than at upload time — a pre-signed PUT cannot enforce a content-length range (that would require a pre-signed POST, which would break the raw-PUT contract). +`upload_signed_url` is valid for `expires_in` seconds (the effective value after server-side capping, which may be shorter than requested); open a new upload session if it lapses. Maximum file size is 25 MB, enforced at registration rather than at upload time — a pre-signed PUT cannot enforce a content-length range (that would require a pre-signed POST, which would break the raw-PUT contract). diff --git a/backend/app/api/routes/documents_v2.py b/backend/app/api/routes/documents_v2.py index 007ddf3a7..7c81ce0d1 100644 --- a/backend/app/api/routes/documents_v2.py +++ b/backend/app/api/routes/documents_v2.py @@ -1,4 +1,4 @@ -"""v2 document upload: pre-signed PUT to a staging key, then registration.""" +"""v2 document upload: pre-signed PUT to a pending key, then registration.""" from pathlib import Path from uuid import UUID, uuid4 @@ -14,8 +14,10 @@ DocumentUploadInitiateResponse, DocumentUploadRequest, ) -from app.services.documents.helpers import validate_filename_format -from app.services.documents.registration import register_uploaded_document +from app.services.documents.registration import ( + register_uploaded_document, + validate_filename_format, +) from app.utils import APIResponse, load_description router = APIRouter(prefix="/documents", tags=["Documents v2"]) @@ -39,7 +41,6 @@ def create_upload_url( storage = get_cloud_storage(session=session, project_id=current_user.project_.id) document_id = uuid4() extension = Path(request.filename).suffix.lower() - # Extension baked into the staging key: a changed filename at registration simply misses it. signed = storage.get_signed_upload_url( Path(f"{document_id}{extension}"), expires_in=UPLOAD_URL_EXPIRY_SECONDS, @@ -48,7 +49,7 @@ def create_upload_url( return APIResponse[DocumentUploadInitiateResponse].success_response( DocumentUploadInitiateResponse( document_id=document_id, - upload_url=signed.url, + upload_signed_url=signed.url, expires_in=signed.expires_in, ) ) diff --git a/backend/app/core/cloud/storage.py b/backend/app/core/cloud/storage.py index d7c3146e8..a0753821d 100644 --- a/backend/app/core/cloud/storage.py +++ b/backend/app/core/cloud/storage.py @@ -147,12 +147,10 @@ def from_url(cls, url: str): return cls(Bucket=url.netloc, Key=str(path)) -# Leads the key rather than following storage_path: S3 lifecycle filters are literal -# prefixes with no wildcard, so a per-project segment in front makes abandoned uploads -# unreapable by a single rule. -# WARNING: a live S3 lifecycle rule deletes everything under this prefix after 1 day -# (staging + production buckets). Never write anything here that must outlive a day. -STAGING_PREFIX = "pending" +# Unregistered uploads. Unrelated to the staging *environment*: this is a holding +# area, and one literal-prefix lifecycle rule reaps it for every project. +PENDING_PREFIX = "pending" +PENDING_TTL_DAYS = 1 class CloudStorage(ABC): @@ -160,20 +158,25 @@ def __init__(self, project_id: int, storage_path: UUID): self.project_id = project_id self.storage_path = str(storage_path) - def url_for(self, file_path: Path) -> SimpleStorageName: + def url_for(self, file_path: Path, is_pending: bool = False) -> SimpleStorageName: """Resolve a project-relative path into a fully qualified storage name. - The single place storage_path is joined — callers never build keys themselves. + The single place a key is built — callers never join storage_path themselves. + + Args: + file_path: Path relative to the project's storage root. + is_pending: Place the key under ``PENDING_PREFIX`` — the holding area for + uploads that have not been registered yet. An S3 lifecycle rule deletes + everything there after ``PENDING_TTL_DAYS`` (1 day), so pass True only + for objects that are meant to disappear if registration never happens. + Registered documents use the default (False) and are never expired. """ if file_path.is_absolute(): raise ValueError("file_path must be relative to the project's storage root") - key = Path(self.storage_path) / file_path - return SimpleStorageName(key.as_posix()) - - def staging_url_for(self, file_path: Path) -> SimpleStorageName: - """Resolve a pre-registration staging key, under the bucket-wide staging prefix.""" - name = self.url_for(file_path) - return SimpleStorageName(f"{STAGING_PREFIX}/{name.Key}", name.Bucket) + roots = ( + (PENDING_PREFIX, self.storage_path) if is_pending else (self.storage_path,) + ) + return SimpleStorageName(Path(*roots, file_path).as_posix()) @abstractmethod def put(self, source: UploadFile, filepath: Path) -> SimpleStorageName: @@ -206,8 +209,8 @@ def get_signed_upload_url( ) -> SignedUpload: """Generate a signed URL the client can upload (PUT) to directly. - Always resolves through the staging prefix: nothing is ever presigned to a - final key, or an abandoned upload would be indistinguishable from a document. + Always resolves under PENDING_PREFIX: nothing is ever presigned to a final + key, or an abandoned upload would be indistinguishable from a document. """ pass @@ -347,12 +350,12 @@ def get_signed_upload_url( self, file_path: Path, expires_in: int = 3600 ) -> SignedUpload: """ - Generate a signed S3 URL the client can PUT raw bytes to, under the staging prefix. + Generate a signed S3 URL the client can PUT raw bytes to, under PENDING_PREFIX. No content type is signed, so the client sends no headers beyond the body. """ expires_in = min(expires_in, self.MAX_SIGNED_URL_EXPIRY) - name = self.staging_url_for(file_path) + name = self.url_for(file_path, is_pending=True) try: signed_url = self.aws.client.generate_presigned_url( "put_object", diff --git a/backend/app/models/document.py b/backend/app/models/document.py index d487d12c0..bcdb6077a 100644 --- a/backend/app/models/document.py +++ b/backend/app/models/document.py @@ -127,9 +127,9 @@ class DocumentUploadRequest(SQLModel): class DocumentUploadInitiateResponse(SQLModel): document_id: UUID = Field( - description="Identifier of the document to be; the completion endpoint takes it as a path parameter" + description="Identifier to register the document under; the registration endpoint takes it as a path parameter" ) - upload_url: str = Field( + upload_signed_url: str = Field( description="Pre-signed URL to PUT the raw file bytes to, with no auth header and no form encoding" ) expires_in: int = Field( diff --git a/backend/app/services/documents/helpers.py b/backend/app/services/documents/helpers.py index 5e002d9ea..ffbb1096b 100644 --- a/backend/app/services/documents/helpers.py +++ b/backend/app/services/documents/helpers.py @@ -87,14 +87,6 @@ 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: - raise HTTPException(status_code=400, detail=str(e)) - - def pre_transform_validation( *, src_filename: str, @@ -110,7 +102,10 @@ def pre_transform_validation( Returns: (source_format, actual_transformer_or_none) Raises: HTTPException(400) on client errors. """ - source_format = validate_filename_format(src_filename) + try: + source_format = get_file_format(src_filename) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) actual_transformer: Optional[str] = None if target_format: diff --git a/backend/app/services/documents/registration.py b/backend/app/services/documents/registration.py index 286a964ac..3e209b88e 100644 --- a/backend/app/services/documents/registration.py +++ b/backend/app/services/documents/registration.py @@ -12,22 +12,30 @@ from app.crud import DocumentCrud from app.models import Document from app.services.collections.helpers import MAX_DOC_SIZE_MB -from app.services.documents.helpers import validate_filename_format +from app.services.doctransform.registry import get_file_format DUPLICATE_DOCUMENT_DETAIL = ( "This document_id is already registered. Request a new upload URL." ) -def verify_staged_object( +def validate_filename_format(filename: str) -> str: + """Resolve the document format from the extension; HTTPException(400) if unsupported.""" + try: + return get_file_format(filename) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) + + +def verify_pending_object( *, storage: CloudStorage, - staged_url: str, + pending_url: str, document_id: UUID, ) -> float: - """Confirm the staged object exists and fits the size budget; return size in KB.""" + """Confirm the pending object exists and fits the size budget; return size in KB.""" try: - file_size_kb = storage.get_file_size_kb(staged_url) + file_size_kb = storage.get_file_size_kb(pending_url) except ObjectNotFoundError: raise HTTPException( status_code=400, @@ -38,7 +46,7 @@ def verify_staged_object( file_size_mb = file_size_kb / 1024 if file_size_mb > MAX_DOC_SIZE_MB: - storage.delete(staged_url) + storage.delete(pending_url) 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. " @@ -64,16 +72,18 @@ def register_uploaded_document( storage = get_cloud_storage(session=session, project_id=project_id) extension = Path(filename).suffix.lower() - staged_url = str(storage.staging_url_for(Path(f"{document_id}{extension}"))) + pending_url = str( + storage.url_for(Path(f"{document_id}{extension}"), is_pending=True) + ) - file_size_kb = verify_staged_object( + file_size_kb = verify_pending_object( storage=storage, - staged_url=staged_url, + pending_url=pending_url, document_id=document_id, ) - object_store_url = storage.copy(staged_url, Path(str(document_id))) - storage.delete(staged_url) + object_store_url = storage.copy(pending_url, Path(str(document_id))) + storage.delete(pending_url) try: document = document_crud.update( 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 index a64d395fb..7b3f51daa 100644 --- 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 @@ -24,8 +24,8 @@ UPLOADS_ROUTE = f"{DOCUMENTS_ROUTE}/uploads" -def staged_key(auth: TestAuthContext, document_id: UUID, extension: str) -> str: - # The staging prefix leads the key so one S3 lifecycle rule covers every project. +def pending_key(auth: TestAuthContext, document_id: UUID, extension: str) -> str: + # The pending prefix leads the key so one S3 lifecycle rule covers every project. return f"pending/{auth.project.storage_path}/{document_id}{extension}" @@ -60,7 +60,7 @@ def register( @mock_aws @pytest.mark.usefixtures("aws_credentials") class TestDocumentRegisterV2: - def test_registers_staged_object_under_its_final_key( + def test_registers_pending_object_under_its_final_key( self, db: Session, client: TestClient, @@ -68,8 +68,8 @@ def test_registers_staged_object_under_its_final_key( ) -> None: AmazonCloudStorageClient().create() document_id = uuid4() - staged = staged_key(user_api_key, document_id, ".pdf") - put_object(staged, b"x" * 2048) + pending = pending_key(user_api_key, document_id, ".pdf") + put_object(pending, b"x" * 2048) response = register(client, user_api_key, document_id, "report.pdf") @@ -88,7 +88,7 @@ def test_registers_staged_object_under_its_final_key( ) assert document.project_id == user_api_key.project_id - assert_absent(staged) + assert_absent(pending) def test_extension_other_than_the_presigned_one_is_rejected( self, @@ -98,18 +98,18 @@ def test_extension_other_than_the_presigned_one_is_rejected( ) -> None: AmazonCloudStorageClient().create() document_id = uuid4() - staged = staged_key(user_api_key, document_id, ".pdf") - put_object(staged, b"x" * 1024) + pending = pending_key(user_api_key, document_id, ".pdf") + put_object(pending, b"x" * 1024) response = register(client, user_api_key, document_id, "report.txt") assert response.status_code == 400 assert "No uploaded file found" in response.json()["error"] assert db.get(Document, document_id) is None - # The bytes the client actually staged are untouched, so a retry with the + # The bytes the client actually uploaded are untouched, so a retry with the # right filename still works. assert AmazonCloudStorageClient().client.head_object( - Bucket=settings.AWS_S3_BUCKET, Key=staged + Bucket=settings.AWS_S3_BUCKET, Key=pending ) def test_missing_object_is_rejected( @@ -135,8 +135,8 @@ def test_oversized_object_is_rejected_and_deleted( ) -> None: AmazonCloudStorageClient().create() document_id = uuid4() - staged = staged_key(user_api_key, document_id, ".pdf") - put_object(staged, b"x" * 1024) + pending = pending_key(user_api_key, document_id, ".pdf") + put_object(pending, b"x" * 1024) # Faking the reported size keeps a >25 MB body out of the test. oversized_kb = (MAX_DOC_SIZE_MB + 1) * 1024 @@ -148,7 +148,7 @@ def test_oversized_object_is_rejected_and_deleted( assert response.status_code == 413 assert "exceeds the maximum allowed size" in response.json()["error"] assert db.get(Document, document_id) is None - assert_absent(staged) + assert_absent(pending) assert_absent(final_key(user_api_key, document_id)) def test_duplicate_document_id_is_rejected( @@ -161,7 +161,7 @@ def test_duplicate_document_id_is_rejected( existing = next(DocumentMaker(project_id=user_api_key.project_id, session=db)) db.add(existing) db.commit() - put_object(staged_key(user_api_key, existing.id, ".pdf"), b"x" * 1024) + put_object(pending_key(user_api_key, existing.id, ".pdf"), b"x" * 1024) response = register(client, user_api_key, existing.id, "report.pdf") @@ -185,7 +185,7 @@ def test_concurrent_registration_loses_the_insert_race( with Session(engine) as outside: outside.add(winner) outside.commit() - put_object(staged_key(user_api_key, document_id, ".pdf"), b"x" * 1024) + put_object(pending_key(user_api_key, document_id, ".pdf"), b"x" * 1024) try: # exists() returning False simulates the racing request that also saw no row. @@ -230,7 +230,7 @@ def test_unsupported_extension_is_rejected( ) -> None: AmazonCloudStorageClient().create() document_id = uuid4() - put_object(staged_key(user_api_key, document_id, ".xyz"), b"x" * 1024) + put_object(pending_key(user_api_key, document_id, ".xyz"), b"x" * 1024) response = register(client, user_api_key, document_id, "report.xyz") @@ -264,7 +264,7 @@ def test_uploads_then_put_then_register( json={"filename": "handbook.pdf"}, ) assert url_response.status_code == 200 - upload_url = url_response.json()["data"]["upload_url"] + upload_url = url_response.json()["data"]["upload_signed_url"] document_id = UUID(url_response.json()["data"]["document_id"]) put_response = requests.put(upload_url, data=b"y" * 3072) @@ -281,4 +281,4 @@ def test_uploads_then_put_then_register( assert document.object_store_url == ( f"s3://{settings.AWS_S3_BUCKET}/{final_key(user_api_key, document_id)}" ) - assert_absent(staged_key(user_api_key, document_id, ".pdf")) + assert_absent(pending_key(user_api_key, document_id, ".pdf")) diff --git a/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py b/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py index 1da1eb64f..849876b5a 100644 --- a/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py +++ b/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py @@ -20,15 +20,15 @@ def signed_key(upload_url: str) -> str: return path.removeprefix(f"{settings.AWS_S3_BUCKET}/") -def staged_key(auth: TestAuthContext, document_id: str, extension: str) -> str: - # The staging prefix leads the key so one S3 lifecycle rule covers every project. +def pending_key(auth: TestAuthContext, document_id: str, extension: str) -> str: + # The pending prefix leads the key so one S3 lifecycle rule covers every project. return f"pending/{auth.project.storage_path}/{document_id}{extension}" @mock_aws @pytest.mark.usefixtures("aws_credentials") class TestDocumentUploadsV2: - def test_returns_presigned_put_url_for_staging_key( + def test_returns_presigned_put_url_for_pending_key( self, db: Session, client: TestClient, @@ -47,9 +47,11 @@ def test_returns_presigned_put_url_for_staging_key( assert data["expires_in"] == 3600 document_id = UUID(data["document_id"]) - staging_key = f"pending/{user_api_key.project.storage_path}/{document_id}.pdf" - assert staging_key in data["upload_url"] - assert "X-Amz-Signature" in data["upload_url"] + assert ( + pending_key(user_api_key, str(document_id), ".pdf") + in data["upload_signed_url"] + ) + assert "X-Amz-Signature" in data["upload_signed_url"] def test_upload_url_does_not_target_the_final_key( self, @@ -66,12 +68,12 @@ def test_upload_url_does_not_target_the_final_key( ) data = response.json()["data"] - # Compared whole: the final key is a substring of the staged one, so `not in` never holds. - key = signed_key(data["upload_url"]) - assert key == staged_key(user_api_key, data["document_id"], ".pdf") + # Compared whole: the final key is a substring of the pending one, so `not in` never holds. + key = signed_key(data["upload_signed_url"]) + assert key == pending_key(user_api_key, data["document_id"], ".pdf") assert key != f"{user_api_key.project.storage_path}/{data['document_id']}" - def test_extension_is_lowercased_in_the_staging_key( + def test_extension_is_lowercased_in_the_pending_key( self, db: Session, client: TestClient, @@ -86,10 +88,10 @@ def test_extension_is_lowercased_in_the_staging_key( ) data = response.json()["data"] - staging_key = ( - f"pending/{user_api_key.project.storage_path}/{data['document_id']}.pdf" + assert ( + pending_key(user_api_key, data["document_id"], ".pdf") + in data["upload_signed_url"] ) - assert staging_key in data["upload_url"] def test_does_not_create_document_row( self, diff --git a/backend/app/tests/core/cloud/test_storage.py b/backend/app/tests/core/cloud/test_storage.py index 1610ebe9c..9b1e5c10d 100644 --- a/backend/app/tests/core/cloud/test_storage.py +++ b/backend/app/tests/core/cloud/test_storage.py @@ -41,11 +41,19 @@ class TestUrlFor: def test_joins_the_projects_storage_path(self) -> None: storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) - name = storage.url_for(Path("pending") / "report.pdf") + name = storage.url_for(Path("report.pdf")) - assert name.Key == f"{storage.storage_path}/pending/report.pdf" + assert name.Key == f"{storage.storage_path}/report.pdf" assert name.Bucket == settings.AWS_S3_BUCKET + def test_pending_prefix_leads_the_key(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + + name = storage.url_for(Path("report.pdf"), is_pending=True) + + # Ahead of storage_path, not after it: one literal-prefix rule must match every project. + assert name.Key == f"pending/{storage.storage_path}/report.pdf" + def test_absolute_path_is_rejected(self) -> None: storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) @@ -131,9 +139,9 @@ def test_copies_the_source_bytes_to_the_destination_key(self) -> None: aws = AmazonCloudStorageClient() aws.create() storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) - source = storage.url_for(Path("pending") / "staged.pdf") + source = storage.url_for(Path("waiting.pdf"), is_pending=True) aws.client.put_object( - Bucket=source.Bucket, Key=source.Key, Body=b"staged bytes" + Bucket=source.Bucket, Key=source.Key, Body=b"pending bytes" ) destination = Path("final.pdf") @@ -143,12 +151,12 @@ def test_copies_the_source_bytes_to_the_destination_key(self) -> None: Key=f"{storage.storage_path}/final.pdf", Bucket=settings.AWS_S3_BUCKET ) copied = aws.client.get_object(Bucket=target.Bucket, Key=target.Key) - assert copied["Body"].read() == b"staged bytes" + assert copied["Body"].read() == b"pending bytes" def test_missing_source_raises_object_not_found(self) -> None: AmazonCloudStorageClient().create() storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) - source = str(storage.url_for(Path("pending") / "absent.pdf")) + source = str(storage.url_for(Path("absent.pdf"), is_pending=True)) with pytest.raises(ObjectNotFoundError): storage.copy(source, Path("final.pdf")) diff --git a/docs/wiki/modules/knowledge-base.md b/docs/wiki/modules/knowledge-base.md index e39ce3190..a93c68abc 100644 --- a/docs/wiki/modules/knowledge-base.md +++ b/docs/wiki/modules/knowledge-base.md @@ -7,7 +7,7 @@ All paths relative to `backend/app/`. ## Routes - `api/routes/documents.py` — upload/list (v1 multipart upload, the only path that transforms) -- `api/routes/documents_v2.py` — v2 pre-signed upload: `POST /documents/uploads` → 200 (issues a PUT URL to a staging key; nothing persisted) then `PUT /documents/{document_id}` → 201 (registers the staged object); no transformation +- `api/routes/documents_v2.py` — v2 pre-signed upload: `POST /documents/uploads` → 200 (issues a PUT URL to a pending key; nothing persisted) then `PUT /documents/{document_id}` → 201 (registers the pending 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 @@ -23,7 +23,7 @@ All paths relative to `backend/app/`. ## Services / CRUD - `services/collections/` — `create_collection.py`, `delete_collection.py`, `providers/`, `helpers.py` -- `services/documents/` — `helpers.py` (v1 upload path), `registration.py` (v2 registration policy), `constants.py`, `validator.py` +- `services/documents/` — `helpers.py` (v1 upload path), `registration.py` (v2 upload policy), `validator.py` - `services/doctransform/` — `job.py`, `registry.py`, `transformer.py`, `zerox_transformer.py` - `crud/collection/`, `crud/document/`, `crud/document_collection.py`, `crud/rag/`, `crud/file.py` @@ -35,8 +35,8 @@ All paths relative to `backend/app/`. ## Gotchas - Uploads de-duplicate by provider file ID (see deep dive §7). -- v2 registration trusts only the extension and the object's size — bytes never reach the backend, so `validate_document_content` sniffing (v1 only) is skipped. Uploads are staged at `pending/{storage_path}/{document_id}{ext}`; registration verifies that key, copies to the final `{storage_path}/{document_id}` (same shape as v1), deletes the staged copy, and deletes the staged object instead when oversized. Because the extension is baked into the staging key, a filename mismatch between the two calls needs no explicit check — the key simply misses and the normal 400 fires. -- The staging prefix leads the key (`pending/{storage_path}/…`, not `{storage_path}/pending/…`) because **S3 lifecycle filters are literal prefixes with no wildcard support**. With the per-project `storage_path` in front, no single rule could match every project's staging area. `CloudStorage.staging_url_for` owns this layout and `get_signed_upload_url` always routes through it — never presign to a final key. +- v2 registration trusts only the extension and the object's size — bytes never reach the backend, so `validate_document_content` sniffing (v1 only) is skipped. Uploads land at `pending/{storage_path}/{document_id}{ext}`; registration verifies that key, copies to the final `{storage_path}/{document_id}` (same shape as v1), deletes the pending copy, and deletes it instead when oversized. Because the extension is baked into the pending key, a filename mismatch between the two calls needs no explicit check — the key simply misses and the normal 400 fires. +- The pending prefix leads the key (`pending/{storage_path}/…`, not `{storage_path}/pending/…`) because **S3 lifecycle filters are literal prefixes with no wildcard support**. With the per-project `storage_path` in front, no single rule could match every project. `CloudStorage.url_for(path, is_pending=True)` owns this layout and `get_signed_upload_url` always routes through it — never presign to a final key. Note `PENDING_PREFIX` has nothing to do with the staging *environment*; it means "uploaded but not registered". - **`pending/` has a 1-day TTL. Do not write anything else under it.** An S3 lifecycle rule (`expire-pending-uploads`, `Prefix: pending/`, `Expiration: 1 day`) is live on `ai-platform-documents-staging` and `-production`, and reaps abandoned v2 uploads — nothing in application code does. Consequences to know before touching this prefix: - Any object written under `pending/`, by any code path, is **deleted within ~24-48h** (lifecycle sweeps run about once a day, so expiry is not exact). Never park anything there you expect to keep. - If you do add a new writer under `pending/`, say so in this gotcha and in the PR — a reviewer cannot see the bucket config from the diff. From d6f3703972643399ce511bf1baa27e266ad58e03 Mon Sep 17 00:00:00 2001 From: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com> Date: Sun, 6 Sep 2026 22:22:40 +0530 Subject: [PATCH 4/6] fix(documents): Keep the pending object until registration commits storage.delete ran between the copy and the DocumentCrud.update commit, so a failed insert left the final object with no row and nothing to retry from - the client's bytes were already gone. Delete after the row is committed instead: a failed insert leaves the upload retryable, and a pending object nobody comes back for expires on its own. Also correct the initiation contract in the docs. Only the extension is carried across the two calls, not the whole filename, so report.pdf then invoice.pdf is accepted and the step 3 name is what gets stored. --- backend/app/api/docs/documents/initiate_v2.md | 4 ++-- backend/app/services/documents/registration.py | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/backend/app/api/docs/documents/initiate_v2.md b/backend/app/api/docs/documents/initiate_v2.md index 07553c698..20c969445 100644 --- a/backend/app/api/docs/documents/initiate_v2.md +++ b/backend/app/api/docs/documents/initiate_v2.md @@ -4,8 +4,8 @@ Step 1 of the three-step flow: 1. `POST /api/v2/documents/uploads` with the filename — returns a `document_id` and an `upload_signed_url`. 2. `PUT` the raw file bytes to `upload_signed_url` — the body is the file itself, with no auth header, no form encoding, and no extra headers. -3. `PUT /api/v2/documents/{document_id}` with the same filename to create the document. +3. `PUT /api/v2/documents/{document_id}` with the filename to create the document. -The filename extension is validated here, so an unsupported file type fails before any upload. Nothing is persisted at this step — hence the `200` rather than a `201` — the `document_id` only becomes a document once you register it in step 3. The filename sent in step 3 must be the same one this URL was issued for: its extension determines where the bytes are staged, so a different extension will find nothing to register. +The filename extension is validated here, so an unsupported file type fails before any upload. Nothing is persisted at this step — hence the `200` rather than a `201` — the `document_id` only becomes a document once you register it in step 3. Only the **extension** is carried across the two calls: it determines where the bytes are staged, so step 3 must use the same extension or it will find nothing to register. The rest of the name is free to differ — `report.pdf` here and `invoice.pdf` in step 3 both work, and the step 3 name is what gets stored. `upload_signed_url` is valid for `expires_in` seconds (the effective value after server-side capping, which may be shorter than requested); open a new upload session if it lapses. Maximum file size is 25 MB, enforced at registration rather than at upload time — a pre-signed PUT cannot enforce a content-length range (that would require a pre-signed POST, which would break the raw-PUT contract). diff --git a/backend/app/services/documents/registration.py b/backend/app/services/documents/registration.py index 3e209b88e..dc6f335f3 100644 --- a/backend/app/services/documents/registration.py +++ b/backend/app/services/documents/registration.py @@ -83,7 +83,6 @@ def register_uploaded_document( ) object_store_url = storage.copy(pending_url, Path(str(document_id))) - storage.delete(pending_url) try: document = document_crud.update( @@ -100,4 +99,7 @@ def register_uploaded_document( session.rollback() raise HTTPException(status_code=409, detail=DUPLICATE_DOCUMENT_DETAIL) + # Only once the row is committed: a failed insert leaves the upload retryable, + # and an abandoned pending object expires on its own. + storage.delete(pending_url) return document From 91c8a709f256adf33bdedd9f3ee8e23813818987 Mon Sep 17 00:00:00 2001 From: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com> Date: Mon, 7 Sep 2026 09:35:20 +0530 Subject: [PATCH 5/6] feat(documents): Enforce upload size at the edge with a pre-signed POST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switch the v2 upload from a pre-signed PUT to a pre-signed POST so the 25 MB limit is enforced by S3 as the file uploads (content-length-range) rather than only at registration. An oversized file is rejected outright and never stored. The filename is signed into object metadata (x-amz-meta-filename), so it can no longer be swapped between issuing the ticket and registering, and the client no longer sends it twice — registration reads it back from the object and takes no request body. The pending key drops its extension as a result. Registration copies before it measures: the pending object stays writable through its ticket, so the size is read from the frozen final key, closing the check-then-copy race on the recorded size. Storage gains create_upload_ticket (replacing get_signed_upload_url) and head (size + filename); the v1 get_file_size_kb path is untouched. --- backend/app/api/docs/documents/initiate_v2.md | 12 +- backend/app/api/docs/documents/register_v2.md | 10 +- backend/app/api/routes/documents_v2.py | 18 +-- backend/app/core/cloud/__init__.py | 3 +- backend/app/core/cloud/storage.py | 92 +++++++++--- backend/app/models/document.py | 5 +- .../app/services/documents/registration.py | 67 +++------ .../test_route_document_register_v2.py | 135 +++++------------- .../test_route_document_uploads_v2.py | 47 +++--- backend/app/tests/core/cloud/test_storage.py | 82 ++++++++--- docs/wiki/modules/knowledge-base.md | 7 +- 11 files changed, 241 insertions(+), 237 deletions(-) diff --git a/backend/app/api/docs/documents/initiate_v2.md b/backend/app/api/docs/documents/initiate_v2.md index 20c969445..77d658dec 100644 --- a/backend/app/api/docs/documents/initiate_v2.md +++ b/backend/app/api/docs/documents/initiate_v2.md @@ -1,11 +1,11 @@ -Open a v2 upload session: get a pre-signed URL to send a document straight to Kaapi's object storage. +Open a v2 upload session: get a URL and form fields to send a document straight to Kaapi's object storage. Step 1 of the three-step flow: -1. `POST /api/v2/documents/uploads` with the filename — returns a `document_id` and an `upload_signed_url`. -2. `PUT` the raw file bytes to `upload_signed_url` — the body is the file itself, with no auth header, no form encoding, and no extra headers. -3. `PUT /api/v2/documents/{document_id}` with the filename to create the document. +1. `POST /api/v2/documents/uploads` with the filename — returns a `document_id`, an `upload_url`, and `upload_fields`. +2. Upload the file with a single `multipart/form-data` POST to `upload_url`: send every entry in `upload_fields` as a form field, then the file **last** in a field named `file`. No auth header on this call. +3. `PUT /api/v2/documents/{document_id}` to create the document — no body needed. -The filename extension is validated here, so an unsupported file type fails before any upload. Nothing is persisted at this step — hence the `200` rather than a `201` — the `document_id` only becomes a document once you register it in step 3. Only the **extension** is carried across the two calls: it determines where the bytes are staged, so step 3 must use the same extension or it will find nothing to register. The rest of the name is free to differ — `report.pdf` here and `invoice.pdf` in step 3 both work, and the step 3 name is what gets stored. +The filename is validated here (an unsupported type fails before any upload) and travels with the file, so registration in step 3 uses it automatically. Nothing is persisted at this step — hence the `200` rather than a `201` — the `document_id` only becomes a document once you register it. -`upload_signed_url` is valid for `expires_in` seconds (the effective value after server-side capping, which may be shorter than requested); open a new upload session if it lapses. Maximum file size is 25 MB, enforced at registration rather than at upload time — a pre-signed PUT cannot enforce a content-length range (that would require a pre-signed POST, which would break the raw-PUT contract). +Maximum file size is 25 MB, enforced by storage as the file uploads: a larger file is rejected outright with `400 EntityTooLarge` and nothing is stored. `upload_url` is valid for `expires_in` seconds (the effective value after server-side capping, which may be shorter than requested); open a new upload session if it lapses. diff --git a/backend/app/api/docs/documents/register_v2.md b/backend/app/api/docs/documents/register_v2.md index e31c7b1ae..30239c8be 100644 --- a/backend/app/api/docs/documents/register_v2.md +++ b/backend/app/api/docs/documents/register_v2.md @@ -1,9 +1,7 @@ -Register a document at the `document_id` issued by `POST /api/v2/documents/uploads`, from the bytes staged at its pre-signed URL. +Register the document at the `document_id` issued by `POST /api/v2/documents/uploads`, from the file uploaded to its pre-signed URL. -Final step of the v2 upload flow. Send the filename the upload URL was issued for — its extension determines where the bytes were staged. The staged object is moved to its permanent location, the document row is created, and the response carries a fresh signed URL for reading the file back. +Final step of the v2 upload flow, and it takes no request body. The uploaded object is moved to its permanent location, the document row is created with the filename captured at step 1, and the response carries a fresh signed URL for reading the file back. -Errors: `400` if nothing is staged for that `document_id` and filename (the upload never happened, lapsed, or the filename differs from the one the URL was issued for) or the extension is unsupported; `413` if the uploaded file exceeds 25 MB, in which case the staged object is deleted; `409` if the `document_id` was already registered — open a new upload session in that case. +Errors: `400` if nothing was uploaded for that `document_id` (the upload never happened or the URL lapsed); `409` if the `document_id` was already registered — open a new upload session in that case. -The 25 MB cap is enforced here rather than at upload time: a pre-signed PUT cannot enforce a content-length range (that would require a pre-signed POST, which would break the raw-PUT contract). - -Document transformation is not available on v2. Use `POST /api/v1/documents` if you need a `target_format` conversion. +The 25 MB cap is enforced by storage while the file uploads, so an oversized file never reaches this step. 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/routes/documents_v2.py b/backend/app/api/routes/documents_v2.py index 7c81ce0d1..74b2e0a2a 100644 --- a/backend/app/api/routes/documents_v2.py +++ b/backend/app/api/routes/documents_v2.py @@ -1,4 +1,4 @@ -"""v2 document upload: pre-signed PUT to a pending key, then registration.""" +"""v2 document upload: pre-signed POST to a pending key, then registration.""" from pathlib import Path from uuid import UUID, uuid4 @@ -14,6 +14,7 @@ DocumentUploadInitiateResponse, DocumentUploadRequest, ) +from app.services.collections.helpers import MAX_DOC_SIZE_MB from app.services.documents.registration import ( register_uploaded_document, validate_filename_format, @@ -23,6 +24,7 @@ router = APIRouter(prefix="/documents", tags=["Documents v2"]) UPLOAD_URL_EXPIRY_SECONDS = 3600 +MAX_UPLOAD_BYTES = MAX_DOC_SIZE_MB * 1024 * 1024 @router.post( @@ -40,17 +42,19 @@ def create_upload_url( storage = get_cloud_storage(session=session, project_id=current_user.project_.id) document_id = uuid4() - extension = Path(request.filename).suffix.lower() - signed = storage.get_signed_upload_url( - Path(f"{document_id}{extension}"), + ticket = storage.create_upload_ticket( + Path(str(document_id)), + filename=request.filename, + max_bytes=MAX_UPLOAD_BYTES, expires_in=UPLOAD_URL_EXPIRY_SECONDS, ) return APIResponse[DocumentUploadInitiateResponse].success_response( DocumentUploadInitiateResponse( document_id=document_id, - upload_signed_url=signed.url, - expires_in=signed.expires_in, + upload_url=ticket.url, + upload_fields=ticket.fields, + expires_in=ticket.expires_in, ) ) @@ -65,7 +69,6 @@ def create_upload_url( def register_document( session: SessionDep, current_user: AuthContextDep, - request: DocumentUploadRequest, document_id: UUID = FastPath( description="Document id issued by the upload session" ), @@ -74,7 +77,6 @@ def register_document( session=session, project_id=current_user.project_.id, document_id=document_id, - filename=request.filename, ) storage = get_cloud_storage(session=session, project_id=current_user.project_.id) diff --git a/backend/app/core/cloud/__init__.py b/backend/app/core/cloud/__init__.py index 1e68e7389..2611b76bd 100644 --- a/backend/app/core/cloud/__init__.py +++ b/backend/app/core/cloud/__init__.py @@ -4,7 +4,8 @@ CloudStorage, CloudStorageError, ObjectNotFoundError, - SignedUpload, + StoredObject, + UploadTicket, get_cloud_storage, upload_audio_to_gcs, ) diff --git a/backend/app/core/cloud/storage.py b/backend/app/core/cloud/storage.py index a0753821d..8bbe23baa 100644 --- a/backend/app/core/cloud/storage.py +++ b/backend/app/core/cloud/storage.py @@ -7,7 +7,7 @@ import functools as ft from pathlib import Path from dataclasses import dataclass, asdict -from urllib.parse import ParseResult, urlparse, urlunparse +from urllib.parse import ParseResult, quote, unquote, urlparse, urlunparse from abc import ABC, abstractmethod from typing import Any, NamedTuple @@ -52,12 +52,25 @@ def _to_storage_error(err: ClientError, url: str) -> CloudStorageError: return CloudStorageError(message) -class SignedUpload(NamedTuple): +# S3 user-metadata key that carries the client's filename, signed into the upload +# ticket so the client cannot change it and registration can read it back. +FILENAME_METADATA_KEY = "filename" + + +class UploadTicket(NamedTuple): url: str + # Form fields the client must POST alongside the file, the file part last. + fields: dict[str, str] # Effective expiry after capping, which may be shorter than what the caller asked for. expires_in: int +class StoredObject(NamedTuple): + size_kb: float + # The client filename recovered from object metadata, None if the object carries none. + filename: str | None + + class AmazonCloudStorageClient: @ft.cached_property def client(self): @@ -198,17 +211,29 @@ def get_file_size_kb(self, url: str) -> float: """Return the file size in KB""" pass + @abstractmethod + def head(self, url: str) -> StoredObject: + """Return the object's size and the filename recorded in its metadata.""" + pass + @abstractmethod 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, file_path: Path, expires_in: int = 3600 - ) -> SignedUpload: - """Generate a signed URL the client can upload (PUT) to directly. - + def create_upload_ticket( + self, + file_path: Path, + *, + filename: str, + max_bytes: int, + expires_in: int = 3600, + ) -> UploadTicket: + """Create a pre-signed POST the client uploads a file directly to. + + The ticket caps the body at max_bytes (S3 rejects anything larger at the + edge) and pins filename into signed metadata so the client cannot change it. Always resolves under PENDING_PREFIX: nothing is ever presigned to a final key, or an abandoned upload would be indistinguishable from a document. """ @@ -313,6 +338,24 @@ def get_file_size_kb(self, url: str) -> float: ) raise _to_storage_error(err, url) from err + def head(self, url: str) -> StoredObject: + name = SimpleStorageName.from_url(url) + try: + response = self.aws.client.head_object(**asdict(name)) + except ClientError as err: + logger.error( + f"[AmazonCloudStorage.head] AWS head object error | " + f"{{'project_id': '{self.project_id}', 'bucket': '{_mask(name.Bucket)}', 'key': '{_mask(name.Key)}', 'error': '{str(err)}'}}", + exc_info=True, + ) + raise _to_storage_error(err, url) from err + + encoded = response.get("Metadata", {}).get(FILENAME_METADATA_KEY) + return StoredObject( + size_kb=round(response["ContentLength"] / 1024, 2), + filename=unquote(encoded) if encoded else None, + ) + # Maximum allowed expiry for signed URLs (24 hours) MAX_SIGNED_URL_EXPIRY = 86400 @@ -346,26 +389,41 @@ 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, file_path: Path, expires_in: int = 3600 - ) -> SignedUpload: + def create_upload_ticket( + self, + file_path: Path, + *, + filename: str, + max_bytes: int, + expires_in: int = 3600, + ) -> UploadTicket: """ - Generate a signed S3 URL the client can PUT raw bytes to, under PENDING_PREFIX. - No content type is signed, so the client sends no headers beyond the body. + Pre-signed POST the client uploads to, under PENDING_PREFIX. The size cap is + enforced by S3 at the edge, and the filename is signed into metadata so it + cannot be swapped between issuing the ticket and registration. """ expires_in = min(expires_in, self.MAX_SIGNED_URL_EXPIRY) name = self.url_for(file_path, is_pending=True) + encoded = quote(filename) + meta_field = f"x-amz-meta-{FILENAME_METADATA_KEY}" try: - signed_url = self.aws.client.generate_presigned_url( - "put_object", - Params=asdict(name), + post = self.aws.client.generate_presigned_post( + name.Bucket, + name.Key, + Fields={meta_field: encoded}, + Conditions=[ + ["content-length-range", 1, max_bytes], + {meta_field: encoded}, + ], ExpiresIn=expires_in, ) - return SignedUpload(url=signed_url, expires_in=expires_in) + return UploadTicket( + url=post["url"], fields=post["fields"], expires_in=expires_in + ) except ClientError as err: logger.error( - f"[AmazonCloudStorage.get_signed_upload_url] AWS presign error | " + f"[AmazonCloudStorage.create_upload_ticket] AWS presign error | " f"{{'project_id': '{self.project_id}', 'bucket': '{_mask(name.Bucket)}', 'key': '{_mask(name.Key)}', 'error': '{str(err)}'}}", exc_info=True, ) diff --git a/backend/app/models/document.py b/backend/app/models/document.py index bcdb6077a..37e051daf 100644 --- a/backend/app/models/document.py +++ b/backend/app/models/document.py @@ -129,8 +129,9 @@ class DocumentUploadInitiateResponse(SQLModel): document_id: UUID = Field( description="Identifier to register the document under; the registration endpoint takes it as a path parameter" ) - upload_signed_url: str = Field( - description="Pre-signed URL to PUT the raw file bytes to, with no auth header and no form encoding" + upload_url: str = Field(description="URL to POST the file to") + upload_fields: dict[str, str] = Field( + description="Form fields to send with the file in the multipart POST, the file part last" ) expires_in: int = Field( description="Effective lifetime of the upload URL in seconds, after server-side capping" diff --git a/backend/app/services/documents/registration.py b/backend/app/services/documents/registration.py index dc6f335f3..16cb3c5ca 100644 --- a/backend/app/services/documents/registration.py +++ b/backend/app/services/documents/registration.py @@ -1,4 +1,4 @@ -"""v2 upload policy: verify what the client staged, then promote it to a document row.""" +"""v2 upload policy: promote what the client uploaded into a document row.""" from pathlib import Path from uuid import UUID @@ -8,15 +8,18 @@ from sqlmodel import Session from app.core.cloud import get_cloud_storage -from app.core.cloud.storage import CloudStorage, ObjectNotFoundError +from app.core.cloud.storage import ObjectNotFoundError from app.crud import DocumentCrud from app.models import Document -from app.services.collections.helpers import MAX_DOC_SIZE_MB from app.services.doctransform.registry import get_file_format DUPLICATE_DOCUMENT_DETAIL = ( "This document_id is already registered. Request a new upload URL." ) +MISSING_UPLOAD_DETAIL = ( + "No uploaded file found for this document_id. Upload the file to the " + "pre-signed URL before registering it." +) def validate_filename_format(filename: str) -> str: @@ -27,69 +30,41 @@ def validate_filename_format(filename: str) -> str: raise HTTPException(status_code=400, detail=str(e)) -def verify_pending_object( - *, - storage: CloudStorage, - pending_url: str, - document_id: UUID, -) -> float: - """Confirm the pending object exists and fits the size budget; return size in KB.""" - try: - file_size_kb = storage.get_file_size_kb(pending_url) - except ObjectNotFoundError: - raise HTTPException( - status_code=400, - detail="No uploaded file found for this document_id. Upload the file to the " - "pre-signed URL first, and pass the same filename the upload URL was issued " - "for — its extension determines where the bytes were staged.", - ) - - file_size_mb = file_size_kb / 1024 - if file_size_mb > MAX_DOC_SIZE_MB: - storage.delete(pending_url) - 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 register_uploaded_document( *, session: Session, project_id: int, document_id: UUID, - filename: str, ) -> Document: - """Promote a staged upload into a document row, moving the object to its final key.""" - validate_filename_format(filename) + """Promote an uploaded object into a document row, moving it to its final key. + The filename comes from the object's own signed metadata, not the request, so + it cannot differ from what the upload ticket was issued for. Size is already + capped by the ticket, so the copied object is measured only to record its size. + """ document_crud = DocumentCrud(session, project_id) if document_crud.exists(document_id): raise HTTPException(status_code=409, detail=DUPLICATE_DOCUMENT_DETAIL) storage = get_cloud_storage(session=session, project_id=project_id) - extension = Path(filename).suffix.lower() - pending_url = str( - storage.url_for(Path(f"{document_id}{extension}"), is_pending=True) - ) + pending_url = str(storage.url_for(Path(str(document_id)), is_pending=True)) - file_size_kb = verify_pending_object( - storage=storage, - pending_url=pending_url, - document_id=document_id, - ) + # Copy first, then read the frozen final key: the pending object stays writable + # through its ticket, so measuring it directly would race the copy. + try: + object_store_url = storage.copy(pending_url, Path(str(document_id))) + except ObjectNotFoundError: + raise HTTPException(status_code=400, detail=MISSING_UPLOAD_DETAIL) - object_store_url = storage.copy(pending_url, Path(str(document_id))) + stored = storage.head(str(object_store_url)) + filename = stored.filename or str(document_id) try: document = document_crud.update( Document( id=document_id, fname=filename, - file_size_kb=file_size_kb, + file_size_kb=stored.size_kb, object_store_url=str(object_store_url), project_id=project_id, ) 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 index 7b3f51daa..c5dda997e 100644 --- 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 @@ -1,4 +1,5 @@ from unittest.mock import patch +from urllib.parse import quote from uuid import UUID, uuid4 import pytest @@ -10,13 +11,11 @@ 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.db import engine from app.core.util import now from app.crud import DocumentCrud 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 @@ -24,18 +23,24 @@ UPLOADS_ROUTE = f"{DOCUMENTS_ROUTE}/uploads" -def pending_key(auth: TestAuthContext, document_id: UUID, extension: str) -> str: - # The pending prefix leads the key so one S3 lifecycle rule covers every project. - return f"pending/{auth.project.storage_path}/{document_id}{extension}" +def pending_key(auth: TestAuthContext, document_id: UUID) -> str: + # No extension: the filename rides in signed metadata, not in the key. + return f"pending/{auth.project.storage_path}/{document_id}" def final_key(auth: TestAuthContext, document_id: UUID) -> str: return f"{auth.project.storage_path}/{document_id}" -def put_object(key: str, body: bytes) -> None: +def put_pending( + auth: TestAuthContext, document_id: UUID, body: bytes, filename: str +) -> None: + """Simulate a client upload: the object plus the filename the ticket would have pinned.""" AmazonCloudStorageClient().client.put_object( - Bucket=settings.AWS_S3_BUCKET, Key=key, Body=body + Bucket=settings.AWS_S3_BUCKET, + Key=pending_key(auth, document_id), + Body=body, + Metadata={"filename": quote(filename)}, ) @@ -47,13 +52,10 @@ def assert_absent(key: str) -> None: assert excinfo.value.response["Error"]["Code"] == "404" -def register( - client: TestClient, auth: TestAuthContext, document_id: UUID, filename: str -) -> Response: +def register(client: TestClient, auth: TestAuthContext, document_id: UUID) -> Response: return client.put( f"{DOCUMENTS_ROUTE}/{document_id}", headers={"X-API-KEY": auth.key}, - json={"filename": filename}, ) @@ -68,10 +70,9 @@ def test_registers_pending_object_under_its_final_key( ) -> None: AmazonCloudStorageClient().create() document_id = uuid4() - pending = pending_key(user_api_key, document_id, ".pdf") - put_object(pending, b"x" * 2048) + put_pending(user_api_key, document_id, b"x" * 2048, "report.pdf") - response = register(client, user_api_key, document_id, "report.pdf") + response = register(client, user_api_key, document_id) assert response.status_code == 201 data = response.json()["data"] @@ -88,29 +89,7 @@ def test_registers_pending_object_under_its_final_key( ) assert document.project_id == user_api_key.project_id - assert_absent(pending) - - def test_extension_other_than_the_presigned_one_is_rejected( - self, - db: Session, - client: TestClient, - user_api_key: TestAuthContext, - ) -> None: - AmazonCloudStorageClient().create() - document_id = uuid4() - pending = pending_key(user_api_key, document_id, ".pdf") - put_object(pending, b"x" * 1024) - - response = register(client, user_api_key, document_id, "report.txt") - - assert response.status_code == 400 - assert "No uploaded file found" in response.json()["error"] - assert db.get(Document, document_id) is None - # The bytes the client actually uploaded are untouched, so a retry with the - # right filename still works. - assert AmazonCloudStorageClient().client.head_object( - Bucket=settings.AWS_S3_BUCKET, Key=pending - ) + assert_absent(pending_key(user_api_key, document_id)) def test_missing_object_is_rejected( self, @@ -121,36 +100,12 @@ def test_missing_object_is_rejected( AmazonCloudStorageClient().create() document_id = uuid4() - response = register(client, user_api_key, document_id, "report.pdf") + response = register(client, user_api_key, document_id) 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: - AmazonCloudStorageClient().create() - document_id = uuid4() - pending = pending_key(user_api_key, document_id, ".pdf") - put_object(pending, 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 - assert_absent(pending) - assert_absent(final_key(user_api_key, document_id)) - def test_duplicate_document_id_is_rejected( self, db: Session, @@ -161,9 +116,9 @@ def test_duplicate_document_id_is_rejected( existing = next(DocumentMaker(project_id=user_api_key.project_id, session=db)) db.add(existing) db.commit() - put_object(pending_key(user_api_key, existing.id, ".pdf"), b"x" * 1024) + put_pending(user_api_key, existing.id, b"x" * 1024, "report.pdf") - response = register(client, user_api_key, existing.id, "report.pdf") + response = register(client, user_api_key, existing.id) assert response.status_code == 409 assert "already registered" in response.json()["error"] @@ -185,12 +140,12 @@ def test_concurrent_registration_loses_the_insert_race( with Session(engine) as outside: outside.add(winner) outside.commit() - put_object(pending_key(user_api_key, document_id, ".pdf"), b"x" * 1024) + put_pending(user_api_key, document_id, b"x" * 1024, "report.pdf") try: # exists() returning False simulates the racing request that also saw no row. with patch.object(DocumentCrud, "exists", return_value=False): - response = register(client, user_api_key, document_id, "report.pdf") + response = register(client, user_api_key, document_id) assert response.status_code == 409 assert "already registered" in response.json()["error"] @@ -217,32 +172,13 @@ def test_soft_deleted_document_id_is_rejected( db.add(deleted) db.commit() - response = register(client, user_api_key, deleted.id, "report.pdf") + response = register(client, user_api_key, deleted.id) 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(pending_key(user_api_key, document_id, ".xyz"), 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.put( - f"{DOCUMENTS_ROUTE}/{uuid4()}", - json={"filename": "report.pdf"}, - ) + response = client.put(f"{DOCUMENTS_ROUTE}/{uuid4()}") assert response.status_code == 401 @@ -250,7 +186,7 @@ def test_missing_api_key_is_unauthorized(self, client: TestClient) -> None: @mock_aws @pytest.mark.usefixtures("aws_credentials") class TestDocumentUploadRoundTripV2: - def test_uploads_then_put_then_register( + def test_uploads_then_post_then_register( self, db: Session, client: TestClient, @@ -258,27 +194,30 @@ def test_uploads_then_put_then_register( ) -> None: AmazonCloudStorageClient().create() - url_response = client.post( + init = client.post( UPLOADS_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_signed_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 + assert init.status_code == 200 + data = init.json()["data"] + document_id = UUID(data["document_id"]) + + upload = requests.post( + data["upload_url"], + data=data["upload_fields"], + files={"file": ("handbook.pdf", b"y" * 3072)}, + ) + assert upload.status_code in (200, 204) - response = register(client, user_api_key, document_id, "handbook.pdf") + response = register(client, user_api_key, document_id) 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}/{final_key(user_api_key, document_id)}" ) - assert_absent(pending_key(user_api_key, document_id, ".pdf")) + assert_absent(pending_key(user_api_key, document_id)) diff --git a/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py b/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py index 849876b5a..6760426b7 100644 --- a/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py +++ b/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py @@ -1,5 +1,4 @@ -from urllib.parse import urlparse -from uuid import UUID +from urllib.parse import quote import pytest from fastapi.testclient import TestClient @@ -14,21 +13,15 @@ UPLOADS_ROUTE = f"{settings.API_V2_STR}/documents/uploads" -def signed_key(upload_url: str) -> str: - """The object key a pre-signed URL points at, with host and bucket stripped.""" - path = urlparse(upload_url).path.lstrip("/") - return path.removeprefix(f"{settings.AWS_S3_BUCKET}/") - - -def pending_key(auth: TestAuthContext, document_id: str, extension: str) -> str: - # The pending prefix leads the key so one S3 lifecycle rule covers every project. - return f"pending/{auth.project.storage_path}/{document_id}{extension}" +def pending_key(auth: TestAuthContext, document_id: str) -> str: + # No extension: the filename lives in signed metadata, not in the key. + return f"pending/{auth.project.storage_path}/{document_id}" @mock_aws @pytest.mark.usefixtures("aws_credentials") class TestDocumentUploadsV2: - def test_returns_presigned_put_url_for_pending_key( + def test_returns_presigned_post_for_pending_key( self, db: Session, client: TestClient, @@ -46,14 +39,11 @@ def test_returns_presigned_put_url_for_pending_key( data = response.json()["data"] assert data["expires_in"] == 3600 - document_id = UUID(data["document_id"]) - assert ( - pending_key(user_api_key, str(document_id), ".pdf") - in data["upload_signed_url"] - ) - assert "X-Amz-Signature" in data["upload_signed_url"] + fields = data["upload_fields"] + assert fields["key"] == pending_key(user_api_key, data["document_id"]) + assert "x-amz-signature" in fields - def test_upload_url_does_not_target_the_final_key( + def test_upload_target_is_the_pending_key_not_the_final_one( self, db: Session, client: TestClient, @@ -68,12 +58,11 @@ def test_upload_url_does_not_target_the_final_key( ) data = response.json()["data"] - # Compared whole: the final key is a substring of the pending one, so `not in` never holds. - key = signed_key(data["upload_signed_url"]) - assert key == pending_key(user_api_key, data["document_id"], ".pdf") + key = data["upload_fields"]["key"] + assert key == pending_key(user_api_key, data["document_id"]) assert key != f"{user_api_key.project.storage_path}/{data['document_id']}" - def test_extension_is_lowercased_in_the_pending_key( + def test_filename_is_pinned_in_signed_metadata( self, db: Session, client: TestClient, @@ -84,14 +73,12 @@ def test_extension_is_lowercased_in_the_pending_key( response = client.post( UPLOADS_ROUTE, headers={"X-API-KEY": user_api_key.key}, - json={"filename": "Quarterly-Report.PDF"}, + json={"filename": "Quarterly Report.pdf"}, ) - data = response.json()["data"] - assert ( - pending_key(user_api_key, data["document_id"], ".pdf") - in data["upload_signed_url"] - ) + # Signed as a field so the client cannot change it; URL-encoded for the header. + fields = response.json()["data"]["upload_fields"] + assert fields["x-amz-meta-filename"] == quote("Quarterly Report.pdf") def test_does_not_create_document_row( self, @@ -107,6 +94,8 @@ def test_does_not_create_document_row( json={"filename": "notes.txt"}, ) + from uuid import UUID + document_id = UUID(response.json()["data"]["document_id"]) assert db.get(Document, document_id) is None diff --git a/backend/app/tests/core/cloud/test_storage.py b/backend/app/tests/core/cloud/test_storage.py index 9b1e5c10d..6dd0c8c01 100644 --- a/backend/app/tests/core/cloud/test_storage.py +++ b/backend/app/tests/core/cloud/test_storage.py @@ -2,7 +2,7 @@ from pathlib import Path from unittest.mock import patch -from urllib.parse import parse_qs, urlparse +from urllib.parse import quote from uuid import uuid4 import pytest @@ -78,58 +78,98 @@ def test_other_codes_stay_generic(self) -> None: @mock_aws @pytest.mark.usefixtures("aws_credentials") -class TestGetSignedUploadURL: - def test_url_targets_the_requested_key(self) -> None: +class TestCreateUploadTicket: + def test_targets_the_pending_key(self) -> None: storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) - file_path = Path("pending") / f"{uuid4()}.pdf" + document_id = uuid4() - signed = storage.get_signed_upload_url(file_path) + ticket = storage.create_upload_ticket( + Path(str(document_id)), filename="report.pdf", max_bytes=25 * 1024 * 1024 + ) + + assert ticket.fields["key"] == f"pending/{storage.storage_path}/{document_id}" + assert "x-amz-signature" in ticket.fields + + def test_filename_is_pinned_url_encoded(self) -> None: + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + + ticket = storage.create_upload_ticket( + Path(str(uuid4())), filename="my report.pdf", max_bytes=1024 + ) - parsed = urlparse(signed.url) - assert parsed.path.endswith(f"{storage.storage_path}/{file_path}") - assert settings.AWS_S3_BUCKET in f"{parsed.netloc}{parsed.path}" - assert "X-Amz-Signature" in parse_qs(parsed.query) + assert ticket.fields["x-amz-meta-filename"] == quote("my report.pdf") def test_expiry_is_capped_at_one_day(self) -> None: storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) - signed = storage.get_signed_upload_url( - Path("key.pdf"), expires_in=7 * 24 * 3600 + ticket = storage.create_upload_ticket( + Path("f"), filename="a.pdf", max_bytes=1024, expires_in=7 * 24 * 3600 ) - assert signed.expires_in == 86400 - assert parse_qs(urlparse(signed.url).query)["X-Amz-Expires"] == ["86400"] + assert ticket.expires_in == 86400 def test_shorter_expiry_is_preserved(self) -> None: storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) - signed = storage.get_signed_upload_url(Path("key.pdf"), expires_in=600) + ticket = storage.create_upload_ticket( + Path("f"), filename="a.pdf", max_bytes=1024, expires_in=600 + ) - assert signed.expires_in == 600 - assert parse_qs(urlparse(signed.url).query)["X-Amz-Expires"] == ["600"] + assert ticket.expires_in == 600 def test_aws_error_is_wrapped(self) -> None: storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) with patch.object( storage.aws.client, - "generate_presigned_url", + "generate_presigned_post", side_effect=client_error("AccessDenied", "PutObject"), ): with pytest.raises(CloudStorageError, match="AccessDenied"): - storage.get_signed_upload_url(Path("key.pdf")) + storage.create_upload_ticket( + Path("f"), filename="a.pdf", max_bytes=1024 + ) @mock_aws @pytest.mark.usefixtures("aws_credentials") -class TestGetFileSizeKB: +class TestHead: + def test_returns_size_and_decoded_filename(self) -> None: + aws = AmazonCloudStorageClient() + aws.create() + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + name = storage.url_for(Path(str(uuid4())), is_pending=True) + aws.client.put_object( + Bucket=name.Bucket, + Key=name.Key, + Body=b"x" * 2048, + Metadata={"filename": quote("my report.pdf")}, + ) + + stored = storage.head(str(name)) + + assert stored.size_kb == 2.0 + assert stored.filename == "my report.pdf" + + def test_filename_is_none_without_metadata(self) -> None: + aws = AmazonCloudStorageClient() + aws.create() + storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) + name = storage.url_for(Path(str(uuid4()))) + aws.client.put_object(Bucket=name.Bucket, Key=name.Key, Body=b"x" * 1024) + + stored = storage.head(str(name)) + + assert stored.size_kb == 1.0 + assert stored.filename is None + def test_missing_key_raises_object_not_found(self) -> None: AmazonCloudStorageClient().create() storage = AmazonCloudStorage(project_id=1, storage_path=uuid4()) - url = str(storage.url_for(Path(f"{uuid4()}.pdf"))) + url = str(storage.url_for(Path(str(uuid4())))) with pytest.raises(ObjectNotFoundError): - storage.get_file_size_kb(url) + storage.head(url) @mock_aws diff --git a/docs/wiki/modules/knowledge-base.md b/docs/wiki/modules/knowledge-base.md index a93c68abc..0603fe01a 100644 --- a/docs/wiki/modules/knowledge-base.md +++ b/docs/wiki/modules/knowledge-base.md @@ -7,7 +7,7 @@ All paths relative to `backend/app/`. ## Routes - `api/routes/documents.py` — upload/list (v1 multipart upload, the only path that transforms) -- `api/routes/documents_v2.py` — v2 pre-signed upload: `POST /documents/uploads` → 200 (issues a PUT URL to a pending key; nothing persisted) then `PUT /documents/{document_id}` → 201 (registers the pending object); no transformation +- `api/routes/documents_v2.py` — v2 pre-signed upload: `POST /documents/uploads` → 200 (issues a pre-signed POST to a pending key; nothing persisted) then `PUT /documents/{document_id}` → 201 (registers the pending object; no body); no transformation - `api/routes/collections.py`, `api/routes/collection_job.py` — collection CRUD + job status - `api/routes/doc_transformation_job.py` — transform job status @@ -35,8 +35,9 @@ All paths relative to `backend/app/`. ## Gotchas - Uploads de-duplicate by provider file ID (see deep dive §7). -- v2 registration trusts only the extension and the object's size — bytes never reach the backend, so `validate_document_content` sniffing (v1 only) is skipped. Uploads land at `pending/{storage_path}/{document_id}{ext}`; registration verifies that key, copies to the final `{storage_path}/{document_id}` (same shape as v1), deletes the pending copy, and deletes it instead when oversized. Because the extension is baked into the pending key, a filename mismatch between the two calls needs no explicit check — the key simply misses and the normal 400 fires. -- The pending prefix leads the key (`pending/{storage_path}/…`, not `{storage_path}/pending/…`) because **S3 lifecycle filters are literal prefixes with no wildcard support**. With the per-project `storage_path` in front, no single rule could match every project. `CloudStorage.url_for(path, is_pending=True)` owns this layout and `get_signed_upload_url` always routes through it — never presign to a final key. Note `PENDING_PREFIX` has nothing to do with the staging *environment*; it means "uploaded but not registered". +- v2 never sees the bytes, so `validate_document_content` sniffing (v1 only) is skipped. Uploads land at `pending/{storage_path}/{document_id}` (no extension); registration copies to the final `{storage_path}/{document_id}` (same shape as v1) and deletes the pending copy once the row commits. The size cap is enforced by the pre-signed POST's `content-length-range` at upload time, and the filename is signed into object metadata (`x-amz-meta-filename`, URL-encoded) — registration reads it back via `head`, so the client never sends the filename twice and cannot swap it. +- Registration copies **before** it measures: the pending object stays writable through its ticket, so it heads the frozen final key (never presigned) for the size it records. This closes the check-then-copy race on the recorded size. +- The pending prefix leads the key (`pending/{storage_path}/…`, not `{storage_path}/pending/…`) because **S3 lifecycle filters are literal prefixes with no wildcard support**. With the per-project `storage_path` in front, no single rule could match every project. `CloudStorage.url_for(path, is_pending=True)` owns this layout and `create_upload_ticket` always routes through it — never presign to a final key. Note `PENDING_PREFIX` has nothing to do with the staging *environment*; it means "uploaded but not registered". - **`pending/` has a 1-day TTL. Do not write anything else under it.** An S3 lifecycle rule (`expire-pending-uploads`, `Prefix: pending/`, `Expiration: 1 day`) is live on `ai-platform-documents-staging` and `-production`, and reaps abandoned v2 uploads — nothing in application code does. Consequences to know before touching this prefix: - Any object written under `pending/`, by any code path, is **deleted within ~24-48h** (lifecycle sweeps run about once a day, so expiry is not exact). Never park anything there you expect to keep. - If you do add a new writer under `pending/`, say so in this gotcha and in the PR — a reviewer cannot see the bucket config from the diff. From 1e4d4d544f04396632426e3f96295b3b59b8f2ff Mon Sep 17 00:00:00 2001 From: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com> Date: Mon, 7 Sep 2026 11:16:12 +0530 Subject: [PATCH 6/6] feat(documents): Guide the client through the v2 upload in the responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upload response now carries a next_step note — upload to the pre-signed URL, then register the document — and the register response notes that the upload URL is spent, so a client can follow the flow from the responses alone. --- backend/app/api/routes/documents_v2.py | 15 +++++++++++++-- .../documents/test_route_document_register_v2.py | 1 + .../documents/test_route_document_uploads_v2.py | 1 + 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/backend/app/api/routes/documents_v2.py b/backend/app/api/routes/documents_v2.py index 74b2e0a2a..9f3a39d33 100644 --- a/backend/app/api/routes/documents_v2.py +++ b/backend/app/api/routes/documents_v2.py @@ -55,7 +55,13 @@ def create_upload_url( upload_url=ticket.url, upload_fields=ticket.fields, expires_in=ticket.expires_in, - ) + ), + metadata={ + "next_step": ( + f"Upload the file to the pre-signed S3 URL with the provided fields " + f"(file part last), then PUT /api/v2/documents/{document_id} to register the document." + ) + }, ) @@ -83,4 +89,9 @@ def register_document( document_schema = DocumentPublic.model_validate(document, from_attributes=True) document_schema.signed_url = storage.get_signed_url(document.object_store_url) - return APIResponse[DocumentPublic].success_response(document_schema) + return APIResponse[DocumentPublic].success_response( + document_schema, + metadata={ + "note": "Document registered. The upload URL is spent and cannot be reused." + }, + ) 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 index c5dda997e..62fd67a3c 100644 --- 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 @@ -79,6 +79,7 @@ def test_registers_pending_object_under_its_final_key( assert data["id"] == str(document_id) assert data["fname"] == "report.pdf" assert data["signed_url"] + assert "cannot be reused" in response.json()["metadata"]["note"] document = db.get(Document, document_id) assert document is not None diff --git a/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py b/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py index 6760426b7..2adaf7e59 100644 --- a/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py +++ b/backend/app/tests/api/routes/documents/test_route_document_uploads_v2.py @@ -42,6 +42,7 @@ def test_returns_presigned_post_for_pending_key( fields = data["upload_fields"] assert fields["key"] == pending_key(user_api_key, data["document_id"]) assert "x-amz-signature" in fields + assert "register" in response.json()["metadata"]["next_step"] def test_upload_target_is_the_pending_key_not_the_final_one( self,