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/src/main/java/com/ibm/fhir/server/rest/FHIRRestInteractionVisitorMeta.java

@@ -178,7 +178,7 @@ public FHIRRestOperationResponse doUpdate(int entryIndex, FHIRPersistenceEvent e
         return doInteraction(entryIndex, requestDescription, accumulatedTime, () -> {
             helpers.validateInteraction(Interaction.UPDATE, type);
 
-            FHIRRestOperationResponse metaResponse = helpers.doUpdateMeta(event, type, id, null, resource, ifMatchValue, searchQueryString, skippableUpdate, !DO_VALIDATION, warnings);
+            FHIRRestOperationResponse metaResponse = helpers.doUpdateMeta(event, type, id, null, resource, ifMatchValue, searchQueryString, skippableUpdate, ifNoneMatch, !DO_VALIDATION, warnings);
 
             // If the update was skippable we might be able to skip the future persistence step
             if (metaResponse.isCompleted()) {
@@ -219,7 +219,7 @@ public FHIRRestOperationResponse doPatch(int entryIndex, FHIRPersistenceEvent ev
         return doInteraction(entryIndex, requestDescription, accumulatedTime, () -> {
             // Validate that interaction is allowed for given resource type
             helpers.validateInteraction(Interaction.PATCH, type);
-            FHIRRestOperationResponse metaResponse = helpers.doUpdateMeta(event, type, id, patch, null, ifMatchValue, searchQueryString, skippableUpdate, !DO_VALIDATION, warnings);
+            FHIRRestOperationResponse metaResponse = helpers.doUpdateMeta(event, type, id, patch, null, ifMatchValue, searchQueryString, skippableUpdate, null, !DO_VALIDATION, warnings);
 
             // If the update was skippable we might be able to skip the future persistence step
             if (metaResponse.isCompleted()) {

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

@@ -458,7 +458,7 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
             // Do the first phase, which includes updating the meta in the resource
             FHIRPersistenceEvent event = new FHIRPersistenceEvent(newResource, buildPersistenceEventProperties(type, id, null, null, null));
             List<Issue> warnings = new ArrayList<>();
-            FHIRRestOperationResponse metaResponse = doUpdateMeta(event, type, id, patch, newResource, ifMatchValue, searchQueryString, skippableUpdate, doValidation, warnings);
+            FHIRRestOperationResponse metaResponse = doUpdateMeta(event, type, id, patch, newResource, ifMatchValue, searchQueryString, skippableUpdate, ifNoneMatch, doValidation, warnings);
             if (metaResponse.isCompleted()) {
                 // skip the update, so we can short-circuit here
                 txn.commit();
@@ -496,7 +496,7 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
 
     @Override
     public FHIRRestOperationResponse doUpdateMeta(FHIRPersistenceEvent event, String type, String id, FHIRPatch patch, Resource newResource,
-            String ifMatchValue, String searchQueryString, boolean skippableUpdate, boolean doValidation,
+            String ifMatchValue, String searchQueryString, boolean skippableUpdate, Integer ifNoneMatch, boolean doValidation,
             List<Issue> warnings) throws Exception {
         log.entering(this.getClass().getName(), "doUpdateMeta");
 
@@ -676,6 +676,13 @@ public FHIRRestOperationResponse doUpdateMeta(FHIRPersistenceEvent event, String
 
             // Perform the "version-aware" update check
             if (ior.getPrevResource() != null) {
+                if (ifNoneMatch != null && ifNoneMatch.equals(0)) {
+                    Integer existingVersion = Integer.parseInt(ior.getPrevResource().getMeta().getVersionId().getValue());
+                    handleIfNoneMatchExisted(type, id, ior, existingVersion);
+
+                    ior.setCompleted(true);
+                    return ior; // early exit, before firing any update events
+                }
                 performVersionAwareUpdateCheck(ior.getPrevResource(), ifMatchValue);
 
                 // In the case of a patch, we should not be updating meaninglessly.
@@ -701,7 +708,7 @@ public FHIRRestOperationResponse doUpdateMeta(FHIRPersistenceEvent event, String
                                 .build());
 
                         ior.setCompleted(true);
-                        return ior; // early exit, before firing any events
+                        return ior; // early exit, before firing any update events
                     }
                 }
             }
@@ -802,16 +809,7 @@ public FHIRRestOperationResponse doPatchOrUpdatePersist(FHIRPersistenceEvent eve
 
                     // Another thread snuck in and created the resource. Because the client requested
                     // If-None-Match, we skip any update and return 304 Not Modified.
-                    ior.setResource(null); // null, because we're in createOnUpdate
-                    ior.setLocationURI(FHIRUtil.buildLocationURI(type, id, result.getIfNoneMatchVersion()));
-                    Boolean ifNoneMatchNotModified = FHIRConfigHelper.getBooleanProperty(
-                        FHIRConfiguration.PROPERTY_IF_NONE_MATCH_RETURNS_NOT_MODIFIED, Boolean.FALSE);
-                    if (ifNoneMatchNotModified != null && ifNoneMatchNotModified) {
-                        // Don't treat as an error
-                        ior.setStatus(Response.Status.NOT_MODIFIED);
-                    } else {
-                        throw new FHIRPersistenceIfNoneMatchException("IfNoneMatch precondition failed.");
-                    }
+                    handleIfNoneMatchExisted(type, id, ior, result.getIfNoneMatchVersion());
                 } else {
                     ior.setStatus(Response.Status.CREATED);
                     ior.setLocationURI(FHIRUtil.buildLocationURI(type, newResource));
@@ -821,19 +819,9 @@ public FHIRRestOperationResponse doPatchOrUpdatePersist(FHIRPersistenceEvent eve
             } else {
                 // prevResource exists
                 if (result.getStatus() == InteractionStatus.IF_NONE_MATCH_EXISTED) {
-                    // Use the location assigned to the previous resource because we're
-                    // not updating anything. Also, we don't fire any 'after' event
-                    // for the same reason.
-                    ior.setResource(prevResource); // 304 Not Modified never needs to return content
-                    ior.setLocationURI(FHIRUtil.buildLocationURI(type, id, result.getIfNoneMatchVersion()));
-                    Boolean ifNoneMatchNotModified = FHIRConfigHelper.getBooleanProperty(
-                        FHIRConfiguration.PROPERTY_IF_NONE_MATCH_RETURNS_NOT_MODIFIED, Boolean.FALSE);
-                    if (ifNoneMatchNotModified != null && ifNoneMatchNotModified) {
-                        // Don't treat as an error
-                        ior.setStatus(Response.Status.NOT_MODIFIED);
-                    } else {
-                        throw new FHIRPersistenceIfNoneMatchException("IfNoneMatch precondition failed.");
-                    }
+                    // this should have been handled before the db interaction; this is just a double check
+                    log.warning("If-None-Match precondition check succeeded in REST layer but shouldn't have.");
+                    handleIfNoneMatchExisted(type, id, ior, result.getIfNoneMatchVersion());
                 } else {
                     // update, so make sure the location is configured correctly for the event
                     ior.setLocationURI(FHIRUtil.buildLocationURI(type, newResource));
@@ -872,6 +860,20 @@ public FHIRRestOperationResponse doPatchOrUpdatePersist(FHIRPersistenceEvent eve
         }
     }
 
+    private void handleIfNoneMatchExisted(String type, String id, FHIRRestOperationResponse ior,
+            Integer ifNoneMatchVersion) throws FHIRPersistenceIfNoneMatchException {
+        ior.setResource(null); // the resource shouldn't be needed for either case (302 or 412)
+        ior.setLocationURI(FHIRUtil.buildLocationURI(type, id, ifNoneMatchVersion));
+        Boolean ifNoneMatchNotModified = FHIRConfigHelper.getBooleanProperty(
+            FHIRConfiguration.PROPERTY_IF_NONE_MATCH_RETURNS_NOT_MODIFIED, Boolean.FALSE);
+        if (ifNoneMatchNotModified != null && ifNoneMatchNotModified) {
+            // Don't treat as an error
+            ior.setStatus(Response.Status.NOT_MODIFIED);
+        } else {
+            throw new FHIRPersistenceIfNoneMatchException("IfNoneMatch precondition failed.");
+        }
+    }
+
     /**
      * Check that the id and meta fields in the resource have been set up
      * @param resource
@@ -1760,6 +1762,8 @@ private void methodValidation(HTTPVerb method, Resource resource) throws FHIRPer
      *
      * @param currentResource
      *            the current latest version of the resource
+     * @param ifMatchValue
+     *            the string value of the If-Match header
      */
     private void performVersionAwareUpdateCheck(Resource currentResource, String ifMatchValue) throws FHIROperationException {
         if (ifMatchValue != null) {

operation/fhir-operation-erase/src/test/java/com/ibm/fhir/operation/erase/mock/MockFHIRResourceHelpers.java

@@ -213,8 +213,8 @@ public FHIRRestOperationResponse doCreatePersist(FHIRPersistenceEvent event, Lis
 
     @Override
     public 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 {
+            Resource newResource, String ifMatchValue, String searchQueryString, boolean skippableUpdate, Integer ifNoneMatch,
+            boolean doValidation, List<Issue> warnings) throws Exception {
         return null;
     }
 

operation/fhir-operation-member-match/src/test/java/com/ibm/fhir/operation/davinci/hrex/test/MemberMatchTest.java

@@ -1730,7 +1730,7 @@ public FHIRRestOperationResponse doCreatePersist(FHIRPersistenceEvent event, Lis
 
         @Override
         public 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 ifMatchValue, String searchQueryString, boolean skippableUpdate, Integer ifNoneMatch, boolean doValidation, List<Issue> warnings) throws Exception {
             throw new AssertionError("Unused");
         }