fixed mdmjsonlink serialization issues (#5987)

* fixed mdmjsonlink serialization issues * spotless * test fixing * spotless * review fixes * spotless * fixing * spotless --------- Co-authored-by: leif stawnyczy <[email protected]>
hapifhir · Jun 7, 2024 · 1437c35 · 1437c35
1 parent 5b75639
commit 1437c35
Show file tree

Hide file tree

Showing 12 changed files with 190 additions and 54 deletions.
diff --git a/...ces/ca/uhn/hapi/fhir/changelog/7_4_0/5986-fixing-bug-with-deserializing-mdmlink-json.yaml b/...ces/ca/uhn/hapi/fhir/changelog/7_4_0/5986-fixing-bug-with-deserializing-mdmlink-json.yaml
@@ -0,0 +1,8 @@
+---
+type: fix
+issue: 5985
+title: "Fixed an issue where MDM json links were unserializable
+ due to using the IResourcePersistenceId objects.
+ This has been fixed, and IResourcePersistenceId objects
+ will not be serialized or returned to users.
+"
diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java
@@ -125,7 +125,7 @@ public class IdHelperService implements IIdHelperService<JpaPid> {
  private boolean myDontCheckActiveTransactionForUnitTest;
 
  @VisibleForTesting
- void setDontCheckActiveTransactionForUnitTest(boolean theDontCheckActiveTransactionForUnitTest) {
+ protected void setDontCheckActiveTransactionForUnitTest(boolean theDontCheckActiveTransactionForUnitTest) {
  myDontCheckActiveTransactionForUnitTest = theDontCheckActiveTransactionForUnitTest;
  }
 

diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java
@@ -20,11 +20,20 @@
 import org.mockito.ArgumentMatchers;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
+import org.mockito.Spy;
 import org.mockito.junit.jupiter.MockitoExtension;
 
 import java.util.List;
 import java.util.Map;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -37,16 +46,16 @@
 public class IdHelperServiceTest {
 
  @InjectMocks
- private final IdHelperService subject = new IdHelperService();
+ private final IdHelperService myHelperSvc = new IdHelperService();
 
  @Mock
  protected IResourceTableDao myResourceTableDao;
 
  @Mock
  private JpaStorageSettings myStorageSettings;
 
- @Mock
- private FhirContext myFhirCtx;
+ @Spy
+ private FhirContext myFhirCtx = FhirContext.forR4Cached();
 
  @Mock
  private MemoryCacheService myMemoryCacheService;
@@ -59,10 +68,11 @@ public class IdHelperServiceTest {
 
  @BeforeEach
  void setUp() {
- subject.setDontCheckActiveTransactionForUnitTest(true);
+ myHelperSvc.setDontCheckActiveTransactionForUnitTest(true);
 
- when(myStorageSettings.isDeleteEnabled()).thenReturn(true);
- when(myStorageSettings.getResourceClientIdStrategy()).thenReturn(JpaStorageSettings.ClientIdStrategyEnum.ANY);
+ // lenient because some tests require this setup, and others do not
+ lenient().doReturn(true).when(myStorageSettings).isDeleteEnabled();
+ lenient().doReturn(JpaStorageSettings.ClientIdStrategyEnum.ANY).when(myStorageSettings).getResourceClientIdStrategy();
  }
 
  @Test
@@ -81,7 +91,7 @@ public void testResolveResourcePersistentIds() {
  // configure mock behaviour
  when(myStorageSettings.isDeleteEnabled()).thenReturn(true);
 
- final ResourceNotFoundException resourceNotFoundException = assertThrows(ResourceNotFoundException.class, () -> subject.resolveResourcePersistentIds(requestPartitionId, resourceType, ids, theExcludeDeleted));
+ final ResourceNotFoundException resourceNotFoundException = assertThrows(ResourceNotFoundException.class, () -> myHelperSvc.resolveResourcePersistentIds(requestPartitionId, resourceType, ids, theExcludeDeleted));
  assertEquals("HAPI-2001: Resource Patient/123 is not known", resourceNotFoundException.getMessage());
  }
 
@@ -102,13 +112,14 @@ public void testResolveResourcePersistentIdsDeleteFalse() {
  // configure mock behaviour
  when(myStorageSettings.isDeleteEnabled()).thenReturn(false);
 
- Map<String, JpaPid> actualIds = subject.resolveResourcePersistentIds(requestPartitionId, resourceType, ids, theExcludeDeleted);
+ Map<String, JpaPid> actualIds = myHelperSvc.resolveResourcePersistentIds(requestPartitionId, resourceType, ids, theExcludeDeleted);
 
  //verifyResult
  assertFalse(actualIds.isEmpty());
  assertNull(actualIds.get(ids.get(0)));
  }
 
+
  private Root<ResourceTable> getMockedFrom() {
  @SuppressWarnings("unchecked")
  Path<Object> path = mock(Path.class);

diff --git a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/config/MdmSurvivorshipConfig.java b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/config/MdmSurvivorshipConfig.java
@@ -22,6 +22,7 @@
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
+import ca.uhn.fhir.jpa.dao.tx.HapiTransactionService;
 import ca.uhn.fhir.mdm.api.IMdmLinkQuerySvc;
 import ca.uhn.fhir.mdm.api.IMdmSettings;
 import ca.uhn.fhir.mdm.api.IMdmSurvivorshipService;
@@ -57,10 +58,18 @@ public class MdmSurvivorshipConfig {
  @Autowired
  private IIdHelperService<?> myIIdHelperService;
 
+ @Autowired
+ private HapiTransactionService myTransactionService;
+
  @Bean
  public IMdmSurvivorshipService mdmSurvivorshipService() {
  return new MdmSurvivorshipSvcImpl(
- myFhirContext, goldenResourceHelper(), myDaoRegistry, myMdmLinkQuerySvc, myIIdHelperService);
+ myFhirContext,
+ goldenResourceHelper(),
+ myDaoRegistry,
+ myMdmLinkQuerySvc,
+ myIIdHelperService,
+ myTransactionService);
  }
 
  @Bean

diff --git a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/MdmModelConverterSvcImpl.java b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/MdmModelConverterSvcImpl.java
@@ -44,13 +44,17 @@ public MdmLinkJson toJson(IMdmLink theLink) {
  .toVersionless()
  .getValue();
  retVal.setSourceId(sourceId);
- retVal.setSourcePid(theLink.getSourcePersistenceId());
+ if (theLink.getSourcePersistenceId() != null) {
+ retVal.setSourcePid(theLink.getSourcePersistenceId());
+ }
  String goldenResourceId = myIdHelperService
  .resourceIdFromPidOrThrowException(theLink.getGoldenResourcePersistenceId(), theLink.getMdmSourceType())
  .toVersionless()
  .getValue();
  retVal.setGoldenResourceId(goldenResourceId);
- retVal.setGoldenPid(theLink.getGoldenResourcePersistenceId());
+ if (theLink.getGoldenResourcePersistenceId() != null) {
+ retVal.setGoldenPid(theLink.getGoldenResourcePersistenceId());
+ }
  retVal.setCreated(theLink.getCreated());
  retVal.setEidMatch(theLink.getEidMatch());
  retVal.setLinkSource(theLink.getLinkSource());

diff --git a/...hir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmModelConverterSvcImplTest.java b/...hir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmModelConverterSvcImplTest.java
@@ -50,8 +50,15 @@ public void testBasicMdmLinkConversion() {
 
  ourLog.info("actualMdmLinkJson: {}", actualMdmLinkJson);
 
- MdmLinkJson exepctedMdmLinkJson = getExepctedMdmLinkJson(mdmLink.getGoldenResourcePersistenceId().getId(), mdmLink.getSourcePersistenceId().getId(), MdmMatchResultEnum.MATCH, MdmLinkSourceEnum.MANUAL, version, createTime, updateTime, isLinkCreatedResource, scoreRounded);
- assertEquals(exepctedMdmLinkJson, actualMdmLinkJson);
+ MdmLinkJson expectedMdmLinkJson = getExepctedMdmLinkJson(mdmLink.getGoldenResourcePersistenceId().getId(), mdmLink.getSourcePersistenceId().getId(), MdmMatchResultEnum.MATCH, MdmLinkSourceEnum.MANUAL, version, createTime, updateTime, isLinkCreatedResource, scoreRounded);
+ assertEquals(expectedMdmLinkJson.getSourceId(), actualMdmLinkJson.getSourceId());
+ assertEquals(expectedMdmLinkJson.getGoldenResourceId(), actualMdmLinkJson.getGoldenResourceId());
+ assertEquals(expectedMdmLinkJson.getGoldenPid().getId(), actualMdmLinkJson.getGoldenPid().getId());
+ assertEquals(expectedMdmLinkJson.getSourcePid().getId(), actualMdmLinkJson.getSourcePid().getId());
+ assertEquals(expectedMdmLinkJson.getVector(), actualMdmLinkJson.getVector());
+ assertEquals(expectedMdmLinkJson.getScore(), actualMdmLinkJson.getScore());
+ assertEquals(expectedMdmLinkJson.getMatchResult(), actualMdmLinkJson.getMatchResult());
+ assertEquals(expectedMdmLinkJson.getLinkSource(), actualMdmLinkJson.getLinkSource());
  }
 
  @Test

diff --git a/...-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplTest.java b/...-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmSurvivorshipSvcImplTest.java
@@ -1,11 +1,12 @@
 package ca.uhn.fhir.jpa.mdm.svc;
 
 import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.interceptor.model.RequestPartitionId;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
-import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
 import ca.uhn.fhir.jpa.dao.index.IdHelperService;
-import ca.uhn.fhir.jpa.entity.MdmLink;
+import ca.uhn.fhir.jpa.dao.tx.HapiTransactionService;
+import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.mdm.api.IMdmLinkQuerySvc;
 import ca.uhn.fhir.mdm.api.IMdmSettings;
@@ -21,6 +22,7 @@
 import ca.uhn.fhir.mdm.util.MdmPartitionHelper;
 import ca.uhn.fhir.mdm.util.MdmResourceUtil;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
+import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.r4.model.Patient;
 import org.junit.jupiter.api.BeforeEach;
@@ -31,20 +33,23 @@
 import org.mockito.Spy;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.springframework.data.domain.Page;
+import org.springframework.data.domain.PageImpl;
 
-import java.time.Instant;
-import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
@@ -69,12 +74,15 @@ public class MdmSurvivorshipSvcImplTest {
  @Mock
  private MdmPartitionHelper myMdmPartitionHelper;
 
- @Spy
- private IIdHelperService<?> myIIdHelperService = new IdHelperService();
+ @Mock
+ private IdHelperService myIIdHelperService;
 
  @Mock
  private IMdmLinkQuerySvc myMdmLinkQuerySvc;
 
+ @Mock
+ private HapiTransactionService myTransactionService;
+
  private MdmSurvivorshipSvcImpl mySvc;
 
  @BeforeEach
@@ -91,7 +99,8 @@ public void before() {
  myGoldenResourceHelper,
  myDaoRegistry,
  myMdmLinkQuerySvc,
- myIIdHelperService
+ myIIdHelperService,
+ myTransactionService
  );
  }
 
@@ -115,7 +124,7 @@ public void rebuildGoldenResourceCurrentLinksUsingSurvivorshipRules_withManyLink
 
  List<IBaseResource> resources = new ArrayList<>();
  List<MdmLinkJson> links = new ArrayList<>();
-
+ Map<String, IResourcePersistentId> sourceIdToPid = new HashMap<>();
  for (int i = 0; i < 10; i++) {
  // we want our resources to be slightly different
  Patient patient = new Patient();
@@ -137,24 +146,34 @@ public void rebuildGoldenResourceCurrentLinksUsingSurvivorshipRules_withManyLink
  );
  link.setSourcePid(JpaPid.fromId((long)i));
  link.setGoldenPid(JpaPid.fromId((long)goldenId));
+ link.setSourceId(patient.getId());
+ link.setGoldenResourceId(goldenPatient.getId());
  links.add(link);
+ sourceIdToPid.put(patient.getId(), link.getSourcePid());
  }
 
  IFhirResourceDao resourceDao = mock(IFhirResourceDao.class);
 
  // when
+ IHapiTransactionService.IExecutionBuilder executionBuilder = mock(IHapiTransactionService.IExecutionBuilder.class);
+ when(myTransactionService.withRequest(any())).thenReturn(executionBuilder);
+ doAnswer(a -> {
+ Runnable callback = a.getArgument(0);
+ callback.run();
+ return 0;
+ }).when(executionBuilder).execute(any(Runnable.class));
  when(myDaoRegistry.getResourceDao(eq("Patient")))
  .thenReturn(resourceDao);
  AtomicInteger counter = new AtomicInteger();
  when(resourceDao.readByPid(any()))
  .thenAnswer(params -> resources.get(counter.getAndIncrement()));
- Page<MdmLinkJson> linkPage = mock(Page.class);
+ Page<MdmLinkJson> linkPage = new PageImpl<>(links);
  when(myMdmLinkQuerySvc.queryLinks(any(), any()))
  .thenReturn(linkPage);
- when(linkPage.get())
- .thenReturn(links.stream());
  when(myMdmSettings.getMdmRules())
  .thenReturn(new MdmRulesJson());
+ doReturn(sourceIdToPid).when(myIIdHelperService)
+ .resolveResourcePersistentIds(any(RequestPartitionId.class), anyString(), any(List.class));
  // we will return a non-empty list to reduce mocking
  when(myEIDHelper.getExternalEid(any()))
  .thenReturn(Collections.singletonList(new CanonicalEID("example", "value", "use")));
@@ -178,19 +197,6 @@ public void rebuildGoldenResourceCurrentLinksUsingSurvivorshipRules_withManyLink
  .update(eq(goldenPatientRebuilt), any(RequestDetails.class));
  }
 
- private MdmLink createLinkWithoutUpdateDate(Patient theSource, Patient theGoldenResource) {
- MdmLink link = new MdmLink();
- link.setCreated(Date.from(
- Instant.now().minus(2, ChronoUnit.DAYS)
- ));
- link.setLinkSource(MdmLinkSourceEnum.AUTO);
- link.setMatchResult(MdmMatchResultEnum.MATCH);
- link.setSourcePersistenceId(JpaPid.fromId(theSource.getIdElement().getIdPartAsLong()));
- link.setGoldenResourcePersistenceId(JpaPid.fromId(theGoldenResource.getIdElement().getIdPartAsLong()));
-
- return link;
- }
-
  private MdmTransactionContext createTransactionContext() {
  MdmTransactionContext context = new MdmTransactionContext();
  context.setRestOperation(MdmTransactionContext.OperationType.UPDATE_LINK);

diff --git a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/model/mdmevents/MdmLinkJson.java b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/model/mdmevents/MdmLinkJson.java
@@ -23,6 +23,7 @@
 import ca.uhn.fhir.mdm.api.MdmMatchResultEnum;
 import ca.uhn.fhir.model.api.IModelJson;
 import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId;
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
 
 import java.util.Date;
@@ -45,13 +46,13 @@ public class MdmLinkJson implements IModelJson {
  /**
  * Golden resource PID
  */
- @JsonProperty("goldenPid")
+ @JsonIgnore
  private IResourcePersistentId<?> myGoldenPid;
 
  /**
  * Source PID
  */
- @JsonProperty("sourcePid")
+ @JsonIgnore
  private IResourcePersistentId<?> mySourcePid;
 
  /**