feat(api): Accepting all types for attributes values #207

shihpatrick · 2018-08-21T23:49:34Z

Summary

As a part of Audience Match Type, I changed all relevant function signatures so that it accepts an Object variable rather than just a String for functions that accept and uses attributes. Any filtering of types would be done later, when evaluating audience conditions.

Testing

Passes all current unit tests. Further testing of other types for attribute values (like filtering out types that aren't Boolean, String, or Number will be done when dealing with audience conditions.

coveralls · 2018-08-21T23:57:36Z

Pull Request Test Coverage Report for Build 599

20 of 22 (90.91%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.02%) to 89.388%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/Optimizely.java	6	7	85.71%
core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java	8	9	88.89%

Totals
Change from base Build 589:	-0.02%
Covered Lines:	2350
Relevant Lines:	2629

💛 - Coveralls

tylerbrandt · 2018-08-22T00:42:21Z

build

mikeproeng37 · 2018-08-22T01:12:01Z

build-e2e

thomaszurkan-optimizely

Let’s add a instance of check for type string before cast for the bucketing id.

thomaszurkan-optimizely · 2018-08-22T10:43:43Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

@@ -131,7 +131,7 @@ public DecisionService(@Nonnull Bucketer bucketer,
        if (ExperimentUtils.isUserInExperiment(projectConfig, experiment, filteredAttributes)) {
            String bucketingId = userId;
            if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {
-                bucketingId = filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
+                bucketingId = (String) filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());


We should probably check the type just to be safe.

If it isn't a string, should we keep it as userId?

Correct, we don't use the bucketingId if it's not a string and instead keep using the userId

And I assume you're on top of this, but we should log a ~warning if we're ignoring the bucketingId due to type mismatch.

And while you're at it...we should also log at ~info level in cases where do ingest a usable bucketingId!

thomaszurkan-optimizely · 2018-08-22T13:57:53Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

@@ -212,7 +212,7 @@ public DecisionService(@Nonnull Bucketer bucketer,
        int rolloutRulesLength = rollout.getExperiments().size();
        String bucketingId = userId;
        if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {


I think we should check for null value and that the value is a string.

mikeproeng37 · 2018-08-22T17:26:50Z

core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java

-    public boolean evaluate(Map<String, String> attributes) {
-        String userAttributeValue = attributes.get(name);
+    public boolean evaluate(Map<String, ?> attributes) {
+        Object userAttributeValue = attributes.get(name);


Hmm should be careful with this. Eventually when we do accept objects and arrays to be passed in, the equals evaluation below will likely perform a reference check as well. That said, I think it's fine for now, but just leave a message saying that we should be extra considerate when we decide to allow objects and arrays.

mikeproeng37

Please add unit tests for the different value types that we support: integers, doubles, booleans, strings in the activate API

wangjoshuah · 2018-08-22T18:05:27Z

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

@@ -765,7 +766,7 @@ public UserProfileService getUserProfileService() {
        if (unknownAttributes != null) {
            logger.warn("Attribute(s) {} not in the datafile.", unknownAttributes);
            // make a copy of the passed through attributes, then remove the unknown list
-            attributes = new HashMap<String, String>(attributes);
+            attributes = new HashMap<String, Object>(attributes);


You should add a test that actually removes unknown attributes to see what happens here. But my understanding is that Object is actually different from ?.
See https://stackoverflow.com/questions/678822/what-is-the-difference-between-and-object-in-java-generics

You can't use a question mark in a constructor.

I actually think that the proper way to use the constructor is HashMap<>(attributes). So, @wangjoshuah is correct.

Please update to something like:
HashMap<String, ?> attr = new HashMap<>();

Looks like <> was not introduced until Java 7 and we're currently on Java 6. Thoughts?

https://stackoverflow.com/questions/16612157/why-does-new-hashmap-produce-an-error-in-jdk-1-6-but-not-1-7?lq=1

Okay, it sounds like

new HashMap<String, Object>();

is indeed the way to go...

#208 is the way to go. Good call @thomaszurkan-optimizely

thomaszurkan-optimizely

lets change that constructor to new HashMap<>(attributes)

thomaszurkan-optimizely · 2018-08-22T18:34:10Z

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

@@ -765,7 +766,7 @@ public UserProfileService getUserProfileService() {
        if (unknownAttributes != null) {
            logger.warn("Attribute(s) {} not in the datafile.", unknownAttributes);
            // make a copy of the passed through attributes, then remove the unknown list
-            attributes = new HashMap<String, String>(attributes);
+            attributes = new HashMap<String, Object>(attributes);


I actually think that the proper way to use the constructor is HashMap<>(attributes). So, @wangjoshuah is correct.

Please update to something like:
HashMap<String, ?> attr = new HashMap<>();

…k into pshih/attribute-types

shihpatrick · 2018-08-24T22:31:53Z

build

shihpatrick · 2018-08-24T23:07:23Z

build

thomaszurkan-optimizely · 2018-08-27T18:58:31Z

build

mikeproeng37 · 2018-08-27T21:42:33Z

build

shihpatrick · 2018-08-28T00:59:48Z

build

shihpatrick · 2018-08-28T23:58:29Z

Testing the changes

This e2e test does not have the proper configuration and the appropriate configuration is on Jenkins 4318.

Testing the original code

I pieced together some material to hopefully override the current failing of the e2e tests.
I wanted to see if the errors are consistent with the original masters of both the java-sdk and java-testapp.

ran a e2e test with parameters:
- master for java-sdk
- pshih/revert-func-signatures for java-testapp

Comparison

If we compare the consoles of the two tests, we would notice that the errors from the first test that includes changes to attributes are the same as the errors from the original code (but there is one more error that shows up in the original code*). This may suggest that the changes in this PR does not cause the errors.

*The extra error caused is with tests/test_user_profiles.py:285

@mikeng13 @thomaszurkan-optimizely

mikeproeng37 · 2018-08-29T00:43:21Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

+                    logger.info("bucketingId is valid");
+                } 
+                else {
+                    logger.warn("bucketingID has type mismatch, defaulted to userId ");


nit: This should just say BucketingID attribute is not a string. Defaulted to userId.

mikeproeng37 · 2018-08-29T00:44:25Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

-                bucketingId = filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
+                if (String.class.isInstance(filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString()))) {
+                    bucketingId = (String) filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
+                    logger.info("bucketingId is valid");


nit: would be useful to print out what that bucketing ID is. Also, this should be a debug message.

mikeproeng37 · 2018-08-29T00:45:07Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

@@ -212,7 +218,13 @@ public DecisionService(@Nonnull Bucketer bucketer,
        int rolloutRulesLength = rollout.getExperiments().size();
        String bucketingId = userId;
        if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {
-            bucketingId = filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
+            if (String.class.isInstance(filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString()))) {


This logic is repeated from up above. Can you consolidate into a helper function?

mikeproeng37 · 2018-08-29T00:46:57Z

core-api/src/main/java/com/optimizely/ab/config/audience/Condition.java

@@ -23,5 +23,5 @@
 */
 public interface Condition {

-    boolean evaluate(Map<String, String> attributes);
+    boolean evaluate(Map<String, ?> attributes);


Update the header on this file to be 2016-2018. Please also do it for other files you've touched in this commit.

mikeproeng37 · 2018-08-29T00:47:27Z

core-api/src/main/java/com/optimizely/ab/config/audience/NotCondition.java

@@ -37,7 +37,7 @@ public Condition getCondition() {
        return condition;
    }

-    public boolean evaluate(Map<String, String> attributes) {
+    public boolean evaluate(Map<String, ?> attributes) {


Do we need to update the toString method on this class to account for non-string values? Seems like it could lead to exceptions

mikeproeng37 · 2018-08-29T00:47:38Z

core-api/src/main/java/com/optimizely/ab/config/audience/OrCondition.java

@@ -36,7 +36,7 @@ public OrCondition(@Nonnull List<Condition> conditions) {
        return conditions;
    }

-    public boolean evaluate(Map<String, String> attributes) {
+    public boolean evaluate(Map<String, ?> attributes) {


Same as above re: the toString method

mikeproeng37 · 2018-08-29T00:49:35Z

core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java


-    public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String value) {
+    public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable Object value) {


@thomaszurkan-optimizely do we need to overload this constructor to allow you to pass in a String value? I don't think this change will break current usage of the class.

Did you mean Object value?
Would this change be more appropriate when dealing with audience conditions?

Nevermind, I think this will be fine as is. Can ignore my previous comment.

mikeproeng37 · 2018-08-29T00:53:30Z

core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java

@@ -43,6 +43,7 @@
 import com.optimizely.ab.notification.NotificationCenter;
 import com.optimizely.ab.notification.TrackNotificationListener;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.hamcrest.Matcher;


Is this still needed? Maybe I am missing where it's used

Accidentally left that in, my bad!

mikeproeng37 · 2018-08-29T00:55:08Z

core-api/src/test/java/com/optimizely/ab/notification/NotificationCenterTest.java

@@ -70,7 +70,7 @@ public void onActivate(@Nonnull Experiment experiment, @Nonnull String userId, @
    public void testAddActivateNotification() {
        int notificationId = notificationCenter.addActivateNotificationListener(new ActivateNotificationListenerInterface() {
            @Override
-            public void onActivate(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map<String, String> attributes, @Nonnull Variation variation, @Nonnull LogEvent event) {
+            public void onActivate(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map<String, ?> attributes, @Nonnull Variation variation, @Nonnull LogEvent event) {


Will this break current usage of the notification listeners? If someone has already implemented notification listeners with just Map<String, String>, will they suddenly see build errors?

It would break current usage (I had to make changes in unit test and the java-testapp). Does it make sense to leave it as Map<String, String> though? I think there would be errors if someone tries to use Notification Listeners on attributes with Map<String, Object> if we leave this as Map<String, String>.

Maybe provide both as abstract methods?

Let's hold off on this until we discuss it today. My inclination is that we leave Map<String, String>, and in our notification center, coerce all values to String types

From today's discussion -- looks like we're using Map<String, ?>

Yep we are.

mikeproeng37

Almost there! I just have a few small suggestions

mikeproeng37 · 2018-08-30T17:00:02Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

@@ -343,4 +337,25 @@ void saveVariation(@Nonnull Experiment experiment,
            }
        }
    }
+
+    /**
+     * @param userId The userId of the user.


Please provide a description for this method. Also, please make it private as it is not used outside this class.

mikeproeng37 · 2018-08-30T17:00:15Z

core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java

-            if (filteredAttributes.containsKey(ControlAttribute.BUCKETING_ATTRIBUTE.toString())) {
-                bucketingId = filteredAttributes.get(ControlAttribute.BUCKETING_ATTRIBUTE.toString());
-            }
+            String bucketingId = getBucketingId(userId,filteredAttributes);


nit: space after filteredAttributes

mikeproeng37 · 2018-08-30T17:01:24Z

core-api/src/main/java/com/optimizely/ab/config/audience/Audience.java

@@ -1,6 +1,6 @@
 /**
 *
- *    Copyright 2016-2017, Optimizely and contributors
+ *    Copyright 2016-2018, Optimizely and contributors


You didn't change this class so no need to update the year.

…k into pshih/attribute-types

mikeproeng37 · 2018-08-30T20:09:20Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationListener.java

@@ -1,6 +1,6 @@
 /**
 *
- *    Copyright 2016-2017, Optimizely and contributors
+ *    Copyright 2016-2018, Optimizely and contributors


Please also revert this change since we aren't touching this file this year

mikeproeng37

lgtm

addressed

patrickshih-optimizely added 6 commits August 16, 2018 11:38

wildcard replaces string; implement type checking

c401466

deleting invalid types for filtered attributes

d73f4eb

attribute with null values are filtered out

6173eee

resolved issues with null

be4de1c

paramater/return changes

0b23827

remove filtering invalid types

cc9a5a6

shihpatrick requested review from mikeproeng37 and thomaszurkan-optimizely August 21, 2018 23:57

Merge branch 'master' into pshih/attribute-types

20f83bf

thomaszurkan-optimizely suggested changes Aug 22, 2018

View reviewed changes

mikeproeng37 reviewed Aug 22, 2018

View reviewed changes

mikeproeng37 suggested changes Aug 22, 2018

View reviewed changes

shihpatrick changed the title ~~refactor(api): Accepting all types for attributes values~~ feat(api): Accepting all types for attributes values Aug 22, 2018

wangjoshuah reviewed Aug 22, 2018

View reviewed changes

thomaszurkan-optimizely previously requested changes Aug 22, 2018

View reviewed changes

patrickshih-optimizely and others added 10 commits August 22, 2018 16:01

added from suggestions + unit tests

881774f

Merge branch 'master' into pshih/attribute-types

6274260

logger, work-around for unit test

a47f8af

diamond notation for hashmap

e49c473

Merge branch 'master' into pshih/attribute-types

20974f4

Merge branch 'pshih/attribute-types' of github.com:optimizely/java-sd…

6ef444d

…k into pshih/attribute-types

funky casting to get Map<String,?> working

7c868cb

fixed unit tests

9f0593f

notification listener, features function signature change

b484960

notif. listener tests function signature change

4aa66fe

mikeproeng37 closed this Aug 27, 2018

mikeproeng37 reopened this Aug 27, 2018

mikeproeng37 suggested changes Aug 29, 2018

View reviewed changes

patrickshih-optimizely added 3 commits August 29, 2018 13:37

code review changes

7e1dd58

Update valid-project-config-v4.json

a17bdbd

Update valid-project-config-v4.json

f90b466

mikeproeng37 suggested changes Aug 30, 2018

View reviewed changes

patrickshih-optimizely added 2 commits August 30, 2018 10:36

small changes

d9d23e2

Merge branch 'pshih/attribute-types' of github.com:optimizely/java-sd…

0b6aad9

…k into pshih/attribute-types

mikeproeng37 reviewed Aug 30, 2018

View reviewed changes

Update NotificationListener.java

182c4fe

mikeproeng37 approved these changes Aug 30, 2018

View reviewed changes

shihpatrick merged commit 4ecd765 into master Aug 31, 2018

shihpatrick deleted the pshih/attribute-types branch September 5, 2018 01:13


		public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String value) {
		public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable Object value) {

feat(api): Accepting all types for attributes values #207

feat(api): Accepting all types for attributes values #207

Uh oh!

Conversation

shihpatrick commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 599

💛 - Coveralls

Uh oh!

tylerbrandt commented Aug 22, 2018

Uh oh!

mikeproeng37 commented Aug 22, 2018

Uh oh!

thomaszurkan-optimizely 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!

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!

mikeproeng37 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!

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!

thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihpatrick commented Aug 24, 2018

Uh oh!

shihpatrick commented Aug 24, 2018

Uh oh!

thomaszurkan-optimizely commented Aug 27, 2018

Uh oh!

mikeproeng37 commented Aug 27, 2018

Uh oh!

shihpatrick commented Aug 28, 2018

Uh oh!

shihpatrick commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing the changes

Testing the original code

Comparison

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!

Choose a reason for hiding this comment

shihpatrick commented Aug 21, 2018 •

edited

Loading

coveralls commented Aug 21, 2018 •

edited

Loading

shihpatrick commented Aug 28, 2018 •

edited

Loading

shihpatrick Aug 29, 2018 •

edited

Loading