Skip to content
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

fix uss etag #1813

Merged
merged 2 commits into from
May 18, 2022
Merged

fix uss etag #1813

merged 2 commits into from
May 18, 2022

Conversation

tiantn
Copy link
Contributor

@tiantn tiantn commented May 17, 2022

Signed-off-by: Tina tiantn@cn.ibm.com

Proposed changes

Release Notes

Milestone:

Changelog:

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • [ x] Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Signed-off-by: Tina <tiantn@cn.ibm.com>
@tiantn tiantn requested review from std4lqi and zFernand0 May 17, 2022 09:00
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A unit test around this will be great so that we don't break it in the future 😋

@JillieBeanSim
Copy link
Contributor

@zFernand0 are we going to wait for the unit test to merge this in? I only ask since it has the approvals.

@zFernand0
Copy link
Member

I'm fine to merge this as is.
We will definitely come back to this when we attempt to achieve the Gold CII badge (by End-of-Year) 😋

@JillieBeanSim JillieBeanSim merged commit 9ed98e7 into zowe:master May 18, 2022
@mmoore96
Copy link

mmoore96 commented May 19, 2022

Adding below from #1768 discussion:
@tiantn @std4lqi

Problems:

1. When clicking on a USS member after it is already opened, it does not perform a "Pull from mainframe" like it does on Datasets.

I logged variables and etags below for testing.

When clicking on a file in uss after it has all ready been opened gets you the following log:

Found 28 matching files
Listing USS files in the directory '/u/[User#]/javatest'
Found 28 matching files
Listing USS files in the directory '/u/[User#]/javatest'
Found 28 matching files

Right clicking on the file and selecting "Pull from mainframe" get you this following log:

ERROR message from extension Zowe Explorer FTP Extension: We are at getContents
Downloading USS file '/u/[User#]/javatest/EXPGM5.java' to local file 'c:\[Users]\[user#]\.vscode\extensions\zowe.vscode-extension-for-zowe-2.0.2-SNAPSHOT\resources\temp\_U_\zftp_GMM\u\[User#]\javatest\EXPGM5.java' in transfer mode 'ascii
Successfully downloaded 3413 bytes of content from /u/[User#]/javatest/EXPGM5.java
ERROR message from extension Zowe Explorer FTP Extension: We are at hashFile
ERROR message from extension Zowe Explorer FTP Extension: We downloaded the file. The etag from hashFile is: da39a3ee5e6b4b0d3255bfef95601890afd80709

When saving a file in USS to the mainframe you get the following log:

Message from extension Zowe Explorer FTP Extension: We are at putContents
Message from extension Zowe Explorer FTP Extension: Save with FTP: Loading the file first
Message from extension Zowe Explorer FTP Extension: We are at getContents
Downloading USS file '/u/[User#]/javatest/EXPGM5.java' to local file 'C:\[Users]\[user#]\AppData\Local\Temp\tmp-25836-jk0P1Wo4z4PA' in transfer mode 'ascii
Successfully downloaded 3413 bytes of content from /u/[User#]/javatest/EXPGM5.java
Message from extension Zowe Explorer FTP Extension: We are at hashFile
Message from extension Zowe Explorer FTP Extension: We downloaded the file. The etag from hashFile is: 64df5d9cda0fff998c3d469a48525e9472c7ed0e
Message from extension Zowe Explorer FTP Extension: We are at getContentsTag. etag is: 64df5d9cda0fff998c3d469a48525e9472c7ed0e
Message from extension Zowe Explorer FTP Extension: contentsTag: 64df5d9cda0fff998c3d469a48525e9472c7ed0e
Message from extension Zowe Explorer FTP Extension: etag: da39a3ee5e6b4b0d3255bfef95601890afd80709
Message from extension Zowe Explorer FTP Extension: Save conflict. Please pull the latest content from mainframe first.

Not sure if this is expected. In datasets if I click on the same member I have open, it will pull it from the mainframe again.

2. Issue with Etag:

As you can also see from the above log, there is a discrepancy between the etag that is generated from the getContents method and the putContents. I belive this is what you are referring to about LF vs CRLF. In USS there is no extra line added to the file, however with windows there is.

However, there is an issue with that reasoning. When the files is downloaded the either vscode directory or AppData directory it is not going to matter because in both cases there is a Carriage return at the end of the file. zFTP handles these conversions.
In addition, if this was the problem then we would get a save conflict error every time we tried to save.

The problem:

The issue is that the behavior of clicking on uss files (after it has been opened) runs the following:

Found 28 matching files
Listing USS files in the directory '/u/[User#]/javatest'
Found 28 matching files
Listing USS files in the directory '/u/[User#]/javatest'
Found 28 matching files

So in certain cases of switching from different files and saving them, there becomes a mismatch of etags.

I have also seen that you can save a file without any etag checking being done:

Attempting to upload from local file 'c:\[Users]\p079584\mainframeFiles\temp\_U_\zftp_GMM\u\[user#]\javatest\Test.java' to USS file '/u/[user#]/javatest/Test.java' in transfer mode 'ascii'
Adding carriage returns to string of length 7478
ERROR message from extension Zowe Explorer FTP Extension: We are at putContents
Attempting to upload from local file 'c:\[Users]\p079584\mainframeFiles\temp\_U_\zftp_GMM\u\[user#]\javatest\Test.java' to USS file '/u/[user#]/javatest/Test.java' in transfer mode 'ascii'
Adding carriage returns to string of length 7477
ERROR message from extension Zowe Explorer FTP Extension: We are at putContents
Attempting to upload from local file 'c:\[Users]\p079584\mainframeFiles\temp\_U_\zftp_GMM\u\[user#]\javatest\Test.java' to USS file '/u/[user#]/javatest/Test.java' in transfer mode 'ascii'
Adding carriage returns to string of length 7477
ERROR message from extension Zowe Explorer FTP Extension: We are at putContents
Attempting to upload from local file 'c:\[Users]\p079584\mainframeFiles\temp\_U_\zftp_GMM\u\[user#]\javatest\Test.java' to USS file '/u/[user#]/javatest/Test.java' in transfer mode 'ascii'
Adding carriage returns to string of length 7478 

I also sometimes get stuck user messages when navigating USS:
stuckZowe

The solution:

The clicking behavior should been the same between datasets and USS. I believe this would fix the etag mismatches.

Even the UI performs different with Datasets vs USS:

When a pull from mainframe is done on Datasets:
testzowedatasets

When a pull from mainframe is done on USS:
testzoweuss

When you first open USS and click on a member it does not do the above.
Only when you, right click and select "Pull from mainframe" does it change the file icon.

@tiantn
Copy link
Contributor Author

tiantn commented May 20, 2022

Hi @mmoore96, thank you for the detail test information.
I think all this strange phenomena are related to the fact that etag was not got correctly before. I also met the same problems. In this PR #1813, I changed the response type to get the etag correctly. Now the etag problem fixed. Sorry to bring more confusion to you. And other enhancements we discussed will be added in the future. Thank you very much!

@mmoore96
Copy link

mmoore96 commented May 20, 2022

@tiantn
I performed all of this testing with your fix already implemented.

I also added code to save the file two both the VSCode and AppData directories. I still find a hash mismatch under certain conditions.

I also performed a byte by byte comparison between the AppData file and the VSCode one, and found no differences.

When I look back at the my log, the issue is when saving to the mainframe, the "new" hash is not updated and stored correctly every time. While debugging you can see this happening.

Furthermore, running the production v2.0.2 with your fix in place still results in erroneous save conflicts.

I do not believe it is a CRLF vs LF issue. We are not performing SHA-1 hashing on the mainframe. We are doing it on windows after it is FTP'd which would handle these conversations. If we hashed the organic mainframe file, it would never match the windows hash due to the EBCDIC encoding on the mainframe vs. ASCII on Windows.

Would we not have this same issue with dataset's etag mismatching if it was a CRLF vs. LF problem?

Please correct me if I am missing something.

@tiantn
Copy link
Contributor Author

tiantn commented May 20, 2022

Hi @mmoore96, thank you. I add some print messages in code, like below:
图片
图片

I tested the save normally and save with conflict. All works as expected.

save normally:
图片

save with conflict:
图片

I don't find problems in my tests. 🤔

@mmoore96
Copy link

mmoore96 commented May 20, 2022

@tiantn

I see that. I can get the same results with a simple test.

The issue arises when doing work with multiple files. Such as saving, re-opening them, going to other files and rinse and repeat.

Opening and saving one file, it works properly (~90% of the time). But the problem is it throws these errors too often in the work day with working with multiple files.

At no point during my testing did I ever make a change to the file on the mainframe. So a save conflict should never be thrown.

Are you able to supply your USS file settings via TSO IShell? Just to make sure we have the same file types/settings on the mainframe.

Furthermore, maybe next week we could hop on a call and I can show you exactly what I am talking about on my end.

I am not saying your last 2 PRs did nothing. It has greatly reduced the number of times a save conflict is thrown. However, there are still times in which a etag mismatch is present and it should not be.

Thank you for supplying your results.

@tiantn
Copy link
Contributor Author

tiantn commented May 22, 2022

Hi @mmoore96, thank you for the information.

So the issue arises randomly when edit multiple files and with multiple times edit, right? Does the one file with multiple times edit works well?

And my test file attributes are as below:

图片
图片
图片

Thank you!

@tiantn
Copy link
Contributor Author

tiantn commented May 23, 2022

Hi @mmoore96, we found the conflict happened when Save multiple times in a short time. Means the save is not completed (message File uploaded successfully. not display) while the second save is issued.

Is this case with you? Thank you!

@mmoore96
Copy link

mmoore96 commented May 23, 2022

@tiantn
Yes that is correct. For me to get an error, I have to open and save multiple files in a short time.
I should have expressed that in my testing. At my job, I am always editing multiple USS files.

I am happy you were able to recreate the error!

@tiantn
Copy link
Contributor Author

tiantn commented May 24, 2022

Hi @mmoore96 , thank you to confirm this.

Sure, it is a issue in current implementations because commands are executed separately. Shall we open a new issue to record it and we will investigate and enhance it in the future? Thank you!

@mmoore96
Copy link

mmoore96 commented May 25, 2022

@tiantn
That sounds good. I will open an issue tomorrow.

I know that there are some differences between Dataset and USS implementation. However, I have never ran across this issue with Datasets.
Is the logic for sending commands (that much) different with datasets?

Thank you!

@tiantn
Copy link
Contributor Author

tiantn commented May 25, 2022

Hi @mmoore96 , thank you for spending a lot of time and effort on looking into this problem.

There is no difference between Dataset and Uss on sending commands from FTP extension side. So I think the future enhancement may consider in general.

Thank you!

@zFernand0 zFernand0 mentioned this pull request Jul 5, 2022
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants