Conversation
…for temporary file download
…ity and consistency
…dOneById to use it
There was a problem hiding this comment.
Pull request overview
Adds support for uploading “temporary” files in the files-storage module by introducing a storageDirectory concept (currently TEMP), adjusting persistence/pathing, and exposing an expiry timestamp to clients.
Changes:
- Add
StorageDirectory(TEMP) and propagate it through DTOs/factories/service intoFileRecord+ DB entity. - Add a new
POST /file/temp/upload/...endpoint that uploads into the TEMP directory. - Expose
expiresAtinFileRecordResponseand update record lookup to include TEMP records.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/files-storage/testing/file-dto.test.factory.ts | Updates test DTO factory to include the new storageDirectory field. |
| src/modules/files-storage/repo/file-record.repo.ts | Changes findOneById filtering to include TEMP records. |
| src/modules/files-storage/repo/file-record.entity.ts | Persists storageDirectory on the Mongo entity. |
| src/modules/files-storage/repo/file-record-scope.ts | Adds byNotDeletedOrTemp() scope used for TEMP record retrieval. |
| src/modules/files-storage/domain/service/files-storage.service.ts | Passes storageDirectory into FileRecordFactory.buildFromExternalInput during upload. |
| src/modules/files-storage/domain/file-record.do.ts | Introduces StorageDirectory, adds getExpiresAt(), and prefixes S3 paths with storageDirectory. |
| src/modules/files-storage/domain/factory/pass-through-file-dto.factory.ts | Carries storageDirectory into pass-through DTOs. |
| src/modules/files-storage/domain/factory/file-record.factory.ts | Allows building a FileRecord with an optional storageDirectory. |
| src/modules/files-storage/domain/factory/file-dto.factory.ts | Extends FileDtoFactory.create to accept storageDirectory. |
| src/modules/files-storage/domain/dto/pass-through-file.dto.ts | Adds storageDirectory to PassThroughFileDto. |
| src/modules/files-storage/domain/dto/file.dto.ts | Adds storageDirectory to FileDto. |
| src/modules/files-storage/api/uc/files-storage.uc.ts | Adds tempUpload() and threads storageDirectory through busboy upload. |
| src/modules/files-storage/api/mapper/file-dto.mapper.ts | Maps busboy file info into FileDto with optional storageDirectory. |
| src/modules/files-storage/api/dto/file-storage.response.ts | Adds expiresAt to the API response model. |
| src/modules/files-storage/api/controller/files-storage.controller.ts | Adds the new temporary upload route. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public byNotDeletedOrTemp(): this { | ||
| this.addQuery({ $or: [{ deletedSince: null }, { storageDirectory: StorageDirectory.TEMP }] }); |
There was a problem hiding this comment.
byNotDeletedOrTemp() will return TEMP records regardless of deletedSince. This means a TEMP file that is explicitly deleted (or otherwise marked) can still be fetched via findOneById, undermining delete semantics. If TEMP files need to be readable only until expiry, consider filtering TEMP by an explicit expiry window/field (or excluding those additionally marked-for-delete).
| this.addQuery({ $or: [{ deletedSince: null }, { storageDirectory: StorageDirectory.TEMP }] }); | |
| this.addQuery({ deletedSince: null }); |
| public async findOneById(id: EntityId): Promise<FileRecord> { | ||
| const scope = new FileRecordScope().byFileRecordId(id).byMarkedForDelete(false); | ||
| const scope = new FileRecordScope().byFileRecordId(id).byNotDeletedOrTemp(); | ||
| const fileRecord = await this.findOneOrFail(scope); | ||
|
|
There was a problem hiding this comment.
findOneById() now includes TEMP records via byNotDeletedOrTemp(), but other repo methods (e.g. findMultipleById, findByParentId) still use byMarkedForDelete(false) and will exclude TEMP records because TEMP sets deletedSince. This inconsistency can lead to surprising behavior where single-id operations work but multi-id/parent-based operations silently omit temp uploads. Consider introducing explicit "includeTemp" variants or aligning the filtering strategy across read APIs.
| public getExpiresAt(): Date | undefined { | ||
| if (!this.isTempFile() || !this.props.deletedSince) { | ||
| return; | ||
| } | ||
|
|
||
| const sevenDaysInMs = 7 * 24 * 60 * 60 * 1000; | ||
| const expiresAt = new Date(this.props.deletedSince.getTime() + sevenDaysInMs); | ||
|
|
||
| return expiresAt; | ||
| } |
There was a problem hiding this comment.
New TEMP behavior (StorageDirectory, getExpiresAt()) is not covered by the existing file-record.do.spec.ts tests. Please add unit tests covering at least: (1) TEMP records produce an expiresAt based on the chosen retention, and (2) non-TEMP records return undefined.
| public createPath(): string { | ||
| const path = [this.props.storageLocationId, this.id].join('/'); | ||
| const path = [this.props.storageDirectory, this.props.storageLocationId, this.id].filter(Boolean).join('/'); | ||
|
|
||
| return path; |
There was a problem hiding this comment.
createPath() now conditionally prefixes storageDirectory, but there is no test asserting the TEMP path shape (e.g. temp/<storageLocationId>/<id>). Adding a unit test for TEMP paths would help prevent accidental breaking changes to S3 key layout.
| @ApiResponse({ status: 400, type: BadRequestException }) | ||
| @ApiResponse({ status: 403, type: ForbiddenException }) | ||
| @ApiResponse({ status: 500, type: InternalServerErrorException }) | ||
| @ApiConsumes('multipart/form-data') |
There was a problem hiding this comment.
The new streaming temp upload endpoint does not apply CurrentUploadMetricsInterceptor, while the normal multipart upload endpoint does. If upload metrics/limits rely on that interceptor, temp uploads will be missing from dashboards and alerts. Consider adding the same interceptor here for parity.
| @ApiConsumes('multipart/form-data') | |
| @ApiConsumes('multipart/form-data') | |
| @UseInterceptors(CurrentUploadMetricsInterceptor) |
|
Description
Links to Tickets or other pull requests
Screenshots of UI changes
Approval for review