Don't delete existing file if download fails

packages/zosfiles/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 All notable changes to the Zowe z/OS files SDK package will be documented in this file.
 
+## Recent Changes
+
+- BugFix: Fixed an issue where if a file fails to download and one already exists in the destination, the old file is deleted. [#2617](https://github.com/zowe/zowe-cli/pull/2617)
+
 ## `8.24.0`
 
 - Enhancement: Added an optional `searchExactName` option to the `ISearchOptions` interface to search the contents of one data set or PDS. [#2529](https://github.com/zowe/zowe-cli/pull/2529)

packages/zosfiles/__tests__/__unit__/methods/download/Download.unit.test.ts

@@ -603,6 +603,68 @@ describe("z/OS Files - Download", () => {
             expect(ioCreateDirSpy).toHaveBeenCalledWith(destination);
             expect(ioWriteStreamSpy).toHaveBeenCalledTimes(1);
         });
+
+        it("should not delete existing file when download fails", async () => {
+            let response;
+            let caughtError;
+            const file = "existing/file.txt";
+            const ioExistsSpy = jest.spyOn(IO, "existsSync");
+            const ioDeleteSpy = jest.spyOn(IO, "deleteFile");
+
+            ioExistsSpy.mockReturnValue(true);
+            ioDeleteSpy.mockImplementation(() => null);
+
+            const dummyError = new Error("Connection lost");
+            zosmfGetFullSpy.mockImplementation(() => {
+                throw dummyError;
+            });
+
+            try {
+                response = await Download.dataSet(dummySession, dsname, { file });
+            } catch (e) {
+                caughtError = e;
+            }
+
+            expect(response).toBeUndefined();
+            expect(caughtError).toEqual(dummyError);
+
+            expect(ioExistsSpy).toHaveBeenCalledWith(file);
+            expect(ioDeleteSpy).not.toHaveBeenCalled();
+
+            ioExistsSpy.mockRestore();
+            ioDeleteSpy.mockRestore();
+        });
+
+        it("should delete new file when download fails", async () => {
+            let response;
+            let caughtError;
+            const file = "new/file.txt";
+            const ioExistsSpy = jest.spyOn(IO, "existsSync");
+            const ioDeleteSpy = jest.spyOn(IO, "deleteFile");
+
+            ioExistsSpy.mockReturnValue(false);
+            ioDeleteSpy.mockImplementation(() => null);
+
+            const dummyError = new Error("Connection lost");
+            zosmfGetFullSpy.mockImplementation(() => {
+                throw dummyError;
+            });
+
+            try {
+                response = await Download.dataSet(dummySession, dsname, { file });
+            } catch (e) {
+                caughtError = e;
+            }
+
+            expect(response).toBeUndefined();
+            expect(caughtError).toEqual(dummyError);
+
+            expect(ioExistsSpy).toHaveBeenCalledWith(file);
+            expect(ioDeleteSpy).toHaveBeenCalledWith(file);
+
+            ioExistsSpy.mockRestore();
+            ioDeleteSpy.mockRestore();
+        });
     });
 
     describe("allMembers", () => {
@@ -1222,6 +1284,76 @@ describe("z/OS Files - Download", () => {
                 }
             );
         });
+
+        it("should not delete existing member files when download fails", async () => {
+            let response;
+            let caughtError;
+            const ioExistsSpy = jest.spyOn(IO, "existsSync");
+            const ioDeleteSpy = jest.spyOn(IO, "deleteFile");
+
+            ioExistsSpy.mockImplementation((filePath) => {
+                const fileName = path.basename(filePath.toString());
+                return fileName === "m1.txt" || fileName === "m2.txt";
+            });
+            ioDeleteSpy.mockImplementation(() => null);
+
+            const dummyError = new Error("Connection lost");
+            downloadDatasetSpy.mockImplementation(async () => {
+                throw dummyError;
+            });
+
+            try {
+                response = await Download.allMembers(dummySession, dsname, { failFast: false });
+            } catch (e) {
+                caughtError = e;
+            }
+
+            expect(response).toBeUndefined();
+            expect(caughtError).toBeDefined();
+            expect(caughtError.message).toContain(ZosFilesMessages.memberDownloadFailed.message);
+
+            expect(ioExistsSpy).toHaveBeenCalledWith(path.posix.join(dsFolder, "m1.txt"));
+            expect(ioExistsSpy).toHaveBeenCalledWith(path.posix.join(dsFolder, "m2.txt"));
+
+            expect(ioDeleteSpy).not.toHaveBeenCalled();
+
+            ioExistsSpy.mockRestore();
+            ioDeleteSpy.mockRestore();
+        });
+
+        it("should delete new member files when download fails", async () => {
+            let response;
+            let caughtError;
+            const ioExistsSpy = jest.spyOn(IO, "existsSync");
+            const ioDeleteSpy = jest.spyOn(IO, "deleteFile");
+
+            ioExistsSpy.mockReturnValue(false);
+            ioDeleteSpy.mockImplementation(() => null);
+
+            const dummyError = new Error("Connection lost");
+            downloadDatasetSpy.mockImplementation(async () => {
+                throw dummyError;
+            });
+
+            try {
+                response = await Download.allMembers(dummySession, dsname, { failFast: false });
+            } catch (e) {
+                caughtError = e;
+            }
+
+            expect(response).toBeUndefined();
+            expect(caughtError).toBeDefined();
+            expect(caughtError.message).toContain(ZosFilesMessages.memberDownloadFailed.message);
+
+            expect(ioExistsSpy).toHaveBeenCalledWith(path.posix.join(dsFolder, "m1.txt"));
+            expect(ioExistsSpy).toHaveBeenCalledWith(path.posix.join(dsFolder, "m2.txt"));
+
+            expect(ioDeleteSpy).toHaveBeenCalledWith(path.posix.join(dsFolder, "m1.txt"));
+            expect(ioDeleteSpy).toHaveBeenCalledWith(path.posix.join(dsFolder, "m2.txt"));
+
+            ioExistsSpy.mockRestore();
+            ioDeleteSpy.mockRestore();
+        });
     });
 
     describe("allDataSets", () => {

packages/zosfiles/src/methods/download/Download.ts

@@ -82,6 +82,8 @@ export class Download {
         ImperativeExpect.toNotBeNullOrUndefined(dataSetName, ZosFilesMessages.missingDatasetName.message);
         ImperativeExpect.toNotBeEqual(dataSetName, "", ZosFilesMessages.missingDatasetName.message);
         let destination: string;
+        let existedBefore = false;
+        let downloadStarted = false;
 
         try {
             // Format the endpoint to send the request to
@@ -125,6 +127,9 @@ export class Download {
                     return generatedFilePath + IO.normalizeExtension(extension);
                 })();
 
+                // Track if a file at destination already exists before staring the download
+                existedBefore = IO.existsSync(destination);
+
                 IO.createDirsSyncFromFilePath(destination);
             }
 
@@ -149,6 +154,8 @@ export class Download {
                 requestOptions.dataToReturn = [CLIENT_PROPERTY.response];
             }
 
+            downloadStarted = true;
+
             const request: IRestClientResponse = await ZosmfRestClient.getExpectFullResponse(session, requestOptions);
 
             // By default, apiResponse is empty when downloading
@@ -168,8 +175,13 @@ export class Download {
         } catch (error) {
             Logger.getAppLogger().error(error);
 
-            if (destination != null) {
-                IO.deleteFile(destination);
+            // Only delete the file if it did not exist before and error occured during download
+            if (destination != null && downloadStarted && !existedBefore) {
+                try {
+                    IO.deleteFile(destination);
+                } catch (deleteError) {
+                    Logger.getAppLogger().warn(`Failed to clean up partially download file ${destination}: ${deleteError.message}`);
+                }
             }
 
             throw error;
@@ -271,18 +283,29 @@ export class Download {
                     extension = extension.replace(/^\.+/g, "");
                 }
 
+                const memberFilePath = posix.join(baseDir, fileName + IO.normalizeExtension(extension));
+                const memberExistedBefore = IO.existsSync(memberFilePath);
+
                 return this.dataSet(session, `${dataSetName}(${mem.member})`, {
                     volume: options.volume,
-                    file: posix.join(baseDir, fileName + IO.normalizeExtension(extension)),
+                    file: memberFilePath,
                     binary: options.binary,
                     record: options.record,
                     encoding: options.encoding,
                     responseTimeout: options.responseTimeout
                 }).catch((err) => {
                     downloadErrors.push(err);
                     failedMembers.push(fileName);
-                    // Delete the file that could not be downloaded
-                    IO.deleteFile(join(baseDir, fileName + IO.normalizeExtension(extension)));
+
+                    // Only delete the file if it didn't exist before the download attempt
+                    if (!memberExistedBefore) {
+                        try {
+                            IO.deleteFile(memberFilePath);
+                        } catch (deleteError) {
+                            Logger.getAppLogger().warn(`Failed to clean up partially download file ${memberFilePath}: ${deleteError.message}`);
+                        }
+                    }
+
                     // If we should fail fast, rethrow error
                     if (options.failFast || options.failFast === undefined) {
                         throw err;