diff --git a/app/src/androidTest/java/org/thoughtcrime/securesms/database/GroupTableTest.kt b/app/src/androidTest/java/org/thoughtcrime/securesms/database/GroupTableTest.kt index 990daeff2b..a626a68b80 100644 --- a/app/src/androidTest/java/org/thoughtcrime/securesms/database/GroupTableTest.kt +++ b/app/src/androidTest/java/org/thoughtcrime/securesms/database/GroupTableTest.kt @@ -122,6 +122,37 @@ class GroupTableTest { assertEquals(setOf(harness.self.id, harness.others[1]), groupRecord.members.toSet()) } + @Test + fun givenAGroup_whenIRemapRecipientsThatHaveAConflict_thenIExpectDeletion() { + val v2Group = insertPushGroupWithSelfAndOthers( + listOf( + harness.others[0], + harness.others[1] + ) + ) + + insertThread(v2Group) + + groupTable.remapRecipient(harness.others[0], harness.others[1]) + + val groupRecord = groupTable.getGroup(v2Group).get() + + assertEquals(setOf(harness.self.id, harness.others[1]), groupRecord.members.toSet()) + } + + @Test + fun givenAGroup_whenIRemapRecipients_thenIExpectRemap() { + val v2Group = insertPushGroup() + insertThread(v2Group) + + val newId = harness.others[1] + groupTable.remapRecipient(harness.others[0], newId) + + val groupRecord = groupTable.getGroup(v2Group).get() + + assertEquals(setOf(harness.self.id, newId), groupRecord.members.toSet()) + } + @Test fun givenAGroupAndMember_whenIIsCurrentMember_thenIExpectTrue() { val v2Group = insertPushGroup() @@ -282,4 +313,29 @@ class GroupTableTest { return groupTable.create(groupMasterKey, decryptedGroupState)!! } + + private fun insertPushGroupWithSelfAndOthers(others: List): GroupId { + val groupMasterKey = GroupMasterKey(Random.nextBytes(GroupMasterKey.SIZE)) + + val selfMember: DecryptedMember = DecryptedMember.newBuilder() + .setUuid(harness.self.requireServiceId().toByteString()) + .setJoinedAtRevision(0) + .setRole(Member.Role.DEFAULT) + .build() + + val otherMembers: List = others.map { id -> + DecryptedMember.newBuilder() + .setUuid(Recipient.resolved(id).requireServiceId().toByteString()) + .setJoinedAtRevision(0) + .setRole(Member.Role.DEFAULT) + .build() + } + + val decryptedGroupState = DecryptedGroup.newBuilder() + .addAllMembers(listOf(selfMember) + otherMembers) + .setRevision(0) + .build() + + return groupTable.create(groupMasterKey, decryptedGroupState)!! + } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt index 67f5820e12..4fccfeab78 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupTable.kt @@ -135,7 +135,7 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT "CREATE UNIQUE INDEX IF NOT EXISTS group_recipient_id_index ON $TABLE_NAME ($RECIPIENT_ID);", "CREATE UNIQUE INDEX IF NOT EXISTS expected_v2_id_index ON $TABLE_NAME ($EXPECTED_V2_ID);", "CREATE UNIQUE INDEX IF NOT EXISTS group_distribution_id_index ON $TABLE_NAME($DISTRIBUTION_ID);" - ) + ) + MembershipTable.CREATE_INDEXES private val GROUP_PROJECTION = arrayOf( GROUP_ID, @@ -190,10 +190,14 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT CREATE TABLE $TABLE_NAME ( $ID INTEGER PRIMARY KEY, $GROUP_ID TEXT NOT NULL, - $RECIPIENT_ID INTEGER NOT NULL, + $RECIPIENT_ID INTEGER NOT NULL REFERENCES ${RecipientTable.TABLE_NAME} (${RecipientTable.ID}) ON DELETE CASCADE, UNIQUE($GROUP_ID, $RECIPIENT_ID) ) """ + + val CREATE_INDEXES = arrayOf( + "CREATE INDEX IF NOT EXISTS group_membership_recipient_id ON $TABLE_NAME ($RECIPIENT_ID)" + ) } } @@ -213,44 +217,22 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT .query(select, query.whereArgs) .use { cursor -> return if (cursor.moveToFirst()) { - var refreshCursor = false val groupRecord = getGroup(cursor) if (groupRecord.isPresent && RemappedRecords.getInstance().areAnyRemapped(groupRecord.get().members)) { val groupId = groupRecord.get().id val remaps = RemappedRecords.getInstance().buildRemapDescription(groupRecord.get().members) Log.w(TAG, "Found a group with remapped recipients in it's membership list! Updating the list. GroupId: $groupId, Remaps: $remaps", true) - val oldToNew: List> = groupRecord.get().members.map { - it to RemappedRecords.getInstance().getRecipient(it).orElse(null) - }.filterNot { (old, new) -> new == null || old == new } + val oldToNew: List> = groupRecord.get().members + .map { it to RemappedRecords.getInstance().getRecipient(it).orElse(null) } + .filterNot { (old, new) -> new == null || old == new } - var updateCount = 0 if (oldToNew.isNotEmpty()) { writableDatabase.withinTransaction { db -> - for ((old, new) in oldToNew) { - updateCount += db.update(MembershipTable.TABLE_NAME) - .values(MembershipTable.RECIPIENT_ID to new!!.serialize()) - .where("${MembershipTable.GROUP_ID} = ? AND ${MembershipTable.RECIPIENT_ID} = ?", groupId, old) - .run(conflictStrategy = SQLiteDatabase.CONFLICT_IGNORE) - - if (updateCount == 0) { - db.delete(MembershipTable.TABLE_NAME) - .where("${MembershipTable.GROUP_ID} = ? AND ${MembershipTable.RECIPIENT_ID} = ?", groupId, old) - .run() - } - } + oldToNew.forEach { remapRecipient(it.first, it.second) } } } - if (updateCount > 0) { - Log.i(TAG, "Successfully updated $updateCount rows. GroupId: $groupId, Remaps: $remaps", true) - refreshCursor = true - } else { - Log.w(TAG, "Failed to update any rows. GroupId: $groupId, Remaps: $remaps", true) - } - } - - if (refreshCursor) { readableDatabase.query(select, query.whereArgs).use { refreshedCursor -> if (refreshedCursor.moveToFirst()) { getGroup(refreshedCursor) @@ -1131,13 +1113,31 @@ class GroupTable(context: Context?, databaseHelper: SignalDatabase?) : DatabaseT } override fun remapRecipient(fromId: RecipientId, toId: RecipientId) { - writableDatabase - .update(MembershipTable.TABLE_NAME) - .values(RECIPIENT_ID to toId.serialize()) - .where("${MembershipTable.RECIPIENT_ID} = ?", fromId) - .run(conflictStrategy = SQLiteDatabase.CONFLICT_IGNORE) + // Remap all recipients that would not result in conflicts + writableDatabase.execSQL( + """ + UPDATE ${MembershipTable.TABLE_NAME} AS parent + SET ${MembershipTable.RECIPIENT_ID} = ? + WHERE + ${MembershipTable.RECIPIENT_ID} = ? + AND NOT EXISTS ( + SELECT 1 + FROM ${MembershipTable.TABLE_NAME} child + WHERE + child.${MembershipTable.RECIPIENT_ID} = ? + AND parent.${MembershipTable.GROUP_ID} = child.${MembershipTable.GROUP_ID} + ) + """, + buildArgs(toId, fromId, toId) + ) - for (group in getGroupsContainingMember(fromId, false, true)) { + // Delete the remaining fromId's (the only remaining ones should be those in groups where the toId is already present) + writableDatabase + .delete(MembershipTable.TABLE_NAME) + .where("${MembershipTable.RECIPIENT_ID} = ?", fromId) + .run() + + for (group in getGroupsContainingMember(fromId, pushOnly = false, includeInactive = true)) { if (group.isV2Group) { removeUnmigratedV1Members(group.id.requireV2(), listOf(fromId)) } 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 5b87bb0d81..9d9a86443e 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 @@ -50,6 +50,7 @@ import org.thoughtcrime.securesms.database.helpers.migration.V191_UniqueMessageM import org.thoughtcrime.securesms.database.helpers.migration.V192_CallLinkTableNullableRootKeys import org.thoughtcrime.securesms.database.helpers.migration.V193_BackCallLinksWithRecipient import org.thoughtcrime.securesms.database.helpers.migration.V194_KyberPreKeyMigration +import org.thoughtcrime.securesms.database.helpers.migration.V195_GroupMemberForeignKeyMigration /** * Contains all of the database migrations for [SignalDatabase]. Broken into a separate file for cleanliness. @@ -58,7 +59,7 @@ object SignalDatabaseMigrations { val TAG: String = Log.tag(SignalDatabaseMigrations.javaClass) - const val DATABASE_VERSION = 194 + const val DATABASE_VERSION = 195 @JvmStatic fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { @@ -245,6 +246,10 @@ object SignalDatabaseMigrations { if (oldVersion < 194) { V194_KyberPreKeyMigration.migrate(context, db, oldVersion, newVersion) } + + if (oldVersion < 195) { + V195_GroupMemberForeignKeyMigration.migrate(context, db, oldVersion, newVersion) + } } @JvmStatic diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V195_GroupMemberForeignKeyMigration.kt b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V195_GroupMemberForeignKeyMigration.kt new file mode 100644 index 0000000000..cf36f287f5 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/database/helpers/migration/V195_GroupMemberForeignKeyMigration.kt @@ -0,0 +1,85 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.database.helpers.migration + +import android.app.Application +import net.zetetic.database.sqlcipher.SQLiteDatabase +import org.signal.core.util.SqlUtil +import org.signal.core.util.Stopwatch +import org.signal.core.util.logging.Log +import org.signal.core.util.readToList +import org.signal.core.util.requireLong + +/** + * Back CallLinks with a Recipient to ease integration and ensure we can support + * different features which would require that relation in the future. + */ +object V195_GroupMemberForeignKeyMigration : SignalDatabaseMigration { + + private val TAG = Log.tag(V195_GroupMemberForeignKeyMigration::class.java) + + override fun migrate(context: Application, db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + val stopwatch = Stopwatch("migration") + db.execSQL( + """ + CREATE TABLE group_membership_tmp ( + _id INTEGER PRIMARY KEY, + group_id TEXT NOT NULL, + recipient_id INTEGER NOT NULL REFERENCES recipient (_id) ON DELETE CASCADE, + UNIQUE(group_id, recipient_id) + ) + """ + ) + stopwatch.split("create") + + db.rawQuery("SELECT * FROM remapped_recipients") + .readToList { it.requireLong("old_id") to it.requireLong("new_id") } + .forEach { remapMembership(db, it) } + stopwatch.split("fix-remapping") + + db.execSQL("DELETE FROM group_membership WHERE recipient_id NOT IN (SELECT _id FROM RECIPIENT)") + stopwatch.split("trim-bad-fk") + + db.execSQL("INSERT INTO group_membership_tmp SELECT * FROM group_membership") + db.execSQL("DROP TABLE group_membership") + db.execSQL("ALTER TABLE group_membership_tmp RENAME TO group_membership") + stopwatch.split("copy-data") + + db.execSQL("CREATE INDEX IF NOT EXISTS group_membership_recipient_id ON group_membership (recipient_id)") + stopwatch.split("index") + + val foreignKeyViolations: List = SqlUtil.getForeignKeyViolations(db, "groups") + SqlUtil.getForeignKeyViolations(db, "group_membership") + if (foreignKeyViolations.isNotEmpty()) { + Log.w(TAG, "Foreign key violations!\n${foreignKeyViolations.joinToString(separator = "\n")}") + throw IllegalStateException("Foreign key violations!") + } + stopwatch.split("fk-check") + + stopwatch.stop(TAG) + } + + private fun remapMembership(db: SQLiteDatabase, remap: Pair) { + val fromId = remap.first + val toId = remap.second + + db.execSQL( + """ + UPDATE group_membership AS parent + SET recipient_id = ? + WHERE + recipient_id = ? + AND NOT EXISTS ( + SELECT 1 + FROM group_membership child + WHERE + child.recipient_id = ? + AND parent.group_id = child.group_id + ) + """, + SqlUtil.buildArgs(toId, fromId, toId) + ) + } +}