From 8bb3a83ccb0b9f5f4d8c32be4e1a498245a24153 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 22 Jun 2022 12:18:48 +0100 Subject: [PATCH] Add tests for contact list worker. --- .../mailbox/OwnMailboxContactListWorker.java | 22 +- .../OwnMailboxContactListWorkerTest.java | 406 ++++++++++++++++++ 2 files changed, 426 insertions(+), 2 deletions(-) create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorkerTest.java diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java index d457f6026..f1f520d63 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java @@ -90,6 +90,9 @@ class OwnMailboxContactListWorker @Nullable private Cancellable apiCall = null; + /** + * A queue of updates waiting to be applied to the remote contact list. + */ @GuardedBy("lock") private final Queue updates = new LinkedList<>(); @@ -101,6 +104,7 @@ class OwnMailboxContactListWorker MailboxApi mailboxApi, MailboxUpdateManager mailboxUpdateManager, MailboxProperties mailboxProperties) { + if (!mailboxProperties.isOwner()) throw new IllegalArgumentException(); this.ioExecutor = ioExecutor; this.db = db; this.connectivityChecker = connectivityChecker; @@ -290,12 +294,17 @@ class OwnMailboxContactListWorker @IoExecutor private void apiCallRemoveContact(ContactId c) - throws IOException, ApiException, TolerableFailureException { + throws IOException, ApiException { synchronized (lock) { if (state != State.UPDATING_CONTACT_LIST) return; } LOG.info("Removing contact from remote contact list"); - mailboxApi.deleteContact(mailboxProperties, c); + try { + mailboxApi.deleteContact(mailboxProperties, c); + } catch (TolerableFailureException e) { + // Catch this so we can continue to the next update + logException(LOG, INFO, e); + } updateContactList(); } @@ -319,6 +328,7 @@ class OwnMailboxContactListWorker } updates.add(new Update(true, c)); if (state == State.WAITING_FOR_CHANGES) { + state = State.UPDATING_CONTACT_LIST; ioExecutor.execute(this::updateContactList); } } @@ -333,13 +343,21 @@ class OwnMailboxContactListWorker } updates.add(new Update(false, c)); if (state == State.WAITING_FOR_CHANGES) { + state = State.UPDATING_CONTACT_LIST; ioExecutor.execute(this::updateContactList); } } } + /** + * An update that should be applied to the remote contact list. + */ private static class Update { + /** + * True if the contact should be added, false if the contact should be + * removed. + */ private final boolean add; private final ContactId contactId; diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorkerTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorkerTest.java new file mode 100644 index 000000000..12232122e --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorkerTest.java @@ -0,0 +1,406 @@ +package org.briarproject.bramble.mailbox; + +import org.briarproject.bramble.api.Cancellable; +import org.briarproject.bramble.api.contact.Contact; +import org.briarproject.bramble.api.contact.ContactId; +import org.briarproject.bramble.api.contact.event.ContactAddedEvent; +import org.briarproject.bramble.api.contact.event.ContactRemovedEvent; +import org.briarproject.bramble.api.db.DatabaseComponent; +import org.briarproject.bramble.api.db.NoSuchContactException; +import org.briarproject.bramble.api.db.Transaction; +import org.briarproject.bramble.api.event.EventBus; +import org.briarproject.bramble.api.mailbox.MailboxProperties; +import org.briarproject.bramble.api.mailbox.MailboxUpdate; +import org.briarproject.bramble.api.mailbox.MailboxUpdateManager; +import org.briarproject.bramble.api.mailbox.MailboxUpdateWithMailbox; +import org.briarproject.bramble.mailbox.MailboxApi.MailboxContact; +import org.briarproject.bramble.mailbox.MailboxApi.TolerableFailureException; +import org.briarproject.bramble.test.BrambleMockTestCase; +import org.briarproject.bramble.test.CaptureArgumentAction; +import org.briarproject.bramble.test.DbExpectations; +import org.briarproject.bramble.test.RunAction; +import org.jmock.Expectations; +import org.jmock.lib.action.DoAllAction; +import org.junit.Test; + +import java.util.List; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicReference; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static org.briarproject.bramble.api.mailbox.MailboxConstants.CLIENT_SUPPORTS; +import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; +import static org.briarproject.bramble.test.TestUtils.getContact; +import static org.briarproject.bramble.test.TestUtils.getMailboxProperties; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; + +public class OwnMailboxContactListWorkerTest extends BrambleMockTestCase { + + private final Executor ioExecutor = context.mock(Executor.class); + private final DatabaseComponent db = context.mock(DatabaseComponent.class); + private final EventBus eventBus = context.mock(EventBus.class); + private final ConnectivityChecker connectivityChecker = + context.mock(ConnectivityChecker.class); + private final MailboxApiCaller mailboxApiCaller = + context.mock(MailboxApiCaller.class); + private final MailboxApi mailboxApi = context.mock(MailboxApi.class); + private final MailboxUpdateManager mailboxUpdateManager = + context.mock(MailboxUpdateManager.class); + private final Cancellable apiCall = context.mock(Cancellable.class); + + private final MailboxProperties mailboxProperties = + getMailboxProperties(true, CLIENT_SUPPORTS); + private final Contact contact1 = getContact(), contact2 = getContact(); + private final MailboxProperties contactProperties1 = + getMailboxProperties(false, CLIENT_SUPPORTS); + private final MailboxProperties contactProperties2 = + getMailboxProperties(false, CLIENT_SUPPORTS); + private final MailboxUpdateWithMailbox update1 = + new MailboxUpdateWithMailbox(CLIENT_SUPPORTS, contactProperties1); + private final MailboxUpdateWithMailbox update2 = + new MailboxUpdateWithMailbox(CLIENT_SUPPORTS, contactProperties2); + + private final OwnMailboxContactListWorker worker = + new OwnMailboxContactListWorker(ioExecutor, db, eventBus, + connectivityChecker, mailboxApiCaller, mailboxApi, + mailboxUpdateManager, mailboxProperties); + + @Test + public void testChecksConnectivityWhenStartedAndRemovesObserverWhenDestroyed() { + // When the worker is started it should start a connectivity check + expectStartConnectivityCheck(); + + worker.start(); + + // When the worker is destroyed it should remove the connectivity + // observer and event listener + expectRemoveConnectivityObserverAndEventListener(); + + worker.destroy(); + } + + @Test + public void testUpdatesContactListWhenConnectivityCheckSucceeds() + throws Exception { + // When the worker is started it should start a connectivity check + expectStartConnectivityCheck(); + + worker.start(); + + // When the connectivity check succeeds, the worker should start a + // task to fetch the remote contact list + AtomicReference fetchList = new AtomicReference<>(); + expectStartTaskToFetchRemoteContactList(fetchList); + + worker.onConnectivityCheckSucceeded(); + + // When the fetch task runs it should fetch the remote contact list, + // load the local contact list, and find the differences. Contact 2 + // needs to be added and contact 1 needs to be removed. The worker + // should load the mailbox update for contact 2 and start a task to + // add contact 2 to the mailbox + expectFetchRemoteContactList(singletonList(contact1.getId())); + expectRunTaskOnIoExecutor(); + expectLoadLocalContactList(singletonList(contact2)); + expectRunTaskOnIoExecutor(); + expectLoadMailboxUpdate(contact2, update2); + AtomicReference addContact = new AtomicReference<>(); + expectStartTaskToAddContact(addContact); + + assertFalse(fetchList.get().callApi()); + + // When the add-contact task runs it should add contact 2 to the + // mailbox, then continue with the next update + AtomicReference added = new AtomicReference<>(); + expectAddContactToMailbox(added); + AtomicReference removeContact = new AtomicReference<>(); + expectStartTaskToRemoveContact(removeContact); + + assertFalse(addContact.get().callApi()); + + // Check that the added contact has the expected properties + MailboxContact expected = new MailboxContact(contact2.getId(), + contactProperties2.getAuthToken(), + requireNonNull(contactProperties2.getInboxId()), + requireNonNull(contactProperties2.getOutboxId())); + assertMailboxContactEquals(expected, added.get()); + + // When the remove-contact task runs it should remove contact 1 from + // the mailbox + expectRemoveContactFromMailbox(contact1); + + assertFalse(removeContact.get().callApi()); + + // When the worker is destroyed it should remove the connectivity + // observer and event listener + expectRemoveConnectivityObserverAndEventListener(); + + worker.destroy(); + } + + @Test + public void testHandlesEventsAfterMakingInitialUpdates() throws Exception { + // When the worker is started it should start a connectivity check + expectStartConnectivityCheck(); + + worker.start(); + + // When the connectivity check succeeds, the worker should start a + // task to fetch the remote contact list + AtomicReference fetchList = new AtomicReference<>(); + expectStartTaskToFetchRemoteContactList(fetchList); + + worker.onConnectivityCheckSucceeded(); + + // When the fetch task runs it should fetch the remote contact list, + // load the local contact list, and find the differences. The lists + // are the same, so the worker should wait for changes + expectFetchRemoteContactList(emptyList()); + expectRunTaskOnIoExecutor(); + expectLoadLocalContactList(emptyList()); + + assertFalse(fetchList.get().callApi()); + + // When a contact is added, the worker should load the contact's + // mailbox update and start a task to add the contact to the mailbox + expectRunTaskOnIoExecutor(); + expectLoadMailboxUpdate(contact1, update1); + AtomicReference addContact = new AtomicReference<>(); + expectStartTaskToAddContact(addContact); + + worker.eventOccurred(new ContactAddedEvent(contact1.getId(), true)); + + // When the add-contact task runs it should add contact 1 to the + // mailbox + AtomicReference added = new AtomicReference<>(); + expectAddContactToMailbox(added); + + assertFalse(addContact.get().callApi()); + + // Check that the added contact has the expected properties + MailboxContact expected = new MailboxContact(contact1.getId(), + contactProperties1.getAuthToken(), + requireNonNull(contactProperties1.getInboxId()), + requireNonNull(contactProperties1.getOutboxId())); + assertMailboxContactEquals(expected, added.get()); + + // When the contact is removed again, the worker should start a task + // to remove the contact from the mailbox + expectRunTaskOnIoExecutor(); + AtomicReference removeContact = new AtomicReference<>(); + expectStartTaskToRemoveContact(removeContact); + + worker.eventOccurred(new ContactRemovedEvent(contact1.getId())); + + // When the remove-contact task runs it should remove the contact from + // the mailbox + expectRemoveContactFromMailbox(contact1); + + assertFalse(removeContact.get().callApi()); + + // When the worker is destroyed it should remove the connectivity + // observer and event listener + expectRemoveConnectivityObserverAndEventListener(); + + worker.destroy(); + } + + + @Test + public void testHandlesNoSuchContactException() throws Exception { + // When the worker is started it should start a connectivity check + expectStartConnectivityCheck(); + + worker.start(); + + // When the connectivity check succeeds, the worker should start a + // task to fetch the remote contact list + AtomicReference fetchList = new AtomicReference<>(); + expectStartTaskToFetchRemoteContactList(fetchList); + + worker.onConnectivityCheckSucceeded(); + + // When the fetch task runs it should fetch the remote contact list, + // load the local contact list, and find the differences. Contact 1 + // needs to be added, so the worker should submit a task to the + // IO executor to load the contact's mailbox update + expectFetchRemoteContactList(emptyList()); + expectRunTaskOnIoExecutor(); + expectLoadLocalContactList(singletonList(contact1)); + AtomicReference loadUpdate = new AtomicReference<>(); + context.checking(new Expectations() {{ + oneOf(ioExecutor).execute(with(any(Runnable.class))); + will(new CaptureArgumentAction<>(loadUpdate, Runnable.class, 0)); + }}); + + assertFalse(fetchList.get().callApi()); + + // Before the contact's mailbox update can be loaded, the contact + // is removed + worker.eventOccurred(new ContactRemovedEvent(contact1.getId())); + + // When the load-update task runs, a NoSuchContactException is thrown. + // The worker should abandon adding the contact and move on to the + // next update, which is the removal of the same contact. The worker + // should start a task to remove the contact from the mailbox + Transaction txn = new Transaction(null, false); + context.checking(new DbExpectations() {{ + oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); + oneOf(mailboxUpdateManager).getLocalUpdate(txn, contact1.getId()); + will(throwException(new NoSuchContactException())); + }}); + AtomicReference removeContact = new AtomicReference<>(); + expectStartTaskToRemoveContact(removeContact); + + loadUpdate.get().run(); + + // When the remove-contact task runs it should remove the contact from + // the mailbox. The contact was never added, so the call throws a + // TolerableFailureException + context.checking(new Expectations() {{ + oneOf(mailboxApi).deleteContact(mailboxProperties, + contact1.getId()); + will(throwException(new TolerableFailureException())); + }}); + + assertFalse(removeContact.get().callApi()); + + // When the worker is destroyed it should remove the connectivity + // observer and event listener + expectRemoveConnectivityObserverAndEventListener(); + + worker.destroy(); + } + + @Test + public void testCancelsApiCallWhenDestroyed() { + // When the worker is started it should start a connectivity check + expectStartConnectivityCheck(); + + worker.start(); + + // When the connectivity check succeeds, the worker should start a + // task to fetch the remote contact list + AtomicReference fetchList = new AtomicReference<>(); + expectStartTaskToFetchRemoteContactList(fetchList); + + worker.onConnectivityCheckSucceeded(); + + // The worker is destroyed before the task runs. The worker should + // cancel the task remove the connectivity observer and event listener + context.checking(new Expectations() {{ + oneOf(apiCall).cancel(); + }}); + expectRemoveConnectivityObserverAndEventListener(); + + worker.destroy(); + } + + private void expectStartConnectivityCheck() { + context.checking(new Expectations() {{ + oneOf(connectivityChecker).checkConnectivity( + with(mailboxProperties), with(worker)); + }}); + } + + private void expectRemoveConnectivityObserverAndEventListener() { + context.checking(new Expectations() {{ + oneOf(connectivityChecker).removeObserver(worker); + oneOf(eventBus).removeListener(worker); + }}); + } + + private void expectStartTaskToFetchRemoteContactList( + AtomicReference task) { + context.checking(new Expectations() {{ + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new DoAllAction( + new CaptureArgumentAction<>(task, ApiCall.class, 0), + returnValue(apiCall) + )); + }}); + } + + private void expectFetchRemoteContactList(List remote) + throws Exception { + context.checking(new Expectations() {{ + oneOf(mailboxApi).getContacts(mailboxProperties); + will(returnValue(remote)); + }}); + } + + private void expectLoadLocalContactList(List local) + throws Exception { + Transaction txn = new Transaction(null, true); + + context.checking(new DbExpectations() {{ + oneOf(db).transaction(with(true), withDbRunnable(txn)); + oneOf(db).getContacts(txn); + will(returnValue(local)); + }}); + } + + private void expectLoadMailboxUpdate(Contact c, MailboxUpdate update) + throws Exception { + Transaction txn = new Transaction(null, true); + + context.checking(new DbExpectations() {{ + oneOf(db).transactionWithResult(with(true), + withDbCallable(txn)); + oneOf(mailboxUpdateManager).getLocalUpdate(txn, c.getId()); + will(returnValue(update)); + }}); + } + + private void expectStartTaskToAddContact(AtomicReference task) { + context.checking(new Expectations() {{ + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new DoAllAction( + new CaptureArgumentAction<>(task, ApiCall.class, 0), + returnValue(apiCall) + )); + }}); + } + + private void expectAddContactToMailbox( + AtomicReference added) throws Exception { + context.checking(new DbExpectations() {{ + oneOf(mailboxApi).addContact(with(mailboxProperties), + with(any(MailboxContact.class))); + will(new CaptureArgumentAction<>(added, MailboxContact.class, 1)); + }}); + } + + private void expectStartTaskToRemoveContact(AtomicReference task) { + context.checking(new DbExpectations() {{ + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new DoAllAction( + new CaptureArgumentAction<>(task, ApiCall.class, 0), + returnValue(apiCall) + )); + }}); + } + + private void expectRemoveContactFromMailbox(Contact c) throws Exception { + context.checking(new Expectations() {{ + oneOf(mailboxApi).deleteContact(mailboxProperties, c.getId()); + }}); + } + + private void expectRunTaskOnIoExecutor() { + context.checking(new Expectations() {{ + oneOf(ioExecutor).execute(with(any(Runnable.class))); + will(new RunAction()); + }}); + } + + private void assertMailboxContactEquals(MailboxContact expected, + MailboxContact actual) { + assertEquals(expected.contactId, actual.contactId); + assertEquals(expected.token, actual.token); + assertEquals(expected.inboxId, actual.inboxId); + assertEquals(expected.outboxId, actual.outboxId); + } +}