issue #3614 - IfNoneMatch short-circuit behavior

Previously we just passed the value to FHIRPersistence and let it handle
this.  We still need FHIRPersistence to double-check it (in case of a
race), but we should be able to handle the simple case up-front from the
REST layer.

The real motivator for this change is that we compute "skippable
updates" in the REST layer *prior* to passing it to FHIRPersistence and
I wanted to check the precondition before we perform that optimization.

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

fhir-persistence/src/main/java/com/ibm/fhir/persistence/exception/FHIRPersistenceIfNoneMatchException.java

@@ -6,13 +6,17 @@
 
 package com.ibm.fhir.persistence.exception;
 
+import static com.ibm.fhir.core.FHIRConstants.EXT_BASE;
+import static com.ibm.fhir.model.type.String.string;
+
+import com.ibm.fhir.model.type.Extension;
 import com.ibm.fhir.model.type.code.IssueType;
 import com.ibm.fhir.model.util.FHIRUtil;
 
 /**
  * This exception is thrown when an IfNoneMatch precondition check
  * fails and the server is configured to treat this as an error
- * (412 Precondition Failed). 
+ * (412 Precondition Failed).
  * See FHIRConfiguration.PROPERTY_IF_NONE_MATCH_RETURNS_NOT_MODIFIED.
  */
 public class FHIRPersistenceIfNoneMatchException extends FHIRPersistenceException {
@@ -22,6 +26,11 @@ public class FHIRPersistenceIfNoneMatchException extends FHIRPersistenceExceptio
 
     public FHIRPersistenceIfNoneMatchException(String message) {
         super(message);
-        withIssue(FHIRUtil.buildOperationOutcomeIssue(getMessage(), IssueType.CONFLICT));
+        withIssue(FHIRUtil.buildOperationOutcomeIssue(getMessage(), IssueType.CONFLICT.toBuilder()
+                .extension(Extension.builder()
+                    .url(EXT_BASE + "http-failed-precondition")
+                    .value(string("If-None-Match"))
+                    .build())
+                .build()));
     }
 }

fhir-server-spi/src/main/java/com/ibm/fhir/server/spi/operation/FHIRResourceHelpers.java

@@ -152,13 +152,14 @@ FHIRRestOperationResponse doCreateMeta(FHIRPersistenceEvent event, List<Issue> w
      * @param ifMatchValue
      * @param searchQueryString
      * @param skippableUpdate
+     * @param ifNoneMatch
      * @param doValidation
      * @param warnings
      * @return
      * @throws Exception
      */
     FHIRRestOperationResponse doUpdateMeta(FHIRPersistenceEvent event, String type, String id, FHIRPatch patch, Resource newResource, String ifMatchValue,
-        String searchQueryString, boolean skippableUpdate, boolean doValidation, List<Issue> warnings) throws Exception;
+        String searchQueryString, boolean skippableUpdate, Integer ifNoneMatch, boolean doValidation, List<Issue> warnings) throws Exception;
 
     /**
      * Persist the newResource value for patch or update interactions
@@ -214,7 +215,7 @@ Map<String, Object> buildPersistenceEventProperties(String type, String id,
      *            if true, and the resource content in the update matches the existing resource on the server, then skip the update;
      *            if false, then always attempt the update
      * @param ifNoneMatch
-     *            conditional create-on-update
+     *            for conditional create-on-update, set to 0; otherwise leave null
      * @return a FHIRRestOperationResponse that contains the results of the operation
      * @throws Exception
      */
@@ -242,7 +243,7 @@ default FHIRRestOperationResponse doUpdate(String type, String id, Resource newR
      * @param doValidation
      *            if true, validate the resource; if false, assume the resource has already been validated
      * @param ifNoneMatch
-     *            conditional create-on-update
+     *            for conditional create-on-update, set to 0; otherwise leave null
      * @return a FHIRRestOperationResponse that contains the results of the operation
      * @throws Exception
      */

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

@@ -534,8 +534,8 @@ public void testBatchUpdatesVersionAware() throws Exception {
         WebTarget target = getWebTarget();
 
         // Make a small change to each patient.
-        patientBVA2 = patientBVA2.toBuilder().active(com.ibm.fhir.model.type.Boolean.TRUE).build();
-        patientBVA1 = patientBVA1.toBuilder().active(com.ibm.fhir.model.type.Boolean.FALSE).build();
+        patientBVA2 = patientBVA2.toBuilder().deceased(true).build();
+        patientBVA1 = patientBVA1.toBuilder().deceased(false).build();
 
         Bundle bundle = buildBundle(BundleType.BATCH);
         bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, "Patient/" + patientBVA2.getId(), "W/\"1\"",
@@ -622,8 +622,8 @@ public void testBatchUpdatesVersionAwareError2() throws Exception {
         assertNotNull(patientVA1);
 
         // Make a small change to each patient.
-        patientVA2 = patientVA2.toBuilder().active(com.ibm.fhir.model.type.Boolean.TRUE).build();
-        patientVA1 = patientVA1.toBuilder().active(com.ibm.fhir.model.type.Boolean.FALSE).build();
+        patientVA2 = patientVA2.toBuilder().language(Code.of("en")).build();
+        patientVA1 = patientVA1.toBuilder().language(Code.of("en")).build();
 
         Bundle bundle = buildBundle(BundleType.BATCH);
         bundle = addRequestToBundle(null, bundle, HTTPVerb.PUT, "Patient/" + patientVA2.getId(), "W/\"1\"",
@@ -949,6 +949,11 @@ public void testBatchMixture() throws Exception {
         String method = "testBatchMixture";
         WebTarget target = getWebTarget();
 
+        // change at least one field so that the update below isn't skipped
+        patientB1 = patientB1.toBuilder()
+                .deceased(true)
+                .build();
+
         // Perform a mixture of request types.
         Bundle bundle = buildBundle(BundleType.BATCH);
         // create

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

@@ -58,7 +58,7 @@ public void testConcurrentUpdateCreate() throws Exception {
 
             // Prepare PatientUpdater instances for running each on its own thread.
             for (int i = 0; i < maxThreads; i++) {
-                concurrentUpdates.add(new PatientUpdater(patient));
+                concurrentUpdates.add(new PatientUpdater(patient, i));
             }
 
             // Run each PatientUpdater on its own thread.
@@ -107,8 +107,11 @@ public void testConcurrentUpdateCreate() throws Exception {
     private class PatientUpdater implements Callable<Patient> {
         private Patient patient;
 
-        PatientUpdater(Patient newPatient) {
-            this.patient = newPatient;
+        PatientUpdater(Patient newPatient, int counter) {
+            // the counter is used to ensure that each instance is unique
+            this.patient = newPatient.toBuilder()
+                    .multipleBirth(counter)
+                    .build();
         }
 
         @Override

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

@@ -96,13 +96,15 @@ public void testCreateEndpoint() throws Exception {
 
         // Add the endpoint to the resource registry.
         addToResourceRegistry("Endpoint", endpointId);
-        
+
         // Next, call the 'read' API to retrieve the new Endpoint and verify it.
         response = target.path("Endpoint/" + endpointId).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
 
         // Call the 'update' API.
-        endpoint = response.readEntity(Endpoint.class);
+        endpoint = response.readEntity(Endpoint.class).toBuilder()
+                .language(Code.of("en"))
+                .build();
         entity = Entity.entity(endpoint, FHIRMediaType.APPLICATION_FHIR_JSON);
         response = target.path("Endpoint/" + endpointId).request().put(entity, Response.class);
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -135,7 +137,7 @@ public void testCreateOrganization1() throws Exception {
 
         // Add the organization to the resource registry.
         addToResourceRegistry("Organization", organization1Id);
-        
+
         // Next, call the 'read' API to retrieve the new organization and verify it.
         response = target.path("Organization/" + organization1Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -162,7 +164,7 @@ public void testCreateOrganization2() throws Exception {
 
         // Add the organization to the resource registry.
         addToResourceRegistry("Organization", organization2Id);
-        
+
         // Next, call the 'read' API to retrieve the new organization and verify it.
         response = target.path("Organization/" + organization2Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -205,7 +207,7 @@ public void testCreatePatient1() throws Exception {
 
         // Add the patient to the resource registry.
         addToResourceRegistry("Patient", patient1Id);
-        
+
         // Next, call the 'read' API to retrieve the new patient and verify it.
         response = target.path("Patient/" + patient1Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -242,7 +244,7 @@ public void testCreatePatient2() throws Exception {
 
         // Add the patient to the resource registry.
         addToResourceRegistry("Patient", patient2Id);
-        
+
         // Next, call the 'read' API to retrieve the new patient and verify it.
         response = target.path("Patient/" + patient2Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -273,7 +275,7 @@ public void testCreateProcedure1() throws Exception {
 
         // Add the procedure to the resource registry.
         addToResourceRegistry("Procedure", procedure1Id);
-        
+
         // Next, call the 'read' API to retrieve the new procedure and verify it.
         response = target.path("Procedure/" + procedure1Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -306,7 +308,7 @@ public void testCreateProcedure2() throws Exception {
 
         // Add the procedure to the resource registry.
         addToResourceRegistry("Procedure", procedure2Id);
-        
+
         // Next, call the 'read' API to retrieve the new procedure and verify it.
         response = target.path("Procedure/" + procedure2Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -337,7 +339,7 @@ public void testCreateEncounter1() throws Exception {
 
         // Add the encounter to the resource registry.
         addToResourceRegistry("Encounter", encounter1Id);
-        
+
         // Next, call the 'read' API to retrieve the new encounter and verify it.
         response = target.path("Encounter/" + encounter1Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -368,7 +370,7 @@ public void testCreateEncounter2() throws Exception {
 
         // Add the encounter to the resource registry.
         addToResourceRegistry("Encounter", encounter2Id);
-        
+
         // Next, call the 'read' API to retrieve the new encounter and verify it.
         response = target.path("Encounter/" + encounter2Id).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -394,7 +396,7 @@ public void testCreateLocation() throws Exception {
 
         // Add the location to the resource registry.
         addToResourceRegistry("Location", locationId);
-        
+
         // Next, call the 'read' API to retrieve the new Location and verify it.
         response   = target.path("Location/" + locationId).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -419,7 +421,7 @@ public void testCreatePractitionerRole() throws Exception {
 
         // Add the practitionerRole to the resource registry.
         addToResourceRegistry("PractitionerRole", practitionerRoleId);
-        
+
         // Next, call the 'read' API to retrieve the new PractitionerRole and verify it.
         response   = target.path("PractitionerRole/" + practitionerRoleId).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -447,7 +449,7 @@ public void testCreateLibrary() throws Exception {
 
         // Add the library to the resource registry.
         addToResourceRegistry("Library", libraryId);
-        
+
         // Next, call the 'read' API to retrieve the new library and verify it.
         response = target.path("Library/" + libraryId).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -476,7 +478,7 @@ public void testCreateMeasure() throws Exception {
 
         // Add the measure to the resource registry.
         addToResourceRegistry("Measure", measureId);
-        
+
         // Next, call the 'read' API to retrieve the new measure and verify it.
         response = target.path("Measure/" + measureId).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -503,7 +505,7 @@ public void testCreateCarePlan() throws Exception {
 
         // Add the carePlan to the resource registry.
         addToResourceRegistry("CarePlan", carePlanId);
-        
+
         // Next, call the 'read' API to retrieve the new carePlan and verify it.
         response = target.path("CarePlan/" + carePlanId).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());
@@ -530,7 +532,7 @@ public void testCreateMeasureReport() throws Exception {
 
         // Add the measureReport to the resource registry.
         addToResourceRegistry("MeasureReport", measureReportId);
-        
+
         // Next, call the 'read' API to retrieve the new measureReport and verify it.
         response = target.path("MeasureReport/" + measureReportId).request(FHIRMediaType.APPLICATION_FHIR_JSON).get();
         assertResponse(response, Response.Status.OK.getStatusCode());

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

@@ -127,12 +127,13 @@ public void testCreateOrganization() throws Exception {
         Organization responseOrganization = response.readEntity(Organization.class);
         TestUtil.assertResourceEquals(organization, responseOrganization);
 
-        // Call the 'update' API.
+        // Call the 'update' API with force-update to ensure version 2 gets created.
         entity = Entity.entity(responseOrganization, FHIRMediaType.APPLICATION_FHIR_JSON);
         response =
                 target.path("Organization/" + organizationId).request()
                 .header("X-FHIR-TENANT-ID", tenantName)
                 .header("X-FHIR-DSID", dataStoreId)
+                .header("X-FHIR-FORCE-UPDATE", "true")
                 .put(entity, Response.class);
         assertResponse(response, Response.Status.OK.getStatusCode());
 
@@ -144,7 +145,7 @@ public void testCreateOrganization() throws Exception {
                 .get();
         assertResponse(response, Response.Status.OK.getStatusCode());
         organization = response.readEntity(Organization.class);
-        assertEquals("2", organization.getMeta().getVersionId().getValue());
+        assertEquals(organization.getMeta().getVersionId().getValue(), "2");
     }
 
     @Test(groups = { "server-search" }, dependsOnMethods = {"testCreateOrganization" })
@@ -1984,11 +1985,11 @@ public void testSearchPractitionerRoleWithValidVersionedOrganizationIncluded() {
         }
         assertNotNull(organization);
         assertNotNull(practitionerRole);
-        assertEquals(organizationId, organization.getId());
-        assertEquals("2", organization.getMeta().getVersionId().getValue());
-        assertEquals(practitionerRoleId, practitionerRole.getId());
-        assertEquals("Organization/"
-                + organizationId + "/_history/2", practitionerRole.getOrganization().getReference().getValue());
+        assertEquals(organization.getId(), organizationId);
+        assertEquals(organization.getMeta().getVersionId().getValue(), "2");
+        assertEquals(practitionerRole.getId(), practitionerRoleId);
+        assertEquals(practitionerRole.getOrganization().getReference().getValue(),
+                "Organization/" + organizationId + "/_history/2");
     }
 
     @Test(groups = { "server-search" }, dependsOnMethods = {"testCreatePractitionerRole" })
@@ -2018,11 +2019,11 @@ public void testSearchOrganizationWithPractitionerRoleValidVersionedReferenceRev
         }
         assertNotNull(organization);
         assertNotNull(practitionerRole);
-        assertEquals(organizationId, organization.getId());
-        assertEquals("2", organization.getMeta().getVersionId().getValue());
-        assertEquals(practitionerRoleId, practitionerRole.getId());
-        assertEquals("Organization/"
-                + organizationId + "/_history/2", practitionerRole.getOrganization().getReference().getValue());
+        assertEquals(organization.getId(), organizationId);
+        assertEquals(organization.getMeta().getVersionId().getValue(), "2");
+        assertEquals(practitionerRole.getId(), practitionerRoleId);
+        assertEquals(practitionerRole.getOrganization().getReference().getValue(),
+                "Organization/" + organizationId + "/_history/2");
     }
 
     @Test(groups = { "server-search" }, dependsOnMethods = {"testCreatePatient" })
@@ -2041,8 +2042,8 @@ public void testSearchOrganizationWithPatientInvalidVersionedReferenceRevinclude
         assertTrue(bundle.getEntry().size() == 1);
         Organization organization = (Organization) bundle.getEntry().get(0).getResource();
         assertNotNull(organization);
-        assertEquals(organizationId, organization.getId());
-        assertEquals("2", organization.getMeta().getVersionId().getValue());
+        assertEquals(organization.getId(), organizationId);
+        assertEquals(organization.getMeta().getVersionId().getValue(), "2");
     }
 
     @Test(groups = { "server-search" })
@@ -2075,7 +2076,7 @@ public void test_SearchCarePlan_APDate() throws Exception {
             }
         }
         assertNotNull(responseCarePlan);
-        assertEquals(carePlanId, responseCarePlan.getId());
+        assertEquals(responseCarePlan.getId(), carePlanId);
     }
 
     @Test(groups = { "server-search" }, dependsOnMethods = { "testCreateAllergyIntolerance" })
@@ -2092,7 +2093,7 @@ public void testSearchAllergyIntoleranceWithClinicalStatusIn() {
         Bundle bundle = response.readEntity(Bundle.class);
         assertNotNull(bundle);
         assertTrue(bundle.getEntry().size() == 1);
-        assertEquals(allergyIntoleranceId, bundle.getEntry().get(0).getResource().getId());
+        assertEquals(bundle.getEntry().get(0).getResource().getId(), allergyIntoleranceId);
     }
 
     @Test(groups = { "server-search" }, dependsOnMethods = { "testCreateAllergyIntolerance" })
@@ -2109,7 +2110,7 @@ public void testSearchAllergyIntoleranceWithClinicalStatusNotIn() {
         Bundle bundle = response.readEntity(Bundle.class);
         assertNotNull(bundle);
         assertTrue(bundle.getEntry().size() == 1);
-        assertEquals(allergyIntoleranceId, bundle.getEntry().get(0).getResource().getId());
+        assertEquals(bundle.getEntry().get(0).getResource().getId(), allergyIntoleranceId);
     }
 
     @Test(groups = { "server-search" }, dependsOnMethods = { "testCreateAllergyIntolerance" })
@@ -2158,7 +2159,7 @@ public void testSearchAllergyIntoleranceWithImplicitCodeSystemIn() {
         Bundle bundle = response.readEntity(Bundle.class);
         assertNotNull(bundle);
         assertTrue(bundle.getEntry().size() == 1);
-        assertEquals(allergyIntoleranceId, bundle.getEntry().get(0).getResource().getId());
+        assertEquals(bundle.getEntry().get(0).getResource().getId(), allergyIntoleranceId);
     }
 
     @Test(groups = { "server-search" })