Skip to content

Commit

Permalink
issue #4101 - call beforeX interceptor methods prior to checking if u…
Browse files Browse the repository at this point in the history
…pdate is skippable

Previously, a skippable update (one that would not affect the contents of the resource being updated)
would result in a success without invoking beforeUpdate or beforePatch interceptor methods.

For users of fhir-smart, this behavior is confusing because the user may not have write access to this
resource type, but the server is "allowing" the write anyway (even though its being optimized away).

Signed-off-by: Lee Surprenant <lmsurpre@merative.com>
  • Loading branch information
lmsurpre committed Dec 9, 2022
1 parent 24a1ba5 commit 0744f23
Showing 1 changed file with 31 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -715,33 +715,6 @@ public FHIRRestOperationResponse doUpdateMeta(FHIRPersistenceEvent event, String
return ior; // early exit, before firing any update events
}
performVersionAwareUpdateCheck(ior.getPrevResource(), ifMatchValue);

// In the case of a patch, we should not be updating meaninglessly.
if ((skippableUpdate || patch != null) && !isDeleted) {
ResourceFingerprintVisitor fingerprinter = new ResourceFingerprintVisitor();
ior.getPrevResource().accept(fingerprinter);
SaltHash baseline = fingerprinter.getSaltAndHash();

fingerprinter = new ResourceFingerprintVisitor(baseline);
newResource.accept(fingerprinter);
if (fingerprinter.getSaltAndHash().equals(baseline)) {
ior.setResource(ior.getPrevResource());
ior.setStatus(Status.OK);
ior.setLocationURI(FHIRUtil.buildLocationURI(type, ior.getPrevResource()));
ior.setOperationOutcome(OperationOutcome.builder()
.issue(Issue.builder()
.severity(IssueSeverity.INFORMATION)
.code(IssueType.INFORMATIONAL)
.details(CodeableConcept.builder()
.text(string("Update resource matches the existing resource; skipping the update"))
.build())
.build())
.build());

ior.setCompleted(true);
return ior; // early exit, before firing any update events
}
}
}

// Configure the persistence event ready to fire the "before create|update|patch" events.
Expand All @@ -764,6 +737,33 @@ public FHIRRestOperationResponse doUpdateMeta(FHIRPersistenceEvent event, String
// capture the resource in case the interceptors modified it in some way
newResource = event.getFhirResource();

// In the case of a patch, we should not be updating meaninglessly.
if ((skippableUpdate || patch != null) && !isDeleted && ior.getPrevResource() != null) {
ResourceFingerprintVisitor fingerprinter = new ResourceFingerprintVisitor();
ior.getPrevResource().accept(fingerprinter);
SaltHash baseline = fingerprinter.getSaltAndHash();

fingerprinter = new ResourceFingerprintVisitor(baseline);
newResource.accept(fingerprinter);
if (fingerprinter.getSaltAndHash().equals(baseline)) {
ior.setResource(ior.getPrevResource());
ior.setStatus(Status.OK);
ior.setLocationURI(FHIRUtil.buildLocationURI(type, ior.getPrevResource()));
ior.setOperationOutcome(OperationOutcome.builder()
.issue(Issue.builder()
.severity(IssueSeverity.INFORMATION)
.code(IssueType.INFORMATIONAL)
.details(CodeableConcept.builder()
.text(string("Update resource matches the existing resource; skipping the update"))
.build())
.build())
.build());

ior.setCompleted(true);
return ior; // early exit
}
}

// update the meta in the new resource. Use the version from the previous resource - this gets checked
// again under a database lock during the persistence phase and the request will be rejected if there's
// a mismatch (can happen when there are concurrent updates).
Expand Down Expand Up @@ -1404,7 +1404,7 @@ public Bundle doSearch(String type, String compartment, String compartmentId,
MultiResourceResult searchResult =
persistence.search(persistenceContext, resourceType);
bundle = createSearchResponseBundle(searchResult.getResourceResults(), searchContext, type);

if (requestUri != null) {
bundle = addLinks(searchContext, bundle, requestUri, searchResult.geFirstId(), searchResult.getLastId(), searchResult.getExpectedNextId(), searchResult.getExpectedPreviousId());
}
Expand Down Expand Up @@ -1448,7 +1448,7 @@ private String getResourceId(ResourceResult<? extends Resource> resourceResult)
return null;
}
return resourceResult.getResource().getId();

}

@Override
Expand Down Expand Up @@ -2570,7 +2570,7 @@ private Bundle addLinks(FHIRPagingContext context, Bundle responseBundle, String

// add the expected resource Id of the last resource in the previous page of search results as a query parameter to previous url
prevLinkUrl = addParameterToUrl(prevLinkUrl, SearchConstants.LAST_ID, expectedPreviousId);


// create 'previous' link
Bundle.Link prevLink =
Expand All @@ -2581,7 +2581,7 @@ private Bundle addLinks(FHIRPagingContext context, Bundle responseBundle, String

return bundleBuilder.build();
}

/**
* Add a new parameter with the name and value to the url.
* @param url the url to which the parameter has to be added
Expand Down

0 comments on commit 0744f23

Please sign in to comment.