-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add configurable file upload options and related tests #403
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ import { GetFile } from './domain/useCases/GetFile' | |
| import { GetFileCitation } from './domain/useCases/GetFileCitation' | ||
| import { GetFileAndDataset } from './domain/useCases/GetFileAndDataset' | ||
| import { UploadFile } from './domain/useCases/UploadFile' | ||
| import { DirectUploadClient } from './infra/clients/DirectUploadClient' | ||
| import { DirectUploadClient, DirectUploadClientConfig } from './infra/clients/DirectUploadClient' | ||
| import { AddUploadedFilesToDataset } from './domain/useCases/AddUploadedFilesToDataset' | ||
| import { DeleteFile } from './domain/useCases/DeleteFile' | ||
| import { ReplaceFile } from './domain/useCases/ReplaceFile' | ||
|
|
@@ -20,8 +20,40 @@ import { UpdateFileCategories } from './domain/useCases/UpdateFileCategories' | |
| import { GetFileVersionSummaries } from './domain/useCases/GetFileVersionSummaries' | ||
| import { IsFileDeleted } from './domain/useCases/IsFileDeleted' | ||
|
|
||
| /** | ||
| * Configuration for file upload operations. | ||
| * Use FilesConfig.init() to configure upload behavior before using uploadFile. | ||
| */ | ||
| class FilesConfig { | ||
| private static uploadConfig: DirectUploadClientConfig = {} | ||
|
|
||
| /** | ||
| * Initialize file upload configuration. | ||
| * @param config - Configuration options for file uploads | ||
| * @param config.useS3Tagging - Whether to include S3 tagging header (x-amz-tagging: dv-state=temp). | ||
| * Set to false if your S3 implementation doesn't support object tagging. Default: true | ||
| * @param config.maxMultipartRetries - Maximum number of retries for multipart upload parts. Default: 5 | ||
| * @param config.fileUploadTimeoutMs - Timeout in milliseconds for file upload operations. Default: 60000 | ||
| */ | ||
| static init(config: DirectUploadClientConfig) { | ||
| this.uploadConfig = config | ||
| } | ||
|
|
||
| static getConfig(): DirectUploadClientConfig { | ||
| return this.uploadConfig | ||
| } | ||
| } | ||
|
|
||
| const filesRepository = new FilesRepository() | ||
| const directUploadClient = new DirectUploadClient(filesRepository) | ||
| // DirectUploadClient is created lazily to allow configuration before first use | ||
| let directUploadClientInstance: DirectUploadClient | null = null | ||
|
|
||
| const getDirectUploadClient = (): DirectUploadClient => { | ||
| if (!directUploadClientInstance) { | ||
| directUploadClientInstance = new DirectUploadClient(filesRepository, FilesConfig.getConfig()) | ||
| } | ||
| return directUploadClientInstance | ||
| } | ||
|
Comment on lines
+51
to
+56
|
||
|
|
||
| const getDatasetFiles = new GetDatasetFiles(filesRepository) | ||
| const getDatasetFileCounts = new GetDatasetFileCounts(filesRepository) | ||
|
|
@@ -32,7 +64,6 @@ const getDatasetFilesTotalDownloadSize = new GetDatasetFilesTotalDownloadSize(fi | |
| const getFile = new GetFile(filesRepository) | ||
| const getFileAndDataset = new GetFileAndDataset(filesRepository) | ||
| const getFileCitation = new GetFileCitation(filesRepository) | ||
| const uploadFile = new UploadFile(directUploadClient) | ||
| const addUploadedFilesToDataset = new AddUploadedFilesToDataset(filesRepository) | ||
| const deleteFile = new DeleteFile(filesRepository) | ||
| const replaceFile = new ReplaceFile(filesRepository) | ||
|
|
@@ -43,6 +74,27 @@ const updateFileCategories = new UpdateFileCategories(filesRepository) | |
| const getFileVersionSummaries = new GetFileVersionSummaries(filesRepository) | ||
| const isFileDeleted = new IsFileDeleted(filesRepository) | ||
|
|
||
| // uploadFile is created lazily to respect FilesConfig settings | ||
| let uploadFileInstance: UploadFile | null = null | ||
|
|
||
| /** | ||
| * Uploads a file to remote storage and returns the storage identifier. | ||
| * Respects FilesConfig settings (call FilesConfig.init() before first upload if you need custom config). | ||
| */ | ||
| const uploadFile = { | ||
| execute: ( | ||
| datasetId: number | string, | ||
| file: File, | ||
| progress: (now: number) => void, | ||
| abortController: AbortController | ||
| ): Promise<string> => { | ||
| if (!uploadFileInstance) { | ||
| uploadFileInstance = new UploadFile(getDirectUploadClient()) | ||
| } | ||
| return uploadFileInstance.execute(datasetId, file, progress, abortController) | ||
| } | ||
| } | ||
|
|
||
| export { | ||
| getDatasetFiles, | ||
| getFileDownloadCount, | ||
|
|
@@ -62,7 +114,8 @@ export { | |
| updateFileCategories, | ||
| replaceFile, | ||
| getFileVersionSummaries, | ||
| isFileDeleted | ||
| isFileDeleted, | ||
| FilesConfig | ||
| } | ||
|
|
||
| export { FileModel as File, FileEmbargo, FileChecksum } from './domain/models/FileModel' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,6 +87,56 @@ describe('uploadFile', () => { | |
|
|
||
| expect(actual).toEqual(testDestination.storageId) | ||
| }) | ||
|
|
||
| test('should include S3 tagging header by default', async () => { | ||
| const filesRepositoryStub: IFilesRepository = {} as IFilesRepository | ||
| const testDestination: FileUploadDestination = createSingleFileUploadDestinationModel() | ||
| filesRepositoryStub.getFileUploadDestination = jest.fn().mockResolvedValue(testDestination) | ||
|
|
||
| const axiosPutSpy = jest.spyOn(axios, 'put').mockResolvedValue(undefined) | ||
|
|
||
| const sut = new DirectUploadClient(filesRepositoryStub) | ||
|
|
||
| const progressMock = jest.fn() | ||
| const abortController = new AbortController() | ||
|
|
||
| await sut.uploadFile(1, testFile, progressMock, abortController) | ||
|
|
||
| expect(axiosPutSpy).toHaveBeenCalledWith( | ||
| testDestination.urls[0], | ||
| expect.anything(), | ||
| expect.objectContaining({ | ||
| headers: expect.objectContaining({ | ||
| 'x-amz-tagging': 'dv-state=temp' | ||
| }) | ||
| }) | ||
| ) | ||
| }) | ||
|
|
||
| test('should not include S3 tagging header when useS3Tagging is false', async () => { | ||
| const filesRepositoryStub: IFilesRepository = {} as IFilesRepository | ||
| const testDestination: FileUploadDestination = createSingleFileUploadDestinationModel() | ||
| filesRepositoryStub.getFileUploadDestination = jest.fn().mockResolvedValue(testDestination) | ||
|
|
||
| const axiosPutSpy = jest.spyOn(axios, 'put').mockResolvedValue(undefined) | ||
|
|
||
| const sut = new DirectUploadClient(filesRepositoryStub, { useS3Tagging: false }) | ||
|
|
||
| const progressMock = jest.fn() | ||
| const abortController = new AbortController() | ||
|
|
||
| await sut.uploadFile(1, testFile, progressMock, abortController) | ||
|
|
||
| expect(axiosPutSpy).toHaveBeenCalledWith( | ||
| testDestination.urls[0], | ||
| expect.anything(), | ||
| expect.objectContaining({ | ||
| headers: expect.not.objectContaining({ | ||
| 'x-amz-tagging': expect.anything() | ||
| }) | ||
| }) | ||
| ) | ||
| }) | ||
|
Comment on lines
+116
to
+139
|
||
| }) | ||
|
|
||
| describe('Multiple parts file', () => { | ||
|
|
@@ -113,7 +163,7 @@ describe('uploadFile', () => { | |
| jest.spyOn(axios, 'delete').mockResolvedValue(undefined) | ||
| jest.spyOn(axios, 'put').mockRejectedValue('error') | ||
|
|
||
| const sut = new DirectUploadClient(filesRepositoryStub, 1) | ||
| const sut = new DirectUploadClient(filesRepositoryStub, { maxMultipartRetries: 1 }) | ||
|
|
||
| const progressMock = jest.fn() | ||
| const abortController = new AbortController() | ||
|
|
@@ -143,7 +193,7 @@ describe('uploadFile', () => { | |
| const progressMock = jest.fn() | ||
| const abortController = new AbortController() | ||
|
|
||
| const sut = new DirectUploadClient(filesRepositoryStub, 1) | ||
| const sut = new DirectUploadClient(filesRepositoryStub, { maxMultipartRetries: 1 }) | ||
|
|
||
| await expect(sut.uploadFile(1, testFile, progressMock, abortController)).rejects.toThrow( | ||
| MultipartAbortError | ||
|
|
@@ -165,7 +215,7 @@ describe('uploadFile', () => { | |
| const progressMock = jest.fn() | ||
| const abortController = new AbortController() | ||
|
|
||
| const sut = new DirectUploadClient(filesRepositoryStub, 1) | ||
| const sut = new DirectUploadClient(filesRepositoryStub, { maxMultipartRetries: 1 }) | ||
| await expect(sut.uploadFile(1, testFile, progressMock, abortController)).rejects.toThrow( | ||
| MultipartCompletionError | ||
| ) | ||
|
|
@@ -187,7 +237,7 @@ describe('uploadFile', () => { | |
| .mockResolvedValueOnce(successfulPartResponse) | ||
| .mockResolvedValueOnce(undefined) | ||
|
|
||
| const sut = new DirectUploadClient(filesRepositoryStub, 1) | ||
| const sut = new DirectUploadClient(filesRepositoryStub, { maxMultipartRetries: 1 }) | ||
|
|
||
| const progressMock = jest.fn() | ||
| const abortController = new AbortController() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import { FilesConfig } from '../../../src/files' | ||
|
|
||
| describe('FilesConfig', () => { | ||
| beforeEach(() => { | ||
| // Reset config before each test | ||
| FilesConfig.init({}) | ||
| }) | ||
|
Comment on lines
+4
to
+7
|
||
|
|
||
| describe('init', () => { | ||
| test('should set useS3Tagging configuration', () => { | ||
| FilesConfig.init({ useS3Tagging: false }) | ||
|
|
||
| const config = FilesConfig.getConfig() | ||
| expect(config.useS3Tagging).toBe(false) | ||
| }) | ||
|
|
||
| test('should set maxMultipartRetries configuration', () => { | ||
| FilesConfig.init({ maxMultipartRetries: 10 }) | ||
|
|
||
| const config = FilesConfig.getConfig() | ||
| expect(config.maxMultipartRetries).toBe(10) | ||
| }) | ||
|
|
||
| test('should set fileUploadTimeoutMs configuration', () => { | ||
| FilesConfig.init({ fileUploadTimeoutMs: 120000 }) | ||
|
|
||
| const config = FilesConfig.getConfig() | ||
| expect(config.fileUploadTimeoutMs).toBe(120000) | ||
| }) | ||
|
|
||
| test('should set multiple configuration options', () => { | ||
| FilesConfig.init({ | ||
| useS3Tagging: false, | ||
| maxMultipartRetries: 3, | ||
| fileUploadTimeoutMs: 30000 | ||
| }) | ||
|
|
||
| const config = FilesConfig.getConfig() | ||
| expect(config.useS3Tagging).toBe(false) | ||
| expect(config.maxMultipartRetries).toBe(3) | ||
| expect(config.fileUploadTimeoutMs).toBe(30000) | ||
| }) | ||
| }) | ||
|
|
||
| describe('getConfig', () => { | ||
| test('should return empty config by default', () => { | ||
| const config = FilesConfig.getConfig() | ||
| expect(config).toEqual({}) | ||
| }) | ||
|
|
||
| test('should return previously set config', () => { | ||
| const expectedConfig = { useS3Tagging: true, maxMultipartRetries: 5 } | ||
| FilesConfig.init(expectedConfig) | ||
|
|
||
| const config = FilesConfig.getConfig() | ||
| expect(config).toEqual(expectedConfig) | ||
| }) | ||
| }) | ||
| }) | ||
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.
Typo: "Summeries" should be "Summaries"