Address floc review comments (#6691)

* Address floc review comments * Fix style * fix bad synthesize * Remove segmentation constants from core * fix style
firebase · Oct 8, 2020 · 4baeb82 · 4baeb82
1 parent 13beb58
commit 4baeb82
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 84 deletions.
diff --git a/FirebaseSegmentation/Sources/Public/FIRSegmentation.h b/FirebaseSegmentation/Sources/Public/FIRSegmentation.h
@@ -20,7 +20,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 @class FIRApp;
 /**
- * The Firebase Segmentation SDK is used to associate a custom, non-Firebase custom installation
+ * The Firebase Segmentation SDK is used to associate a custom, non-Firebase installation
  * identifier to Firebase. Once this custom installation identifier is set, developers can use the
  * current app installation for segmentation purposes. If the custom installation identifier is
  * explicitely set to nil, any existing custom installation identifier data will be removed.
@@ -56,7 +56,7 @@ typedef NS_ENUM(NSInteger, FIRSegmentationErrorCode) {
 /// Returns the FIRSegmentation instance for your Firebase application. This singleton class
 /// instance lets you set your own custom identifier to be used for targeting purposes within
 /// Firebase.
-+ (instancetype)segmentationWithApp:(nonnull FIRApp *)app;
++ (instancetype)segmentationWithApp:(FIRApp *)app;
 
 /**
  *  :nodoc:

diff --git a/FirebaseSegmentation/Sources/SEGContentManager.h b/FirebaseSegmentation/Sources/SEGContentManager.h
@@ -23,10 +23,10 @@ NS_ASSUME_NONNULL_BEGIN
 @interface SEGContentManager : NSObject
 
 /// Shared Singleton Instance
-+ (instancetype)sharedInstanceWithOptions:(FIROptions*)options;
++ (instancetype)sharedInstanceWithOptions:(FIROptions *)options;
 
-- (void)associateCustomInstallationIdentiferNamed:(NSString*)customInstallationID
-                                      firebaseApp:(NSString*)appName
+- (void)associateCustomInstallationIdentiferNamed:(NSString *)customInstallationID
+                                      firebaseApp:(NSString *)appName
                                        completion:(SEGRequestCompletion)completionHandler;
 
 @end

diff --git a/FirebaseSegmentation/Sources/SEGContentManager.m b/FirebaseSegmentation/Sources/SEGContentManager.m
@@ -19,20 +19,14 @@
 #import "FirebaseSegmentation/Sources/Public/FIRSegmentation.h"
 #import "FirebaseSegmentation/Sources/SEGDatabaseManager.h"
 #import "FirebaseSegmentation/Sources/SEGNetworkManager.h"
-#import "FirebaseSegmentation/Sources/SEGSegmentationConstants.h"
 
-NSString *const kErrorDescription = @"ErrorDescription";
-
-@interface SEGContentManager () {
+@implementation SEGContentManager {
   NSMutableDictionary<NSString *, id> *_associationData;
   NSString *_installationIdentifier;
   NSString *_installationIdentifierToken;
   SEGDatabaseManager *_databaseManager;
   SEGNetworkManager *_networkManager;
 }
-@end
-
-@implementation SEGContentManager
 
 + (instancetype)sharedInstanceWithOptions:(FIROptions *)options {
   static dispatch_once_t onceToken;
@@ -111,19 +105,13 @@ - (void)associateInstallationWithLatestIdentifier:(NSString *_Nullable)identifie
 
   _installationIdentifier = identifier;
 
-  __weak SEGContentManager *weakSelf = self;
   [installation authTokenWithCompletion:^(FIRInstallationsAuthTokenResult *_Nullable tokenResult,
                                           NSError *_Nullable error) {
-    SEGContentManager *strongSelf = weakSelf;
-    if (!strongSelf) {
-      completionHandler(NO, @{kErrorDescription : @"Internal Error getting installation token."});
-      return;
-    }
-    [strongSelf associateInstallationWithToken:tokenResult
-                          customizedIdentifier:customInstallationID
-                                   firebaseApp:firebaseApp
-                                         error:error
-                                    completion:completionHandler];
+    [self associateInstallationWithToken:tokenResult
+                    customizedIdentifier:customInstallationID
+                             firebaseApp:firebaseApp
+                                   error:error
+                              completion:completionHandler];
   }];
 }
 

diff --git a/FirebaseSegmentation/Sources/SEGDatabaseManager.m b/FirebaseSegmentation/Sources/SEGDatabaseManager.m
@@ -38,16 +38,16 @@ static BOOL SEGAddSkipBackupAttributeToItemAtPath(NSString *filePathString) {
   NSError *error = nil;
   BOOL success = [URL setResourceValue:@YES forKey:NSURLIsExcludedFromBackupKey error:&error];
   if (!success) {
-    // TODO(dmandar): log error.
-    NSLog(@"Error excluding %@ from backup %@.", [URL lastPathComponent], error);
+    FIRLogError(kFIRLoggerSegmentation, @"I-SEG000001", @"Error excluding %@ from backup %@.",
+                [URL lastPathComponent], error);
   }
   return success;
 }
 
 static BOOL SEGCreateFilePathIfNotExist(NSString *filePath) {
-  if (!filePath || !filePath.length) {
-    // TODO(dmandar) log error.
-    NSLog(@"Failed to create subdirectory for an empty file path.");
+  if (!filePath.length) {
+    FIRLogError(kFIRLoggerSegmentation, @"I-SEG000002",
+                @"Failed to create subdirectory for an empty file path.");
     return NO;
   }
   NSFileManager *fileManager = [NSFileManager defaultManager];
@@ -58,23 +58,20 @@ static BOOL SEGCreateFilePathIfNotExist(NSString *filePath) {
                             attributes:nil
                                  error:&error];
     if (error) {
-      // TODO(dmandar) log error.
-      NSLog(@"Failed to create subdirectory for database file: %@.", error);
+      FIRLogError(kFIRLoggerSegmentation, @"I-SEG000003",
+                  @"Failed to create subdirectory for database file: %@.", error);
       return NO;
     }
   }
   return YES;
 }
 
-@interface SEGDatabaseManager () {
+@implementation SEGDatabaseManager {
   /// Database storing all the config information.
   sqlite3 *_database;
   /// Serial queue for database read/write operations.
   dispatch_queue_t _databaseOperationQueue;
 }
-@end
-
-@implementation SEGDatabaseManager
 
 + (instancetype)sharedInstance {
   static dispatch_once_t onceToken;
@@ -97,41 +94,35 @@ - (instancetype)init {
 #pragma mark - Public Methods
 
 - (void)loadMainTableWithCompletion:(SEGRequestCompletion)completionHandler {
-  __weak SEGDatabaseManager *weakSelf = self;
   dispatch_async(_databaseOperationQueue, ^{
-    SEGDatabaseManager *strongSelf = weakSelf;
-    if (!strongSelf) {
-      completionHandler(NO, @{@"Database Error" : @"Internal database error"});
-    }
-
     // Read the database into memory.
     NSDictionary<NSString *, NSDictionary<NSString *, NSString *> *> *associations =
         [self loadMainTable];
-    completionHandler(YES, associations);
+    if (associations != nil) {
+      completionHandler(YES, associations);
+    } else {
+      FIRLogError(kFIRLoggerSegmentation, @"I-SEG000004", @"Failed to load main table.");
+      completionHandler(NO, @{});
+    }
   });
   return;
 }
 
 - (void)createOrOpenDatabaseWithCompletion:(SEGRequestCompletion)completionHandler {
-  __weak SEGDatabaseManager *weakSelf = self;
   dispatch_async(_databaseOperationQueue, ^{
-    SEGDatabaseManager *strongSelf = weakSelf;
-    if (!strongSelf) {
-      completionHandler(NO, @{@"ErrorDescription" : @"Internal database error"});
-    }
     NSString *dbPath = [SEGDatabaseManager pathForSegmentationDatabase];
-    // TODO(dmandar) log.
-    NSLog(@"Loading segmentation database at path %@", dbPath);
+    FIRLogDebug(kFIRLoggerSegmentation, @"I-SEG000005", @"Loading segmentation database at path %@",
+                dbPath);
     const char *databasePath = dbPath.UTF8String;
     // Create or open database path.
     if (!SEGCreateFilePathIfNotExist(dbPath)) {
-      completionHandler(NO, @{@"ErrorDescription" : @"Could not create database file at path"});
+      completionHandler(NO, @{kSEGErrorDescription : @"Could not create database file at path"});
     }
     int flags = SQLITE_OPEN_CREATE | SQLITE_OPEN_READWRITE | SQLITE_OPEN_FILEPROTECTION_COMPLETE |
                 SQLITE_OPEN_FULLMUTEX;
-    if (sqlite3_open_v2(databasePath, &strongSelf->_database, flags, NULL) == SQLITE_OK) {
+    if (sqlite3_open_v2(databasePath, &self->_database, flags, NULL) == SQLITE_OK) {
       // Create table if does not exist already.
-      if ([strongSelf createTableSchema]) {
+      if ([self createTableSchema]) {
         // DB file created or already exists.
         // Exclude the app data used from iCloud backup.
         SEGAddSkipBackupAttributeToItemAtPath(dbPath);
@@ -142,42 +133,44 @@ - (void)createOrOpenDatabaseWithCompletion:(SEGRequestCompletion)completionHandl
 
       } else {
         // Remove database before fail.
-        [strongSelf removeDatabase:dbPath];
-        FIRLogError(kFIRLoggerSegmentation, @"I-SEG000010", @"Failed to create table.");
+        [self removeDatabase:dbPath];
+        FIRLogError(kFIRLoggerSegmentation, @"I-SEG00006", @"Failed to create table.");
         // Create a new database if existing database file is corrupted.
         if (!SEGCreateFilePathIfNotExist(dbPath)) {
-          completionHandler(NO,
-                            @{@"ErrorDescription" : @"Could not recreate database file at path"});
+          completionHandler(
+              NO,
+              @{kSEGErrorDescription : @"Could not recreate database file at path: %@", dbpath});
+          return;
         }
-        if (sqlite3_open_v2(databasePath, &strongSelf->_database, flags, NULL) == SQLITE_OK) {
-          if (![strongSelf createTableSchema]) {
+        // Try to open the database with the new file.
+        if (sqlite3_open_v2(databasePath, &self->_database, flags, NULL) == SQLITE_OK) {
+          if (![self createTableSchema]) {
             // Remove database before fail.
-            [strongSelf removeDatabase:dbPath];
+            [self removeDatabase:dbPath];
             // If it failed again, there's nothing we can do here.
-            FIRLogError(kFIRLoggerSegmentation, @"I-SEG000010", @"Failed to create table.");
+            FIRLogError(kFIRLoggerSegmentation, @"I-SEG00007", @"Failed to create table.");
+            completionHandler(NO, @{kSEGErrorDescription : @"Failed to re-open new database file"});
           } else {
             // Exclude the app data used from iCloud backup.
             SEGAddSkipBackupAttributeToItemAtPath(dbPath);
+            // Skip reading the db into memory, since it's empty.
+            completionHandler(YES, @{});
           }
         } else {
-          [strongSelf logDatabaseError];
-          completionHandler(NO, @{@"ErrorDescription" : @"Could not create database."});
+          [self logDatabaseError];
+          completionHandler(NO, @{kSEGErrorDescription : @"Could not create database."});
         }
       }
     } else {
-      [strongSelf logDatabaseError];
-      completionHandler(NO, @{@"ErrorDescription" : @"Error creating database."});
+      [self logDatabaseError];
+      completionHandler(NO, @{kSEGErrorDescription : @"Error creating database."});
     }
   });
 }
 
 - (void)removeDatabase:(NSString *)path completion:(SEGRequestCompletion)completionHandler {
   dispatch_async(_databaseOperationQueue, ^{
-    SEGDatabaseManager *strongSelf = self;
-    if (!strongSelf) {
-      return;
-    }
-    [strongSelf removeDatabase:path];
+    [self removeDatabase:path];
     completionHandler(YES, nil);
   });
 }
@@ -193,6 +186,8 @@ - (NSDictionary *)loadMainTable {
 
   sqlite3_stmt *statement = [self prepareSQL:[SQLQuery cStringUsingEncoding:NSUTF8StringEncoding]];
   if (!statement) {
+    FIRLogError(kFIRLoggerSegmentation, @"I-SEG00008",
+                @"Failed to create sqlite statement with query: %@.", SQLQuery);
     return nil;
   }
 
@@ -279,12 +274,11 @@ - (void)insertMainTableApplicationNamed:(NSString *)firebaseApplication
                       associationStatus:(NSString *)associationStatus
                       completionHandler:(SEGRequestCompletion)handler {
   // TODO: delete the row first.
-  __weak SEGDatabaseManager *weakSelf = self;
   dispatch_async(_databaseOperationQueue, ^{
-    NSArray<NSString *> *values =
-        [[NSArray alloc] initWithObjects:firebaseApplication, customInstanceIdentifier,
-                                         firebaseInstanceIdentifier, associationStatus, nil];
-    BOOL success = [weakSelf insertMainTableWithValues:values];
+    NSArray<NSString *> *values = @[
+      firebaseApplication, customInstanceIdentifier, firebaseInstanceIdentifier, associationStatus
+    ];
+    BOOL success = [self insertMainTableWithValues:values];
     if (handler) {
       dispatch_async(dispatch_get_main_queue(), ^{
         handler(success, nil);
@@ -367,7 +361,7 @@ - (BOOL)bindStringsToStatement:(sqlite3_stmt *)statement stringArray:(NSArray *)
   int index = 1;
   for (NSString *param in array) {
     if (![self bindStringToStatement:statement index:index string:param]) {
-      return [self logErrorWithSQL:nil finalizeStatement:statement returnValue:NO];
+      return [self logErrorWithSQL:sql finalizeStatement:statement returnValue:NO];
     }
     index++;
   }
@@ -408,6 +402,15 @@ - (BOOL)logErrorWithSQL:(const char *)SQL
             returnValue:(BOOL)returnValue {
   if (SQL) {
     FIRLogError(kFIRLoggerSegmentation, @"I-SEG000016", @"Failed with SQL: %s.", SQL);
+  } else {
+    const char *sqlString = sqlite3_sql(statement);
+    NSString *sql;
+    if (sqlString != NULL) {
+      sql = [NSString stringWithCString:sqlString encoding:NSUTF8StringEncoding];
+    }
+    if (sql) {
+      FIRLogError(kFIRLoggerSegmentation, @"I-SEG000016", @"Failed with SQL: %s.", SQL);
+    }
   }
   [self logDatabaseError];
 

diff --git a/FirebaseSegmentation/Sources/SEGNetworkManager.m b/FirebaseSegmentation/Sources/SEGNetworkManager.m
@@ -92,7 +92,7 @@ - (void)makeAssociationRequestToBackendWithData:
   NSString *URL = [self constructServerURLWithAssociationData:associationData];
   if (!URL) {
     FIRLogError(kFIRLoggerSegmentation, @"I-SEG000020", @"Could not construct backend URL.");
-    completionHandler(NO, @{@"errorDescription" : @"Could not construct backend URL"});
+    completionHandler(NO, @{kSEGErrorDescription : @"Could not construct backend URL"});
   }
 
   FIRLogDebug(kFIRLoggerSegmentation, @"I-SEG000019", @"%@",
@@ -112,7 +112,7 @@ - (void)makeAssociationRequestToBackendWithData:
     FIRLogError(kFIRLoggerSegmentation, @"I-SEG000021", @"Could not create request data. %@",
                 error.localizedDescription);
     completionHandler(NO,
-                      @{@"errorDescription" : @"Could not serialize JSON data for network call."});
+                      @{kSEGErrorDescription : @"Could not serialize JSON data for network call."});
   }
 
   // Handle NSURLSession completion.
@@ -126,7 +126,7 @@ - (void)makeAssociationRequestToBackendWithData:
                     FIRLogError(kFIRLoggerSegmentation, @"I-SEG000022",
                                 @"Internal error making network request.");
                     completionHandler(
-                        NO, @{@"errorDescription" : @"Internal error making network request."});
+                        NO, @{kSEGErrorDescription : @"Internal error making network request."});
                     return;
                   }
 
@@ -140,7 +140,7 @@ - (void)makeAssociationRequestToBackendWithData:
                                 @"SEGNetworkManager: Network request failed with status code:%lu",
                                 (long)statusCode);
                     completionHandler(NO, @{
-                      @"ErrorDescription" :
+                      kSEGErrorDescription :
                           [NSString stringWithFormat:@"Network Error: %lu", (long)statusCode]
                     });
                   };

diff --git a/FirebaseSegmentation/Sources/SEGSegmentationConstants.h b/FirebaseSegmentation/Sources/SEGSegmentationConstants.h
@@ -16,9 +16,6 @@
 
 #import <Foundation/Foundation.h>
 
-#ifndef SEGSegmentationConstants_h
-#define SEGSegmentationConstants_h
-
 #if defined(DEBUG)
 #define SEG_MUST_NOT_BE_MAIN_THREAD()                                                 \
   do {                                                                                \
@@ -42,11 +39,15 @@ static NSString* const kSEGAssociationStatusPending = @"PENDING";
 static NSString* const kSEGAssociationStatusAssociated = @"ASSOCIATED";
 
 /// Segmentation error domain when logging errors.
-static NSString* const kFirebaseSegmentationErrorDomain = @"com.firebase.segmentation";
+kFirebaseSegmentationErrorDomain = @"com.firebase.segmentation";
+
+/// Segmentation FIRLogger domain.
+static NSString* const kFIRLoggerSegmentation = @"[Firebase/Segmentation]";
+
+/// Used for reporting generic internal Segmentation errors.
+NSString* const kSEGErrorDescription = @"SEGErrorDescription";
 
 /// Segmentation Request Completion callback.
 /// @param success Decide whether the network operation succeeds.
 /// @param result  Return operation result data.
 typedef void (^SEGRequestCompletion)(BOOL success, NSDictionary<NSString*, id>* result);
-
-#endif /* SEGSegmentationConstants_h */