Respond to Sebastian's review comments

Author: samtstern

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

@@ -186,9 +186,7 @@ static FirebaseFirestore newInstance(
     this.emulatorSettings = emulatorSettings;
 
     this.settings = new FirebaseFirestoreSettings.Builder().build();
-    if (this.emulatorSettings != null) {
-      this.settings = mergeEmulatorSettings(settings, emulatorSettings);
-    }
+    this.settings = mergeEmulatorSettings(settings, emulatorSettings);
   }
 
   /** Returns the settings used by this {@code FirebaseFirestore} object. */
@@ -204,6 +202,8 @@ public FirebaseFirestoreSettings getFirestoreSettings() {
   public void setFirestoreSettings(@NonNull FirebaseFirestoreSettings settings) {
     synchronized (databaseId) {
       checkNotNull(settings, "Provided settings must not be null.");
+      settings = mergeEmulatorSettings(settings, emulatorSettings);
+
       // As a special exception, don't throw if the same settings are passed repeatedly. This
       // should make it simpler to get a Firestore instance in an activity.
       if (client != null && !this.settings.equals(settings)) {
@@ -213,10 +213,6 @@ public void setFirestoreSettings(@NonNull FirebaseFirestoreSettings settings) {
                 + "FirebaseFirestore object.");
       }
 
-      if (this.emulatorSettings != null) {
-        settings = mergeEmulatorSettings(settings, this.emulatorSettings);
-      }
-
       this.settings = settings;
     }
   }
@@ -241,13 +237,14 @@ private void ensureClientConfigured() {
 
   private FirebaseFirestoreSettings mergeEmulatorSettings(
       @NonNull FirebaseFirestoreSettings settings,
-      @NonNull EmulatedServiceSettings emulatorSettings) {
+      @Nullable EmulatedServiceSettings emulatorSettings) {
+    if (emulatorSettings == null) {
+      return settings;
+    }
 
     if (!FirebaseFirestoreSettings.DEFAULT_HOST.equals(settings.getHost())) {
       throw new IllegalStateException(
-          "Cannot change the Firestore host through FirebaseFirestoreSettings when "
-              + "EmulatedServiceSettings are also in effect. "
-              + "Make sure to only set the host in one location.");
+          "Cannot specify the host in FirebaseFirestoreSettings when EmulatedServiceSettings is provided.");
     }
 
     return new FirebaseFirestoreSettings.Builder(settings)

firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreTest.java

@@ -16,6 +16,8 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import androidx.annotation.NonNull;
 import androidx.test.platform.app.InstrumentationRegistry;
@@ -69,7 +71,7 @@ FirebaseFirestore.EMULATOR, new EmulatedServiceSettings("10.0.2.2", 8080))
     assertFalse(settings.isPersistenceEnabled());
   }
 
-  @Test(expected = IllegalStateException.class)
+  @Test
   public void getInstance_withEmulator_mergeSettingsFailure() {
     FirebaseApp app = getApp("getInstance_withEmulator_mergeSettingsFailure");
     app.enableEmulators(
@@ -78,9 +80,64 @@ public void getInstance_withEmulator_mergeSettingsFailure() {
                 FirebaseFirestore.EMULATOR, new EmulatedServiceSettings("10.0.2.2", 8080))
             .build());
 
+    try {
+      FirebaseFirestore firestore = FirebaseFirestore.getInstance(app);
+      firestore.setFirestoreSettings(
+          new FirebaseFirestoreSettings.Builder().setHost("myhost.com").build());
+      fail("Exception should be thrown");
+    } catch (Exception e) {
+      assertTrue(e instanceof IllegalStateException);
+      assertEquals(
+          e.getMessage(),
+          "Cannot specify the host in FirebaseFirestoreSettings when EmulatedServiceSettings is provided.");
+    }
+  }
+
+  @Test
+  public void setSettings_repeatedSuccess() {
+    FirebaseApp app = getApp("setSettings_repeatedSuccess");
     FirebaseFirestore firestore = FirebaseFirestore.getInstance(app);
-    firestore.setFirestoreSettings(
-        new FirebaseFirestoreSettings.Builder().setHost("myhost.com").build());
+
+    FirebaseFirestoreSettings settings =
+        new FirebaseFirestoreSettings.Builder().setHost("myhost.com").setSslEnabled(false).build();
+    firestore.setFirestoreSettings(settings);
+
+    // This should 'start' Firestore
+    DocumentReference reference = firestore.document("foo/bar");
+
+    // Second settings set should pass because the settings are equal
+    firestore.setFirestoreSettings(settings);
+  }
+
+  @Test
+  public void setSettings_repeatedFailure() {
+    FirebaseApp app = getApp("setSettings_repeatedFailure");
+    FirebaseFirestore firestore = FirebaseFirestore.getInstance(app);
+
+    FirebaseFirestoreSettings settings =
+        new FirebaseFirestoreSettings.Builder().setHost("myhost.com").setSslEnabled(false).build();
+
+    FirebaseFirestoreSettings otherSettings =
+        new FirebaseFirestoreSettings.Builder()
+            .setHost("otherhost.com")
+            .setSslEnabled(false)
+            .build();
+
+    firestore.setFirestoreSettings(settings);
+
+    // This should 'start' Firestore
+    DocumentReference reference = firestore.document("foo/bar");
+
+    try {
+      firestore.setFirestoreSettings(otherSettings);
+      fail("Exception should be thrown");
+    } catch (Exception e) {
+      assertTrue(e instanceof IllegalStateException);
+      assertTrue(
+          e.getMessage()
+              .startsWith(
+                  "FirebaseFirestore has already been started and its settings can no longer be changed."));
+    }
   }
 
   @NonNull