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 Issue #708: Save files with string content #911

Merged

Conversation

LincolnPuzey
Copy link
Contributor

@LincolnPuzey LincolnPuzey commented Jul 27, 2020

Wraps file objects open in string mode so they produce bytes and can be passed to obj.upload_fileobj()

@LincolnPuzey LincolnPuzey force-pushed the fix_708_save_string_content branch 2 times, most recently from cdf3180 to 0c64612 Compare July 27, 2020 08:12
@LincolnPuzey LincolnPuzey marked this pull request as ready for review July 27, 2020 08:24
@LincolnPuzey
Copy link
Contributor Author

This fixes #708, #843.

@jschneier Can I get a review on this?

You can see my new tests that reproduce the issue, fail in the build on commit c3d00d9.

The latest build is just flake8 failing which is also happening on master.

@@ -440,6 +440,10 @@ def _save(self, name, content):
name = self._normalize_name(cleaned_name)
params = self._get_write_parameters(name, content)

# wrap content so read() always returns bytes. This is required for passing it
# to obj.upload_fileobj() or self._compress_content()
content = ReadBytesWrapper(content)
Copy link
Contributor Author

@LincolnPuzey LincolnPuzey Jul 27, 2020

Choose a reason for hiding this comment

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

This is the core part of the fix. This approach has the benefit of not making any extra read() or seek() calls to the file object.

@LincolnPuzey LincolnPuzey force-pushed the fix_708_save_string_content branch 2 times, most recently from 641cbfd to ca9c466 Compare September 19, 2020 05:37
@LincolnPuzey
Copy link
Contributor Author

@jschneier Can I get this reviewed? Most of the diff is in tests. The new tests in S3Boto3StorageTestsWithMoto reproduce the original error and show its fixed.

@LincolnPuzey
Copy link
Contributor Author

@jschneier Still waiting for a review

@LincolnPuzey
Copy link
Contributor Author

Also fixes #961

@wiktr
Copy link

wiktr commented Jan 17, 2021

@LincolnPuzey I see there are conflicts, could you please resolve them?
@jschneier please take a look at your earliest convenience. I think it's worth resolving this and related tickets.

I hit the described issue using versions:

boto3==1.16.56
botocore==1.19.56
django-storages==1.11.1

Would be lovely if django storages worked with s3 and boto3 out of the box, without workarounds.

@LincolnPuzey
Copy link
Contributor Author

I'm happy to resolve the conflicts if @jschneier or another maintainer can indicate they're willing to review and merge the PR.

@pauloxnet
Copy link
Contributor

Thanks for the PR , it fix a problem we have in our projects.
I can image all maintainers are busy, but I hope one of them can review this PR and then merge.

@dvf
Copy link

dvf commented Jul 19, 2021

Any news here folks? The docs use an example that breaks too:

image

@mertcangokgoz
Copy link

Any update?

@hugo-reveni
Copy link

@LincolnPuzey here is another one interested in this topic, as we're having problems with text files uploaded from Admin using django-import-export v3.2.0

Any merge prevision?

LincolnPuzey and others added 6 commits September 4, 2023 12:26
This makes .read() always return bytes.
If .read() returns a string, it will be encoded to bytes before being returned.
The encoding to use can be specified, otherwise will use the .encoding property of the original file, otherwise will use utf-8.
…per.

This correctly handles file-like-objects that are open in text/string mode by converting to strings returned by read() to bytes, which is what upload_fileobj() requires.
This is done before gzip compressing, so also removed force_bytes() call in _compress_content().
Add these tests in a new test class that uses moto. Remove old test for saving ContentFile
Move test for detecting content-type to this new class. Add some more tests around this.
Fix tests that fail because settings.AWS_STORAGE_BUCKET_NAME is now defined.
Fix tests that fail because content is always wrapped.
Fix test for gzipped file since that now only takes bytes.
@jschneier jschneier merged commit 19a15c2 into jschneier:master Sep 4, 2023
15 checks passed
@jschneier
Copy link
Owner

Thanks, moto is a big improvement. Seems boto has grown several more features in the last few years that necessitated a few more tweaks.

@hugo-reveni
Copy link

Thanks a lot @jschneier, we will wait for the next release with this improvement 👏🏽

@jschneier
Copy link
Owner

jschneier commented Sep 5, 2023 via email

@hugo-reveni
Copy link

@jschneier sorry I didn't find new releases in Github tags section like past ones but now I see it was in here. Thanks!

@jschneier
Copy link
Owner

jschneier commented Sep 5, 2023 via email

@dvf
Copy link

dvf commented Sep 5, 2023

It's people like @jschneier that make OSS awesome. Thanks Josh ❤️

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.

7 participants