From f0726a32d58532a096c5b0d7ec6489c0335f76a9 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Fri, 5 May 2023 15:06:03 -0400 Subject: [PATCH] Fixes a bug with tags. (#4813) * Test, fix * Drop constraint, add migration * Add changelog * Fix userSelected null vs false * Fix merge * Fix up checkstyle whining * One more failure * Fix test * wip * changelog clarity Co-authored-by: James Agnew * change index --------- Co-authored-by: Michael Buckley Co-authored-by: James Agnew --- .../main/java/ca/uhn/fhir/model/api/Tag.java | 48 +++--- .../java/ca/uhn/fhir/parser/JsonParser.java | 139 +++++++++--------- .../java/ca/uhn/fhir/model/api/TagTest.java | 2 +- .../canonical/VersionCanonicalizer.java | 4 +- .../6_6_0/4819-tag-userSelected-null.yaml | 4 + .../6_8_0/4813-fix-tag-nonunique.yaml | 4 + .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 24 +-- .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 1 + .../java/ca/uhn/fhir/jpa/dao/CodingSpy.java | 61 ++++++++ .../jpa/dao/JpaStorageResourceParser.java | 29 +++- .../tasks/HapiFhirJpaMigrationTasks.java | 31 +++- .../ca/uhn/fhir/jpa/dao/CodingSpyTest.java | 83 +++++++++++ .../fhir/jpa/model/entity/TagDefinition.java | 16 +- .../jpa/dao/r4/FhirResourceDaoR4MetaTest.java | 28 ++++ .../dao/r4/FhirResourceDaoR4UpdateTest.java | 29 ++++ 15 files changed, 373 insertions(+), 130 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4819-tag-userSelected-null.yaml create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4813-fix-tag-nonunique.yaml create mode 100644 hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/CodingSpy.java create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/CodingSpyTest.java diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/model/api/Tag.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/model/api/Tag.java index 456b0edbd0f..b0173ecc487 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/model/api/Tag.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/model/api/Tag.java @@ -19,13 +19,14 @@ */ package ca.uhn.fhir.model.api; -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; +import java.util.Objects; + /** * A single tag *

@@ -58,7 +59,7 @@ public class Tag extends BaseElement implements IElement, IBaseCoding { private String myScheme; private String myTerm; private String myVersion; - private boolean myUserSelected; + private Boolean myUserSelected; public Tag() { } @@ -114,37 +115,22 @@ public class Tag extends BaseElement implements IElement, IBaseCoding { 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; - } else if (!myTerm.equals(other.myTerm)) - return false; - if (myVersion == null) { - if (other.getVersion() != null) - return false; - } else if (!myVersion.equals(other.getVersion())) - return false; - - if (myUserSelected != other.getUserSelected()) - return false; - - return true; + return + Objects.equals(myScheme, other.myScheme) && + Objects.equals(myTerm, other.myTerm) && + Objects.equals(myVersion, other.myVersion) && + Objects.equals(myUserSelected, other.myUserSelected); } @Override public int hashCode() { final int prime = 31; int result = 1; - result = prime * result + ((myScheme == null) ? 0 : myScheme.hashCode()); - result = prime * result + ((myTerm == null) ? 0 : myTerm.hashCode()); - result = prime * result + ((myVersion == null) ? 0 : myVersion.hashCode()); - result = prime * result + Boolean.hashCode(myUserSelected); + result = prime * result + Objects.hashCode(myScheme); + result = prime * result + Objects.hashCode(myTerm); + result = prime * result + Objects.hashCode(myVersion); + result = prime * result + Objects.hashCode(myUserSelected); return result; } @@ -234,7 +220,9 @@ public class Tag extends BaseElement implements IElement, IBaseCoding { } @Override - public boolean getUserSelected() { return myUserSelected; } + public boolean getUserSelected() { return myUserSelected != null && myUserSelected; } + + public Boolean getUserSelectedBoolean() { return myUserSelected; } @Override public IBaseCoding setUserSelected(boolean theUserSelected) { @@ -242,4 +230,8 @@ public class Tag extends BaseElement implements IElement, IBaseCoding { return this; } + public void setUserSelectedBoolean(Boolean theUserSelected) { + myUserSelected = theUserSelected; + } + } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java index ae56cee0b73..43d29557289 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java @@ -724,73 +724,7 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { } if (theResource instanceof IResource) { - IResource resource = (IResource) theResource; - // Object securityLabelRawObj = - - List securityLabels = extractMetadataListNotNull(resource, ResourceMetadataKeyEnum.SECURITY_LABELS); - List profiles = extractMetadataListNotNull(resource, ResourceMetadataKeyEnum.PROFILES); - profiles = super.getProfileTagsForEncoding(resource, profiles); - - TagList tags = getMetaTagsForEncoding(resource, theEncodeContext); - InstantDt updated = (InstantDt) resource.getResourceMetadata().get(ResourceMetadataKeyEnum.UPDATED); - IdDt resourceId = resource.getId(); - String versionIdPart = resourceId.getVersionIdPart(); - if (isBlank(versionIdPart)) { - versionIdPart = ResourceMetadataKeyEnum.VERSION.get(resource); - } - List, Object>> extensionMetadataKeys = getExtensionMetadataKeys(resource); - - if (super.shouldEncodeResourceMeta(resource) && (ElementUtil.isEmpty(versionIdPart, updated, securityLabels, tags, profiles) == false) || !extensionMetadataKeys.isEmpty()) { - beginObject(theEventWriter, "meta"); - - if (shouldEncodePath(resource, "meta.versionId")) { - writeOptionalTagWithTextNode(theEventWriter, "versionId", versionIdPart); - } - if (shouldEncodePath(resource, "meta.lastUpdated")) { - writeOptionalTagWithTextNode(theEventWriter, "lastUpdated", updated); - } - - if (profiles != null && profiles.isEmpty() == false) { - beginArray(theEventWriter, "profile"); - for (IIdType profile : profiles) { - if (profile != null && isNotBlank(profile.getValue())) { - theEventWriter.write(profile.getValue()); - } - } - theEventWriter.endArray(); - } - - if (securityLabels.isEmpty() == false) { - beginArray(theEventWriter, "security"); - for (BaseCodingDt securityLabel : securityLabels) { - theEventWriter.beginObject(); - theEncodeContext.pushPath("security", false); - encodeCompositeElementChildrenToStreamWriter(resDef, resource, securityLabel, theEventWriter, theContainedResource, null, theEncodeContext); - theEncodeContext.popPath(); - theEventWriter.endObject(); - } - theEventWriter.endArray(); - } - - if (tags != null && tags.isEmpty() == false) { - beginArray(theEventWriter, "tag"); - for (Tag tag : tags) { - if (tag.isEmpty()) { - continue; - } - theEventWriter.beginObject(); - writeOptionalTagWithTextNode(theEventWriter, "system", tag.getScheme()); - writeOptionalTagWithTextNode(theEventWriter, "code", tag.getTerm()); - writeOptionalTagWithTextNode(theEventWriter, "display", tag.getLabel()); - theEventWriter.endObject(); - } - theEventWriter.endArray(); - } - - addExtensionMetadata(theResDef, theResource, theContainedResource, extensionMetadataKeys, resDef, theEventWriter, theEncodeContext); - - theEventWriter.endObject(); // end meta - } + parseMetaForDSTU2(theResDef, theResource, theEventWriter, theContainedResource, theEncodeContext, resDef); } encodeCompositeElementToStreamWriter(theResDef, theResource, theResource, theEventWriter, theContainedResource, new CompositeChildElement(resDef, theEncodeContext), theEncodeContext); @@ -798,6 +732,77 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { theEventWriter.endObject(); } + private void parseMetaForDSTU2(RuntimeResourceDefinition theResDef, IBaseResource theResource, BaseJsonLikeWriter theEventWriter, boolean theContainedResource, EncodeContext theEncodeContext, RuntimeResourceDefinition resDef) throws IOException { + IResource resource = (IResource) theResource; + // Object securityLabelRawObj = + + List securityLabels = extractMetadataListNotNull(resource, ResourceMetadataKeyEnum.SECURITY_LABELS); + List profiles = extractMetadataListNotNull(resource, ResourceMetadataKeyEnum.PROFILES); + profiles = super.getProfileTagsForEncoding(resource, profiles); + + TagList tags = getMetaTagsForEncoding(resource, theEncodeContext); + InstantDt updated = (InstantDt) resource.getResourceMetadata().get(ResourceMetadataKeyEnum.UPDATED); + IdDt resourceId = resource.getId(); + String versionIdPart = resourceId.getVersionIdPart(); + if (isBlank(versionIdPart)) { + versionIdPart = ResourceMetadataKeyEnum.VERSION.get(resource); + } + List, Object>> extensionMetadataKeys = getExtensionMetadataKeys(resource); + + if (super.shouldEncodeResourceMeta(resource) && (ElementUtil.isEmpty(versionIdPart, updated, securityLabels, tags, profiles) == false) || !extensionMetadataKeys.isEmpty()) { + beginObject(theEventWriter, "meta"); + + if (shouldEncodePath(resource, "meta.versionId")) { + writeOptionalTagWithTextNode(theEventWriter, "versionId", versionIdPart); + } + if (shouldEncodePath(resource, "meta.lastUpdated")) { + writeOptionalTagWithTextNode(theEventWriter, "lastUpdated", updated); + } + + if (profiles != null && profiles.isEmpty() == false) { + beginArray(theEventWriter, "profile"); + for (IIdType profile : profiles) { + if (profile != null && isNotBlank(profile.getValue())) { + theEventWriter.write(profile.getValue()); + } + } + theEventWriter.endArray(); + } + + if (securityLabels.isEmpty() == false) { + beginArray(theEventWriter, "security"); + for (BaseCodingDt securityLabel : securityLabels) { + theEventWriter.beginObject(); + theEncodeContext.pushPath("security", false); + encodeCompositeElementChildrenToStreamWriter(resDef, resource, securityLabel, theEventWriter, theContainedResource, null, theEncodeContext); + theEncodeContext.popPath(); + theEventWriter.endObject(); + } + theEventWriter.endArray(); + } + + if (tags != null && tags.isEmpty() == false) { + beginArray(theEventWriter, "tag"); + for (Tag tag : tags) { + if (tag.isEmpty()) { + continue; + } + theEventWriter.beginObject(); + writeOptionalTagWithTextNode(theEventWriter, "system", tag.getScheme()); + writeOptionalTagWithTextNode(theEventWriter, "code", tag.getTerm()); + writeOptionalTagWithTextNode(theEventWriter, "display", tag.getLabel()); + // wipmb should we be writing the new properties here? There must be another path. + theEventWriter.endObject(); + } + theEventWriter.endArray(); + } + + addExtensionMetadata(theResDef, theResource, theContainedResource, extensionMetadataKeys, resDef, theEventWriter, theEncodeContext); + + theEventWriter.endObject(); // end meta + } + } + private void addExtensionMetadata(RuntimeResourceDefinition theResDef, IBaseResource theResource, boolean theContainedResource, diff --git a/hapi-fhir-base/src/test/java/ca/uhn/fhir/model/api/TagTest.java b/hapi-fhir-base/src/test/java/ca/uhn/fhir/model/api/TagTest.java index 02230007192..ab747cf2923 100644 --- a/hapi-fhir-base/src/test/java/ca/uhn/fhir/model/api/TagTest.java +++ b/hapi-fhir-base/src/test/java/ca/uhn/fhir/model/api/TagTest.java @@ -27,7 +27,7 @@ public class TagTest { @Test public void testHashCode() { Tag tag1 = new Tag().setScheme("scheme").setTerm("term").setLabel("label"); - assertEquals(-1029266947, tag1.hashCode()); + assertEquals(-1029268184, tag1.hashCode()); } @Test diff --git a/hapi-fhir-converter/src/main/java/ca/uhn/hapi/converters/canonical/VersionCanonicalizer.java b/hapi-fhir-converter/src/main/java/ca/uhn/hapi/converters/canonical/VersionCanonicalizer.java index f6922cb196b..2aff63449b0 100644 --- a/hapi-fhir-converter/src/main/java/ca/uhn/hapi/converters/canonical/VersionCanonicalizer.java +++ b/hapi-fhir-converter/src/main/java/ca/uhn/hapi/converters/canonical/VersionCanonicalizer.java @@ -364,7 +364,9 @@ public class VersionCanonicalizer { retVal.setSystem(coding.getSystem()); retVal.setDisplay(coding.getDisplay()); retVal.setVersion(coding.getVersion()); - retVal.setUserSelected(!coding.getUserSelectedElement().isEmpty() && coding.getUserSelected()); + if (!coding.getUserSelectedElement().isEmpty()) { + retVal.setUserSelected( coding.getUserSelected() ); + } return retVal; } diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4819-tag-userSelected-null.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4819-tag-userSelected-null.yaml new file mode 100644 index 00000000000..bf5bfd26918 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4819-tag-userSelected-null.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 4819 +title: "Tags no longer provide a default `false` value for `userSelected`." diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4813-fix-tag-nonunique.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4813-fix-tag-nonunique.yaml new file mode 100644 index 00000000000..c7aa497b7d6 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4813-fix-tag-nonunique.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 4813 +title: "Under heavy concurrency, a bug resulted in identical tag definitions being rejected with a `NonUniqueResultException` some of the time. This has been corrected." 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 4c5c0af9930..938b297a2e7 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 @@ -247,6 +247,8 @@ public abstract class BaseHapiFhirDao extends BaseStora @Autowired private PlatformTransactionManager myTransactionManager; + protected final CodingSpy myCodingSpy = new CodingSpy(); + @VisibleForTesting public void setExternallyStoredResourceServiceRegistryForUnitTest(ExternallyStoredResourceServiceRegistry theExternallyStoredResourceServiceRegistry) { myExternallyStoredResourceServiceRegistry = theExternallyStoredResourceServiceRegistry; @@ -283,7 +285,7 @@ public abstract class BaseHapiFhirDao extends BaseStora if (tagList != null) { for (Tag next : tagList) { TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getScheme(), next.getTerm(), - next.getLabel(), next.getVersion(), next.getUserSelected()); + next.getLabel(), next.getVersion(), myCodingSpy.getBooleanObject(next)); if (def != null) { ResourceTag tag = theEntity.addTag(def); allDefs.add(tag); @@ -323,7 +325,7 @@ public abstract class BaseHapiFhirDao extends BaseStora if (tagList != null) { for (IBaseCoding next : tagList) { TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getSystem(), next.getCode(), - next.getDisplay(), next.getVersion(), next.getUserSelected()); + next.getDisplay(), next.getVersion(), myCodingSpy.getBooleanObject(next)); if (def != null) { ResourceTag tag = theEntity.addTag(def); theAllTags.add(tag); @@ -335,7 +337,7 @@ public abstract class BaseHapiFhirDao extends BaseStora List securityLabels = theResource.getMeta().getSecurity(); if (securityLabels != null) { for (IBaseCoding next : securityLabels) { - TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystem(), next.getCode(), next.getDisplay(), null, null); + TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystem(), next.getCode(), next.getDisplay(), next.getVersion(), myCodingSpy.getBooleanObject(next)); if (def != null) { ResourceTag tag = theEntity.addTag(def); theAllTags.add(tag); @@ -394,7 +396,6 @@ public abstract class BaseHapiFhirDao extends BaseStora MemoryCacheService.TagDefinitionCacheKey key = toTagDefinitionMemoryCacheKey(theTagType, theScheme, theTerm, theVersion, theUserSelected); TagDefinition retVal = myMemoryCacheService.getIfPresent(MemoryCacheService.CacheEnum.TAG_DEFINITION, key); - if (retVal == null) { HashMap resolvedTagDefinitions = theTransactionDetails .getOrCreateUserData(HapiTransactionService.XACT_USERDATA_KEY_RESOLVED_TAG_DEFINITIONS, HashMap::new); @@ -425,6 +426,7 @@ public abstract class BaseHapiFhirDao extends BaseStora String theVersion, Boolean theUserSelected) { TypedQuery q = buildTagQuery(theTagType, theScheme, theTerm, theVersion, theUserSelected); + q.setMaxResults(1); TransactionTemplate template = new TransactionTemplate(myTransactionManager); template.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); @@ -525,9 +527,9 @@ public abstract class BaseHapiFhirDao extends BaseStora ? 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"))); + predicates.add( isNull(theUserSelected) + ? builder.isNull(from.get("myUserSelected")) + : builder.equal(from.get("myUserSelected"), theUserSelected)); cq.where(predicates.toArray(new Predicate[0])); return myEntityManager.createQuery(cq); @@ -817,12 +819,14 @@ public abstract class BaseHapiFhirDao extends BaseStora // Update the resource to contain the old tags allTagsOld.forEach(tag -> { - theResource.getMeta() + IBaseCoding iBaseCoding = theResource.getMeta() .addTag() .setCode(tag.getTag().getCode()) .setSystem(tag.getTag().getSystem()) - .setVersion(tag.getTag().getVersion()) - .setUserSelected(tag.getTag().getUserSelected()); + .setVersion(tag.getTag().getVersion()); + if (tag.getTag().getUserSelected() != null) { + iBaseCoding.setUserSelected(tag.getTag().getUserSelected()); + } }); theEntity.setHasTags(!allTagsNew.isEmpty()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java index 2f990493b33..d48ba0719fe 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java @@ -834,6 +834,7 @@ public abstract class BaseHapiFhirResourceDao extends B } } + if (!hasTag) { theEntity.setHasTags(true); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/CodingSpy.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/CodingSpy.java new file mode 100644 index 00000000000..d9dfede8480 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/CodingSpy.java @@ -0,0 +1,61 @@ +package ca.uhn.fhir.jpa.dao; + +import ca.uhn.fhir.i18n.Msg; +import ca.uhn.fhir.model.primitive.BooleanDt; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.hl7.fhir.instance.model.api.IBaseBooleanDatatype; +import org.hl7.fhir.instance.model.api.IBaseCoding; + +import java.lang.reflect.Field; +import java.util.HashMap; +import java.util.Map; + +/** + * We are trying to preserve null behaviour despite IBaseCoding using primitive boolean for userSelected. + */ +public class CodingSpy { + final Map mySpies = new HashMap<>(); + + /** + * Reach into the Coding and pull out the Boolean instead of the boolean. + * @param theValue the Coding, or CodingDt + * @return the Boolean + */ + public Boolean getBooleanObject(IBaseCoding theValue) { + Field spy = getSpy(theValue); + try { + Object o = spy.get(theValue); + if (o == null) { + return null; + } + if (o instanceof BooleanDt) { + BooleanDt booleanDt = (BooleanDt) o; + return booleanDt.getValue(); + } + if (o instanceof IBaseBooleanDatatype) { + IBaseBooleanDatatype booleanValue = (IBaseBooleanDatatype) o; + return booleanValue.getValue(); + } + if (o instanceof Boolean) { + return (Boolean) o; + } + throw new RuntimeException(Msg.code(2342) + "unsupported type :" + theValue.getClass().getName()); + } catch (IllegalAccessException theException) { + // should never happen - all Coding models have this field. + throw new RuntimeException(Msg.code(2343) + "illegal access during reflection", theException); + } + } + + private Field getSpy(IBaseCoding theValue) { + return mySpies.computeIfAbsent(theValue.getClass(), k -> getFieldHandle(k)); + } + + private static Field getFieldHandle(Class k) { + Field result = FieldUtils.getField(k, "userSelected", true); + if (result == null) { + result = FieldUtils.getField(k, "myUserSelected", true); + } + return result; + } + +} diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java index 71185b1c9a0..776a9b6bc4b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java @@ -39,6 +39,7 @@ import ca.uhn.fhir.jpa.model.entity.ResourceEncodingEnum; import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable; import ca.uhn.fhir.jpa.model.entity.ResourceTable; import ca.uhn.fhir.jpa.model.entity.ResourceTag; +import ca.uhn.fhir.jpa.model.entity.TagDefinition; import ca.uhn.fhir.jpa.model.entity.TagTypeEnum; import ca.uhn.fhir.jpa.partition.IPartitionLookupSvc; import ca.uhn.fhir.model.api.IResource; @@ -72,7 +73,7 @@ import java.util.Date; import java.util.List; import static ca.uhn.fhir.jpa.dao.BaseHapiFhirDao.decodeResource; -import static org.apache.commons.lang3.StringUtils.defaultString; +import static java.util.Objects.nonNull; import static org.apache.commons.lang3.StringUtils.isNotBlank; public class JpaStorageResourceParser implements IJpaStorageResourceParser { @@ -350,19 +351,26 @@ public class JpaStorageResourceParser implements IJpaStorageResourceParser { List securityLabels = new ArrayList<>(); List profiles = new ArrayList<>(); for (BaseTag next : theTagList) { - switch (next.getTag().getTagType()) { + TagDefinition nextTag = next.getTag(); + switch (nextTag.getTagType()) { case PROFILE: - profiles.add(new IdDt(next.getTag().getCode())); + profiles.add(new IdDt(nextTag.getCode())); break; case SECURITY_LABEL: IBaseCoding secLabel = (IBaseCoding) myFhirContext.getVersion().newCodingDt(); - secLabel.setSystem(next.getTag().getSystem()); - secLabel.setCode(next.getTag().getCode()); - secLabel.setDisplay(next.getTag().getDisplay()); + secLabel.setSystem(nextTag.getSystem()); + secLabel.setCode(nextTag.getCode()); + secLabel.setDisplay(nextTag.getDisplay()); + // wipmb these technically support userSelected and version securityLabels.add(secLabel); break; case TAG: - tagList.add(new Tag(next.getTag().getSystem(), next.getTag().getCode(), next.getTag().getDisplay())); + // wipmb check xml, etc. + Tag e = new Tag(nextTag.getSystem(), nextTag.getCode(), nextTag.getDisplay()); + e.setVersion(nextTag.getVersion()); + // careful! These are Boolean, not boolean. + e.setUserSelectedBoolean(nextTag.getUserSelected()); + tagList.add(e); break; } } @@ -434,7 +442,12 @@ public class JpaStorageResourceParser implements IJpaStorageResourceParser { tag.setCode(next.getTag().getCode()); tag.setDisplay(next.getTag().getDisplay()); tag.setVersion(next.getTag().getVersion()); - tag.setUserSelected(next.getTag().getUserSelected()); + Boolean userSelected = next.getTag().getUserSelected(); + // the tag is created with a null userSelected, but the api is primitive boolean. + // Only update if we are non-null. + if (nonNull(userSelected)) { + tag.setUserSelected(userSelected); + } break; } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java index 3b259a94513..18db9408637 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java @@ -101,13 +101,26 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks { protected void init680() { Builder version = forVersion(VersionEnum.V6_8_0); - // HAPI-FHIR #4801 - Add New Index On HFJ_RESOURCE - Builder.BuilderWithTableName resourceTable = version.onTable("HFJ_RESOURCE"); - resourceTable - .addIndex("20230502.1", "IDX_RES_RESID_UPDATED") + // HAPI-FHIR #4801 - Add New Index On HFJ_RESOURCE + Builder.BuilderWithTableName resourceTable = version.onTable("HFJ_RESOURCE"); + resourceTable + .addIndex("20230502.1", "IDX_RES_RESID_UPDATED") + .unique(false) + .online(true) + .withColumns("RES_ID", "RES_UPDATED", "PARTITION_ID"); + + Builder.BuilderWithTableName tagDefTable = version.onTable("HFJ_TAG_DEF"); + tagDefTable.dropIndex("20230505.1", "IDX_TAGDEF_TYPESYSCODEVERUS"); + + tagDefTable.dropIndex("20230505.2", "IDX_TAG_DEF_TP_CD_SYS"); + tagDefTable + .addIndex("20230505.3", "IDX_TAG_DEF_TP_CD_SYS") .unique(false) - .online(true) - .withColumns("RES_ID", "RES_UPDATED", "PARTITION_ID"); + .online(false) + .withColumns("TAG_TYPE", "TAG_CODE", "TAG_SYSTEM", "TAG_ID", "TAG_VERSION", "TAG_USER_SELECTED"); + + + } protected void init660() { @@ -362,8 +375,14 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks { .online(true) .withColumns("SEARCH_PID") .onlyAppliesToPlatforms(NON_AUTOMATIC_FK_INDEX_PLATFORMS); + + { //We added this constraint when userSelected and Version were added. It is no longer necessary. + Builder.BuilderWithTableName tagDefTable = version.onTable("HFJ_TAG_DEF"); + tagDefTable.dropIndex("20230503.1", "IDX_TAGDEF_TYPESYSCODEVERUS"); + } } + private void init620() { Builder version = forVersion(VersionEnum.V6_2_0); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/CodingSpyTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/CodingSpyTest.java new file mode 100644 index 00000000000..f321c205e02 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/CodingSpyTest.java @@ -0,0 +1,83 @@ +package ca.uhn.fhir.jpa.dao; + +import com.google.common.collect.Lists; +import org.hl7.fhir.instance.model.api.IBaseCoding; +import org.hl7.fhir.r4.model.BooleanType; +import org.hl7.fhir.r4.model.Coding; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import javax.annotation.Nonnull; +import java.util.List; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.blankOrNullString; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +class CodingSpyTest { + + /** + * Ensure we can read the default null value of userSelected on Coding + */ + + @ParameterizedTest + @MethodSource("getCases") + void canReadValueUserSelected(IBaseCoding theObject, Boolean theValue) { + IBaseCoding value = theObject.setSystem("http://example.com").setCode("value"); + if (theValue != null) { + theObject.setUserSelected(theValue); + } + + Boolean result = new CodingSpy().getBooleanObject(theObject); + + assertEquals(theValue, result); + } + + @Test + void canReadNullUserSelected() { + Coding value = new Coding().setSystem("http://example.com").setCode("value"); + + Boolean result = new CodingSpy().getBooleanObject(value); + + assertNull(result); + } + + static List getCases() { + var classes = List.of( + org.hl7.fhir.r4.model.Coding.class, + org.hl7.fhir.r5.model.Coding.class, + ca.uhn.fhir.model.api.Tag.class + ); + var values = Lists.newArrayList(true, false, null); + return classes.stream() + .flatMap(k-> values.stream() + .map(v-> Arguments.of(getNewInstance(k), v))) + .toList(); + } + + @Nonnull + private static Object getNewInstance(Class k) { + try { + return k.getDeclaredConstructor().newInstance(); + } catch (Exception theE) { + throw new RuntimeException(theE); + } + } + + + @Test + void booleanNulls() { + // given + BooleanType b = new BooleanType(); + + // when + var s = b.asStringValue(); + + assertThat(s, blankOrNullString()); + } + + +} diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/TagDefinition.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/TagDefinition.java index 72ccb2a14d5..c3e260f8fde 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/TagDefinition.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/TagDefinition.java @@ -46,11 +46,7 @@ import java.util.Collection; @Table( name = "HFJ_TAG_DEF", indexes = { - @Index(name = "IDX_TAG_DEF_TP_CD_SYS", columnList = "TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_ID"), - }, - uniqueConstraints = { - @UniqueConstraint(name = "IDX_TAGDEF_TYPESYSCODEVERUS", - columnNames = {"TAG_TYPE", "TAG_SYSTEM", "TAG_CODE", "TAG_VERSION", "TAG_USER_SELECTED"}) + @Index(name = "IDX_TAG_DEF_TP_CD_SYS", columnList = "TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_ID, TAG_VERSION, TAG_USER_SELECTED"), } ) public class TagDefinition implements Serializable { @@ -161,14 +157,16 @@ public class TagDefinition implements Serializable { } } + /** + * Warning - this is nullable, while IBaseCoding getUserSelected isn't. + * wipmb maybe rename? + */ public Boolean getUserSelected() { - // TODO: LD: this is not ideal as we are implicitly assuming null is false. - // Ideally we should fix IBaseCoding to return wrapper Boolean but that will involve another core/hapi release - return myUserSelected != null ? myUserSelected : false; + return myUserSelected; } public void setUserSelected(Boolean theUserSelected) { - myUserSelected = theUserSelected != null && theUserSelected; + myUserSelected = theUserSelected; } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4MetaTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4MetaTest.java index fa89ec23990..8584d4be187 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4MetaTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4MetaTest.java @@ -9,6 +9,7 @@ import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.param.TokenParam; import ca.uhn.fhir.system.HapiTestSystemProperties; +import ch.qos.logback.classic.Level; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.Bundle; import org.hl7.fhir.r4.model.Coding; @@ -41,6 +42,8 @@ import static org.hamcrest.Matchers.empty; import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -153,6 +156,30 @@ public class FhirResourceDaoR4MetaTest extends BaseJpaR4Test { assertEquals("bar", patient2.getMeta().getSecurityFirstRep().getCode()); } + /** + * Make sure round-trips with tags don't add a userSelected property. + * wipmb tag userSelected test + * Verify https://github.com/hapifhir/hapi-fhir/issues/4819 + */ + @Test + void testAddTag_userSelectedStaysNull() { + // given + Patient patient1 = new Patient(); + patient1.getMeta().addTag().setSystem("http://foo").setCode("bar"); + patient1.setActive(true); + IIdType pid1 = myPatientDao.create(patient1).getId(); + + // when + var patientReadback = myPatientDao.read(pid1); + ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(patientReadback)); + + // then + var tag = patientReadback.getMeta().getTag().get(0); + assertNotNull(tag); + assertNull(tag.getUserSelectedElement().asStringValue()); + } + + @Nested public class TestTagWithVersionAndUserSelected { @@ -204,6 +231,7 @@ public class FhirResourceDaoR4MetaTest extends BaseJpaR4Test { .setSystem(expectedSystem1) .setCode(expectedCode1) .setDisplay(expectedDisplay1); + assertNull(newTag.getUserSelectedElement().asStringValue()); assertFalse(newTag.getUserSelected()); newTag.setVersion(expectedVersion1) .setUserSelected(expectedUserSelected1); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java index 61cf03ba0f3..07552eeb139 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java @@ -5,6 +5,8 @@ import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; import ca.uhn.fhir.jpa.model.dao.JpaPid; import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable; import ca.uhn.fhir.jpa.model.entity.ResourceTable; +import ca.uhn.fhir.jpa.model.entity.TagDefinition; +import ca.uhn.fhir.jpa.model.entity.TagTypeEnum; import ca.uhn.fhir.jpa.model.util.JpaConstants; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; @@ -96,6 +98,33 @@ public class FhirResourceDaoR4UpdateTest extends BaseJpaR4Test { } + @Test + public void testTagCollision() { + TagDefinition def = new TagDefinition(); + def.setTagType(TagTypeEnum.TAG); + def.setSystem("system"); + def.setCode("coding"); + def.setDisplay("display"); + def.setUserSelected(null); + + + Patient p = new Patient(); + p.getMeta().addTag("system", "coding", "display"); + + myMemoryCacheService.invalidateAllCaches(); + myPatientDao.create(p, new SystemRequestDetails()); + //inject conflicting. + myTagDefinitionDao.saveAndFlush(def); + myMemoryCacheService.invalidateAllCaches(); + + myPatientDao.create(p, new SystemRequestDetails()); + myMemoryCacheService.invalidateAllCaches(); + + myPatientDao.create(p, new SystemRequestDetails()); + + } + + @Test public void testCreateAndUpdateWithoutRequest() { String methodName = "testUpdateByUrl";