issue #3507 - treat "undelete" updates like create-on-update in changes

Update db2 and postgresql add_any_resource.sql stored procs (and
DerbyResourceDAO.storeResource) to write a 'C' instead of a 'U' for this
case. This way, system and type-level history interactions can properly
report the response status as a 201 instead of a 200.

Note, however, that there are ongoing
limitations for the accuracy of instance-level history entries as
outlined at
#3507 (comment)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/derby/DerbyResourceDAO.java

@@ -203,9 +203,9 @@ public Resource  insert(Resource resource, List<ExtractedParameterValue> paramet
      * @return the resource_id for the entry we created
      * @throws Exception
      */
-    public long storeResource(String tablePrefix, List<ExtractedParameterValue> parameters, 
+    public long storeResource(String tablePrefix, List<ExtractedParameterValue> parameters,
             String p_logical_id, InputStream p_payload, Timestamp p_last_updated, boolean p_is_deleted,
-            String p_source_key, Integer p_version, String p_parameterHashB64, Connection conn, 
+            String p_source_key, Integer p_version, String p_parameterHashB64, Connection conn,
             ParameterDAO parameterDao, Integer ifNoneMatch, String resourcePayloadKey,
             AtomicInteger outInteractionStatus, AtomicInteger outIfNoneMatchVersion) throws Exception {
 
@@ -452,7 +452,7 @@ public long storeResource(String tablePrefix, List<ExtractedParameterValue> para
             stmt.setLong(1, v_resource_id);
             stmt.setLong(2, v_logical_resource_id);
             stmt.setInt(3, p_version);
-            
+
             if (p_payload != null) {
                 stmt.setBinaryStream(4, p_payload);
             } else {
@@ -521,7 +521,7 @@ identityCache, getResourceReferenceDAO(), getTransactionData())) {
 
         // Finally, write a record to RESOURCE_CHANGE_LOG which records each event
         // related to resources changes (issue-1955)
-        String changeType = p_is_deleted ? "D" : v_new_resource ? "C" :  "U";
+        String changeType = p_is_deleted ? "D" : (v_new_resource || v_currently_deleted) ? "C" : "U";
         String INSERT_CHANGE_LOG = "INSERT INTO resource_change_log(resource_id, change_tstamp, resource_type_id, logical_resource_id, version_id, change_type)"
                 + " VALUES (?,?,?,?,?,?)";
         try (PreparedStatement ps = conn.prepareStatement(INSERT_CHANGE_LOG)) {

fhir-persistence-schema/src/main/resources/db2/add_any_resource.sql

@@ -168,11 +168,11 @@ BEGIN
     -- check the current vs new parameter hash to see if we can bypass the delete/insert
     IF o_current_parameter_hash IS NULL OR o_current_parameter_hash != p_parameter_hash_b64
     THEN
-	    -- existing resource, so need to delete all its parameters. 
-	    -- TODO patch parameter sets instead of all delete/all insert.
-	    PREPARE stmt FROM 'CALL ' || v_schema_name || '.delete_resource_parameters(?,?)';
-	    EXECUTE stmt USING p_resource_type, v_logical_resource_id;
-	END IF; -- end if parameter hash is different    
+        -- existing resource, so need to delete all its parameters. 
+        -- TODO patch parameter sets instead of all delete/all insert.
+        PREPARE stmt FROM 'CALL ' || v_schema_name || '.delete_resource_parameters(?,?)';
+        EXECUTE stmt USING p_resource_type, v_logical_resource_id;
+    END IF; -- end if parameter hash is different    
   END IF; -- end if existing resource
 
   PREPARE stmt FROM
@@ -191,18 +191,18 @@ BEGIN
     PREPARE stmt FROM 'UPDATE ' || v_schema_name || '.logical_resources SET is_deleted = ?, last_updated = ?, parameter_hash = ? WHERE logical_resource_id = ?';
     EXECUTE stmt USING p_is_deleted, p_last_updated, p_parameter_hash_b64, v_logical_resource_id;
   END IF;
-  
+
   -- DB2 doesn't support user defined array types in dynamic SQL UNNEST/CAST statements,
   -- so we can no longer insert the parameters here - instead we have to use individual
   -- JDBC statements.
-  
+
   -- Finally, write a record to RESOURCE_CHANGE_LOG which records each event
   -- related to resources changes (issue-1955)
   IF p_is_deleted = 'Y'
   THEN
     SET v_change_type = 'D';
   ELSE 
-    IF v_new_resource = 0
+    IF v_new_resource = 0 AND v_currently_deleted = 'N'
     THEN
       SET v_change_type = 'U';
     ELSE

fhir-persistence-schema/src/main/resources/postgres/add_any_resource.sql

@@ -165,7 +165,7 @@ BEGIN
       || ' VALUES ($1, $2, $3, $4, $5, $6, $7)'
     USING v_resource_id, v_logical_resource_id, p_version, p_payload, p_last_updated, p_is_deleted, p_resource_payload_key;
 
-  
+
   IF v_new_resource = 0 THEN
     -- As this is an existing logical resource, we need to update the xx_logical_resource values to match
     -- the values of the current resource. For new resources, these are added by the insert so we don't
@@ -184,7 +184,7 @@ BEGIN
   THEN
     v_change_type := 'D';
   ELSE 
-    IF v_new_resource = 0
+    IF v_new_resource = 0 AND v_currently_deleted = 'N'
     THEN
       v_change_type := 'U';
     ELSE

fhir-server-test/src/test/java/com/ibm/fhir/server/test/BundleTest.java

@@ -1,5 +1,5 @@
 /*
- * (C) Copyright IBM Corp. 2017, 2021
+ * (C) Copyright IBM Corp. 2017, 2022
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -16,10 +16,6 @@
 import java.util.List;
 import java.util.UUID;
 
-import jakarta.json.Json;
-import jakarta.json.JsonArray;
-import jakarta.json.JsonObject;
-import jakarta.json.JsonObjectBuilder;
 import javax.ws.rs.client.Entity;
 import javax.ws.rs.client.WebTarget;
 import javax.ws.rs.core.Response;
@@ -54,6 +50,11 @@
 import com.ibm.fhir.model.type.code.HTTPVerb;
 import com.ibm.fhir.model.type.code.IssueType;
 
+import jakarta.json.Json;
+import jakarta.json.JsonArray;
+import jakarta.json.JsonObject;
+import jakarta.json.JsonObjectBuilder;
+
 /**
  * This class tests 'batch' and 'transaction' interactions.
  */
@@ -787,7 +788,7 @@ public void testBatchHistory() throws Exception {
             if(entry.getResponse() != null){
                 String returnedStatus = entry.getResponse().getStatus().getValue();
                 assertNotNull(returnedStatus);
-                assertTrue(returnedStatus.startsWith("200"));
+                assertTrue(returnedStatus.startsWith("200") || returnedStatus.startsWith("201"));
                 result = true;
             }
         }
@@ -1431,7 +1432,7 @@ public void testTransactionUpdatesError() throws Exception {
         assertHistoryResults(target, "Patient/" + patientT1.getId() + "/_history", 2);
         assertHistoryResults(target, "Patient/" + patientT2.getId() + "/_history", 2);
     }
-    
+
     @Test(groups = { "transaction" }, dependsOnMethods = { "testTransactionUpdates" })
     public void testTransactionInvalidRequestError() throws Exception {
         String method = "testTransactionInvalidRequestError";
@@ -1451,8 +1452,8 @@ public void testTransactionInvalidRequestError() throws Exception {
                 patientT1);
         bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, "Patient/" + patientT2.getId(), null,
                 patientT2);
-        
-        
+
+
         // the URL does not have an id and nor is it a conditional update, so this is an error
         // which should be picked up when translating the bundle entry into an interaction
         bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, "Observation", null,
@@ -2041,7 +2042,7 @@ public void testTransactionLocalRefs2() throws Exception {
         Resource patientResource = patientEntry.getResource();
         assertTrue(patientResource instanceof Patient);
         patient = (Patient) patientResource;
-        
+
         // Verify the Patient.managingOrganization field.
         assertNotNull(patient.getManagingOrganization());
         assertNotNull(patient.getManagingOrganization().getReference());
@@ -2055,7 +2056,7 @@ public void testTransactionLocalRefs2() throws Exception {
         actualReference = patient.getGeneralPractitioner().get(0).getReference().getValue();
         assertNotNull(actualReference);
         assertEquals(actualReference, expectedPractitionerReference);
-        
+
         // Next, check each observation to make sure their local references were
         // processed correctly.
         for (int i = 3; i < 5; i++) {
@@ -2382,10 +2383,10 @@ public void testBatchConditionalUpdates() throws Exception {
 
         Bundle bundle = buildBundle(BundleType.BATCH);
         bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, urlString, null, patient);
-        
+
         // Removed for 1869. We no longer support bundles with multiple updates for the same resource.
         // bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, urlString, null, patient.toBuilder().id(null).build());
-        
+
         bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, multipleMatches, null, patient);
         bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, badSearch, null, patient);
 
@@ -2641,7 +2642,7 @@ public void testTransactionBundleWithSkippableUpdates() throws Exception {
         assertEquals(entry1.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
         Patient responsePatient = entry1.getResource().as(Patient.class);
 
-        
+
         // Transaction 2. PUT the same patient again. Should be skipped because the resource matches
         Bundle.Entry bundleEntry2 = Bundle.Entry.builder()
                 .fullUrl(Uri.of("urn:2"))
@@ -2715,7 +2716,7 @@ public void testTransactionBundleWithSkippableUpdates() throws Exception {
     }
 
     /**
-     * To test If-None-Match (conditional create-on-update) we must use 
+     * To test If-None-Match (conditional create-on-update) we must use
      * multiple requests because:
      *   "A resource can only appear in a transaction once (by identity)."
      * Requests:
@@ -2767,7 +2768,7 @@ public void testTransactionBundleWithIfNoneMatch() throws Exception {
         assertEquals(entry1.getResponse().getStatus().getValue(), "201");
         assertEquals(entry1.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
 
-        
+
         // Interaction 2. PUT the same patient again. Should be skipped because IfNoneMatch - because the
         // processing error for the entry is 412, this should fail the bundle with a 400
         Bundle.Entry bundleEntry2 = Bundle.Entry.builder()
@@ -2794,7 +2795,7 @@ public void testTransactionBundleWithIfNoneMatch() throws Exception {
         FHIRResponse response3 = client.delete(Patient.class.getSimpleName(), randomId);
         assertNotNull(response3);
         assertResponse(response3.getResponse(), Response.Status.OK.getStatusCode());
-        
+
         // Interaction 4. Undelete
         Bundle.Entry bundleEntry4 = Bundle.Entry.builder()
                 .fullUrl(Uri.of("urn:2"))
@@ -2821,13 +2822,13 @@ public void testTransactionBundleWithIfNoneMatch() throws Exception {
 
         // Undelete uses 201 Created to pass Touchstone
         assertEquals(entry4.getResponse().getStatus().getValue(), "201");
-        
+
         // Version 2 is the delete marker, so should be version 3 after undelete
         assertEquals(entry4.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/3");
     }
 
     /**
-     * To test If-None-Match (conditional create-on-update) we must use 
+     * To test If-None-Match (conditional create-on-update) we must use
      * multiple requests because:
      *   "A resource can only appear in a transaction once (by identity)."
      * Requests:
@@ -2877,7 +2878,7 @@ public void testBatchBundleWithIfNoneMatch() throws Exception {
         assertEquals(entry1.getResponse().getStatus().getValue(), "201");
         assertEquals(entry1.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
 
-        
+
         // Interaction 2. PUT the same patient again. Should be skipped because IfNoneMatch
         Bundle.Entry bundleEntry2 = Bundle.Entry.builder()
                 .fullUrl(Uri.of("urn:2"))
@@ -2907,7 +2908,7 @@ public void testBatchBundleWithIfNoneMatch() throws Exception {
         FHIRResponse response3 = client.delete(Patient.class.getSimpleName(), randomId);
         assertNotNull(response3);
         assertResponse(response3.getResponse(), Response.Status.OK.getStatusCode());
-        
+
         // Interaction 4. Undelete
         Bundle.Entry bundleEntry4 = Bundle.Entry.builder()
                 .fullUrl(Uri.of("urn:2"))
@@ -2934,7 +2935,7 @@ public void testBatchBundleWithIfNoneMatch() throws Exception {
 
         // Undelete uses 201 Created to pass Touchstone
         assertEquals(entry4.getResponse().getStatus().getValue(), "201");
-        
+
         // Version 2 is the delete marker, so should be version 3 after undelete
         assertEquals(entry4.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/3");
     }
@@ -3005,7 +3006,7 @@ private void assertGoodGetResponse(Bundle.Entry entry, int expectedStatusCode, H
         assertNotNull(response);
 
         assertNotNull(response.getStatus());
-        
+
         // TestNG: ACTUAL, EXPECTED
         assertEquals(response.getStatus().getValue(), Integer.toString(expectedStatusCode));