From ebb2f39e4d46c0455b25aba4cc272bb6f751bcd0 Mon Sep 17 00:00:00 2001 From: Grzegorz Orczykowski Date: Mon, 16 Dec 2024 00:39:14 +0100 Subject: [PATCH] Improved the Query class to avoid passing SQL --- .../entities/DatabaseEntitiesRepository.kt | 7 +- .../java/org/odk/collect/db/sqlite/Query.kt | 24 ++++- .../filter/LocalEntitiesFilterStrategy.kt | 19 ++-- .../filter/PullDataFunctionHandler.kt | 5 +- .../intance/LocalEntitiesInstanceAdapter.kt | 5 +- .../entities/storage/EntitiesRepository.kt | 4 +- .../storage/InMemEntitiesRepository.kt | 87 +++++++++---------- .../entities/LocalEntityUseCasesTest.kt | 9 +- 8 files changed, 84 insertions(+), 76 deletions(-) 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 97a85a440c9..66911743974 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 @@ -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): List { + override fun query(list: String, query: Query): List { 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( diff --git a/db/src/main/java/org/odk/collect/db/sqlite/Query.kt b/db/src/main/java/org/odk/collect/db/sqlite/Query.kt index 7b932fa686c..34983e03e75 100644 --- a/db/src/main/java/org/odk/collect/db/sqlite/Query.kt +++ b/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 -) +) { + 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 + ) +} diff --git a/entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt b/entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt index 9f61122dbc0..176eae509d5 100644 --- a/entities/src/main/java/org/odk/collect/entities/javarosa/filter/LocalEntitiesFilterStrategy.kt +++ b/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> ): List { 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 diff --git a/entities/src/main/java/org/odk/collect/entities/javarosa/filter/PullDataFunctionHandler.kt b/entities/src/main/java/org/odk/collect/entities/javarosa/filter/PullDataFunctionHandler.kt index 088a19f8fae..c07bd1f9048 100644 --- a/entities/src/main/java/org/odk/collect/entities/javarosa/filter/PullDataFunctionHandler.kt +++ b/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) ?: "" diff --git a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceAdapter.kt b/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceAdapter.kt index 9a834c3beb8..0b40ce33b52 100644 --- a/entities/src/main/java/org/odk/collect/entities/javarosa/intance/LocalEntitiesInstanceAdapter.kt +++ b/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): List { + fun query(list: String, query: Query): List { return entitiesRepository - .query(list, selection, selectionArgs) + .query(list, query) .map { convertToElement(it) } } diff --git a/entities/src/main/java/org/odk/collect/entities/storage/EntitiesRepository.kt b/entities/src/main/java/org/odk/collect/entities/storage/EntitiesRepository.kt index b0ef6ff9e0c..bdb92107149 100644 --- a/entities/src/main/java/org/odk/collect/entities/storage/EntitiesRepository.kt +++ b/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 @@ -8,7 +10,7 @@ interface EntitiesRepository { fun clear() fun addList(list: String) fun delete(id: String) - fun query(list: String, selection: String, selectionArgs: Array): List + fun query(list: String, query: Query): List fun getById(list: String, id: String): Entity.Saved? fun getByIdNot(list: String, id: String): List fun getByLabel(list: String, label: String?): List diff --git a/entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt b/entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt index 046eb83a234..58abc9a5032 100644 --- a/entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt +++ b/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() @@ -46,56 +48,47 @@ class InMemEntitiesRepository : EntitiesRepository { } } - override fun query( - list: String, - selection: String, - selectionArgs: Array - ): List { - 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 { + 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, operators: List): 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? { 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 a1b51fa0bf1..1e0c8438110 100644 --- a/entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt +++ b/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 - ): List { + override fun query(list: String, query: Query): List { accesses += 1 - return wrapped.query(list, selection, selectionArgs) + return wrapped.query(list, query) } override fun getById(list: String, id: String): Entity.Saved? {