From 82ca9cfc30b275ab648cea802d9a2bc0d5599e9b Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Fri, 3 Nov 2023 16:32:46 +1300 Subject: [PATCH 1/2] Refactor fastCheck, move startMs and move setAutoCommitFalse() - Move startMs to be before the resource loading (include that in the time) - Move the setAutoCommitFalse() to occur after fastCheck query (nb: the fast check query will rollback() if executed - Move fast check logic into FirstCheck class - Rename method derivePlatformName() -> derivePlatform()) - Change MigrationTable constructor to task FirstCheck --- .../io/ebean/migration/runner/FirstCheck.java | 89 +++++++++++++++++++ .../migration/runner/MigrationEngine.java | 82 ++++------------- .../migration/runner/MigrationTable.java | 34 ++----- .../migration/runner/MigrationTable1Test.java | 17 ++-- .../migration/runner/MigrationTable2Test.java | 14 ++- .../runner/MigrationTableAsyncTest.java | 7 +- .../MigrationTableCreateTableRaceTest.java | 3 +- .../runner/MigrationTableTestDb2.java | 5 +- 8 files changed, 148 insertions(+), 103 deletions(-) create mode 100644 ebean-migration/src/main/java/io/ebean/migration/runner/FirstCheck.java diff --git a/ebean-migration/src/main/java/io/ebean/migration/runner/FirstCheck.java b/ebean-migration/src/main/java/io/ebean/migration/runner/FirstCheck.java new file mode 100644 index 0000000..72becb1 --- /dev/null +++ b/ebean-migration/src/main/java/io/ebean/migration/runner/FirstCheck.java @@ -0,0 +1,89 @@ +package io.ebean.migration.runner; + +import io.ebean.migration.MigrationConfig; + +import java.sql.Connection; +import java.sql.SQLException; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * First initial check to see if migrations exist and exactly match. + */ +final class FirstCheck { + + final MigrationConfig config; + final MigrationPlatform platform; + final Connection connection; + final String schema; + final String table; + final String sqlTable; + boolean tableKnownToExist; + private int count; + + FirstCheck(MigrationConfig config, Connection connection, MigrationPlatform platform) { + this.config = config; + this.platform = platform; + this.connection = connection; + this.schema = config.getDbSchema(); + this.table = config.getMetaTable(); + this.sqlTable = schema != null ? schema + '.' + table : table; + } + + MigrationTable initTable(boolean checkStateOnly) { + return new MigrationTable(this, checkStateOnly); + } + + boolean fastModeCheck(List versions) { + try { + final List rows = fastRead(); + tableKnownToExist = !rows.isEmpty(); + if (rows.size() != versions.size() + 1) { + // difference in count of migrations + return false; + } + final Map dbChecksums = dbChecksumMap(rows); + for (LocalMigrationResource local : versions) { + Integer dbChecksum = dbChecksums.get(local.key()); + if (dbChecksum == null) { + // no match, unexpected missing migration + return false; + } + int localChecksum = checksumFor(local); + if (localChecksum != dbChecksum) { + // no match, perhaps repeatable migration change + return false; + } + } + // successful fast check + count = versions.size(); + return true; + } catch (SQLException e) { + // probably migration table does not exist + return false; + } + } + + private static Map dbChecksumMap(List rows) { + return rows.stream().collect(Collectors.toMap(MigrationMetaRow::version, MigrationMetaRow::checksum)); + } + + private int checksumFor(LocalMigrationResource local) { + if (local instanceof LocalUriMigrationResource) { + return ((LocalUriMigrationResource) local).checksum(); + } else if (local instanceof LocalDdlMigrationResource) { + return Checksum.calculate(local.content()); + } else { + return ((LocalJdbcMigrationResource) local).checksum(); + } + } + + List fastRead() throws SQLException { + return platform.fastReadMigrations(sqlTable, connection); + } + + int count() { + return count; + } +} diff --git a/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationEngine.java b/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationEngine.java index 056f3ce..7547dc1 100644 --- a/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationEngine.java +++ b/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationEngine.java @@ -7,13 +7,11 @@ import java.sql.Connection; import java.sql.SQLException; -import java.util.Collections; import java.util.List; -import java.util.Map; -import java.util.stream.Collectors; import static java.lang.System.Logger.Level.*; import static java.lang.System.Logger.Level.WARNING; +import static java.util.Collections.emptyList; /** * Actually runs the migrations. @@ -25,8 +23,6 @@ public class MigrationEngine { private final MigrationConfig migrationConfig; private final boolean checkStateOnly; private final boolean fastMode; - private int fastModeCount; - private MigrationTable table; /** * Create with the MigrationConfig. @@ -42,23 +38,25 @@ public MigrationEngine(MigrationConfig migrationConfig, boolean checkStateOnly) */ public List run(Connection connection) { try { + long startMs = System.currentTimeMillis(); LocalMigrationResources resources = new LocalMigrationResources(migrationConfig); if (!resources.readResources() && !resources.readInitResources()) { log.log(DEBUG, "no migrations to check"); - return Collections.emptyList(); + return emptyList(); } - long startMs = System.currentTimeMillis(); - setAutoCommitFalse(connection); - table = initMigrationTable(connection); - if (fastMode && fastModeCheck(resources.versions())) { + final var platform = derivePlatform(migrationConfig, connection); + final var firstCheck = new FirstCheck(migrationConfig, connection, platform); + if (fastMode && firstCheck.fastModeCheck(resources.versions())) { long checkMs = System.currentTimeMillis() - startMs; - log.log(INFO, "DB migrations completed in {0}ms - totalMigrations:{1}", checkMs, fastModeCount); - return Collections.emptyList(); + log.log(INFO, "DB migrations completed in {0}ms - totalMigrations:{1}", checkMs, firstCheck.count()); + return emptyList(); } + // ensure running with autoCommit false + setAutoCommitFalse(connection); - initialiseMigrationTable(connection); + final MigrationTable table = initialiseMigrationTable(firstCheck, connection); try { - List result = runMigrations(resources.versions()); + List result = runMigrations(table, resources.versions()); connection.commit(); if (!checkStateOnly) { long commitMs = System.currentTimeMillis(); @@ -92,57 +90,11 @@ private static void setAutoCommitFalse(Connection connection) { } } - private MigrationTable initMigrationTable(Connection connection) { - final MigrationPlatform platform = derivePlatformName(migrationConfig, connection); - return new MigrationTable(migrationConfig, connection, checkStateOnly, platform); - } - - private boolean fastModeCheck(List versions) { - try { - final List rows = table.fastRead(); - if (rows.size() != versions.size() + 1) { - // difference in count of migrations - return false; - } - final Map dbChecksums = dbChecksumMap(rows); - for (LocalMigrationResource local : versions) { - Integer dbChecksum = dbChecksums.get(local.key()); - if (dbChecksum == null) { - // no match, unexpected missing migration - return false; - } - int localChecksum = checksumFor(local); - if (localChecksum != dbChecksum) { - // no match, perhaps repeatable migration change - return false; - } - } - // successful fast check - fastModeCount = versions.size(); - return true; - } catch (SQLException e) { - // probably migration table does not exist - return false; - } - } - - private static Map dbChecksumMap(List rows) { - return rows.stream().collect(Collectors.toMap(MigrationMetaRow::version, MigrationMetaRow::checksum)); - } - - private int checksumFor(LocalMigrationResource local) { - if (local instanceof LocalUriMigrationResource) { - return ((LocalUriMigrationResource)local).checksum(); - } else if (local instanceof LocalDdlMigrationResource) { - return Checksum.calculate(local.content()); - } else { - return ((LocalJdbcMigrationResource) local).checksum(); - } - } - - private void initialiseMigrationTable(Connection connection) { + private MigrationTable initialiseMigrationTable(FirstCheck firstCheck, Connection connection) { try { + final MigrationTable table = firstCheck.initTable(checkStateOnly); table.createIfNeededAndLock(); + return table; } catch (Throwable e) { rollback(connection); throw new MigrationException("Error initialising db migrations table", e); @@ -152,7 +104,7 @@ private void initialiseMigrationTable(Connection connection) { /** * Run all the migrations as needed. */ - private List runMigrations(List localVersions) throws SQLException { + private List runMigrations(MigrationTable table, List localVersions) throws SQLException { // get the migrations in version order if (table.isEmpty()) { LocalMigrationResource initVersion = lastInitVersion(); @@ -182,7 +134,7 @@ private LocalMigrationResource lastInitVersion() { /** * Return the platform deriving from connection if required. */ - private MigrationPlatform derivePlatformName(MigrationConfig migrationConfig, Connection connection) { + private MigrationPlatform derivePlatform(MigrationConfig migrationConfig, Connection connection) { final String platform = migrationConfig.getPlatform(); if (platform != null) { return DbNameUtil.platform(platform); diff --git a/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationTable.java b/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationTable.java index ca17608..93550fe 100644 --- a/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationTable.java +++ b/ebean-migration/src/main/java/io/ebean/migration/runner/MigrationTable.java @@ -74,13 +74,15 @@ final class MigrationTable { private MigrationMetaRow initMetaRow; private boolean tableKnownToExist; - /** - * Construct with server, configuration and jdbc connection (DB admin user). - */ - MigrationTable(MigrationConfig config, Connection connection, boolean checkStateOnly, MigrationPlatform platform) { - this.config = config; - this.platform = platform; - this.connection = connection; + public MigrationTable(FirstCheck firstCheck, boolean checkStateOnly) { + this.config = firstCheck.config; + this.platform = firstCheck.platform; + this.connection = firstCheck.connection; + this.schema = firstCheck.schema; + this.table = firstCheck.table; + this.sqlTable = firstCheck.sqlTable; + this.tableKnownToExist = firstCheck.tableKnownToExist; + this.scriptRunner = new MigrationScriptRunner(connection, platform); this.checkStateOnly = checkStateOnly; this.earlyChecksumMode = config.isEarlyChecksumMode(); @@ -93,11 +95,8 @@ final class MigrationTable { this.minVersionFailMessage = config.getMinVersionFailMessage(); this.skipMigrationRun = config.isSkipMigrationRun(); this.skipChecksum = config.isSkipChecksum(); - this.schema = config.getDbSchema(); - this.table = config.getMetaTable(); this.basePlatformName = config.getBasePlatform(); this.platformName = config.getPlatform(); - this.sqlTable = initSqlTable(); this.insertSql = MigrationMetaRow.insertSql(sqlTable); this.updateSql = MigrationMetaRow.updateSql(sqlTable); this.updateChecksumSql = MigrationMetaRow.updateChecksumSql(sqlTable); @@ -109,14 +108,6 @@ private MigrationVersion initMinVersion(String minVersion) { return (minVersion == null || minVersion.isEmpty()) ? null : MigrationVersion.parse(minVersion); } - private String initSqlTable() { - if (schema != null) { - return schema + "." + table; - } else { - return table; - } - } - private String sqlPrimaryKey() { return "pk_" + table; } @@ -193,13 +184,6 @@ void unlockMigrationTable() { platform.unlockMigrationTable(sqlTable, connection); } - List fastRead() throws SQLException { - final var result = platform.fastReadMigrations(sqlTable, connection); - // if we know the migration table exists we can skip those checks - tableKnownToExist = !result.isEmpty(); - return result; - } - /** * Read the migration table with details on what migrations have run. * This must execute after we have completed the wait for the lock on diff --git a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable1Test.java b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable1Test.java index 8d9e67b..99723e6 100644 --- a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable1Test.java +++ b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable1Test.java @@ -42,6 +42,12 @@ public void shutdown() { dataSource.shutdown(); } + + private MigrationTable migrationTable(Connection conn) { + var fc = new FirstCheck(config, conn, platform); + return new MigrationTable(fc, false); + } + @Test public void testMigrationTableBase() throws Exception { @@ -50,7 +56,7 @@ public void testMigrationTableBase() throws Exception { MigrationRunner runner = new MigrationRunner(config); runner.run(dataSource); try (Connection conn = dataSource.getConnection()) { - MigrationTable table = new MigrationTable(config, conn, false, platform); + MigrationTable table = migrationTable(conn); table.createIfNeededAndLock(); assertThat(table.versions()).containsExactly("hello", "1.1", "1.2", "1.2.1", "m2_view"); @@ -68,6 +74,7 @@ public void testMigrationTableBase() throws Exception { } } + @Test public void testMigrationTableRepeatableOk() throws Exception { @@ -77,7 +84,7 @@ public void testMigrationTableRepeatableOk() throws Exception { runner.run(dataSource); try (Connection conn = dataSource.getConnection()) { - MigrationTable table = new MigrationTable(config, conn, false, platform); + MigrationTable table = migrationTable(conn); table.createIfNeededAndLock(); assertThat(table.versions()).containsExactly("1.1"); table.unlockMigrationTable(); @@ -90,7 +97,7 @@ public void testMigrationTableRepeatableOk() throws Exception { runner.run(dataSource); try (Connection conn = dataSource.getConnection()) { - MigrationTable table = new MigrationTable(config, conn, false, platform); + MigrationTable table = migrationTable(conn); table.createIfNeededAndLock(); assertThat(table.versions()).containsExactly("1.1", "1.2", "m2_view"); table.unlockMigrationTable(); @@ -108,7 +115,7 @@ public void testMigrationTableRepeatableFail() throws Exception { runner.run(dataSource); try (Connection conn = dataSource.getConnection()) { - MigrationTable table = new MigrationTable(config, conn, false, platform); + MigrationTable table = migrationTable(conn); table.createIfNeededAndLock(); assertThat(table.versions()).containsExactly("1.1"); table.unlockMigrationTable(); @@ -141,7 +148,7 @@ public void testMigrationTableRepeatableFail() throws Exception { assertThat(m3Content).contains("text with ; sign"); // we expect, that 1.1 and 1.2 is executed (but not the R__ script) - MigrationTable table = new MigrationTable(config, conn, false, platform); + MigrationTable table = migrationTable(conn); table.createIfNeededAndLock(); assertThat(table.versions()).containsExactly("1.1", "1.2"); conn.rollback(); diff --git a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable2Test.java b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable2Test.java index 26eb4c2..f916dd0 100644 --- a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable2Test.java +++ b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTable2Test.java @@ -10,13 +10,19 @@ public class MigrationTable2Test { + + private static MigrationTable migrationTable(MigrationConfig config) { + var fc = new FirstCheck(config, null, new MigrationPlatform()); + return new MigrationTable(fc, false); + } + @Test void testCreateTableDdl() throws Exception { MigrationConfig config = new MigrationConfig(); config.setDbSchema("foo"); - MigrationTable mt = new MigrationTable(config, null, false, new MigrationPlatform()); + MigrationTable mt = migrationTable(config); String tableSql = mt.createTableDdl(); assertThat(tableSql).contains("create table foo.db_migration "); @@ -30,7 +36,7 @@ void testCreateTableDdl_sqlserver() throws Exception { config.setDbSchema("bar"); config.setPlatform(DbPlatformNames.SQLSERVER); - MigrationTable mt = new MigrationTable(config, null, false, new MigrationPlatform()); + MigrationTable mt = migrationTable(config); String tableSql = mt.createTableDdl(); assertThat(tableSql).contains("datetime2 "); @@ -46,7 +52,7 @@ void test_skipMigration_Repeatable() throws Exception { LocalMigrationResource local = local("R__hello"); MigrationMetaRow existing = new MigrationMetaRow(12, "R", "", "comment", 42, null, null, 0); - MigrationTable mt = new MigrationTable(config, null, false, new MigrationPlatform()); + MigrationTable mt = migrationTable(config); // checksum different - no skip assertFalse(mt.skipMigration(100, 100, local, existing)); // checksum same - skip @@ -59,7 +65,7 @@ void test_skipMigration_skipChecksum() throws Exception { MigrationConfig config = new MigrationConfig(); config.setSkipChecksum(true); - MigrationTable mt = new MigrationTable(config, null, false, new MigrationPlatform()); + MigrationTable mt = migrationTable(config); // skip regardless of checksum difference LocalMigrationResource local = local("R__hello"); diff --git a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java index 51cf7e8..1f263eb 100644 --- a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java +++ b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java @@ -185,7 +185,7 @@ private void runTest(boolean withExisting) throws SQLException, InterruptedExcep config.setPlatform(derivedPlatformName); MigrationPlatform platform = DbNameUtil.platform(derivedPlatformName); - MigrationTable table = new MigrationTable(config, conn, false, platform); + MigrationTable table = migrationTable(platform); table.createIfNeededAndLock(); table.unlockMigrationTable(); conn.commit(); @@ -203,6 +203,11 @@ private void runTest(boolean withExisting) throws SQLException, InterruptedExcep } } + private static MigrationTable migrationTable(MigrationPlatform platform) { + var fc = new FirstCheck(config, null, platform); + return new MigrationTable(fc, false); + } + private void dropTable(String tableName) throws SQLException { try (Connection conn = dataSource.getConnection(); Statement stmt = conn.createStatement()) { diff --git a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableCreateTableRaceTest.java b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableCreateTableRaceTest.java index 521d168..b5d226c 100644 --- a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableCreateTableRaceTest.java +++ b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableCreateTableRaceTest.java @@ -51,7 +51,8 @@ void testRaceCondition_expect_loserOfCreateTableCanPerformTableExistsCheck() thr try (Connection conn = dataSource.getConnection()) { dropTable(conn); - MigrationTable table = new MigrationTable(config, conn, false, platform); + var fc = new FirstCheck(config, conn, platform); + MigrationTable table = new MigrationTable(fc, false); table.createTable(); try { // simulate losing the race, this createTable() will fail as the table exists diff --git a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableTestDb2.java b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableTestDb2.java index 1024a6b..4777ed4 100644 --- a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableTestDb2.java +++ b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableTestDb2.java @@ -15,7 +15,7 @@ public class MigrationTableTestDb2 { private static MigrationConfig config; private static DataSourcePool dataSource; - private MigrationPlatform platform = new MigrationPlatform(); + private final MigrationPlatform platform = new MigrationPlatform(); @BeforeEach public void setUp() { @@ -44,7 +44,8 @@ public void testMigrationTableBase() throws Exception { config.setMigrationPath("dbmig"); try (Connection conn = dataSource.getConnection()) { - MigrationTable table = new MigrationTable(config, conn, false, platform); + var fc = new FirstCheck(config, conn, platform); + MigrationTable table = new MigrationTable(fc, false); table.createIfNeededAndLock(); table.unlockMigrationTable(); table.createIfNeededAndLock(); From 7877351255d13227cff78a064b7ce9b6d2dd2ce9 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Fri, 3 Nov 2023 16:41:24 +1300 Subject: [PATCH 2/2] Fix test MigrationTableAsyncTest as I messed it up leaving out the connection --- .../io/ebean/migration/runner/MigrationTableAsyncTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java index 1f263eb..38bc0b6 100644 --- a/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java +++ b/ebean-migration/src/test/java/io/ebean/migration/runner/MigrationTableAsyncTest.java @@ -185,7 +185,7 @@ private void runTest(boolean withExisting) throws SQLException, InterruptedExcep config.setPlatform(derivedPlatformName); MigrationPlatform platform = DbNameUtil.platform(derivedPlatformName); - MigrationTable table = migrationTable(platform); + MigrationTable table = migrationTable(platform, conn); table.createIfNeededAndLock(); table.unlockMigrationTable(); conn.commit(); @@ -203,8 +203,8 @@ private void runTest(boolean withExisting) throws SQLException, InterruptedExcep } } - private static MigrationTable migrationTable(MigrationPlatform platform) { - var fc = new FirstCheck(config, null, platform); + private static MigrationTable migrationTable(MigrationPlatform platform, Connection connection) { + var fc = new FirstCheck(config, connection, platform); return new MigrationTable(fc, false); }