classification
Title: zipfile: change RuntimeError to more appropriate exception type
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alanmcintyre, martin.panter, ned.deily, python-dev, ronaldoussoren, serhiy.storchaka, twouters
Priority: normal Keywords: patch

Created on 2015-07-23 12:23 by serhiy.storchaka, last changed 2016-09-10 18:36 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
zipfile_exceptions.patch serhiy.storchaka, 2015-07-23 12:23 review
zipfile_exceptions2.patch serhiy.storchaka, 2016-05-15 06:45 review
Messages (5)
msg247189 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-23 12:23
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.
msg265558 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-05-14 23:18
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.
msg265588 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-15 06:45
Updated patch includes changed tests and the documentation. It includes changes for new exceptions added in issue26039. 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.
msg275668 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-10 18:32
New changeset 22112359abcf by Serhiy Storchaka in branch 'default':
Issue #24693: Changed some RuntimeError's in the zipfile module to more
https://hg.python.org/cpython/rev/22112359abcf
msg275669 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-10 18:36
Changed cases 3, 4, 5, 7, and 8.
History
Date User Action Args
2016-09-10 18:36:56serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg275669

stage: patch review -> resolved
2016-09-10 18:32:38python-devsetnosy: + python-dev
messages: + msg275668
2016-05-15 06:45:19serhiy.storchakasetfiles: + zipfile_exceptions2.patch

messages: + msg265588
2016-05-14 23:18:23martin.pantersetnosy: + martin.panter
messages: + msg265558
2015-07-23 12:23:44serhiy.storchakacreate