Repository: aurora Updated Branches: refs/heads/master 5ecded788 -> 57468d221
Fix bug when checking to ensure that all db migrations on the classpath have been applied. While testing applying the multiple migrations for storing images, I noticed that we were rolling one back on startup when no migration should be performed. It turned out I misunderstood the semantics of the Java8 stream API filter/findFirst combination. Reviewed at https://reviews.apache.org/r/45933/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/57468d22 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/57468d22 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/57468d22 Branch: refs/heads/master Commit: 57468d2212c9d61bcd4f10022ac575ae2d036ffe Parents: 5ecded7 Author: Joshua Cohen <jco...@apache.org> Authored: Fri Apr 8 13:48:05 2016 -0500 Committer: Joshua Cohen <jco...@apache.org> Committed: Fri Apr 8 13:48:05 2016 -0500 ---------------------------------------------------------------------- config/checkstyle/suppressions.xml | 2 +- .../storage/db/MigrationManagerImpl.java | 2 +- .../storage/db/MigrationManagerImplIT.java | 101 +++++++++++++------ .../db/testmigration/V002_TestMigration2.java | 40 ++++++++ 4 files changed, 114 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/57468d22/config/checkstyle/suppressions.xml ---------------------------------------------------------------------- diff --git a/config/checkstyle/suppressions.xml b/config/checkstyle/suppressions.xml index 30a32ce..d9fe92a 100644 --- a/config/checkstyle/suppressions.xml +++ b/config/checkstyle/suppressions.xml @@ -21,6 +21,6 @@ limitations under the License. <!-- Allow use of System.exit() in main. --> <suppress files="org/apache/aurora/scheduler/app/SchedulerMain.java" checks="RegexpSinglelineJava"/> - <suppress files="org/apache/aurora/scheduler/storage/db/testmigration/V001_TestMigration.java" + <suppress files="org/apache/aurora/scheduler/storage/db/testmigration/.*" checks="TypeName" /> </suppressions> http://git-wip-us.apache.org/repos/asf/aurora/blob/57468d22/src/main/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImpl.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImpl.java b/src/main/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImpl.java index e2b8416..ca11e59 100644 --- a/src/main/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImpl.java +++ b/src/main/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImpl.java @@ -94,7 +94,7 @@ public class MigrationManagerImpl implements MigrationManager { // which includes class in its equality check rather than checking instanceof. Instead we just // find the first element in changes whose id matches our applied change. If it does not exist // then this must be a rollback. - if (changes.stream().findFirst().filter(c -> c.getId().equals(change.getId())).isPresent()) { + if (changes.stream().anyMatch(c -> c.getId().equals(change.getId()))) { LOG.info("Change " + change.getId() + " has been applied, no other downgrades are " + "necessary"); break; http://git-wip-us.apache.org/repos/asf/aurora/blob/57468d22/src/test/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImplIT.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImplIT.java b/src/test/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImplIT.java index 5edc2ad..d594bce 100644 --- a/src/test/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImplIT.java +++ b/src/test/java/org/apache/aurora/scheduler/storage/db/MigrationManagerImplIT.java @@ -19,6 +19,7 @@ import java.sql.SQLException; import java.util.List; import java.util.Optional; +import com.google.common.collect.ImmutableList; import com.google.common.io.CharStreams; import com.google.inject.Injector; @@ -42,33 +43,91 @@ public class MigrationManagerImplIT { new DbModule.MigrationManagerModule(migrationLoader)); } + /** + * Ensure all changes have been applied and their downgrade scripts stored appropriately. + * + * @param sqlSessionFactory The sql session factory. + * @param loader A migration loader. + * @throws Exception If the changes were not applied properly. + */ + private void assertMigrationComplete( + SqlSessionFactory sqlSessionFactory, + MigrationLoader loader) throws Exception { + + try (SqlSession session = sqlSessionFactory.openSession()) { + MigrationMapper mapper = session.getMapper(MigrationMapper.class); + + List<MigrationChangelogEntry> appliedChanges = mapper.selectAll(); + + for (Change change : loader.getMigrations()) { + Optional<MigrationChangelogEntry> appliedChange = appliedChanges + .stream() + .filter(c -> c.getId().equals(change.getId())) + .findFirst(); + + assertTrue(appliedChange.isPresent()); + assertEquals( + CharStreams.toString(loader.getScriptReader(change, true /* undo */)), + appliedChange.get().getDowngradeScript()); + } + + // As long as the tables exist, neither of these statements should fail. + try (Connection c = session.getConnection()) { + try (PreparedStatement select = c.prepareStatement("SELECT * FROM V001_test_table")) { + select.execute(); + } + try (PreparedStatement select = c.prepareStatement("SELECT * FROM V002_test_table2")) { + select.execute(); + } + } + } + } + @Test public void testMigrate() throws Exception { MigrationLoader loader = new JavaMigrationLoader( "org.apache.aurora.scheduler.storage.db.testmigration"); Injector injector = createMigrationInjector(loader); + injector.getInstance(MigrationManager.class).migrate(); + assertMigrationComplete(injector.getInstance(SqlSessionFactory.class), loader); + } + + @Test + public void testNoMigrationNecessary() throws Exception { + MigrationLoader loader = new JavaMigrationLoader( + "org.apache.aurora.scheduler.storage.db.testmigration"); + Injector injector = createMigrationInjector(loader); + MigrationManager migrationManager = injector.getInstance(MigrationManager.class); migrationManager.migrate(); SqlSessionFactory sqlSessionFactory = injector.getInstance(SqlSessionFactory.class); + assertMigrationComplete(sqlSessionFactory, loader); + + // Run the migration a second time, no changes should be made. + migrationManager.migrate(); + assertMigrationComplete(sqlSessionFactory, loader); + } + private void assertRollbackComplete(SqlSessionFactory sqlSessionFactory) throws Exception { try (SqlSession session = sqlSessionFactory.openSession()) { MigrationMapper mapper = session.getMapper(MigrationMapper.class); - List<MigrationChangelogEntry> appliedChanges = mapper.selectAll(); - // Ensure all changes have been applied and their downgrade scripts stored appropriately. - for (Change change : loader.getMigrations()) { - Optional<MigrationChangelogEntry> appliedChange = appliedChanges - .stream() - .findFirst() - .filter(c -> c.getId().equals(change.getId())); - - assertTrue(appliedChange.isPresent()); - assertEquals( - CharStreams.toString(loader.getScriptReader(change, true /* undo */)), - appliedChange.get().getDowngradeScript()); + assertTrue(mapper.selectAll().isEmpty()); + try (Connection c = session.getConnection()) { + for (String table : ImmutableList.of("V001_test_table", "V002_test_table2")) { + try (PreparedStatement select = c.prepareStatement("SELECT * FROM " + table)) { + select.execute(); + fail("Select from " + table + " should have failed, the table should have been " + + "dropped."); + } catch (SQLException e) { + // This exception is expected. + assertTrue( + e.getMessage().startsWith("Table \"" + table.toUpperCase() + "\" not found")); + } + } } } } @@ -93,22 +152,6 @@ public class MigrationManagerImplIT { rollbackManager.migrate(); - SqlSessionFactory sqlSessionFactory = rollbackInjector.getInstance(SqlSessionFactory.class); - - try (SqlSession session = sqlSessionFactory.openSession()) { - MigrationMapper mapper = session.getMapper(MigrationMapper.class); - - assertTrue(mapper.selectAll().isEmpty()); - try (Connection c = session.getConnection()) { - try (PreparedStatement select = c.prepareStatement("SELECT * FROM V001_test_table")) { - select.execute(); - fail("Select from V001_test_table should have failed, the table should have been " - + "dropped."); - } catch (SQLException e) { - // This exception is expected. - assertTrue(e.getMessage().startsWith("Table \"V001_TEST_TABLE\" not found")); - } - } - } + assertRollbackComplete(rollbackInjector.getInstance(SqlSessionFactory.class)); } } http://git-wip-us.apache.org/repos/asf/aurora/blob/57468d22/src/test/java/org/apache/aurora/scheduler/storage/db/testmigration/V002_TestMigration2.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/db/testmigration/V002_TestMigration2.java b/src/test/java/org/apache/aurora/scheduler/storage/db/testmigration/V002_TestMigration2.java new file mode 100644 index 0000000..621ca4c --- /dev/null +++ b/src/test/java/org/apache/aurora/scheduler/storage/db/testmigration/V002_TestMigration2.java @@ -0,0 +1,40 @@ +/** + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.aurora.scheduler.storage.db.testmigration; + +import java.math.BigDecimal; + +import org.apache.ibatis.migration.MigrationScript; + +public class V002_TestMigration2 implements MigrationScript { + @Override + public BigDecimal getId() { + return BigDecimal.valueOf(2L); + } + + @Override + public String getDescription() { + return "A second test migration"; + } + + @Override + public String getUpScript() { + return "CREATE TABLE V002_test_table2(id int);"; + } + + @Override + public String getDownScript() { + return "DROP TABLE V002_test_table2;"; + } +}
