From 90aca601ace0a9f283e70d50099df76521a25522 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/6] 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 14988fcab72817..a9373871286f70 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 c30915c5eacf9b..d1ffe150cf1278 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 912dc86d6fa5083a42959de40bcaa3c35294f643 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/6] 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 a9373871286f70..d9fac3c4f0252a 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 50cbe381f3a551e1cceabc99953d923d5eee0ce3 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/6] 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 1fe9d7503cfecbac38005210b6911f1aa80fbb04 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/6] 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 2f63f9f0a09002fff88067a7358263c40fa7c16c 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/6] 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 d9fac3c4f0252a..ababbfadc5d679 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 d1ffe150cf1278..0ff07d12dcc6e6 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 5f2c4f58fecf7ca288aeb89117af1ba6fa71d578 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/6] 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 ababbfadc5d679..d54cb5a9547131 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 0ff07d12dcc6e6..2d8f4273ad10a8 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"; }