From 8e1239c369e0dfe4c26f2dce1b05d093f6e18aff Mon Sep 17 00:00:00 2001 From: Justin Fiedler Date: Tue, 18 Apr 2023 11:49:54 -0700 Subject: [PATCH] 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 --- .github/workflows/semantic-pr.yml | 2 +- Sources/Amplitude/AMPIdentifyInterceptor.m | 20 ++--- Sources/Amplitude/AMPUtils.h | 1 + Sources/Amplitude/AMPUtils.m | 12 +++ Tests/IdentifyInterceptorTests.m | 88 ++++++++++++++++++++++ 5 files changed, 108 insertions(+), 15 deletions(-) diff --git a/.github/workflows/semantic-pr.yml b/.github/workflows/semantic-pr.yml index ad6474d5..f71fe995 100644 --- 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 index f6783e22..6c44c7b5 100644 --- a/Sources/Amplitude/AMPIdentifyInterceptor.m +++ b/Sources/Amplitude/AMPIdentifyInterceptor.m @@ -34,6 +34,7 @@ #import #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 index c4ac09bf..2e29e2ac 100644 --- 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 index cc1a3c7e..30e7b2cc 100644 --- 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 index 135d85cf..0561e9f6 100644 --- 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