Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate compression method in all ZipFile.Add overloads #421

Merged

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Feb 9, 2020

… to validate the method themselves, and throw consistent exceptions for unsupported methods

Related to the discussions in #333 and some of the issues that occur when adding extra compression methods due to using ZipEntry for compression method validation:

There are currently the following methods in ZipFile to add a new entry and specify a compression method to use:

public void Add(string fileName, CompressionMethod compressionMethod, bool useUnicodeText);
public void Add(string fileName, CompressionMethod compressionMethod);
public void Add(IStaticDataSource dataSource, string entryName, CompressionMethod compressionMethod);
public void Add(IStaticDataSource dataSource, string entryName, CompressionMethod compressionMethod, bool useUnicodeText);
public void Add(IStaticDataSource dataSource, ZipEntry entry);

(there is also one that just takes a ZipEntry which only accepts 0 length entries, so i'm not sure if this applies to that one).

Out of those:

The first two check if the compression method is supported themselves, and throw ArgumentOutOfRangeException if it isn't.

The next two don't check themselves, but use the CompressionMethod property setter on ZipEntry, which throws NotSupportedException for methods it things are unsupported.

The final one does no checks, which ends up relying on the property setter in ZipEntry I think? (could hypothetically be an issue if you read an entry with an unsupported method from an existing file?)

This PR makes two changes:

  1. do explicit validation in the Add functions, and throw ArgumentOutOfRangeException for unsupported methods,

  2. Use a local method to decide what is supported instead of deferring to ZipEntry (it's ZipFile that knows what it supports after all).

This somewhat counts as an API change as it changes the exceptions thrown by a couple of the functions from NotSupportedException to ArgumentOutOfRangeException (i'm not sure which exception is best to use, though as ArgumentOutOfRangeException is the one that is specified in the doc comments and NotSupportedException isn't documented, that seemed the simple one to standardize on)

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@piksel piksel self-requested a review February 9, 2020 21:24
@piksel piksel changed the title Change all the variants of ZipFile.Add that take a compression method… Validate compression method in all ZipFile.Add overloads Feb 9, 2020
@piksel
Copy link
Member

piksel commented Feb 9, 2020

I think NotImplementedException is the one we should use (if that's the case of course). Since both NotSupportedException and Argument*Exceptions indicate that the operation or argument is invalid.
ArgumentOutOfRangeException is also just true on a machine level. An enum is not really a number, it's a token for a commonly understood API concept.

That being said, NotImplementedException is most commonly used when not implementing parts of an interface, so NotSupportedException might still be better. The documentation does not have any scenarios matching ours unfortunately.

@@ -1667,11 +1667,7 @@ public void Add(string fileName, CompressionMethod compressionMethod, bool useUn
throw new ObjectDisposedException("ZipFile");
}

if (!ZipEntry.IsCompressionMethodSupported(compressionMethod))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just reads out as being wrong. Why would ZipFile ask an entry if it supports compressing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it seems inside out.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I am not sure about what to do with the exception, though. But I'm fine with it if you want to stick with it.

@Numpsy
Copy link
Contributor Author

Numpsy commented Feb 9, 2020

NotImplemented sounds better than ArgumentOutOfRange, I can change that if you're ok with changing the documented excdeption types on a couple of functions.

@Numpsy
Copy link
Contributor Author

Numpsy commented Feb 9, 2020

Related: ZipOutputStream.PutNextEntry is in a similar situation with not validating the compression type is supported, and it could perhaps do with throwing the same exception type there.

@piksel
Copy link
Member

piksel commented Feb 10, 2020

Yeah, they should probably use the same exception.

The change in API should not be a problem, since this is not an exception that is usually catched by the consumer and handled. Instead, most people would just see it whenever they attempt to use a feature that is not supported by the library (yet).

@Numpsy Numpsy force-pushed the rw/zipfile/verify_compression_method branch from b1a3d2f to d6ef992 Compare February 11, 2020 21:49
@Numpsy
Copy link
Contributor Author

Numpsy commented Feb 11, 2020

Exception changed to NotImplementedException

… to validate the method themselves, and throw consistent exceptions for unsupported methods
@Numpsy Numpsy force-pushed the rw/zipfile/verify_compression_method branch from d6ef992 to 194db48 Compare March 29, 2020 14:24
@Numpsy
Copy link
Contributor Author

Numpsy commented Mar 29, 2020

rebased on top of #420 to fix a merge conflict

@piksel piksel merged commit fed3bd2 into icsharpcode:master Mar 29, 2020
@Numpsy Numpsy deleted the rw/zipfile/verify_compression_method branch March 29, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants