issue #2965 - modify getRequestBaseUri logic (#2966)

* issue #2965 - modify getRequestBaseUri logic

Previously, we looked for the last instnace of `/[resourceType]` in the
path and assumed that was the first thing after the baseUrl. However, if
a resourceId happened to match the resource type, this proved erroneous.

The updated logic looks for the first instance of `/[resourceType]/` in
the path instead.
If that doesn't exist, then we fall back to the old approach of using
the last index of `/[resourceType]` (now that we know it should be safe
to do so).

This updated logic could break if someone insisted on including a
baseUrl that includes a path segment that matches `/[resourceType]/`
(e.g. https://example.com/my/Patient/api/ ) but I think that is an
acceptable risk because that would be very dumb to do.

Alternatives would be to either
A. rely solely on a configured baseUrl; or
B. do more processing of the URL to ensure we're stripping a path and
not a hostname

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

* issue #2965 - document known limitation for custom base urls

and add a test to OriginalRequestRewriteServerTest for the exact
examples used in the docs

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

Author: lmsurpre

docs/src/pages/guides/FHIRServerUsersGuide.md

@@ -2049,7 +2049,7 @@ This section contains reference information about each of the configuration prop
 |`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/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.|
+|`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|
 |`fhirServer/term/capabilitiesUrl`|string|The URL that is embedded in the Terminology Capabilities statement using `mode=terminology`|
 |`fhirServer/term/disableCaching`|boolean|Indicates whether caching is disabled for the FHIR terminology module, this includes caching in `CodeSystemSupport`, `ValueSetSupport`, `GraphTermServiceProvider`, and `RemoteTermServiceProvider`|
@@ -2720,7 +2720,7 @@ IBM FHIR Server Supports the following custom HTTP Headers:
 |------------------|----------------------------|
 |`X-FHIR-TENANT-ID`|Specifies which tenant config should be used for the request. Default is `default`. The header name can be overridden via config property `fhirServer/core/tenantIdHeaderName`.|
 |`X-FHIR-DSID`|Specifies which datastore config should be used for the request. Default is `default`. The header name can be overridden via config property `fhirServer/core/dataSourceIdHeaderName`.|
-|`X-FHIR-FORWARDED-URL`|The original (user-facing) request URL; used for constructing absolute URLs within the server response. Only enabled when explicitly configured in the default fhir-server-config.json. If either the config property or the header itself is missing, the server will use the actual request URL. The header name can be overridden via config property `fhirServer/core/originalRequestUriHeaderName`. Note, `fhirServer/core/externalBaseUrl` overrides the `X-FHIR-FORWARDED-URL` and is used to construct the absolute URL.|
+|`X-FHIR-FORWARDED-URL`|The original (user-facing) request URL; used for constructing absolute URLs within the server response. Only enabled when explicitly configured in the default fhir-server-config.json. If either the config property or the header itself is missing, the server will use the actual request URL. The header name can be overridden via config property `fhirServer/core/originalRequestUriHeaderName`. Note that `fhirServer/core/externalBaseUrl` overrides the `X-FHIR-FORWARDED-URL` and is used to construct the absolute URL. Also note that the base URL's value 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.|
 |`X-FHIR-UPDATE-IF-MODIFIED`|When set to true, for update and patch requests, the server will perform a resource comparison and only perform the update if the contents of the resource have changed. For all other values, the update will be executed as normal.|
 
 # 6 Related topics

fhir-model/src/main/java/com/ibm/fhir/model/util/FHIRUtil.java

@@ -259,18 +259,14 @@ public static OperationOutcome buildOperationOutcome(String message, IssueType i
 
     /**
      * Builds a relative "Location" header value for the specified resource. This will be a string of the form
-     * <code>"<resource-type>/<id>/_history/<version>"</code>. Note that the server will turn this into an absolute URL prior to
+     * <code>"[resource-type]/[id]/_history/[version]"</code>. Note that the server will turn this into an absolute URL prior to
      * returning it to the client.
      *
      * @param resource
      *            the resource for which the location header value should be returned
      */
     public static URI buildLocationURI(String type, Resource resource) {
-        String resourceTypeName = resource.getClass().getSimpleName();
-        if (!resourceTypeName.equals(type)) {
-            resourceTypeName = type;
-        }
-        return URI.create(resourceTypeName + "/" + resource.getId() + "/_history/" + resource.getMeta().getVersionId().getValue());
+        return URI.create(type + "/" + resource.getId() + "/_history/" + resource.getMeta().getVersionId().getValue());
     }
 
     /**

fhir-persistence/src/main/java/com/ibm/fhir/persistence/FHIRPersistence.java

@@ -12,7 +12,6 @@
 
 import com.ibm.fhir.model.resource.OperationOutcome;
 import com.ibm.fhir.model.resource.Resource;
-import com.ibm.fhir.model.type.Instant;
 import com.ibm.fhir.persistence.context.FHIRPersistenceContext;
 import com.ibm.fhir.persistence.erase.EraseDTO;
 import com.ibm.fhir.persistence.exception.FHIRPersistenceException;
@@ -28,7 +27,7 @@ public interface FHIRPersistence {
 
     /**
      * Stores a new FHIR Resource in the datastore. Id assignment handled by the implementation.
-     * This method has been deprecated. Instead, generate the logical id first and use the 
+     * This method has been deprecated. Instead, generate the logical id first and use the
      * createWithMeta(context, resource) call instead.
      * @param context the FHIRPersistenceContext instance associated with the current request
      * @param resource the FHIR Resource instance to be created in the datastore
@@ -38,7 +37,7 @@ public interface FHIRPersistence {
      */
     @Deprecated
     <T extends Resource> SingleResourceResult<T> create(FHIRPersistenceContext context, T resource) throws FHIRPersistenceException;
-    
+
     /**
      * Stores a new FHIR Resource in the datastore. The resource is not modified before it is stored. It
      * must therefore already include correct Meta fields. Should be used instead of
@@ -142,7 +141,7 @@ default <T extends Resource> SingleResourceResult<T> delete(FHIRPersistenceConte
      * @throws FHIRPersistenceException
      */
     MultiResourceResult<Resource> search(FHIRPersistenceContext context, Class<? extends Resource> resourceType) throws FHIRPersistenceException;
-    
+
     /**
      * Returns true iff the persistence layer implementation supports transactions.
      */
@@ -167,7 +166,7 @@ default <T extends Resource> SingleResourceResult<T> delete(FHIRPersistenceConte
     default boolean isDeleteSupported() {
         return false;
     }
-    
+
     /**
      * Returns true iff the persistence layer implementation supports update/create and it has been
      * configured in the persistence config.

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

@@ -580,7 +580,7 @@ public void assertResponse(Response response, int expectedStatusCode) {
         assertNotNull(response);
         assertEquals(expectedStatusCode, response.getStatus());
     }
-    
+
     /**
      * Verify that the status code in the Response is of the expected status
      * family.
@@ -594,7 +594,7 @@ protected void assertResponse(Response response, Response.Status.Family family)
         if( ! response.getStatusInfo().getFamily().equals(family) ) {
             message = response.readEntity(String.class);
         }
-        assertEquals(message, response.getStatusInfo().getFamily(), family);
+        assertEquals(message, family, response.getStatusInfo().getFamily());
     }
 
     /**
@@ -667,10 +667,24 @@ protected void assertValidationOperationOutcome(OperationOutcome oo, String msgP
     // Misc. common functions used by testcases.
     //
 
+    /**
+     * Assert that the locationURI is formatted appropriately with the expected baseURL, resourceType, resourceId, and
+     * and versionId.
+     *
+     * @throws Exception
+     * @implNote Uses {@link #getRestBaseURL()} to construct the base URL for the expected location URI
+     */
+    public void validateLocationURI(String locationURI, String resourceType,
+            String expectedResourceId, String expectedVersionId) throws Exception {
+        String expectedLocation = getRestBaseURL() + "/" + resourceType + "/"
+                + expectedResourceId + "/_history/" + expectedVersionId;
+        assertEquals(expectedLocation, locationURI.toString());
+    }
+
     /**
      * For the specified response, this function will extract the logical id value
      * from the response's Location header. The format of a location header value
-     * should be: <code>[base]/<resource-type>/<id>/_history/<version></code>
+     * should be: <code>[base]/[resource-type]/[id]/_history/[version]</code>
      *
      * @param response the response object for a REST API invocation
      * @return the logical id value
@@ -734,20 +748,6 @@ private String getAbsoluteFilename(String filename) {
         return null;
     }
 
-    protected String[] getLocationURITokens(String locationURI) {
-        String[] temp = locationURI.split("/");
-        String[] tokens;
-        if (temp.length > 4) {
-            tokens = new String[4];
-            for (int i = 0; i < 4; i++) {
-                tokens[i] = temp[temp.length - 4 + i];
-            }
-        } else {
-            tokens = temp;
-        }
-        return tokens;
-    }
-
     protected Patient setUniqueFamilyName(Patient patient, String uniqueName) {
         List <HumanName> nameList = new ArrayList<HumanName>();
         for(HumanName humanName: patient.getName()) {
@@ -787,10 +787,10 @@ public void checkForIssuesWithValidation(Resource resource, boolean failOnValida
             }
 
             System.out.println("count = [" + issues.size() + "]");
-            assertEquals(nonWarning,0);
+            assertEquals(0, nonWarning);
 
             if(failOnWarning) {
-                assertEquals(allOtherIssues,0);
+                assertEquals(0, allOtherIssues);
             }
         }
         else {
@@ -799,9 +799,9 @@ public void checkForIssuesWithValidation(Resource resource, boolean failOnValida
     }
 
     /**
-     * Parses a location URI into the resourceType, resourceId, and (optionally) the version id.
+     * Parses a location URI into the resourceType, resource id, and version id.
      * @param location
-     * @return
+     * @return A string array with three parts:  resourceType, resourceId, and versionId (in that order)
      */
     public static String[] parseLocationURI(String location) {
         String[] result = null;
@@ -810,25 +810,15 @@ public static String[] parseLocationURI(String location) {
         }
 
         String[] tokens = location.split("/");
+        // 1      2  3              4           5   6  7       8  9        10
+        // https: // localhost:9443 fhir-server api v4 Patient id _history version
+        assertEquals(10, tokens.length);
+
         // Check if we should expect 4 tokens or only 2.
-        if (location.contains("_history")) {
-            if (tokens.length >= 4) {
-                result = new String[3];
-                result[0] = tokens[tokens.length - 4];
-                result[1] = tokens[tokens.length - 3];
-                result[2] = tokens[tokens.length - 1];
-            } else {
-                throw new IllegalArgumentException("Incorrect location value specified: " + location);
-            }
-        } else {
-            if (tokens.length >= 2) {
-                result = new String[2];
-                result[0] = tokens[tokens.length - 2];
-                result[1] = tokens[tokens.length - 1];
-            } else {
-                throw new IllegalArgumentException("Incorrect location value specified: " + location);
-            }
-        }
+        result = new String[3];
+        result[0] = tokens[tokens.length - 4];
+        result[1] = tokens[tokens.length - 3];
+        result[2] = tokens[tokens.length - 1];
         return result;
     }
 

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

@@ -97,4 +97,42 @@ public void testWholeSystemSearch_BaseUrlRewrite() throws Exception {
         assertTrue(responseBundle.getEntry().stream().allMatch(e -> e.getFullUrl().getValue().startsWith(ORIGINAL_URI + "/Patient")),
                 "All search response bundle entry fullUrls start with the passed in uri");
     }
+
+    /**
+     * Create a Patient and ensure the Location header reflects the OriginalRequestUri
+     */
+    @Test
+    public void testCreatePatientWithCustomBaseUrls() throws Exception {
+        WebTarget target = getWebTarget();
+        String customBaseUrl;
+        Patient patient = TestUtil.getMinimalResource(Patient.class);
+        Entity<Patient> entity = Entity.entity(patient, FHIRMediaType.APPLICATION_FHIR_JSON);
+        Response response;
+
+        // Build a new Patient and then call the 'create' API.
+        customBaseUrl = "https://example.com";
+        response = target.path("Patient").request()
+                .header("X-FHIR-Forwarded-URL", customBaseUrl + "/Patient")
+                .post(entity, Response.class);
+        assertResponse(response, Response.Status.CREATED.getStatusCode());
+        assertTrue(response.getLocation().toString().startsWith(customBaseUrl),
+                response.getLocation().toString() + " should begin with " + customBaseUrl);
+
+        customBaseUrl = "https://example.com/my/patient/api";
+        response = target.path("Patient").request()
+                .header("X-FHIR-Forwarded-URL", customBaseUrl + "/Patient")
+                .post(entity, Response.class);
+        assertResponse(response, Response.Status.CREATED.getStatusCode());
+        assertTrue(response.getLocation().toString().startsWith(customBaseUrl),
+                response.getLocation().toString() + " should begin with " + customBaseUrl);
+
+        customBaseUrl = "https://example.com/my/Patient/api";
+        response = target.path("Patient").request()
+                .header("X-FHIR-Forwarded-URL", customBaseUrl + "/Patient")
+                .post(entity, Response.class);
+        assertResponse(response, Response.Status.CREATED.getStatusCode());
+        assertFalse(response.getLocation().toString().startsWith(customBaseUrl),
+                response.getLocation().toString() + " doesn't begin with " + customBaseUrl
+                + " because of a documented limitation");
+    }
 }

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

@@ -661,7 +661,7 @@ public void testConditionalUpdateObservation2() throws Exception {
         String obsId = UUID.randomUUID().toString();
         Observation obs = TestUtil.readLocalResource("Observation1.json");
         obs = obs.toBuilder()
-                .subject(Reference.builder().reference(string(fakePatientRef)).build())
+                .subject(Reference.builder().reference(fakePatientRef).build())
                 .build();
 
         // First conditional update should find no matches, so we should get back a 201
@@ -673,11 +673,11 @@ public void testConditionalUpdateObservation2() throws Exception {
         String locationURI = response.getLocation();
         assertNotNull(locationURI);
 
-        String[] tokens = parseLocationURI(locationURI);
-        String resourceId = tokens[1];
+        // get the server-assigned resource id from the location header
+        String resourceId = getLocationLogicalId(response.getResponse());
 
-        // Second conditional update should find 1 match, but because there is a un-matching
-        // resourceId in the input resource, so we should get back a 400 error.
+        // Second conditional update should find 1 match, but because there is an un-matching
+        // resourceId in the input resource, we should get back a 400 error.
         query = new FHIRParameters().searchParam("_id", resourceId);
         obs = obs.toBuilder().id(obsId).build();
         response = client.conditionalUpdate(obs, query);