Skip to content
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

[UPY-7] Support async operations #6

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

farmisen
Copy link
Owner

@farmisen farmisen commented Feb 13, 2025

Introduces async API client and refactors codebase for dual sync/async support

Adds a new AsyncUTApi class for asynchronous operations while refactoring the existing UTApi into a synchronous implementation. Both clients inherit from a new BaseUTApi abstract class that defines common functionality and interface methods.

Changes

  • Introduces new AsyncUTApi class implementing asynchronous versions of all API operations
  • Creates abstract BaseUTApi class to share common functionality between sync/async clients
  • Refactors existing UTApi to inherit from BaseUTApi and handle synchronous operations
  • Updates documentation to reflect both client implementations

Impact

  • Adds support for async/await patterns while maintaining existing synchronous API
  • Requires new dependency pytest-asyncio for testing async functionality
  • No breaking changes to existing synchronous API interface
  • Improves code organization through shared base class abstraction

Summary by CodeRabbit

  • Documentation

    • Enhanced documentation with clearer descriptions and examples for synchronous and asynchronous file operations.
  • New Features

    • Introduced an asynchronous client for efficient, non-blocking file management.
    • Added a demonstration showcasing asynchronous file uploads, listings, and deletions.
  • Tests

    • Expanded test coverage to validate both synchronous and asynchronous functionalities.
  • Chores

    • Updated development dependencies to support asynchronous testing.
  • Refactor

    • Streamlined the client implementation to improve consistency and error handling.

Copy link

coderabbitai bot commented Feb 13, 2025

Walkthrough

This pull request restructures and enhances the upyloadthing SDK. The changes update documentation to clarify both synchronous and asynchronous client implementations, including new type hints and detailed response model fields. A new abstract base class centralizes shared logic, while the synchronous client now subclasses it. The asynchronous client is introduced with concurrent upload capabilities and robust error handling. Additionally, test suites have been expanded for both client types, examples have been updated to demonstrate multi-file operations, and a new development dependency for asynchronous testing was added.

Changes

File(s) Change Summary
README.md Documentation restructured: updated section header from "UTApi Methods" to "Client Classes", detailed method signatures with type hints, and refined response models.
upyloadthing/client.py, upyloadthing/async_client.py Updated method signatures for upload_files, delete_files, list_files, and get_usage_info with explicit type hints; synchronous client now subclasses a new base class; asynchronous client added with concurrent file upload and enhanced error handling.
upyloadthing/base_client.py New abstract class BaseUTApi created to centralize common functionalities (headers, request preparation, error handling, file data preparation) for both synchronous and asynchronous operations.
tests/test_client.py, tests/test_async_client.py Test suites enhanced: added tests for multiple file uploads (including different file types and custom dispositions), deletion, listing, and error handling for both synchronous and asynchronous clients.
examples/main.py, examples/async_main.py Updated examples demonstrating multi-file upload and deletion; new asynchronous demo (async_main.py) added to showcase non-blocking API operations.
pyproject.toml Added development dependency pytest-asyncio = "^0.23.2" for asynchronous testing support.

Sequence Diagram(s)

sequenceDiagram
    participant Main
    participant AsyncUTApi
    participant API
    Main->>AsyncUTApi: Initialize with token
    Main->>AsyncUTApi: get_usage_info()
    AsyncUTApi->>API: Send usage info request
    API-->>AsyncUTApi: Return usage information
    Main->>AsyncUTApi: list_files(limit=5)
    AsyncUTApi->>API: Request list files
    API-->>AsyncUTApi: Return list of files
    Main->>AsyncUTApi: upload_files([png, jpeg])
    AsyncUTApi->>API: Concurrently upload files
    API-->>AsyncUTApi: Return upload results
    Main->>AsyncUTApi: delete_files(file_keys)
    AsyncUTApi->>API: Request deletion of files
    API-->>AsyncUTApi: Return deletion result
Loading

Possibly related PRs

Poem

I'm a bunny with a hop and a dance,
Code changes bloom like spring at first glance.
Sync and async now shine side by side,
Uploading files with a joyful stride.
Test cases and docs, all neat and tight,
Hop along with me into code's light! 🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (10)
upyloadthing/client.py (1)

22-67: Consider persistent HTTP sessions for efficiency.

Currently, _request spins up a new httpx.Client() in a context manager for every request. For high-throughput scenarios, reusing a single session can reduce connection overhead. If usage patterns call for multiple requests in short succession, a persistent session is typically beneficial.

 def _request(
     self,
     method: str,
     path: str,
     data: dict | None = None,
     timeout: float = 30.0,
 ) -> dict:
     ...
-    try:
-        with httpx.Client() as client:
+    if not self._session:
+        self._session = httpx.Client()
+    try:
-        response = client.request(
+        response = self._session.request(
             method=method, url=url, headers=headers, **request_kwargs
         )
     ...
upyloadthing/async_client.py (2)

23-68: Evaluate whether to separate request creation logic from execution.

Currently, _request both prepares and executes the HTTP request. For testing, debugging, or advanced usage, consider splitting request creation (URL, headers, etc.) from the actual network call for improved mockability in tests.


129-177: Potential for advanced retry logic on HTTP failures.

Beyond raising exceptions on HTTP failures, you may wish to retry certain transient errors (timeouts, 5xx statuses) to increase reliability when uploading or deleting files in asynchronous workflows.

upyloadthing/base_client.py (2)

118-143: Extend docstrings with usage examples if needed.

Docstrings are sufficient, but consider adding short examples (e.g., snippet usage for typical file uploads) within method docstrings. This can help clarify tricky parameters like acl and content_disposition.


144-243: Potential optimization for larger files.

The _prepare_file_data function loads entire files into memory when computing size with file.seek(0, 2). For huge files, a chunk-based approach may be necessary, or you might coordinate with the hosting service on streaming.

tests/test_client.py (2)

85-85: Consider adding error case for empty file list.

The test verifies successful upload but doesn't check how the API handles an empty file list.

 def test_upload_files(respx_mock: respx.MockRouter, ut_api, mock_file):
     """Test file upload functionality."""
+    # Test empty file list
+    with pytest.raises(ValueError, match="No files provided"):
+        ut_api.upload_files([])
+
     upload_response = {

175-199: Consider adding validation for content disposition.

While the test verifies the API call, it doesn't validate that the content disposition is correctly set in the request.

     respx_mock.put(url__regex="https://sea2.ingest.uploadthing.com/").mock(
-        return_value=httpx.Response(200, json=upload_response)
+        side_effect=lambda request: 
+            httpx.Response(200, json=upload_response)
+            if "attachment" in request.headers.get("content-disposition", "")
+            else httpx.Response(400)
     )
tests/test_async_client.py (3)

31-36: Consider adding cleanup for AsyncUTApi instance.

The fixture should handle cleanup of any async resources.

 @pytest.fixture
-def ut_api():
+async def ut_api():
     """Fixture to create an AsyncUTApi instance with mock token."""
     options = UTApiOptions(token=MOCK_TOKEN)
-    return AsyncUTApi(options)
+    api = AsyncUTApi(options)
+    yield api
+    await api.close()  # Clean up any async resources

69-103: Consider testing concurrent uploads.

While the test verifies basic upload functionality, it doesn't test concurrent uploads which is a key feature of async APIs.

+@pytest.mark.asyncio
+@respx.mock
+async def test_concurrent_uploads(respx_mock: respx.MockRouter, ut_api):
+    """Test concurrent file uploads."""
+    files = [BytesIO(f"content{i}".encode()) for i in range(5)]
+    for i, f in enumerate(files):
+        f.name = f"test{i}.txt"
+
+    upload_response = {
+        "fileHash": "hash",
+        "url": "url",
+        "ufsUrl": "ufs_url",
+        "appUrl": "app_url",
+    }
+
+    respx_mock.put(url__regex="https://sea2.ingest.uploadthing.com/").mock(
+        return_value=httpx.Response(200, json=upload_response)
+    )
+
+    results = await asyncio.gather(
+        *[ut_api.upload_files([f]) for f in files]
+    )
+    assert len(results) == 5
+    assert all(isinstance(r[0], UploadResult) for r in results)

266-276: Consider testing timeout scenarios.

The error handling test should include timeout scenarios which are particularly important for async operations.

+@pytest.mark.asyncio
+@respx.mock
+async def test_request_with_timeout(respx_mock: respx.MockRouter, ut_api):
+    """Test handling of request timeouts."""
+    respx_mock.get(f"{API_URL}/test").mock(side_effect=httpx.TimeoutException)
+
+    with pytest.raises(httpx.TimeoutException):
+        await ut_api._request("GET", "/test")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 801e6fd and 4bd00bc.

⛔ Files ignored due to path filters (2)
  • examples/test.jpg is excluded by !**/*.jpg
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • README.md (1 hunks)
  • examples/main.py (2 hunks)
  • examples/main_async.py (1 hunks)
  • pyproject.toml (1 hunks)
  • tests/test_async_client.py (1 hunks)
  • tests/test_client.py (3 hunks)
  • upyloadthing/async_client.py (1 hunks)
  • upyloadthing/base_client.py (1 hunks)
  • upyloadthing/client.py (3 hunks)
🔇 Additional comments (19)
upyloadthing/client.py (3)

5-19: Adopted inheritance from BaseUTApi successfully.

Overall, the transition to inherit from BaseUTApi appears clean. The docstring updates to reflect the new synchronous client are clear, and no immediate functional issues stand out.


68-115: Handle multi-file upload partial failures.

When uploading a list of files, a single file failure could be masked if the rest succeed. Consider improving error handling so that any partial successes or failures are clearly reported (e.g., storing these results separately or raising a combined exception with detailed info).


117-165: Ensure alignment of delete/list usage with new key types.

The updated methods allow specifying key_type and returning typed responses. Ensure that the calling code, especially any existing consumers of these methods, correctly passes and interprets key_type. This might require updates elsewhere to avoid mismatched usage.

upyloadthing/async_client.py (2)

1-21: Async client introduction is well-structured.

You have added clear type hints and docstrings for the new AsyncUTApi. The usage of asyncio and httpx.AsyncClient is straightforward.


69-128: Parallel file uploading is nicely implemented.

The _upload_single_file helper method and asyncio.gather usage for parallel uploads are a good approach. Just be mindful of concurrency limits such as open file descriptors or remote server constraints.

upyloadthing/base_client.py (3)

1-34: Abstract base class design fosters maintainability.

The introduction of BaseUTApi(ABC) cleanly separates shared logic from synchronous/asynchronous specifics. This design considerably improves maintainability.


35-64: Validate token decoding carefully.

You decode the base64 token and parse it into UTtoken. If the token is malformed or expires, ensure that consumers know how to handle such errors. Allowing more precise exception handling or user-friendly messages might be beneficial.


65-116: Approve the abstraction for file operations.

Defining abstract methods _request, upload_files, delete_files, list_files, and get_usage_info ensures consistent structure across sync and async implementations. The docstrings are comprehensive.

examples/main.py (3)

2-2: LGTM!

The List import is correctly added to support type hints for the new multi-file upload functionality.


31-33: LGTM!

The implementation demonstrates a clear and well-structured approach to handling multiple file uploads:

  1. Clear section headers for better readability
  2. Proper handling of both file system (PNG) and in-memory (JPEG) files
  3. Correct type hints for the upload results
  4. Clear result printing

Also applies to: 34-39, 40-43, 44-47, 48-52


54-58: LGTM!

The file deletion logic is correctly updated to handle multiple files:

  1. Proper extraction of file keys from upload results
  2. Clear error message printing
examples/main_async.py (4)

1-7: LGTM!

The imports are correctly organized and include all necessary modules for async operations.


9-14: LGTM!

The async main function is properly defined and the client initialization is correct.


15-20: LGTM!

The implementation demonstrates proper async/await usage throughout all API calls:

  1. Usage info retrieval
  2. File listing
  3. Multiple file uploads
  4. File deletion

Also applies to: 22-30, 32-53, 55-60


63-64: LGTM!

The main function is correctly executed using asyncio.run() for the async entry point.

tests/test_client.py (1)

138-173: LGTM!

The test for multiple file types is well-structured and verifies:

  1. Correct handling of different file types
  2. Proper MIME type detection
  3. Correct response structure
pyproject.toml (1)

30-30:

✅ Verification successful

LGTM! Addition of pytest-asyncio for async testing.

The addition of pytest-asyncio aligns with the PR objectives to support async operations. The version constraint allows compatible updates.

Let's verify the version compatibility:


🌐 Web query:

What is the latest version of pytest-asyncio and what Python versions does it support?

💡 Result:

The latest version of pytest-asyncio is v0.25.3, released on January 28, 2025[1][4]. This version supports Python 3.8+, with explicit compatibility declarations up to Python 3.13 in recent releases[7][9].

Key details about compatibility:

  • Dropped support for Python ≤3.7 starting with v0.23 series (2024)[7]
  • Added Python 3.13 support in v0.23 series (2024)[7]
  • Requires pytest ≥8 since v0.24 series (2024)[4][7]

Recent compatibility milestones:

Version Python Support Pytest Requirement Notable Changes
0.25.x 3.8+ ≥8 Latest maintenance releases[1][4][7]
0.24.x 3.8+ ≥8 Introduced loop_scope parameter[4][7]
0.23.x ≥3.8 ≥7 Added Python 3.13 support[7][9]

The plugin follows semantic versioning and maintains compatibility with actively supported Python versions[1][4][7]. Users should upgrade regularly to receive security patches and new features while checking release notes for any breaking changes[4][7].

Citations:


Verified: pytest-asyncio Dependency Addition Approved

  • The inclusion of pytest-asyncio aligns with the PR’s goal to support async testing.
  • Web verification confirms that the latest pytest-asyncio release is v0.25.3, which supports Python 3.8+ (with compatibility through Python 3.13) and requires pytest ≥ 8 from v0.24 onward.
  • Note that the current version constraint (^0.23.2) in pyproject.toml restricts updates to the 0.23 series (due to pre-1.0 semver rules). This is acceptable if you intend to avoid a pytest upgrade; however, if you want to benefit from newer features or fixes in later releases (v0.24.x or v0.25.x), consider updating the version constraint accordingly.
README.md (2)

103-122: LGTM! Clear documentation of sync and async clients.

The documentation clearly distinguishes between UTApi (sync) and AsyncUTApi (async) clients with appropriate examples. This helps users understand the two available patterns.


143-168: LGTM! Comprehensive documentation of response models.

The response models section clearly documents all fields with their types, making it easy for users to understand the API responses.

Comment on lines +123 to +141
### Methods

Both clients provide these methods:

- `upload_files(files: BinaryIO | List[BinaryIO], content_disposition: str = "inline", acl: str | None = "public-read") -> List[UploadResult]`
- Upload one or more files
- Returns list of upload results

- `delete_files(keys: str | List[str], key_type: str = "file_key") -> DeleteFileResponse`
- Delete one or more files by key or custom ID
- Returns deletion result

- `list_files(limit: int | None = None, offset: int | None = None) -> ListFileResponse`
- List uploaded files with optional pagination
- Returns file listing

- `get_usage_info() -> UsageInfoResponse`
- Get account usage statistics
- Returns usage information
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Method signatures are well documented, but error handling needs update.

The method signatures with type hints are clear and helpful. However, there's a discrepancy in the error handling section.

The error handling example uses requests.exceptions.HTTPError but the client uses httpx. Update the error handling section to use the correct exception:

-from requests.exceptions import HTTPError
+from httpx import HTTPError

try:
    api.upload_files(file)
except HTTPError as e:

Committable suggestion skipped: line range outside the PR's diff.

@farmisen farmisen force-pushed the farmisen/upy-7-support-sync-operations branch from 4bd00bc to c564094 Compare February 13, 2025 19:32
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (11)
examples/main.py (1)

56-60: Consider adding error handling for file deletion.

While the implementation is correct, it would be beneficial to add try-except blocks to handle potential API errors during deletion.

     # Delete the uploaded files
     print("🗑️ Deleting test files...")
     file_keys = [result.file_key for result in upload_results]
-    delete_result = api.delete_files(file_keys)
-    print(f"Deleted {delete_result.deleted_count} file(s)")
+    try:
+        delete_result = api.delete_files(file_keys)
+        print(f"Deleted {delete_result.deleted_count} file(s)")
+    except Exception as e:
+        print(f"Error deleting files: {e}")
examples/async_main.py (1)

9-63: Consider using async context manager for API client.

For better resource management, consider initializing the API client using an async context manager.

-async def main():
+async def main():
+    async with AsyncUTApi() as api:
         print("🚀 UploadThing API Demo (Async)\n")
-
-        # Initialize the client
-        api = AsyncUTApi()
upyloadthing/client.py (1)

87-90: Consider adding batch size limit for file uploads.

To prevent potential memory issues or API rate limits, consider adding a maximum batch size for file uploads.

+        MAX_BATCH_SIZE = 10
+        if len(files) > MAX_BATCH_SIZE:
+            raise ValueError(f"Maximum batch size of {MAX_BATCH_SIZE} exceeded")
+
         files_data = [
             self._prepare_file_data(file, content_disposition, acl)
             for file in files
         ]
upyloadthing/async_client.py (1)

51-67: Consider adding retry logic for transient failures.

For improved reliability, consider implementing retry logic for transient HTTP errors.

+    async def _request_with_retry(self, *args, max_retries: int = 3, **kwargs):
+        for attempt in range(max_retries):
+            try:
+                return await self._request(*args, **kwargs)
+            except httpx.TransientHTTPError as e:
+                if attempt == max_retries - 1:
+                    raise
+                await asyncio.sleep(2 ** attempt)  # Exponential backoff
upyloadthing/base_client.py (3)

28-52: Add validation for region configuration.

While the token validation is good, consider adding validation to ensure the provided region exists in self.token.regions.

 self.region = (
     options.region
     if options and options.region
     else os.getenv("UPLOADTHING_REGION") or self.token.regions[0]
 )
+if self.region not in self.token.regions:
+    raise ValueError(f"Region {self.region} not found in available regions: {self.token.regions}")
 self.api_url = API_URL

144-188: Enhance file type detection and validation.

The current implementation could be more robust in handling file types and validation:

  1. Consider using python-magic for more accurate file type detection
  2. Add validation for file size limits
  3. Validate file name for security
+import magic
+
 def _prepare_file_data(
     self, file: BinaryIO, content_disposition: str, acl: str | None
 ) -> dict:
     file_seed = uuid.uuid4().hex
     file_key = generate_key(file_seed, self.token.app_id)
     file_name = getattr(file, "name", f"upload_{uuid.uuid4()}")
+    # Validate file name
+    if not self._is_safe_filename(file_name):
+        raise ValueError("Invalid file name")
+
     file_size = file.seek(0, 2)
     file.seek(0)
-    file_type = (
-        mimetypes.guess_type(file_name)[0] or "application/octet-stream"
-    )
+    # More accurate file type detection
+    file_type = magic.from_buffer(file.read(2048), mime=True)
+    file.seek(0)
+
+    # Validate file size
+    max_size = 100 * 1024 * 1024  # 100MB example limit
+    if file_size > max_size:
+        raise ValueError(f"File size exceeds limit of {max_size} bytes")

     ingest_url = make_presigned_url(
         self.region,

189-242: Enhance error handling with specific error types.

Consider creating custom exception types for different error scenarios to allow more granular error handling by clients.

+class UTApiError(Exception):
+    """Base exception for UploadThing API errors."""
+    pass
+
+class UTApiAuthError(UTApiError):
+    """Authentication related errors."""
+    pass
+
+class UTApiQuotaError(UTApiError):
+    """Quota exceeded errors."""
+    pass
+
 def _handle_error_response(self, e: httpx.HTTPStatusError) -> None:
     error_msg = f"UploadThing API error: {e.response.status_code}"
     try:
         error_data = e.response.json()
         if "error" in error_data:
             error_msg += f" - {error_data['error']}"
+        # Map status codes to specific exceptions
+        if e.response.status_code == 401:
+            raise UTApiAuthError(error_msg)
+        elif e.response.status_code == 429:
+            raise UTApiQuotaError(error_msg)
     except Exception:
         pass
     raise httpx.HTTPStatusError(
         error_msg, request=e.request, response=e.response
     ) from e
tests/test_client.py (3)

1-44: Add edge case test fixtures.

Consider adding fixtures for:

  1. Files with special characters in names
  2. Empty files
  3. Files with non-standard MIME types
@pytest.fixture
def special_char_file():
    """Fixture for file with special characters in name."""
    file = BytesIO(b"test content")
    file.name = "test@#$%.jpg"
    return file

@pytest.fixture
def empty_file():
    """Fixture for empty file."""
    file = BytesIO(b"")
    file.name = "empty.txt"
    return file

@pytest.fixture
def custom_mime_file():
    """Fixture for file with custom MIME type."""
    file = BytesIO(b"custom content")
    file.name = "data.custom"
    return file

46-104: Enhance test assertions for edge cases.

Add assertions to verify:

  1. Error cases for invalid tokens
  2. Region validation
  3. Header content validation
 def test_init_with_options():
     options = UTApiOptions(token=MOCK_TOKEN, region="eu-west-1")
     api = UTApi(options)
     assert api.token.app_id == "test-app"
     assert api.token.api_key == "test-key"
     assert api.region == "eu-west-1"
+    # Add assertions for invalid region
+    with pytest.raises(ValueError, match="Region .* not found in available regions"):
+        UTApi(UTApiOptions(token=MOCK_TOKEN, region="invalid-region"))
+
+def test_init_with_invalid_token():
+    """Test initialization with invalid token format."""
+    with pytest.raises(ValueError, match="Invalid token format"):
+        UTApi(UTApiOptions(token="invalid-token"))

105-293: Add tests for API error responses.

Consider adding tests for various API error scenarios:

  1. Rate limiting responses
  2. Authentication failures
  3. Quota exceeded errors
@respx.mock
def test_upload_files_rate_limit(respx_mock: respx.MockRouter, ut_api, mock_file):
    """Test handling of rate limit errors."""
    respx_mock.put(url__regex="https://sea2.ingest.uploadthing.com/").mock(
        return_value=httpx.Response(
            429,
            json={"error": "Rate limit exceeded"}
        )
    )
    with pytest.raises(httpx.HTTPError, match="Rate limit exceeded"):
        ut_api.upload_files(mock_file)

@respx.mock
def test_upload_files_auth_error(respx_mock: respx.MockRouter, ut_api, mock_file):
    """Test handling of authentication errors."""
    respx_mock.put(url__regex="https://sea2.ingest.uploadthing.com/").mock(
        return_value=httpx.Response(
            401,
            json={"error": "Invalid API key"}
        )
    )
    with pytest.raises(httpx.HTTPError, match="Invalid API key"):
        ut_api.upload_files(mock_file)
🧰 Tools
🪛 Gitleaks (8.21.2)

222-222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

tests/test_async_client.py (1)

1-292: Add async-specific test scenarios.

Consider adding tests for:

  1. Concurrent uploads with varying file sizes
  2. Connection timeouts
  3. Cancellation handling
@pytest.mark.asyncio
@respx.mock
async def test_concurrent_uploads_varying_sizes(respx_mock: respx.MockRouter, ut_api):
    """Test concurrent uploads with varying file sizes."""
    files = [
        BytesIO(b"small" * 100),    # 500B
        BytesIO(b"medium" * 1000),  # 6KB
        BytesIO(b"large" * 10000),  # 60KB
    ]
    for i, f in enumerate(files):
        f.name = f"file{i}.txt"

    upload_response = {
        "fileHash": "hash",
        "url": "https://example.com/file",
        "ufsUrl": "https://example.com/ufs",
        "appUrl": "https://example.com/app",
    }

    respx_mock.put(url__regex="https://sea2.ingest.uploadthing.com/").mock(
        return_value=httpx.Response(200, json=upload_response)
    )

    result = await ut_api.upload_files(files)
    assert len(result) == 3
    assert all(isinstance(r, UploadResult) for r in result)

@pytest.mark.asyncio
@respx.mock
async def test_upload_timeout(respx_mock: respx.MockRouter, ut_api, mock_file):
    """Test handling of connection timeouts."""
    respx_mock.put(url__regex="https://sea2.ingest.uploadthing.com/").mock(
        side_effect=httpx.TimeoutException("Connection timed out")
    )
    with pytest.raises(httpx.TimeoutException):
        await ut_api.upload_files(mock_file)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4bd00bc and c564094.

⛔ Files ignored due to path filters (2)
  • examples/test.jpg is excluded by !**/*.jpg
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • README.md (1 hunks)
  • examples/async_main.py (1 hunks)
  • examples/main.py (2 hunks)
  • pyproject.toml (1 hunks)
  • tests/test_async_client.py (1 hunks)
  • tests/test_client.py (3 hunks)
  • upyloadthing/async_client.py (1 hunks)
  • upyloadthing/base_client.py (1 hunks)
  • upyloadthing/client.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • pyproject.toml
🔇 Additional comments (5)
examples/main.py (1)

31-54: LGTM! The multi-file upload implementation looks good.

The code effectively demonstrates how to prepare and upload multiple files simultaneously using the updated API.

upyloadthing/client.py (1)

44-66: Excellent error handling implementation!

The error handling is comprehensive and provides clear error messages. The use of type guards and proper exception chaining is a great practice.

upyloadthing/base_client.py (2)

1-26: LGTM! Well-organized imports and constants.

The imports are logically grouped and the constants are appropriately defined at module level.


65-142: LGTM! Well-defined abstract interface.

The abstract methods are well-documented with comprehensive type hints and clear docstrings.

README.md (1)

88-99: Update error handling example to use httpx.

The error handling example uses requests.exceptions.HTTPError but the client uses httpx.

-from requests.exceptions import HTTPError
+from httpx import HTTPError

try:
    api.upload_files(file)
except HTTPError as e:
    if e.response.status_code == 413:
        print("File too large")
    else:
        print(f"Upload failed: {e}")
except Exception as e:
    print(f"Unexpected error: {e}")

Comment on lines +47 to +54
# Upload both files
upload_results: List[UploadResult] = await api.upload_files(
[png_file, jpeg_file], acl="public-read"
)

print("Upload results:")
for result in upload_results:
print(f"- {result.name}: {result.file_key}")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for concurrent file uploads.

The concurrent upload implementation looks good, but consider adding error handling for individual file failures.

     # Upload both files
-    upload_results: List[UploadResult] = await api.upload_files(
-        [png_file, jpeg_file], acl="public-read"
-    )
+    try:
+        upload_results: List[UploadResult] = await api.upload_files(
+            [png_file, jpeg_file], acl="public-read"
+        )
+    except Exception as e:
+        print(f"Error uploading files: {e}")
+        return
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Upload both files
upload_results: List[UploadResult] = await api.upload_files(
[png_file, jpeg_file], acl="public-read"
)
print("Upload results:")
for result in upload_results:
print(f"- {result.name}: {result.file_key}")
# Upload both files
try:
upload_results: List[UploadResult] = await api.upload_files(
[png_file, jpeg_file], acl="public-read"
)
except Exception as e:
print(f"Error uploading files: {e}")
return
print("Upload results:")
for result in upload_results:
print(f"- {result.name}: {result.file_key}")

Comment on lines +122 to +127
# Upload all files in parallel
results = await asyncio.gather(
*[self._upload_single_file(file_data) for file_data in files_data]
)

return results
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding concurrency control for large batches.

While using asyncio.gather for concurrent uploads is efficient, consider adding a semaphore to limit concurrent connections for large batches.

+        # Limit concurrent connections
+        semaphore = asyncio.Semaphore(5)  # Limit to 5 concurrent uploads
+
+        async def upload_with_semaphore(file_data):
+            async with semaphore:
+                return await self._upload_single_file(file_data)
+
         # Upload all files in parallel
         results = await asyncio.gather(
-            *[self._upload_single_file(file_data) for file_data in files_data]
+            *[upload_with_semaphore(file_data) for file_data in files_data]
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Upload all files in parallel
results = await asyncio.gather(
*[self._upload_single_file(file_data) for file_data in files_data]
)
return results
# Limit concurrent connections
semaphore = asyncio.Semaphore(5) # Limit to 5 concurrent uploads
async def upload_with_semaphore(file_data):
async with semaphore:
return await self._upload_single_file(file_data)
# Upload all files in parallel
results = await asyncio.gather(
*[upload_with_semaphore(file_data) for file_data in files_data]
)
return results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant