Improve Room transactions when downloading a survey

app/src/main/java/org/groundplatform/android/data/local/room/dao/BaseDao.kt

@@ -35,6 +35,12 @@ interface BaseDao<E> {
   @Delete suspend fun delete(entity: E)
 }
 
+/**
+ * Conservative SQLite variable limit. The actual limit is ~999, but 900 ensures
+ * compatibility across SQLite versions. Use when chunking IN clauses.
+ */
+const val MAX_SQL_VARIABLES = 900
+
 /** Try to update the specified entity, and if it doesn't yet exist, create it. Main-safe. */
 suspend fun <E> BaseDao<E>.insertOrUpdate(entity: E) {
   val count = update(entity)

app/src/main/java/org/groundplatform/android/data/local/room/dao/LocationOfInterestDao.kt

@@ -17,6 +17,7 @@ package org.groundplatform.android.data.local.room.dao
 
 import androidx.room.Dao
 import androidx.room.Query
+import androidx.room.Upsert
 import kotlinx.coroutines.flow.Flow
 import org.groundplatform.android.data.local.room.entity.LocationOfInterestEntity
 import org.groundplatform.android.data.local.room.fields.EntityDeletionState
@@ -25,6 +26,16 @@ import org.groundplatform.android.data.local.room.fields.EntityDeletionState
 @Dao
 interface LocationOfInterestDao : BaseDao<LocationOfInterestEntity> {
 
+  /** Inserts or updates all the given LOIs in a single transaction. */
+  @Upsert suspend fun upsertAll(entities: List<LocationOfInterestEntity>)
+
+  @Query("SELECT id FROM location_of_interest WHERE survey_id = :surveyId")
+  suspend fun getIds(surveyId: String): List<String>
+
+  /** Deletes the LOIs with the given IDs. Callers must respect [MAX_SQL_VARIABLES]. */
+  @Query("DELETE FROM location_of_interest WHERE id IN (:ids)")
+  suspend fun deleteByIds(ids: List<String>)
+
   @Query(
     "SELECT * FROM location_of_interest WHERE survey_id = :surveyId AND state = :deletionState"
   )
@@ -40,11 +51,4 @@ interface LocationOfInterestDao : BaseDao<LocationOfInterestEntity> {
 
   @Query("SELECT * FROM location_of_interest WHERE id = :id")
   suspend fun findById(id: String): LocationOfInterestEntity?
-
-  /**
-   * Deletes all LOIs in specified survey whose IDs are not present in the specified list..
-   * Main-safe.
-   */
-  @Query("DELETE FROM location_of_interest WHERE survey_id = :surveyId AND id NOT IN (:ids)")
-  suspend fun deleteNotIn(surveyId: String, ids: List<String>)
 }

app/src/main/java/org/groundplatform/android/data/local/room/stores/RoomLocationOfInterestStore.kt

@@ -15,15 +15,18 @@
  */
 package org.groundplatform.android.data.local.room.stores
 
+import androidx.room.withTransaction
 import javax.inject.Inject
 import javax.inject.Singleton
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.map
 import org.groundplatform.android.data.local.room.LocalDataStoreException
+import org.groundplatform.android.data.local.room.LocalDatabase
 import org.groundplatform.android.data.local.room.converter.toLocalDataStoreObject
 import org.groundplatform.android.data.local.room.converter.toModelObject
 import org.groundplatform.android.data.local.room.dao.LocationOfInterestDao
 import org.groundplatform.android.data.local.room.dao.LocationOfInterestMutationDao
+import org.groundplatform.android.data.local.room.dao.MAX_SQL_VARIABLES
 import org.groundplatform.android.data.local.room.dao.insertOrUpdate
 import org.groundplatform.android.data.local.room.entity.LocationOfInterestEntity
 import org.groundplatform.android.data.local.room.entity.LocationOfInterestMutationEntity
@@ -43,6 +46,7 @@ class RoomLocationOfInterestStore @Inject internal constructor() : LocalLocation
   @Inject lateinit var locationOfInterestDao: LocationOfInterestDao
   @Inject lateinit var locationOfInterestMutationDao: LocationOfInterestMutationDao
   @Inject lateinit var userStore: RoomUserStore
+  @Inject lateinit var localDatabase: LocalDatabase
 
   override suspend fun getLoiCount(surveyId: String): Int =
     locationOfInterestDao.countByDeletionState(surveyId, EntityDeletionState.DEFAULT)
@@ -131,6 +135,23 @@ class RoomLocationOfInterestStore @Inject internal constructor() : LocalLocation
     locationOfInterestDao.insertOrUpdate(loi.toLocalDataStoreObject())
   }
 
-  override suspend fun deleteNotIn(surveyId: String, ids: List<String>) =
-    locationOfInterestDao.deleteNotIn(surveyId, ids)
+  override suspend fun insertOrUpdateAll(lois: List<LocationOfInterest>) {
+    val entities =
+      lois.map {
+        require(!it.geometry.isEmpty()) { "Cannot save LOI ${it.id} with empty geometry" }
+        it.toLocalDataStoreObject()
+      }
+    locationOfInterestDao.upsertAll(entities)
+  }
+
+  override suspend fun deleteNotIn(surveyId: String, ids: List<String>) {
+    val idsToKeep = ids.toSet()
+    localDatabase.withTransaction {
+      locationOfInterestDao
+        .getIds(surveyId)
+        .filterNot { it in idsToKeep }
+        .chunked(MAX_SQL_VARIABLES)
+        .forEach { locationOfInterestDao.deleteByIds(it) }
+    }
+  }
 }

app/src/main/java/org/groundplatform/android/data/local/stores/LocalLocationOfInterestStore.kt

@@ -62,5 +62,8 @@ interface LocalLocationOfInterestStore :
 
   suspend fun insertOrUpdate(loi: LocationOfInterest)
 
+  /** Inserts or updates all the given LOIs in a single transaction. */
+  suspend fun insertOrUpdateAll(lois: List<LocationOfInterest>)
+
   suspend fun deleteNotIn(surveyId: String, ids: List<String>)
 }

app/src/main/java/org/groundplatform/android/repository/LocationOfInterestRepository.kt

@@ -99,22 +99,19 @@ constructor(
     lois: List<LocationOfInterest>,
     pendingLois: List<String>,
   ) {
-    // Insert new or update existing LOIs in local db.
-    lois.forEach { validateAndInsertOrUpdate(it) }
+    localLoiStore.insertOrUpdateAll(lois.onEach { validateGeometry(it) })
     // Delete LOIs in local db not returned in latest list from server, skipping pending mutations.
     localLoiStore.deleteNotIn(surveyId, lois.map { it.id } + pendingLois)
   }
 
   /**
-   * Validates LOI geometry before inserting or updating it in the local store. Throws
-   * IllegalArgumentException if the geometry has empty coordinates.
+   * Throws IllegalArgumentException if the LOI's geometry has empty coordinates, which would
+   * otherwise be persisted and later fail to render.
    */
-  private suspend fun validateAndInsertOrUpdate(loi: LocationOfInterest) {
+  private fun validateGeometry(loi: LocationOfInterest) {
     require(!loi.geometry.isEmpty()) {
       "Attempted to save LOI ${loi.id} with empty geometry. LOI: $loi"
     }
-
-    localLoiStore.insertOrUpdate(loi)
   }
 
   override suspend fun getOfflineLoi(surveyId: String, loiId: String): LocationOfInterest? {

app/src/test/java/org/groundplatform/android/data/local/LocalLocationOfInterestStoreTest.kt

@@ -20,6 +20,7 @@ import com.google.common.truth.Truth.assertThat
 import dagger.hilt.android.testing.HiltAndroidTest
 import javax.inject.Inject
 import kotlin.test.assertFailsWith
+import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.test.advanceUntilIdle
 import org.groundplatform.android.BaseHiltTest
 import org.groundplatform.android.FakeData
@@ -29,7 +30,9 @@ import org.groundplatform.android.data.local.room.converter.formatVertices
 import org.groundplatform.android.data.local.room.converter.parseVertices
 import org.groundplatform.android.data.local.room.converter.toLocalDataStoreObject
 import org.groundplatform.android.data.local.room.dao.LocationOfInterestDao
+import org.groundplatform.android.data.local.room.dao.MAX_SQL_VARIABLES
 import org.groundplatform.android.data.local.room.fields.EntityDeletionState
+import org.groundplatform.android.data.local.room.stores.RoomLocationOfInterestStore
 import org.groundplatform.android.data.local.stores.LocalLocationOfInterestStore
 import org.groundplatform.android.data.local.stores.LocalSubmissionStore
 import org.groundplatform.android.data.local.stores.LocalSurveyStore
@@ -45,6 +48,7 @@ import org.groundplatform.domain.model.geometry.Point
 import org.groundplatform.domain.model.geometry.Polygon
 import org.groundplatform.domain.model.job.Job
 import org.groundplatform.domain.model.job.Style
+import org.groundplatform.domain.model.locationofinterest.LocationOfInterest
 import org.groundplatform.domain.model.mutation.Mutation
 import org.groundplatform.domain.model.mutation.Mutation.SyncStatus
 import org.groundplatform.domain.model.mutation.SubmissionMutation
@@ -274,6 +278,104 @@ class LocalLocationOfInterestStoreTest : BaseHiltTest() {
       assertFailsWith<IllegalArgumentException> { localLoiStore.insertOrUpdate(invalidLoi) }
     }
 
+  @Test
+  fun `deleteNotIn deletes in chunks small enough for SQLite to bind`() = runWithTestDispatcher {
+    val ids = (1..MAX_SQL_VARIABLES * 2 + 1).map { "loi-$it" }
+    val chunkSizes = mutableListOf<Int>()
+
+    withDao(
+      object : LocationOfInterestDao by locationOfInterestDao {
+        override suspend fun getIds(surveyId: String) = ids
+
+        override suspend fun deleteByIds(ids: List<String>) {
+          chunkSizes.add(ids.size)
+        }
+      }
+    ) {
+      localLoiStore.deleteNotIn(TEST_SURVEY.id, emptyList())
+    }
+
+    assertThat(chunkSizes.sum()).isEqualTo(ids.size)
+    assertThat(chunkSizes.max()).isAtMost(MAX_SQL_VARIABLES)
+  }
+
+  @Test
+  fun `deleteNotIn deletes LOIs missing from the given list`() = runWithTestDispatcher {
+    localUserStore.insertOrUpdateUser(TEST_USER)
+    localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY)
+    localLoiStore.insertOrUpdate(testLoi("keep"))
+    localLoiStore.insertOrUpdate(testLoi("drop"))
+
+    localLoiStore.deleteNotIn(TEST_SURVEY.id, listOf("keep"))
+
+    assertThat(localLoiStore.getValidLois(TEST_SURVEY).first().map { it.id })
+      .containsExactly("keep")
+  }
+
+  @Test
+  fun `deleteNotIn leaves LOIs of other surveys untouched`() = runWithTestDispatcher {
+    localUserStore.insertOrUpdateUser(TEST_USER)
+    localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY)
+    localSurveyStore.insertOrUpdateSurvey(OTHER_SURVEY)
+    localLoiStore.insertOrUpdate(testLoi("mine"))
+    localLoiStore.insertOrUpdate(testLoi("theirs", surveyId = OTHER_SURVEY.id))
+
+    localLoiStore.deleteNotIn(TEST_SURVEY.id, emptyList())
+
+    assertThat(localLoiStore.getLoiCount(TEST_SURVEY.id)).isEqualTo(0)
+    assertThat(localLoiStore.getLoiCount(OTHER_SURVEY.id)).isEqualTo(1)
+  }
+
+  @Test
+  fun `insertOrUpdateAll inserts new LOIs and updates existing ones`() = runWithTestDispatcher {
+    localUserStore.insertOrUpdateUser(TEST_USER)
+    localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY)
+    localLoiStore.insertOrUpdateAll(listOf(testLoi("a"), testLoi("b")))
+
+    localLoiStore.insertOrUpdateAll(listOf(testLoi("b", customId = "updated"), testLoi("c")))
+
+    val lois = localLoiStore.getValidLois(TEST_SURVEY).first()
+    assertThat(lois.map { it.id }).containsExactly("a", "b", "c")
+    assertThat(lois.first { it.id == "b" }.customId).isEqualTo("updated")
+  }
+
+  @Test
+  fun `insertOrUpdateAll throws exception when any LOI has empty coordinates`() =
+    runWithTestDispatcher {
+      localUserStore.insertOrUpdateUser(TEST_USER)
+      localSurveyStore.insertOrUpdateSurvey(TEST_SURVEY)
+
+      val invalidLoi = testLoi("invalid").copy(geometry = Polygon(LinearRing(emptyList())))
+
+      assertFailsWith<IllegalArgumentException> {
+        localLoiStore.insertOrUpdateAll(listOf(testLoi("valid"), invalidLoi))
+      }
+    }
+
+  private suspend fun withDao(dao: LocationOfInterestDao, block: suspend () -> Unit) {
+    val store = localLoiStore as RoomLocationOfInterestStore
+    val real = store.locationOfInterestDao
+    store.locationOfInterestDao = dao
+    try {
+      block()
+    } finally {
+      store.locationOfInterestDao = real
+    }
+  }
+
+  private fun testLoi(
+    id: String,
+    surveyId: String = TEST_SURVEY.id,
+    customId: String = "",
+  ): LocationOfInterest =
+    FakeData.LOCATION_OF_INTEREST.copy(
+      id = id,
+      surveyId = surveyId,
+      customId = customId,
+      job = TEST_JOB,
+      geometry = TEST_POINT,
+    )
+
   companion object {
     private val TEST_USER = User(FakeData.USER_ID, "user@gmail.com", "user 1")
     private val TEST_TASK = Task("task id", 1, Task.Type.TEXT, "task label", false)
@@ -288,6 +390,14 @@ class LocalLocationOfInterestStoreTest : BaseHiltTest() {
         mapOf(Pair(TEST_JOB.id, TEST_JOB)),
         generalAccess = FAKE_GENERAL_ACCESS,
       )
+    private val OTHER_SURVEY =
+      Survey(
+        "other-survey-id",
+        "survey 2",
+        "bar description",
+        mapOf(Pair(TEST_JOB.id, TEST_JOB)),
+        generalAccess = FAKE_GENERAL_ACCESS,
+      )
     private val TEST_POINT = Point(Coordinates(110.0, -23.1))
     private val TEST_POINT_2 = Point(Coordinates(51.0, 44.0))
     private val TEST_POLYGON_1 =