[release/6.0-staging] Allow setting ZipArchiveEntry general-purpose flag bits #102096
Conversation
…ArchiveEntry general purpose bit flag unicode value is unset. (#88978)
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.
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.
* 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.)
* 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.
There was a problem hiding this comment.
Thanks @carlossanlop. .NET 6.0 doesn't have the ZipArchiveEntry.Comment property, which complicates the backport slightly. I've added a change which should hopefully allow the test to keep on testing the specification behaviour regardless of this.
The simpler alternative is just to remove the reference to entry.Comment on line 39 of ReflectionTests.cs. It depends whether we need to test for behaviour when it's in the spec, but not in the public API surface.
Co-authored-by: Edward Neal <55035479+edwardneal@users.noreply.github.com>
|
@edwardneal the good news is your suggested test fix worked (I had to adjust a couple of var names to make it build but it was a good suggestion). The bad news is that we're now hitting failures in System.IO.Packaging, and it's the tests that are related to general purpose bits. If you get a chance to look at them let me know, otherwise I'll try to get to it in the next few days. |
|
Thanks @carlossanlop - I see what you mean about the variable names and test errors. Apologies for those! The test errors appear to be because the general purpose bit flag has the Unicode encoding bit (bit 11) set in the general purpose bit flags. The test is simply to make sure the compression levels roundtrip, so arguably this could be masked away and the test would pass. I'll check the tests over the next few days and submit another review with corrections. |
There was a problem hiding this comment.
Finding the cause took less time than I thought.
By default, .NET 6.0 creates the ZipArchive with UTF8 encoding, which eventually feeds through to setting bit 11 in the general purpose bitflag for each archive entry. PR #87883 corrects this for .NET 8.0, and hasn't been backported to .NET 6.0 - so the values of the general purpose bit flags for a ZIP archive entry will be different between .NET 6.0 and .NET 8.0.
The purpose of the Roundtrip_Compression_Option test only requires us to validate bits 1 & 2, so I've just decided to mask the other bits out. The exception should never be triggered in the .NET 8.0 test because we don't create any files which need the extended encoding support, but I can update it to keep it in sync with the .NET 6.0 test if needed.
The other two review comments are just the result of me checking the tests in Visual Studio rather than eyeballing the code in GitHub...
Co-authored-by: Edward Neal <55035479+edwardneal@users.noreply.github.com>
Co-authored-by: Edward Neal <55035479+edwardneal@users.noreply.github.com>
|
@ViktorHofer @ericstj can I get a review and a sign-off for this servicing fix, please? |
carlossanlop
added
the
Servicing-consider
carlossanlop
removed
the
Servicing-consider
|
Closing while we get clarity on whether this bug's impact is severe enough to justify a backport. |
2 participants
Backport of #98278 to release/6.0-staging
/cc @carlossanlop @edwardneal
Customer Impact
Originally reported in dotnet/Open-XML-SDK.
.NET Framework allows consumers of
System.IO.Packaging.ZipPackageto useCompressionOptionto update bits 1 & 2 in the ZIP archive header's general purpose bitfield. This functionality was lost in .NET Core.OpenXML is affected because when a document like XSLX is created with our APIs, it is not compliant with the OpenXML specification ISO/IEC 29500-2, as the standard bits 1 and 2 are mandatory and are not reflecting the applied zip compression. Full discussions in #88812 and dotnet/Open-XML-SDK#1443
Specifying the
CompressionLevelofSystem.IO.Compression.ZipArchivewill now change these bits, and theZipArchive'sCompressionLevelvalue will be read from the bitfield.Regression
This PR also introduces a breaking change which is reported in detail here: dotnet/docs#40299 . Summarized, the
CompressionOptionvalues fromSystem.IO.Packagingwill map to differentCompressionLevellevels inSystem.IO.Compressiondepending if using .NET Framework or .NET Core.Testing
New tests were added to both
System.IO.CompressionandSystem.IO.Packagingto verify that setting theCompressionLevelsets the expected bits, that they can be re-read correctly and that the file data can still be decompressed correctly.Risk
Low.
Thanks to @edwardneal for fixing the issue and @maedula for the detailed report of this problem.