Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix property eq expressions for entities #6351

Merged
merged 8 commits into from
Aug 22, 2024
Merged

Fix property eq expressions for entities #6351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fb5d51e
Handle cases where entities are saved with different properties
seadowg Aug 15, 2024
96fa1b1
Support expressions that query against non-existent properties
seadowg Aug 15, 2024
8c5d1a0
Create new module for db
seadowg Aug 16, 2024
d443e52
Move SQLite shared code to db
seadowg Aug 16, 2024
51d3999
Move doesColumnExist to SQLiteUtils
seadowg Aug 16, 2024
9c0b3d9
Remove unneeded dependency
seadowg Aug 22, 2024
34b3427
Improve test
seadowg Aug 22, 2024
4f70dd6
Optimize imports
seadowg Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions collect_app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ dependencies {
implementation project(':printer')
implementation project(':lists')
implementation project(':web-page')
implementation project(':db')

if (getSecrets().getProperty('MAPBOX_DOWNLOADS_TOKEN', '') != '') {
implementation project(':mapbox')
Expand Down
2 changes: 1 addition & 1 deletion ...t/java/org/odk/collect/android/instrumented/utilities/CustomSQLiteQueryExecutionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import org.junit.runner.RunWith;
import org.odk.collect.android.support.rules.RunnableRule;
import org.odk.collect.android.support.rules.TestRuleChain;
import org.odk.collect.androidshared.sqlite.CustomSQLiteQueryExecutor;
import org.odk.collect.db.sqlite.CustomSQLiteQueryExecutor;

import java.io.File;
import java.io.IOException;
Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/androidTest/java/org/odk/collect/android/support/rules/ResetStateRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import org.odk.collect.android.injection.config.AppDependencyComponent
import org.odk.collect.android.injection.config.AppDependencyModule
import org.odk.collect.android.support.CollectHelpers
import org.odk.collect.android.views.DecoratedBarcodeView
import org.odk.collect.androidshared.sqlite.DatabaseConnection
import org.odk.collect.androidshared.ui.ToastUtils
import org.odk.collect.androidshared.ui.multiclicksafe.MultiClickGuard
import org.odk.collect.db.sqlite.DatabaseConnection
import org.odk.collect.material.BottomSheetBehavior
import org.odk.collect.shared.files.DirectoryUtils
import java.io.IOException
Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/main/java/org/odk/collect/android/activities/InstanceChooserList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
import org.odk.collect.android.R;
import org.odk.collect.android.adapters.InstanceListCursorAdapter;
import org.odk.collect.android.dao.CursorLoaderFactory;
import org.odk.collect.androidshared.sqlite.DatabaseConnection;
import org.odk.collect.db.sqlite.DatabaseConnection;
import org.odk.collect.android.database.instances.DatabaseInstanceColumns;
import org.odk.collect.android.entities.EntitiesRepositoryProvider;
import org.odk.collect.android.external.FormUriActivity;
Expand Down
45 changes: 27 additions & 18 deletions ...app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,18 @@ import android.database.sqlite.SQLiteDatabase
import android.database.sqlite.SQLiteException
import android.provider.BaseColumns._ID
import androidx.core.database.sqlite.transaction
import org.odk.collect.androidshared.sqlite.CursorExt.first
import org.odk.collect.androidshared.sqlite.CursorExt.foldAndClose
import org.odk.collect.androidshared.sqlite.CursorExt.getInt
import org.odk.collect.androidshared.sqlite.CursorExt.getIntOrNull
import org.odk.collect.androidshared.sqlite.CursorExt.getString
import org.odk.collect.androidshared.sqlite.CursorExt.getStringOrNull
import org.odk.collect.androidshared.sqlite.DatabaseConnection
import org.odk.collect.androidshared.sqlite.DatabaseMigrator
import org.odk.collect.androidshared.sqlite.SQLiteColumns.ROW_ID
import org.odk.collect.androidshared.sqlite.SQLiteDatabaseExt.delete
import org.odk.collect.androidshared.sqlite.SQLiteDatabaseExt.query
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.DatabaseConnection
import org.odk.collect.db.sqlite.DatabaseMigrator
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
import org.odk.collect.db.sqlite.SQLiteDatabaseExt.query
import org.odk.collect.entities.storage.EntitiesRepository
import org.odk.collect.entities.storage.Entity

Expand Down Expand Up @@ -213,12 +214,20 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
return emptyList()
}

return queryWithAttachedRowId(
list,
selectionColumn = property,
selectionArg = value
).foldAndClose {
mapCursorRowToEntity(list, it, it.getInt(ROW_ID))
return if (databaseConnection.readableDatabase.doesColumnExist(list, property)) {
queryWithAttachedRowId(
list,
selectionColumn = property,
selectionArg = value
).foldAndClose {
mapCursorRowToEntity(list, it, it.getInt(ROW_ID))
}
} else if (value == "") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can properties have blank values like " "? Is it sanitized somewhere? I'm wondering if here we should use value.isBlank() instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case only occurs if the property doesn't actually exist for the entities. It's the special case described above:

It's interestingly up for debate whether the behaviour for 2 is correct or not though in the case where thing is not a node that exists on the filtered nodes: JavaRosa would return all nodes, whereas Enketo would return none.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case only occurs if the property doesn't actually exist for the entities.

I see but isn't it somehow possible that it doesn't exist and the value here is not "" but " "?

Copy link
Member Author

@seadowg seadowg Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see but isn't it somehow possible that it doesn't exist and the value here is not "" but " "?

Yes, but in that case we'd return an empty list because we'd be searching for nodes with a non-existent property equal to "something" rather than "nothing" and JavaRosa treats non-existent nodes as having the value "". Basically thing = ' ' is the same as thing = 'blah' if thing doesn't exist.

queryWithAttachedRowId(list).foldAndClose {
mapCursorRowToEntity(list, it, it.getInt(ROW_ID))
}
} else {
emptyList()
}
}

Expand Down Expand Up @@ -339,7 +348,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep
try {
databaseConnection.writeableDatabase.execSQL(
"""
ALTER TABLE ${entity.list} ADD $it text;
ALTER TABLE ${entity.list} ADD $it text NOT NULL DEFAULT "";
""".trimIndent()
)
} catch (e: SQLiteException) {
Expand Down
2 changes: 1 addition & 1 deletion ...ect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import android.os.StrictMode;

import org.jetbrains.annotations.NotNull;
import org.odk.collect.androidshared.sqlite.DatabaseConnection;
import org.odk.collect.db.sqlite.DatabaseConnection;
import org.odk.collect.android.database.DatabaseConstants;
import org.odk.collect.android.utilities.FileUtils;
import org.odk.collect.forms.Form;
Expand Down
4 changes: 2 additions & 2 deletions collect_app/src/main/java/org/odk/collect/android/database/forms/FormDatabaseMigrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import android.database.SQLException;
import android.database.sqlite.SQLiteDatabase;

import org.odk.collect.androidshared.sqlite.DatabaseMigrator;
import org.odk.collect.androidshared.sqlite.SQLiteUtils;
import org.odk.collect.db.sqlite.DatabaseMigrator;
import org.odk.collect.db.sqlite.SQLiteUtils;

import static android.provider.BaseColumns._ID;
import static org.odk.collect.android.database.DatabaseConstants.FORMS_TABLE_NAME;
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import android.database.sqlite.SQLiteQueryBuilder;
import android.os.StrictMode;

import org.odk.collect.androidshared.sqlite.DatabaseConnection;
import org.odk.collect.db.sqlite.DatabaseConnection;
import org.odk.collect.android.database.DatabaseConstants;
import org.odk.collect.forms.instances.Instance;
import org.odk.collect.forms.instances.InstancesRepository;
Expand Down
7 changes: 4 additions & 3 deletions ...pp/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
import static org.odk.collect.android.database.instances.DatabaseInstanceColumns.LAST_STATUS_CHANGE_DATE;
import static org.odk.collect.android.database.instances.DatabaseInstanceColumns.STATUS;
import static org.odk.collect.android.database.instances.DatabaseInstanceColumns.SUBMISSION_URI;
import static org.odk.collect.db.sqlite.SQLiteDatabaseExt.doesColumnExist;

import android.database.sqlite.SQLiteDatabase;

import org.odk.collect.androidshared.sqlite.DatabaseMigrator;
import org.odk.collect.androidshared.sqlite.SQLiteUtils;
import org.odk.collect.db.sqlite.DatabaseMigrator;
import org.odk.collect.db.sqlite.SQLiteUtils;
import org.odk.collect.forms.instances.Instance;

import java.util.Arrays;
Expand Down Expand Up @@ -71,7 +72,7 @@ public void onDowngrade(SQLiteDatabase db) {
}

private void upgradeToVersion2(SQLiteDatabase db) {
if (!SQLiteUtils.doesColumnExist(db, INSTANCES_TABLE_NAME, CAN_EDIT_WHEN_COMPLETE)) {
if (!doesColumnExist(db, INSTANCES_TABLE_NAME, CAN_EDIT_WHEN_COMPLETE)) {
SQLiteUtils.addColumn(db, INSTANCES_TABLE_NAME, CAN_EDIT_WHEN_COMPLETE, "text");

db.execSQL("UPDATE " + INSTANCES_TABLE_NAME + " SET "
Expand Down
8 changes: 4 additions & 4 deletions ...src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_DATABASE_VE
import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_TABLE_NAME
import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.FORM_DB_ID
import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.INSTANCE_DB_ID
import org.odk.collect.androidshared.sqlite.CursorExt.foldAndClose
import org.odk.collect.androidshared.sqlite.DatabaseConnection
import org.odk.collect.androidshared.sqlite.SQLiteDatabaseExt.delete
import org.odk.collect.androidshared.sqlite.SQLiteDatabaseExt.query
import org.odk.collect.db.sqlite.CursorExt.foldAndClose
import org.odk.collect.db.sqlite.DatabaseConnection
import org.odk.collect.db.sqlite.SQLiteDatabaseExt.delete
import org.odk.collect.db.sqlite.SQLiteDatabaseExt.query
import org.odk.collect.forms.savepoints.Savepoint
import org.odk.collect.forms.savepoints.SavepointsRepository
import org.odk.collect.shared.PathUtils
Expand Down
4 changes: 2 additions & 2 deletions ...p/src/main/java/org/odk/collect/android/database/savepoints/SavepointsDatabaseMigrator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.FOR
import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.INSTANCE_DB_ID
import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.INSTANCE_FILE_PATH
import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.SAVEPOINT_FILE_PATH
import org.odk.collect.androidshared.sqlite.DatabaseMigrator
import org.odk.collect.androidshared.sqlite.SQLiteUtils
import org.odk.collect.db.sqlite.DatabaseMigrator
import org.odk.collect.db.sqlite.SQLiteUtils

class SavepointsDatabaseMigrator :
DatabaseMigrator {
Expand Down
8 changes: 4 additions & 4 deletions ...ct_app/src/main/java/org/odk/collect/android/dynamicpreload/ExternalSQLiteOpenHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@
import com.opencsv.CSVReaderBuilder;

import org.odk.collect.android.application.Collect;
import org.odk.collect.androidshared.sqlite.AltDatabasePathContext;
import org.odk.collect.db.sqlite.AltDatabasePathContext;
import org.odk.collect.android.exception.ExternalDataException;
import org.odk.collect.androidshared.sqlite.CustomSQLiteQueryBuilder;
import org.odk.collect.androidshared.sqlite.CustomSQLiteQueryExecutor;
import org.odk.collect.androidshared.sqlite.SQLiteUtils;
import org.odk.collect.db.sqlite.CustomSQLiteQueryBuilder;
import org.odk.collect.db.sqlite.CustomSQLiteQueryExecutor;
import org.odk.collect.db.sqlite.SQLiteUtils;
import org.odk.collect.shared.strings.Md5;

import java.io.File;
Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import android.database.sqlite.SQLiteOpenHelper;

import org.odk.collect.android.application.Collect;
import org.odk.collect.androidshared.sqlite.AltDatabasePathContext;
import org.odk.collect.db.sqlite.AltDatabasePathContext;
import org.odk.collect.android.storage.StoragePathProvider;
import org.odk.collect.android.storage.StorageSubdirectory;
import org.odk.collect.shared.PathUtils;
Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/main/java/org/odk/collect/android/projects/ProjectDeleter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import org.odk.collect.android.backgroundwork.InstanceSubmitScheduler
import org.odk.collect.android.storage.StoragePathProvider
import org.odk.collect.android.utilities.ChangeLockProvider
import org.odk.collect.android.utilities.InstancesRepositoryProvider
import org.odk.collect.androidshared.sqlite.DatabaseConnection
import org.odk.collect.db.sqlite.DatabaseConnection
import org.odk.collect.forms.instances.Instance
import org.odk.collect.projects.Project
import org.odk.collect.projects.ProjectsRepository
Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/test/java/org/odk/collect/android/application/RobolectricApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import androidx.work.Configuration;
import androidx.work.WorkManager;

import org.odk.collect.androidshared.sqlite.DatabaseConnection;
import org.odk.collect.db.sqlite.DatabaseConnection;
import org.odk.collect.androidshared.ui.multiclicksafe.MultiClickGuard;
import org.odk.collect.crashhandler.CrashHandler;
import org.robolectric.shadows.ShadowApplication;
Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/test/java/org/odk/collect/android/database/DatabaseConnectionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.odk.collect.android.database.forms.FormDatabaseMigrator
import org.odk.collect.androidshared.sqlite.DatabaseConnection
import org.odk.collect.db.sqlite.DatabaseConnection
import org.odk.collect.shared.TempFiles.createTempDir
import java.io.File

Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/test/java/org/odk/collect/android/database/FormDatabaseMigratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.odk.collect.android.database.forms.FormDatabaseMigrator;
import org.odk.collect.androidshared.sqlite.SQLiteUtils;
import org.odk.collect.db.sqlite.SQLiteUtils;

import java.util.List;

Expand Down
4 changes: 2 additions & 2 deletions collect_app/src/test/java/org/odk/collect/android/dynamicpreload/ExternalDataReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
import org.odk.collect.android.storage.StoragePathProvider;
import org.odk.collect.android.storage.StorageSubdirectory;
import org.odk.collect.android.support.CollectHelpers;
import org.odk.collect.androidshared.sqlite.CustomSQLiteQueryBuilder;
import org.odk.collect.db.sqlite.CustomSQLiteQueryBuilder;
import org.odk.collect.android.utilities.FileUtils;
import org.odk.collect.androidshared.sqlite.SQLiteUtils;
import org.odk.collect.db.sqlite.SQLiteUtils;
import org.odk.collect.shared.strings.Md5;

import java.io.BufferedWriter;
Expand Down
84 changes: 84 additions & 0 deletions collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,34 @@ abstract class EntitiesRepositoryTest {
assertThat(wines[0].properties, contains("window" to "2019-2038", "score" to "92"))
}

@Test
fun `#save adds new properties to existing entities`() {
val repository = buildSubject()

val wine = Entity.New(
"wines",
"1",
"Léoville Barton 2008",
properties = listOf("window" to "2019-2038"),
version = 1
)
repository.save(wine)

val otherWine = Entity.New(
"wines",
"2",
"Léoville Barton 2009",
properties = listOf("score" to "92"),
version = 2
)
repository.save(otherWine)

val wines = repository.getEntities("wines")
assertThat(wines.size, equalTo(2))
assertThat(wines[0].properties, contains("window" to "2019-2038", "score" to ""))
assertThat(wines[1].properties, contains("window" to "", "score" to "92"))
}

@Test
fun `#save updates existing properties`() {
val repository = buildSubject()
Expand Down Expand Up @@ -437,6 +465,62 @@ abstract class EntitiesRepositoryTest {
)
}

@Test
fun `#getByAllByProperty returns entities without property when searching for empty string`() {
grzesiek2010 marked this conversation as resolved.
Show resolved Hide resolved
val repository = buildSubject()

val leoville = Entity.New(
"wines",
"1",
"Léoville Barton 2008",
properties = listOf("vintage" to "2008")
)

val canet = Entity.New(
"wines",
"2",
"Pontet-Canet 2014",
properties = listOf("score" to "93")
)

repository.save(leoville)
repository.save(canet)

val allByProperty = repository.getAllByProperty("wines", "score", "")
assertThat(allByProperty.size, equalTo(1))
assertThat(allByProperty[0].id, equalTo("1"))
}

@Test
fun `#getByAllByProperty returns entities when searching for empty string for property that doesn't exist`() {
val repository = buildSubject()

val leoville = Entity.New(
"wines",
"1",
"Léoville Barton 2008",
properties = listOf("vintage" to "2008")
)

repository.save(leoville)
assertThat(repository.getAllByProperty("wines", "score", "").size, equalTo(1))
}

@Test
fun `#getByAllByProperty returns empty list when searching for non empty string for property that doesn't exist`() {
val repository = buildSubject()

val leoville = Entity.New(
"wines",
"1",
"Léoville Barton 2008",
properties = listOf("vintage" to "2008")
)

repository.save(leoville)
assertThat(repository.getAllByProperty("wines", "score", "92").size, equalTo(0))
}

@Test
fun `#getAllByProperty returns empty list when there are no matches`() {
val repository = buildSubject()
Expand Down
1 change: 1 addition & 0 deletions db/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/build
Loading