From 11e55cebf55241c4392d74fef49ba6683f94d924 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Sat, 14 Dec 2019 17:26:56 -0500 Subject: [PATCH 1/5] begin with failing test --- .../uhn/fhir/jpa/migrate/SchemaMigrator.java | 21 ++++++++++++++ .../fhir/jpa/migrate/SchemaMigratorTest.java | 28 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java index 00d0f132270..7ff36d30761 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java @@ -44,6 +44,7 @@ public class SchemaMigrator { private final List> myMigrationTasks; private boolean myDontUseFlyway; private boolean myOutOfOrderPermitted; + private String mySkipVersions; private DriverTypeEnum myDriverType; /** @@ -132,4 +133,24 @@ public class SchemaMigrator { public void setDriverType(DriverTypeEnum theDriverType) { myDriverType = theDriverType; } + + public SchemaMigrator setSkipVersions(String theSkipVersions) { + mySkipVersions = theSkipVersions; + setDoNothingOnSkippedTasks(); + return this; + } + + private void setDoNothingOnSkippedTasks() { + if (isBlank(mySkipVersions) || myMigrationTasks.isEmpty()) { + return; + } + Set skippedVersionSet = Stream.of(mySkipVersions.split(",")).map(String::trim).collect(Collectors.toSet()); + + for (BaseTask task : myMigrationTasks) { + if (skippedVersionSet.contains(task.getFlywayVersion())) { + ourLog.info("Will skip {}: {}", task.getFlywayVersion(), task.getDescription()); + task.setDoNothing(true); + } + } + } } diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java index 924cbdc5975..8d80b3d7be1 100644 --- a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java @@ -66,6 +66,34 @@ public class SchemaMigratorTest extends BaseTest { schemaMigrator.migrate(); } + @Test + public void testSkipSchemaVersion() throws SQLException { + AddTableRawSqlTask taskA = new AddTableRawSqlTask("V4_1_0", "20191214.1"); + taskA.setTableName("SOMETABLE_A"); + taskA.addSql(DriverTypeEnum.H2_EMBEDDED, "create table SOMETABLE_A (PID bigint not null, TEXTCOL varchar(255))"); + + AddTableRawSqlTask taskB = new AddTableRawSqlTask("V4_1_0", "20191214.2"); + taskB.setTableName("SOMETABLE_B"); + taskB.addSql(DriverTypeEnum.H2_EMBEDDED, "create table SOMETABLE_B (PID bigint not null, TEXTCOL varchar(255))"); + + AddTableRawSqlTask taskC = new AddTableRawSqlTask("V4_1_0", "20191214.3"); + taskC.setTableName("SOMETABLE_C"); + taskC.addSql(DriverTypeEnum.H2_EMBEDDED, "create table SOMETABLE_C (PID bigint not null, TEXTCOL varchar(255))"); + + AddTableRawSqlTask taskD = new AddTableRawSqlTask("V4_1_0", "20191214.4"); + taskD.setTableName("SOMETABLE_D"); + taskD.addSql(DriverTypeEnum.H2_EMBEDDED, "create table SOMETABLE_D (PID bigint not null, TEXTCOL varchar(255))"); + + SchemaMigrator schemaMigrator = new SchemaMigrator(SchemaMigrator.HAPI_FHIR_MIGRATION_TABLENAME, getDataSource(), new Properties(), ImmutableList.of(taskA, taskB, taskC, taskD)); + schemaMigrator.setDriverType(DriverTypeEnum.H2_EMBEDDED); + + schemaMigrator.setSkipVersions("4_1_0.20191214.2, 4_1_0.20191214.4"); + schemaMigrator.migrate(); + + DriverTypeEnum.ConnectionProperties connectionProperties = DriverTypeEnum.H2_EMBEDDED.newConnectionProperties(getDataSource().getUrl(), getDataSource().getUsername(), getDataSource().getPassword()); + Set tableNames = JdbcUtils.getTableNames(connectionProperties); + assertThat(tableNames, Matchers.containsInAnyOrder("SOMETABLE_A", "SOMETABLE_C")); + } @Test public void testMigrationRequiredNoFlyway() throws SQLException { From 4946fffac243d27781c652042ffc10cfe12bb433 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Sat, 14 Dec 2019 17:27:17 -0500 Subject: [PATCH 2/5] pass test --- .../ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java | 3 ++- .../main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java index 69e3c5c6c0b..5828bf897a7 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java +++ b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java @@ -47,6 +47,7 @@ public abstract class BaseFlywayMigrateDatabaseCommand extends B public static final String NO_COLUMN_SHRINK = "no-column-shrink"; public static final String DONT_USE_FLYWAY = "dont-use-flyway"; public static final String OUT_OF_ORDER_PERMITTED = "out-of-order-permitted"; + public static final String SKIP_VERSIONS = "skip-versions"; private Set myFlags; private String myMigrationTableName; @@ -80,7 +81,6 @@ public abstract class BaseFlywayMigrateDatabaseCommand extends B Options retVal = new Options(); addOptionalOption(retVal, "r", "dry-run", false, "Log the SQL statements that would be executed but to not actually make any changes"); - addRequiredOption(retVal, "u", "url", "URL", "The JDBC database URL"); addRequiredOption(retVal, "n", "username", "Username", "The JDBC database username"); addRequiredOption(retVal, "p", "password", "Password", "The JDBC database password"); @@ -89,6 +89,7 @@ public abstract class BaseFlywayMigrateDatabaseCommand extends B addOptionalOption(retVal, null, DONT_USE_FLYWAY,false, "If this option is set, the migrator will not use FlywayDB for migration. This setting should only be used if you are trying to migrate a legacy database platform that is not supported by FlywayDB."); addOptionalOption(retVal, null, OUT_OF_ORDER_PERMITTED,false, "If this option is set, the migrator will permit migration tasks to be run out of order. It shouldn't be required in most cases, however may be the solution if you see the error message 'Detected resolved migration not applied to database'."); addOptionalOption(retVal, null, NO_COLUMN_SHRINK, false, "If this flag is set, the system will not attempt to reduce the length of columns. This is useful in environments with a lot of existing data, where shrinking a column can take a very long time."); + addOptionalOption(retVal, null, SKIP_VERSIONS, "Versions", "A comma separated list of schema versions to skip. E.g. 4_1_0.20191214.2,4_1_0.20191214.4\""); return retVal; } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java index 7ff36d30761..3bd7210c75a 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java @@ -34,6 +34,11 @@ import java.sql.SQLException; import java.util.List; import java.util.Optional; import java.util.Properties; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static org.apache.commons.lang3.StringUtils.isBlank; public class SchemaMigrator { public static final String HAPI_FHIR_MIGRATION_TABLENAME = "FLY_HFJ_MIGRATION"; From 7c3514fecce4638e98a8f132f5b654dd72524aa4 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Sat, 14 Dec 2019 19:00:58 -0500 Subject: [PATCH 3/5] Skip versions tested --- .../cli/BaseFlywayMigrateDatabaseCommand.java | 24 ++--- .../cli/HapiFlywayMigrateDatabaseCommand.java | 6 +- .../ca/uhn/fhir/jpa/migrate/BaseMigrator.java | 13 +-- .../uhn/fhir/jpa/migrate/FlywayMigrator.java | 4 +- .../ca/uhn/fhir/jpa/migrate/IMigrator.java | 16 ++++ .../jpa/migrate/MigrationTaskSkipper.java | 34 +++++++ .../ca/uhn/fhir/jpa/migrate/Migrator.java | 4 +- .../uhn/fhir/jpa/migrate/SchemaMigrator.java | 32 +------ .../fhir/jpa/migrate/TaskOnlyMigrator.java | 6 +- .../migrate/tasks/api/BaseMigrationTasks.java | 18 ++-- .../fhir/jpa/migrate/tasks/api/Builder.java | 6 +- .../jpa/migrate/MigrationTaskSkipperTest.java | 91 +++++++++++++++++++ .../fhir/jpa/migrate/SchemaMigratorTest.java | 6 +- .../fhir/jpa/migrate/taskdef/HashTest.java | 4 +- .../tasks/api/BaseMigrationTasksTest.java | 8 +- 15 files changed, 193 insertions(+), 79 deletions(-) create mode 100644 hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/IMigrator.java create mode 100644 hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java create mode 100644 hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java diff --git a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java index 5828bf897a7..648625c38a3 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java +++ b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java @@ -20,19 +20,16 @@ package ca.uhn.fhir.cli; * #L% */ -import ca.uhn.fhir.jpa.migrate.BaseMigrator; -import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; -import ca.uhn.fhir.jpa.migrate.FlywayMigrator; -import ca.uhn.fhir.jpa.migrate.TaskOnlyMigrator; +import ca.uhn.fhir.jpa.migrate.*; +import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; import org.apache.commons.cli.CommandLine; import org.apache.commons.cli.Options; import org.apache.commons.cli.ParseException; import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; import static org.apache.commons.lang3.StringUtils.defaultString; @@ -41,7 +38,7 @@ import static org.apache.commons.lang3.StringUtils.defaultString; * NB since 2019-12-05: This class is kind of weirdly named now, since it can either use Flyway or not use Flyway */ public abstract class BaseFlywayMigrateDatabaseCommand extends BaseCommand { - + private static final Logger ourLog = LoggerFactory.getLogger(BaseFlywayMigrateDatabaseCommand.class); public static final String MIGRATE_DATABASE = "migrate-database"; public static final String NO_COLUMN_SHRINK = "no-column-shrink"; @@ -138,13 +135,18 @@ public abstract class BaseFlywayMigrateDatabaseCommand extends B migrator.setDryRun(dryRun); migrator.setNoColumnShrink(noColumnShrink); migrator.setOutOfOrderPermitted(outOfOrderPermitted); - addTasks(migrator); + String skipVersions = theCommandLine.getOptionValue(BaseFlywayMigrateDatabaseCommand.SKIP_VERSIONS); + addTasks(migrator, skipVersions); migrator.migrate(); } - protected abstract void addTasks(BaseMigrator theMigrator); + protected abstract void addTasks(BaseMigrator theMigrator, String theSkippedVersions); public void setMigrationTableName(String theMigrationTableName) { myMigrationTableName = theMigrationTableName; } + + protected void setDoNothingOnSkippedTasks(Collection theTasks, String theSkipVersions) { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(theTasks, theSkipVersions); + } } diff --git a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/HapiFlywayMigrateDatabaseCommand.java b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/HapiFlywayMigrateDatabaseCommand.java index 19e1b52e746..13165207d09 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/HapiFlywayMigrateDatabaseCommand.java +++ b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/HapiFlywayMigrateDatabaseCommand.java @@ -21,7 +21,6 @@ package ca.uhn.fhir.cli; */ import ca.uhn.fhir.jpa.migrate.BaseMigrator; -import ca.uhn.fhir.jpa.migrate.FlywayMigrator; import ca.uhn.fhir.jpa.migrate.SchemaMigrator; import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; import ca.uhn.fhir.jpa.migrate.tasks.HapiFhirJpaMigrationTasks; @@ -45,8 +44,9 @@ public class HapiFlywayMigrateDatabaseCommand extends BaseFlywayMigrateDatabaseC } @Override - protected void addTasks(BaseMigrator theMigrator) { - List> tasks = new HapiFhirJpaMigrationTasks(getFlags()).getAllTasks(VersionEnum.values()); + protected void addTasks(BaseMigrator theMigrator, String theSkipVersions) { + List tasks = new HapiFhirJpaMigrationTasks(getFlags()).getAllTasks(VersionEnum.values()); + super.setDoNothingOnSkippedTasks(tasks, theSkipVersions); theMigrator.addTasks(tasks); } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/BaseMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/BaseMigrator.java index fef2384c5cb..cfe0b62aad6 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/BaseMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/BaseMigrator.java @@ -21,14 +21,15 @@ package ca.uhn.fhir.jpa.migrate; */ import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; -import org.flywaydb.core.api.MigrationInfoService; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.List; import java.util.Objects; -import java.util.Optional; -public abstract class BaseMigrator { +public abstract class BaseMigrator implements IMigrator { + private static final Logger ourLog = LoggerFactory.getLogger(BaseMigrator.class); private boolean myDryRun; private boolean myNoColumnShrink; @@ -39,8 +40,6 @@ public abstract class BaseMigrator { private String myPassword; private List myExecutedStatements = new ArrayList<>(); - public abstract void migrate(); - public boolean isDryRun() { return myDryRun; } @@ -57,10 +56,6 @@ public abstract class BaseMigrator { myNoColumnShrink = theNoColumnShrink; } - public abstract Optional getMigrationInfo(); - - public abstract void addTasks(List> theMigrationTasks); - public DriverTypeEnum getDriverType() { return myDriverType; } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java index bbf3189c353..20458b8c691 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/FlywayMigrator.java @@ -62,7 +62,7 @@ public class FlywayMigrator extends BaseMigrator { myMigrationTableName = theMigrationTableName; } - public void addTask(BaseTask theTask) { + public void addTask(BaseTask theTask) { myTasks.add(new FlywayMigration(theTask, this)); } @@ -97,7 +97,7 @@ public class FlywayMigrator extends BaseMigrator { } @Override - public void addTasks(List> theTasks) { + public void addTasks(List theTasks) { theTasks.forEach(this::addTask); } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/IMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/IMigrator.java new file mode 100644 index 00000000000..237acf888a2 --- /dev/null +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/IMigrator.java @@ -0,0 +1,16 @@ +package ca.uhn.fhir.jpa.migrate; + +import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; +import org.flywaydb.core.api.MigrationInfoService; + +import java.util.List; +import java.util.Optional; + +public interface IMigrator { + + void migrate(); + + Optional getMigrationInfo(); + + void addTasks(List theMigrationTasks); +} diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java new file mode 100644 index 00000000000..9ca06f3224d --- /dev/null +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java @@ -0,0 +1,34 @@ +package ca.uhn.fhir.jpa.migrate; + +import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.Collection; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static org.apache.commons.lang3.StringUtils.isBlank; + +public class MigrationTaskSkipper { + private static final Logger ourLog = LoggerFactory.getLogger(MigrationTaskSkipper.class); + + public static void setDoNothingOnSkippedTasks(Collection theTasks, String theSkipVersions) { + if (isBlank(theSkipVersions) || theTasks.isEmpty()) { + return; + } + Set skippedVersionSet = Stream.of(theSkipVersions.split(",")) + .map(String::trim) + .filter(StringUtils::isNotBlank) + .collect(Collectors.toSet()); + + for (BaseTask task : theTasks) { + if (skippedVersionSet.contains(task.getFlywayVersion())) { + ourLog.info("Will skip {}: {}", task.getFlywayVersion(), task.getDescription()); + task.setDoNothing(true); + } + } + } +} diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/Migrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/Migrator.java index 9cadd2feb20..5820c464c35 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/Migrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/Migrator.java @@ -79,7 +79,7 @@ public class Migrator { myConnectionProperties = myDriverType.newConnectionProperties(myConnectionUrl, myUsername, myPassword); try { - for (BaseTask next : myTasks) { + for (BaseTask next : myTasks) { next.setDriverType(myDriverType); next.setConnectionProperties(myConnectionProperties); next.setDryRun(myDryRun); @@ -125,7 +125,7 @@ public class Migrator { } - public void addTasks(List> theTasks) { + public void addTasks(List theTasks) { theTasks.forEach(this::addTask); } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java index 3bd7210c75a..82242aad2e8 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/SchemaMigrator.java @@ -34,28 +34,22 @@ import java.sql.SQLException; import java.util.List; import java.util.Optional; import java.util.Properties; -import java.util.Set; -import java.util.stream.Collectors; -import java.util.stream.Stream; - -import static org.apache.commons.lang3.StringUtils.isBlank; public class SchemaMigrator { - public static final String HAPI_FHIR_MIGRATION_TABLENAME = "FLY_HFJ_MIGRATION"; private static final Logger ourLog = LoggerFactory.getLogger(SchemaMigrator.class); + public static final String HAPI_FHIR_MIGRATION_TABLENAME = "FLY_HFJ_MIGRATION"; private final BasicDataSource myDataSource; private final boolean mySkipValidation; private final String myMigrationTableName; - private final List> myMigrationTasks; + private final List myMigrationTasks; private boolean myDontUseFlyway; private boolean myOutOfOrderPermitted; - private String mySkipVersions; private DriverTypeEnum myDriverType; /** * Constructor */ - public SchemaMigrator(String theMigrationTableName, BasicDataSource theDataSource, Properties jpaProperties, List> theMigrationTasks) { + public SchemaMigrator(String theMigrationTableName, BasicDataSource theDataSource, Properties jpaProperties, List theMigrationTasks) { myDataSource = theDataSource; myMigrationTableName = theMigrationTableName; myMigrationTasks = theMigrationTasks; @@ -138,24 +132,4 @@ public class SchemaMigrator { public void setDriverType(DriverTypeEnum theDriverType) { myDriverType = theDriverType; } - - public SchemaMigrator setSkipVersions(String theSkipVersions) { - mySkipVersions = theSkipVersions; - setDoNothingOnSkippedTasks(); - return this; - } - - private void setDoNothingOnSkippedTasks() { - if (isBlank(mySkipVersions) || myMigrationTasks.isEmpty()) { - return; - } - Set skippedVersionSet = Stream.of(mySkipVersions.split(",")).map(String::trim).collect(Collectors.toSet()); - - for (BaseTask task : myMigrationTasks) { - if (skippedVersionSet.contains(task.getFlywayVersion())) { - ourLog.info("Will skip {}: {}", task.getFlywayVersion(), task.getDescription()); - task.setDoNothing(true); - } - } - } } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java index 98503be3a24..68b44222d88 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/TaskOnlyMigrator.java @@ -38,13 +38,13 @@ import java.util.Optional; public class TaskOnlyMigrator extends BaseMigrator { private static final Logger ourLog = LoggerFactory.getLogger(TaskOnlyMigrator.class); - private List> myTasks = new ArrayList<>(); + private List myTasks = new ArrayList<>(); @Override public void migrate() { DriverTypeEnum.ConnectionProperties connectionProperties = getDriverType().newConnectionProperties(getConnectionUrl(), getUsername(), getPassword()); - for (BaseTask next : myTasks) { + for (BaseTask next : myTasks) { next.setDriverType(getDriverType()); next.setDryRun(isDryRun()); next.setNoColumnShrink(isNoColumnShrink()); @@ -71,7 +71,7 @@ public class TaskOnlyMigrator extends BaseMigrator { } @Override - public void addTasks(List> theMigrationTasks) { + public void addTasks(List theMigrationTasks) { myTasks.addAll(theMigrationTasks); } } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasks.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasks.java index 615ac93f51b..bf197f78138 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasks.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasks.java @@ -33,16 +33,16 @@ import java.util.Collection; import java.util.List; public class BaseMigrationTasks { - private Multimap> myTasks = MultimapBuilder.hashKeys().arrayListValues().build(); + private Multimap myTasks = MultimapBuilder.hashKeys().arrayListValues().build(); MigrationVersion lastVersion; @SuppressWarnings("unchecked") - public List> getTasks(@Nonnull T theFrom, @Nonnull T theTo) { + public List getTasks(@Nonnull T theFrom, @Nonnull T theTo) { Validate.notNull(theFrom); Validate.notNull(theTo); Validate.isTrue(theFrom.ordinal() < theTo.ordinal(), "From version must be lower than to version"); - List> retVal = new ArrayList<>(); + List retVal = new ArrayList<>(); for (Object nextVersion : EnumUtils.getEnumList(theFrom.getClass())) { if (((T) nextVersion).ordinal() <= theFrom.ordinal()) { continue; @@ -51,7 +51,7 @@ public class BaseMigrationTasks { continue; } - Collection> nextValues = myTasks.get((T) nextVersion); + Collection nextValues = myTasks.get((T) nextVersion); if (nextValues != null) { retVal.addAll(nextValues); } @@ -68,10 +68,10 @@ public class BaseMigrationTasks { return new Builder(theRelease.name(), sink); } - public List> getAllTasks(T[] theVersionEnumValues) { - List> retval = new ArrayList<>(); + public List getAllTasks(T[] theVersionEnumValues) { + List retval = new ArrayList<>(); for (T nextVersion : theVersionEnumValues) { - Collection> nextValues = myTasks.get(nextVersion); + Collection nextValues = myTasks.get(nextVersion); if (nextValues != null) { validate(nextValues); retval.addAll(nextValues); @@ -81,7 +81,7 @@ public class BaseMigrationTasks { return retval; } - void validate(Collection> theTasks) { + void validate(Collection theTasks) { for (BaseTask task: theTasks) { task.validateVersion(); String version = task.getFlywayVersion(); @@ -96,6 +96,6 @@ public class BaseMigrationTasks { } public interface IAcceptsTasks { - void addTask(BaseTask theTask); + void addTask(BaseTask theTask); } } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java index 5e122b98478..b732c5c8b2e 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java @@ -39,7 +39,7 @@ public class Builder { return new BuilderWithTableName(myRelease, mySink, theTableName); } - public void addTask(BaseTask theTask) { + public void addTask(BaseTask theTask) { mySink.addTask(theTask); } @@ -122,7 +122,7 @@ public class Builder { } @Override - public void addTask(BaseTask theTask) { + public void addTask(BaseTask theTask) { if (theTask instanceof AddColumnTask) { myTask.addAddColumnTask((AddColumnTask) theTask); } else { @@ -202,7 +202,7 @@ public class Builder { } @Override - public void addTask(BaseTask theTask) { + public void addTask(BaseTask theTask) { ((BaseTableTask) theTask).setTableName(myTableName); mySink.addTask(theTask); } diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java new file mode 100644 index 00000000000..fe18bd45fff --- /dev/null +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java @@ -0,0 +1,91 @@ +package ca.uhn.fhir.jpa.migrate; + +import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; +import ca.uhn.fhir.jpa.migrate.taskdef.DropIndexTask; +import org.jetbrains.annotations.NotNull; +import org.junit.Before; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; + +public class MigrationTaskSkipperTest { + public static final String RELEASE = "4_1_0"; + public static final String DATE_PREFIX = "20191214."; + private static final String VERSION_PREFIX = RELEASE + "." + DATE_PREFIX; + private List myTasks; + + @Before + public void before() { + myTasks = new ArrayList<>(); + myTasks.add(new DropIndexTask(RELEASE, DATE_PREFIX + 1)); + myTasks.add(new DropIndexTask(RELEASE, DATE_PREFIX + 2)); + myTasks.add(new DropIndexTask(RELEASE, DATE_PREFIX + 3)); + myTasks.add(new DropIndexTask(RELEASE, DATE_PREFIX + 4)); + myTasks.add(new DropIndexTask(RELEASE, DATE_PREFIX + 5)); + myTasks.add(new DropIndexTask(RELEASE, DATE_PREFIX + 6)); + } + + @Test + public void skipNull() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, null); + assertSkipped(myTasks); + } + + @Test + public void skipAll() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, makeSkipString(1, 2, 3, 4, 5, 6)); + assertSkipped(myTasks, 1, 2, 3, 4, 5, 6); + } + + @Test + public void skipOne() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, makeSkipString(4)); + assertSkipped(myTasks, 4); + } + + @Test + public void skipWeirdSpacing() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, " " + VERSION_PREFIX + 2 + " , " + VERSION_PREFIX + 3 + " "); + assertSkipped(myTasks, 2, 3); + } + + @Test + public void testDoubleComma() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, VERSION_PREFIX + 2 + ",," + VERSION_PREFIX + 3); + assertSkipped(myTasks, 2, 3); + } + + @Test + public void startComma() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, "," + VERSION_PREFIX + 2 + "," + VERSION_PREFIX + 3); + assertSkipped(myTasks, 2, 3); + } + + @Test + public void endComma() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, VERSION_PREFIX + 2 + "," + VERSION_PREFIX + 3 + ","); + assertSkipped(myTasks, 2, 3); + } + + private String makeSkipString(Integer... theVersions) { + return integersToVersions(theVersions).collect(Collectors.joining(",")); + } + + private void assertSkipped(List theTasks, Integer... theVersions) { + Set expectedVersions = integersToVersions(theVersions).collect(Collectors.toSet()); + Set taskVersions = theTasks.stream().filter(BaseTask::isDoNothing).map(BaseTask::getFlywayVersion).collect(Collectors.toSet()); + assertThat(taskVersions, equalTo(expectedVersions)); + } + + @NotNull + private Stream integersToVersions(Integer[] theVersions) { + return Stream.of(theVersions).map(s -> VERSION_PREFIX + s); + } +} diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java index 8d80b3d7be1..8ddf044a879 100644 --- a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/SchemaMigratorTest.java @@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.migrate; import ca.uhn.fhir.context.ConfigurationException; import ca.uhn.fhir.jpa.migrate.taskdef.AddTableRawSqlTask; +import ca.uhn.fhir.jpa.migrate.taskdef.BaseTask; import ca.uhn.fhir.jpa.migrate.taskdef.BaseTest; import com.google.common.collect.ImmutableList; import org.flywaydb.core.api.FlywayException; @@ -84,10 +85,11 @@ public class SchemaMigratorTest extends BaseTest { taskD.setTableName("SOMETABLE_D"); taskD.addSql(DriverTypeEnum.H2_EMBEDDED, "create table SOMETABLE_D (PID bigint not null, TEXTCOL varchar(255))"); - SchemaMigrator schemaMigrator = new SchemaMigrator(SchemaMigrator.HAPI_FHIR_MIGRATION_TABLENAME, getDataSource(), new Properties(), ImmutableList.of(taskA, taskB, taskC, taskD)); + ImmutableList taskList = ImmutableList.of(taskA, taskB, taskC, taskD); + MigrationTaskSkipper.setDoNothingOnSkippedTasks(taskList, "4_1_0.20191214.2, 4_1_0.20191214.4"); + SchemaMigrator schemaMigrator = new SchemaMigrator(SchemaMigrator.HAPI_FHIR_MIGRATION_TABLENAME, getDataSource(), new Properties(), taskList); schemaMigrator.setDriverType(DriverTypeEnum.H2_EMBEDDED); - schemaMigrator.setSkipVersions("4_1_0.20191214.2, 4_1_0.20191214.4"); schemaMigrator.migrate(); DriverTypeEnum.ConnectionProperties connectionProperties = DriverTypeEnum.H2_EMBEDDED.newConnectionProperties(getDataSource().getUrl(), getDataSource().getUsername(), getDataSource().getPassword()); diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/HashTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/HashTest.java index ffc5219726e..97ef8770d88 100644 --- a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/HashTest.java +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/HashTest.java @@ -33,7 +33,7 @@ public class HashTest { @Test public void testCheckAllHashes() { - List> tasks1 = new HapiFhirJpaMigrationTasks(Collections.emptySet()).getAllTasks(VersionEnum.values()); + List tasks1 = new HapiFhirJpaMigrationTasks(Collections.emptySet()).getAllTasks(VersionEnum.values()); Map hashesByVersion = new HashMap<>(); for (BaseTask task : tasks1) { String version = task.getFlywayVersion(); @@ -41,7 +41,7 @@ public class HashTest { hashesByVersion.put(version, task.hashCode()); } - List> tasks2 = new HapiFhirJpaMigrationTasks(Collections.emptySet()).getAllTasks(VersionEnum.values()); + List tasks2 = new HapiFhirJpaMigrationTasks(Collections.emptySet()).getAllTasks(VersionEnum.values()); for (BaseTask task : tasks2) { String version = task.getFlywayVersion(); int origHash = hashesByVersion.get(version); diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasksTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasksTest.java index 1e7e11e3e13..8b263688bed 100644 --- a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasksTest.java +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/tasks/api/BaseMigrationTasksTest.java @@ -17,7 +17,7 @@ public class BaseMigrationTasksTest { @Test public void testValidateCorrectOrder() { MyMigrationTasks migrationTasks = new MyMigrationTasks(); - List> tasks = new ArrayList<>(); + List tasks = new ArrayList<>(); tasks.add(new DropTableTask("1", "20191029.1")); tasks.add(new DropTableTask("1", "20191029.2")); migrationTasks.validate(tasks); @@ -26,7 +26,7 @@ public class BaseMigrationTasksTest { @Test public void testValidateVersionWrongOrder() { MyMigrationTasks migrationTasks = new MyMigrationTasks(); - List> tasks = new ArrayList<>(); + List tasks = new ArrayList<>(); tasks.add(new DropTableTask("1", "20191029.2")); tasks.add(new DropTableTask("1", "20191029.1")); try { @@ -40,7 +40,7 @@ public class BaseMigrationTasksTest { @Test public void testValidateSameVersion() { MyMigrationTasks migrationTasks = new MyMigrationTasks(); - List> tasks = new ArrayList<>(); + List tasks = new ArrayList<>(); tasks.add(new DropTableTask("1", "20191029.1")); tasks.add(new DropTableTask("1", "20191029.1")); try { @@ -54,7 +54,7 @@ public class BaseMigrationTasksTest { @Test public void testValidateWrongDateOrder() { MyMigrationTasks migrationTasks = new MyMigrationTasks(); - List> tasks = new ArrayList<>(); + List tasks = new ArrayList<>(); tasks.add(new DropTableTask("1", "20191029.1")); tasks.add(new DropTableTask("1", "20191028.1")); try { From 9f422efd7a71bbc3789859785d55de40541765d2 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Sat, 14 Dec 2019 21:02:31 -0500 Subject: [PATCH 4/5] handle case where version list is quoted --- .../jpa/migrate/MigrationTaskSkipper.java | 3 +++ .../jpa/migrate/MigrationTaskSkipperTest.java | 24 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java index 9ca06f3224d..3cf0053c76f 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipper.java @@ -21,6 +21,9 @@ public class MigrationTaskSkipper { } Set skippedVersionSet = Stream.of(theSkipVersions.split(",")) .map(String::trim) + // TODO KHS filter out all characters that aren't numbers, periods and underscores + .map(s -> s.replace("'", "")) + .map(s -> s.replace("\"", "")) .filter(StringUtils::isNotBlank) .collect(Collectors.toSet()); diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java index fe18bd45fff..b1d3a4db886 100644 --- a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/MigrationTaskSkipperTest.java @@ -50,6 +50,12 @@ public class MigrationTaskSkipperTest { assertSkipped(myTasks, 4); } + @Test + public void skipTwo() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, VERSION_PREFIX + 2 + "," + VERSION_PREFIX + 3); + assertSkipped(myTasks, 2, 3); + } + @Test public void skipWeirdSpacing() { MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, " " + VERSION_PREFIX + 2 + " , " + VERSION_PREFIX + 3 + " "); @@ -68,6 +74,24 @@ public class MigrationTaskSkipperTest { assertSkipped(myTasks, 2, 3); } + @Test + public void quoted() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, "\"" + VERSION_PREFIX + 2 + "," + VERSION_PREFIX + 3 + "\""); + assertSkipped(myTasks, 2, 3); + } + + @Test + public void allQuoted() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, "\"" + VERSION_PREFIX + 2 + "\",\"" + VERSION_PREFIX + 3 + "\""); + assertSkipped(myTasks, 2, 3); + } + + @Test + public void oneQuoted() { + MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, "\"" + VERSION_PREFIX + 2 + "\"" ); + assertSkipped(myTasks, 2); + } + @Test public void endComma() { MigrationTaskSkipper.setDoNothingOnSkippedTasks(myTasks, VERSION_PREFIX + 2 + "," + VERSION_PREFIX + 3 + ","); From e3922d5c017dd7edbe39ef5d121d3606e1f91ef0 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Sun, 15 Dec 2019 08:32:45 -0500 Subject: [PATCH 5/5] pre-review cleanup --- .../java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java index 648625c38a3..c3118f401fc 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java +++ b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseFlywayMigrateDatabaseCommand.java @@ -86,7 +86,7 @@ public abstract class BaseFlywayMigrateDatabaseCommand extends B addOptionalOption(retVal, null, DONT_USE_FLYWAY,false, "If this option is set, the migrator will not use FlywayDB for migration. This setting should only be used if you are trying to migrate a legacy database platform that is not supported by FlywayDB."); addOptionalOption(retVal, null, OUT_OF_ORDER_PERMITTED,false, "If this option is set, the migrator will permit migration tasks to be run out of order. It shouldn't be required in most cases, however may be the solution if you see the error message 'Detected resolved migration not applied to database'."); addOptionalOption(retVal, null, NO_COLUMN_SHRINK, false, "If this flag is set, the system will not attempt to reduce the length of columns. This is useful in environments with a lot of existing data, where shrinking a column can take a very long time."); - addOptionalOption(retVal, null, SKIP_VERSIONS, "Versions", "A comma separated list of schema versions to skip. E.g. 4_1_0.20191214.2,4_1_0.20191214.4\""); + addOptionalOption(retVal, null, SKIP_VERSIONS, "Versions", "A comma separated list of schema versions to skip. E.g. 4_1_0.20191214.2,4_1_0.20191214.4"); return retVal; }