Merge pull request GitLiveApp#562 from splendo/feature/552-ios-settings-crash

nbransby · web-flow · commit 101869ced66a · 2024-07-11T18:34:02.000+08:00
Fixed a crash for settings when iOS would access the same Firebase instance twice
diff --git a/firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt b/firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt
@@ -1,14 +1,7 @@
 package dev.gitlive.firebase.firestore.internal
 
-import com.google.android.gms.tasks.TaskExecutors
-import com.google.firebase.firestore.MemoryCacheSettings
-import com.google.firebase.firestore.MemoryEagerGcSettings
-import com.google.firebase.firestore.MemoryLruGcSettings
-import com.google.firebase.firestore.PersistentCacheSettings
 import com.google.firebase.firestore.firestoreSettings
 import dev.gitlive.firebase.firestore.FirebaseFirestoreSettings
-import dev.gitlive.firebase.firestore.LocalCacheSettings
-import dev.gitlive.firebase.firestore.MemoryGarbageCollectorSettings
 import dev.gitlive.firebase.firestore.NativeFirebaseFirestore
 import dev.gitlive.firebase.firestore.NativeTransaction
 import dev.gitlive.firebase.firestore.android
@@ -17,59 +10,6 @@ import kotlinx.coroutines.tasks.await
 
 internal actual class NativeFirebaseFirestoreWrapper actual constructor(actual val native: NativeFirebaseFirestore) {
 
-    actual var settings: FirebaseFirestoreSettings
-        get() = with(native.firestoreSettings) {
-            FirebaseFirestoreSettings(
-                isSslEnabled,
-                host,
-                cacheSettings?.let { localCacheSettings ->
-                    when (localCacheSettings) {
-                        is MemoryCacheSettings -> {
-                            val garbageCollectionSettings =
-                                when (val settings = localCacheSettings.garbageCollectorSettings) {
-                                    is MemoryEagerGcSettings -> MemoryGarbageCollectorSettings.Eager
-                                    is MemoryLruGcSettings -> MemoryGarbageCollectorSettings.LRUGC(
-                                        settings.sizeBytes,
-                                    )
-
-                                    else -> throw IllegalArgumentException("Existing settings does not have valid GarbageCollectionSettings")
-                                }
-                            LocalCacheSettings.Memory(garbageCollectionSettings)
-                        }
-
-                        is PersistentCacheSettings -> LocalCacheSettings.Persistent(
-                            localCacheSettings.sizeBytes,
-                        )
-
-                        else -> throw IllegalArgumentException("Existing settings is not of a valid type")
-                    }
-                } ?: kotlin.run {
-                    @Suppress("DEPRECATION")
-                    when {
-                        isPersistenceEnabled -> LocalCacheSettings.Persistent(cacheSizeBytes)
-                        cacheSizeBytes == FirebaseFirestoreSettings.CACHE_SIZE_UNLIMITED -> LocalCacheSettings.Memory(
-                            MemoryGarbageCollectorSettings.Eager,
-                        )
-
-                        else -> LocalCacheSettings.Memory(
-                            MemoryGarbageCollectorSettings.LRUGC(
-                                cacheSizeBytes,
-                            ),
-                        )
-                    }
-                },
-                callbackExecutorMap[native] ?: TaskExecutors.MAIN_THREAD,
-            )
-        }
-        set(value) {
-            native.firestoreSettings = firestoreSettings {
-                isSslEnabled = value.sslEnabled
-                host = value.host
-                setLocalCacheSettings(value.cacheSettings.android)
-            }
-            callbackExecutorMap[native] = value.callbackExecutor
-        }
-
     actual fun collection(collectionPath: String) = native.collection(collectionPath)
 
     actual fun collectionGroup(collectionId: String) = native.collectionGroup(collectionId)
@@ -82,6 +22,15 @@ internal actual class NativeFirebaseFirestoreWrapper actual constructor(actual v
     actual fun setLoggingEnabled(loggingEnabled: Boolean) =
         com.google.firebase.firestore.FirebaseFirestore.setLoggingEnabled(loggingEnabled)
 
+    actual fun applySettings(settings: FirebaseFirestoreSettings) {
+        native.firestoreSettings = firestoreSettings {
+            isSslEnabled = settings.sslEnabled
+            host = settings.host
+            setLocalCacheSettings(settings.cacheSettings.android)
+        }
+        callbackExecutorMap[native] = settings.callbackExecutor
+    }
+
     actual suspend fun <T> runTransaction(func: suspend NativeTransaction.() -> T): T =
         native.runTransaction { runBlocking { it.func() } }.await()
 
diff --git a/firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt b/firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt
@@ -44,10 +44,12 @@ public class FirebaseFirestore internal constructor(private val wrapper: NativeF
 
     // Important to leave this as a get property since on JS it is initialized lazily
     internal val native: NativeFirebaseFirestore get() = wrapper.native
+
     public var settings: FirebaseFirestoreSettings
-        get() = wrapper.settings
+        @Deprecated("Property can only be written.", level = DeprecationLevel.ERROR)
+        get() = throw NotImplementedError()
         set(value) {
-            wrapper.settings = value
+            wrapper.applySettings(value)
         }
 
     public fun collection(collectionPath: String): CollectionReference = CollectionReference(wrapper.collection(collectionPath))
@@ -65,7 +67,7 @@ public class FirebaseFirestore internal constructor(private val wrapper: NativeF
         wrapper.useEmulator(host, port)
     }
 
-    @Deprecated("Use settings instead", replaceWith = ReplaceWith("settings = firestoreSettings{}"))
+    @Deprecated("Use SettingsBuilder instead", replaceWith = ReplaceWith("settings = firestoreSettings { }", "dev.gitlive.firebase.firestore.firestoreSettings"))
     public fun setSettings(
         persistenceEnabled: Boolean? = null,
         sslEnabled: Boolean? = null,
@@ -76,18 +78,22 @@ public class FirebaseFirestore internal constructor(private val wrapper: NativeF
             this.sslEnabled = sslEnabled ?: true
             this.host = host ?: FirebaseFirestoreSettings.DEFAULT_HOST
             this.cacheSettings = if (persistenceEnabled != false) {
-                LocalCacheSettings.Persistent(cacheSizeBytes ?: FirebaseFirestoreSettings.CACHE_SIZE_UNLIMITED)
+                LocalCacheSettings.Persistent(
+                    cacheSizeBytes ?: FirebaseFirestoreSettings.CACHE_SIZE_UNLIMITED,
+                )
             } else {
                 val cacheSize = cacheSizeBytes ?: FirebaseFirestoreSettings.CACHE_SIZE_UNLIMITED
-                val garbageCollectionSettings = if (cacheSize == FirebaseFirestoreSettings.CACHE_SIZE_UNLIMITED) {
-                    MemoryGarbageCollectorSettings.Eager
-                } else {
-                    MemoryGarbageCollectorSettings.LRUGC(cacheSize)
-                }
+                val garbageCollectionSettings =
+                    if (cacheSize == FirebaseFirestoreSettings.CACHE_SIZE_UNLIMITED) {
+                        MemoryGarbageCollectorSettings.Eager
+                    } else {
+                        MemoryGarbageCollectorSettings.LRUGC(cacheSize)
+                    }
                 LocalCacheSettings.Memory(garbageCollectionSettings)
             }
         }
     }
+
     public suspend fun disableNetwork() {
         wrapper.disableNetwork()
     }
diff --git a/firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt b/firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt
@@ -9,13 +9,13 @@ import dev.gitlive.firebase.firestore.NativeWriteBatch
 
 internal expect class NativeFirebaseFirestoreWrapper internal constructor(native: NativeFirebaseFirestore) {
     val native: NativeFirebaseFirestore
-    var settings: FirebaseFirestoreSettings
 
     fun collection(collectionPath: String): NativeCollectionReference
     fun collectionGroup(collectionId: String): NativeQuery
     fun document(documentPath: String): NativeDocumentReference
     fun batch(): NativeWriteBatch
     fun setLoggingEnabled(loggingEnabled: Boolean)
+    fun applySettings(settings: FirebaseFirestoreSettings)
     suspend fun clearPersistence()
     suspend fun <T> runTransaction(func: suspend NativeTransaction.() -> T): T
     fun useEmulator(host: String, port: Int)
diff --git a/firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/FirestoreSourceTest.kt b/firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/FirestoreSourceTest.kt
@@ -39,14 +39,14 @@ class FirestoreSourceTest {
         )
 
         firestore = Firebase.firestore(app).apply {
-            useEmulator(emulatorHost, 8080)
-            settings = firestoreSettings(settings) {
+            settings = firestoreSettings {
                 cacheSettings = if (persistenceEnabled) {
                     persistentCacheSettings { }
                 } else {
                     memoryCacheSettings { }
                 }
             }
+            useEmulator(emulatorHost, 8080)
         }
     }
 
diff --git a/firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt b/firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt
@@ -5,6 +5,7 @@
 package dev.gitlive.firebase.firestore
 
 import dev.gitlive.firebase.Firebase
+import dev.gitlive.firebase.FirebaseApp
 import dev.gitlive.firebase.FirebaseOptions
 import dev.gitlive.firebase.apps
 import dev.gitlive.firebase.internal.decode
@@ -85,6 +86,7 @@ class FirebaseFirestoreTest {
         )
     }
 
+    lateinit var firebaseApp: FirebaseApp
     lateinit var firestore: FirebaseFirestore
 
     @BeforeTest
@@ -100,14 +102,15 @@ class FirebaseFirestoreTest {
                 gcmSenderId = "846484016111",
             ),
         )
+        firebaseApp = app
 
         firestore = Firebase.firestore(app).apply {
-            useEmulator(emulatorHost, 8080)
-            settings = firestoreSettings(settings) {
+            settings = firestoreSettings {
                 cacheSettings = memoryCacheSettings {
                     gcSettings = memoryEagerGcSettings { }
                 }
             }
+            useEmulator(emulatorHost, 8080)
         }
     }
 
@@ -1031,6 +1034,12 @@ class FirebaseFirestoreTest {
         fieldQuery.assertDocuments(FirestoreTest.serializer(), testOne)
     }
 
+    @Test
+    fun testMultiple() = runTest {
+        Firebase.firestore(firebaseApp).disableNetwork()
+        Firebase.firestore(firebaseApp).enableNetwork()
+    }
+
     private suspend fun setupFirestoreData(
         documentOne: FirestoreTest = testOne,
         documentTwo: FirestoreTest = testTwo,
diff --git a/firebase-firestore/src/iosMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt b/firebase-firestore/src/iosMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt
@@ -6,21 +6,11 @@ import dev.gitlive.firebase.firestore.NativeFirebaseFirestore
 import dev.gitlive.firebase.firestore.NativeTransaction
 import dev.gitlive.firebase.firestore.await
 import dev.gitlive.firebase.firestore.awaitResult
-import dev.gitlive.firebase.firestore.firestoreSettings
-import dev.gitlive.firebase.firestore.memoryCacheSettings
 import kotlinx.coroutines.runBlocking
 
 @Suppress("UNCHECKED_CAST")
 internal actual class NativeFirebaseFirestoreWrapper internal actual constructor(actual val native: NativeFirebaseFirestore) {
 
-    actual var settings: FirebaseFirestoreSettings = firestoreSettings { }.also {
-        native.settings = it.ios
-    }
-        set(value) {
-            field = value
-            native.settings = value.ios
-        }
-
     actual fun collection(collectionPath: String) = native.collectionWithPath(collectionPath)
 
     actual fun collectionGroup(collectionId: String) = native.collectionGroupWithID(collectionId)
@@ -33,6 +23,10 @@ internal actual class NativeFirebaseFirestoreWrapper internal actual constructor
     actual fun setLoggingEnabled(loggingEnabled: Boolean): Unit =
         FIRFirestore.enableLogging(loggingEnabled)
 
+    actual fun applySettings(settings: FirebaseFirestoreSettings) {
+        native.settings = settings.ios
+    }
+
     actual suspend fun <T> runTransaction(func: suspend NativeTransaction.() -> T) =
         awaitResult<Any?> {
             native.runTransactionWithBlock(
@@ -46,10 +40,8 @@ internal actual class NativeFirebaseFirestoreWrapper internal actual constructor
 
     actual fun useEmulator(host: String, port: Int) {
         native.useEmulatorWithHost(host, port.toLong())
-        settings = firestoreSettings(settings) {
-            this.host = "$host:$port"
-            cacheSettings = memoryCacheSettings { }
-            sslEnabled = false
+        native.settings = native.settings.apply {
+            this.sslEnabled = false
         }
     }
 
diff --git a/firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt b/firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt
@@ -39,7 +39,9 @@ public actual fun Firebase.firestore(app: FirebaseApp): FirebaseFirestore =
 
 internal actual data class NativeFirebaseFirestore(val js: JsFirestore)
 
-public operator fun FirebaseFirestore.Companion.invoke(js: JsFirestore): FirebaseFirestore = FirebaseFirestore(NativeFirebaseFirestore(js))
+public operator fun FirebaseFirestore.Companion.invoke(js: JsFirestore): FirebaseFirestore = FirebaseFirestore(
+    NativeFirebaseFirestore(js),
+)
 public val FirebaseFirestore.js: JsFirestore get() = native.js
 
 public actual data class FirebaseFirestoreSettings(
diff --git a/firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt b/firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt
@@ -10,6 +10,7 @@ import dev.gitlive.firebase.firestore.NativeWriteBatch
 import dev.gitlive.firebase.firestore.externals.clearIndexedDbPersistence
 import dev.gitlive.firebase.firestore.externals.connectFirestoreEmulator
 import dev.gitlive.firebase.firestore.externals.doc
+import dev.gitlive.firebase.firestore.externals.getFirestore
 import dev.gitlive.firebase.firestore.externals.initializeFirestore
 import dev.gitlive.firebase.firestore.externals.setLogLevel
 import dev.gitlive.firebase.firestore.externals.writeBatch
@@ -21,38 +22,45 @@ import kotlinx.coroutines.await
 import kotlinx.coroutines.promise
 
 internal actual class NativeFirebaseFirestoreWrapper internal constructor(
-    private val createNative: NativeFirebaseFirestoreWrapper.() -> NativeFirebaseFirestore,
+    private val createNative: NativeFirebaseFirestoreWrapper.(FirebaseFirestoreSettings?) -> NativeFirebaseFirestore,
 ) {
 
-    internal actual constructor(native: NativeFirebaseFirestore) : this({ native })
+    internal actual constructor(native: NativeFirebaseFirestore) : this(
+        { settings ->
+            settings?.let {
+                NativeFirebaseFirestore(initializeFirestore(native.js.app, settings))
+            } ?: native
+        },
+    )
     internal constructor(app: FirebaseApp) : this(
-        {
+        { settings ->
             NativeFirebaseFirestore(
-                initializeFirestore(app, settings.js).also {
-                    emulatorSettings?.run {
-                        connectFirestoreEmulator(it, host, port)
+                settings?.let {
+                    initializeFirestore(app, it.js).also {
+                        emulatorSettings?.run {
+                            connectFirestoreEmulator(it, host, port)
+                        }
                     }
-                },
+                } ?: getFirestore(app),
             )
         },
     )
 
     private data class EmulatorSettings(val host: String, val port: Int)
 
-    actual var settings: FirebaseFirestoreSettings = FirebaseFirestoreSettings.Builder().build()
+    private var settings: FirebaseFirestoreSettings? = null
         set(value) {
             if (lazyNative.isInitialized()) {
-                throw IllegalStateException("FirebaseFirestore has already been started and its settings can no longer be changed. You can only call setFirestoreSettings() before calling any other methods on a FirebaseFirestore object.")
-            } else {
-                field = value
+                createNative(value) // Call initialize again to ensure native crash occurs if settings have changed
             }
+            field = value
         }
     private var emulatorSettings: EmulatorSettings? = null
 
     // initializeFirestore must be called before any call, including before `getFirestore()`
     // To allow settings to be updated, we defer creating the wrapper until the first call to `native`
     private val lazyNative = lazy {
-        createNative()
+        createNative(settings)
     }
     actual val native: NativeFirebaseFirestore by lazyNative
     private val js get() = native.js
@@ -89,6 +97,10 @@ internal actual class NativeFirebaseFirestoreWrapper internal constructor(
     actual fun setLoggingEnabled(loggingEnabled: Boolean) =
         rethrow { setLogLevel(if (loggingEnabled) "error" else "silent") }
 
+    actual fun applySettings(settings: FirebaseFirestoreSettings) {
+        this.settings = settings
+    }
+
     @OptIn(DelicateCoroutinesApi::class)
     actual suspend fun <T> runTransaction(func: suspend NativeTransaction.() -> T) =
         rethrow {
@@ -102,8 +114,10 @@ internal actual class NativeFirebaseFirestoreWrapper internal constructor(
         rethrow { clearIndexedDbPersistence(js).await() }
 
     actual fun useEmulator(host: String, port: Int) = rethrow {
-        settings = firestoreSettings(settings) {
-            this.host = "$host:$port"
+        if (settings != null) {
+            settings = firestoreSettings(settings) {
+                this.host = "$host:$port"
+            }
         }
         emulatorSettings = EmulatorSettings(host, port)
     }

Original file line number	Diff line number	Diff line change
`@@ -39,14 +39,14 @@ class FirestoreSourceTest {`
`39`	`39`	`)`
`40`	`40`
`41`	`41`	`firestore = Firebase.firestore(app).apply {`
`42`		`- useEmulator(emulatorHost, 8080)`
`43`		`- settings = firestoreSettings(settings) {`
	`42`	`+ settings = firestoreSettings {`
`44`	`43`	`cacheSettings = if (persistenceEnabled) {`
`45`	`44`	`persistentCacheSettings { }`
`46`	`45`	`} else {`
`47`	`46`	`memoryCacheSettings { }`
`48`	`47`	`}`
`49`	`48`	`}`
	`49`	`+ useEmulator(emulatorHost, 8080)`
`50`	`50`	`}`
`51`	`51`	`}`
`52`	`52`
Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,7 @@`
`5`	`5`	`package dev.gitlive.firebase.firestore`
`6`	`6`
`7`	`7`	`import dev.gitlive.firebase.Firebase`
	`8`	`+import dev.gitlive.firebase.FirebaseApp`
`8`	`9`	`import dev.gitlive.firebase.FirebaseOptions`
`9`	`10`	`import dev.gitlive.firebase.apps`
`10`	`11`	`import dev.gitlive.firebase.internal.decode`
`@@ -85,6 +86,7 @@ class FirebaseFirestoreTest {`
`85`	`86`	`)`
`86`	`87`	`}`
`87`	`88`
	`89`	`+ lateinit var firebaseApp: FirebaseApp`
`88`	`90`	`lateinit var firestore: FirebaseFirestore`
`89`	`91`
`90`	`92`	`@BeforeTest`
`@@ -100,14 +102,15 @@ class FirebaseFirestoreTest {`
`100`	`102`	`gcmSenderId = "846484016111",`
`101`	`103`	`),`
`102`	`104`	`)`
	`105`	`+ firebaseApp = app`
`103`	`106`
`104`	`107`	`firestore = Firebase.firestore(app).apply {`
`105`		`- useEmulator(emulatorHost, 8080)`
`106`		`- settings = firestoreSettings(settings) {`
	`108`	`+ settings = firestoreSettings {`
`107`	`109`	`cacheSettings = memoryCacheSettings {`
`108`	`110`	`gcSettings = memoryEagerGcSettings { }`
`109`	`111`	`}`
`110`	`112`	`}`
	`113`	`+ useEmulator(emulatorHost, 8080)`
`111`	`114`	`}`
`112`	`115`	`}`
`113`	`116`
`@@ -1031,6 +1034,12 @@ class FirebaseFirestoreTest {`
`1031`	`1034`	`fieldQuery.assertDocuments(FirestoreTest.serializer(), testOne)`
`1032`	`1035`	`}`
`1033`	`1036`
	`1037`	`+ @Test`
	`1038`	`+ fun testMultiple() = runTest {`
	`1039`	`+ Firebase.firestore(firebaseApp).disableNetwork()`
	`1040`	`+ Firebase.firestore(firebaseApp).enableNetwork()`
	`1041`	`+ }`
	`1042`	`+`
`1034`	`1043`	`private suspend fun setupFirestoreData(`
`1035`	`1044`	`documentOne: FirestoreTest = testOne,`
`1036`	`1045`	`documentTwo: FirestoreTest = testTwo,`