Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue #2284 - support skippable updates for batch/transaction methods #2295

Merged
merged 2 commits into from
Apr 28, 2021
Merged

issue #2284 - support skippable updates for batch/transaction methods #2295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2fb9e2
issue #2284 - support skippable updates for batch/transaction methods
lmsurpre Apr 28, 2021
17fdd43
Update testcase description
lmsurpre Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions fhir-path/src/main/java/com/ibm/fhir/path/util/FHIRPathUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,11 @@ public static <T extends Visitable> T replace(T elementOrResource, String fhirPa

FHIRPathTree tree = evaluator.getEvaluationContext().getTree();
FHIRPathNode parentNode = tree.getParent(node);

if (parentNode == null) {
throw new FHIRPatchException("Unable to compute the parent for '" + elementName + "';" +
" a FHIRPathPatch replace FHIRPath must select a node with a parent", fhirPath);
}
Comment on lines +909 to +912
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a somewhat unrelated change that might merit its own separate issue, but i found this NPE bug when I added a patch test and got the patch wrong

Visitable parent = parentNode.isResourceNode() ?
parentNode.asResourceNode().resource() : parentNode.asElementNode().element();

Expand Down
93 changes: 92 additions & 1 deletion fhir-server-test/src/test/java/com/ibm/fhir/server/test/BundleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

import static com.ibm.fhir.model.test.TestUtil.isResourceInResponse;
import static com.ibm.fhir.model.type.String.string;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
import static org.testng.AssertJUnit.assertEquals;
import static org.testng.AssertJUnit.assertTrue;

import java.util.ArrayList;
Expand Down Expand Up @@ -45,6 +45,7 @@
import com.ibm.fhir.model.resource.Practitioner;
import com.ibm.fhir.model.resource.Resource;
import com.ibm.fhir.model.test.TestUtil;
import com.ibm.fhir.model.type.Code;
import com.ibm.fhir.model.type.Extension;
import com.ibm.fhir.model.type.HumanName;
import com.ibm.fhir.model.type.Reference;
Expand Down Expand Up @@ -106,6 +107,8 @@ public class BundleTest extends FHIRServerTestBase {
private static final String PREFER_HEADER_RETURN_REPRESENTATION = "return=representation";
private static final String PREFER_HEADER_NAME = "Prefer";

private static final String UPDATE_IF_MODIFIED_HEADER_NAME = "X-FHIR-UPDATE-IF-MODIFIED";

/**
* Retrieve the server's conformance statement to determine the status of
* certain runtime options.
Expand Down Expand Up @@ -2331,6 +2334,94 @@ public void testTransactionConditionalDeletes() throws Exception {
assertGoodGetResponse(responseBundle.getEntry().get(1), Status.OK.getStatusCode(), HTTPReturnPreference.MINIMAL);
}

/**
* Sets UPDATE_IF_MODIFIED_HEADER_NAME and posts a transaction bundle with an update and a patch; both should be skipped on the server
* Procedure has local reference to Patient.
*/
@Test
public void testTransactionBundleWithSkippableUpdates() throws Exception {
String randomId = UUID.randomUUID().toString();
Patient patient = Patient.builder()
.id(randomId)
.active(com.ibm.fhir.model.type.Boolean.TRUE)
.build();
Bundle.Entry.Request bundleEntryRequest = Bundle.Entry.Request.builder()
.method(HTTPVerb.PUT)
.url(Uri.of("Patient/"+randomId))
.build();
Bundle.Entry bundleEntry = Bundle.Entry.builder()
.fullUrl(Uri.of("urn:1"))
.resource(patient)
.request(bundleEntryRequest)
.build();
Bundle.Entry bundleEntry2 = Bundle.Entry.builder()
.fullUrl(Uri.of("urn:2"))
.resource(patient)
.request(bundleEntryRequest)
.build();

Bundle.Entry.Request patchRequest = Bundle.Entry.Request.builder()
.method(HTTPVerb.PATCH)
.url(Uri.of("Patient/"+randomId))
.build();
Parameters nopPatch = Parameters.builder()
.parameter(Parameter.builder()
.name(string("operation"))
.part(Parameter.builder()
.name(string("type"))
.value(Code.of("replace"))
.build())
.part(Parameter.builder()
.name(string("path"))
.value(string("Patient.active"))
.build())
.part(Parameter.builder()
.name(string("value"))
.value(com.ibm.fhir.model.type.Boolean.TRUE)
.build())
.build())
.build();
Bundle.Entry bundleEntry3 = Bundle.Entry.builder()
.fullUrl(Uri.of("urn:3"))
.resource(nopPatch)
.request(patchRequest)
.build();

Bundle requestBundle = Bundle.builder()
.id("bundle1")
.type(BundleType.TRANSACTION)
.entry(bundleEntry, bundleEntry2, bundleEntry3)
.build();

// Process bundle
FHIRRequestHeader returnPref = FHIRRequestHeader.header(PREFER_HEADER_NAME, PREFER_HEADER_RETURN_REPRESENTATION);
FHIRRequestHeader updateOnlyIfModified = FHIRRequestHeader.header(UPDATE_IF_MODIFIED_HEADER_NAME, true);
FHIRResponse response = client.transaction(requestBundle, returnPref, updateOnlyIfModified);
assertNotNull(response);
assertResponse(response.getResponse(), Response.Status.OK.getStatusCode());
Bundle responseBundle = response.getResource(Bundle.class);
printBundle("PUT", "response", responseBundle);

// Validate results
assertNotNull(responseBundle);
assertEquals(3, responseBundle.getEntry().size());

Bundle.Entry entry1 = responseBundle.getEntry().get(0);
assertNotNull(entry1.getResource());
assertEquals(entry1.getResponse().getStatus().getValue(), "201");
assertEquals(entry1.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
Patient responsePatient = entry1.getResource().as(Patient.class);

Bundle.Entry entry2 = responseBundle.getEntry().get(1);
assertEquals(entry2.getResponse().getStatus().getValue(), "200");
assertEquals(entry2.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
assertEquals(entry2.getResource(), responsePatient);

Bundle.Entry entry3 = responseBundle.getEntry().get(2);
assertEquals(entry3.getResponse().getStatus().getValue(), "200");
assertEquals(entry3.getResponse().getLocation().getValue(), "Patient/"+randomId+"/_history/1");
assertEquals(entry3.getResource(), responsePatient);
}

/**
* Helper function to create a set of Observations, and return them in a
Expand Down
5 changes: 4 additions & 1 deletion fhir-server/src/main/java/com/ibm/fhir/server/operation/spi/FHIRResourceHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,12 @@ Resource doInvoke(FHIROperationContext operationContext, String resourceTypeName
*
* @param bundle
* the request Bundle
* @param skippableUpdates
* if true, and the bundle contains an update for which the resource content in the update matches the existing
* resource on the server, then skip the update; if false, then always attempt the updates specified in the bundle
* @return the response Bundle
*/
Bundle doBundle(Bundle bundle) throws Exception;
Bundle doBundle(Bundle bundle, boolean skippableUpdates) throws Exception;

FHIRPersistenceTransaction getTransaction() throws Exception;

Expand Down
6 changes: 4 additions & 2 deletions fhir-server/src/main/java/com/ibm/fhir/server/resources/Batch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
import javax.annotation.security.RolesAllowed;
import javax.enterprise.context.RequestScoped;
import javax.ws.rs.Consumes;
import javax.ws.rs.HeaderParam;
import javax.ws.rs.POST;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status;

import com.ibm.fhir.core.FHIRConstants;
import com.ibm.fhir.core.FHIRMediaType;
import com.ibm.fhir.exception.FHIROperationException;
import com.ibm.fhir.model.resource.Bundle;
Expand All @@ -46,7 +48,7 @@ public Batch() throws Exception {
}

@POST
public Response bundle(Resource resource) {
public Response bundle(Resource resource, @HeaderParam(FHIRConstants.UPDATE_IF_MODIFIED_HEADER) boolean updateOnlyIfModified) {
log.entering(this.getClass().getName(), "bundle(Bundle)");
Date startTime = new Date();
Response.Status status = null;
Expand All @@ -65,7 +67,7 @@ public Response bundle(Resource resource) {
}

FHIRRestHelper helper = new FHIRRestHelper(getPersistenceImpl());
responseBundle = helper.doBundle(inputBundle);
responseBundle = helper.doBundle(inputBundle, updateOnlyIfModified);
status = Status.OK;
return Response.ok(responseBundle).build();
} catch (FHIRRestBundledRequestException e) {
Expand Down
51 changes: 27 additions & 24 deletions fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,17 +1166,8 @@ public Resource doInvoke(FHIROperationContext operationContext, String resourceT
}
}

/**
* Processes a bundled request.
*
* @param bundleResource
* the request Bundle
* @param requestProperties
* additional request properties which supplement the HTTP headers associated with this request
* @return the response Bundle
*/
@Override
public Bundle doBundle(Bundle inputBundle) throws Exception {
public Bundle doBundle(Bundle inputBundle, boolean skippableUpdates) throws Exception {
log.entering(this.getClass().getName(), "doBundle");

// Save the current request context.
Expand All @@ -1187,7 +1178,7 @@ public Bundle doBundle(Bundle inputBundle) throws Exception {
Map<Integer, Entry> validationResponseEntries = validateBundle(inputBundle);

// Next, process each of the entries in the bundle.
return processBundleEntries(inputBundle, validationResponseEntries);
return processBundleEntries(inputBundle, validationResponseEntries, skippableUpdates);
} finally {
// Restore the original request context.
FHIRRequestContext.set(requestContext);
Expand Down Expand Up @@ -1560,8 +1551,12 @@ private String getVersionIdFromETagValue(String ifMatchValue) {
* the bundle containing the requests
* @param validationResponseEntries
* a map from entry indices to the corresponding response entries created during validation
* @param skippableUpdates
* if true, and the bundle contains an update for which the resource content in the update matches the existing
* resource on the server, then skip the update; if false, then always attempt the updates specified in the bundle
* @return a response bundle
*/
private Bundle processBundleEntries(Bundle requestBundle, Map<Integer, Entry> validationResponseEntries) throws Exception {
private Bundle processBundleEntries(Bundle requestBundle, Map<Integer, Entry> validationResponseEntries, boolean skippableUpdates) throws Exception {
log.entering(this.getClass().getName(), "processBundleEntries");

FHIRTransactionHelper txn = null;
Expand Down Expand Up @@ -1589,7 +1584,7 @@ private Bundle processBundleEntries(Bundle requestBundle, Map<Integer, Entry> va

// Process entries.
List<Entry> responseEntries = processEntriesByMethod(requestBundle, validationResponseEntries,
txn != null, localRefMap, bundleRequestCorrelationId);
txn != null, localRefMap, bundleRequestCorrelationId, skippableUpdates);

// Build the response bundle.
Bundle.Builder bundleResponseBuilder = Bundle.builder().entry(responseEntries);
Expand Down Expand Up @@ -1647,12 +1642,14 @@ private Bundle processBundleEntries(Bundle requestBundle, Map<Integer, Entry> va
* the bundle request properties
* @param bundleRequestCorrelationId
* the bundle request correlation ID
* @return
* the response bundle
* @param skippableUpdates
* if true, and the bundle contains an update for which the resource content in the update matches the existing
* resource on the server, then skip the update; if false, then always attempt the updates specified in the bundle
* @return a list of entries for the response bundle
* @throws Exception
*/
private List<Entry> processEntriesByMethod(Bundle requestBundle, Map<Integer, Entry> validationResponseEntries,
boolean failFast, Map<String, String> localRefMap, String bundleRequestCorrelationId) throws Exception {
boolean failFast, Map<String, String> localRefMap, String bundleRequestCorrelationId, boolean skippableUpdates) throws Exception {
log.entering(this.getClass().getName(), "processEntriesByMethod");

try {
Expand Down Expand Up @@ -1750,10 +1747,10 @@ private List<Entry> processEntriesByMethod(Bundle requestBundle, Map<Integer, En
} else if (request.getMethod().equals(HTTPVerb.PUT)) {
Entry validationResponseEntry = validationResponseEntries.get(entryIndex);
responseEntries[entryIndex] = processEntryForPut(requestEntry, validationResponseEntry, responseIndexAndEntries,
entryIndex, localRefMap, requestURL, absoluteUri, requestDescription.toString(), initialTime);
entryIndex, localRefMap, requestURL, absoluteUri, requestDescription.toString(), initialTime, skippableUpdates);
} else if (request.getMethod().equals(HTTPVerb.PATCH)) {
responseEntries[entryIndex] = processEntryforPatch(requestEntry, requestURL,entryIndex,
requestDescription.toString(), initialTime);
requestDescription.toString(), initialTime, skippableUpdates);
} else if (request.getMethod().equals(HTTPVerb.DELETE)) {
responseEntries[entryIndex] = processEntryForDelete(requestURL, requestDescription.toString(), initialTime);
} else {
Expand Down Expand Up @@ -1833,11 +1830,14 @@ private List<Entry> processEntriesByMethod(Bundle requestBundle, Map<Integer, En
* a description of the request
* @param initialTime
* the time the bundle entry processing started
* @param skippableUpdate
* 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
* @return the bundle entry response
* @throws Exception
*/
private Entry processEntryforPatch(Entry requestEntry, FHIRUrlParser requestURL, Integer entryIndex, String requestDescription, long initialTime)
throws Exception {
private Entry processEntryforPatch(Entry requestEntry, FHIRUrlParser requestURL, Integer entryIndex, String requestDescription,
long initialTime, boolean skippableUpdate) throws Exception {
FHIRRestOperationResponse ior = null;
String[] pathTokens = requestURL.getPathTokens();
String resourceType = null;
Expand Down Expand Up @@ -1866,7 +1866,7 @@ private Entry processEntryforPatch(Entry requestEntry, FHIRUrlParser requestURL,

Parameters parameters = requestEntry.getResource().as(Parameters.class);
FHIRPatch patch = FHIRPathPatch.from(parameters);
ior = doPatch(resourceType, resourceId, patch, null, null, !SKIPPABLE_UPDATE);
ior = doPatch(resourceType, resourceId, patch, null, null, skippableUpdate);

return buildResponseBundleEntry(ior, null, requestDescription, initialTime);
}
Expand Down Expand Up @@ -2113,12 +2113,15 @@ private Entry processEntryForPost(Entry requestEntry, Entry validationResponseEn
* a description of the request
* @param initialTime
* the time the bundle entry processing started
* @param skippableUpdate
* 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
* @return the bundle entry response
* @throws Exception
*/
private Entry processEntryForPut(Entry requestEntry, Entry validationResponseEntry, Map<Integer, Entry> responseIndexAndEntries,
Integer entryIndex, Map<String, String> localRefMap, FHIRUrlParser requestURL, String absoluteUri, String requestDescription, long initialTime)
throws Exception {
Integer entryIndex, Map<String, String> localRefMap, FHIRUrlParser requestURL, String absoluteUri, String requestDescription,
long initialTime, boolean skippableUpdate) throws Exception {

String[] pathTokens = requestURL.getPathTokens();
String type = null;
Expand Down Expand Up @@ -2155,7 +2158,7 @@ private Entry processEntryForPut(Entry requestEntry, Entry validationResponseEnt
if (requestEntry.getRequest().getIfMatch() != null) {
ifMatchBundleValue = requestEntry.getRequest().getIfMatch().getValue();
}
FHIRRestOperationResponse ior = doUpdate(type, id, resource, ifMatchBundleValue, requestURL.getQuery(), !DO_VALIDATION, !SKIPPABLE_UPDATE);
FHIRRestOperationResponse ior = doUpdate(type, id, resource, ifMatchBundleValue, requestURL.getQuery(), skippableUpdate, !DO_VALIDATION);

// If this was a conditional update, and if a local identifier was present and not already mapped to its external identifier, add mapping.
if (pathTokens.length == 1) {
Expand Down
8 changes: 4 additions & 4 deletions fhir-server/src/test/java/com/ibm/fhir/server/test/InteractionValidationConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1647,7 +1647,7 @@ public void testTransactionBundleWithCreateValidSpecificResourceType() throws Ex
FHIRRequestContext.get().setOriginalRequestUri("test");
FHIRRequestContext.get().setReturnPreference(HTTPReturnPreference.OPERATION_OUTCOME);
try {
Bundle bundle = helper.doBundle(requestBundle);
Bundle bundle = helper.doBundle(requestBundle, false);
assertEquals(2, bundle.getEntry().size());
for (Entry bundleEntry : bundle.getEntry()) {
assertEquals(ALL_OK, bundleEntry.getResource().as(OperationOutcome.class));
Expand Down Expand Up @@ -1715,7 +1715,7 @@ public void testTransactionBundleWithCreateNotValidSpecificResourceType() throws
FHIRRequestContext.get().setOriginalRequestUri("test");
FHIRRequestContext.get().setReturnPreference(HTTPReturnPreference.OPERATION_OUTCOME);
try {
helper.doBundle(requestBundle);
helper.doBundle(requestBundle, false);
fail();
} catch (FHIROperationException e) {
// Validate results
Expand Down Expand Up @@ -1786,7 +1786,7 @@ public void testBatchBundleWithCreateValidSpecificResourceType() throws Exceptio
FHIRRequestContext.get().setOriginalRequestUri("test");
FHIRRequestContext.get().setReturnPreference(HTTPReturnPreference.OPERATION_OUTCOME);
try {
Bundle bundle = helper.doBundle(requestBundle);
Bundle bundle = helper.doBundle(requestBundle, false);
assertEquals(2, bundle.getEntry().size());
for (Entry bundleEntry : bundle.getEntry()) {
assertEquals(ALL_OK, bundleEntry.getResource().as(OperationOutcome.class));
Expand Down Expand Up @@ -1854,7 +1854,7 @@ public void testBatchBundleWithCreateNotValidSpecificResourceType() throws Excep
FHIRRequestContext.get().setOriginalRequestUri("test");
FHIRRequestContext.get().setReturnPreference(HTTPReturnPreference.OPERATION_OUTCOME);
try {
Bundle bundle = helper.doBundle(requestBundle);
Bundle bundle = helper.doBundle(requestBundle, false);
assertEquals(2, bundle.getEntry().size());
assertEquals("The requested interaction of type 'create' is not allowed for resource type 'Patient'",
bundle.getEntry().get(0).getResource().as(OperationOutcome.class).getIssue().get(0).getDetails().getText().getValue());
Expand Down
Loading