Merge branch 'transport-property-manager-cleanup' into 'master'

Simplify management of old transport property updates

See merge request !629

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

@@ -129,8 +129,7 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 	public Map<TransportId, TransportProperties> getLocalProperties()
 			throws DbException {
 		Map<TransportId, TransportProperties> local;
-		// TODO: Transaction can be read-only when code is simplified
+		Transaction txn = db.startTransaction(true);
-		Transaction txn = db.startTransaction(false);
 		try {
 			local = getLocalProperties(txn);
 			db.commitTransaction(txn);
@@ -165,8 +164,7 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 			throws DbException {
 		try {
 			TransportProperties p = null;
-			// TODO: Transaction can be read-only when code is simplified
+			Transaction txn = db.startTransaction(true);
-			Transaction txn = db.startTransaction(false);
 			try {
 				// Find the latest local update
 				LatestUpdate latest = findLatest(txn, localGroup.getId(), t,
@@ -192,8 +190,7 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 	public Map<ContactId, TransportProperties> getRemoteProperties(
 			TransportId t) throws DbException {
 		Map<ContactId, TransportProperties> remote = new HashMap<>();
-		// TODO: Transaction can be read-only when code is simplified
+		Transaction txn = db.startTransaction(true);
-		Transaction txn = db.startTransaction(false);
 		try {
 			for (Contact c : db.getContacts(txn))
 				remote.put(c.getId(), getRemoteProperties(txn, c, t));
@@ -227,8 +224,7 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 	public TransportProperties getRemoteProperties(ContactId c, TransportId t)
 			throws DbException {
 		TransportProperties p;
-		// TODO: Transaction can be read-only when code is simplified
+		Transaction txn = db.startTransaction(true);
-		Transaction txn = db.startTransaction(false);
 		try {
 			p = getRemoteProperties(txn, db.getContact(txn, c), t);
 			db.commitTransaction(txn);
@@ -318,7 +314,6 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 
 	private Map<TransportId, LatestUpdate> findLatestLocal(Transaction txn)
 			throws DbException, FormatException {
-		// TODO: This can be simplified before 1.0
 		Map<TransportId, LatestUpdate> latestUpdates = new HashMap<>();
 		Map<MessageId, BdfDictionary> metadata = clientHelper
 				.getMessageMetadataAsDictionary(txn, localGroup.getId());
@@ -326,17 +321,7 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 			BdfDictionary meta = e.getValue();
 			TransportId t = new TransportId(meta.getString("transportId"));
 			long version = meta.getLong("version");
-			LatestUpdate latest = latestUpdates.get(t);
+			latestUpdates.put(t, new LatestUpdate(e.getKey(), version));
-			if (latest == null) {
-				latestUpdates.put(t, new LatestUpdate(e.getKey(), version));
-			} else if (version > latest.version) {
-				// This update is newer - delete the previous one
-				db.removeMessage(txn, latest.messageId);
-				latestUpdates.put(t, new LatestUpdate(e.getKey(), version));
-			} else {
-				// We've already found a newer update - delete this one
-				db.removeMessage(txn, e.getKey());
-			}
 		}
 		return latestUpdates;
 	}
@@ -344,38 +329,16 @@ class TransportPropertyManagerImpl implements TransportPropertyManager,
 	@Nullable
 	private LatestUpdate findLatest(Transaction txn, GroupId g, TransportId t,
 			boolean local) throws DbException, FormatException {
-		// TODO: This can be simplified before 1.0
-		LatestUpdate latest = null;
 		Map<MessageId, BdfDictionary> metadata =
 				clientHelper.getMessageMetadataAsDictionary(txn, g);
 		for (Entry<MessageId, BdfDictionary> e : metadata.entrySet()) {
 			BdfDictionary meta = e.getValue();
 			if (meta.getString("transportId").equals(t.getString())
 					&& meta.getBoolean("local") == local) {
-				long version = meta.getLong("version");
+				return new LatestUpdate(e.getKey(), meta.getLong("version"));
-				if (latest == null) {
-					latest = new LatestUpdate(e.getKey(), version);
-				} else if (version > latest.version) {
-					// This update is newer - delete the previous one
-					if (local) {
-						db.removeMessage(txn, latest.messageId);
-					} else {
-						db.deleteMessage(txn, latest.messageId);
-						db.deleteMessageMetadata(txn, latest.messageId);
-					}
-					latest = new LatestUpdate(e.getKey(), version);
-				} else {
-					// We've already found a newer update - delete this one
-					if (local) {
-						db.removeMessage(txn, e.getKey());
-					} else {
-						db.deleteMessage(txn, e.getKey());
-						db.deleteMessageMetadata(txn, e.getKey());
-					}
-				}
 			}
 		}
-		return latest;
+		return null;
 	}
 
 	private TransportProperties parseProperties(BdfList message)

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

@@ -215,6 +215,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		long timestamp = 123456789;
 		Message message = getMessage(contactGroupId, timestamp);
 		Metadata meta = new Metadata();
+		// Version 4 is being delivered
 		BdfDictionary metaDictionary = BdfDictionary.of(
 				new BdfEntry("transportId", "foo"),
 				new BdfEntry("version", 4),
@@ -222,19 +223,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		);
 		Map<MessageId, BdfDictionary> messageMetadata =
 				new LinkedHashMap<>();
-		// Old remote updates for the same transport should be deleted
+		// An older remote update for the same transport should be deleted
-		MessageId fooVersion2 = new MessageId(getRandomId());
-		messageMetadata.put(fooVersion2, BdfDictionary.of(
-				new BdfEntry("transportId", "foo"),
-				new BdfEntry("version", 2),
-				new BdfEntry("local", false)
-		));
-		MessageId fooVersion1 = new MessageId(getRandomId());
-		messageMetadata.put(fooVersion1, BdfDictionary.of(
-				new BdfEntry("transportId", "foo"),
-				new BdfEntry("version", 1),
-				new BdfEntry("local", false)
-		));
 		MessageId fooVersion3 = new MessageId(getRandomId());
 		messageMetadata.put(fooVersion3, BdfDictionary.of(
 				new BdfEntry("transportId", "foo"),
@@ -248,11 +237,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 			oneOf(clientHelper).getMessageMetadataAsDictionary(txn,
 					contactGroupId);
 			will(returnValue(messageMetadata));
-			// Versions 1-3 should be deleted
+			// The previous update (version 3) should be deleted
-			oneOf(db).deleteMessage(txn, fooVersion1);
-			oneOf(db).deleteMessageMetadata(txn, fooVersion1);
-			oneOf(db).deleteMessage(txn, fooVersion2);
-			oneOf(db).deleteMessageMetadata(txn, fooVersion2);
 			oneOf(db).deleteMessage(txn, fooVersion3);
 			oneOf(db).deleteMessageMetadata(txn, fooVersion3);
 		}});
@@ -268,6 +253,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		long timestamp = 123456789;
 		Message message = getMessage(contactGroupId, timestamp);
 		Metadata meta = new Metadata();
+		// Version 3 is being delivered
 		BdfDictionary metaDictionary = BdfDictionary.of(
 				new BdfEntry("transportId", "foo"),
 				new BdfEntry("version", 3),
@@ -275,19 +261,6 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		);
 		Map<MessageId, BdfDictionary> messageMetadata =
 				new LinkedHashMap<>();
-		// Old remote updates for the same transport should be deleted
-		MessageId fooVersion2 = new MessageId(getRandomId());
-		messageMetadata.put(fooVersion2, BdfDictionary.of(
-				new BdfEntry("transportId", "foo"),
-				new BdfEntry("version", 2),
-				new BdfEntry("local", false)
-		));
-		MessageId fooVersion1 = new MessageId(getRandomId());
-		messageMetadata.put(fooVersion1, BdfDictionary.of(
-				new BdfEntry("transportId", "foo"),
-				new BdfEntry("version", 1),
-				new BdfEntry("local", false)
-		));
 		// A newer remote update for the same transport should not be deleted
 		MessageId fooVersion4 = new MessageId(getRandomId());
 		messageMetadata.put(fooVersion4, BdfDictionary.of(
@@ -302,11 +275,6 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 			oneOf(clientHelper).getMessageMetadataAsDictionary(txn,
 					contactGroupId);
 			will(returnValue(messageMetadata));
-			// Versions 1 and 2 should be deleted, version 4 should not
-			oneOf(db).deleteMessage(txn, fooVersion1);
-			oneOf(db).deleteMessageMetadata(txn, fooVersion1);
-			oneOf(db).deleteMessage(txn, fooVersion2);
-			oneOf(db).deleteMessageMetadata(txn, fooVersion2);
 			// The update being delivered (version 3) should be deleted
 			oneOf(db).deleteMessage(txn, message.getId());
 			oneOf(db).deleteMessageMetadata(txn, message.getId());
@@ -343,7 +311,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 
 	@Test
 	public void testReturnsLatestLocalProperties() throws Exception {
-		Transaction txn = new Transaction(null, false);
+		Transaction txn = new Transaction(null, true);
 
 		expectGetLocalProperties(txn);
 
@@ -357,7 +325,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 	@Test
 	public void testReturnsEmptyPropertiesIfNoLocalPropertiesAreFound()
 			throws Exception {
-		Transaction txn = new Transaction(null, false);
+		Transaction txn = new Transaction(null, true);
 		Map<MessageId, BdfDictionary> messageMetadata =
 				new LinkedHashMap<>();
 		// A local update for another transport should be ignored
@@ -369,7 +337,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		));
 
 		context.checking(new Expectations() {{
-			oneOf(db).startTransaction(false);
+			oneOf(db).startTransaction(true);
 			will(returnValue(txn));
 			oneOf(clientHelper).getMessageMetadataAsDictionary(txn,
 					localGroup.getId());
@@ -384,7 +352,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 
 	@Test
 	public void testReturnsLocalProperties() throws Exception {
-		Transaction txn = new Transaction(null, false);
+		Transaction txn = new Transaction(null, true);
 		Map<MessageId, BdfDictionary> messageMetadata =
 				new LinkedHashMap<>();
 		// A local update for another transport should be ignored
@@ -404,7 +372,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		BdfList fooUpdate = BdfList.of("foo", 1, fooPropertiesDict);
 
 		context.checking(new Expectations() {{
-			oneOf(db).startTransaction(false);
+			oneOf(db).startTransaction(true);
 			will(returnValue(txn));
 			oneOf(clientHelper).getMessageMetadataAsDictionary(txn,
 					localGroup.getId());
@@ -423,7 +391,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 	@Test
 	public void testReturnsRemotePropertiesOrEmptyProperties()
 			throws Exception {
-		Transaction txn = new Transaction(null, false);
+		Transaction txn = new Transaction(null, true);
 		Contact contact1 = getContact(false);
 		Contact contact2 = getContact(true);
 		Contact contact3 = getContact(true);
@@ -457,7 +425,7 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		BdfList fooUpdate = BdfList.of("foo", 1, fooPropertiesDict);
 
 		context.checking(new Expectations() {{
-			oneOf(db).startTransaction(false);
+			oneOf(db).startTransaction(true);
 			will(returnValue(txn));
 			oneOf(db).getContacts(txn);
 			will(returnValue(contacts));
@@ -638,28 +606,14 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 		return new Message(messageId, g, timestamp, raw);
 	}
 
-	private void expectGetLocalProperties(Transaction txn)
+	private void expectGetLocalProperties(Transaction txn) throws Exception {
-			throws Exception {
+		Map<MessageId, BdfDictionary> messageMetadata = new LinkedHashMap<>();
-		Map<MessageId, BdfDictionary> messageMetadata =
+		// The latest update for transport "foo" should be returned
-				new LinkedHashMap<>();
-		// The only update for transport "foo" should be returned
 		MessageId fooVersion999 = new MessageId(getRandomId());
 		messageMetadata.put(fooVersion999, BdfDictionary.of(
 				new BdfEntry("transportId", "foo"),
 				new BdfEntry("version", 999)
 		));
-		// An old update for transport "bar" should be deleted
-		MessageId barVersion2 = new MessageId(getRandomId());
-		messageMetadata.put(barVersion2, BdfDictionary.of(
-				new BdfEntry("transportId", "bar"),
-				new BdfEntry("version", 2)
-		));
-		// An even older update for transport "bar" should be deleted
-		MessageId barVersion1 = new MessageId(getRandomId());
-		messageMetadata.put(barVersion1, BdfDictionary.of(
-				new BdfEntry("transportId", "bar"),
-				new BdfEntry("version", 1)
-		));
 		// The latest update for transport "bar" should be returned
 		MessageId barVersion3 = new MessageId(getRandomId());
 		messageMetadata.put(barVersion3, BdfDictionary.of(
@@ -674,8 +628,6 @@ public class TransportPropertyManagerImplTest extends BrambleMockTestCase {
 			oneOf(clientHelper).getMessageMetadataAsDictionary(txn,
 					localGroup.getId());
 			will(returnValue(messageMetadata));
-			oneOf(db).removeMessage(txn, barVersion1);
-			oneOf(db).removeMessage(txn, barVersion2);
 			// Retrieve and parse the latest local properties
 			oneOf(clientHelper).getMessageAsList(txn, fooVersion999);
 			will(returnValue(fooUpdate));