diff --git a/collect_app/build.gradle b/collect_app/build.gradle index 8ffebae9bcc..ac3892c7da6 100644 --- a/collect_app/build.gradle +++ b/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\"") } } diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/benchmark/EntitiesBenchmarkTest.kt b/collect_app/src/androidTest/java/org/odk/collect/android/benchmark/EntitiesBenchmarkTest.kt index 17f7019f472..bdc9d039ab2 100644 --- a/collect_app/src/androidTest/java/org/odk/collect/android/benchmark/EntitiesBenchmarkTest.kt +++ b/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.cacheDir.deleteRecursively() - application.cacheDir.mkdir() - } -} - -private class Stopwatch { - - private val times = mutableMapOf() - - fun 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 , Y : Page> 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() - - fun 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.cacheDir.deleteRecursively() + application.cacheDir.mkdir() } diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/benchmark/SearchBenchmarkTest.kt b/collect_app/src/androidTest/java/org/odk/collect/android/benchmark/SearchBenchmarkTest.kt new file mode 100644 index 00000000000..59f5ab328b8 --- /dev/null +++ b/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() + } +} diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/benchmark/support/Benchmarker.kt b/collect_app/src/androidTest/java/org/odk/collect/android/benchmark/support/Benchmarker.kt new file mode 100644 index 00000000000..a6b74cd1090 --- /dev/null +++ b/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() + + fun 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 , Y : Page> 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() + + fun 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]!! + } +} 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 index 94918df4408..3d37a608506 100644 --- 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 @@ -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,10 +378,10 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep val properties = propertyColumns.fold(emptyList>()) { 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 @@ -388,14 +389,14 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep 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]!! ) } diff --git a/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt b/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt index 9080aef73b1..06f49c273c4 100644 --- a/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt +++ b/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() diff --git a/db/src/main/java/org/odk/collect/db/sqlite/CursorExt.kt b/db/src/main/java/org/odk/collect/db/sqlite/CursorExt.kt index 1003ad19b64..33e56d786ab 100644 --- a/db/src/main/java/org/odk/collect/db/sqlite/CursorExt.kt +++ b/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 { + return this.columnNames.foldIndexed(mutableMapOf()) { index, map, column -> + map[column] = this.getString(index) + map + } + } } diff --git a/entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt b/entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt index 9688a839cc7..2515d31b822 100644 --- a/entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt +++ b/entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt @@ -8,6 +8,7 @@ import org.odk.collect.entities.javarosa.spec.EntityAction import org.odk.collect.entities.storage.EntitiesRepository import org.odk.collect.entities.storage.Entity import java.io.File +import java.util.UUID object LocalEntityUseCases { @@ -26,7 +27,8 @@ object LocalEntityUseCases { id, formEntity.label, 1, - formEntity.properties + formEntity.properties, + branchId = UUID.randomUUID().toString() ) entitiesRepository.save(entity) @@ -72,7 +74,7 @@ object LocalEntityUseCases { val existing = missingFromServer.remove(serverEntity.id) if (existing == null || existing.version <= serverEntity.version) { - newAndUpdated.add(serverEntity) + newAndUpdated.add(serverEntity.copy(branchId = UUID.randomUUID().toString())) } else if (existing.state == Entity.State.OFFLINE) { val update = existing.copy(state = Entity.State.ONLINE) newAndUpdated.add(update) @@ -92,8 +94,8 @@ object LocalEntityUseCases { private fun parseEntityFromRecord( record: CSVRecord, list: String - ): Entity? { - val map = record.toMap().toMutableMap() + ): Entity.New? { + val map = record.toMap() val id = map.remove(EntityItemElement.ID) val label = map.remove(EntityItemElement.LABEL) @@ -102,16 +104,12 @@ object LocalEntityUseCases { return null } - val properties = map.entries.fold(emptyList>()) { properties, entry -> - properties + Pair(entry.key, entry.value) - } - return Entity.New( list, id, label, version, - properties, + map.toList(), state = Entity.State.ONLINE, trunkVersion = version ) diff --git a/entities/src/main/java/org/odk/collect/entities/storage/Entity.kt b/entities/src/main/java/org/odk/collect/entities/storage/Entity.kt index 66aeeee5158..6251ffede84 100644 --- a/entities/src/main/java/org/odk/collect/entities/storage/Entity.kt +++ b/entities/src/main/java/org/odk/collect/entities/storage/Entity.kt @@ -1,7 +1,5 @@ package org.odk.collect.entities.storage -import java.util.UUID - sealed interface Entity { val list: String val id: String @@ -20,7 +18,7 @@ sealed interface Entity { override val properties: List> = emptyList(), override val state: State = State.OFFLINE, override val trunkVersion: Int? = null, - override val branchId: String = UUID.randomUUID().toString() + override val branchId: String = "" ) : Entity data class Saved( @@ -32,7 +30,7 @@ sealed interface Entity { override val state: State = State.OFFLINE, val index: Int, override val trunkVersion: Int? = null, - override val branchId: String = UUID.randomUUID().toString() + override val branchId: String = "" ) : Entity enum class State { diff --git a/entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt b/entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt index cdebb5f87d9..7d2e195915e 100644 --- a/entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt +++ b/entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt @@ -142,6 +142,7 @@ class LocalEntityUseCasesTest { assertThat(songs[0].version, equalTo(2)) assertThat(songs[0].state, equalTo(Entity.State.ONLINE)) assertThat(songs[0].trunkVersion, equalTo(2)) + assertThat(songs[0].branchId, not(blankOrNullString())) assertThat(songs[0].branchId, not(equalTo(offline.branchId))) } @@ -183,6 +184,7 @@ class LocalEntityUseCasesTest { assertThat(songs[0].version, equalTo(2)) assertThat(songs[0].state, equalTo(Entity.State.ONLINE)) assertThat(songs[0].trunkVersion, equalTo(2)) + assertThat(songs[0].branchId, not(blankOrNullString())) assertThat(songs[0].branchId, not(equalTo(offline.branchId))) } @@ -204,6 +206,7 @@ class LocalEntityUseCasesTest { assertThat(songs[0].version, equalTo(3)) assertThat(songs[0].state, equalTo(Entity.State.ONLINE)) assertThat(songs[0].trunkVersion, equalTo(3)) + assertThat(songs[0].branchId, not(blankOrNullString())) assertThat(songs[0].branchId, not(equalTo(onlineBranched.branchId))) }