Add support for custom CloseableHttpClient for Proxy Support #441

The-inside-man · 2021-08-18T13:44:31Z

Summary

Adding additional newDefaultInstance method to support custom CloseableHttpClient object
Allows for customs object to support Proxy Settings

Customer support

Test plan

FSC and TestCase added

Issues

"OASIS-7899"

coveralls · 2021-08-18T13:57:11Z

Pull Request Test Coverage Report for Build 1829

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.005%) to 90.517%

Totals
Change from base Build 1828:	0.005%
Covered Lines:	4725
Relevant Lines:	5220

💛 - Coveralls

…s static.

… httpCient and sdkkey.

msohailhussain · 2021-08-24T17:16:43Z

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java

+        return newDefaultInstance(sdkKey, "");
+    }
+
+    public static Optimizely newDefaultInstance(String sdkKey, CloseableHttpClient httpClient) {


I don't think so we need this method.

What about removing this and add httpClient option to "newDefaultInstance(String sdkKey, String fallback, String datafileAccessToken, ClosableHttpClient httpClient)? Then no need for extra set method. Considering demands for customized httpclient, it may be reasonable.

we can add default null in newDefaultInstance but that wouldn't be easy for the customer to initialize using your suggested method.

msohailhussain · 2021-08-24T17:20:48Z

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java

-            .withSdkKey(sdkKey);
-
+        HttpProjectConfigManager.Builder builder;
+        if (customHttpClient != null) {


this condition should only set
.withOptimizelyHttpClient like we have for datafileAccessToken.
or without if condition you may set null using ternary operator that won't have any impact on OptimizelyHttpClient.

.withOptimizelyHttpClient( customHttpClient == null ? null : OHC instance

msohailhussain

have few suggestions otherwise looks good to me.

jaeopt

Looks good. A couple of suggestions.

jaeopt · 2021-08-24T17:35:42Z

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java

+    public static void setCustomHttpClient(CloseableHttpClient httpClient) {
+        if (httpClient == null) {
+            logger.error("CloseableHttpClient must not be null. Please initialize object.");
+        }
+        customHttpClient = httpClient;
+    }


I think we can remove this method (not consistent with the rest of set methods here for property settings) and feed into newInstance parameter.

jaeopt · 2021-08-24T17:47:08Z

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java

+        return newDefaultInstance(sdkKey, "");
+    }
+
+    public static Optimizely newDefaultInstance(String sdkKey, CloseableHttpClient httpClient) {


What about removing this and add httpClient option to "newDefaultInstance(String sdkKey, String fallback, String datafileAccessToken, ClosableHttpClient httpClient)? Then no need for extra set method. Considering demands for customized httpclient, it may be reasonable.

…sary functions.

jaeopt

Changes look good. A couple of more suggestions.

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java

jaeopt

One more cleanup

jaeopt · 2021-08-25T17:13:59Z

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java

+        NotificationCenter notificationCenter = new NotificationCenter();
+        HttpProjectConfigManager.Builder builder;
+        builder = HttpProjectConfigManager.builder()
            .withDatafile(fallback)
            .withNotificationCenter(notificationCenter)
            .withSdkKey(sdkKey);


Let's remove these and use "newDefaultInstance(sdkKey, fallback, datafileAccessToken, null)"

ahhh I see, makes sense. Just a minute.

Made the change, let me know if that is what you were thinking.

jaeopt

LGTM

Initial commit of custom CloseableHttpClient for OptimizelyFactory.

4c5ec28

The-inside-man added the chore label Aug 18, 2021

optimizely-codereview-bot assigned The-inside-man Aug 18, 2021

The-inside-man added 3 commits August 18, 2021 13:24

Update tests and add setter for customHttpClient

758a83d

Resetting the customHttpClient to null after test since implemented a…

db5712a

…s static.

Add additional testcase for null HttpClient and modify logic for null…

abfe16e

… httpCient and sdkkey.

The-inside-man removed the chore label Aug 18, 2021

The-inside-man removed their assignment Aug 18, 2021

The-inside-man marked this pull request as ready for review August 18, 2021 19:17

The-inside-man requested a review from a team as a code owner August 18, 2021 19:17

The-inside-man requested a review from jaeopt August 18, 2021 19:19

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs 🤖 20 hrs 🤖 24 hrs and removed 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs 🤖 16 hrs 🤖 20 hrs labels Aug 18, 2021

msohailhussain reviewed Aug 24, 2021

View reviewed changes

msohailhussain approved these changes Aug 24, 2021

View reviewed changes

jaeopt requested changes Aug 24, 2021

View reviewed changes

optimizely-codereview-bot removed the 🤖 24 hrs label Aug 24, 2021

optimizely-codereview-bot assigned The-inside-man Aug 24, 2021

Update suggested changes to clean up some functions and remove uneces…

3e27f0f

…sary functions.

Correct null check.

ae505ea

The-inside-man requested a review from jaeopt August 25, 2021 13:48

jaeopt requested changes Aug 25, 2021

View reviewed changes

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java Outdated Show resolved Hide resolved

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java Show resolved Hide resolved

The-inside-man added 2 commits August 25, 2021 13:04

Add old constructor back to not break existing customer usage

e026106

Remove unused object.

0d3e1ba

jaeopt requested changes Aug 25, 2021

View reviewed changes

The-inside-man and others added 2 commits August 25, 2021 13:18

Update method to use newDefaultInstance as other methods.

43c9ba7

Merge branch 'master' into jbrown/proxy-builder

c0b4d42

jaeopt approved these changes Aug 25, 2021

View reviewed changes

Merge branch 'master' into jbrown/proxy-builder

055c806

The-inside-man merged commit 674cade into master Aug 26, 2021

The-inside-man deleted the jbrown/proxy-builder branch August 26, 2021 18:57

The-inside-man mentioned this pull request Aug 26, 2021

Enable Proxy support for OptimizelyHttpClient #330

Closed

Add support for custom CloseableHttpClient for Proxy Support #441

Add support for custom CloseableHttpClient for Proxy Support #441

Uh oh!

Conversation

The-inside-man commented Aug 18, 2021

Summary

Test plan

Issues

Uh oh!

coveralls commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1829

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Aug 18, 2021 •

edited

Loading