fix(api): Only track attributes with valid attribute types. (#103)

mfahadahmed · mikeproeng37 · commit 17c718118273 · 2018-09-20T14:51:33.000-07:00
diff --git a/OptimizelySDK.Tests/EventTests/EventBuilderTest.cs b/OptimizelySDK.Tests/EventTests/EventBuilderTest.cs
@@ -328,6 +328,115 @@ public void TestCreateImpressionEventWithTypedAttributes()
             Assert.IsTrue(TestData.CompareObjects(expectedLogEvent, logEvent));
         }
 
+        [Test]
+        public void TestCreateImpressionEventRemovesInvalidAttributesFromPayload()
+        {
+            var guid = Guid.NewGuid();
+            var timeStamp = TestData.SecondsSince1970();
+
+            var payloadParams = new Dictionary<string, object>
+            {
+                { "visitors", new object[]
+                    {
+                        new Dictionary<string, object>()
+                        {
+                            { "snapshots", new object[]
+                                {
+                                    new Dictionary<string, object>
+                                    {
+                                        { "decisions", new object[]
+                                            {
+                                                new Dictionary<string, object>
+                                                {
+                                                    {"campaign_id", "7719770039" },
+                                                    {"experiment_id", "7716830082" },
+                                                    {"variation_id", "7722370027" }
+                                                }
+                                            }
+                                        },
+                                        { "events", new object[]
+                                            {
+                                                new Dictionary<string, object>
+                                                {
+                                                    {"entity_id", "7719770039" },
+                                                    {"timestamp", timeStamp },
+                                                    {"uuid", guid },
+                                                    {"key", "campaign_activated" }
+                                                }
+                                            }
+                                        }
+                                    }
+                                }
+                            },
+                            {"attributes", new object[]
+                                {
+                                    new Dictionary<string, object>
+                                    {
+                                        {"entity_id", "7723280020" },
+                                        {"key", "device_type" },
+                                        {"type", "custom" },
+                                        {"value", "iPhone"}
+                                    },
+                                    new Dictionary<string, object>
+                                    {
+                                        {"entity_id", "323434545" },
+                                        {"key", "boolean_key" },
+                                        {"type", "custom" },
+                                        {"value", true}
+                                    },
+                                    new Dictionary<string, object>
+                                    {
+                                        {"entity_id", "808797686" },
+                                        {"key", "double_key" },
+                                        {"type", "custom" },
+                                        {"value", 3.14}
+                                    },
+                                    new Dictionary<string, object>
+                                    {
+                                        {"entity_id", ControlAttributes.BOT_FILTERING_ATTRIBUTE},
+                                        {"key", ControlAttributes.BOT_FILTERING_ATTRIBUTE},
+                                        {"type", "custom" },
+                                        {"value", true }
+                                    }
+                                }
+                            },
+                            { "visitor_id", TestUserId }
+                        }
+                    }
+                },
+                {"project_id", "7720880029" },
+                {"account_id", "1592310167" },
+                {"client_name", "csharp-sdk" },
+                {"client_version", Optimizely.SDK_VERSION },
+                {"revision", "15" },
+                {"anonymize_ip", false}
+            };
+
+            var expectedLogEvent = new LogEvent("https://logx.optimizely.com/v1/events",
+                payloadParams,
+                "POST",
+                new Dictionary<string, string>
+                {
+                    { "Content-Type", "application/json" }
+                });
+
+            var userAttributes = new UserAttributes
+            {
+                {"device_type", "iPhone" },
+                {"boolean_key", true },
+                {"double_key", 3.14 },
+                { "", "Android" },
+                { "null", null },
+                { "objects", new object() },
+                { "arrays", new string[] { "a", "b", "c" } },
+            };
+
+            var logEvent = EventBuilder.CreateImpressionEvent(Config, Config.GetExperimentFromKey("test_experiment"), "7722370027", TestUserId, userAttributes);
+            TestData.ChangeGUIDAndTimeStamp(logEvent.Params, timeStamp, guid);
+
+            Assert.IsTrue(TestData.CompareObjects(expectedLogEvent, logEvent));
+        }
+
         [Test]
         public void TestCreateConversionEventNoAttributesNoValue()
         {
diff --git a/OptimizelySDK.Tests/OptimizelyTest.cs b/OptimizelySDK.Tests/OptimizelyTest.cs
@@ -401,9 +401,12 @@ public void TestActivateWithAttributes()
             EventBuilderMock.Verify(b => b.CreateImpressionEvent(It.IsAny<ProjectConfig>(), Config.GetExperimentFromKey("test_experiment"),
                     "7722370027", "test_user", OptimizelyHelper.UserAttributes), Times.Once);
 
+
             LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(6));
+            LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"test_user\" is not in the forced variation map."), Times.Once);
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Assigned bucket [3037] to user [test_user] with bucketing ID [test_user]."), Times.Once);
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "User [test_user] is in variation [control] of experiment [test_experiment]."), Times.Once);
+            LoggerMock.Verify(l => l.Log(LogLevel.INFO, "This decision will not be saved since the UserProfileService is null."));
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Activating user test_user in experiment test_experiment."), Times.Once);
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, @"Dispatching impression event to URL logx.optimizely.com/decision with params {""param1"":""val1""}."), Times.Once);
 
@@ -423,17 +426,14 @@ public void TestActivateWithNullAttributes()
             var variation = (Variation)optly.Invoke("Activate", "test_experiment", "test_user", OptimizelyHelper.NullUserAttributes);
 
             EventBuilderMock.Verify(b => b.CreateImpressionEvent(It.IsAny<ProjectConfig>(), Config.GetExperimentFromKey("test_experiment"),
-                    "7722370027", "test_user", OptimizelyHelper.UserAttributes), Times.Once);
+                    "7722370027", "test_user", OptimizelyHelper.NullUserAttributes), Times.Once);
 
-            LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(9));
+            LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(6));
 
             //"User "test_user" is not in the forced variation map."
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"test_user\" is not in the forced variation map."), Times.Once);
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Assigned bucket [3037] to user [test_user] with bucketing ID [test_user]."), Times.Once);
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "User [test_user] is in variation [control] of experiment [test_experiment]."), Times.Once);
-            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key null_value removed and will not be sent to results."), Times.Once);
-            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key wont_be_sent removed and will not be sent to results."), Times.Once);
-            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key bad_food removed and will not be sent to results."), Times.Once);
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Activating user test_user in experiment test_experiment."), Times.Once);
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, @"Dispatching impression event to URL logx.optimizely.com/decision with params {""param1"":""val1""}."), Times.Once);
 
@@ -683,7 +683,7 @@ public void TestTrackWithNullAttributesWithNullEventValue()
                 { "wont_send_null", null}
             });
 
-            LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(21));
+            LoggerMock.Verify(l => l.Log(It.IsAny<LogLevel>(), It.IsAny<string>()), Times.Exactly(18));
 
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"test_user\" is not in the forced variation map."), Times.Exactly(3));
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Assigned bucket [3037] to user [test_user] with bucketing ID [test_user]."));
@@ -698,9 +698,6 @@ public void TestTrackWithNullAttributesWithNullEventValue()
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "This decision will not be saved since the UserProfileService is null."));
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Experiment \"paused_experiment\" is not running."));
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Not tracking user \"test_user\" for experiment \"paused_experiment\""));
-            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key null_value removed and will not be sent to results."));
-            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key wont_be_sent removed and will not be sent to results."));
-            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[UserAttributes] Null value for key bad_food removed and will not be sent to results."));
             LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "[EventTags] Null value for key wont_send_null removed and will not be sent to results."));
             LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Tracking event purchase for user test_user."));
             LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Dispatching conversion event to URL logx.optimizely.com/track with params {\"param1\":\"val1\"}."));
diff --git a/OptimizelySDK.Tests/UtilsTests/ValidatorTest.cs b/OptimizelySDK.Tests/UtilsTests/ValidatorTest.cs
@@ -1,5 +1,5 @@
 ﻿/* 
- * Copyright 2017, Optimizely
+ * Copyright 2017-2018, Optimizely
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -126,6 +126,7 @@ public void TestAreEventTagsValidValidEventTags()
             }));
         }
 
+        [Test]
         public void TestAreEventTagsValidInvalidTags()
         {
             // Some of the tests cases are not applicable because C# is strongly typed.
@@ -136,5 +137,46 @@ public void TestAreEventTagsValidInvalidTags()
             }));
         }
 
+        [Test]
+        public void TestIsUserAttributeValidWithValidValues()
+        {
+            var userAttributes = new UserAttributes
+            {
+                { "device_type", "Android" },
+                { "is_firefox", true },
+                { "num_users", 15 },
+                { "pi_value", 3.14 }
+            };
+
+            foreach (var attribute in userAttributes)
+                Assert.True(Validator.IsUserAttributeValid(attribute));
+        }
+
+        [Test]
+        public void TestIsUserAttributeValidWithInvalidValues()
+        {
+            var invalidUserAttributes = new UserAttributes
+            {
+                { "objects", new object() },
+                { "arrays", new string[] { "a", "b", "c" } }
+            };
+
+            foreach (var attribute in invalidUserAttributes)
+                Assert.False(Validator.IsUserAttributeValid(attribute));
+        }
+
+        [Test]
+        public void TestIsUserAttributeValidWithEmptyKeyOrValue()
+        {
+            var validUserAttributes = new UserAttributes
+            {
+                { "", "Android" },
+                { "integer", 0 },
+                { "string", string.Empty }
+            };
+
+            foreach (var attribute in validUserAttributes)
+                Assert.True(Validator.IsUserAttributeValid(attribute));    
+        }
     }
 }
diff --git a/OptimizelySDK/Entity/UserAttributes.cs b/OptimizelySDK/Entity/UserAttributes.cs
@@ -20,17 +20,6 @@ namespace OptimizelySDK.Entity
 {
     public class UserAttributes : Dictionary<string, object>
     {
-        public UserAttributes FilterNullValues(ILogger logger)
-        {
-            UserAttributes answer = new UserAttributes();
-            foreach (KeyValuePair<string, object> pair in this) {
-                if (pair.Value != null) {
-                    answer[pair.Key] = pair.Value;
-                } else {
-                    logger.Log(LogLevel.ERROR, string.Format("[UserAttributes] Null value for key {0} removed and will not be sent to results.", pair.Key));
-                }
-            }
-            return answer;
-        }
+        
     }
 }
diff --git a/OptimizelySDK/Event/Builder/EventBuilder.cs b/OptimizelySDK/Event/Builder/EventBuilder.cs
@@ -85,7 +85,7 @@ private Dictionary<string, object> GetCommonParams(ProjectConfig config, string
                     { "snapshots", new object[0]},
                     { "visitor_id", userId },
                     { "attributes", new object[0] }
-                };
+            };
 
             comonParams[Params.VISITORS] = new object[] { visitor };
             comonParams[Params.PROJECT_ID] = config.ProjectId;
@@ -97,20 +97,23 @@ private Dictionary<string, object> GetCommonParams(ProjectConfig config, string
 
             var userFeatures = new List<Dictionary<string, object>>();
 
-            foreach (var userAttribute in userAttributes.Where(a => !string.IsNullOrEmpty(a.Key))) {
-                var attributeId = config.GetAttributeId(userAttribute.Key);
+            //Omit attribute values that are not supported by the log endpoint.
+            foreach (var validUserAttribute in userAttributes.Where(attribute => Validator.IsUserAttributeValid(attribute)))
+            {                
+                var attributeId = config.GetAttributeId(validUserAttribute.Key);
                 if (!string.IsNullOrEmpty(attributeId)) {
                     userFeatures.Add(new Dictionary<string, object>
                     {
-                            { "entity_id", attributeId },
-                            { "key", userAttribute.Key },
-                            { "type", CUSTOM_ATTRIBUTE_FEATURE_TYPE },
-                            { "value", userAttribute.Value}
-                        });
+                        { "entity_id", attributeId },
+                        { "key", validUserAttribute.Key },
+                        { "type", CUSTOM_ATTRIBUTE_FEATURE_TYPE },
+                        { "value", validUserAttribute.Value}
+                    });
                 }
             }
 
-            if (config.BotFiltering.HasValue) {
+            if (config.BotFiltering.HasValue)
+            {
                 userFeatures.Add(new Dictionary<string, object>
                 {
                         { "entity_id", ControlAttributes.BOT_FILTERING_ATTRIBUTE },
@@ -121,6 +124,7 @@ private Dictionary<string, object> GetCommonParams(ProjectConfig config, string
             }
 
             visitor["attributes"] = userFeatures;
+
             return comonParams;
         }
 
diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs
@@ -195,10 +195,6 @@ public Variation Activate(string experimentKey, string userId, UserAttributes us
                 return null;
             }
 
-            if (userAttributes != null) {
-                userAttributes = userAttributes.FilterNullValues(Logger);
-            }
-
             SendImpressionEvent(experiment, variation, userId, userAttributes);
 
             return variation;
@@ -270,11 +266,6 @@ public void Track(string eventKey, string userId, UserAttributes userAttributes
             if (validExperimentIdToVariationMap.Count > 0)
             {
 
-                if (userAttributes != null)
-                {
-                    userAttributes = userAttributes.FilterNullValues(Logger);
-                }
-
                 if (eventTags != null)
                 {
                     eventTags = eventTags.FilterNullValues(Logger);
diff --git a/OptimizelySDK/Utils/Validator.cs b/OptimizelySDK/Utils/Validator.cs
@@ -1,5 +1,5 @@
 ﻿/* 
- * Copyright 2017, Optimizely
+ * Copyright 2017-2018, Optimizely
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 using OptimizelySDK.Entity;
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using Attribute = OptimizelySDK.Entity.Attribute;
@@ -22,7 +23,6 @@ namespace OptimizelySDK.Utils
 {
     public static class Validator
     {
-
         /// <summary>
         /// Validate the ProjectConfig JSON
         /// </summary>
@@ -87,7 +87,7 @@ public static bool AreEventTagsValid(Dictionary<string, object> eventTags) {
         public static bool IsFeatureFlagValid(ProjectConfig projectConfig, FeatureFlag featureFlag)
         {
             var experimentIds = featureFlag.ExperimentIds;
-            
+
             if (experimentIds == null || experimentIds.Count <= 1)
                 return true;
 
@@ -102,6 +102,18 @@ public static bool IsFeatureFlagValid(ProjectConfig projectConfig, FeatureFlag f
 
             return true;
         }
+
+        /// <summary>
+        /// Determine if given user attribute is valid.
+        /// </summary>
+        /// <param name="attribute">Attribute key and value pair</param>
+        /// <returns>true if attribute key is not null and value is one of the supported type, false otherwise</returns>
+        public static bool IsUserAttributeValid(KeyValuePair<string, object> attribute)
+        {
+            return (attribute.Key != null) && 
+                (attribute.Value is int || attribute.Value is string || attribute.Value is double 
+                 || attribute.Value is bool || attribute.Value is float || attribute.Value is long);
+        }
     }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -20,17 +20,6 @@ namespace OptimizelySDK.Entity`
`20`	`20`	`{`
`21`	`21`	`public class UserAttributes : Dictionary<string, object>`
`22`	`22`	`{`
`23`		`- public UserAttributes FilterNullValues(ILogger logger)`
`24`		`- {`
`25`		`- UserAttributes answer = new UserAttributes();`
`26`		`- foreach (KeyValuePair<string, object> pair in this) {`
`27`		`- if (pair.Value != null) {`
`28`		`- answer[pair.Key] = pair.Value;`
`29`		`- } else {`
`30`		`- logger.Log(LogLevel.ERROR, string.Format("[UserAttributes] Null value for key {0} removed and will not be sent to results.", pair.Key));`
`31`		`- }`
`32`		`- }`
`33`		`- return answer;`
`34`		`- }`
	`23`	`+`
`35`	`24`	`}`
`36`	`25`	`}`
Original file line number	Diff line number	Diff line change
`@@ -195,10 +195,6 @@ public Variation Activate(string experimentKey, string userId, UserAttributes us`
`195`	`195`	`return null;`
`196`	`196`	`}`
`197`	`197`
`198`		`- if (userAttributes != null) {`
`199`		`- userAttributes = userAttributes.FilterNullValues(Logger);`
`200`		`- }`
`201`		`-`
`202`	`198`	`SendImpressionEvent(experiment, variation, userId, userAttributes);`
`203`	`199`
`204`	`200`	`return variation;`
`@@ -270,11 +266,6 @@ public void Track(string eventKey, string userId, UserAttributes userAttributes`
`270`	`266`	`if (validExperimentIdToVariationMap.Count > 0)`
`271`	`267`	`{`
`272`	`268`
`273`		`- if (userAttributes != null)`
`274`		`- {`
`275`		`- userAttributes = userAttributes.FilterNullValues(Logger);`
`276`		`- }`
`277`		`-`
`278`	`269`	`if (eventTags != null)`
`279`	`270`	`{`
`280`	`271`	`eventTags = eventTags.FilterNullValues(Logger);`