-
Notifications
You must be signed in to change notification settings - Fork 5
fix: improve json parsing error handling #231
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.
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
Conversation
DevCycle/Models/UserConfig.swift
Outdated
| guard let variationName = dictionary["variationName"] else { throw UserConfigError.MissingProperty("variationName in Feature object") } | ||
| init (from dictionary: [String: Any]) throws { | ||
| guard let id = dictionary["_id"] as? String else { | ||
| throw UserConfigError.MissingProperty("_id in Feature object") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update these error messages to say we expect these to be typed as strings.
af95065 to
60230d1
Compare
| } | ||
|
|
||
|
|
||
| func testSuccessfulConfigParsingWithNonStringValuesOnFeatures() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we actually add a test that uses test configs that have unknown properties in all parts of the config instead of just the feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea can we add a test that shows it throwing for UserConfigError.InvalidProperty and UserConfigError.MissingProperty above? Also a test that shows that these errors don't propagate past the config fetching.
…coming User Configuration from the API
…ariables map parsing fails
…rom the user config
b6e88cb to
57e2933
Compare
| self.features = features | ||
| } else { | ||
| Log.warn("Invalid feature map format", tags: ["config", "JSONParsing"]) | ||
| throw UserConfigError.InvalidProperty("features") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So where are these errors caught?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These errors are caught by DevCycleService.getConfig completion handler of the method, defined as: completion: @escaping ConfigCompletionHandler)
57e2933 to
deae363
Compare
[ String: Any ]to allow values to be of type other than Stringif letprotection when parsing thefeatureMapandvariablesMapon the UserConfig to prevent errors from JSON parsing to cause SDK crashes, instead we will log warnings