fail the schema-update if there are existing Evidence or EvidenceVariable resource instances

[lmsurpre]

Is your feature request related to a problem? Please describe. Evidence and EvidenceVariable are the two resource types in R4B where a valid R4 instance is basically guaranteed not to parse.

That means that, after migration to our 5.0.0, certain interactions could blow up on any existing resources of this type in the db.

Describe the solution you'd like To avoid a big surprise down-the-line, I think it would be better just to fail the schema-upgrade in cases where there is data in the Evidence_RESOURCES or EvidenceVariable_RESOURCES tables.

Describe alternatives you've considered Provide steps to patch the old Evidence and EvidenceVariable resources instead

Acceptance Criteria

1. GIVEN a 4.11.1 schema with Evidence or EvidenceVariable resources WHEN 5.0.0-SNAPSHOT schema upgrade is applied THEN the upgrade is aborted AND there is a useful error message near the bottom of the output AND no other schema changes were applied

2. GIVEN a 4.11.1 schema with no Evidence or EvidenceVariable resources WHEN 5.0.0-SNAPSHOT schema upgrade is applied THEN the schema upgrade is successful

Additional context EvidenceVariable too?

[punktilious]

Implementation tips:

Add a new version to FhirSchemaVersion:

    ,V0030(30, "issue-3681 test for existence of Evidence and EvidenceVariable resources", false)

In com.ibm.fhir.schema.app.Main, see the method:

    protected void updateFhirSchema() {
        ...
                    // Finally, update the whole schema version
                    svm.updateSchemaVersion();
        ...

Before the svm.updateSchemaVersion() we''ll need a new method call which will check for data in the evidence_logical_resources and evidencevariable_logical_resources tables. That method will look something like this:

    private boolean checkIfDataExistsForV0030() {

        IDatabaseAdapter adapter = getDbAdapter(dbType, connectionPool);

        try (ITransaction tx = TransactionFactory.openTransaction(connectionPool)) {
            try {
                checkIfDataExistsForV0030(adapter, schema.getSchemaName());
            } catch (DataAccessException x) {
                // Something went wrong, so mark the transaction as failed
                tx.setRollbackOnly();
                throw x;
            }
        }
    }

    private void checkIfDataExistsForV0030(IDatabaseAdapter adapter, String schemaName) {
        TableHasData cmd = new s(schemaName, "evidence_logical_resources");
        if (adapter.runStatement(cmd)) {
            logger.severe("At least one Evidence resource exists. Cannot upgrade " + schemaName);
            return true;
        }
        cmd = new s(schemaName, "evidencevariable_logical_resources");
        if (adapter.runStatement(cmd)) {
            logger.severe("At least one EvidenceVariable resource exists. Cannot upgrade " + schemaName);
            return true;
        }
        return false;
    }

The checkIfDataExistsForV0030 should only be called if the current schema is less before V0030:

                    int currentSchemaVersion = svm.getVersionForSchema();
                    if (currentSchemaVersion < FhirSchemaVersion.V0030.vid()) {
                        if (checkIfDataExistsForV0030()) {
                             ...signal error
                        }
                    }

There are a number of example DAO command implementations you can use as the basic for the TableHasData class. GetXXLogicalResourceNeedsMigration is a good example. The SQL statement you'll want to be executing is as simple as:

    "SELECT 1 FROM evidence_logical_resources " + translator.limit("1");

Obviously the table name (evidence_logical_resources) will be configurable (a member variable of the DAO).

[punktilious]

Because the table name is a parameter to the DAO command class, we should check it is a valid name (secure programming). DataDefinitionUtil has a couple of methods to support this. The simplest would be:

   this.tableName = DataDefinitionUtil.assertValidName(tableName);

in the constructor.

[punktilious]

Unit testing this particular case is a bit more involved. The schema migration tests currently don't take into account any of the migration steps we apply in the Main class. So in this instance, it is probably reasonable to not bother with "migration" at all, and simply just test the DAO:

1. find a current test where the schema is created so you can add a new test
2. make sure that Evidence is one of the resource types created (for this testing, we tend to restrict schema creation to just Observation to make it faster and lighter).
3. test your DAO to make sure it gives the correct answer
4. create a record in evidence_logical_resources
5. Test your DAO to make sure it gives the correct answer.

This unit test will be in fhir-persistence-schema, which means you don't have access to the fhir-persistence-jdbc layer, so you need to create the resource record yourself using a plain insert:

            final String insert = "INSERT INTO " + tablePrefix + "_logical_resources (logical_resource_id, logical_id, is_deleted, last_updated, version_id) VALUES (?, ?, ?, ?, ?)";
            try (PreparedStatement stmt = conn.prepareStatement(insert)) {
                // bind parameters
                stmt.setLong(1, v_logical_resource_id);
                stmt.setString(2, 'evidence123');
                stmt.setString(3,  "N");
                stmt.setTimestamp(4, p_last_updated, UTC); // can use Instant.now() converted to timestamp
                stmt.setInt(5, 1); // initial version
                stmt.executeUpdate();
    }

The v_logical_resource_id value could probably be just 1. Theoretically it should be assigned the next val from fhir_sequence, but for this test case I don't think it matters.

[punktilious]

Oh, because your DAO should be generic, it doesn't matter if you test Evidence or Observation. In this case because our tests currently use Observation, just go with that.

[punktilious]

Unfortunately because we're not unit-testing the data migration logic contained in the Main class, this means that we won't have a unit test to check the schema version (< V0030) logic. We may have to check this with inspection and QA.

[lmsurpre]

the schema upgrade is split into multiple transactions, so we need to move the check to the beginning to avoid complicated rollback logic

[punktilious]

QA: Using a schema which contains tables for Evidence and EvidenceVariable, I manually inserted row into one of evidence_logical_resources or evidencevariable_logical_resources, for example:

insert into fhirdata.evidence_logical_resources(logical_resource_id, logical_id, is_deleted) values (3880, 'foo', 'Y');

Also, we need to manipulate the whole_schema_version to a prior value so that we don't bypass everything:

update fhirdata.whole_schema_version set version_id = 29;

Running first with an evidence row, we see:

2022-08-30 11:22:24.524 00000001    INFO .common.JdbcConnectionProvider Opening connection to database: jdbc:postgresql://localhost:5432/fhirdb
2022-08-30 11:22:24.686 00000001    INFO base.utils.schema.LeaseManager Requesting update lease for schema 'FHIRDATA' [attempt 1]
2022-08-30 11:22:24.700 00000001 WARNING ls.pool.PoolConnectionProvider Connection open count is > 1
2022-08-30 11:22:24.709 00000001  SEVERE forhealth.fhir.schema.app.Main At least one Evidence resource exists. Cannot upgrade FHIRDATA
2022-08-30 11:22:24.711 00000001    INFO base.utils.schema.LeaseManager Canceling lease for schema 'FHIRDATA'
2022-08-30 11:22:24.716 00000001  SEVERE forhealth.fhir.schema.app.Main schema tool failed

Cannot update schema due to existing Evidence or EvidenceVariable resource instances
java.lang.IllegalStateException: Cannot update schema due to existing Evidence or EvidenceVariable resource instances
	at org.linuxforhealth.fhir.schema.app.Main.updateFhirSchema(Main.java:525)
	at org.linuxforhealth.fhir.schema.app.Main.updateSchemas(Main.java:452)
	at org.linuxforhealth.fhir.schema.app.Main.process(Main.java:1882)
	at org.linuxforhealth.fhir.schema.app.Main.main(Main.java:2007)
2022-08-30 11:22:24.717 00000001  SEVERE forhealth.fhir.schema.app.Main SCHEMA CHANGE: RUNTIME ERROR

Then the same with an evidencevariable_logical_resources record:

2022-08-30 11:23:18.580 00000001    INFO .common.JdbcConnectionProvider Opening connection to database: jdbc:postgresql://localhost:5432/fhirdb
2022-08-30 11:23:18.744 00000001    INFO base.utils.schema.LeaseManager Requesting update lease for schema 'FHIRDATA' [attempt 1]
2022-08-30 11:23:18.763 00000001 WARNING ls.pool.PoolConnectionProvider Connection open count is > 1
2022-08-30 11:23:18.772 00000001 WARNING ls.pool.PoolConnectionProvider Connection open count is > 1
2022-08-30 11:23:18.776 00000001  SEVERE forhealth.fhir.schema.app.Main At least one EvidenceVariable resource exists. Cannot upgrade FHIRDATA
2022-08-30 11:23:18.778 00000001    INFO base.utils.schema.LeaseManager Canceling lease for schema 'FHIRDATA'
2022-08-30 11:23:18.782 00000001  SEVERE forhealth.fhir.schema.app.Main schema tool failed

Cannot update schema due to existing Evidence or EvidenceVariable resource instances
java.lang.IllegalStateException: Cannot update schema due to existing Evidence or EvidenceVariable resource instances
	at org.linuxforhealth.fhir.schema.app.Main.updateFhirSchema(Main.java:525)
	at org.linuxforhealth.fhir.schema.app.Main.updateSchemas(Main.java:452)
	at org.linuxforhealth.fhir.schema.app.Main.process(Main.java:1882)
	at org.linuxforhealth.fhir.schema.app.Main.main(Main.java:2007)
2022-08-30 11:23:18.784 00000001  SEVERE forhealth.fhir.schema.app.Main SCHEMA CHANGE: RUNTIME ERROR

After deleting the offending record, the schema update completes successfully:

2022-08-30 11:29:31.605 00000001    INFO forhealth.fhir.schema.app.Main SCHEMA CHANGE: OK
...
select * from fhirdata.whole_schema_version;
 record_id | version_id 
-----------+------------
         1 |         30