From 5dfe5753484fbbeab276715ef2877de5862f0b15 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Tue, 30 Apr 2024 12:02:39 -0400 Subject: [PATCH] Ensure a non-numeric FHIR ID doesn't result in a NumberFormatException when processing survivorship rules (#5883) * Add failing test as well as commented out potential solution. * Fix for NumberFormatException. * Add conditional test for survivorship rules. * Spotless. * Add changelog. * Code review feedback. --- ...-survivorship-number-format-exception.yaml | 6 ++++ .../jpa/mdm/svc/MdmSurvivorshipSvcImplIT.java | 22 ++++++++++++++ .../mdm/svc/MdmSurvivorshipSvcImplTest.java | 19 ++++++++---- .../fhir/mdm/svc/MdmSurvivorshipSvcImpl.java | 29 +++++++++++++++---- 4 files changed, 65 insertions(+), 11 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5886-mdm-apply-survivorship-number-format-exception.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5886-mdm-apply-survivorship-number-format-exception.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5886-mdm-apply-survivorship-number-format-exception.yaml new file mode 100644 index 00000000000..9dae5a3b34a --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5886-mdm-apply-survivorship-number-format-exception.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 5886 +title: "Previously, either updating links on, or deleting one of two patients with non-numeric IDs linked to a golden + patient would result in a HAPI-0389 if there were survivorship rules. + This issue has been fixed for both the update links and delete cases." diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplIT.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplIT.java index febcaf4c6e9..5e3aaad9f42 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplIT.java +++ b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplIT.java @@ -2,11 +2,16 @@ package ca.uhn.fhir.jpa.mdm.svc; import ca.uhn.fhir.jpa.mdm.BaseMdmR4Test; import ca.uhn.fhir.mdm.api.IMdmSurvivorshipService; +import ca.uhn.fhir.mdm.api.MdmLinkSourceEnum; +import ca.uhn.fhir.mdm.api.MdmMatchOutcome; import ca.uhn.fhir.mdm.model.MdmTransactionContext; +import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import java.util.List; + import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; @@ -55,4 +60,21 @@ class MdmSurvivorshipSvcImplIT extends BaseMdmR4Test { assertEquals(p1.getTelecom().size(), p1.getTelecom().size()); assertTrue(p2.getTelecomFirstRep().equalsDeep(p1.getTelecomFirstRep())); } + + @Test + public void matchingPatientsWith_NON_Numeric_Ids_matches_doesNotThrow_NumberFormatException() { + final Patient frankPatient1 = buildFrankPatient(); + frankPatient1.setId("patA"); + myPatientDao.update(frankPatient1, new SystemRequestDetails()); + final Patient frankPatient2 = buildFrankPatient(); + frankPatient2.setId("patB"); + myPatientDao.update(frankPatient2, new SystemRequestDetails()); + final Patient goldenPatient = buildFrankPatient(); + myPatientDao.create(goldenPatient, new SystemRequestDetails()); + + myMdmLinkDaoSvc.createOrUpdateLinkEntity(goldenPatient, frankPatient1, MdmMatchOutcome.NEW_GOLDEN_RESOURCE_MATCH, MdmLinkSourceEnum.MANUAL, createContextForCreate("Patient")); + myMdmLinkDaoSvc.createOrUpdateLinkEntity(goldenPatient, frankPatient2, MdmMatchOutcome.NEW_GOLDEN_RESOURCE_MATCH, MdmLinkSourceEnum.MANUAL, createContextForCreate("Patient")); + + myMdmSurvivorshipService.rebuildGoldenResourceWithSurvivorshipRules(goldenPatient, new MdmTransactionContext(MdmTransactionContext.OperationType.UPDATE_LINK)); + } } diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplTest.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplTest.java index a6502687415..65cc7bde0c4 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplTest.java +++ b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplTest.java @@ -25,8 +25,9 @@ import ca.uhn.fhir.rest.api.server.RequestDetails; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; @@ -96,8 +97,9 @@ public class MdmSurvivorshipSvcImplTest { } @SuppressWarnings({"rawtypes", "unchecked"}) - @Test - public void rebuildGoldenResourceCurrentLinksUsingSurvivorshipRules_withManyLinks_rebuildsInUpdateOrder() { + @ParameterizedTest + @ValueSource(booleans = {true,false}) + public void rebuildGoldenResourceCurrentLinksUsingSurvivorshipRules_withManyLinks_rebuildsInUpdateOrder(boolean theIsUseNonNumericId) { // setup // create resources Patient goldenPatient = new Patient(); @@ -126,7 +128,7 @@ public class MdmSurvivorshipSvcImplTest { patient.addIdentifier() .setSystem("http://example.com") .setValue("Value" + i); - patient.setId("Patient/" + i); + patient.setId("Patient/" + (theIsUseNonNumericId ? "pat"+i : Integer.toString(i))); resources.add(patient); MdmLinkJson link = createLinkJson( @@ -149,8 +151,13 @@ public class MdmSurvivorshipSvcImplTest { when(myDaoRegistry.getResourceDao(eq("Patient"))) .thenReturn(resourceDao); AtomicInteger counter = new AtomicInteger(); - when(resourceDao.readByPid(any())) - .thenAnswer(params -> resources.get(counter.getAndIncrement())); + if (theIsUseNonNumericId) { + when(resourceDao.read(any(), any())) + .thenAnswer(params -> resources.get(counter.getAndIncrement())); + } else { + when(resourceDao.readByPid(any())) + .thenAnswer(params -> resources.get(counter.getAndIncrement())); + } Page linkPage = mock(Page.class); when(myMdmLinkQuerySvc.queryLinks(any(), any())) .thenReturn(linkPage); diff --git a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/svc/MdmSurvivorshipSvcImpl.java b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/svc/MdmSurvivorshipSvcImpl.java index 27d89757a9c..403db4605b6 100644 --- a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/svc/MdmSurvivorshipSvcImpl.java +++ b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/svc/MdmSurvivorshipSvcImpl.java @@ -31,17 +31,22 @@ import ca.uhn.fhir.mdm.api.params.MdmQuerySearchParameters; import ca.uhn.fhir.mdm.model.MdmTransactionContext; import ca.uhn.fhir.mdm.model.mdmevents.MdmLinkJson; import ca.uhn.fhir.mdm.util.GoldenResourceHelper; +import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId; import ca.uhn.fhir.util.TerserUtil; +import org.apache.commons.lang3.StringUtils; import org.hl7.fhir.instance.model.api.IAnyResource; import org.hl7.fhir.instance.model.api.IBase; import org.hl7.fhir.instance.model.api.IBaseResource; import org.springframework.data.domain.Page; +import java.util.regex.Pattern; import java.util.stream.Stream; public class MdmSurvivorshipSvcImpl implements IMdmSurvivorshipService { + private static final Pattern IS_UUID = + Pattern.compile("[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"); protected final FhirContext myFhirContext; @@ -133,16 +138,30 @@ public class MdmSurvivorshipSvcImpl implements IMdmSurvivorshipService { String sourceId = link.getSourceId(); // +1 because of "/" in id: "ResourceType/Id" - IResourcePersistentId pid = getResourcePID(sourceId.substring(resourceType.length() + 1), resourceType); + final String sourceIdUnqualified = sourceId.substring(resourceType.length() + 1); - // this might be a bit unperformant - // but it depends how many links there are - // per golden resource (unlikely to be thousands) - return dao.readByPid(pid); + // myMdmLinkQuerySvc.queryLinks populates sourceId with the FHIR_ID, not the RES_ID, so if we don't + // add this conditional logic, on JPA, myIIdHelperService.newPidFromStringIdAndResourceName will fail with + // NumberFormatException + if (isNumericOrUuid(sourceIdUnqualified)) { + IResourcePersistentId pid = getResourcePID(sourceIdUnqualified, resourceType); + + // this might be a bit unperformant + // but it depends how many links there are + // per golden resource (unlikely to be thousands) + return dao.readByPid(pid); + } else { + return dao.read(new IdDt(sourceId), new SystemRequestDetails()); + } }); } private IResourcePersistentId getResourcePID(String theId, String theResourceType) { return myIIdHelperService.newPidFromStringIdAndResourceName(theId, theResourceType); } + + private boolean isNumericOrUuid(String theLongCandidate) { + return StringUtils.isNumeric(theLongCandidate) + || IS_UUID.matcher(theLongCandidate).matches(); + } }