From 6805f34b2ff8da37e7059a46386f48c4d707ff27 Mon Sep 17 00:00:00 2001 From: Trae Yelovich Date: Tue, 24 Oct 2023 13:12:50 -0400 Subject: [PATCH 1/5] fix(ftp): Use fileSync instead of tmpNameSync when checking e-tag Signed-off-by: Trae Yelovich --- .../zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts | 5 +++-- .../zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts index 4ad04c06bb..db1be5ff5e 100644 --- a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts +++ b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts @@ -360,13 +360,14 @@ export class FtpMvsApi extends AbstractFtpApi implements ZoweExplorerApi.IMvs { } private async getContentsTag(dataSetName: string): Promise { - const tmpFileName = tmp.tmpNameSync(); + const tmpFile = tmp.fileSync({ discardDescriptor: true }); const options: zowe.IDownloadOptions = { binary: false, - file: tmpFileName, + file: tmpFile.name, }; const loadResult = await this.getContents(dataSetName, options); const etag: string = loadResult.apiResponse.etag; + fs.rmSync(tmpFile.name); return etag; } private getDefaultResponse(): zowe.IZosFilesResponse { diff --git a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts index 17642df3e5..128596ee62 100644 --- a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts +++ b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts @@ -267,13 +267,14 @@ export class FtpUssApi extends AbstractFtpApi implements ZoweExplorerApi.IUss { } private async getContentsTag(ussFilePath: string): Promise { - const tmpFileName = tmp.tmpNameSync(); + const tmpFile = tmp.fileSync({ discardDescriptor: true }); const options: zowe.IDownloadOptions = { binary: false, - file: tmpFileName, + file: tmpFile.name, }; const loadResult = await this.getContents(ussFilePath, options); const etag: string = loadResult.apiResponse.etag; + fs.rmSync(tmpFile.name); return etag; } From 610a81255f2f84b4ba38229e27d2b44a3b64192f Mon Sep 17 00:00:00 2001 From: Trae Yelovich Date: Tue, 24 Oct 2023 13:27:22 -0400 Subject: [PATCH 2/5] test(ftp): Update test cases to verify tmp.fileSync call Signed-off-by: Trae Yelovich --- .../__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts | 5 ++++- .../__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts index 1836670676..46be2da10a 100644 --- a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts +++ b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts @@ -100,6 +100,7 @@ describe("FtpMvsApi", () => { it("should upload content to dataset.", async () => { const localFile = tmp.tmpNameSync({ tmpdir: "/tmp" }); + const fileSyncSpy = jest.spyOn(tmp, "fileSync"); fs.writeFileSync(localFile, "hello"); const response = TestUtils.getSingleLineStream(); @@ -113,7 +114,7 @@ describe("FtpMvsApi", () => { dataSetName: " (IBMUSER).DS2", options: { encoding: "", returnEtag: true, etag: "utf8" }, }; - jest.spyOn(MvsApi as any, "getContentsTag").mockReturnValue(undefined); + jest.spyOn(MvsApi as any, "getContents").mockResolvedValueOnce({ apiResponse: { etag: "utf8" } }); jest.spyOn(fs, "readFileSync").mockReturnValue("test"); jest.spyOn(Gui, "warningMessage").mockImplementation(); const result = await MvsApi.putContents(mockParams.inputFilePath, mockParams.dataSetName, mockParams.options); @@ -121,6 +122,8 @@ describe("FtpMvsApi", () => { expect(DataSetUtils.listDataSets).toBeCalledTimes(1); expect(DataSetUtils.uploadDataSet).toBeCalledTimes(1); expect(MvsApi.releaseConnection).toBeCalled(); + // check that correct function is called from node-tmp + expect(fileSyncSpy).toHaveBeenCalledWith({ discardDescriptor: true }); }); it("should upload single space to dataset when secureFtp is true and contents are empty", async () => { diff --git a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts index fa8d863d88..de1bc5b727 100644 --- a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts +++ b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts @@ -96,6 +96,7 @@ describe("FtpUssApi", () => { const localFile = tmp.tmpNameSync({ tmpdir: "/tmp" }); const response = TestUtils.getSingleLineStream(); UssUtils.uploadFile = jest.fn().mockReturnValue(response); + const fileSyncSpy = jest.spyOn(tmp, "fileSync"); jest.spyOn(UssApi, "getContents").mockResolvedValue({ apiResponse: { etag: "test" } } as any); const mockParams = { inputFilePath: localFile, @@ -107,11 +108,13 @@ describe("FtpUssApi", () => { }, }; const result = await UssApi.putContents(mockParams.inputFilePath, mockParams.ussFilePath, undefined, undefined, "test", true); - jest.spyOn(UssApi as any, "getContentsTag").mockReturnValue("test"); + jest.spyOn(UssApi as any, "getContents").mockResolvedValueOnce({ apiResponse: { etag: "test" } }); expect(result.commandResponse).toContain("File uploaded successfully."); expect(UssUtils.downloadFile).toBeCalledTimes(1); expect(UssUtils.uploadFile).toBeCalledTimes(1); expect(UssApi.releaseConnection).toBeCalled(); + // check that correct function is called from node-tmp + expect(fileSyncSpy).toHaveBeenCalledWith({ discardDescriptor: true }); }); it("should call putContents when calling putContent", async () => { From 6c1c9b6c7e72aee307c94ee4e83955c7b979b914 Mon Sep 17 00:00:00 2001 From: Trae Yelovich Date: Tue, 24 Oct 2023 14:13:53 -0400 Subject: [PATCH 3/5] fix(ftp): adjust getContentsTag calls so only one FTP connection is active Signed-off-by: Trae Yelovich --- .../src/ZoweExplorerFtpMvsApi.ts | 23 +++++++++++-------- .../src/ZoweExplorerFtpUssApi.ts | 18 +++++++++------ 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts index db1be5ff5e..975266ed41 100644 --- a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts +++ b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts @@ -131,6 +131,17 @@ export class FtpMvsApi extends AbstractFtpApi implements ZoweExplorerApi.IMvs { } const result = this.getDefaultResponse(); const profile = this.checkedProfile(); + + // Save-Save with FTP requires loading the file first + // (moved this block above connection request so only one connection is active at a time) + if (options.returnEtag && options.etag) { + const contentsTag = await this.getContentsTag(dataSetName); + if (contentsTag && contentsTag !== options.etag) { + result.success = false; + result.commandResponse = "Rest API failure with HTTP(S) status 412 Save conflict."; + return result; + } + } let connection; try { connection = await this.ftpClient(profile); @@ -138,15 +149,6 @@ export class FtpMvsApi extends AbstractFtpApi implements ZoweExplorerApi.IMvs { ZoweLogger.logImperativeMessage(result.commandResponse, MessageSeverity.ERROR); throw new Error(result.commandResponse); } - // Save-Save with FTP requires loading the file first - if (options.returnEtag && options.etag) { - const contentsTag = await this.getContentsTag(dataSetName); - if (contentsTag && contentsTag !== options.etag) { - result.success = false; - result.commandResponse = "Rest API failure with HTTP(S) status 412 Save conflict."; - return result; - } - } const lrecl: number = dsAtrribute.apiResponse.items[0].lrecl; const data = fs.readFileSync(inputFilePath, { encoding: "utf8" }); const transferOptions: Record = { @@ -178,6 +180,9 @@ export class FtpMvsApi extends AbstractFtpApi implements ZoweExplorerApi.IMvs { await DataSetUtils.uploadDataSet(connection, targetDataset, transferOptions); result.success = true; if (options.returnEtag) { + // release this connection instance because a new one will be made with getContentsTag + this.releaseConnection(connection); + connection = null; const contentsTag = await this.getContentsTag(dataSetName); result.apiResponse = [ { diff --git a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts index 128596ee62..baf4509c0c 100644 --- a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts +++ b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts @@ -126,23 +126,27 @@ export class FtpUssApi extends AbstractFtpApi implements ZoweExplorerApi.IUss { localFile: inputFilePath, }; const result = this.getDefaultResponse(); + // Save-Save with FTP requires loading the file first + // (moved this block above connection request so only one connection is active at a time) + if (returnEtag && etag) { + const contentsTag = await this.getContentsTag(ussFilePath); + if (contentsTag && contentsTag !== etag) { + throw new Error("Rest API failure with HTTP(S) status 412 Save conflict."); + } + } let connection; try { connection = await this.ftpClient(this.checkedProfile()); if (!connection) { throw new Error(result.commandResponse); } - // Save-Save with FTP requires loading the file first - if (returnEtag && etag) { - const contentsTag = await this.getContentsTag(ussFilePath); - if (contentsTag && contentsTag !== etag) { - throw new Error("Rest API failure with HTTP(S) status 412 Save conflict."); - } - } await UssUtils.uploadFile(connection, ussFilePath, transferOptions); result.success = true; if (returnEtag) { + // release this connection instance because a new one will be made with getContentsTag + this.releaseConnection(connection); + connection = null; const contentsTag = await this.getContentsTag(ussFilePath); result.apiResponse.etag = contentsTag; } From 4e55a999b4e49cf9a2f31b910f775297e609497a Mon Sep 17 00:00:00 2001 From: Trae Yelovich Date: Tue, 24 Oct 2023 14:30:16 -0400 Subject: [PATCH 4/5] chore: update FTP changelog Signed-off-by: Trae Yelovich --- packages/zowe-explorer-ftp-extension/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/zowe-explorer-ftp-extension/CHANGELOG.md b/packages/zowe-explorer-ftp-extension/CHANGELOG.md index 1d58ca0e0c..83ba9a3a0e 100644 --- a/packages/zowe-explorer-ftp-extension/CHANGELOG.md +++ b/packages/zowe-explorer-ftp-extension/CHANGELOG.md @@ -7,6 +7,8 @@ All notable changes to the "zowe-explorer-ftp-extension" extension will be docum ### Bug fixes - Fixed ECONNRESET error when trying to upload or create an empty data set member. [#2350](https://github.com/zowe/vscode-extension-for-zowe/issues/2350) +- Fixed issue where temporary files for e-tag comparison were not deleted after use. +- Fixed issue where another connection attempt was made inside `putContents` (in `getContentsTag`) even though a connection was already active. ## `2.11.2` From d493cf4c72f40214ea0a882bc5bf6c5f5e6d0a10 Mon Sep 17 00:00:00 2001 From: Trae Yelovich Date: Tue, 24 Oct 2023 17:38:21 -0400 Subject: [PATCH 5/5] fix(ftp,tests): Revert back to tmpNameSync, add assertions in tests - `UssUtils.downloadFile` ultimately calls `fs.createWriteStream`, so we don't need to make the file first. - Added checks in tests for `tmpNameSync` and `rmSync` to verify that temp files are removed Signed-off-by: Trae Yelovich --- .../__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts | 6 ++++-- .../__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts | 6 ++++-- .../src/ZoweExplorerFtpMvsApi.ts | 6 +++--- .../src/ZoweExplorerFtpUssApi.ts | 6 +++--- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts index 46be2da10a..2d4ca5f5ff 100644 --- a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts +++ b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Mvs/ZoweExplorerFtpMvsApi.unit.test.ts @@ -100,7 +100,8 @@ describe("FtpMvsApi", () => { it("should upload content to dataset.", async () => { const localFile = tmp.tmpNameSync({ tmpdir: "/tmp" }); - const fileSyncSpy = jest.spyOn(tmp, "fileSync"); + const tmpNameSyncSpy = jest.spyOn(tmp, "tmpNameSync"); + const rmSyncSpy = jest.spyOn(fs, "rmSync"); fs.writeFileSync(localFile, "hello"); const response = TestUtils.getSingleLineStream(); @@ -123,7 +124,8 @@ describe("FtpMvsApi", () => { expect(DataSetUtils.uploadDataSet).toBeCalledTimes(1); expect(MvsApi.releaseConnection).toBeCalled(); // check that correct function is called from node-tmp - expect(fileSyncSpy).toHaveBeenCalledWith({ discardDescriptor: true }); + expect(tmpNameSyncSpy).toHaveBeenCalled(); + expect(rmSyncSpy).toHaveBeenCalled(); }); it("should upload single space to dataset when secureFtp is true and contents are empty", async () => { diff --git a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts index de1bc5b727..18ecf95c59 100644 --- a/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts +++ b/packages/zowe-explorer-ftp-extension/__tests__/__unit__/Uss/ZoweExplorerFtpUssApi.unit.test.ts @@ -96,7 +96,8 @@ describe("FtpUssApi", () => { const localFile = tmp.tmpNameSync({ tmpdir: "/tmp" }); const response = TestUtils.getSingleLineStream(); UssUtils.uploadFile = jest.fn().mockReturnValue(response); - const fileSyncSpy = jest.spyOn(tmp, "fileSync"); + const tmpNameSyncSpy = jest.spyOn(tmp, "tmpNameSync"); + const rmSyncSpy = jest.spyOn(fs, "rmSync"); jest.spyOn(UssApi, "getContents").mockResolvedValue({ apiResponse: { etag: "test" } } as any); const mockParams = { inputFilePath: localFile, @@ -114,7 +115,8 @@ describe("FtpUssApi", () => { expect(UssUtils.uploadFile).toBeCalledTimes(1); expect(UssApi.releaseConnection).toBeCalled(); // check that correct function is called from node-tmp - expect(fileSyncSpy).toHaveBeenCalledWith({ discardDescriptor: true }); + expect(tmpNameSyncSpy).toHaveBeenCalled(); + expect(rmSyncSpy).toHaveBeenCalled(); }); it("should call putContents when calling putContent", async () => { diff --git a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts index 975266ed41..c893ec5eae 100644 --- a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts +++ b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpMvsApi.ts @@ -365,14 +365,14 @@ export class FtpMvsApi extends AbstractFtpApi implements ZoweExplorerApi.IMvs { } private async getContentsTag(dataSetName: string): Promise { - const tmpFile = tmp.fileSync({ discardDescriptor: true }); + const tmpFileName = tmp.tmpNameSync(); const options: zowe.IDownloadOptions = { binary: false, - file: tmpFile.name, + file: tmpFileName, }; const loadResult = await this.getContents(dataSetName, options); const etag: string = loadResult.apiResponse.etag; - fs.rmSync(tmpFile.name); + fs.rmSync(tmpFileName, { force: true }); return etag; } private getDefaultResponse(): zowe.IZosFilesResponse { diff --git a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts index baf4509c0c..db048ecd1c 100644 --- a/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts +++ b/packages/zowe-explorer-ftp-extension/src/ZoweExplorerFtpUssApi.ts @@ -271,14 +271,14 @@ export class FtpUssApi extends AbstractFtpApi implements ZoweExplorerApi.IUss { } private async getContentsTag(ussFilePath: string): Promise { - const tmpFile = tmp.fileSync({ discardDescriptor: true }); + const tmpFileName = tmp.tmpNameSync(); const options: zowe.IDownloadOptions = { binary: false, - file: tmpFile.name, + file: tmpFileName, }; const loadResult = await this.getContents(ussFilePath, options); const etag: string = loadResult.apiResponse.etag; - fs.rmSync(tmpFile.name); + fs.rmSync(tmpFileName, { force: true }); return etag; }