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

Reset image stream position after using tell #105

Merged
merged 1 commit into from
Feb 24, 2018

Conversation

ezarowny
Copy link
Contributor

@ezarowny ezarowny commented Feb 9, 2018

For uploads, I noticed that using django-markdownx with something like django-storages doesn't work because the image stream position is set to the end of the steam when the InMemoryUploadedFile is instantiated. Simply resetting the image stream position back to the beginning should do the trick.

In the future, when you guys drop Python 2 support, something like getbuffer or getvalue could be used to get the length of the buffer instead of using tell.

@xenatisch
Copy link
Collaborator

Nice catch.
You tested this in different scenarios? It's not breaking any of the forthcoming features?

@xenatisch xenatisch added the bug label Feb 24, 2018
@ezarowny
Copy link
Contributor Author

I'll run down some more test cases. I've mostly been testing this against Google Cloud Storage but it shouldn't really make much of a difference.

@xenatisch
Copy link
Collaborator

Thanks again.

It's not the storage I'm worried about; it's the forthcoming attributes that are applied. Just test it against PNG, JPEG, and SVG files. If they all work, then we're good to go.

@xenatisch xenatisch self-assigned this Feb 24, 2018
@xenatisch xenatisch self-requested a review February 24, 2018 01:13
@ezarowny
Copy link
Contributor Author

ezarowny commented Feb 24, 2018

What would you like me to look for besides the upload working and displaying inline in admin? The file types are correct in storage (I'm testing locally to rule out any Google weirdness).

JPEG, PNG and SVG all seem to work for me.

@xenatisch
Copy link
Collaborator

Nothing... We're all good if all the uploads are working.

Is Google storage really that weird? I keep hearing about it!

Copy link
Collaborator

@xenatisch xenatisch left a comment

Choose a reason for hiding this comment

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

Confirmed on different image formats.
Confirmed in admins (upload + preview).

@xenatisch
Copy link
Collaborator

In the future, when you guys drop Python 2 support, something like getbuffer or getvalue could be used to get the length of the buffer instead of using tell.

I know - and how I wish to see the back of Python 2!

Would you mind adding this recommendation to #60 - just so we won't forget! Or better yet, just change it yourself and send a pull request our way. We are dropping Python 2 support in the next version, so...

@xenatisch xenatisch merged commit af8f17e into neutronX:master Feb 24, 2018
@ezarowny
Copy link
Contributor Author

ezarowny commented Feb 26, 2018

Eh, I wouldn't say that GCS is all that weird. You just can't necessarily treat it like a normal file stream. At least that's the case for the Google-provided Python library.

👍 for the Python 3 love

@ezarowny ezarowny mentioned this pull request Feb 26, 2018
@ezarowny ezarowny deleted the fix-zero-bytes-remaining branch February 26, 2018 18:20
@ezarowny ezarowny restored the fix-zero-bytes-remaining branch February 26, 2018 18:20
@ezarowny ezarowny deleted the fix-zero-bytes-remaining branch December 30, 2019 19:26
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.

None yet

2 participants