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

s3: Fix newline handling for text-mode files #1352

Closed
wants to merge 5 commits into from

Conversation

craigds
Copy link
Contributor

@craigds craigds commented Feb 13, 2024

This fixes the newline issue at #1351. Files open in text mode (r) now get newline handling consistent with other python file-like objects.

run these specific tests with

tox -e py3.10-django4.2 -- tests/test_s3.py -k newlines

@craigds craigds marked this pull request as ready for review February 14, 2024 20:54
@craigds craigds changed the title S3 fix newlines s3: Fix newline handling for text-mode files Feb 14, 2024
@craigds
Copy link
Contributor Author

craigds commented Feb 14, 2024

Thanks @skim618 for finishing this off

@skim618
Copy link

skim618 commented Feb 14, 2024

There were two possible ways of doing this:

  1. Change the initialization of self._file as a NamedTemporaryFile instead of spooled, so we can reopen the file with the correct mode after the contents have been downloaded.
  2. Wrap the underlying self._file's binary object with TextIOWrapper

The changes in this PR implement (2).

self._force_mode has been removed, as this change will now correctly read the file with the right mode (Hence not needing to decode a binary when given "r"). The relevant test(s) has been moved down to the S3 test class, so we can correctly test the storage.save() and storage.open() and flow and thus test the S3File._get_file method correctly.

@@ -123,7 +124,6 @@ def __init__(self, name, mode, storage, buffer_size=None):
self._storage = storage
self.name = name[len(self._storage.location) :].lstrip("/")
self._mode = mode
self._force_mode = (lambda b: b) if "b" in mode else (lambda b: b.decode())
Copy link
Owner

Choose a reason for hiding this comment

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

Are the changes to _force_mode necessary? It's interesting that there are no failing tests when those are moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the TextIOWrapper takes care of decoding the file as it's read, so this is no longer necessary

@jschneier
Copy link
Owner

This change looks good but tests are failing. Is this due to 3.7 not being dropped yet? Once I release the next version the intent is to drop that.

@craigds
Copy link
Contributor Author

craigds commented Mar 11, 2024

yes, looks like moto dropped 3.7 support: getmoto/moto@2fd5e80

I've fixed the tests by undoing my previous moto upgrade and instead pinning to <5 so the tests will run

@craigds
Copy link
Contributor Author

craigds commented Mar 11, 2024

are those gcloud failures expected? I also get them locally. Seems unrelated to the changes in this PR though

@jschneier
Copy link
Owner

are those gcloud failures expected? I also get them locally. Seems unrelated to the changes in this PR though

Seems like a breaking change related to a google cloud dependency.

I'm moving your moto fix and that to a separate PR so I can unblock everything else. Thanks for flagging.

@jschneier
Copy link
Owner

This actually also exposed a bug in Google Cloud Storage handling of gzip files. I was looking into who wrote the tests in such a confusing way and of course it was...me.

@jschneier
Copy link
Owner

Alright we are back to green, go ahead and rebase. And thanks again.

@jschneier
Copy link
Owner

Moved this to #1381

@jschneier jschneier closed this Apr 21, 2024
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.

None yet

3 participants