diff --git a/app/src/androidTest/java/org/thoughtcrime/securesms/database/DatabaseConsistencyTest.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/database/DatabaseConsistencyTest.kt index 8736df6b2c..64d551339a 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/database/DatabaseConsistencyTest.kt +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/database/DatabaseConsistencyTest.kt @@ -8,6 +8,10 @@ import net.zetetic.database.sqlcipher.SQLiteOpenHelper import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.signal.core.util.ForeignKeyConstraint +import org.signal.core.util.Index +import org.signal.core.util.getForeignKeys +import org.signal.core.util.getIndexes import org.signal.core.util.readToList import org.signal.core.util.requireNonNullString import org.thoughtcrime.securesms.database.helpers.SignalDatabaseMigrations @@ -24,7 +28,7 @@ class DatabaseConsistencyTest { val harness = SignalActivityRule() @Test - fun test() { + fun testUpgradeConsistency() { val currentVersionStatements = SignalDatabase.rawDatabase.getAllCreateStatements() val testHelper = InMemoryTestHelper(ApplicationDependencies.getApplication()).also { it.onUpgrade(it.writableDatabase, 181, SignalDatabaseMigrations.DATABASE_VERSION) @@ -61,6 +65,30 @@ class DatabaseConsistencyTest { } } + @Test + fun testForeignKeyIndexCoverage() { + /** We may deem certain indexes non-critical if deletion frequency is low or table size is small. */ + val ignoredColumns: List> = listOf( + StorySendTable.TABLE_NAME to StorySendTable.DISTRIBUTION_ID + ) + + val foreignKeys: List = SignalDatabase.rawDatabase.getForeignKeys() + val indexesByFirstColumn: List = SignalDatabase.rawDatabase.getIndexes() + + val notFound: List> = foreignKeys + .filterNot { ignoredColumns.contains(it.table to it.column) } + .filterNot { foreignKey -> + indexesByFirstColumn.hasPrimaryIndexFor(foreignKey.table, foreignKey.column) + } + .map { it.table to it.column } + + assertTrue("Missing indexes to cover: $notFound", notFound.isEmpty()) + } + + private fun List.hasPrimaryIndexFor(table: String, column: String): Boolean { + return this.any { index -> index.table == table && index.columns[0] == column } + } + private data class Statement( val name: String, val sql: String @@ -74,6 +102,7 @@ class DatabaseConsistencyTest { sql = cursor.requireNonNullString("sql").normalizeSql() ) } + .filterNot { it.name.startsWith("sqlite_stat") } .sortedBy { it.name } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/CallTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/CallTable.kt index 7ec1805362..59b3fee1e8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/CallTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/CallTable.kt @@ -81,7 +81,9 @@ class CallTable(context: Context, databaseHelper: SignalDatabase) : DatabaseTabl val CREATE_INDEXES = arrayOf( "CREATE INDEX call_call_id_index ON $TABLE_NAME ($CALL_ID)", - "CREATE INDEX call_message_id_index ON $TABLE_NAME ($MESSAGE_ID)" + "CREATE INDEX call_message_id_index ON $TABLE_NAME ($MESSAGE_ID)", + "CREATE INDEX call_call_link_index ON $TABLE_NAME ($CALL_LINK)", + "CREATE INDEX call_peer_index ON $TABLE_NAME ($PEER)" ) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/DistributionListTables.kt b/app/src/main/java/org/thoughtcrime/securesms/database/DistributionListTables.kt index d53e9511d4..2ef8e7b649 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/DistributionListTables.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/DistributionListTables.kt @@ -41,7 +41,7 @@ class DistributionListTables constructor(context: Context?, databaseHelper: Sign val CREATE_TABLE: Array = arrayOf(ListTable.CREATE_TABLE, MembershipTable.CREATE_TABLE) @JvmField - val CREATE_INDEXES: Array = arrayOf(MembershipTable.CREATE_INDEX) + val CREATE_INDEXES: Array = MembershipTable.CREATE_INDEXES const val RECIPIENT_ID = ListTable.RECIPIENT_ID const val DISTRIBUTION_ID = ListTable.DISTRIBUTION_ID @@ -123,7 +123,10 @@ class DistributionListTables constructor(context: Context?, databaseHelper: Sign ) """ - const val CREATE_INDEX = "CREATE UNIQUE INDEX distribution_list_member_list_id_recipient_id_privacy_mode_index ON $TABLE_NAME ($LIST_ID, $RECIPIENT_ID, $PRIVACY_MODE)" + val CREATE_INDEXES = arrayOf( + "CREATE UNIQUE INDEX distribution_list_member_list_id_recipient_id_privacy_mode_index ON $TABLE_NAME ($LIST_ID, $RECIPIENT_ID, $PRIVACY_MODE)", + "CREATE INDEX distribution_list_member_recipient_id ON $TABLE_NAME ($RECIPIENT_ID)" + ) } /** diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/MessageSendLogTables.kt b/app/src/main/java/org/thoughtcrime/securesms/database/MessageSendLogTables.kt index 0644e988db..10967643e2 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/MessageSendLogTables.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/MessageSendLogTables.kt @@ -143,7 +143,8 @@ class MessageSendLogTables constructor(context: Context?, databaseHelper: Signal /** Created for [MslPayloadTable.CREATE_TRIGGERS] and [deleteAllRelatedToMessage] */ val CREATE_INDEXES = arrayOf( - "CREATE INDEX msl_message_message_index ON $TABLE_NAME ($MESSAGE_ID, $PAYLOAD_ID)" + "CREATE INDEX msl_message_message_index ON $TABLE_NAME ($MESSAGE_ID, $PAYLOAD_ID)", + "CREATE INDEX msl_message_payload_index ON $TABLE_NAME ($PAYLOAD_ID)" ) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt index 9f71694106..5986cfc8d8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/SignalDatabaseMigrations.kt @@ -42,6 +42,7 @@ import org.thoughtcrime.securesms.database.helpers.migration.V183_CallLinkTableM import org.thoughtcrime.securesms.database.helpers.migration.V184_CallLinkReplaceIndexMigration import org.thoughtcrime.securesms.database.helpers.migration.V185_MessageRecipientsAndEditMessageMigration import org.thoughtcrime.securesms.database.helpers.migration.V186_ForeignKeyIndicesMigration +import org.thoughtcrime.securesms.database.helpers.migration.V187_MoreForeignKeyIndexesMigration /** * Contains all of the database migrations for [SignalDatabase]. Broken into a separate file for cleanliness. @@ -50,7 +51,7 @@ object SignalDatabaseMigrations { val TAG: String = Log.tag(SignalDatabaseMigrations.javaClass) - const val DATABASE_VERSION = 186 + const val DATABASE_VERSION = 187 @JvmStatic fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { @@ -205,6 +206,10 @@ object SignalDatabaseMigrations { if (oldVersion < 186) { V186_ForeignKeyIndicesMigration.migrate(context, db, oldVersion, newVersion) } + + if (oldVersion < 187) { + V187_MoreForeignKeyIndexesMigration.migrate(context, db, oldVersion, newVersion) + } } @JvmStatic diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V187_MoreForeignKeyIndexesMigration.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V187_MoreForeignKeyIndexesMigration.kt new file mode 100644 index 0000000000..1c2dc2e66f --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V187_MoreForeignKeyIndexesMigration.kt @@ -0,0 +1,32 @@ +package org.thoughtcrime.securesms.database.helpers.migration + +import android.app.Application +import net.zetetic.database.sqlcipher.SQLiteDatabase +import org.signal.core.util.Stopwatch +import org.signal.core.util.logging.Log + +/** + * I found some other tables that didn't have the proper indexes setup to correspond with their foreign keys. + */ +object V187_MoreForeignKeyIndexesMigration : SignalDatabaseMigration { + + private val TAG = Log.tag(V187_MoreForeignKeyIndexesMigration::class.java) + + override fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + val stopwatch = Stopwatch("migration") + + db.execSQL("CREATE INDEX IF NOT EXISTS call_call_link_index ON call (call_link)") + stopwatch.split("call_link") + + db.execSQL("CREATE INDEX IF NOT EXISTS call_peer_index ON call (peer)") + stopwatch.split("call_peer") + + db.execSQL("CREATE INDEX IF NOT EXISTS distribution_list_member_recipient_id ON distribution_list_member (recipient_id)") + stopwatch.split("dlist_member") + + db.execSQL("CREATE INDEX IF NOT EXISTS msl_message_payload_index ON msl_message (payload_id)") + stopwatch.split("msl_payload") + + stopwatch.stop(TAG) + } +} diff --git a/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt b/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt index 8c057838ab..51ad843eec 100644 --- a/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt +++ b/core-util/src/main/java/org/signal/core/util/SQLiteDatabaseExtensions.kt @@ -35,6 +35,34 @@ fun SupportSQLiteDatabase.getTableRowCount(table: String): Int { } } +fun SupportSQLiteDatabase.getForeignKeys(): List { + return SqlUtil.getAllTables(this) + .map { table -> + this.query("PRAGMA foreign_key_list($table)").readToList { cursor -> + ForeignKeyConstraint( + table = table, + column = cursor.requireNonNullString("from"), + dependsOnTable = cursor.requireNonNullString("table"), + dependsOnColumn = cursor.requireNonNullString("to"), + onDelete = cursor.requireString("on_delete") ?: "NOTHING" + ) + } + } + .flatten() +} + +fun SupportSQLiteDatabase.getIndexes(): List { + return this.query("SELECT name, tbl_name FROM sqlite_master WHERE type='index' ORDER BY name ASC").readToList { cursor -> + val indexName = cursor.requireNonNullString("name") + + Index( + name = indexName, + table = cursor.requireNonNullString("tbl_name"), + columns = this.query("PRAGMA index_info($indexName)").readToList { it.requireNonNullString("name") } + ) + } +} + /** * Checks if a row exists that matches the query. */ @@ -345,3 +373,17 @@ class InsertBuilderPart2( return db.insert(tableName, conflictStrategy, values) } } + +data class ForeignKeyConstraint( + val table: String, + val column: String, + val dependsOnTable: String, + val dependsOnColumn: String, + val onDelete: String +) + +data class Index( + val name: String, + val table: String, + val columns: List +) diff --git a/spinner/lib/src/main/java/org/signal/spinner/DatabaseUtil.kt b/spinner/lib/src/main/java/org/signal/spinner/DatabaseUtil.kt index 0ceedb94c3..14f374b843 100644 --- a/spinner/lib/src/main/java/org/signal/spinner/DatabaseUtil.kt +++ b/spinner/lib/src/main/java/org/signal/spinner/DatabaseUtil.kt @@ -2,10 +2,6 @@ package org.signal.spinner import android.database.Cursor import androidx.sqlite.db.SupportSQLiteDatabase -import org.signal.core.util.SqlUtil -import org.signal.core.util.readToList -import org.signal.core.util.requireNonNullString -import org.signal.core.util.requireString fun SupportSQLiteDatabase.getTableNames(): List { val out = mutableListOf() @@ -30,22 +26,6 @@ fun SupportSQLiteDatabase.getTriggers(): Cursor { return this.query("SELECT * FROM sqlite_master WHERE type='trigger' ORDER BY name ASC") } -fun SupportSQLiteDatabase.getForeignKeys(): List { - return SqlUtil.getAllTables(this) - .map { table -> - this.query("PRAGMA foreign_key_list($table)").readToList { cursor -> - ForeignKeyConstraint( - table = table, - column = cursor.requireNonNullString("from"), - dependsOnTable = cursor.requireNonNullString("table"), - dependsOnColumn = cursor.requireNonNullString("to"), - onDelete = cursor.requireString("on_delete") ?: "NOTHING" - ) - } - } - .flatten() -} - fun SupportSQLiteDatabase.getTableRowCount(table: String): Int { return this.query("SELECT COUNT(*) FROM $table").use { if (it.moveToFirst()) { @@ -55,11 +35,3 @@ fun SupportSQLiteDatabase.getTableRowCount(table: String): Int { } } } - -data class ForeignKeyConstraint( - val table: String, - val column: String, - val dependsOnTable: String, - val dependsOnColumn: String, - val onDelete: String -) diff --git a/spinner/lib/src/main/java/org/signal/spinner/SpinnerServer.kt b/spinner/lib/src/main/java/org/signal/spinner/SpinnerServer.kt index 087ee28539..fd86b60190 100644 --- a/spinner/lib/src/main/java/org/signal/spinner/SpinnerServer.kt +++ b/spinner/lib/src/main/java/org/signal/spinner/SpinnerServer.kt @@ -8,6 +8,8 @@ import com.github.jknack.handlebars.Template import com.github.jknack.handlebars.helper.ConditionalHelpers import fi.iki.elonen.NanoHTTPD import org.signal.core.util.ExceptionUtil +import org.signal.core.util.ForeignKeyConstraint +import org.signal.core.util.getForeignKeys import org.signal.core.util.logging.Log import org.signal.spinner.Spinner.DatabaseConfig import java.lang.IllegalArgumentException