feat: add method to add metadata section values as a dictionary (#470)

bugsnag · Feb 27, 2020 · 8ab74ff · 8ab74ff
1 parent 27ba1e3
commit 8ab74ff
Show file tree

Hide file tree

Showing 5 changed files with 298 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -49,9 +49,8 @@ Bugsnag Notifiers on other platforms.
   [#459](https://github.com/bugsnag/bugsnag-cocoa/pull/459)
 
 * Added `Bugsnag.getMetadata(_ section: key:)`
-[#463](https://github.com/bugsnag/bugsnag-cocoa/pull/463)
+  [#463](https://github.com/bugsnag/bugsnag-cocoa/pull/463)
 
-
 * Add a per-Event `apiKey` property.  This defaults to the global 
   `BugsnagConfiguration` value but can be overridden in event passed to the 
   `Bugsnag.notify()` callback.
@@ -90,6 +89,17 @@ Bugsnag Notifiers on other platforms.
   the `BugsnagLogger.h` header.
   [#472](https://github.com/bugsnag/bugsnag-cocoa/pull/472)
 
+* Added a method to allow merging supplied and existing Event metadata.
+  `BugsnagMetadata.addMetadataToSection:values:` allows Event 
+  callbacks to modify Event metadata en-mass.  Supplied metadata should 
+  be a JSON-serializable dictionary.  The resulting Event metadata is the 
+  result of applying the following rules to the existing metadata for each supplied
+  value:
+  - Non-null values replace any existing key/value pair. 
+  - Null values remove a key/value pair.  
+  - Invalid values are logged and ignored.
+  [#470](https://github.com/bugsnag/bugsnag-cocoa/pull/470)
+
 ## Bug fixes
 
 * Fix possible report corruption when using `notify()` from multiple threads

diff --git a/Source/BugsnagMetadata.h b/Source/BugsnagMetadata.h
@@ -79,6 +79,27 @@
 
 @property(unsafe_unretained) id<BugsnagMetadataDelegate> _Nullable delegate;
 
+/**
+ * Merge supplied and existing metadata.
+ *
+ * - Non-null values will replace existing values for identical keys.
+ 
+ * - Null values will remove the existing key/value pair if the key exists.
+ *   Where null-valued keys do not exist they will not be set.  (Since ObjC
+ *   dicts can't store 'nil' directly we assume [NSNUll null])
+ *
+ * - Tabs are only created if at least one value is valid.
+ *
+ * - Invalid values (i.e. unserializable to JSON) are logged and ignored.
+ *
+ * @param section The name of the metadata section
+ *
+ * @param values A dictionary of string -> id key/value pairs.
+ *               Values should be serializable to JSON.
+ */
+- (void)addMetadataToSection:(NSString *_Nonnull)section
+                      values:(NSDictionary *_Nullable)values;
+
 @end
 
 @protocol BugsnagMetadataDelegate <NSObject>

diff --git a/Source/BugsnagMetadata.m b/Source/BugsnagMetadata.m
@@ -94,33 +94,108 @@ - (NSDictionary *)toDictionary {
     }
 }
 
+/**
+ * Add a single key/value to a metadata Tab/Section.
+ */
 - (void)addAttribute:(NSString *)attributeName
            withValue:(id)value
-       toTabWithName:(NSString *)sectionName
-{
+       toTabWithName:(NSString *)tabName {
+
+    bool metadataChanged = false;
     @synchronized(self) {
-        if (value) {
+        if (value && value != [NSNull null]) {
             id cleanedValue = BSGSanitizeObject(value);
             if (cleanedValue) {
-                NSDictionary *section = [self getMetadata:sectionName];
+                // Value is OK, try and set it
+                NSMutableDictionary *section = [self getMetadata:tabName];
                 if (!section) {
                     section = [NSMutableDictionary new];
-                    [[self dictionary] setObject:section forKey:sectionName];
+                    [[self dictionary] setObject:section forKey:tabName];
                 }
-                [section setValue:cleanedValue forKey:attributeName];
-            }
-            else {
+                section[attributeName] = cleanedValue;
+                metadataChanged = true;
+            } else {
                 Class klass = [value class];
                 bsg_log_err(@"Failed to add metadata: Value of class %@ is not "
                             @"JSON serializable",
                             klass);
             }
         }
+
+        // It's some form of nil/null
         else {
-            [[self getMetadata:sectionName] removeObjectForKey:attributeName];
+            [[self getMetadata:tabName] removeObjectForKey:attributeName];
+            metadataChanged = true;
         }
     }
-    [self.delegate metadataChanged:self];
+
+    // Call the delegate if we've materially changed it
+    if (metadataChanged) {
+        [self.delegate metadataChanged:self];
+    }
 }
 
+/**
+ * Merge supplied and existing metadata.
+ */
+- (void)addMetadataToSection:(NSString *)section
+                      values:(NSDictionary *)values
+{
+    @synchronized(self) {
+        if (values) {
+            // Check each value in turn.  Remove nulls, add/replace others
+            // Fast enumeration over the (unmodified) supplied values for simplicity
+            bool metadataChanged = false;
+            for (id key in values) {
+                // Ensure keys are (JSON-serializable) strings
+                if ([[key class] isSubclassOfClass:[NSString class]]) {
+                    id value = [values objectForKey:key];
+
+                    // The common case: adding sensible values
+                    if (value && value != [NSNull null]) {
+                        id cleanedValue = BSGSanitizeObject(value);
+                        if (cleanedValue) {
+                            // We only want to create a tab if we have a valid value.
+                            NSMutableDictionary *metadata = [self getMetadata:section];
+                            if (!metadata) {
+                                metadata = [NSMutableDictionary new];
+                                [[self dictionary] setObject:metadata forKey:section];
+                            }
+                            [metadata setObject:cleanedValue forKey:key];
+                            metadataChanged = true;
+                        }
+                        // Log the failure but carry on
+                        else {
+                            Class klass = [value class];
+                            bsg_log_err(@"Failed to add metadata: Value of class %@ is not "
+                                        @"JSON serializable.", klass);
+                        }
+                    }
+
+                    // Remove existing value if supplied null.
+                    // Ensure we don't inadvertently create a section.
+                    else if (value == [NSNull null]
+                             && [self.dictionary objectForKey:section]
+                             && [[self.dictionary objectForKey:section] objectForKey:key])
+                    {
+                        [[self.dictionary objectForKey:section] removeObjectForKey:key];
+                        metadataChanged = true;
+                    }
+                }
+
+                // Something went wrong...
+                else {
+                    bsg_log_err(@"Failed to update metadata: Section: %@, Values: %@", section, values);
+                }
+            }
+
+            // Call the delegate if we've materially changed it
+            if (metadataChanged) {
+                [self.delegate metadataChanged:self];
+            }
+        }
+    }
+}
+
+
 @end
diff --git a/Tests/BugsnagMetadataTests.m b/Tests/BugsnagMetadataTests.m
@@ -9,12 +9,183 @@
 #import <XCTest/XCTest.h>
 #import "BugsnagMetadata.h"
 
-@interface BugsnagMetadataTests : XCTestCase
+// MARK: - Expose tested-class internals
 
+@interface BugsnagMetadataTests : XCTestCase <BugsnagMetadataDelegate>
+@property BOOL delegateCalled;
+@property BugsnagMetadata *metadata;
 @end
 
+@interface BugsnagMetadata ()
+@property(atomic, strong) NSMutableDictionary *dictionary;
+@end
+
+// MARK: - DummyClass
+
+@interface DummyClass : NSObject <NSCopying> // <NSCopying> allows it to be used as a dictionary key
+@property NSString *name;
+@end
+
+@implementation DummyClass
+@synthesize name;
+- (nonnull id)copyWithZone:(nullable NSZone *)zone {
+    DummyClass *copy = [DummyClass new];
+    copy.name = self.name;
+    return copy;
+}
+
+@end
+
+// MARK: - Tests
+
 @implementation BugsnagMetadataTests
 
+@synthesize delegateCalled;
+@synthesize metadata;
+
+-(void) setUp {
+    metadata = [[BugsnagMetadata alloc] init];
+    XCTAssertNil([metadata delegate]);
+    metadata.delegate = self;
+}
+
+- (void)test_addAttribute_withName_creation {
+
+    // Creation
+    delegateCalled = NO;
+    XCTAssertNotNil(metadata);
+    XCTAssertFalse(delegateCalled, "Did not expect the delegate's metadataChanged: method to be called.");
+
+    XCTAssertNotNil([metadata delegate]);
+    XCTAssertFalse(delegateCalled, "Did not expect the delegate's metadataChanged: method to be called.");
+}
+
+- (void)test_addAttribute_withName_create_return {
+    // Arbitrary tab name creates and returns itself
+    delegateCalled = NO;
+    NSMutableDictionary *tab = [metadata getMetadata:@"unknown"];
+    XCTAssertNil(tab);
+    XCTAssertEqual([[metadata toDictionary] count], 0);
+    XCTAssertFalse(delegateCalled, "Didn't expect the delegate's metadataChanged: method to be called.");
+}
+
+- (void)test_addAttribute_withName_named_tab_set {
+    // Check that the arbitrary named tab was set.
+    delegateCalled = NO;
+    [metadata addAttribute:@"foo" withValue:@"aValue" toTabWithName:@"SecondTab"];
+    XCTAssertEqual([[metadata toDictionary] count], 1);
+    XCTAssertTrue(delegateCalled, "Expected the delegate's metadataChanged: method to be called.");
+
+    [metadata addAttribute:@"foo" withValue:@"aValue" toTabWithName:@"FirstTab"];
+    NSMutableDictionary *tab2 = [metadata getMetadata:@"FirstTab"];
+    XCTAssertNotNil(tab2);
+    XCTAssertEqual(tab2.count, 1);
+
+    [metadata addAttribute:@"bar" withValue:@"anotherValue" toTabWithName:@"FirstTab"];
+    tab2 = [metadata getMetadata:@"FirstTab"];
+    XCTAssertEqual(tab2.count, 2);
+
+    NSDictionary *dict = [metadata toDictionary];
+    XCTAssertEqual([dict count], 2);
+
+    delegateCalled = NO;
+    [metadata clearMetadataInSection:@"FirstTab"];
+    tab2 = [metadata getMetadata:@"FirstTab"];
+    XCTAssertEqual(tab2.count, 0);
+    XCTAssertTrue(delegateCalled, "Expected the delegate's metadataChanged: method to be called.");
+}
+
+- (void)test_addAttribute_withName_invalid_values {
+    NSMutableDictionary *tab2 = [metadata getMetadata:@"FirstTab"];
+
+    // Adding invalid values should fail silently (and not add the value)
+    delegateCalled = NO;
+    [metadata addAttribute:@"bar" withValue:[DummyClass new] toTabWithName:@"FirstTab"];
+    tab2 = [metadata getMetadata:@"FirstTab"];
+    XCTAssertEqual(tab2.count, 0);
+    XCTAssertFalse(delegateCalled, "Did not expect the delegate's metadataChanged: method to be called.");
+
+    // Again, add valid value
+    [metadata addAttribute:@"foo" withValue:@"aValue" toTabWithName:@"FirstTab"];
+    tab2 = [metadata getMetadata:@"FirstTab"];
+    XCTAssertNotNil(tab2);
+    XCTAssertEqual(tab2.count, 1);
+
+    // Adding null - should remove the key
+    delegateCalled = NO;
+    [metadata addAttribute:@"bar" withValue:nil toTabWithName:@"FirstTab"];
+    tab2 = [metadata getMetadata:@"FirstTab"];
+    XCTAssertEqual(tab2.count, 1);
+    XCTAssertTrue(delegateCalled, "Expected the delegate's metadataChanged: method to be called.");
+}
+
+- (void) test_addMetadata_values {
+    // Creation
+    BugsnagMetadata *metadata = [[BugsnagMetadata alloc] init];
+    XCTAssertNotNil(metadata);
+
+    // Don't want to create a tab if none of the values are not valid
+    [metadata addMetadataToSection:@"NewTab" values:@{@"aKey" : [DummyClass new]}];
+    XCTAssertEqual([[metadata dictionary] count], 0);
+    [metadata addMetadataToSection:@"NewTab" values:@{@"aKey" : [DummyClass new], @"anotherKey" : [DummyClass new]}];
+    XCTAssertEqual([[metadata dictionary] count], 0);
+
+    // Tab created if at least one value is valid
+    [metadata addMetadataToSection:@"NewTab" values:@{
+        @"aKey" : [DummyClass new],
+        @"secondKey" : @12345}];
+    XCTAssertEqual([[metadata dictionary] count], 1);
+    NSMutableDictionary *tab = [metadata getMetadata:@"NewTab"];
+    XCTAssertEqual([tab count], 1);
+    [metadata addMetadataToSection:@"NewTab" values:@{@"thirdKey" : @"FooBarBaz"}];
+    tab = [metadata getMetadata:@"NewTab"];
+    XCTAssertEqual([tab count], 2);
+    XCTAssertEqual([[metadata dictionary] count], 1);
+
+    // Remove [NSNull null] values
+    [metadata addMetadataToSection:@"NewTab" values:@{@"thirdKey" : [NSNull null]}];
+    tab = [metadata getMetadata:@"NewTab"];
+    XCTAssertEqual([tab count], 1);
+    XCTAssertEqual([[metadata dictionary] count], 1);
+
+    // Addition *AND* removal are possible in a single call
+    [metadata addMetadataToSection:@"NewTab" values:@{@"secondKey" : [NSNull null], @"sixthKey" : @"mother"}];
+    tab = [metadata getMetadata:@"NewTab"];
+    XCTAssertEqual([tab count], 1);
+    XCTAssertEqual([[metadata dictionary] count], 1);
+
+    // Check delegate method gets called
+    delegateCalled = NO;
+    metadata.delegate = self;
+    [metadata addMetadataToSection:@"OtherTab" values:@{@"key" : @"value"}];
+    XCTAssertTrue(delegateCalled, "Expected the delegate's metadataChanged: method to be called.");
+    delegateCalled = NO;
+    [metadata addMetadataToSection:@"OtherTab" values:@{@"key" : [NSNull null]}];
+    XCTAssertTrue(delegateCalled, "Expected the delegate's metadataChanged: method to be called.");
+}
+
+-(void) test_addMetadata_values_invalid_key {
+    BugsnagMetadata *metadata = [[BugsnagMetadata alloc] init];
+    XCTAssertNotNil(metadata);
+    XCTAssertEqual(metadata.dictionary.count, 0);
+
+    // Check for invalid keys
+    delegateCalled = NO;
+    DummyClass *dummyObj = [DummyClass new];
+    dummyObj.name = @"aName";
+
+    [metadata addMetadataToSection:@"invalidKeyTab" values:@{dummyObj : @"someValue"}];
+    XCTAssertEqual(metadata.dictionary.count, 0);
+    XCTAssertFalse(delegateCalled);
+
+    // Once more with a delegate
+    delegateCalled = NO;
+    metadata.delegate = self;
+    [metadata addMetadataToSection:@"invalidKeyTab" values:@{dummyObj : @"someValue"}];
+    XCTAssertEqual(metadata.dictionary.count, 0);
+    XCTAssertFalse(delegateCalled);
+}
+
 - (void)testMutableCopyWithZone {
 
     BugsnagMetadata *metadata = [BugsnagMetadata new];
@@ -70,4 +241,10 @@ - (void)testGetMetadataSectionKey {
     XCTAssertNil([metadata getMetadata:@"noSection" key:@"noKey"]);
 }
 
+// MARK: - <BugsnagMetadataDelegate>
+
+- (void)metadataChanged:(BugsnagMetadata * _Nonnull)metadata {
+    delegateCalled = YES;
+}
+
 @end
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -94,6 +94,8 @@ ObjC:
 - [BugsnagMetadata getTab:]
 + [BugsnagMetadata getSection:]
 
++ [BugsnagMetadata addMetadataToSection:values:]
+
 Swift:
 
 - BugsnagMetadata.clearTab(name:)