Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: S3 ignore seek requests to the current position #748

Closed
wants to merge 2 commits into from
Closed

fix: S3 ignore seek requests to the current position #748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3fea83
fix: ignore seek requests to the current position
rustyconover Dec 18, 2022
c5f6954
fix: adjust test to match new seek behavior
rustyconover Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions smart_open/s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,10 @@ def seek(self, offset, whence=constants.WHENCE_START):
whence = constants.WHENCE_START
offset += self._current_pos

self._current_pos = self._raw_reader.seek(offset, whence)
if not (whence == constants.WHENCE_START and offset == self._current_pos):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not (whence == constants.WHENCE_START and offset == self._current_pos):
if not (whence == constants.WHENCE_START and offset == self._current_pos):

The optimization idea is good, but we need to maintain the current behavior of the library. That is, the seek should always be performed when opening the file. Perhaps we could initialize self._current_pos to -1 to achieve this.

self._current_pos = self._raw_reader.seek(offset, whence)
self._buffer.empty()

self._buffer.empty()
self._eof = self._current_pos == self._raw_reader._content_length
return self._current_pos

Expand Down
6 changes: 4 additions & 2 deletions smart_open/tests/test_s3_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ def test_good_id(self):
def test_bad_id(self):
"""Does passing an invalid version_id exception into the s3 submodule get handled correctly?"""
params = {'version_id': 'bad-version-does-not-exist'}
with self.assertRaises(IOError):
open(self.url, 'rb', transport_params=params)
with open(self.url, 'rb', transport_params=params) as fin:
with self.assertRaises(IOError):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not change this test. Opening the URL to a non-existent object should raise an error, before any reading is attempted.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I hadn't changed this in my fork of the code. But then again I wasn't aiming to make a PR. I could check what errors the fix raise during tests and see if there is a sensible way to incorporate them without changing core functionality.

fin.read()


def test_bad_mode(self):
"""Do we correctly handle non-None version when writing?"""
Expand Down