issue #3501 - reconcile minor difference between history apis

1. add Entry.response.location to instance-history response
2. set 201 for the initial create entry (we need #3507 for the undelete
case)
3. remove the status code name from Entry.response.status for system and
type-level history interactions (e.g. `200 OK` -> `200`)

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

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

@@ -200,12 +200,15 @@ public Resource  insert(Resource resource, List<ExtractedParameterValue> paramet
      * @param conn
      * @param parameterDao
      * @param ifNoneMatch 0 for conditional create-on-update behavior; otherwise null
+     * @param resourcePayloadKey
+     * @param outInteractionStatus
+     * @param outIfNoneMatchVersion
      * @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 +455,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 +524,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 ? "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-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.
  */
@@ -761,7 +762,7 @@ public void testBatchHistory() throws Exception {
         String method = "testBatchHistory";
         WebTarget target = getWebTarget();
 
-        // Perform a 'read' and a 'vread'.
+        // Get the history for patientB1
         Bundle bundle = buildBundle(BundleType.BATCH);
         bundle = addRequestToBundle(null, bundle, HTTPVerb.GET, "Patient/" + patientB1.getId() + "/_history",
                 null, null);
@@ -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("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));
 

fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java

@@ -700,7 +700,7 @@ public FHIRRestOperationResponse doUpdateMeta(FHIRPersistenceEvent event, String
             event.setFhirResource(newResource);
             event.setPrevFhirResource(ior.getPrevResource());
 
-            // Next, invoke the 'beforeUpdate' or 'beforeCreate' interceptor methods as appropriate.
+            // Next, invoke the 'beforeCreate', 'beforePatch', or 'beforeUpdate' interceptor methods as appropriate.
             boolean updateCreate = (ior.getPrevResource() == null);
             if (updateCreate) {
                 getInterceptorMgr().fireBeforeCreateEvent(event);
@@ -2335,12 +2335,17 @@ private Bundle createHistoryBundle(List<ResourceResult<? extends Resource>> reso
 
             // Determine the correct method to include in this history entry (POST, PUT, DELETE).
             HTTPVerb method;
+            String status;
             if (resourceResult.isDeleted() || resource == null) {
                 method = HTTPVerb.DELETE;
+                status = "200";
             } else if (resourceResult.getVersion() == 1) {
                 method = HTTPVerb.POST;
+                status = "201";
             } else {
                 method = HTTPVerb.PUT;
+                // TODO can we set this more intelligently (e.g. in the case of a DELETE followed by a PUT)
+                status = "200";
             }
 
             // Create the 'request' entry, and set the request.url field.
@@ -2353,14 +2358,17 @@ private Bundle createHistoryBundle(List<ResourceResult<? extends Resource>> reso
                     .url(Url.of(method == HTTPVerb.POST ? resourceType : resourcePath))
                     .build();
 
+            String fullUrl = getRequestBaseUri(type) + "/" + resourcePath;
+
             Entry.Response response = Entry.Response.builder()
-                    .status(string("200"))
+                    .status(status)
                     .etag(getEtagValue(resourceResult.getVersion()))
+                    .location(Uri.of(fullUrl + "/_history/" + resourceResult.getVersion()))
                     .lastModified(com.ibm.fhir.model.type.Instant.of(resourceResult.getLastUpdated().atZone(UTC)))
                     .build();
 
             Entry entry = Entry.builder()
-                    .fullUrl(Uri.of(getRequestBaseUri(type) + "/" + resourcePath))
+                    .fullUrl(Uri.of(fullUrl))
                     .request(request)
                     .response(response)
                     .resource(resource)
@@ -3097,17 +3105,17 @@ public Bundle doHistory(MultivaluedMap<String, String> queryParameters, String r
             case CREATE:
                 requestBuilder.method(changeRecord.getVersionId() > 1 ? HTTPVerb.PUT : HTTPVerb.POST);
                 requestBuilder.url(Url.of(changeRecord.getResourceTypeName()));
-                responseBuilder.status(com.ibm.fhir.model.type.String.of("201"));
+                responseBuilder.status("201");
                 break;
             case UPDATE:
                 requestBuilder.method(HTTPVerb.PUT);
                 requestBuilder.url(Url.of(changeRecord.getResourceTypeName() + "/" + changeRecord.getLogicalId()));
-                responseBuilder.status(com.ibm.fhir.model.type.String.of("200"));
+                responseBuilder.status("200");
                 break;
             case DELETE:
                 requestBuilder.method(HTTPVerb.DELETE);
                 requestBuilder.url(Url.of(changeRecord.getResourceTypeName() + "/" + changeRecord.getLogicalId()));
-                responseBuilder.status(com.ibm.fhir.model.type.String.of("200"));
+                responseBuilder.status("200");
                 break;
             }