Skip to content

Commit

Permalink
first cut
Browse files Browse the repository at this point in the history
  • Loading branch information
@thotegowda
thotegowda committed Mar 22, 2017
1 parent 477f2a6 commit b9aaf77
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 88 deletions.
2 changes: 1 addition & 1 deletion ReactAndroid/src/main/java/com/facebook/react/modules/fresco/FrescoModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public static ImagePipelineConfig.Builder getDefaultConfigBuilder(ReactContext c
HashSet<RequestListener> requestListeners = new HashSet<>();
requestListeners.add(new SystraceRequestListener());

OkHttpClient client = OkHttpClientProvider.createClient();
OkHttpClient client = new OkHttpClientProvider().get();

This comment has been minimized.

Sign in to view

Copy link
@mkonicek

mkonicek May 8, 2017

What does new OkHttpClientProvider().get() do compared to OkHttpClientProvider.createClient()? Are you changing this because you want Fresco to use the custom OkHttp client?

This comment has been minimized.

Sign in to view

Copy link
@thotegowda

thotegowda May 11, 2017

Author Owner

createClient() was creating a OkHttpClient instance and returning it. But get() will return the configured OkHttpClient, if not present, will create one.

// make sure to forward cookies for any requests via the okHttpClient
// so that image requests to endpoints that use cookies still work
Expand Down
64 changes: 64 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

package com.facebook.react.modules.network;

import android.os.Build;

import com.facebook.common.logging.FLog;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;

import okhttp3.ConnectionSpec;
import okhttp3.OkHttpClient;
import okhttp3.TlsVersion;

public class DefaultOkHttpProvider implements OkHttpClientProvider {

public OkHttpClient get() {
// No timeouts by default
OkHttpClient.Builder client = new OkHttpClient.Builder()
.connectTimeout(0, TimeUnit.MILLISECONDS)
.readTimeout(0, TimeUnit.MILLISECONDS)
.writeTimeout(0, TimeUnit.MILLISECONDS)
.cookieJar(new ReactCookieJarContainer());

return enableTls12OnPreLollipop(client).build();
}

/*
On Android 4.1-4.4 (API level 16 to 19) TLS 1.1 and 1.2 are
available but not enabled by default. The following method
enables it.
*/
public static OkHttpClient.Builder enableTls12OnPreLollipop(OkHttpClient.Builder client) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && Build.VERSION.SDK_INT <= Build.VERSION_CODES.KITKAT) {
try {
client.sslSocketFactory(new TLSSocketFactory());

ConnectionSpec cs = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS)
.tlsVersions(TlsVersion.TLS_1_2)
.build();

List<ConnectionSpec> specs = new ArrayList<>();
specs.add(cs);
specs.add(ConnectionSpec.COMPATIBLE_TLS);
specs.add(ConnectionSpec.CLEARTEXT);

client.connectionSpecs(specs);
} catch (Exception exc) {
FLog.e("OkHttpClientProvider", "Error while enabling TLS 1.2", exc);
}
}

This comment has been minimized.

Sign in to view

Copy link
@mkonicek

mkonicek May 8, 2017

I feel like I've seen this code somewhere, is it copied from somewhere else?

This comment has been minimized.

Sign in to view

Copy link
@autovalue

autovalue May 11, 2017

He moved all of this code into this class as an interface instance to be used as a default. Allowing anyone to pass in their own interface if they so please.

This comment has been minimized.

Sign in to view

Copy link
@thotegowda

thotegowda May 11, 2017

Author Owner

Correct, this code OkHttpClientProvider.java code

return client;
}
}
41 changes: 28 additions & 13 deletions ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,43 +76,58 @@ public final class NetworkingModule extends ReactContextBaseJavaModule {
/* package */ NetworkingModule(
ReactApplicationContext reactContext,
@Nullable String defaultUserAgent,
OkHttpClient client,
OkHttpClientProvider okHttpClientProvider,
@Nullable List<NetworkInterceptorCreator> networkInterceptorCreators) {
super(reactContext);

mClient = createOkHttpClient(okHttpClientProvider, networkInterceptorCreators);
mCookieHandler = new ForwardingCookieHandler(reactContext);
mCookieJarContainer = (CookieJarContainer) mClient.cookieJar();
mShuttingDown = false;
mDefaultUserAgent = defaultUserAgent;
mRequestIds = new HashSet<>();
}

private OkHttpClient createOkHttpClient(
OkHttpClientProvider okHttpClientProvider,
@Nullable List<NetworkInterceptorCreator> networkInterceptorCreators) {
OkHttpClient client = okHttpClientProvider == null ? new DefaultOkHttpProvider().get() : okHttpClientProvider.get();
if (networkInterceptorCreators != null) {
OkHttpClient.Builder clientBuilder = client.newBuilder();
for (NetworkInterceptorCreator networkInterceptorCreator : networkInterceptorCreators) {
clientBuilder.addNetworkInterceptor(networkInterceptorCreator.create());
}
client = clientBuilder.build();
}
mClient = client;
mCookieHandler = new ForwardingCookieHandler(reactContext);
mCookieJarContainer = (CookieJarContainer) mClient.cookieJar();
mShuttingDown = false;
mDefaultUserAgent = defaultUserAgent;
mRequestIds = new HashSet<>();
return client;
}

/**
* @param context the ReactContext of the application
* @param defaultUserAgent the User-Agent header that will be set for all requests where the
* caller does not provide one explicitly
* @param client the {@link OkHttpClient} to be used for networking
* @param okHttpClientProvider the {@link OkHttpClient} provider to be used for networking
*/
/* package */ NetworkingModule(
ReactApplicationContext context,
@Nullable String defaultUserAgent,
OkHttpClient client) {
this(context, defaultUserAgent, client, null);
OkHttpClientProvider okHttpClientProvider) {
this(context, defaultUserAgent, okHttpClientProvider, null);
}

/**
* @param context the ReactContext of the application
*/
public NetworkingModule(final ReactApplicationContext context) {
this(context, null, OkHttpClientProvider.createClient(), null);
this(context, null, new DefaultOkHttpProvider(), null);
}

/**
* @param context the ReactContext of the application
* @param okHttpClientProvider to provide OkHttpInstance with different settings
*/
public NetworkingModule(final ReactApplicationContext context, OkHttpClientProvider okHttpClientProvider) {
this(context, null, okHttpClientProvider, null);
}

/**
Expand All @@ -123,7 +138,7 @@ public NetworkingModule(final ReactApplicationContext context) {
public NetworkingModule(
ReactApplicationContext context,
List<NetworkInterceptorCreator> networkInterceptorCreators) {
this(context, null, OkHttpClientProvider.createClient(), networkInterceptorCreators);
this(context, null, new DefaultOkHttpProvider(), networkInterceptorCreators);
}

/**
Expand All @@ -132,7 +147,7 @@ public NetworkingModule(
* caller does not provide one explicitly
*/
public NetworkingModule(ReactApplicationContext context, String defaultUserAgent) {
this(context, defaultUserAgent, OkHttpClientProvider.createClient(), null);
this(context, defaultUserAgent, new DefaultOkHttpProvider(), null);
}

@Override
Expand Down
74 changes: 3 additions & 71 deletions ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,79 +9,11 @@

package com.facebook.react.modules.network;

import android.os.Build;

import com.facebook.common.logging.FLog;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;

import javax.annotation.Nullable;

import okhttp3.ConnectionSpec;
import okhttp3.OkHttpClient;
import okhttp3.TlsVersion;

/**
* Helper class that provides the same OkHttpClient instance that will be used for all networking
* requests.
* Interface that provides the OkHttpClient instance, that will be used for all networking requests.
*/
public class OkHttpClientProvider {

// Centralized OkHttpClient for all networking requests.
private static @Nullable OkHttpClient sClient;

public static OkHttpClient getOkHttpClient() {
if (sClient == null) {
sClient = createClient();
}
return sClient;
}

// okhttp3 OkHttpClient is immutable
// This allows app to init an OkHttpClient with custom settings.
public static void replaceOkHttpClient(OkHttpClient client) {
sClient = client;
}

public static OkHttpClient createClient() {
// No timeouts by default
OkHttpClient.Builder client = new OkHttpClient.Builder()
.connectTimeout(0, TimeUnit.MILLISECONDS)
.readTimeout(0, TimeUnit.MILLISECONDS)
.writeTimeout(0, TimeUnit.MILLISECONDS)
.cookieJar(new ReactCookieJarContainer());

return enableTls12OnPreLollipop(client).build();
}

/*
On Android 4.1-4.4 (API level 16 to 19) TLS 1.1 and 1.2 are
available but not enabled by default. The following method
enables it.
*/
public static OkHttpClient.Builder enableTls12OnPreLollipop(OkHttpClient.Builder client) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && Build.VERSION.SDK_INT <= Build.VERSION_CODES.KITKAT) {
try {
client.sslSocketFactory(new TLSSocketFactory());

ConnectionSpec cs = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS)
.tlsVersions(TlsVersion.TLS_1_2)
.build();

List<ConnectionSpec> specs = new ArrayList<>();
specs.add(cs);
specs.add(ConnectionSpec.COMPATIBLE_TLS);
specs.add(ConnectionSpec.CLEARTEXT);

client.connectionSpecs(specs);
} catch (Exception exc) {
FLog.e("OkHttpClientProvider", "Error while enabling TLS 1.2", exc);
}
}

return client;
}

public interface OkHttpClientProvider {
OkHttpClient get();
}
17 changes: 15 additions & 2 deletions ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,31 +10,44 @@
package com.facebook.react.shell;

import com.facebook.imagepipeline.core.ImagePipelineConfig;
import com.facebook.react.modules.network.OkHttpClientProvider;

/**
* Configuration for {@link MainReactPackage}
*/
public class MainPackageConfig {

private ImagePipelineConfig mFrescoConfig;
private final OkHttpClientProvider okHttpClientProvider;

This comment has been minimized.

Sign in to view

Copy link
@mkonicek

mkonicek May 8, 2017

This probably needs an m prefix.

This comment has been minimized.

Sign in to view

Copy link
@mkonicek

mkonicek May 8, 2017

This field should have a comment containing the context so that people reading the code in the future understand why it's there:

The main purpose of this commit is to make brown-field applications inject their existing OKHtttpClient instance to react-native networking system. This will enable them to use fetch in javascript and still re-use their existing networking infrastructure (like headers, interceptors, logging, fallbacks, error handling etc) of their existing application, instead of bridging themselves.

This comment has been minimized.

Sign in to view

Copy link
@thotegowda

thotegowda May 11, 2017

Author Owner

Will do
private final ImagePipelineConfig mFrescoConfig;

private MainPackageConfig(Builder builder) {
mFrescoConfig = builder.mFrescoConfig;
okHttpClientProvider = builder.okHttpClientProvider;
}

public ImagePipelineConfig getFrescoConfig() {
return mFrescoConfig;
}

public OkHttpClientProvider getOkHttpClientProvider() {
return okHttpClientProvider;
}

public static class Builder {

private ImagePipelineConfig mFrescoConfig;
private OkHttpClientProvider okHttpClientProvider;

public Builder setFrescoConfig(ImagePipelineConfig frescoConfig) {
public Builder withFrescoConfig(ImagePipelineConfig frescoConfig) {
mFrescoConfig = frescoConfig;
return this;
}

public Builder withOkHttpClientProvider(OkHttpClientProvider okHttpClientProvider) {
this.okHttpClientProvider = okHttpClientProvider;
return this;
}

public MainPackageConfig build() {
return new MainPackageConfig(this);
}
Expand Down
2 changes: 1 addition & 1 deletion ReactAndroid/src/main/java/com/facebook/react/shell/MainReactPackage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public NativeModule get() {
new ModuleSpec(NetworkingModule.class, new Provider<NativeModule>() {
@Override
public NativeModule get() {
return new NetworkingModule(context);
return mConfig == null ? new NetworkingModule(context) : new NetworkingModule(context, mConfig.getOkHttpClientProvider());
}
}),
new ModuleSpec(NetInfoModule.class, new Provider<NativeModule>() {
Expand Down

8 comments on commit b9aaf77

@olegbl
Copy link

@olegbl olegbl commented on b9aaf77 May 5, 2017

Choose a reason for hiding this comment

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

Sorry, I'm not the right person for reviewing this. I'll ping this commit internally to see if any RN Android people want to jump on it - but your best bet is probably taking this through the normal PR process.

@mkonicek
Copy link

@mkonicek mkonicek commented on b9aaf77 May 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please create a PR with a lot of context explaining what issue this is solving?

What is the use case for having a different OkHttp "provider"? What would anyone want to do that What is a provider? This has probably been discussed before elsewhere (?) but I have no context unfortunately.

Creating a pull request explaining why and how anyone would use this code would be a good start to make it possible for the people on the React Native team to review this (I'm not on that team anymore but can help review).

@mkonicek
Copy link

Choose a reason for hiding this comment

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

Or is the purpose of this commit solely to enable TLS on Android 4.1-4.4 (API level 16 to 19)?

@thotegowda
Copy link
Owner Author

@thotegowda thotegowda commented on b9aaf77 May 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@mkonicek - Thanks for looking at this.
The main purpose of this commit is to make brown-field applications inject their existing OKHtttpClient instance to react-native networking system. This will enable them to use fetch in javascript and still re-use their existing networking infrastructure (like headers, interceptors, logging, fallbacks, error handling etc) of their existing application, instead of bridging themselves.

I have made few more changes on this. Will send a PR with all the details.

@mkonicek
Copy link

Choose a reason for hiding this comment

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

Makes sense, thank s for the explanation @thotegowda! From a quick look the idea looks fine, please send the pull request. Added some inline comments.

@thotegowda
Copy link
Owner Author

Choose a reason for hiding this comment

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

Working on comments as well as test. Will submit PR in a day or two

@amilcar-andrade
Copy link

Choose a reason for hiding this comment

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

@thotegowda do you have a PR?

@thotegowda
Copy link
Owner Author

Choose a reason for hiding this comment

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

@mkonicek Thank you very much for your feedback. I have incorporated them and submitted a PR. Please review this facebook#14068

Please sign in to comment.