From 734bb3cbff368f78c76b7c2a7cf9716d9bb771d3 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 28 Mar 2023 12:25:59 -0400 Subject: [PATCH] Ensure querying $mdm-link-history on only golden resource or resource only does not result in an error (#4681) * Ensure querying $mdm-link-history on only golden resource or resource ID does not result in an error. * Change MdmHistorySearchParameters semantics back to set**** so we don't need to a hapi-fhir to cdr bump. * Remove envers disabling from HibernatePropertiesProvider.getDialect as by then it's too late. * Update hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4680-mdm-link-history-only-one-of-each-param-no-error.yaml Co-authored-by: Ken Stevens --------- Co-authored-by: Ken Stevens --- ...story-only-one-of-each-param-no-error.yaml | 4 ++ .../config/HibernatePropertiesProvider.java | 7 -- .../fhir/jpa/dao/mdm/MdmLinkDaoJpaImpl.java | 19 ++++- .../fhir/jpa/mdm/dao/MdmLinkDaoSvcTest.java | 70 +++++++++++++++++++ .../mdm/api/MdmHistorySearchParameters.java | 5 +- .../fhir/mdm/provider/BaseMdmProvider.java | 2 +- 6 files changed, 95 insertions(+), 12 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4680-mdm-link-history-only-one-of-each-param-no-error.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4680-mdm-link-history-only-one-of-each-param-no-error.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4680-mdm-link-history-only-one-of-each-param-no-error.yaml new file mode 100644 index 00000000000..580fc44306a --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4680-mdm-link-history-only-one-of-each-param-no-error.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 4680 +title: "Previously, invoking $mdm-link-history on only the golden resource ID or source ID would fail due to faulty validation. This has been corrected." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/HibernatePropertiesProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/HibernatePropertiesProvider.java index 1848e7fd54b..1ca901a5752 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/HibernatePropertiesProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/HibernatePropertiesProvider.java @@ -57,13 +57,6 @@ public class HibernatePropertiesProvider { myDialect = dialect; } - if (myEntityManagerFactory != null) { - final Map jpaPropertyMap = myEntityManagerFactory.getJpaPropertyMap(); - if (! jpaPropertyMap.containsKey(Constants.HIBERNATE_INTEGRATION_ENVERS_ENABLED)) { - jpaPropertyMap.put(Constants.HIBERNATE_INTEGRATION_ENVERS_ENABLED, myStorageSettings.isNonResourceDbHistoryEnabled()); - } - } - return dialect; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/mdm/MdmLinkDaoJpaImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/mdm/MdmLinkDaoJpaImpl.java index 9203f81d60c..b7b3572145c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/mdm/MdmLinkDaoJpaImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/mdm/MdmLinkDaoJpaImpl.java @@ -45,6 +45,7 @@ import org.hibernate.envers.AuditReader; import org.hibernate.envers.RevisionType; import org.hibernate.envers.query.AuditEntity; import org.hibernate.envers.query.AuditQueryCreator; +import org.hibernate.envers.query.criteria.AuditCriterion; import org.hl7.fhir.instance.model.api.IIdType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -332,10 +333,24 @@ public class MdmLinkDaoJpaImpl implements IMdmLinkDao { final AuditQueryCreator auditQueryCreator = myAuditReader.createQuery(); try { + final AuditCriterion goldenResourceIdCriterion = AuditEntity.property(GOLDEN_RESOURCE_PID_NAME).in(convertToLongIds(theMdmHistorySearchParameters.getGoldenResourceIds())); + final AuditCriterion resourceIdCriterion = AuditEntity.property(SOURCE_PID_NAME).in(convertToLongIds(theMdmHistorySearchParameters.getSourceIds())); + + final AuditCriterion goldenResourceAndOrResourceIdCriterion; + + if (! theMdmHistorySearchParameters.getGoldenResourceIds().isEmpty() && ! theMdmHistorySearchParameters.getSourceIds().isEmpty()) { + goldenResourceAndOrResourceIdCriterion = AuditEntity.or(goldenResourceIdCriterion, resourceIdCriterion); + } else if (! theMdmHistorySearchParameters.getGoldenResourceIds().isEmpty()) { + goldenResourceAndOrResourceIdCriterion = goldenResourceIdCriterion; + } else if (! theMdmHistorySearchParameters.getSourceIds().isEmpty()) { + goldenResourceAndOrResourceIdCriterion = resourceIdCriterion; + } else { + throw new IllegalArgumentException(Msg.code(2298) + "$mdm-link-history Golden resource and source query IDs cannot both be empty."); + } + @SuppressWarnings("unchecked") final List mdmLinksWithRevisions = auditQueryCreator.forRevisionsOfEntity(MdmLink.class, false, false) - .add(AuditEntity.or(AuditEntity.property(GOLDEN_RESOURCE_PID_NAME).in(convertToLongIds(theMdmHistorySearchParameters.getGoldenResourceIds())), - AuditEntity.property(SOURCE_PID_NAME).in(convertToLongIds(theMdmHistorySearchParameters.getSourceIds())))) + .add(goldenResourceAndOrResourceIdCriterion) .addOrder(AuditEntity.property(GOLDEN_RESOURCE_PID_NAME).asc()) .addOrder(AuditEntity.property(SOURCE_PID_NAME).asc()) .addOrder(AuditEntity.revisionNumber().desc()) diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/dao/MdmLinkDaoSvcTest.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/dao/MdmLinkDaoSvcTest.java index cb1e103c84b..27c9e8835c6 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/dao/MdmLinkDaoSvcTest.java +++ b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/dao/MdmLinkDaoSvcTest.java @@ -38,6 +38,7 @@ import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; public class MdmLinkDaoSvcTest extends BaseMdmR4Test { @@ -144,6 +145,75 @@ public class MdmLinkDaoSvcTest extends BaseMdmR4Test { assertMdmRevisionsEqual(expectedMdLinkRevisions, actualMdmLinkRevisions); } + @Test + public void testHistoryForGoldenResourceIdsOnly() { + final List mdmLinksWithLinkedPatients1 = createMdmLinksWithLinkedPatients(MdmMatchResultEnum.MATCH, 3); + createMdmLinksWithLinkedPatients(MdmMatchResultEnum.MATCH, 4); + final List mdmLinksWithLinkedPatients3 = createMdmLinksWithLinkedPatients(MdmMatchResultEnum.MATCH, 2); + flipLinksTo(mdmLinksWithLinkedPatients3, MdmMatchResultEnum.NO_MATCH); + + final MdmHistorySearchParameters mdmHistorySearchParameters = + new MdmHistorySearchParameters() + .setGoldenResourceIds(getIdsFromMdmLinks(MdmLink::getGoldenResourcePersistenceId, mdmLinksWithLinkedPatients1.get(0), mdmLinksWithLinkedPatients3.get(0))); + + final List> actualMdmLinkRevisions = myMdmLinkDaoSvc.findMdmLinkHistory(mdmHistorySearchParameters); + + final JpaPid goldenResourceId1 = mdmLinksWithLinkedPatients1.get(0).getGoldenResourcePersistenceId(); + final JpaPid goldenResourceId3 = mdmLinksWithLinkedPatients3.get(0).getGoldenResourcePersistenceId(); + + final JpaPid sourceId1_1 = mdmLinksWithLinkedPatients1.get(0).getSourcePersistenceId(); + final JpaPid sourceId1_2 = mdmLinksWithLinkedPatients1.get(1).getSourcePersistenceId(); + final JpaPid sourceId1_3 = mdmLinksWithLinkedPatients1.get(2).getSourcePersistenceId(); + + final JpaPid sourceId3_1 = mdmLinksWithLinkedPatients3.get(0).getSourcePersistenceId(); + final JpaPid sourceId3_2 = mdmLinksWithLinkedPatients3.get(1).getSourcePersistenceId(); + + final List> expectedMdLinkRevisions = List.of( + buildMdmLinkWithRevision(1, RevisionType.ADD, MdmMatchResultEnum.MATCH, goldenResourceId1, sourceId1_1), + buildMdmLinkWithRevision(2, RevisionType.ADD, MdmMatchResultEnum.MATCH, goldenResourceId1, sourceId1_2), + buildMdmLinkWithRevision(3, RevisionType.ADD, MdmMatchResultEnum.MATCH, goldenResourceId1, sourceId1_3), + buildMdmLinkWithRevision(10, RevisionType.MOD, MdmMatchResultEnum.NO_MATCH, goldenResourceId3, sourceId3_1), + buildMdmLinkWithRevision(8, RevisionType.ADD, MdmMatchResultEnum.MATCH, goldenResourceId3, sourceId3_1), + buildMdmLinkWithRevision(11, RevisionType.MOD, MdmMatchResultEnum.NO_MATCH, goldenResourceId3, sourceId3_2), + buildMdmLinkWithRevision(9, RevisionType.ADD, MdmMatchResultEnum.MATCH, goldenResourceId3, sourceId3_2) + ); + + assertMdmRevisionsEqual(expectedMdLinkRevisions, actualMdmLinkRevisions); + } + + @Test + public void testHistoryForSourceIdsOnly() { + final List mdmLinksWithLinkedPatients1 = createMdmLinksWithLinkedPatients(MdmMatchResultEnum.MATCH, 3); + final List mdmLinksWithLinkedPatients2 = createMdmLinksWithLinkedPatients(MdmMatchResultEnum.MATCH, 4); + final List mdmLinksWithLinkedPatients3 = createMdmLinksWithLinkedPatients(MdmMatchResultEnum.MATCH, 2); + flipLinksTo(mdmLinksWithLinkedPatients3, MdmMatchResultEnum.NO_MATCH); + + final MdmHistorySearchParameters mdmHistorySearchParameters = + new MdmHistorySearchParameters() + .setSourceIds(getIdsFromMdmLinks(MdmLink::getSourcePersistenceId, mdmLinksWithLinkedPatients1.get(0), mdmLinksWithLinkedPatients2.get(0))); + + final List> actualMdmLinkRevisions = myMdmLinkDaoSvc.findMdmLinkHistory(mdmHistorySearchParameters); + + final JpaPid goldenResourceId1 = mdmLinksWithLinkedPatients1.get(0).getGoldenResourcePersistenceId(); + final JpaPid goldenResourceId2 = mdmLinksWithLinkedPatients2.get(0).getGoldenResourcePersistenceId(); + + final JpaPid sourceId1_1 = mdmLinksWithLinkedPatients1.get(0).getSourcePersistenceId(); + + final JpaPid sourceId2_1 = mdmLinksWithLinkedPatients2.get(0).getSourcePersistenceId(); + + final List> expectedMdLinkRevisions = List.of( + buildMdmLinkWithRevision(1, RevisionType.ADD, MdmMatchResultEnum.MATCH, goldenResourceId1, sourceId1_1), + buildMdmLinkWithRevision(4, RevisionType.ADD, MdmMatchResultEnum.MATCH, goldenResourceId2, sourceId2_1) + ); + + assertMdmRevisionsEqual(expectedMdLinkRevisions, actualMdmLinkRevisions); + } + + @Test + public void testHistoryForNoIdsOnly() { + assertThrows(IllegalArgumentException.class, () -> myMdmLinkDaoSvc.findMdmLinkHistory(new MdmHistorySearchParameters())); + } + @Nonnull private static List getIdsFromMdmLinks(Function getIdFunction, MdmLink... mdmLinks) { return Arrays.stream(mdmLinks) diff --git a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/api/MdmHistorySearchParameters.java b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/api/MdmHistorySearchParameters.java index 747dc86fa19..c4a9e230a9c 100644 --- a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/api/MdmHistorySearchParameters.java +++ b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/api/MdmHistorySearchParameters.java @@ -27,13 +27,14 @@ import org.hl7.fhir.instance.model.api.IIdType; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; public class MdmHistorySearchParameters { - private List myGoldenResourceIds; - private List mySourceIds; + private List myGoldenResourceIds = new ArrayList<>(); + private List mySourceIds = new ArrayList<>(); public MdmHistorySearchParameters() {} diff --git a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/provider/BaseMdmProvider.java b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/provider/BaseMdmProvider.java index 94d259d1246..cea40b6571e 100644 --- a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/provider/BaseMdmProvider.java +++ b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/provider/BaseMdmProvider.java @@ -73,7 +73,7 @@ public abstract class BaseMdmProvider { } private void validateBothCannotBeNullOrEmpty(String theFirstName, List> theFirstList, String theSecondName, List> theSecondList) { - if ((theFirstList == null || theFirstList.isEmpty()) || (theSecondList == null || theSecondList.isEmpty())) { + if ((theFirstList == null || theFirstList.isEmpty()) && (theSecondList == null || theSecondList.isEmpty())) { throw new InvalidRequestException(Msg.code(2292) + "both ["+theFirstName+"] and ["+theSecondName+"] cannot be null or empty"); } }