Do not double-insert change number events.

main
Greyson Parrelli 2022-11-11 12:14:13 -05:00
rodzic 4e871e2dd8
commit ccee7577f7
4 zmienionych plików z 5 dodań i 104 usunięć

Wyświetl plik

@ -12,7 +12,6 @@ import org.junit.Before
import org.junit.Test import org.junit.Test
import org.junit.runner.RunWith import org.junit.runner.RunWith
import org.signal.core.util.CursorUtil import org.signal.core.util.CursorUtil
import org.signal.core.util.ThreadUtil
import org.signal.libsignal.protocol.IdentityKey import org.signal.libsignal.protocol.IdentityKey
import org.signal.libsignal.protocol.SignalProtocolAddress import org.signal.libsignal.protocol.SignalProtocolAddress
import org.signal.libsignal.protocol.state.SessionRecord import org.signal.libsignal.protocol.state.SessionRecord
@ -26,9 +25,7 @@ import org.thoughtcrime.securesms.database.model.Mention
import org.thoughtcrime.securesms.database.model.MessageId import org.thoughtcrime.securesms.database.model.MessageId
import org.thoughtcrime.securesms.database.model.MessageRecord import org.thoughtcrime.securesms.database.model.MessageRecord
import org.thoughtcrime.securesms.database.model.ReactionRecord import org.thoughtcrime.securesms.database.model.ReactionRecord
import org.thoughtcrime.securesms.dependencies.ApplicationDependencies
import org.thoughtcrime.securesms.groups.GroupId import org.thoughtcrime.securesms.groups.GroupId
import org.thoughtcrime.securesms.jobs.RecipientChangedNumberJob
import org.thoughtcrime.securesms.keyvalue.AccountValues import org.thoughtcrime.securesms.keyvalue.AccountValues
import org.thoughtcrime.securesms.keyvalue.KeyValueDataSet import org.thoughtcrime.securesms.keyvalue.KeyValueDataSet
import org.thoughtcrime.securesms.keyvalue.KeyValueStore import org.thoughtcrime.securesms.keyvalue.KeyValueStore
@ -229,9 +226,6 @@ class RecipientDatabaseTest_getAndPossiblyMerge {
/** Merge two different users into one. You should prefer the ACI user. Not shown: merging threads, dropping e164 sessions, etc. */ /** Merge two different users into one. You should prefer the ACI user. Not shown: merging threads, dropping e164 sessions, etc. */
@Test @Test
fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge() { fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge() {
val changeNumberListener = ChangeNumberListener()
changeNumberListener.enqueue()
val existingAciId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, null) val existingAciId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, null)
val existingE164Id: RecipientId = recipientDatabase.getAndPossiblyMerge(null, E164_A) val existingE164Id: RecipientId = recipientDatabase.getAndPossiblyMerge(null, E164_A)
@ -245,16 +239,12 @@ class RecipientDatabaseTest_getAndPossiblyMerge {
val existingE164Recipient = Recipient.resolved(existingE164Id) val existingE164Recipient = Recipient.resolved(existingE164Id)
assertEquals(mergedId, existingE164Recipient.id) assertEquals(mergedId, existingE164Recipient.id)
changeNumberListener.waitForJobManager() // TODO [greyson] Change number
assertFalse(changeNumberListener.numberChangeWasEnqueued)
} }
/** Same as [getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge], but with a number change. */ /** Same as [getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge], but with a number change. */
@Test @Test
fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge_changedNumber() { fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_merge_changedNumber() {
val changeNumberListener = ChangeNumberListener()
changeNumberListener.enqueue()
val existingAciId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B) val existingAciId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B)
val existingE164Id: RecipientId = recipientDatabase.getAndPossiblyMerge(null, E164_A) val existingE164Id: RecipientId = recipientDatabase.getAndPossiblyMerge(null, E164_A)
@ -268,16 +258,12 @@ class RecipientDatabaseTest_getAndPossiblyMerge {
val existingE164Recipient = Recipient.resolved(existingE164Id) val existingE164Recipient = Recipient.resolved(existingE164Id)
assertEquals(retrievedId, existingE164Recipient.id) assertEquals(retrievedId, existingE164Recipient.id)
changeNumberListener.waitForJobManager() // TODO [greyson] Change number
assert(changeNumberListener.numberChangeWasEnqueued)
} }
/** No new rules here, just a more complex scenario to show how different rules interact. */ /** No new rules here, just a more complex scenario to show how different rules interact. */
@Test @Test
fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_complex() { fun getAndPossiblyMerge_bothAciAndE164MapToExistingUser_aciAndE164_complex() {
val changeNumberListener = ChangeNumberListener()
changeNumberListener.enqueue()
val existingId1: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B) val existingId1: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B)
val existingId2: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_B, E164_A) val existingId2: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_B, E164_A)
@ -292,8 +278,7 @@ class RecipientDatabaseTest_getAndPossiblyMerge {
assertEquals(ACI_B, existingRecipient2.requireServiceId()) assertEquals(ACI_B, existingRecipient2.requireServiceId())
assertFalse(existingRecipient2.hasE164()) assertFalse(existingRecipient2.hasE164())
changeNumberListener.waitForJobManager() // TODO [greyson] Change number
assert(changeNumberListener.numberChangeWasEnqueued)
} }
/** /**
@ -383,9 +368,6 @@ class RecipientDatabaseTest_getAndPossiblyMerge {
/** Verifying a case where a change number job is expected to be enqueued. */ /** Verifying a case where a change number job is expected to be enqueued. */
@Test @Test
fun getAndPossiblyMerge_aciMapsToExistingUserButE164DoesNot_changedNumber() { fun getAndPossiblyMerge_aciMapsToExistingUserButE164DoesNot_changedNumber() {
val changeNumberListener = ChangeNumberListener()
changeNumberListener.enqueue()
val existingId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_A) val existingId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_A)
val retrievedId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B) val retrievedId: RecipientId = recipientDatabase.getAndPossiblyMerge(ACI_A, E164_B)
@ -395,8 +377,7 @@ class RecipientDatabaseTest_getAndPossiblyMerge {
assertEquals(ACI_A, retrievedRecipient.requireServiceId()) assertEquals(ACI_A, retrievedRecipient.requireServiceId())
assertEquals(E164_B, retrievedRecipient.requireE164()) assertEquals(E164_B, retrievedRecipient.requireE164())
changeNumberListener.waitForJobManager() // TODO [greyson] Change number
assert(changeNumberListener.numberChangeWasEnqueued)
} }
/** High trust lets you merge two different users into one. You should prefer the ACI user. Not shown: merging threads, dropping e164 sessions, etc. */ /** High trust lets you merge two different users into one. You should prefer the ACI user. Not shown: merging threads, dropping e164 sessions, etc. */
@ -630,24 +611,6 @@ class RecipientDatabaseTest_getAndPossiblyMerge {
val threadId: Long val threadId: Long
) )
private class ChangeNumberListener {
var numberChangeWasEnqueued = false
private set
fun waitForJobManager() {
ApplicationDependencies.getJobManager().flush()
ThreadUtil.sleep(500)
}
fun enqueue() {
ApplicationDependencies.getJobManager().addListener(
{ job -> job.factoryKey == RecipientChangedNumberJob.KEY },
{ _, _ -> numberChangeWasEnqueued = true }
)
}
}
companion object { companion object {
val ACI_A = ACI.from(UUID.fromString("3436efbe-5a76-47fa-a98a-7e72c948a82e")) val ACI_A = ACI.from(UUID.fromString("3436efbe-5a76-47fa-a98a-7e72c948a82e"))
val ACI_B = ACI.from(UUID.fromString("8de7f691-0b60-4a68-9cd9-ed2f8453f9ed")) val ACI_B = ACI.from(UUID.fromString("8de7f691-0b60-4a68-9cd9-ed2f8453f9ed"))

Wyświetl plik

@ -75,7 +75,6 @@ import org.thoughtcrime.securesms.groups.GroupId.V1
import org.thoughtcrime.securesms.groups.GroupId.V2 import org.thoughtcrime.securesms.groups.GroupId.V2
import org.thoughtcrime.securesms.groups.v2.ProfileKeySet import org.thoughtcrime.securesms.groups.v2.ProfileKeySet
import org.thoughtcrime.securesms.groups.v2.processing.GroupsV2StateProcessor import org.thoughtcrime.securesms.groups.v2.processing.GroupsV2StateProcessor
import org.thoughtcrime.securesms.jobs.RecipientChangedNumberJob
import org.thoughtcrime.securesms.jobs.RequestGroupV2InfoJob import org.thoughtcrime.securesms.jobs.RequestGroupV2InfoJob
import org.thoughtcrime.securesms.jobs.RetrieveProfileJob import org.thoughtcrime.securesms.jobs.RetrieveProfileJob
import org.thoughtcrime.securesms.keyvalue.SignalStore import org.thoughtcrime.securesms.keyvalue.SignalStore
@ -490,10 +489,6 @@ open class RecipientDatabase(context: Context, databaseHelper: SignalDatabase) :
StorageSyncHelper.scheduleSyncForDataChange() StorageSyncHelper.scheduleSyncForDataChange()
RecipientId.clearCache() RecipientId.clearCache()
} }
if (result.changedNumberId != null) {
ApplicationDependencies.getJobManager().add(RecipientChangedNumberJob(result.changedNumberId!!))
}
} }
} }

Wyświetl plik

@ -157,7 +157,6 @@ public final class JobManagerFactories {
put(PushProcessMessageJob.KEY, new PushProcessMessageJob.Factory()); put(PushProcessMessageJob.KEY, new PushProcessMessageJob.Factory());
put(PushTextSendJob.KEY, new PushTextSendJob.Factory()); put(PushTextSendJob.KEY, new PushTextSendJob.Factory());
put(ReactionSendJob.KEY, new ReactionSendJob.Factory()); put(ReactionSendJob.KEY, new ReactionSendJob.Factory());
put(RecipientChangedNumberJob.KEY, new RecipientChangedNumberJob.Factory());
put(RefreshAttributesJob.KEY, new RefreshAttributesJob.Factory()); put(RefreshAttributesJob.KEY, new RefreshAttributesJob.Factory());
put(RefreshOwnProfileJob.KEY, new RefreshOwnProfileJob.Factory()); put(RefreshOwnProfileJob.KEY, new RefreshOwnProfileJob.Factory());
put(RemoteConfigRefreshJob.KEY, new RemoteConfigRefreshJob.Factory()); put(RemoteConfigRefreshJob.KEY, new RemoteConfigRefreshJob.Factory());
@ -254,6 +253,7 @@ public final class JobManagerFactories {
put("RotateSignedPreKeyJob", new PreKeysSyncJob.Factory()); put("RotateSignedPreKeyJob", new PreKeysSyncJob.Factory());
put("CreateSignedPreKeyJob", new PreKeysSyncJob.Factory()); put("CreateSignedPreKeyJob", new PreKeysSyncJob.Factory());
put("RefreshPreKeysJob", new PreKeysSyncJob.Factory()); put("RefreshPreKeysJob", new PreKeysSyncJob.Factory());
put("RecipientChangedNumberJob", new FailingJob.Factory());
}}; }};
} }

Wyświetl plik

@ -1,57 +0,0 @@
package org.thoughtcrime.securesms.jobs
import org.signal.core.util.logging.Log
import org.thoughtcrime.securesms.database.SignalDatabase
import org.thoughtcrime.securesms.jobmanager.Data
import org.thoughtcrime.securesms.jobmanager.Job
import org.thoughtcrime.securesms.recipients.Recipient
import org.thoughtcrime.securesms.recipients.RecipientId
/**
* Insert change number update items in all threads (1:1 and group) with [recipientId].
*/
class RecipientChangedNumberJob(parameters: Parameters, private val recipientId: RecipientId) : BaseJob(parameters) {
constructor(recipientId: RecipientId) : this(
Parameters.Builder().setQueue("RecipientChangedNumberJob_${recipientId.toQueueKey()}").build(),
recipientId
)
override fun serialize(): Data {
return Data.Builder()
.putString(KEY_RECIPIENT_ID, recipientId.serialize())
.build()
}
override fun getFactoryKey(): String {
return KEY
}
override fun onRun() {
val recipient: Recipient = Recipient.resolved(recipientId)
if (!recipient.isBlocked && !recipient.isGroup && !recipient.isSelf) {
Log.i(TAG, "Writing a number change event.")
SignalDatabase.sms.insertNumberChangeMessages(recipient.id)
} else {
Log.i(TAG, "Number changed but not relevant. blocked: ${recipient.isBlocked} isGroup: ${recipient.isGroup} isSelf: ${recipient.isSelf}")
}
}
override fun onShouldRetry(e: Exception): Boolean = false
override fun onFailure() = Unit
class Factory : Job.Factory<RecipientChangedNumberJob> {
override fun create(parameters: Parameters, data: Data): RecipientChangedNumberJob {
return RecipientChangedNumberJob(parameters, RecipientId.from(data.getString(KEY_RECIPIENT_ID)))
}
}
companion object {
const val KEY = "RecipientChangedNumberJob"
private val TAG = Log.tag(RecipientChangedNumberJob::class.java)
private const val KEY_RECIPIENT_ID = "recipient_id"
}
}