fix: version history is case sensitive, and fails when customers chan… #1782
Conversation
…ge the case sensisitivity of their schema names Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
...-database-utils/src/main/java/com/ibm/fhir/database/utils/version/VersionHistoryService.java
Outdated
Show resolved
Hide resolved
...-database-utils/src/main/java/com/ibm/fhir/database/utils/version/VersionHistoryService.java
Outdated
Show resolved
Hide resolved
| @@ -59,7 +59,7 @@ public GetLatestVersionDAO(String adminSchemaName, String schemaName) { | |||
| ps.setString(2, schemaName); | |||
| ResultSet rs = ps.executeQuery(); | |||
| while (rs.next()) { | |||
| String schema = rs.getString(1); | |||
| String schema = rs.getString(1).toUpperCase(); | |||
There was a problem hiding this comment.
I threw this in as uppercase for consistency
There was a problem hiding this comment.
Performance note: using WHERE UPPER(my_col) = ... can have performance implications because it often means that you can't use an index (on my_col). But in this case, it's unlikely to have a significant impact, because the row counts are relatively low.
| @@ -50,7 +50,7 @@ public GetLatestVersionDAO(String adminSchemaName, String schemaName) { | |||
| final String cols = DataDefinitionUtil.join(SchemaConstants.SCHEMA_NAME, SchemaConstants.OBJECT_TYPE, SchemaConstants.OBJECT_NAME); | |||
| final String sql = "SELECT " + cols | |||
| + ", max(" + SchemaConstants.VERSION + ") FROM " + tbl | |||
| + " WHERE " + SchemaConstants.SCHEMA_NAME + " IN (?, ?) " | |||
| + " WHERE UCASE(" + SchemaConstants.SCHEMA_NAME + ") IN (UCASE(?), UCASE(?)) " | |||
There was a problem hiding this comment.
I wrapped this in ucase for backwards compatiblity
There was a problem hiding this comment.
I don't think you should UCASE(?) the args like that...because it suggests you don't know what case the args are. We should be making sure the args are upper-case before that (because it's probably important for equals and HashMap lookups in other places). The first place I'd force case is when we parse the CLI argument in Main.
There was a problem hiding this comment.
OK - I sort of agree, However there is an inconsistency across versions I'd like too account for, also schema names in postgres tend to be lowercase versus uppercase in db2.
There was a problem hiding this comment.
I was thinking normalizing casing here was critical for backwards compatibility. If there is an existing version history service value thats been saved as lowercase, we need the new uppercase schema name to match it, right?
There was a problem hiding this comment.
Correct, I do that when we process the result sets down further on in
Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
Signed-off-by: Paul Bastide <pbastide@us.ibm.com>
…ge the case sensisitivity of their schema names
Signed-off-by: Paul Bastide pbastide@us.ibm.com