Convert to HttpURLConnection/OkHttp #12

zefei · 2015-11-03T00:51:01Z

Fix #9

overloaded Twitter.signRequest to accept HttpURLConnection
use OkHttp instead of HttpURLConnection if OkHttp exists in user's classpath

Test:

Logged all changed code paths to see see that DefaultOAuthConsumer/Provider work,
HttpURLConnection and OkHttp both work in different dependency settings
Logs were removed afterwards

facebook-github-bot · 2015-11-03T00:51:14Z

By analyzing the blame information on this pull request, we identified @wangmengyan95 to be a potential reviewer.

grantland · 2015-11-04T01:00:05Z

This looks super good! Just a few nits 😄

It also looks like you've resolved #7 with this as well!

grantland · 2015-11-04T01:00:10Z

library/src/main/java/com/parse/internal/signpost/basic/DefaultOAuthProvider.java

@@ -38,7 +38,7 @@ public DefaultOAuthProvider(String requestTokenEndpointUrl, String accessTokenEn

    protected HttpRequest createRequest(String endpointUrl) throws MalformedURLException,
            IOException {
-        HttpURLConnection connection = (HttpURLConnection) new URL(endpointUrl).openConnection();
+        HttpURLConnection connection = HttpURLConnectionProvider.getInstance().open(new URL(endpointUrl));


Let's pass in a HttpURLConnectionProvider instance through the constructor to match the CommonsHttpOAuthProvider.

facebook-github-bot · 2015-11-04T02:08:18Z

@zefei updated the pull request.

grantland · 2015-11-04T23:04:28Z

library/src/main/java/com/parse/internal/signpost/basic/HttpURLConnectionClient.java

+    private Object okUrlFactory = null;
+    private Method okUrlFactoryOpen = null;
+
+    protected HttpURLConnectionClient() {


Might as well make this private

grantland · 2015-11-04T23:04:59Z

Passing back for revision. I'm ok with the 4 spaces in that file since it matches the rest of that package.

facebook-github-bot · 2015-11-05T02:23:11Z

@zefei updated the pull request.

grantland · 2015-11-05T02:38:39Z

Everything looks good! Could you also remove this line since we won't be needing org.apache.http anymore: https://github.com/ParsePlatform/ParseTwitterUtils-Android/blob/master/library/build.gradle#L22

Lastly, it'd be great if we didn't use reflection to utilize OkHttp, so we can rely on compiler errors if any OkHttp APIs change (they shouldn't, but just in case). We have an example on how we did it in our SDK that you can follow and you'll also want to add provided dependency as well. This totally slipped my mind earlier so feel free to skip this part if you're too busy since I remembered it a bit late.

zefei · 2015-11-05T02:55:50Z

I don't think we can remove apache because Twitter.signRequest is public and allows apache request to be passed.

zefei · 2015-11-05T03:31:50Z

When I was trying to use provided scope, I found that this might not be ideal. Since we don't have explicit dependency on OkHttp, wouldn't compile/runtime error confuse users if they or their dependencies use an imcompatible OkHttp?

grantland · 2015-11-05T05:14:58Z

I don't think we can remove apache because Twitter.signRequest is public and allows apache request to be passed.

Good call!

When I was trying to use provided scope, I found that this might not be ideal. Since we don't have explicit dependency on OkHttp, wouldn't compile/runtime error confuse users if they or their dependencies use an imcompatible OkHttp?

As long as we don't pull any classes into the runtime that directly import anything from OkHttp before we know OkHttp exists in the runtime, we should be fine. The link I sent you is an example of this and you should be able to test a working implementation by adding/removing OkHttp from your application that depends on ParseTwitterUtils.

If you don't have time for this part, feel free to just squash your commits and I can merge.

zefei · 2015-11-05T05:39:15Z

I'm still not sure that this is safe, please see this: http://stackoverflow.com/questions/31319953/error-parse-1-9-3-and-okhttp-2-0-0

Even though OkHttp exists in runtime, it could be an incompatible version that satisfies the symbol lookup, but still fails actual signature matching/lookup. Since this is basically runtime dispatching, I can't think of any ways to avoid runtime error without reflection.

grantland · 2015-11-05T05:53:13Z

I agree that your solution is safer in the sense that it'll "work" regardless of a breaking change in OkHttp as this library will fall back to HttpURLConnection, however "working" is relative. It won't crash when there's a version mismatch between ParseTwitterUtils and OkHttp, but it won't work as intended either. It would silently fail the SDK would go on unnoticed incorrectly using HttpURLConnection instead of OkHttp even if it existed in the runtime.

If we were to fail hard in the case of a breaking OkHttp change, a developer might be burdened to using a older version of OkHttp until this library has been updated but they wouldn't release a "bad" build.

There might be an odd case where a developer requires an older version of a library, but we can add some specific checking for the newer API in hasOkHttpOnClassPath() if that issue ever occurs.

I'd love to hear your thoughts if you still feel strongly about your opinion on this.

zefei · 2015-11-05T06:35:25Z

I agree that falling back silently is bad, I will change how it's handled. But I'm still in favor of reflection. I don't have a strong opinion though as this is a trade off.

My thinking is: the nature of this is dynamic dispatching, which is hard to handle at compile time. If we use provided and OkHttp changes its API, we will be forced to choose one version of OkHttp to import, and thus force users to go with one version. This couples our version with OkHttp version, but we gain compile time checks. Going reflection loses compile checks, but also looses coupling.

- use DefaultOAuthConsumer/Provider instead of CommonsHttpOauthConsumer/Provider - overloaded Twitter.signRequest to accept HttpURLConnection - use OkHttp instead of HttpURLConnection if OkHttp exists in user's classpath

facebook-github-bot · 2015-11-05T19:57:14Z

@zefei updated the pull request.

grantland · 2015-11-05T20:07:03Z

Yeah, there's no silver bullet here with regards to working with other APIs. Thanks for putting in the effort for implementing this functionality!

Convert to HttpURLConnection/OkHttp

zefei · 2015-11-05T20:10:57Z

Thank you for the very detailed reviews and guidance!

facebook-github-bot added GH Review: review-needed CLA Signed labels Nov 3, 2015

grantland reviewed Nov 4, 2015
View reviewed changes

grantland added GH Review: needs-revision and removed GH Review: review-needed labels Nov 4, 2015

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 5, 2015

convert to HttpURLConnection/OkHttp

5cd3788

- use DefaultOAuthConsumer/Provider instead of CommonsHttpOauthConsumer/Provider - overloaded Twitter.signRequest to accept HttpURLConnection - use OkHttp instead of HttpURLConnection if OkHttp exists in user's classpath

zefei force-pushed the convert_to_httpurlconnection_okhttp branch from 7c61e40 to 5cd3788 Compare November 5, 2015 19:56

grantland added a commit that referenced this pull request Nov 5, 2015

Merge pull request #12 from zefei/convert_to_httpurlconnection_okhttp

3fe89c3

Convert to HttpURLConnection/OkHttp

grantland merged commit 3fe89c3 into parse-community:master Nov 5, 2015

grantland mentioned this pull request Nov 5, 2015

Re-use HttpClient #7

Closed

Uh oh!

Convert to HttpURLConnection/OkHttp #12

Convert to HttpURLConnection/OkHttp #12

Uh oh!

Conversation

zefei commented Nov 3, 2015

Uh oh!

facebook-github-bot commented Nov 3, 2015

Uh oh!

grantland commented Nov 4, 2015

Uh oh!

grantland Nov 4, 2015

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 4, 2015

Uh oh!

grantland Nov 4, 2015

Choose a reason for hiding this comment

Uh oh!

grantland commented Nov 4, 2015

Uh oh!

facebook-github-bot commented Nov 5, 2015

Uh oh!

grantland commented Nov 5, 2015

Uh oh!

zefei commented Nov 5, 2015

Uh oh!

zefei commented Nov 5, 2015

Uh oh!

grantland commented Nov 5, 2015

Uh oh!

zefei commented Nov 5, 2015

Uh oh!

grantland commented Nov 5, 2015

Uh oh!

zefei commented Nov 5, 2015

Uh oh!

facebook-github-bot commented Nov 5, 2015

Uh oh!

grantland commented Nov 5, 2015

Uh oh!

zefei commented Nov 5, 2015

Uh oh!

Uh oh!