[FSSDK-9079] fix: send odp event empty values (#346)

* add empty check to SendOdpEvent

* eval idempotence_id per event

* convert fs_user_id identifier variants

* add unit tests

* Optimize fs_user_id updates

* use constant for error msg

* make SendOdpEvent type and  data optional

---------

Co-authored-by: Mike Chu <[email protected]>

Author: andrewleap-optimizely

OptimizelySDK.Tests/OdpTests/OdpEventManagerTests.cs

@@ -1,4 +1,4 @@
-/* 
+/*
  * Copyright 2022-2023, Optimizely
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -308,6 +308,118 @@ public void ShouldDiscardEventsWithInvalidIdentifiersDictionary()
                 Times.Exactly(2));
         }
 
+        [Test]
+        public void ShouldConvertFsUserIdIdentifier()
+        {
+            var event1 = new OdpEvent("t3", "a3",
+                new Dictionary<string, string>
+                {
+                    {
+                        "fs-user-id", "123"
+                    }
+                },
+                new Dictionary<string, object>()
+            );
+
+            var event2 = new OdpEvent("t3", "a3",
+                new Dictionary<string, string>
+                {
+                    {
+                        "FS-user-ID", "123"
+                    }
+                },
+                new Dictionary<string, object>()
+            );
+
+            var event3 = new OdpEvent("t3", "a3",
+                new Dictionary<string, string>
+                {
+                    {
+                        "FS_USER_ID", "123"
+                    },
+                    {
+                        "fs.user.id", "456"
+                    }
+                },
+                new Dictionary<string, object>()
+            );
+
+            var event4 = new OdpEvent("t3", "a3",
+                new Dictionary<string, string>
+                {
+                    {
+                        "fs_user_id", "123"
+                    },
+                    {
+                        "fsuserid", "456"
+                    }
+                },
+                new Dictionary<string, object>()
+            );
+
+            var expectedIdentifiers = new List<Dictionary<string, string>> {
+                new Dictionary<string, string> {
+                    {
+                        "fs_user_id", "123"
+                    }
+                },
+                new Dictionary<string, string> {
+                    {
+                        "fs_user_id", "123"
+                    }
+                },
+                new Dictionary<string, string> {
+                    {
+                        "fs_user_id", "123"
+                    },
+                    {
+                        "fs.user.id", "456"
+                    }
+                },
+                new Dictionary<string, string>
+                {
+                    {
+                        "fs_user_id", "123"
+                    },
+                                        {
+                        "fsuserid", "456"
+                    }
+                },
+            };
+
+            var cde = new CountdownEvent(4);
+            var eventsCollector = new List<List<OdpEvent>>();
+            _mockApiManager.Setup(api => api.SendEvents(
+                It.IsAny<string>(),
+                It.IsAny<string>(),
+                Capture.In(eventsCollector))
+            ).Callback(() => cde.Signal());
+
+            var eventManager = new OdpEventManager.Builder().
+                WithOdpEventApiManager(_mockApiManager.Object).
+                WithLogger(_mockLogger.Object).
+                WithEventQueue(new BlockingCollection<object>(10)). // max capacity of 10
+                WithFlushInterval(TimeSpan.FromMilliseconds(0)).
+                Build();
+            eventManager.UpdateSettings(_odpConfig);
+
+            eventManager.SendEvent(event1);
+            eventManager.SendEvent(event2);
+            eventManager.SendEvent(event3);
+            eventManager.SendEvent(event4);
+            cde.Wait(MAX_COUNT_DOWN_EVENT_WAIT_MS);
+
+
+            foreach (int i in Enumerable.Range(0, 4))
+            {
+                CollectionAssert.AreEquivalent(
+                    expectedIdentifiers[i],
+                    eventsCollector[i][0].Identifiers
+                );
+
+            }
+        }
+
         [Test]
         public void ShouldAddAdditionalInformationToEachEvent()
         {

OptimizelySDK.Tests/OptimizelyTest.cs

@@ -55,6 +55,7 @@ public class OptimizelyTest
         private Mock<EventProcessor> EventProcessorMock;
         private NotificationCenter NotificationCenter;
         private Mock<TestNotificationCallbacks> NotificationCallbackMock;
+        private Mock<IOdpManager> OdpManagerMock;
         private Variation VariationWithKeyControl;
         private Variation VariationWithKeyVariation;
         private Variation GroupVariation;
@@ -109,6 +110,8 @@ public void Initialize()
                 CallBase = true,
             };
 
+            OdpManagerMock = new Mock<IOdpManager>();
+
             DecisionServiceMock = new Mock<DecisionService>(new Bucketer(LoggerMock.Object),
                 ErrorHandlerMock.Object,
                 null, LoggerMock.Object);
@@ -6166,5 +6169,62 @@ public static void SetCulture(string culture)
         }
 
         #endregion Test Culture
+
+        #region Test SendOdpEvent
+
+        [Test]
+        public void TestSendOdpEventNullAction()
+        {
+            var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object, odpManager: OdpManagerMock.Object);
+            optly.SendOdpEvent(action: null, identifiers: new Dictionary<string, string>(), type: "type");
+            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, Constants.ODP_INVALID_ACTION_MESSAGE),
+                Times.Exactly(1));
+
+            optly.Dispose();
+        }
+
+        [Test]
+        public void TestSendOdpEventEmptyStringAction()
+        {
+            var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object, odpManager: OdpManagerMock.Object);
+            optly.SendOdpEvent(action: "", identifiers: new Dictionary<string, string>(), type: "type");
+            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, Constants.ODP_INVALID_ACTION_MESSAGE),
+                Times.Exactly(1));
+
+            optly.Dispose();
+        }
+        [Test]
+        public void TestSendOdpEventNullType()
+        {
+            var identifiers = new Dictionary<string, string>();
+            var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object, odpManager: OdpManagerMock.Object);
+
+            optly.SendOdpEvent(action: "action", identifiers: identifiers, type: null);
+
+            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, It.IsAny<string>()),
+                Times.Never);
+            OdpManagerMock.Verify(e => e.SendEvent("fullstack", "action", identifiers, null),
+                Times.Once);
+
+            optly.Dispose();
+        }
+
+        [Test]
+        public void TestSendOdpEventEmptyStringType()
+        {
+            var identifiers = new Dictionary<string, string>();
+            var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object, odpManager: OdpManagerMock.Object);
+
+            optly.SendOdpEvent(action: "action", identifiers: identifiers, type: "");
+
+            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, It.IsAny<string>()),
+                Times.Never);
+            OdpManagerMock.Verify(e => e.SendEvent("fullstack", "action", identifiers, null),
+                Times.Once);
+
+            optly.Dispose();
+        }
+
+        #endregion Test SendOdpEvent
     }
 }

OptimizelySDK/IOptimizely.cs

@@ -83,7 +83,7 @@ Variation GetVariation(string experimentKey, string userId,
         bool SetForcedVariation(string experimentKey, string userId, string variationKey);
 
         /// <summary>
-        /// Gets the forced variation key for the given user and experiment.  
+        /// Gets the forced variation key for the given user and experiment.
         /// </summary>
         /// <param name="experimentKey">The experiment key</param>
         /// <param name="userId">The user ID</param>
@@ -162,7 +162,7 @@ string GetFeatureVariableString(string featureKey, string variableKey, string us
         #endregion
 
 #if USE_ODP
-        void SendOdpEvent(string type, string action, Dictionary<string, string> identifiers,
+        void SendOdpEvent(string action, Dictionary<string, string> identifiers, string type,
             Dictionary<string, object> data
         );
 #endif

OptimizelySDK/Odp/Constants.cs

@@ -67,6 +67,11 @@ public static class Constants
         /// </summary>
         public const string ODP_INVALID_DATA_MESSAGE = "ODP event send failed.";
 
+        /// <summary>
+        /// Default message to log when an ODP Event contains invalid action
+        /// </summary>
+        public const string ODP_INVALID_ACTION_MESSAGE = "ODP action is not valid (cannot be empty).";
+
         /// <summary>
         /// Default message to log when sending ODP event fails
         /// </summary>
@@ -116,5 +121,15 @@ public static class Constants
         /// Type of ODP key used for fetching segments & sending events
         /// </summary>
         public const string FS_USER_ID = "fs_user_id";
+
+        /// <summary>
+        /// Alternate form of ODP key that is auto-converted to FS_USER_ID
+        /// </summary>
+        public const string FS_USER_ID_ALIAS = "fs-user-id";
+
+        /// <summary>
+        /// Unique identifier used for ODP events
+        /// </summary>
+        public const string IDEMPOTENCE_ID = "idempotence_id";
     }
 }

OptimizelySDK/Odp/Entity/OdpEvent.cs

@@ -36,7 +36,7 @@ public class OdpEvent
         /// <summary>
         /// Dictionary for identifiers. The caller must provide at least one key-value pair.
         /// </summary>
-        public Dictionary<string, string> Identifiers { get; }
+        public Dictionary<string, string> Identifiers { get; set; }
 
         /// <summary>
         /// Event data in a key-value pair format

OptimizelySDK/Odp/OdpEventManager.cs

@@ -40,6 +40,10 @@ public class OdpEventManager : IOdpEventManager, IDisposable
         private ILogger _logger;
         private IErrorHandler _errorHandler;
         private BlockingCollection<object> _eventQueue;
+        private static readonly string[] fsUserIdMatches = {
+            Constants.FS_USER_ID,
+            Constants.FS_USER_ID_ALIAS,
+        };
 
         /// <summary>
         /// Object to ensure mutually exclusive locking for thread safety
@@ -302,6 +306,8 @@ public void SendEvent(OdpEvent odpEvent)
                 return;
             }
 
+            odpEvent.Identifiers = ConvertCriticalIdentifiers(odpEvent.Identifiers);
+
             odpEvent.Data = AugmentCommonData(odpEvent.Data);
             if (!_eventQueue.TryAdd(odpEvent))
             {
@@ -429,7 +435,40 @@ private Dictionary<string, dynamic> AugmentCommonData(
             Dictionary<string, dynamic> sourceData
         )
         {
-            return sourceData.MergeInPlace<string, object>(_commonData);
+            sourceData.MergeInPlace<string, object>(_commonData);
+            if (!sourceData.ContainsKey(Constants.IDEMPOTENCE_ID))
+            {
+                sourceData.Add(Constants.IDEMPOTENCE_ID, Guid.NewGuid());
+            }
+            return sourceData;
+        }
+
+        /// <summary>
+        /// Convert critical identifiers to the correct format for ODP
+        /// </summary>
+        /// <param name="identifiers">Collection of identifiers to validate</param>
+        /// <returns>New version of identifiers with corrected values</returns>
+        private static Dictionary<string, string> ConvertCriticalIdentifiers(
+            Dictionary<string, string> identifiers
+        )
+        {
+            if (identifiers.ContainsKey(Constants.FS_USER_ID))
+            {
+                return identifiers;
+            }
+
+            var identList = new List<KeyValuePair<string, string>>(identifiers);
+            foreach (var kvp in identList)
+            {
+                if (fsUserIdMatches.Contains(kvp.Key, StringComparer.OrdinalIgnoreCase))
+                {
+                    identifiers.Remove(kvp.Key);
+                    identifiers[Constants.FS_USER_ID] = kvp.Value;
+                    break;
+                }
+            }
+
+            return identifiers;
         }
 
         /// <summary>
@@ -562,7 +601,6 @@ public OdpEventManager Build()
 
                 manager._commonData = new Dictionary<string, object>
                 {
-                    { "idempotence_id", Guid.NewGuid() },
                     { "data_source_type", "sdk" },
                     { "data_source", Optimizely.SDK_TYPE },
                     { "data_source_version", Optimizely.SDK_VERSION },

OptimizelySDK/Optimizely.cs

@@ -1354,14 +1354,25 @@ internal void IdentifyUser(string userId)
         /// <summary>
         /// Add event to queue for sending to ODP
         /// </summary>
-        /// <param name="type">Type of event (typically `fullstack` from server-side SDK events)</param>
         /// <param name="action">Subcategory of the event type</param>
         /// <param name="identifiers">Dictionary for identifiers. The caller must provide at least one key-value pair.</param>
-        /// <param name="data">Event data in a key-value pair format</param>
-        public void SendOdpEvent(string type, string action, Dictionary<string, string> identifiers,
-            Dictionary<string, object> data
+        /// <param name="type">Type of event (defaults to `fullstack`)</param>
+        /// <param name="data">Optional event data in a key-value pair format</param>
+        public void SendOdpEvent(string action, Dictionary<string, string> identifiers, string type = Constants.ODP_EVENT_TYPE,
+            Dictionary<string, object> data = null
         )
         {
+            if (String.IsNullOrEmpty(action))
+            {
+                Logger.Log(LogLevel.ERROR, Constants.ODP_INVALID_ACTION_MESSAGE);
+                return;
+            }
+
+            if (String.IsNullOrEmpty(type))
+            {
+                type = Constants.ODP_EVENT_TYPE;
+            }
+
             OdpManager?.SendEvent(type, action, identifiers, data);
         }
 #endif

OptimizelySDK/Utils/CollectionExtensions.cs

@@ -1,5 +1,5 @@
-/* 
- * Copyright 2022, Optimizely
+/*
+ * Copyright 2022-2023, Optimizely
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,10 +29,9 @@ public static class CollectionExtensions
         /// <param name="right">Dictionary to merge into another</param>
         /// <typeparam name="TKey">Key type</typeparam>
         /// <typeparam name="TValue">Value type</typeparam>
-        /// <returns>Merged Dictionary</returns>
         /// <exception cref="ArgumentNullException">Thrown if target Dictionary is null</exception>
-        public static Dictionary<TKey, TValue> MergeInPlace<TKey, TValue>(
-            this Dictionary<TKey, TValue> left, Dictionary<TKey, TValue> right
+        public static void MergeInPlace<TKey, TValue>(this Dictionary<TKey, TValue> left,
+            Dictionary<TKey, TValue> right
         )
         {
             if (left == null)
@@ -43,16 +42,15 @@ this Dictionary<TKey, TValue> left, Dictionary<TKey, TValue> right
 
             if (right == null)
             {
-                return left;
+                return;
             }
 
+            // Only update the left dictionary when the key doesn't exist
             foreach (var kvp in right.Where(
                          kvp => !left.ContainsKey(kvp.Key)))
             {
                 left.Add(kvp.Key, kvp.Value);
             }
-
-            return left;
         }
     }
 }