Issues #242146 Feat: Download provision for uploaded file#351
Issues #242146 Feat: Download provision for uploaded file#351vaibhavsTekdi wants to merge 1 commit intotekdi:aspire-leadersfrom
Conversation
WalkthroughA secure file download feature was introduced, allowing users to download files associated with fields, with access restricted based on user roles (admin or owner). This required new endpoint/controller logic, service methods for authorization and file retrieval, interface extensions, and storage provider support for downloading files from both local and S3 backends. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FieldsController
participant FileUploadService
participant FieldsService
participant StorageProvider
participant UserRoleRepo
Client->>FieldsController: GET /download-file/:fieldId (with JWT)
FieldsController->>FileUploadService: downloadFile(fieldId, userId, userRole)
FileUploadService->>FieldsService: getField(fieldId)
FieldsService-->>FileUploadService: Field or null
alt userRole not provided
FileUploadService->>UserRoleRepo: getUserRole(userId)
UserRoleRepo-->>FileUploadService: userRole or null
end
alt user is admin
FileUploadService->>FieldsService: getFieldValuesByFieldId(fieldId)
FieldsService-->>FileUploadService: [FieldValue]
FileUploadService->>StorageProvider: download(filePath)
else user is owner
FileUploadService->>FieldsService: getFieldValue(fieldId, userId)
FieldsService-->>FileUploadService: FieldValue or null
FileUploadService->>StorageProvider: download(filePath)
end
StorageProvider-->>FileUploadService: {buffer, contentType, originalName, size}
FileUploadService-->>FieldsController: {buffer, contentType, originalName, size}
FieldsController-->>Client: File download response with headers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/storage/providers/local-storage.provider.ts (3)
117-119: Improve consistency by using async file operations.Mixed synchronous and asynchronous file operations can impact performance. Consider using
fs.promises.access()instead offs.existsSync()for consistency.- if (!fs.existsSync(filePath)) { - throw new Error('File not found in local storage'); - } + try { + await fs.promises.access(filePath, fs.constants.F_OK); + } catch { + throw new Error('File not found in local storage'); + }
132-146: Consider using a more robust content type detection.The hardcoded content type mapping is functional but limited. Consider using a library like
mime-typesfor more comprehensive and maintainable content type detection.+import { lookup as getMimeType } from 'mime-types'; + - // Simple content type mapping - const contentTypeMap: { [key: string]: string } = { - '.pdf': 'application/pdf', - '.jpg': 'image/jpeg', - '.jpeg': 'image/jpeg', - '.png': 'image/png', - '.gif': 'image/gif', - '.txt': 'text/plain', - '.doc': 'application/msword', - '.docx': 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', - '.xls': 'application/vnd.ms-excel', - '.xlsx': 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', - '.csv': 'text/csv', - '.zip': 'application/zip', - '.rar': 'application/x-rar-compressed' - }; - - if (contentTypeMap[ext]) { - contentType = contentTypeMap[ext]; - } + contentType = getMimeType(filePath) || 'application/octet-stream';
152-153: Consider preserving actual original filename.Extracting the filename from the file path may not preserve the original filename uploaded by the user. Consider storing and retrieving the original filename from file metadata or database.
The current implementation uses
path.basename(filePath)which returns the generated filename. If you need to preserve the original filename uploaded by users, consider storing it in the database and retrieving it along with file metadata.src/storage/file-upload.service.ts (1)
589-589: Fix grammatical errors in error messagesThe error messages have grammatical errors - "does not found" should be "not found".
Apply these fixes:
- throw new FileValidationException('Field values does not found for this field'); + throw new FileValidationException('Field values not found for this field');- throw new FileValidationException('Field values does not found for this field and user'); + throw new FileValidationException('Field values not found for this field and user');Also applies to: 597-597
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
src/common/utils/api-id.config.ts(1 hunks)src/fields/fields.controller.ts(2 hunks)src/fields/fields.service.ts(2 hunks)src/storage/file-upload.service.ts(5 hunks)src/storage/interfaces/field-operations.interface.ts(1 hunks)src/storage/interfaces/storage.provider.ts(2 hunks)src/storage/providers/local-storage.provider.ts(1 hunks)src/storage/providers/s3-storage.provider.ts(2 hunks)src/storage/storage.module.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: "Review the JavaScript code for conformity with the Google JavaScript...
**/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/common/utils/api-id.config.tssrc/storage/storage.module.tssrc/storage/interfaces/field-operations.interface.tssrc/storage/interfaces/storage.provider.tssrc/storage/providers/s3-storage.provider.tssrc/fields/fields.controller.tssrc/fields/fields.service.tssrc/storage/providers/local-storage.provider.tssrc/storage/file-upload.service.ts
🧬 Code Graph Analysis (3)
src/fields/fields.controller.ts (3)
src/common/utils/api-id.config.ts (1)
APIID(1-68)src/storage/exceptions/file-validation.exception.ts (1)
FileValidationException(14-30)src/common/utils/response.messages.ts (1)
API_RESPONSES(1-225)
src/fields/fields.service.ts (1)
src/storage/interfaces/field-operations.interface.ts (2)
Field(39-50)FieldValue(16-33)
src/storage/file-upload.service.ts (2)
src/storage/interfaces/field-operations.interface.ts (1)
IFieldOperations(52-94)src/storage/exceptions/file-validation.exception.ts (1)
FileValidationException(14-30)
🔇 Additional comments (15)
src/common/utils/api-id.config.ts (1)
40-40: LGTM! Consistent API identifier addition.The new
FIELDVALUES_DOWNLOADAPI identifier follows the established naming convention and value pattern correctly.src/storage/storage.module.ts (2)
9-11: LGTM! Proper RBAC entity imports.The TypeOrmModule and RBAC entity imports are correctly added to support role-based authorization for file operations.
16-17: LGTM! Correct TypeOrmModule configuration.The TypeOrmModule.forFeature configuration properly enables dependency injection of UserRoleMapping and Role repositories for the storage services.
src/storage/interfaces/storage.provider.ts (2)
7-7: LGTM! Documentation properly updated.The interface documentation correctly includes file download operations in the contract description.
29-34: LGTM! Well-designed download method interface.The download method signature is properly typed and documented, providing all necessary file metadata for download operations.
src/storage/interfaces/field-operations.interface.ts (1)
80-85: LGTM! Well-designed admin access method.The
getFieldValuesByFieldIdmethod signature is properly typed and documented for admin-level access to field values by field ID.src/storage/providers/local-storage.provider.ts (1)
114-167: LGTM! Solid download implementation with good error handling.The download method correctly implements the interface contract with comprehensive error handling and proper file operations.
src/fields/fields.controller.ts (1)
832-857: LGTM! Error handling is consistentThe error handling follows the established pattern in this controller, properly distinguishing between FileValidationException and other errors.
src/storage/providers/s3-storage.provider.ts (1)
318-372: LGTM! Well-implemented download methodThe download method is properly implemented with:
- Good error handling for missing files
- Correct stream-to-buffer conversion using async iteration
- Proper metadata extraction with fallbacks
- Appropriate content type defaulting
src/fields/fields.service.ts (3)
446-466: LGTM! Improved type safety with explicit interface mappingThe method now properly converts the ORM entity to the interface type with null handling, improving abstraction and type safety.
468-496: LGTM! Consistent interface mapping for field valuesThe method follows the same pattern as getField, properly converting the entity to the interface type with null safety.
498-522: LGTM! Well-implemented method for admin access patternsThe new method properly supports retrieving all field values for a given field ID, which aligns with the admin access requirements in the file download feature.
src/storage/file-upload.service.ts (3)
51-77: LGTM! Well-implemented role resolution from databaseThe getUserRole method properly queries the database for user roles with good error handling, returning null on any failure which allows for graceful fallback.
465-501: LGTM! Improved role checking with database lookupThe enhanced role resolution and case-insensitive admin role checking improves the robustness of the authorization logic.
577-614: LGTM! Well-designed authorization logic for file downloadsThe authorization logic properly handles both admin and non-admin users:
- Admins can download any file associated with the field
- Non-admin users can only download their own files
- Clear separation of access patterns based on user role
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Other Improvements