Enhance the log storage class (#2275)

* Rename fields related to the previous notion of 'backends' to 'uploaders'.

Also changes the declaration of the uploader callback block.

* Add the ability to delete a set of logs by filename, and a conversion method for hashes to files.

* Change the log storage removeLogs method to be log hashes instead of URLS

* Style, and change the completion block to include an error

* Change to sync, since async to adding more async created race condition opportunity.

* Test new storage methods

* Fix coordinator method declarations

Author: mikehaney24

GoogleDataLogger/GoogleDataLogger/Classes/GDLLogStorage.h

@@ -45,6 +45,20 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)removeLog:(NSNumber *)logHash logTarget:(NSNumber *)logTarget;
 
+/** Removes a set of log fields specified by their filenames.
+ *
+ * @param logHashes The set of log files to remove.
+ * @param logTarget The log target the log files correspond to.
+ */
+- (void)removeLogs:(NSSet<NSNumber *> *)logHashes logTarget:(NSNumber *)logTarget;
+
+/** Converts a set of log hashes to a set of log files.
+ *
+ * @param logHashes A set of log hashes to get the files of.
+ * @return A set of equivalent length, containing all the filenames corresponding to the hashes.
+ */
+- (NSSet<NSURL *> *)logHashesToFiles:(NSSet<NSNumber *> *)logHashes;
+
 @end
 
 NS_ASSUME_NONNULL_END

GoogleDataLogger/GoogleDataLogger/Classes/GDLLogStorage.m

@@ -56,7 +56,7 @@ - (instancetype)init {
   if (self) {
     _storageQueue = dispatch_queue_create("com.google.GDLLogStorage", DISPATCH_QUEUE_SERIAL);
     _logHashToLogFile = [[NSMutableDictionary alloc] init];
-    _logTargetToLogFileSet = [[NSMutableDictionary alloc] init];
+    _logTargetToLogHashSet = [[NSMutableDictionary alloc] init];
     _uploader = [GDLUploadCoordinator sharedInstance];
   }
   return self;
@@ -90,7 +90,7 @@ - (void)storeLog:(GDLLogEvent *)log {
 
     // Check the QoS, if it's high priority, notify the log target that it has a high priority log.
     if (shortLivedLog.qosTier == GDLLogQoSFast) {
-      NSSet<NSURL *> *allLogsForLogTarget = self.logTargetToLogFileSet[@(logTarget)];
+      NSSet<NSNumber *> *allLogsForLogTarget = self.logTargetToLogHashSet[@(logTarget)];
       [self.uploader forceUploadLogs:allLogsForLogTarget target:logTarget];
     }
 
@@ -117,13 +117,33 @@ - (void)removeLog:(NSNumber *)logHash logTarget:(NSNumber *)logTarget {
 
     // Remove from the tracking collections.
     [self.logHashToLogFile removeObjectForKey:logHash];
-    NSMutableSet<NSURL *> *logFiles = self.logTargetToLogFileSet[logTarget];
-    GDLAssert(logFiles, @"There wasn't a logSet for this logTarget.");
-    [logFiles removeObject:logFile];
+    NSMutableSet<NSNumber *> *logHashes = self.logTargetToLogHashSet[logTarget];
+    GDLAssert(logHashes, @"There wasn't a logSet for this logTarget.");
+    [logHashes removeObject:logHash];
     // It's fine to not remove the set if it's empty.
   });
 }
 
+- (void)removeLogs:(NSSet<NSNumber *> *)logHashes logTarget:(NSNumber *)logTarget {
+  dispatch_sync(_storageQueue, ^{
+    for (NSNumber *logHash in logHashes) {
+      [self removeLog:logHash logTarget:logTarget];
+    }
+  });
+}
+
+- (NSSet<NSURL *> *)logHashesToFiles:(NSSet<NSNumber *> *)logHashes {
+  NSMutableSet<NSURL *> *logFiles = [[NSMutableSet alloc] init];
+  dispatch_sync(_storageQueue, ^{
+    for (NSNumber *hashNumber in logHashes) {
+      NSURL *logURL = self.logHashToLogFile[hashNumber];
+      GDLAssert(logURL, @"A log file URL couldn't be found for the given hash");
+      [logFiles addObject:logURL];
+    }
+  });
+  return logFiles;
+}
+
 #pragma mark - Private helper methods
 
 /** Creates the log directory if it does not exist. */
@@ -170,13 +190,15 @@ - (NSURL *)saveLogProtoToDisk:(NSData *)logProtoBytes logHash:(NSUInteger)logHas
  */
 - (void)addLogToTrackingCollections:(GDLLogEvent *)log logFile:(NSURL *)logFile {
   NSInteger logTarget = log.logTarget;
-  self.logHashToLogFile[@(log.hash)] = logFile;
-  NSMutableSet<NSURL *> *logs = self.logTargetToLogFileSet[@(logTarget)];
+  NSNumber *logHash = @(log.hash);
+  NSNumber *logTargetNumber = @(logTarget);
+  self.logHashToLogFile[logHash] = logFile;
+  NSMutableSet<NSNumber *> *logs = self.logTargetToLogHashSet[logTargetNumber];
   if (logs) {
-    [logs addObject:logFile];
+    [logs addObject:logHash];
   } else {
-    NSMutableSet<NSURL *> *logSet = [NSMutableSet setWithObject:logFile];
-    self.logTargetToLogFileSet[@(logTarget)] = logSet;
+    NSMutableSet<NSNumber *> *logSet = [NSMutableSet setWithObject:logHash];
+    self.logTargetToLogHashSet[logTargetNumber] = logSet;
   }
 }
 
@@ -185,8 +207,8 @@ - (void)addLogToTrackingCollections:(GDLLogEvent *)log logFile:(NSURL *)logFile
 /** The NSKeyedCoder key for the logHashToFile property. */
 static NSString *const kGDLLogHashToLogFileKey = @"logHashToLogFileKey";
 
-/** The NSKeyedCoder key for the logTargetToLogFileSet property. */
-static NSString *const kGDLLogTargetToLogSetKey = @"logTargetToLogFileSetKey";
+/** The NSKeyedCoder key for the logTargetToLogHashSet property. */
+static NSString *const kGDLLogTargetToLogHashSetKey = @"logTargetToLogHashSetKey";
 
 + (BOOL)supportsSecureCoding {
   return YES;
@@ -199,8 +221,8 @@ - (instancetype)initWithCoder:(NSCoder *)aDecoder {
     Class NSMutableDictionaryClass = [NSMutableDictionary class];
     sharedInstance->_logHashToLogFile = [aDecoder decodeObjectOfClass:NSMutableDictionaryClass
                                                                forKey:kGDLLogHashToLogFileKey];
-    sharedInstance->_logTargetToLogFileSet =
-        [aDecoder decodeObjectOfClass:NSMutableDictionaryClass forKey:kGDLLogTargetToLogSetKey];
+    sharedInstance->_logTargetToLogHashSet =
+        [aDecoder decodeObjectOfClass:NSMutableDictionaryClass forKey:kGDLLogTargetToLogHashSetKey];
   });
   return sharedInstance;
 }
@@ -209,7 +231,8 @@ - (void)encodeWithCoder:(NSCoder *)aCoder {
   GDLLogStorage *sharedInstance = [self.class sharedInstance];
   dispatch_sync(sharedInstance.storageQueue, ^{
     [aCoder encodeObject:sharedInstance->_logHashToLogFile forKey:kGDLLogHashToLogFileKey];
-    [aCoder encodeObject:sharedInstance->_logTargetToLogFileSet forKey:kGDLLogTargetToLogSetKey];
+    [aCoder encodeObject:sharedInstance->_logTargetToLogHashSet
+                  forKey:kGDLLogTargetToLogHashSetKey];
   });
 }
 

GoogleDataLogger/GoogleDataLogger/Classes/GDLRegistrar.m

@@ -33,13 +33,13 @@ - (instancetype)init {
   self = [super init];
   if (self) {
     _logTargetToPrioritizer = [[NSMutableDictionary alloc] init];
-    _logTargetToBackend = [[NSMutableDictionary alloc] init];
+    _logTargetToUploader = [[NSMutableDictionary alloc] init];
   }
   return self;
 }
 
 - (void)registerBackend:(id<GDLLogUploader>)backend forLogTarget:(GDLLogTarget)logTarget {
-  self.logTargetToBackend[@(logTarget)] = backend;
+  self.logTargetToUploader[@(logTarget)] = backend;
 }
 
 - (void)registerLogPrioritizer:(id<GDLLogPrioritizer>)prioritizer

GoogleDataLogger/GoogleDataLogger/Classes/GDLUploadCoordinator.h

@@ -33,8 +33,11 @@ NS_ASSUME_NONNULL_BEGIN
 
 /** Forces the backend specified by the target to upload the provided set of logs. This should only
  * ever happen when the QoS tier of a log requires it.
+ *
+ * @param logHashes The set of log hashes to force upload.
+ * @param logTarget The log target that should force an upload.
  */
-- (void)forceUploadLogs:(NSSet<NSURL *> *)logFiles target:(GDLLogTarget)logTarget;
+- (void)forceUploadLogs:(NSSet<NSNumber *> *)logHashes target:(GDLLogTarget)logTarget;
 
 @end
 

GoogleDataLogger/GoogleDataLogger/Classes/GDLUploadCoordinator.m

@@ -29,7 +29,7 @@ + (instancetype)sharedInstance {
   return sharedUploader;
 }
 
-- (void)forceUploadLogs:(NSSet<NSURL *> *)logFiles target:(GDLLogTarget)logTarget {
+- (void)forceUploadLogs:(NSSet<NSNumber *> *)logHashes target:(GDLLogTarget)logTarget {
   // Ask the prioritizer of the target for some logs
 }
 

GoogleDataLogger/GoogleDataLogger/Classes/Private/GDLLogStorage_Private.h

@@ -28,7 +28,7 @@
 
 /** A map of logTargets to a set of log hash values. */
 @property(nonatomic)
-    NSMutableDictionary<NSNumber *, NSMutableSet<NSURL *> *> *logTargetToLogFileSet;
+    NSMutableDictionary<NSNumber *, NSMutableSet<NSNumber *> *> *logTargetToLogHashSet;
 
 /** The log uploader instance to use. */
 @property(nonatomic) GDLUploadCoordinator *uploader;

GoogleDataLogger/GoogleDataLogger/Classes/Private/GDLRegistrar_Private.h

@@ -19,7 +19,7 @@
 @interface GDLRegistrar ()
 
 /** A map of logTargets to backend implementations. */
-@property(nonatomic) NSMutableDictionary<NSNumber *, id<GDLLogUploader>> *logTargetToBackend;
+@property(nonatomic) NSMutableDictionary<NSNumber *, id<GDLLogUploader>> *logTargetToUploader;
 
 /** A map of logTargets to prioritizer implementations. */
 @property(nonatomic) NSMutableDictionary<NSNumber *, id<GDLLogPrioritizer>> *logTargetToPrioritizer;

GoogleDataLogger/GoogleDataLogger/Classes/Public/GDLLogTargets.h

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2019 Google
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#import <Foundation/Foundation.h>
+
+/** The list of targets supported by the shared logging infrastructure. If adding a new target,
+ * please use the previous value +1, and increment GDLLogTargetLast by 1.
+ */
+typedef NS_ENUM(NSInteger, GDLLogTarget) {
+
+  /** The CCT log target. */
+  kGDLLogTargetCCT = 1000,
+
+  GDLLogTargetLast = 1001
+};

GoogleDataLogger/GoogleDataLogger/Classes/Public/GDLLogUploader.h

@@ -16,13 +16,21 @@
 
 #import <Foundation/Foundation.h>
 
+#import <GoogleDataLogger/GDLClock.h>
+#import <GoogleDataLogger/GDLLogTargets.h>
+
 NS_ASSUME_NONNULL_BEGIN
 
 /** A convenient typedef to define the block to be called upon completion of an upload to the
  * backend.
+ *
+ * target: The log target that was uploading.
+ * nextUploadAttemptUTC: The desired next upload attempt time.
+ * uploadError: Populated with any upload error. If non-nil, a retry will be attempted.
  */
-typedef void (^GDLBackendCompletionBlock)(NSSet<NSURL *> *_Nullable successfulUploads,
-                                          NSSet<NSURL *> *_Nullable unsuccessfulUploads);
+typedef void (^GDLUploaderCompletionBlock)(GDLLogTarget target,
+                                           GDLClock *nextUploadAttemptUTC,
+                                           NSError *_Nullable uploadError);
 
 /** This protocol defines the common interface for logging backend implementations. */
 @protocol GDLLogUploader <NSObject>
@@ -36,7 +44,7 @@ typedef void (^GDLBackendCompletionBlock)(NSSet<NSURL *> *_Nullable successfulUp
  *   - successfulUploads: The set of filenames uploaded successfully.
  *   - unsuccessfulUploads: The set of filenames not uploaded successfully.
  */
-- (void)uploadLogs:(NSSet<NSURL *> *)logFiles onComplete:(GDLBackendCompletionBlock)onComplete;
+- (void)uploadLogs:(NSSet<NSURL *> *)logFiles onComplete:(GDLUploaderCompletionBlock)onComplete;
 
 @end
 

GoogleDataLogger/GoogleDataLogger/Classes/Public/GDLRegistrar.h

@@ -17,17 +17,11 @@
 #import <Foundation/Foundation.h>
 
 #import <GoogleDataLogger/GDLLogPrioritizer.h>
+#import <GoogleDataLogger/GDLLogTargets.h>
 #import <GoogleDataLogger/GDLLogUploader.h>
 
 NS_ASSUME_NONNULL_BEGIN
 
-/** The list of targets supported by the shared logging infrastructure. */
-typedef NS_ENUM(NSInteger, GDLLogTarget) {
-
-  /** The CCT log target. */
-  kGDLLogTargetCCT = 1000
-};
-
 /** Manages the registration of log targets with the logging SDK. */
 @interface GDLRegistrar : NSObject
 

GoogleDataLogger/Tests/Unit/Categories/GDLLogStorage+Testing.m

@@ -20,7 +20,7 @@ @implementation GDLLogStorage (Testing)
 
 - (void)reset {
   dispatch_sync(self.storageQueue, ^{
-    [self.logTargetToLogFileSet removeAllObjects];
+    [self.logTargetToLogHashSet removeAllObjects];
     [self.logHashToLogFile removeAllObjects];
   });
 }

GoogleDataLogger/Tests/Unit/Categories/GDLRegistrar+Testing.m

@@ -22,7 +22,7 @@ @implementation GDLRegistrar (Testing)
 
 - (void)reset {
   [self.logTargetToPrioritizer removeAllObjects];
-  [self.logTargetToBackend removeAllObjects];
+  [self.logTargetToUploader removeAllObjects];
 }
 
 @end