Skip to content

[FSSDK-9533] Log error & reject on track event with null, empty or whitespace event key #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

[FSSDK-9533] Log error & reject on track event with null, empty or whitespace event key #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7a2fd20
Add early check for eventKey being null/empty
mikechu-optimizely Jul 25, 2023
aa6e213
Adjust Track tests for null/empty event key
mikechu-optimizely Jul 25, 2023
7a6cbb3
Update method docs
mikechu-optimizely Jul 25, 2023
0251e5a
Use IsNullOrWhiteSpace for eventKey
mikechu-optimizely Jul 25, 2023
0a265c9
Add doc about no whitespace
mikechu-optimizely Jul 25, 2023
dcbedf4
Refactor code; Update logged error message
mikechu-optimizely Jul 25, 2023
1ab6c55
Further refactors
mikechu-optimizely Jul 25, 2023
0b6f70a
Lint fixes
mikechu-optimizely Jul 25, 2023
9bf7230
Fixing whitespace lints
mikechu-optimizely Jul 25, 2023
2597098
Fix lints ...
mikechu-optimizely Jul 25, 2023
a2b8d54
Put back needed Linq import
mikechu-optimizely Jul 25, 2023
aad09e1
Fix for .NET 3.5 lacking IsNullOrWhiteSpace()
mikechu-optimizely Jul 25, 2023
609973e
Merge branch 'master' into mike/track-event-event-key
mikechu-optimizely Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 46 additions & 17 deletions OptimizelySDK.Tests/OptimizelyTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ public void TestInvalidInstanceLogMessages()
Times.Once);
LoggerMock.Verify(
log => log.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'Track'."),
Times.Once);
Times.Never); // Erroring on null or empty event key takes priority over invalid datafile.
LoggerMock.Verify(
log => log.Log(LogLevel.ERROR,
"Datafile has invalid format. Failing 'IsFeatureEnabled'."), Times.Once);
Expand Down Expand Up @@ -5668,20 +5668,41 @@ public void TestGetVariationValidateInputValues()
[Test]
public void TestTrackValidateInputValues()
{
// Verify that ValidateStringInputs does not log error for valid values.
Optimizely.Track("purchase", "test_user");
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "Provided User Id is in invalid format."),
const string GOOD_EVENT_KEY = "purchase";
const string EXPECTED_EVENT_KEY_ERROR_MESSAGE =
"Event key cannot be null, empty, or whitespace string. Failing 'Track'.";
const string GOOD_USER = "test_user";
const string EXPECTED_USER_ID_ERROR_MESSAGE = "Provided User Id is in invalid format.";

Optimizely.Track(GOOD_EVENT_KEY, GOOD_USER);
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, EXPECTED_USER_ID_ERROR_MESSAGE),
Times.Never);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, "Provided Event Key is in invalid format."),
l => l.Log(LogLevel.ERROR, EXPECTED_EVENT_KEY_ERROR_MESSAGE),
Times.Never);
LoggerMock.ResetCalls();

// Verify that ValidateStringInputs logs error for invalid values.
Optimizely.Track("", null);
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, "Provided User Id is in invalid format."),
Optimizely.Track("", GOOD_USER);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, EXPECTED_EVENT_KEY_ERROR_MESSAGE),
Times.Once);
LoggerMock.ResetCalls();

Optimizely.Track(" ", GOOD_USER);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, "Provided Event Key is in invalid format."), Times.Once);
l => l.Log(LogLevel.ERROR, EXPECTED_EVENT_KEY_ERROR_MESSAGE),
Times.Once);
LoggerMock.ResetCalls();

Optimizely.Track(null, GOOD_USER);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, EXPECTED_EVENT_KEY_ERROR_MESSAGE),
Times.Once);
LoggerMock.ResetCalls();

Optimizely.Track(GOOD_EVENT_KEY, null);
LoggerMock.Verify(l => l.Log(LogLevel.ERROR, EXPECTED_USER_ID_ERROR_MESSAGE),
Times.Once);
}

#endregion Test ValidateStringInputs
Expand Down Expand Up @@ -6139,8 +6160,10 @@ public static void SetCulture(string culture)
[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");
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));

Expand All @@ -6151,7 +6174,8 @@ public void TestSendOdpEventNullAction()
public void TestSendOdpEventInvalidOptimizelyObject()
{
var optly = new Optimizely("Random datafile", null, LoggerMock.Object);
optly.SendOdpEvent("some_action", new Dictionary<string, string>() { { "some_key", "some_value" } }, "some_event");
optly.SendOdpEvent("some_action",
new Dictionary<string, string>() { { "some_key", "some_value" } }, "some_event");
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'SendOdpEvent'."),
Times.Once);
Expand All @@ -6160,8 +6184,10 @@ public void TestSendOdpEventInvalidOptimizelyObject()
[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");
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));

Expand All @@ -6172,7 +6198,8 @@ public void TestSendOdpEventEmptyStringAction()
public void TestSendOdpEventNullType()
{
var identifiers = new Dictionary<string, string>();
var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object, odpManager: OdpManagerMock.Object);
var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object,
odpManager: OdpManagerMock.Object);

optly.SendOdpEvent(action: "action", identifiers: identifiers, type: null);

Expand All @@ -6188,7 +6215,8 @@ public void TestSendOdpEventNullType()
public void TestSendOdpEventEmptyStringType()
{
var identifiers = new Dictionary<string, string>();
var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object, odpManager: OdpManagerMock.Object);
var optly = new Optimizely(TestData.OdpIntegrationDatafile, logger: LoggerMock.Object,
odpManager: OdpManagerMock.Object);

optly.SendOdpEvent(action: "action", identifiers: identifiers, type: "");

Expand All @@ -6210,7 +6238,8 @@ public void TestFetchQualifiedSegmentsInvalidOptimizelyObject()
var optly = new Optimizely("Random datafile", null, LoggerMock.Object);
optly.FetchQualifiedSegments("some_user", null);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'FetchQualifiedSegments'."),
l => l.Log(LogLevel.ERROR,
"Datafile has invalid format. Failing 'FetchQualifiedSegments'."),
Times.Once);
}

Expand Down
1 change: 1 addition & 0 deletions OptimizelySDK/Config/HttpProjectConfigManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#endif

using System;
using System.Linq;
using System.Net;
using System.Threading.Tasks;
using OptimizelySDK.ErrorHandler;
Expand Down
45 changes: 27 additions & 18 deletions OptimizelySDK/Optimizely.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
using OptimizelySDK.OptlyConfig;
using OptimizelySDK.OptimizelyDecisions;
using System.Linq;
using System.Text.RegularExpressions;

#if USE_ODP
using OptimizelySDK.Odp;
Expand Down Expand Up @@ -87,7 +88,7 @@ public static String SDK_VERSION
{
get
{
// Example output: "2.1.0" . Should be kept in synch with NuGet package version.
// Example output: "2.1.0". Should be kept in sync with NuGet package version.
#if NET35 || NET40
var assembly = Assembly.GetExecutingAssembly();
#else
Expand Down Expand Up @@ -338,54 +339,60 @@ private bool ValidateInputs(string datafile, bool skipJsonValidation)
/// <summary>
/// Sends conversion event to Optimizely.
/// </summary>
/// <param name="eventKey">Event key representing the event which needs to be recorded</param>
/// <param name="eventKey">Event key representing the event (must not be null, empty, or whitespace)</param>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should read: "Event key representing the event in the datafile which must not be null, empty, or whitespace"

/// <param name="userId">ID for user</param>
/// <param name="userAttributes">Attributes of the user</param>
/// <param name="eventTags">eventTags array Hash representing metadata associated with the event.</param>
public void Track(string eventKey, string userId, UserAttributes userAttributes = null,
EventTags eventTags = null
)
{
var config = ProjectConfigManager?.GetConfig();

if (config == null)
Comment on lines -349 to -351
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this config ready check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to call .GetEvent(eventKey) later (L365 below on this older version) to check if the provided eventKey is in the datafile, I think we'd probably still need this check, right?

if (eventKey == null || Regex.IsMatch(eventKey, @"^\s*$"))
Copy link
Contributor Author

@mikechu-optimizely mikechu-optimizely Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an earlier eventKey check since it's cheaper before digging into the ProjectConfig to see if the eventKey is present in the datafile since it's required per the docs.

{
Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'Track'.");
Logger.Log(LogLevel.ERROR,
"Event key cannot be null, empty, or whitespace string. Failing 'Track'.");
return;
}

var inputValues = new Dictionary<string, string>
{ { USER_ID, userId }, { EVENT_KEY, eventKey } };
{
{ USER_ID, userId },
{ EVENT_KEY, eventKey },
};

if (!ValidateStringInputs(inputValues))
{
return;
}

var eevent = config.GetEvent(eventKey);

if (eevent.Key == null)
Comment on lines -365 to -367
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these 2 lines will filter out null or empty eventKey cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what you're talking about: The eventKey must be in the datafile before it can be tracked anyway, regardless of whether it's not null, empty, or whitespace.

var config = ProjectConfigManager?.GetConfig();
if (config == null)
{
Logger.Log(LogLevel.INFO,
string.Format("Not tracking user {0} for event {1}.", userId, eventKey));
Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'Track'.");
return;
}

if (eventTags != null)
var eventToTrack = config.GetEvent(eventKey);
if (eventToTrack.Key == null)
{
eventTags = eventTags.FilterNullValues(Logger);
Logger.Log(LogLevel.INFO, $"Not tracking user {userId} for event {eventKey}.");
return;
}

eventTags = eventTags?.FilterNullValues(Logger);

var userEvent = UserEventFactory.CreateConversionEvent(config, eventKey, userId,
userAttributes, eventTags);

EventProcessor.Process(userEvent);
Logger.Log(LogLevel.INFO,
string.Format("Tracking event {0} for user {1}.", eventKey, userId));

Logger.Log(LogLevel.INFO, $"Tracking event {eventKey} for user {userId}.");

if (NotificationCenter.GetNotificationCount(NotificationCenter.NotificationType.Track) >
0)
{
var conversionEvent = EventFactory.CreateLogEvent(userEvent, Logger);

NotificationCenter.SendNotifications(NotificationCenter.NotificationType.Track,
eventKey, userId,
userAttributes, eventTags, conversionEvent);
Expand Down Expand Up @@ -1347,7 +1354,8 @@ List<OdpSegmentOption> segmentOptions

if (config == null)
{
Logger.Log(LogLevel.ERROR, "Datafile has invalid format. Failing 'FetchQualifiedSegments'.");
Logger.Log(LogLevel.ERROR,
"Datafile has invalid format. Failing 'FetchQualifiedSegments'.");
return null;
}

Expand Down Expand Up @@ -1378,7 +1386,8 @@ internal void IdentifyUser(string userId)
/// <param name="identifiers">Dictionary for identifiers. The caller must provide at least one key-value pair.</param>
/// <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,
public void SendOdpEvent(string action, Dictionary<string, string> identifiers, string type
= Constants.ODP_EVENT_TYPE,
Dictionary<string, object> data = null
)
{
Expand Down
4 changes: 2 additions & 2 deletions OptimizelySDK/OptimizelyUserContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ OptimizelyDecideOption[] options
/// <summary>
/// Track an event.
/// </summary>
/// <param name="eventName">The event name.</param>
/// <param name="eventName">The event name (must not be null, empty, or whitespace).</param>
public virtual void TrackEvent(string eventName)
{
TrackEvent(eventName, new EventTags());
Expand All @@ -357,7 +357,7 @@ public virtual void TrackEvent(string eventName)
/// <summary>
/// Track an event.
/// </summary>
/// <param name="eventName">The event name.</param>
/// <param name="eventName">The event name (must not be null, empty, or whitespace).</param>
/// <param name="eventTags">A map of event tag names to event tag values.</param>
public virtual void TrackEvent(string eventName,
EventTags eventTags
Expand Down