fix(datafile-parsing): Prevent newer versions datafile #101
Conversation
…has version other than supported versions
| [Test] | ||
| public void TestCreateThrowsWithUnsopportedDatafileVersion() | ||
| { | ||
| var unsupportedConfig = ProjectConfig.Create(TestData.Datafile, null, null); |
There was a problem hiding this comment.
Please change this testcase. First Deserialize into JSON object and make a change, serialize into JSON string and passing to ProjectConfig. Don't serialize & deserialize project config object.
OptimizelySDK/Optimizely.cs
Outdated
| @@ -111,9 +112,15 @@ public class Optimizely : IOptimizely | |||
| IsValid = true; | |||
| DecisionService = new DecisionService(Bucketer, ErrorHandler, Config, userProfileService, Logger); | |||
| } | |||
| catch (ConfigParseException configException) | |||
| { | |||
| Logger.Log(LogLevel.ERROR, configException.Message); | |||
There was a problem hiding this comment.
add unit test in optimizely class and expect handleerror is called.
There was a problem hiding this comment.
Let's consolidate these two catches into one and handle the logging based on the type of the exception. We can avoid duplicating some of the code, like passing the error to the error handler
OptimizelySDK/ProjectConfig.cs
Outdated
| if (configData == null) | ||
| throw new ConfigParseException("Unable to parse null datafile."); | ||
|
|
||
| if (configData.Length == 0) |
There was a problem hiding this comment.
use string.isnullorempty
OptimizelySDK/Optimizely.cs
Outdated
| @@ -111,9 +112,15 @@ public class Optimizely : IOptimizely | |||
| IsValid = true; | |||
| DecisionService = new DecisionService(Bucketer, ErrorHandler, Config, userProfileService, Logger); | |||
| } | |||
| catch (ConfigParseException configException) | |||
| { | |||
| Logger.Log(LogLevel.ERROR, configException.Message); | |||
There was a problem hiding this comment.
Let's consolidate these two catches into one and handle the logging based on the type of the exception. We can avoid duplicating some of the code, like passing the error to the error handler
OptimizelySDK/ProjectConfig.cs
Outdated
|
|
||
| var config = JsonConvert.DeserializeObject<ProjectConfig>(configData); | ||
|
|
||
| if (SupportedVersions.TrueForAll((obj) => !(((int)obj).ToString() == config.Version))) |
There was a problem hiding this comment.
Please don't use the generic variable name obj. Instead go for supportVersion
| [Test] | ||
| public void TestCreateThrowsWithUnsupportedDatafileVersion() | ||
| { | ||
| var unsupportedConfig = JsonConvert.DeserializeObject<ProjectConfig>(TestData.Datafile); |
There was a problem hiding this comment.
Please create a separate mock unsupported datafile and load it into this test case
| } | ||
|
|
||
| [Test] | ||
| public void TestErrorHandlingWithInvalidConfigVersion() |
There was a problem hiding this comment.
Please split these up into three separate unit tests. Makes it easier to read and determine what actually failed
No description provided.