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 2283 - eliminate redundant reads during ingestion #2426

Merged
merged 2 commits into from
May 26, 2021
Merged

issue 2283 - eliminate redundant reads during ingestion #2426

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36b217a
issue 2283 - eliminate redundant reads during ingestion
punktilious May 25, 2021
c7fcd55
issue 2283 javadoc from review comments
punktilious May 25, 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
13 changes: 6 additions & 7 deletions ...stence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1496,7 +1496,8 @@ public <T extends Resource> SingleResourceResult<T> read(FHIRPersistenceContext
ResourceDAO resourceDao = makeResourceDAO(connection);

resourceDTO = resourceDao.read(logicalId, resourceType.getSimpleName());
if (resourceDTO != null && resourceDTO.isDeleted() && !context.includeDeleted()) {
boolean resourceIsDeleted = resourceDTO != null && resourceDTO.isDeleted();
if (resourceIsDeleted && !context.includeDeleted()) {
throw new FHIRPersistenceResourceDeletedException("Resource '" +
resourceType.getSimpleName() + "/" + logicalId + "' is deleted.");
}
Expand All @@ -1505,19 +1506,17 @@ public <T extends Resource> SingleResourceResult<T> read(FHIRPersistenceContext
SingleResourceResult<T> result = new SingleResourceResult.Builder<T>()
.success(true)
.resource(resource)
.deleted(resourceIsDeleted)
.build();

return result;
}
catch(FHIRPersistenceResourceDeletedException e) {
} catch(FHIRPersistenceResourceDeletedException e) {
lmsurpre marked this conversation as resolved.
Show resolved Hide resolved
throw e;
}
catch(Throwable e) {
} catch(Throwable e) {
FHIRPersistenceException fx = new FHIRPersistenceException("Unexpected error while performing a read operation.");
log.log(Level.SEVERE, fx.getMessage(), e);
throw fx;
}
finally {
} finally {
log.exiting(CLASSNAME, METHODNAME);
}
}
Expand Down
66 changes: 44 additions & 22 deletions fhir-persistence/src/main/java/com/ibm/fhir/persistence/SingleResourceResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2019
* (C) Copyright IBM Corp. 2019, 2021
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -14,36 +14,47 @@
/**
* A Result wrapper for FHIR interactions that return a single resource.
* Instances are immutable and can be constructed via {@code new SingleResourceResult.Builder<T>()}.
*
*
* @author lmsurpre
*/
public class SingleResourceResult<T extends Resource> {
@Required
final boolean success;
final T resource;
final OperationOutcome outcome;

final boolean deleted;

private SingleResourceResult(Builder<T> builder) {
success = ValidationSupport.requireNonNull(builder.success, "success");
resource = builder.resource;
outcome = builder.outcome;
deleted = builder.deleted;
if (!success && (outcome == null || outcome.getIssue().isEmpty())) {
throw new IllegalStateException("Failed interaction results must include an OperationOutcome with one or more issue.");
}
}

/**
* Whether or not the interaction was successful
*
*
* @return
* whether the interaction was successful
*/
public boolean isSuccess() {
return success;
}

/**
* Whether or not the resource is deleted
* @return whether the resource is deleted
*/
public boolean isDeleted() {
return deleted;
}

/**
* The resource resulting from the interaction
*
*
* @return
* An immutable object of type {@link Resource}.
*/
Expand All @@ -52,74 +63,85 @@ public T getResource() {
}
/**
* An OperationOutcome that represents the outcome of the interaction
*
*
* @return
* An immutable object of type {@link OperationOutcome}
*/
public OperationOutcome getOutcome() {
return outcome;
}

// result builder
public static class Builder<T extends Resource> {
boolean success;
T resource;
OperationOutcome outcome;

boolean deleted;

/**
* Whether or not the interaction was successful
*
*
* <p>This field is required.
*
*
* @param success
* whether the interaction was successful
*
*
* @return
* A reference to this Builder instance
*/
public Builder<T> success(boolean success) {
this.success = success;
return this;
}


/**
* Whether or not the resource is deleted
* @param flag
* @return A reference to this Builder instance
*/
public Builder<T> deleted(boolean flag) {
this.deleted = flag;
return this;
}

/**
* The resulting resource from the interaction
*
*
* @param resource
* the resulting resource from the interaction; this may be null if the interaction was not successful
*
*
* @return
* A reference to this Builder instance
*/
public Builder<T> resource(T resource) {
this.resource = resource;
return this;
}

/**
* An OperationOutcome that represents the outcome of the interaction
*
*
* <p>This field is required when the interaction is not successful
*
*
* @param outcome
* the outcome of the interaction
*
*
* @return
* A reference to this Builder instance
*/
public Builder<T> outcome(OperationOutcome outcome) {
this.outcome = outcome;
return this;
}

/**
* Build the {@link SingleResourceResult}
*
*
* <p>Required fields:
* <ul>
* <li>success</li>
* </ul>
*
*
* @return
* An immutable object of type {@link SingleResourceResult}
*/
Expand Down
5 changes: 3 additions & 2 deletions 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 @@ -17,6 +17,7 @@
import com.ibm.fhir.model.resource.Resource;
import com.ibm.fhir.persistence.FHIRPersistenceTransaction;
import com.ibm.fhir.persistence.ResourceEraseRecord;
import com.ibm.fhir.persistence.SingleResourceResult;
import com.ibm.fhir.persistence.erase.EraseDTO;

/**
Expand Down Expand Up @@ -162,7 +163,7 @@ FHIRRestOperationResponse doPatch(String type, String id, FHIRPatch patch, Strin
* @return the Resource
lmsurpre marked this conversation as resolved.
Show resolved Hide resolved
* @throws Exception
*/
default Resource doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
default SingleResourceResult<? extends Resource> doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
Resource contextResource) throws Exception {
return doRead(type, id, throwExcOnNull, includeDeleted, contextResource, null);
}
Expand All @@ -185,7 +186,7 @@ default Resource doRead(String type, String id, boolean throwExcOnNull, boolean
* @return the Resource
lmsurpre marked this conversation as resolved.
Show resolved Hide resolved
* @throws Exception
*/
Resource doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
SingleResourceResult<? extends Resource> doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
Resource contextResource, MultivaluedMap<String, String> queryParameters) throws Exception;

/**
Expand Down
2 changes: 1 addition & 1 deletion fhir-server/src/main/java/com/ibm/fhir/server/resources/Read.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public Response read(@PathParam("type") String type, @PathParam("id") String id,
long modifiedSince = parseIfModifiedSince();

FHIRRestHelper helper = new FHIRRestHelper(getPersistenceImpl());
Resource resource = helper.doRead(type, id, true, false, null, queryParameters);
Resource resource = helper.doRead(type, id, true, false, null, queryParameters).getResource();
int version2Match = -1;
// Support ETag value with or without " (and W/)
// e.g: 1, "1", W/1, W/"1" (the first format is used by TouchStone)
Expand Down
45 changes: 25 additions & 20 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 @@ -337,6 +337,7 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
// Save the current request context.
FHIRRequestContext requestContext = FHIRRequestContext.get();

boolean isDeleted; // stash the deleted status of the resource when we first read it
FHIRRestOperationResponse ior = new FHIRRestOperationResponse();

try {
Expand Down Expand Up @@ -392,6 +393,9 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
} else {
id = newResource.getId();
}

// No match, so deletion status doesn't matter
isDeleted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

The boolean is already going to be false by default, but I guess it doesn't hurt to do this for clarity (same on line 415) along with the comment.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i actually prefer explicitly initializing it (even though it is redundant)...just one less thing to remember :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it's not initialized on purpose:

        boolean isDeleted; // stash the deleted status of the resource when we first read it

I do this intentionally to make sure I set it in each of the if/then/else paths. If I miss a path, the compiler will catch it.

Copy link
Member

Choose a reason for hiding this comment

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

I think that is true for Boolean fields, but is it true for booleans?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Default is false if it's a member of the class. But for local variables in the method, default is unassigned (Eclipse will complain too if you try to use it without assigning a value).

} else if (resultCount == 1) {
// If we found a single match, then we'll perform a normal update on the matched resource.
ior.setPrevResource(responseBundle.getEntry().get(0).getResource());
Expand All @@ -406,11 +410,15 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa

// Make sure the id of the newResource is not null and is the same as the id of the found resource.
newResource = newResource.toBuilder().id(id).build();

// Got a match, so definitely can't be deleted
isDeleted = false;
} else {
String msg =
"The search criteria specified for a conditional update/patch operation returned multiple matches.";
throw buildRestException(msg, IssueType.MULTIPLE_MATCHES);
}

} else {
// Make sure an id value was passed in.
if (id == null) {
Expand All @@ -433,8 +441,11 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
}
}

// Retrieve the resource to be updated using the type and id values.
ior.setPrevResource(doRead(type, id, (patch != null), true, newResource, null, false));
// Retrieve the resource to be updated using the type and id values. Include
// the resource even if it has been deleted
SingleResourceResult<? extends Resource> srr = doRead(type, id, (patch != null), true, newResource, null, false);
ior.setPrevResource(srr.getResource());
isDeleted = srr.isDeleted();
}

if (patch != null) {
Expand All @@ -457,16 +468,9 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa
List<Issue> warnings = doValidation ? new ArrayList<>(validateInput(newResource)) : new ArrayList<>() ;

// Perform the "version-aware" update check, and also find out if the resource was deleted.
boolean isDeleted = false;
if (ior.getPrevResource() != null) {
performVersionAwareUpdateCheck(ior.getPrevResource(), ifMatchValue);

try {
doRead(type, id, (patch != null), false, newResource, null, false);
} catch (FHIRPersistenceResourceDeletedException e) {
isDeleted = true;
}

if (skippableUpdate && !isDeleted) {
ResourceFingerprintVisitor fingerprinter = new ResourceFingerprintVisitor();
ior.getPrevResource().accept(fingerprinter);
Expand Down Expand Up @@ -667,7 +671,7 @@ public FHIRRestOperationResponse doDelete(String type, String id, String searchQ

// Read the resource so it will be available to the beforeDelete interceptor methods.
try {
resourceToDelete = doRead(type, id, false, false, null, null, false);
resourceToDelete = doRead(type, id, false, false, null, null, false).getResource();
if (resourceToDelete != null) {
responseBundle = Bundle.builder().type(BundleType.SEARCHSET)
.id(UUID.randomUUID().toString())
Expand All @@ -680,7 +684,7 @@ public FHIRRestOperationResponse doDelete(String type, String id, String searchQ
}
} catch (FHIRPersistenceResourceDeletedException e) {
// Absorb this exception.
ior.setResource(doRead(type, id, false, true, null, null, false));
ior.setResource(doRead(type, id, false, true, null, null, false).getResource());
warnings.add(buildOperationOutcomeIssue(IssueSeverity.WARNING, IssueType.DELETED, "Resource of type '"
+ type + "' with id '" + id + "' is already deleted."));
}
Expand Down Expand Up @@ -753,13 +757,13 @@ public FHIRRestOperationResponse doDelete(String type, String id, String searchQ
}

@Override
public Resource doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
public SingleResourceResult<? extends Resource> doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
Resource contextResource) throws Exception {
return doRead(type, id, throwExcOnNull, includeDeleted, contextResource, null);
}

@Override
public Resource doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
public SingleResourceResult<? extends Resource> doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to pass includeDeleted now that we have the isDeleted marker in the response?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be possible to refactor the entire handling of deleted status, but this is a much bigger task. Currently the persistence layer will throw an exception to signal the resource has been deleted. Handling this without using an exception is non-trivial.

Copy link
Member

Choose a reason for hiding this comment

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

Currently the persistence layer will throw an exception to signal the resource has been deleted.

but only conditionally, based on this flag.

I guess was was hoping we could maybe get away with only breaking this method signature once and remove includeDeleted now. I'm fine with saving that for "future work" and I agree its probably not a priority.

Resource contextResource, MultivaluedMap<String, String> queryParameters) throws Exception {
return doRead(type, id, throwExcOnNull, includeDeleted, contextResource, queryParameters, true);
}
Expand All @@ -783,14 +787,16 @@ public Resource doRead(String type, String id, boolean throwExcOnNull, boolean i
* for supporting _elements and _summary for resource read
* @param checkInteractionAllowed
* if true, check if this interaction is allowed per the tenant configuration; if false, assume interaction is allowed
* @return the Resource
* @return a {@link SingleResourceResult} containing the Resource
* @throws Exception
*/
private Resource doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
private SingleResourceResult<? extends Resource> doRead(String type, String id, boolean throwExcOnNull, boolean includeDeleted,
Resource contextResource, MultivaluedMap<String, String> queryParameters, boolean checkInteractionAllowed)
throws Exception {
log.entering(this.getClass().getName(), "doRead");

SingleResourceResult<? extends Resource> result;

// Validate that interaction is allowed for given resource type
if (checkInteractionAllowed) {
validateInteraction(Interaction.READ.value(), type);
Expand All @@ -800,8 +806,6 @@ private Resource doRead(String type, String id, boolean throwExcOnNull, boolean
FHIRTransactionHelper txn = new FHIRTransactionHelper(getTransaction());
txn.begin();

Resource resource = null;

// Save the current request context.
FHIRRequestContext requestContext = FHIRRequestContext.get();

Expand All @@ -826,7 +830,8 @@ private Resource doRead(String type, String id, boolean throwExcOnNull, boolean

FHIRPersistenceContext persistenceContext =
FHIRPersistenceContextFactory.createPersistenceContext(event, includeDeleted, searchContext);
resource = persistence.read(persistenceContext, resourceType, id).getResource();
result = persistence.read(persistenceContext, resourceType, id);
Resource resource = result.getResource();
if (resource == null && throwExcOnNull) {
throw new FHIRPersistenceResourceNotFoundException("Resource '" + type + "/" + id + "' not found.");
}
Expand All @@ -840,7 +845,7 @@ private Resource doRead(String type, String id, boolean throwExcOnNull, boolean
txn.commit();
txn = null;

return resource;
return result;
} finally {
// Restore the original request context.
FHIRRequestContext.set(requestContext);
Expand Down Expand Up @@ -1945,7 +1950,7 @@ private Entry processEntryForGet(Entry.Request entryRequest, FHIRUrlParser reque
}
} else if (pathTokens.length == 2) {
// This is a 'read' request.
resource = doRead(pathTokens[0], pathTokens[1], true, false, null);
resource = doRead(pathTokens[0], pathTokens[1], true, false, null).getResource();
} else if (pathTokens.length == 3) {
if ("_history".equals(pathTokens[2])) {
// This is a 'history' request.
Expand Down
Loading