zipfile and creat/update comment

@mdickinson

BPO	14399
Nosy	@mdickinson, @bitdancer, @ethanfurman, @serhiy-storchaka
Files	bug_zipfile_comment.py fix_zipfile_comment_simple.patch: Simple patch fix_zipfile_comment_complex.patch: No so simple patch fix_zipfile_comment_2.patch fix_zipfile_comment_3.patch fix_zipfile_comment_4.patch: Fix TypeError raising fix_zipfile_comment_4-2.7.patch: For Python 2.7

^{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 2012-04-12.22:47:15.784>
created_at = <Date 2012-03-24.16:27:10.624>
labels = ['easy', 'type-bug', 'library']
title = 'zipfile and creat/update comment'
updated_at = <Date 2015-05-14.12:20:14.337>
user = 'https://bugs.python.org/acassaigne'

bugs.python.org fields:

activity = <Date 2015-05-14.12:20:14.337>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2012-04-12.22:47:15.784>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2012-03-24.16:27:10.624>
creator = 'acassaigne'
dependencies = []
files = ['25011', '25038', '25039', '25043', '25047', '25190', '25191']
hgrepos = []
issue_num = 14399
keywords = ['patch', 'easy']
message_count = 26.0
messages = ['156699', '156793', '156804', '156885', '156886', '156912', '156919', '156925', '156926', '156931', '156944', '156950', '156954', '158094', '158128', '158136', '158182', '158183', '158193', '158196', '158197', '158208', '158244', '158245', '243180', '243184']
nosy_count = 8.0
nosy_names = ['mark.dickinson', 'r.david.murray', 'ethan.furman', 'python-dev', 'Anthony.Kong', 'serhiy.storchaka', 'jeffknupp', 'acassaigne']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14399'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']

I want to update or create a comment to zip file.

For instance, I have test.zip file.
I'm using these statement to create a comment :

from zipfile import ZipFile

z=ZipFile('test.zip','a')
z.comment='Create a new comment'
z.close()

After to ran this script, the zip file test.zip doesn't including the new comment !

I can have the expected behavior when I add a new file inner zip archive.

I'm marking this as easy hoping someone in the mentoring group will pick it up and take a look. The relevant code is in Python, and I'm guessing there is some logic bug when only the comment is set (and nothing is added to the zipfile), but I haven't looked at the code. I'm also adding 3.2 and 3.3; it fails on 3.3 as well.

Tanks à lot. To complete Information about this bug.

it up and take a look. The relevant code is in Python, and I'm guessing
there is some logic bug when only the comment is set (and nothing is added
to the zipfile), but I haven't looked at the code. I'm also adding 3.2 and
3.3; it fails on 3.3 as well.

----------
keywords: +easy
nosy: +r.david.murray
stage: -> needs patch
versions: +Python 3.2, Python 3.3

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue14399\>

I'm unable to reproduce on either 2.7 or 3.3. Running the following:

from zipfile import ZipFile

z=ZipFile('test.zip','a')
z.comment='Create a new comment'
z.close()

produces the output:

Archive: test.zip
Create a new comment

with the comment changed to b'...' for 3.3. Note that in 3.3 if you set the comment to a string and try to close, an exception will be raised and the test.zip file will not have a comment.

Odd that I don't see the bug on either branch. Can someone else confirm?

Hmm. When I did the same (changed to b'...') and ran it, I got an archive test.zip. When I then opened that archive using ZipFile, its comment attribute was b''. Thus I concluded that the bug exists in Python3. I don't remember seeing any output from running the script, and indeed I would not expect to. How did you run it?

Also, I used the uniz unzip command on the produced archive, and got a corrupted archive error message and a warning about an incorrect comment length.

The bug only occurs for non-empty archives.

There are two ways of fixing this bug -- very simple and no so simple. Patches are attached.

Thanks for the patch.

The advantage of the more complicated way being that you get an immediate TypeError instead of a delayed one? Is there any other advantage? (That's probably enough reason, though :)

Now we just need a unit test for this.

This is the second advantage. The first one is that nothing is to be written when zipfile opened in a mode "a", if we don't change anything.

Got you. We'll definitely go with the more complete fix, then.

Fixed patch with tests attached.

The tests don't seem to be included in the new patch. Also, you could modernize the property definition (@Property, @comment.setter).

My fault.

Serhiy: this looks good. I get some test errors when I apply it on 2.7 though. Would you be interested in doing a 2.7 version as well?

(Minor comment: the test method would be better as two test methods, and it would be nice to have a third test method that checked for the TypeError for non-binary comments...that won't apply to 2.7, obviously).

Thank you, the TypeError test helped me find the error. Here is the
corrected patch.

For 2.7 it was necessary to turn the ZipFile in the new-style class.

Thanks.

We've had trouble in the past with a conversion to new style class breaking people's code. People are less likely to be subclassing ZipFile, though, so it is probably OK.

New changeset ac0ec1f31b0a by R David Murray in branch '2.7':
bpo-14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/ac0ec1f31b0a

New changeset 4186f20d9fa4 by R David Murray in branch '3.2':
bpo-14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/4186f20d9fa4

New changeset e5b30d4b0647 by R David Murray in branch 'default':
Merge bpo-14399: zipfile now correctly handles comments added to empty zipfiles.
http://hg.python.org/cpython/rev/e5b30d4b0647

Thanks, Serhiy.

I made one small change, using 'with self.assertEqual' in the TypeError test. You might want to check that out, it is a useful technique.

Oh, and I removed the type check from the 2.7 patch. You can use a unicode string as long as it doesn't contain non-ASCII (the reason we created python3!), so it would be a backward incompatible change to raise a TypeError there.

I mean "with self.assertRaises(TypeError):".

I mean "with self.assertRaises(TypeError):".

Thank you, I did not know that the context managers may catch exceptions
from the nested block.

bpo-14399: zipfile now correctly handles comments added to empty zipfiles.

This is a wrong description of the issue. On the contrary, the error
occurred when adding or changing the comment in a *non-empty* zipfile
without adding or changing files.

Description in the Misc/NEWS also wrong.

Ah. I based that on the fact that the third test passed without the change. I thought you were adding that test of changing the comment just as a double check. I should have asked instead of assuming.

New changeset b3b7f9dd7ce4 by R David Murray in branch '3.2':
bpo-14399: corrected news item
http://hg.python.org/cpython/rev/b3b7f9dd7ce4

New changeset 225126c9d4b5 by R David Murray in branch '2.7':
bpo-14399: corrected news item
http://hg.python.org/cpython/rev/225126c9d4b5

New changeset 160245735299 by R David Murray in branch 'default':
Merge bpo-14399: corrected news item
http://hg.python.org/cpython/rev/160245735299

I must have been seeing what I expected to see. The test that failed was the non-empty test.

News item fixed, thanks for the correction.

Just for the record, David's comment at msg158136 is apposite:

We've had trouble in the past with a conversion to new style class
breaking people's code. People are less likely to be subclassing
ZipFile, though, so it is probably OK.

We just spent some time this morning fixing a bug in some internal software; the cause was precisely this unexpected change from old-style class to new-style class in a bugfix release. And indeed, we were subclassing ZipFile. :-(

:( :(

OK, next time this comes up I won't say "it will probably be OK".

Navigation Menu

Repository navigation

zipfile and creat/update comment #58607

1 remaining item

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

zipfile and creat/update comment #58607

Description

Activity

acassaigne commented on Mar 24, 2012

bitdancer commented on Mar 26, 2012

acassaigne commented on Mar 26, 2012

jeffknupp commented on Mar 27, 2012

bitdancer commented on Mar 27, 2012

serhiy-storchaka commented on Mar 27, 2012

bitdancer commented on Mar 27, 2012

serhiy-storchaka commented on Mar 27, 2012

bitdancer commented on Mar 27, 2012

serhiy-storchaka commented on Mar 27, 2012

bitdancer commented on Mar 27, 2012

serhiy-storchaka commented on Mar 27, 2012

1 remaining item

bitdancer commented on Apr 12, 2012

serhiy-storchaka commented on Apr 12, 2012

bitdancer commented on Apr 12, 2012

python-dev commented on Apr 12, 2012

bitdancer commented on Apr 12, 2012

bitdancer commented on Apr 13, 2012

serhiy-storchaka commented on Apr 13, 2012

serhiy-storchaka commented on Apr 13, 2012

bitdancer commented on Apr 13, 2012

python-dev commented on Apr 14, 2012

bitdancer commented on Apr 14, 2012

mdickinson commented on May 14, 2015

bitdancer commented on May 14, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions