Skip to content

Fix upload_attachment to work with images#30

Closed
jonchun wants to merge 1 commit intolinode:masterfrom
jonchun:fix-upload
Closed

Fix upload_attachment to work with images#30
jonchun wants to merge 1 commit intolinode:masterfrom
jonchun:fix-upload

Conversation

@jonchun
Copy link

@jonchun jonchun commented Jul 18, 2017

No longer throws UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte when attempting to upload jpgs (or other images)

Fixes #17

No longer throws UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte when attempting to upload jpgs (or other images)
Copy link
Collaborator

@Dorthu Dorthu left a comment

Choose a reason for hiding this comment

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

Thanks 👍 I probably shoulda thought of that

errors = []
j = result.json()

# Try/catch here because we may not get back json if we get an error at the webserver level
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get a webserver error in normal server operation? If we can, I'd argue we should fix it there instead of handling it here.

Copy link
Author

@jonchun jonchun Jul 18, 2017

Choose a reason for hiding this comment

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

Yes you can. One very obvious example (and one I hit while testing) was 413 from nginx. While it looks like nginx is not configured correctly at this time (gives a 413 for what seems to be anything bigger than 1.5MB or something rather than allowing up to 5MB as documented)

This will be a problem even in the future if that limit is raised if you try to upload a file that is too large. (rare scenario, but still possible. instead of giving an api error, it gives a cannot parse json error. i figured i'd just catch all webserver level errors just in case there's something we missed)

@jonchun
Copy link
Author

jonchun commented Oct 10, 2017

Does this PR need anything else? I haven't looked at it in a while. Didn't realize it was still open.

@jonchun jonchun closed this Apr 26, 2018
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.

2 participants