Skip to content

Commit

Permalink
issue #3501 - use absolute URLs in the system/type history response
Browse files Browse the repository at this point in the history 
For both `Entry.fullUrl` and `Entry.response.location`.

I also added the etag element to the Entry.response objects to be more
consistent with the instance-level history implementation.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Mar 24, 2022
1 parent 5f97f94 commit 4271a48
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 15 deletions.
4 changes: 1 addition & 3 deletions fhir-search/src/test/java/com/ibm/fhir/search/test/TypeParameterParseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@
import com.ibm.fhir.search.context.FHIRSearchContext;

/**
* This testng test class contains methods that test the parsing of the search result _type parameter in the
* SearchUtil class.
* Test the parsing of the search result _type parameter in the SearchUtil class.
*/
public class TypeParameterParseTest extends BaseSearchTest {

Expand Down Expand Up @@ -75,7 +74,6 @@ public void testTypeNonSystemSearch_strict() throws Exception {
} catch(Exception ex) {
isExceptionThrown = true;
assertEquals(ex.getMessage(), "_type parameter is only supported for whole-system search");

}
assertTrue(isExceptionThrown);
}
Expand Down
20 changes: 16 additions & 4 deletions fhir-server-test/src/test/java/com/ibm/fhir/server/test/SystemHistoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.ibm.fhir.client.FHIRResponse;
import com.ibm.fhir.core.FHIRMediaType;
import com.ibm.fhir.model.resource.Bundle;
import com.ibm.fhir.model.resource.Bundle.Entry;
import com.ibm.fhir.model.resource.Bundle.Link;
import com.ibm.fhir.model.resource.Observation;
import com.ibm.fhir.model.resource.OperationOutcome;
Expand Down Expand Up @@ -151,7 +152,7 @@ public void testSystemHistoryWithTypePatient() throws Exception {
for (Bundle.Entry be: bundle.getEntry()) {
// simple way to see if our patient has appeared
String fullUrl = be.getFullUrl().getValue();
if (fullUrl.contains("Patient/" + patientId)) {
if (fullUrl.equals(getRestBaseURL() + "Patient/" + patientId)) {
found = true;
}
}
Expand Down Expand Up @@ -225,6 +226,17 @@ public void testSystemHistoryWithNoType() throws Exception {
assertNotNull(bundle);
assertNotNull(bundle.getEntry());
assertTrue(bundle.getEntry().size() >= 5);

for (Entry entry : bundle.getEntry()) {
String fullUrl = entry.getFullUrl().getValue();
Resource resource = entry.getResource();
if (resource == null) {
assertTrue(fullUrl.startsWith(getRestBaseURL()));
} else {
assertEquals(fullUrl, getRestBaseURL() + "/" +
resource.getClass().getSimpleName() + "/" + resource.getId());
}
}
}

@Test(dependsOnMethods = {"populateResourcesForHistory"})
Expand Down Expand Up @@ -280,7 +292,7 @@ public void testSystemHistoryWithTypePatientAndOrderNone() throws Exception {
for (Bundle.Entry be: bundle.getEntry()) {
// simple way to see if our patient has appeared
String fullUrl = be.getFullUrl().getValue();
if (fullUrl.contains("Patient/" + patientId)) {
if (fullUrl.equals(getRestBaseURL() + "Patient/" + patientId)) {
found = true;
}

Expand Down Expand Up @@ -358,7 +370,7 @@ public void testSystemHistoryWithTypePatientAndOrderASC() throws Exception {
for (Bundle.Entry be: bundle.getEntry()) {
// simple way to see if our patient has appeared
String fullUrl = be.getFullUrl().getValue();
if (fullUrl.contains("Patient/" + patientId)) {
if (fullUrl.equals(getRestBaseURL() + "Patient/" + patientId)) {
found = true;
}

Expand Down Expand Up @@ -435,7 +447,7 @@ public void testSystemHistoryWithTypePatientAndOrderDESC() throws Exception {
for (Bundle.Entry be: bundle.getEntry()) {
// simple way to see if our patient has appeared
String fullUrl = be.getFullUrl().getValue();
if (fullUrl.contains("Patient/" + patientId)) {
if (fullUrl.equals(getRestBaseURL() + "Patient/" + patientId)) {
found = true;
}

Expand Down
18 changes: 10 additions & 8 deletions fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3055,8 +3055,7 @@ public Bundle doHistory(MultivaluedMap<String, String> queryParameters, String r
}
}
} catch (FHIRPersistenceDataAccessException x) {
log.log(Level.SEVERE, "Error reading history; params = {" + historyContext + "}",
x);
log.log(Level.SEVERE, "Error reading history; params = {" + historyContext + "}", x);
throw x;
} finally {
txn.end();
Expand Down Expand Up @@ -3096,29 +3095,32 @@ public Bundle doHistory(MultivaluedMap<String, String> queryParameters, String r
Entry.Response.Builder responseBuilder = Entry.Response.builder();
switch (changeRecord.getChangeType()) {
case CREATE:
requestBuilder.method(HTTPVerb.POST);
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 Created"));
responseBuilder.status(com.ibm.fhir.model.type.String.of("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 OK"));
responseBuilder.status(com.ibm.fhir.model.type.String.of("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 OK"));
responseBuilder.status(com.ibm.fhir.model.type.String.of("200"));
break;
}

String fullUrl = getRequestBaseUri(resourceType) + "/" + changeRecord.getResourceTypeName() + "/" + changeRecord.getLogicalId();

responseBuilder.lastModified(com.ibm.fhir.model.type.Instant.of(changeRecord.getChangeTstamp().atZone(UTC)));
responseBuilder.location(Url.of(changeRecord.getResourceTypeName() + "/" + changeRecord.getLogicalId() + "/_history/" + changeRecord.getVersionId()));
responseBuilder.etag("W/\"" + changeRecord.getVersionId() + "\"");
responseBuilder.location(Url.of(fullUrl + "/_history/" + changeRecord.getVersionId()));

// Per the R4 spec, the fullUrl should not contain _history/:vid
Entry.Builder entryBuilder = Entry.builder();
entryBuilder.id(Long.toString(changeRecord.getChangeId()));
entryBuilder.fullUrl(Url.of(changeRecord.getResourceTypeName() + "/" + changeRecord.getLogicalId()));
entryBuilder.fullUrl(Url.of(fullUrl));
entryBuilder.request(requestBuilder.build());
entryBuilder.response(responseBuilder.build());

Expand Down

0 comments on commit 4271a48

Please sign in to comment.