From 9d7a05c5fac2c3ac749757ed578623e09f05d0a1 Mon Sep 17 00:00:00 2001 From: Edward Neal <55035479+edwardneal@users.noreply.github.com> Date: Sun, 11 Feb 2024 15:00:28 +0000 Subject: [PATCH 1/7] Use CompressionLevel to set general-purpose bit flags Also changed mapping of ZipPackage's compression options, such that CompressionOption.Maximum now sets the compression level to SmallestSize, and SuperFast now sets the compression level to NoCompression. Both of these changes restore compatibility with the .NET Framework. --- .../System/IO/Compression/ZipArchiveEntry.cs | 37 ++++++++++-- .../tests/ZipArchive/zip_CreateTests.cs | 59 +++++++++++++++++++ .../src/System/IO/Packaging/ZipPackage.cs | 6 +- 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs index 3d415f8e3690f1..ac6bc8df1a1a4e 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs @@ -86,7 +86,7 @@ internal ZipArchiveEntry(ZipArchive archive, ZipCentralDirectoryFileHeader cd) _fileComment = cd.FileComment; - _compressionLevel = null; + _compressionLevel = GetCompressionLevel(_generalPurposeBitFlag); } // Initializes a ZipArchiveEntry instance for a new archive entry with a specified compression level. @@ -98,6 +98,7 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName, CompressionLevel { CompressionMethod = CompressionMethodValues.Stored; } + _generalPurposeBitFlag = MapDeflateCompressionOption(_generalPurposeBitFlag, _compressionLevel); } // Initializes a ZipArchiveEntry instance for a new archive entry. @@ -111,7 +112,8 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName) _versionMadeByPlatform = CurrentZipPlatform; _versionMadeBySpecification = ZipVersionNeededValues.Default; _versionToExtract = ZipVersionNeededValues.Default; // this must happen before following two assignment - _generalPurposeBitFlag = 0; + _compressionLevel = null; + _generalPurposeBitFlag = MapDeflateCompressionOption(0, _compressionLevel); CompressionMethod = CompressionMethodValues.Deflate; _lastModified = DateTimeOffset.Now; @@ -138,8 +140,6 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName) _fileComment = Array.Empty(); - _compressionLevel = null; - if (_storedEntryNameBytes.Length > ushort.MaxValue) throw new ArgumentException(SR.EntryNamesTooLong); @@ -799,6 +799,35 @@ private bool IsOpenable(bool needToUncompress, bool needToLoadIntoMemory, out st private bool SizesTooLarge() => _compressedSize > uint.MaxValue || _uncompressedSize > uint.MaxValue; + private static CompressionLevel? GetCompressionLevel(BitFlagValues generalPurposeBitFlag) + { + // Information about the Deflate compression option is stored in bits 1 and 2 of the general purpose bit flags. + int deflateCompressionOption = (int)generalPurposeBitFlag & 0x6; + + return deflateCompressionOption switch + { + 0 => CompressionLevel.Optimal, + 2 => CompressionLevel.SmallestSize, + 4 => CompressionLevel.Fastest, + 6 => CompressionLevel.NoCompression, + _ => null + }; + } + + private static BitFlagValues MapDeflateCompressionOption(BitFlagValues generalPurposeBitFlag, CompressionLevel? compressionLevel) + { + ushort deflateCompressionOptions = compressionLevel switch + { + CompressionLevel.Optimal => 0, + CompressionLevel.SmallestSize => 2, + CompressionLevel.Fastest => 4, + CompressionLevel.NoCompression => 6, + _ => 0 + }; + + return (BitFlagValues)(((int)generalPurposeBitFlag & ~0x6) | deflateCompressionOptions); + } + // return value is true if we allocated an extra field for 64 bit headers, un/compressed size private bool WriteLocalFileHeader(bool isEmptyFile) { diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs index fe1daa839bfe75..1029158d125fc2 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs @@ -142,6 +142,65 @@ public static void CreateUncompressedArchive() } } + // This test checks to ensure that setting the compression level of an archive entry sets the general-purpose + // bit flags correctly. It verifies that these have been set by reading from the MemoryStream manually, and by + // reopening the generated file to confirm that the compression levels match. + [Theory] + [InlineData(CompressionLevel.NoCompression, 6)] + [InlineData(CompressionLevel.Optimal, 0)] + [InlineData(CompressionLevel.SmallestSize, 2)] + [InlineData(CompressionLevel.Fastest, 4)] + public static void CreateArchiveEntriesWithBitFlags(CompressionLevel compressionLevel, ushort expectedGeneralBitFlags) + { + byte[] fileContent; + + using (var testStream = new MemoryStream()) + { + var testfilename = "testfile"; + var testFileContent = "Lorem ipsum dolor sit amet, consectetur adipiscing elit."; + + using (var zip = new ZipArchive(testStream, ZipArchiveMode.Create)) + { + var utf8WithoutBom = new Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: false); + ZipArchiveEntry newEntry = zip.CreateEntry(testfilename, compressionLevel); + using (var writer = new StreamWriter(newEntry.Open(), utf8WithoutBom)) + { + writer.Write(testFileContent); + writer.Flush(); + } + + ZipArchiveEntry secondNewEntry = zip.CreateEntry(testFileContent + "_post", CompressionLevel.NoCompression); + } + + fileContent = testStream.ToArray(); + } + + // expected bit flags are at position 6 in the file header + var generalBitFlags = System.Buffers.Binary.BinaryPrimitives.ReadUInt16LittleEndian(fileContent.AsSpan(6)); + + Assert.Equal(expectedGeneralBitFlags, generalBitFlags); + + using (var reReadStream = new MemoryStream(fileContent)) + { + using (var reReadZip = new ZipArchive(reReadStream, ZipArchiveMode.Read)) + { + var firstArchive = reReadZip.Entries[0]; + var secondArchive = reReadZip.Entries[1]; + var compressionLevelFieldInfo = typeof(ZipArchiveEntry).GetField("_compressionLevel", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); + var generalBitFlagsFieldInfo = typeof(ZipArchiveEntry).GetField("_generalPurposeBitFlag", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); + + var reReadCompressionLevel = (CompressionLevel)compressionLevelFieldInfo.GetValue(firstArchive); + var reReadGeneralBitFlags = (ushort)generalBitFlagsFieldInfo.GetValue(firstArchive); + + Assert.Equal(compressionLevel, reReadCompressionLevel); + Assert.Equal(expectedGeneralBitFlags, reReadGeneralBitFlags); + + reReadCompressionLevel = (CompressionLevel)compressionLevelFieldInfo.GetValue(secondArchive); + Assert.Equal(CompressionLevel.NoCompression, reReadCompressionLevel); + } + } + } + [Fact] public static void CreateNormal_VerifyDataDescriptor() { diff --git a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs index d142afc3e0b577..c37411cbc26181 100644 --- a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs +++ b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs @@ -383,7 +383,11 @@ internal static void GetZipCompressionMethodFromOpcCompressionOption( break; case CompressionOption.Maximum: { +#if (!NETSTANDARD2_0 && !NETFRAMEWORK) + compressionLevel = CompressionLevel.SmallestSize; +#else compressionLevel = CompressionLevel.Optimal; +#endif } break; case CompressionOption.Fast: @@ -393,7 +397,7 @@ internal static void GetZipCompressionMethodFromOpcCompressionOption( break; case CompressionOption.SuperFast: { - compressionLevel = CompressionLevel.Fastest; + compressionLevel = CompressionLevel.NoCompression; } break; From 6a496548ae2e737c07a9f9ca381dc532d625376c Mon Sep 17 00:00:00 2001 From: Edward Neal <55035479+edwardneal@users.noreply.github.com> Date: Sun, 11 Feb 2024 15:58:05 +0000 Subject: [PATCH 2/7] Made function verbs consistent --- .../src/System/IO/Compression/ZipArchiveEntry.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs index ac6bc8df1a1a4e..514bad262127fb 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs @@ -86,7 +86,7 @@ internal ZipArchiveEntry(ZipArchive archive, ZipCentralDirectoryFileHeader cd) _fileComment = cd.FileComment; - _compressionLevel = GetCompressionLevel(_generalPurposeBitFlag); + _compressionLevel = MapCompressionLevel(_generalPurposeBitFlag); } // Initializes a ZipArchiveEntry instance for a new archive entry with a specified compression level. @@ -799,7 +799,7 @@ private bool IsOpenable(bool needToUncompress, bool needToLoadIntoMemory, out st private bool SizesTooLarge() => _compressedSize > uint.MaxValue || _uncompressedSize > uint.MaxValue; - private static CompressionLevel? GetCompressionLevel(BitFlagValues generalPurposeBitFlag) + private static CompressionLevel? MapCompressionLevel(BitFlagValues generalPurposeBitFlag) { // Information about the Deflate compression option is stored in bits 1 and 2 of the general purpose bit flags. int deflateCompressionOption = (int)generalPurposeBitFlag & 0x6; From 1f5a5d9105c78f171ee1b789621f6cdfc4da9dd6 Mon Sep 17 00:00:00 2001 From: Edward Neal <55035479+edwardneal@users.noreply.github.com> Date: Sun, 11 Feb 2024 16:12:28 +0000 Subject: [PATCH 3/7] Added test to verify read file contents --- .../tests/ZipArchive/zip_CreateTests.cs | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs index 1029158d125fc2..06299fb210ae89 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs @@ -152,16 +152,17 @@ public static void CreateUncompressedArchive() [InlineData(CompressionLevel.Fastest, 4)] public static void CreateArchiveEntriesWithBitFlags(CompressionLevel compressionLevel, ushort expectedGeneralBitFlags) { - byte[] fileContent; + var testfilename = "testfile"; + var testFileContent = "Lorem ipsum dolor sit amet, consectetur adipiscing elit."; + var utf8WithoutBom = new Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: false); + + byte[] zipFileContent; using (var testStream = new MemoryStream()) { - var testfilename = "testfile"; - var testFileContent = "Lorem ipsum dolor sit amet, consectetur adipiscing elit."; using (var zip = new ZipArchive(testStream, ZipArchiveMode.Create)) { - var utf8WithoutBom = new Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: false); ZipArchiveEntry newEntry = zip.CreateEntry(testfilename, compressionLevel); using (var writer = new StreamWriter(newEntry.Open(), utf8WithoutBom)) { @@ -172,15 +173,15 @@ public static void CreateArchiveEntriesWithBitFlags(CompressionLevel compression ZipArchiveEntry secondNewEntry = zip.CreateEntry(testFileContent + "_post", CompressionLevel.NoCompression); } - fileContent = testStream.ToArray(); + zipFileContent = testStream.ToArray(); } // expected bit flags are at position 6 in the file header - var generalBitFlags = System.Buffers.Binary.BinaryPrimitives.ReadUInt16LittleEndian(fileContent.AsSpan(6)); + var generalBitFlags = System.Buffers.Binary.BinaryPrimitives.ReadUInt16LittleEndian(zipFileContent.AsSpan(6)); Assert.Equal(expectedGeneralBitFlags, generalBitFlags); - using (var reReadStream = new MemoryStream(fileContent)) + using (var reReadStream = new MemoryStream(zipFileContent)) { using (var reReadZip = new ZipArchive(reReadStream, ZipArchiveMode.Read)) { @@ -197,6 +198,17 @@ public static void CreateArchiveEntriesWithBitFlags(CompressionLevel compression reReadCompressionLevel = (CompressionLevel)compressionLevelFieldInfo.GetValue(secondArchive); Assert.Equal(CompressionLevel.NoCompression, reReadCompressionLevel); + + using (var strm = firstArchive.Open()) + { + var readBuffer = new byte[firstArchive.Length]; + + strm.Read(readBuffer); + + var readText = Text.Encoding.UTF8.GetString(readBuffer); + + Assert.Equal(readText, testFileContent); + } } } } From fb78d54e7bc3af29c3ba7f0120a64a9f3f5f0bfc Mon Sep 17 00:00:00 2001 From: Edward Neal <55035479+edwardneal@users.noreply.github.com> Date: Sun, 11 Feb 2024 16:59:56 +0000 Subject: [PATCH 4/7] Corrected failing Packaging test This test was intended to ensure that bit 11 isn't set. It was actually performing a blind comparison of the entire bit field. Other tests in System.IO.Packaging function properly. --- src/libraries/System.IO.Packaging/tests/ReflectionTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.IO.Packaging/tests/ReflectionTests.cs b/src/libraries/System.IO.Packaging/tests/ReflectionTests.cs index 78f080aa6ce782..91e31b356e855d 100644 --- a/src/libraries/System.IO.Packaging/tests/ReflectionTests.cs +++ b/src/libraries/System.IO.Packaging/tests/ReflectionTests.cs @@ -34,7 +34,7 @@ public void Verify_GeneralPurposeBitFlag_NotSetTo_Unicode() FieldInfo fieldInfo = typeof(ZipArchiveEntry).GetField("_generalPurposeBitFlag", BindingFlags.Instance | BindingFlags.NonPublic); object fieldObject = fieldInfo.GetValue(entry); ushort shortField = (ushort)fieldObject; - Assert.Equal(0, shortField); // If it was UTF8, we would set the general purpose bit flag to 0x800 (UnicodeFileNameAndComment) + Assert.Equal(0, shortField & 0x800); // If it was UTF8, we would set the general purpose bit flag to 0x800 (UnicodeFileNameAndComment) CheckCharacters(entry.Name); CheckCharacters(entry.Comment); // Unavailable in .NET Framework } From c24d3690660b907e470bbbd966b230cecb87183a Mon Sep 17 00:00:00 2001 From: Edward Neal <55035479+edwardneal@users.noreply.github.com> Date: Fri, 22 Mar 2024 21:26:58 +0000 Subject: [PATCH 5/7] Changes following code review * Updated the conditional compilation directives for the .NET Framework/Core package CompressionLevel mappings. * Specifying a CompressionMethod other than Deflate or Deflate64 will now set the compression level to NoCompression, and will write zeros to the relevant general purpose bits. * The CompressionLevel always defaulted to Optimal for new archives, but this is now explicitly set (rather than relying upon setting it to null and null-coalescing it to Optimal.) This removes a condition to test for. * Updated the test data for the CreateArchiveEntriesWithBitFlags test. If the compression level is set to NoCompression, we should expect the CompressionMethod to become Stored (which unsets the general purpose bits, returning an expected result of zero.) --- .../System/IO/Compression/ZipArchiveEntry.cs | 46 +++++++++++-------- .../tests/ZipArchive/zip_CreateTests.cs | 3 +- .../src/System/IO/Packaging/ZipPackage.cs | 2 +- 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs index 514bad262127fb..3552576235de4f 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs @@ -44,7 +44,7 @@ public partial class ZipArchiveEntry private List? _cdUnknownExtraFields; private List? _lhUnknownExtraFields; private byte[] _fileComment; - private readonly CompressionLevel? _compressionLevel; + private readonly CompressionLevel _compressionLevel; // Initializes a ZipArchiveEntry instance for an existing archive entry. internal ZipArchiveEntry(ZipArchive archive, ZipCentralDirectoryFileHeader cd) @@ -86,7 +86,7 @@ internal ZipArchiveEntry(ZipArchive archive, ZipCentralDirectoryFileHeader cd) _fileComment = cd.FileComment; - _compressionLevel = MapCompressionLevel(_generalPurposeBitFlag); + _compressionLevel = MapCompressionLevel(_generalPurposeBitFlag, CompressionMethod); } // Initializes a ZipArchiveEntry instance for a new archive entry with a specified compression level. @@ -98,7 +98,7 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName, CompressionLevel { CompressionMethod = CompressionMethodValues.Stored; } - _generalPurposeBitFlag = MapDeflateCompressionOption(_generalPurposeBitFlag, _compressionLevel); + _generalPurposeBitFlag = MapDeflateCompressionOption(_generalPurposeBitFlag, _compressionLevel, CompressionMethod); } // Initializes a ZipArchiveEntry instance for a new archive entry. @@ -112,9 +112,9 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName) _versionMadeByPlatform = CurrentZipPlatform; _versionMadeBySpecification = ZipVersionNeededValues.Default; _versionToExtract = ZipVersionNeededValues.Default; // this must happen before following two assignment - _compressionLevel = null; - _generalPurposeBitFlag = MapDeflateCompressionOption(0, _compressionLevel); + _compressionLevel = CompressionLevel.Optimal; CompressionMethod = CompressionMethodValues.Deflate; + _generalPurposeBitFlag = MapDeflateCompressionOption(0, _compressionLevel, CompressionMethod); _lastModified = DateTimeOffset.Now; _compressedSize = 0; // we don't know these yet @@ -632,7 +632,7 @@ private CheckSumAndSizeWriteStream GetDataCompressor(Stream backingStream, bool case CompressionMethodValues.Deflate: case CompressionMethodValues.Deflate64: default: - compressorStream = new DeflateStream(backingStream, _compressionLevel ?? CompressionLevel.Optimal, leaveBackingStreamOpen); + compressorStream = new DeflateStream(backingStream, _compressionLevel, leaveBackingStreamOpen); break; } @@ -799,31 +799,39 @@ private bool IsOpenable(bool needToUncompress, bool needToLoadIntoMemory, out st private bool SizesTooLarge() => _compressedSize > uint.MaxValue || _uncompressedSize > uint.MaxValue; - private static CompressionLevel? MapCompressionLevel(BitFlagValues generalPurposeBitFlag) + private static CompressionLevel MapCompressionLevel(BitFlagValues generalPurposeBitFlag, CompressionMethodValues compressionMethod) { // Information about the Deflate compression option is stored in bits 1 and 2 of the general purpose bit flags. - int deflateCompressionOption = (int)generalPurposeBitFlag & 0x6; + // If the compression method is not Deflate, the Deflate compression option is invalid - default to NoCompression. + int deflateCompressionOption = compressionMethod == CompressionMethodValues.Deflate || compressionMethod == CompressionMethodValues.Deflate64 + ? (int)generalPurposeBitFlag & 0x6 + : 6; - return deflateCompressionOption switch + return deflateCompressionOption switch { 0 => CompressionLevel.Optimal, 2 => CompressionLevel.SmallestSize, 4 => CompressionLevel.Fastest, 6 => CompressionLevel.NoCompression, - _ => null + _ => CompressionLevel.Optimal }; } - private static BitFlagValues MapDeflateCompressionOption(BitFlagValues generalPurposeBitFlag, CompressionLevel? compressionLevel) + private static BitFlagValues MapDeflateCompressionOption(BitFlagValues generalPurposeBitFlag, CompressionLevel compressionLevel, CompressionMethodValues compressionMethod) { - ushort deflateCompressionOptions = compressionLevel switch - { - CompressionLevel.Optimal => 0, - CompressionLevel.SmallestSize => 2, - CompressionLevel.Fastest => 4, - CompressionLevel.NoCompression => 6, - _ => 0 - }; + ushort deflateCompressionOptions = (ushort)( + // The Deflate compression level is only valid if the compression method is actually Deflate (or Deflate64). If it's not, the + // value of the two bits is undefined and they should be zeroed out. + compressionMethod == CompressionMethodValues.Deflate || compressionMethod == CompressionMethodValues.Deflate64 + ? compressionLevel switch + { + CompressionLevel.Optimal => 0, + CompressionLevel.SmallestSize => 2, + CompressionLevel.Fastest => 4, + CompressionLevel.NoCompression => 6, + _ => 0 + } + : 0); return (BitFlagValues)(((int)generalPurposeBitFlag & ~0x6) | deflateCompressionOptions); } diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs index 06299fb210ae89..dabafb70915d7a 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs @@ -146,7 +146,8 @@ public static void CreateUncompressedArchive() // bit flags correctly. It verifies that these have been set by reading from the MemoryStream manually, and by // reopening the generated file to confirm that the compression levels match. [Theory] - [InlineData(CompressionLevel.NoCompression, 6)] + // Special-case NoCompression: in this case, the CompressionMethod becomes Stored and the bits are unset. + [InlineData(CompressionLevel.NoCompression, 0)] [InlineData(CompressionLevel.Optimal, 0)] [InlineData(CompressionLevel.SmallestSize, 2)] [InlineData(CompressionLevel.Fastest, 4)] diff --git a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs index c37411cbc26181..f42a72789ddf4a 100644 --- a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs +++ b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs @@ -383,7 +383,7 @@ internal static void GetZipCompressionMethodFromOpcCompressionOption( break; case CompressionOption.Maximum: { -#if (!NETSTANDARD2_0 && !NETFRAMEWORK) +#if NET compressionLevel = CompressionLevel.SmallestSize; #else compressionLevel = CompressionLevel.Optimal; From 779e0859ecdeffe74153dfcb6daeb8ee4a1d281e Mon Sep 17 00:00:00 2001 From: Edward Neal <55035479+edwardneal@users.noreply.github.com> Date: Mon, 25 Mar 2024 21:14:27 +0000 Subject: [PATCH 6/7] Code review changes * Updated mapping between general purpose bit fields and CompressionLevel. * Updated mapping from CompressionOption to CompressionLevel. * Added test to verify round-trip of CompressionOption and its setting of the general purpose bit fields. --- .../System/IO/Compression/ZipArchiveEntry.cs | 27 ++++++------ .../tests/ZipArchive/zip_CreateTests.cs | 2 +- .../src/System/IO/Packaging/ZipPackage.cs | 2 +- .../System.IO.Packaging/tests/Tests.cs | 41 +++++++++++++++++++ 4 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs index 3552576235de4f..b146a929760061 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs @@ -803,18 +803,21 @@ private static CompressionLevel MapCompressionLevel(BitFlagValues generalPurpose { // Information about the Deflate compression option is stored in bits 1 and 2 of the general purpose bit flags. // If the compression method is not Deflate, the Deflate compression option is invalid - default to NoCompression. - int deflateCompressionOption = compressionMethod == CompressionMethodValues.Deflate || compressionMethod == CompressionMethodValues.Deflate64 - ? (int)generalPurposeBitFlag & 0x6 - : 6; - - return deflateCompressionOption switch + if (compressionMethod == CompressionMethodValues.Deflate || compressionMethod == CompressionMethodValues.Deflate64) + { + return ((int)generalPurposeBitFlag & 0x6) switch + { + 0 => CompressionLevel.Optimal, + 2 => CompressionLevel.SmallestSize, + 4 => CompressionLevel.Fastest, + 6 => CompressionLevel.Fastest, + _ => CompressionLevel.Optimal + }; + } + else { - 0 => CompressionLevel.Optimal, - 2 => CompressionLevel.SmallestSize, - 4 => CompressionLevel.Fastest, - 6 => CompressionLevel.NoCompression, - _ => CompressionLevel.Optimal - }; + return CompressionLevel.NoCompression; + } } private static BitFlagValues MapDeflateCompressionOption(BitFlagValues generalPurposeBitFlag, CompressionLevel compressionLevel, CompressionMethodValues compressionMethod) @@ -827,7 +830,7 @@ private static BitFlagValues MapDeflateCompressionOption(BitFlagValues generalPu { CompressionLevel.Optimal => 0, CompressionLevel.SmallestSize => 2, - CompressionLevel.Fastest => 4, + CompressionLevel.Fastest => 6, CompressionLevel.NoCompression => 6, _ => 0 } diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs index dabafb70915d7a..dae65db374f40d 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs @@ -150,7 +150,7 @@ public static void CreateUncompressedArchive() [InlineData(CompressionLevel.NoCompression, 0)] [InlineData(CompressionLevel.Optimal, 0)] [InlineData(CompressionLevel.SmallestSize, 2)] - [InlineData(CompressionLevel.Fastest, 4)] + [InlineData(CompressionLevel.Fastest, 6)] public static void CreateArchiveEntriesWithBitFlags(CompressionLevel compressionLevel, ushort expectedGeneralBitFlags) { var testfilename = "testfile"; diff --git a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs index f42a72789ddf4a..b224f5d1042d04 100644 --- a/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs +++ b/src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs @@ -397,7 +397,7 @@ internal static void GetZipCompressionMethodFromOpcCompressionOption( break; case CompressionOption.SuperFast: { - compressionLevel = CompressionLevel.NoCompression; + compressionLevel = CompressionLevel.Fastest; } break; diff --git a/src/libraries/System.IO.Packaging/tests/Tests.cs b/src/libraries/System.IO.Packaging/tests/Tests.cs index 4d547db5e1c199..2d814bdde5051a 100644 --- a/src/libraries/System.IO.Packaging/tests/Tests.cs +++ b/src/libraries/System.IO.Packaging/tests/Tests.cs @@ -3988,6 +3988,47 @@ public void CreatePackUriWithFragment() } + [Theory] +#if NET + [InlineData(CompressionOption.NotCompressed, CompressionOption.Normal, 0)] + [InlineData(CompressionOption.Normal, CompressionOption.Normal, 0)] + [InlineData(CompressionOption.Maximum, CompressionOption.Normal, 2)] + [InlineData(CompressionOption.Fast, CompressionOption.Normal, 6)] + [InlineData(CompressionOption.SuperFast, CompressionOption.Normal, 6)] +#else + [InlineData(CompressionOption.NotCompressed, CompressionOption.NotCompressed, 0)] + [InlineData(CompressionOption.Normal, CompressionOption.Normal, 0)] + [InlineData(CompressionOption.Maximum, CompressionOption.Maximum, 2)] + [InlineData(CompressionOption.Fast, CompressionOption.Fast, 4)] + [InlineData(CompressionOption.SuperFast, CompressionOption.SuperFast, 6)] +#endif + public void Roundtrip_Compression_Option(CompressionOption createdCompressionOption, CompressionOption expectedCompressionOption, ushort expectedZipFileBitFlags) + { + var documentPath = "untitled.txt"; + Uri partUriDocument = PackUriHelper.CreatePartUri(new Uri(documentPath, UriKind.Relative)); + + using (MemoryStream ms = new MemoryStream()) + { + Package package = Package.Open(ms, FileMode.Create, FileAccess.ReadWrite); + PackagePart part = package.CreatePart(partUriDocument, "application/text", createdCompressionOption); + + package.Flush(); + package.Close(); + (package as IDisposable).Dispose(); + + ms.Seek(0, SeekOrigin.Begin); + + var zipBytes = ms.ToArray(); + var generalBitFlags = System.Buffers.Binary.BinaryPrimitives.ReadUInt16LittleEndian(zipBytes.AsSpan(6)); + + package = Package.Open(ms, FileMode.Open, FileAccess.Read); + part = package.GetPart(partUriDocument); + + Assert.Equal(expectedZipFileBitFlags, generalBitFlags); + Assert.Equal(expectedCompressionOption, part.CompressionOption); + } + } + private const string DocumentRelationshipType = "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument"; } From ba47fd9ecf2ea79a44716784d17cbedfa4e3b2ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20S=C3=A1nchez=20L=C3=B3pez?= <1175054+carlossanlop@users.noreply.github.com> Date: Tue, 28 May 2024 20:35:27 -0600 Subject: [PATCH 7/7] Package authoring changes for System.IO.Packaging --- .../System.IO.Packaging/src/System.IO.Packaging.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj b/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj index fd5fa77c1cea4c..6d38af53ab005b 100644 --- a/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj +++ b/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj @@ -3,6 +3,8 @@ $(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum) true true + true + 1 Provides classes that support storage of multiple data objects in a single container.