More on-device logging and better error handling. (#7397)

manjanac · web-flow · commit c64ca064315d · 2021-01-28T13:11:31.000-08:00
* Restores ML Pods after M77.

* Fix Package.swift

* Re-add catalyst to GHA workflow.

* Add new error type for expired url + fix tests.

* On-device logging + updates to tests.

* Add new error type for expired url + fix tests.

* On-device logging + updates to tests.

* Revert to enum for ModelFileManager.swift

* Remove GoogleService-Info.plist

* Remove GoogleService-Info.plist

* Improve errors, tests, on-device logging.

* Add device compatibility to check for available space.

* Use CocoaError to check for space constraints.
diff --git a/FirebaseMLModelDownloader/Sources/DeviceLogger.swift b/FirebaseMLModelDownloader/Sources/DeviceLogger.swift
@@ -40,6 +40,9 @@ enum LoggerMessageCode: Int {
   case invalidOptions
   case retryDownload
   case anotherDownloadInProgressError
+  case invalidModelName
+  case permissionDenied
+  case notEnoughSpace
   case validModelDownloadResponse
   case hostnameError
   case invalidDownloadSessionError
diff --git a/FirebaseMLModelDownloader/Sources/ModelDownloadConditions.swift b/FirebaseMLModelDownloader/Sources/ModelDownloadConditions.swift
@@ -18,6 +18,6 @@ import Foundation
 public struct ModelDownloadConditions {
   /// Allow model downloading on a cellular connection. Default is `YES`.
   public var allowsCellularAccess = true
-  /// Public init
+  /// Public init.
   public init() {}
 }
diff --git a/FirebaseMLModelDownloader/Sources/ModelDownloadTask.swift b/FirebaseMLModelDownloader/Sources/ModelDownloadTask.swift
@@ -126,16 +126,23 @@ extension ModelDownloadTask {
   /// Handle model download response.
   func handleResponse(response: HTTPURLResponse, tempURL: URL, completion: @escaping Completion) {
     guard (200 ..< 299).contains(response.statusCode) else {
+      switch response.statusCode {
       /// Possible failure due to download URL expiry.
-      if response.statusCode == 400 {
+      case 400:
+        let currentDateTime = Date()
+        /// Check if download url has expired.
+        guard currentDateTime > remoteModelInfo.urlExpiryTime else {
+          completion(.failure(.invalidArgument))
+          return
+        }
         completion(.failure(.expiredDownloadURL))
-        return
+      case 401, 403: completion(.failure(.permissionDenied))
+      case 404: completion(.failure(.notFound))
+      default:
+        let description = ModelDownloadTask.ErrorDescription
+          .modelDownloadFailed(response.statusCode)
+        completion(.failure(.internalError(description: description)))
       }
-      let description = ModelDownloadTask.ErrorDescription.modelDownloadFailed(response.statusCode)
-      DeviceLogger.logEvent(level: .debug,
-                            message: description,
-                            messageCode: .modelDownloadError)
-      completion(.failure(.internalError(description: description)))
       return
     }
 
@@ -145,7 +152,11 @@ extension ModelDownloadTask {
     )
 
     do {
-      try ModelFileManager.moveFile(at: tempURL, to: modelFileURL)
+      try ModelFileManager.moveFile(
+        at: tempURL,
+        to: modelFileURL,
+        size: Int64(remoteModelInfo.size)
+      )
       DeviceLogger.logEvent(level: .debug,
                             message: ModelDownloadTask.DebugDescription.savedModelFile,
                             messageCode: .downloadedModelFileSaved)
@@ -168,9 +179,15 @@ extension ModelDownloadTask {
     } catch let error as DownloadError {
       downloadStatus = .failed
       telemetryLogger?.logModelDownloadEvent(eventName: .modelDownload, status: downloadStatus)
-      DeviceLogger.logEvent(level: .debug,
-                            message: ModelDownloadTask.ErrorDescription.saveModel,
-                            messageCode: .downloadedModelSaveError)
+      if error == .notEnoughSpace {
+        DeviceLogger.logEvent(level: .debug,
+                              message: ModelDownloadTask.ErrorDescription.notEnoughSpace,
+                              messageCode: .notEnoughSpace)
+      } else {
+        DeviceLogger.logEvent(level: .debug,
+                              message: ModelDownloadTask.ErrorDescription.saveModel,
+                              messageCode: .downloadedModelSaveError)
+      }
       completion(.failure(error))
       return
     } catch {
@@ -209,6 +226,7 @@ extension ModelDownloadTask {
       "Could not get valid server response for model downloading."
     static let unknownDownloadError = "Unable to download model due to unknown error."
     static let saveModel = "Unable to save downloaded remote model file."
+    static let notEnoughSpace = "Not enough space on device."
     static let expiredModelInfo = "Unable to update expired model info."
     static let anotherDownloadInProgress = "Download already in progress."
   }
diff --git a/FirebaseMLModelDownloader/Sources/ModelDownloader.swift b/FirebaseMLModelDownloader/Sources/ModelDownloader.swift
@@ -28,8 +28,6 @@ public enum DownloadError: Error, Equatable {
   case notEnoughSpace
   /// Malformed model name.
   case invalidArgument
-  /// Expired download URL.
-  case expiredDownloadURL
   /// Other errors with description.
   case internalError(description: String)
 }
@@ -44,6 +42,13 @@ public enum DownloadedModelError: Error {
   case internalError(description: String)
 }
 
+/// Extension to handle internally meaningful errors.
+extension DownloadError {
+  static let expiredDownloadURL: DownloadError = {
+    DownloadError.internalError(description: "Expired model download URL.")
+  }()
+}
+
 /// Possible ways to get a custom model.
 public enum ModelDownloadType {
   /// Get local model stored on device.
@@ -380,15 +385,25 @@ extension ModelDownloader {
                                       .modelDownloadFailed(error),
                                     messageCode: .modelDownloadError)
               switch error {
+              case .notFound:
+                DeviceLogger.logEvent(level: .debug,
+                                      message: ModelDownloader.ErrorDescription
+                                        .modelNotFound(modelName),
+                                      messageCode: .modelNotFound)
+                self.mainQueueHandler(completion(.failure(.notFound)))
+              case .invalidArgument:
+                DeviceLogger.logEvent(level: .debug,
+                                      message: ModelDownloader.ErrorDescription
+                                        .invalidModelName(modelName),
+                                      messageCode: .invalidModelName)
+                self.mainQueueHandler(completion(.failure(.invalidArgument)))
+              case .permissionDenied:
+                DeviceLogger.logEvent(level: .debug,
+                                      message: ModelDownloader.ErrorDescription.permissionDenied,
+                                      messageCode: .permissionDenied)
+                self.mainQueueHandler(completion(.failure(.permissionDenied)))
               /// This is the error returned when URL expired.
-              // TODO: Should we use a different error here?
               case .expiredDownloadURL:
-                let currentDateTime = Date()
-                /// Check if download url has expired.
-                guard currentDateTime > remoteModelInfo.urlExpiryTime else {
-                  self.mainQueueHandler(completion(.failure(error)))
-                  return
-                }
                 /// Check if retries are allowed.
                 guard self.numberOfRetries > 0 else {
                   self
@@ -429,8 +444,8 @@ extension ModelDownloader {
 
           self.mainQueueHandler(completion(.success(localModel)))
         }
-      case let .failure(downloadError):
-        self.mainQueueHandler(completion(.failure(downloadError)))
+      case let .failure(error):
+        self.mainQueueHandler(completion(.failure(error)))
       }
     }
   }
@@ -490,10 +505,15 @@ extension ModelDownloader {
       "No model found with name: \(name)"
     }
 
+    static let invalidModelName = { (name: String) in
+      "Invalid model name: \(name)"
+    }
+
     static let modelDownloadFailed = { (error: Error) in
       "Model download failed with error: \(error)"
     }
 
+    static let permissionDenied = "Invalid or missing permissions to download model."
     static let outdatedModelPath = "Outdated model paths in local storage."
     static let deletedLocalModelInfo =
       "Model unavailable due to deleted local model info."
diff --git a/FirebaseMLModelDownloader/Sources/ModelFileManager.swift b/FirebaseMLModelDownloader/Sources/ModelFileManager.swift
@@ -15,7 +15,8 @@
 import Foundation
 
 /// Manager for common file operations.
-class ModelFileManager: FileManager {
+// TODO: Consider mocking this for tests?
+enum ModelFileManager {
   private static let nameSeparator = "__"
   private static let modelNamePrefix = "fbml_model"
   private static let fileManager = FileManager.default
@@ -61,7 +62,7 @@ class ModelFileManager: FileManager {
   }
 
   /// Move file at a location to another location.
-  static func moveFile(at sourceURL: URL, to destinationURL: URL) throws {
+  static func moveFile(at sourceURL: URL, to destinationURL: URL, size: Int64) throws {
     if isFileReachable(at: destinationURL) {
       do {
         try fileManager.removeItem(at: destinationURL)
@@ -75,7 +76,9 @@ class ModelFileManager: FileManager {
       }
     }
     do {
-      try FileManager.default.moveItem(at: sourceURL, to: destinationURL)
+      try fileManager.moveItem(at: sourceURL, to: destinationURL)
+    } catch CocoaError.fileWriteOutOfSpace {
+      throw DownloadError.notEnoughSpace
     } catch {
       throw DownloadError
         .internalError(description: ModelFileManager
@@ -134,5 +137,13 @@ extension ModelFileManager {
     static let replaceFile = { (error: String) in
       "Could not replace existing model file: \(error)"
     }
+
+    static let availableStorage = { (error: String?) -> String in
+      if let error = error {
+        return "Failed to check storage capacity on device: \(error)"
+      } else {
+        return "Failed to check storage capacity on device."
+      }
+    }
   }
 }
diff --git a/FirebaseMLModelDownloader/Sources/ModelInfoRetriever.swift b/FirebaseMLModelDownloader/Sources/ModelInfoRetriever.swift
@@ -186,6 +186,10 @@ class ModelInfoRetriever {
                 return
               }
               completion(.success(.notModified))
+            case 400:
+              completion(.failure(.invalidArgument))
+            case 401, 403:
+              completion(.failure(.permissionDenied))
             case 404:
               completion(.failure(.notFound))
             // TODO: Handle more http status codes
diff --git a/FirebaseMLModelDownloader/Sources/TelemetryLogger.swift b/FirebaseMLModelDownloader/Sources/TelemetryLogger.swift
@@ -23,7 +23,7 @@ extension SystemInfo {
     // TODO: Reconsider using app version.
     let appVersionKey = "CFBundleShortVersionString"
     appVersion = Bundle.main.infoDictionary?[appVersionKey] as? String ?? "unknownAppVersion"
-    // TODO: May also need to log SDK version.
+    // TODO: Need to log SDK version.
     self.apiKey = apiKey ?? "unknownAPIKey"
     firebaseProjectID = projectID ?? "unknownProjectID"
   }
diff --git a/FirebaseMLModelDownloader/Tests/Unit/ModelDownloaderUnitTests.swift b/FirebaseMLModelDownloader/Tests/Unit/ModelDownloaderUnitTests.swift

Original file line number	Diff line number	Diff line change
`@@ -18,6 +18,6 @@ import Foundation`
`18`	`18`	`public struct ModelDownloadConditions {`
`19`	`19`	/// Allow model downloading on a cellular connection. Default is `YES`.
`20`	`20`	`public var allowsCellularAccess = true`
`21`		`- /// Public init`
	`21`	`+ /// Public init.`
`22`	`22`	`public init() {}`
`23`	`23`	`}`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ extension SystemInfo {`
`23`	`23`	`// TODO: Reconsider using app version.`
`24`	`24`	`let appVersionKey = "CFBundleShortVersionString"`
`25`	`25`	`appVersion = Bundle.main.infoDictionary?[appVersionKey] as? String ?? "unknownAppVersion"`
`26`		`- // TODO: May also need to log SDK version.`
	`26`	`+ // TODO: Need to log SDK version.`
`27`	`27`	`self.apiKey = apiKey ?? "unknownAPIKey"`
`28`	`28`	`firebaseProjectID = projectID ?? "unknownProjectID"`
`29`	`29`	`}`