From 4c611583267c26ee207d2ac5c39ef7ddad62843e Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 31 Jan 2018 11:50:00 +0000 Subject: [PATCH 1/5] Migrate database schema if a migration is available. --- .../bramble/db/DatabaseConstants.java | 6 ---- .../briarproject/bramble/db/JdbcDatabase.java | 35 +++++++++++++------ .../briarproject/bramble/db/Migration.java | 18 ++++++++++ 3 files changed, 43 insertions(+), 16 deletions(-) create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/db/Migration.java diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java b/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java index 080a8ce8e..707234b1f 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java @@ -23,10 +23,4 @@ interface DatabaseConstants { */ String SCHEMA_VERSION_KEY = "schemaVersion"; - /** - * The {@link Settings} key under which the minimum supported database - * schema version is stored. - */ - String MIN_SCHEMA_VERSION_KEY = "minSchemaVersion"; - } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 8b8c3562a..9cbc48e1a 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -47,6 +47,7 @@ import java.util.logging.Logger; import javax.annotation.Nullable; +import static java.util.logging.Level.INFO; import static java.util.logging.Level.WARNING; import static org.briarproject.bramble.api.db.Metadata.REMOVE; import static org.briarproject.bramble.api.sync.Group.Visibility.INVISIBLE; @@ -57,7 +58,6 @@ 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; -import static org.briarproject.bramble.db.DatabaseConstants.MIN_SCHEMA_VERSION_KEY; import static org.briarproject.bramble.db.DatabaseConstants.SCHEMA_VERSION_KEY; import static org.briarproject.bramble.db.ExponentialBackoff.calculateExpiry; @@ -68,8 +68,7 @@ import static org.briarproject.bramble.db.ExponentialBackoff.calculateExpiry; @NotNullByDefault abstract class JdbcDatabase implements Database { - private static final int SCHEMA_VERSION = 30; - private static final int MIN_SCHEMA_VERSION = 30; + private static final int CODE_SCHEMA_VERSION = 30; private static final String CREATE_SETTINGS = "CREATE TABLE settings" @@ -310,17 +309,33 @@ abstract class JdbcDatabase implements Database { private boolean checkSchemaVersion(Connection txn) throws DbException { Settings s = getSettings(txn, DB_SETTINGS_NAMESPACE); - int schemaVersion = s.getInt(SCHEMA_VERSION_KEY, -1); - if (schemaVersion == SCHEMA_VERSION) return true; - if (schemaVersion < MIN_SCHEMA_VERSION) return false; - int minSchemaVersion = s.getInt(MIN_SCHEMA_VERSION_KEY, -1); - return SCHEMA_VERSION >= minSchemaVersion; + int dataSchemaVersion = s.getInt(SCHEMA_VERSION_KEY, -1); + if (dataSchemaVersion == CODE_SCHEMA_VERSION) return true; + if (CODE_SCHEMA_VERSION < dataSchemaVersion) return false; + // Do we have a suitable migration? + for (Migration m : getMigrations()) { + int start = m.getStartVersion(), end = m.getEndVersion(); + if (start == dataSchemaVersion && end == CODE_SCHEMA_VERSION) { + if (LOG.isLoggable(INFO)) + LOG.info("Migrating from schema " + start + " to " + end); + // Apply the migration + m.migrate(txn); + // Store the new schema version + storeSchemaVersion(txn); + return true; + } + } + // No suitable migration + return false; + } + + private Collection> getMigrations() { + return Collections.emptyList(); } private void storeSchemaVersion(Connection txn) throws DbException { Settings s = new Settings(); - s.putInt(SCHEMA_VERSION_KEY, SCHEMA_VERSION); - s.putInt(MIN_SCHEMA_VERSION_KEY, MIN_SCHEMA_VERSION); + s.putInt(SCHEMA_VERSION_KEY, CODE_SCHEMA_VERSION); mergeSettings(txn, s, DB_SETTINGS_NAMESPACE); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/Migration.java b/bramble-core/src/main/java/org/briarproject/bramble/db/Migration.java new file mode 100644 index 000000000..56b04effe --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/Migration.java @@ -0,0 +1,18 @@ +package org.briarproject.bramble.db; + +import org.briarproject.bramble.api.db.DbException; + +interface Migration { + + /** + * Returns the schema version from which this migration starts. + */ + int getStartVersion(); + + /** + * Returns the schema version at which this migration ends. + */ + int getEndVersion(); + + void migrate(T txn) throws DbException; +} From 8faa456eb25e1de983930664f18d7d69df5eca58 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 31 Jan 2018 15:41:21 +0000 Subject: [PATCH 2/5] Add unit tests for migration logic. --- .../briarproject/bramble/db/JdbcDatabase.java | 7 +- .../bramble/db/DatabaseMigrationTest.java | 183 ++++++++++++++++++ .../bramble/db/H2MigrationTest.java | 21 ++ 3 files changed, 209 insertions(+), 2 deletions(-) create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 9cbc48e1a..98ccb953d 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -68,7 +68,8 @@ import static org.briarproject.bramble.db.ExponentialBackoff.calculateExpiry; @NotNullByDefault abstract class JdbcDatabase implements Database { - private static final int CODE_SCHEMA_VERSION = 30; + // Package access for testing + static final int CODE_SCHEMA_VERSION = 30; private static final String CREATE_SETTINGS = "CREATE TABLE settings" @@ -310,6 +311,7 @@ abstract class JdbcDatabase implements Database { private boolean checkSchemaVersion(Connection txn) throws DbException { Settings s = getSettings(txn, DB_SETTINGS_NAMESPACE); int dataSchemaVersion = s.getInt(SCHEMA_VERSION_KEY, -1); + if (dataSchemaVersion == -1) return false; if (dataSchemaVersion == CODE_SCHEMA_VERSION) return true; if (CODE_SCHEMA_VERSION < dataSchemaVersion) return false; // Do we have a suitable migration? @@ -329,7 +331,8 @@ abstract class JdbcDatabase implements Database { return false; } - private Collection> getMigrations() { + // Package access for testing + Collection> getMigrations() { return Collections.emptyList(); } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java b/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java new file mode 100644 index 000000000..79ff1b792 --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java @@ -0,0 +1,183 @@ +package org.briarproject.bramble.db; + +import org.briarproject.bramble.api.db.DatabaseConfig; +import org.briarproject.bramble.api.db.DbException; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.settings.Settings; +import org.briarproject.bramble.api.system.Clock; +import org.briarproject.bramble.system.SystemClock; +import org.briarproject.bramble.test.BrambleMockTestCase; +import org.briarproject.bramble.test.TestDatabaseConfig; +import org.briarproject.bramble.test.TestUtils; +import org.jmock.Expectations; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.sql.Connection; +import java.util.Collection; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static org.briarproject.bramble.db.DatabaseConstants.DB_SETTINGS_NAMESPACE; +import static org.briarproject.bramble.db.DatabaseConstants.SCHEMA_VERSION_KEY; +import static org.briarproject.bramble.db.JdbcDatabase.CODE_SCHEMA_VERSION; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +@NotNullByDefault +public abstract class DatabaseMigrationTest extends BrambleMockTestCase { + + private final File testDir = TestUtils.getTestDirectory(); + @SuppressWarnings("unchecked") + private final Migration migration = + context.mock(Migration.class); + + protected final DatabaseConfig config = + new TestDatabaseConfig(testDir, 1024 * 1024); + protected final Clock clock = new SystemClock(); + + abstract Database createDatabase( + Collection> migrations) throws Exception; + + @Before + public void setUp() { + assertTrue(testDir.mkdirs()); + } + + @After + public void tearDown() { + TestUtils.deleteTestDirectory(testDir); + } + + @Test + public void testDoesNotRunMigrationsWhenCreatingDatabase() + throws Exception { + Database db = createDatabase(singletonList(migration)); + assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + db.close(); + } + + @Test(expected = DbException.class) + public void testThrowsExceptionIfDataSchemaVersionIsMissing() + throws Exception { + // Open the DB for the first time + Database db = createDatabase(singletonList(migration)); + assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + // Override the data schema version + setDataSchemaVersion(db, -1); + db.close(); + // Reopen the DB - an exception should be thrown + db = createDatabase(singletonList(migration)); + db.open(); + } + + @Test + public void testDoesNotRunMigrationsIfSchemaVersionsMatch() + throws Exception { + // Open the DB for the first time + Database db = createDatabase(singletonList(migration)); + assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + db.close(); + // Reopen the DB - migrations should not be run + db = createDatabase(singletonList(migration)); + assertTrue(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + db.close(); + } + + @Test(expected = DbException.class) + public void testThrowsExceptionIfDataIsNewerThanCode() throws Exception { + // Open the DB for the first time + Database db = createDatabase(singletonList(migration)); + assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + // Override the data schema version + setDataSchemaVersion(db, CODE_SCHEMA_VERSION + 1); + db.close(); + // Reopen the DB - an exception should be thrown + db = createDatabase(singletonList(migration)); + db.open(); + } + + @Test(expected = DbException.class) + public void testThrowsExceptionIfCodeIsNewerThanDataAndNoMigrations() + throws Exception { + // Open the DB for the first time + Database db = createDatabase(emptyList()); + assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 1); + db.close(); + // Reopen the DB - an exception should be thrown + db = createDatabase(emptyList()); + db.open(); + } + + @Test(expected = DbException.class) + public void testThrowsExceptionIfCodeIsNewerThanDataAndNoSuitableMigration() + throws Exception { + context.checking(new Expectations() {{ + oneOf(migration).getStartVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 2)); + oneOf(migration).getEndVersion(); + will(returnValue(CODE_SCHEMA_VERSION)); + }}); + + // Open the DB for the first time + Database db = createDatabase(singletonList(migration)); + assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + // Override the data schema version + setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 1); + db.close(); + // Reopen the DB - an exception should be thrown + db = createDatabase(singletonList(migration)); + db.open(); + } + + @Test + public void testRunsMigrationIfCodeIsNewerThanDataAndSuitableMigration() + throws Exception { + context.checking(new Expectations() {{ + oneOf(migration).getStartVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 1)); + oneOf(migration).getEndVersion(); + will(returnValue(CODE_SCHEMA_VERSION)); + oneOf(migration).migrate(with(any(Connection.class))); + }}); + + // Open the DB for the first time + Database db = createDatabase(singletonList(migration)); + assertFalse(db.open()); + // Override the data schema version + setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 1); + db.close(); + // Reopen the DB - the migration should be run + db = createDatabase(singletonList(migration)); + assertTrue(db.open()); + db.close(); + } + + private int getDataSchemaVersion(Database db) + throws Exception { + Connection txn = db.startTransaction(); + Settings s = db.getSettings(txn, DB_SETTINGS_NAMESPACE); + db.commitTransaction(txn); + return s.getInt(SCHEMA_VERSION_KEY, -1); + } + + private void setDataSchemaVersion(Database db, int version) + throws Exception { + Settings s = new Settings(); + s.putInt(SCHEMA_VERSION_KEY, version); + Connection txn = db.startTransaction(); + db.mergeSettings(txn, s, DB_SETTINGS_NAMESPACE); + db.commitTransaction(txn); + } +} diff --git a/bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java b/bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java new file mode 100644 index 000000000..1bf215ad7 --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java @@ -0,0 +1,21 @@ +package org.briarproject.bramble.db; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import java.sql.Connection; +import java.util.Collection; + +@NotNullByDefault +public class H2MigrationTest extends DatabaseMigrationTest { + + @Override + Database createDatabase( + Collection> migrations) throws Exception { + return new H2Database(config, clock) { + @Override + Collection> getMigrations() { + return migrations; + } + }; + } +} From 8c8c1158f446a3446777656a63f24f1a8c4562f6 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 1 Feb 2018 10:14:34 +0000 Subject: [PATCH 3/5] Apply more than one migration if suitable. --- .../briarproject/bramble/db/JdbcDatabase.java | 20 ++--- .../bramble/db/DatabaseMigrationTest.java | 82 +++++++++++++++---- .../bramble/db/H2MigrationTest.java | 8 +- 3 files changed, 79 insertions(+), 31 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 98ccb953d..1e49baa58 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -298,7 +298,7 @@ abstract class JdbcDatabase implements Database { if (!checkSchemaVersion(txn)) throw new DbException(); } else { createTables(txn); - storeSchemaVersion(txn); + storeSchemaVersion(txn, CODE_SCHEMA_VERSION); } createIndexes(txn); commitTransaction(txn); @@ -314,31 +314,31 @@ abstract class JdbcDatabase implements Database { if (dataSchemaVersion == -1) return false; if (dataSchemaVersion == CODE_SCHEMA_VERSION) return true; if (CODE_SCHEMA_VERSION < dataSchemaVersion) return false; - // Do we have a suitable migration? + // Apply any suitable migrations in order for (Migration m : getMigrations()) { int start = m.getStartVersion(), end = m.getEndVersion(); - if (start == dataSchemaVersion && end == CODE_SCHEMA_VERSION) { + if (start == dataSchemaVersion) { if (LOG.isLoggable(INFO)) LOG.info("Migrating from schema " + start + " to " + end); // Apply the migration m.migrate(txn); // Store the new schema version - storeSchemaVersion(txn); - return true; + storeSchemaVersion(txn, end); + dataSchemaVersion = end; } } - // No suitable migration - return false; + return dataSchemaVersion == CODE_SCHEMA_VERSION; } // Package access for testing - Collection> getMigrations() { + List> getMigrations() { return Collections.emptyList(); } - private void storeSchemaVersion(Connection txn) throws DbException { + private void storeSchemaVersion(Connection txn, int version) + throws DbException { Settings s = new Settings(); - s.putInt(SCHEMA_VERSION_KEY, CODE_SCHEMA_VERSION); + s.putInt(SCHEMA_VERSION_KEY, version); mergeSettings(txn, s, DB_SETTINGS_NAMESPACE); } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java b/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java index 79ff1b792..de6cabe05 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java @@ -16,8 +16,9 @@ import org.junit.Test; import java.io.File; import java.sql.Connection; -import java.util.Collection; +import java.util.List; +import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.briarproject.bramble.db.DatabaseConstants.DB_SETTINGS_NAMESPACE; @@ -33,14 +34,17 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { private final File testDir = TestUtils.getTestDirectory(); @SuppressWarnings("unchecked") private final Migration migration = - context.mock(Migration.class); + context.mock(Migration.class, "migration"); + @SuppressWarnings("unchecked") + private final Migration migration1 = + context.mock(Migration.class, "migration1"); protected final DatabaseConfig config = new TestDatabaseConfig(testDir, 1024 * 1024); protected final Clock clock = new SystemClock(); abstract Database createDatabase( - Collection> migrations) throws Exception; + List> migrations) throws Exception; @Before public void setUp() { @@ -65,14 +69,14 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { public void testThrowsExceptionIfDataSchemaVersionIsMissing() throws Exception { // Open the DB for the first time - Database db = createDatabase(singletonList(migration)); + Database db = createDatabase(asList(migration, migration1)); assertFalse(db.open()); assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); // Override the data schema version setDataSchemaVersion(db, -1); db.close(); // Reopen the DB - an exception should be thrown - db = createDatabase(singletonList(migration)); + db = createDatabase(asList(migration, migration1)); db.open(); } @@ -80,12 +84,12 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { public void testDoesNotRunMigrationsIfSchemaVersionsMatch() throws Exception { // Open the DB for the first time - Database db = createDatabase(singletonList(migration)); + Database db = createDatabase(asList(migration, migration1)); assertFalse(db.open()); assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); db.close(); // Reopen the DB - migrations should not be run - db = createDatabase(singletonList(migration)); + db = createDatabase(asList(migration, migration1)); assertTrue(db.open()); assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); db.close(); @@ -94,14 +98,14 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { @Test(expected = DbException.class) public void testThrowsExceptionIfDataIsNewerThanCode() throws Exception { // Open the DB for the first time - Database db = createDatabase(singletonList(migration)); + Database db = createDatabase(asList(migration, migration1)); assertFalse(db.open()); assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); // Override the data schema version setDataSchemaVersion(db, CODE_SCHEMA_VERSION + 1); db.close(); // Reopen the DB - an exception should be thrown - db = createDatabase(singletonList(migration)); + db = createDatabase(asList(migration, migration1)); db.open(); } @@ -126,18 +130,22 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { oneOf(migration).getStartVersion(); will(returnValue(CODE_SCHEMA_VERSION - 2)); oneOf(migration).getEndVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 1)); + oneOf(migration1).getStartVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 1)); + oneOf(migration1).getEndVersion(); will(returnValue(CODE_SCHEMA_VERSION)); }}); // Open the DB for the first time - Database db = createDatabase(singletonList(migration)); + Database db = createDatabase(asList(migration, migration1)); assertFalse(db.open()); assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); // Override the data schema version - setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 1); + setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 3); db.close(); // Reopen the DB - an exception should be thrown - db = createDatabase(singletonList(migration)); + db = createDatabase(asList(migration, migration1)); db.open(); } @@ -145,22 +153,62 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { public void testRunsMigrationIfCodeIsNewerThanDataAndSuitableMigration() throws Exception { context.checking(new Expectations() {{ + // First migration should be run, increasing schema version by 2 oneOf(migration).getStartVersion(); - will(returnValue(CODE_SCHEMA_VERSION - 1)); + will(returnValue(CODE_SCHEMA_VERSION - 2)); oneOf(migration).getEndVersion(); will(returnValue(CODE_SCHEMA_VERSION)); oneOf(migration).migrate(with(any(Connection.class))); + // Second migration is not suitable and should be skipped + oneOf(migration1).getStartVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 1)); + oneOf(migration1).getEndVersion(); + will(returnValue(CODE_SCHEMA_VERSION)); }}); // Open the DB for the first time - Database db = createDatabase(singletonList(migration)); + Database db = createDatabase(asList(migration, migration1)); assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); // Override the data schema version - setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 1); + setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 2); db.close(); - // Reopen the DB - the migration should be run - db = createDatabase(singletonList(migration)); + // Reopen the DB - the first migration should be run + db = createDatabase(asList(migration, migration1)); assertTrue(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + db.close(); + } + + @Test + public void testRunsMigrationsIfCodeIsNewerThanDataAndSuitableMigrations() + throws Exception { + context.checking(new Expectations() {{ + // First migration should be run, incrementing schema version + oneOf(migration).getStartVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 2)); + oneOf(migration).getEndVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 1)); + oneOf(migration).migrate(with(any(Connection.class))); + // Second migration should be run, incrementing schema version again + oneOf(migration1).getStartVersion(); + will(returnValue(CODE_SCHEMA_VERSION - 1)); + oneOf(migration1).getEndVersion(); + will(returnValue(CODE_SCHEMA_VERSION)); + oneOf(migration1).migrate(with(any(Connection.class))); + }}); + + // Open the DB for the first time + Database db = createDatabase(asList(migration, migration1)); + assertFalse(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); + // Override the data schema version + setDataSchemaVersion(db, CODE_SCHEMA_VERSION - 2); + db.close(); + // Reopen the DB - both migrations should be run + db = createDatabase(asList(migration, migration1)); + assertTrue(db.open()); + assertEquals(CODE_SCHEMA_VERSION, getDataSchemaVersion(db)); db.close(); } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java b/bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java index 1bf215ad7..6a868fd42 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/db/H2MigrationTest.java @@ -3,17 +3,17 @@ package org.briarproject.bramble.db; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import java.sql.Connection; -import java.util.Collection; +import java.util.List; @NotNullByDefault public class H2MigrationTest extends DatabaseMigrationTest { @Override - Database createDatabase( - Collection> migrations) throws Exception { + Database createDatabase(List> migrations) + throws Exception { return new H2Database(config, clock) { @Override - Collection> getMigrations() { + List> getMigrations() { return migrations; } }; From 088564f22f37157baa26340b9055547b61862718 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 1 Feb 2018 10:22:10 +0000 Subject: [PATCH 4/5] Add comment. --- .../main/java/org/briarproject/bramble/db/JdbcDatabase.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 1e49baa58..2a140d5de 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -308,6 +308,12 @@ abstract class JdbcDatabase implements Database { } } + /** + * Compares the schema version stored in the database with the schema + * version used by the current code, applies any suitable migrations to the + * data if necessary, and returns true if the schema now matches the + * current code. + */ private boolean checkSchemaVersion(Connection txn) throws DbException { Settings s = getSettings(txn, DB_SETTINGS_NAMESPACE); int dataSchemaVersion = s.getInt(SCHEMA_VERSION_KEY, -1); From 2d87e34aa2e65d2f50c8a9a5a54d036761a2de9d Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 1 Feb 2018 17:07:54 +0000 Subject: [PATCH 5/5] Throw meaningful exceptions for schema errors. --- .../bramble/api/db/DataTooNewException.java | 7 +++++ .../bramble/api/db/DataTooOldException.java | 8 ++++++ .../bramble/api/db/DatabaseComponent.java | 5 ++++ .../org/briarproject/bramble/db/Database.java | 7 +++++ .../briarproject/bramble/db/JdbcDatabase.java | 26 ++++++++++++------- .../bramble/db/DatabaseMigrationTest.java | 8 +++--- 6 files changed, 49 insertions(+), 12 deletions(-) create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooNewException.java create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooOldException.java diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooNewException.java b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooNewException.java new file mode 100644 index 000000000..001f28f13 --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooNewException.java @@ -0,0 +1,7 @@ +package org.briarproject.bramble.api.db; + +/** + * Thrown when the database uses a newer schema than the current code. + */ +public class DataTooNewException extends DbException { +} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooOldException.java b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooOldException.java new file mode 100644 index 000000000..9d3e16d33 --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DataTooOldException.java @@ -0,0 +1,8 @@ +package org.briarproject.bramble.api.db; + +/** + * Thrown when the database uses an older schema than the current code and + * cannot be migrated. + */ +public class DataTooOldException extends DbException { +} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseComponent.java b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseComponent.java index f12a0680b..ff6ef3885 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseComponent.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/db/DatabaseComponent.java @@ -37,6 +37,11 @@ public interface DatabaseComponent { /** * Opens the database and returns true if the database already existed. + * + * @throws DataTooNewException if the data uses a newer schema than the + * current code + * @throws DataTooOldException if the data uses an older schema than the + * current code and cannot be migrated */ boolean open() throws DbException; diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java b/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java index 93e7bf8c6..71b351452 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java @@ -2,6 +2,8 @@ package org.briarproject.bramble.db; import org.briarproject.bramble.api.contact.Contact; import org.briarproject.bramble.api.contact.ContactId; +import org.briarproject.bramble.api.db.DataTooNewException; +import org.briarproject.bramble.api.db.DataTooOldException; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.db.Metadata; import org.briarproject.bramble.api.identity.Author; @@ -37,6 +39,11 @@ interface Database { /** * Opens the database and returns true if the database already existed. + * + * @throws DataTooNewException if the data uses a newer schema than the + * current code + * @throws DataTooOldException if the data uses an older schema than the + * current code and cannot be migrated */ boolean open() throws DbException; diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 2a140d5de..9aec96283 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -3,6 +3,8 @@ package org.briarproject.bramble.db; import org.briarproject.bramble.api.contact.Contact; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.crypto.SecretKey; +import org.briarproject.bramble.api.db.DataTooNewException; +import org.briarproject.bramble.api.db.DataTooOldException; import org.briarproject.bramble.api.db.DbClosedException; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.db.Metadata; @@ -295,7 +297,7 @@ abstract class JdbcDatabase implements Database { Connection txn = startTransaction(); try { if (reopen) { - if (!checkSchemaVersion(txn)) throw new DbException(); + checkSchemaVersion(txn); } else { createTables(txn); storeSchemaVersion(txn, CODE_SCHEMA_VERSION); @@ -310,16 +312,21 @@ abstract class JdbcDatabase implements Database { /** * Compares the schema version stored in the database with the schema - * version used by the current code, applies any suitable migrations to the - * data if necessary, and returns true if the schema now matches the - * current code. + * version used by the current code and applies any suitable migrations to + * the data if necessary. + * + * @throws DataTooNewException if the data uses a newer schema than the + * current code + * @throws DataTooOldException if the data uses an older schema than the + * current code and cannot be migrated */ - private boolean checkSchemaVersion(Connection txn) throws DbException { + private void checkSchemaVersion(Connection txn) throws DbException { Settings s = getSettings(txn, DB_SETTINGS_NAMESPACE); int dataSchemaVersion = s.getInt(SCHEMA_VERSION_KEY, -1); - if (dataSchemaVersion == -1) return false; - if (dataSchemaVersion == CODE_SCHEMA_VERSION) return true; - if (CODE_SCHEMA_VERSION < dataSchemaVersion) return false; + if (dataSchemaVersion == -1) throw new DbException(); + if (dataSchemaVersion == CODE_SCHEMA_VERSION) return; + if (CODE_SCHEMA_VERSION < dataSchemaVersion) + throw new DataTooNewException(); // Apply any suitable migrations in order for (Migration m : getMigrations()) { int start = m.getStartVersion(), end = m.getEndVersion(); @@ -333,7 +340,8 @@ abstract class JdbcDatabase implements Database { dataSchemaVersion = end; } } - return dataSchemaVersion == CODE_SCHEMA_VERSION; + if (dataSchemaVersion != CODE_SCHEMA_VERSION) + throw new DataTooOldException(); } // Package access for testing diff --git a/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java b/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java index de6cabe05..203f4dfd2 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/db/DatabaseMigrationTest.java @@ -1,5 +1,7 @@ package org.briarproject.bramble.db; +import org.briarproject.bramble.api.db.DataTooNewException; +import org.briarproject.bramble.api.db.DataTooOldException; import org.briarproject.bramble.api.db.DatabaseConfig; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; @@ -95,7 +97,7 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { db.close(); } - @Test(expected = DbException.class) + @Test(expected = DataTooNewException.class) public void testThrowsExceptionIfDataIsNewerThanCode() throws Exception { // Open the DB for the first time Database db = createDatabase(asList(migration, migration1)); @@ -109,7 +111,7 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { db.open(); } - @Test(expected = DbException.class) + @Test(expected = DataTooOldException.class) public void testThrowsExceptionIfCodeIsNewerThanDataAndNoMigrations() throws Exception { // Open the DB for the first time @@ -123,7 +125,7 @@ public abstract class DatabaseMigrationTest extends BrambleMockTestCase { db.open(); } - @Test(expected = DbException.class) + @Test(expected = DataTooOldException.class) public void testThrowsExceptionIfCodeIsNewerThanDataAndNoSuitableMigration() throws Exception { context.checking(new Expectations() {{