issue 2050 ifNoneMatch returns 412 unless ifNoneMatchReturnsNotModified (#2978)

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

Author: punktilious

docs/src/pages/Conformance.md

@@ -58,7 +58,8 @@ The IBM FHIR Server supports a custom header, `X-FHIR-UPDATE-IF-MODIFIED`, for c
 The IBM FHIR Server also supports conditional create-on-update using the `If-None-Match` header. This IBM FHIR Server-specific feature allows clients to use a `PUT` (update) interaction which behaves as follows:
 
     1. `If-None-Match: "*"`: If the resource does not yet exist, create the resource and return `201 Created`;
-    2. `If-None-Match: "*"`: If the resource does exist, do nothing and return `304 Not Modified`.
+    2. `If-None-Match: "*"`: If the resource does exist, do nothing and return `412 Precondition Failed` (default behavior);
+    3. `If-None-Match: "*"`: If the resource does exist and the fhir-server-config element `fhirServer/core/ifNoneMatchReturnsNotModified` is set to `true`, do nothing and return `304 Not Modified`.
 
 The only supported value for If-None-Match conditional create-on-update is `"*"`. This feature can also be used for `PUT` requests in transaction or batch bundles by specifying the `ifNoneMatch` field similarly in the request element:
 ```
@@ -88,7 +89,9 @@ The only supported value for If-None-Match conditional create-on-update is `"*"`
 
 ```
 
-If a match is found, the response bundle entry contains the location of the current resource and a status of `304` (Not Modified):
+If a match is found and the fhir-server-config element `fhirServer/core/ifNoneMatchReturnsNotModified` is not configured or is set to `false`, the condition is treated as an error which will cause transaction bundles to fail, returning a status of `400` (Bad Request). For batch bundles, the entry response status will be `412` (Precondition Failed).
+
+If a match is found and the fhir-server-config element `fhirServer/core/ifNoneMatchReturnsNotModified` is set to `true`, the response bundle entry contains the location of the current resource and a response status of `304` (Not Modified):
 ```
 {
   "resourceType": "Bundle",

docs/src/pages/guides/FHIRServerUsersGuide.md

@@ -2048,6 +2048,7 @@ This section contains reference information about each of the configuration prop
 |`fhirServer/core/defaultPageSize`|integer|Sets the page size for search and history request results when no `_count` parameter is specified.|
 |`fhirServer/core/maxPageSize`|integer|Sets the maximum page size for search and history request results. If a user-specified `_count` parameter value exceeds the maximum page size, then a warning is logged and the maximum page size will be used.|
 |`fhirServer/core/maxPageIncludeCount`|integer|Sets the maximum number of 'include' resources allowed per page for search and history request results. If the number of 'include' resources returned for a page of results from a search or history request will exceed the maximum number of 'include' resources allowed per page, then an error will be returned in the request results.|
+|`fhirServer/core/ifNoneMatchReturnsNotModified`|boolean|When If-None-Match is specified, overrides the standard return status "412 Precondition Failed" to be "304 Not Modified". Useful in transaction bundles for clients not wanting the bundle to fail when a conflict is found.|
 |`fhirServer/core/capabilitiesUrl`|string|The URL that is embedded in the default Capabilities statement|
 |`fhirServer/core/externalBaseUrl`|string|The base URL that is embedded in the Search bundle response, as of version 4.9.0. Note that the base URL must not include a path segment that matches any FHIR resource type name (case-sensitive). For example, "https://example.com" or "https://example.com/my/patient/api" are fine, but "https://example.com/my/Patient/api" is not.|
 |`fhirServer/validation/failFast`|boolean|Indicates whether validation should fail fast on create and update interactions|
@@ -2233,6 +2234,7 @@ This section contains reference information about each of the configuration prop
 |`fhirServer/core/maxPageIncludeCount`|1000|
 |`fhirServer/core/capabilitiesUrl`|null|
 |`fhirServer/core/externalBaseUrl`|null|
+|`fhirServer/core/ifNoneMatchReturnsNotModified`|false|
 |`fhirServer/validation/failFast`|false|
 |`fhirServer/term/capabilitiesUrl`|null|
 |`fhirServer/term/cachingDisabled`|false|
@@ -2382,6 +2384,7 @@ must restart the server for that change to take effect.
 |`fhirServer/core/extendedCodeableConceptValidation`|N|N|
 |`fhirServer/core/disabledOperations`|N|N|
 |`fhirServer/core/defaultPageSize`|Y|Y|
+|`fhirServer/core/ifNoneMatchReturnsNotModified`|Y|Y|
 |`fhirServer/core/maxPageSize`|Y|Y|
 |`fhirServer/core/maxPageIncludeCount`|Y|Y|
 |`fhirServer/core/capabilitiesUrl`|Y|Y|

fhir-config/src/main/java/com/ibm/fhir/config/FHIRConfiguration.java

@@ -33,6 +33,7 @@ public class FHIRConfiguration {
     public static final String PROPERTY_CHECK_REFERENCE_TYPES = "fhirServer/core/checkReferenceTypes";
     public static final String PROPERTY_CHECK_CONTROL_CHARS = "fhirServer/core/checkControlCharacters";
     public static final String PROPERTY_CONDITIONAL_DELETE_MAX_NUMBER = "fhirServer/core/conditionalDeleteMaxNumber";
+    public static final String PROPERTY_IF_NONE_MATCH_RETURNS_NOT_MODIFIED = "fhirServer/core/ifNoneMatchReturnsNotModified";
     public static final String PROPERTY_SERVER_REGISTRY_RESOURCE_PROVIDER_ENABLED = "fhirServer/core/serverRegistryResourceProviderEnabled";
     public static final String PROPERTY_SERVER_RESOLVE_FUNCTION_ENABLED = "fhirServer/core/serverResolveFunctionEnabled";
     public static final String PROPERTY_CAPABILITY_STATEMENT_CACHE = "fhirServer/core/capabilityStatementCacheTimeout";

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

@@ -0,0 +1,27 @@
+/*
+ * (C) Copyright IBM Corp. 2021
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package com.ibm.fhir.persistence.exception;
+
+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). 
+ * See FHIRConfiguration.PROPERTY_IF_NONE_MATCH_RETURNS_NOT_MODIFIED.
+ */
+public class FHIRPersistenceIfNoneMatchException extends FHIRPersistenceException {
+
+    // Generated serialVersionUID
+    private static final long serialVersionUID = -6078163237462666409L;
+
+    public FHIRPersistenceIfNoneMatchException(String message) {
+        super(message);
+        withIssue(FHIRUtil.buildOperationOutcomeIssue(getMessage(), IssueType.CONFLICT));
+    }
+}

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

@@ -2514,7 +2514,7 @@ public void testTransactionBundleWithSkippableUpdates() throws Exception {
      * Requests:
      * <pre>
      *   1. Transaction Bundle with PUT Patient/randomId (create-on-update - 201 Created)
-     *   2. Transaction Bundle with PUT Patient/randomId (skip update - 304 Not Modified)
+     *   2. Transaction Bundle with PUT Patient/randomId (skip update - 400 error)
      *   3. DELETE Patient/randomId (deleted - 200 OK)
      *   4. Transaction Bundle with PUT Patient/randomId (create-on-update - 201 Created)
      * </pre>
@@ -2561,7 +2561,8 @@ public void testTransactionBundleWithIfNoneMatch() throws Exception {
         assertEquals(entry1.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
 
 
-        // Interaction 2. PUT the same patient again. Should be skipped because IfNoneMatch
+        // 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()
                 .fullUrl(Uri.of("urn:2"))
                 .resource(patient)
@@ -2576,17 +2577,11 @@ public void testTransactionBundleWithIfNoneMatch() throws Exception {
         // Process bundle
         FHIRResponse response2 = client.transaction(requestBundle, returnPref);
         assertNotNull(response2);
-        assertResponse(response2.getResponse(), Response.Status.OK.getStatusCode());
-        Bundle responseBundle2 = response2.getResource(Bundle.class);
-        printBundle("PUT", "response", responseBundle2);
+        assertResponse(response2.getResponse(), Response.Status.BAD_REQUEST.getStatusCode());
+        OperationOutcome operationOutcome2 = response2.getResource(OperationOutcome.class);
 
         // Validate results
-        assertNotNull(responseBundle2);
-        assertEquals(responseBundle2.getEntry().size(), 1);
-        Bundle.Entry entry2 = responseBundle2.getEntry().get(0);
-        assertEquals(entry2.getResponse().getStatus().getValue(), "304");
-        assertEquals(entry2.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
-        // 304 does not include a response body, so can't check the response resource
+        assertNotNull(operationOutcome2);
 
         // Interaction 3. DELETE the patient
         FHIRResponse response3 = client.delete(Patient.class.getSimpleName(), randomId);
@@ -2629,10 +2624,10 @@ public void testTransactionBundleWithIfNoneMatch() throws Exception {
      * multiple requests because:
      *   "A resource can only appear in a transaction once (by identity)."
      * Requests:
-     *   1. Transaction Bundle with PUT Patient/randomId (create-on-update - 201 Created)
-     *   2. Transaction Bundle with PUT Patient/randomId (skip update - 304 Not Modified)
+     *   1. Batch Bundle with PUT Patient/randomId (create-on-update - 201 Created)
+     *   2. Batch Bundle with PUT Patient/randomId (skip update - 412 Precondition Failed)
      *   3. DELETE Patient/randomId (deleted - 200 OK)
-     *   4. Transaction Bundle with PUT Patient/randomId (create-on-update - 201 Created)
+     *   4. Batch Bundle with PUT Patient/randomId (create-on-update - 201 Created)
      */
     @Test
     public void testBatchBundleWithIfNoneMatch() throws Exception {
@@ -2660,7 +2655,7 @@ public void testBatchBundleWithIfNoneMatch() throws Exception {
 
         // Process bundle
         FHIRRequestHeader returnPref = FHIRRequestHeader.header(PREFER_HEADER_NAME, PREFER_HEADER_RETURN_REPRESENTATION);
-        FHIRResponse response = client.transaction(requestBundle, returnPref);
+        FHIRResponse response = client.batch(requestBundle, returnPref);
         assertNotNull(response);
         assertResponse(response.getResponse(), Response.Status.OK.getStatusCode());
         Bundle responseBundle = response.getResource(Bundle.class);
@@ -2689,7 +2684,7 @@ public void testBatchBundleWithIfNoneMatch() throws Exception {
                 .build();
 
         // Process bundle
-        FHIRResponse response2 = client.transaction(requestBundle, returnPref);
+        FHIRResponse response2 = client.batch(requestBundle, returnPref);
         assertNotNull(response2);
         assertResponse(response2.getResponse(), Response.Status.OK.getStatusCode());
         Bundle responseBundle2 = response2.getResource(Bundle.class);
@@ -2699,9 +2694,7 @@ public void testBatchBundleWithIfNoneMatch() throws Exception {
         assertNotNull(responseBundle2);
         assertEquals(responseBundle2.getEntry().size(), 1);
         Bundle.Entry entry2 = responseBundle2.getEntry().get(0);
-        assertEquals(entry2.getResponse().getStatus().getValue(), "304");
-        assertEquals(entry2.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
-        // 304 does not include a response body, so can't check the response resource
+        assertEquals(entry2.getResponse().getStatus().getValue(), "412");
 
         // Interaction 3. DELETE the patient
         FHIRResponse response3 = client.delete(Patient.class.getSimpleName(), randomId);
@@ -2721,7 +2714,7 @@ public void testBatchBundleWithIfNoneMatch() throws Exception {
                 .build();
 
         // Process bundle
-        FHIRResponse response4 = client.transaction(requestBundle, returnPref);
+        FHIRResponse response4 = client.batch(requestBundle, returnPref);
         assertNotNull(response4);
         assertResponse(response4.getResponse(), Response.Status.OK.getStatusCode());
         Bundle responseBundle4 = response4.getResource(Bundle.class);

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

@@ -43,8 +43,9 @@ public void testCreateOnUpdate() throws Exception {
             final FHIRRequestHeader ifNoneMatch = new FHIRRequestHeader(HEADERNAME_IF_NONE_MATCH, "*");
             response = client.update(patient, ifNoneMatch);
             status = response.getStatus();
-            assertEquals(status, 304);
-            assertEquals(response.getETag(), "W/\"1\""); // not updated
+            
+            // by default, hitting If-None-Match is considered a 412 Precondition Failed error
+            assertEquals(status, 412);
 
             // Read back the patient and make sure it is still at version 1
             response = client.read(Patient.class.getSimpleName(), patientLogicalId);

fhir-server/src/main/java/com/ibm/fhir/server/resources/Update.java

@@ -33,6 +33,7 @@
 import com.ibm.fhir.exception.FHIROperationException;
 import com.ibm.fhir.model.resource.Resource;
 import com.ibm.fhir.model.type.code.IssueType;
+import com.ibm.fhir.persistence.exception.FHIRPersistenceIfNoneMatchException;
 import com.ibm.fhir.persistence.exception.FHIRPersistenceResourceNotFoundException;
 import com.ibm.fhir.server.spi.operation.FHIRRestOperationResponse;
 import com.ibm.fhir.server.util.FHIRRestHelper;
@@ -98,6 +99,10 @@ public Response update(@PathParam("type") String type, @PathParam("id") String i
             // By default, NOT_FOUND is mapped to HTTP 404, so explicitly set it to HTTP 405
             status = Status.METHOD_NOT_ALLOWED;
             return exceptionResponse(e, status);
+        } catch (FHIRPersistenceIfNoneMatchException e) {
+            // IfNoneMatch being treated as an error (default behavior)
+            status = Status.PRECONDITION_FAILED;
+            return exceptionResponse(e, status);
         } catch (FHIROperationException e) {
             status = issueListToStatus(e.getIssues());
             return exceptionResponse(e, status);

fhir-server/src/main/java/com/ibm/fhir/server/rest/FHIRRestInteractionVisitorBase.java

@@ -12,20 +12,26 @@
 import static javax.servlet.http.HttpServletResponse.SC_GONE;
 import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
 import static javax.servlet.http.HttpServletResponse.SC_OK;
+import static javax.servlet.http.HttpServletResponse.SC_PRECONDITION_FAILED;
 
 import java.net.URI;
+import java.util.Collections;
+import java.util.List;
 import java.util.Map;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 import com.ibm.fhir.config.FHIRRequestContext;
 import com.ibm.fhir.core.HTTPReturnPreference;
 import com.ibm.fhir.exception.FHIROperationException;
 import com.ibm.fhir.model.resource.Bundle.Entry;
+import com.ibm.fhir.model.resource.OperationOutcome.Issue;
 import com.ibm.fhir.model.resource.OperationOutcome;
 import com.ibm.fhir.model.resource.Resource;
 import com.ibm.fhir.model.type.Uri;
 import com.ibm.fhir.model.util.ModelSupport;
+import com.ibm.fhir.server.exception.FHIRRestBundledRequestException;
 import com.ibm.fhir.server.spi.operation.FHIRResourceHelpers;
 import com.ibm.fhir.server.spi.operation.FHIRRestOperationResponse;
 
@@ -42,6 +48,7 @@ public abstract class FHIRRestInteractionVisitorBase implements FHIRRestInteract
     protected static final com.ibm.fhir.model.type.String SC_NOT_FOUND_STRING = string(Integer.toString(SC_NOT_FOUND));
     protected static final com.ibm.fhir.model.type.String SC_ACCEPTED_STRING = string(Integer.toString(SC_ACCEPTED));
     protected static final com.ibm.fhir.model.type.String SC_OK_STRING = string(Integer.toString(SC_OK));
+    protected static final com.ibm.fhir.model.type.String SC_PRECONDITION_FAILED_STRING = string(Integer.toString(SC_PRECONDITION_FAILED));
 
     // the helper we use to do most of the heavy lifting
     protected final FHIRResourceHelpers helpers;
@@ -158,4 +165,22 @@ protected void addLocalRefMapping(String localIdentifier, Resource resource) {
             }
         }
     }
+    
+    /**
+     * Wrap the cause with a FHIRRestbundledRequestException and update each issue with
+     * the entryIndex before throwing.
+     * 
+     * @param entryIndex
+     * @param cause
+     * @throws FHIROperationException
+     */
+    protected void updateIssuesWithEntryIndexAndThrow(Integer entryIndex, FHIROperationException cause) throws FHIROperationException {
+        String msg = "Error while processing request bundle on entry " + entryIndex;
+        List<Issue> updatedIssues = cause.getIssues().stream()
+                .map(i -> i.toBuilder().expression(string("Bundle.entry[" + entryIndex + "]")).build())
+                .collect(Collectors.toList());
+        // no need to keep the issues in the cause any more since we've "promoted" them to the wrapped exception
+        cause.withIssue(Collections.emptyList());
+        throw new FHIRRestBundledRequestException(msg, cause).withIssue(updatedIssues);
+    }
 }

fhir-server/src/main/java/com/ibm/fhir/server/rest/FHIRRestInteractionVisitorMeta.java

@@ -439,14 +439,4 @@ private FHIRRestOperationResponse doInteraction(int entryIndex, String requestDe
 
         return null;
     }
-
-    private void updateIssuesWithEntryIndexAndThrow(Integer entryIndex, FHIROperationException cause) throws FHIROperationException {
-        String msg = "Error while processing request bundle on entry " + entryIndex;
-        List<Issue> updatedIssues = cause.getIssues().stream()
-                .map(i -> i.toBuilder().expression(string("Bundle.entry[" + entryIndex + "]")).build())
-                .collect(Collectors.toList());
-        // no need to keep the issues in the cause any more since we've "promoted" them to the wrapped exception
-        cause.withIssue(Collections.emptyList());
-        throw new FHIRRestBundledRequestException(msg, cause).withIssue(updatedIssues);
-    }
 }