Fixes a bug in custom key and user ID logging (#9289)

elenadoty · web-flow · commit 78b9b7fb10fa · 2022-02-03T11:31:02.000-05:00
diff --git a/Crashlytics/CHANGELOG.md b/Crashlytics/CHANGELOG.md
@@ -1,3 +1,6 @@
+# Unreleased
+- [fixed] Fixed an issue where passing nil as a value for a custom key or user ID did not clear the stored value as expected.
+
 # v8.9.0
 - [fixed] Fixed an issue where exceptions with `nil` reasons weren't properly recorded (#8671).
 
diff --git a/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m b/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m
@@ -41,7 +41,8 @@
 #pragma mark - Prototypes
 static void FIRCLSUserLoggingWriteKeysAndValues(NSDictionary *keysAndValues,
                                                 FIRCLSUserLoggingKVStorage *storage,
-                                                uint32_t *counter);
+                                                uint32_t *counter,
+                                                BOOL containsNullValue);
 static void FIRCLSUserLoggingCheckAndSwapABFiles(FIRCLSUserLoggingABStorage *storage,
                                                  const char **activePath,
                                                  off_t fileSize);
@@ -75,7 +76,7 @@ void FIRCLSUserLoggingWriteInternalKeyValue(NSString *key, NSString *value) {
   NSDictionary *keysAndValues = key ? @{key : value ?: [NSNull null]} : nil;
   FIRCLSUserLoggingWriteKeysAndValues(keysAndValues,
                                       &_firclsContext.readonly->logging.internalKVStorage,
-                                      &_firclsContext.writable->logging.internalKVCount);
+                                      &_firclsContext.writable->logging.internalKVCount, NO);
 }
 
 void FIRCLSUserLoggingRecordUserKeyValue(NSString *key, id value) {
@@ -250,6 +251,8 @@ void FIRCLSUserLoggingRecordKeysAndValues(NSDictionary *keysAndValues,
   }
 
   NSMutableDictionary *sanitizedKeysAndValues = [keysAndValues mutableCopy];
+  BOOL containsNullValue = NO;
+
   for (NSString *key in keysAndValues) {
     if (!FIRCLSIsValidPointer(key)) {
       FIRCLSSDKLogWarn("User provided bad key\n");
@@ -264,23 +267,26 @@ void FIRCLSUserLoggingRecordKeysAndValues(NSDictionary *keysAndValues,
       sanitizedKeysAndValues[key] = [NSNull null];
     }
 
-    if ([value respondsToSelector:@selector(description)]) {
+    if ([value respondsToSelector:@selector(description)] && ![value isEqual:[NSNull null]]) {
       sanitizedKeysAndValues[key] = [value description];
     } else {
       // passing nil will result in a JSON null being written, which is deserialized as [NSNull
       // null], signaling to remove the key during compaction
       sanitizedKeysAndValues[key] = [NSNull null];
+      containsNullValue = YES;
     }
   }
 
   dispatch_sync(FIRCLSGetLoggingQueue(), ^{
-    FIRCLSUserLoggingWriteKeysAndValues(sanitizedKeysAndValues, storage, counter);
+    FIRCLSUserLoggingWriteKeysAndValues(sanitizedKeysAndValues, storage, counter,
+                                        containsNullValue);
   });
 }
 
 static void FIRCLSUserLoggingWriteKeysAndValues(NSDictionary *keysAndValues,
                                                 FIRCLSUserLoggingKVStorage *storage,
-                                                uint32_t *counter) {
+                                                uint32_t *counter,
+                                                BOOL containsNullValue) {
   FIRCLSFile file;
 
   if (!FIRCLSIsValidPointer(storage) || !FIRCLSIsValidPointer(counter)) {
@@ -297,7 +303,7 @@ static void FIRCLSUserLoggingWriteKeysAndValues(NSDictionary *keysAndValues,
   FIRCLSFileClose(&file);
 
   *counter += keysAndValues.count;
-  if (*counter >= storage->maxIncrementalCount) {
+  if (*counter >= storage->maxIncrementalCount || containsNullValue) {
     dispatch_async(FIRCLSGetLoggingQueue(), ^{
       FIRCLSUserLoggingCompactKVEntries(storage);
       *counter = 0;
diff --git a/Crashlytics/Crashlytics/FIRCrashlytics.m b/Crashlytics/Crashlytics/FIRCrashlytics.m
@@ -294,13 +294,13 @@ - (void)deleteUnsentReports {
 }
 
 #pragma mark - API: setUserID
-- (void)setUserID:(NSString *)userID {
+- (void)setUserID:(nullable NSString *)userID {
   FIRCLSUserLoggingRecordInternalKeyValue(FIRCLSUserIdentifierKey, userID);
 }
 
 #pragma mark - API: setCustomValue
 
-- (void)setCustomValue:(id)value forKey:(NSString *)key {
+- (void)setCustomValue:(nullable id)value forKey:(NSString *)key {
   FIRCLSUserLoggingRecordUserKeyValue(key, value);
 }
 
diff --git a/Crashlytics/Crashlytics/FIRCrashlyticsReport.m b/Crashlytics/Crashlytics/FIRCrashlyticsReport.m
@@ -167,7 +167,7 @@ - (void)logWithFormat:(NSString *)format arguments:(va_list)args {
 
 #pragma mark - API: setUserID
 
-- (void)setUserID:(NSString *)userID {
+- (void)setUserID:(nullable NSString *)userID {
   if (![self checkContextForMethod:@"setUserID:"]) {
     return;
   }
@@ -178,7 +178,7 @@ - (void)setUserID:(NSString *)userID {
 
 #pragma mark - API: setCustomValue
 
-- (void)setCustomValue:(id)value forKey:(NSString *)key {
+- (void)setCustomValue:(nullable id)value forKey:(NSString *)key {
   if (![self checkContextForMethod:@"setCustomValue:forKey:"]) {
     return;
   }
diff --git a/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlytics.h b/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlytics.h
@@ -82,7 +82,7 @@ NS_SWIFT_NAME(Crashlytics)
  * @param value The value to be associated with the key
  * @param key A unique key
  */
-- (void)setCustomValue:(id)value forKey:(NSString *)key;
+- (void)setCustomValue:(nullable id)value forKey:(NSString *)key;
 
 /**
  * Sets custom keys and values to be associated with subsequent fatal and non-fatal reports.
@@ -104,7 +104,7 @@ NS_SWIFT_NAME(Crashlytics)
  * @param userID An arbitrary user identifier string that associates a user to a record in your
  * system.
  */
-- (void)setUserID:(NSString *)userID;
+- (void)setUserID:(nullable NSString *)userID;
 
 /**
  * Records a non-fatal event described by an Error object. The events are
diff --git a/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlyticsReport.h b/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlyticsReport.h
@@ -79,7 +79,7 @@ NS_SWIFT_NAME(CrashlyticsReport)
  * @param value The value to be associated with the key
  * @param key A unique key
  */
-- (void)setCustomValue:(id)value forKey:(NSString *)key;
+- (void)setCustomValue:(nullable id)value forKey:(NSString *)key;
 
 /**
  * Sets custom keys and values to be associated with subsequent fatal and non-fatal reports.
@@ -101,7 +101,7 @@ NS_SWIFT_NAME(CrashlyticsReport)
  * @param userID An arbitrary user identifier string that associates a user to a record in your
  * system.
  */
-- (void)setUserID:(NSString *)userID;
+- (void)setUserID:(nullable NSString *)userID;
 
 @end
 
diff --git a/Crashlytics/UnitTests/FIRCrashlyticsReportTests.m b/Crashlytics/UnitTests/FIRCrashlyticsReportTests.m
@@ -106,6 +106,29 @@ - (void)testSetUserID {
   XCTAssertEqualObjects(entries[0][@"kv"][@"value"], FIRCLSFileHexEncodeString("12345-6"), @"");
 }
 
+- (void)testClearUserID {
+  FIRCrashlyticsReport *report = [self createTempCopyOfReportWithName:@"metadata_only_report"];
+
+  // Add a user ID
+  [report setUserID:@"12345-6"];
+  NSArray *entries = FIRCLSFileReadSections(
+      [[report.internalReport pathForContentFile:FIRCLSReportInternalIncrementalKVFile]
+          fileSystemRepresentation],
+      false, nil);
+
+  XCTAssertEqual([entries count], 1, @"");
+
+  // Now remove it
+  [report setUserID:nil];
+
+  entries = FIRCLSFileReadSections(
+      [[report.internalReport pathForContentFile:FIRCLSReportInternalCompactedKVFile]
+          fileSystemRepresentation],
+      false, nil);
+
+  XCTAssertEqual([entries count], 0, @"");
+}
+
 - (void)testCustomKeysNoExisting {
   FIRCrashlyticsReport *report = [self createTempCopyOfReportWithName:@"metadata_only_report"];
 
@@ -168,6 +191,45 @@ - (void)testCustomKeysWithExisting {
   XCTAssertEqualObjects(entries[4][@"kv"][@"value"], FIRCLSFileHexEncodeString("10"), @"");
 }
 
+- (void)testClearCustomKeys {
+  FIRCrashlyticsReport *report = [self createTempCopyOfReportWithName:@"metadata_only_report"];
+
+  // Add keys
+  [report setCustomValue:@"hello" forKey:@"mykey"];
+  [report setCustomValue:@"goodbye" forKey:@"anotherkey"];
+
+  [report setCustomKeysAndValues:@{
+    @"is_test" : @(YES),
+    @"test_number" : @(10),
+  }];
+
+  NSArray *entries = FIRCLSFileReadSections(
+      [[report.internalReport pathForContentFile:FIRCLSReportUserIncrementalKVFile]
+          fileSystemRepresentation],
+      false, nil);
+
+  XCTAssertEqual([entries count], 4, @"");
+
+  // Now remove them
+  [report setCustomValue:nil forKey:@"mykey"];
+  [report setCustomValue:nil forKey:@"anotherkey"];
+  entries = FIRCLSFileReadSections(
+      [[report.internalReport pathForContentFile:FIRCLSReportUserIncrementalKVFile]
+          fileSystemRepresentation],
+      false, nil);
+  [report setCustomKeysAndValues:@{
+    @"is_test" : [NSNull null],
+    @"test_number" : [NSNull null],
+  }];
+
+  entries = FIRCLSFileReadSections(
+      [[report.internalReport pathForContentFile:FIRCLSReportInternalCompactedKVFile]
+          fileSystemRepresentation],
+      false, nil);
+
+  XCTAssertEqual([entries count], 0, @"");
+}
+
 - (void)testCustomKeysLimits {
   FIRCrashlyticsReport *report = [self createTempCopyOfReportWithName:@"ios_all_files_crash"];
 

Original file line number	Diff line number	Diff line change
`@@ -294,13 +294,13 @@ - (void)deleteUnsentReports {`
`294`	`294`	`}`
`295`	`295`
`296`	`296`	`#pragma mark - API: setUserID`
`297`		`-- (void)setUserID:(NSString *)userID {`
	`297`	`+- (void)setUserID:(nullable NSString *)userID {`
`298`	`298`	`FIRCLSUserLoggingRecordInternalKeyValue(FIRCLSUserIdentifierKey, userID);`
`299`	`299`	`}`
`300`	`300`
`301`	`301`	`#pragma mark - API: setCustomValue`
`302`	`302`
`303`		`-- (void)setCustomValue:(id)value forKey:(NSString *)key {`
	`303`	`+- (void)setCustomValue:(nullable id)value forKey:(NSString *)key {`
`304`	`304`	`FIRCLSUserLoggingRecordUserKeyValue(key, value);`
`305`	`305`	`}`
`306`	`306`
Original file line number	Diff line number	Diff line change
`@@ -167,7 +167,7 @@ - (void)logWithFormat:(NSString *)format arguments:(va_list)args {`
`167`	`167`
`168`	`168`	`#pragma mark - API: setUserID`
`169`	`169`
`170`		`-- (void)setUserID:(NSString *)userID {`
	`170`	`+- (void)setUserID:(nullable NSString *)userID {`
`171`	`171`	`if (![self checkContextForMethod:@"setUserID:"]) {`
`172`	`172`	`return;`
`173`	`173`	`}`
`@@ -178,7 +178,7 @@ - (void)setUserID:(NSString *)userID {`
`178`	`178`
`179`	`179`	`#pragma mark - API: setCustomValue`
`180`	`180`
`181`		`-- (void)setCustomValue:(id)value forKey:(NSString *)key {`
	`181`	`+- (void)setCustomValue:(nullable id)value forKey:(NSString *)key {`
`182`	`182`	`if (![self checkContextForMethod:@"setCustomValue:forKey:"]) {`
`183`	`183`	`return;`
`184`	`184`	`}`