Skip to content

Commit

Permalink
FUT1-5892 revert "3156 multiple tag race condition (hapifhir#3297)"
Browse files Browse the repository at this point in the history 
- Issue hapifhir#3412
  • Loading branch information
@tyfoni-systematic
tyge.folke.nielsen authored and tyfoni-systematic committed Aug 19, 2022
1 parent b15e5b3 commit be0df29
Showing 1 changed file with 39 additions and 124 deletions.
163 changes: 39 additions & 124 deletions hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
Expand Down Expand Up @@ -120,12 +119,8 @@
import org.springframework.context.ApplicationContextAware;
import org.springframework.stereotype.Repository;
import org.springframework.transaction.PlatformTransactionManager;
import org.springframework.transaction.TransactionDefinition;
import org.springframework.transaction.TransactionStatus;
import org.springframework.transaction.support.TransactionCallback;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import org.springframework.transaction.support.TransactionTemplate;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -153,7 +148,6 @@
import java.util.Set;
import java.util.StringTokenizer;
import java.util.UUID;
import java.util.stream.Collectors;

import static ca.uhn.fhir.jpa.model.util.JpaConstants.ALL_PARTITIONS_NAME;
import static org.apache.commons.lang3.StringUtils.defaultIfBlank;
Expand Down Expand Up @@ -256,9 +250,6 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
@Autowired(required = false)
private IFulltextSearchSvc myFulltextSearchSvc;

@Autowired
private PlatformTransactionManager myTransactionManager;

@VisibleForTesting
public void setSearchParamPresenceSvc(ISearchParamPresenceSvc theSearchParamPresenceSvc) {
mySearchParamPresenceSvc = theSearchParamPresenceSvc;
Expand Down Expand Up @@ -406,15 +397,40 @@ protected TagDefinition getTagOrNull(TransactionDetails theTransactionDetails, T

MemoryCacheService.TagDefinitionCacheKey key = toTagDefinitionMemoryCacheKey(theTagType, theScheme, theTerm);


TagDefinition retVal = myMemoryCacheService.getIfPresent(MemoryCacheService.CacheEnum.TAG_DEFINITION, key);


if (retVal == null) {
HashMap<MemoryCacheService.TagDefinitionCacheKey, TagDefinition> resolvedTagDefinitions = theTransactionDetails.getOrCreateUserData(HapiTransactionService.XACT_USERDATA_KEY_RESOLVED_TAG_DEFINITIONS, () -> new HashMap<>());
retVal = resolvedTagDefinitions.get(key);

if (retVal == null) {
// actual DB hit(s) happen here
retVal = getOrCreateTag(theTagType, theScheme, theTerm, theLabel);
CriteriaBuilder builder = myEntityManager.getCriteriaBuilder();
CriteriaQuery<TagDefinition> cq = builder.createQuery(TagDefinition.class);
Root<TagDefinition> from = cq.from(TagDefinition.class);

if (isNotBlank(theScheme)) {
cq.where(
builder.and(
builder.equal(from.get("myTagType"), theTagType),
builder.equal(from.get("mySystem"), theScheme),
builder.equal(from.get("myCode"), theTerm)));
} else {
cq.where(
builder.and(
builder.equal(from.get("myTagType"), theTagType),
builder.isNull(from.get("mySystem")),
builder.equal(from.get("myCode"), theTerm)));
}

TypedQuery<TagDefinition> q = myEntityManager.createQuery(cq);
try {
retVal = q.getSingleResult();
} catch (NoResultException e) {
retVal = new TagDefinition(theTagType, theScheme, theTerm, theLabel);
myEntityManager.persist(retVal);
}

TransactionSynchronization sync = new AddTagDefinitionToCacheAfterCommitSynchronization(key, retVal);
TransactionSynchronizationManager.registerSynchronization(sync);
Expand All @@ -426,110 +442,6 @@ protected TagDefinition getTagOrNull(TransactionDetails theTransactionDetails, T
return retVal;
}

/**
* Gets the tag defined by the fed in values, or saves it if it does not
* exist.
* <p>
* Can also throw an InternalErrorException if something bad happens.
*/
private TagDefinition getOrCreateTag(TagTypeEnum theTagType, String theScheme, String theTerm, String theLabel) {
CriteriaBuilder builder = myEntityManager.getCriteriaBuilder();
CriteriaQuery<TagDefinition> cq = builder.createQuery(TagDefinition.class);
Root<TagDefinition> from = cq.from(TagDefinition.class);

if (isNotBlank(theScheme)) {
cq.where(
builder.and(
builder.equal(from.get("myTagType"), theTagType),
builder.equal(from.get("mySystem"), theScheme),
builder.equal(from.get("myCode"), theTerm)));
} else {
cq.where(
builder.and(
builder.equal(from.get("myTagType"), theTagType),
builder.isNull(from.get("mySystem")),
builder.equal(from.get("myCode"), theTerm)));
}

TypedQuery<TagDefinition> q = myEntityManager.createQuery(cq);

TransactionTemplate template = new TransactionTemplate(myTransactionManager);
template.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);

// this transaction will attempt to get or create the tag,
// repeating (on any failure) 10 times.
// if it fails more than this, we will throw exceptions
TagDefinition retVal;
int count = 0;
HashSet<Throwable> throwables = new HashSet<>();
do {
try {
retVal = template.execute(new TransactionCallback<TagDefinition>() {

// do the actual DB call(s) to read and/or write the values
private TagDefinition readOrCreate() {
TagDefinition val;
try {
val = q.getSingleResult();
} catch (NoResultException e) {
val = new TagDefinition(theTagType, theScheme, theTerm, theLabel);
myEntityManager.persist(val);
}
return val;
}

@Override
public TagDefinition doInTransaction(TransactionStatus status) {
TagDefinition tag = null;

try {
tag = readOrCreate();
} catch (Exception ex) {
// log any exceptions - just in case
// they may be signs of things to come...
ourLog.warn(
"Tag read/write failed: "
+ ex.getMessage() + ". "
+ "This is not a failure on its own, "
+ "but could be useful information in the result of an actual failure."
);
throwables.add(ex);
}

return tag;
}
});
} catch (Exception ex) {
// transaction template can fail if connections to db are exhausted
// and/or timeout
ourLog.warn("Transaction failed with: "
+ ex.getMessage() + ". "
+ "Transaction will rollback and be reattempted."
);
retVal = null;
}
count++;
} while (retVal == null && count < TOTAL_TAG_READ_ATTEMPTS);

if (retVal == null) {
// if tag is still null,
// something bad must be happening
// - throw
String msg = throwables.stream()
.map(Throwable::getMessage)
.collect(Collectors.joining(", "));
throw new InternalErrorException(
Msg.code(2023)
+ "Tag get/create failed after "
+ TOTAL_TAG_READ_ATTEMPTS
+ " attempts with error(s): "
+ msg
);
}

return retVal;
}

protected IBundleProvider history(RequestDetails theRequest, String theResourceType, Long theResourcePid, Date theRangeStartInclusive, Date theRangeEndInclusive, Integer theOffset) {

String resourceName = defaultIfBlank(theResourceType, null);
Expand Down Expand Up @@ -967,7 +879,7 @@ protected void postUpdate(ResourceTable theEntity, T theResource, RequestDetails

@CoverageIgnore
public BaseHasResource readEntity(IIdType theValueId, RequestDetails theRequest) {
throw new NotImplementedException(Msg.code(927) + "");
throw new NotImplementedException("");
}

/**
Expand Down Expand Up @@ -1181,7 +1093,7 @@ public <R extends IBaseResource> R toResource(Class<R> theResourceType, IBaseRes
b.append(e.getMessage());
String msg = b.toString();
ourLog.error(msg, e);
throw new DataFormatException(Msg.code(928) + msg, e);
throw new DataFormatException(msg, e);
}

} else {
Expand Down Expand Up @@ -1260,7 +1172,7 @@ private void verifyMatchUrlForConditionalCreate(IBaseResource theResource, Strin
// Make sure that the match URL was actually appropriate for the supplied resource
InMemoryMatchResult outcome = myInMemoryResourceMatcher.match(theIfNoneExist, theResource, theParams);
if (outcome.supported() && !outcome.matched()) {
throw new InvalidRequestException(Msg.code(929) + "Failed to process conditional create. The supplied resource did not satisfy the conditional URL.");
throw new InvalidRequestException("Failed to process conditional create. The supplied resource did not satisfy the conditional URL.");
}
}

Expand Down Expand Up @@ -1663,7 +1575,8 @@ private void createHistoryEntry(RequestDetails theRequest, IBaseResource theReso
private void validateIncomingResourceTypeMatchesExisting(IBaseResource theResource, BaseHasResource entity) {
String resourceType = myContext.getResourceType(theResource);
if (!resourceType.equals(entity.getResourceType())) {
throw new UnprocessableEntityException(Msg.code(930) + "Existing resource ID[" + entity.getIdDt().toUnqualifiedVersionless() + "] is of type[" + entity.getResourceType() + "] - Cannot update with [" + resourceType + "]");
throw new UnprocessableEntityException(
"Existing resource ID[" + entity.getIdDt().toUnqualifiedVersionless() + "] is of type[" + entity.getResourceType() + "] - Cannot update with [" + resourceType + "]");
}
}

Expand Down Expand Up @@ -1810,7 +1723,8 @@ private void validateChildReferenceTargetTypes(IBase theElement, String thePath)
if (!isLogicalReference(referencedId)) {
if (!referencedId.getValue().contains("?")) {
if (!validTypes.contains(referencedId.getResourceType())) {
throw new UnprocessableEntityException(Msg.code(931) + "Invalid reference found at path '" + newPath + "'. Resource type '" + referencedId.getResourceType() + "' is not valid for this path");
throw new UnprocessableEntityException(
"Invalid reference found at path '" + newPath + "'. Resource type '" + referencedId.getResourceType() + "' is not valid for this path");
}
}
}
Expand All @@ -1825,7 +1739,7 @@ private void validateChildReferenceTargetTypes(IBase theElement, String thePath)
protected void validateMetaCount(int theMetaCount) {
if (myConfig.getResourceMetaCountHardLimit() != null) {
if (theMetaCount > myConfig.getResourceMetaCountHardLimit()) {
throw new UnprocessableEntityException(Msg.code(932) + "Resource contains " + theMetaCount + " meta entries (tag/profile/security label), maximum is " + myConfig.getResourceMetaCountHardLimit());
throw new UnprocessableEntityException("Resource contains " + theMetaCount + " meta entries (tag/profile/security label), maximum is " + myConfig.getResourceMetaCountHardLimit());
}
}
}
Expand Down Expand Up @@ -1862,7 +1776,7 @@ protected void validateResourceForStorage(T theResource, ResourceTable theEntity
}

if (tag != null) {
throw new UnprocessableEntityException(Msg.code(933) + "Resource contains the 'subsetted' tag, and must not be stored as it may contain a subset of available data");
throw new UnprocessableEntityException("Resource contains the 'subsetted' tag, and must not be stored as it may contain a subset of available data");
}

if (getConfig().isEnforceReferenceTargetTypes()) {
Expand Down Expand Up @@ -1986,7 +1900,7 @@ private static String parseNarrativeTextIntoWords(IBaseResource theResource) {
}
}
} catch (Exception e) {
throw new DataFormatException(Msg.code(934) + "Unable to convert DIV to string", e);
throw new DataFormatException("Unable to convert DIV to string", e);
}

}
Expand Down Expand Up @@ -2016,7 +1930,8 @@ private static List<BaseCodingDt> toBaseCodingList(List<IBaseCoding> theSecurity

public static void validateResourceType(BaseHasResource theEntity, String theResourceName) {
if (!theResourceName.equals(theEntity.getResourceType())) {
throw new ResourceNotFoundException(Msg.code(935) + "Resource with ID " + theEntity.getIdDt().getIdPart() + " exists but it is not of type " + theResourceName + ", found resource of type " + theEntity.getResourceType());
throw new ResourceNotFoundException(
"Resource with ID " + theEntity.getIdDt().getIdPart() + " exists but it is not of type " + theResourceName + ", found resource of type " + theEntity.getResourceType());
}
}

Expand Down

0 comments on commit be0df29

Please sign in to comment.