Fix group membership recipient remapping.
This commit is contained in:
Greyson Parrelli
parent
a64bffd83a
commit
44ab1643fa
4 changed files with 181 additions and 35 deletions
|
|
@ -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<RecipientId>): 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<DecryptedMember> = 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)!!
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Pair<RecipientId, RecipientId?>> = groupRecord.get().members.map {
|
||||
it to RemappedRecords.getInstance().getRecipient(it).orElse(null)
|
||||
}.filterNot { (old, new) -> new == null || old == new }
|
||||
val oldToNew: List<Pair<RecipientId, RecipientId>> = 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))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.ForeignKeyViolation> = 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<Long, Long>) {
|
||||
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)
|
||||
)
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue