Merge pull request #484 from smartdevicelink/bugfix/single-file-multiple-uploads

renonick87 · web-flow · commit a7173913a5da · 2021-09-29T16:45:14.000-04:00
Fix case where file can have multiple upload/delete tasks created for it
diff --git a/lib/js/src/manager/file/_DeleteFileOperation.js b/lib/js/src/manager/file/_DeleteFileOperation.js
@@ -40,12 +40,14 @@ class _DeleteFileOperation extends _Task {
      * @class
      * @param {_LifecycleManager} lifecycleManager - An instance of _LifecycleManager.
      * @param {String} fileName - Name of the file to be deleted.
+     * @param {String[]} remoteFileNames - A list of file names uploaded to the system
      * @param {Function} completionListener - Listener called when the operation has completed.
      */
-    constructor (lifecycleManager, fileName, completionListener) {
+    constructor (lifecycleManager, fileName, remoteFileNames = [], completionListener) {
         super('DeleteFilesOperation');
         this._lifecycleManager = lifecycleManager;
         this._fileName = fileName;
+        this._remoteFileNames = remoteFileNames;
         this._completionListener = completionListener;
     }
 
@@ -66,6 +68,13 @@ class _DeleteFileOperation extends _Task {
         if (this.getState() === _Task.CANCELED) {
             return;
         }
+
+        if (!this._remoteFileNames.includes(this._fileName)) {
+            const errorMessage = 'File to delete is no longer on the head unit, aborting the operation';
+            console.log(errorMessage);
+            this._completionListener(false, _FileManagerBase.SPACE_AVAILABLE_MAX_VALUE, this._remoteFileNames, errorMessage);
+            return this.onFinished();
+        }
         await this._deleteFile();
     }
 
diff --git a/lib/js/src/manager/file/_FileManagerBase.js b/lib/js/src/manager/file/_FileManagerBase.js
@@ -146,7 +146,7 @@ class _FileManagerBase extends _SubManagerBase {
             return;
         }
 
-        const operation = new _DeleteFileOperation(this._lifecycleManager, fileName, (success, bytesAvailable, fileNames, errorMessage) => {
+        const operation = new _DeleteFileOperation(this._lifecycleManager, fileName, this._remoteFiles, (success, bytesAvailable, fileNames, errorMessage) => {
             if (success) {
                 this._bytesAvailable = bytesAvailable;
                 this._remoteFiles = _ArrayTools.arrayRemove(this._remoteFiles, fileName);
@@ -234,21 +234,6 @@ class _FileManagerBase extends _SubManagerBase {
             return;
         }
 
-        const msgVersion = this._lifecycleManager.getSdlMsgVersion();
-        const rpcVersion = new Version(msgVersion.getMajorVersion(), msgVersion.getMinorVersion(), msgVersion.getPatchVersion());
-        if (!file.isPersistent() && !this.hasUploadedFile(file) && new Version(4, 0, 0).isNewerThan(rpcVersion) === 1) {
-            file.setOverwrite(true);
-        }
-
-        if (!file.getOverwrite() && this._remoteFiles.includes(file.getName())) {
-            const errorMessage = 'Cannot overwrite remote file. The remote file system already has a file of this name, and the file manager is set to not automatically overwrite files.';
-            console.log(`FileManagerBase: ${errorMessage}`);
-            if (typeof listener === 'function') {
-                listener(false, this._bytesAvailable, null, errorMessage);
-            }
-            return;
-        }
-
         this._sdlUploadFilePrivate(file, listener);
     }
 
@@ -258,20 +243,23 @@ class _FileManagerBase extends _SubManagerBase {
      * @param {Function} listener - listener called when the upload has finished
      */
     _sdlUploadFilePrivate (file, listener) {
-        const fileName = file.getName();
+        const fileClone = file.clone();
+        const fileName = fileClone.getName();
 
-        const fileWrapper = new _SdlFileWrapper(file, (success, bytesAvailable, fileNames, errorMessage) => {
+        const fileWrapper = new _SdlFileWrapper(fileClone, (success, bytesAvailable, fileNames, errorMessage) => {
             if (success) {
                 this._bytesAvailable = bytesAvailable;
                 this._remoteFiles.push(fileName);
-                this._uploadedEphemeralFileNames.push(fileName);
-            } else {
+                if (!fileClone.isPersistent()) {
+                    this._uploadedEphemeralFileNames.push(fileName);
+                }
+            } else if (errorMessage !== _UploadFileOperation.fileManagerCannotOverwriteError) {
                 this._incrementFailedUploadCountForFileName(fileName, this._failedFileUploadsCount);
 
-                const maxUploadCount = (file instanceof SdlArtwork) ? this._maxArtworkUploadAttempts : this._maxFileUploadAttempts;
-                if (this._canFileBeUploadedAgain(file, maxUploadCount, this._failedFileUploadsCount)) {
-                    console.log(`FileManagerBase: Attempting to resend file with name ${file.getName()} after a failed upload attempt`);
-                    this._sdlUploadFilePrivate(file, listener);
+                const maxUploadCount = (fileClone instanceof SdlArtwork) ? this._maxArtworkUploadAttempts : this._maxFileUploadAttempts;
+                if (this._canFileBeUploadedAgain(fileClone, maxUploadCount, this._failedFileUploadsCount)) {
+                    console.log(`FileManagerBase: Attempting to resend file with name ${fileClone.getName()} after a failed upload attempt`);
+                    this._sdlUploadFilePrivate(fileClone, listener);
                     return;
                 }
             }
@@ -422,5 +410,6 @@ class _FileManagerBase extends _SubManagerBase {
 }
 
 const SPACE_AVAILABLE_MAX_VALUE = _FileManagerBase.SPACE_AVAILABLE_MAX_VALUE = 2000000000;
+_FileManagerBase.fileManagerCannotOverwriteError = 'Cannot overwrite remote file. The remote file system already has a file of this name, and the file manager is set to not automatically overwrite files.';
 
 export { _FileManagerBase };
diff --git a/lib/js/src/manager/file/_UploadFileOperation.js b/lib/js/src/manager/file/_UploadFileOperation.js
@@ -4,6 +4,7 @@ import { PutFile } from '../../rpc/messages/PutFile';
 import { _Task } from '../_Task';
 import { _FileManagerBase } from './_FileManagerBase';
 import { _FileUtils } from '../../util/_FileUtils';
+import { Version } from '../../util/Version';
 
 class _UploadFileOperation extends _Task {
     /**
@@ -38,11 +39,27 @@ class _UploadFileOperation extends _Task {
             return;
         }
 
+        const file = this._fileWrapper.getFile();
+        const msgVersion = this._lifecycleManager.getSdlMsgVersion();
+        const rpcVersion = new Version(msgVersion.getMajorVersion(), msgVersion.getMinorVersion(), msgVersion.getPatchVersion());
+        if (!file.isPersistent() && !this._fileManager.hasUploadedFile(file) && new Version(4, 4, 0).isNewerThan(rpcVersion) === 1) {
+            file.setOverwrite(true);
+        }
+
+        if (!this._fileManager.fileNeedsUpload(file)) {
+            const errorMessage = _FileManagerBase.fileManagerCannotOverwriteError;
+            console.log(errorMessage);
+            if (typeof this._fileWrapper.getCompletionListener() === 'function') {
+                this._fileWrapper.getCompletionListener()(false, null, null, errorMessage);
+            }
+            return this.onFinished();
+        }
+
         let mtuSize = 0;
         if (this._lifecycleManager !== null) {
             mtuSize = this._lifecycleManager.getMtu(_ServiceType.RPC);
         }
-        await this._sendFile(this._fileWrapper.getFile(), mtuSize, this._fileWrapper.getCompletionListener());
+        await this._sendFile(file, mtuSize, this._fileWrapper.getCompletionListener());
     }
 
     /**
@@ -56,25 +73,16 @@ class _UploadFileOperation extends _Task {
         let bytesAvailable = _FileManagerBase.SPACE_AVAILABLE_MAX_VALUE;
         let highestCorrelationId = -1;
 
-        if (this.getState() === _Task.CANCELED) {
-            const errorMessage = 'The file upload transaction was canceled before it could be completed.';
-            completionListener(false, bytesAvailable, null, errorMessage);
-            this.onFinished();
-            return;
-        }
-
         if (file === null) {
             const errorMessage = 'The file manager was unable to send the file. This could be because the file does not exist at the specified file path or that passed data is invalid.';
             completionListener(false, bytesAvailable, null, errorMessage);
-            this.onFinished();
-            return;
+            return this.onFinished();
         }
 
         if (file.getName() === null || file.getName() === undefined) {
             const errorMessage = 'You must specify a file name in the SdlFile';
             completionListener(false, bytesAvailable, null, errorMessage);
-            this.onFinished();
-            return;
+            return this.onFinished();
         }
 
         let data;
@@ -84,16 +92,14 @@ class _UploadFileOperation extends _Task {
             if (data === null) {
                 const errorMessage = 'File at path was empty';
                 completionListener(false, bytesAvailable, null, errorMessage);
-                this.onFinished();
-                return;
+                return this.onFinished();
             }
         } else if (file.getFileData() !== null && file.getFileData() !== undefined) {
             data = file.getFileData();
         } else {
             const errorMessage = 'The SdlFile to upload does not specify its file data.';
             completionListener(false, bytesAvailable, null, errorMessage);
-            this.onFinished();
-            return;
+            return this.onFinished();
         }
         const fileSize = data.length;
 
diff --git a/lib/js/src/manager/file/filetypes/SdlArtwork.js b/lib/js/src/manager/file/filetypes/SdlArtwork.js
@@ -117,6 +117,19 @@ class SdlArtwork extends SdlFile {
         }
         return image;
     }
+
+    /**
+     * Creates a deep copy of the object
+     * @returns {ChoiceCell} - A deep copy of the object
+     */
+    clone () {
+        const clonedParams = Object.assign({}, this); // shallow copy
+
+        if (clonedParams._imageRPC !== null) {
+            clonedParams._imageRPC = Object.assign(new Image(), clonedParams._imageRPC);
+        }
+        return Object.assign(new SdlArtwork(), clonedParams);
+    }
 }
 
 export { SdlArtwork };
diff --git a/lib/js/src/manager/file/filetypes/SdlFile.js b/lib/js/src/manager/file/filetypes/SdlFile.js
@@ -215,6 +215,19 @@ class SdlFile {
         }
         return true;
     }
+
+    /**
+     * Creates a deep copy of the object
+     * @returns {ChoiceCell} - A deep copy of the object
+     */
+    clone () {
+        const clonedParams = Object.assign({}, this); // shallow copy
+
+        if (clonedParams._fileData !== null && clonedParams._fileData !== undefined) {
+            clonedParams._fileData = Uint8Array.from(clonedParams._fileData);
+        }
+        return Object.assign(new SdlFile(), clonedParams);
+    }
 }
 
 
diff --git a/tests/managers/file/FileManagerTests.js b/tests/managers/file/FileManagerTests.js
@@ -121,37 +121,6 @@ module.exports = function (appClient) {
             });
         }
 
-        let failEveryOtherRequest = true;
-        /**
-         * Handle multiple PutFile RPCs, succeeding in one and failing in the next.
-         * @returns {Promise} - A promise.
-         */
-        function onMultiplePutFileSuccess () {
-            const response = new SDL.rpc.messages.PutFileResponse({
-                functionName: SDL.rpc.enums.FunctionID.PutFiles,
-            })
-                .setSpaceAvailable(Test.GENERAL_INT);
-            if (failEveryOtherRequest) {
-                failEveryOtherRequest = !failEveryOtherRequest;
-                response.setSuccess(true);
-                // _handleRpc triggers the listener
-                sdlManager._lifecycleManager._handleRpc(response);
-
-                return new Promise((resolve, reject) => {
-                    resolve(response);
-                });
-            } else {
-                failEveryOtherRequest = !failEveryOtherRequest;
-                response.setSuccess(false);
-                // _handleRpc triggers the listener
-                sdlManager._lifecycleManager._handleRpc(response);
-
-                return new Promise((resolve, reject) => {
-                    resolve(response);
-                });
-            }
-        }
-
         /**
          * Handle DeleteFile success.
          * @returns {Promise} - A promise.
@@ -411,12 +380,11 @@ module.exports = function (appClient) {
 
             Validator.assertNull(await fileManager.deleteRemoteFilesWithNames([uploadedFileNames[0], uploadedFileNames[1]]));
             await new Promise (innerResolve => {
-                const deleteOperation = new SDL.manager.file._DeleteFileOperation(sdlManager._lifecycleManager, uploadedFileNames[2], (success, bytesAvailable, fileNames, errorMessage) => {
+                const deleteOperation = new SDL.manager.file._DeleteFileOperation(sdlManager._lifecycleManager, uploadedFileNames[2], [uploadedFileNames[2]], (success, bytesAvailable, fileNames, errorMessage) => {
                     Validator.assertTrue(success);
                     Validator.assertNull(errorMessage);
                     innerResolve();
                 });
-                // await deleteOperation.onExecute();
                 fileManager._addTask(deleteOperation);
             });
 
@@ -426,70 +394,6 @@ module.exports = function (appClient) {
             stub.restore();
         });
 
-        it('testMultipleFileUploadPartialFailure', async function () {
-            let firstFile = true;
-            const expectSuccessThenFail = function (response) {
-                if (firstFile) {
-                    Validator.assertTrue(response.getSuccess());
-                } else {
-                    Validator.assertTrue(!response.getSuccess());
-                }
-                firstFile = !firstFile;
-            };
-            const expectSuccess = function (response) {
-                Validator.assertTrue(response.getSuccess());
-            };
-
-            sdlManager.addRpcListener(SDL.rpc.enums.FunctionID.ListFiles, expectSuccess);
-            sdlManager.addRpcListener(SDL.rpc.enums.FunctionID.PutFile, expectSuccessThenFail);
-
-            const stub = sinon.stub(lifecycleManager, 'sendRpcResolve');
-            stub.withArgs(sinon.match.instanceOf(SDL.rpc.messages.ListFiles))
-                .callsFake(onListFilesSuccess);
-            stub.withArgs(sinon.match.instanceOf(SDL.rpc.messages.PutFile))
-                .callsFake(onMultiplePutFileSuccess);
-
-            await new Promise (resolve => {
-                const listOperation = new SDL.manager.file._ListFilesOperation(sdlManager._lifecycleManager, (success, bytesAvailable, fileNames, errorMessage) => {
-                    resolve();
-                });
-                listOperation.onExecute();
-            });
-
-            const baseFileName = 'file';
-            let fileNum = 0;
-            const filesToUpload = [];
-            let sdlFile = new SDL.manager.file.filetypes.SdlFile();
-            sdlFile.setName(baseFileName + fileNum++);
-            sdlFile.setFilePath('./test_icon_1.png');
-            filesToUpload.push(sdlFile);
-
-            sdlFile = new SDL.manager.file.filetypes.SdlFile();
-            sdlFile.setName(baseFileName + fileNum++);
-            sdlFile.setFileData(new Array(50));
-            sdlFile.setPersistent(true);
-            sdlFile.setType(SDL.rpc.enums.FileType.BINARY);
-            filesToUpload.push(sdlFile);
-            let uploadedFileNames = fileManager.getRemoteFileNames();
-
-            await fileManager.uploadFiles(filesToUpload[0]);
-            await new Promise (resolve => {
-                // this task will run after the previous
-                const uploadOperation = new SDL.manager.file._UploadFileOperation(sdlManager._lifecycleManager, sdlManager._fileManager, new SDL.manager.file._SdlFileWrapper(filesToUpload[1], (success, bytesAvailable, fileNames, errorMessage) => {
-                    resolve();
-                }));
-                sdlManager._fileManager._addTask(uploadOperation);
-            });
-            stub.restore();
-
-            uploadedFileNames = fileManager.getRemoteFileNames();
-            Validator.assertTrue(!uploadedFileNames.includes(filesToUpload[0].getName()));
-            Validator.assertTrue(uploadedFileNames.includes(filesToUpload[1].getName()));
-
-            sdlManager.removeRpcListener(SDL.rpc.enums.FunctionID.PutFile, expectSuccessThenFail);
-            sdlManager.removeRpcListener(SDL.rpc.enums.FunctionID.ListFiles, expectSuccess);
-        });
-
         it('testMultipleArtworkUploadSuccess', async function () {
             const expectSuccess = function (response) {
                 Validator.assertTrue(response.getSuccess());
diff --git a/tests/managers/file/SdlFileTests.js b/tests/managers/file/SdlFileTests.js
diff --git a/tests/managers/file/UploadFileOperationTests.js b/tests/managers/file/UploadFileOperationTests.js
diff --git a/tests/managers/index.js b/tests/managers/index.js

Original file line number	Diff line number	Diff line change
`@@ -117,6 +117,19 @@ class SdlArtwork extends SdlFile {`
`117`	`117`	`}`
`118`	`118`	`return image;`
`119`	`119`	`}`
	`120`	`+`
	`121`	`+ /**`
	`122`	`+ * Creates a deep copy of the object`
	`123`	`+ * @returns {ChoiceCell} - A deep copy of the object`
	`124`	`+ */`
	`125`	`+ clone () {`
	`126`	`+ const clonedParams = Object.assign({}, this); // shallow copy`
	`127`	`+`
	`128`	`+ if (clonedParams._imageRPC !== null) {`
	`129`	`+ clonedParams._imageRPC = Object.assign(new Image(), clonedParams._imageRPC);`
	`130`	`+ }`
	`131`	`+ return Object.assign(new SdlArtwork(), clonedParams);`
	`132`	`+ }`
`120`	`133`	`}`
`121`	`134`
`122`	`135`	`export { SdlArtwork };`
Original file line number	Diff line number	Diff line change
`@@ -215,6 +215,19 @@ class SdlFile {`
`215`	`215`	`}`
`216`	`216`	`return true;`
`217`	`217`	`}`
	`218`	`+`
	`219`	`+ /**`
	`220`	`+ * Creates a deep copy of the object`
	`221`	`+ * @returns {ChoiceCell} - A deep copy of the object`
	`222`	`+ */`
	`223`	`+ clone () {`
	`224`	`+ const clonedParams = Object.assign({}, this); // shallow copy`
	`225`	`+`
	`226`	`+ if (clonedParams._fileData !== null && clonedParams._fileData !== undefined) {`
	`227`	`+ clonedParams._fileData = Uint8Array.from(clonedParams._fileData);`
	`228`	`+ }`
	`229`	`+ return Object.assign(new SdlFile(), clonedParams);`
	`230`	`+ }`
`218`	`231`	`}`
`219`	`232`
`220`	`233`