diff --git a/OptimizelySDK.Tests/EventTests/EventBuilderTest.cs b/OptimizelySDK.Tests/EventTests/EventBuilderTest.cs index e25a0ff6..3cb73021 100644 --- 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 + { + { "visitors", new object[] + { + new Dictionary() + { + { "snapshots", new object[] + { + new Dictionary + { + { "decisions", new object[] + { + new Dictionary + { + {"campaign_id", "7719770039" }, + {"experiment_id", "7716830082" }, + {"variation_id", "7722370027" } + } + } + }, + { "events", new object[] + { + new Dictionary + { + {"entity_id", "7719770039" }, + {"timestamp", timeStamp }, + {"uuid", guid }, + {"key", "campaign_activated" } + } + } + } + } + } + }, + {"attributes", new object[] + { + new Dictionary + { + {"entity_id", "7723280020" }, + {"key", "device_type" }, + {"type", "custom" }, + {"value", "iPhone"} + }, + new Dictionary + { + {"entity_id", "323434545" }, + {"key", "boolean_key" }, + {"type", "custom" }, + {"value", true} + }, + new Dictionary + { + {"entity_id", "808797686" }, + {"key", "double_key" }, + {"type", "custom" }, + {"value", 3.14} + }, + new Dictionary + { + {"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 + { + { "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 index 14a17a57..14523eec 100644 --- a/OptimizelySDK.Tests/OptimizelyTest.cs +++ b/OptimizelySDK.Tests/OptimizelyTest.cs @@ -401,9 +401,12 @@ public void TestActivateWithAttributes() EventBuilderMock.Verify(b => b.CreateImpressionEvent(It.IsAny(), Config.GetExperimentFromKey("test_experiment"), "7722370027", "test_user", OptimizelyHelper.UserAttributes), Times.Once); + LoggerMock.Verify(l => l.Log(It.IsAny(), It.IsAny()), 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(), 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(), It.IsAny()), Times.Exactly(9)); + LoggerMock.Verify(l => l.Log(It.IsAny(), It.IsAny()), 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(), It.IsAny()), Times.Exactly(21)); + LoggerMock.Verify(l => l.Log(It.IsAny(), It.IsAny()), 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 index 404712ba..9bedfb6d 100644 --- 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)); + } } } \ No newline at end of file diff --git a/OptimizelySDK/Entity/UserAttributes.cs b/OptimizelySDK/Entity/UserAttributes.cs index e189cfe1..f6df4cf0 100644 --- a/OptimizelySDK/Entity/UserAttributes.cs +++ b/OptimizelySDK/Entity/UserAttributes.cs @@ -20,17 +20,6 @@ namespace OptimizelySDK.Entity { public class UserAttributes : Dictionary { - public UserAttributes FilterNullValues(ILogger logger) - { - UserAttributes answer = new UserAttributes(); - foreach (KeyValuePair 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 index 14592297..261b5b2b 100644 --- a/OptimizelySDK/Event/Builder/EventBuilder.cs +++ b/OptimizelySDK/Event/Builder/EventBuilder.cs @@ -85,7 +85,7 @@ private Dictionary 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 GetCommonParams(ProjectConfig config, string var userFeatures = new List>(); - 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 { - { "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 { { "entity_id", ControlAttributes.BOT_FILTERING_ATTRIBUTE }, @@ -121,6 +124,7 @@ private Dictionary GetCommonParams(ProjectConfig config, string } visitor["attributes"] = userFeatures; + return comonParams; } diff --git a/OptimizelySDK/Optimizely.cs b/OptimizelySDK/Optimizely.cs index 182cf9e3..e1ac1016 100644 --- 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 index 5276821b..7e301fd9 100644 --- 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 { - /// /// Validate the ProjectConfig JSON /// @@ -87,7 +87,7 @@ public static bool AreEventTagsValid(Dictionary 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; } + + /// + /// Determine if given user attribute is valid. + /// + /// Attribute key and value pair + /// true if attribute key is not null and value is one of the supported type, false otherwise + public static bool IsUserAttributeValid(KeyValuePair 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); + } } }