honeymoose
/
hapi-fhir
mirror of https://github.com/hapifhir/hapi-fhir.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Enhance migration for MSSQL to change the collation for HFJ_RESOURCE.FHIR_ID to case sensitive (#6135) 

* MSSQL:  Migrate HFJ_RESOURCE.FHIR_ID to new collation:  SQL_Latin1_General_CP1_CS_AS

* Spotless.

* Enhance test.  Fix case in ResourceSearchView to defend against future migration to case insensitive collation.

* Remove TODOs.  Add comment to ResourceSearchView explaining why all columns are uppercase.  Changelog.

* Update hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6146-mssql-hfj-resource-fhir-id-colllation.yaml

Code reviewer suggestion

Co-authored-by: Michael Buckley <michaelabuckley@gmail.com>

* Code review fixes:  Make changes conditional on the collation including _CI_, otherwise, leave it alone.

---------

Co-authored-by: Michael Buckley <michaelabuckley@gmail.com>
This commit is contained in:
Luke deGruchy 2024-07-25 10:22:54 -04:00 committed by GitHub
parent 33b766c658
commit a70d28acae
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 146 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6146-mssql-hfj-resource-fhir-id-colllation.yaml Normal file
View File

@ -0,0 +1,10 @@
---
type: fix
issue: 6146
jira: SMILE-8191
title: "Previously, on MSSQL, two resources with IDs that are identical except for case
(ex: Patient1 vs. patient1) would be considered to have the same ID because the database collation is
case insensitive (SQL_Latin1_General_CP1_CI_AS). Among other things, this would manifest
itself when trying to delete and re-create one of the resources.
This has been fixed with a migration step that makes the collation on the resource ID case sensitive
(SQL_Latin1_General_CP1_CS_AS)."

32
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/ResourceSearchView.java
View File

@ -46,27 +46,29 @@ import java.util.Date;
@SuppressWarnings("SqlDialectInspection")
@Entity
@Immutable
@Subselect("SELECT h.pid as pid, "
+ " r.res_id as res_id, "
+ " h.res_type as res_type, "
+ " h.res_version as res_version, "
// Ideally, all tables and columns should be in UPPERCASE if we ever choose to use a case-sensitive collation for MSSQL
// and there's a risk that queries on lowercase database objects fail.
@Subselect("SELECT h.PID as PID, "
+ " r.RES_ID as RES_ID, "
+ " h.RES_TYPE as RES_TYPE, "
+ " h.RES_VERSION as RES_VERSION, "
// FHIR version
+ " h.res_ver as res_ver, "
+ " h.RES_VER as RES_VER, "
// resource version
+ " h.has_tags as has_tags, "
+ " h.res_deleted_at as res_deleted_at, "
+ " h.res_published as res_published, "
+ " h.res_updated as res_updated, "
+ " h.res_text as res_text, "
+ " h.res_text_vc as res_text_vc, "
+ " h.res_encoding as res_encoding, "
+ " h.HAS_TAGS as HAS_TAGS, "
+ " h.RES_DELETED_AT as RES_DELETED_AT, "
+ " h.RES_PUBLISHED as RES_PUBLISHED, "
+ " h.RES_UPDATED as RES_UPDATED, "
+ " h.RES_TEXT as RES_TEXT, "
+ " h.RES_TEXT_VC as RES_TEXT_VC, "
+ " h.RES_ENCODING as RES_ENCODING, "
+ " h.PARTITION_ID as PARTITION_ID, "
+ " p.SOURCE_URI as PROV_SOURCE_URI,"
+ " p.REQUEST_ID as PROV_REQUEST_ID,"
+ " r.fhir_id as FHIR_ID "
+ " r.FHIR_ID as FHIR_ID "
+ "FROM HFJ_RESOURCE r "
+ " INNER JOIN HFJ_RES_VER h ON r.res_id = h.res_id and r.res_ver = h.res_ver"
+ " LEFT OUTER JOIN HFJ_RES_VER_PROV p ON p.res_ver_pid = h.pid ")
+ " INNER JOIN HFJ_RES_VER h ON r.RES_ID = h.RES_ID and r.RES_VER = h.RES_VER"
+ " LEFT OUTER JOIN HFJ_RES_VER_PROV p ON p.RES_VER_PID = h.PID ")
public class ResourceSearchView implements IBaseResourceEntity, Serializable {
private static final long serialVersionUID = 1L;

49
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java
View File

@ -49,6 +49,7 @@ import ca.uhn.fhir.jpa.model.entity.StorageSettings;
import ca.uhn.fhir.util.ClasspathUtil;
import ca.uhn.fhir.util.VersionEnum;
import org.apache.commons.lang3.StringUtils;
import org.intellij.lang.annotations.Language;
import java.util.Arrays;
import java.util.HashMap;
@ -473,6 +474,54 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks<VersionEnum> {
.modifyColumn("20240722.1", Search.SEARCH_UUID)
.nullable()
.withType(ColumnTypeEnum.STRING, 48);
{
final Builder.BuilderWithTableName hfjResource = version.onTable("HFJ_RESOURCE");
@Language(("SQL"))
final String onlyIfSql = "SELECT CASE CHARINDEX('_CI_', COLLATION_NAME) WHEN 0 THEN 0 ELSE 1 END "
+ "FROM INFORMATION_SCHEMA.COLUMNS "
+ "WHERE TABLE_SCHEMA = SCHEMA_NAME() "
+ "AND TABLE_NAME = 'HFJ_RESOURCE' "
+ "AND COLUMN_NAME = 'FHIR_ID' ";
final String onlyfIReason =
"Skipping change to HFJ_RESOURCE.FHIR_ID collation to SQL_Latin1_General_CP1_CS_AS because it is already using it";
hfjResource
.dropIndex("20240724.10", "IDX_RES_FHIR_ID")
.onlyAppliesToPlatforms(DriverTypeEnum.MSSQL_2012)
.onlyIf(onlyIfSql, onlyfIReason);
hfjResource
.dropIndex("20240724.20", "IDX_RES_TYPE_FHIR_ID")
.onlyAppliesToPlatforms(DriverTypeEnum.MSSQL_2012)
.onlyIf(onlyIfSql, onlyfIReason);
version.executeRawSql(
"20240724.30",
"ALTER TABLE HFJ_RESOURCE ALTER COLUMN FHIR_ID varchar(64) COLLATE SQL_Latin1_General_CP1_CS_AS")
.onlyAppliesToPlatforms(DriverTypeEnum.MSSQL_2012)
.onlyIf(onlyIfSql, onlyfIReason);
hfjResource
.addIndex("20240724.40", "IDX_RES_FHIR_ID")
.unique(false)
.online(true)
.withColumns("FHIR_ID")
.onlyAppliesToPlatforms(DriverTypeEnum.MSSQL_2012)
.onlyIf(onlyIfSql, onlyfIReason);
hfjResource
.addIndex("20240724.50", "IDX_RES_TYPE_FHIR_ID")
.unique(true)
.online(true)
// include res_id and our deleted flag so we can satisfy Observation?_sort=_id from the index on
// platforms that support it.
.includeColumns("RES_ID, RES_DELETED_AT")
.withColumns("RES_TYPE", "FHIR_ID")
.onlyAppliesToPlatforms(DriverTypeEnum.MSSQL_2012)
.onlyIf(onlyIfSql, onlyfIReason);
}
}
protected void init720() {

74
hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/embedded/HapiSchemaMigrationTest.java
View File

@ -13,6 +13,7 @@ import ca.uhn.fhir.util.VersionEnum;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import org.apache.commons.dbcp2.BasicDataSource;
import org.intellij.lang.annotations.Language;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
@ -25,7 +26,9 @@ import org.springframework.jdbc.core.JdbcTemplate;
import javax.sql.DataSource;
import java.sql.Connection;
import java.sql.DatabaseMetaData;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.sql.Types;
import java.util.ArrayList;
@ -68,6 +71,8 @@ public class HapiSchemaMigrationTest {
private static final String COLUMN_VAL_VC = "VAL_VC";
private static final String NULL_PLACEHOLDER = "[NULL]";
private static final String COLLATION_CASE_INSENSITIVE = "SQL_Latin1_General_CP1_CI_AS";
private static final String COLLATION_CASE_SENSITIVE = "SQL_Latin1_General_CP1_CS_AS";
static {
HapiSystemProperties.enableUnitTestMode();
@ -127,6 +132,8 @@ public class HapiSchemaMigrationTest {
verifyHfjResSearchUrlMigration(database, theDriverType);
verifyTrm_Concept_Desig(database, theDriverType);
verifyHfjResourceFhirIdCollation(database, theDriverType);
}
/**
@ -170,7 +177,7 @@ public class HapiSchemaMigrationTest {
try (final Connection connection = theDatabase.getDataSource().getConnection()) {
final DatabaseMetaData tableMetaData = connection.getMetaData();
final List<Map<String,String>> actualColumnResults = new ArrayList<>();
final List<Map<String, String>> actualColumnResults = new ArrayList<>();
try (final ResultSet columnsResultSet = tableMetaData.getColumns(null, null, TABLE_HFJ_RES_SEARCH_URL, null)) {
while (columnsResultSet.next()) {
final Map<String, String> columnMap = new HashMap<>();
@ -183,7 +190,7 @@ public class HapiSchemaMigrationTest {
}
}
final List<Map<String,String>> actualPrimaryKeyResults = new ArrayList<>();
final List<Map<String, String>> actualPrimaryKeyResults = new ArrayList<>();
try (final ResultSet primaryKeyResultSet = tableMetaData.getPrimaryKeys(null, null, TABLE_HFJ_RES_SEARCH_URL)) {
while (primaryKeyResultSet.next()) {
@ -300,7 +307,7 @@ public class HapiSchemaMigrationTest {
: Integer.toString(Types.VARCHAR);
}
private void extractAndAddToMap(ResultSet theResultSet, Map<String,String> theMap, String theColumn) throws SQLException {
private void extractAndAddToMap(ResultSet theResultSet, Map<String, String> theMap, String theColumn) throws SQLException {
theMap.put(theColumn, Optional.ofNullable(theResultSet.getString(theColumn))
.map(defaultValueNonNull -> defaultValueNonNull.equals("((-1))") ? "-1" : defaultValueNonNull) // MSSQL returns "((-1))" for default value
.map(String::toUpperCase)
@ -336,7 +343,6 @@ public class HapiSchemaMigrationTest {
dataSource.setUsername("SA");
dataSource.setPassword("SA");
dataSource.start();
MigrationTaskList migrationTasks = new HapiFhirJpaMigrationTasks(Collections.emptySet()).getTaskList(VersionEnum.V6_0_0, VersionEnum.V6_4_0);
HapiMigrationDao hapiMigrationDao = new HapiMigrationDao(dataSource, DriverTypeEnum.H2_EMBEDDED, HAPI_FHIR_MIGRATION_TABLENAME);
HapiMigrationStorageSvc hapiMigrationStorageSvc = new HapiMigrationStorageSvc(hapiMigrationDao);
@ -349,4 +355,64 @@ public class HapiSchemaMigrationTest {
assertFalse(schemaMigrator.createMigrationTableIfRequired());
}
private void verifyHfjResourceFhirIdCollation(JpaEmbeddedDatabase database, DriverTypeEnum theDriverType) throws SQLException {
if (DriverTypeEnum.MSSQL_2012 == theDriverType) { // Other databases are unaffected by this migration and are irrelevant
try (final Connection connection = database.getDataSource().getConnection()) {
@Language("SQL")
final String databaseCollationSql = """
SELECT collation_name
FROM sys.databases
WHERE name = 'master'
""";
final Map<String, Object> databaseCollationRow = querySingleRow(connection, databaseCollationSql);
assertThat(databaseCollationRow.get("collation_name")).isEqualTo(COLLATION_CASE_INSENSITIVE);
@Language("SQL")
final String tableColumnSql = """
SELECT c.collation_name
FROM sys.columns c
INNER JOIN sys.tables t on c.object_id = t.object_id
INNER JOIN sys.schemas s on t.schema_id = s.schema_id
INNER JOIN sys.databases d on s.principal_id = d.database_id
where d.name = 'master'
AND s.name = 'dbo'
AND t.name = 'HFJ_RESOURCE'
AND c.name = 'FHIR_ID';
""";
final Map<String, Object> tableColumnCollationRow = querySingleRow(connection, tableColumnSql);
assertThat(tableColumnCollationRow.get("collation_name")).isEqualTo(COLLATION_CASE_SENSITIVE);
// We have not changed the database collation, so we can reference the table and column names with the wrong
// case and the query will work
@Language("SQL")
final String fhirIdSql = """
SELECT fhir_id
FROM hfj_resource
WHERE fhir_id = '2029'
""";
final Map<String, Object> fhirIdRow = querySingleRow(connection, fhirIdSql);
assertThat(fhirIdRow.get("fhir_id")).isEqualTo("2029");
}
}
}
private Map<String,Object> querySingleRow(Connection connection, String theSql) throws SQLException {
final Map<String, Object> row = new HashMap<>();
try (final PreparedStatement preparedStatement = connection.prepareStatement(theSql)) {
try (final ResultSet resultSet = preparedStatement.executeQuery()) {
if (resultSet.next()) {
final ResultSetMetaData resultSetMetadata = resultSet.getMetaData();
for (int index = 1; index < resultSetMetadata.getColumnCount() +1; index++) {
row.put(resultSetMetadata.getColumnName(index), resultSet.getObject(index));
}
}
}
}
return row;
}
}