From 5dcbed04384e87dc20abe630fb20a73b121861bc Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 17 Oct 2019 15:52:35 -0400 Subject: [PATCH] Fix foreign key handling when dropping tables --- .../ca/uhn/fhir/jpa/migrate/JdbcUtils.java | 45 +++++++++++-------- .../migrate/taskdef/DropForeignKeyTask.java | 32 ++++++++----- .../jpa/migrate/taskdef/DropIndexTask.java | 2 + .../jpa/migrate/taskdef/DropTableTask.java | 14 +++++- .../jpa/migrate/taskdef/DropTableTest.java | 16 +++++++ .../rest/server/method/IncludeParameter.java | 2 +- .../resources/vm/jpa_resource_provider.vm | 3 +- src/changes/changes.xml | 4 ++ 8 files changed, 84 insertions(+), 34 deletions(-) diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java index 14ba712aa2d..d9af234231e 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java @@ -42,6 +42,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.jdbc.core.ColumnMapRowMapper; +import javax.annotation.Nullable; import javax.sql.DataSource; import java.sql.*; import java.util.*; @@ -256,8 +257,9 @@ public class JdbcUtils { /** * Retrieve all index names */ - public static Set getForeignKeys(DriverTypeEnum.ConnectionProperties theConnectionProperties, String theTableName, String theForeignTable) throws SQLException { + public static Set getForeignKeys(DriverTypeEnum.ConnectionProperties theConnectionProperties, String theTableName, @Nullable String theForeignTable) throws SQLException { DataSource dataSource = Objects.requireNonNull(theConnectionProperties.getDataSource()); + try (Connection connection = dataSource.getConnection()) { return theConnectionProperties.getTxTemplate().execute(t -> { DatabaseMetaData metadata; @@ -265,27 +267,30 @@ public class JdbcUtils { metadata = connection.getMetaData(); String catalog = connection.getCatalog(); String schema = connection.getSchema(); - String parentTable = massageIdentifier(metadata, theTableName); - String foreignTable = massageIdentifier(metadata, theForeignTable); - ResultSet indexes = metadata.getCrossReference(catalog, schema, parentTable, catalog, schema, foreignTable); - Set columnNames = new HashSet<>(); - while (indexes.next()) { - String tableName = toUpperCase(indexes.getString("PKTABLE_NAME"), Locale.US); - if (!theTableName.equalsIgnoreCase(tableName)) { - continue; - } - tableName = toUpperCase(indexes.getString("FKTABLE_NAME"), Locale.US); - if (!theForeignTable.equalsIgnoreCase(tableName)) { - continue; - } - String fkName = indexes.getString("FK_NAME"); - fkName = toUpperCase(fkName, Locale.US); - columnNames.add(fkName); + List parentTables = new ArrayList<>(); + if (theTableName != null) { + parentTables.add(massageIdentifier(metadata, theTableName)); + } else { + // If no foreign table is specified, we'll try all of them + parentTables.addAll(JdbcUtils.getTableNames(theConnectionProperties)); } - return columnNames; + String foreignTable = massageIdentifier(metadata, theForeignTable); + + Set fkNames = new HashSet<>(); + for (String nextParentTable : parentTables) { + ResultSet indexes = metadata.getCrossReference(catalog, schema, nextParentTable, catalog, schema, foreignTable); + + while (indexes.next()) { + String fkName = indexes.getString("FK_NAME"); + fkName = toUpperCase(fkName, Locale.US); + fkNames.add(fkName); + } + } + + return fkNames; } catch (SQLException e) { throw new InternalErrorException(e); } @@ -486,7 +491,9 @@ public class JdbcUtils { private static String massageIdentifier(DatabaseMetaData theMetadata, String theCatalog) throws SQLException { String retVal = theCatalog; - if (theMetadata.storesLowerCaseIdentifiers()) { + if (theCatalog == null) { + return null; + } else if (theMetadata.storesLowerCaseIdentifiers()) { retVal = retVal.toLowerCase(); } else { retVal = retVal.toUpperCase(); diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java index 4b4c231814d..e74510e329f 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java @@ -20,12 +20,16 @@ package ca.uhn.fhir.jpa.migrate.taskdef; * #L% */ +import ca.uhn.fhir.jpa.migrate.DriverTypeEnum; import ca.uhn.fhir.jpa.migrate.JdbcUtils; import org.apache.commons.lang3.Validate; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nonnull; import java.sql.SQLException; +import java.util.ArrayList; +import java.util.List; import java.util.Set; import static org.apache.commons.lang3.StringUtils.isNotBlank; @@ -61,13 +65,22 @@ public class DropForeignKeyTask extends BaseTableTask { return; } - String sql = null; - String sql2 = null; - switch (getDriverType()) { + List sqls = generateSql(getTableName(), myConstraintName, getDriverType()); + + for (String next : sqls) { + executeSql(getTableName(), next); + } + + } + + @Nonnull + static List generateSql(String theTableName, String theConstraintName, DriverTypeEnum theDriverType) { + List sqls = new ArrayList<>(); + switch (theDriverType) { case MYSQL_5_7: // Lousy MYQL.... - sql = "alter table " + getTableName() + " drop constraint " + myConstraintName; - sql2 = "alter table " + getTableName() + " drop index " + myConstraintName; + sqls.add("alter table " + theTableName + " drop constraint " + theConstraintName); + sqls.add("alter table " + theTableName + " drop index " + theConstraintName); break; case MARIADB_10_1: case POSTGRES_9_4: @@ -75,17 +88,12 @@ public class DropForeignKeyTask extends BaseTableTask { case H2_EMBEDDED: case ORACLE_12C: case MSSQL_2012: - sql = "alter table " + getTableName() + " drop constraint " + myConstraintName; + sqls.add("alter table " + theTableName + " drop constraint " + theConstraintName); break; default: throw new IllegalStateException(); } - - executeSql(getTableName(), sql); - if (isNotBlank(sql2)) { - executeSql(getTableName(), sql2); - } - + return sqls; } } diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java index 3389b2bac0f..dd42aa1cb2d 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java @@ -84,6 +84,8 @@ public class DropIndexTask extends BaseTableTask { sql = "drop index " + theIndexName; break; case POSTGRES_9_4: + sql = "alter table " + theTableName + " drop constraint " + theIndexName + " cascade"; + break; case ORACLE_12C: case MSSQL_2012: sql = "alter table " + theTableName + " drop constraint " + theIndexName; diff --git a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTask.java b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTask.java index 0950e420de7..d628fc99799 100644 --- a/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTask.java +++ b/hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTask.java @@ -21,11 +21,11 @@ package ca.uhn.fhir.jpa.migrate.taskdef; */ import ca.uhn.fhir.jpa.migrate.JdbcUtils; -import org.apache.commons.lang3.Validate; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.sql.SQLException; +import java.util.List; import java.util.Set; public class DropTableTask extends BaseTableTask { @@ -39,7 +39,19 @@ public class DropTableTask extends BaseTableTask { return; } + Set foreignKeys = JdbcUtils.getForeignKeys(getConnectionProperties(), null, getTableName()); + ourLog.info("Table {} has the following foreign keys: {}", getTableName(), foreignKeys); + Set indexNames = JdbcUtils.getIndexNames(getConnectionProperties(), getTableName()); + ourLog.info("Table {} has the following indexes: {}", getTableName(), indexNames); + + for (String next : foreignKeys) { + List sql = DropForeignKeyTask.generateSql(getTableName(), next, getDriverType()); + for (String nextSql : sql) { + executeSql(getTableName(), nextSql); + } + } + for (String nextIndex : indexNames) { String sql = DropIndexTask.createDropIndexSql(getConnectionProperties(), getTableName(), nextIndex, getDriverType()); ourLog.info("Dropping index {} on table {} in preparation for table delete", nextIndex, getTableName()); diff --git a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTest.java b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTest.java index b2d9eb57326..a033d359d4d 100644 --- a/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTest.java +++ b/hapi-fhir-jpaserver-migrate/src/test/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTest.java @@ -29,6 +29,22 @@ public class DropTableTest extends BaseTest { assertThat(JdbcUtils.getTableNames(getConnectionProperties()), not(hasItems("SOMETABLE"))); } + @Test + public void testDropTableWithForeignKey() throws SQLException { + executeSql("create table FOREIGNTABLE (PID bigint not null, TEXTCOL varchar(255), primary key (PID))"); + executeSql("create table SOMETABLE (PID bigint not null, REMOTEPID bigint not null, primary key (PID))"); + executeSql("alter table SOMETABLE add constraint FK_MYFK foreign key (REMOTEPID) references FOREIGNTABLE;"); + + DropTableTask task = new DropTableTask(); + task.setTableName("SOMETABLE"); + getMigrator().addTask(task); + + assertThat(JdbcUtils.getTableNames(getConnectionProperties()), (hasItems("SOMETABLE"))); + + getMigrator().migrate(); + + assertThat(JdbcUtils.getTableNames(getConnectionProperties()), not(hasItems("SOMETABLE"))); + } @Test public void testDropNonExistingTable() throws SQLException { diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/IncludeParameter.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/IncludeParameter.java index 2c9c6de0767..e7dea5bc20d 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/IncludeParameter.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/IncludeParameter.java @@ -49,7 +49,7 @@ class IncludeParameter extends BaseQueryParameter { myInstantiableCollectionType = theInstantiableCollectionType; myReverse = theAnnotation.reverse(); if (theAnnotation.allow().length > 0) { - myAllow = new HashSet(); + myAllow = new HashSet<>(); for (String next : theAnnotation.allow()) { if (next != null) { myAllow.add(next); diff --git a/hapi-tinder-plugin/src/main/resources/vm/jpa_resource_provider.vm b/hapi-tinder-plugin/src/main/resources/vm/jpa_resource_provider.vm index b4bf18b0fb7..9540cc488b2 100644 --- a/hapi-tinder-plugin/src/main/resources/vm/jpa_resource_provider.vm +++ b/hapi-tinder-plugin/src/main/resources/vm/jpa_resource_provider.vm @@ -117,8 +117,9 @@ public class ${className}ResourceProvider extends @IncludeParam(allow= { #foreach ( $include in $includes ) - "${include.path}" #{if}($foreach.hasNext), #{end} + "${include.path}", #end + "*" }) Set theIncludes, diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 17854b444c2..e7d7e26d0a4 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -408,6 +408,10 @@ An unintended dependency from hapi-fhir-base on Jetty was introduced in HAPI FHIR 4.0.0. This has been removed. + + The JPA migrator tool was not able to correctly drop tables containing foreign key references + in some cases. This has been corrected. +