-
Notifications
You must be signed in to change notification settings - Fork 86
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
[v2] Download special char issue #2362
Conversation
Signed-off-by: jace-roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## zowe-v2-lts #2362 +/- ##
=============================================
Coverage 91.24% 91.25%
=============================================
Files 638 638
Lines 19146 19158 +12
Branches 3943 4060 +117
=============================================
+ Hits 17470 17482 +12
Misses 1675 1675
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Co-authored-by: Timothy Johnson <[email protected]> Signed-off-by: Jace Roell <[email protected]>
Signed-off-by: jace-roell <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for porting 😋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how we are handling buffers in the processNewLines
function.
Left a couple of comments, but I don't believe that they should stop this PR from being merged. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to generate this file as part of the test resources, and remove after the test is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be handled in a separate PR, my apologies 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once Fernando's questions are resolved, I approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 😋
Apologies for the oversight. Zowe Explorer still has the issue, but the data on the mainframe looks good 🙏
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
This PR resolves an issue where special characters could become corrupted when downloading large files. The corruption was caused by each chunk buffer being converted to a string when calling the processNewLines() function and then converted back to a buffer. This PR introduces support for passing buffers directly into the processNewLines() function to prevent corruption.
How to Test
The issue arises when a special character appears as the last character of a stream chunk. To reproduce, create a PDS file containing exclusively special characters with a size larger than the default chunk size (>8KB). In such cases, the last character of each chunk may be corrupted and appear as two unknown characters.
To observe this issue, download a PDS file that meets the criteria using the following example command (assuming the use of IBM-1147 encoding):
zowe zos-files download ds <PDSMEMBER> --encoding "IBM-1147"
where
<PDSMEMBER>
is a file larger than 8KB containing special characters.Example Line (about 105 are required in a file to exceed 8KB):
àèéìòùÀÈÉÌÒÙ°àèéìòùÀÈÉÌÒÙ°àèéìòùÀÈÉÌÒÙ°àèéìòùÀÈÉÌÒÙ°àèéìòùÀÈÉÌÒÙ°àèéìòùÀÈÉÌÒÙ°à
Review Checklist
I certify that I have: