-
Notifications
You must be signed in to change notification settings - Fork 10
feat(documents): Add v2 upload endpoints #1179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()) | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
Comment on lines
+316
to
+319
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Denial of Service (CWE-400): Uncontrolled Resource Consumption Reachability: External · Exploitability: Moderate Enforce a size limit before direct upload. This presigned S3 PUT has no 🤖 Prompt for AI Agents🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Prevent reuse of the upload URL after registration
🤖 Prompt for AI Agents |
||
| ) | ||
| 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) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Comment on lines
+19
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Restore the environment after the fixture. These assignments persist after the class-scoped fixture finishes. Later tests can observe the dummy AWS credentials or region and behave differently based on test order. Snapshot the previous values and restore them in a 🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make duplicate rejection atomic.
Line 85 performs a separate existence check before
crud.update. Two concurrent registration requests for the same uploaded object can both observe that the ID is absent. One database write then conflicts and becomes a 500 response instead of the required 409 response.Create the record atomically. Catch the unique-key integrity error, roll back the session, and return HTTP 409.
🤖 Prompt for AI Agents