Skip to content

Commit

Permalink
Add UserSelected and Version to resource tag (#4591)
Browse files Browse the repository at this point in the history 
* First commit:  Add userSelected and version to ResourceTag and ResourceHistoryTag.  Add migration step to migration tasks.  For some reason SchemaMigrationTest is failing.

* Fix nullability, update constructors, truncate versions over 30 chars.

* yay we're done just cleanup

* Checkpoint for recent changes.

* Last commit before pausing work.

* Fix bad code to find the version, tweak unit test pid ID to versionless and add TODO about why is update part of the test not passing?

* Fix bad code in extractTagsRi():  Make it leverage the IBaseCoding variable already in the for loop.  Fix bad code in populateResourceMetadataRi().   Leverage the existing BaseTag and use it to populate the IBaseCoding tag instance instead.  Add more and better assertions in the unit test.

* Merge rel_6_4 in

* Current state for pairing

* Fix CodingDt.getUserSelected method return code in tinder

* Complete migration task and cleanup

* Remove missed fixme

* Bump version temporarily to use it for building core lib

* Use primitive for compatibility with tinder generated CodingDt

* Copyright updates

* Hack to bypass NPE generated by bogus tinder-generated code

* Simplify by removing temporary deprecations as version bump is needed anyway because of TagDefinition entity changes

* Adjust tests

* Fix migration task

* Add test to show problem with tinder generated BooleanDt

* More test adjustments

* Improve BooleanDt NPE avoidance

* Update core dep

* Reverse Juan CVE VersionEnumTest changes.

* Fix unit test.

---------

Co-authored-by: Ken Stevens <ken@smilecdr.com>
Co-authored-by: juan.marchionatto <juan.marchionatto@smilecdr.com>
Co-authored-by: Tadgh <garygrantgraham@gmail.com>
  • Loading branch information
@lukedegruchy @tadgh
4 people committed Feb 27, 2023
1 parent ce46f4d commit 9d70dbd
Show file tree
Hide file tree
Showing 15 changed files with 359 additions and 63 deletions.
7 changes: 4 additions & 3 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/model/api/Tag.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,14 @@
* limitations under the License.
* #L%
*/
import static org.apache.commons.lang3.StringUtils.isNotBlank;

import java.net.URI;

import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.commons.lang3.builder.ToStringStyle;
import org.hl7.fhir.instance.model.api.IBaseCoding;

import java.net.URI;

/**
* A single tag
* <p>
Expand Down Expand Up @@ -115,12 +114,14 @@ public boolean equals(Object obj) {
return false;
if (getClass() != obj.getClass())
return false;

Tag other = (Tag) obj;
if (myScheme == null) {
if (other.myScheme != null)
return false;
} else if (!myScheme.equals(other.myScheme))
return false;

if (myTerm == null) {
if (other.myTerm != null)
return false;
Expand Down
6 changes: 6 additions & 0 deletions ...og/6_4_0/4204-default-config-meta-tags-user-selected-and-version-missing-in-database.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
type: fix
issue: 4204
jira: SMILE-4688
title: "With default configuration, Resource meta.tag properties: `userSelected` and `version`, were not stored in the database.
This is now fixed."
90 changes: 57 additions & 33 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 @@ -126,6 +126,7 @@
import javax.persistence.TypedQuery;
import javax.persistence.criteria.CriteriaBuilder;
import javax.persistence.criteria.CriteriaQuery;
import javax.persistence.criteria.Predicate;
import javax.persistence.criteria.Root;
import javax.xml.stream.events.Characters;
import javax.xml.stream.events.XMLEvent;
Expand All @@ -143,7 +144,9 @@
import java.util.StringTokenizer;
import java.util.stream.Collectors;

import static java.util.Objects.isNull;
import static java.util.Objects.nonNull;
import static org.apache.commons.lang3.BooleanUtils.isFalse;
import static org.apache.commons.lang3.StringUtils.defaultString;
import static org.apache.commons.lang3.StringUtils.isBlank;
import static org.apache.commons.lang3.StringUtils.isNotBlank;
Expand Down Expand Up @@ -276,7 +279,8 @@ private void extractTagsHapi(TransactionDetails theTransactionDetails, IResource
TagList tagList = ResourceMetadataKeyEnum.TAG_LIST.get(theResource);
if (tagList != null) {
for (Tag next : tagList) {
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getScheme(), next.getTerm(), next.getLabel());
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getScheme(), next.getTerm(),
next.getLabel(), next.getVersion(), next.getUserSelected());
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
allDefs.add(tag);
Expand All @@ -288,7 +292,8 @@ private void extractTagsHapi(TransactionDetails theTransactionDetails, IResource
List<BaseCodingDt> securityLabels = ResourceMetadataKeyEnum.SECURITY_LABELS.get(theResource);
if (securityLabels != null) {
for (BaseCodingDt next : securityLabels) {
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystemElement().getValue(), next.getCodeElement().getValue(), next.getDisplayElement().getValue());
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystemElement().getValue(),
next.getCodeElement().getValue(), next.getDisplayElement().getValue(), null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
allDefs.add(tag);
Expand All @@ -300,7 +305,7 @@ private void extractTagsHapi(TransactionDetails theTransactionDetails, IResource
List<IdDt> profiles = ResourceMetadataKeyEnum.PROFILES.get(theResource);
if (profiles != null) {
for (IIdType next : profiles) {
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null);
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
allDefs.add(tag);
Expand All @@ -314,7 +319,8 @@ private void extractTagsRi(TransactionDetails theTransactionDetails, IAnyResourc
List<? extends IBaseCoding> tagList = theResource.getMeta().getTag();
if (tagList != null) {
for (IBaseCoding next : tagList) {
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getSystem(), next.getCode(), next.getDisplay());
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getSystem(), next.getCode(),
next.getDisplay(), next.getVersion(), next.getUserSelected());
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
theAllTags.add(tag);
Expand All @@ -326,7 +332,7 @@ private void extractTagsRi(TransactionDetails theTransactionDetails, IAnyResourc
List<? extends IBaseCoding> securityLabels = theResource.getMeta().getSecurity();
if (securityLabels != null) {
for (IBaseCoding next : securityLabels) {
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystem(), next.getCode(), next.getDisplay());
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystem(), next.getCode(), next.getDisplay(), null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
theAllTags.add(tag);
Expand All @@ -338,7 +344,7 @@ private void extractTagsRi(TransactionDetails theTransactionDetails, IAnyResourc
List<? extends IPrimitiveType<String>> profiles = theResource.getMeta().getProfile();
if (profiles != null) {
for (IPrimitiveType<String> next : profiles) {
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null);
TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, next.getValue(), null, null, null);
if (def != null) {
ResourceTag tag = theEntity.addTag(def);
theAllTags.add(tag);
Expand Down Expand Up @@ -376,22 +382,25 @@ public void setContext(FhirContext 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) {
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);
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, () -> new HashMap<>());
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 = getOrCreateTag(theTagType, theScheme, theTerm, theLabel);
retVal = getOrCreateTag(theTagType, theScheme, theTerm, theLabel, theVersion, theUserSelected);

TransactionSynchronization sync = new AddTagDefinitionToCacheAfterCommitSynchronization(key, retVal);
TransactionSynchronizationManager.registerSynchronization(sync);
Expand All @@ -409,26 +418,10 @@ protected TagDefinition getTagOrNull(TransactionDetails theTransactionDetails, T
* <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);
private TagDefinition getOrCreateTag(TagTypeEnum theTagType, String theScheme, String theTerm, String theLabel,
String theVersion, Boolean theUserSelected) {

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);
TypedQuery<TagDefinition> q = buildTagQuery(theTagType, theScheme, theTerm, theVersion, theUserSelected);

TransactionTemplate template = new TransactionTemplate(myTransactionManager);
template.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
Expand All @@ -450,6 +443,8 @@ private TagDefinition readOrCreate() {
val = q.getSingleResult();
} catch (NoResultException e) {
val = new TagDefinition(theTagType, theScheme, theTerm, theLabel);
val.setVersion(theVersion);
val.setUserSelected(theUserSelected);
myEntityManager.persist(val);
}
return val;
Expand Down Expand Up @@ -507,6 +502,34 @@ public TagDefinition doInTransaction(TransactionStatus status) {
return retVal;
}

private TypedQuery<TagDefinition> buildTagQuery(TagTypeEnum theTagType, String theScheme, String theTerm,
String theVersion, Boolean theUserSelected) {
CriteriaBuilder builder = myEntityManager.getCriteriaBuilder();
CriteriaQuery<TagDefinition> cq = builder.createQuery(TagDefinition.class);
Root<TagDefinition> from = cq.from(TagDefinition.class);

List<Predicate> predicates = new ArrayList<>();
predicates.add(
builder.and(
builder.equal(from.get("myTagType"), theTagType),
builder.equal(from.get("myCode"), theTerm)));

predicates.add( isBlank(theScheme)
? builder.isNull(from.get("mySystem"))
: builder.equal(from.get("mySystem"), theScheme));

predicates.add( isBlank(theVersion)
? builder.isNull(from.get("myVersion"))
: builder.equal(from.get("myVersion"), theVersion));

predicates.add( isNull(theUserSelected) || isFalse(theUserSelected)
? builder.isFalse(from.get("myUserSelected"))
: builder.isTrue(from.get("myUserSelected")));

cq.where(predicates.toArray(new Predicate[0]));
return myEntityManager.createQuery(cq);
}


void incrementId(T theResource, ResourceTable theSavedEntity, IIdType theResourceId) {
String newVersion;
Expand Down Expand Up @@ -741,10 +764,10 @@ private boolean updateTags(TransactionDetails theTransactionDetails, RequestDeta
}

RuntimeResourceDefinition def = myContext.getResourceDefinition(theResource);
if (def.isStandardType() == false) {
if ( ! def.isStandardType()) {
String profile = def.getResourceProfile("");
if (isNotBlank(profile)) {
TagDefinition profileDef = getTagOrNull(theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, profile, null);
TagDefinition profileDef = getTagOrNull(theTransactionDetails, TagTypeEnum.PROFILE, NS_JPA_PROFILE, profile, null, null, null);

ResourceTag tag = theEntity.addTag(profileDef);
allDefs.add(tag);
Expand Down Expand Up @@ -1584,8 +1607,9 @@ public void afterCommit() {
}

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

static String cleanProvenanceSourceUri(String theProvenanceSourceUri) {
Expand Down
12 changes: 8 additions & 4 deletions hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
Expand Down Expand Up @@ -740,9 +741,11 @@ private <MT extends IBaseMetaType> void doMetaAdd(MT theMetaAdd, BaseHasResource

boolean hasTag = false;
for (BaseTag next : new ArrayList<>(theEntity.getTags())) {
if (ObjectUtil.equals(next.getTag().getTagType(), nextDef.getTagType()) &&
ObjectUtil.equals(next.getTag().getSystem(), nextDef.getSystem()) &&
ObjectUtil.equals(next.getTag().getCode(), nextDef.getCode())) {
if (Objects.equals(next.getTag().getTagType(), nextDef.getTagType()) &&
Objects.equals(next.getTag().getSystem(), nextDef.getSystem()) &&
Objects.equals(next.getTag().getCode(), nextDef.getCode()) &&
Objects.equals(next.getTag().getVersion(), nextDef.getVersion()) &&
Objects.equals(next.getTag().getUserSelected(), nextDef.getUserSelected())) {
hasTag = true;
break;
}
Expand All @@ -751,7 +754,8 @@ private <MT extends IBaseMetaType> void doMetaAdd(MT theMetaAdd, BaseHasResource
if (!hasTag) {
theEntity.setHasTags(true);

TagDefinition def = getTagOrNull(theTransactionDetails, nextDef.getTagType(), nextDef.getSystem(), nextDef.getCode(), nextDef.getDisplay());
TagDefinition def = getTagOrNull(theTransactionDetails, nextDef.getTagType(), nextDef.getSystem(),
nextDef.getCode(), nextDef.getDisplay(), nextDef.getVersion(), nextDef.getUserSelected());
if (def != null) {
BaseTag newEntity = theEntity.addTag(def);
if (newEntity.getTagId() == null) {
Expand Down
2 changes: 2 additions & 0 deletions hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,8 @@ private <R extends IBaseResource> R populateResourceMetadataRi(IBaseResourceEnti
tag.setSystem(next.getTag().getSystem());
tag.setCode(next.getTag().getCode());
tag.setDisplay(next.getTag().getDisplay());
tag.setVersion(next.getTag().getVersion());
tag.setUserSelected(next.getTag().getUserSelected());
break;
}
}
Expand Down
29 changes: 29 additions & 0 deletions ...jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,38 @@ public HapiFhirJpaMigrationTasks(Set<String> theFlags) {
}

protected void init640() {
Builder version = forVersion(VersionEnum.V6_4_0);

{
Builder.BuilderWithTableName tagDefTable = version.onTable("HFJ_TAG_DEF");

// add columns
tagDefTable
.addColumn("20230209.1", "TAG_VERSION")
.nullable()
.type(ColumnTypeEnum.STRING, 30);
tagDefTable
.addColumn("20230209.2", "TAG_USER_SELECTED")
.nullable()
.type(ColumnTypeEnum.BOOLEAN);

// Update indexing
tagDefTable.dropIndex("20230209.3", "IDX_TAGDEF_TYPESYSCODE");

tagDefTable.dropIndex("20230209.4", "IDX_TAGDEF_TYPESYSCODEVERUS");
Map<DriverTypeEnum, String> addTagDefConstraint = new HashMap<>();
addTagDefConstraint.put(DriverTypeEnum.H2_EMBEDDED, "ALTER TABLE HFJ_TAG_DEF ADD CONSTRAINT IDX_TAGDEF_TYPESYSCODEVERUS UNIQUE (TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_VERSION, TAG_USER_SELECTED)");
addTagDefConstraint.put(DriverTypeEnum.MARIADB_10_1, "ALTER TABLE HFJ_TAG_DEF ADD CONSTRAINT IDX_TAGDEF_TYPESYSCODEVERUS UNIQUE (TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_VERSION, TAG_USER_SELECTED)");
addTagDefConstraint.put(DriverTypeEnum.MSSQL_2012, "ALTER TABLE HFJ_TAG_DEF ADD CONSTRAINT IDX_TAGDEF_TYPESYSCODEVERUS UNIQUE (TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_VERSION, TAG_USER_SELECTED)");
addTagDefConstraint.put(DriverTypeEnum.MYSQL_5_7, "ALTER TABLE HFJ_TAG_DEF ADD CONSTRAINT IDX_TAGDEF_TYPESYSCODEVERUS UNIQUE (TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_VERSION, TAG_USER_SELECTED)");
addTagDefConstraint.put(DriverTypeEnum.ORACLE_12C, "ALTER TABLE HFJ_TAG_DEF ADD CONSTRAINT IDX_TAGDEF_TYPESYSCODEVERUS UNIQUE (TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_VERSION, TAG_USER_SELECTED)");
addTagDefConstraint.put(DriverTypeEnum.POSTGRES_9_4, "ALTER TABLE HFJ_TAG_DEF ADD CONSTRAINT IDX_TAGDEF_TYPESYSCODEVERUS UNIQUE (TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_VERSION, TAG_USER_SELECTED)");
version.executeRawSql("20230209.5", addTagDefConstraint);
}

}


protected void init630() {
Builder version = forVersion(VersionEnum.V6_3_0);

Expand Down
3 changes: 1 addition & 2 deletions hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.commons.lang3.builder.ToStringStyle;
import org.hibernate.annotations.GenericGenerator;
import org.hibernate.Session;
import org.hibernate.annotations.GenerationTime;
import org.hibernate.annotations.GeneratorType;
import org.hibernate.annotations.GenericGenerator;
import org.hibernate.annotations.OptimisticLock;
import org.hibernate.search.engine.backend.types.Projectable;
import org.hibernate.search.engine.backend.types.Searchable;
Expand Down Expand Up @@ -64,7 +64,6 @@
import javax.persistence.OneToOne;
import javax.persistence.PrePersist;
import javax.persistence.PreUpdate;
import org.hibernate.annotations.GenericGenerator;
import javax.persistence.Table;
import javax.persistence.Transient;
import javax.persistence.Version;
Expand Down
Loading

0 comments on commit 9d70dbd

Please sign in to comment.