[WEB-2718] fix: background task metadata#5909
Conversation
WalkthroughThe changes in this pull request primarily focus on optimizing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🧹 Outside diff range and nitpick comments (5)
apiserver/plane/app/views/asset/base.py (1)
Line range hint
1-99: Consider creating a base mixin for asset operations.There's significant duplication in the soft delete/restore logic across endpoints. Consider creating a mixin class to encapsulate common functionality:
- Error handling
- Permission checks
- Soft delete/restore operations
Example implementation:
class AssetOperationMixin: def perform_asset_operation(self, asset_key, operation, workspace_id=None): try: filters = {"asset": asset_key} if workspace_id: if not self.request.user.has_workspace_permission(workspace_id, operation): return Response( {"error": f"You don't have permission to {operation} this asset"}, status=status.HTTP_403_FORBIDDEN ) else: filters["created_by"] = self.request.user file_asset = FileAsset.objects.get(**filters) if operation == "delete": file_asset.is_deleted = True elif operation == "restore": file_asset.is_deleted = False file_asset.save(update_fields=["is_deleted"]) return Response(status=status.HTTP_204_NO_CONTENT) except FileAsset.DoesNotExist: return Response( {"error": "Asset not found"}, status=status.HTTP_404_NOT_FOUND )This would simplify the endpoint implementations and ensure consistent behavior.
apiserver/plane/space/views/asset.py (3)
196-196: LGTM! Consider wrapping in a transaction.The selective field update for soft deletion is correct and efficient. However, since we're updating multiple fields that should be atomically consistent, consider wrapping the operation in a transaction.
+ from django.db import transaction ... def delete(self, request, anchor, pk): + with transaction.atomic(): # ... existing code ... asset.is_deleted = True asset.deleted_at = timezone.now() asset.save(update_fields=["is_deleted", "deleted_at"])
221-221: LGTM! Apply same transaction pattern for consistency.The selective field update for asset restoration is correct. For consistency with the delete operation, consider applying the same transaction pattern here.
+ from django.db import transaction ... def post(self, request, anchor, asset_id): + with transaction.atomic(): # ... existing code ... asset.is_deleted = False asset.deleted_at = None asset.save(update_fields=["is_deleted", "deleted_at"])
Line range hint
172-221: Consider adding logging for asset state changes.For better observability and debugging, consider adding logging when asset states change (upload completion, deletion, restoration). This would help track the lifecycle of assets and investigate any issues.
Example logging implementation:
import logging logger = logging.getLogger(__name__) # In patch method: logger.info(f"Asset {asset.id} marked as uploaded in workspace {deploy_board.workspace_id}") # In delete method: logger.info(f"Asset {asset.id} marked as deleted in workspace {deploy_board.workspace_id}") # In restore method: logger.info(f"Asset {asset.id} restored in workspace {deploy_board.workspace_id}")apiserver/plane/app/views/asset/v2.py (1)
Line range hint
1-800: Security Advisory: Consider additional file upload validations.While the code includes basic file type and size validations, consider implementing additional security measures:
- Content-Type verification using magic numbers
- Virus/malware scanning
- File name sanitization
- Maximum filename length restrictions
Consider implementing a middleware or utility class that handles these security checks consistently across all file upload endpoints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apiserver/plane/app/views/asset/base.py (3 hunks)
- apiserver/plane/app/views/asset/v2.py (9 hunks)
- apiserver/plane/bgtasks/storage_metadata_task.py (1 hunks)
- apiserver/plane/space/views/asset.py (3 hunks)
🔇 Additional comments (11)
apiserver/plane/bgtasks/storage_metadata_task.py (2)
22-22: LGTM! Good optimization using update_fields.The change to specify
update_fields=["storage_metadata"]is a good optimization that prevents unnecessary field updates and resolves the issue of the creator field being nullified.
22-22: Verify fix for creator field issue.Let's verify that this change completely resolves the creator field issue by checking the model's behavior.
✅ Verification successful
Let me try a different approach to gather the necessary information.
The previous commands didn't yield results, so I'm trying with broader patterns to understand the model structure and its usage. This will help us verify if the creator field issue is properly addressed in the codebase.
Let me check the migrations to understand the creator field structure and any recent changes.
This will help us understand the model structure, including any creator-related fields from both the base model and migrations.
Let me check one more thing to understand the AuditModel that BaseModel inherits from, as it might contain the creator field.
This will help us understand the complete model structure and the storage_metadata field's context.
Let me check one final thing to see the UserAuditModel that contains the creator field.
This will give us the complete picture of the model inheritance chain and creator field implementation.
Now I have enough information to provide a final response about the creator field issue.
The creator field is properly handled - no issues found
The FileAsset model inherits from BaseModel which in turn inherits from AuditModel (combining TimeAuditModel, UserAuditModel, and SoftDeleteModel). The creator field is properly implemented through UserAuditModel as
created_bywith a foreign key to the User model. The code in question only updates thestorage_metadatafield usingupdate_fields, which is a safe operation that won't affect the creator field.Key findings:
- The creator field (
created_by) is properly defined in UserAuditModel- The storage_metadata update is isolated and doesn't impact other fields
- The migration (0079) preserves the creator information during data migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify FileAsset model fields and their handling # Check model definition for creator field ast-grep --pattern 'class FileAsset(models.Model): $$$ creator = $$$ $$$' # Check for any other places where FileAsset is saved rg -l "FileAsset.*save\(" --type py # Check for any migrations that might affect the creator field fd -e py -p ".*migrations.*" | xargs rg "creator"Length of output: 169
Script:
#!/bin/bash # Let's first locate the FileAsset model definition rg -A 20 "class FileAsset" --type py # Check all references to FileAsset and save operations rg -B 2 -A 2 "FileAsset.*save" --type py # Check for any creator field usage with FileAsset rg -B 2 -A 2 "FileAsset.*creator" --type py # Look for migrations containing FileAsset rg -l "FileAsset" --type py | grep "migrations"Length of output: 5495
Script:
#!/bin/bash # Check the recent migration that might be related cat apiserver/plane/db/migrations/0079_auto_20241009_0619.py # Check the initial migration for FileAsset model structure cat apiserver/plane/db/migrations/0001_initial.py | grep -A 20 "class FileAsset" # Let's also check the BaseModel since FileAsset inherits from it rg -A 10 "class BaseModel" --type pyLength of output: 2878
Script:
#!/bin/bash # Check AuditModel definition rg -A 10 "class AuditModel" --type py # Also check the storage_metadata field usage since that's what's being updated rg -B 2 -A 2 "storage_metadata" --type pyLength of output: 5293
Script:
#!/bin/bash # Check UserAuditModel definition which should contain creator field rg -A 10 "class UserAuditModel" --type py # Also check TimeAuditModel and SoftDeleteModel for completeness rg -A 10 "class (TimeAuditModel|SoftDeleteModel)" --type pyLength of output: 1968
apiserver/plane/app/views/asset/base.py (1)
53-53: Changes successfully address the creator field issue.The addition of
update_fields=["is_deleted"]to all save() calls effectively prevents the creator field from being accidentally updated to null during metadata updates. This is a good fix that:
- Solves the specific issue
- Improves performance by limiting field updates
- Prevents similar issues with other fields
Also applies to: 62-62, 99-99
apiserver/plane/space/views/asset.py (1)
172-172: LGTM! Optimized save operation prevents unintended field updates.The selective field update ensures only the modified fields (
attributesandis_uploaded) are saved, preventing any unintended changes to other fields likecreator. This change not only fixes the reported issue but also optimizes the database operation.apiserver/plane/app/views/asset/v2.py (7)
37-37: LGTM! Optimized save operation.The update_fields parameter correctly specifies only the modified fields, preventing unintended updates to other fields.
223-223: LGTM! Consistent with asset_delete implementation.The update_fields parameter matches the asset_delete method implementation, maintaining consistency across the codebase.
282-282: LGTM! Maintains consistency across endpoint classes.The update_fields parameter is consistent with the UserAssetsV2Endpoint implementation, following the same pattern for asset deletion.
462-462: LGTM! Consistent implementation across endpoint methods.The update_fields parameters in both patch and delete methods are consistent with their counterparts in UserAssetsV2Endpoint.
Also applies to: 473-473
552-552: LGTM! Consistent with deletion operations.The update_fields parameter matches other deletion-related operations, maintaining consistency in how these fields are handled.
693-693: LGTM! Consistent implementation across all endpoint classes.The update_fields parameters in both patch and delete methods maintain consistency with other endpoint implementations.
Also applies to: 708-708
212-212: Consider potential race condition in metadata handling.While the save optimization is correct, there's a potential race condition between checking
if not asset.storage_metadataand saving the asset. The background taskget_asset_object_metadatamight update the metadata while we're saving here.Consider implementing a synchronization mechanism or handling concurrent metadata updates gracefully.
| ) | ||
| file_asset.is_deleted = True | ||
| file_asset.save() | ||
| file_asset.save(update_fields=["is_deleted"]) |
There was a problem hiding this comment.
Add error handling for the delete operation.
While this endpoint correctly scopes to user's own assets, it still needs error handling.
Consider implementing these changes:
def delete(self, request, asset_key):
- file_asset = FileAsset.objects.get(
- asset=asset_key, created_by=request.user
- )
- file_asset.is_deleted = True
- file_asset.save(update_fields=["is_deleted"])
- return Response(status=status.HTTP_204_NO_CONTENT)
+ try:
+ file_asset = FileAsset.objects.get(
+ asset=asset_key, created_by=request.user
+ )
+ file_asset.is_deleted = True
+ file_asset.save(update_fields=["is_deleted"])
+ return Response(status=status.HTTP_204_NO_CONTENT)
+ except FileAsset.DoesNotExist:
+ return Response(
+ {"error": "Asset not found"},
+ status=status.HTTP_404_NOT_FOUND
+ )Committable suggestion was skipped due to low confidence.
| file_asset = FileAsset.objects.get(asset=asset_key) | ||
| file_asset.is_deleted = True | ||
| file_asset.save() | ||
| file_asset.save(update_fields=["is_deleted"]) |
There was a problem hiding this comment.
Add permission checks and error handling.
While the update_fields optimization is good, this endpoint needs additional safeguards:
- Add permission checks to ensure the user has delete rights
- Add error handling for non-existent assets
Consider implementing these changes:
def delete(self, request, workspace_id, asset_key):
asset_key = str(workspace_id) + "/" + asset_key
- file_asset = FileAsset.objects.get(asset=asset_key)
- file_asset.is_deleted = True
- file_asset.save(update_fields=["is_deleted"])
- return Response(status=status.HTTP_204_NO_CONTENT)
+ try:
+ file_asset = FileAsset.objects.get(asset=asset_key)
+
+ # Add permission check
+ if not request.user.has_workspace_permission(workspace_id, "delete"):
+ return Response(
+ {"error": "You don't have permission to delete this asset"},
+ status=status.HTTP_403_FORBIDDEN
+ )
+
+ file_asset.is_deleted = True
+ file_asset.save(update_fields=["is_deleted"])
+ return Response(status=status.HTTP_204_NO_CONTENT)
+ except FileAsset.DoesNotExist:
+ return Response(
+ {"error": "Asset not found"},
+ status=status.HTTP_404_NOT_FOUND
+ )Committable suggestion was skipped due to low confidence.
| file_asset = FileAsset.objects.get(asset=asset_key) | ||
| file_asset.is_deleted = False | ||
| file_asset.save() | ||
| file_asset.save(update_fields=["is_deleted"]) |
There was a problem hiding this comment.
Add permission checks and error handling for restore operation.
Similar to the delete endpoint, this restore operation needs additional safeguards.
Consider implementing these changes:
def restore(self, request, workspace_id, asset_key):
asset_key = str(workspace_id) + "/" + asset_key
- file_asset = FileAsset.objects.get(asset=asset_key)
- file_asset.is_deleted = False
- file_asset.save(update_fields=["is_deleted"])
- return Response(status=status.HTTP_204_NO_CONTENT)
+ try:
+ file_asset = FileAsset.objects.get(asset=asset_key)
+
+ # Add permission check
+ if not request.user.has_workspace_permission(workspace_id, "update"):
+ return Response(
+ {"error": "You don't have permission to restore this asset"},
+ status=status.HTTP_403_FORBIDDEN
+ )
+
+ file_asset.is_deleted = False
+ file_asset.save(update_fields=["is_deleted"])
+ return Response(status=status.HTTP_204_NO_CONTENT)
+ except FileAsset.DoesNotExist:
+ return Response(
+ {"error": "Asset not found"},
+ status=status.HTTP_404_NOT_FOUND
+ )📝 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.
| file_asset.save(update_fields=["is_deleted"]) | |
| def restore(self, request, workspace_id, asset_key): | |
| asset_key = str(workspace_id) + "/" + asset_key | |
| try: | |
| file_asset = FileAsset.objects.get(asset=asset_key) | |
| # Add permission check | |
| if not request.user.has_workspace_permission(workspace_id, "update"): | |
| return Response( | |
| {"error": "You don't have permission to restore this asset"}, | |
| status=status.HTTP_403_FORBIDDEN | |
| ) | |
| file_asset.is_deleted = False | |
| file_asset.save(update_fields=["is_deleted"]) | |
| return Response(status=status.HTTP_204_NO_CONTENT) | |
| except FileAsset.DoesNotExist: | |
| return Response( | |
| {"error": "Asset not found"}, | |
| status=status.HTTP_404_NOT_FOUND | |
| ) |
3 participants
fix:
Issue Link: WEB-2718
Summary by CodeRabbit
New Features
Bug Fixes
Chores