Add no-shrink-columns option to migrator (#1500)

* Add no-shrink-columns option to migrator

* Address review comments

hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseMigrateDatabaseCommand.java

@@ -78,6 +78,7 @@ public abstract class BaseMigrateDatabaseCommand<T extends Enum> extends BaseCom
 		addRequiredOption(retVal, "t", "to", "Version", "The database schema version to migrate TO");
 		addRequiredOption(retVal, "d", "driver", "Driver", "The database driver to use (Options are " + driverOptions() + ")");
 		addOptionalOption(retVal, "x", "flags", "Flags", "A comma-separated list of any specific migration flags (these flags are version specific, see migrator documentation for details)");
+		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.");
 
 		return retVal;
 	}
@@ -106,6 +107,7 @@ public abstract class BaseMigrateDatabaseCommand<T extends Enum> extends BaseCom
 		validateVersionSupported(to);
 
 		boolean dryRun = theCommandLine.hasOption("r");
+		boolean noColumnShrink = theCommandLine.hasOption("no-column-shrink");
 
 		String flags = theCommandLine.getOptionValue("x");
 		myFlags = Arrays.stream(defaultString(flags).split(","))
@@ -119,6 +121,7 @@ public abstract class BaseMigrateDatabaseCommand<T extends Enum> extends BaseCom
 		migrator.setUsername(username);
 		migrator.setPassword(password);
 		migrator.setDryRun(dryRun);
+		migrator.setNoColumnShrink(noColumnShrink);
 		addTasks(migrator, from, to);
 
 		migrator.migrate();

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java

@@ -22,6 +22,9 @@ package ca.uhn.fhir.jpa.migrate;
 
 import ca.uhn.fhir.jpa.migrate.taskdef.BaseTableColumnTypeTask;
 import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.apache.commons.lang3.builder.ToStringBuilder;
 import org.hibernate.boot.model.naming.Identifier;
 import org.hibernate.dialect.Dialect;
 import org.hibernate.engine.jdbc.dialect.internal.StandardDialectResolver;
@@ -48,6 +51,73 @@ import static org.thymeleaf.util.StringUtils.toUpperCase;
 public class JdbcUtils {
 	private static final Logger ourLog = LoggerFactory.getLogger(JdbcUtils.class);
 
+	public static class ColumnType {
+		private final BaseTableColumnTypeTask.ColumnTypeEnum myColumnTypeEnum;
+		private final Long myLength;
+
+		public ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum theColumnType, Long theLength) {
+			myColumnTypeEnum = theColumnType;
+			myLength = theLength;
+		}
+
+		public ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum theColumnType, int theLength) {
+			this(theColumnType, (long) theLength);
+		}
+
+		public ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum theColumnType) {
+			this(theColumnType, null);
+		}
+
+		@Override
+		public boolean equals(Object theO) {
+			if (this == theO) {
+				return true;
+			}
+
+			if (theO == null || getClass() != theO.getClass()) {
+				return false;
+			}
+
+			ColumnType that = (ColumnType) theO;
+
+			return new EqualsBuilder()
+				.append(myColumnTypeEnum, that.myColumnTypeEnum)
+				.append(myLength, that.myLength)
+				.isEquals();
+		}
+
+		@Override
+		public int hashCode() {
+			return new HashCodeBuilder(17, 37)
+				.append(myColumnTypeEnum)
+				.append(myLength)
+				.toHashCode();
+		}
+
+		@Override
+		public String toString() {
+			ToStringBuilder b = new ToStringBuilder(this);
+			b.append("type", myColumnTypeEnum);
+			if (myLength != null) {
+				b.append("length", myLength);
+			}
+			return b.toString();
+		}
+
+		public BaseTableColumnTypeTask.ColumnTypeEnum getColumnTypeEnum() {
+			return myColumnTypeEnum;
+		}
+
+		public Long getLength() {
+			return myLength;
+		}
+
+		public boolean equals(BaseTableColumnTypeTask.ColumnTypeEnum theColumnType, Long theColumnLength) {
+			return myColumnTypeEnum == theColumnType && (myLength == null || myLength.equals(theColumnLength));
+		}
+
+	}
+
 	/**
 	 * Retrieve all index names
 	 */
@@ -127,7 +197,7 @@ public class JdbcUtils {
 	/**
 	 * Retrieve all index names
 	 */
-	public static String getColumnType(DriverTypeEnum.ConnectionProperties theConnectionProperties, String theTableName, String theColumnName) throws SQLException {
+	public static ColumnType getColumnType(DriverTypeEnum.ConnectionProperties theConnectionProperties, String theTableName, String theColumnName) throws SQLException {
 		DataSource dataSource = Objects.requireNonNull(theConnectionProperties.getDataSource());
 		try (Connection connection = dataSource.getConnection()) {
 			return theConnectionProperties.getTxTemplate().execute(t -> {
@@ -153,18 +223,18 @@ public class JdbcUtils {
 						Long length = indexes.getLong("COLUMN_SIZE");
 						switch (dataType) {
 							case Types.VARCHAR:
-								return BaseTableColumnTypeTask.ColumnTypeEnum.STRING.getDescriptor(length);
+								return new ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.STRING, length);
 							case Types.NUMERIC:
 							case Types.BIGINT:
 							case Types.DECIMAL:
-								return BaseTableColumnTypeTask.ColumnTypeEnum.LONG.getDescriptor(null);
+								return new ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.LONG, length);
 							case Types.INTEGER:
-								return BaseTableColumnTypeTask.ColumnTypeEnum.INT.getDescriptor(null);
+								return new ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.INT, length);
 							case Types.TIMESTAMP:
 							case Types.TIMESTAMP_WITH_TIMEZONE:
-								return BaseTableColumnTypeTask.ColumnTypeEnum.DATE_TIMESTAMP.getDescriptor(null);
+								return new ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.DATE_TIMESTAMP, length);
 							case Types.BLOB:
-								return BaseTableColumnTypeTask.ColumnTypeEnum.BLOB.getDescriptor(null);
+								return new ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.BLOB, length);
 							default:
 								throw new IllegalArgumentException("Don't know how to handle datatype " + dataType + " for column " + theColumnName + " on table " + theTableName);
 						}

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/Migrator.java

@@ -44,6 +44,7 @@ public class Migrator {
 	private int myChangesCount;
 	private boolean myDryRun;
 	private List<BaseTask.ExecutedStatement> myExecutedStatements = new ArrayList<>();
+	private boolean myNoColumnShrink;
 
 	public int getChangesCount() {
 		return myChangesCount;
@@ -82,6 +83,7 @@ public class Migrator {
 				next.setDriverType(myDriverType);
 				next.setConnectionProperties(myConnectionProperties);
 				next.setDryRun(myDryRun);
+				next.setNoColumnShrink(myNoColumnShrink);
 				try {
 					next.execute();
 				} catch (SQLException e) {
@@ -126,4 +128,8 @@ public class Migrator {
 	public void addTasks(List<BaseTask<?>> theTasks) {
 		theTasks.forEach(this::addTask);
 	}
+
+	public void setNoColumnShrink(boolean theNoColumnShrink) {
+		myNoColumnShrink = theNoColumnShrink;
+	}
 }

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/ArbitrarySqlTask.java

@@ -71,7 +71,7 @@ public class ArbitrarySqlTask extends BaseTask<ArbitrarySqlTask> {
 		}
 
 		for (TableAndColumn next : myConditionalOnExistenceOf) {
-			String columnType = JdbcUtils.getColumnType(getConnectionProperties(), next.getTable(), next.getColumn());
+			JdbcUtils.ColumnType columnType = JdbcUtils.getColumnType(getConnectionProperties(), next.getTable(), next.getColumn());
 			if (columnType == null) {
 				ourLog.info("Table {} does not have column {} - No action performed", next.getTable(), next.getColumn());
 				return;

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/BaseTableColumnTypeTask.java

@@ -164,74 +164,23 @@ public abstract class BaseTableColumnTypeTask<T extends BaseTableTask> extends B
 		return myColumnLength;
 	}
 
-	public BaseTableColumnTypeTask<T> setColumnLength(int theColumnLength) {
+	public BaseTableColumnTypeTask<T> setColumnLength(long theColumnLength) {
-		myColumnLength = (long) theColumnLength;
+		myColumnLength = theColumnLength;
 		return this;
 	}
 
 
 	public enum ColumnTypeEnum {
 
-		LONG {
+		LONG,
-			@Override
+		STRING,
-			public String getDescriptor(Long theColumnLength) {
+		DATE_TIMESTAMP,
-				Assert.isTrue(theColumnLength == null, "Must not supply a column length");
+		BOOLEAN,
-				return "bigint";
+		FLOAT,
-			}
+		INT,
-		},
+		BLOB,
-		STRING {
+		CLOB
-			@Override
+		;
-			public String getDescriptor(Long theColumnLength) {
-				Assert.isTrue(theColumnLength != null, "Must supply a column length");
-				return "varchar(" + theColumnLength + ")";
-			}
-		},
-		DATE_TIMESTAMP {
-			@Override
-			public String getDescriptor(Long theColumnLength) {
-				Assert.isTrue(theColumnLength == null, "Must not supply a column length");
-				return "timestamp";
-			}
-		},
-		BOOLEAN {
-			@Override
-			public String getDescriptor(Long theColumnLength) {
-				Assert.isTrue(theColumnLength == null, "Must not supply a column length");
-				return "boolean";
-			}
-		},
-		FLOAT {
-			@Override
-			public String getDescriptor(Long theColumnLength) {
-				Assert.isTrue(theColumnLength == null, "Must not supply a column length");
-				return "float";
-			}
-		},
-		INT {
-			@Override
-			public String getDescriptor(Long theColumnLength) {
-				Assert.isTrue(theColumnLength == null, "Must not supply a column length");
-				return "int";
-			}
-		},
-
-		BLOB {
-			@Override
-			public String getDescriptor(Long theColumnLength) {
-				Assert.isTrue(theColumnLength == null, "Must not supply a column length");
-				return "blob";
-			}
-		},
-
-		CLOB {
-			@Override
-			public String getDescriptor(Long theColumnLength) {
-				Assert.isTrue(theColumnLength == null, "Must not supply a column length");
-				return "clob";
-			}
-		};
-
-		public abstract String getDescriptor(Long theColumnLength);
 
 	}
 

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/BaseTask.java

@@ -42,6 +42,15 @@ public abstract class BaseTask<T extends BaseTask> {
 	private int myChangesCount;
 	private boolean myDryRun;
 	private List<ExecutedStatement> myExecutedStatements = new ArrayList<>();
+	private boolean myNoColumnShrink;
+
+	public boolean isNoColumnShrink() {
+		return myNoColumnShrink;
+	}
+
+	public void setNoColumnShrink(boolean theNoColumnShrink) {
+		myNoColumnShrink = theNoColumnShrink;
+	}
 
 	public boolean isDryRun() {
 		return myDryRun;

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/ModifyColumnTask.java

@@ -36,7 +36,7 @@ public class ModifyColumnTask extends BaseTableColumnTypeTask<ModifyColumnTask>
 	@Override
 	public void execute() throws SQLException {
 
-		String existingType;
+		JdbcUtils.ColumnType existingType;
 		boolean nullable;
 
 		Set<String> columnNames = JdbcUtils.getColumnNames(getConnectionProperties(), getTableName());
@@ -52,11 +52,17 @@ public class ModifyColumnTask extends BaseTableColumnTypeTask<ModifyColumnTask>
 			throw new InternalErrorException(e);
 		}
 
-		String wantedType = getColumnType().getDescriptor(getColumnLength());
+		if (isNoColumnShrink()) {
-		boolean alreadyOfCorrectType = existingType.equals(wantedType);
+			long existingLength = existingType.getLength() != null ? existingType.getLength() : 0;
+			if (existingLength > getColumnLength()) {
+				setColumnLength(existingLength);
+			}
+		}
+
+		boolean alreadyOfCorrectType = existingType.equals(getColumnType(), getColumnLength());
 		boolean alreadyCorrectNullable = isNullable() == nullable;
 		if (alreadyOfCorrectType && alreadyCorrectNullable) {
-			ourLog.info("Column {} on table {} is already of type {} and has nullable {} - No action performed", getColumnName(), getTableName(), wantedType, nullable);
+			ourLog.info("Column {} on table {} is already of type {} and has nullable {} - No action performed", getColumnName(), getTableName(), existingType, nullable);
 			return;
 		}
 

hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/AddColumnTest.java

@@ -40,8 +40,8 @@ public class AddColumnTest extends BaseTest {
 
 		getMigrator().migrate();
 
-		String type = JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "newcolint");
+		JdbcUtils.ColumnType type = JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "newcolint");
-		assertEquals(BaseTableColumnTypeTask.ColumnTypeEnum.INT.getDescriptor(null), type);
+		assertEquals(BaseTableColumnTypeTask.ColumnTypeEnum.INT, type.getColumnTypeEnum());
 	}
 
 	@Test

hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/ModifyColumnTest.java

@@ -25,7 +25,32 @@ public class ModifyColumnTest extends BaseTest {
 
 		getMigrator().migrate();
 
-		assertEquals("varchar(300)", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.STRING, 300), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
+		assertEquals(1, task.getExecutedStatements().size());
+
+		// Make sure additional migrations don't crash
+		getMigrator().migrate();
+		getMigrator().migrate();
+
+	}
+
+	@Test
+	public void testNoShrink_SameNullable() throws SQLException {
+		executeSql("create table SOMETABLE (PID bigint not null, TEXTCOL varchar(255), newcol bigint)");
+
+		ModifyColumnTask task = new ModifyColumnTask();
+		task.setTableName("SOMETABLE");
+		task.setColumnName("TEXTCOL");
+		task.setColumnType(AddColumnTask.ColumnTypeEnum.STRING);
+		task.setNullable(true);
+		task.setColumnLength(200);
+
+		getMigrator().setNoColumnShrink(true);
+		getMigrator().addTask(task);
+		getMigrator().migrate();
+
+		assertEquals(0, task.getExecutedStatements().size());
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.STRING, 255), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
 
 		// Make sure additional migrations don't crash
 		getMigrator().migrate();
@@ -38,8 +63,8 @@ public class ModifyColumnTest extends BaseTest {
 		executeSql("create table SOMETABLE (PID bigint not null, TEXTCOL varchar(255) not null)");
 		assertFalse(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "PID"));
 		assertFalse(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
-		assertEquals("bigint", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.LONG, 19), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
-		assertEquals("varchar(255)", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.STRING, 255), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
 
 		// PID
 		ModifyColumnTask task = new ModifyColumnTask();
@@ -63,8 +88,8 @@ public class ModifyColumnTest extends BaseTest {
 
 		assertTrue(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "PID"));
 		assertTrue(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
-		assertEquals("bigint", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.LONG, 19), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
-		assertEquals("varchar(255)", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.STRING, 255), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
 
 		// Make sure additional migrations don't crash
 		getMigrator().migrate();
@@ -78,8 +103,8 @@ public class ModifyColumnTest extends BaseTest {
 		executeSql("create table SOMETABLE (PID bigint, TEXTCOL varchar(255))");
 		assertTrue(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "PID"));
 		assertTrue(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
-		assertEquals("bigint", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.LONG, 19), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
-		assertEquals("varchar(255)", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.STRING, 255), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
 
 		// PID
 		ModifyColumnTask task = new ModifyColumnTask();
@@ -103,8 +128,8 @@ public class ModifyColumnTest extends BaseTest {
 
 		assertFalse(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "PID"));
 		assertFalse(JdbcUtils.isColumnNullable(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
-		assertEquals("bigint", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.LONG, 19), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "PID"));
-		assertEquals("varchar(255)", JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
+		assertEquals(new JdbcUtils.ColumnType(BaseTableColumnTypeTask.ColumnTypeEnum.STRING, 255), JdbcUtils.getColumnType(getConnectionProperties(), "SOMETABLE", "TEXTCOL"));
 
 		// Make sure additional migrations don't crash
 		getMigrator().migrate();

src/changes/changes.xml

@@ -193,6 +193,10 @@
 				resulted in some ValueSets with duplicate codes. This has been corrected by specifying a path with each
 				filename.
 			</action>
+			<action type="add">
+				A new flag has been added to the JPA migrator tool that causes the migrator to not try to reduce the length
+				of existing columns in the schema.
+			</action>
 			<action type="fix" issue="1483">
 				Some resource IDs and URLs for LOINC ValueSets and ConceptMaps were inconsistently populated by the
 				terminology uploader. This has been corrected.