Skip to content

Commit

Permalink
issue #2050 - perform extra read in case of IfNoneMatch conflict
Browse files Browse the repository at this point in the history 
Question: should undeleting a deleted resource be handled like an
updateCreate?

Without this, we could run into cases where the locationURI could list
the deleted version of a resource.
Basically, handling the undelete like create-on-update means that we can
avoid doing the extra read in the normal IfNoneMatch case.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Nov 10, 2021
1 parent 6503979 commit a75cd43
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 13 deletions.
3 changes: 2 additions & 1 deletion fhir-server/src/main/java/com/ibm/fhir/server/rest/FHIRRestInteractionVisitorPersist.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ public FHIRRestOperationResponse doUpdate(int entryIndex, FHIRPersistenceEvent e

doInteraction(entryIndex, requestDescription, initialTime, () -> {

FHIRRestOperationResponse ior = helpers.doPatchOrUpdatePersist(event, type, id, false, newResource, prevResource, warnings, isDeleted, ifNoneMatch);
FHIRRestOperationResponse ior = helpers.doPatchOrUpdatePersist(event, type, id, false, newResource,
prevResource, warnings, isDeleted, ifNoneMatch);
OperationOutcome validationOutcome = null;
if (validationResponseEntry != null && validationResponseEntry.getResponse() != null) {
validationOutcome = validationResponseEntry.getResponse().getOutcome().as(OperationOutcome.class);
Expand Down
42 changes: 30 additions & 12 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 @@ -436,7 +436,8 @@ 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));
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, doValidation, warnings);
if (metaResponse.isCompleted()) {
// skip the update, so we can short-circuit here
txn.commit();
Expand All @@ -445,7 +446,8 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
}

// Persist the resource
FHIRRestOperationResponse ior = doPatchOrUpdatePersist(event, type, id, patch != null, metaResponse.getResource(), metaResponse.getPrevResource(), warnings, metaResponse.isDeleted(),
FHIRRestOperationResponse ior = doPatchOrUpdatePersist(event, type, id, patch != null,
metaResponse.getResource(), metaResponse.getPrevResource(), warnings, metaResponse.isDeleted(),
ifNoneMatch);

txn.commit();
Expand All @@ -468,8 +470,8 @@ 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,
List<Issue> warnings) throws Exception {
String ifMatchValue, String searchQueryString, boolean skippableUpdate, boolean doValidation,
List<Issue> warnings) throws Exception {
log.entering(this.getClass().getName(), "doUpdateMeta");

// Do everything we need to get the resource ready for storage. This includes handling
Expand Down Expand Up @@ -739,7 +741,7 @@ public FHIRRestOperationResponse doPatchOrUpdatePersist(FHIRPersistenceEvent eve

FHIRPersistenceContext persistenceContext =
FHIRPersistenceContextFactory.createPersistenceContext(event, ifNoneMatch);
boolean updateCreate = (prevResource == null);
boolean updateCreate = (prevResource == null || isDeleted);
final SingleResourceResult<Resource> result;
if (updateCreate) {
// resource shouldn't exist, so we assume it's a create
Expand All @@ -764,12 +766,8 @@ public FHIRRestOperationResponse doPatchOrUpdatePersist(FHIRPersistenceEvent eve
// Invoke the 'afterUpdate' interceptor methods.
if (updateCreate) {
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); // resource wasn't changed
ior.setLocationURI(FHIRUtil.buildLocationURI(type, prevResource));
ior.setStatus(Response.Status.NOT_MODIFIED);
// Someone must have snuck in and created the resource after our read but before our create/update.
handleIfNoneMatchConflict(type, id, ior);
} else {
ior.setStatus(Response.Status.CREATED);
getInterceptorMgr().fireAfterCreateEvent(event);
Expand Down Expand Up @@ -817,6 +815,26 @@ public FHIRRestOperationResponse doPatchOrUpdatePersist(FHIRPersistenceEvent eve
}
}

private void handleIfNoneMatchConflict(String type, String id, FHIRRestOperationResponse ior) throws FHIRPersistenceException {
// Attempt to read the newly-written resource so that we can set the locationURI appropriately
FHIRPersistenceEvent eventForRead =
new FHIRPersistenceEvent(null, buildPersistenceEventProperties(type, id, null, null));
FHIRPersistenceContext persistenceContextForRead =
FHIRPersistenceContextFactory.createPersistenceContext(eventForRead, false);
SingleResourceResult<? extends Resource> readResult =
persistence.read(persistenceContextForRead, ModelSupport.getResourceType(type), id);

if (readResult.isSuccess()) {
Resource prevResource = readResult.getResource();
ior.setResource(prevResource);
ior.setLocationURI(FHIRUtil.buildLocationURI(type, prevResource));
} else if (log.isLoggable(Level.FINE)){
log.fine("Unable to read IfNoneMatch conflict version: " + readResult.getOutcome());
}
ior.setStatus(Response.Status.NOT_MODIFIED);
// We don't fire any 'after' event because nothing was actually updated by this interaction.
}

/**
* Check that the id and meta fields in the resource have been set up
* @param resource
Expand Down Expand Up @@ -1606,7 +1624,7 @@ private Map<Integer, Entry> validateBundle(Bundle bundle) throws Exception {
throw buildRestException(msg, IssueType.VALUE);
}
String fullUrlPlusVersion = fullUrl;
if (resource != null && resource.getMeta() != null
if (resource != null && resource.getMeta() != null
&& resource.getMeta().getVersionId() != null && resource.getMeta().getVersionId().hasValue()) {
fullUrlPlusVersion = fullUrl + resource.getMeta().getVersionId().getValue();
} else {
Expand Down

0 comments on commit a75cd43

Please sign in to comment.