zipfile: change RuntimeError to more appropriate exception type

[serhiy-storchaka]

BPO	24693
Nosy	@Yhg1s, @ronaldoussoren, @ned-deily, @vadmium, @serhiy-storchaka
Files	zipfile_exceptions.patch zipfile_exceptions2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-09-10.18:36:56.902>
created_at = <Date 2015-07-23.12:23:44.373>
labels = ['type-feature', 'library']
title = 'zipfile: change RuntimeError to more appropriate exception type'
updated_at = <Date 2016-09-10.18:36:56.901>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-09-10.18:36:56.901>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2016-09-10.18:36:56.902>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2015-07-23.12:23:44.373>
creator = 'serhiy.storchaka'
dependencies = []
files = ['39994', '42853']
hgrepos = []
issue_num = 24693
keywords = ['patch']
message_count = 5.0
messages = ['247189', '265558', '265588', '275668', '275669']
nosy_count = 7.0
nosy_names = ['twouters', 'ronaldoussoren', 'alanmcintyre', 'ned.deily', 'python-dev', 'martin.panter', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue24693'
versions = ['Python 3.6']

[serhiy-storchaka]

RuntimeError is raised in the zipfile module in many cases where more appropriate exception type is expected. Proposed patch changes a number of RuntimeErrors to one of BadZipFile, NotImplementedError, or ValueError. Only changing to NotImplementedError is backward compatible (NotImplementedError is subclass of RuntimeError), other changes are not.

[vadmium]

I have hardly used the zipfile module, but here are my thoughts on some of the exceptions anyway :)

Some of these exceptions are documented, so the documentation would need updating.

BadZipFile for corrupted field seems reasonable.

The purpose of RuntimeError is not clearly documented, but I have the feeling it gets used mainly for programmer errors, rather than errors caused by external data. So I tend to agree a couple of the changes away from RuntimeError, such as the invalid password case.

For programmer errors, I doubt the exception type matters much for compatibility. RuntimeError seems fine to me, but I accept that ValueError is more consistent with open(..., mode="invalid"), operations on closed files, etc.

[serhiy-storchaka]

Updated patch includes changed tests and the documentation. It includes changes for new exceptions added in bpo-26039. Perhaps not all changes should be accepted, but I have included them for demonstrating.

RuntimeError was used in following cases:

1. Compressed or uncompressed file size unexpectedly exceeded 32-bit limit. I think this should left RuntimeError.

2. Compression requires the (missing) zlib/bz2/lzma module. I think this should left RuntimeError.

3. Unsupported compression method. This can be changed to more specific NotImplementedError without breaking compatibility.

4. Corrupted extra field. BadZipFile is more appropriate exception. Not documented.

5. Invalid mode for ZipFile() or ZipFile.open(). ValueError is more appropriate exception, this is programming error. Not documented.

6. File not ending with ".py" for PyZipFile.writepy(). ValueError looks more appropriate exception, but RuntimeError is documented.

7. Write in reading mode, use after closing. ValueError or io.UnsupportedOperation would be more consistent with files, but RuntimeError can be also used in similar cases. RuntimeError is explicitly documented (maybe because it is not typical exception for such cases). Unfortunately there is no special exception type for using an object in wrong state.

8. Read, write or close when there is other open writing handler. These are new exceptions and we are free to change them. There are arguments for using ValueError and RuntimeError in similar cases.

9. Absent or incorrect password for encrypted file. If file encryption is expected, this is programming error and can be ValueError, but if it is not expected, it should be RuntimeError.

I'm going to push changes for cases 3 and 4, this looks safe. Cases 5 and 6 are more discussable. There are reasons for RuntimeError in cases 7, 8 and 9. And I think RuntimeError is appropriate in cases 1 and 2.

[python-dev]

New changeset 22112359abcf by Serhiy Storchaka in branch 'default':
Issue bpo-24693: Changed some RuntimeError's in the zipfile module to more
https://hg.python.org/cpython/rev/22112359abcf

[serhiy-storchaka]

Changed cases 3, 4, 5, 7, and 8.