Separate error messages into extensions (#7322)

* Restores ML Pods after M77.

* Fix Package.swift

* Re-add catalyst to GHA workflow.

* Update error handling.

* Add logging for background download failure.

* Remove empty file.

* Slight refactor of error descriptions + using ISO8601DateFormatter where available.

* Slight refactor error descriptions + adding availability for ISO8601DateFormatter.

Author: manjanac

FirebaseMLModelDownloader/Sources/DeviceLogger.swift

@@ -22,6 +22,7 @@ enum LoggerMessageCode {
   case downloadedModelMovedToURL
   case analyticsEventEncodeError
   case telemetryInitError
+  case backgroundDownloadError
 }
 
 /// On-device logger.

FirebaseMLModelDownloader/Sources/LocalModelInfo.swift

@@ -21,9 +21,6 @@ class LocalModelInfo {
   /// Model name.
   let name: String
 
-  /// Download URL for the model file, as returned by server.
-  let downloadURL: URL
-
   /// Hash of the model, as returned by server.
   let modelHash: String
 
@@ -33,9 +30,8 @@ class LocalModelInfo {
   /// Local path of the model.
   let path: String
 
-  init(name: String, downloadURL: URL, modelHash: String, size: Int, path: String) {
+  init(name: String, modelHash: String, size: Int, path: String) {
     self.name = name
-    self.downloadURL = downloadURL
     self.modelHash = modelHash
     self.size = size
     self.path = path
@@ -45,7 +41,6 @@ class LocalModelInfo {
   convenience init(from remoteModelInfo: RemoteModelInfo, path: String) {
     self.init(
       name: remoteModelInfo.name,
-      downloadURL: remoteModelInfo.downloadURL,
       modelHash: remoteModelInfo.modelHash,
       size: remoteModelInfo.size,
       path: path
@@ -55,15 +50,12 @@ class LocalModelInfo {
   /// Convenience init to create local model info from stored info in user defaults.
   convenience init?(fromDefaults defaults: UserDefaults, name: String, appName: String) {
     let defaultsPrefix = LocalModelInfo.getUserDefaultsKeyPrefix(appName: appName, modelName: name)
-    guard let downloadURL = defaults
-      .value(forKey: "\(defaultsPrefix).model-download-url") as? String,
-      let url = URL(string: downloadURL),
-      let modelHash = defaults.value(forKey: "\(defaultsPrefix).model-hash") as? String,
+    guard let modelHash = defaults.value(forKey: "\(defaultsPrefix).model-hash") as? String,
       let size = defaults.value(forKey: "\(defaultsPrefix).model-size") as? Int,
       let path = defaults.value(forKey: "\(defaultsPrefix).model-path") as? String else {
       return nil
     }
-    self.init(name: name, downloadURL: url, modelHash: modelHash, size: size, path: path)
+    self.init(name: name, modelHash: modelHash, size: size, path: path)
   }
 }
 
@@ -78,7 +70,6 @@ extension LocalModelInfo: DownloaderUserDefaultsWriteable {
   /// Write local model info to user defaults.
   func writeToDefaults(_ defaults: UserDefaults, appName: String) {
     let defaultsPrefix = LocalModelInfo.getUserDefaultsKeyPrefix(appName: appName, modelName: name)
-    defaults.setValue(downloadURL.absoluteString, forKey: "\(defaultsPrefix).model-download-url")
     defaults.setValue(modelHash, forKey: "\(defaultsPrefix).model-hash")
     defaults.setValue(size, forKey: "\(defaultsPrefix).model-size")
     defaults.setValue(path, forKey: "\(defaultsPrefix).model-path")

FirebaseMLModelDownloader/Sources/ModelDownloadTask.swift

@@ -89,14 +89,37 @@ extension ModelDownloadTask: URLSessionDownloadDelegate {
     return "fbml_model__\(appName)__\(remoteModelInfo.name)"
   }
 
+  /// Handle client-side errors.
   func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) {
-    // TODO: Handle model download url expiry and other download errors
+    // TODO: Log this.
+    guard task == downloadTask else { return }
+    guard let error = error else { return }
+    /// Unable to resolve hostname or connect to host.
+    DispatchQueue.main.async {
+      self.downloadHandlers
+        .completion(.failure(.internalError(description: error.localizedDescription)))
+    }
   }
 
   func urlSession(_ session: URLSession,
                   downloadTask: URLSessionDownloadTask,
                   didFinishDownloadingTo location: URL) {
-    assert(downloadTask == self.downloadTask)
+    // TODO: Log this.
+    guard downloadTask == self.downloadTask else { return }
+    guard let response = downloadTask.response as? HTTPURLResponse else {
+      DispatchQueue.main.async {
+        self.downloadHandlers
+          .completion(.failure(.internalError(description: ModelDownloadTask
+              .ErrorDescription.invalidServerResponse)))
+      }
+      return
+    }
+
+    guard (200 ..< 299).contains(response.statusCode) else {
+      // TODO: Handle download url expiry + retries.
+      return
+    }
+
     let modelFileURL = ModelFileManager.getDownloadedModelFilePath(
       appName: appName,
       modelName: remoteModelInfo.name
@@ -109,7 +132,7 @@ extension ModelDownloadTask: URLSessionDownloadDelegate {
       DeviceLogger.logEvent(
         level: .info,
         category: .modelDownload,
-        message: "Unable to save downloaded remote model file.",
+        message: ModelDownloadTask.ErrorDescription.saveModel,
         messageCode: .modelDownloaded
       )
       DispatchQueue.main.async {
@@ -122,7 +145,7 @@ extension ModelDownloadTask: URLSessionDownloadDelegate {
       DeviceLogger.logEvent(
         level: .info,
         category: .modelDownload,
-        message: "Unable to save downloaded remote model file.",
+        message: ModelDownloadTask.ErrorDescription.saveModel,
         messageCode: .modelDownloaded
       )
       DispatchQueue.main.async {
@@ -154,7 +177,8 @@ extension ModelDownloadTask: URLSessionDownloadDelegate {
                   didWriteData bytesWritten: Int64,
                   totalBytesWritten: Int64,
                   totalBytesExpectedToWrite: Int64) {
-    assert(downloadTask == self.downloadTask)
+    // TODO: Log this.
+    guard downloadTask == self.downloadTask else { return }
     /// Check if progress handler is unspecified.
     guard let progressHandler = downloadHandlers.progressHandler else { return }
     let calculatedProgress = Float(totalBytesWritten) / Float(totalBytesExpectedToWrite)
@@ -163,3 +187,14 @@ extension ModelDownloadTask: URLSessionDownloadDelegate {
     }
   }
 }
+
+/// Possible error messages for model downloading.
+extension ModelDownloadTask {
+  /// Error descriptions.
+  private enum ErrorDescription {
+    static let invalidServerResponse =
+      "Could not get server response for model downloading."
+    static let saveModel: StaticString =
+      "Unable to save downloaded remote model file."
+  }
+}

FirebaseMLModelDownloader/Sources/ModelDownloader.swift

@@ -78,18 +78,20 @@ public class ModelDownloader {
     telemetryLogger = TelemetryLogger(app: app)
     userDefaults = defaults
 
+    let notificationName = "FIRAppDeleteNotification"
     NotificationCenter.default.addObserver(
       self,
       selector: #selector(deleteModelDownloader),
-      name: Notification.Name("FIRAppDeleteNotification"),
+      name: Notification.Name(notificationName),
       object: nil
     )
   }
 
   /// Handles app deletion notification.
   @objc private func deleteModelDownloader(notification: Notification) {
+    let userInfoKey = "FIRAppNameKey"
     if let userInfo = notification.userInfo,
-      let appName = userInfo["FIRAppNameKey"] as? String {
+      let appName = userInfo[userInfoKey] as? String {
       ModelDownloader.modelDownloaderDictionary.removeValue(forKey: appName)
       // TODO: Clean up user defaults
       // TODO: Clean up local instances of app
@@ -99,7 +101,7 @@ public class ModelDownloader {
   /// Model downloader with default app.
   public static func modelDownloader() -> ModelDownloader {
     guard let defaultApp = FirebaseApp.app() else {
-      fatalError("Default Firebase app not configured.")
+      fatalError(ModelDownloader.ErrorDescription.defaultAppNotConfigured)
     }
     return modelDownloader(app: defaultApp)
   }
@@ -144,9 +146,14 @@ public class ModelDownloader {
             progressHandler: nil,
             completion: { result in
               switch result {
-              // TODO: Log outcome of background download
               case .success: break
-              case .failure: break
+              case .failure:
+                DeviceLogger.logEvent(
+                  level: .info,
+                  category: .modelDownload,
+                  message: ModelDownloader.ErrorDescription.backgroundModelDownload,
+                  messageCode: .backgroundDownloadError
+                )
               }
             }
           )
@@ -176,18 +183,22 @@ public class ModelDownloader {
       var customModels = Set<CustomModel>()
       for path in modelPaths {
         guard let modelName = ModelFileManager.getModelNameFromFilePath(path) else {
-          completion(.failure(.internalError(description: "Invalid model file name.")))
+          completion(.failure(.internalError(description: ModelDownloader
+              .ErrorDescription
+              .parseModelName(path.absoluteString))))
           return
         }
         guard let modelInfo = getLocalModelInfo(modelName: modelName) else {
           completion(
-            .failure(.internalError(description: "Failed to get model info for model file."))
+            .failure(.internalError(description: ModelDownloader.ErrorDescription
+                .retrieveLocalModelInfo))
           )
           return
         }
         guard modelInfo.path == path.absoluteString else {
           completion(
-            .failure(.internalError(description: "Outdated model paths in local storage."))
+            .failure(.internalError(description: ModelDownloader.ErrorDescription
+                .outdatedModelPath))
           )
           return
         }
@@ -206,7 +217,6 @@ public class ModelDownloader {
   public func deleteDownloadedModel(name modelName: String,
                                     completion: @escaping (Result<Void, DownloadedModelError>)
                                       -> Void) {
-    // TODO: Delete previously downloaded model
     guard let localModelInfo = getLocalModelInfo(modelName: modelName),
       let localPath = URL(string: localModelInfo.path)
     else {
@@ -276,16 +286,18 @@ extension ModelDownloader {
             progressHandler: progressHandler,
             completion: completion
           )
+          // TODO: Is it possible for download URL to expire here?
           downloadTask.resumeModelDownload()
         /// Local model info is the latest model info.
         case .notModified:
           guard let localModel = self.getLocalModel(modelName: modelName) else {
             DispatchQueue.main.async {
               /// This can only happen if either local model info or the model file was suddenly wiped out in the middle of model info request and server response
-              // TODO: Consider handling: if model file is deleted after local model info is retrieved but before model info network call
+              // TODO: Consider handling: If model file is deleted after local model info is retrieved but before model info network call.
               completion(
                 .failure(
-                  .internalError(description: "Model unavailable due to deleted local model info.")
+                  .internalError(description: ModelDownloader.ErrorDescription
+                    .deletedLocalModelInfo)
                 )
               )
             }
@@ -320,3 +332,23 @@ extension ModelDownloader {
     }
   }
 }
+
+/// Possible error messages while using model downloader.
+extension ModelDownloader {
+  /// Error descriptions.
+  private enum ErrorDescription {
+    static let defaultAppNotConfigured =
+      "Default Firebase app not configured."
+    static let parseModelName = { (path: String) in
+      "Unable to parse model file name at \(path)."
+    }
+
+    static let retrieveLocalModelInfo =
+      "Failed to get stored model info for model file."
+    static let outdatedModelPath = "Outdated model paths in local storage."
+    static let deletedLocalModelInfo =
+      "Model unavailable due to deleted local model info."
+    static let backgroundModelDownload: StaticString =
+      "Failed to update model in background."
+  }
+}

FirebaseMLModelDownloader/Sources/ModelFileManager.swift

@@ -68,15 +68,17 @@ enum ModelFileManager {
       } catch {
         throw DownloadError
           .internalError(
-            description: "Could not replace existing model file - \(error.localizedDescription)"
+            description: ModelFileManager.ErrorDescription
+              .replaceFile(error.localizedDescription)
           )
       }
     }
     do {
       try FileManager.default.moveItem(at: sourceURL, to: destinationURL)
     } catch {
       throw DownloadError
-        .internalError(description: "Unable to save model file - \(error.localizedDescription)")
+        .internalError(description: ModelFileManager
+          .ErrorDescription.saveFile(error.localizedDescription))
     }
   }
 
@@ -87,7 +89,7 @@ enum ModelFileManager {
     } catch {
       throw DownloadedModelError
         .internalError(
-          description: "Could not delete old model file - \(error.localizedDescription)"
+          description: ModelFileManager.ErrorDescription.deleteFile(error.localizedDescription)
         )
     }
   }
@@ -105,8 +107,31 @@ enum ModelFileManager {
     } catch {
       throw DownloadedModelError
         .internalError(
-          description: "Could not retrieve model files in directory - \(error.localizedDescription)"
+          description: ModelFileManager.ErrorDescription
+            .retrieveFile(error.localizedDescription)
         )
     }
   }
 }
+
+/// Possible error messages during file management.
+extension ModelFileManager {
+  /// Error descriptions.
+  private enum ErrorDescription {
+    static let retrieveFile = { (error: String) in
+      "Could not retrieve model files in directory: \(error)"
+    }
+
+    static let deleteFile = { (error: String) in
+      "Could not delete old model file: \(error)"
+    }
+
+    static let saveFile = { (error: String) in
+      "Unable to save model file: \(error)"
+    }
+
+    static let replaceFile = { (error: String) in
+      "Could not replace existing model file: \(error)"
+    }
+  }
+}