Closing Firestore instance on FirebaseApp.delete() (firebase#227)

* Closing Firestore instance on FirebaseApp.delete()

* Updated javadocs

* Updated docs and tests

Author: hiranya911

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Unreleased
 
--
+- [fixed] `Firestore` instances initialized by the SDK are now cleaned
+  up, when `FirebaseApp.delete()` is called.
 
 # v6.6.0
 

src/main/java/com/google/firebase/cloud/FirestoreClient.java

@@ -11,6 +11,8 @@
 import com.google.firebase.ImplFirebaseTrampolines;
 import com.google.firebase.internal.FirebaseService;
 import com.google.firebase.internal.NonNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * {@code FirestoreClient} provides access to Google Cloud Firestore. Use this API to obtain a
@@ -26,6 +28,8 @@
  */
 public class FirestoreClient {
 
+  private static final Logger logger = LoggerFactory.getLogger(FirestoreClient.class);
+
   private final Firestore firestore;
 
   private FirestoreClient(FirebaseApp app) {
@@ -48,7 +52,9 @@ private FirestoreClient(FirebaseApp app) {
   }
 
   /**
-   * Returns the Firestore instance associated with the default Firebase app.
+   * Returns the Firestore instance associated with the default Firebase app. Returns the same
+   * instance for all invocations. The Firestore instance and all references obtained from it
+   * becomes unusable, once the default app is deleted.
    *
    * @return A non-null <a href="https://googlecloudplatform.github.io/google-cloud-java/google-cloud-clients/apidocs/com/google/cloud/firestore/Firestore.html">{@code Firestore}</a>
    *     instance.
@@ -59,7 +65,9 @@ public static Firestore getFirestore() {
   }
 
   /**
-   * Returns the Firestore instance associated with the specified Firebase app.
+   * Returns the Firestore instance associated with the specified Firebase app. For a given app,
+   * always returns the same instance. The Firestore instance and all references obtained from it
+   * becomes unusable, once the specified app is deleted.
    *
    * @param app A non-null {@link FirebaseApp}.
    * @return A non-null <a href="https://googlecloudplatform.github.io/google-cloud-java/google-cloud-clients/apidocs/com/google/cloud/firestore/Firestore.html">{@code Firestore}</a>
@@ -89,10 +97,11 @@ private static class FirestoreClientService extends FirebaseService<FirestoreCli
 
     @Override
     public void destroy() {
-      // NOTE: We don't explicitly tear down anything here (for now). User won't be able to call
-      // FirestoreClient.getFirestore() any more, but already created Firestore instances will
-      // continue to work. Request Firestore team to provide a cleanup/teardown method on the
-      // Firestore object.
+      try {
+        instance.firestore.close();
+      } catch (Exception e) {
+        logger.warn("Error while closing the Firestore instance", e);
+      }
     }
   }
 

src/test/java/com/google/firebase/cloud/FirestoreClientTest.java

@@ -7,20 +7,29 @@
 import static org.junit.Assert.fail;
 
 import com.google.auth.oauth2.GoogleCredentials;
+import com.google.cloud.firestore.DocumentReference;
 import com.google.cloud.firestore.Firestore;
 import com.google.cloud.firestore.FirestoreOptions;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.FirebaseOptions;
 import com.google.firebase.FirebaseOptions.Builder;
 import com.google.firebase.ImplFirebaseTrampolines;
 import com.google.firebase.TestOnlyImplFirebaseTrampolines;
+import com.google.firebase.auth.MockGoogleCredentials;
 import com.google.firebase.testing.ServiceAccount;
 import java.io.IOException;
 import org.junit.After;
 import org.junit.Test;
 
 public class FirestoreClientTest {
 
+  public static final FirestoreOptions FIRESTORE_OPTIONS = FirestoreOptions.newBuilder()
+      // Setting credentials is not required (they get overridden by Admin SDK), but without
+      // this Firestore logs an ugly warning during tests.
+      .setCredentials(new MockGoogleCredentials("test-token"))
+      .setTimestampsInSnapshotsEnabled(true)
+      .build();
+
   @After
   public void tearDown() {
     TestOnlyImplFirebaseTrampolines.clearInstancesForTest();
@@ -31,9 +40,7 @@ public void testExplicitProjectId() throws IOException {
     FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder()
         .setCredentials(GoogleCredentials.fromStream(ServiceAccount.EDITOR.asStream()))
         .setProjectId("explicit-project-id")
-        .setFirestoreOptions(FirestoreOptions.newBuilder()
-            .setTimestampsInSnapshotsEnabled(true)
-            .build())
+        .setFirestoreOptions(FIRESTORE_OPTIONS)
         .build());
     Firestore firestore = FirestoreClient.getFirestore(app);
     assertEquals("explicit-project-id", firestore.getOptions().getProjectId());
@@ -46,9 +53,7 @@ public void testExplicitProjectId() throws IOException {
   public void testServiceAccountProjectId() throws IOException {
     FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder()
         .setCredentials(GoogleCredentials.fromStream(ServiceAccount.EDITOR.asStream()))
-        .setFirestoreOptions(FirestoreOptions.newBuilder()
-            .setTimestampsInSnapshotsEnabled(true)
-            .build())
+        .setFirestoreOptions(FIRESTORE_OPTIONS)
         .build());
     Firestore firestore = FirestoreClient.getFirestore(app);
     assertEquals("mock-project-id", firestore.getOptions().getProjectId());
@@ -62,9 +67,7 @@ public void testFirestoreOptions() throws IOException {
     FirebaseApp app = FirebaseApp.initializeApp(new Builder()
         .setCredentials(GoogleCredentials.fromStream(ServiceAccount.EDITOR.asStream()))
         .setProjectId("explicit-project-id")
-        .setFirestoreOptions(FirestoreOptions.newBuilder()
-            .setTimestampsInSnapshotsEnabled(true)
-            .build())
+        .setFirestoreOptions(FIRESTORE_OPTIONS)
         .build());
     Firestore firestore = FirestoreClient.getFirestore(app);
     assertEquals("explicit-project-id", firestore.getOptions().getProjectId());
@@ -104,12 +107,12 @@ public void testAppDelete() throws IOException {
     FirebaseApp app = FirebaseApp.initializeApp(new FirebaseOptions.Builder()
         .setCredentials(GoogleCredentials.fromStream(ServiceAccount.EDITOR.asStream()))
         .setProjectId("mock-project-id")
-        .setFirestoreOptions(FirestoreOptions.newBuilder()
-            .setTimestampsInSnapshotsEnabled(true)
-            .build())
+        .setFirestoreOptions(FIRESTORE_OPTIONS)
         .build());
 
-    assertNotNull(FirestoreClient.getFirestore(app));
+    Firestore firestore = FirestoreClient.getFirestore(app);
+    assertNotNull(firestore);
+    DocumentReference document = firestore.collection("collection").document("doc");
     app.delete();
     try {
       FirestoreClient.getFirestore(app);
@@ -118,12 +121,18 @@ public void testAppDelete() throws IOException {
       // ignore
     }
 
+    try {
+      document.get();
+      fail("No error thrown for deleted app");
+    } catch (IllegalStateException expected) {
+      // ignore
+    }
+
     try {
       FirestoreClient.getFirestore();
       fail("No error thrown for deleted app");
     } catch (IllegalStateException expected) {
       // ignore
     }
   }
-
 }