Optimize more entity filter expressions

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

@@ -18,8 +18,12 @@ import org.odk.collect.db.sqlite.SQLiteDatabaseExt.doesColumnExist
 import org.odk.collect.db.sqlite.SQLiteDatabaseExt.getColumnNames
 import org.odk.collect.db.sqlite.SQLiteDatabaseExt.query
 import org.odk.collect.db.sqlite.SynchronizedDatabaseConnection
+import org.odk.collect.db.sqlite.toSql
+import org.odk.collect.entities.javarosa.parse.EntitySchema
 import org.odk.collect.entities.storage.EntitiesRepository
 import org.odk.collect.entities.storage.Entity
+import org.odk.collect.shared.Query
+import org.odk.collect.shared.mapColumns
 
 private object ListsTable {
     const val TABLE_NAME = "lists"
@@ -155,12 +159,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
             return emptyList()
         }
 
-        return queryWithAttachedRowId(list).foldAndClose {
-            mapCursorRowToEntity(
-                it,
-                it.getInt(ROW_ID)
-            )
-        }
+        return queryWithAttachedRowId(list, null)
     }
 
     override fun getCount(list: String): Int {
@@ -208,18 +207,27 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
         updateRowIdTables()
     }
 
+    override fun query(list: String, query: Query): List<Entity.Saved> {
+        if (!listExists(list)) {
+            return emptyList()
+        }
+
+        return queryWithAttachedRowId(list, query.mapColumns { columnName ->
+            when (columnName) {
+                EntitySchema.ID -> EntitiesTable.COLUMN_ID
+                EntitySchema.LABEL -> EntitiesTable.COLUMN_LABEL
+                EntitySchema.VERSION -> EntitiesTable.COLUMN_VERSION
+                else -> EntitiesTable.getPropertyColumn(columnName)
+            }
+        })
+    }
+
     override fun getById(list: String, id: String): Entity.Saved? {
         if (!listExists(list)) {
             return null
         }
 
-        return queryWithAttachedRowId(
-            list,
-            selectionColumn = EntitiesTable.COLUMN_ID,
-            selectionArg = id
-        ).first {
-            mapCursorRowToEntity(it, it.getInt(ROW_ID))
-        }
+        return queryWithAttachedRowId(list, Query.Eq(EntitiesTable.COLUMN_ID, id)).firstOrNull()
     }
 
     override fun getAllByProperty(
@@ -238,15 +246,10 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
         return if (propertyExists) {
             queryWithAttachedRowId(
                 list,
-                selectionColumn = EntitiesTable.getPropertyColumn(property),
-                selectionArg = value
-            ).foldAndClose {
-                mapCursorRowToEntity(it, it.getInt(ROW_ID))
-            }
+                Query.Eq(EntitiesTable.getPropertyColumn(property), value)
+            )
         } else if (value == "") {
-            queryWithAttachedRowId(list).foldAndClose {
-                mapCursorRowToEntity(it, it.getInt(ROW_ID))
-            }
+            queryWithAttachedRowId(list, null)
         } else {
             emptyList()
         }
@@ -257,51 +260,39 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
             return null
         }
 
-        return databaseConnection.withConnection {
-            readableDatabase
-                .rawQuery(
-                    """
-                    SELECT *, i.$ROW_ID
-                    FROM "$list" e, "${getRowIdTableName(list)}" i
-                    WHERE e._id = i._id AND i.$ROW_ID = ?
-                    """.trimIndent(),
-                    arrayOf((index + 1).toString())
-                ).first {
-                    mapCursorRowToEntity(it, it.getInt(ROW_ID))
-                }
-        }
+        return queryWithAttachedRowId(list, Query.Eq("i.$ROW_ID", (index + 1).toString())).firstOrNull()
     }
 
-    private fun queryWithAttachedRowId(list: String): Cursor {
-        return databaseConnection.withConnection {
-            readableDatabase
-                .rawQuery(
-                    """
-                    SELECT *, i.$ROW_ID
-                    FROM "$list" e, "${getRowIdTableName(list)}" i
-                    WHERE e._id = i._id
-                    ORDER BY i.$ROW_ID
-                    """.trimIndent(),
-                    null
-                )
-        }
-    }
-
-    private fun queryWithAttachedRowId(
-        list: String,
-        selectionColumn: String,
-        selectionArg: String
-    ): Cursor {
-        return databaseConnection.withConnection {
-            readableDatabase.rawQuery(
-                """
-                SELECT *, i.$ROW_ID
-                FROM "$list" e, "${getRowIdTableName(list)}" i
-                WHERE e._id = i._id AND $selectionColumn = ?
-                ORDER BY i.$ROW_ID
-                """.trimIndent(),
-                arrayOf(selectionArg)
-            )
+    private fun queryWithAttachedRowId(list: String, query: Query?): List<Entity.Saved> {
+        return if (query == null) {
+            databaseConnection.withConnection {
+                readableDatabase
+                    .rawQuery(
+                        """
+                        SELECT *, i.$ROW_ID
+                        FROM "$list" e, "${getRowIdTableName(list)}" i
+                        WHERE e._id = i._id
+                        ORDER BY i.$ROW_ID
+                        """.trimIndent(),
+                        null
+                    )
+            }
+        } else {
+            databaseConnection.withConnection {
+                val sqlQuery = query.toSql()
+                readableDatabase
+                    .rawQuery(
+                        """
+                        SELECT *, i.$ROW_ID
+                        FROM "$list" e, "${getRowIdTableName(list)}" i
+                        WHERE e._id = i._id AND ${sqlQuery.selection}
+                        ORDER BY i.$ROW_ID
+                        """.trimIndent(),
+                        sqlQuery.selectionArgs
+                    )
+            }
+        }.foldAndClose {
+            mapCursorRowToEntity(it, it.getInt(ROW_ID))
         }
     }
 

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

@@ -8,6 +8,7 @@ import org.junit.Test
 import org.odk.collect.android.entities.support.EntitySameAsMatcher.Companion.sameEntityAs
 import org.odk.collect.entities.storage.EntitiesRepository
 import org.odk.collect.entities.storage.Entity
+import org.odk.collect.shared.Query
 
 abstract class EntitiesRepositoryTest {
 
@@ -769,4 +770,80 @@ abstract class EntitiesRepositoryTest {
         assertThat(savedEntities[0].properties.size, equalTo(1))
         assertThat(savedEntities[0].properties[0].first, equalTo("prop"))
     }
+
+    @Test
+    fun `#query returns matching entities`() {
+        val repository = buildSubject()
+
+        val leoville = Entity.New(
+            "1",
+            "Léoville Barton 2008",
+            version = 1,
+            properties = listOf("vintage" to "2008")
+        )
+
+        val canet = Entity.New(
+            "2",
+            "Pontet-Canet 2014",
+            version = 2,
+            properties = listOf("vintage" to "2009")
+        )
+
+        repository.save("wines", leoville, canet)
+
+        val wines = repository.query("wines", Query.Eq("name", "2"))
+        assertThat(wines, containsInAnyOrder(sameEntityAs(canet)))
+    }
+
+    @Test
+    fun `#query returns empty list when there are no matches`() {
+        val repository = buildSubject()
+
+        val leoville = Entity.New(
+            "1",
+            "Léoville Barton 2008",
+            version = 1,
+            properties = listOf("vintage" to "2008")
+        )
+
+        repository.save("wines", leoville)
+
+        val wines = repository.query("wines", Query.Eq("name", "3"))
+        assertThat(wines, equalTo(emptyList()))
+    }
+
+    @Test
+    fun `#query returns empty list when there is a match in a different list`() {
+        val repository = buildSubject()
+
+        val leoville = Entity.New("1", "Léoville Barton 2008")
+        val ardbeg = Entity.New("2", "Ardbeg 10",)
+
+        repository.save("wines", leoville)
+        repository.save("whisky", ardbeg)
+
+        assertThat(repository.query("wines", Query.Eq("label", "Ardbeg 10")), equalTo(emptyList()))
+    }
+
+    @Test
+    fun `#query returns empty list where there are no entities in the list`() {
+        val repository = buildSubject()
+        assertThat(repository.query("wines", Query.Eq("label", "Léoville Barton 2008")), equalTo(emptyList()))
+    }
+
+    @Test
+    fun `#query supports list names with dots and dashes`() {
+        val repository = buildSubject()
+
+        val leoville = Entity.New("1", "Léoville Barton 2008")
+        val canet = Entity.New("2", "Pontet-Canet 2014")
+        repository.save("favourite-wines", leoville)
+        repository.save("other.favourite.wines", canet)
+
+        val queriedLeoville = repository.query("favourite-wines", Query.Eq("label", "Léoville Barton 2008"))
+        assertThat(queriedLeoville, containsInAnyOrder(sameEntityAs(leoville)))
+
+        val queriedCanet = repository.query("other.favourite.wines", Query.Eq("label", "Pontet-Canet 2014"))
+        assertThat(queriedCanet, containsInAnyOrder(sameEntityAs(canet)))
+    }
 }

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

@@ -0,0 +1,31 @@
+package org.odk.collect.db.sqlite
+
+import org.odk.collect.shared.Query
+
+data class SqlQuery(
+    val selection: String,
+    val selectionArgs: Array<String>
+)
+
+fun Query.toSql(): SqlQuery {
+    return when (this) {
+        is Query.Eq -> SqlQuery("$column = ?", arrayOf(value))
+        is Query.NotEq -> SqlQuery("$column != ?", arrayOf(value))
+        is Query.And -> {
+            val sqlA = queryA.toSql()
+            val sqlB = queryB.toSql()
+            SqlQuery(
+                "(${sqlA.selection} AND ${sqlB.selection})",
+                sqlA.selectionArgs + sqlB.selectionArgs
+            )
+        }
+        is Query.Or -> {
+            val sqlA = queryA.toSql()
+            val sqlB = queryB.toSql()
+            SqlQuery(
+                "(${sqlA.selection} OR ${sqlB.selection})",
+                sqlA.selectionArgs + sqlB.selectionArgs
+            )
+        }
+    }
+}

db/src/test/java/org/odk/collect/db/sqlite/SqlQueryTest.kt

@@ -0,0 +1,56 @@
+package org.odk.collect.db.sqlite
+
+import org.hamcrest.CoreMatchers.equalTo
+import org.hamcrest.MatcherAssert.assertThat
+import org.junit.Test
+import org.odk.collect.shared.Query
+
+class SqlQueryTest {
+    @Test
+    fun `Eq query generates correct selection and arguments`() {
+        val query = Query.Eq("name", "John").toSql()
+
+        assertThat(query.selection, equalTo("name = ?"))
+        assertThat(query.selectionArgs, equalTo(arrayOf("John")))
+    }
+
+    @Test
+    fun `NotEq query generates correct selection and arguments`() {
+        val query = Query.NotEq("age", "30").toSql()
+
+        assertThat(query.selection, equalTo("age != ?"))
+        assertThat(query.selectionArgs, equalTo(arrayOf("30")))
+    }
+
+    @Test
+    fun `And query generates correct selection and arguments`() {
+        val queryA = Query.Eq("name", "John")
+        val queryB = Query.NotEq("age", "30")
+        val combinedQuery = Query.And(queryA, queryB).toSql()
+
+        assertThat(combinedQuery.selection, equalTo("(name = ? AND age != ?)"))
+        assertThat(combinedQuery.selectionArgs, equalTo(arrayOf("John", "30")))
+    }
+
+    @Test
+    fun `Or query generates correct selection and arguments`() {
+        val queryA = Query.Eq("city", "New York")
+        val queryB = Query.NotEq("country", "Canada")
+        val combinedQuery = Query.Or(queryA, queryB).toSql()
+
+        assertThat(combinedQuery.selection, equalTo("(city = ? OR country != ?)"))
+        assertThat(combinedQuery.selectionArgs, equalTo(arrayOf("New York", "Canada")))
+    }
+
+    @Test
+    fun `nested And and Or queries generates correct selection and arguments`() {
+        val queryA = Query.Eq("status", "active")
+        val queryB = Query.NotEq("role", "admin")
+        val queryC = Query.Eq("team", "engineering")
+
+        val combinedQuery = Query.And(Query.Or(queryA, queryB), queryC).toSql()
+
+        assertThat(combinedQuery.selection, equalTo("((status = ? OR role != ?) AND team = ?)"))
+        assertThat(combinedQuery.selectionArgs, equalTo(arrayOf("active", "admin", "engineering")))
+    }
+}

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt

@@ -3,7 +3,7 @@ package org.odk.collect.entities
 import org.apache.commons.csv.CSVRecord
 import org.javarosa.core.model.instance.SecondaryInstanceCSVParserBuilder
 import org.odk.collect.entities.javarosa.finalization.EntitiesExtra
-import org.odk.collect.entities.javarosa.parse.EntityItemElement
+import org.odk.collect.entities.javarosa.parse.EntitySchema
 import org.odk.collect.entities.javarosa.spec.EntityAction
 import org.odk.collect.entities.storage.EntitiesRepository
 import org.odk.collect.entities.storage.Entity
@@ -125,9 +125,9 @@ object LocalEntityUseCases {
     private fun parseEntityFromRecord(record: CSVRecord): ServerEntity? {
         val map = record.toMap()
 
-        val id = map.remove(EntityItemElement.ID)
-        val label = map.remove(EntityItemElement.LABEL)
-        val version = map.remove(EntityItemElement.VERSION)?.toInt()
+        val id = map.remove(EntitySchema.ID)
+        val label = map.remove(EntitySchema.LABEL)
+        val version = map.remove(EntitySchema.VERSION)?.toInt()
         if (id == null || label == null || version == null) {
             return null
         }