gh-81719: Add private members to zipfileZipFile to make it easier to subclass #137101

adiroiban · 2025-07-25T13:48:21Z

This is the bare minimum changes to allow subclassing ZipFile to implement a custom encryption method.

I am using this code to implement WinZIP AES.

This is my first contribution to CPyton , so I try to keep it minimum.

Maybe the private method should not be documented... but just leave a comment in the code.

I am using this code to implement WinZip AES on top of this patch

There are a few other changes that will make it easier to reuse code... but those are not criticial.

For example, the current code validates the Zip password outside of the _ZipDecrypter code.
This means, that if you implement a differrent ZipDecrypter that has a different password validation method, you can't just rewrite ZipExtFile._init_decrypter() and reuse ZipExtFile.__init__

Let me know if you are happy for a bit more of refactoring

Thanks

Issue: Refactor zipfile to ease subclassing and enhancement #81719

python-cla-bot · 2025-07-25T13:48:25Z

All commit authors signed the Contributor License Agreement.

Lib/zipfile/__init__.py

bedevere-app · 2025-07-25T14:17:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

adiroiban

Thanks for the review.

I have updated the code.

I was thinking that we can keep these attributes private, at least as a start, while downstream projects try to extend the code.

Once we have more projects using the code, we can look into making this public and defining a public interface.

Lib/zipfile/__init__.py

adiroiban · 2025-07-25T15:28:39Z

I have made the requested changes; please review again

bedevere-app · 2025-07-25T15:28:44Z

Thanks for making the requested changes!

@ZeroIntensity: please review the changes made to this pull request.

ZeroIntensity

Ok, now please add a test case. If you're confused about that in any way, feel free to ask me or take a look at the devguide. We should also add an entry to the "What's New in Python 3.15" page.

Once we have more projects using the code, we can look into making this public and defining a public interface.

Well, we don't really want people to be using a private interface. If we encourage people to use it (i.e., by documenting it), then we still have to follow all the usual backwards compatibility rules, even if it's "private". If we don't document it, then people won't really know to use it.

Misc/NEWS.d/next/Library/2025-07-25-14-27-29.gh-issue-81719.hWp7Mn.rst

Lib/zipfile/__init__.py

adiroiban · 2025-07-25T15:50:36Z

Thanks. I will clean the code and add automated tests.

adiroiban · 2025-07-27T12:22:36Z

Thanks Peter for the review.

Docs was updated here https://cpython-previews--137101.org.readthedocs.build/en/137101/whatsnew/3.15.html#zipfile

I now see that we don't have auto-doc enabled for zipfile

https://cpython-previews--137101.org.readthedocs.build/en/137101/library/zipfile.html#zipfile.ZipFile

Should I update the documentation page at Doc/library/zipfile.rst?

The current changes are targeted to develpers that want to extend the ZipFile code,
and not to developers that want to use the code.

So, if you want to extend the code for ZipFile, you will have to read the source code.

So, maybe leave Doc/library/zipfile.rst as it is, since it is targeted to "end users"

Have zipinfo_class documentated only inside the source code.

My bad for forgetting about the tests.

I tried to write the minimum amount of test to cover all the new code.

./python -m pytest Lib/test/test_zipfile/ --cov zipfile --cov-report=xml
./python -m diff_cover.diff_cover_tool coverage.xml 
-------------
Diff Coverage
Diff: origin/main...HEAD, staged and unstaged changes
-------------
Lib/zipfile/__init__.py (100%)
-------------
Total:   12 lines
Missing: 0 lines
Coverage: 100%
-------------

Let me know if you prefer to split the current tests into separate test for each use case.
I tried to split them based on the main functionality: read, write and extract

I have made the requested changes; please review again

bedevere-app · 2025-07-27T12:22:39Z

Thanks for making the requested changes!

@ZeroIntensity: please review the changes made to this pull request.

ZeroIntensity · 2025-07-27T13:37:39Z

Lib/test/test_zipfile/test_core.py

+    """
+    Test in which the files inside the archive are not compressed.
+    """


This docstring isn't needed.

OK. I have removed it. No problem.

I had to read the other tests to decide where I should add the new tests and I thought that this comment can help.

ZeroIntensity · 2025-07-27T13:38:32Z

Lib/test/test_zipfile/test_core.py

@@ -676,6 +680,104 @@ def test_add_file_after_2107(self):
            self.assertEqual(zinfo.date_time, (2107, 12, 31, 23, 59, 59))


+    class CustomZipInfo(zipfile.ZipInfo):
+        """


Same here; please just avoid adding docstrings. This is all private code and keeping them up to date tends to be extra maintenance that we don't want.

The idea is to document why this class was use and decide if you can reuse it in another test.

I have removed the docstring.

ZeroIntensity · 2025-07-27T13:41:49Z

Lib/test/test_zipfile/test_core.py

+        destination = io.BytesIO()
+        with zipfile.ZipFile(
+                destination, 'w', zipinfo_class=self.CustomZipInfo) as zipfp:
+            # It can write using the specific custom classe.


Typo:

Suggested change

# It can write using the specific custom classe.

# It can write using the specific custom class.

ZeroIntensity · 2025-07-27T13:43:10Z

Lib/test/test_zipfile/test_core.py

+            # When creating a new member using just the name,
+            # the custom ZipInfo is used internally.
+            with zipfp.open('other-member.txt', mode='w') as memberfp:
+                self.assertIsInstance(memberfp._zinfo, self.CustomZipInfo)


Please avoid using private members in tests. If we change how they work, it adds additional maintenance.

Thanks. Make sense.

ZeroIntensity · 2025-07-27T13:46:34Z

Lib/test/test_zipfile/test_core.py

+            # When writing from an external file, the file is created using
+            # the custom ZipInfo
+            with temp_dir() as source_dir:
+                source_file = pathlib.Path(source_dir).joinpath('source.txt')


You should be able to just use Path(source_dir) / 'source.txt' here.

ZeroIntensity · 2025-07-27T13:48:08Z

Lib/test/test_zipfile/test_core.py

+            with temp_dir() as extract_dir:
+                zipfp.extract(dir_info, path=extract_dir)
+                zipfp.extract('dir-as-text/', path=extract_dir)


I'm not sure this part of the test is necessary. We aren't stressing anything on the custom classes.

This is to test this change

def _extract_member(self, member, targetpath, pwd): """Extract the ZipInfo object 'member' to a physical file on the path targetpath. """ - if not isinstance(member, ZipInfo): + if not isinstance(member, self._ZipInfo): member = self.getinfo(member)

There is not much public API to check for this method.

I have updated the assertions as an end to end test.

ZeroIntensity · 2025-07-27T13:51:25Z

Should I update the documentation page at Doc/library/zipfile.rst?

Sorry, forgot to mention this in the review. Yeah, please update the documentation. Docstrings are generally very broad and can go out of date quickly.

adiroiban

Thanks for the review.

I will wait for the test to pass and then I will request a new review.

I tried to add some "narrative" documentation.

https://cpython-previews--137101.org.readthedocs.build/en/137101/library/zipfile.html#zipfile-objects

I find it hard, as there is a lot of private API

For example in ZipFile.writestr we have

            zinfo = self._ZipInfo(zinfo_or_arcname)._for_archive(self)

Maybe as a followup, we should refactor to use a public method and document the ZipInfo and ZipExtFile public API.

I tried to add tests to cover all changes... but the tests are kind of useless with class CustomZipInfo(zipfile.ZipInfo): as CustomZipInfo will return true for both isinstance(obj, ZipInfo) or isinstance(obj, self._ZipInfo)

I tried to write the tests using delegation for ZipInfo...but I was not to write sometime useful

I got something like this...but I a not sure if this will not make it hard to maintain the tests

class CustomZipInfo:
    """
    Support for testing extending and subclassing ZipFile.
    """
    def __init__(self, *args, **kwargs):
        self._delegated = zipfile.ZipInfo(*args, **kwargs)

    def _for_archive(self, archive):
        self._delegated._for_archive(archive)
        return self

    def __getattribute__(self, name):
        if name in ['_delegated', '_for_archive']:
            return object.__getattribute__(self, name)

        return getattr(self._delegated, name)

    def __setattribute__(self, name, value):
        if name == '_delegated':
            object.__setattribute__(self, name, value)
            return

        return setattr(self._delegated, name, value)


    @classmethod
    def from_file(cls, filename, arcname=None, *, strict_timestamps=True):
        if isinstance(filename, os.PathLike):
            filename = os.fspath(filename)
        st = os.stat(filename)
        isdir = stat.S_ISDIR(st.st_mode)
        mtime = time.localtime(st.st_mtime)
        date_time = mtime[0:6]
        if not strict_timestamps and date_time[0] < 1980:
            date_time = (1980, 1, 1, 0, 0, 0)
        elif not strict_timestamps and date_time[0] > 2107:
            date_time = (2107, 12, 31, 23, 59, 59)
        # Create ZipInfo instance to store file information
        if arcname is None:
            arcname = filename
        arcname = os.path.normpath(os.path.splitdrive(arcname)[1])
        while arcname[0] in (os.sep, os.altsep):
            arcname = arcname[1:]
        if isdir:
            arcname += '/'
        zinfo = cls(arcname, date_time)
        zinfo.external_attr = (st.st_mode & 0xFFFF) << 16  # Unix attributes
        if isdir:
            zinfo.file_size = 0
            zinfo.external_attr |= 0x10  # MS-DOS directory flag
        else:
            zinfo.file_size = st.st_size

        return zinfo

adiroiban · 2025-07-28T12:43:25Z

Lib/test/test_zipfile/test_core.py

+            with temp_dir() as extract_dir:
+                zipfp.extract(dir_info, path=extract_dir)
+                zipfp.extract('dir-as-text/', path=extract_dir)


This is to test this change

def _extract_member(self, member, targetpath, pwd): """Extract the ZipInfo object 'member' to a physical file on the path targetpath. """ - if not isinstance(member, ZipInfo): + if not isinstance(member, self._ZipInfo): member = self.getinfo(member)

There is not much public API to check for this method.

I have updated the assertions as an end to end test.

adiroiban · 2025-07-28T12:47:53Z

Lib/test/test_zipfile/test_core.py

+            # When creating a new member using just the name,
+            # the custom ZipInfo is used internally.
+            with zipfp.open('other-member.txt', mode='w') as memberfp:
+                self.assertIsInstance(memberfp._zinfo, self.CustomZipInfo)


Thanks. Make sense.

adiroiban · 2025-07-28T12:49:04Z

Lib/test/test_zipfile/test_core.py

@@ -676,6 +680,104 @@ def test_add_file_after_2107(self):
            self.assertEqual(zinfo.date_time, (2107, 12, 31, 23, 59, 59))


+    class CustomZipInfo(zipfile.ZipInfo):
+        """


The idea is to document why this class was use and decide if you can reuse it in another test.

I have removed the docstring.

adiroiban · 2025-07-28T12:52:19Z

Lib/test/test_zipfile/test_core.py

+    """
+    Test in which the files inside the archive are not compressed.
+    """


OK. I have removed it. No problem.

I had to read the other tests to decide where I should add the new tests and I thought that this comment can help.

adiroiban · 2025-07-28T14:14:16Z

I have made the requested changes; please review again

bedevere-app · 2025-07-28T14:14:19Z

Thanks for making the requested changes!

@ZeroIntensity: please review the changes made to this pull request.

Doc/library/zipfile.rst

Doc/whatsnew/3.15.rst

Misc/NEWS.d/next/Library/2025-07-25-14-27-29.gh-issue-81719.hWp7Mn.rst

adiroiban · 2025-07-28T15:04:02Z

Feedback @serhiy-storchaka from from the issue discussion #136741 (comment)

This looks reasonable.

Is is fine to have class variable or methods purposed for overriding in a subclass. Constructor argument is not needed -- you cannot make ZipFile supporting AES encryption by just passing a custom ZipInfo subclass. This is not what users want to do. A ZipFile subclass which implements all necessary hooks would be more convenient. Of course, you cannot combine several ZipFile subclasses which provide different extended functions, but I don't think this is needed. And it will be easier to include support of AES encryption in the base class.

But I have several general notes:

The isinstance() checks should check against ZipInfo. Basic ZipInfo should still be accepted in these functions. Creation of new ZipInfo objects can be customized.

Instead of self._ZipInfo.from_file(), call a special ZipFile method. This is more flexible.

Instead of _ZipInfo, _ZipExtFile and _ZipWriteFile use methods with more meaningful name and maybe with better designed signature. For example, ZipExtFile() is always called with mode='rb' and close_fileobj=True. They should not be parameters of the method that creates ZipExtFile.

adiroiban · 2025-07-28T15:11:54Z

@serhiy-storchaka thanks for your feedback.

The __init__ arguments were requested by Peter.

I think that for this PR the focus is whether we want public init arguments or private class variables

Regarding

. For example, ZipExtFile() is always called with mode='rb' and close_fileobj=True. They should not be parameters of the method that creates ZipExtFile.

This looks like a refactoring that should be done in a separate issue/PR.

I have also mentioned usage of private API in ZipFile.writestr

            zinfo = self._ZipInfo(zinfo_or_arcname)._for_archive(self)

ZeroIntensity · 2025-07-28T15:18:29Z

I really don't want to encourage using private class or instance variables to expose use of ZipInfo or ZipExtFile. I think that more fine-grained methods are a good approach too, they're just more work for users here (compared to dropping in their own class in a constructor).

brianschubert · 2025-07-28T15:23:56Z

(side note: it looks like there's a typo in the email address that was configured for your last few commits, which is preventing GitHub from associating them with your account. I think there's a missing n at the end?)

bedevere-app bot added the awaiting review label Jul 25, 2025

bedevere-app bot mentioned this pull request Jul 25, 2025

Refactor zipfile to ease subclassing and enhancement #81719

Open

Add private members via init to make it easier to subclass.

54b7e31

adiroiban force-pushed the 81719-zipfile-refactor-for-subclass branch from dd4fbc8 to 54b7e31 Compare July 25, 2025 13:51

adiroiban mentioned this pull request Jul 25, 2025

Make it easier to extend zipfile code #136741

Open

ZeroIntensity requested changes Jul 25, 2025

View reviewed changes

Lib/zipfile/__init__.py Outdated Show resolved Hide resolved

bedevere-app bot added awaiting changes and removed awaiting review labels Jul 25, 2025

adiroiban commented Jul 25, 2025

View reviewed changes

Lib/zipfile/__init__.py Outdated Show resolved Hide resolved

Lib/zipfile/__init__.py Outdated Show resolved Hide resolved

Lib/zipfile/__init__.py Outdated Show resolved Hide resolved

Lib/zipfile/__init__.py Outdated Show resolved Hide resolved

Apply suggestions from code review

609d44f

adiroiban requested a review from ZeroIntensity July 25, 2025 15:24

bedevere-app bot added awaiting change review and removed awaiting changes labels Jul 25, 2025

ZeroIntensity reviewed Jul 25, 2025

View reviewed changes

Misc/NEWS.d/next/Library/2025-07-25-14-27-29.gh-issue-81719.hWp7Mn.rst Outdated Show resolved Hide resolved

Lib/zipfile/__init__.py Outdated Show resolved Hide resolved

Lib/zipfile/__init__.py Outdated Show resolved Hide resolved

Adi Roiban added 3 commits July 27, 2025 12:58

Update after review.

a6d22b7

Add tests.

36defbc

Add what's new and fix sphinx lint.

a3be112

bedevere-app bot requested a review from ZeroIntensity July 27, 2025 12:22

Fix docstring.

2c998fb

ZeroIntensity reviewed Jul 27, 2025

View reviewed changes

Adi Roiban added 2 commits July 28, 2025 13:53

Update tests

63856e8

Add documentation

d7baea1

adiroiban commented Jul 28, 2025

View reviewed changes

bedevere-app bot requested a review from ZeroIntensity July 28, 2025 14:14

brianschubert reviewed Jul 28, 2025

View reviewed changes

Doc/library/zipfile.rst Outdated Show resolved Hide resolved

Doc/library/zipfile.rst Outdated Show resolved Hide resolved

Doc/whatsnew/3.15.rst Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2025-07-25-14-27-29.gh-issue-81719.hWp7Mn.rst Outdated Show resolved Hide resolved

Adi Roiban added 2 commits July 28, 2025 16:14

Update after review from Brian

83e7979

Another feedback from Brian.

7ac534a

	# It can write using the specific custom classe.
	# It can write using the specific custom class.

Uh oh!

gh-81719: Add private members to zipfileZipFile to make it easier to subclass #137101

Are you sure you want to change the base?

gh-81719: Add private members to zipfileZipFile to make it easier to subclass #137101

Conversation

adiroiban commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

python-cla-bot bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bedevere-app bot commented Jul 25, 2025

Uh oh!

adiroiban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adiroiban commented Jul 25, 2025

Uh oh!

bedevere-app bot commented Jul 25, 2025

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adiroiban commented Jul 25, 2025

Uh oh!

adiroiban commented Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Jul 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity commented Jul 27, 2025

Uh oh!

adiroiban left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adiroiban commented Jul 28, 2025

Uh oh!

bedevere-app bot commented Jul 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adiroiban commented Jul 25, 2025 •

edited

Loading

python-cla-bot bot commented Jul 25, 2025 •

edited

Loading

adiroiban commented Jul 27, 2025 •

edited

Loading

adiroiban left a comment •

edited

Loading