Skip to content

Commit

Permalink
Refactor tag creation logic and handle concurrent access:
Browse files Browse the repository at this point in the history
- Simplified tag creation by removing unnecessary transaction complexity, since we allow duplicate tags in hfj_tag_def from hapifhir#4813
- Removed redundant retry logic based on updated DB constraints
  • Loading branch information
iyt-trifork committed Sep 13, 2024
1 parent 06c8fd0 commit 966509c
Show file tree
Hide file tree
Showing 9 changed files with 302 additions and 738 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import ca.uhn.fhir.jpa.bulk.imprt.svc.BulkDataImportSvcImpl;
import ca.uhn.fhir.jpa.cache.IResourceVersionSvc;
import ca.uhn.fhir.jpa.cache.ResourceVersionSvcDaoImpl;
import ca.uhn.fhir.jpa.dao.CacheTagDefinitionDao;
import ca.uhn.fhir.jpa.dao.DaoSearchParamProvider;
import ca.uhn.fhir.jpa.dao.HistoryBuilder;
import ca.uhn.fhir.jpa.dao.HistoryBuilderFactory;
Expand All @@ -56,6 +57,7 @@
import ca.uhn.fhir.jpa.dao.TransactionProcessor;
import ca.uhn.fhir.jpa.dao.data.IResourceModifiedDao;
import ca.uhn.fhir.jpa.dao.data.IResourceSearchUrlDao;
import ca.uhn.fhir.jpa.dao.data.ITagDefinitionDao;
import ca.uhn.fhir.jpa.dao.expunge.ExpungeEverythingService;
import ca.uhn.fhir.jpa.dao.expunge.ExpungeOperation;
import ca.uhn.fhir.jpa.dao.expunge.ExpungeService;
Expand Down Expand Up @@ -893,4 +895,10 @@ public ResourceHistoryCalculator resourceHistoryCalculator(
FhirContext theFhirContext, HibernatePropertiesProvider theHibernatePropertiesProvider) {
return new ResourceHistoryCalculator(theFhirContext, theHibernatePropertiesProvider.isOracleDialect());
}

@Bean
public CacheTagDefinitionDao tagDefinitionDao(
ITagDefinitionDao tagDefinitionDao, MemoryCacheService memoryCacheService) {
return new CacheTagDefinitionDao(tagDefinitionDao, memoryCacheService);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@
import ca.uhn.fhir.jpa.sp.ISearchParamPresenceSvc;
import ca.uhn.fhir.jpa.term.api.ITermReadSvc;
import ca.uhn.fhir.jpa.util.AddRemoveCount;
import ca.uhn.fhir.jpa.util.MemoryCacheService;
import ca.uhn.fhir.jpa.util.QueryChunker;
import ca.uhn.fhir.model.api.IResource;
import ca.uhn.fhir.model.api.ResourceMetadataKeyEnum;
Expand Down Expand Up @@ -106,7 +105,6 @@
import jakarta.annotation.Nullable;
import jakarta.annotation.PostConstruct;
import jakarta.persistence.EntityManager;
import jakarta.persistence.EntityManagerFactory;
import jakarta.persistence.PersistenceContext;
import jakarta.persistence.PersistenceContextType;
import org.apache.commons.lang3.NotImplementedException;
Expand All @@ -130,15 +128,11 @@
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
import org.springframework.stereotype.Repository;
import org.springframework.transaction.PlatformTransactionManager;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.List;
Expand Down Expand Up @@ -235,22 +229,15 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
@Autowired
private IPartitionLookupSvc myPartitionLookupSvc;

@Autowired
private MemoryCacheService myMemoryCacheService;

@Autowired(required = false)
private IFulltextSearchSvc myFulltextSearchSvc;

@Autowired
protected ResourceHistoryCalculator myResourceHistoryCalculator;

@Autowired
private PlatformTransactionManager myTransactionManager;

@Autowired
private EntityManagerFactory myEntityManagerFactory;
protected CacheTagDefinitionDao cacheTagDefinitionDao;

protected TagDefinitionDao tagDefinitionDao;
protected final CodingSpy myCodingSpy = new CodingSpy();

@VisibleForTesting
Expand Down Expand Up @@ -298,7 +285,7 @@ private void extractHapiTags(
TagList tagList = ResourceMetadataKeyEnum.TAG_LIST.get(theResource);
if (tagList != null) {
for (Tag next : tagList) {
TagDefinition def = getTagOrNull(
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails,
TagTypeEnum.TAG,
next.getScheme(),
Expand All @@ -317,7 +304,7 @@ private void extractHapiTags(
List<BaseCodingDt> securityLabels = ResourceMetadataKeyEnum.SECURITY_LABELS.get(theResource);
if (securityLabels != null) {
for (BaseCodingDt next : securityLabels) {
TagDefinition def = getTagOrNull(
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails,
TagTypeEnum.SECURITY_LABEL,
next.getSystemElement().getValue(),
Expand All @@ -336,7 +323,7 @@ private void extractHapiTags(
List<IdDt> profiles = ResourceMetadataKeyEnum.PROFILES.get(theResource);
if (profiles != null) {
for (IIdType next : profiles) {
TagDefinition def = getTagOrNull(
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
Expand All @@ -355,7 +342,7 @@ private void extractRiTags(
List<? extends IBaseCoding> tagList = theResource.getMeta().getTag();
if (tagList != null) {
for (IBaseCoding next : tagList) {
TagDefinition def = getTagOrNull(
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails,
TagTypeEnum.TAG,
next.getSystem(),
Expand All @@ -374,7 +361,7 @@ private void extractRiTags(
List<? extends IBaseCoding> securityLabels = theResource.getMeta().getSecurity();
if (securityLabels != null) {
for (IBaseCoding next : securityLabels) {
TagDefinition def = getTagOrNull(
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails,
TagTypeEnum.SECURITY_LABEL,
next.getSystem(),
Expand All @@ -393,7 +380,7 @@ private void extractRiTags(
List<? extends IPrimitiveType<String>> profiles = theResource.getMeta().getProfile();
if (profiles != null) {
for (IPrimitiveType<String> next : profiles) {
TagDefinition def = getTagOrNull(
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
Expand All @@ -413,7 +400,7 @@ private void extractProfileTags(
if (!def.isStandardType()) {
String profile = def.getResourceProfile("");
if (isNotBlank(profile)) {
TagDefinition profileDef = getTagOrNull(
TagDefinition profileDef = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, profile, null, null, null);

ResourceTag tag = theEntity.addTag(profileDef);
Expand Down Expand Up @@ -447,47 +434,6 @@ public void setContext(FhirContext theContext) {
myContext = theContext;
}

/**
* <code>null</code> will only be returned if the scheme and tag are both blank
*/
protected TagDefinition getTagOrNull(
TransactionDetails theTransactionDetails,
TagTypeEnum theTagType,
String theScheme,
String theTerm,
String theLabel,
String theVersion,
Boolean theUserSelected) {
if (isBlank(theScheme) && isBlank(theTerm) && isBlank(theLabel)) {
return null;
}

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

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, HashMap::new);

retVal = resolvedTagDefinitions.get(key);

if (retVal == null) {
// actual DB hit(s) happen here
retVal = tagDefinitionDao.getOrCreateTag(
theTagType, theScheme, theTerm, theLabel, theVersion, theUserSelected);

TransactionSynchronization sync = new AddTagDefinitionToCacheAfterCommitSynchronization(key, retVal);
TransactionSynchronizationManager.registerSynchronization(sync);

resolvedTagDefinitions.put(key, retVal);
}
}

return retVal;
}

void incrementId(T theResource, ResourceTable theSavedEntity, IIdType theResourceId) {
if (theResourceId == null || theResourceId.getVersionIdPart() == null) {
theSavedEntity.initializeVersion();
Expand Down Expand Up @@ -1713,9 +1659,7 @@ protected void validateResourceForStorage(T theResource, ResourceTable theEntity
}

@PostConstruct
public void start() {
this.tagDefinitionDao = new TagDefinitionDao(myEntityManagerFactory, myTransactionManager);
}
public void start() {}

@VisibleForTesting
public void setStorageSettingsForUnitTest(JpaStorageSettings theStorageSettings) {
Expand Down Expand Up @@ -1754,30 +1698,6 @@ public void setJpaStorageResourceParserForUnitTest(IJpaStorageResourceParser the
myJpaStorageResourceParser = theJpaStorageResourceParser;
}

private class AddTagDefinitionToCacheAfterCommitSynchronization implements TransactionSynchronization {

private final TagDefinition myTagDefinition;
private final MemoryCacheService.TagDefinitionCacheKey myKey;

public AddTagDefinitionToCacheAfterCommitSynchronization(
MemoryCacheService.TagDefinitionCacheKey theKey, TagDefinition theTagDefinition) {
myTagDefinition = theTagDefinition;
myKey = theKey;
}

@Override
public void afterCommit() {
myMemoryCacheService.put(MemoryCacheService.CacheEnum.TAG_DEFINITION, myKey, myTagDefinition);
}
}

@Nonnull
public static MemoryCacheService.TagDefinitionCacheKey toTagDefinitionMemoryCacheKey(
TagTypeEnum theTagType, String theScheme, String theTerm, String theVersion, Boolean theUserSelected) {
return new MemoryCacheService.TagDefinitionCacheKey(
theTagType, theScheme, theTerm, theVersion, theUserSelected);
}

@SuppressWarnings("unchecked")
public static String parseContentTextIntoWords(FhirContext theContext, IBaseResource theResource) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1043,7 +1043,7 @@ private <MT extends IBaseMetaType> void doMetaAdd(
if (!entityHasTag) {
theEntity.setHasTags(true);

TagDefinition def = getTagOrNull(
TagDefinition def = cacheTagDefinitionDao.getTagOrNull(
theTransactionDetails,
nextDef.getTagType(),
nextDef.getSystem(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package ca.uhn.fhir.jpa.dao;

import ca.uhn.fhir.jpa.dao.data.ITagDefinitionDao;
import ca.uhn.fhir.jpa.model.entity.TagDefinition;
import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
import ca.uhn.fhir.jpa.util.MemoryCacheService;
import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
import jakarta.annotation.Nonnull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Repository;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;

import java.util.HashMap;
import java.util.List;

import static org.apache.commons.lang3.StringUtils.isBlank;

@Repository
public class CacheTagDefinitionDao {
private static final Logger ourLog = LoggerFactory.getLogger(CacheTagDefinitionDao.class);

private final ITagDefinitionDao tagDefinitionDao;
private final MemoryCacheService memoryCacheService;

public CacheTagDefinitionDao(ITagDefinitionDao tagDefinitionDao, MemoryCacheService memoryCacheService) {
this.tagDefinitionDao = tagDefinitionDao;
this.memoryCacheService = memoryCacheService;
}

/**
* Returns a TagDefinition or null if the scheme, term, and label are all blank.
*/
protected TagDefinition getTagOrNull(
TransactionDetails transactionDetails,
TagTypeEnum tagType,
String scheme,
String term,
String label,
String version,
Boolean userSelected) {

if (isBlank(scheme) && isBlank(term) && isBlank(label)) {
return null;
}

MemoryCacheService.TagDefinitionCacheKey key =
toTagDefinitionMemoryCacheKey(tagType, scheme, term, version, userSelected);
TagDefinition tagDefinition = memoryCacheService.getIfPresent(MemoryCacheService.CacheEnum.TAG_DEFINITION, key);

if (tagDefinition == null) {
HashMap<MemoryCacheService.TagDefinitionCacheKey, TagDefinition> resolvedTagDefinitions =
transactionDetails.getOrCreateUserData("resolvedTagDefinitions", HashMap::new);

tagDefinition = resolvedTagDefinitions.get(key);

if (tagDefinition == null) {
tagDefinition = getOrCreateTag(tagType, scheme, term, label, version, userSelected);

TransactionSynchronization sync =
new AddTagDefinitionToCacheAfterCommitSynchronization(key, tagDefinition);
TransactionSynchronizationManager.registerSynchronization(sync);

resolvedTagDefinitions.put(key, tagDefinition);
}
}

return tagDefinition;
}

/**
* Gets or creates a TagDefinition entity.
*/
private TagDefinition getOrCreateTag(
TagTypeEnum tagType, String scheme, String term, String label, String version, Boolean userSelected) {
List<TagDefinition> result = tagDefinitionDao.findByTagTypeAndSchemeAndTermAndVersionAndUserSelected(
tagType, scheme, term, version, userSelected, Pageable.ofSize(1));

if (!result.isEmpty()) {
return result.get(0);
} else {
// Create a new TagDefinition if no result is found
TagDefinition newTag = new TagDefinition(tagType, scheme, term, label);
newTag.setVersion(version);
newTag.setUserSelected(userSelected);
return tagDefinitionDao.save(newTag);
}
}

@Nonnull
private static MemoryCacheService.TagDefinitionCacheKey toTagDefinitionMemoryCacheKey(
TagTypeEnum tagType, String scheme, String term, String version, Boolean userSelected) {
return new MemoryCacheService.TagDefinitionCacheKey(tagType, scheme, term, version, userSelected);
}

private class AddTagDefinitionToCacheAfterCommitSynchronization implements TransactionSynchronization {
private final TagDefinition tagDefinition;
private final MemoryCacheService.TagDefinitionCacheKey key;

public AddTagDefinitionToCacheAfterCommitSynchronization(
MemoryCacheService.TagDefinitionCacheKey key, TagDefinition tagDefinition) {
this.tagDefinition = tagDefinition;
this.key = key;
}

@Override
public void afterCommit() {
memoryCacheService.put(MemoryCacheService.CacheEnum.TAG_DEFINITION, key, tagDefinition);
}
}
}
Loading

0 comments on commit 966509c

Please sign in to comment.