-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[WEB 3053]feat: support LibreOffice file attachments in issues #6343
Conversation
WalkthroughThis pull request introduces significant enhancements to the issue attachment handling in the API server. The changes focus on improving file attachment management by adding file type validation, implementing soft delete functionality, and expanding support for various file formats. The modifications include adding a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Finishing Touches
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 (
|
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.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apiserver/plane/app/views/issue/attachment.py (1)
Line range hint
183-198
: HandleFileAsset
not found inget
methodWhen a
pk
is provided in theget
method, the code assumes theFileAsset
exists. If it doesn't (e.g., invalidpk
), aDoesNotExist
exception will be raised, resulting in a 500 Internal Server Error.Add exception handling to return a 404 response when the
FileAsset
is not found.Apply this diff to handle exceptions:
if pk: + try: # Get the asset asset = FileAsset.objects.get( id=pk, workspace__slug=slug, project_id=project_id ) + except FileAsset.DoesNotExist: + return Response( + {"error": "Asset not found.", "status": False}, + status=status.HTTP_404_NOT_FOUND, + ) # Check if the asset is uploaded if not asset.is_uploaded: return Response(
🧹 Nitpick comments (3)
apiserver/plane/app/views/issue/attachment.py (2)
123-128
: CreateFileAsset
after successful presigned URL generationCurrently, the
FileAsset
is created before generating the presigned URL. If the presigned URL generation fails, you'll have an orphanedFileAsset
record without a valid upload path.Consider creating the
FileAsset
only after successfully generating the presigned URL, or implement cleanup logic for failed cases.Apply this diff to adjust the creation timing:
-# Create a File Asset -asset = FileAsset.objects.create( - attributes={"name": name, "type": type, "size": size_limit}, - asset=asset_key, - size=size_limit, - workspace_id=workspace.id, - created_by=request.user, - issue_id=issue_id, - project_id=project_id, - entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT, -) # Get the presigned URL storage = S3Storage(request=request) try: presigned_url = storage.generate_presigned_post( object_name=asset_key, file_type=type, file_size=size_limit ) +except Exception as e: + return Response( + {"error": "Failed to generate upload URL.", "details": str(e), "status": False}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + ) +# Create a File Asset only after successful presigned URL generation +asset = FileAsset.objects.create( + attributes={"name": name, "type": type, "size": size_limit}, + asset=asset_key, + size=size_limit, + workspace_id=workspace.id, + created_by=request.user, + issue_id=issue_id, + project_id=project_id, + entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT, +)
Line range hint
155-160
: Remove physical files upon soft deleteIn the
delete
method, theFileAsset
is marked as deleted in the database, but the actual file in storage remains. This can lead to unnecessary storage costs and potential security risks since obsolete files are still accessible.Consider implementing a background task or modifying the deletion logic to remove the physical file from storage when an asset is soft deleted.
apiserver/plane/settings/common.py (1)
364-375
: Correct typo in comment and verify MIME typesThere's a typo in the comment on line 374~: "Open Office Bae" should be "OpenOffice Base."
Also, ensure that all added MIME types are accurate and necessary for your application's use cases.
Apply this diff to fix the typo:
- # Open Office Bae + # OpenOffice Base
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apiserver/plane/app/views/issue/attachment.py
(1 hunks)apiserver/plane/settings/common.py
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-apiserver
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apiserver/plane/app/views/issue/attachment.py (1)
123-128
: Verify handling of file size limitsThe
size_limit
is determined by thesize
provided in the request, but there is no guarantee that the actual file uploaded will respect this limit. If a user uploads a file larger than the declaredsize
, it could cause issues or exceed storage quotas.Ensure that the storage backend enforces the file size limit or consider adding server-side validation after the upload is complete.
apiserver/plane/settings/common.py (1)
364-375
: Review security implications of added MIME typesSome added MIME types, such as OpenDocument formats and certain image types, might contain executable content or scripts, potentially leading to security vulnerabilities if not properly handled.
Ensure that the application performs thorough validation and sanitization of these file types to mitigate security risks.
Also applies to: 393-394
Description
This PR allows LibreOffice, Microsoft Viseo, Netpbm, Open Office Bea files in the issue attachments.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes