OkHttp Builder Exposed for REST and AWS #628

Jawnnypoo · 2017-04-06T05:45:47Z

As discussed in #620 this adds the ability to specify an OkHttpClient.Builder for the REST API as well as the AWS network calls.

To do this, you just pass it to your Parse.Configuration.Builder:

Parse.Configuration config = new Parse.Configuration.Builder(context)
            .awsClientBuilder(new OkHttpClient.Builder())
            .clientBuilder(new OkHttpClient.Builder())
            .build();

This involves removing ParseNetworkInterceptor due to the fact that it would require quite a bit of extra work to support both, and it would be unclear which interceptors are called first, etc. I think it best to just remove it. This will however require the next version to be something like 1.15, and require us to update some docs.

In the end, I think that this is needed since Parse servers can have all kinds of configurations, such as specific pinning, SSL configurations, etc that would be very cumbersome to support without exposing the internal OkHttpClient.

@rogerhu Let me know what you think.

…ts from failing

rogerhu · 2017-04-06T06:22:36Z

Parse/build.gradle

@@ -41,12 +41,17 @@ android {

 ext.okhttpVersion = '3.6.0'
 dependencies {
+    compile 'com.android.support:support-annotations:25.3.1'


the support version we use should be an ext { } definition likely in the root build.gradle. this way the starter project that you updated can also just pull from this version

Sounds good 👍

rogerhu · 2017-04-06T06:25:22Z

Parse/src/main/java/com/parse/Parse.java

       * @return The same builder, for easy chaining.
       */
      public Builder enableLocalDataStore() {
        localDataStoreEnabled = true;
        return this;
      }

-      /* package for tests */ Builder setNetworkInterceptors(Collection<ParseNetworkInterceptor> interceptors) {


can you confirm Stetho works fine with the new builder? I'm trying to figure out why we needed a separate parse interceptor. https://github.com/parse-community/ParseInterceptors-Android

Hmm yeah I'll have to check on that, not a Stetho user myself

Yeah, we are just going to have to remove support for that library and instead point users to using Stetho on their OkHttpClient Builder that they create for Parse http://facebook.github.io/stetho/

Somewhat unfortunate, but I think it is the best thing moving forward

Guys, be really careful when you abrubptly plan to deprecate a feature that is really useful for debugging/ troubleshooting.
Moreover, relying on an additional 3rd party library seems to be a step back from having an easy to use interceptor as this one.

I believe https://github.com/parse-community/ParseInterceptors-Android is redundant. Before we upgraded the library to rely mostly on the OkHttp networking, this repo was used to provide a more generic interface for the different ways to handle networking.

Most of the Android world relies on Stetho/HttpLogInterceptor that provides out of the box so I don't see a reason we need to continue maintaining ParseInterceptors (also it is not really maintained)

@flovilmart I get your concern here, but OkHttp is pretty much standard for an Android library/application that is using HTTP. We are deprecating that interceptors library in favor of just having people use OkHttp interceptors for logging, which allows for more control and selection for interceptors (including logging ones) such as https://github.com/jgilfelt/chuck. Most people will probably be more familiar with setting up an OkHttp interceptor than a Parse specific one. Also, relying on a tried and true third party library is favorable in my opinion over having three different HTTP implementations to maintain/debug.

Don't forget to update the README, wiki and docs repository then :)

rogerhu · 2017-04-06T06:26:50Z

Parse/src/main/java/com/parse/Parse.java

@@ -643,37 +629,24 @@ private static ParseEventuallyQueue getEventuallyQueue(Context context) {
    }
  }

-  static void checkInit() {


What's the reason for deleting? Parse LiveQuery depends on this function to check to see if Parse has been initialized.

Ah, I saw that it was unused, and since it was not public, I figured it was safe to delete. I did not realize that ParseLiveQuery was of the same package name and would be accessing things that were package protected within Parse. I'll add it back, but it would probably be best to annotate or comment on things where this is the case

rogerhu · 2017-04-06T06:30:23Z

Parse/src/main/java/com/parse/ParseFileController.java

@@ -41,7 +41,7 @@ public ParseFileController(ParseHttpClient restClient, File cachePath) {
  /* package */ ParseHttpClient awsClient() {
    synchronized (lock) {
      if (awsClient == null) {
-        awsClient = ParsePlugins.get().newHttpClient();
+        awsClient = ParsePlugins.get().awsClient();


The awsclient is interesting to me... what is the use case for exposing it publicly (and separate from the OkHttpInstance builder)?

Currently, the REST API uses a different HttpClient from the AWS client, and I was not really too sure of the reasons. Possibly just so that both could be making requests at the same time. Do you think they should use the same builder? I can make that the case, and they both just create their own client as needed if that sounds better.

Should we rename to fileClient? Files are not necessarily hosted on AWS anymore.

+1 for fileClient.

Let's use one builder for now until someone decides they need a separate one. It was never exposed in the past. I think the major reason is that there isn't a shared HttpClient because there is a parseClient() that is used for injecting Parse auth headers (see ParsePlugins.java)

Do we actually need to expose the fileClient builder? It does not need headers, and it just requests public file URLs that shouldn’t require any particular certificate / intercepting behavior.

If we use a single client, all requests to files will send useless (private) headers + any other burden that developer applies to the builder.. right?

Yeah, I will go ahead and have the HTTP and File clients be the same builder, just two different client instances to mock the present behavior. The reasoning for having them both be configurable would just be in case we have someone who has two different servers, one for Parse and one that hosts files, and it is possible though unlikely that they could be configured with different HTTPS configurations. But I am all for keeping it limited to the one builder for now.

rogerhu · 2017-04-10T07:40:29Z

1 left:

com.parse.ParseCloudTest > testGetCloudCodeController FAILED
    java.lang.IllegalStateException: Another currentUser controller was already registered: com.parse.CachedCurrentUserController@1f197fa9
        at com.parse.ParseCorePlugins.registerCurrentUserController(ParseCorePlugins.java:146)
        at com.parse.ParseTestUtils.setTestParseUser(ParseTestUtils.java:39)
        at com.parse.ParseCloudTest.setUp(ParseCloudTest.java:47)
Gradle Test Executor 1 finished executing tests.

codecov · 2017-04-11T01:49:40Z

Codecov Report

Merging #628 into master will decrease coverage by 0.31%.
The diff coverage is 57.78%.

@@             Coverage Diff              @@
##             master     #628      +/-   ##
============================================
- Coverage     51.27%   50.95%   -0.32%     
+ Complexity     1536     1525      -11     
============================================
  Files           129      127       -2     
  Lines          9836     9727     -109     
  Branches       1327     1313      -14     
============================================
- Hits           5043     4956      -87     
+ Misses         4385     4371      -14     
+ Partials        408      400       -8

Impacted Files	Coverage Δ	Complexity Δ
...arse/src/main/java/com/parse/ParseFileRequest.java	`83.33% <100%> (ø)`	`3 <0> (?)`
Parse/src/main/java/com/parse/ParsePlugins.java	`43.75% <41.75%> (-5.72%)`	`12 <12> (-1)`
Parse/src/main/java/com/parse/Parse.java	`49.76% <64.28%> (+2.73%)`	`23 <0> (ø)`	⬇️
...e/src/main/java/com/parse/ParseFileController.java	`80.41% <71.42%> (ø)`	`14 <0> (ø)`	⬇️
Parse/src/main/java/com/parse/ParseHttpClient.java	`72.34% <72.09%> (+4.84%)`	`18 <16> (+11)`	⬆️
...se/src/main/java/com/parse/ParseKeyValueCache.java	`46.53% <0%> (+1.98%)`	`16% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d66c5a3...ef4635b. Read the comment docs.

Jawnnypoo · 2017-04-11T01:52:43Z

@rogerhu This should be good now. I went ahead and made tests that were failing due to Plugins reset the plugins before and after test execution so that this hopefully wont be a problem for any more PRs

rogerhu · 2017-04-11T04:03:41Z

Thanks this is awesome! Going to merge.

Can you draft an update to the Parse docs (https://github.com/parse-community/docs)?
We may need to put up an update on Parse network interceptors that it is no longer going to be compatible with the next 1.14.2 release.

Jawnnypoo · 2017-04-11T04:43:40Z

@rogerhu Gotcha, I can get to that hopefully tomorrow. I for sure think this should be a 1.15.0 release though, since we are removing ParseInterceptors and it would be bad to remove a public class that people might depend on in a dot release. What do you think?

rogerhu · 2017-04-11T04:50:07Z

Yep let's do that!

wangmengyan95 · 2017-04-11T07:39:07Z

I can add some context here.

The reason we create Parse Interceptor is because we need to support three http libraries, eg apache, URLConnection and OKHttp. Since the first two libraries' API are so different, to make sure we do not make huge change to the controller level, we create a wrapper class ParseHttpClient and build the Parse Interceptor on top of it. The interceptor idea and mechanism are from OKHttp. If we only use OKHttp to power Parse, we can just use a standard OKHttp Interceptor.

The reason we need to support three different http libraries is because when we open source parse

There were still lots of users using < 4.4 os (I remembered more than 30%), which means we have to support the apache library. Based on the current data here https://developer.android.com/about/dashboards/index.html, only 10% users are under 4.4.
We are very conservative about adding any external dependency. So we just support OKHttp, but not require it.

I am fine with require OkHttp since it is actually the engine power the URLConnection in AOSP. If you guys need to make some API changes, make sure you clearly doc it and put it in the release notes.

Thanks for the contribution!

flovilmart · 2017-04-11T11:18:32Z

Thanks @wangmengyan95 for the clarification!

Jawnnypoo added 5 commits April 5, 2017 20:40

Update some dependencies

ffd58bf

Direct access to ParseOkHttpClient since it is the only one

c99854a

ParseOkHttpClient becomes ParseHttpClient

0d5a535

Remove ParseNetworkInterceptor in favor of OkHttp interceptors

7a11aef

Modify tests that need Plugin teardown to do so to prevent random tes…

01ada38

…ts from failing

rogerhu reviewed Apr 6, 2017

View reviewed changes

Jawnnypoo added 4 commits April 9, 2017 21:54

Joint support library version

388c483

Add back in checkinit

20f31c3

Remove AWS client builder (build the http builder instead). AWS -> File

69bef08

Fix ParseFileRequestTest

0f6cfb3

Reset in between test cases in ParseCloudTest

3d22c50

Add plugin resetting to both the setup and tear down of tests

ef4635b

rogerhu merged commit 6cfc4a0 into parse-community:master Apr 11, 2017

Jawnnypoo mentioned this pull request Apr 11, 2017

Allow specifying a custom OkHttp client #620

Closed

Jawnnypoo deleted the okhttp-builder branch April 11, 2017 04:42

Jawnnypoo mentioned this pull request Apr 12, 2017

Add deprecation notice parse-community/ParseInterceptors-Android#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OkHttp Builder Exposed for REST and AWS #628

OkHttp Builder Exposed for REST and AWS #628

Jawnnypoo commented Apr 6, 2017

rogerhu Apr 6, 2017

Jawnnypoo Apr 6, 2017

rogerhu Apr 6, 2017

Jawnnypoo Apr 6, 2017

Jawnnypoo Apr 10, 2017

flovilmart Apr 10, 2017

rogerhu Apr 10, 2017

Jawnnypoo Apr 10, 2017 •

edited

Loading

flovilmart Apr 10, 2017

rogerhu Apr 6, 2017

Jawnnypoo Apr 6, 2017

rogerhu Apr 6, 2017

Jawnnypoo Apr 6, 2017

natario1 Apr 6, 2017

rogerhu Apr 6, 2017 •

edited

Loading

natario1 Apr 6, 2017

Jawnnypoo Apr 10, 2017

rogerhu commented Apr 10, 2017

codecov bot commented Apr 11, 2017 •

edited

Loading

Jawnnypoo commented Apr 11, 2017

rogerhu commented Apr 11, 2017

Jawnnypoo commented Apr 11, 2017

rogerhu commented Apr 11, 2017 via email

wangmengyan95 commented Apr 11, 2017

flovilmart commented Apr 11, 2017

OkHttp Builder Exposed for REST and AWS #628

OkHttp Builder Exposed for REST and AWS #628

Conversation

Jawnnypoo commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jawnnypoo Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu commented Apr 10, 2017

codecov bot commented Apr 11, 2017 • edited Loading

Codecov Report

Jawnnypoo commented Apr 11, 2017

rogerhu commented Apr 11, 2017

Jawnnypoo commented Apr 11, 2017

rogerhu commented Apr 11, 2017 via email

wangmengyan95 commented Apr 11, 2017

flovilmart commented Apr 11, 2017

Jawnnypoo Apr 10, 2017 •

edited

Loading

rogerhu Apr 6, 2017 •

edited

Loading

codecov bot commented Apr 11, 2017 •

edited

Loading