From 0f7b556b5032728a94515e4ff0678919db92ff79 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 20 May 2019 14:36:57 -0700 Subject: [PATCH] Remove synchronous exception throwing from clearPersistence() (#447) --- .../database/IntegrationTestHelpers.java | 1 - .../firebase/firestore/FirestoreTest.java | 22 +++++++++--- .../testutil/IntegrationTestUtil.java | 5 ++- .../firebase/firestore/FirebaseFirestore.java | 35 +++++++++++-------- .../firestore/core/FirestoreClient.java | 12 ++++--- 5 files changed, 50 insertions(+), 25 deletions(-) diff --git a/firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java b/firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java index f9df68428f4..114d86b6e32 100644 --- a/firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java +++ b/firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java @@ -61,7 +61,6 @@ public class IntegrationTestHelpers { private static List contexts = new ArrayList(); private static String testSecret = null; - private static boolean appInitialized = false; public static Path path(String path) { return new Path(path); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index d717c79e27f..64993198628 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/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 transactionTask = AccessHelper.clearPersistence(firestore); + waitForException(transactionTask); + assertFalse(transactionTask.isSuccessful()); + Exception e = transactionTask.getException(); + FirebaseFirestoreException firestoreException = (FirebaseFirestoreException) e; + assertEquals(Code.FAILED_PRECONDITION, firestoreException.getCode()); } } diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 8c49f076585..8b72d5cea76 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/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 toDataMap(QuerySnapshot qrySnap) { Map result = new HashMap<>(); for (DocumentSnapshot docSnap : qrySnap.getDocuments()) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index e9354166c23..7f6907d891d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/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 runBatch(@NonNull WriteBatch.Function batchFunction) { Task 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. + * + *

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. * - *

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()). + *

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 clearPersistence() { - if (this.clientRunning) { - throw new IllegalStateException("Persistence cannot be cleared while the client is running."); - } final TaskCompletionSource 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) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index 293e2249497..b856cedffca 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/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 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 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"); } }