Improved the Query class to avoid passing SQL

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

@@ -12,6 +12,7 @@ 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.DatabaseMigrator
+import org.odk.collect.db.sqlite.Query
 import org.odk.collect.db.sqlite.SQLiteColumns.ROW_ID
 import org.odk.collect.db.sqlite.SQLiteDatabaseExt.delete
 import org.odk.collect.db.sqlite.SQLiteDatabaseExt.doesColumnExist
@@ -208,17 +209,17 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
         updateRowIdTables()
     }
 
-    override fun query(list: String, selection: String, selectionArgs: Array<String>): List<Entity.Saved> {
+    override fun query(list: String, query: Query): List<Entity.Saved> {
         return databaseConnection.withConnection {
             readableDatabase
                 .rawQuery(
                     """
                     SELECT *, i.$ROW_ID
                     FROM "$list" e, "${getRowIdTableName(list)}" i
-                    WHERE $selection
+                    WHERE ${query.selection}
                     ORDER BY i.$ROW_ID
                     """.trimIndent(),
-                    selectionArgs
+                    query.selectionArgs
                 )
         }.foldAndClose {
             mapCursorRowToEntity(

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

@@ -1,6 +1,26 @@
 package org.odk.collect.db.sqlite
 
-data class Query(
+sealed class Query(
     val selection: String,
     val selectionArgs: Array<String>
-)
+) {
+    class Eq(val column: String, val value: String) : Query(
+        "$column = ?",
+        arrayOf(value)
+    )
+
+    class NotEq(val column: String, val value: String) : Query(
+        "$column != ?",
+        arrayOf(value)
+    )
+
+    class And(val queryA: Query, val queryB: Query) : Query(
+        "(${queryA.selection} AND ${queryB.selection})",
+        queryA.selectionArgs + queryB.selectionArgs
+    )
+
+    class Or(val queryA: Query, val queryB: Query) : Query(
+        "(${queryA.selection} OR ${queryB.selection})",
+        queryA.selectionArgs + queryB.selectionArgs
+    )
+}

entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt

@@ -68,13 +68,11 @@ class LocalEntitiesFilterStrategy(entitiesRepository: EntitiesRepository) :
         val queryB = xPathExpressionToQuery(predicate.b, sourceInstance, evaluationContext)
 
         return if (queryA != null && queryB != null) {
-            val selection = if (predicate.op == XPathBoolExpr.AND) {
-                "(${queryA.selection} AND ${queryB.selection})"
+            if (predicate.op == XPathBoolExpr.AND) {
+                Query.And(queryA, queryB)
             } else {
-                "(${queryA.selection} OR ${queryB.selection})"
+                Query.Or(queryA, queryB)
             }
-
-            return Query(selection, arrayOf(*queryA.selectionArgs, *queryB.selectionArgs))
         } else {
             null
         }
@@ -91,14 +89,11 @@ class LocalEntitiesFilterStrategy(entitiesRepository: EntitiesRepository) :
             val child = candidate.nodeSide.steps[0].name.name
             val value = candidate.evalContextSide(sourceInstance, evaluationContext) as String
 
-            val selection = if (predicate.isEqual) {
-                "$child = ?"
+            if (predicate.isEqual) {
+                Query.Eq(child, value)
             } else {
-                "$child != ?"
+                Query.NotEq(child, value)
             }
-            val selectionArgs = arrayOf(value)
-
-            Query(selection, selectionArgs)
         } else {
             null
         }
@@ -110,7 +105,7 @@ class LocalEntitiesFilterStrategy(entitiesRepository: EntitiesRepository) :
         next: Supplier<MutableList<TreeReference>>
     ): List<TreeReference> {
         return if (query != null) {
-            val results = instanceAdapter.query(sourceInstance.instanceId, query.selection, query.selectionArgs)
+            val results = instanceAdapter.query(sourceInstance.instanceId, query)
             sourceInstance.replacePartialElements(results)
             results.map {
                 it.parent = sourceInstance.root

entities/src/main/java/org/odk/collect/entities/javarosa/filter/PullDataFunctionHandler.kt

@@ -3,6 +3,7 @@ package org.odk.collect.entities.javarosa.filter
 import org.javarosa.core.model.condition.EvaluationContext
 import org.javarosa.core.model.condition.IFunctionHandler
 import org.javarosa.xpath.expr.XPathFuncExpr
+import org.odk.collect.db.sqlite.Query
 import org.odk.collect.entities.javarosa.intance.LocalEntitiesInstanceAdapter
 import org.odk.collect.entities.storage.EntitiesRepository
 
@@ -37,9 +38,7 @@ class PullDataFunctionHandler(
             val filterChild = XPathFuncExpr.toString(args[2])
             val filterValue = XPathFuncExpr.toString(args[3])
 
-            val selection = "$filterChild = ?"
-            val selectionArgs = arrayOf(filterValue)
-            instanceAdapter.query(instanceId, selection, selectionArgs).firstOrNull()
+            instanceAdapter.query(instanceId, Query.Eq(filterChild, filterValue)).firstOrNull()
                 ?.getFirstChild(child)?.value?.value ?: ""
         } else {
             fallback?.eval(args, ec) ?: ""

entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceAdapter.kt

@@ -2,6 +2,7 @@ package org.odk.collect.entities.javarosa.intance
 
 import org.javarosa.core.model.data.StringData
 import org.javarosa.core.model.instance.TreeElement
+import org.odk.collect.db.sqlite.Query
 import org.odk.collect.entities.javarosa.parse.EntityItemElement
 import org.odk.collect.entities.storage.EntitiesRepository
 import org.odk.collect.entities.storage.Entity
@@ -38,9 +39,9 @@ class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepos
         }
     }
 
-    fun query(list: String, selection: String, selectionArgs: Array<String>): List<TreeElement> {
+    fun query(list: String, query: Query): List<TreeElement> {
         return entitiesRepository
-            .query(list, selection, selectionArgs)
+            .query(list, query)
             .map { convertToElement(it) }
     }
 

entities/src/main/java/org/odk/collect/entities/storage/EntitiesRepository.kt

@@ -1,5 +1,7 @@
 package org.odk.collect.entities.storage
 
+import org.odk.collect.db.sqlite.Query
+
 interface EntitiesRepository {
     fun save(list: String, vararg entities: Entity)
     fun getLists(): Set<String>
@@ -8,7 +10,7 @@ interface EntitiesRepository {
     fun clear()
     fun addList(list: String)
     fun delete(id: String)
-    fun query(list: String, selection: String, selectionArgs: Array<String>): List<Entity.Saved>
+    fun query(list: String, query: Query): List<Entity.Saved>
     fun getById(list: String, id: String): Entity.Saved?
     fun getByIdNot(list: String, id: String): List<Entity.Saved>
     fun getByLabel(list: String, label: String?): List<Entity.Saved>

entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt

@@ -1,5 +1,7 @@
 package org.odk.collect.entities.storage
 
+import org.odk.collect.db.sqlite.Query
+
 class InMemEntitiesRepository : EntitiesRepository {
 
     private val lists = mutableSetOf<String>()
@@ -46,56 +48,47 @@ class InMemEntitiesRepository : EntitiesRepository {
         }
     }
 
-    override fun query(
-        list: String,
-        selection: String,
-        selectionArgs: Array<String>
-    ): List<Entity.Saved> {
-        val conditions = selection.split("AND", "OR").map { it.trim() }
-        val operators = Regex("(AND|OR)").findAll(selection).map { it.value }.toList()
-
-        return getEntities(list).filter { entity ->
-            val results = conditions.mapIndexed { index, condition ->
-                val (fieldName, operator, _) = condition.split(" ").map { it }
-                val value = selectionArgs.getOrNull(index) ?: ""
-
-                evaluateCondition(entity, fieldName, operator, value)
+    override fun query(list: String, query: Query): List<Entity.Saved> {
+        val entities = getEntities(list)
+
+        return when (query) {
+            is Query.Eq -> {
+                entities.filter {
+                    val fieldName: String? = when (query.column) {
+                        "name" -> it.id
+                        "label" -> it.label
+                        "__version" -> it.version.toString()
+                        else -> it.properties.find { propertyName ->
+                            propertyName.first == query.column
+                        }?.second
+                    }
+                    fieldName == query.value
+                }
             }
-            combineResults(results, operators)
-        }
-    }
-
-    private fun evaluateCondition(
-        entity: Entity.Saved,
-        fieldName: String,
-        operator: String,
-        value: String
-    ): Boolean {
-        val fieldValue = when (fieldName) {
-            "name" -> entity.id
-            "label" -> entity.label
-            "__version" -> entity.version
-            else -> entity.properties.find { it.first == fieldName }?.second
-        }.toString()
-
-        return when (operator) {
-            "=" -> fieldValue == value
-            "!=" -> fieldValue != value
-            else -> false
-        }
-    }
-
-    private fun combineResults(results: List<Boolean>, operators: List<String>): Boolean {
-        var combinedResult = results.firstOrNull() ?: false
-
-        for (i in 1 until results.size) {
-            when (operators.getOrNull(i - 1)) {
-                "AND" -> combinedResult = combinedResult && results[i]
-                "OR" -> combinedResult = combinedResult || results[i]
+            is Query.NotEq -> {
+                entities.filter {
+                    val fieldName: String? = when (query.column) {
+                        "name" -> it.id
+                        "label" -> it.label
+                        "__version" -> it.version.toString()
+                        else -> it.properties.find { propertyName ->
+                            propertyName.first == query.column
+                        }?.second
+                    }
+                    fieldName != query.value
+                }
+            }
+            is Query.And -> {
+                val queryAResult = query(list, query.queryA)
+                val queryBResult = query(list, query.queryB)
+                queryAResult.intersect(queryBResult.toSet()).toList()
+            }
+            is Query.Or -> {
+                val queryAResult = query(list, query.queryA)
+                val queryBResult = query(list, query.queryB)
+                queryAResult.union(queryBResult.toSet()).toList()
             }
         }
-
-        return combinedResult
     }
 
     override fun getById(list: String, id: String): Entity.Saved? {

entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt

@@ -8,6 +8,7 @@ import org.hamcrest.Matchers.containsInAnyOrder
 import org.hamcrest.Matchers.equalTo
 import org.hamcrest.Matchers.not
 import org.junit.Test
+import org.odk.collect.db.sqlite.Query
 import org.odk.collect.entities.javarosa.finalization.EntitiesExtra
 import org.odk.collect.entities.javarosa.finalization.FormEntity
 import org.odk.collect.entities.javarosa.parse.EntityItemElement
@@ -541,13 +542,9 @@ private class MeasurableEntitiesRepository(private val wrapped: EntitiesReposito
         wrapped.delete(id)
     }
 
-    override fun query(
-        list: String,
-        selection: String,
-        selectionArgs: Array<String>
-    ): List<Entity.Saved> {
+    override fun query(list: String, query: Query): List<Entity.Saved> {
         accesses += 1
-        return wrapped.query(list, selection, selectionArgs)
+        return wrapped.query(list, query)
     }
 
     override fun getById(list: String, id: String): Entity.Saved? {