From be0df29b9c9ae37a1499ff06d07269f35f790291 Mon Sep 17 00:00:00 2001 From: "tyge.folke.nielsen" Date: Tue, 22 Feb 2022 14:59:23 +0100 Subject: [PATCH] FUT1-5892 revert "3156 multiple tag race condition (#3297)" - Issue https://github.com/hapifhir/hapi-fhir/issues/3412 --- .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 163 +++++------------- 1 file changed, 39 insertions(+), 124 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index d1aa54081ee0..418a84017e11 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -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; @@ -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; @@ -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; @@ -256,9 +250,6 @@ public abstract class BaseHapiFhirDao extends BaseStora @Autowired(required = false) private IFulltextSearchSvc myFulltextSearchSvc; - @Autowired - private PlatformTransactionManager myTransactionManager; - @VisibleForTesting public void setSearchParamPresenceSvc(ISearchParamPresenceSvc theSearchParamPresenceSvc) { mySearchParamPresenceSvc = theSearchParamPresenceSvc; @@ -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 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 cq = builder.createQuery(TagDefinition.class); + Root 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 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); @@ -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. - *

- * 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 cq = builder.createQuery(TagDefinition.class); - Root 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 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 throwables = new HashSet<>(); - do { - try { - retVal = template.execute(new TransactionCallback() { - - // 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); @@ -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(""); } /** @@ -1181,7 +1093,7 @@ public R toResource(Class 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 { @@ -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."); } } @@ -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 + "]"); } } @@ -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"); } } } @@ -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()); } } } @@ -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()) { @@ -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); } } @@ -2016,7 +1930,8 @@ private static List toBaseCodingList(List 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()); } }