From: Stephen Smith Date: Fri, 17 Mar 2023 00:11:27 +0000 (-0400) Subject: 20029: Refactor replaceFiles operations X-Git-Tag: 2.6.0~10^2~4 X-Git-Url: https://git.arvados.org/arvados-workbench2.git/commitdiff_plain/157416e6afb2c0e1a206de4707a3984796e08bbd 20029: Refactor replaceFiles operations * Perform single collection operations in a single request * Rename operations to replace existing operations * Add renameFile operation Arvados-DCO-1.1-Signed-off-by: Stephen Smith --- diff --git a/src/common/webdav.ts b/src/common/webdav.ts index c95d8747..bb8a68bd 100644 --- a/src/common/webdav.ts +++ b/src/common/webdav.ts @@ -88,15 +88,6 @@ export class WebDAV { method: 'DELETE' }) - mkdir = (url: string, config: WebDAVRequestConfig = {}) => - this.request({ - ...config, url, - method: 'MKCOL', - headers: { - ...config.headers, - } - }) - private request = (config: RequestConfig) => { return new Promise((resolve, reject) => { const r = this.createRequest(); diff --git a/src/services/collection-service/collection-service.test.ts b/src/services/collection-service/collection-service.test.ts index 4649437a..0547a8f2 100644 --- a/src/services/collection-service/collection-service.test.ts +++ b/src/services/collection-service/collection-service.test.ts @@ -7,7 +7,7 @@ import MockAdapter from 'axios-mock-adapter'; import { snakeCase } from 'lodash'; import { CollectionResource, defaultCollectionSelectedFields } from 'models/collection'; import { AuthService } from '../auth-service/auth-service'; -import { CollectionService } from './collection-service'; +import { CollectionService, emptyCollectionUuid } from './collection-service'; describe('collection-service', () => { let collectionService: CollectionService; @@ -129,33 +129,53 @@ describe('collection-service', () => { describe('deleteFiles', () => { it('should remove no files', async () => { // given + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); const filePaths: string[] = []; - const collectionUUID = ''; + const collectionUUID = 'zzzzz-tpzed-5o5tg0l9a57gxxx'; // when await collectionService.deleteFiles(collectionUUID, filePaths); // then - expect(webdavClient.delete).not.toHaveBeenCalled(); + expect(serverApi.put).toHaveBeenCalledTimes(1); + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${collectionUUID}`, { + collection: { + preserve_version: true + }, + replace_files: {}, + } + ); }); it('should remove only root files', async () => { // given + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); const filePaths: string[] = ['/root/1', '/root/1/100', '/root/1/100/test.txt', '/root/2', '/root/2/200', '/root/3/300/test.txt']; - const collectionUUID = ''; + const collectionUUID = 'zzzzz-tpzed-5o5tg0l9a57gxxx'; // when await collectionService.deleteFiles(collectionUUID, filePaths); // then - expect(webdavClient.delete).toHaveBeenCalledTimes(3); - expect(webdavClient.delete).toHaveBeenCalledWith("c=/root/3/300/test.txt"); - expect(webdavClient.delete).toHaveBeenCalledWith("c=/root/2"); - expect(webdavClient.delete).toHaveBeenCalledWith("c=/root/1"); + expect(serverApi.put).toHaveBeenCalledTimes(1); + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${collectionUUID}`, { + collection: { + preserve_version: true + }, + replace_files: { + '/root/3/300/test.txt': '', + '/root/2': '', + '/root/1': '', + }, + } + ); }); it('should remove files with uuid prefix', async () => { // given + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); const filePaths: string[] = ['/root/1']; const collectionUUID = 'zzzzz-tpzed-5o5tg0l9a57gxxx'; @@ -163,12 +183,19 @@ describe('collection-service', () => { await collectionService.deleteFiles(collectionUUID, filePaths); // then - expect(webdavClient.delete).toHaveBeenCalledTimes(1); - expect(webdavClient.delete).toHaveBeenCalledWith("c=zzzzz-tpzed-5o5tg0l9a57gxxx/root/1"); + expect(serverApi.put).toHaveBeenCalledTimes(1); + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${collectionUUID}`, { + collection: { + preserve_version: true + }, + replace_files: { + '/root/1': '', + }, + } + ); }); - }); - describe('batch file operations', () => { it('should batch remove files', async () => { serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); // given @@ -176,7 +203,7 @@ describe('collection-service', () => { const collectionUUID = 'zzzzz-4zz18-5o5tg0l9a57gxxx'; // when - await collectionService.batchFileDelete(collectionUUID, filePaths); + await collectionService.deleteFiles(collectionUUID, filePaths); // then expect(serverApi.put).toHaveBeenCalledTimes(1); @@ -193,19 +220,43 @@ describe('collection-service', () => { } ); }); + }); + + describe('renameFile', () => { + it('should rename file', async () => { + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); + const collectionUuid = 'zzzzz-4zz18-ywq0rvhwwhkjnfq'; + const collectionPdh = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; + const oldPath = '/old/path'; + const newPath = '/new/filename'; + await collectionService.renameFile(collectionUuid, collectionPdh, oldPath, newPath); + + expect(serverApi.put).toHaveBeenCalledTimes(1); + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${collectionUuid}`, { + collection: { + preserve_version: true + }, + replace_files: { + [newPath]: `${collectionPdh}${oldPath}`, + }, + } + ); + }); + }); + + describe('copyFiles', () => { it('should batch copy files', async () => { serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); - // given const filePaths: string[] = ['/root/1', '/secondFile', 'barefile.txt']; - // const collectionUUID = 'zzzzz-4zz18-5o5tg0l9a57gxxx'; - const collectionPDH = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; + const sourcePdh = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; const destinationUuid = 'zzzzz-4zz18-ywq0rvhwwhkjnfq'; const destinationPath = '/destinationPath'; // when - await collectionService.batchFileCopy(collectionPDH, filePaths, destinationUuid, destinationPath); + await collectionService.copyFiles(sourcePdh, filePaths, destinationUuid, destinationPath); // then expect(serverApi.put).toHaveBeenCalledTimes(1); @@ -215,26 +266,76 @@ describe('collection-service', () => { preserve_version: true }, replace_files: { - [`${destinationPath}/1`]: `${collectionPDH}/root/1`, - [`${destinationPath}/secondFile`]: `${collectionPDH}/secondFile`, - [`${destinationPath}/barefile.txt`]: `${collectionPDH}/barefile.txt`, + [`${destinationPath}/1`]: `${sourcePdh}/root/1`, + [`${destinationPath}/secondFile`]: `${sourcePdh}/secondFile`, + [`${destinationPath}/barefile.txt`]: `${sourcePdh}/barefile.txt`, + }, + } + ); + }); + + it('should copy files from rooth', async () => { + // Test copying from root paths + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); + const filePaths: string[] = ['/']; + const sourcePdh = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; + + const destinationUuid = 'zzzzz-4zz18-ywq0rvhwwhkjnfq'; + const destinationPath = '/destinationPath'; + + await collectionService.copyFiles(sourcePdh, filePaths, destinationUuid, destinationPath); + + expect(serverApi.put).toHaveBeenCalledTimes(1); + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${destinationUuid}`, { + collection: { + preserve_version: true + }, + replace_files: { + [`${destinationPath}`]: `${sourcePdh}/`, }, } ); }); + it('should copy files to root path', async () => { + // Test copying to root paths + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); + const filePaths: string[] = ['/']; + const sourcePdh = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; + + const destinationUuid = 'zzzzz-4zz18-ywq0rvhwwhkjnfq'; + const destinationPath = '/'; + + await collectionService.copyFiles(sourcePdh, filePaths, destinationUuid, destinationPath); + + expect(serverApi.put).toHaveBeenCalledTimes(1); + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${destinationUuid}`, { + collection: { + preserve_version: true + }, + replace_files: { + "/": `${sourcePdh}/`, + }, + } + ); + }); + }); + + describe('moveFiles', () => { it('should batch move files', async () => { serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); // given const filePaths: string[] = ['/rootFile', '/secondFile', '/subpath/subfile', 'barefile.txt']; const srcCollectionUUID = 'zzzzz-4zz18-5o5tg0l9a57gxxx'; - const srcCollectionPDH = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; + const srcCollectionPdh = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; const destinationUuid = 'zzzzz-4zz18-ywq0rvhwwhkjnfq'; const destinationPath = '/destinationPath'; // when - await collectionService.batchFileMove(srcCollectionUUID, srcCollectionPDH, filePaths, destinationUuid, destinationPath); + await collectionService.moveFiles(srcCollectionUUID, srcCollectionPdh, filePaths, destinationUuid, destinationPath); // then expect(serverApi.put).toHaveBeenCalledTimes(2); @@ -245,10 +346,10 @@ describe('collection-service', () => { preserve_version: true }, replace_files: { - [`${destinationPath}/rootFile`]: `${srcCollectionPDH}/rootFile`, - [`${destinationPath}/secondFile`]: `${srcCollectionPDH}/secondFile`, - [`${destinationPath}/subfile`]: `${srcCollectionPDH}/subpath/subfile`, - [`${destinationPath}/barefile.txt`]: `${srcCollectionPDH}/barefile.txt`, + [`${destinationPath}/rootFile`]: `${srcCollectionPdh}/rootFile`, + [`${destinationPath}/secondFile`]: `${srcCollectionPdh}/secondFile`, + [`${destinationPath}/subfile`]: `${srcCollectionPdh}/subpath/subfile`, + [`${destinationPath}/barefile.txt`]: `${srcCollectionPdh}/barefile.txt`, }, } ); @@ -268,6 +369,40 @@ describe('collection-service', () => { ); }); + it('should batch move files within collection', async () => { + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); + // given + const filePaths: string[] = ['/one', '/two', '/subpath/subfile', 'barefile.txt']; + const srcCollectionUUID = 'zzzzz-4zz18-5o5tg0l9a57gxxx'; + const srcCollectionPdh = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; + + const destinationPath = '/destinationPath'; + + // when + await collectionService.moveFiles(srcCollectionUUID, srcCollectionPdh, filePaths, srcCollectionUUID, destinationPath); + + // then + expect(serverApi.put).toHaveBeenCalledTimes(1); + // Verify copy + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${srcCollectionUUID}`, { + collection: { + preserve_version: true + }, + replace_files: { + [`${destinationPath}/one`]: `${srcCollectionPdh}/one`, + ['/one']: '', + [`${destinationPath}/two`]: `${srcCollectionPdh}/two`, + ['/two']: '', + [`${destinationPath}/subfile`]: `${srcCollectionPdh}/subpath/subfile`, + ['/subpath/subfile']: '', + [`${destinationPath}/barefile.txt`]: `${srcCollectionPdh}/barefile.txt`, + ['/barefile.txt']: '', + }, + } + ); + }); + it('should abort batch move when copy fails', async () => { // Simulate failure to copy serverApi.put = jest.fn(() => Promise.reject({ @@ -279,14 +414,14 @@ describe('collection-service', () => { // given const filePaths: string[] = ['/rootFile', '/secondFile', '/subpath/subfile', 'barefile.txt']; const srcCollectionUUID = 'zzzzz-4zz18-5o5tg0l9a57gxxx'; - const srcCollectionPDH = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; + const srcCollectionPdh = '8cd9ce1dfa21c635b620b1bfee7aaa08+180'; const destinationUuid = 'zzzzz-4zz18-ywq0rvhwwhkjnfq'; const destinationPath = '/destinationPath'; // when try { - await collectionService.batchFileMove(srcCollectionUUID, srcCollectionPDH, filePaths, destinationUuid, destinationPath); + await collectionService.moveFiles(srcCollectionUUID, srcCollectionPdh, filePaths, destinationUuid, destinationPath); } catch {} // then @@ -298,10 +433,10 @@ describe('collection-service', () => { preserve_version: true }, replace_files: { - [`${destinationPath}/rootFile`]: `${srcCollectionPDH}/rootFile`, - [`${destinationPath}/secondFile`]: `${srcCollectionPDH}/secondFile`, - [`${destinationPath}/subfile`]: `${srcCollectionPDH}/subpath/subfile`, - [`${destinationPath}/barefile.txt`]: `${srcCollectionPDH}/barefile.txt`, + [`${destinationPath}/rootFile`]: `${srcCollectionPdh}/rootFile`, + [`${destinationPath}/secondFile`]: `${srcCollectionPdh}/secondFile`, + [`${destinationPath}/subfile`]: `${srcCollectionPdh}/subpath/subfile`, + [`${destinationPath}/barefile.txt`]: `${srcCollectionPdh}/barefile.txt`, }, } ); @@ -311,20 +446,31 @@ describe('collection-service', () => { describe('createDirectory', () => { it('creates empty directory', async () => { // given - const directoryNames = { - 'newDir': 'newDir', - '/fooDir': 'fooDir', - '/anotherPath/': 'anotherPath', - 'trailingSlash/': 'trailingSlash', - }; - const collectionUUID = 'zzzzz-tpzed-5o5tg0l9a57gxxx'; - - Object.keys(directoryNames).map(async (path) => { + const directoryNames = [ + {in: 'newDir', out: 'newDir'}, + {in: '/fooDir', out: 'fooDir'}, + {in: '/anotherPath/', out: 'anotherPath'}, + {in: 'trailingSlash/', out: 'trailingSlash'}, + ]; + const collectionUuid = 'zzzzz-tpzed-5o5tg0l9a57gxxx'; + + for (var i = 0; i < directoryNames.length; i++) { + serverApi.put = jest.fn(() => Promise.resolve({ data: {} })); // when - await collectionService.createDirectory(collectionUUID, path); + await collectionService.createDirectory(collectionUuid, directoryNames[i].in); // then - expect(webdavClient.mkdir).toHaveBeenCalledWith(`c=${collectionUUID}/${directoryNames[path]}`); - }); + expect(serverApi.put).toHaveBeenCalledTimes(1); + expect(serverApi.put).toHaveBeenCalledWith( + `/collections/${collectionUuid}`, { + collection: { + preserve_version: true + }, + replace_files: { + ["/" + directoryNames[i].out]: emptyCollectionUuid, + }, + } + ); + } }); }); diff --git a/src/services/collection-service/collection-service.ts b/src/services/collection-service/collection-service.ts index 77ad5d38..a95fb47b 100644 --- a/src/services/collection-service/collection-service.ts +++ b/src/services/collection-service/collection-service.ts @@ -10,12 +10,13 @@ import { AuthService } from "../auth-service/auth-service"; import { extractFilesData } from "./collection-service-files-response"; import { TrashableResourceService } from "services/common-service/trashable-resource-service"; import { ApiActions } from "services/api/api-actions"; -import { customEncodeURI } from "common/url"; import { Session } from "models/session"; import { CommonService } from "services/common-service/common-service"; export type UploadProgress = (fileId: number, loaded: number, total: number, currentTime: number) => void; +export const emptyCollectionUuid = 'd41d8cd98f00b204e9800998ecf8427e+0'; + export class CollectionService extends TrashableResourceService { constructor(serverApi: AxiosInstance, private webdavClient: WebDAV, private authService: AuthService, actions: ApiActions) { super(serverApi, "collections", actions, [ @@ -53,27 +54,34 @@ export class CollectionService extends TrashableResourceService a.length - b.length) - .reduce((acc, currentPath) => { - const parentPathFound = acc.find((parentPath) => currentPath.indexOf(`${parentPath}/`) > -1); - - if (!parentPathFound) { - return [...acc, currentPath]; - } - - return acc; - }, []); - - for (const path of sortedUniquePaths) { - if (path.indexOf(collectionUuid) === -1) { - await this.webdavClient.delete(`c=${collectionUuid}${path}`); + private combineFilePath(parts: string[]) { + return parts.reduce((path, part) => { + // Trim leading and trailing slashes + const trimmedPart = part.split('/').filter(Boolean).join('/'); + if (trimmedPart.length) { + const separator = path.endsWith('/') ? '' : '/'; + return `${path}${separator}${trimmedPart}`; } else { - await this.webdavClient.delete(`c=${path}`); + return path; } - } - await this.update(collectionUuid, { preserveVersion: true }); + }, "/"); + } + + private replaceFiles(collectionUuid: string, fileMap: {}, showErrors?: boolean) { + const payload = { + collection: { + preserve_version: true + }, + replace_files: fileMap + }; + + return CommonService.defaultResponse( + this.serverApi + .put(`/${this.resourceType}/${collectionUuid}`, payload), + this.actions, + true, // mapKeys + showErrors + ); } async uploadFiles(collectionUuid: string, files: File[], onProgress?: UploadProgress, targetLocation: string = '') { @@ -85,12 +93,11 @@ export class CollectionService extends TrashableResourceService { @@ -125,64 +132,65 @@ export class CollectionService extends TrashableResourceService { - const pathStart = filePath.startsWith('/') ? '' : '/'; - return { - ...obj, - [`${pathStart}${filePath}`]: '' + deleteFiles(collectionUuid: string, files: string[], showErrors?: boolean) { + const optimizedFiles = files + .sort((a, b) => a.length - b.length) + .reduce((acc, currentPath) => { + const parentPathFound = acc.find((parentPath) => currentPath.indexOf(`${parentPath}/`) > -1); + + if (!parentPathFound) { + return [...acc, currentPath]; } - }, {}) - }; - return CommonService.defaultResponse( - this.serverApi - .put(`/${this.resourceType}/${collectionUuid}`, payload), - this.actions, - true, // mapKeys - showErrors - ); + return acc; + }, []); + + const fileMap = optimizedFiles.reduce((obj, filePath) => { + return { + ...obj, + [this.combineFilePath([filePath])]: '' + } + }, {}) + + return this.replaceFiles(collectionUuid, fileMap, showErrors); } - batchFileCopy(sourcePdh: string, files: string[], destinationCollectionUuid: string, destinationCollectionPath: string, showErrors?: boolean) { - const pathStart = destinationCollectionPath.startsWith('/') ? '' : '/'; - const separator = destinationCollectionPath.endsWith('/') ? '' : '/'; - const destinationPath = `${pathStart}${destinationCollectionPath}${separator}`; - const payload = { - collection: { - preserve_version: true - }, - replace_files: files.reduce((obj, sourceFile) => { - const sourcePath = sourceFile.startsWith('/') ? sourceFile : `/${sourceFile}`; + copyFiles(sourcePdh: string, files: string[], destinationCollectionUuid: string, destinationPath: string, showErrors?: boolean) { + const fileMap = files.reduce((obj, sourceFile) => { + const sourceFileName = sourceFile.split('/').filter(Boolean).slice(-1).join(""); + return { + ...obj, + [this.combineFilePath([destinationPath, sourceFileName])]: `${sourcePdh}${this.combineFilePath([sourceFile])}` + }; + }, {}); + + return this.replaceFiles(destinationCollectionUuid, fileMap, showErrors); + } + + moveFiles(sourceUuid: string, sourcePdh: string, files: string[], destinationCollectionUuid: string, destinationPath: string, showErrors?: boolean) { + if (sourceUuid === destinationCollectionUuid) { + const fileMap = files.reduce((obj, sourceFile) => { + const sourceFileName = sourceFile.split('/').filter(Boolean).slice(-1).join(""); return { ...obj, - [`${destinationPath}${sourceFile.split('/').slice(-1)}`]: `${sourcePdh}${sourcePath}` + [this.combineFilePath([destinationPath, sourceFileName])]: `${sourcePdh}${this.combineFilePath([sourceFile])}`, + [this.combineFilePath([sourceFile])]: '', }; - }, {}) - }; - - return CommonService.defaultResponse( - this.serverApi - .put(`/${this.resourceType}/${destinationCollectionUuid}`, payload), - this.actions, - true, // mapKeys - showErrors - ); + }, {}); + + return this.replaceFiles(sourceUuid, fileMap, showErrors) + } else { + return this.copyFiles(sourcePdh, files, destinationCollectionUuid, destinationPath, showErrors) + .then(() => { + return this.deleteFiles(sourceUuid, files, showErrors); + }); + } } - batchFileMove(sourceUuid: string, sourcePdh: string, files: string[], destinationCollectionUuid: string, destinationPath: string, showErrors?: boolean) { - return this.batchFileCopy(sourcePdh, files, destinationCollectionUuid, destinationPath, showErrors) - .then(() => { - return this.batchFileDelete(sourceUuid, files, showErrors); - }); - } + createDirectory(collectionUuid: string, path: string, showErrors?: boolean) { + const fileMap = {[this.combineFilePath([path])]: emptyCollectionUuid}; - createDirectory(collectionUuid: string, path: string) { - return this.webdavClient.mkdir(`c=${collectionUuid}/${customEncodeURI(path)}`); + return this.replaceFiles(collectionUuid, fileMap, showErrors); } } diff --git a/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts b/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts index 8c5e5b5a..e35c1560 100644 --- a/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts +++ b/src/store/collection-panel/collection-panel-files/collection-panel-files-actions.ts @@ -129,7 +129,7 @@ export const renameFile = (newFullPath: string) => dispatch(startSubmit(RENAME_FILE_DIALOG)); const oldPath = getFileFullPath(file); const newPath = newFullPath; - services.collectionService.moveFile(currentCollection.uuid, oldPath, newPath).then(() => { + services.collectionService.renameFile(currentCollection.uuid, currentCollection.portableDataHash, oldPath, newPath).then(() => { dispatch(dialogActions.CLOSE_DIALOG({ id: RENAME_FILE_DIALOG })); dispatch(snackbarActions.OPEN_SNACKBAR({ message: 'File name changed.', hideDuration: 2000 })); }).catch(e => {