issue #1789 refresh-tenants does not handle Db2 multi-tenant/multi-schema scenarios

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/db2/Db2ResourceReferenceDAO.java

@@ -22,19 +22,18 @@
 import com.ibm.fhir.persistence.jdbc.dao.impl.ResourceReferenceDAO;
 import com.ibm.fhir.persistence.jdbc.dao.impl.ResourceTokenValueRec;
 import com.ibm.fhir.persistence.jdbc.dto.CommonTokenValue;
-import com.ibm.fhir.persistence.jdbc.postgresql.PostgresResourceReferenceDAO;
 
 
 /**
- * Postgres-specific extension of the {@link ResourceReferenceDAO} to work around
+ * Db2-specific extension of the {@link ResourceReferenceDAO} to work around
  * some SQL syntax and Postgres concurrency issues
  */
 public class Db2ResourceReferenceDAO extends ResourceReferenceDAO {
-    private static final Logger logger = Logger.getLogger(PostgresResourceReferenceDAO.class.getName());
+    private static final Logger logger = Logger.getLogger(Db2ResourceReferenceDAO.class.getName());
 
-    // The name of the admin_schema containing the SV_TENANT_ID variable
+    // The name of the admin schema containing the SV_TENANT_ID variable
     private final String adminSchemaName;
-    
+
     /**
      * Public constructor
      * @param t
@@ -46,18 +45,18 @@ public Db2ResourceReferenceDAO(IDatabaseTranslator t, Connection c, String schem
         super(t, c, schemaName, cache);
         this.adminSchemaName = adminSchemaName;
     }
-    
+
     @Override
     public void doCodeSystemsUpsert(String paramList, Collection<String> systemNames) {
         // query is a negative outer join so we only pick the rows where
         // the row "s" from the actual table doesn't exist.
-        
+
         // Derby won't let us use any ORDER BY, even in a sub-select so we need to
         // sort the values externally. It would be better if code_system_id were
         // an identity column, but that's a much bigger change.
         final List<String> sortedNames = new ArrayList<>(systemNames);
         sortedNames.sort((String left, String right) -> left.compareTo(right));
-        
+
         final String nextVal = getTranslator().nextValue(getSchemaName(), "fhir_ref_sequence");
         StringBuilder insert = new StringBuilder();
         insert.append("INSERT INTO code_systems (mt_id, code_system_id, code_system_name) ");
@@ -67,7 +66,7 @@ public void doCodeSystemsUpsert(String paramList, Collection<String> systemNames
         insert.append(" LEFT OUTER JOIN code_systems s ");
         insert.append("              ON s.code_system_name = v.name ");
         insert.append("           WHERE s.code_system_name IS NULL ");
-        
+
         // Note, we use PreparedStatement here on purpose. Partly because it's
         // secure coding best practice, but also because many resources will have the
         // same number of parameters, and hopefully we'll therefore share a small subset
@@ -79,7 +78,7 @@ public void doCodeSystemsUpsert(String paramList, Collection<String> systemNames
             for (String name: sortedNames) {
                 ps.setString(a++, name);
             }
-            
+
             ps.executeUpdate();
         } catch (SQLException x) {
             logger.log(Level.SEVERE, insert.toString(), x);
@@ -98,7 +97,7 @@ protected void doCommonTokenValuesUpsert(String paramList, Collection<CommonToke
         insert.append("              ON ctv.token_value = v.token_value ");
         insert.append("             AND ctv.code_system_id = v.code_system_id ");
         insert.append("      WHERE ctv.token_value IS NULL ");
-        
+
         // Note, we use PreparedStatement here on purpose. Partly because it's
         // secure coding best practice, but also because many resources will have the
         // same number of parameters, and hopefully we'll therefore share a small subset
@@ -111,7 +110,7 @@ protected void doCommonTokenValuesUpsert(String paramList, Collection<CommonToke
                 ps.setString(a++, tv.getTokenValue());
                 ps.setInt(a++, tv.getCodeSystemId());
             }
-            
+
             ps.executeUpdate();
         } catch (SQLException x) {
             StringBuilder values = new StringBuilder();
@@ -125,12 +124,12 @@ protected void doCommonTokenValuesUpsert(String paramList, Collection<CommonToke
                 values.append(tv.getCodeSystemId());
                 values.append("}");
             }
-            
+
             logger.log(Level.SEVERE, insert.toString() + "; [" + values.toString() + "]", x);
             throw getTranslator().translate(x);
         }
     }
-    
+
     @Override
     protected void insertResourceTokenRefs(String resourceType, Collection<ResourceTokenValueRec> xrefs) {
         // Now all the values should have ids assigned so we can go ahead and insert them
@@ -150,7 +149,7 @@ protected void insertResourceTokenRefs(String resourceType, Collection<ResourceT
                 if (xr.getCommonTokenValueId() != null) {
                     ps.setLong(3, xr.getCommonTokenValueId());
                 } else {
-                    ps.setNull(3, Types.BIGINT);                    
+                    ps.setNull(3, Types.BIGINT);
                 }
 
                 // version can be null
@@ -165,7 +164,7 @@ protected void insertResourceTokenRefs(String resourceType, Collection<ResourceT
                     count = 0;
                 }
             }
-            
+
             if (count > 0) {
                 ps.executeBatch();
             }

fhir-persistence-schema/README.md

@@ -141,6 +141,19 @@ Once the server has determined the tenant id for a given request, it uses this t
 used in conjunction to create or retrieve data for this tenant.
 For more information on multi-tenancy, see section [4.9 Multi-tenancy of the IBM FHIR Server Users Guide](https://ibm.github.io/FHIR/guides/FHIRServerUsersGuide#49-multi-tenancy).
 
+### Refresh Tenant Following Schema Update (Db2 only)
+
+After a schema update you must run the refresh-tenants command to ensure that any new tables added by the update have the correct partitions. The refresh-tenants process will iterate over each tenant and allocate new partitions as needed. This step is idempotent, so you can run it more than once if required.
+
+
+```
+    java -jar schema/fhir-persistence-schema-*-cli.jar \
+      --prop-file db2.properties --refresh-tenants
+```
+
+If processing completes successfully, the program will report `SCHEMA CHANGE: OK`. If an error occurs, run the step again after correcting the issue.
+
+
 ### Configure tenant-key (example)  (Db2 only)
 
 Edit `wlp/usr/servers/fhir-server/config/default/fhir-server-config.json` and add the tenant-key captured from the add-tenant step above:

fhir-persistence-schema/src/main/java/com/ibm/fhir/schema/app/Main.java

@@ -24,6 +24,7 @@
 import java.sql.SQLException;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Properties;
@@ -631,6 +632,9 @@ protected void allocateTenant() {
             return;
         }
 
+        // IMPORTANT! Check the schema name aligns with the actual schema for this tenant
+        checkSchemaForTenant();
+
         // The key we'll use for this tenant. This key should be used in subsequent
         // activities related to this tenant, such as setting the tenant context.
         if (tenantKeyFileUtil.keyFileExists(tenantKeyFileName)) {
@@ -715,17 +719,49 @@ protected void allocateTenant() {
     }
 
     /**
-     * Make sure all the tables has a partition created for the configured tenant
+     * Run a check to make sure that if the tenant already exists its schema
+     * matches the specified schema. This prevents users from accidentally
+     * creating a second instance of a tenant in a different schema
      */
-    protected void refreshTenants() {
+    protected void checkSchemaForTenant() {
         if (!MULTITENANT_FEATURE_ENABLED.contains(dbType)) {
+            // only relevant for databases which support multiple tenants within one schema (like Db2)
             return;
         }
+        List<TenantInfo> tenants = getTenantList();
 
-        // Build/update the tables as well as the stored procedures
-        FhirSchemaGenerator gen = new FhirSchemaGenerator(schema.getAdminSchemaName(), schema.getSchemaName(), isMultitenant());
-        PhysicalDataModel pdm = new PhysicalDataModel();
-        gen.buildSchema(pdm);
+        // Scan over the list to see if the tenant we are working with
+        // already exists
+        for (TenantInfo ti: tenants) {
+            if (ti.getTenantName().equals(this.tenantName)) {
+                if (ti.getTenantSchema() == null) {
+                    // The schema is empty so no chance of adding tenants
+                    throw new IllegalArgumentException("Schema '" + schema.getSchemaName() + "'"
+                        + " for tenant '" + ti.getTenantName() + "'"
+                        + " does not contain a valid IBM FHIR Server schema");
+                } else if (!ti.getTenantSchema().equalsIgnoreCase(schema.getSchemaName())) {
+                    // The given schema name doesn't match where we think this tenant
+                    // should be located, so throw an error before any damage is done
+                    throw new IllegalArgumentException("--schema-name argument '" + schema.getSchemaName()
+                    + "' does not match schema '" + ti.getTenantSchema() + "' for tenant '"
+                    + ti.getTenantName() + "'");
+                }
+            }
+        }
+    }
+
+    /**
+     * Get the list of tenants and the schemas each is currently defined in. Because
+     * this tenant-schema mapping isn't stored directly, it has to be inferred by
+     * looking up the schema from one of the tables we know should exist. The schema
+     * name may therefore be null if the tenant record (in FHIR_ADMIN.TENANTS) exists,
+     * but all the tables from that schema have been dropped.
+     * @return
+     */
+    protected List<TenantInfo> getTenantList() {
+        if (!MULTITENANT_FEATURE_ENABLED.contains(dbType)) {
+            return Collections.emptyList();
+        }
 
         // Similar to the allocate tenant processing, except in this case we want to
         // make sure that each table has all the tenant partitions required. This is
@@ -743,10 +779,33 @@ protected void refreshTenants() {
             }
         }
 
+        return tenants;
+    }
+
+    /**
+     * Make sure all the tables has a partition created for the configured tenant
+     */
+    protected void refreshTenants() {
+        if (!MULTITENANT_FEATURE_ENABLED.contains(dbType)) {
+            return;
+        }
+
+        // Similar to the allocate tenant processing, except in this case we want to
+        // make sure that each table has all the tenant partitions required. This is
+        // to handle the case where a schema update has added a new table.
+        List<TenantInfo> tenants = getTenantList();
+
         // make sure the list is sorted by tenantId. Lambdas really do clean up this sort of code
         tenants.sort((TenantInfo left, TenantInfo right) -> left.getTenantId() < right.getTenantId() ? -1 : left.getTenantId() > right.getTenantId() ? 1 : 0);
         for (TenantInfo ti: tenants) {
             if (ti.getTenantSchema() != null) {
+                // It's crucial we use the correct schema for each particular tenant, which
+                // is why we have to build the PhysicalDataModel separately for each tenant
+                FhirSchemaGenerator gen = new FhirSchemaGenerator(schema.getAdminSchemaName(), ti.getTenantSchema(), isMultitenant());
+                PhysicalDataModel pdm = new PhysicalDataModel();
+                gen.buildSchema(pdm);
+
+                Db2Adapter adapter = new Db2Adapter(connectionPool);
                 pdm.addNewTenantPartitions(adapter, ti.getTenantSchema(), ti.getTenantId());
             }
         }