[UPY-13] Add file ACL update functionality with tests #13
Conversation
WalkthroughThis pull request introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client Application
participant API as UTApi/AsyncUTApi
participant Server as /v6/updateACL Endpoint
App->>API: Call update_acl(updates)
API->>Server: POST /v6/updateACL with updates payload
Server-->>API: JSON response { success, updated_count }
API-->>App: Return UpdateACLResponse
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (8)
🔇 Additional comments (33)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
upyloadthing/schemas.py (1)
49-52: Add field validation rules.Consider adding validation rules to ensure:
updated_countis non-negativesuccessaligns withupdated_count(e.g., success should be False when count is 0)class UpdateACLResponse(BaseModel): success: bool - updated_count: int + updated_count: int = Field(ge=0) + + @validator('success') + def validate_success(cls, v: bool, values: Dict[str, Any]) -> bool: + if 'updated_count' in values and values['updated_count'] == 0: + return False + return vtests/test_utils.py (1)
54-54: Add test case for ACL-related field names.Since we're adding ACL update functionality, consider adding test cases for ACL-related field names:
assert snakify("someString") == "someString" assert snakify("limitBytes") == "limitBytes" + assert snakify("fileACL") == "file_acl" + assert snakify("publicACL") == "public_acl" assert snakify(True) is Trueexamples/main.py (2)
75-78: Add example for custom ID-based updates.Consider demonstrating both file key and custom ID-based ACL updates:
acl_updates = [ {"fileKey": upload_results[0].file_key, "acl": "public-read"}, {"fileKey": upload_results[1].file_key, "acl": "public-read"}, + # Example with custom ID + {"customId": "my-custom-id", "acl": "private"}, ]
73-83: Extract ACL values as constants.Consider extracting ACL values as constants to improve maintainability and prevent typos:
+# ACL constants +PUBLIC_READ_ACL = "public-read" +PRIVATE_ACL = "private" + def main(): # ... existing code ... # Update ACL test print("🔒 Updating ACL settings...") acl_updates = [ - {"fileKey": upload_results[0].file_key, "acl": "public-read"}, - {"fileKey": upload_results[1].file_key, "acl": "public-read"}, + {"fileKey": upload_results[0].file_key, "acl": PUBLIC_READ_ACL}, + {"fileKey": upload_results[1].file_key, "acl": PUBLIC_READ_ACL}, ]examples/async_main.py (1)
84-93: Enhance the example to demonstrate both ACL settings.The example could be more comprehensive by demonstrating both 'public-read' and 'private' ACL settings.
Apply this diff to enhance the example:
- {"fileKey": upload_results[0].file_key, "acl": "public-read"}, - {"fileKey": upload_results[1].file_key, "acl": "public-read"}, + {"fileKey": upload_results[0].file_key, "acl": "private"}, + {"fileKey": upload_results[1].file_key, "acl": "public-read"},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(3 hunks)examples/async_main.py(1 hunks)examples/main.py(1 hunks)tests/test_async_client.py(8 hunks)tests/test_client.py(8 hunks)tests/test_utils.py(2 hunks)upyloadthing/__init__.py(2 hunks)upyloadthing/async_client.py(2 hunks)upyloadthing/client.py(2 hunks)upyloadthing/schemas.py(1 hunks)
🔇 Additional comments (7)
upyloadthing/__init__.py (1)
8-8: LGTM!The
UpdateACLResponseschema is correctly imported and exported, maintaining the module's organization and alphabetical order.Also applies to: 26-26
tests/test_utils.py (1)
66-80: LGTM!The new test function
test_snakify_dict_otheris well-structured and provides good coverage for usage info response fields.tests/test_async_client.py (3)
14-14: LGTM!The addition of
UpdateACLResponseto the imports aligns with the new ACL update functionality.
201-201: LGTM!The response field names have been consistently updated to use camelCase format, aligning with the API response structure.
Also applies to: 228-228, 250-253, 272-272, 295-295, 318-318
363-424: LGTM! Comprehensive test coverage for ACL updates.The test suite thoroughly covers all ACL update scenarios:
- Updates using file keys
- Updates using custom IDs
- Mixed updates using both identifiers
README.md (2)
85-105: LGTM! Clear and comprehensive examples.The examples effectively demonstrate all ACL update scenarios and follow the established documentation pattern.
190-196: LGTM! Clear and complete API documentation.The API reference updates thoroughly document:
- The new
update_aclmethod signature and parameters- The
UpdateACLResponsemodel structureAlso applies to: 224-226
upyloadthing/client.py
Outdated
| def update_acl(self, updates: List[dict[str, str]]) -> UpdateACLResponse: | ||
| """Update ACL settings for one or more files synchronously. | ||
|
|
||
| Args: | ||
| updates: List of update objects containing either: | ||
| - fileKey and acl | ||
| - customId and acl | ||
| where acl can be 'public-read' or 'private' | ||
|
|
||
| Returns: | ||
| UpdateACLResponse: Response containing update results | ||
| """ | ||
| result = self._request("POST", "/v6/updateACL", {"updates": updates}) | ||
| return UpdateACLResponse(**result) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for ACL values.
The method should validate that the ACL values are either 'public-read' or 'private' before making the request.
Apply this diff to add input validation:
def update_acl(self, updates: List[dict[str, str]]) -> UpdateACLResponse:
"""Update ACL settings for one or more files synchronously.
Args:
updates: List of update objects containing either:
- fileKey and acl
- customId and acl
where acl can be 'public-read' or 'private'
Returns:
UpdateACLResponse: Response containing update results
"""
+ valid_acls = {'public-read', 'private'}
+ for update in updates:
+ if 'acl' not in update or update['acl'] not in valid_acls:
+ raise ValueError("ACL must be either 'public-read' or 'private'")
result = self._request("POST", "/v6/updateACL", {"updates": updates})
return UpdateACLResponse(**result)📝 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.
| def update_acl(self, updates: List[dict[str, str]]) -> UpdateACLResponse: | |
| """Update ACL settings for one or more files synchronously. | |
| Args: | |
| updates: List of update objects containing either: | |
| - fileKey and acl | |
| - customId and acl | |
| where acl can be 'public-read' or 'private' | |
| Returns: | |
| UpdateACLResponse: Response containing update results | |
| """ | |
| result = self._request("POST", "/v6/updateACL", {"updates": updates}) | |
| return UpdateACLResponse(**result) | |
| def update_acl(self, updates: List[dict[str, str]]) -> UpdateACLResponse: | |
| """Update ACL settings for one or more files synchronously. | |
| Args: | |
| updates: List of update objects containing either: | |
| - fileKey and acl | |
| - customId and acl | |
| where acl can be 'public-read' or 'private' | |
| Returns: | |
| UpdateACLResponse: Response containing update results | |
| """ | |
| valid_acls = {'public-read', 'private'} | |
| for update in updates: | |
| if 'acl' not in update or update['acl'] not in valid_acls: | |
| raise ValueError("ACL must be either 'public-read' or 'private'") | |
| result = self._request("POST", "/v6/updateACL", {"updates": updates}) | |
| return UpdateACLResponse(**result) |
upyloadthing/async_client.py
Outdated
| async def update_acl( | ||
| self, updates: List[dict[str, str]] | ||
| ) -> UpdateACLResponse: | ||
| """Update ACL settings for one or more files asynchronously. | ||
|
|
||
| Args: | ||
| updates: List of update objects containing either: | ||
| - fileKey and acl | ||
| - customId and acl | ||
| where acl can be 'public-read' or 'private' | ||
|
|
||
| Returns: | ||
| UpdateACLResponse: Response containing update results | ||
| """ | ||
| result = await self._request( | ||
| "POST", "/v6/updateACL", {"updates": updates} | ||
| ) | ||
| return UpdateACLResponse(**result) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for ACL values.
The method should validate that the ACL values are either 'public-read' or 'private' before making the request.
Apply this diff to add input validation:
async def update_acl(
self, updates: List[dict[str, str]]
) -> UpdateACLResponse:
"""Update ACL settings for one or more files asynchronously.
Args:
updates: List of update objects containing either:
- fileKey and acl
- customId and acl
where acl can be 'public-read' or 'private'
Returns:
UpdateACLResponse: Response containing update results
"""
+ valid_acls = {'public-read', 'private'}
+ for update in updates:
+ if 'acl' not in update or update['acl'] not in valid_acls:
+ raise ValueError("ACL must be either 'public-read' or 'private'")
result = await self._request(
"POST", "/v6/updateACL", {"updates": updates}
)
return UpdateACLResponse(**result)📝 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.
| async def update_acl( | |
| self, updates: List[dict[str, str]] | |
| ) -> UpdateACLResponse: | |
| """Update ACL settings for one or more files asynchronously. | |
| Args: | |
| updates: List of update objects containing either: | |
| - fileKey and acl | |
| - customId and acl | |
| where acl can be 'public-read' or 'private' | |
| Returns: | |
| UpdateACLResponse: Response containing update results | |
| """ | |
| result = await self._request( | |
| "POST", "/v6/updateACL", {"updates": updates} | |
| ) | |
| return UpdateACLResponse(**result) | |
| async def update_acl( | |
| self, updates: List[dict[str, str]] | |
| ) -> UpdateACLResponse: | |
| """Update ACL settings for one or more files asynchronously. | |
| Args: | |
| updates: List of update objects containing either: | |
| - fileKey and acl | |
| - customId and acl | |
| where acl can be 'public-read' or 'private' | |
| Returns: | |
| UpdateACLResponse: Response containing update results | |
| """ | |
| valid_acls = {'public-read', 'private'} | |
| for update in updates: | |
| if 'acl' not in update or update['acl'] not in valid_acls: | |
| raise ValueError("ACL must be either 'public-read' or 'private'") | |
| result = await self._request( | |
| "POST", "/v6/updateACL", {"updates": updates} | |
| ) | |
| return UpdateACLResponse(**result) |
tests/test_client.py
Outdated
| @respx.mock | ||
| def test_update_acl(respx_mock: respx.MockRouter, ut_api): | ||
| """Test updating ACL settings with file keys.""" | ||
| acl_response = {"success": True, "updatedCount": 2} | ||
|
|
||
| respx_mock.post(f"{API_URL}/v6/updateACL").mock( | ||
| return_value=httpx.Response(200, json=acl_response) | ||
| ) | ||
|
|
||
| updates = [ | ||
| {"fileKey": "file_key_1", "acl": "private"}, | ||
| {"fileKey": "file_key_2", "acl": "public-read"}, | ||
| ] | ||
|
|
||
| result = ut_api.update_acl(updates) | ||
| assert isinstance(result, UpdateACLResponse) | ||
| assert result.success is True | ||
| assert result.updated_count == 2 | ||
|
|
||
|
|
||
| @respx.mock | ||
| def test_update_acl_with_custom_id(respx_mock: respx.MockRouter, ut_api): | ||
| """Test updating ACL settings with custom IDs.""" | ||
| acl_response = {"success": True, "updatedCount": 2} | ||
|
|
||
| respx_mock.post(f"{API_URL}/v6/updateACL").mock( | ||
| return_value=httpx.Response(200, json=acl_response) | ||
| ) | ||
|
|
||
| updates = [ | ||
| {"customId": "custom_123", "acl": "private"}, | ||
| {"customId": "custom_456", "acl": "public-read"}, | ||
| ] | ||
|
|
||
| result = ut_api.update_acl(updates) | ||
| assert isinstance(result, UpdateACLResponse) | ||
| assert result.success is True | ||
| assert result.updated_count == 2 | ||
|
|
||
|
|
||
| @respx.mock | ||
| def test_update_acl_mixed_updates(respx_mock: respx.MockRouter, ut_api): | ||
| """Test updating ACL settings with mixed update types.""" | ||
| acl_response = {"success": True, "updatedCount": 2} | ||
|
|
||
| respx_mock.post(f"{API_URL}/v6/updateACL").mock( | ||
| return_value=httpx.Response(200, json=acl_response) | ||
| ) | ||
|
|
||
| updates = [ | ||
| {"fileKey": "file_key_123", "acl": "private"}, | ||
| {"customId": "custom_456", "acl": "public-read"}, | ||
| ] | ||
|
|
||
| result = ut_api.update_acl(updates) | ||
| assert isinstance(result, UpdateACLResponse) | ||
| assert result.success is True | ||
| assert result.updated_count == 2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error case tests for ACL updates.
The tests should cover error cases such as invalid ACL values and missing required fields.
Add the following test cases:
@respx.mock
def test_update_acl_with_invalid_acl(respx_mock: respx.MockRouter, ut_api):
"""Test updating ACL settings with invalid ACL value."""
updates = [
{"fileKey": "file_key_1", "acl": "invalid-acl"},
]
with pytest.raises(ValueError, match="ACL must be either 'public-read' or 'private'"):
ut_api.update_acl(updates)
@respx.mock
def test_update_acl_with_missing_acl(respx_mock: respx.MockRouter, ut_api):
"""Test updating ACL settings with missing ACL field."""
updates = [
{"fileKey": "file_key_1"}, # Missing acl field
]
with pytest.raises(ValueError, match="ACL must be either 'public-read' or 'private'"):
ut_api.update_acl(updates)
@respx.mock
def test_update_acl_with_missing_identifier(respx_mock: respx.MockRouter, ut_api):
"""Test updating ACL settings with missing identifier."""
updates = [
{"acl": "public-read"}, # Missing fileKey/customId
]
acl_response = {"success": False, "error": "Missing identifier"}
respx_mock.post(f"{API_URL}/v6/updateACL").mock(
return_value=httpx.Response(400, json=acl_response)
)
with pytest.raises(httpx.HTTPError):
ut_api.update_acl(updates)
farmisen
force-pushed
the
farmisen/upy-13-implement-the-update-acl-function
branch
from
53b328a to
1b3cdce
Compare
Add ACL (Access Control List) update functionality with corresponding tests and documentation
This change introduces a new API capability to modify access control settings for uploaded files, supporting both file keys and custom IDs. The implementation includes synchronous and asynchronous client methods, comprehensive test coverage, and documentation updates.
Changes
Impact
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
.gitignoreto exclude specific files from version control.