From 34da5c1b34e396397cc04f2f9b945be57f7cb8b6 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 2 Jul 2024 09:22:55 -0400 Subject: [PATCH] Ensure DropPrimaryKeyTask respects dry-run (#6062) * Remove code to SQL query from BaseTask. Move code to DropPrimaryKeyTask and gate it for NOT dry-run. * Update hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropPrimaryKeyTask.java Commit code review suggestion Co-authored-by: Michael Buckley * Code review feedback. * Code review feedback. * Remove isTransactional(). --------- Co-authored-by: Michael Buckley --- ...rimary-key-task-doesnt-handle-dry-run.yaml | 6 +++ .../jpa/embedded/HapiSchemaMigrationTest.java | 1 - .../fhir/jpa/migrate/taskdef/BaseTask.java | 39 ------------------- .../migrate/taskdef/DropPrimaryKeyTask.java | 19 +-------- 4 files changed, 8 insertions(+), 57 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6061-drop-primary-key-task-doesnt-handle-dry-run.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6061-drop-primary-key-task-doesnt-handle-dry-run.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6061-drop-primary-key-task-doesnt-handle-dry-run.yaml new file mode 100644 index 00000000000..6ac8dc86452 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6061-drop-primary-key-task-doesnt-handle-dry-run.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 6061 +title: "The latest migration for HFJ_RES_SEARCH_URL introduced in [6033](https://github.com/hapifhir/hapi-fhir/issues/6033) + does not fully respect dry-run. + This has been fixed." diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/embedded/HapiSchemaMigrationTest.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/embedded/HapiSchemaMigrationTest.java index 2bd04e8eb4c..b63ffc46b60 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/embedded/HapiSchemaMigrationTest.java +++ b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/embedded/HapiSchemaMigrationTest.java @@ -26,7 +26,6 @@ import javax.sql.DataSource; import java.sql.Connection; import java.sql.DatabaseMetaData; import java.sql.ResultSet; -import java.sql.ResultSetMetaData; import java.sql.SQLException; import java.sql.Types; import java.util.ArrayList; diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/BaseTask.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/BaseTask.java index 776ee801a4e..8f1b1f080c1 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/BaseTask.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/BaseTask.java @@ -25,7 +25,6 @@ import ca.uhn.fhir.jpa.migrate.HapiMigrationException; import ca.uhn.fhir.jpa.migrate.tasks.api.TaskFlagEnum; import ca.uhn.fhir.system.HapiSystemProperties; import jakarta.annotation.Nonnull; -import jakarta.annotation.Nullable; import org.apache.commons.lang3.Validate; import org.apache.commons.lang3.builder.EqualsBuilder; import org.apache.commons.lang3.builder.HashCodeBuilder; @@ -37,7 +36,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.dao.DataAccessException; import org.springframework.jdbc.core.JdbcTemplate; -import org.springframework.jdbc.core.ResultSetExtractor; import org.springframework.transaction.support.TransactionTemplate; import java.sql.SQLException; @@ -176,17 +174,6 @@ public abstract class BaseTask { captureExecutedStatement(theTableName, theSql, theArguments); } - protected T executeSqlWithResult( - @Language("SQL") String theSql, ResultSetExtractor theResultSetExtractor, Object... theArguments) { - if (myTransactional) { - return getConnectionProperties() - .getTxTemplate() - .execute(t -> doExecuteSqlWithResult(theSql, theResultSetExtractor, theArguments)); - } - - return doExecuteSqlWithResult(theSql, theResultSetExtractor, theArguments); - } - protected void executeSqlListInTransaction(String theTableName, List theSqlStatements) { if (!isDryRun()) { Integer changes; @@ -232,32 +219,6 @@ public abstract class BaseTask { } } - @Nullable - private T doExecuteSqlWithResult( - @Language("SQL") String theSql, ResultSetExtractor theResultSetExtractor, Object... theArguments) { - final JdbcTemplate jdbcTemplate = getConnectionProperties().newJdbcTemplate(); - // 0 means no timeout -- we use this for index rebuilds that may take time. - jdbcTemplate.setQueryTimeout(0); - try { - T result = jdbcTemplate.query(theSql, theResultSetExtractor, theArguments); - if (!HapiSystemProperties.isUnitTestModeEnabled()) { - logInfo(ourLog, "SQL \"{}\" returned result {}", theSql, result); - } - return result; - } catch (DataAccessException e) { - if (myFlags.contains(TaskFlagEnum.FAILURE_ALLOWED)) { - ourLog.info( - "Task {} did not exit successfully on doExecuteSqlWithResult(), but task is allowed to fail", - getMigrationVersion()); - ourLog.debug("Error was: {}", e.getMessage(), e); - return null; - } else { - throw new HapiMigrationException( - Msg.code(2532) + "Failed during task " + getMigrationVersion() + ": " + e, e); - } - } - } - protected void captureExecutedStatement( String theTableName, @Language("SQL") String theSql, Object... theArguments) { myExecutedStatements.add(new ExecutedStatement(mySchemaVersion, theTableName, theSql, theArguments)); diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropPrimaryKeyTask.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropPrimaryKeyTask.java index ad078fc7aaa..1437a537308 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropPrimaryKeyTask.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropPrimaryKeyTask.java @@ -26,7 +26,6 @@ import jakarta.annotation.Nullable; import org.intellij.lang.annotations.Language; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.springframework.jdbc.core.ResultSetExtractor; import java.sql.SQLException; @@ -51,28 +50,14 @@ public class DropPrimaryKeyTask extends BaseTableTask { private String generateSql() { ourLog.debug("DropPrimaryKeyTask.generateSql()"); - final ResultSetExtractor resultSetExtractor = rs -> { - if (rs.next()) { - final String singleResult = rs.getString(1); - - if (rs.next()) { - throw new IllegalArgumentException(Msg.code(2533) - + "Expecting only a single result for the table primary but got multiple for task: " - + getMigrationVersion()); - } - - return singleResult; - } - return null; - }; - @Nullable @Language("SQL") final String primaryKeyNameSql = generatePrimaryKeyNameSql(); @Nullable final String primaryKeyName = primaryKeyNameSql != null - ? executeSqlWithResult(primaryKeyNameSql, resultSetExtractor, getTableNameWithDatabaseExpectedCase()) + ? newJdbcTemplate() + .queryForObject(primaryKeyNameSql, String.class, getTableNameWithDatabaseExpectedCase()) : null; ourLog.debug("primaryKeyName: {} for driver: {}", primaryKeyName, getDriverType());