From 4e2afa7362a99a2e34ad1a56b3b657bcacf0a6b0 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 26 Sep 2019 10:53:46 -0400 Subject: [PATCH] Fix some more broken JobMigrations. --- .../securesms/jobmanager/JobManager.java | 2 +- .../securesms/jobmanager/JobMigration.java | 4 + .../RecipientIdFollowUpJobMigration.java | 51 +++++++++++ .../migrations/RecipientIdJobMigration.java | 9 +- .../securesms/jobs/JobManagerFactories.java | 4 +- .../RecipientIdFollowUpJobMigrationTest.java | 90 +++++++++++++++++++ .../RecipientIdJobMigrationTest.java | 75 ++++++++++++++-- 7 files changed, 224 insertions(+), 11 deletions(-) create mode 100644 src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigration.java create mode 100644 test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigrationTest.java diff --git a/src/org/thoughtcrime/securesms/jobmanager/JobManager.java b/src/org/thoughtcrime/securesms/jobmanager/JobManager.java index 2dca45233e..e6bd64e20f 100644 --- a/src/org/thoughtcrime/securesms/jobmanager/JobManager.java +++ b/src/org/thoughtcrime/securesms/jobmanager/JobManager.java @@ -33,7 +33,7 @@ public class JobManager implements ConstraintObserver.Notifier { private static final String TAG = JobManager.class.getSimpleName(); - public static final int CURRENT_VERSION = 2; + public static final int CURRENT_VERSION = 3; private final Application application; private final Configuration configuration; diff --git a/src/org/thoughtcrime/securesms/jobmanager/JobMigration.java b/src/org/thoughtcrime/securesms/jobmanager/JobMigration.java index 1f45a5ef3d..216d2eb0e8 100644 --- a/src/org/thoughtcrime/securesms/jobmanager/JobMigration.java +++ b/src/org/thoughtcrime/securesms/jobmanager/JobMigration.java @@ -39,6 +39,10 @@ public abstract class JobMigration { this.data = data; } + public @NonNull JobData withFactoryKey(@NonNull String newFactoryKey) { + return new JobData(newFactoryKey, queueKey, data); + } + public @NonNull JobData withQueueKey(@Nullable String newQueueKey) { return new JobData(factoryKey, newQueueKey, data); } diff --git a/src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigration.java b/src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigration.java new file mode 100644 index 0000000000..4207c6da0a --- /dev/null +++ b/src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigration.java @@ -0,0 +1,51 @@ +package org.thoughtcrime.securesms.jobmanager.migrations; + +import androidx.annotation.NonNull; + +import org.thoughtcrime.securesms.jobmanager.Data; +import org.thoughtcrime.securesms.jobmanager.JobMigration; + +/** + * Fixes things that went wrong in {@link RecipientIdJobMigration}. In particular, some jobs didn't + * have some necessary data fields carried over. Thankfully they're relatively non-critical, so + * we'll just swap them out with failing jobs if they're missing something. + */ +public class RecipientIdFollowUpJobMigration extends JobMigration { + + public RecipientIdFollowUpJobMigration() { + super(3); + } + + @Override + protected @NonNull JobData migrate(@NonNull JobData jobData) { + switch(jobData.getFactoryKey()) { + case "RequestGroupInfoJob": return migrateRequestGroupInfoJob(jobData); + case "SendDeliveryReceiptJob": return migrateSendDeliveryReceiptJob(jobData); + default: + return jobData; + } + } + + private static @NonNull JobData migrateRequestGroupInfoJob(@NonNull JobData jobData) { + if (!jobData.getData().hasString("source") || !jobData.getData().hasString("group_id")) { + return failingJobData(); + } else { + return jobData; + } + } + + private static @NonNull JobData migrateSendDeliveryReceiptJob(@NonNull JobData jobData) { + if (!jobData.getData().hasString("recipient") || + !jobData.getData().hasLong("message_id") || + !jobData.getData().hasLong("timestamp")) + { + return failingJobData(); + } else { + return jobData; + } + } + + private static JobData failingJobData() { + return new JobData("FailingJob", null, new Data.Builder().build()); + } +} diff --git a/src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigration.java b/src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigration.java index a984ca0d49..6440b83a69 100644 --- a/src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigration.java +++ b/src/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigration.java @@ -73,7 +73,9 @@ public class RecipientIdJobMigration extends JobMigration { private @NonNull JobData migrateRequestGroupInfoJob(@NonNull JobData jobData) { String address = jobData.getData().getString("source"); Recipient recipient = Recipient.external(application, address); - Data updatedData = new Data.Builder().putString("source", recipient.getId().serialize()).build(); + Data updatedData = new Data.Builder().putString("source", recipient.getId().serialize()) + .putString("group_id", jobData.getData().getString("group_id")) + .build(); return jobData.withData(updatedData); } @@ -81,7 +83,10 @@ public class RecipientIdJobMigration extends JobMigration { private @NonNull JobData migrateSendDeliveryReceiptJob(@NonNull JobData jobData) { String address = jobData.getData().getString("address"); Recipient recipient = Recipient.external(application, address); - Data updatedData = new Data.Builder().putString("recipient", recipient.getId().serialize()).build(); + Data updatedData = new Data.Builder().putString("recipient", recipient.getId().serialize()) + .putLong("message_id", jobData.getData().getLong("message_id")) + .putLong("timestamp", jobData.getData().getLong("timestamp")) + .build(); return jobData.withData(updatedData); } diff --git a/src/org/thoughtcrime/securesms/jobs/JobManagerFactories.java b/src/org/thoughtcrime/securesms/jobs/JobManagerFactories.java index fcc5836e1f..643b789e0c 100644 --- a/src/org/thoughtcrime/securesms/jobs/JobManagerFactories.java +++ b/src/org/thoughtcrime/securesms/jobs/JobManagerFactories.java @@ -14,6 +14,7 @@ import org.thoughtcrime.securesms.jobmanager.impl.NetworkConstraintObserver; import org.thoughtcrime.securesms.jobmanager.impl.NetworkOrCellServiceConstraint; import org.thoughtcrime.securesms.jobmanager.impl.SqlCipherMigrationConstraint; import org.thoughtcrime.securesms.jobmanager.impl.SqlCipherMigrationConstraintObserver; +import org.thoughtcrime.securesms.jobmanager.migrations.RecipientIdFollowUpJobMigration; import org.thoughtcrime.securesms.jobmanager.migrations.RecipientIdJobMigration; import org.thoughtcrime.securesms.migrations.DatabaseMigrationJob; import org.thoughtcrime.securesms.migrations.LegacyMigrationJob; @@ -108,6 +109,7 @@ public final class JobManagerFactories { } public static List getJobMigrations(@NonNull Application application) { - return Arrays.asList(new RecipientIdJobMigration(application)); + return Arrays.asList(new RecipientIdJobMigration(application), + new RecipientIdFollowUpJobMigration()); } } diff --git a/test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigrationTest.java b/test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigrationTest.java new file mode 100644 index 0000000000..1556def4e7 --- /dev/null +++ b/test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdFollowUpJobMigrationTest.java @@ -0,0 +1,90 @@ +package org.thoughtcrime.securesms.jobmanager.migrations; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; +import org.thoughtcrime.securesms.jobmanager.Data; +import org.thoughtcrime.securesms.jobmanager.Job; +import org.thoughtcrime.securesms.jobmanager.JobMigration.JobData; +import org.thoughtcrime.securesms.jobs.FailingJob; +import org.thoughtcrime.securesms.jobs.RequestGroupInfoJob; +import org.thoughtcrime.securesms.jobs.SendDeliveryReceiptJob; +import org.thoughtcrime.securesms.recipients.Recipient; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.powermock.api.mockito.PowerMockito.mock; +import static org.powermock.api.mockito.PowerMockito.mockStatic; + +@RunWith(PowerMockRunner.class) +@PrepareForTest({ Recipient.class, Job.Parameters.class }) +public class RecipientIdFollowUpJobMigrationTest { + + @Before + public void init() { + mockStatic(Recipient.class); + mockStatic(Job.Parameters.class); + } + + @Test + public void migrate_requestGroupInfoJob_good() throws Exception { + JobData testData = new JobData("RequestGroupInfoJob", null, new Data.Builder().putString("source", "1") + .putString("group_id", "__textsecure_group__!abcd") + .build()); + RecipientIdFollowUpJobMigration subject = new RecipientIdFollowUpJobMigration(); + JobData converted = subject.migrate(testData); + + assertEquals("RequestGroupInfoJob", converted.getFactoryKey()); + assertNull(converted.getQueueKey()); + assertEquals("1", converted.getData().getString("source")); + assertEquals("__textsecure_group__!abcd", converted.getData().getString("group_id")); + + new RequestGroupInfoJob.Factory().create(mock(Job.Parameters.class), converted.getData()); + } + + @Test + public void migrate_requestGroupInfoJob_bad() throws Exception { + JobData testData = new JobData("RequestGroupInfoJob", null, new Data.Builder().putString("source", "1") + .build()); + RecipientIdFollowUpJobMigration subject = new RecipientIdFollowUpJobMigration(); + JobData converted = subject.migrate(testData); + + assertEquals("FailingJob", converted.getFactoryKey()); + assertNull(converted.getQueueKey()); + + new FailingJob.Factory().create(mock(Job.Parameters.class), converted.getData()); + } + + @Test + public void migrate_sendDeliveryReceiptJob_good() throws Exception { + JobData testData = new JobData("SendDeliveryReceiptJob", null, new Data.Builder().putString("recipient", "1") + .putLong("message_id", 1) + .putLong("timestamp", 2) + .build()); + RecipientIdFollowUpJobMigration subject = new RecipientIdFollowUpJobMigration(); + JobData converted = subject.migrate(testData); + + assertEquals("SendDeliveryReceiptJob", converted.getFactoryKey()); + assertNull(converted.getQueueKey()); + assertEquals("1", converted.getData().getString("recipient")); + assertEquals(1, converted.getData().getLong("message_id")); + assertEquals(2, converted.getData().getLong("timestamp")); + + new SendDeliveryReceiptJob.Factory().create(mock(Job.Parameters.class), converted.getData()); + } + + @Test + public void migrate_sendDeliveryReceiptJob_bad() throws Exception { + JobData testData = new JobData("SendDeliveryReceiptJob", null, new Data.Builder().putString("recipient", "1") + .build()); + RecipientIdFollowUpJobMigration subject = new RecipientIdFollowUpJobMigration(); + JobData converted = subject.migrate(testData); + + assertEquals("FailingJob", converted.getFactoryKey()); + assertNull(converted.getQueueKey()); + + new FailingJob.Factory().create(mock(Job.Parameters.class), converted.getData()); + } +} diff --git a/test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigrationTest.java b/test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigrationTest.java index 81ca9f9338..437a421600 100644 --- a/test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigrationTest.java +++ b/test/unitTest/java/org/thoughtcrime/securesms/jobmanager/migrations/RecipientIdJobMigrationTest.java @@ -9,9 +9,24 @@ import org.junit.runner.RunWith; import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; import org.thoughtcrime.securesms.jobmanager.Data; +import org.thoughtcrime.securesms.jobmanager.Job; import org.thoughtcrime.securesms.jobmanager.JobMigration.JobData; import org.thoughtcrime.securesms.jobmanager.migrations.RecipientIdJobMigration.NewSerializableSyncMessageId; import org.thoughtcrime.securesms.jobmanager.migrations.RecipientIdJobMigration.OldSerializableSyncMessageId; +import org.thoughtcrime.securesms.jobs.DirectoryRefreshJob; +import org.thoughtcrime.securesms.jobs.MultiDeviceContactUpdateJob; +import org.thoughtcrime.securesms.jobs.MultiDeviceReadUpdateJob; +import org.thoughtcrime.securesms.jobs.MultiDeviceVerifiedUpdateJob; +import org.thoughtcrime.securesms.jobs.MultiDeviceViewOnceOpenJob; +import org.thoughtcrime.securesms.jobs.PushGroupSendJob; +import org.thoughtcrime.securesms.jobs.PushGroupUpdateJob; +import org.thoughtcrime.securesms.jobs.PushMediaSendJob; +import org.thoughtcrime.securesms.jobs.PushTextSendJob; +import org.thoughtcrime.securesms.jobs.RequestGroupInfoJob; +import org.thoughtcrime.securesms.jobs.RetrieveProfileAvatarJob; +import org.thoughtcrime.securesms.jobs.RetrieveProfileJob; +import org.thoughtcrime.securesms.jobs.SendDeliveryReceiptJob; +import org.thoughtcrime.securesms.jobs.SmsSendJob; import org.thoughtcrime.securesms.recipients.Recipient; import org.thoughtcrime.securesms.recipients.RecipientId; import org.thoughtcrime.securesms.util.JsonUtils; @@ -28,12 +43,13 @@ import static org.powermock.api.mockito.PowerMockito.mockStatic; import static org.powermock.api.mockito.PowerMockito.when; @RunWith(PowerMockRunner.class) -@PrepareForTest({ Recipient.class }) +@PrepareForTest({ Recipient.class, Job.Parameters.class }) public class RecipientIdJobMigrationTest { @Before public void init() { mockStatic(Recipient.class); + mockStatic(Job.Parameters.class); } @Test @@ -49,6 +65,8 @@ public class RecipientIdJobMigrationTest { assertFalse(converted.getData().getBoolean("force_sync")); assertFalse(converted.getData().hasString("address")); assertEquals("1", converted.getData().getString("recipient")); + + new MultiDeviceContactUpdateJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -63,11 +81,15 @@ public class RecipientIdJobMigrationTest { assertEquals("MultiDeviceRevealUpdateJob", converted.getFactoryKey()); assertNull(converted.getQueueKey()); assertEquals(JsonUtils.toJson(new NewSerializableSyncMessageId("1", 1)), converted.getData().getString("message_id")); + + new MultiDeviceViewOnceOpenJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test public void migrate_requestGroupInfoJob() throws Exception { - JobData testData = new JobData("RequestGroupInfoJob", null, new Data.Builder().putString("source", "+16101234567").build()); + JobData testData = new JobData("RequestGroupInfoJob", null, new Data.Builder().putString("source", "+16101234567") + .putString("group_id", "__textsecure_group__!abcd") + .build()); mockRecipientResolve("+16101234567", 1); RecipientIdJobMigration subject = new RecipientIdJobMigration(mock(Application.class)); @@ -76,11 +98,17 @@ public class RecipientIdJobMigrationTest { assertEquals("RequestGroupInfoJob", converted.getFactoryKey()); assertNull(converted.getQueueKey()); assertEquals("1", converted.getData().getString("source")); + assertEquals("__textsecure_group__!abcd", converted.getData().getString("group_id")); + + new RequestGroupInfoJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test public void migrate_sendDeliveryReceiptJob() throws Exception { - JobData testData = new JobData("SendDeliveryReceiptJob", null, new Data.Builder().putString("address", "+16101234567").build()); + JobData testData = new JobData("SendDeliveryReceiptJob", null, new Data.Builder().putString("address", "+16101234567") + .putLong("message_id", 1) + .putLong("timestamp", 2) + .build()); mockRecipientResolve("+16101234567", 1); RecipientIdJobMigration subject = new RecipientIdJobMigration(mock(Application.class)); @@ -89,12 +117,16 @@ public class RecipientIdJobMigrationTest { assertEquals("SendDeliveryReceiptJob", converted.getFactoryKey()); assertNull(converted.getQueueKey()); assertEquals("1", converted.getData().getString("recipient")); + assertEquals(1, converted.getData().getLong("message_id")); + assertEquals(2, converted.getData().getLong("timestamp")); + + new SendDeliveryReceiptJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test public void migrate_multiDeviceVerifiedUpdateJob() throws Exception { JobData testData = new JobData("MultiDeviceVerifiedUpdateJob", "__MULTI_DEVICE_VERIFIED_UPDATE__", new Data.Builder().putString("destination", "+16101234567") - .putString("identity_key", "abc") + .putString("identity_key", "abcd") .putInt("verified_status", 1) .putLong("timestamp", 123) .build()); @@ -105,10 +137,12 @@ public class RecipientIdJobMigrationTest { assertEquals("MultiDeviceVerifiedUpdateJob", converted.getFactoryKey()); assertEquals("__MULTI_DEVICE_VERIFIED_UPDATE__", converted.getQueueKey()); - assertEquals("abc", converted.getData().getString("identity_key")); + assertEquals("abcd", converted.getData().getString("identity_key")); assertEquals(1, converted.getData().getInt("verified_status")); assertEquals(123, converted.getData().getLong("timestamp")); assertEquals("1", converted.getData().getString("destination")); + + new MultiDeviceVerifiedUpdateJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -122,6 +156,8 @@ public class RecipientIdJobMigrationTest { assertEquals("RetrieveProfileJob", converted.getFactoryKey()); assertNull(converted.getQueueKey()); assertEquals("1", converted.getData().getString("recipient")); + + new RetrieveProfileJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -138,6 +174,8 @@ public class RecipientIdJobMigrationTest { assertEquals(RecipientId.from(5).toQueueKey(), converted.getQueueKey()); assertNull(converted.getData().getString("filter_recipient")); assertFalse(converted.getData().hasString("filter_address")); + + new PushGroupSendJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -155,11 +193,15 @@ public class RecipientIdJobMigrationTest { assertEquals(RecipientId.from(5).toQueueKey(), converted.getQueueKey()); assertEquals("1", converted.getData().getString("filter_recipient")); assertFalse(converted.getData().hasString("filter_address")); + + new PushGroupSendJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test public void migrate_pushGroupUpdateJob() throws Exception { - JobData testData = new JobData("PushGroupUpdateJob", null, new Data.Builder().putString("source", "+16101234567").putString("group_id", "abc").build()); + JobData testData = new JobData("PushGroupUpdateJob", null, new Data.Builder().putString("source", "+16101234567") + .putString("group_id", "__textsecure_group__!abcd") + .build()); mockRecipientResolve("+16101234567", 1); RecipientIdJobMigration subject = new RecipientIdJobMigration(mock(Application.class)); @@ -168,7 +210,9 @@ public class RecipientIdJobMigrationTest { assertEquals("PushGroupUpdateJob", converted.getFactoryKey()); assertNull(converted.getQueueKey()); assertEquals("1", converted.getData().getString("source")); - assertEquals("abc", converted.getData().getString("group_id")); + assertEquals("__textsecure_group__!abcd", converted.getData().getString("group_id")); + + new PushGroupUpdateJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -183,6 +227,8 @@ public class RecipientIdJobMigrationTest { assertNull(converted.getData().getString("recipient")); assertTrue(converted.getData().getBoolean("notify_of_new_users")); assertFalse(converted.getData().hasString("address")); + + new DirectoryRefreshJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -198,6 +244,8 @@ public class RecipientIdJobMigrationTest { assertTrue(converted.getData().getBoolean("notify_of_new_users")); assertEquals("1", converted.getData().getString("recipient")); assertFalse(converted.getData().hasString("address")); + + new DirectoryRefreshJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -211,6 +259,9 @@ public class RecipientIdJobMigrationTest { assertEquals("RetrieveProfileAvatarJob", converted.getFactoryKey()); assertEquals("RetrieveProfileAvatarJob::" + RecipientId.from(1).toQueueKey(), converted.getQueueKey()); assertEquals("1", converted.getData().getString("recipient")); + + + new RetrieveProfileAvatarJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -223,6 +274,8 @@ public class RecipientIdJobMigrationTest { assertEquals("MultiDeviceReadUpdateJob", converted.getFactoryKey()); assertNull(converted.getQueueKey()); assertEquals(0, converted.getData().getStringArray("message_ids").length); + + new MultiDeviceReadUpdateJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -245,6 +298,8 @@ public class RecipientIdJobMigrationTest { assertEquals(JsonUtils.toJson(new NewSerializableSyncMessageId("1", 1)), updated[0]); assertEquals(JsonUtils.toJson(new NewSerializableSyncMessageId("2", 2)), updated[1]); + + new MultiDeviceReadUpdateJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -258,6 +313,8 @@ public class RecipientIdJobMigrationTest { assertEquals("PushTextSendJob", converted.getFactoryKey()); assertEquals(RecipientId.from(1).toQueueKey(), converted.getQueueKey()); assertEquals(1, converted.getData().getLong("message_id")); + + new PushTextSendJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -271,6 +328,8 @@ public class RecipientIdJobMigrationTest { assertEquals("PushMediaSendJob", converted.getFactoryKey()); assertEquals(RecipientId.from(1).toQueueKey(), converted.getQueueKey()); assertEquals(1, converted.getData().getLong("message_id")); + + new PushMediaSendJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } @Test @@ -285,6 +344,8 @@ public class RecipientIdJobMigrationTest { assertEquals(RecipientId.from(1).toQueueKey(), converted.getQueueKey()); assertEquals(1, converted.getData().getLong("message_id")); assertEquals(0, converted.getData().getInt("run_attempt")); + + new SmsSendJob.Factory().create(mock(Job.Parameters.class), converted.getData()); } private void mockRecipientResolve(String address, long recipientId) throws Exception {