feat: Integrated ODPManager with UserContext and Optimizely client #490

zashraf1985 · 2022-10-27T17:30:18Z

Summary

This PR integrates ODP functionality with UserContext and Optimizely client to make it available for users. There are two ways to integrate ODPManager.

ODPManager.Builder.
Use default instance from OptimizelyFactory.

Test plan

Manually tested thoroughly
Will add unit tests after the first review passes.

Issues

FSSDK-8389, FS-8683

jaeopt

I reviewed the high level first and will look at more details again. The builder pattern for ODP configuration looks good. See comments for some changes suggested.

core-api/src/main/java/com/optimizely/ab/Optimizely.java

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

core-api/src/main/java/com/optimizely/ab/odp/ODPSegmentManager.java

jaeopt · 2022-10-28T23:51:59Z

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

@@ -319,10 +322,16 @@ public static Optimizely newDefaultInstance(ProjectConfigManager configManager,
            .withNotificationCenter(notificationCenter)
            .build();

+        ODPApiManager defaultODPApiManager = new DefaultODPApiManager();
+        ODPManager odpManager = ODPManager.builder()
+            .withApiManager(defaultODPApiManager)


Can't we let it use DefaultODPApiManager by default?

We cannot do that because DefaultODPApiManager implements http calls and is only available in the http-client project. The Core SDK does not know about its implementation. This is exactly how we inject polling datafile manager and batch event processor.

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

jaeopt

All changes look great! A few more changes suggested.
I also want to discuss a couple of possible extensions to support Android after wrapping up this PR.

core-api/src/main/java/com/optimizely/ab/Optimizely.java

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

jaeopt · 2022-11-03T20:37:57Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

        ODPManager odpManager = optimizely.getODPManager();
        if (odpManager != null) {
-            List<ODPSegmentOption> segmentOptions = new ArrayList<>();
-            if (ignoreCache) {
-                segmentOptions.add(ODPSegmentOption.IGNORE_CACHE);
-            }
-            if (resetCache) {
-                segmentOptions.add(ODPSegmentOption.RESET_CACHE);
+            synchronized (odpManager) {
+                setQualifiedSegments(odpManager.getSegmentManager().getQualifiedSegments(userId, segmentOptions));
            }


Suggested change

ODPManager odpManager = optimizely.getODPManager();

if (odpManager != null) {

List<ODPSegmentOption> segmentOptions = new ArrayList<>();

if (ignoreCache) {

segmentOptions.add(ODPSegmentOption.IGNORE_CACHE);

}

if (resetCache) {

segmentOptions.add(ODPSegmentOption.RESET_CACHE);

synchronized (odpManager) {

setQualifiedSegments(odpManager.getSegmentManager().getQualifiedSegments(userId, segmentOptions));

}

setQualifiedSegments(optimizely.fetchQualifiedSegments(userId))

This is the only place we have dependency on ODPManager in OptimizelyUserContext. Can we move this logic to Optimizely too?

I will do this but i just wanted to avoid method drilling. I do not want us to end up creating multiple levels of simple wrappers that do not add any value in between. This is why i preferred exposing ODPEventManager and ODPSegmentManager directly as getters. One option is to add wrappers for each ODP related function in the Optimizely client. Another option is to let userContext use ODPManager directly so that we do not need to keep adding wrappers as we add functionality to ODPManager. This wrapper mechanism gets specially tricky when we provide multiple overrides of methods. I would suggest to keep it like this and let userContext access ODPManager through a getter in optimizely client and call its methods direclty.. what do you say?

@zashraf1985 it looks like a style preference with tradeoffs. I'll leave it to you.

core-api/src/main/java/com/optimizely/ab/odp/ODPManager.java

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

jaeopt · 2022-11-03T21:05:37Z

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

+
+        String query = String.format("query($userId: String, $audiences: [String]) {customer(%s: $userId) {audiences(subset: $audiences) {edges {node {name state}}}}}", userKey);
+        String variables = String.format("{\"userId\": \"%s\", \"audiences\": [%s]}", userValue, segmentsString);
+        String requestPayload = String.format("{\"query\": \"%s\", \"variables\": %s}", query, variables);


Looks like no gain at all in Java with this new format. I guess you're trying to avoid use JSON parser here :)

There is still a gain. We are now using graphQL json format which takes variables separately which means if someone wants to inject anything in the query, it wont work because we are not concatenating variables directly in the query, we are passing them as variables.

jaeopt

Async solution looks good to me. A few more small changes suggested.

jaeopt · 2022-11-14T17:15:57Z

core-api/src/main/java/com/optimizely/ab/Optimizely.java

        ODPManager odpManager = getODPManager();
        if (odpManager != null) {
            odpManager.getEventManager().identifyUser(userId);
        }
    }

-    public void identifyUser(@Nullable String vuid, String userId) {
+    public void identifyUser(@Nullable String vuid, @Nullable String userId) {


Wondering if need this api here. If we can determine if userId is vuid or fsUserId, we do not need this call here.

jaeopt · 2022-11-14T17:24:13Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

@@ -299,21 +300,16 @@ public void setQualifiedSegments(List<String> qualifiedSegments) {

    /**
     * Fetch all qualified segments for the user context.
-     *
+     * <p>
     * The segments fetched will be saved and can be accessed at any time by calling {@link #getQualifiedSegments()}.
     */
    public void fetchQualifiedSegments() {


Can we return boolean (true/false for success/failure) for blocking fetchSegment calls. Clients can check the status before continuing with decide call?

jaeopt · 2022-11-14T17:25:39Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

+        ODPManager odpManager = optimizely.getODPManager();
+        if (odpManager == null) {
+            logger.error("Audience segments fetch failed (ODP is not enabled");
+            callback.invokeCallback();


We can also pass the status boolean (success/failure) in the callback parameter.

jaeopt · 2022-11-14T17:28:47Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

+            logger.error("Audience segments fetch failed (ODP is not enabled");
+            callback.invokeCallback();
+        } else {
+            odpManager.getSegmentManager().getQualifiedSegments(userId, segments -> {


If we want to keep this direct call to odpManager, I suggest to make "optimizely.identifyUser" call also direct call (for consistency). I still prefer moving all to optimizely :) I'm concerned about the increasing complexity of UserContext.

Removed the direct call and made a wrapper in optimizely

jaeopt · 2022-11-14T17:30:59Z

core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext.java

+                    setQualifiedSegments(segments);
+                }
+                callback.invokeCallback();
+            }, segmentOptions);


This async solution looks good!

jaeopt · 2022-11-14T17:43:03Z

core-api/src/main/java/com/optimizely/ab/odp/ODPSegmentCallback.java

+
+@FunctionalInterface
+public interface ODPSegmentCallback {
+    void invokeCallback();


This name sounds redundant. What about onCompleted or onFetchComplemeted?

changed it to onCompleted

jaeopt · 2022-11-14T17:43:38Z

core-api/src/main/java/com/optimizely/ab/odp/ODPSegmentCallback.java

+
+@FunctionalInterface
+public interface ODPSegmentCallback {
+    void invokeCallback();


Suggested change

void invokeCallback();

void onCompleted(bool status);

jaeopt · 2022-11-14T17:49:15Z

core-api/src/main/java/com/optimizely/ab/internal/Cache.java

+    Integer getMaxSize();
+    Integer getCacheTimeoutSeconds();


Why adding these to Cache interface? We do not care about these when clients implement their own cache with own size and timeout.

I needed these for some unit tests. I cant figure out any other way to verify some tests

I removed these methods and tests checking for these values.

jaeopt · 2022-11-14T17:54:28Z

core-api/src/main/java/com/optimizely/ab/odp/ODPSegmentManager.java

@@ -106,15 +106,66 @@ public List<String> getQualifiedSegments(ODPUserKey userKey, String userValue, L
        return qualifiedSegments;
    }

+    public void getQualifiedSegments(ODPUserKey userKey, String userValue, ODPSegmentFetchCallback callback, List<ODPSegmentOption> options) {


Looks good!

jaeopt

LGTM!

Integrated ODPManager with UserContext and Optimizely

525ad99

zashraf1985 requested a review from a team as a code owner October 27, 2022 17:30

optimizely-codereview-bot assigned zashraf1985 Oct 27, 2022

fixed an issue in DefaultODPApiManager

5d3cb21

zashraf1985 removed their assignment Oct 27, 2022

optimizely-codereview-bot added the 🤖 4 hrs label Oct 27, 2022

Calling close on ODPEventManager when Optimizely closes

0a1bf21

optimizely-codereview-bot added 🤖 8 hrs 🤖 12 hrs and removed 🤖 4 hrs 🤖 8 hrs labels Oct 28, 2022

jaeopt requested changes Oct 29, 2022

View reviewed changes

optimizely-codereview-bot removed the 🤖 12 hrs label Oct 29, 2022

optimizely-codereview-bot assigned zashraf1985 Oct 29, 2022

zashraf1985 added 2 commits November 2, 2022 09:44

Incorporated review feedback.

0c74a18

Changed GraphQl query to use variables for better sanitization.

42b2425

jaeopt requested changes Nov 3, 2022

View reviewed changes

zashraf1985 added 3 commits November 4, 2022 12:41

Incorporated more review feedback.

79263fc

Added tests for Optimizely

e573a6a

Completed the unit tests.

6d88b86

zashraf1985 removed their assignment Nov 9, 2022

optimizely-codereview-bot requested a review from jaeopt November 9, 2022 00:22

Added Non blocking implementation for fetching segments.

68aa5ff

jaeopt requested changes Nov 14, 2022

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Nov 14, 2022

zashraf1985 added 2 commits November 16, 2022 16:25

Incorporated review feedback

29f2b09

removed redundant methods from cache

49a9686

zashraf1985 removed their assignment Nov 17, 2022

optimizely-codereview-bot requested a review from jaeopt November 17, 2022 00:39

optimizely-codereview-bot added the 🤖 4 hrs label Nov 17, 2022

optimizely-codereview-bot added 🤖 8 hrs 🤖 12 hrs and removed 🤖 4 hrs 🤖 8 hrs labels Nov 18, 2022

jaeopt approved these changes Nov 18, 2022

View reviewed changes

zashraf1985 merged commit b16dff5 into master Nov 21, 2022

zashraf1985 deleted the zeeshan/ats-integration branch November 21, 2022 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Integrated ODPManager with UserContext and Optimizely client #490

feat: Integrated ODPManager with UserContext and Optimizely client #490

zashraf1985 commented Oct 27, 2022 •

edited

Loading

jaeopt left a comment

jaeopt Oct 28, 2022

zashraf1985 Nov 2, 2022

jaeopt left a comment

jaeopt Nov 3, 2022

zashraf1985 Nov 4, 2022 •

edited

Loading

jaeopt Nov 4, 2022

jaeopt Nov 3, 2022

zashraf1985 Nov 4, 2022

jaeopt left a comment

jaeopt Nov 14, 2022

zashraf1985 Nov 17, 2022

jaeopt Nov 14, 2022

zashraf1985 Nov 17, 2022

jaeopt Nov 14, 2022

zashraf1985 Nov 17, 2022

jaeopt Nov 14, 2022

zashraf1985 Nov 17, 2022

jaeopt Nov 14, 2022

jaeopt Nov 14, 2022

zashraf1985 Nov 17, 2022

jaeopt Nov 14, 2022

jaeopt Nov 14, 2022

zashraf1985 Nov 17, 2022

zashraf1985 Nov 17, 2022

jaeopt Nov 14, 2022

jaeopt left a comment

feat: Integrated ODPManager with UserContext and Optimizely client #490

feat: Integrated ODPManager with UserContext and Optimizely client #490

Conversation

zashraf1985 commented Oct 27, 2022 • edited Loading

Summary

Test plan

Issues

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zashraf1985 Nov 4, 2022 • 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

jaeopt left a 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

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

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

jaeopt left a comment

Choose a reason for hiding this comment

zashraf1985 commented Oct 27, 2022 •

edited

Loading

zashraf1985 Nov 4, 2022 •

edited

Loading