fix: updated identify merging logic to ignore nil values (#434)

* fix: updated identify merging logic to ignore nil values to prevent clearing existing user properties * fix: update PR check to run on modern ubuntu * chore: made removeNilValues more efficient * chore: update semantic-pr workflow to use ubuntu 22.04 * chore: improve efficient of identify collapsing logic
amplitude · Apr 18, 2023 · 8e1239c · 8e1239c
1 parent 1db7f83
commit 8e1239c
Show file tree

Hide file tree

Showing 5 changed files with 108 additions and 15 deletions.
diff --git a/.github/workflows/semantic-pr.yml b/.github/workflows/semantic-pr.yml
@@ -7,7 +7,7 @@ on:
 jobs:
   pr-title-check:
     name: Check PR for semantic title
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-22.04
     steps:
       - name: PR title is valid
         if: >

diff --git a/Sources/Amplitude/AMPIdentifyInterceptor.m b/Sources/Amplitude/AMPIdentifyInterceptor.m
@@ -34,6 +34,7 @@
 
 #import <Foundation/Foundation.h>
 #import "AMPConstants.h"
+#import "AMPUtils.h"
 #import "AMPEventUtils.h"
 #import "AMPIdentifyInterceptor.h"
 #import "AMPDatabaseHelper.h"
@@ -212,15 +213,8 @@ - (NSMutableDictionary *_Nonnull)mergeUserPropertyOperations:(NSMutableDictionar
         NSString *operation = _interceptOps[opIndex];
         NSMutableDictionary *mergedOperationKVPs = [NSMutableDictionary dictionary];
 
-        NSMutableDictionary *operationKVPs = userPropertyOperations[operation];
-        if (operationKVPs != nil) {
-            [mergedOperationKVPs addEntriesFromDictionary:operationKVPs];
-        }
-
-        NSMutableDictionary *operationKVPsToMerge = userPropertyOperationsToMerge[operation];
-        if (operationKVPsToMerge != nil) {
-            [mergedOperationKVPs addEntriesFromDictionary:operationKVPsToMerge];
-        }
+        [AMPUtils addNonNilEntriesToDictionary:mergedOperationKVPs fromDictionary:userPropertyOperations[operation]];
+        [AMPUtils addNonNilEntriesToDictionary:mergedOperationKVPs fromDictionary:userPropertyOperationsToMerge[operation]];
 
         if (mergedOperationKVPs.count > 0) {
             [mergedUserProperties setValue:mergedOperationKVPs forKey:operation];
@@ -230,12 +224,10 @@ - (NSMutableDictionary *_Nonnull)mergeUserPropertyOperations:(NSMutableDictionar
     return mergedUserProperties;
 }
 
-- (NSMutableDictionary *_Nonnull)mergeUserProperties:(NSMutableDictionary *_Nonnull) userPropertiess withUserProperties:(NSMutableDictionary *_Nonnull) userPropertiesToMerge {
-    NSMutableDictionary *mergedUserProperties = [userPropertiess mutableCopy] ?: [NSMutableDictionary dictionary];
+- (NSMutableDictionary *_Nonnull)mergeUserProperties:(NSMutableDictionary *_Nonnull) userProperties withUserProperties:(NSMutableDictionary *_Nonnull) userPropertiesToMerge {
+    NSMutableDictionary *mergedUserProperties = [userProperties mutableCopy] ?: [NSMutableDictionary dictionary];
 
-    if (userPropertiesToMerge != nil) {
-        [mergedUserProperties addEntriesFromDictionary:userPropertiesToMerge];
-    }
+    [AMPUtils addNonNilEntriesToDictionary:mergedUserProperties fromDictionary:userPropertiesToMerge];
 
     return mergedUserProperties;
 }

diff --git a/Sources/Amplitude/AMPUtils.h b/Sources/Amplitude/AMPUtils.h
@@ -30,6 +30,7 @@
 
 + (NSString *)generateUUID;
 + (id)makeJSONSerializable:(id)obj;
++ (NSMutableDictionary *)addNonNilEntriesToDictionary:(NSMutableDictionary *)destination fromDictionary:(NSDictionary *)source;
 + (BOOL)isEmptyString:(NSString *)str;
 + (NSDictionary *)validateGroups:(NSDictionary *)obj;
 + (NSString *)platformDataDirectory;

diff --git a/Sources/Amplitude/AMPUtils.m b/Sources/Amplitude/AMPUtils.m
@@ -96,6 +96,18 @@ + (id)makeJSONSerializable:(id)obj {
     return str;
 }
 
++ (NSMutableDictionary *)addNonNilEntriesToDictionary:(NSMutableDictionary *)destination fromDictionary:(NSDictionary *)source {
+    if (source != nil) {
+        for (NSString * key in [source allKeys]) {
+            if (![[source objectForKey:key] isKindOfClass:[NSNull class]]) {
+                [destination setObject:[source objectForKey:key] forKey:key];
+            }
+        }
+    }
+
+    return destination;
+}
+
 + (BOOL)isEmptyString:(NSString *)str {
     return str == nil || [str isKindOfClass:[NSNull class]] || [str length] == 0;
 }

diff --git a/Tests/IdentifyInterceptorTests.m b/Tests/IdentifyInterceptorTests.m
@@ -276,4 +276,92 @@ - (void)testMultipleInterceptedIdentifyIsAppliedToNextActiveEvent {
     XCTAssertTrue([userProperties3[@"set-key-2"] isEqualToString:@"set-value-b"]);
     XCTAssertTrue([userProperties3[@"set-key-3"] isEqualToString:@"set-value-d"]);
 }
+
+- (void)testNullValuesInIdentifySetAreIgnoredOnActiveIdentify {
+    // intercept identify 1
+    AMPIdentify *identify1 = [AMPIdentify.identify set:@"set-key" value:@"set-value-a"];
+    [identify1 set:@"set-key-2" value:@"set-value-b"];
+    NSMutableDictionary *event1 = [self getIdentifyEvent:identify1];
+
+    event1 = [self->_identifyInterceptor intercept:event1];
+
+    XCTAssertNil(event1);
+    XCTAssertEqual(self->_dbHelper.getLastSequenceNumber, 1);
+    XCTAssertEqual(self->_dbHelper.getInterceptedIdentifyCount, 1);
+
+    // intercept identify 2
+    AMPIdentify *identify2 = [AMPIdentify.identify set:@"set-key" value:nil];
+    [identify2 set:@"set-key-2" value:@"set-value-c"];
+    [identify2 set:@"set-key-3" value:nil];
+    NSMutableDictionary *event2 = [self getIdentifyEvent:identify2];
+
+    event2 = [self->_identifyInterceptor intercept:event2];
+
+    XCTAssertNil(event2);
+    XCTAssertEqual(self->_dbHelper.getLastSequenceNumber, 2);
+    XCTAssertEqual(self->_dbHelper.getInterceptedIdentifyCount, 2);
+
+    // active identify
+    AMPIdentify *identify3 = [AMPIdentify.identify add:@"add-key" value:[NSNumber numberWithInt:1]];
+    NSMutableDictionary *event3 = [self getIdentifyEvent:identify3];
+
+    event3 = [self->_identifyInterceptor intercept:event3];
+
+    XCTAssertNotNil(event3);
+    XCTAssertEqual(self->_dbHelper.getLastSequenceNumber, 3);
+    XCTAssertEqual(self->_dbHelper.getInterceptedIdentifyCount, 0);
+
+    // check merged user properties
+    NSMutableDictionary *userProperties3 = [AMPEventUtils getUserProperties:event3];
+    NSArray *userPropertiesOperations3 = [userProperties3 allKeys];
+    XCTAssertNotNil(userProperties3);
+    XCTAssertEqual(userPropertiesOperations3.count, 2);
+    BOOL hasAllOperations = [[NSSet setWithArray:userPropertiesOperations3] isEqualToSet:[NSSet setWithArray:@[AMP_OP_SET, AMP_OP_ADD]]];
+    XCTAssertTrue(hasAllOperations);
+    XCTAssertTrue([userProperties3[AMP_OP_SET][@"set-key"] isEqualToString:@"set-value-a"]);
+    XCTAssertTrue([userProperties3[AMP_OP_SET][@"set-key-2"] isEqualToString:@"set-value-c"]);
+    XCTAssertNil(userProperties3[AMP_OP_SET][@"set-key-3"]);
+}
+
+- (void)testNullValuesInIdentifySetAreIgnoredOnActiveEvent {
+    // intercept identify 1
+    AMPIdentify *identify1 = [AMPIdentify.identify set:@"set-key" value:@"set-value-a"];
+    [identify1 set:@"set-key-2" value:@"set-value-b"];
+    NSMutableDictionary *event1 = [self getIdentifyEvent:identify1];
+
+    event1 = [self->_identifyInterceptor intercept:event1];
+
+    XCTAssertNil(event1);
+    XCTAssertEqual(self->_dbHelper.getLastSequenceNumber, 1);
+    XCTAssertEqual(self->_dbHelper.getInterceptedIdentifyCount, 1);
+
+    // intercept identify 2
+    AMPIdentify *identify2 = [AMPIdentify.identify set:@"set-key" value:nil];
+    [identify2 set:@"set-key-2" value:@"set-value-c"];
+    [identify2 set:@"set-key-3" value:nil];
+    NSMutableDictionary *event2 = [self getIdentifyEvent:identify2];
+
+    event2 = [self->_identifyInterceptor intercept:event2];
+
+    XCTAssertNil(event2);
+    XCTAssertEqual(self->_dbHelper.getLastSequenceNumber, 2);
+    XCTAssertEqual(self->_dbHelper.getInterceptedIdentifyCount, 2);
+
+    // active event
+    NSMutableDictionary *event3 = [self getEvent:@"test"];
+
+    event3 = [self->_identifyInterceptor intercept:event3];
+
+    XCTAssertNotNil(event3);
+    XCTAssertEqual(self->_dbHelper.getLastSequenceNumber, 3);
+    XCTAssertEqual(self->_dbHelper.getInterceptedIdentifyCount, 0);
+
+    // check merged user properties
+    NSMutableDictionary *userProperties3 = [AMPEventUtils getUserProperties:event3];
+    XCTAssertNotNil(userProperties3);
+    XCTAssertEqual(userProperties3.count, 2);
+    XCTAssertTrue([userProperties3[@"set-key"] isEqualToString:@"set-value-a"]);
+    XCTAssertTrue([userProperties3[@"set-key-2"] isEqualToString:@"set-value-c"]);
+    XCTAssertNil(userProperties3[@"set-key-3"]);
+}
 @end