[WEB-2718] fix: issue attachment uploads#5904
Conversation
WalkthroughThe changes in this pull request focus on refining the asset management functionality across various endpoints. Key modifications include standardizing the assignment of 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: 2
🧹 Outside diff range and nitpick comments (2)
apiserver/plane/app/views/asset/v2.py (2)
Line range hint
1-800: Consider implementing a pre-save signal handler for FileAsset.To ensure consistent handling of
created_byacross all creation/update paths, consider implementing a pre-save signal handler for the FileAsset model. This would provide a centralized location for this logic.Example implementation in models.py:
from django.db.models.signals import pre_save from django.dispatch import receiver @receiver(pre_save, sender=FileAsset) def ensure_created_by(sender, instance, **kwargs): if not instance.created_by and hasattr(instance, '_request'): instance.created_by = instance._request.userThen in your views:
def patch(self, request, ...): asset = FileAsset.objects.get(...) asset._request = request # Attach request to instance asset.save()
Line range hint
1-800: Enhance error handling for file uploads.The current implementation could benefit from more robust error handling around file uploads and S3 operations.
Consider:
- Adding retry logic for S3 operations
- Implementing proper cleanup if upload fails
- Adding monitoring/logging for upload failures
- Implementing file upload resume capability for large files
Would you like me to create a GitHub issue to track these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apiserver/plane/app/views/asset/v2.py (3 hunks)
- apiserver/plane/app/views/issue/attachment.py (1 hunks)
- apiserver/plane/space/views/asset.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
apiserver/plane/space/views/asset.py (2)
172-173: LGTM: Consistent created_by assignment style.The explicit assignment of created_by before save is consistent with other asset handling methods across the codebase.
Line range hint
63-77: Consider enhancing file upload security measures.While basic file type and size validation exists, consider these security improvements:
- Expand allowed file types based on use case requirements
- Add content validation beyond extension checking
- Consider implementing virus scanning for uploaded files
- Add support for modern image formats (avif, heic)
Let's check if there's any file type validation elsewhere:
apiserver/plane/app/views/asset/v2.py (1)
212-213: Verify if explicitly settingcreated_byresolves the root issue.The changes consistently set
created_by = request.userbefore callingsave()across all patch methods. While this ensures the creator is properly recorded, let's verify if this addresses the root cause.Also applies to: 463-464, 695-696
| asset.created_by = request.user | ||
| asset.save() |
There was a problem hiding this comment.
Reconsider modifying created_by in PATCH method.
The PATCH method is typically used for updating resource attributes, but here it's modifying the created_by field which is creation metadata. This could lead to inconsistencies in audit trails.
Consider one of these approaches:
- asset.created_by = request.user
- asset.save()
+ # Option 1: Only update if not already set
+ if not asset.created_by:
+ asset.created_by = request.user
+ asset.save()
+ # Option 2: Don't modify created_by in patch
+ asset.save()Committable suggestion was skipped due to low confidence.
|
|
||
| # Update the attachment | ||
| issue_attachment.is_uploaded = True | ||
| issue_attachment.created_by = request.user |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Setting created_by in the patch method is incorrect and should be removed
The FileAsset model inherits from BaseModel and its serializer explicitly marks created_by as a read-only field. This indicates that created_by should only be set during creation and not be modified afterward. The current implementation in the patch method incorrectly overwrites the original creator, which violates this design principle.
Key findings:
- The
FileAssetSerializerexplicitly definescreated_byinread_only_fields - Other operations on
FileAssetinstances (like restore, delete) don't modify thecreated_byfield - The field is only set during object creation, as seen in the initial search results
🔗 Analysis chain
Verify that setting created_by in the patch method does not overwrite the original creator.
Setting issue_attachment.created_by = request.user in the patch method may overwrite the original creator of the attachment. Please confirm that this is intended behavior and that it does not affect audit trails or permissions related to the created_by field.
Run the following script to identify where created_by is set on FileAsset instances:
Let me gather more context about the patch method and how created_by is handled in the codebase.
Let me search for the FileAsset model definition to understand its fields and any history tracking mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find where 'created_by' is assigned on FileAsset instances.
# Test: Search for assignments to 'created_by' during creation.
# Expected: 'created_by' should be set during creation and not modified afterward.
# Search for creation of FileAsset with 'created_by' set
rg "FileAsset\.objects\.create\(" -A 10 | rg "created_by\s*="
# Search for assignments to 'created_by' outside of creation
rg "issue_attachment\.created_by\s*=" --glob "!.*/migrations/*" | rg -v "FileAsset\.objects\.create"
Length of output: 649
Script:
#!/bin/bash
# Get the full context of the patch method and surrounding code
rg -B 10 -A 10 "def patch\(" apiserver/plane/app/views/issue/attachment.py
# Check if there's any audit trail or history tracking for FileAsset
ast-grep --pattern 'class FileAsset($$$)'
Length of output: 882
Script:
#!/bin/bash
# Find the FileAsset model definition
rg -B 5 -A 15 "class FileAsset"
# Check for any model inheritance or history tracking mixins
rg "class FileAsset\(.*\):"
# Look for any update operations on FileAsset
rg "FileAsset\.objects\.update" -A 5
Length of output: 5952
3 participants
fix:
Issue Link: WEB-2718
Summary by CodeRabbit
New Features
Bug Fixes
created_byattribute across various asset handling methods.Documentation