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.

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

Fix "FirebaseFirestore has already been started" sync failures #2834

Merged
merged 3 commits into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,6 @@ abstract class RemotePersistenceModule {
return FirebaseStorage.getInstance().reference
}

@Provides @Singleton fun firebaseFuntions(): FirebaseFunctions = Firebase.functions
@Provides @Singleton fun firebaseFunctions(): FirebaseFunctions = Firebase.functions
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ import javax.inject.Singleton
@Singleton
class FirebaseFirestoreProvider @Inject constructor(settings: FirebaseFirestoreSettings) :
AsyncSingletonProvider<FirebaseFirestore>({
// WARNING: `FirebaseFirestore.getInstance()` should only be called here and nowhere
// else since settings can only be set on first call. Inject FirebaseFirestore instead
// of calling `getInstance()` directly.
FirebaseFirestore.getInstance().apply {
setFirestoreSettings(this, settings)
FirebaseFirestore.setLoggingEnabled(Config.FIRESTORE_LOGGING_ENABLED)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@
package com.google.android.ground.persistence.remote.firebase

import com.google.android.ground.persistence.uuid.OfflineUuidGenerator
import com.google.firebase.firestore.FirebaseFirestore
import javax.inject.Inject

class FirestoreUuidGenerator @Inject constructor() : OfflineUuidGenerator {
class FirestoreUuidGenerator
@Inject
constructor(private val firebaseFirestoreProvider: FirebaseFirestoreProvider) :
OfflineUuidGenerator {

// TODO: Check if this be replaced with the underlying implementation directly Util.autoId()
// Also remove Fake implementation if the dependency on firebase init is removed.
override fun generateUuid(): String =
FirebaseFirestore.getInstance().collection(ID_COLLECTION).document().id
override suspend fun generateUuid(): String =
firebaseFirestoreProvider.get().collection(ID_COLLECTION).document().id

companion object {
const val ID_COLLECTION = "/ids"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@ interface OfflineUuidGenerator {
* Implementations should ensure that the probability of collision is so small to be considered
* insignificant.
*/
fun generateUuid(): String
suspend fun generateUuid(): String
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,18 @@ constructor(
private val rootDir: File?
get() = context.getExternalFilesDir(Environment.DIRECTORY_PICTURES)

private fun createImageFilename(fieldId: String): String =
private suspend fun createImageFilename(fieldId: String): String =
fieldId + "-" + uuidGenerator.generateUuid() + Config.PHOTO_EXT

fun createImageFile(fieldId: String): File = File(rootDir, createImageFilename(fieldId))
suspend fun createImageFile(fieldId: String): File = File(rootDir, createImageFilename(fieldId))

/**
* Creates a new file from bitmap and saves under external app directory.
*
* @throws IOException If path is not accessible or error occurs while saving file
*/
@Throws(IOException::class)
fun savePhoto(bitmap: Bitmap, fieldId: String): File =
suspend fun savePhoto(bitmap: Bitmap, fieldId: String): File =
createImageFile(fieldId).apply {
FileOutputStream(this).use { fos -> bitmap.compress(Bitmap.CompressFormat.JPEG, 100, fos) }
Timber.d("Photo saved %s : %b", path, exists())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,8 @@ internal constructor(

/** Persists the changes locally and enqueues a worker to sync with remote datastore. */
private fun saveChanges(deltas: List<ValueDelta>) {
val collectionId = offlineUuidGenerator.generateUuid()
externalScope.launch(ioDispatcher) {
val collectionId = offlineUuidGenerator.generateUuid()
submitDataUseCase.invoke(loiId, job, surveyId, deltas, customLoiName, collectionId)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import androidx.compose.ui.platform.ComposeView
import androidx.compose.ui.res.stringResource
import androidx.core.content.FileProvider
import androidx.lifecycle.lifecycleScope
import com.google.android.ground.BuildConfig
import com.google.android.ground.R
import com.google.android.ground.coroutines.ApplicationScope
Expand Down Expand Up @@ -66,7 +67,7 @@
// Registers a callback to execute after a user captures a photo from the on-device camera.
private var capturePhotoLauncher: ActivityResultLauncher<Uri> =
registerForActivityResult(ActivityResultContracts.TakePicture()) { result: Boolean ->
if (result) viewModel.savePhotoTaskData(capturedPhotoUri)
externalScope.launch { if (result) viewModel.savePhotoTaskData(capturedPhotoUri) }
}

private var hasRequestedPermissionsOnResume = false
Expand Down Expand Up @@ -169,10 +170,12 @@
}

fun onTakePhoto() {
obtainCapturePhotoPermissions { launchPhotoCapture(viewModel.task.id) }
obtainCapturePhotoPermissions {
lifecycleScope.launch { launchPhotoCapture(viewModel.task.id) }
}

Check warning on line 175 in ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt#L173-L175

Added lines #L173 - L175 were not covered by tests
}

private fun launchPhotoCapture(taskId: String) {
private suspend fun launchPhotoCapture(taskId: String) {

Check warning on line 178 in ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskFragment.kt#L178

Added line #L178 was not covered by tests
try {
val photoFile = userMediaRepository.createImageFile(taskId)
capturedPhotoUri =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
* Saves photo data stored on an on-device URI in Ground-associated storage and prepares it for
* inclusion in a data collection submission.
*/
fun savePhotoTaskData(uri: Uri) {
suspend fun savePhotoTaskData(uri: Uri) {

Check warning on line 70 in ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt

View check run for this annotation

Codecov / codecov/patch

ground/src/main/java/com/google/android/ground/ui/datacollection/tasks/photo/PhotoTaskViewModel.kt#L70

Added line #L70 was not covered by tests
val currentTask = taskWaitingForPhoto
requireNotNull(currentTask) { "Photo captured but no task waiting for the result" }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.google.android.ground.ui.datacollection.tasks.point

import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.viewModelScope
import com.google.android.ground.model.geometry.Point
import com.google.android.ground.model.job.Job
import com.google.android.ground.model.job.getDefaultColor
Expand All @@ -29,6 +30,7 @@ import com.google.android.ground.ui.map.CameraPosition
import com.google.android.ground.ui.map.Feature
import com.google.android.ground.ui.map.FeatureType
import javax.inject.Inject
import kotlinx.coroutines.launch

class DropPinTaskViewModel
@Inject
Expand Down Expand Up @@ -66,13 +68,14 @@ constructor(
dropMarker(point)
}

private fun dropMarker(point: Point) {
val feature = createFeature(point)
features.postValue(setOf(feature))
}
private fun dropMarker(point: Point) =
viewModelScope.launch {
val feature = createFeature(point)
features.postValue(setOf(feature))
}

/** Creates a new map [Feature] representing the point placed by the user. */
private fun createFeature(point: Point): Feature =
private suspend fun createFeature(point: Point): Feature =
Feature(
id = uuidGenerator.generateUuid(),
type = FeatureType.USER_POINT.ordinal,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.stateIn
import kotlinx.coroutines.launch
import timber.log.Timber

@SharedViewModel
Expand Down Expand Up @@ -178,21 +179,22 @@ internal constructor(
}

/** Updates the [Feature] drawn on map based on the value of [vertices]. */
private fun refreshMap() {
_draftArea.value =
if (vertices.isEmpty()) {
null
} else {
Feature(
id = uuidGenerator.generateUuid(),
type = FeatureType.USER_POLYGON.ordinal,
geometry = LineString(vertices),
style = Feature.Style(strokeColor, Feature.VertexStyle.CIRCLE),
clusterable = false,
selected = true,
)
}
}
private fun refreshMap() =
viewModelScope.launch {
_draftArea.value =
if (vertices.isEmpty()) {
null
} else {
Feature(
id = uuidGenerator.generateUuid(),
type = FeatureType.USER_POLYGON.ordinal,
geometry = LineString(vertices),
style = Feature.Style(strokeColor, Feature.VertexStyle.CIRCLE),
clusterable = false,
selected = true,
)
}
}

override fun validate(task: Task, taskData: TaskData?): Int? {
// Invalid response for draw area task.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ class MediaUploadWorkerTest : BaseHiltTest() {
private suspend fun addSubmissionMutationToLocalStorage(status: Mutation.SyncStatus) =
localSubmissionStore.applyAndEnqueue(createSubmissionMutation().copy(syncStatus = status))

private fun createSubmissionMutation(photoName: String? = null): SubmissionMutation {
private suspend fun createSubmissionMutation(photoName: String? = null): SubmissionMutation {
val photo =
userMediaRepository.savePhoto(
Bitmap.createBitmap(1, 1, Bitmap.Config.HARDWARE),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import dagger.hilt.android.testing.HiltAndroidTest
import javax.inject.Inject
import kotlinx.collections.immutable.persistentListOf
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.test.advanceUntilIdle
import org.junit.Test
import org.junit.runner.RunWith
Expand All @@ -68,7 +69,7 @@ class DataCollectionFragmentTest : BaseHiltTest() {
@Inject lateinit var uuidGenerator: OfflineUuidGenerator
lateinit var collectionId: String

override fun setUp() {
override fun setUp() = runBlocking {
super.setUp()
collectionId = uuidGenerator.generateUuid()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ import com.google.android.ground.persistence.uuid.OfflineUuidGenerator
import javax.inject.Inject

class FakeUuidGenerator @Inject internal constructor() : OfflineUuidGenerator {
override fun generateUuid(): String = "TEST UUID"
override suspend fun generateUuid(): String = "TEST UUID"
}