GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

5ec1cff · 2024-12-21T01:55:21Z

Issue: random access uncompressed unencrypted ZipExtFile #128131

bedevere-app · 2024-12-21T01:55:26Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

picnixz · 2024-12-21T01:56:27Z

Closing as a duplicate of #128132. Please do not copy PRs made by others. Thanks.

EDIT: It appears that this is either the same person or two people collaborating with each other. Anyway, please do not update two identical PRs. Thanks

5ec1cff · 2024-12-21T03:39:17Z

Closing as a duplicate of #128132. Please do not copy PRs made by others. Thanks.

EDIT: It appears that this is either the same person or two people collaborating with each other. Anyway, please do not update two identical PRs. Thanks

We discussed about this issue and decide that I submit the pr. vvb2060 has already closed his pr. Please reopen my pr, sorry for inconvenience.

picnixz

Can we have tests for that please?

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst

5ec1cff · 2024-12-21T14:42:53Z

Can we have tests for that please?

Added.

picnixz

I haven't looked at the actual implementation until the tests are (hopefully) simpler.

Lib/test/test_zipfile/test_core.py

…pPmNt.rst Co-authored-by: Bénédikt Tran <[email protected]>

add unittest

Lib/test/test_zipfile/test_core.py

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst

picnixz

Please address this comment: https://github.com/python/cpython/pull/128143/files#r1894920324.

Lib/test/test_zipfile/test_core.py

picnixz

The question is: should we treat this as a bug or a feature? it's half-half but I'm not a maintainer of this module so I don't really know. @jaraco WDYT?

Lib/test/test_zipfile/test_core.py

The PR addressed the requested changes.

Co-authored-by: Bénédikt Tran <[email protected]>

picnixz

Some nits and otherwise I'd like to wait for @jaraco's review (but since we're on holidays, this can take some time)

Lib/test/test_zipfile/test_core.py

danifus · 2024-12-26T11:09:51Z

Thanks for adding the comments. I'm happy with the PR once you've added @picnixz's suggestions.

picnixz

Looks fine to me but I'll wait for Jason's review.

jaraco

I still don't have a good understanding of what the problem is, who cares, and thus what solution or solutions should be considered. Please start by addressing my comment in the main issue.

jaraco · 2024-12-27T19:32:57Z

Lib/test/test_zipfile/test_core.py

+
+
+class StoredZipExtFileRandomReadTest(unittest.TestCase):
+    def test_random_read(self):


Please add a docstring describing what this function is testing. Give an overview of what the problem is and what is being guaranteed by the test. Also, link to the reported issue, where the problem should be described in exquisite detail.

jaraco · 2024-12-27T19:34:06Z

Lib/test/test_zipfile/test_core.py

+                self.assertEqual(fp.tell(), 10102)
+                self.assertEqual(arr, txt[10002:10102])
+                self.assertEqual(fp._left, fp._compress_left)
+                d = sio.bytes_read - old_count


Please use a better name for d that indicates its meaning, or just inline it if it's only needed once and its meaning is inconsequential.

jaraco · 2024-12-27T19:37:06Z

Lib/test/test_zipfile/test_core.py

+        # The seek length must be greater than ZipExtFile.MIN_READ_SIZE
+        # as `ZipExtFile._read2()` reads in blocks of this size and we
+        # need to seek out of the buffered data
+        min_size = zipfile.ZipExtFile.MIN_READ_SIZE


Maybe call this variable "seek_length", since that's what's described in the preceding comment.

jaraco · 2024-12-27T19:40:31Z

Lib/test/test_zipfile/test_core.py

+                # eof flags test
+                fp.seek(0, os.SEEK_END)
+                self.assertTrue(fp._eof)
+                fp.seek(12345, os.SEEK_SET)
+                self.assertFalse(fp._eof)


This feels like it should be a separate test (or two). In fact, I'm not even sure I understand why this private flag is even relevant to the issue at hand. If it's not a separate test with a separate justification, can you explain why it's related to the issue at hand? If these are private attributes, what is the public effect that's being validated?

This test is to ensure that the eof flag is correctly updated after seeking to the end of the file and then seeking back.

jaraco · 2024-12-27T19:55:33Z

Lib/test/test_zipfile/test_core.py

+
+
+class StoredZipExtFileRandomReadTest(unittest.TestCase):
+    def test_random_read(self):


The name of this function seems too broad. It's not simply testing a random read, but it's also testing the case where it's following an optimized path for an uncompressed file. Consider test_stored_seek.

jaraco · 2024-12-27T20:08:16Z

Lib/test/test_zipfile/test_core.py

+        self.assertGreaterEqual(10002, min_size)  # for forward seek test
+        self.assertGreaterEqual(5003, min_size)  # for backward seek test


Let's set variables for these constants.

Suggested change

self.assertGreaterEqual(10002, min_size) # for forward seek test

self.assertGreaterEqual(5003, min_size) # for backward seek test

forward_seek = 10002

backward_seek = 5003

self.assertGreaterEqual(forward_seek, min_size)

self.assertGreaterEqual(backward_seek, min_size)

jaraco · 2024-12-27T20:09:15Z

Lib/test/test_zipfile/test_core.py

+                arr = fp.read(100)
+                self.assertEqual(fp.tell(), 10102)
+                self.assertEqual(arr, txt[10002:10102])


Re-use the variables defined above.

Suggested change

arr = fp.read(100)

self.assertEqual(fp.tell(), 10102)

self.assertEqual(arr, txt[10002:10102])

arr = fp.read(read_length)

self.assertEqual(fp.tell(), forward_seek + read_length)

self.assertEqual(arr, txt[forward_seek:forward_seek + read_length])

jaraco · 2024-12-27T20:09:44Z

Lib/test/test_zipfile/test_core.py

+
+                # backward seek
+                old_count = sio.bytes_read
+                fp.seek(-5003, os.SEEK_CUR)


Suggested change

fp.seek(-5003, os.SEEK_CUR)

fp.seek(-backward_seek, os.SEEK_CUR)

jaraco · 2024-12-27T20:21:57Z

Lib/test/test_zipfile/test_core.py

+                self.assertEqual(fp.tell(), 5099)  # 5099 = 10102 - 5003
+                self.assertEqual(fp._left, fp._compress_left)
+                arr = fp.read(100)
+                self.assertEqual(fp.tell(), 5199)
+                self.assertEqual(arr, txt[5099:5199])


Suggested change

self.assertEqual(fp.tell(), 5099) # 5099 = 10102 - 5003

self.assertEqual(fp._left, fp._compress_left)

arr = fp.read(100)

self.assertEqual(fp.tell(), 5199)

self.assertEqual(arr, txt[5099:5199])

self.assertEqual(fp.tell(), forward_seek - backward_seek + read_length)

self.assertEqual(fp._left, fp._compress_left)

arr = fp.read(read_length)

backward_pos = forward_seek - backward_seek + read_length

self.assertEqual(fp.tell(), backward_pos + read_length)

self.assertEqual(arr, txt[backward_pos:backward_pos + read_length])

jaraco · 2024-12-27T20:30:21Z

Lib/zipfile/__init__.py

@@ -1162,13 +1162,15 @@ def seek(self, offset, whence=os.SEEK_SET):
            self._offset = buff_offset
            read_offset = 0
        # Fast seek uncompressed unencrypted file
-        elif self._compress_type == ZIP_STORED and self._decrypter is None and read_offset > 0:
+        elif self._compress_type == ZIP_STORED and self._decrypter is None:


I'm uneasy about this change, especially because the next block is elif read_offset < 0. This change affects both read_offset == 0 and read_offset < 0. Is that what you intended? I get the feeling what you're really aiming to address is the condition where:

Suggested change

elif self._compress_type == ZIP_STORED and self._decrypter is None:

elif self._compress_type == ZIP_STORED and self._decrypter is None and read_offset >= 0:

This is why it's so important to provide a detailed description of the problem, how you discovered it, and what cases it affects.

No, here we consider the case of read_offset < 0, and should exclude the case of read_offset = 0

bedevere-app · 2024-12-27T20:31:20Z

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.

5ec1cff · 2025-01-02T03:37:40Z

I have made the requested changes; please review again

bedevere-app · 2025-01-02T03:37:45Z

Thanks for making the requested changes!

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

bedevere-app bot mentioned this pull request Dec 21, 2024

random access uncompressed unencrypted ZipExtFile #128131

Open

bedevere-app bot added the awaiting review label Dec 21, 2024

picnixz closed this Dec 21, 2024

5ec1cff deleted the 5ec1cff-patch-1 branch December 21, 2024 01:59

5ec1cff restored the 5ec1cff-patch-1 branch December 21, 2024 03:19

picnixz reopened this Dec 21, 2024

picnixz reviewed Dec 21, 2024

View reviewed changes

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst Outdated Show resolved Hide resolved

5ec1cff changed the title ~~GH-128131: Support forward seek in uncompressed unencrypted ZipExtFile~~ GH-128131: Support fast forward seek in uncompressed unencrypted ZipExtFile Dec 21, 2024

picnixz reviewed Dec 21, 2024

View reviewed changes

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

5ec1cff and others added 5 commits December 22, 2024 11:18

Random access uncompressed unencrypted ZipExtFile

f18239d

📜🤖 Added by blurb_it.

f90340f

Update Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.Q…

5d23be6

…pPmNt.rst Co-authored-by: Bénédikt Tran <[email protected]>

Update test_core.py

1a64610

add unittest

update unittest

57cb51c

5ec1cff force-pushed the 5ec1cff-patch-1 branch from 2c34891 to 57cb51c Compare December 22, 2024 03:19

5ec1cff requested a review from picnixz December 22, 2024 04:23

picnixz reviewed Dec 22, 2024

View reviewed changes

update unittest

4d9cea4

5ec1cff changed the title ~~GH-128131: Support fast forward seek in uncompressed unencrypted ZipExtFile~~ GH-128131: Completely support random access of uncompressed unencrypted ZipExtFile Dec 22, 2024

picnixz reviewed Dec 22, 2024

View reviewed changes

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst Outdated Show resolved Hide resolved

update unittest

3d37f31

5ec1cff changed the title ~~GH-128131: Completely support random access of uncompressed unencrypted ZipExtFile~~ GH-128131: Completely support random access of uncompressed unencrypted files inside ZipFile Dec 22, 2024

5ec1cff changed the title ~~GH-128131: Completely support random access of uncompressed unencrypted files inside ZipFile~~ GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile Dec 22, 2024

5ec1cff requested a review from picnixz December 22, 2024 14:59

picnixz previously requested changes Dec 22, 2024

View reviewed changes

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

bedevere-app bot removed the awaiting review label Dec 22, 2024

add comments

25f0a7e

5ec1cff requested a review from danifus December 24, 2024 06:17

picnixz reviewed Dec 24, 2024

View reviewed changes

bedevere-app bot added awaiting review and removed awaiting core review labels Dec 24, 2024

5ec1cff and others added 2 commits December 25, 2024 10:56

Apply suggestions from code review

f2f2374

Co-authored-by: Bénédikt Tran <[email protected]>

make StatIO non-local

a2c9037

5ec1cff requested a review from picnixz December 25, 2024 04:08

picnixz reviewed Dec 25, 2024

View reviewed changes

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

Lib/test/test_zipfile/test_core.py Show resolved Hide resolved

Lib/test/test_zipfile/test_core.py Show resolved Hide resolved

add suggested changes

6a55aad

5ec1cff requested a review from picnixz December 26, 2024 14:52

picnixz approved these changes Dec 26, 2024

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Dec 26, 2024

picnixz requested review from jaraco and removed request for danifus December 26, 2024 17:09

jaraco requested changes Dec 27, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting core review labels Dec 27, 2024

5ec1cff force-pushed the 5ec1cff-patch-1 branch from e004281 to 806b1a5 Compare January 2, 2025 02:49

update

1239005

5ec1cff force-pushed the 5ec1cff-patch-1 branch from 806b1a5 to 1239005 Compare January 2, 2025 03:04

bedevere-app bot added awaiting change review and removed awaiting changes labels Jan 2, 2025

bedevere-app bot requested a review from jaraco January 2, 2025 03:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

5ec1cff commented Dec 21, 2024 •

edited by bedevere-app bot

Loading

bedevere-app bot commented Dec 21, 2024

picnixz commented Dec 21, 2024 •

edited

Loading

5ec1cff commented Dec 21, 2024 •

edited

Loading

picnixz left a comment

5ec1cff commented Dec 21, 2024

picnixz left a comment

picnixz left a comment

picnixz left a comment

picnixz left a comment

danifus commented Dec 26, 2024

picnixz left a comment

jaraco left a comment

jaraco Dec 27, 2024

jaraco Dec 27, 2024

jaraco Dec 27, 2024

jaraco Dec 27, 2024

5ec1cff Dec 30, 2024 •

edited

Loading

jaraco Dec 27, 2024

jaraco Dec 27, 2024

jaraco Dec 27, 2024

jaraco Dec 27, 2024

jaraco Dec 27, 2024

jaraco Dec 27, 2024

5ec1cff Dec 30, 2024

bedevere-app bot commented Dec 27, 2024

5ec1cff commented Jan 2, 2025

bedevere-app bot commented Jan 2, 2025



		class StoredZipExtFileRandomReadTest(unittest.TestCase):
		def test_random_read(self):

		self.assertGreaterEqual(10002, min_size) # for forward seek test
		self.assertGreaterEqual(5003, min_size) # for backward seek test

-        self.assertGreaterEqual(10002, min_size)  # for forward seek test
-        self.assertGreaterEqual(5003, min_size)  # for backward seek test
+        forward_seek = 10002
+        backward_seek = 5003
+        self.assertGreaterEqual(forward_seek, min_size)
+        self.assertGreaterEqual(backward_seek, min_size)

	fp.seek(-5003, os.SEEK_CUR)
	fp.seek(-backward_seek, os.SEEK_CUR)

	elif self._compress_type == ZIP_STORED and self._decrypter is None:
	elif self._compress_type == ZIP_STORED and self._decrypter is None and read_offset >= 0:

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

Are you sure you want to change the base?

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

Conversation

5ec1cff commented Dec 21, 2024 • edited by bedevere-app bot Loading

bedevere-app bot commented Dec 21, 2024

picnixz commented Dec 21, 2024 • edited Loading

5ec1cff commented Dec 21, 2024 • edited Loading

picnixz left a comment

Choose a reason for hiding this comment

5ec1cff commented Dec 21, 2024

picnixz left a comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

danifus commented Dec 26, 2024

picnixz left a comment

Choose a reason for hiding this comment

jaraco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5ec1cff Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Dec 27, 2024

5ec1cff commented Jan 2, 2025

bedevere-app bot commented Jan 2, 2025

5ec1cff commented Dec 21, 2024 •

edited by bedevere-app bot

Loading

picnixz commented Dec 21, 2024 •

edited

Loading

5ec1cff commented Dec 21, 2024 •

edited

Loading

5ec1cff Dec 30, 2024 •

edited

Loading