Merge branch '1145-avoid-unnecessary-db-queries' into 'master'

Avoid unnecessary DB queries when starting clients

Closes #1145

See merge request akwizgran/briar!660

bramble-core/src/main/java/org/briarproject/bramble/properties/TransportPropertyManagerImpl.java

@@ -64,6 +64,7 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 
 	@Override
 	public void createLocalState(Transaction txn) throws DbException {
+		if (db.containsGroup(txn, localGroup.getId())) return;
 		db.addGroup(txn, localGroup);
 		// Ensure we've set things up for any pre-existing contacts
 		for (Contact c : db.getContacts(txn)) addingContact(txn, c);

bramble-core/src/test/java/org/briarproject/bramble/properties/TransportPropertyManagerImplTest.java

@@ -96,6 +96,8 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		Group contactGroup1 = getGroup(), contactGroup2 = getGroup();
 
 		context.checking(new Expectations() {{
+			oneOf(db).containsGroup(txn, localGroup.getId());
+			will(returnValue(false));
 			oneOf(db).addGroup(txn, localGroup);
 			oneOf(db).getContacts(txn);
 			will(returnValue(contacts));
@@ -127,7 +129,21 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 	}
 
 	@Test
-	public void testCreatesGroupWhenAddingContact() throws Exception {
+	public void testDoesNotCreateGroupsAtStartupIfAlreadyCreated()
+			throws Exception {
+		Transaction txn = new Transaction(null, false);
+
+		context.checking(new Expectations() {{
+			oneOf(db).containsGroup(txn, localGroup.getId());
+			will(returnValue(true));
+		}});
+
+		TransportPropertyManagerImpl t = createInstance();
+		t.createLocalState(txn);
+	}
+
+	@Test
+	public void testCreatesContactGroupWhenAddingContact() throws Exception {
 		Transaction txn = new Transaction(null, false);
 		Contact contact = getContact(true);
 		Group contactGroup = getGroup();

briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java

@@ -106,7 +106,9 @@ class IntroductionManagerImpl extends ConversationClientImpl
 
 	@Override
 	public void createLocalState(Transaction txn) throws DbException {
-		db.addGroup(txn, introductionGroupFactory.createLocalGroup());
+		Group localGroup = introductionGroupFactory.createLocalGroup();
+		if (db.containsGroup(txn, localGroup.getId())) return;
+		db.addGroup(txn, localGroup);
 		// Ensure we've set things up for any pre-existing contacts
 		for (Contact c : db.getContacts(txn)) addingContact(txn, c);
 	}

briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java

@@ -54,6 +54,11 @@ class MessagingManagerImpl extends ConversationClientImpl
 
 	@Override
 	public void createLocalState(Transaction txn) throws DbException {
+		// Create a local group to indicate that we've set this client up
+		Group localGroup = contactGroupFactory.createLocalGroup(CLIENT_ID,
+				CLIENT_VERSION);
+		if (db.containsGroup(txn, localGroup.getId())) return;
+		db.addGroup(txn, localGroup);
 		// Ensure we've set things up for any pre-existing contacts
 		for (Contact c : db.getContacts(txn)) addingContact(txn, c);
 	}

briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java

@@ -96,6 +96,11 @@ class GroupInvitationManagerImpl extends ConversationClientImpl
 
 	@Override
 	public void createLocalState(Transaction txn) throws DbException {
+		// Create a local group to indicate that we've set this client up
+		Group localGroup = contactGroupFactory.createLocalGroup(CLIENT_ID,
+				CLIENT_VERSION);
+		if (db.containsGroup(txn, localGroup.getId())) return;
+		db.addGroup(txn, localGroup);
 		// Ensure we've set things up for any pre-existing contacts
 		for (Contact c : db.getContacts(txn)) addingContact(txn, c);
 	}

briar-core/src/main/java/org/briarproject/briar/sharing/BlogSharingManagerImpl.java

@@ -61,6 +61,9 @@ class BlogSharingManagerImpl extends SharingManagerImpl<Blog>
 	 */
 	@Override
 	public void addingContact(Transaction txn, Contact c) throws DbException {
+		// Return if we've already set things up for this contact
+		if (db.containsGroup(txn, getContactGroup(c).getId())) return;
+
 		// creates a group to share with the contact
 		super.addingContact(txn, c);
 

briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java

@@ -84,6 +84,11 @@ abstract class SharingManagerImpl<S extends Shareable>
 
 	@Override
 	public void createLocalState(Transaction txn) throws DbException {
+		// Create a local group to indicate that we've set this client up
+		Group localGroup = contactGroupFactory.createLocalGroup(getClientId(),
+				getClientVersion());
+		if (db.containsGroup(txn, localGroup.getId())) return;
+		db.addGroup(txn, localGroup);
 		// Ensure we've set things up for any pre-existing contacts
 		for (Contact c : db.getContacts(txn)) addingContact(txn, c);
 	}

briar-core/src/test/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImplTest.java

@@ -46,6 +46,9 @@ import javax.annotation.Nullable;
 import static junit.framework.TestCase.fail;
 import static org.briarproject.bramble.api.sync.Group.Visibility.SHARED;
 import static org.briarproject.bramble.api.sync.SyncConstants.MESSAGE_HEADER_LENGTH;
+import static org.briarproject.bramble.test.TestUtils.getAuthor;
+import static org.briarproject.bramble.test.TestUtils.getGroup;
+import static org.briarproject.bramble.test.TestUtils.getMessage;
 import static org.briarproject.bramble.test.TestUtils.getRandomBytes;
 import static org.briarproject.bramble.test.TestUtils.getRandomId;
 import static org.briarproject.bramble.util.StringUtils.getRandomString;
@@ -93,16 +96,12 @@ public class GroupInvitationManagerImplTest extends BrambleMockTestCase {
 
 	private final Transaction txn = new Transaction(null, false);
 	private final ContactId contactId = new ContactId(0);
-	private final Author author =
+	private final Author author = getAuthor();
-			new Author(new AuthorId(getRandomId()), getRandomString(5),
+	private final Contact contact = new Contact(contactId, author,
-					getRandomBytes(5));
+			new AuthorId(getRandomId()), true, true);
-	private final Contact contact =
+	private final Group localGroup = getGroup(CLIENT_ID);
-			new Contact(contactId, author, new AuthorId(getRandomId()), true,
+	private final Group contactGroup = getGroup(CLIENT_ID);
-					true);
+	private final Group privateGroup = getGroup(CLIENT_ID);
-	private final Group contactGroup =
-			new Group(new GroupId(getRandomId()), CLIENT_ID, getRandomBytes(5));
-	private final Group privateGroup =
-			new Group(new GroupId(getRandomId()), CLIENT_ID, getRandomBytes(5));
 	private final BdfDictionary meta = BdfDictionary.of(new BdfEntry("m", "e"));
 	private final Message message =
 			new Message(new MessageId(getRandomId()), contactGroup.getId(),
@@ -110,9 +109,7 @@ public class GroupInvitationManagerImplTest extends BrambleMockTestCase {
 	private final BdfList body = BdfList.of("body");
 	private final SessionId sessionId =
 			new SessionId(privateGroup.getId().getBytes());
-	private final Message storageMessage =
+	private final Message storageMessage = getMessage(contactGroup.getId());
-			new Message(new MessageId(getRandomId()), contactGroup.getId(),
-					0L, getRandomBytes(MESSAGE_HEADER_LENGTH + 1));
 	private final BdfDictionary bdfSession =
 			BdfDictionary.of(new BdfEntry("f", "o"));
 	private final Map<MessageId, BdfDictionary> oneResult =
@@ -151,8 +148,14 @@ public class GroupInvitationManagerImplTest extends BrambleMockTestCase {
 	}
 
 	@Test
-	public void testCreateLocalState() throws Exception {
+	public void testCreateLocalStateFirstTime() throws Exception {
 		context.checking(new Expectations() {{
+			oneOf(contactGroupFactory).createLocalGroup(CLIENT_ID,
+					CLIENT_VERSION);
+			will(returnValue(localGroup));
+			oneOf(db).containsGroup(txn, localGroup.getId());
+			will(returnValue(false));
+			oneOf(db).addGroup(txn, localGroup);
 			oneOf(db).getContacts(txn);
 			will(returnValue(Collections.singletonList(contact)));
 		}});
@@ -160,6 +163,18 @@ public class GroupInvitationManagerImplTest extends BrambleMockTestCase {
 		groupInvitationManager.createLocalState(txn);
 	}
 
+	@Test
+	public void testCreateLocalStateSubsequentTime() throws Exception {
+		context.checking(new Expectations() {{
+			oneOf(contactGroupFactory).createLocalGroup(CLIENT_ID,
+					CLIENT_VERSION);
+			will(returnValue(localGroup));
+			oneOf(db).containsGroup(txn, localGroup.getId());
+			will(returnValue(true));
+		}});
+		groupInvitationManager.createLocalState(txn);
+	}
+
 	private void expectAddingContact(Contact c, boolean contactExists)
 			throws Exception {
 		context.checking(new Expectations() {{

briar-core/src/test/java/org/briarproject/briar/sharing/BlogSharingManagerImplTest.java

@@ -12,7 +12,6 @@ import org.briarproject.bramble.api.db.DbException;
 import org.briarproject.bramble.api.db.Metadata;
 import org.briarproject.bramble.api.db.Transaction;
 import org.briarproject.bramble.api.identity.Author;
-import org.briarproject.bramble.api.identity.AuthorId;
 import org.briarproject.bramble.api.identity.IdentityManager;
 import org.briarproject.bramble.api.identity.LocalAuthor;
 import org.briarproject.bramble.api.sync.Group;
@@ -34,8 +33,10 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
-import static org.briarproject.bramble.api.identity.AuthorConstants.MAX_PUBLIC_KEY_LENGTH;
 import static org.briarproject.bramble.api.sync.Group.Visibility.SHARED;
+import static org.briarproject.bramble.test.TestUtils.getAuthor;
+import static org.briarproject.bramble.test.TestUtils.getGroup;
+import static org.briarproject.bramble.test.TestUtils.getLocalAuthor;
 import static org.briarproject.bramble.test.TestUtils.getRandomBytes;
 import static org.briarproject.bramble.test.TestUtils.getRandomId;
 import static org.briarproject.briar.api.blog.BlogSharingManager.CLIENT_ID;
@@ -58,22 +59,19 @@ public class BlogSharingManagerImplTest extends BrambleMockTestCase {
 			context.mock(ContactGroupFactory.class);
 	private final BlogManager blogManager = context.mock(BlogManager.class);
 
-	private final AuthorId localAuthorId = new AuthorId(getRandomId());
+	private final LocalAuthor localAuthor = getLocalAuthor();
 	private final ContactId contactId = new ContactId(0);
-	private final AuthorId authorId = new AuthorId(getRandomId());
+	private final Author author = getAuthor();
-	private final Author author = new Author(authorId, "Author",
-			getRandomBytes(MAX_PUBLIC_KEY_LENGTH));
 	private final Contact contact =
-			new Contact(contactId, author, localAuthorId, true, true);
+			new Contact(contactId, author, localAuthor.getId(), true, true);
 	private final Collection<Contact> contacts =
 			Collections.singletonList(contact);
-	private final Group contactGroup =
+	private final Group localGroup = getGroup(CLIENT_ID);
-			new Group(new GroupId(getRandomId()), CLIENT_ID,
+	private final Group contactGroup = getGroup(CLIENT_ID);
-					getRandomBytes(42));
+	private final Group blogGroup = getGroup(BlogManager.CLIENT_ID);
-	private final Group blogGroup =
-			new Group(new GroupId(getRandomId()), BlogManager.CLIENT_ID,
-					getRandomBytes(42));
 	private final Blog blog = new Blog(blogGroup, author, false);
+	private final Group localBlogGroup = getGroup(BlogManager.CLIENT_ID);
+	private final Blog localBlog = new Blog(localBlogGroup, localAuthor, false);
 	@SuppressWarnings("unchecked")
 	private final ProtocolEngine<Blog> engine =
 			context.mock(ProtocolEngine.class);
@@ -94,24 +92,116 @@ public class BlogSharingManagerImplTest extends BrambleMockTestCase {
 	}
 
 	@Test
-	public void testAddingContactFreshState() throws Exception {
+	public void testCreateLocalStateFirstTimeWithExistingContactNotSetUp()
-		Map<MessageId, BdfDictionary> sessions = new HashMap<>(0);
+			throws Exception {
-		testAddingContact(sessions);
+		Transaction txn = new Transaction(null, false);
+
+		context.checking(new Expectations() {{
+			// The local group doesn't exist - we need to set things up
+			oneOf(contactGroupFactory).createLocalGroup(CLIENT_ID,
+					CLIENT_VERSION);
+			will(returnValue(localGroup));
+			oneOf(db).containsGroup(txn, localGroup.getId());
+			will(returnValue(false));
+			oneOf(db).addGroup(txn, localGroup);
+			// Get contacts
+			oneOf(db).getContacts(txn);
+			will(returnValue(contacts));
+		}});
+		// Set things up for the contact
+		expectAddingContact(txn);
+
+		blogSharingManager.createLocalState(txn);
+	}
+
+	private void expectAddingContact(Transaction txn) throws Exception {
+		BdfDictionary meta = BdfDictionary.of(
+				new BdfEntry(GROUP_KEY_CONTACT_ID, contactId.getInt()));
+		Map<MessageId, BdfDictionary> sessions = Collections.emptyMap();
+
+		context.checking(new Expectations() {{
+			// Check for contact group in BlogSharingManagerImpl
+			oneOf(contactGroupFactory).createContactGroup(CLIENT_ID,
+					CLIENT_VERSION, contact);
+			will(returnValue(contactGroup));
+			oneOf(db).containsGroup(txn, contactGroup.getId());
+			will(returnValue(false));
+			// Check for contact group again in SharingManagerImpl
+			oneOf(contactGroupFactory).createContactGroup(CLIENT_ID,
+					CLIENT_VERSION, contact);
+			will(returnValue(contactGroup));
+			oneOf(db).containsGroup(txn, contactGroup.getId());
+			will(returnValue(false));
+			// Create the contact group and share it with the contact
+			oneOf(db).addGroup(txn, contactGroup);
+			oneOf(db).setGroupVisibility(txn, contactId, contactGroup.getId(),
+					SHARED);
+			// Attach the contact ID to the group
+			oneOf(clientHelper)
+					.mergeGroupMetadata(txn, contactGroup.getId(), meta);
+			// Get our blog and the contact's blog
+			oneOf(identityManager).getLocalAuthor(txn);
+			will(returnValue(localAuthor));
+			oneOf(blogManager).getPersonalBlog(localAuthor);
+			will(returnValue(localBlog));
+			oneOf(blogManager).getPersonalBlog(author);
+			will(returnValue(blog));
+		}});
+		// Pre-share our blog with the contact and vice versa
+		expectPreShareShareable(txn, contact, localBlog, sessions);
+		expectPreShareShareable(txn, contact, blog, sessions);
 	}
 
 	@Test
-	public void testAddingContactExistingState() throws Exception {
+	public void testCreateLocalStateFirstTimeWithExistingContactAlreadySetUp()
-		Map<MessageId, BdfDictionary> sessions = new HashMap<>(1);
+			throws Exception {
-		sessions.put(new MessageId(getRandomId()), new BdfDictionary());
+		Transaction txn = new Transaction(null, false);
-		testAddingContact(sessions);
+
+		context.checking(new Expectations() {{
+			// The local group doesn't exist - we need to set things up
+			oneOf(contactGroupFactory).createLocalGroup(CLIENT_ID,
+					CLIENT_VERSION);
+			will(returnValue(localGroup));
+			oneOf(db).containsGroup(txn, localGroup.getId());
+			will(returnValue(false));
+			oneOf(db).addGroup(txn, localGroup);
+			// Get contacts
+			oneOf(db).getContacts(txn);
+			will(returnValue(contacts));
+			// The contact has already been set up
+			oneOf(contactGroupFactory).createContactGroup(CLIENT_ID,
+					CLIENT_VERSION, contact);
+			will(returnValue(contactGroup));
+			oneOf(db).containsGroup(txn, contactGroup.getId());
+			will(returnValue(true));
+		}});
+
+		blogSharingManager.createLocalState(txn);
 	}
 
-	@Test(expected = DbException.class)
+	@Test
-	public void testAddingContactMultipleSessions() throws Exception {
+	public void testCreateLocalStateSubsequentTime() throws Exception {
-		Map<MessageId, BdfDictionary> sessions = new HashMap<>(2);
+		Transaction txn = new Transaction(null, false);
-		sessions.put(new MessageId(getRandomId()), new BdfDictionary());
+
-		sessions.put(new MessageId(getRandomId()), new BdfDictionary());
+		context.checking(new Expectations() {{
-		testAddingContact(sessions);
+			// The local group exists - everything has been set up
+			oneOf(contactGroupFactory).createLocalGroup(CLIENT_ID,
+					CLIENT_VERSION);
+			will(returnValue(localGroup));
+			oneOf(db).containsGroup(txn, localGroup.getId());
+			will(returnValue(true));
+		}});
+
+		blogSharingManager.createLocalState(txn);
+	}
+
+	@Test
+	public void testAddingContact() throws Exception {
+		Transaction txn = new Transaction(null, false);
+
+		expectAddingContact(txn);
+
+		blogSharingManager.addingContact(txn, contact);
 	}
 
 	@Test
@@ -135,47 +225,6 @@ public class BlogSharingManagerImplTest extends BrambleMockTestCase {
 		testRemovingBlog(sessions);
 	}
 
-	private void testAddingContact(Map<MessageId, BdfDictionary> sessions)
-			throws Exception {
-		Transaction txn = new Transaction(null, false);
-		LocalAuthor localAuthor =
-				new LocalAuthor(localAuthorId, "Local Author",
-						getRandomBytes(MAX_PUBLIC_KEY_LENGTH),
-						getRandomBytes(MAX_PUBLIC_KEY_LENGTH),
-						System.currentTimeMillis());
-		BdfDictionary meta = BdfDictionary
-				.of(new BdfEntry(GROUP_KEY_CONTACT_ID, contactId.getInt()));
-		Group localBlogGroup =
-				new Group(new GroupId(getRandomId()), BlogManager.CLIENT_ID,
-						getRandomBytes(42));
-		Blog localBlog = new Blog(localBlogGroup, localAuthor, false);
-
-		context.checking(new Expectations() {{
-			oneOf(db).getContacts(txn);
-			will(returnValue(contacts));
-			oneOf(contactGroupFactory).createContactGroup(CLIENT_ID,
-					CLIENT_VERSION, contact);
-			will(returnValue(contactGroup));
-			oneOf(db).containsGroup(txn, contactGroup.getId());
-			will(returnValue(false));
-			oneOf(db).addGroup(txn, contactGroup);
-			oneOf(db).setGroupVisibility(txn, contactId, contactGroup.getId(),
-					SHARED);
-			oneOf(clientHelper)
-					.mergeGroupMetadata(txn, contactGroup.getId(), meta);
-			oneOf(identityManager).getLocalAuthor(txn);
-			will(returnValue(localAuthor));
-			oneOf(blogManager).getPersonalBlog(localAuthor);
-			will(returnValue(localBlog));
-			oneOf(blogManager).getPersonalBlog(author);
-			will(returnValue(blog));
-		}});
-		expectPreShareShareable(txn, contact, localBlog, sessions);
-		expectPreShareShareable(txn, contact, blog, sessions);
-
-		blogSharingManager.createLocalState(txn);
-	}
-
 	private void expectPreShareShareable(Transaction txn, Contact contact,
 			Blog blog, Map<MessageId, BdfDictionary> sessions)
 			throws Exception {