Delete custom models from device (#2236)

* Reviewer suggested changes.

* Add code for custom model deletion from a device.

* Add code for custom model deletion from a device.

* Update getFile so it checks if the file exists before returning.

* Adding logging todo.

* Adding logging todo.

Author: annzimmer

firebase-ml-modeldownloader/api.txt

@@ -39,13 +39,3 @@ package com.google.firebase.ml.modeldownloader {
 
 }
 
-package com.google.firebase.ml.modeldownloader.internal {
-
-  public class ModelFileManager {
-    ctor public ModelFileManager(@NonNull com.google.firebase.FirebaseApp);
-    method @NonNull public static com.google.firebase.ml.modeldownloader.internal.ModelFileManager getInstance();
-    method @Nullable @WorkerThread public java.io.File moveModelToDestinationFolder(@NonNull com.google.firebase.ml.modeldownloader.CustomModel, @NonNull android.os.ParcelFileDescriptor) throws java.lang.Exception;
-  }
-
-}
-

firebase-ml-modeldownloader/src/main/java/com/google/firebase/ml/modeldownloader/CustomModel.java

@@ -152,7 +152,12 @@ File getFile(ModelFileDownloadService fileDownloadService) throws Exception {
     if (localFilePath == null || localFilePath.isEmpty()) {
       return null;
     }
-    return new File(localFilePath);
+    File modelFile = new File(localFilePath);
+
+    if (!modelFile.exists()) {
+      return null;
+    }
+    return modelFile;
   }
 
   /**

firebase-ml-modeldownloader/src/main/java/com/google/firebase/ml/modeldownloader/FirebaseModelDownloader.java

@@ -27,6 +27,7 @@
 import com.google.firebase.installations.FirebaseInstallationsApi;
 import com.google.firebase.ml.modeldownloader.internal.CustomModelDownloadService;
 import com.google.firebase.ml.modeldownloader.internal.ModelFileDownloadService;
+import com.google.firebase.ml.modeldownloader.internal.ModelFileManager;
 import com.google.firebase.ml.modeldownloader.internal.SharedPreferencesUtil;
 import java.util.Set;
 import java.util.concurrent.Executor;
@@ -37,6 +38,7 @@ public class FirebaseModelDownloader {
   private final FirebaseOptions firebaseOptions;
   private final SharedPreferencesUtil sharedPreferencesUtil;
   private final ModelFileDownloadService fileDownloadService;
+  private final ModelFileManager fileManager;
   private final CustomModelDownloadService modelDownloadService;
   private final Executor executor;
 
@@ -48,7 +50,8 @@ public class FirebaseModelDownloader {
     this.sharedPreferencesUtil = new SharedPreferencesUtil(firebaseApp);
     this.modelDownloadService =
         new CustomModelDownloadService(firebaseOptions, firebaseInstallationsApi);
-    this.executor = Executors.newCachedThreadPool();
+    this.executor = Executors.newSingleThreadExecutor();
+    fileManager = ModelFileManager.getInstance();
   }
 
   @VisibleForTesting
@@ -57,11 +60,13 @@ public class FirebaseModelDownloader {
       SharedPreferencesUtil sharedPreferencesUtil,
       ModelFileDownloadService fileDownloadService,
       CustomModelDownloadService modelDownloadService,
+      ModelFileManager fileManager,
       Executor executor) {
     this.firebaseOptions = firebaseOptions;
     this.sharedPreferencesUtil = sharedPreferencesUtil;
     this.fileDownloadService = fileDownloadService;
     this.modelDownloadService = modelDownloadService;
+    this.fileManager = fileManager;
     this.executor = executor;
   }
 
@@ -187,7 +192,16 @@ public Task<Set<CustomModel>> listDownloadedModels() {
    */
   @NonNull
   public Task<Void> deleteDownloadedModel(@NonNull String modelName) {
-    throw new UnsupportedOperationException("Not yet implemented.");
+
+    TaskCompletionSource<Void> taskCompletionSource = new TaskCompletionSource<>();
+    executor.execute(
+        () -> {
+          // remove all files associated with this model and then clean up model references.
+          fileManager.deleteAllModels(modelName);
+          sharedPreferencesUtil.clearModelDetails(modelName);
+          taskCompletionSource.setResult(null);
+        });
+    return taskCompletionSource.getTask();
   }
 
   /** Returns the nick name of the {@link FirebaseApp} of this {@link FirebaseModelDownloader} */

firebase-ml-modeldownloader/src/main/java/com/google/firebase/ml/modeldownloader/internal/ModelFileManager.java

@@ -18,20 +18,28 @@
 import android.os.Build.VERSION_CODES;
 import android.os.ParcelFileDescriptor;
 import android.os.ParcelFileDescriptor.AutoCloseInputStream;
+import android.util.Log;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 import androidx.annotation.WorkerThread;
+import com.google.android.gms.common.internal.Preconditions;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.ml.modeldownloader.CustomModel;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 
-/** Model File Manager is used to move the downloaded file to the appropriate locations. */
+/**
+ * Model File Manager is used to move the downloaded file to the appropriate locations.
+ *
+ * @hide
+ */
 public class ModelFileManager {
 
+  private static final String TAG = "FirebaseModelFileManage";
+
   @VisibleForTesting
   static final String CUSTOM_MODEL_ROOT_PATH = "com.google.firebase.ml.custom.models";
 
@@ -108,7 +116,7 @@ private int getLatestCachedModelVersion(@NonNull File modelDir) {
       try {
         index = Math.max(index, Integer.parseInt(modelFile.getName()));
       } catch (NumberFormatException e) {
-        System.out.println("Contains non-integer file name " + modelFile.getName());
+        Log.d(TAG, String.format("Contains non-integer file name %s", modelFile.getName()));
       }
     }
     return index;
@@ -118,6 +126,19 @@ private int getLatestCachedModelVersion(@NonNull File modelDir) {
   @Nullable
   File getModelFileDestination(@NonNull CustomModel model) throws Exception {
     File destFolder = getDirImpl(model.getName());
+    if (!destFolder.exists()) {
+      Log.d(TAG, "model folder does not exist, creating one: " + destFolder.getAbsolutePath());
+      if (!destFolder.mkdirs()) {
+        // TODO(annzimmer) change to FirebaseMLException when logging complete. Add test at same
+        // time.
+        throw new Exception("Failed to create model folder: " + destFolder);
+      }
+    } else if (!destFolder.isDirectory()) {
+      // TODO(annzimmer) change to FirebaseMLException when logging complete. Add test at same time.
+      throw new Exception(
+          "Can not create model folder, since an existing file has the same name: " + destFolder);
+    }
+
     int index = getLatestCachedModelVersion(destFolder);
     return new File(destFolder, String.valueOf(index + 1));
   }
@@ -141,6 +162,11 @@ public synchronized File moveModelToDestinationFolder(
       @NonNull CustomModel customModel, @NonNull ParcelFileDescriptor modelFileDescriptor)
       throws Exception {
     File modelFileDestination = getModelFileDestination(customModel);
+    // why would this ever be true?
+    File modelFolder = modelFileDestination.getParentFile();
+    if (!modelFolder.exists()) {
+      modelFolder.mkdirs();
+    }
 
     // Moves to the final destination file in app private folder to avoid the downloaded file from
     // being changed by
@@ -156,11 +182,44 @@ public synchronized File moveModelToDestinationFolder(
       fos.getFD().sync();
     } catch (IOException e) {
       // Failed to copy to destination - clean up.
-      System.out.println("Failed to copy downloaded model file to destination folder: " + e);
+      Log.d(TAG, "Failed to copy downloaded model file to destination folder: " + e.toString());
       modelFileDestination.delete();
       return null;
     }
 
     return modelFileDestination;
   }
+
+  /**
+   * Deletes all previously cached Model File(s) and the model root folder.
+   *
+   * <p>All model and model support files are stored in temp folder until the model gets fully
+   * downloaded, validated and hash-checked. We delete both temp files and files in the final
+   * destination for a model in this method.
+   */
+  @WorkerThread
+  public synchronized void deleteAllModels(@NonNull String modelName) {
+    File modelFolder = getModelDirUnsafe(modelName);
+    deleteRecursively(modelFolder);
+  }
+
+  /**
+   * Deletes all files under the {@code root}. If @{code root} is a file, just itself will be
+   * deleted. If it is a folder, all files and subfolders will be deleted, including {@code root}
+   * itself.
+   */
+  boolean deleteRecursively(@Nullable File root) {
+    if (root == null) {
+      return false;
+    }
+
+    boolean ret = true;
+    if (root.isDirectory()) {
+      for (File f : Preconditions.checkNotNull(root.listFiles())) {
+        ret = ret && deleteRecursively(f);
+      }
+    }
+
+    return ret && root.delete();
+  }
 }

firebase-ml-modeldownloader/src/main/java/com/google/firebase/ml/modeldownloader/internal/SharedPreferencesUtil.java

@@ -193,12 +193,12 @@ public synchronized void setFailedUploadedCustomModelDetails(@NonNull String cus
   /**
    * Clears all stored data related to a local custom model, including download details.
    *
+   * <p>Call ModelFileManager.deleteAllModels() before calling this to trigger successful clean up
+   * of downloaded files.
+   *
    * @param modelName - name of model
    */
-  public synchronized void clearModelDetails(@NonNull String modelName, boolean cleanUpModelFile) {
-    if (cleanUpModelFile) {
-      // TODO(annz) - add code to remove model files from device
-    }
+  public synchronized void clearModelDetails(@NonNull String modelName) {
     Editor editor = getSharedPreferences().edit();
 
     clearDownloadingModelDetails(editor, modelName);

firebase-ml-modeldownloader/src/test/java/com/google/firebase/ml/modeldownloader/CustomModelTest.java

@@ -17,6 +17,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -28,6 +29,8 @@
 import com.google.firebase.FirebaseOptions.Builder;
 import com.google.firebase.ml.modeldownloader.internal.ModelFileDownloadService;
 import java.io.File;
+import java.io.IOException;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -37,33 +40,63 @@
 
 @RunWith(RobolectricTestRunner.class)
 public class CustomModelTest {
+  private static final String MODEL_NAME = "ModelName";
+  private static final String MODEL_HASH = "dsf324";
 
-  public static final String MODEL_NAME = "ModelName";
-  public static final String MODEL_HASH = "dsf324";
-  public static final File TEST_MODEL_FILE = new File("fakeFile.tflite");
-  public static final File TEST_MODEL_FILE_UPDATED = new File("fakeUpdateFile.tflite");
-
-  public static final String MODEL_URL = "https://project.firebase.com/modelName/23424.jpg";
-  public static final String TEST_PROJECT_ID = "777777777777";
-  public static final FirebaseOptions FIREBASE_OPTIONS =
+  private static final String MODEL_URL = "https://project.firebase.com/modelName/23424.jpg";
+  private static final String TEST_PROJECT_ID = "777777777777";
+  private static final FirebaseOptions FIREBASE_OPTIONS =
       new Builder()
           .setApplicationId("1:123456789:android:abcdef")
           .setProjectId(TEST_PROJECT_ID)
           .build();
   private static final long URL_EXPIRATION = 604800L;
-  final CustomModel CUSTOM_MODEL = new CustomModel(MODEL_NAME, MODEL_HASH, 100, 0);
-  final CustomModel CUSTOM_MODEL_URL =
+
+  private final CustomModel CUSTOM_MODEL = new CustomModel(MODEL_NAME, MODEL_HASH, 100, 0);
+  private final CustomModel CUSTOM_MODEL_URL =
       new CustomModel(MODEL_NAME, MODEL_HASH, 100, MODEL_URL, URL_EXPIRATION);
-  final CustomModel CUSTOM_MODEL_FILE =
-      new CustomModel(MODEL_NAME, MODEL_HASH, 100, 0, TEST_MODEL_FILE.getPath());
-  @Mock ModelFileDownloadService fileDownloadService;
+  private final CustomModel CUSTOM_MODEL_BADFILE =
+      new CustomModel(MODEL_NAME, MODEL_HASH, 100, 0, "tmp/some/bad/filepath/model.tflite");
+
+  private File testModelFile;
+  private File testModelFile2;
+  private CustomModel customModelWithFile;
+
+  @Mock private ModelFileDownloadService fileDownloadService;
 
   @Before
-  public void setUp() {
+  public void setUp() throws IOException {
     MockitoAnnotations.initMocks(this);
     FirebaseApp.clearInstancesForTest();
     // default app
-    FirebaseApp.initializeApp(ApplicationProvider.getApplicationContext(), FIREBASE_OPTIONS);
+    FirebaseApp app =
+        FirebaseApp.initializeApp(ApplicationProvider.getApplicationContext(), FIREBASE_OPTIONS);
+    setUpTestingFiles(app);
+    customModelWithFile = new CustomModel(MODEL_NAME, MODEL_HASH, 100, 0, testModelFile.getPath());
+  }
+
+  private void setUpTestingFiles(FirebaseApp app) throws IOException {
+    final File testDir = new File(app.getApplicationContext().getNoBackupFilesDir(), "tmpModels");
+    testDir.mkdirs();
+    // make sure the directory is empty. Doesn't recurse into subdirs, but that's OK since
+    // we're only using this directory for this test and we won't create any subdirs.
+    for (File f : testDir.listFiles()) {
+      if (f.isFile()) {
+        f.delete();
+      }
+    }
+
+    testModelFile = File.createTempFile("tmpModelFile", "tflite");
+    testModelFile2 = File.createTempFile("tmpModelFile2", "tflite");
+
+    assertTrue(testModelFile.exists());
+    assertTrue(testModelFile2.exists());
+  }
+
+  @After
+  public void teardown() {
+    testModelFile.deleteOnExit();
+    testModelFile2.deleteOnExit();
   }
 
   @Test
@@ -96,22 +129,29 @@ public void customModel_getFile_noLocalNoDownloadIncomplete() throws Exception {
   @Test
   public void customModel_getFile_localModelNoDownload() throws Exception {
     when(fileDownloadService.loadNewlyDownloadedModelFile(any(CustomModel.class))).thenReturn(null);
-    assertEquals(CUSTOM_MODEL_FILE.getFile(fileDownloadService), TEST_MODEL_FILE);
+    assertEquals(customModelWithFile.getFile(fileDownloadService), testModelFile);
+    verify(fileDownloadService, times(1)).loadNewlyDownloadedModelFile(any());
+  }
+
+  @Test
+  public void customModel_getFile_localModelNoDownload_BadFile() throws Exception {
+    when(fileDownloadService.loadNewlyDownloadedModelFile(any(CustomModel.class))).thenReturn(null);
+    assertNull(CUSTOM_MODEL_BADFILE.getFile(fileDownloadService));
     verify(fileDownloadService, times(1)).loadNewlyDownloadedModelFile(any());
   }
 
   @Test
   public void customModel_getFile_localModelDownloadComplete() throws Exception {
     when(fileDownloadService.loadNewlyDownloadedModelFile(any(CustomModel.class)))
-        .thenReturn(TEST_MODEL_FILE_UPDATED);
-    assertEquals(CUSTOM_MODEL_FILE.getFile(fileDownloadService), TEST_MODEL_FILE_UPDATED);
+        .thenReturn(testModelFile2);
+    assertEquals(customModelWithFile.getFile(fileDownloadService), testModelFile2);
     verify(fileDownloadService, times(1)).loadNewlyDownloadedModelFile(any());
   }
 
   @Test
   public void customModel_getFile_noLocalDownloadComplete() throws Exception {
-    when(fileDownloadService.loadNewlyDownloadedModelFile(any())).thenReturn(TEST_MODEL_FILE);
-    assertEquals(CUSTOM_MODEL.getFile(fileDownloadService), TEST_MODEL_FILE);
+    when(fileDownloadService.loadNewlyDownloadedModelFile(any())).thenReturn(testModelFile);
+    assertEquals(CUSTOM_MODEL.getFile(fileDownloadService), testModelFile);
     verify(fileDownloadService, times(1)).loadNewlyDownloadedModelFile(any());
   }
 

firebase-ml-modeldownloader/src/test/java/com/google/firebase/ml/modeldownloader/FirebaseModelDownloaderTest.java

@@ -32,6 +32,7 @@
 import com.google.firebase.FirebaseOptions.Builder;
 import com.google.firebase.ml.modeldownloader.internal.CustomModelDownloadService;
 import com.google.firebase.ml.modeldownloader.internal.ModelFileDownloadService;
+import com.google.firebase.ml.modeldownloader.internal.ModelFileManager;
 import com.google.firebase.ml.modeldownloader.internal.SharedPreferencesUtil;
 import java.util.Collections;
 import java.util.Set;
@@ -67,6 +68,7 @@ public class FirebaseModelDownloaderTest {
   FirebaseModelDownloader firebaseModelDownloader;
   @Mock SharedPreferencesUtil mockPrefs;
   @Mock ModelFileDownloadService mockFileDownloadService;
+  @Mock ModelFileManager mockFileManager;
   @Mock CustomModelDownloadService mockModelDownloadService;
   ExecutorService executor;
 
@@ -84,6 +86,7 @@ public void setUp() {
             mockPrefs,
             mockFileDownloadService,
             mockModelDownloadService,
+            mockFileManager,
             executor);
   }
 
@@ -184,9 +187,17 @@ public void listDownloadedModels_returnsModelList() throws Exception {
   }
 
   @Test
-  public void deleteDownloadedModel_unimplemented() {
-    assertThrows(
-        UnsupportedOperationException.class,
-        () -> FirebaseModelDownloader.getInstance().deleteDownloadedModel(MODEL_NAME));
+  public void deleteDownloadedModel() throws Exception {
+    doNothing().when(mockPrefs).clearModelDetails(eq(MODEL_NAME));
+    doNothing().when(mockFileManager).deleteAllModels(eq(MODEL_NAME));
+
+    TestOnCompleteListener<Void> onCompleteListener = new TestOnCompleteListener<>();
+    Task<Void> task = firebaseModelDownloader.deleteDownloadedModel(MODEL_NAME);
+    task.addOnCompleteListener(executor, onCompleteListener);
+    onCompleteListener.await();
+
+    assertThat(task.isComplete()).isTrue();
+    verify(mockPrefs, times(1)).clearModelDetails(eq(MODEL_NAME));
+    verify(mockFileManager, times(1)).deleteAllModels(eq(MODEL_NAME));
   }
 }