-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add object #26
base: main
Are you sure you want to change the base?
feat: add object #26
Conversation
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Signed-off-by: Prati28 <[email protected]>
Reviewer's Guide by SourceryThis PR implements a new file upload functionality using MinIO as the cloud storage backend. The implementation includes chunked file processing and handles multiple file uploads. The API is documented using OpenAPI specifications. Sequence diagram for the new file upload processsequenceDiagram
actor User
participant Client as Flask Client
participant MinIO as MinIO Storage
User->>Client: Upload file(s)
Client->>Client: Generate file_id and save file
loop for each chunk
Client->>Client: Read chunk
Client->>MinIO: Upload chunk
MinIO-->>Client: Acknowledge upload
end
Client->>User: Return success response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 30 45 +15
=========================================
+ Hits 30 45 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Hey @psankhe28 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The implementation doesn't match the API specification - the spec describes using the TUS protocol for uploads but the code implements basic multipart upload instead. Please implement proper TUS protocol support or update the API spec accordingly.
- Security concern: Writing temporary files to /tmp without size limits is dangerous. Consider using stream processing or implementing proper size limits and cleanup on error paths.
- Several checklist items are incomplete: missing tests, documentation, and type annotations. Please add these before the PR can be merged.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
"description": "Chunk upload via Flask", | ||
}, | ||
) | ||
except S3Error as e: |
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.
issue (bug_risk): Add proper cleanup in error cases and improve error handling
Temporary files aren't cleaned up if an error occurs. Consider using a try/finally block and return a sanitized error response instead of exposing raw S3Error messages.
Signed-off-by: Prati28 <[email protected]>
Signed-off-by: Pratiksha Sankhe <[email protected]>
Description
Checklist
of this project, including, in particular, with regard to any style guidelines
Conventional Commits specification; in particular, it clearly
indicates that a change is a breaking change
using the PR title as the commit message
changed behavior
or updated existing ones (only for Python, TypeScript, etc.)
(Google-style Python docstrings) for all
packages/modules/functions/classes/methods or updated existing ones
works
Comments
Summary by Sourcery
Add a new endpoint for object uploads using the TUS protocol, enabling chunked file uploads to cloud storage. Update API specifications to document the new endpoint and its functionality.
New Features:
Enhancements:
Documentation: