Optimize form download with entities

collect_app/build.gradle

@@ -65,6 +65,7 @@ def secrets = getSecrets()
 def googleMapsApiKey = secrets.getProperty('GOOGLE_MAPS_API_KEY', '')
 def mapboxAccessToken = secrets.getProperty('MAPBOX_ACCESS_TOKEN', '')
 def entitiesFilterTestProjectUrl = secrets.getProperty('ENTITIES_FILTER_TEST_PROJECT_URL', '')
+def entitiesFilterSearchTestProjectUrl = secrets.getProperty('ENTITIES_FILTER_SEARCH_TEST_PROJECT_URL', '')
 
 android {
     compileSdk Versions.android_compile_sdk
@@ -146,6 +147,7 @@ android {
             resValue("string", "GOOGLE_MAPS_API_KEY", googleMapsApiKey)
             resValue("string", "mapbox_access_token", mapboxAccessToken)
             buildConfigField("String", "ENTITIES_FILTER_TEST_PROJECT_URL", "\"$entitiesFilterTestProjectUrl\"")
+            buildConfigField("String", "ENTITIES_FILTER_SEARCH_TEST_PROJECT_URL", "\"$entitiesFilterSearchTestProjectUrl\"")
         }
     }
 

collect_app/src/androidTest/java/org/odk/collect/android/benchmark/EntitiesBenchmarkTest.kt

@@ -5,27 +5,25 @@ import androidx.test.core.app.ApplicationProvider
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import org.hamcrest.MatcherAssert.assertThat
 import org.hamcrest.Matchers.blankOrNullString
-import org.hamcrest.Matchers.lessThan
 import org.hamcrest.Matchers.not
 import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.RuleChain
 import org.junit.runner.RunWith
+import org.odk.collect.android.benchmark.support.Benchmarker
+import org.odk.collect.android.benchmark.support.benchmark
 import org.odk.collect.android.support.TestDependencies
 import org.odk.collect.android.support.pages.MainMenuPage
-import org.odk.collect.android.support.pages.Page
 import org.odk.collect.android.support.rules.CollectTestRule
 import org.odk.collect.android.support.rules.TestRuleChain.chain
 import org.odk.collect.android.test.BuildConfig.ENTITIES_FILTER_TEST_PROJECT_URL
-import org.odk.collect.shared.TimeInMs
 import org.odk.collect.strings.R
 
 /**
- * Benchmarks the performance of entity follow up forms. [PROJECT_URL] should be set to a project
- * that contains the "100k Entities Filter" form.
+ * Benchmarks the performance of entity follow up forms. [ENTITIES_FILTER_TEST_PROJECT_URL] should
+ * be set to a project that contains the "100k Entities Filter" form.
  *
  * Devices that currently pass:
- * - Pixel 4a
  * - Fairphone 3
  *
  */
@@ -68,13 +66,13 @@ class EntitiesBenchmarkTest {
             .clickOKOnDialog(MainMenuPage())
 
             .clickGetBlankForm()
-            .benchmark("Downloading form with http cache", 75, benchmarker) {
+            .benchmark("Downloading form with http cache", 25, benchmarker) {
                 it.clickGetSelected()
             }
 
             .clickOK(MainMenuPage())
             .clickGetBlankForm()
-            .benchmark("Downloading form second time with http cache", 90, benchmarker) {
+            .benchmark("Downloading form second time with http cache", 75, benchmarker) {
                 it.clickGetSelected()
             }
 
@@ -97,67 +95,10 @@ class EntitiesBenchmarkTest {
 
         benchmarker.assertResults()
     }
-
-    private fun clearAndroidCache() {
-        val application = ApplicationProvider.getApplicationContext<Application>()
-        application.cacheDir.deleteRecursively()
-        application.cacheDir.mkdir()
-    }
-}
-
-private class Stopwatch {
-
-    private val times = mutableMapOf<String, Long>()
-
-    fun <T> time(name: String, action: () -> T): T {
-        val startTime = System.currentTimeMillis()
-        val result = action()
-        val endTime = System.currentTimeMillis()
-
-        times[name] = (endTime - startTime) / TimeInMs.ONE_SECOND
-        return result
-    }
-
-    fun getTime(name: String): Long {
-        return times[name]!!
-    }
 }
 
-private fun <T : Page<T>, Y : Page<Y>> Y.benchmark(
-    name: String,
-    target: Long,
-    benchmarker: Benchmarker,
-    action: (Y) -> T
-): T {
-    return benchmarker.benchmark(name, target) {
-        action(this)
-    }
-}
-
-private class Benchmarker {
-    private val stopwatch = Stopwatch()
-    private val targets = mutableMapOf<String, Long>()
-
-    fun <T> benchmark(name: String, target: Long, action: () -> T): T {
-        targets[name] = target
-        return stopwatch.time(name) {
-            action()
-        }
-    }
-
-    fun assertResults() {
-        printResults()
-
-        targets.entries.forEach {
-            val time = stopwatch.getTime(it.key)
-            assertThat("\"${it.key}\" took ${time}s!", time, lessThan(it.value))
-        }
-    }
-
-    private fun printResults() {
-        println("Benchmark results:")
-        targets.keys.forEach {
-            println("$it: ${stopwatch.getTime(it)}s")
-        }
-    }
+private fun clearAndroidCache() {
+    val application = ApplicationProvider.getApplicationContext<Application>()
+    application.cacheDir.deleteRecursively()
+    application.cacheDir.mkdir()
 }

collect_app/src/androidTest/java/org/odk/collect/android/benchmark/SearchBenchmarkTest.kt

@@ -0,0 +1,73 @@
+package org.odk.collect.android.benchmark
+
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import org.hamcrest.MatcherAssert.assertThat
+import org.hamcrest.Matchers.blankOrNullString
+import org.hamcrest.Matchers.not
+import org.junit.Rule
+import org.junit.Test
+import org.junit.rules.RuleChain
+import org.junit.runner.RunWith
+import org.odk.collect.android.benchmark.support.Benchmarker
+import org.odk.collect.android.benchmark.support.benchmark
+import org.odk.collect.android.support.TestDependencies
+import org.odk.collect.android.support.pages.MainMenuPage
+import org.odk.collect.android.support.rules.CollectTestRule
+import org.odk.collect.android.support.rules.TestRuleChain.chain
+import org.odk.collect.android.test.BuildConfig.ENTITIES_FILTER_SEARCH_TEST_PROJECT_URL
+
+/**
+ * Benchmarks the performance of search() forms. [ENTITIES_FILTER_SEARCH_TEST_PROJECT_URL] should be
+ * set to a project that contains the "100k Entities Filter search()" form.
+ *
+ * Devices that currently pass:
+ * - Fairphone 3
+ *
+ */
+
+@RunWith(AndroidJUnit4::class)
+class SearchBenchmarkTest {
+
+    private val rule = CollectTestRule(useDemoProject = false)
+
+    @get:Rule
+    var chain: RuleChain = chain(TestDependencies(true)).around(rule)
+
+    @Test
+    fun run() {
+        assertThat(
+            "Need to set ENTITIES_FILTER_SEARCH_TEST_PROJECT_URL before running!",
+            ENTITIES_FILTER_SEARCH_TEST_PROJECT_URL,
+            not(blankOrNullString())
+        )
+
+        val benchmarker = Benchmarker()
+
+        rule.startAtFirstLaunch()
+            .clickManuallyEnterProjectDetails()
+            .inputUrl(ENTITIES_FILTER_SEARCH_TEST_PROJECT_URL)
+            .addProject()
+
+            .clickGetBlankForm()
+            .clickGetSelected()
+            .clickOK(MainMenuPage())
+
+            .clickFillBlankForm()
+            .benchmark("Loading form first time", 20, benchmarker) {
+                it.clickOnForm("100k Entities Filter search()")
+            }
+
+            .pressBackAndDiscardForm()
+            .clickFillBlankForm()
+            .benchmark("Loading form second time", 2, benchmarker) {
+                it.clickOnForm("100k Entities Filter search()")
+            }
+
+            .answerQuestion("Which value do you want to filter by?", "1024")
+            .benchmark("Filtering select", 2, benchmarker) {
+                it.swipeToNextQuestion("Filtered select")
+            }
+
+        benchmarker.assertResults()
+    }
+}

collect_app/src/androidTest/java/org/odk/collect/android/benchmark/support/Benchmarker.kt

@@ -0,0 +1,63 @@
+package org.odk.collect.android.benchmark.support
+
+import org.hamcrest.MatcherAssert.assertThat
+import org.hamcrest.Matchers.lessThan
+import org.odk.collect.android.support.pages.Page
+import org.odk.collect.shared.TimeInMs
+
+class Benchmarker {
+    private val stopwatch = Stopwatch()
+    private val targets = mutableMapOf<String, Long>()
+
+    fun <T> benchmark(name: String, target: Long, action: () -> T): T {
+        targets[name] = target
+        return stopwatch.time(name) {
+            action()
+        }
+    }
+
+    fun assertResults() {
+        printResults()
+
+        targets.entries.forEach {
+            val time = stopwatch.getTime(it.key)
+            assertThat("\"${it.key}\" took ${time}s!", time, lessThan(it.value))
+        }
+    }
+
+    private fun printResults() {
+        println("Benchmark results:")
+        targets.keys.forEach {
+            println("$it: ${stopwatch.getTime(it)}s")
+        }
+    }
+}
+
+fun <T : Page<T>, Y : Page<Y>> Y.benchmark(
+    name: String,
+    target: Long,
+    benchmarker: Benchmarker,
+    action: (Y) -> T
+): T {
+    return benchmarker.benchmark(name, target) {
+        action(this)
+    }
+}
+
+private class Stopwatch {
+
+    private val times = mutableMapOf<String, Long>()
+
+    fun <T> time(name: String, action: () -> T): T {
+        val startTime = System.currentTimeMillis()
+        val result = action()
+        val endTime = System.currentTimeMillis()
+
+        times[name] = (endTime - startTime) / TimeInMs.ONE_SECOND
+        return result
+    }
+
+    fun getTime(name: String): Long {
+        return times[name]!!
+    }
+}

collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt

@@ -10,9 +10,8 @@ import androidx.core.database.sqlite.transaction
 import org.odk.collect.db.sqlite.CursorExt.first
 import org.odk.collect.db.sqlite.CursorExt.foldAndClose
 import org.odk.collect.db.sqlite.CursorExt.getInt
-import org.odk.collect.db.sqlite.CursorExt.getIntOrNull
 import org.odk.collect.db.sqlite.CursorExt.getString
-import org.odk.collect.db.sqlite.CursorExt.getStringOrNull
+import org.odk.collect.db.sqlite.CursorExt.rowToMap
 import org.odk.collect.db.sqlite.DatabaseConnection
 import org.odk.collect.db.sqlite.DatabaseMigrator
 import org.odk.collect.db.sqlite.SQLiteColumns.ROW_ID
@@ -111,7 +110,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
                         it.put(EntitiesTable.COLUMN_STATE, convertStateToInt(entity.state))
 
                         entity.properties.forEach { (name, value) ->
-                            it.put(name, value)
+                            it.put("\"$name\"", value)
                         }
                     }
 
@@ -348,7 +347,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
             try {
                 databaseConnection.writeableDatabase.execSQL(
                     """
-                    ALTER TABLE ${entity.list} ADD $it text NOT NULL DEFAULT "";
+                    ALTER TABLE ${entity.list} ADD "$it" text NOT NULL DEFAULT "";
                     """.trimIndent()
                 )
             } catch (e: SQLiteException) {
@@ -362,7 +361,9 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
         cursor: Cursor,
         rowId: Int
     ): Entity.Saved {
-        val propertyColumns = cursor.columnNames.filter {
+        val map = cursor.rowToMap()
+
+        val propertyColumns = map.keys.filter {
             !listOf(
                 _ID,
                 EntitiesTable.COLUMN_ID,
@@ -377,25 +378,25 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
 
         val properties =
             propertyColumns.fold(emptyList<Pair<String, String>>()) { accum, property ->
-                accum + Pair(property, cursor.getStringOrNull(property) ?: "")
+                accum + Pair(property, map[property] ?: "")
             }
 
-        val state = if (cursor.getInt(EntitiesTable.COLUMN_STATE) == 0) {
+        val state = if (map[EntitiesTable.COLUMN_STATE]!!.toInt() == 0) {
             Entity.State.OFFLINE
         } else {
             Entity.State.ONLINE
         }
 
         return Entity.Saved(
             list,
-            cursor.getString(EntitiesTable.COLUMN_ID),
-            cursor.getStringOrNull(EntitiesTable.COLUMN_LABEL),
-            cursor.getInt(EntitiesTable.COLUMN_VERSION),
+            map[EntitiesTable.COLUMN_ID]!!,
+            map[EntitiesTable.COLUMN_LABEL],
+            map[EntitiesTable.COLUMN_VERSION]!!.toInt(),
             properties,
             state,
             rowId - 1,
-            cursor.getIntOrNull(EntitiesTable.COLUMN_TRUNK_VERSION),
-            cursor.getString(EntitiesTable.COLUMN_BRANCH_ID)
+            map[EntitiesTable.COLUMN_TRUNK_VERSION]?.toInt(),
+            map[EntitiesTable.COLUMN_BRANCH_ID]!!
         )
     }
 

collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt

@@ -262,6 +262,19 @@ abstract class EntitiesRepositoryTest {
         assertThat(repository.getLists(), containsInAnyOrder("wines", "blah"))
     }
 
+    @Test
+    fun `#save supports properties with dots and dashes`() {
+        val repository = buildSubject()
+        val entity = Entity.New(
+            "things",
+            "1",
+            "One",
+            properties = listOf(Pair("a.property", "value"), Pair("a-property", "value"))
+        )
+        repository.save(entity)
+        assertThat(repository.getEntities("things")[0], sameEntityAs(entity))
+    }
+
     @Test
     fun `#clear deletes all entities`() {
         val repository = buildSubject()

db/src/main/java/org/odk/collect/db/sqlite/CursorExt.kt

@@ -65,4 +65,17 @@ object CursorExt {
         val columnIndex = this.getColumnIndex(column)
         return this.getIntOrNull(columnIndex)
     }
+
+    /**
+     * Prevents doing repeated [Cursor.getColumnIndex] lookups and also works around the lack of
+     * support for column names including a "." there (due to the mysterious bug 903852).
+     *
+     * @see [SQLiteCursor source](https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/database/sqlite/SQLiteCursor.java;l=178?q=sqlitecursor)
+     */
+    fun Cursor.rowToMap(): Map<String, String> {
+        return this.columnNames.foldIndexed(mutableMapOf()) { index, map, column ->
+            map[column] = this.getString(index)
+            map
+        }
+    }
 }