Merge pull request #6890 from seadowg/row-number-crash

seadowg · web-flow · commit 9fa7ee8942ff · 2025-09-18T09:36:51.000+01:00
Ensure Cursors don't cause crashes by being processed after a close() call
diff --git a/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt b/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt
@@ -6,7 +6,6 @@ import android.database.Cursor
 import android.database.sqlite.SQLiteDatabase
 import android.database.sqlite.SQLiteException
 import android.provider.BaseColumns._ID
-import androidx.core.database.getLongOrNull
 import org.odk.collect.db.sqlite.CursorExt.first
 import org.odk.collect.db.sqlite.CursorExt.foldAndClose
 import org.odk.collect.db.sqlite.CursorExt.getBoolean
@@ -235,17 +234,22 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String, private val c
 
     private fun queryWithAttachedRowNumber(list: String, query: Query?): List<Entity.Saved> {
         try {
+            val cursorMapper = { cursor: Cursor ->
+                cursor.foldAndClose {
+                    mapCursorRowToEntity(it, it.getInt(ROW_NUMBER))
+                }
+            }
+
             return if (query == null) {
-                databaseConnection.rawQueryWithRowNumber(list)
+                databaseConnection.rawQueryWithRowNumber(list, cursorMapper = cursorMapper)
             } else {
                 val sqlQuery = query.toSql()
                 databaseConnection.rawQueryWithRowNumber(
                     list,
                     sqlQuery.selection,
-                    sqlQuery.selectionArgs
+                    sqlQuery.selectionArgs,
+                    cursorMapper = cursorMapper
                 )
-            }.foldAndClose {
-                mapCursorRowToEntity(it, it.getInt(ROW_NUMBER))
             }
         } catch (e: SQLiteException) {
             throw QueryException(e.message)
diff --git a/collect_app/src/test/java/org/odk/collect/android/database/DatabaseConnectionTest.kt b/collect_app/src/test/java/org/odk/collect/android/database/DatabaseConnectionTest.kt
diff --git a/db/src/main/java/org/odk/collect/db/sqlite/DatabaseConnection.kt b/db/src/main/java/org/odk/collect/db/sqlite/DatabaseConnection.kt
@@ -1,6 +1,7 @@
 package org.odk.collect.db.sqlite
 
 import android.content.Context
+import android.database.Cursor
 import android.database.sqlite.SQLiteDatabase
 import android.database.sqlite.SQLiteDatabase.CursorFactory
 import android.database.sqlite.SQLiteOpenHelper
@@ -91,7 +92,13 @@ open class DatabaseConnection @JvmOverloads constructor(
      */
     fun <T> withSynchronizedConnection(block: DatabaseConnection.() -> T): T {
         return synchronized(dbHelper) {
-            block(this)
+            val result = block(this)
+
+            if (result !is Cursor) {
+                result
+            } else {
+                throw IllegalStateException("Returning a Cursor removes synchronized guarantees!")
+            }
         }
     }
 
diff --git a/db/src/main/java/org/odk/collect/db/sqlite/RowNumbers.kt b/db/src/main/java/org/odk/collect/db/sqlite/RowNumbers.kt
@@ -5,12 +5,12 @@ import org.odk.collect.db.sqlite.SQLiteColumns.ROW_ID
 import org.odk.collect.db.sqlite.SQLiteColumns.ROW_NUMBER
 
 object RowNumbers {
-    fun SynchronizedDatabaseConnection.rawQueryWithRowNumber(table: String, selection: String? = null, selectionArgs: Array<String>? = null): Cursor {
+    fun <T> SynchronizedDatabaseConnection.rawQueryWithRowNumber(table: String, selection: String? = null, selectionArgs: Array<String>? = null, cursorMapper: (Cursor) -> T): T {
         this.ensureRowIdTable(table)
 
-        val cursor = if (selection != null) {
+        return if (selection != null) {
             this.withConnection {
-                readableDatabase
+                val cursor = readableDatabase
                     .rawQuery(
                         """
                         SELECT *, i.$ROW_ID as $ROW_NUMBER
@@ -20,10 +20,12 @@ object RowNumbers {
                         """.trimIndent(),
                         selectionArgs
                     )
+
+                cursorMapper(cursor)
             }
         } else {
             this.withConnection {
-                readableDatabase
+                val cursor = readableDatabase
                     .rawQuery(
                         """
                         SELECT *, i.$ROW_ID as $ROW_NUMBER
@@ -33,10 +35,10 @@ object RowNumbers {
                         """.trimIndent(),
                         null
                     )
+
+                cursorMapper(cursor)
             }
         }
-
-        return cursor
     }
 
     fun SynchronizedDatabaseConnection.invalidateRowNumbers(table: String) {
diff --git a/db/src/main/java/org/odk/collect/db/sqlite/SynchronizedDatabaseConnection.kt b/db/src/main/java/org/odk/collect/db/sqlite/SynchronizedDatabaseConnection.kt
@@ -24,8 +24,8 @@ class SynchronizedDatabaseConnection(
         return databaseConnection.withSynchronizedConnection(block)
     }
 
-    fun <T> transaction(
-        body: SQLiteDatabase.() -> T
+    fun transaction(
+        body: SQLiteDatabase.() -> Unit
     ) {
         return withConnection {
             writableDatabase.transaction {
@@ -38,8 +38,8 @@ class SynchronizedDatabaseConnection(
      * Runs a transaction and then calls [DatabaseConnection.reset]. Useful for transactions
      * that will mutate the DB schema.
      */
-    fun <T> resetTransaction(
-        body: SQLiteDatabase.() -> T
+    fun resetTransaction(
+        body: SQLiteDatabase.() -> Unit
     ) {
         transaction(body)
         databaseConnection.reset()
diff --git a/db/src/test/java/org/odk/collect/db/sqlite/DatabaseConnectionTest.kt b/db/src/test/java/org/odk/collect/db/sqlite/DatabaseConnectionTest.kt
@@ -0,0 +1,75 @@
+package org.odk.collect.db.sqlite
+
+import android.content.Context
+import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Assert.fail
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.odk.collect.db.sqlite.support.NoopMigrator
+import org.odk.collect.shared.TempFiles
+import java.io.File
+
+@RunWith(AndroidJUnit4::class)
+class DatabaseConnectionTest {
+
+    private val context = ApplicationProvider.getApplicationContext<Context>()
+
+    @Test
+    fun `#withSynchronizedConnection cannot return a Cursor`() {
+        val dbConnection = DatabaseConnection(
+            context,
+            TempFiles.createTempDir().absolutePath,
+            "temp.db",
+            NoopMigrator(),
+            1
+        )
+
+        dbConnection.writableDatabase.execSQL("CREATE TABLE blah (id integer);")
+
+        try {
+            dbConnection.withSynchronizedConnection {
+                readableDatabase.query(
+                    "blah",
+                    null,
+                    null,
+                    null,
+                    null,
+                    null,
+                    null
+                )
+            }
+
+            fail()
+        } catch (_: IllegalStateException) {
+            // Expected
+        }
+    }
+
+    // https://github.com/getodk/collect/issues/5042
+    @Test
+    fun `database file should be recreated if removed between operations`() {
+        val dbDir = TempFiles.createTempDir()
+        val dbFileName = "temp.db"
+        val dbPath = dbDir.absolutePath + File.separator + dbFileName
+
+        DatabaseConnection(
+            context,
+            dbDir.absolutePath,
+            dbFileName,
+            NoopMigrator(),
+            1
+        ).also {
+            it.readableDatabase
+            assertTrue(File(dbPath).exists())
+
+            File(dbPath).delete()
+            assertFalse(File(dbPath).exists())
+
+            it.readableDatabase
+            assertTrue(File(dbPath).exists())
+        }
+    }
+}
diff --git a/db/src/test/java/org/odk/collect/db/sqlite/RowNumbersTest.kt b/db/src/test/java/org/odk/collect/db/sqlite/RowNumbersTest.kt
@@ -2,7 +2,6 @@ package org.odk.collect.db.sqlite
 
 import android.content.ContentValues
 import android.content.Context
-import android.database.sqlite.SQLiteDatabase
 import android.provider.BaseColumns._ID
 import androidx.test.core.app.ApplicationProvider
 import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -15,6 +14,7 @@ import org.odk.collect.db.sqlite.CursorExt.rowToMap
 import org.odk.collect.db.sqlite.RowNumbers.invalidateRowNumbers
 import org.odk.collect.db.sqlite.RowNumbers.rawQueryWithRowNumber
 import org.odk.collect.db.sqlite.SQLiteColumns.ROW_NUMBER
+import org.odk.collect.db.sqlite.support.NoopMigrator
 import org.odk.collect.shared.TempFiles
 
 @RunWith(AndroidJUnit4::class)
@@ -41,8 +41,9 @@ class RowNumbersTest {
             insertOrThrow("test_table", null, ContentValues().also { it.put("position", "second") })
         }
 
-        val rows =
-            dbConnection.rawQueryWithRowNumber("test_table").foldAndClose { it.rowToMap() }
+        val rows = dbConnection.rawQueryWithRowNumber("test_table") { cursor ->
+            cursor.foldAndClose { it.rowToMap() }
+        }
         assertThat(rows.size, equalTo(2))
 
         assertThat(rows[0]["position"], equalTo("first"))
@@ -72,8 +73,9 @@ class RowNumbersTest {
             insertOrThrow("test_table", null, ContentValues().also { it.put("position", "third") })
         }
 
-        val beforeRows =
-            dbConnection.rawQueryWithRowNumber("test_table").foldAndClose { it.rowToMap() }
+        val beforeRows = dbConnection.rawQueryWithRowNumber("test_table") { cursor ->
+            cursor.foldAndClose { it.rowToMap() }
+        }
         assertThat(beforeRows.size, equalTo(3))
 
         dbConnection.transaction {
@@ -82,8 +84,9 @@ class RowNumbersTest {
 
         dbConnection.invalidateRowNumbers("test_table")
 
-        val afterRows =
-            dbConnection.rawQueryWithRowNumber("test_table").foldAndClose { it.rowToMap() }
+        val afterRows = dbConnection.rawQueryWithRowNumber("test_table") { cursor ->
+            cursor.foldAndClose { it.rowToMap() }
+        }
         assertThat(afterRows.size, equalTo(2))
 
         assertThat(afterRows[0]["position"], equalTo("first"))
@@ -93,8 +96,3 @@ class RowNumbersTest {
         assertThat(afterRows[1][ROW_NUMBER], equalTo("2"))
     }
 }
-
-private class NoopMigrator : DatabaseMigrator {
-    override fun onCreate(db: SQLiteDatabase?) {}
-    override fun onUpgrade(db: SQLiteDatabase?, oldVersion: Int) {}
-}
diff --git a/db/src/test/java/org/odk/collect/db/sqlite/support/NoopMigrator.kt b/db/src/test/java/org/odk/collect/db/sqlite/support/NoopMigrator.kt
@@ -0,0 +1,9 @@
+package org.odk.collect.db.sqlite.support
+
+import android.database.sqlite.SQLiteDatabase
+import org.odk.collect.db.sqlite.DatabaseMigrator
+
+class NoopMigrator : DatabaseMigrator {
+    override fun onCreate(db: SQLiteDatabase?) {}
+    override fun onUpgrade(db: SQLiteDatabase?, oldVersion: Int) {}
+}
