Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors AWS S3 file-storage code into a shared repository helper and starts adding support for storing temporary files in an S3 bucket, while updating various server modules to use recordFileManager instead of the previous fileManager.
Changes:
- Updated imports across survey/record/collect-import code to use
@server/modules/record/manager/recordFileManager. - Refactored S3 bucket operations into a shared
createS3BucketRepositoryhelper. - Added initial temp-file S3 repository/manager modules.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/modules/survey/service/unpublish/jobs/dataDeleteJob.js | Switches file operations import to recordFileManager. |
| server/modules/survey/manager/surveyManager.js | Switches file operations import to recordFileManager. |
| server/modules/record/service/fileService.js | Switches file operations import to recordFileManager. |
| server/modules/record/repository/fileRepositoryS3Bucket.js | Refactors S3 repo to use shared common helper. |
| server/modules/record/repository/tempFileRepositoryS3Bucket.js | Adds S3-backed temp file repository wrapper. |
| server/modules/record/manager/recordFileManager.js | Moves storage-type selection logic to a shared common module (but currently with a broken import path). |
| server/modules/record/manager/_recordManager/recordUpdateManager.js | Switches file operations import to recordFileManager. |
| server/modules/file/repository/fileRepositoryS3BucketCommon.js | New shared AWS S3 helper used by repositories. |
| server/modules/file/manager/fileManagerCommon.js | New shared storage-type selection + helper to fetch storage-specific implementation. |
| server/modules/file/manager/tempFileManager.js | New temp file manager (S3-only implementation at the moment). |
| server/modules/collectImport/service/collectImport/dataImportJobs/collectAttributeValueExtractor.js | Switches file operations import to recordFileManager. |
Comments suppressed due to low confidence (1)
server/modules/record/manager/recordFileManager.js:11
./fileManagerCommondoes not exist inserver/modules/record/manager(onlyrecordFileManager.js,recordManager.js, etc.). This import will fail at runtime/build. Consider importing the shared helper from@server/modules/file/manager/fileManagerCommon(or move the common file into this directory to match the relative import).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| region: ProcessUtils.ENV.fileStorageAwsS3BucketRegion, | ||
| }) | ||
| import { checkCanAccessS3Bucket, createS3BucketRepository } from './fileRepositoryS3BucketCommon' |
There was a problem hiding this comment.
This file imports ./fileRepositoryS3BucketCommon, but there is no fileRepositoryS3BucketCommon.js in server/modules/record/repository (the new common module is under server/modules/file/repository). This will cause a module resolution error; update the import path or relocate the common repository helper.
| import { checkCanAccessS3Bucket, createS3BucketRepository } from './fileRepositoryS3BucketCommon' | |
| import { checkCanAccessS3Bucket, createS3BucketRepository } from '../../file/repository/fileRepositoryS3BucketCommon' |
| @@ -0,0 +1,9 @@ | |||
| import { checkCanAccessS3Bucket, createS3BucketRepository } from './fileRepositoryS3BucketCommon' | |||
There was a problem hiding this comment.
This repository imports ./fileRepositoryS3BucketCommon, but that module is not present in server/modules/record/repository (it was added under server/modules/file/repository). As written, this new temp S3 repository will not load; adjust the import path or move the common helper next to this file.
| import { checkCanAccessS3Bucket, createS3BucketRepository } from './fileRepositoryS3BucketCommon' | |
| import { checkCanAccessS3Bucket, createS3BucketRepository } from '../../file/repository/fileRepositoryS3BucketCommon' |
| import { StreamUtils } from '@server/utils/streamUtils' | ||
|
|
||
| import { fileContentStorageTypes, getFileContentStorageType, getStorageFunctionOrThrow } from './fileManagerCommon' | ||
| import * as TempFileRepositoryS3Bucket from '../repository/tempFileRepositoryS3Bucket' |
There was a problem hiding this comment.
tempFileManager imports ../repository/tempFileRepositoryS3Bucket, but there is no server/modules/file/repository/tempFileRepositoryS3Bucket.js in this PR (it was added under server/modules/record/repository). This will break module resolution; either move tempFileRepositoryS3Bucket into server/modules/file/repository or update the import path accordingly.
| import * as TempFileRepositoryS3Bucket from '../repository/tempFileRepositoryS3Bucket' | |
| import * as TempFileRepositoryS3Bucket from '../../record/repository/tempFileRepositoryS3Bucket' |
| const contentAsStreamFetchFunctionByStorageType = { | ||
| [fileContentStorageTypes.s3Bucket]: TempFileRepositoryS3Bucket.getFileContentAsStream, | ||
| } | ||
|
|
||
| const contentStoreFunctionByStorageType = { | ||
| [fileContentStorageTypes.s3Bucket]: TempFileRepositoryS3Bucket.uploadFileContent, | ||
| } | ||
|
|
||
| const contentDeleteFunctionByStorageType = { | ||
| [fileContentStorageTypes.s3Bucket]: TempFileRepositoryS3Bucket.deleteFile, | ||
| } |
There was a problem hiding this comment.
insertTempFile / fetchTempFileContentAsStream / deleteTempFile use getStorageFunctionOrThrow, but only the s3Bucket storage type is implemented in the function maps. In deployments using DB or filesystem storage (the default when fileStorageAwsS3BucketName is empty), these operations will throw at runtime. Either implement the missing storage types for temp files, or make temp file storage selection explicit (separate config) and fail fast with a clearer error in checkCanAccessFilesStorage.
No description provided.