Remove synchronous exception throwing from clearPersistence() (#447)

firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java

@@ -61,7 +61,6 @@ public class IntegrationTestHelpers {
 
   private static List<DatabaseConfig> contexts = new ArrayList<DatabaseConfig>();
   private static String testSecret = null;
-  private static boolean appInitialized = false;
 
   public static Path path(String path) {
     return new Path(path);

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java

@@ -970,8 +970,12 @@ public void testMaintainsPersistenceAfterRestarting() {
         testFirestore(provider().projectId(), Level.DEBUG, newTestSettings(), "dbPersistenceKey");
     DocumentReference docRef = firestore.collection("col1").document("doc1");
     waitFor(docRef.set(map("foo", "bar")));
-
     waitFor(AccessHelper.shutdown(firestore));
+    IntegrationTestUtil.removeFirestore(firestore);
+
+    // We restart the app with the same name and options to check that the previous instance's
+    // persistent storage is actually cleared after the restart. Calling testFirestore() without the
+    // parameters would create a new instance of firestore, which defeats the purpose of this test.
     FirebaseFirestore firestore2 =
         testFirestore(provider().projectId(), Level.DEBUG, newTestSettings(), "dbPersistenceKey");
     DocumentReference docRef2 = firestore2.document(docRef.getPath());
@@ -985,9 +989,13 @@ public void testCanClearPersistenceAfterRestarting() throws Exception {
         testFirestore(provider().projectId(), Level.DEBUG, newTestSettings(), "dbPersistenceKey");
     DocumentReference docRef = firestore.collection("col1").document("doc1");
     waitFor(docRef.set(map("foo", "bar")));
-
     waitFor(AccessHelper.shutdown(firestore));
+    IntegrationTestUtil.removeFirestore(firestore);
     waitFor(AccessHelper.clearPersistence(firestore));
+
+    // We restart the app with the same name and options to check that the previous instance's
+    // persistent storage is actually cleared after the restart. Calling testFirestore() without the
+    // parameters would create a new instance of firestore, which defeats the purpose of this test.
     FirebaseFirestore firestore2 =
         testFirestore(provider().projectId(), Level.DEBUG, newTestSettings(), "dbPersistenceKey");
     DocumentReference docRef2 = firestore2.document(docRef.getPath());
@@ -999,8 +1007,12 @@ public void testCanClearPersistenceAfterRestarting() throws Exception {
   public void testClearPersistenceWhileRunningFails() {
     FirebaseFirestore firestore = testFirestore();
     waitFor(firestore.enableNetwork());
-    expectError(
-        () -> waitFor(AccessHelper.clearPersistence(firestore)),
-        "Persistence cannot be cleared while the client is running.");
+
+    Task<Void> transactionTask = AccessHelper.clearPersistence(firestore);
+    waitForException(transactionTask);
+    assertFalse(transactionTask.isSuccessful());
+    Exception e = transactionTask.getException();
+    FirebaseFirestoreException firestoreException = (FirebaseFirestoreException) e;
+    assertEquals(Code.FAILED_PRECONDITION, firestoreException.getCode());
   }
 }

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

@@ -61,7 +61,6 @@ public class IntegrationTestUtil {
   // Production environment. Note that the Android Emulator treats "10.0.2.2" as its host machine.
   // TODO(mrschmidt): Support multiple envrionments (Emulator, QA, Nightly, Production)
   private static final boolean CONNECT_TO_EMULATOR = false;
-
   private static final String EMULATOR_HOST = "10.0.2.2";
   private static final int EMULATOR_PORT = 8081;
 
@@ -411,6 +410,10 @@ public static boolean isNetworkEnabled(FirebaseFirestore firestore) {
     return firestoreStatus.get(firestore);
   }
 
+  public static void removeFirestore(FirebaseFirestore firestore) {
+    firestoreStatus.remove(firestore);
+  }
+
   public static Map<String, Object> toDataMap(QuerySnapshot qrySnap) {
     Map<String, Object> result = new HashMap<>();
     for (DocumentSnapshot docSnap : qrySnap.getDocuments()) {

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

@@ -30,6 +30,7 @@
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.annotations.PublicApi;
 import com.google.firebase.auth.internal.InternalAuthProvider;
+import com.google.firebase.firestore.FirebaseFirestoreException.Code;
 import com.google.firebase.firestore.auth.CredentialsProvider;
 import com.google.firebase.firestore.auth.EmptyCredentialsProvider;
 import com.google.firebase.firestore.auth.FirebaseAuthCredentialsProvider;
@@ -52,6 +53,7 @@
  */
 @PublicApi
 public class FirebaseFirestore {
+
   private static final String TAG = "FirebaseFirestore";
   private final Context context;
   // This is also used as private lock object for this instance. There is nothing inherent about
@@ -61,12 +63,9 @@ public class FirebaseFirestore {
   private final CredentialsProvider credentialsProvider;
   private final AsyncQueue asyncQueue;
   private final FirebaseApp firebaseApp;
-
+  private final UserDataConverter dataConverter;
   private FirebaseFirestoreSettings settings;
   private volatile FirestoreClient client;
-  private final UserDataConverter dataConverter;
-
-  private boolean clientRunning;
 
   @NonNull
   @PublicApi
@@ -190,7 +189,6 @@ private void ensureClientConfigured() {
       if (client != null) {
         return;
       }
-      this.clientRunning = true;
       DatabaseInfo databaseInfo =
           new DatabaseInfo(databaseId, persistenceKey, settings.getHost(), settings.isSslEnabled());
 
@@ -348,7 +346,6 @@ public Task<Void> runBatch(@NonNull WriteBatch.Function batchFunction) {
   Task<Void> shutdown() {
     // The client must be initialized to ensure that all subsequent API usage throws an exception.
     this.ensureClientConfigured();
-    this.clientRunning = false;
     return client.shutdown();
   }
 
@@ -392,22 +389,32 @@ public static void setLoggingEnabled(boolean loggingEnabled) {
   }
 
   /**
-   * Clears the persistent storage.
+   * Clears the persistent storage. This includes pending writes and cached documents.
+   *
+   * <p>Must be called while the firestore instance is not started (after the app is shutdown or
+   * when the app is first initialized). On startup, this method must be called before other methods
+   * (other than setFirestoreSettings()). If the firestore instance is still running, the Task will
+   * fail with an error code of FAILED_PRECONDITION.
    *
-   * <p>Must be called while the client is not started (after the app is shutdown or when the app is
-   * first initialized). On startup, this method must be called before other methods (other than
-   * setFirestoreSettings()).
+   * <p>Note: clearPersistence() is primarily intended to help write reliable tests that use
+   * Firestore. It uses the most efficient mechanism possible for dropping existing data but does
+   * not attempt to securely overwrite or otherwise make cached data unrecoverable. For applications
+   * that are sensitive to the disclosure of cache data in between user sessions we strongly
+   * recommend not to enable persistence in the first place.
    *
-   * @throws IllegalStateException if the client is still running.
+   * @return A Task that is resolved once the persistent storage has been cleared. Otherwise, the
+   *     Task is rejected with an error.
    */
   Task<Void> clearPersistence() {
-    if (this.clientRunning) {
-      throw new IllegalStateException("Persistence cannot be cleared while the client is running.");
-    }
     final TaskCompletionSource<Void> source = new TaskCompletionSource<>();
     asyncQueue.enqueueAndForget(
         () -> {
           try {
+            if (client != null && !client.isShutdown()) {
+              throw new FirebaseFirestoreException(
+                  "Persistence cannot be cleared while the firestore instance is running.",
+                  Code.FAILED_PRECONDITION);
+            }
             SQLitePersistence.clearPersistence(context, databaseId, persistenceKey);
             source.setResult(null);
           } catch (FirebaseFirestoreException e) {

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

@@ -75,7 +75,7 @@ public final class FirestoreClient implements RemoteStore.RemoteStoreCallback {
   private RemoteStore remoteStore;
   private SyncEngine syncEngine;
   private EventManager eventManager;
-  private volatile boolean isShutdown = false;
+  private volatile boolean clientShutdown = false;
 
   // LRU-related
   @Nullable private LruGarbageCollector.Scheduler lruScheduler;
@@ -140,17 +140,21 @@ public Task<Void> shutdown() {
     credentialsProvider.removeChangeListener();
     return asyncQueue.enqueue(
         () -> {
-          if (!this.isShutdown) {
+          if (!this.clientShutdown) {
             remoteStore.shutdown();
             persistence.shutdown();
             if (lruScheduler != null) {
               lruScheduler.stop();
             }
-            this.isShutdown = true;
+            this.clientShutdown = true;
           }
         });
   }
 
+  public boolean isShutdown() {
+    return this.clientShutdown;
+  }
+
   /** Starts listening to a query. */
   public QueryListener listen(
       Query query, ListenOptions options, EventListener<ViewSnapshot> listener) {
@@ -268,7 +272,7 @@ private void initialize(Context context, User user, boolean usePersistence, long
   }
 
   private void verifyNotShutdown() {
-    if (this.isShutdown) {
+    if (this.clientShutdown) {
       throw new IllegalArgumentException("The client has already been shutdown");
     }
   }