[FSSDK-9533] Log error & reject on track event with null, empty or whitespace event key#362
[FSSDK-9533] Log error & reject on track event with null, empty or whitespace event key#362mikechu-optimizely wants to merge 13 commits into
Conversation
|
This will need to be part of a cherry-pick and release for a v3.11.4 along with #361 |
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
Can we create a test case to reproduce the suspected failure cases first? The current solution looks fine and also validated with FSC tests as well.
| var config = ProjectConfigManager?.GetConfig(); | ||
|
|
||
| if (config == null) |
There was a problem hiding this comment.
can we remove this config ready check?
There was a problem hiding this comment.
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?
| var eevent = config.GetEvent(eventKey); | ||
|
|
||
| if (eevent.Key == null) |
There was a problem hiding this comment.
I think these 2 lines will filter out null or empty eventKey cases.
There was a problem hiding this comment.
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.
| /// 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> |
There was a problem hiding this comment.
I guess this should read: "Event key representing the event in the datafile which must not be null, empty, or whitespace"
| var config = ProjectConfigManager?.GetConfig(); | ||
|
|
||
| if (config == null) | ||
| if (eventKey == null || Regex.IsMatch(eventKey, @"^\s*$")) |
There was a problem hiding this comment.
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.
2 participants
Summary
Test plan
Issues