Expose Comment in ZipArchive and ZipArchiveEntry #59442

carlossanlop · 2021-09-22T00:29:56Z

Spec: https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

dotnet-issue-labeler · 2021-09-22T00:30:02Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-09-22T00:30:03Z

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #1547

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO.Compression`
Milestone:	7.0.0

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

adamsitnik

@carlossanlop thank you for your contribution! PTAL at my comments

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs

carlossanlop · 2021-12-06T04:03:09Z

@adamsitnik I addressed your last comments.

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

adamsitnik

LGTM, thank you @carlossanlop !

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs

…ebug.Assert.

…ing action when disposing.

Co-authored-by: Adam Sitnik <[email protected]>

…long characters like emojis. Simplify tests.

ericstj · 2022-01-31T23:02:13Z

Is this one ready to merge?

carlossanlop · 2022-02-01T17:58:34Z

Is this one ready to merge?

I'd like @jozkee to take a last look, since it was his comments which I addressed in my last commit.

jozkee · 2022-02-01T21:55:12Z

src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs

+        protected const string LowerCaseOUmlautChar = "\u00F6";
+        protected const string CopyrightChar = "\u00A9";


What's special about these versus '1' or 'a'?

I needed chars that cannot be represented in ASCII.

Can you express that in the name of the const or in a comment?

jozkee · 2022-02-01T22:00:48Z

src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs

+            foreach (object[] e in SharedComment_Data())
+            {
+                yield return e;
+            }


Move this to the end of the method to avoid mixing what is being reused from what is unique to this method.

I prefer to yield the shared cases first.

jozkee · 2022-02-01T22:03:54Z

src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs

+            string asciiExpectedExactMaxLength = new('a', ushort.MaxValue);
+            string asciiOriginalOverMaxLength = asciiExpectedExactMaxLength + "aaa";


Why are these prefixed as Ascii considering that the method name is Utf8Comment_Data?

Suggested change

string asciiExpectedExactMaxLength = new('a', ushort.MaxValue);

string asciiOriginalOverMaxLength = asciiExpectedExactMaxLength + "aaa";

string utf8ExpectedExactMaxLength = new('a', ushort.MaxValue);

string utf8OriginalOverMaxLength = asciiExpectedExactMaxLength + "aaa";

It's test data. This method return test cases for both ASCII and UTF8. Renaming these variables would be incorrect.

jozkee · 2022-02-01T22:06:30Z

src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs

+            string asciiOriginalOverMaxLength = asciiExpectedExactMaxLength + "aaa";
+
+            // A smiley emoji code point consists of two characters,
+            // meaning the whole emoji should be fully truncated


You should add a case where the emoji does not get truncated.

The only case where the string is truncated is when it's length is greater than expected. The contents don't matter. So if a string is smaller than the max length, it should remain untouched, regardless of its contents.

jozkee · 2022-02-01T23:23:24Z

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs

+            bytes = encoding.GetBytes(text);
+            return maxBytes < bytes.Length ? bytes[0..maxBytes] : bytes;


bytes[0..maxBytes] is an allocation of it's own, isn't it? If so, consider implementing ArrayPool.

This was discussed before. We don't expect either the Comment setter or the EntryName setter to be called in a loop. That would be weird.

it is being called in a foreach loop in your own tests.

runtime/src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.Comments.cs

Line 67 in 3444631

Assert.Equal(expectedCreateComment, entry.Comment);

I don't know if enumeration of entries should care or not about one allocation, but it can definitely be improved to my suggestion.

I opened issue #64767 to follow up on this.

src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.Comments.cs

jozkee · 2022-02-01T23:36:41Z

src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.Comments.cs

+        [Theory]
+        [MemberData(nameof(Utf8Comment_Data))]
+        public static void Create_Comment_Utf8EntryName_NullEncoding(string originalComment, string expectedComment) =>
+            Create_Comment_EntryName_Encoding_Internal($"{SmileyEmoji}.txt", originalComment, expectedComment, null);


Does the entry name really need to be tested? It is not something that is being added in this PR.

The entry name is specifically being affected by this PR, because both the comment and the entry name share the same encoding the user specifies.

I was expecting that there were already tests that covered EntryName with multiple encodings, if that's not the case then it is fine.

jozkee · 2022-02-01T23:40:40Z

src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.Comments.cs

+        [Theory]
+        [MemberData(nameof(Latin1Comment_Data))]
+        public static void Create_Comment_Utf8EntryName_Latin1Encoding(string originalComment, string expectedComment) =>
+            // Emoji not supported by latin1


What happens if you pass an emoji with latin1 encoding? I assume it just encodes it as something meaningless. We should assert that the latin1-encoded-emoji still gets truncated.

I'm not sure how I would assert that in the tests. I just wanted to make sure the string got truncated, even if the final outputted string, when viewed by the user, will be garbage where the emoji was saved.

I'm not sure how I would assert that in the tests.

On Latin1Comment_Data add something like this:

string latin1ExpectedALettersAndTruncatedEmoji = new string('a', ushort.MaxValue - 1) + SmileyEmoji[0]; string latin1OriginalALettersAndEmoji = new string('a', ushort.MaxValue - 1) + SmileyEmoji;

My point being that the truncation logic only runs when it makes sense (UTF8).

jozkee · 2022-02-01T23:45:45Z

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.Comments.cs

+        public static void Update_Comment_AsciiEntryName_NullEncoding(string originalComment, string expectedComment) =>
+            Update_Comment_EntryName_Encoding_Internal("file.txt",
+                originalComment, expectedComment, null,
+                new string('a', ushort.MaxValue - 1) + $"{CopyrightChar}", new string('a', ushort.MaxValue - 1));


This is testing Utf8Comment_Data.Count() times the string new string('a', ushort.MaxValue - 1) + $"{CopyrightChar}", you should create another Utf8Comment_Data that includes this as a parameter and remove the redundancy.

jozkee · 2022-02-01T23:50:56Z

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.Comments.cs

+            using (var zip = new ZipArchive(ms, ZipArchiveMode.Create, leaveOpen: true, encoding))
            {
                ZipArchiveEntry entry = zip.CreateEntry(entryName, CompressionLevel.NoCompression);
-                entry.Comment = comment;
-
-                Assert.Equal(entryName, entry.FullName);
-                Assert.Equal(comment, entry.Comment);
+                entry.Comment = originalCreateComment;
+                Assert.Equal(expectedCreateComment, entry.Comment);
            }

-            // Open with no encoding to verify
-            using (var zip = new ZipArchive(testStream, ZipArchiveMode.Read, leaveOpen: true))
+            using (var zip = new ZipArchive(ms, ZipArchiveMode.Read, leaveOpen: true, encoding))
            {
-                ZipArchiveEntry entry = zip.Entries.FirstOrDefault();
-
-                Assert.Equal(entryName, entry.FullName);
-                Assert.Equal(comment, entry.Comment);
+                foreach (ZipArchiveEntry entry in zip.Entries)
+                {
+                    Assert.Equal(expectedCreateComment, entry.Comment);
+                }
            }


Call Create_Comment_EntryName_Encoding_Internal instead to avoid code duplication.

Or even better, you could test ZipArchiveMode.Update inside Create_Comment_EntryName_Encoding_Internal and remove zip_UpdateTests.Comments.cs entirely.

jozkee · 2022-02-02T00:35:15Z

src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs

@@ -383,5 +383,57 @@ internal static void AddEntry(ZipArchive archive, string name, string contents,
                w.WriteLine(contents);


This file should be in tests folder.

Good point. I don't know why it was created there. Maybe we used to share something with another assembly, but it got removed.

Ah search didn't return the other file that consumes ZipTestHelper. It's the ZipFile.csproj. I'll revert the move of this file.

jozkee

Other than the test nits; LGTM.

…sion.ZipFile depends on it too.

carlossanlop · 2022-02-04T18:30:26Z

FYI I ran the MacOS CI leg twice but it failed with errors unrelated to this change:

runtime 20220203.90 / Libraries Test Run release coreclr OSX x64 Debug
❌[Log] The job running on agent Azure Pipelines 5 ran longer than the maximum time of 150 minutes.
runtime 20220203.90 / Libraries Test Run release coreclr OSX x64 Debug / Send to Helix
❌[Log] The operation was canceled.

carlossanlop added the area-System.IO.Compression label Sep 22, 2021

carlossanlop added this to the 7.0.0 milestone Sep 22, 2021

carlossanlop requested review from adamsitnik and jozkee September 22, 2021 00:29

carlossanlop self-assigned this Sep 22, 2021

dotnet-issue-labeler bot added the new-api-needs-documentation label Sep 22, 2021

carlossanlop requested a review from ericstj September 22, 2021 00:30

carlossanlop commented Sep 22, 2021

View reviewed changes

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs Outdated Show resolved Hide resolved

GrabYourPitchforks reviewed Sep 23, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.Compression/tests/ZipArchive/zip_UpdateTests.cs Outdated Show resolved Hide resolved

ericstj reviewed Sep 28, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs Outdated Show resolved Hide resolved

jozkee reviewed Sep 29, 2021

View reviewed changes

carlossanlop force-pushed the zipComment branch from 218c347 to be8acc7 Compare October 6, 2021 03:13

runfoapp bot mentioned this pull request Oct 19, 2021

Error in CI Mono llvmfullaot Pri0 Runtime Tests Run Linux arm64 release #60234

Closed

carlossanlop force-pushed the zipComment branch from 8a36a78 to eff8e13 Compare November 27, 2021 19:53

carlossanlop requested a review from jozkee November 27, 2021 19:53

danmoseley reviewed Nov 29, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs Outdated Show resolved Hide resolved

danmoseley reviewed Nov 29, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs Outdated Show resolved Hide resolved

danmoseley reviewed Nov 29, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs Outdated Show resolved Hide resolved

adamsitnik reviewed Dec 1, 2021

View reviewed changes

carlossanlop force-pushed the zipComment branch 2 times, most recently from 57cfa5a to cdb57d5 Compare December 4, 2021 03:10

adamsitnik reviewed Dec 6, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs Outdated Show resolved Hide resolved

jozkee reviewed Dec 6, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs Outdated Show resolved Hide resolved

carlossanlop force-pushed the zipComment branch from cdb57d5 to d429b27 Compare December 7, 2021 04:40

adamsitnik approved these changes Dec 7, 2021

View reviewed changes

src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs Outdated Show resolved Hide resolved

carlossanlop and others added 14 commits January 18, 2022 09:34

Add more test cases

0a29149

Adjust tests

b93305b

Add newline so comment only applies to one line

7496628

Ensure string byte truncation is aligned to encoding's char size.

a949883

Remove empty check for non-nullable string. Also remove unnecessary D…

9738c30

…ebug.Assert.

Defer calculation of truncated encoding string to getter and to writ…

5179ea2

…ing action when disposing.

Rename test arguments

0e47de4

Only use bytes[]

18bbcd4

Remove unnecessary bit comment

31602b3

Remove unnecessary length check

0ae8a64

Address feedback

e9d5b67

Suggestion by adamsitnik: write only if length > 0

77b6ee5

Co-authored-by: Adam Sitnik <[email protected]>

Simplify EntryName code

8daedcf

Move entryName code to original location

1596bb3

carlossanlop force-pushed the zipComment branch from 3b0f43c to 1596bb3 Compare January 18, 2022 17:35

In UTF8, use Runes to detect code point length to prevent truncating …

3444631

…long characters like emojis. Simplify tests.

jozkee reviewed Feb 1, 2022

View reviewed changes

jozkee reviewed Feb 2, 2022

View reviewed changes

jozkee approved these changes Feb 2, 2022

View reviewed changes

Address suggestions

0749c84

carlossanlop mentioned this pull request Feb 3, 2022

Improve how byte arrays are created in ZipHelper.GetEncodedTruncatedBytesFromString #64767

Open

Move ZipTestHelper back to its original position because S.IO.Compres…

b08f0ac

…sion.ZipFile depends on it too.

carlossanlop merged commit 7065279 into dotnet:main Feb 4, 2022

carlossanlop deleted the zipComment branch February 4, 2022 03:35

jozkee mentioned this pull request Feb 25, 2022

Fix ZipArchiveEntry names shown as corrupted on other zip programs #65886

Merged

ghost locked as resolved and limited conversation to collaborators Mar 6, 2022

		protected const string LowerCaseOUmlautChar = "\u00F6";
		protected const string CopyrightChar = "\u00A9";

		string asciiExpectedExactMaxLength = new('a', ushort.MaxValue);
		string asciiOriginalOverMaxLength = asciiExpectedExactMaxLength + "aaa";

		bytes = encoding.GetBytes(text);
		return maxBytes < bytes.Length ? bytes[0..maxBytes] : bytes;

		@@ -383,5 +383,57 @@ internal static void AddEntry(ZipArchive archive, string name, string contents,
		w.WriteLine(contents);

Expose Comment in ZipArchive and ZipArchiveEntry #59442

Expose Comment in ZipArchive and ZipArchiveEntry #59442

Conversation

carlossanlop commented Sep 22, 2021 • edited Loading

dotnet-issue-labeler bot commented Sep 22, 2021

ghost commented Sep 22, 2021

adamsitnik left a comment

Choose a reason for hiding this comment

carlossanlop commented Dec 6, 2021

adamsitnik left a comment

Choose a reason for hiding this comment

ericstj commented Jan 31, 2022

carlossanlop commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee left a comment

Choose a reason for hiding this comment

carlossanlop commented Feb 4, 2022

carlossanlop commented Sep 22, 2021 •

edited

Loading

carlossanlop Feb 2, 2022 •

edited

Loading