[FSSDK-9079] fix: send odp event empty values (optimizely#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 <michael.chu@optimizely.com>

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;
  }
  }
 }