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 bd5072a9b69..273c89f44c1 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 @@ -4,21 +4,20 @@ import android.content.ContentValues import android.content.Context import android.database.Cursor 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.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.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 import org.odk.collect.db.sqlite.SQLiteDatabaseExt.delete 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.entities.storage.EntitiesRepository import org.odk.collect.entities.storage.Entity @@ -39,13 +38,12 @@ private object EntitiesTable { class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRepository { - private val databaseConnection: DatabaseConnection = DatabaseConnection( + private val databaseConnection = SynchronizedDatabaseConnection( context, dbPath, "entities.db", EntitiesDatabaseMigrator(), - 1, - true + 1 ) override fun save(list: String, vararg entities: Entity) { @@ -60,7 +58,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep updatePropertyColumns(list, entities.first()) - databaseConnection.writeableDatabase.transaction { + databaseConnection.transaction { entities.forEach { entity -> val existing = if (listExists) { query( @@ -121,10 +119,11 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep } override fun getLists(): Set { - return databaseConnection - .readableDatabase - .query(ListsTable.TABLE_NAME) - .foldAndClose(emptySet()) { set, cursor -> set + cursor.getString(ListsTable.COLUMN_NAME) } + return databaseConnection.withConnection { + readableDatabase + .query(ListsTable.TABLE_NAME) + .foldAndClose(emptySet()) { set, cursor -> set + cursor.getString(ListsTable.COLUMN_NAME) } + } } override fun updateListHash(list: String, hash: String) { @@ -132,21 +131,22 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep it.put(ListsTable.COLUMN_HASH, hash) } - databaseConnection - .writeableDatabase - .update( + databaseConnection.withConnection { + writableDatabase.update( ListsTable.TABLE_NAME, contentValues, "${ListsTable.COLUMN_NAME} = ?", arrayOf(list) ) + } } override fun getListHash(list: String): String? { - return databaseConnection - .readableDatabase - .query(ListsTable.TABLE_NAME, "${ListsTable.COLUMN_NAME} = ?", arrayOf(list)) - .first { it.getStringOrNull(ListsTable.COLUMN_HASH) } + return databaseConnection.withConnection { + readableDatabase + .query(ListsTable.TABLE_NAME, "${ListsTable.COLUMN_NAME} = ?", arrayOf(list)) + .first { it.getStringOrNull(ListsTable.COLUMN_HASH) } + } } override fun getEntities(list: String): List { @@ -168,23 +168,27 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep return 0 } - return databaseConnection.readableDatabase.rawQuery( - """ - SELECT COUNT(*) - FROM $list - """.trimIndent(), - null - ).first { - it.getInt(0) - }!! + return databaseConnection.withConnection { + readableDatabase.rawQuery( + """ + SELECT COUNT(*) + FROM $list + """.trimIndent(), + null + ).first { + it.getInt(0) + }!! + } } override fun clear() { - getLists().forEach { - databaseConnection.writeableDatabase.delete(it) - } + databaseConnection.withConnection { + getLists().forEach { + writableDatabase.delete(it) + } - databaseConnection.writeableDatabase.delete(ListsTable.TABLE_NAME) + writableDatabase.delete(ListsTable.TABLE_NAME) + } } override fun addList(list: String) { @@ -195,12 +199,14 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep } override fun delete(id: String) { - getLists().forEach { - databaseConnection.writeableDatabase.delete( - it, - "${EntitiesTable.COLUMN_ID} = ?", - arrayOf(id) - ) + databaseConnection.withConnection { + getLists().forEach { + writableDatabase.delete( + it, + "${EntitiesTable.COLUMN_ID} = ?", + arrayOf(id) + ) + } } updateRowIdTables() @@ -229,7 +235,11 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep return emptyList() } - return if (databaseConnection.readableDatabase.doesColumnExist(list, property)) { + val propertyExists = databaseConnection.withConnection { + readableDatabase.doesColumnExist(list, property) + } + + return if (propertyExists) { queryWithAttachedRowId( list, selectionColumn = property, @@ -251,29 +261,33 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep return null } - return databaseConnection.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(list, it, it.getInt(ROW_ID)) - } + 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(list, it, it.getInt(ROW_ID)) + } + } } private fun queryWithAttachedRowId(list: String): Cursor { - return databaseConnection.readableDatabase - .rawQuery( - """ - SELECT *, i.$ROW_ID - FROM $list e, ${getRowIdTableName(list)} i - WHERE e._id = i._id - """.trimIndent(), - null - ) + return databaseConnection.withConnection { + readableDatabase + .rawQuery( + """ + SELECT *, i.$ROW_ID + FROM $list e, ${getRowIdTableName(list)} i + WHERE e._id = i._id + """.trimIndent(), + null + ) + } } private fun queryWithAttachedRowId( @@ -281,8 +295,8 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep selectionColumn: String, selectionArg: String ): Cursor { - return databaseConnection.readableDatabase - .rawQuery( + return databaseConnection.withConnection { + readableDatabase.rawQuery( """ SELECT *, i.$ROW_ID FROM $list e, ${getRowIdTableName(list)} i @@ -290,6 +304,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep """.trimIndent(), arrayOf(selectionArg) ) + } } /** @@ -300,34 +315,38 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep * function, but that's not available in all the supported versions of Android. */ private fun updateRowIdTables() { - getLists().forEach { - databaseConnection.writeableDatabase.execSQL( - """ - DROP TABLE IF EXISTS ${getRowIdTableName(it)}; - """.trimIndent() - ) + databaseConnection.withConnection { + getLists().forEach { + writableDatabase.execSQL( + """ + DROP TABLE IF EXISTS ${getRowIdTableName(it)}; + """.trimIndent() + ) - databaseConnection.writeableDatabase.execSQL( - """ - CREATE TABLE ${getRowIdTableName(it)} AS SELECT _id FROM $it; - """.trimIndent() - ) + writableDatabase.execSQL( + """ + CREATE TABLE ${getRowIdTableName(it)} AS SELECT _id FROM $it; + """.trimIndent() + ) + } } } private fun getRowIdTableName(it: String) = "${it}_row_numbers" private fun listExists(list: String): Boolean { - return databaseConnection.readableDatabase - .query( - ListsTable.TABLE_NAME, - selection = "${ListsTable.COLUMN_NAME} = ?", - selectionArgs = arrayOf(list) - ).use { it.count } > 0 + return databaseConnection.withConnection { + readableDatabase + .query( + ListsTable.TABLE_NAME, + selection = "${ListsTable.COLUMN_NAME} = ?", + selectionArgs = arrayOf(list) + ).use { it.count } > 0 + } } private fun createList(list: String) { - databaseConnection.writeableDatabase.transaction { + databaseConnection.transaction { val contentValues = ContentValues() contentValues.put(ListsTable.COLUMN_NAME, list) insertOrThrow( @@ -359,15 +378,21 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep } private fun updatePropertyColumns(list: String, entity: Entity) { - entity.properties.map { it.first }.forEach { - try { - databaseConnection.writeableDatabase.execSQL( - """ - ALTER TABLE $list ADD "$it" text NOT NULL DEFAULT ""; - """.trimIndent() - ) - } catch (e: SQLiteException) { - // Ignored + val columnNames = databaseConnection.withConnection { + readableDatabase.getColumnNames(list) + } + + val missingColumns = + entity.properties.map { it.first }.filterNot { columnNames.contains(it) } + if (missingColumns.isNotEmpty()) { + databaseConnection.resetTransaction { + missingColumns.forEach { + execSQL( + """ + ALTER TABLE $list ADD "$it" text NOT NULL DEFAULT ""; + """.trimIndent() + ) + } } } } diff --git a/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java b/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java index 3797a7f5a0e..db305178f92 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java +++ b/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java @@ -238,13 +238,13 @@ private Cursor queryAndReturnCursor(Map projectionMap, String[] } private Long insertForm(ContentValues values) { - SQLiteDatabase writeableDatabase = databaseConnection.getWriteableDatabase(); - return writeableDatabase.insertOrThrow(FORMS_TABLE_NAME, null, values); + SQLiteDatabase writableDatabase = databaseConnection.getWritableDatabase(); + return writableDatabase.insertOrThrow(FORMS_TABLE_NAME, null, values); } private void updateForm(Long id, ContentValues values) { - SQLiteDatabase writeableDatabase = databaseConnection.getWriteableDatabase(); - writeableDatabase.update(FORMS_TABLE_NAME, values, _ID + "=?", new String[]{String.valueOf(id)}); + SQLiteDatabase writableDatabase = databaseConnection.getWritableDatabase(); + writableDatabase.update(FORMS_TABLE_NAME, values, _ID + "=?", new String[]{String.valueOf(id)}); } private void deleteForms(String selection, String[] selectionArgs) { @@ -255,8 +255,8 @@ private void deleteForms(String selection, String[] selectionArgs) { deleteFilesForForm(form); } - SQLiteDatabase writeableDatabase = databaseConnection.getWriteableDatabase(); - writeableDatabase.delete(FORMS_TABLE_NAME, selection, selectionArgs); + SQLiteDatabase writableDatabase = databaseConnection.getWritableDatabase(); + writableDatabase.delete(FORMS_TABLE_NAME, selection, selectionArgs); } @NotNull diff --git a/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java b/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java index 18f06367c03..8c9d33c3a44 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java +++ b/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java @@ -144,7 +144,7 @@ public List getAllNotDeletedByFormIdAndVersion(String jrFormId, String public void delete(Long id) { Instance instance = get(id); - databaseConnection.getWriteableDatabase().delete( + databaseConnection.getWritableDatabase().delete( INSTANCES_TABLE_NAME, _ID + "=?", new String[]{String.valueOf(id)} @@ -157,7 +157,7 @@ public void delete(Long id) { public void deleteAll() { List instances = getAll(); - databaseConnection.getWriteableDatabase().delete( + databaseConnection.getWritableDatabase().delete( INSTANCES_TABLE_NAME, null, null @@ -253,7 +253,7 @@ private Cursor query(String[] projection, String selection, String[] selectionAr } private long insert(ContentValues values) { - return databaseConnection.getWriteableDatabase().insertOrThrow( + return databaseConnection.getWritableDatabase().insertOrThrow( INSTANCES_TABLE_NAME, null, values @@ -261,7 +261,7 @@ private long insert(ContentValues values) { } private void update(Long instanceId, ContentValues values) { - databaseConnection.getWriteableDatabase().update( + databaseConnection.getWritableDatabase().update( INSTANCES_TABLE_NAME, values, _ID + "=?", diff --git a/collect_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java b/collect_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java index f4cbcb4ce33..22b6ff150ab 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java +++ b/collect_app/src/main/java/org/odk/collect/android/database/instances/InstanceDatabaseMigrator.java @@ -19,6 +19,7 @@ import android.database.sqlite.SQLiteDatabase; import org.odk.collect.db.sqlite.DatabaseMigrator; +import org.odk.collect.db.sqlite.SQLiteDatabaseExt; import org.odk.collect.db.sqlite.SQLiteUtils; import org.odk.collect.forms.instances.Instance; @@ -118,7 +119,7 @@ private void upgradeToVersion5(SQLiteDatabase db) { * @param temporaryTableName the name of the temporary table to use and then drop */ private void dropObsoleteColumns(SQLiteDatabase db, String[] relevantColumns, String temporaryTableName) { - List columns = SQLiteUtils.getColumnNames(db, INSTANCES_TABLE_NAME); + List columns = SQLiteDatabaseExt.getColumnNames(db, INSTANCES_TABLE_NAME); columns.retainAll(Arrays.asList(relevantColumns)); String[] columnsToKeep = columns.toArray(new String[0]); diff --git a/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt b/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt index b9029fee1ad..6b126b04acd 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt +++ b/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt @@ -66,7 +66,7 @@ class DatabaseSavepointsRepository( val values = getValuesFromSavepoint(savepoint, cachePath, instancesPath) databaseConnection - .writeableDatabase + .writableDatabase .insertOrThrow(SAVEPOINTS_TABLE_NAME, null, values) } @@ -86,7 +86,7 @@ class DatabaseSavepointsRepository( } databaseConnection - .writeableDatabase + .writableDatabase .delete(SAVEPOINTS_TABLE_NAME, selection, selectionArgs) File(savepoint.savepointFilePath).delete() @@ -98,7 +98,7 @@ class DatabaseSavepointsRepository( } databaseConnection - .writeableDatabase + .writableDatabase .delete(SAVEPOINTS_TABLE_NAME) } diff --git a/db/src/main/java/org/odk/collect/db/sqlite/DatabaseConnection.kt b/db/src/main/java/org/odk/collect/db/sqlite/DatabaseConnection.kt index ff50599270e..b5690057d99 100644 --- a/db/src/main/java/org/odk/collect/db/sqlite/DatabaseConnection.kt +++ b/db/src/main/java/org/odk/collect/db/sqlite/DatabaseConnection.kt @@ -24,7 +24,9 @@ open class DatabaseConnection @JvmOverloads constructor( private val strict: Boolean = false ) { - val writeableDatabase: SQLiteDatabase + private val databasePath = path + File.separator + name + + val writableDatabase: SQLiteDatabase get() { StrictMode.noteSlowCall("Accessing writable DB") return dbHelper.writableDatabase @@ -40,30 +42,59 @@ open class DatabaseConnection @JvmOverloads constructor( private val dbHelper: SQLiteOpenHelper get() { - val databasePath = path + File.separator + name - if (openHelpers.containsKey(databasePath) && !File(databasePath).exists()) { - /** - * Ideally we should close the database here as well but it was causing crashes in - * our tests as DB connections seem to be getting used after being closed. These - * "removed" helpers will be closed in [closeAll] rather than when they are - * replaced. - */ - openHelpers.remove(databasePath)?.let { - toClose.add(it) + return synchronized(openHelpers) { + if (openHelpers.containsKey(databasePath) && !File(databasePath).exists()) { + /** + * Ideally we should close the database here as well but it was causing crashes in + * our tests as DB connections seem to be getting used after being closed. These + * "removed" helpers will be closed in [closeAll] rather than when they are + * replaced. + */ + openHelpers.remove(databasePath)?.let { + toClose.add(it) + } } - } - return openHelpers.getOrPut(databasePath) { - DatabaseMigratorSQLiteOpenHelper( - AltDatabasePathContext(path, context), - name, - null, - databaseVersion, - migrator - ) + openHelpers.getOrPut(databasePath) { + DatabaseMigratorSQLiteOpenHelper( + AltDatabasePathContext(path, context), + name, + null, + databaseVersion, + migrator + ) + } } } + /** + * Closes the underlying connection and clears state so that subsequent accesses will create + * a new connection. + * + * This can be dangerous if the database is being access by multiple threads as the connection + * might be closed while a transaction is running or while another thread is using a + * [SQLiteDatabase] instance obtained via [writableDatabase] or [readableDatabase]. Using + * [SynchronizedDatabaseConnection] is recommended in those scenarios. + */ + fun reset() { + openHelpers.remove(databasePath)?.close() + } + + /** + * Access the database in a synchronized manner. This is not usually required, but should be + * used if a calls to [reset] will be made. + * + * Does not guarantee synchronized access if this or another [DatabaseConnection] for the + * same `.db` file uses [writableDatabase] or [readableDatabase]. + * [SynchronizedDatabaseConnection] can be used to ensure synchronized writes/reads to the + * underlying DB. + */ + fun withSynchronizedConnection(block: DatabaseConnection.() -> T): T { + return synchronized(dbHelper) { + block(this) + } + } + companion object { private val openHelpers = mutableMapOf() diff --git a/db/src/main/java/org/odk/collect/db/sqlite/SQLiteDatabaseExt.kt b/db/src/main/java/org/odk/collect/db/sqlite/SQLiteDatabaseExt.kt index 3f10577f071..642545eced6 100644 --- a/db/src/main/java/org/odk/collect/db/sqlite/SQLiteDatabaseExt.kt +++ b/db/src/main/java/org/odk/collect/db/sqlite/SQLiteDatabaseExt.kt @@ -23,6 +23,16 @@ object SQLiteDatabaseExt { @JvmStatic fun SQLiteDatabase.doesColumnExist(table: String, column: String): Boolean { - return SQLiteUtils.getColumnNames(this, table).contains(column) + return this.getColumnNames(table).contains(column) + } + + @JvmStatic + fun SQLiteDatabase.getColumnNames(table: String): List { + var columnNames: Array + this.query(table, null, null, null, null, null, null).use { c -> + columnNames = c.columnNames + } + + return columnNames.toList() } } diff --git a/db/src/main/java/org/odk/collect/db/sqlite/SQLiteUtils.java b/db/src/main/java/org/odk/collect/db/sqlite/SQLiteUtils.java index f6248f3f1a9..df3d57ec180 100644 --- a/db/src/main/java/org/odk/collect/db/sqlite/SQLiteUtils.java +++ b/db/src/main/java/org/odk/collect/db/sqlite/SQLiteUtils.java @@ -5,10 +5,6 @@ import android.database.Cursor; import android.database.sqlite.SQLiteDatabase; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; - /** * @deprecated use {@link SQLiteDatabaseExt} instead. */ @@ -36,16 +32,6 @@ public static boolean doesTableExist(SQLiteDatabase db, String tableName) { return foundTable; } - public static List getColumnNames(SQLiteDatabase db, String tableName) { - String[] columnNames; - try (Cursor c = db.query(tableName, null, null, null, null, null, null)) { - columnNames = c.getColumnNames(); - } - - // Build a full-featured ArrayList rather than the limited array-backed List from asList - return new ArrayList<>(Arrays.asList(columnNames)); - } - public static void addColumn(SQLiteDatabase db, String table, String column, String type) { if (!doesColumnExist(db, table, column)) { CustomSQLiteQueryExecutor.begin(db) diff --git a/db/src/main/java/org/odk/collect/db/sqlite/SynchronizedDatabaseConnection.kt b/db/src/main/java/org/odk/collect/db/sqlite/SynchronizedDatabaseConnection.kt new file mode 100644 index 00000000000..21e63608aa4 --- /dev/null +++ b/db/src/main/java/org/odk/collect/db/sqlite/SynchronizedDatabaseConnection.kt @@ -0,0 +1,46 @@ +package org.odk.collect.db.sqlite + +import android.content.Context +import android.database.sqlite.SQLiteDatabase +import androidx.core.database.sqlite.transaction + +class SynchronizedDatabaseConnection( + context: Context, + path: String, + name: String, + migrator: DatabaseMigrator, + databaseVersion: Int +) { + private val databaseConnection = DatabaseConnection( + context, + path, + name, + migrator, + databaseVersion + ) + + fun withConnection(block: DatabaseConnection.() -> T): T { + return databaseConnection.withSynchronizedConnection(block) + } + + fun transaction( + body: SQLiteDatabase.() -> T + ) { + return withConnection { + writableDatabase.transaction { + body() + } + } + } + + /** + * Runs a transaction and then calls [DatabaseConnection.reset]. Useful for transactions + * that will mutate the DB schema. + */ + fun resetTransaction( + body: SQLiteDatabase.() -> T + ) { + transaction(body) + databaseConnection.reset() + } +}