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

Image upload with too big image #783

Merged
merged 1 commit into from Aug 16, 2016
Merged

Image upload with too big image #783

merged 1 commit into from Aug 16, 2016

Conversation

tak1n
Copy link
Contributor

@tak1n tak1n commented Jun 29, 2016

In https://dev.twitter.com/rest/reference/post/media/upload is following written:

Supported media types and filesize

Supported media types: jpg, png, gif, webp

Image file size must be <= 5 MB

The filesize limit above is enforced by the media upload endpoint. In addition, there is a separate product entity specific filesize limit which is applied when calling the Tweet creation (or similar) endpoints with media_id. For example, it is possible to upload a 5 MB image, but the Tweet creation endpoint requires images to be <= 3 MB.

Twitter returns a empty body and status code 413 when trying to upload a image which exceeds this size limit. This will lead to an unhelpful error for twitter gem users as it will lead to following:

irb(main):001:0> response_body = ''
=> ""
irb(main):002:0> response_body[:media_id]
TypeError: no implicit conversion of Symbol into Integer
        from (irb):2:in `[]'
        from (irb):2
        from /home/benny/.rubies/2.3.1/bin/irb:11:in `<main>'

This is happening here: https://github.com/sferik/twitter/blob/master/lib/twitter/rest/tweets.rb#L227
This PR fixes this in the way that it raises an exception here: https://github.com/sferik/twitter/blob/master/lib/twitter/rest/request.rb#L81 instead of passing the empty string (empty response body) further along.

I did not add an explicit spec for that as it seems to me in error spec all error cases are dynamically spec'd (adding an error includes this error automatically in the spec)

@calebhearth
Copy link

I had just tracked this error down in my own app. The current behavior led to a lot of confusion, thanks for fixing this, @tak1n!

@calebhearth
Copy link

Playing with the request in Pry, the response I get from https://github.com/sferik/twitter/blob/master/lib/twitter/rest/request.rb#L36 for a large 13 meg gif is a 413 Payload Too Large. Seems like this would fix it. @sferik

@sferik
Copy link
Owner

sferik commented Aug 16, 2016

Thanks @tak1n for the patch and thanks @calebthompson for testing it.

@sferik sferik merged commit aa909b3 into sferik:master Aug 16, 2016
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