Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a crash for settings when iOS would access the same Firebase instance twice #562

Merged
merged 7 commits into from
Jul 11, 2024
Merged

Fixed a crash for settings when iOS would access the same Firebase instance twice #562

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a9086b8
Fixed a crash for settings when iOS would access the same Firebase in…
Daeda88 Jul 9, 2024
457926f
Linting
Daeda88 Jul 9, 2024
be243a9
Fix for #551
Daeda88 Jul 9, 2024
09c1e71
Properly retain setting on JS
Daeda88 Jul 9, 2024
fef2847
Fix for test
Daeda88 Jul 9, 2024
4be99fd
PR remarks: Replaced readable settings with write only method
Daeda88 Jul 10, 2024
c987bea
Settings as write only property
Daeda88 Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -85,6 +86,7 @@ class FirebaseFirestoreTest {
)
}

lateinit var firebaseApp: FirebaseApp
lateinit var firestore: FirebaseFirestore

@BeforeTest
Expand All @@ -100,6 +102,7 @@ class FirebaseFirestoreTest {
gcmSenderId = "846484016111",
),
)
firebaseApp = app

firestore = Firebase.firestore(app).apply {
useEmulator(emulatorHost, 8080)
Expand Down Expand Up @@ -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,
Expand Down
36 changes: 32 additions & 4 deletions .../iosMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,51 @@
package dev.gitlive.firebase.firestore.internal

import cocoapods.FirebaseFirestoreInternal.FIRFirestore
import cocoapods.FirebaseFirestoreInternal.FIRMemoryCacheSettings
import cocoapods.FirebaseFirestoreInternal.FIRPersistentCacheSettings
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.await
import dev.gitlive.firebase.firestore.awaitResult
import dev.gitlive.firebase.firestore.firestoreSettings
import dev.gitlive.firebase.firestore.memoryCacheSettings
import dev.gitlive.firebase.firestore.persistentCacheSettings
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
}
actual var settings: FirebaseFirestoreSettings
get() = firestoreSettings {
host = native.settings.host
sslEnabled = native.settings.sslEnabled
dispatchQueue = native.settings.dispatchQueue
@Suppress("SENSELESS_NULL_IN_WHEN")
cacheSettings = when (val nativeCacheSettings = native.settings.cacheSettings) {
is FIRPersistentCacheSettings -> persistentCacheSettings {
// SizeBytes cannot be determined
}
is FIRMemoryCacheSettings -> memoryCacheSettings {
// Garbage collection settings cannot be determined
}
null -> when {
native.settings.persistenceEnabled -> LocalCacheSettings.Persistent(native.settings.cacheSizeBytes)
native.settings.cacheSizeBytes == FirebaseFirestoreSettings.CACHE_SIZE_UNLIMITED -> LocalCacheSettings.Memory(
MemoryGarbageCollectorSettings.Eager,
)
else -> LocalCacheSettings.Memory(
MemoryGarbageCollectorSettings.LRUGC(
native.settings.cacheSizeBytes,
),
)
}
else -> error("Unknown cache settings $nativeCacheSettings")
}
}
set(value) {
field = value
native.settings = value.ios
}

Expand Down
24 changes: 19 additions & 5 deletions ...c/jsMain/kotlin/dev/gitlive/firebase/firestore/internal/NativeFirebaseFirestoreWrapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -20,28 +21,41 @@ import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.await
import kotlinx.coroutines.promise

// There is currently no way to check whether Firestore was already initialized for a given app without actually initializing it
// Therefore we keep track of this internally
private val appsWithFirestore = mutableListOf<FirebaseApp>()

Copy link
Member

@nbransby nbransby Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are we doing exactly that is straightforward on android but not supported natively on ios and js? The other option here would be to cater for the lowest common denominator, in other words don't support it on android either, but I'm still not exactly clear what the differences are?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are two things:

First and foremost, the biggest difference between the mobile platforms and JS is that for the mobile platforms, the FirebaseFirestore object internally keeps a state machine that allows it to change settings until it is first used. So, you can do:

firestore.settings.host = "1.1.1.1"
firestore.enableNetwork()
firstore.settings.host = "2.2.2.2" // This is not allowed

Javascript on the other hand has no such stateMachine with regards to settings. You can either get a FirebaseFirestore instance with whatever settings it was previously initialized, or you can initialize it with settings provided it has not been initialized before (there's no telling whether it is btw). The only one you can do is enable the emulator, which you can do after initialization but only before doing any operations.

This difference is the main reason for this major discrepancy. The approach of this library has always been to get the FirebaseFirestore object and change its settings before operations are executed. If you look at the setSettings method on JS pre 1.13.0, you'll realize that it would've crashed if you called the method as it calls the initializationCode after it has already gotten an initialized instance.

I guess it might be possible to essentially remove the state machine logic out of the library, so that you must provide the settings when getting a FirebaseFirestore instance, but this might have unknown side effects and is a breaking change for sure.

The second problem is smaller, and could be solved by omission: Settings on Android are fully readable, on iOS readable except for the cache settings, and on JS (due to initialization mentioned above) not readable at all. As of this PR, we get the settings on Android/iOS (though we miss a few cache settings on iOS) and we track it ourselves for JS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok well for the second issue I think removing the getter so you can't read settings on any platform is sensible.

As for the first issue, looking at https://firebase.google.com/docs/firestore/manage-data/enable-offline#configure_offline_persistence, I suggest the following:

  • make it only possible to set all the settings at once so firestore.settings.host = "1.1.1.1 shouldn't be possible (which will be the case once the getter is removed)
  • on JS, since Firebase.firestore.settings = firestoreSettings { ... } needs to call initializeFirestore before getFirestore is called can the js property be lazy so getFirestore is not called doing Firebase.firestore.settings = firestoreSettings { ... }
  • if the user attempts to set the settings after doing another operation which means getFirestore has already been called then let the js sdk (and others) throw the relevant error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed:

  • Removed settings property in favour of setSettings(builder) and applySettings(settings) methods. This is an api change compared to 1.13.0 but I think we're both fine with that.
  • The JS implementation now triggers "setting" the settings if the firestore object has been initialized. This should ensure you get a native crash if you try and set settings after already having initialized the firestore object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets keep the syntax matching the android sdk with a setter only property rather that two new functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not an officially supported feature, so it has some downsides as your link shows. Regardless, Ive implemented it as requested

internal actual class NativeFirebaseFirestoreWrapper internal constructor(
private val createNative: NativeFirebaseFirestoreWrapper.() -> NativeFirebaseFirestore,
private val canUpdateSettings: () -> Boolean,
) {

internal actual constructor(native: NativeFirebaseFirestore) : this({ native })
internal actual constructor(native: NativeFirebaseFirestore) : this({ native }, { false })
internal constructor(app: FirebaseApp) : this(
{
NativeFirebaseFirestore(
initializeFirestore(app, settings.js).also {
emulatorSettings?.run {
connectFirestoreEmulator(it, host, port)
if (appsWithFirestore.contains(app)) {
getFirestore(app)
} else {
initializeFirestore(app, settings.js).also {
emulatorSettings?.run {
connectFirestoreEmulator(it, host, port)
}
appsWithFirestore.add(app)
}
},
)
},
{
!appsWithFirestore.contains(app)
},
)

private data class EmulatorSettings(val host: String, val port: Int)

actual var settings: FirebaseFirestoreSettings = FirebaseFirestoreSettings.Builder().build()
set(value) {
if (lazyNative.isInitialized()) {
if (lazyNative.isInitialized() || !canUpdateSettings()) {
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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we removed this check in favour of letting the native sdk throw the exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that there is no native sdk to fall back to, as explained in the other remark. We shouldnt be calling initializeFirestore here yet, as settings may still be updated after this.

} else {
field = value
Expand Down
Loading