[release/6.0-staging] Allow setting ZipArchiveEntry general-purpose flag bits #102096
Conversation
…ArchiveEntry general purpose bit flag unicode value is unset. (dotnet#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>
Backport of #98278 to release/6.0-staging
/cc @carlossanlop @edwardneal
Customer Impact
Regression
Testing
Risk