From 268f5c807dd568008a73eae21e9a82f15a4f0bee Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Sat, 24 Dec 2022 10:56:54 -0500 Subject: [PATCH] Ensure that remapped records are valid. Fixes #12691 --- .../database/RemappedRecordTables.kt | 38 +++++++++++++++++++ .../securesms/database/RemappedRecords.java | 2 +- .../V166_ThreadAndMessageForeignKeys.kt | 12 +++++- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecordTables.kt b/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecordTables.kt index a727df205e..0e4664ca94 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecordTables.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecordTables.kt @@ -3,6 +3,8 @@ package org.thoughtcrime.securesms.database import android.content.Context import android.database.Cursor import androidx.core.content.contentValuesOf +import org.signal.core.util.delete +import org.signal.core.util.logging.Log import org.signal.core.util.readToList import org.signal.core.util.requireLong import org.signal.core.util.select @@ -19,6 +21,8 @@ import java.util.HashMap class RemappedRecordTables internal constructor(context: Context?, databaseHelper: SignalDatabase?) : DatabaseTable(context, databaseHelper) { companion object { + val TAG = Log.tag(RemappedRecordTables::class.java) + val CREATE_TABLE = arrayOf(Recipients.CREATE_TABLE, Threads.CREATE_TABLE) } @@ -51,6 +55,8 @@ class RemappedRecordTables internal constructor(context: Context?, databaseHelpe } fun getAllRecipientMappings(): Map { + clearInvalidRecipientMappings() + val recipientMap: MutableMap = HashMap() readableDatabase.withinTransaction { db -> @@ -66,6 +72,8 @@ class RemappedRecordTables internal constructor(context: Context?, databaseHelpe } fun getAllThreadMappings(): Map { + clearInvalidThreadMappings() + val threadMap: MutableMap = HashMap() readableDatabase.withinTransaction { db -> @@ -87,6 +95,7 @@ class RemappedRecordTables internal constructor(context: Context?, databaseHelpe } fun getAllRecipients(): Cursor { + clearInvalidRecipientMappings() return readableDatabase .select() .from(Recipients.TABLE_NAME) @@ -94,6 +103,7 @@ class RemappedRecordTables internal constructor(context: Context?, databaseHelpe } fun getAllThreads(): Cursor { + clearInvalidThreadMappings() return readableDatabase .select() .from(Threads.TABLE_NAME) @@ -120,5 +130,33 @@ class RemappedRecordTables internal constructor(context: Context?, databaseHelpe databaseHelper.signalWritableDatabase.insert(table, null, values) } + /** + * The old_id should never exist -- this class is intended to remap from IDs that were deleted. + */ + private fun clearInvalidRecipientMappings() { + val count = writableDatabase + .delete(Recipients.TABLE_NAME) + .where("$OLD_ID IN (SELECT ${RecipientTable.ID} FROM ${RecipientTable.TABLE_NAME})") + .run() + + if (count > 0) { + Log.w(TAG, "Deleted $count invalid recipient mappings!", true) + } + } + + /** + * The old_id should never exist -- this class is intended to remap from IDs that were deleted. + */ + private fun clearInvalidThreadMappings() { + val count = writableDatabase + .delete(Threads.TABLE_NAME) + .where("$OLD_ID IN (SELECT ${ThreadTable.ID} FROM ${ThreadTable.TABLE_NAME})") + .run() + + if (count > 0) { + Log.w(TAG, "Deleted $count invalid recipient mappings!", true) + } + } + private class Mapping(val oldId: Long, val newId: Long) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecords.java b/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecords.java index bddc44c41a..cbd5737c82 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecords.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RemappedRecords.java @@ -20,7 +20,7 @@ import java.util.Set; * * There should be very few of these, so we keep them in a fast, lazily-loaded memory cache. * - * One important thing to note is that this class will often be accesses inside of database + * One important thing to note is that this class will often be accessed inside of database * transactions. As a result, it cannot attempt to acquire a database lock while holding a * separate lock. Instead, we use the database lock itself as a locking mechanism. */ diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V166_ThreadAndMessageForeignKeys.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V166_ThreadAndMessageForeignKeys.kt index c5b44c04b9..54b2bf1a23 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V166_ThreadAndMessageForeignKeys.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V166_ThreadAndMessageForeignKeys.kt @@ -23,7 +23,7 @@ object V166_ThreadAndMessageForeignKeys : SignalDatabaseMigration { // Some crashes we were seeing indicated that we may have been running this migration twice on some unlucky devices, likely due // to some gaps that were left between some transactions during the upgrade path. if (!SqlUtil.columnExists(db, "thread", "thread_recipient_id")) { - Log.w(TAG, "Migration must have already run! Skipping.") + Log.w(TAG, "Migration must have already run! Skipping.", true) return } @@ -63,7 +63,7 @@ object V166_ThreadAndMessageForeignKeys : SignalDatabaseMigration { while (cursor.moveToNext()) { val recipientId = cursor.requireLong("thread_recipient_id") val count = cursor.requireLong("thread_count") - Log.w(TAG, "There were $count threads for RecipientId::$recipientId. Merging.") + Log.w(TAG, "There were $count threads for RecipientId::$recipientId. Merging.", true) val threads: List = getThreadsByRecipientId(db, cursor.requireLong("thread_recipient_id")) mergeThreads(db, threads) @@ -112,6 +112,14 @@ object V166_ThreadAndMessageForeignKeys : SignalDatabaseMigration { .where("thread_id = ?", secondaryId) .run() + // We're dealing with threads that exist, so we don't need to remap old_ids + + val count = db.update("remapped_threads") + .values("new_id" to primaryId) + .where("new_id = ?", secondaryId) + .run() + Log.w(TAG, "Remapped $count remapped_threads new_ids from $secondaryId to $primaryId", true) + db.delete("thread") .where("_id = ?", secondaryId) .run()