Implement media storage enhancements with Metadata Saving#120
Implement media storage enhancements with Metadata Saving#120gabito1451 wants to merge 5 commits into
Conversation
WalkthroughIntroduces a full media storage subsystem: new storage providers (local, S3), storage interface, enhanced upload service (virus scanning, image processing, multi-backend uploads/deletes), file metadata entity and persistence, upload route/controller wiring, scheduled cleanup job, and package dependency updates for clamav and sharp. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant FileUploadRoute
participant Multer
participant FileUploadController
participant FileUploadService
participant ClamAV
participant Sharp
participant StorageProvider
participant MetadataDB
Client->>ExpressApp: POST /api/files/upload (multipart)
ExpressApp->>FileUploadRoute: dispatch
FileUploadRoute->>Multer: parse file
Multer->>FileUploadController: attach file to req
FileUploadController->>FileUploadService: processAndUploadFile(file)
FileUploadService->>ClamAV: scan file
ClamAV-->>FileUploadService: scan result
alt clean
FileUploadService->>Sharp: process image (resize/compress)
Sharp-->>FileUploadService: processed buffer
FileUploadService->>StorageProvider: uploadFile(processed)
StorageProvider-->>FileUploadService: fileKey / URL
FileUploadService->>MetadataDB: save metadata (FileMetadata)
MetadataDB-->>FileUploadService: saved record
FileUploadService-->>FileUploadController: url & metadata
FileUploadController-->>Client: 200 {url, metadata}
else infected
FileUploadService-->>FileUploadController: error (rejected)
FileUploadController-->>Client: 400/422
end
sequenceDiagram
participant Cron
participant MetadataDB
participant ReferenceQuery
participant FileUploadService
participant StorageProvider
Cron->>MetadataDB: fetch metadata (batches)
loop per file
MetadataDB->>ReferenceQuery: check references (user_documents)
alt referenced or tagged important
Note right of Cron: skip deletion
else eligible
Cron->>FileUploadService: deleteFile(fileUrl)
FileUploadService->>StorageProvider: deleteFile(key)
StorageProvider-->>FileUploadService: confirmation
FileUploadService->>MetadataDB: remove record
end
end
Cron-->>Cron: log completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (8)
src/controllers/fileUpload.controller.ts (1)
17-20: Use safer property access for req.file.The code uses non-null assertion (
req.file.filename,req.file.mimetype, etc.) without additional null checks beyond the initialif (!req.file)check. While this should be safe given the guard clause, using optional chaining or additional validation would be more defensive.Consider using safer property access:
const metadata = await fileUploadService.saveFileMetadata({ - filename: req.file.filename, - mimetype: req.file.mimetype, - size: req.file.size, + filename: req.file?.filename || '', + mimetype: req.file?.mimetype || '', + size: req.file?.size || 0, });src/tasks/cleanUpTask.ts (1)
7-7: Consider making the cron schedule configurable.The cleanup schedule is hardcoded to run at 2:00 AM daily. Consider making this configurable through environment variables for different deployment environments.
- cron.schedule("0 2 * * *", async () => { + const schedule = process.env.CLEANUP_CRON_SCHEDULE || "0 2 * * *"; + cron.schedule(schedule, async () => {src/routes/fileUpload.route.ts (1)
10-10: Avoid creating multiple service instances.Creating a new
FileUploadServiceinstance in the route when the controller also creates one could lead to inconsistent state or unnecessary resource usage.Consider using dependency injection or a singleton pattern:
-const fileUploadService = new FileUploadService(); +// Import from a shared instance or use dependency injection +import { fileUploadService } from '../services/fileUpload.service';src/storage/localStorageProvider.ts (1)
19-21: Add return type and validate fileKey- getFileUrl(fileKey: string) { + getFileUrl(fileKey: string): string { + // Ensure fileKey doesn't contain path separators + const safeKey = path.basename(fileKey); - return `/uploads/${fileKey}`; + return `/uploads/${safeKey}`; }src/storage/storageProvider.ts (1)
3-3: Consider renaming to avoid confusion with the StorageProvider typeThere's a
StorageProvidertype defined insrc/models/fileMetaData.model.tsas a string union type. Consider renaming this interface toIStorageProviderorStorageProviderInterfaceto avoid confusion.-export interface StorageProvider { +export interface IStorageProvider {src/models/fileMetaData.model.ts (1)
36-37: Consider database portability for array columnsPostgreSQL array columns aren't supported by all databases (e.g., MySQL, SQLite). Consider using a many-to-many relationship with a separate tags table for better portability.
src/services/fileUpload.service.ts (2)
89-102: Handle buffer input and make image processing configurableThe method assumes
file.pathexists, which won't work with memory storage.private async processImage( file: Express.Multer.File, ): Promise<Express.Multer.File> { - const buffer = await sharp(file.path) - .resize(800, 800, { fit: "inside" }) - .jpeg({ quality: 80 }) + const maxDimension = parseInt(process.env.IMAGE_MAX_DIMENSION || '800', 10); + const quality = parseInt(process.env.IMAGE_QUALITY || '80', 10); + + const input = file.path || file.buffer; + if (!input) throw new Error('No file data available'); + + const buffer = await sharp(input) + .resize(maxDimension, maxDimension, { fit: "inside" }) + .jpeg({ quality }) .toBuffer(); return { ...file, buffer, size: buffer.length, }; }
15-153: Consider using the StorageProvider pattern consistentlyThis service duplicates logic that's already in
LocalStorageProviderandS3StorageProvider. Consider refactoring to use those implementations:// Example refactor export class FileUploadService { private storageProvider: StorageProvider; constructor() { const storageType = process.env.STORAGE || 'local'; this.storageProvider = storageType === 's3' ? new S3StorageProvider() : new LocalStorageProvider(); } async processAndUploadFile(file: Express.Multer.File): Promise<string> { // Scan and process... return this.storageProvider.uploadFile(processedFile); } }This would:
- Reduce code duplication
- Make the service more focused (SRP)
- Make it easier to add new storage providers
- Improve testability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(3 hunks)src/app.ts(2 hunks)src/controllers/fileUpload.controller.ts(1 hunks)src/models/fileMetaData.model.ts(1 hunks)src/routes/fileUpload.route.ts(1 hunks)src/services/fileUpload.service.ts(1 hunks)src/storage/localStorageProvider.ts(1 hunks)src/storage/s3StorageProvider.ts(1 hunks)src/storage/storageProvider.ts(1 hunks)src/tasks/cleanUpTask.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/storage/localStorageProvider.ts (2)
src/models/fileMetaData.model.ts (1)
StorageProvider(10-10)src/storage/storageProvider.ts (1)
StorageProvider(3-10)
src/routes/fileUpload.route.ts (2)
src/services/fileUpload.service.ts (1)
FileUploadService(15-153)src/middlewares/merchantAuth.ts (1)
asyncHandler(20-24)
src/storage/s3StorageProvider.ts (2)
src/models/fileMetaData.model.ts (1)
StorageProvider(10-10)src/storage/storageProvider.ts (1)
StorageProvider(3-10)
src/storage/storageProvider.ts (1)
src/models/fileMetaData.model.ts (1)
StorageProvider(10-10)
src/models/fileMetaData.model.ts (1)
src/storage/storageProvider.ts (1)
StorageProvider(3-10)
🔇 Additional comments (3)
src/app.ts (1)
28-28: LGTM! Route integration follows established patterns.The import and route mounting for the new file upload functionality are correctly implemented and follow the same pattern as existing routes in the application.
Also applies to: 154-154
src/storage/storageProvider.ts (1)
3-10: Good interface design with proper typingThe interface is well-designed with async methods and optional parameters for extensibility.
src/models/fileMetaData.model.ts (1)
12-57: Well-structured entity with good practicesGood implementation details:
- Proper use of indexes on frequently queried fields
- Snake_case column naming for database consistency
- Automatic timestamp with
@CreateDateColumn- UUID primary key for better distribution
| "axios": "^1.8.1", | ||
| "bcryptjs": "^2.4.3", | ||
| "bull": "^4.16.5", | ||
| "clamav.js": "^0.12.0", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify security of new dependencies and document ClamAV requirement.
The addition of clamav.js and sharp introduces new security considerations:
clamav.jsrequires a ClamAV daemon running on port 3310 (as seen in the service code)sharpis a native module that processes image data
Please verify these packages for known vulnerabilities and document the ClamAV setup requirement:
Also applies to: 50-50
🏁 Script executed:
#!/bin/bash
# Check for known vulnerabilities in the new packages
npm audit --only=prod
# Check specific packages for security advisories
curl -s "https://registry.npmjs.org/clamav.js" | jq '.versions | keys | .[-5:]'
curl -s "https://registry.npmjs.org/sharp" | jq '.versions | keys | .[-5:]'
# Verify latest versions
npm view clamav.js version
npm view sharp versionLength of output: 793
Security verification & documentation needed for clamav.js@^0.12.0 and sharp@^0.34.3
• Both clamav.js and sharp are pinned to their latest published versions.
• npm audit --only=prod failed due to a missing lockfile. Please:
– Generate and commit a lockfile (npm i --package-lock-only).
– Re-run vulnerability checks (npm audit --omit=dev or review the npm/GitHub advisories directly).
• Document the ClamAV daemon requirement in your README or deployment docs:
– Default host/port (e.g. localhost:3310).
– Example Docker-Compose service for ClamAV.
• Document Sharp’s native prerequisites (libvips, build tools) or link to Sharp’s official install guide.
🤖 Prompt for AI Agents
In package.json at line 26, the dependencies clamav.js@^0.12.0 and sharp@^0.34.3
are used without a lockfile, causing npm audit to fail. Generate and commit a
package-lock.json by running 'npm i --package-lock-only', then re-run 'npm audit
--omit=dev' to check for vulnerabilities. Additionally, update the README or
deployment documentation to include ClamAV daemon requirements such as default
host and port (localhost:3310), provide an example Docker-Compose service for
ClamAV, and document Sharp's native prerequisites like libvips and build tools
or link to Sharp's official installation guide.
| async deleteFile(fileKey: string) { | ||
| const params = { | ||
| Bucket: this.bucketName, | ||
| Key: fileKey, | ||
| }; | ||
| await s3.deleteObject(params).promise(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update to AWS SDK v3 syntax and add return type
- async deleteFile(fileKey: string) {
+ async deleteFile(fileKey: string): Promise<void> {
const params = {
Bucket: this.bucketName,
Key: fileKey,
};
- await s3.deleteObject(params).promise();
+ await this.s3Client.send(new DeleteObjectCommand(params));
}📝 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 deleteFile(fileKey: string) { | |
| const params = { | |
| Bucket: this.bucketName, | |
| Key: fileKey, | |
| }; | |
| await s3.deleteObject(params).promise(); | |
| } | |
| async deleteFile(fileKey: string): Promise<void> { | |
| const params = { | |
| Bucket: this.bucketName, | |
| Key: fileKey, | |
| }; | |
| await this.s3Client.send(new DeleteObjectCommand(params)); | |
| } |
🤖 Prompt for AI Agents
In src/storage/s3StorageProvider.ts around lines 25 to 31, the deleteFile method
uses AWS SDK v2 syntax and lacks an explicit return type. Update the method to
use AWS SDK v3 syntax by importing and using the DeleteObjectCommand with the S3
client’s send method. Also, add an explicit return type Promise<void> to the
deleteFile method signature.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/config/db.ts (1)
33-48: Critical: Missing FileMetadata entity registration.The
FileMetadataentity fromsrc/models/fileMetaData.model.tsis not included in the entities array, which will cause runtime errors when the cleanup task and file upload services try to access this entity.Add the FileMetadata entity to the entities array:
entities: [ User, TwoFactorAuth, Session, EmailVerification, WalletVerification, MerchantEntity, MerchantWebhookEntity, MerchantWebhookEventEntity, PaymentLink, Referral, ReferralReward, ReferralProgram, AuditLog, Wallet, + FileMetadata, ],You'll also need to add the import:
+import { FileMetadata } from "../models/fileMetaData.model";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(3 hunks)src/config/db.ts(1 hunks)src/controllers/fileUpload.controller.ts(1 hunks)src/models/fileMetaData.model.ts(1 hunks)src/storage/s3StorageProvider.ts(1 hunks)src/storage/storageProvider.ts(1 hunks)src/tasks/cleanUpTask.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/controllers/fileUpload.controller.ts
- src/storage/s3StorageProvider.ts
- src/storage/storageProvider.ts
- package.json
🔇 Additional comments (4)
src/config/db.ts (1)
22-22: LGTM! Export enables cross-module database access.The change from local to exported constant correctly enables the cleanup task and other modules to access the database connection.
src/tasks/cleanUpTask.ts (1)
51-78: LGTM! Proper reference checking implemented.The function now correctly checks actual file references in the database and has been appropriately renamed. The query runner usage and error handling are well implemented.
src/models/fileMetaData.model.ts (2)
9-9: LGTM! Good type naming to avoid conflicts.The
StorageProviderTypename appropriately avoids conflicts with theStorageProviderinterface mentioned in past reviews.
11-57: Well-designed entity with proper indexing and constraints.The entity design is solid with:
- Appropriate indexes on
userIdandcategoryfor query optimization- Proper column mappings with snake_case database names
- Sensible defaults for
tagsarray andisWatermarkedboolean- Good use of TypeORM decorators and nullable fields
The structure supports the file upload and cleanup functionality well.
|
GM Ser @respp , please kindly review |
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
Hi @gabito1451, thanks for your PR. You’ve implemented a solid foundation: the FileMetadata model, the storage providers (local and S3), and a well-designed cleanup system. To complete the implementation according the issue, it’s important to register the FileMetadata entity in db.ts and finish the CRUD API (list, retrieve, update, and delete files). Some basic validations like file type and size are also missing. Additionally, I noticed you used atomic commits, which is a great practice, but the commit messages are not descriptive at all. You should follow the Git contribution guidelines for that. By addressing this and the suggestions from the CodeRabbit AI, you’ll be much closer to completing your implementation. The foundation is strong, but it’s not ready to be merged just yet.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(3 hunks)src/app.ts(6 hunks)src/config/db.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/config/db.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/app.ts (7)
src/middlewares/ipValidation.middleware.ts (1)
validateIpAddress(15-66)src/middlewares/globalRateLimiter.middleware.ts (1)
globalRateLimiter(3-12)src/middlewares/requestLogger.middleware.ts (1)
requestLogger(9-31)src/middleware/rateLimiter.ts (1)
intelligentRateLimiter(24-253)src/utils/schedular.ts (1)
startExpiredSessionCleanupCronJobs(4-12)src/utils/subscriptionScheduler.ts (1)
subscriptionScheduler(59-59)src/config/auth0Config.ts (1)
oauthConfig(1-19)
🪛 GitHub Actions: CI Pipeline
src/app.ts
[error] 1-1: Prettier formatting check failed during 'npm run format:check'. src/app.ts has formatting issues. Run 'prettier --write' to fix.
🔇 Additional comments (1)
src/app.ts (1)
1-174: Fix Prettier formatting to unblock CI.
npm run format:checkis failing on this file. Please run Prettier locally (e.g.,prettier --write src/app.ts) so the pipeline passes.
| app.use(RateLimitMonitoringService.createRateLimitMonitoringMiddleware() as RequestHandler); | ||
| app.use(globalRateLimiter as RequestHandler); | ||
| app.use(requestLogger as RequestHandler); | ||
| app.use(intelligentRateLimiter); | ||
| app.use(rateLimitMonitoringService.createRateLimitMonitoringMiddleware()); | ||
|
|
There was a problem hiding this comment.
Remove duplicate rate-limit monitoring middleware.
Both RateLimitMonitoringService.createRateLimitMonitoringMiddleware() and rateLimitMonitoringService.createRateLimitMonitoringMiddleware() resolve to the same middleware, so registering them twice will double-log every request (and double any downstream persistence work). Keep just one invocation to avoid duplicate side effects.
🤖 Prompt for AI Agents
In src/app.ts around lines 74 to 79, the rate-limit monitoring middleware is
registered twice
(RateLimitMonitoringService.createRateLimitMonitoringMiddleware() and
rateLimitMonitoringService.createRateLimitMonitoringMiddleware()); remove the
duplicate registration to avoid double-logging and double persistence—delete the
first invocation
(RateLimitMonitoringService.createRateLimitMonitoringMiddleware()) and keep the
existing rateLimitMonitoringService.createRateLimitMonitoringMiddleware() call.
|
any updates?? @gabito1451 |
2 participants
Pull Request Overview
📝 Summary
This PR introduces improvements to the file upload feature in the Express backend. It includes middleware for handling file uploads and a method for saving file metadata.
🔗 Related Issues
🔄 Changes Made
controllers/fileUpload.controller.tsAdded
handleFileUploadmiddleware:FileUploadServiceto generate file URL.fileUrltoreq.body.Added
saveFileMetadatacontroller method:services/fileUpload.service.ts🖼️ Current Output
Give evidence like screenshots of what your job looks like.
🧪 Testing
If applicable, describe the tests performed. Provide screenshots, test outputs, or any resources that can help reviewers understand how the changes were tested.
💬 Comments
Any additional context, questions, or considerations for the reviewers.
Summary by CodeRabbit
New Features
Bug Fixes
Chores