From 4e5ca2ee15d384cfffab62abba773aac898e6abc Mon Sep 17 00:00:00 2001 From: ianmarshall Date: Fri, 3 Jul 2020 13:08:11 -0400 Subject: [PATCH 1/2] Changes from code review --- .../jpa/term/TermDeferredStorageSvcImpl.java | 7 ++-- .../term/TermCodeSystemStorageSvcTest.java | 37 +++++++++++-------- .../term/TermDeferredStorageSvcImplTest.java | 8 ++-- 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java index b966887be7e..afb60c1b580 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java @@ -50,6 +50,7 @@ import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.TransactionTemplate; import javax.annotation.PostConstruct; +import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -145,10 +146,10 @@ public class TermDeferredStorageSvcImpl implements ITermDeferredStorageSvc { ourLog.info("Saving {} deferred concepts...", count); while (codeCount < count && myDeferredConcepts.size() > 0) { TermConcept next = myDeferredConcepts.remove(0); - if(myCodeSystemVersionDao.findById(next.getCodeSystemVersion().getPid()).isPresent()) { + try { codeCount += myCodeSystemStorageSvc.saveConcept(next); - } else { - ourLog.warn("Unable to save deferred TermConcept {} because Code System {} version PID {} is no longer valid. Code system may have since been replaced.", + } catch (Exception theE) { + ourLog.warn("Unable to save deferred TermConcept {}, possibly because Code System {} version PID {} may have since been replaced.", next.getCode(), next.getCodeSystemVersion().getCodeSystemDisplayName(), next.getCodeSystemVersion().getPid()); } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcTest.java index 007c0b8ee20..5fd251daa5b 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcTest.java @@ -12,6 +12,28 @@ public class TermCodeSystemStorageSvcTest extends BaseJpaR4Test { public static final String URL_MY_CODE_SYSTEM = "http://example.com/my_code_system"; + @Test + public void testStoreNewCodeSystemVersionForExistingCodeSystem() { + CodeSystem upload = createCodeSystemWithMoreThan100Concepts(); + + // Create CodeSystem resource + ResourceTable codeSystemResourceEntity = (ResourceTable)myCodeSystemDao.create(upload, mySrd).getEntity(); + + // Update the CodeSystem resource + runInTransaction(() -> myTermCodeSystemStorageSvc.storeNewCodeSystemVersionIfNeeded(upload, codeSystemResourceEntity)); + + /* + Because there are more than 100 concepts in the code system, the first 100 will be persisted immediately and + the remaining 25 concepts will be queued up for "deferred save". + + As the CodeSystem was persisted twice, the extra 25 term concepts will be queued twice, each with a different + CodeSystem version PID. Only one set of the term concepts should be persisted (i.e. 125 term concepts in total). + */ + myTerminologyDeferredStorageSvc.setProcessDeferred(true); + myTerminologyDeferredStorageSvc.saveDeferred(); + assertEquals(125, myTermConceptDao.count()); + } + private CodeSystem createCodeSystemWithMoreThan100Concepts() { CodeSystem codeSystem = new CodeSystem(); codeSystem.setUrl(URL_MY_CODE_SYSTEM); @@ -25,20 +47,5 @@ public class TermCodeSystemStorageSvcTest extends BaseJpaR4Test { } - @Test - public void testStoreNewCodeSystemVersionForExistingCodeSystem() { - CodeSystem upload = createCodeSystemWithMoreThan100Concepts(); - - ResourceTable codeSystemResourceEntity = (ResourceTable)myCodeSystemDao.create(upload, mySrd).getEntity(); - - runInTransaction(() -> myTermCodeSystemStorageSvc.storeNewCodeSystemVersionIfNeeded(upload, codeSystemResourceEntity)); - - myTerminologyDeferredStorageSvc.setProcessDeferred(true); - myTerminologyDeferredStorageSvc.saveDeferred(); - myTerminologyDeferredStorageSvc.saveDeferred(); - - assertEquals(125, myTermConceptDao.count()); - } - } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java index a17ee86863d..e98b68c5270 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java @@ -13,15 +13,12 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import org.springframework.transaction.PlatformTransactionManager; -import java.util.Optional; - import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.*; @RunWith(MockitoJUnitRunner.class) public class TermDeferredStorageSvcImplTest { - @Mock private PlatformTransactionManager myTxManager; @Mock @@ -53,7 +50,6 @@ public class TermDeferredStorageSvcImplTest { svc.setCodeSystemStorageSvcForUnitTest(myTermConceptStorageSvc); svc.setDaoConfigForUnitTest(new DaoConfig()); - when(myTermCodeSystemVersionDao.findById(anyLong())).thenReturn(Optional.of(myTermCodeSystemVersion)); svc.setCodeSystemVersionDaoForUnitTest(myTermCodeSystemVersionDao); svc.setProcessDeferred(true); svc.addConceptToStorageQueue(concept); @@ -76,11 +72,13 @@ public class TermDeferredStorageSvcImplTest { svc.setCodeSystemStorageSvcForUnitTest(myTermConceptStorageSvc); svc.setDaoConfigForUnitTest(new DaoConfig()); + when(myTermConceptStorageSvc.saveConcept(concept)).thenThrow(new RuntimeException("Foreign Constraint Violation")); svc.setCodeSystemVersionDaoForUnitTest(myTermCodeSystemVersionDao); svc.setProcessDeferred(true); svc.addConceptToStorageQueue(concept); svc.saveDeferred(); - verify(myTermConceptStorageSvc, times(0)).saveConcept(same(concept)); + + verify(myTermConceptStorageSvc, times(1)).saveConcept(same(concept)); verifyNoMoreInteractions(myTermConceptStorageSvc); } From b1ff6cbe378f9de084b02035ea80b433a144357d Mon Sep 17 00:00:00 2001 From: ianmarshall Date: Fri, 3 Jul 2020 14:25:24 -0400 Subject: [PATCH 2/2] Changes from code review --- .../jpa/term/TermDeferredStorageSvcImpl.java | 14 +++++--- .../term/TermDeferredStorageSvcImplTest.java | 32 +++++++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java index afb60c1b580..589652cdb36 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImpl.java @@ -50,7 +50,6 @@ import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.TransactionTemplate; import javax.annotation.PostConstruct; -import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -146,10 +145,15 @@ public class TermDeferredStorageSvcImpl implements ITermDeferredStorageSvc { ourLog.info("Saving {} deferred concepts...", count); while (codeCount < count && myDeferredConcepts.size() > 0) { TermConcept next = myDeferredConcepts.remove(0); - try { - codeCount += myCodeSystemStorageSvc.saveConcept(next); - } catch (Exception theE) { - ourLog.warn("Unable to save deferred TermConcept {}, possibly because Code System {} version PID {} may have since been replaced.", + if(myCodeSystemVersionDao.findById(next.getCodeSystemVersion().getPid()).isPresent()) { + try { + codeCount += myCodeSystemStorageSvc.saveConcept(next); + } catch (Exception theE) { + ourLog.error("Exception thrown when attempting to save TermConcept {} in Code System {}", + next.getCode(), next.getCodeSystemVersion().getCodeSystemDisplayName(), theE); + } + } else { + ourLog.warn("Unable to save deferred TermConcept {} because Code System {} version PID {} is no longer valid. Code system may have since been replaced.", next.getCode(), next.getCodeSystemVersion().getCodeSystemDisplayName(), next.getCodeSystemVersion().getPid()); } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java index e98b68c5270..131065c49e5 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TermDeferredStorageSvcImplTest.java @@ -13,6 +13,8 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import org.springframework.transaction.PlatformTransactionManager; +import java.util.Optional; + import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.*; @@ -50,6 +52,7 @@ public class TermDeferredStorageSvcImplTest { svc.setCodeSystemStorageSvcForUnitTest(myTermConceptStorageSvc); svc.setDaoConfigForUnitTest(new DaoConfig()); + when(myTermCodeSystemVersionDao.findById(anyLong())).thenReturn(Optional.of(myTermCodeSystemVersion)); svc.setCodeSystemVersionDaoForUnitTest(myTermCodeSystemVersionDao); svc.setProcessDeferred(true); svc.addConceptToStorageQueue(concept); @@ -65,6 +68,7 @@ public class TermDeferredStorageSvcImplTest { concept.setCode("CODE_A"); TermCodeSystemVersion myTermCodeSystemVersion = new TermCodeSystemVersion(); + myTermCodeSystemVersion.setId(1L); concept.setCodeSystemVersion(myTermCodeSystemVersion); TermDeferredStorageSvcImpl svc = new TermDeferredStorageSvcImpl(); @@ -72,6 +76,34 @@ public class TermDeferredStorageSvcImplTest { svc.setCodeSystemStorageSvcForUnitTest(myTermConceptStorageSvc); svc.setDaoConfigForUnitTest(new DaoConfig()); + when(myTermCodeSystemVersionDao.findById(anyLong())).thenReturn(Optional.empty()); + svc.setCodeSystemVersionDaoForUnitTest(myTermCodeSystemVersionDao); + svc.setProcessDeferred(true); + svc.addConceptToStorageQueue(concept); + svc.saveDeferred(); + + verify(myTermConceptStorageSvc, times(0)).saveConcept(same(concept)); + verifyNoMoreInteractions(myTermConceptStorageSvc); + + } + + @Test + public void testSaveDeferred_Concept_Exception() { + // There is a small + TermConcept concept = new TermConcept(); + concept.setCode("CODE_A"); + + TermCodeSystemVersion myTermCodeSystemVersion = new TermCodeSystemVersion(); + myTermCodeSystemVersion.setId(1L); + concept.setCodeSystemVersion(myTermCodeSystemVersion); + + TermDeferredStorageSvcImpl svc = new TermDeferredStorageSvcImpl(); + svc.setTransactionManagerForUnitTest(myTxManager); + svc.setCodeSystemStorageSvcForUnitTest(myTermConceptStorageSvc); + svc.setDaoConfigForUnitTest(new DaoConfig()); + + // Simulate the case where an exception is thrown despite a valid code system version. + when(myTermCodeSystemVersionDao.findById(anyLong())).thenReturn(Optional.of(myTermCodeSystemVersion)); when(myTermConceptStorageSvc.saveConcept(concept)).thenThrow(new RuntimeException("Foreign Constraint Violation")); svc.setCodeSystemVersionDaoForUnitTest(myTermCodeSystemVersionDao); svc.setProcessDeferred(true);