Merge branch '545-message-dependencies' into 'master'

Add denormalised columns to messageDependencies table

See merge request akwizgran/briar!733

bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseComponent.java

@@ -18,7 +18,6 @@ import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.bramble.api.sync.MessageStatus;
 import org.briarproject.bramble.api.sync.Offer;
 import org.briarproject.bramble.api.sync.Request;
-import org.briarproject.bramble.api.sync.ValidationManager;
 import org.briarproject.bramble.api.transport.TransportKeys;
 
 import java.util.Collection;
@@ -339,12 +338,8 @@ public interface DatabaseComponent {
 
 	/**
 	 * Returns the IDs and states of all dependencies of the given message.
-	 * Missing dependencies have the state
+	 * For missing dependencies and dependencies in other groups, the state
-	 * {@link ValidationManager.State UNKNOWN}.
+	 * {@link State UNKNOWN} is returned.
-	 * Dependencies in other groups have the state
-	 * {@link ValidationManager.State INVALID}.
-	 * Note that these states are not set on the dependencies themselves; the
-	 * returned states should only be taken in the context of the given message.
 	 * <p/>
 	 * Read-only.
 	 */
@@ -352,9 +347,9 @@ public interface DatabaseComponent {
 			throws DbException;
 
 	/**
-	 * Returns all IDs of messages that depend on the given message.
+	 * Returns the IDs and states of all dependents of the given message.
-	 * Messages in other groups that declare a dependency on the given message
+	 * Dependents in other groups are not returned. If the given message is
-	 * will be returned even though such dependencies are invalid.
+	 * missing, no dependents are returned.
 	 * <p/>
 	 * Read-only.
 	 */

bramble-core/src/main/java/org/briarproject/bramble/db/Database.java

@@ -105,10 +105,11 @@ interface Database<T> {
 			@Nullable ContactId sender) throws DbException;
 
 	/**
-	 * Adds a dependency between two messages in the given group.
+	 * Adds a dependency between two messages, where the dependent message is
+	 * in the given state.
 	 */
-	void addMessageDependency(T txn, GroupId g, MessageId dependent,
+	void addMessageDependency(T txn, Message dependent, MessageId dependency,
-			MessageId dependency) throws DbException;
+			State dependentState) throws DbException;
 
 	/**
 	 * Records that a message has been offered by the given contact.
@@ -292,10 +293,8 @@ interface Database<T> {
 
 	/**
 	 * Returns the IDs and states of all dependencies of the given message.
-	 * Missing dependencies have the state {@link State UNKNOWN}.
+	 * For missing dependencies and dependencies in other groups, the state
-	 * Dependencies in other groups have the state {@link State INVALID}.
+	 * {@link State UNKNOWN} is returned.
-	 * Note that these states are not set on the dependencies themselves; the
-	 * returned states should only be taken in the context of the given message.
 	 * <p/>
 	 * Read-only.
 	 */
@@ -303,9 +302,9 @@ interface Database<T> {
 			throws DbException;
 
 	/**
-	 * Returns all IDs and states of all dependents of the given message.
+	 * Returns the IDs and states of all dependents of the given message.
-	 * Messages in other groups that declare a dependency on the given message
+	 * Dependents in other groups are not returned. If the given message is
-	 * will be returned even though such dependencies are invalid.
+	 * missing, no dependents are returned.
 	 * <p/>
 	 * Read-only.
 	 */

bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseComponentImpl.java

@@ -765,6 +765,7 @@ class DatabaseComponentImpl<T> implements DatabaseComponent {
 		T txn = unbox(transaction);
 		if (!db.containsMessage(txn, m))
 			throw new NoSuchMessageException();
+		// TODO: Don't allow messages with dependents to be removed
 		db.removeMessage(txn, m);
 	}
 
@@ -850,9 +851,9 @@ class DatabaseComponentImpl<T> implements DatabaseComponent {
 		T txn = unbox(transaction);
 		if (!db.containsMessage(txn, dependent.getId()))
 			throw new NoSuchMessageException();
+		State dependentState = db.getMessageState(txn, dependent.getId());
 		for (MessageId dependency : dependencies) {
-			db.addMessageDependency(txn, dependent.getGroupId(),
+			db.addMessageDependency(txn, dependent, dependency, dependentState);
-					dependent.getId(), dependency);
 		}
 	}
 

bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java

@@ -50,6 +50,7 @@ import java.util.logging.Logger;
 
 import javax.annotation.Nullable;
 
+import static java.sql.Types.INTEGER;
 import static java.util.logging.Level.INFO;
 import static java.util.logging.Level.WARNING;
 import static org.briarproject.bramble.api.db.Metadata.REMOVE;
@@ -57,7 +58,6 @@ import static org.briarproject.bramble.api.sync.Group.Visibility.INVISIBLE;
 import static org.briarproject.bramble.api.sync.Group.Visibility.SHARED;
 import static org.briarproject.bramble.api.sync.Group.Visibility.VISIBLE;
 import static org.briarproject.bramble.api.sync.ValidationManager.State.DELIVERED;
-import static org.briarproject.bramble.api.sync.ValidationManager.State.INVALID;
 import static org.briarproject.bramble.api.sync.ValidationManager.State.PENDING;
 import static org.briarproject.bramble.api.sync.ValidationManager.State.UNKNOWN;
 import static org.briarproject.bramble.db.DatabaseConstants.DB_SETTINGS_NAMESPACE;
@@ -72,7 +72,7 @@ import static org.briarproject.bramble.db.ExponentialBackoff.calculateExpiry;
 abstract class JdbcDatabase implements Database<Connection> {
 
 	// Package access for testing
-	static final int CODE_SCHEMA_VERSION = 35;
+	static final int CODE_SCHEMA_VERSION = 36;
 
 	private static final String CREATE_SETTINGS =
 			"CREATE TABLE settings"
@@ -170,6 +170,10 @@ abstract class JdbcDatabase implements Database<Connection> {
 					+ " (groupId _HASH NOT NULL,"
 					+ " messageId _HASH NOT NULL,"
 					+ " dependencyId _HASH NOT NULL," // Not a foreign key
+					+ " messageState INT NOT NULL," // Denormalised
+					// Denormalised, null if dependency is missing or in a
+					// different group
+					+ " dependencyState INT,"
 					+ " FOREIGN KEY (groupId)"
 					+ " REFERENCES groups (groupId)"
 					+ " ON DELETE CASCADE,"
@@ -264,6 +268,10 @@ abstract class JdbcDatabase implements Database<Connection> {
 			"CREATE INDEX IF NOT EXISTS messageMetadataByGroupIdState"
 					+ " ON messageMetadata (groupId, state)";
 
+	private static final String INDEX_MESSAGE_DEPENDENCIES_BY_DEPENDENCY_ID =
+			"CREATE INDEX IF NOT EXISTS messageDependenciesByDependencyId"
+					+ " ON messageDependencies (dependencyId)";
+
 	private static final String INDEX_STATUSES_BY_CONTACT_ID_GROUP_ID =
 			"CREATE INDEX IF NOT EXISTS statusesByContactIdGroupId"
 					+ " ON statuses (contactId, groupId)";
@@ -423,6 +431,7 @@ abstract class JdbcDatabase implements Database<Connection> {
 			s.executeUpdate(INDEX_CONTACTS_BY_AUTHOR_ID);
 			s.executeUpdate(INDEX_GROUPS_BY_CLIENT_ID);
 			s.executeUpdate(INDEX_MESSAGE_METADATA_BY_GROUP_ID_STATE);
+			s.executeUpdate(INDEX_MESSAGE_DEPENDENCIES_BY_DEPENDENCY_ID);
 			s.executeUpdate(INDEX_STATUSES_BY_CONTACT_ID_GROUP_ID);
 			s.executeUpdate(INDEX_STATUSES_BY_CONTACT_ID_TIMESTAMP);
 			s.close();
@@ -715,6 +724,17 @@ abstract class JdbcDatabase implements Database<Connection> {
 						m.getLength(), state, e.getValue(), messageShared,
 						false, seen);
 			}
+			// Update denormalised column in messageDependencies if dependency
+			// is in same group as dependent
+			sql = "UPDATE messageDependencies SET dependencyState = ?"
+					+ " WHERE groupId = ? AND dependencyId = ?";
+			ps = txn.prepareStatement(sql);
+			ps.setInt(1, state.getValue());
+			ps.setBytes(2, m.getGroupId().getBytes());
+			ps.setBytes(3, m.getId().getBytes());
+			affected = ps.executeUpdate();
+			if (affected < 0) throw new DbStateException();
+			ps.close();
 		} catch (SQLException e) {
 			tryToClose(ps);
 			throw new DbException(e);
@@ -784,21 +804,42 @@ abstract class JdbcDatabase implements Database<Connection> {
 	}
 
 	@Override
-	public void addMessageDependency(Connection txn, GroupId g,
+	public void addMessageDependency(Connection txn, Message dependent,
-			MessageId dependent, MessageId dependency) throws DbException {
+			MessageId dependency, State dependentState) throws DbException {
 		PreparedStatement ps = null;
+		ResultSet rs = null;
 		try {
-			String sql = "INSERT INTO messageDependencies"
+			// Get state of dependency if present and in same group as dependent
-					+ " (groupId, messageId, dependencyId)"
+			String sql = "SELECT state FROM messages"
-					+ " VALUES (?, ?, ?)";
+					+ " WHERE messageId = ? AND groupId = ?";
 			ps = txn.prepareStatement(sql);
-			ps.setBytes(1, g.getBytes());
+			ps.setBytes(1, dependency.getBytes());
-			ps.setBytes(2, dependent.getBytes());
+			ps.setBytes(2, dependent.getGroupId().getBytes());
+			rs = ps.executeQuery();
+			State dependencyState = null;
+			if (rs.next()) {
+				dependencyState = State.fromValue(rs.getInt(1));
+				if (rs.next()) throw new DbStateException();
+			}
+			rs.close();
+			ps.close();
+			// Create messageDependencies row
+			sql = "INSERT INTO messageDependencies"
+					+ " (groupId, messageId, dependencyId, messageState,"
+					+ " dependencyState)"
+					+ " VALUES (?, ?, ?, ? ,?)";
+			ps = txn.prepareStatement(sql);
+			ps.setBytes(1, dependent.getGroupId().getBytes());
+			ps.setBytes(2, dependent.getId().getBytes());
 			ps.setBytes(3, dependency.getBytes());
+			ps.setInt(4, dependentState.getValue());
+			if (dependencyState == null) ps.setNull(5, INTEGER);
+			else ps.setInt(5, dependencyState.getValue());
 			int affected = ps.executeUpdate();
 			if (affected != 1) throw new DbStateException();
 			ps.close();
 		} catch (SQLException e) {
+			tryToClose(rs);
 			tryToClose(ps);
 			throw new DbException(e);
 		}
@@ -1663,11 +1704,9 @@ abstract class JdbcDatabase implements Database<Connection> {
 		PreparedStatement ps = null;
 		ResultSet rs = null;
 		try {
-			String sql = "SELECT d.dependencyId, m.state, d.groupId, m.groupId"
+			String sql = "SELECT dependencyId, dependencyState"
-					+ " FROM messageDependencies AS d"
+					+ " FROM messageDependencies"
-					+ " LEFT OUTER JOIN messages AS m"
+					+ " WHERE messageId = ?";
-					+ " ON d.dependencyId = m.messageId"
-					+ " WHERE d.messageId = ?";
 			ps = txn.prepareStatement(sql);
 			ps.setBytes(1, m.getBytes());
 			rs = ps.executeQuery();
@@ -1675,14 +1714,8 @@ abstract class JdbcDatabase implements Database<Connection> {
 			while (rs.next()) {
 				MessageId dependency = new MessageId(rs.getBytes(1));
 				State state = State.fromValue(rs.getInt(2));
-				if (rs.wasNull()) {
+				if (rs.wasNull())
-					state = UNKNOWN; // Missing dependency
+					state = UNKNOWN; // Missing or in a different group
-				} else {
-					GroupId dependentGroupId = new GroupId(rs.getBytes(3));
-					GroupId dependencyGroupId = new GroupId(rs.getBytes(4));
-					if (!dependentGroupId.equals(dependencyGroupId))
-						state = INVALID; // Dependency in another group
-				}
 				dependencies.put(dependency, state);
 			}
 			rs.close();
@@ -1701,11 +1734,12 @@ abstract class JdbcDatabase implements Database<Connection> {
 		PreparedStatement ps = null;
 		ResultSet rs = null;
 		try {
-			String sql = "SELECT d.messageId, m.state"
+			// Exclude dependencies that are missing or in a different group
-					+ " FROM messageDependencies AS d"
+			// from the dependent
-					+ " JOIN messages AS m"
+			String sql = "SELECT messageId, messageState"
-					+ " ON d.messageId = m.messageId"
+					+ " FROM messageDependencies"
-					+ " WHERE dependencyId = ?";
+					+ " WHERE dependencyId = ?"
+					+ " AND dependencyState IS NOT NULL";
 			ps = txn.prepareStatement(sql);
 			ps.setBytes(1, m.getBytes());
 			rs = ps.executeQuery();
@@ -2731,6 +2765,25 @@ abstract class JdbcDatabase implements Database<Connection> {
 			affected = ps.executeUpdate();
 			if (affected < 0) throw new DbStateException();
 			ps.close();
+			// Update denormalised column in messageDependencies
+			sql = "UPDATE messageDependencies SET messageState = ?"
+					+ " WHERE messageId = ?";
+			ps = txn.prepareStatement(sql);
+			ps.setInt(1, state.getValue());
+			ps.setBytes(2, m.getBytes());
+			affected = ps.executeUpdate();
+			if (affected < 0) throw new DbStateException();
+			ps.close();
+			// Update denormalised column in messageDependencies if dependency
+			// is present and in same group as dependent
+			sql = "UPDATE messageDependencies SET dependencyState = ?"
+					+ " WHERE dependencyId = ? AND dependencyState IS NOT NULL";
+			ps = txn.prepareStatement(sql);
+			ps.setInt(1, state.getValue());
+			ps.setBytes(2, m.getBytes());
+			affected = ps.executeUpdate();
+			if (affected < 0) throw new DbStateException();
+			ps.close();
 		} catch (SQLException e) {
 			tryToClose(ps);
 			throw new DbException(e);

bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseComponentImplTest.java

@@ -1518,10 +1518,12 @@ public class DatabaseComponentImplTest extends BrambleMockTestCase {
 			// addMessageDependencies()
 			oneOf(database).containsMessage(txn, messageId);
 			will(returnValue(true));
-			oneOf(database).addMessageDependency(txn, groupId, messageId,
+			oneOf(database).getMessageState(txn, messageId);
-					messageId1);
+			will(returnValue(DELIVERED));
-			oneOf(database).addMessageDependency(txn, groupId, messageId,
+			oneOf(database).addMessageDependency(txn, message, messageId1,
-					messageId2);
+					DELIVERED);
+			oneOf(database).addMessageDependency(txn, message, messageId2,
+					DELIVERED);
 			// getMessageDependencies()
 			oneOf(database).containsMessage(txn, messageId);
 			will(returnValue(true));

bramble-core/src/test/java/org/briarproject/bramble/db/DatabasePerformanceTest.java

@@ -572,8 +572,9 @@ public abstract class DatabasePerformanceTest extends BrambleTestCase {
 					messageMeta.get(g.getId()).add(mm);
 					db.mergeMessageMetadata(txn, m.getId(), mm);
 					if (k > 0) {
-						db.addMessageDependency(txn, g.getId(), m.getId(),
+						MessageId dependency =
-								pickRandom(groupMessages.get(g.getId())));
+								pickRandom(groupMessages.get(g.getId()));
+						db.addMessageDependency(txn, m, dependency, state);
 					}
 					groupMessages.get(g.getId()).add(m.getId());
 				}
@@ -598,8 +599,9 @@ public abstract class DatabasePerformanceTest extends BrambleTestCase {
 				messageMeta.get(g.getId()).add(mm);
 				db.mergeMessageMetadata(txn, m.getId(), mm);
 				if (j > 0) {
-					db.addMessageDependency(txn, g.getId(), m.getId(),
+					MessageId dependency =
-							pickRandom(groupMessages.get(g.getId())));
+							pickRandom(groupMessages.get(g.getId()));
+					db.addMessageDependency(txn, m, dependency, DELIVERED);
 				}
 				groupMessages.get(g.getId()).add(m.getId());
 			}

bramble-core/src/test/java/org/briarproject/bramble/db/JdbcDatabaseTest.java

@@ -1227,6 +1227,8 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase {
 		MessageId messageId4 = new MessageId(getRandomId());
 		Message message1 = new Message(messageId1, groupId, timestamp, raw);
 		Message message2 = new Message(messageId2, groupId, timestamp, raw);
+		Message message3 = new Message(messageId3, groupId, timestamp, raw);
+		Message message4 = new Message(messageId4, groupId, timestamp, raw);
 
 		Database<Connection> db = open(false);
 		Connection txn = db.startTransaction();
@@ -1234,21 +1236,21 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase {
 		// Add a group and some messages
 		db.addGroup(txn, group);
 		db.addMessage(txn, message, PENDING, true, contactId);
-		db.addMessage(txn, message1, DELIVERED, true, contactId);
+		db.addMessage(txn, message1, PENDING, true, contactId);
 		db.addMessage(txn, message2, INVALID, true, contactId);
 
 		// Add dependencies
-		db.addMessageDependency(txn, groupId, messageId, messageId1);
+		db.addMessageDependency(txn, message, messageId1, PENDING);
-		db.addMessageDependency(txn, groupId, messageId, messageId2);
+		db.addMessageDependency(txn, message, messageId2, PENDING);
-		db.addMessageDependency(txn, groupId, messageId1, messageId3);
+		db.addMessageDependency(txn, message1, messageId3, PENDING);
-		db.addMessageDependency(txn, groupId, messageId2, messageId4);
+		db.addMessageDependency(txn, message2, messageId4, INVALID);
 
 		Map<MessageId, State> dependencies;
 
 		// Retrieve dependencies for root
 		dependencies = db.getMessageDependencies(txn, messageId);
 		assertEquals(2, dependencies.size());
-		assertEquals(DELIVERED, dependencies.get(messageId1));
+		assertEquals(PENDING, dependencies.get(messageId1));
 		assertEquals(INVALID, dependencies.get(messageId2));
 
 		// Retrieve dependencies for message 1
@@ -1281,10 +1283,24 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase {
 		assertEquals(1, dependents.size());
 		assertEquals(PENDING, dependents.get(messageId));
 
+		// Message 3 is missing, so it has no dependents
+		dependents = db.getMessageDependents(txn, messageId3);
+		assertEquals(0, dependents.size());
+
+		// Add message 3
+		db.addMessage(txn, message3, UNKNOWN, false, contactId);
+
 		// Message 3 has message 1 as a dependent
 		dependents = db.getMessageDependents(txn, messageId3);
 		assertEquals(1, dependents.size());
-		assertEquals(DELIVERED, dependents.get(messageId1));
+		assertEquals(PENDING, dependents.get(messageId1));
+
+		// Message 4 is missing, so it has no dependents
+		dependents = db.getMessageDependents(txn, messageId4);
+		assertEquals(0, dependents.size());
+
+		// Add message 4
+		db.addMessage(txn, message4, UNKNOWN, false, contactId);
 
 		// Message 4 has message 2 as a dependent
 		dependents = db.getMessageDependents(txn, messageId4);
@@ -1324,16 +1340,16 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase {
 		db.addMessage(txn, message3, DELIVERED, true, contactId);
 
 		// Add dependencies between the messages
-		db.addMessageDependency(txn, groupId, messageId, messageId1);
+		db.addMessageDependency(txn, message, messageId1, PENDING);
-		db.addMessageDependency(txn, groupId, messageId, messageId2);
+		db.addMessageDependency(txn, message, messageId2, PENDING);
-		db.addMessageDependency(txn, groupId, messageId, messageId3);
+		db.addMessageDependency(txn, message, messageId3, PENDING);
 
 		// Retrieve the dependencies for the root
 		Map<MessageId, State> dependencies;
 		dependencies = db.getMessageDependencies(txn, messageId);
 
-		// The cross-group dependency should have state INVALID
+		// The cross-group dependency should have state UNKNOWN
-		assertEquals(INVALID, dependencies.get(messageId1));
+		assertEquals(UNKNOWN, dependencies.get(messageId1));
 
 		// The missing dependency should have state UNKNOWN
 		assertEquals(UNKNOWN, dependencies.get(messageId2));
@@ -1345,8 +1361,8 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase {
 		Map<MessageId, State> dependents;
 		dependents = db.getMessageDependents(txn, messageId1);
 
-		// The cross-group dependent should have its real state
+		// The cross-group dependent should be excluded
-		assertEquals(PENDING, dependents.get(messageId));
+		assertFalse(dependents.containsKey(messageId));
 
 		db.commitTransaction(txn);
 		db.close();
@@ -1411,9 +1427,9 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase {
 		db.addMessage(txn, m4, DELIVERED, true, contactId);
 
 		// Introduce dependencies between the messages
-		db.addMessageDependency(txn, groupId, mId1, mId2);
+		db.addMessageDependency(txn, m1, mId2, DELIVERED);
-		db.addMessageDependency(txn, groupId, mId3, mId1);
+		db.addMessageDependency(txn, m3, mId1, DELIVERED);
-		db.addMessageDependency(txn, groupId, mId4, mId3);
+		db.addMessageDependency(txn, m4, mId3, DELIVERED);
 
 		// Retrieve messages to be shared
 		Collection<MessageId> result = db.getMessagesToShare(txn);