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

feat: add support for authenticated datafile access #378

Merged
merged 7 commits into from
Jun 24, 2020
Merged

feat: add support for authenticated datafile access #378

merged 7 commits into from
Jun 24, 2020

Conversation

jaeopt
Copy link
Contributor

@jaeopt jaeopt commented Jun 12, 2020

Summary

  • add support for authenticated datafile access
  • auth-token can be set by calling either
    (1) Optimizely optimizely = OptimizelyFactory.newDefaultInstance(String sdkKey,
    String authDatafileToken,
    String fallback); or
    (2) HttpProjectConfigManager.Builder builder = HttpProjectConfigManager.builder()
    .withSdkKey(sdkKey)
    .withAuthDatafileToken(authDatafileToken);
    Optimizely optimizely = OptimizelyFactory.newDefaultInstance(builder.build());

Test plan

  • test the target url and authenticated header are set appropriately both when auth-token is specified (authenticated datafile) or not (regular datafaile)

@coveralls
Copy link

coveralls commented Jun 12, 2020

Pull Request Test Coverage Report for Build 1412

  • 19 of 20 (95.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 89.472%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java 15 16 93.75%
Totals Coverage Status
Change from base Build 1401: 0.04%
Covered Lines: 3952
Relevant Lines: 4417

💛 - Coveralls

* @param authDatafileToken Token for authenticated datafile access.
* @param fallback Fallback datafile string used by the ProjectConfigManager to be immediately available.
*/
public static Optimizely newDefaultInstance(String sdkKey, String authDatafileToken, String fallback) {
Copy link

Choose a reason for hiding this comment

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

In other SDKs we named it datafileAuthToken, could you please follow that for all variable & method names?

@@ -125,14 +130,30 @@ protected ProjectConfig poll() {
return null;
}

HttpGet createHttpRequest() {
Copy link

Choose a reason for hiding this comment

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

Should createHttpRequest be private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Private must be right one. This default access will be useful for header testing.

Copy link

Choose a reason for hiding this comment

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

Then let's use the VisibleForTesting annotation.

jaeopt and others added 2 commits June 15, 2020 17:13
@jaeopt jaeopt requested a review from mjc1283 June 16, 2020 00:43
@@ -125,14 +130,30 @@ protected ProjectConfig poll() {
return null;
}

HttpGet createHttpRequest() {
Copy link

Choose a reason for hiding this comment

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

Then let's use the VisibleForTesting annotation.

* @param datafileAuthToken Token for authenticated datafile access.
* @param fallback Fallback datafile string used by the ProjectConfigManager to be immediately available.
*/
public static Optimizely newDefaultInstance(String sdkKey, String datafileAuthToken, String fallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The cascading signatures here don't line up nicely. We should consistently call the next method in the chain.

public static Optimizely newDefaultInstance(String sdkKey) {
    ...
    return newDefaultInstance(sdkKey, null)
}

public static Optimizely newDefaultInstance(String sdkKey, String fallback) {
    ...
    String datafileAuthToken = PropertyUtils.get(HttpProjectConfigManager.CONFIG_SDK_AUTH_TOKEN);
    return newDefaultInstance(sdkKey, fallback, datafileAuthToken)
}

public static Optimizely newDefaultInstance(String sdkKey, String fallback, String datafileAuthToken) {
    ...
    return newDefaultInstance(builder.build(), notificationCenter);
}

Unfortunately these are all strings so order needs to be paid attention to. We should also provide a configuration option for datafileAuthToken so customers can deploy this via optimizely.properties and not have explicitly provide this value to the factory method. See the example above.

@jaeopt jaeopt requested review from mikecdavis and mjc1283 June 22, 2020 17:15
@@ -179,13 +179,29 @@ public static Optimizely newDefaultInstance(String sdkKey) {
* @param fallback Fallback datafile string used by the ProjectConfigManager to be immediately available.
*/
public static Optimizely newDefaultInstance(String sdkKey, String fallback) {
String datafileAuthToken = PropertyUtils.get(HttpProjectConfigManager.CONFIG_DATAFILE_AUTH_TOKEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been using datafileAccessToken in the other SDKs. Let's stick with the same name throughout.

@jaeopt jaeopt requested a review from mikeproeng37 June 22, 2020 20:31
@jaeopt jaeopt dismissed mikecdavis’s stale review June 22, 2020 23:17

On vacation. Will get it reviewed later.

Copy link

@pawels-optimizely pawels-optimizely left a comment

Choose a reason for hiding this comment

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

lgtm

@jaeopt jaeopt merged commit 0d3776a into master Jun 24, 2020
@jaeopt jaeopt deleted the jae/adf branch June 24, 2020 21:04
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.

6 participants