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.

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

Fix reading empty file or seeking past end of file for s3 backend #549

Merged
merged 3 commits into from
Oct 30, 2020

Conversation

jcushman
Copy link
Contributor

Motivation

This PR fixes seeking past the end of an s3 file, which I broke in #495. E.g. this will now work instead of raising a boto error:

with open('s3://some/random/uri', 'rb') as file:
    file.seek(1_000_000_000)

file.tell() will then give the current position as the actual end of the file, and f.read() will return b''.

This also fixes opening an empty file for reading, as reported in #548 -- that turns out to be a special case of seeking past the end of a file being broken.

The reason this wasn't working before is that moto doesn't return the same API response as real S3 when requesting an invalid range with GetObject, as reported here. We were correctly handling moto-style responses but failing to handle real-S3-style responses. So this PR:

  • Updates _SeekableRawReader to expect responses as provided by real S3
  • Mocks _get in the relevant tests to return real-S3-style responses. This mock could be removed when the upstream bug in moto is fixed
  • Adds tests for reading an empty file and seeking past the end of a file

Fixes #548

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

LGTM!

smart_open/tests/test_s3.py Outdated Show resolved Hide resolved
smart_open/tests/test_s3.py Outdated Show resolved Hide resolved
@jcushman
Copy link
Contributor Author

FYI that the moto mock for testing will apparently be unnecessary after the next release of moto -- getmoto/moto#2981

@mpenkov mpenkov merged commit 7f80c37 into piskvorky:develop Oct 30, 2020
@mpenkov
Copy link
Collaborator

mpenkov commented Oct 30, 2020

Merged. Thank you for your contribution and your patience @jcushman !

@Andrew-Sheridan
Copy link

@mpenkov I'm curious, when is the next release is scheduled?

Would be nice to have this updated logic available.

@mpenkov
Copy link
Collaborator

mpenkov commented Nov 18, 2020

I expect to release within a week from now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reading a 0 B file from s3 raises OSError
3 participants