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

add proxy support to REST and Streaming clients #663

Merged
merged 1 commit into from
May 12, 2015
Merged

Conversation

stve
Copy link
Collaborator

@stve stve commented Feb 24, 2015

@sferik this is pending some testing (see #639), but assuming all goes well, this should be good to merge. Thankfully, it was fairly easy to add support in both places.

Note: I needed to disable rubocop's ClassLength check on Twitter::REST::Request. I hope that's ok. I tried reducing by using ternary operators in a few places (which I typically don't like to do), but couldn't squeeze it down to <100. It seems like there could be an opportunity to reduce the number of lines once http.rb supports multipart requests.

@sferik
Copy link
Owner

sferik commented Feb 24, 2015

Awesome work on this, Steve. It appears the tests failed because YARD documentation coverage went down. Specifically, the Twitter::REST::Request methods http_client, net_http_client, and proxy don’t have any inline documentation. Are these all supposed to be public methods? The tests don’t seem to consume any of them directly. Please either mark them as private or add documentation and I will merge after testing is complete.

@sferik
Copy link
Owner

sferik commented Feb 25, 2015

👍

@stve
Copy link
Collaborator Author

stve commented Feb 25, 2015

@sferik wow, I waited and waited for the build to run on Travis. The methods were private but I think the number of lines tipped the doc coverage ratio so I just added docs to get the build passing. I also updated the configuration example to mention the proxy config.

Btw, what are the blockers on doing multipart requests with http.rb?

@sferik
Copy link
Owner

sferik commented Feb 25, 2015

@stve Since httprb/http#167 has been merged and released, there are no remaining blockers for making multipart requests with http.rb. I just need to get around to implementing it in.

@sferik
Copy link
Owner

sferik commented Feb 26, 2015

@stve 💥 115e800. That was easy! Very nice job by @ixti on the form_data.rb interface.

That said, the twitter v6 release is still blocked by httprb/http#121.

@stve
Copy link
Collaborator Author

stve commented Feb 26, 2015

@sferik 🤘 I was looking at that last night, figured I'd work on it after we merged this but you beat me to the punch 😸

I'll rebase this branch as it was based off multipart requests being sent via Net::HTTP

@sferik
Copy link
Owner

sferik commented Feb 27, 2015

@stve You may need to rebase again. I just updated it to use the http-form_data gem, which is the new name for the form_data gem.

@stve
Copy link
Collaborator Author

stve commented Feb 27, 2015

👌 rebased, good to go

@sferik
Copy link
Owner

sferik commented Mar 27, 2015

@stve FYI, v6 is no longer blocked by httprb/http#121, which was resolved in httprb/http#184. Now it’s just blocked by a http 0.8.0 gem release, which is currently scheduled for April 1.

@stve
Copy link
Collaborator Author

stve commented Mar 27, 2015

@sferik ok, looks like i might become the blocker on v6 then 😁 The last few weeks have been tough on me but i'll try to pick this back up to get through the remaining items.

Do you still want to consider switching the objects to use Values or virtus as we discussed in #651? I recently built out an API client for an internal API we have and Virtus worked out better than Values in that case. In particular, it's value object support was top notch. While they didn't fit my use-case, custom coercions are a nice feature too. I think this would add a lot of clarity to the top-level Twitter response objects. While I'd ❤️ to focus on that, i'll keep at the streaming changes, just food for thought.

@sferik
Copy link
Owner

sferik commented Mar 27, 2015

👍 for using Virtus. You should feel welcome to work on that instead of streaming, if you prefer. There’s no big rush to ship v6. As I discussed here, there may even be some advantages to waiting (e.g. the ability to drop Ruby 1.9.3 support without losing JRuby users). The milestone has no due date. It should ship when it’s ready. There are lots of exciting features left to work on (streaming improvements, GZip compression, Ads API, TON, etc.) I suppose at some point, people will start demanding a release but, as long as tests remain passing on the master branch, the most eager users can live on the edge.

@56yards
Copy link

56yards commented Apr 10, 2015

I've just tried this branch and I'm receiving error Unknown MIME type: text/html on line client.search("foo").take(1).collect do |tweet| when using a proxy. Has anyone else experienced this issue?

@sferik
Copy link
Owner

sferik commented Apr 10, 2015

@56yards That sounds like an issue with your proxy server not passing through the correct Content-Type from the source. Another possibility is that your proxy is inserting some sort of HTML interstitial into your request (e.g. a page where you must click “I agree” before it lets you make any further requests). Can you please try making the same request via twurl and include the full response body?

@timrwilliams
Copy link

Weighing in here, this branch needs to move its HTTP dependency to > 0.8.4 to fully test. That release included a pull request adding HTTPS proxy support. httprb/http#186

As Twitter requires you to connect via HTTPS without this pull request the client isn't sending the correct request to the proxy (hence the error mentioned by @56yards).

Essentially when you're connecting via a Proxy to an HTTPS end point the client needs to issue a CONNECT request first rather than a standard GET which was only introduced in v0.8.4.

@sferik
Copy link
Owner

sferik commented May 8, 2015

I just rebased this branch against master, so it should have picked up the http ~> 0.8.4 dependency.

@56yards @timrwilliams Can you please test this branch this again and confirm it’s working? If it is, I think we can merge it into master. 😄

@planarian
Copy link

Right now the search method returns "HTTP::Error: Unknown MIME type: text/html"

@timrwilliams
Copy link

I'm seeing the same error using one of our proxies but I believe its an issue with the underlying HTTP gem.

If you try running this from irb (using an anonymous, free HTTP proxy taken from http://proxylist.hidemyass.com/search-1297445#listable)

HTTP.via("176.31.170.159", 8080).get("https://www.google.com/")

Then it just hangs forever.

Running the same thing from curl on the command line works instantly:

curl --proxy http://176.31.170.159:8080 -k https://www.google.co.uk/

I'll file an issue over on the http repo and see what they come back with.

@timrwilliams
Copy link

HTTP have fixed their bug and released as 0.8.8. Can you update the dependency on this branch and then I'll test?

@sferik
Copy link
Owner

sferik commented May 10, 2015

The dependency on the branch should be good. Just be sure to bundle update before you test it.

@planarian
Copy link

Hmm. I'm still getting the same error.

@timrwilliams
Copy link

I've verified this works for me now so am happy to sign off on it.

Jay - I think something might still be off with your dependencies but we'll work through that offline.

sferik added a commit that referenced this pull request May 12, 2015
add proxy support to REST and Streaming clients
@sferik sferik merged commit b299925 into master May 12, 2015
@sferik
Copy link
Owner

sferik commented May 12, 2015

Thanks everyone!

@stve stve deleted the proxy-support branch September 15, 2015 01:32
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.

5 participants