fix: Adds fallback dictionary when serialising device info. #279

fractalwrench · 2018-05-15T09:45:11Z

Goal

Previously the device tab was serialised using the dictionary supplied from the report, which could be null if the keypath did not exist. This could result in the device tab being completely null. This fix uses an empty dictionary as a fallback so that at least some information (freeDisk, locale, simulator) will always be returned.

Changeset

Changed

Initialise empty dictionary and defensively add items from report to it.

Tests

Added a failing unit test for the encountered scenario then passed it.

Review

For the submitter, initial self-review:

Commented on code changes inline explain the reasoning behind the approach
Reviewed the test cases added for completeness and possible points for discussion
A changelog entry was added for the goal of this pull request
Check the scope of the changeset - is everything in the diff required for the pull request?
This pull request is ready for:
- Initial review of the intended approach, not yet feature complete
- Structural review of the classes, functions, and properties modified
- Final review

For the pull request reviewer(s), this changeset has been reviewed for:

Consistency across platforms for structures or concepts added or modified
Consistency between the changeset and the goal stated above
Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
Usage friction - is the proposed change in usage cumbersome or complicated?
Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
Concurrency concerns - if components are accessed asynchronously, what issues will arise
Thoroughness of added tests and any missing edge cases
Idiomatic use of the language

Previously the device tab was serialised using the dictionary supplied from the report, which could be null if the keypath did not exist. This fix uses an empty dictionary as a fallback so that at least some information (freeDisk, locale, simulator) will always be returned.

kattrali

Looks good, left a few nits.

kattrali · 2018-05-15T15:41:31Z

Source/BugsnagKSCrashSysInfoParser.m

-    [[report valueForKeyPath:@"user.state.deviceState"] mutableCopy];
-
+    NSMutableDictionary *device = [NSMutableDictionary new];
+    NSDictionary *state = [[report valueForKeyPath:@"user.state.deviceState"] mutableCopy];


This no longer needs to be a mutable copy since the entries are being copied instead of appending to the original.

For consideration: Since valueForKey: can throw exceptions for incorrect keys, and since keypaths can't be checked by the compiler, is it worth considering wrapping these in a @try/@catch? It's certainly very unlikely these will throw, but not impossible.

kattrali · 2018-05-15T15:42:30Z

iOS/BugsnagTests/BugsnagKSCrashSysInfoParserTest.m

+    XCTAssertNotNil(device[@"simulator"]);
+}
+
+@end


What happens if the report argument is nil?

I've tested this out and a dictionary is returned with the same contents as if an empty dictionary had been passed in. Adding this as a test would currently fail the suite with an error, presumably due to the fact we've enabled strict build settings and this method parameter is enabled as NonNull. Do you feel we need to change anything so that this can be unit tested?

Is it possible to loosen the restrictions on just the test files? It seems like it might be useful to have a test like that

kastiglione · 2018-05-15T23:00:25Z

👍

martin308

Mostly looks good to me, added a question and a thought on the tests

martin308 · 2018-05-16T17:14:35Z

Source/BugsnagKSCrashSysInfoParser.m

-    NSMutableDictionary *device =
-    [[report valueForKeyPath:@"user.state.deviceState"] mutableCopy];
-
+    NSMutableDictionary *device = [NSMutableDictionary new];


Would it be better to do something like this? I'm not sure how initWithDictionary handles nil

NSDictionary *state = [report valueForKeyPath:@"user.state.deviceState"]; NSMutableDictionary* device = [[NSMutableDictionary alloc] initWithDictionary: state];

initWithDictionary: requires a Nonnull parameter, whereas addEntriesFromDictionary: handles a nil param.

martin308 · 2018-05-16T17:15:02Z

iOS/BugsnagTests/BugsnagKSCrashSysInfoParserTest.m

+    XCTAssertNotNil(device[@"simulator"]);
+}
+
+@end


Is it possible to loosen the restrictions on just the test files? It seems like it might be useful to have a test like that

fractalwrench · 2018-05-17T11:02:11Z

@martin308 I've added a test for a nil dictionary by using #pragma to loosen the restrictions.

kastiglione · 2018-05-17T15:43:25Z

iOS/BugsnagTests/BugsnagKSCrashSysInfoParserTest.m

+#pragma clang diagnostic ignored "-Wnonnull"
+
+- (void)testNilDictSerialisation {
+    XCTAssertNotNil(BSGParseDevice(nil));


This could return an empty dictionary and still pass. I think this should check the contents too.

Ditto this 👆 - the test should check the values are what is expected.

kattrali · 2018-05-18T05:18:57Z

iOS/BugsnagTests/BugsnagKSCrashSysInfoParserTest.m

+#pragma clang diagnostic ignored "-Wnonnull"
+
+- (void)testNilDictSerialisation {
+    XCTAssertNotNil(BSGParseDevice(nil));


Ditto this 👆 - the test should check the values are what is expected.

kattrali · 2018-05-18T05:21:32Z

iOS/BugsnagTests/BugsnagKSCrashSysInfoParserTest.m

+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wnonnull"


The pragma declarations should wrap what is being ignored as closely as possible to avoid any unintended usages.

#pragma clang diagnostic push #pragma clang diagnostic ignored "-Wnonnull" NSDictionary *device = BSGParseDevice(nil); #pragma clang diagnostic pop XCTAssertNotNil(device); // ... (more)

…larations

fractalwrench · 2018-05-18T08:42:58Z

Thanks for the review comments - I've addressed the pragma + validation comments.

martin308 · 2018-05-21T17:15:30Z

iOS/BugsnagTests/BugsnagKSCrashSysInfoParserTest.m

+}
+
+
+@end


Do we need a test for a valid dictionary that contains the expected key?

This is tested elsewhere I believe

martin308

Left one question about another possible test, otherwise this LGTM

$fractalwrench$

$@fractalwrench$ fractalwrench requested a review from a team May 15, 2018 09:45

$@fractalwrench$

docs: add changelog entry

9060518

kattrali previously approved these changes May 15, 2018

View reviewed changes

$@fractalwrench$

refactor: Avoid making unnecessary mutable copy of dictionary

7dbe713

$@fractalwrench$ fractalwrench dismissed kattrali’s stale review via 7dbe713 May 16, 2018 08:14

martin308 previously approved these changes May 16, 2018

View reviewed changes

$@fractalwrench$

test: Add test for nil device parameter when parsing report

2fb139d

$@fractalwrench$ fractalwrench dismissed martin308’s stale review via 2fb139d May 17, 2018 11:01

$@fractalwrench$ fractalwrench requested a review from a team May 17, 2018 13:47

kastiglione reviewed May 17, 2018

View reviewed changes

kattrali reviewed May 18, 2018

View reviewed changes

$@fractalwrench$

test: Check contents of device dictionary, reduce scope of pragma dec…

c2bf343

…larations

martin308 reviewed May 21, 2018

View reviewed changes

martin308 approved these changes May 21, 2018

View reviewed changes

kattrali merged commit 61aab87 into master May 24, 2018

kattrali deleted the device-state-serialisation-fix branch May 24, 2018 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Adds fallback dictionary when serialising device info. #279

fix: Adds fallback dictionary when serialising device info. #279

$@fractalwrench$ fractalwrench commented May 15, 2018 •

edited by kattrali

Loading

kattrali left a comment

kattrali May 15, 2018

kastiglione May 15, 2018

kattrali May 15, 2018

$@fractalwrench$ fractalwrench May 16, 2018

martin308 May 16, 2018

kastiglione commented May 15, 2018

martin308 left a comment

martin308 May 16, 2018

$@fractalwrench$ fractalwrench May 17, 2018

martin308 May 16, 2018

fractalwrench commented May 17, 2018

kastiglione May 17, 2018

kattrali May 18, 2018

kattrali May 18, 2018

kattrali May 18, 2018

fractalwrench commented May 18, 2018

martin308 May 21, 2018

$@fractalwrench$ fractalwrench May 22, 2018

martin308 left a comment

		}


		@end

fix: Adds fallback dictionary when serialising device info. #279

fix: Adds fallback dictionary when serialising device info. #279

Conversation

fractalwrench commented May 15, 2018 • edited by kattrali Loading

Goal

Changeset

Changed

Tests

Review

kattrali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione commented May 15, 2018

martin308 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin308 left a comment

Choose a reason for hiding this comment

$@fractalwrench$ fractalwrench commented May 15, 2018 •

edited by kattrali

Loading