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

Use compressed content instead of changing original object #183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

melbic
Copy link

@melbic melbic commented Aug 4, 2016

This fixes a bug that the upload of a file is not possible when gzip is activated (django-cms/django-filer#885)

When gzip activated, the current version changes the actual object and overwrites its current file
with the gzipped datastream. When the object instance is simultaneously used by other "algorithms" (e.g. django-filer) this ends in wrong behavior/exceptions.

When gzip activated, the current version changes the actual object and overwrites its current file
with the gzipped datastream. When the object instance is simultaneously used by other "algorithms" (e.g. django-filer)
this ends in wrong  behavior/exceptions.
@jschneier
Copy link
Owner

Hi, I think is another report of the same issue. My worry is that I don't think your fix will work, as the reporter says they are seeing an exception with the .size parameter not existing, the zbuf object doesn't have a .size property either.

@jschneier
Copy link
Owner

As such this PR was submitted. I'm trying to deal with this issue now but I don't understand it yet, any help would be greatly appreciated.

@melbic
Copy link
Author

melbic commented Aug 5, 2016

Yes, the size method/ivar is the problem there, but it only occurs because we exchange the existing file in content with the new ByteIO stream. My approach is to leave the object the way it was and only save the new stream to S3.

You may test the behavior by installing django-filer==1.1.0 enable GZIP for the images and upload an image. It will silent fail.
The exception happening here:

django.db.models.fields.FieldFile._get_size:
    def _get_size(self):
        self._require_file()
        if not self._committed:
            return self.file.size <----
        return self.storage.size(self.name)

@melbic
Copy link
Author

melbic commented Mar 17, 2017

@jschneier I'd really like to switch back to the pypi version instead of using our fork. Any progression in the matter of this PR?

@jschneier
Copy link
Owner

Can you please add a test. Or the outlines of one and I can take a go at adding it if you don't have the time.

@melbic
Copy link
Author

melbic commented Mar 20, 2017

Ok, I try to find the time this or the next week.

@melbic
Copy link
Author

melbic commented Mar 23, 2017

I just tried to adjust test_s3boto.py::test_storage_save_gzip, but unfortunately I wasn't able to figure out what exactly the arguments in key.set_contents_from_file.assert_called_with should be for gzipped data. Any hints?

@gposton
Copy link

gposton commented Jun 15, 2018

@jschneier, we've ran into this bug as well... I commented on #74, but this PR seems to be more recent. Can we get this merged? Our issue is the same. Cannot upload small files. We've verified the PR fixes our issue on python 2.7.9, boto 2.48, django 1.6.8. Would like to remain on main-stream if possible :)

@gposton
Copy link

gposton commented Jun 15, 2018

Not sure if this helps, but the issue is not present in this combo: python 2.7.9, boto 2.36, django-storages 1.1.8

@gposton
Copy link

gposton commented Jun 15, 2018

I suspect this broke in 1.6.6 w/ the change from StringIO to ByteIO: 1c64e83

@sww314 sww314 added the s3boto label Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants