Issue 4486 mdm inconsistent possible match score values (#4487)

* Extract method for readability

* Save always normalized score values in POSSIBLE_MATCH links.

* Avoid setting properties to null values. Adjust test.

* Simplify fix

* Fix test. Add RangeTestHelper.

---------

Co-authored-by: juan.marchionatto <[email protected]>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4486-mdm-inconsistent-possible-match-scores.yaml

@@ -0,0 +1,3 @@
+type: fix
+issue: 4486
+title: "Previously, some MDM links of type `POSSIBLE_MATCH` were saved with unnormalized score values. This has been fixed."

hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/dao/MdmLinkDaoSvc.java

@@ -73,11 +73,9 @@ public M createOrUpdateLinkEntity(IAnyResource theGoldenResource, IAnyResource t
  mdmLink.setEidMatch(theMatchOutcome.isEidMatch() | mdmLink.isEidMatchPresent());
  mdmLink.setHadToCreateNewGoldenResource(theMatchOutcome.isCreatedNewResource() | mdmLink.getHadToCreateNewGoldenResource());
  mdmLink.setMdmSourceType(myFhirContext.getResourceType(theSourceResource));
- if (mdmLink.getScore() != null) {
- mdmLink.setScore(Math.max(theMatchOutcome.score, mdmLink.getScore()));
- } else {
- mdmLink.setScore(theMatchOutcome.score);
- }
+
+ setScoreProperties(theMatchOutcome, mdmLink);
+
  // Add partition for the mdm link if it's available in the source resource
  RequestPartitionId partitionId = (RequestPartitionId) theSourceResource.getUserData(Constants.RESOURCE_PARTITION_ID);
  if (partitionId != null && partitionId.getFirstPartitionIdOrNull() != null) {
@@ -91,6 +89,24 @@ public M createOrUpdateLinkEntity(IAnyResource theGoldenResource, IAnyResource t
  return mdmLink;
  }
 
+ private void setScoreProperties(MdmMatchOutcome theMatchOutcome, M mdmLink) {
+ if (theMatchOutcome.getScore() != null) {
+ mdmLink.setScore( mdmLink.getScore() != null
+ ? Math.max(theMatchOutcome.getNormalizedScore(), mdmLink.getScore())
+ : theMatchOutcome.getNormalizedScore() );
+ }
+
+ if (theMatchOutcome.getVector() != null) {
+ mdmLink.setVector( mdmLink.getVector() != null
+ ? Math.max(theMatchOutcome.getVector(), mdmLink.getVector())
+ : theMatchOutcome.getVector() );
+ }
+
+ mdmLink.setRuleCount( mdmLink.getRuleCount() != null
+ ? Math.max(theMatchOutcome.getMdmRuleCount(), mdmLink.getRuleCount())
+ : theMatchOutcome.getMdmRuleCount() );
+ }
+
  @Nonnull
  public M getOrCreateMdmLinkByGoldenResourceAndSourceResource(
  IAnyResource theGoldenResource, IAnyResource theSourceResource
@@ -127,7 +143,6 @@ public Optional<M> getLinkByGoldenResourcePidAndSourceResourcePid(Long theGolden
  * @param theSourceResourcePid The ResourcepersistenceId of the Source resource
  * @return The {@link IMdmLink} entity that matches these criteria if exists
  */
- @SuppressWarnings("unchecked")
  public Optional<M> getLinkByGoldenResourcePidAndSourceResourcePid(P theGoldenResourcePid, P theSourceResourcePid) {
  if (theSourceResourcePid == null || theGoldenResourcePid == null) {
  return Optional.empty();

hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/MdmMatchLinkSvc.java

@@ -94,7 +94,7 @@ private MdmTransactionContext doMdmUpdate(IAnyResource theResource, MdmTransacti
 
  private void handleMdmWithMultipleCandidates(IAnyResource theResource, CandidateList theCandidateList, MdmTransactionContext theMdmTransactionContext) {
  MatchedGoldenResourceCandidate firstMatch = theCandidateList.getFirstMatch();
- IResourcePersistentId sampleGoldenResourcePid = firstMatch.getCandidateGoldenResourcePid();
+ IResourcePersistentId<?> sampleGoldenResourcePid = firstMatch.getCandidateGoldenResourcePid();
  boolean allSameGoldenResource = theCandidateList.stream()
  .allMatch(candidate -> candidate.getCandidateGoldenResourcePid().equals(sampleGoldenResourcePid));
 
@@ -105,17 +105,7 @@ private void handleMdmWithMultipleCandidates(IAnyResource theResource, Candidate
  log(theMdmTransactionContext, "MDM received multiple match candidates, that were linked to different Golden Resources. Setting POSSIBLE_DUPLICATES and POSSIBLE_MATCHES.");
 
  //Set them all as POSSIBLE_MATCH
- List<IAnyResource> goldenResources = new ArrayList<>();
- for (MatchedGoldenResourceCandidate matchedGoldenResourceCandidate : theCandidateList.getCandidates()) {
- IAnyResource goldenResource = myMdmGoldenResourceFindingSvc
- .getGoldenResourceFromMatchedGoldenResourceCandidate(matchedGoldenResourceCandidate, theMdmTransactionContext.getResourceType());
- MdmMatchOutcome outcome = new MdmMatchOutcome(matchedGoldenResourceCandidate.getMatchResult().vector,
- matchedGoldenResourceCandidate.getMatchResult().getNormalizedScore());
- outcome.setMatchResultEnum(MdmMatchResultEnum.POSSIBLE_MATCH);
- outcome.setEidMatch(theCandidateList.isEidMatch());
- myMdmLinkSvc.updateLink(goldenResource, theResource, outcome, MdmLinkSourceEnum.AUTO, theMdmTransactionContext);
- goldenResources.add(goldenResource);
- }
+ List<IAnyResource> goldenResources = createPossibleMatches(theResource, theCandidateList, theMdmTransactionContext);
 
  //Set all GoldenResources as POSSIBLE_DUPLICATE of the last GoldenResource.
  IAnyResource firstGoldenResource = goldenResources.get(0);
@@ -129,6 +119,26 @@ private void handleMdmWithMultipleCandidates(IAnyResource theResource, Candidate
  }
  }
 
+ private List<IAnyResource> createPossibleMatches(IAnyResource theResource, CandidateList theCandidateList, MdmTransactionContext theMdmTransactionContext) {
+ List<IAnyResource> goldenResources = new ArrayList<>();
+
+ for (MatchedGoldenResourceCandidate matchedGoldenResourceCandidate : theCandidateList.getCandidates()) {
+ IAnyResource goldenResource = myMdmGoldenResourceFindingSvc
+ .getGoldenResourceFromMatchedGoldenResourceCandidate(matchedGoldenResourceCandidate, theMdmTransactionContext.getResourceType());
+
+ MdmMatchOutcome outcome = new MdmMatchOutcome(matchedGoldenResourceCandidate.getMatchResult().getVector(),
+ matchedGoldenResourceCandidate.getMatchResult().getScore())
+ .setMdmRuleCount( matchedGoldenResourceCandidate.getMatchResult().getMdmRuleCount());
+
+ outcome.setMatchResultEnum(MdmMatchResultEnum.POSSIBLE_MATCH);
+ outcome.setEidMatch(theCandidateList.isEidMatch());
+ myMdmLinkSvc.updateLink(goldenResource, theResource, outcome, MdmLinkSourceEnum.AUTO, theMdmTransactionContext);
+ goldenResources.add(goldenResource);
+ }
+
+ return goldenResources;
+ }
+
  private void handleMdmWithNoCandidates(IAnyResource theResource, MdmTransactionContext theMdmTransactionContext) {
  log(theMdmTransactionContext, String.format("There were no matched candidates for MDM, creating a new %s Golden Resource.", theResource.getIdElement().getResourceType()));
  IAnyResource newGoldenResource = myGoldenResourceHelper.createGoldenResourceFromMdmSourceResource(theResource, theMdmTransactionContext);

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/dao/MdmLinkDaoSvcTest.java

@@ -100,7 +100,6 @@ private MdmLink createPatientAndLinkTo(Long thePatientPid, MdmMatchResultEnum th
  mdmLink.setUpdated(new Date());
  mdmLink.setGoldenResourcePersistenceId(JpaPid.fromId(thePatientPid));
  mdmLink.setSourcePersistenceId(runInTransaction(()->myIdHelperService.getPidOrNull(RequestPartitionId.allPartitions(), patient)));
- MdmLink saved= myMdmLinkDao.save(mdmLink);
- return saved;
+ return myMdmLinkDao.save(mdmLink);
  }
 }

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderQueryLinkR4Test.java

@@ -10,6 +10,7 @@
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
+import ca.uhn.fhir.test.utilities.RangeTestHelper;
 import ca.uhn.fhir.util.ParametersUtil;
 import ca.uhn.fhir.util.StopWatch;
 import org.apache.commons.lang3.StringUtils;
@@ -377,7 +378,7 @@ public void testQueryLnkThreeMatches() {
  List<Parameters.ParametersParameterComponent> list = getParametersByName(result, "link");
  assertThat(list, hasSize(4));
  List<Parameters.ParametersParameterComponent> part = list.get(3).getPart();
- assertMdmLink(MDM_LINK_PROPERTY_COUNT, part, goldenResourceId.getValue(), patientId.getValue(), MdmMatchResultEnum.MATCH, "false", "false", "2");
+ assertMdmLink(MDM_LINK_PROPERTY_COUNT, part, goldenResourceId.getValue(), patientId.getValue(), MdmMatchResultEnum.MATCH, "false", "false", ".666");
  }
 
  @Test
@@ -459,7 +460,7 @@ private void assertMdmLink(int theExpectedSize, List<Parameters.ParametersParame
  assertThat(thePart.get(5).getValue().primitiveValue(), is(theNewGoldenResource));
 
  assertThat(thePart.get(6).getName(), is("score"));
- assertThat(thePart.get(6).getValue().primitiveValue(), is(theScore));
+ RangeTestHelper.checkInRange(theScore, thePart.get(6).getValue().primitiveValue());
  }
  }
 

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmLinkUpdaterSvcImplTest.java

@@ -16,6 +16,7 @@
 
 import static ca.uhn.fhir.mdm.api.MdmMatchResultEnum.MATCH;
 import static ca.uhn.fhir.mdm.api.MdmMatchResultEnum.NO_MATCH;
+import static ca.uhn.fhir.mdm.api.MdmMatchResultEnum.POSSIBLE_MATCH;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
@@ -52,6 +53,23 @@ public void testUpdateLinkNoMatch() {
  assertLinksMatchedByEid(false, false);
  }
 
+ @Test
+ public void testUpdateLinkPossibleMatchSavesNormalizedScore() {
+ final Patient goldenPatient = createGoldenPatient(buildJanePatient());
+ final Patient patient1 = createPatient(buildJanePatient());
+ buildUpdateLinkMdmTransactionContext();
+
+ MdmMatchOutcome matchOutcome = new MdmMatchOutcome(61L, 5.0).setMdmRuleCount(6).setMatchResultEnum(POSSIBLE_MATCH);
+ myMdmLinkDaoSvc.createOrUpdateLinkEntity(goldenPatient, patient1, matchOutcome, MdmLinkSourceEnum.MANUAL, createContextForCreate("Patient"));
+
+ final List<MdmLink> targets = myMdmLinkDaoSvc.findMdmLinksByGoldenResource(goldenPatient);
+ assertFalse(targets.isEmpty());
+ assertEquals(1, targets.size());
+ final MdmLink mdmLink = targets.get(0);
+
+ assertEquals(matchOutcome.getNormalizedScore(), mdmLink.getScore());
+ }
+
  @Test
  public void testUpdateLinkMatchAfterVersionChange() {
  myMdmSettings.getMdmRules().setVersion("1");

hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/api/MdmMatchOutcome.java

@@ -36,13 +36,13 @@ public final class MdmMatchOutcome {
  /**
  * A bitmap that indicates which rules matched
  */
- public final Long vector;
+ private final Long vector;
 
  /**
  * The sum of all scores for all rules evaluated. Similarity rules add the similarity score (between 0.0 and 1.0) whereas
  * matcher rules add either a 0.0 or 1.0.
  */
- public final Double score;
+ private final Double score;
 
  /**
  * Did the MDM match operation result in creating a new golden resource resource?
@@ -134,6 +134,10 @@ public MdmMatchOutcome setEidMatch(boolean theEidMatch) {
  return this;
  }
 
+ public Double getScore() { return score; }
+
+ public Long getVector() { return vector; }
+
  /**
  * Gets normalized score that is in the range from zero to one
  *

hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/rules/svc/MdmResourceMatcherSvc.java

@@ -89,12 +89,12 @@ public MdmMatchOutcome getMatchResult(IBaseResource theLeftResource, IBaseResour
 
  MdmMatchOutcome match(IBaseResource theLeftResource, IBaseResource theRightResource) {
  MdmMatchOutcome matchResult = getMatchOutcome(theLeftResource, theRightResource);
- MdmMatchResultEnum matchResultEnum = myMdmRulesJson.getMatchResult(matchResult.vector);
+ MdmMatchResultEnum matchResultEnum = myMdmRulesJson.getMatchResult(matchResult.getVector());
  matchResult.setMatchResultEnum(matchResultEnum);
  if (ourLog.isDebugEnabled()) {
  ourLog.debug("{} {}: {}", matchResult.getMatchResultEnum(), theRightResource.getIdElement().toUnqualifiedVersionless(), matchResult);
  if (ourLog.isTraceEnabled()) {
- ourLog.trace("Field matcher results:\n{}", myMdmRulesJson.getDetailedFieldMatchResultWithSuccessInformation(matchResult.vector));
+ ourLog.trace("Field matcher results:\n{}", myMdmRulesJson.getDetailedFieldMatchResultWithSuccessInformation(matchResult.getVector()));
  }
  }
  return matchResult;
@@ -133,8 +133,8 @@ private MdmMatchOutcome getMatchOutcome(IBaseResource theLeftResource, IBaseReso
  ourLog.trace("Matcher {} is valid for resource type: {}. Evaluating match.", fieldComparator.getName(), resourceType);
  MdmMatchEvaluation matchEvaluation = fieldComparator.match(theLeftResource, theRightResource);
  if (matchEvaluation.match) {
- vector |= (1 << i);
- ourLog.trace("Match: Successfully matched matcher {} with score {}.", fieldComparator.getName(), matchEvaluation.score);
+ vector |= (1L << i);
+ ourLog.trace("Match: Successfully matched matcher {} with score {}. New vector: {}", fieldComparator.getName(), matchEvaluation.score, vector);
  } else {
  ourLog.trace("No match: Matcher {} did not match (score: {}).", fieldComparator.getName(), matchEvaluation.score);
  }

hapi-fhir-server-mdm/src/test/java/ca/uhn/fhir/mdm/BaseR4Test.java

@@ -43,8 +43,8 @@ protected void assertMatch(MdmMatchResultEnum theExpectedMatchEnum, MdmMatchOutc
  }
 
  protected void assertMatchResult(MdmMatchResultEnum theExpectedMatchEnum, long theExpectedVector, double theExpectedScore, boolean theExpectedNewGoldenResource, boolean theExpectedEidMatch, MdmMatchOutcome theMatchResult) {
- assertEquals(theExpectedScore, theMatchResult.score, 0.001);
- assertEquals(theExpectedVector, theMatchResult.vector);
+ assertEquals(theExpectedScore, theMatchResult.getScore(), 0.001);
+ assertEquals(theExpectedVector, theMatchResult.getVector());
  assertEquals(theExpectedEidMatch, theMatchResult.isEidMatch());
  assertEquals(theExpectedNewGoldenResource, theMatchResult.isCreatedNewResource());
  assertEquals(theExpectedMatchEnum, theMatchResult.getMatchResultEnum());

hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/test/utilities/RangeTestHelper.java

@@ -0,0 +1,62 @@
+package ca.uhn.fhir.test.utilities;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.both;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.lessThanOrEqualTo;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+public class RangeTestHelper {
+
+ public static final double THOUSANDTH = .001d;
+
+
+ public static void checkInRange(double base, double value) {
+ checkInRange(base, THOUSANDTH, value);
+ }
+
+ public static void checkInRange(double theBase, double theRange, double theValue) {
+ double lowerBound = theBase - theRange;
+ double upperBound = theBase + theRange;
+ checkWithinBounds(lowerBound, upperBound, theValue);
+ }
+
+ public static void checkInRange(String theBase, String theValue) {
+ // ease tests
+ if (theBase == null && theValue == null) {
+ return;
+ }
+
+ double value = Double.parseDouble(theValue);
+ double base = Double.parseDouble(theBase);
+ checkInRange(base, THOUSANDTH, value);
+ }
+
+ public static void checkInRange(String theBase, double theRange, String theValue) {
+ // ease tests
+ if (theBase == null && theValue == null) {
+ return;
+ }
+
+ double value = Double.parseDouble(theValue);
+ double base = Double.parseDouble(theBase);
+ checkInRange(base, theRange, value);
+ }
+
+ public static void checkWithinBounds(double theLowerBound, double theUpperBound, double theValue) {
+ assertThat(theValue, is(both(greaterThanOrEqualTo(theLowerBound)).and(lessThanOrEqualTo(theUpperBound))));
+ }
+
+ public static void checkWithinBounds(String theLowerBound, String theUpperBound, String theValue) {
+ assertNotNull(theLowerBound, "theLowerBound");
+ assertNotNull(theUpperBound, "theUpperBound");
+ assertNotNull(theValue, "theValue");
+ double lowerBound = Double.parseDouble(theLowerBound);
+ double upperBound = Double.parseDouble(theUpperBound);
+ double value = Double.parseDouble(theValue);
+ checkWithinBounds(lowerBound, upperBound, value);
+ }
+
+
+}