From fd6dcf636315eac7e81168601936beead5f7c403 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Wed, 25 Aug 2021 18:16:47 -0400 Subject: [PATCH 01/12] Fix bug in code. Add test. Add changelog --- .../2920-lookup-language-by-lang-only.yaml | 6 + .../CommonCodeSystemsTerminologyService.java | 135 +++++++++++------- ...mmonCodeSystemsTerminologyServiceTest.java | 7 + 3 files changed, 96 insertions(+), 52 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2920-lookup-language-by-lang-only.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2920-lookup-language-by-lang-only.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2920-lookup-language-by-lang-only.yaml new file mode 100644 index 00000000000..f77e2309f48 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2920-lookup-language-by-lang-only.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 2920 +jira: SMILE-2971 +title: "Previously, validation against bcp47 (urn:ietf:bcp:47) as a language would fail validation if the region was absent. This has been fixed, and the validate +operation will now correctly validate simple languages, e.g. `nl` instead of requiring `nl-DE` or `nl-NL`" diff --git a/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyService.java b/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyService.java index eae628d2965..f4891ed7398 100644 --- a/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyService.java +++ b/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyService.java @@ -257,69 +257,100 @@ public class CommonCodeSystemsTerminologyService implements IValidationSupport { Map languagesMap = myLanguagesLanugageMap; Map regionsMap = myLanguagesRegionMap; if (languagesMap == null || regionsMap == null) { + initializeBcp47LanguageMap(); + } - ourLog.info("Loading BCP47 Language Registry"); + int langRegionSeparatorIndex = StringUtils.indexOfAny(theCode, '-', '_'); + boolean hasRegionAndCodeSegments = langRegionSeparatorIndex > 0; + String language; + String region; - String input = ClasspathUtil.loadResource("org/hl7/fhir/common/hapi/validation/support/registry.json"); - ArrayNode map; - try { - map = (ArrayNode) new ObjectMapper().readTree(input); - } catch (JsonProcessingException e) { - throw new ConfigurationException(e); + if (hasRegionAndCodeSegments) { + language = myLanguagesLanugageMap.get(theCode.substring(0, langRegionSeparatorIndex)); + region = myLanguagesRegionMap.get(theCode.substring(langRegionSeparatorIndex + 1)); + + if (language == null || region == null) { + //In case the user provides both a language and a region, they must both be valid for the lookup to succeed. + ourLog.warn("Couldn't find a valid bcp47 language-region combination from code: {}", theCode); + return buildNotFoundLookupCodeResult(theCode); + } else { + return buildLookupResultForLanguageAndRegion(theCode, language, region); } - - languagesMap = new HashMap<>(); - regionsMap = new HashMap<>(); - - for (int i = 0; i < map.size(); i++) { - ObjectNode next = (ObjectNode) map.get(i); - String type = next.get("Type").asText(); - if ("language".equals(type)) { - String language = next.get("Subtag").asText(); - ArrayNode descriptions = (ArrayNode) next.get("Description"); - String description = null; - if (descriptions.size() > 0) { - description = descriptions.get(0).asText(); - } - languagesMap.put(language, description); - } - if ("region".equals(type)) { - String region = next.get("Subtag").asText(); - ArrayNode descriptions = (ArrayNode) next.get("Description"); - String description = null; - if (descriptions.size() > 0) { - description = descriptions.get(0).asText(); - } - regionsMap.put(region, description); - } - + } else { + //In case user has only provided a language, we build the lookup from only that. + language = myLanguagesLanugageMap.get(theCode); + if (language == null) { + ourLog.warn("Couldn't find a valid bcp47 language from code: {}", theCode); + return buildNotFoundLookupCodeResult(theCode); + } else { + return buildLookupResultForLanguage(theCode, language); } + } + } + private LookupCodeResult buildLookupResultForLanguageAndRegion(@Nonnull String theOriginalCode, @Nonnull String theLanguage, @Nonnull String theRegion) { + LookupCodeResult lookupCodeResult = buildNotFoundLookupCodeResult(theOriginalCode); + lookupCodeResult.setCodeDisplay(theLanguage + " " + theRegion); + lookupCodeResult.setFound(true); + return lookupCodeResult; + } + private LookupCodeResult buildLookupResultForLanguage(@Nonnull String theOriginalCode, @Nonnull String theLanguage) { + LookupCodeResult lookupCodeResult = buildNotFoundLookupCodeResult(theOriginalCode); + lookupCodeResult.setCodeDisplay(theLanguage); + lookupCodeResult.setFound(true); + return lookupCodeResult; + } - ourLog.info("Have {} languages and {} regions", languagesMap.size(), regionsMap.size()); + private LookupCodeResult buildNotFoundLookupCodeResult(@Nonnull String theOriginalCode) { + LookupCodeResult lookupCodeResult = new LookupCodeResult(); + lookupCodeResult.setFound(false); + lookupCodeResult.setSearchedForSystem(LANGUAGES_CODESYSTEM_URL); + lookupCodeResult.setSearchedForCode(theOriginalCode); + return lookupCodeResult; + } - myLanguagesLanugageMap = languagesMap; - myLanguagesRegionMap = regionsMap; + private void initializeBcp47LanguageMap() { + Map regionsMap; + Map languagesMap; + ourLog.info("Loading BCP47 Language Registry"); + + String input = ClasspathUtil.loadResource("org/hl7/fhir/common/hapi/validation/support/registry.json"); + ArrayNode map; + try { + map = (ArrayNode) new ObjectMapper().readTree(input); + } catch (JsonProcessingException e) { + throw new ConfigurationException(e); } - int idx = StringUtils.indexOfAny(theCode, '-', '_'); - String language = null; - String region = null; - if (idx > 0) { - language = languagesMap.get(theCode.substring(0, idx)); - region = regionsMap.get(theCode.substring(idx + 1)); + languagesMap = new HashMap<>(); + regionsMap = new HashMap<>(); + + for (int i = 0; i < map.size(); i++) { + ObjectNode next = (ObjectNode) map.get(i); + String type = next.get("Type").asText(); + if ("language".equals(type)) { + String language = next.get("Subtag").asText(); + ArrayNode descriptions = (ArrayNode) next.get("Description"); + String description = null; + if (descriptions.size() > 0) { + description = descriptions.get(0).asText(); + } + languagesMap.put(language, description); + } + if ("region".equals(type)) { + String region = next.get("Subtag").asText(); + ArrayNode descriptions = (ArrayNode) next.get("Description"); + String description = null; + if (descriptions.size() > 0) { + description = descriptions.get(0).asText(); + } + regionsMap.put(region, description); + } } - LookupCodeResult retVal = new LookupCodeResult(); - retVal.setSearchedForCode(theCode); - retVal.setSearchedForSystem(LANGUAGES_CODESYSTEM_URL); + ourLog.info("Have {} languages and {} regions", languagesMap.size(), regionsMap.size()); - if (language != null && region != null) { - String display = language + " " + region; - retVal.setFound(true); - retVal.setCodeDisplay(display); - } - - return retVal; + myLanguagesLanugageMap = languagesMap; + myLanguagesRegionMap = regionsMap; } @Nonnull diff --git a/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java b/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java index 5353864fc45..7204984a456 100644 --- a/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java +++ b/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java @@ -105,6 +105,13 @@ public class CommonCodeSystemsTerminologyServiceTest { assertEquals("English (United States)", outcome.getDisplay()); } + @Test + public void testLanguages_CommonLanguagesVs_OnlyLanguage_NoRegion() { + IValidationSupport.LookupCodeResult nl = mySvc.lookupCode(newSupport(), "urn:ietf:bcp:47", "nl"); + assertTrue(nl.isFound()); + assertEquals("Dutch", nl.getCodeDisplay()); + } + @Test public void testLanguages_CommonLanguagesVs_BadCode() { IValidationSupport.CodeValidationResult outcome = mySvc.validateCode(newSupport(), newOptions(), "urn:ietf:bcp:47", "FOO", null, "http://hl7.org/fhir/ValueSet/languages"); From 0304ea2d624181fdc383f8168a5652e8e262833e Mon Sep 17 00:00:00 2001 From: Tadgh Date: Thu, 26 Aug 2021 12:03:00 -0400 Subject: [PATCH 02/12] Add requested test --- .../fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java | 2 -- .../support/CommonCodeSystemsTerminologyServiceTest.java | 7 +++++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java index 25eecd5c164..51a43daf31e 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java @@ -104,7 +104,6 @@ import org.hl7.fhir.r4.model.Organization; import org.hl7.fhir.r4.model.Patient; import org.hl7.fhir.r4.model.Period; import org.hl7.fhir.r4.model.Practitioner; -import org.hl7.fhir.r4.model.Procedure; import org.hl7.fhir.r4.model.Provenance; import org.hl7.fhir.r4.model.Quantity; import org.hl7.fhir.r4.model.Questionnaire; @@ -5304,7 +5303,6 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test { createObservationWithEffective("YES22", "2011-01-02T00:00:00+10:00"); createObservationWithEffective("YES23", "2011-01-02T00:00:00+11:00"); - SearchParameterMap map = new SearchParameterMap(); map.setLoadSynchronous(true); map.add(Observation.SP_DATE, new DateParam("2011-01-02")); diff --git a/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java b/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java index 7204984a456..863eac1fd4f 100644 --- a/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java +++ b/hapi-fhir-validation/src/test/java/org/hl7/fhir/common/hapi/validation/support/CommonCodeSystemsTerminologyServiceTest.java @@ -112,6 +112,13 @@ public class CommonCodeSystemsTerminologyServiceTest { assertEquals("Dutch", nl.getCodeDisplay()); } + @Test + public void testLanguages_CommonLanguagesVs_LanguageAndRegion() { + IValidationSupport.LookupCodeResult nl = mySvc.lookupCode(newSupport(), "urn:ietf:bcp:47", "nl-NL"); + assertTrue(nl.isFound()); + assertEquals("Dutch Netherlands", nl.getCodeDisplay()); + } + @Test public void testLanguages_CommonLanguagesVs_BadCode() { IValidationSupport.CodeValidationResult outcome = mySvc.validateCode(newSupport(), newOptions(), "urn:ietf:bcp:47", "FOO", null, "http://hl7.org/fhir/ValueSet/languages"); From c2c6e0b440274400ce6a1aaf9292cba31fba62ef Mon Sep 17 00:00:00 2001 From: Tadgh Date: Fri, 27 Aug 2021 12:01:10 -0400 Subject: [PATCH 03/12] Force the caller to run the bundle task if we are operating with <= 1 pool size --- .../uhn/fhir/jpa/dao/BaseTransactionProcessor.java | 13 +++++++++---- .../ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java | 4 ++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index 3bcf236007b..fb66c22e0d5 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -154,7 +154,7 @@ public abstract class BaseTransactionProcessor { private InMemoryResourceMatcher myInMemoryResourceMatcher; private ThreadPoolTaskExecutor myExecutor ; - + @VisibleForTesting public void setDaoConfig(DaoConfig theDaoConfig) { myDaoConfig = theDaoConfig; @@ -349,7 +349,12 @@ public abstract class BaseTransactionProcessor { for (int i=0; i { private CountDownLatch myCompletedLatch; - private ServletRequestDetails myRequestDetails; + private RequestDetails myRequestDetails; private IBase myNextReqEntry; private Map myResponseMap; private int myResponseOrder; @@ -1565,7 +1570,7 @@ public abstract class BaseTransactionProcessor { protected BundleTask(CountDownLatch theCompletedLatch, RequestDetails theRequestDetails, Map theResponseMap, int theResponseOrder, IBase theNextReqEntry, boolean theNestedMode) { this.myCompletedLatch = theCompletedLatch; - this.myRequestDetails = (ServletRequestDetails)theRequestDetails; + this.myRequestDetails = theRequestDetails; this.myNextReqEntry = theNextReqEntry; this.myResponseMap = theResponseMap; this.myResponseOrder = theResponseOrder; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java index a1b7bb0ae52..d5428cc9fd1 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java @@ -117,12 +117,16 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest { myDaoConfig.setAllowInlineMatchUrlReferences(false); myDaoConfig.setAllowMultipleDelete(new DaoConfig().isAllowMultipleDelete()); myModelConfig.setNormalizedQuantitySearchLevel(NormalizedQuantitySearchLevel.NORMALIZED_QUANTITY_SEARCH_NOT_SUPPORTED); + myDaoConfig.setBundleBatchPoolSize(new DaoConfig().getBundleBatchPoolSize()); + myDaoConfig.setBundleBatchMaxPoolSize(new DaoConfig().getBundleBatchMaxPoolSize()); } @BeforeEach public void beforeDisableResultReuse() { myInterceptorRegistry.registerInterceptor(myInterceptor); myDaoConfig.setReuseCachedSearchResultsForMillis(null); + myDaoConfig.setBundleBatchPoolSize(1); + myDaoConfig.setBundleBatchMaxPoolSize(1); } private Bundle createInputTransactionWithPlaceholderIdInMatchUrl(HTTPVerb theVerb) { From faf4dfd056af79f250e2d7a82d1e4435a1ae5da3 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Fri, 27 Aug 2021 12:04:56 -0400 Subject: [PATCH 04/12] fix other test class --- .../java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java | 3 --- .../ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java | 4 ++++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index fb66c22e0d5..825e424471c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -174,9 +174,6 @@ public abstract class BaseTransactionProcessor { ourLog.trace("Starting transaction processor"); myExecutor = new ThreadPoolTaskExecutor(); myExecutor.setThreadNamePrefix("bundle_batch_"); - // For single thread set the value to 1 - //myExecutor.setCorePoolSize(1); - //myExecutor.setMaxPoolSize(1); myExecutor.setCorePoolSize(myDaoConfig.getBundleBatchPoolSize()); myExecutor.setMaxPoolSize(myDaoConfig.getBundleBatchMaxPoolSize()); myExecutor.setQueueCapacity(DaoConfig.DEFAULT_BUNDLE_BATCH_QUEUE_CAPACITY); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java index fc330a0e49d..a97890cbe02 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java @@ -116,12 +116,16 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest { myDaoConfig.setAllowMultipleDelete(new DaoConfig().isAllowMultipleDelete()); myDaoConfig.setIndexMissingFields(new DaoConfig().getIndexMissingFields()); myDaoConfig.setMaximumDeleteConflictQueryCount(new DaoConfig().getMaximumDeleteConflictQueryCount()); + myDaoConfig.setBundleBatchPoolSize(new DaoConfig().getBundleBatchPoolSize()); + myDaoConfig.setBundleBatchMaxPoolSize(new DaoConfig().getBundleBatchMaxPoolSize()); } @BeforeEach public void beforeDisableResultReuse() { myDaoConfig.setReuseCachedSearchResultsForMillis(null); + myDaoConfig.setBundleBatchPoolSize(1); + myDaoConfig.setBundleBatchMaxPoolSize(1); } private Bundle createInputTransactionWithPlaceholderIdInMatchUrl(HTTPVerb theVerb) { From 513bc387e9166c7a2dbfd1f84b3f947808988582 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Fri, 27 Aug 2021 13:20:29 -0400 Subject: [PATCH 05/12] Remove essentially duplicate bean definition --- .../ca/uhn/fhir/jpa/batch/CommonBatchJobConfig.java | 7 +++++++ .../jpa/reindex/job/ReindexEverythingJobConfig.java | 10 +++------- .../ca/uhn/fhir/jpa/reindex/job/ReindexJobConfig.java | 10 +++------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch/CommonBatchJobConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch/CommonBatchJobConfig.java index dc0ec59b1ff..a681e7e6bbb 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch/CommonBatchJobConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch/CommonBatchJobConfig.java @@ -22,6 +22,7 @@ package ca.uhn.fhir.jpa.batch; import ca.uhn.fhir.jpa.batch.processor.GoldenResourceAnnotatingProcessor; import ca.uhn.fhir.jpa.batch.processor.PidToIBaseResourceProcessor; +import ca.uhn.fhir.jpa.reindex.job.ReindexWriter; import org.springframework.batch.core.configuration.annotation.StepScope; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -41,4 +42,10 @@ public class CommonBatchJobConfig { return new GoldenResourceAnnotatingProcessor(); } + @Bean + @StepScope + public ReindexWriter reindexWriter() { + return new ReindexWriter(); + } + } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexEverythingJobConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexEverythingJobConfig.java index b0a0b77a642..ec66e2a3e0d 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexEverythingJobConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexEverythingJobConfig.java @@ -49,6 +49,8 @@ public class ReindexEverythingJobConfig { private StepBuilderFactory myStepBuilderFactory; @Autowired private JobBuilderFactory myJobBuilderFactory; + @Autowired + private ReindexWriter myReindexWriter; @Bean(name = REINDEX_EVERYTHING_JOB_NAME) @Lazy @@ -63,7 +65,7 @@ public class ReindexEverythingJobConfig { return myStepBuilderFactory.get(REINDEX_EVERYTHING_STEP_NAME) ., List>chunk(1) .reader(cronologicalBatchAllResourcePidReader()) - .writer(reindexWriter()) + .writer(myReindexWriter) .listener(reindexEverythingPidCountRecorderListener()) .listener(reindexEverythingPromotionListener()) .build(); @@ -75,12 +77,6 @@ public class ReindexEverythingJobConfig { return new CronologicalBatchAllResourcePidReader(); } - @Bean - @StepScope - public ReindexWriter reindexWriter() { - return new ReindexWriter(); - } - @Bean @StepScope public PidReaderCounterListener reindexEverythingPidCountRecorderListener() { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexJobConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexJobConfig.java index 9b5bf617332..ee62e9a0d93 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexJobConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/job/ReindexJobConfig.java @@ -51,6 +51,8 @@ public class ReindexJobConfig extends MultiUrlProcessorJobConfig { private StepBuilderFactory myStepBuilderFactory; @Autowired private JobBuilderFactory myJobBuilderFactory; + @Autowired + private ReindexWriter myReindexWriter; @Bean(name = REINDEX_JOB_NAME) @Lazy @@ -66,18 +68,12 @@ public class ReindexJobConfig extends MultiUrlProcessorJobConfig { return myStepBuilderFactory.get(REINDEX_URL_LIST_STEP_NAME) ., List>chunk(1) .reader(reverseCronologicalBatchResourcePidReader()) - .writer(reindexWriter()) + .writer(myReindexWriter) .listener(pidCountRecorderListener()) .listener(reindexPromotionListener()) .build(); } - @Bean - @StepScope - public ReindexWriter reindexWriter() { - return new ReindexWriter(); - } - @Bean public ExecutionContextPromotionListener reindexPromotionListener() { ExecutionContextPromotionListener listener = new ExecutionContextPromotionListener(); From 7534ab58f3db6a421c7fce9c7ba3733693cd95fa Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 31 Aug 2021 09:58:38 -0400 Subject: [PATCH 06/12] Add failing test --- .../fhir/jpa/dao/TransactionProcessor.java | 4 +- .../r4/FhirResourceDaoR4SearchNoFtTest.java | 80 +++++++++++++++++++ .../src/test/resources/logback-test.xml | 2 +- 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java index 748ffa6352b..bc73ea4a623 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java @@ -261,7 +261,7 @@ public class TransactionProcessor extends BaseTransactionProcessor { // No matches .filter(match -> !match.myResolved) .forEach(match -> { - ourLog.warn("Was unable to match url {} from database", match.myRequestUrl); + ourLog.debug("Was unable to match url {} from database", match.myRequestUrl); theTransactionDetails.addResolvedMatchUrl(match.myRequestUrl, TransactionDetails.NOT_FOUND); }); } @@ -337,7 +337,7 @@ public class TransactionProcessor extends BaseTransactionProcessor { } private void setSearchToResolvedAndPrefetchFoundResourcePid(TransactionDetails theTransactionDetails, List idsToPreFetch, ResourceIndexedSearchParamToken nextResult, MatchUrlToResolve nextSearchParameterMap) { - ourLog.warn("Matched url {} from database", nextSearchParameterMap.myRequestUrl); + ourLog.debug("Matched url {} from database", nextSearchParameterMap.myRequestUrl); idsToPreFetch.add(nextResult.getResourcePid()); myMatchResourceUrlService.matchUrlResolved(theTransactionDetails, nextSearchParameterMap.myResourceDefinition.getName(), nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid())); theTransactionDetails.addResolvedMatchUrl(nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid())); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java index 51a43daf31e..c7782f16389 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java @@ -20,6 +20,7 @@ import ca.uhn.fhir.jpa.model.entity.ResourceLink; import ca.uhn.fhir.jpa.model.entity.ResourceTable; import ca.uhn.fhir.jpa.model.search.StorageProcessingMessage; import ca.uhn.fhir.jpa.model.util.UcumServiceUtil; +import ca.uhn.fhir.jpa.partition.SystemRequestDetails; import ca.uhn.fhir.jpa.searchparam.MatchUrlService; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap.EverythingModeEnum; @@ -150,12 +151,14 @@ import java.util.stream.Collectors; import static ca.uhn.fhir.rest.api.Constants.PARAM_TYPE; import static org.apache.commons.lang3.StringUtils.countMatches; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.endsWith; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.hasSize; @@ -1356,6 +1359,83 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test { assertThat(actual, contains(id)); } + @Test + public void testDuplicateConditionalCreatesOnToken() { + String bundle = "{\n" + + " \"resourceType\": \"Bundle\",\n" + + " \"type\": \"transaction\",\n" + + " \"entry\": [ {\n" + + " \"fullUrl\": \"urn:uuid:33b76421-1c91-471f-ae1c-e7486e804f18\",\n" + + " \"resource\": {\n" + + " \"resourceType\": \"Organization\",\n" + + " \"identifier\": [ {\n" + + " \"system\": \"https://fhir.infoway-inforoute.ca/NamingSystem/ca-on-health-care-facility-id\",\n" + + " \"value\": \"3972\"\n" + + " } ]\n" + + " },\n" + + " \"request\": {\n" + + " \"method\": \"POST\",\n" + + " \"url\": \"/Organization\",\n" + + " \"ifNoneExist\": \"Organization?identifier=https%3A%2F%2Ffhir.infoway-inforoute.ca%2FNamingSystem%2Fca-on-health-care-facility-id|3972\"\n" + + " }\n" + + " }, {\n" + + " \"fullUrl\": \"urn:uuid:65d2bf18-543e-4d05-b66b-07cee541172f\",\n" + + " \"resource\": {\n" + + " \"resourceType\": \"Organization\",\n" + + " \"identifier\": [ {\n" + + " \"system\": \"https://fhir.infoway-inforoute.ca/NamingSystem/ca-on-health-care-facility-id\",\n" + + " \"value\": \"3972\"\n" + + " } ]\n" + + " },\n" + + " \"request\": {\n" + + " \"method\": \"POST\",\n" + + " \"url\": \"/Organization\",\n" + + " \"ifNoneExist\": \"Organization?identifier=https%3A%2F%2Ffhir.infoway-inforoute.ca%2FNamingSystem%2Fca-on-health-care-facility-id|3972\"\n" + + " }\n" + + " }, {\n" + + " \"fullUrl\": \"urn:uuid:2a4635e2-e678-4ed7-9a92-901d67787434\",\n" + + " \"resource\": {\n" + + " \"resourceType\": \"ServiceRequest\",\n" + + " \"identifier\": [ {\n" + + " \"system\": \"https://corhealth-ontario.ca/NamingSystem/service-request-id\",\n" + + " \"value\": \"1\"\n" + + " } ],\n" + + " \"performer\": [ {\n" + + " \"reference\": \"urn:uuid:65d2bf18-543e-4d05-b66b-07cee541172f\",\n" + + " \"type\": \"Organization\"\n" + + " } ]\n" + + " },\n" + + " \"request\": {\n" + + " \"method\": \"PUT\",\n" + + " \"url\": \"/ServiceRequest?identifier=https%3A%2F%2Fcorhealth-ontario.ca%2FNamingSystem%2Fservice-request-id|1\"\n" + + " }\n" + + " } ]\n" + + "}"; + + Bundle bundle1 = (Bundle) myFhirCtx.newJsonParser().parseResource(bundle); + Bundle duplicateBundle = (Bundle) myFhirCtx.newJsonParser().parseResource(bundle); + ourLog.error("TRANS 1"); + Bundle bundleResponse = mySystemDao.transaction(new SystemRequestDetails(), bundle1); + bundleResponse.getEntry().stream() + .forEach( entry -> { + assertThat(entry.getResponse().getStatus(), is(equalTo("201 Created"))); + }); + + IBundleProvider search = myOrganizationDao.search(new SearchParameterMap().setLoadSynchronous(true)); + assertEquals(1, search.getAllResources().size()); + + //Running the bundle again should just result in 0 new resources created, as the org should already exist, and there is no update to the SR. + ourLog.error("TRANS 2"); + bundleResponse= mySystemDao.transaction(new SystemRequestDetails(), duplicateBundle); + bundleResponse.getEntry().stream() + .forEach( entry -> { + assertThat(entry.getResponse().getStatus(), is(equalTo("200 OK"))); + }); + + search = myOrganizationDao.search(new SearchParameterMap().setLoadSynchronous(true), new SystemRequestDetails()); + assertEquals(1, search.getAllResources().size()); + } + @Test public void testIndexNoDuplicatesToken() { Patient res = new Patient(); diff --git a/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml b/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml index ac75e0d04be..36a83af6599 100644 --- a/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml +++ b/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml @@ -42,7 +42,7 @@ - + From 0f6ae50105b3ba2e7c6e57f276b9400d5c1a46d9 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 31 Aug 2021 10:47:23 -0400 Subject: [PATCH 07/12] Fix bug in hashToSearchMap --- .../fhir/jpa/dao/TransactionProcessor.java | 22 ++++-- .../r4/FhirResourceDaoR4SearchNoFtTest.java | 74 ++++--------------- .../duplicate-conditional-create.json | 66 +++++++++++++++++ 3 files changed, 94 insertions(+), 68 deletions(-) create mode 100644 hapi-fhir-jpaserver-base/src/test/resources/duplicate-conditional-create.json diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java index bc73ea4a623..c8e25d517e3 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java @@ -244,17 +244,21 @@ public class TransactionProcessor extends BaseTransactionProcessor { if (orPredicates.size() > 1) { cq.where(cb.or(orPredicates.toArray(EMPTY_PREDICATE_ARRAY))); - Map hashToSearchMap = buildHashToSearchMap(searchParameterMapsToResolve); + Map> hashToSearchMap = buildHashToSearchMap(searchParameterMapsToResolve); TypedQuery query = myEntityManager.createQuery(cq); List results = query.getResultList(); for (ResourceIndexedSearchParamToken nextResult : results) { - Optional matchedSearch = Optional.ofNullable(hashToSearchMap.get(nextResult.getHashSystemAndValue())); + Optional> matchedSearch = Optional.ofNullable(hashToSearchMap.get(nextResult.getHashSystemAndValue())); if (!matchedSearch.isPresent()) { matchedSearch = Optional.ofNullable(hashToSearchMap.get(nextResult.getHashValue())); } - matchedSearch.ifPresent(matchUrlToResolve -> setSearchToResolvedAndPrefetchFoundResourcePid(theTransactionDetails, idsToPreFetch, nextResult, matchUrlToResolve)); + matchedSearch.ifPresent(matchUrlsToResolve -> { + matchUrlsToResolve.forEach(matchUrl -> { + setSearchToResolvedAndPrefetchFoundResourcePid(theTransactionDetails, idsToPreFetch, nextResult, matchUrl); + }); + }); } //For each SP Map which did not return a result, tag it as not found. searchParameterMapsToResolve.stream() @@ -322,15 +326,19 @@ public class TransactionProcessor extends BaseTransactionProcessor { return hashPredicate; } - private Map buildHashToSearchMap(List searchParameterMapsToResolve) { - Map hashToSearch = new HashMap<>(); + private Map> buildHashToSearchMap(List searchParameterMapsToResolve) { + Map> hashToSearch = new HashMap<>(); //Build a lookup map so we don't have to iterate over the searches repeatedly. for (MatchUrlToResolve nextSearchParameterMap : searchParameterMapsToResolve) { if (nextSearchParameterMap.myHashSystemAndValue != null) { - hashToSearch.put(nextSearchParameterMap.myHashSystemAndValue, nextSearchParameterMap); + List matchUrlsToResolve = hashToSearch.getOrDefault(nextSearchParameterMap.myHashSystemAndValue, new ArrayList<>()); + matchUrlsToResolve.add(nextSearchParameterMap); + hashToSearch.put(nextSearchParameterMap.myHashSystemAndValue, matchUrlsToResolve); } if (nextSearchParameterMap.myHashValue!= null) { - hashToSearch.put(nextSearchParameterMap.myHashValue, nextSearchParameterMap); + List matchUrlsToResolve = hashToSearch.getOrDefault(nextSearchParameterMap.myHashValue, new ArrayList<>()); + matchUrlsToResolve.add(nextSearchParameterMap); + hashToSearch.put(nextSearchParameterMap.myHashValue, matchUrlsToResolve); } } return hashToSearch; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java index c7782f16389..71b3e7bcdc2 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java @@ -127,6 +127,7 @@ import org.hl7.fhir.r4.model.ValueSet; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; @@ -1360,74 +1361,25 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test { } @Test - public void testDuplicateConditionalCreatesOnToken() { - String bundle = "{\n" + - " \"resourceType\": \"Bundle\",\n" + - " \"type\": \"transaction\",\n" + - " \"entry\": [ {\n" + - " \"fullUrl\": \"urn:uuid:33b76421-1c91-471f-ae1c-e7486e804f18\",\n" + - " \"resource\": {\n" + - " \"resourceType\": \"Organization\",\n" + - " \"identifier\": [ {\n" + - " \"system\": \"https://fhir.infoway-inforoute.ca/NamingSystem/ca-on-health-care-facility-id\",\n" + - " \"value\": \"3972\"\n" + - " } ]\n" + - " },\n" + - " \"request\": {\n" + - " \"method\": \"POST\",\n" + - " \"url\": \"/Organization\",\n" + - " \"ifNoneExist\": \"Organization?identifier=https%3A%2F%2Ffhir.infoway-inforoute.ca%2FNamingSystem%2Fca-on-health-care-facility-id|3972\"\n" + - " }\n" + - " }, {\n" + - " \"fullUrl\": \"urn:uuid:65d2bf18-543e-4d05-b66b-07cee541172f\",\n" + - " \"resource\": {\n" + - " \"resourceType\": \"Organization\",\n" + - " \"identifier\": [ {\n" + - " \"system\": \"https://fhir.infoway-inforoute.ca/NamingSystem/ca-on-health-care-facility-id\",\n" + - " \"value\": \"3972\"\n" + - " } ]\n" + - " },\n" + - " \"request\": {\n" + - " \"method\": \"POST\",\n" + - " \"url\": \"/Organization\",\n" + - " \"ifNoneExist\": \"Organization?identifier=https%3A%2F%2Ffhir.infoway-inforoute.ca%2FNamingSystem%2Fca-on-health-care-facility-id|3972\"\n" + - " }\n" + - " }, {\n" + - " \"fullUrl\": \"urn:uuid:2a4635e2-e678-4ed7-9a92-901d67787434\",\n" + - " \"resource\": {\n" + - " \"resourceType\": \"ServiceRequest\",\n" + - " \"identifier\": [ {\n" + - " \"system\": \"https://corhealth-ontario.ca/NamingSystem/service-request-id\",\n" + - " \"value\": \"1\"\n" + - " } ],\n" + - " \"performer\": [ {\n" + - " \"reference\": \"urn:uuid:65d2bf18-543e-4d05-b66b-07cee541172f\",\n" + - " \"type\": \"Organization\"\n" + - " } ]\n" + - " },\n" + - " \"request\": {\n" + - " \"method\": \"PUT\",\n" + - " \"url\": \"/ServiceRequest?identifier=https%3A%2F%2Fcorhealth-ontario.ca%2FNamingSystem%2Fservice-request-id|1\"\n" + - " }\n" + - " } ]\n" + - "}"; + @DisplayName("Duplicate Conditional Creates all resolve to the same match") + public void testDuplicateConditionalCreatesOnToken() throws IOException { + String inputString = IOUtils.toString(getClass().getResourceAsStream("/duplicate-conditional-create.json"), StandardCharsets.UTF_8); + Bundle firstBundle = myFhirCtx.newJsonParser().parseResource(Bundle.class, inputString); - Bundle bundle1 = (Bundle) myFhirCtx.newJsonParser().parseResource(bundle); - Bundle duplicateBundle = (Bundle) myFhirCtx.newJsonParser().parseResource(bundle); - ourLog.error("TRANS 1"); - Bundle bundleResponse = mySystemDao.transaction(new SystemRequestDetails(), bundle1); - bundleResponse.getEntry().stream() - .forEach( entry -> { - assertThat(entry.getResponse().getStatus(), is(equalTo("201 Created"))); - }); + //Before you ask, yes, this has to be separately parsed. The reason for this is that the parameters passed to mySystemDao.transaction are _not_ immutable, so we cannot + //simply reuse the original bundle object. + Bundle duplicateBundle = myFhirCtx.newJsonParser().parseResource(Bundle.class, inputString); + + Bundle bundleResponse = mySystemDao.transaction(new SystemRequestDetails(), firstBundle); + bundleResponse.getEntry() + .forEach( entry -> assertThat(entry.getResponse().getStatus(), is(equalTo("201 Created")))); IBundleProvider search = myOrganizationDao.search(new SearchParameterMap().setLoadSynchronous(true)); assertEquals(1, search.getAllResources().size()); //Running the bundle again should just result in 0 new resources created, as the org should already exist, and there is no update to the SR. - ourLog.error("TRANS 2"); bundleResponse= mySystemDao.transaction(new SystemRequestDetails(), duplicateBundle); - bundleResponse.getEntry().stream() + bundleResponse.getEntry() .forEach( entry -> { assertThat(entry.getResponse().getStatus(), is(equalTo("200 OK"))); }); diff --git a/hapi-fhir-jpaserver-base/src/test/resources/duplicate-conditional-create.json b/hapi-fhir-jpaserver-base/src/test/resources/duplicate-conditional-create.json new file mode 100644 index 00000000000..26ea0369f1f --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/resources/duplicate-conditional-create.json @@ -0,0 +1,66 @@ +{ + "resourceType": "Bundle", + "type": "transaction", + "entry": [ + { + "fullUrl": "urn:uuid:4cd35592-5d4d-462b-8483-e404c023d316", + "resource": { + "resourceType": "Organization", + "identifier": [ + { + "system": "https://fhir.tester.ca/NamingSystem/ca-on-health-care-facility-id", + "value": "3972" + } + ] + }, + "request": { + "method": "POST", + "url": "/Organization", + "ifNoneExist": "Organization?identifier=https://fhir.tester.ca/NamingSystem/ca-on-health-care-facility-id|3972" + } + }, + { + "fullUrl": "urn:uuid:02643c1d-94d1-4991-a063-036fa0f57ec2", + "resource": { + "resourceType": "Organization", + "identifier": [ + { + "system": "https://fhir.tester.ca/NamingSystem/ca-on-health-care-facility-id", + "value": "3972" + } + ] + }, + "request": { + "method": "POST", + "url": "/Organization", + "ifNoneExist": "Organization?identifier=https://fhir.tester.ca/NamingSystem/ca-on-health-care-facility-id|3972" + } + }, + { + "fullUrl": "urn:uuid:8271e94f-e08b-498e-ad6d-751928c3ff99", + "resource": { + "resourceType": "ServiceRequest", + "identifier": [ + { + "system": "https://fhir-tester.ca/NamingSystem/service-request-id", + "value": "1" + } + ], + "performer": [ + { + "reference": "urn:uuid:4cd35592-5d4d-462b-8483-e404c023d316", + "type": "Organization" + }, + { + "reference": "urn:uuid:02643c1d-94d1-4991-a063-036fa0f57ec2", + "type": "Organization" + } + ] + }, + "request": { + "method": "PUT", + "url": "/ServiceRequest?identifier=https://fhir-tester.ca/NamingSystem/service-request-id|1" + } + } + ] +} From 37c88392b4db6762c694e69d38d2d2d12b801f32 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 31 Aug 2021 10:53:14 -0400 Subject: [PATCH 08/12] Add changelog --- .../changelog/5_6_0/2933-fix-double-conditional-create.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2933-fix-double-conditional-create.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2933-fix-double-conditional-create.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2933-fix-double-conditional-create.yaml new file mode 100644 index 00000000000..49727c0125c --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2933-fix-double-conditional-create.yaml @@ -0,0 +1,6 @@ +--- +type: add +issue: 2933 +jira: SMILE-3056 +title: "Fixed a regression which causes transactions with multiple identical ifNoneExist clauses to create duplicate data." + From 2f6d8c7b35e1ba1c42d2d7fc78d93fc03e0eecec Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 31 Aug 2021 10:56:31 -0400 Subject: [PATCH 09/12] Revert logging --- hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml b/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml index 36a83af6599..ac75e0d04be 100644 --- a/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml +++ b/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml @@ -42,7 +42,7 @@ - + From c9f5dfd6d339e232ff89c9e9484b3a050d6725e8 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 31 Aug 2021 11:33:06 -0400 Subject: [PATCH 10/12] Swap to SyncTaskExecutor --- .../jpa/dao/BaseTransactionProcessor.java | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index 825e424471c..b7308c75eda 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -94,6 +94,8 @@ import org.hl7.fhir.r4.model.Task; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.core.task.SyncTaskExecutor; +import org.springframework.core.task.TaskExecutor; import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; import org.springframework.transaction.PlatformTransactionManager; import org.springframework.transaction.TransactionDefinition; @@ -153,7 +155,7 @@ public abstract class BaseTransactionProcessor { @Autowired private InMemoryResourceMatcher myInMemoryResourceMatcher; - private ThreadPoolTaskExecutor myExecutor ; + private TaskExecutor myExecutor ; @VisibleForTesting public void setDaoConfig(DaoConfig theDaoConfig) { @@ -172,13 +174,19 @@ public abstract class BaseTransactionProcessor { @PostConstruct public void start() { ourLog.trace("Starting transaction processor"); - myExecutor = new ThreadPoolTaskExecutor(); - myExecutor.setThreadNamePrefix("bundle_batch_"); - myExecutor.setCorePoolSize(myDaoConfig.getBundleBatchPoolSize()); - myExecutor.setMaxPoolSize(myDaoConfig.getBundleBatchMaxPoolSize()); - myExecutor.setQueueCapacity(DaoConfig.DEFAULT_BUNDLE_BATCH_QUEUE_CAPACITY); + if (myDaoConfig.getBundleBatchPoolSize() > 1) { + ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); + executor.setThreadNamePrefix("bundle_batch_"); + executor.setCorePoolSize(myDaoConfig.getBundleBatchPoolSize()); + executor.setMaxPoolSize(myDaoConfig.getBundleBatchMaxPoolSize()); + executor.setQueueCapacity(DaoConfig.DEFAULT_BUNDLE_BATCH_QUEUE_CAPACITY); + executor.initialize(); + myExecutor = executor; - myExecutor.initialize(); + } else { + SyncTaskExecutor executor = new SyncTaskExecutor(); + myExecutor = executor; + } } public BUNDLE transaction(RequestDetails theRequestDetails, BUNDLE theRequest, boolean theNestedMode) { @@ -346,12 +354,7 @@ public abstract class BaseTransactionProcessor { for (int i=0; i { + public class BundleTask implements Runnable { private CountDownLatch myCompletedLatch; private RequestDetails myRequestDetails; @@ -1575,10 +1578,8 @@ public abstract class BaseTransactionProcessor { } @Override - public Void call() { - + public void run() { BaseServerResponseExceptionHolder caughtEx = new BaseServerResponseExceptionHolder(); - try { IBaseBundle subRequestBundle = myVersionAdapter.createBundle(org.hl7.fhir.r4.model.Bundle.BundleType.TRANSACTION.toCode()); myVersionAdapter.addEntry(subRequestBundle, (IBase) myNextReqEntry); @@ -1611,7 +1612,6 @@ public abstract class BaseTransactionProcessor { // checking for the parallelism ourLog.debug("processing bacth for {} is completed", myVersionAdapter.getEntryRequestUrl((IBase)myNextReqEntry)); myCompletedLatch.countDown(); - return null; } } } From b1515b5963c1b750d62b18d5c97097539233d5ab Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 31 Aug 2021 12:57:45 -0400 Subject: [PATCH 11/12] Lazy load executor so that tests work safely. --- .../jpa/dao/BaseTransactionProcessor.java | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index b7308c75eda..91fb57f8621 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -174,19 +174,24 @@ public abstract class BaseTransactionProcessor { @PostConstruct public void start() { ourLog.trace("Starting transaction processor"); - if (myDaoConfig.getBundleBatchPoolSize() > 1) { - ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); - executor.setThreadNamePrefix("bundle_batch_"); - executor.setCorePoolSize(myDaoConfig.getBundleBatchPoolSize()); - executor.setMaxPoolSize(myDaoConfig.getBundleBatchMaxPoolSize()); - executor.setQueueCapacity(DaoConfig.DEFAULT_BUNDLE_BATCH_QUEUE_CAPACITY); - executor.initialize(); - myExecutor = executor; + } + public TaskExecutor getTaskExecutor() { + if (myExecutor == null) { + if (myDaoConfig.getBundleBatchPoolSize() > 1) { + ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); + executor.setThreadNamePrefix("bundle_batch_"); + executor.setCorePoolSize(myDaoConfig.getBundleBatchPoolSize()); + executor.setMaxPoolSize(myDaoConfig.getBundleBatchMaxPoolSize()); + executor.setQueueCapacity(DaoConfig.DEFAULT_BUNDLE_BATCH_QUEUE_CAPACITY); + executor.initialize(); + myExecutor = executor; - } else { - SyncTaskExecutor executor = new SyncTaskExecutor(); - myExecutor = executor; + } else { + SyncTaskExecutor executor = new SyncTaskExecutor(); + myExecutor = executor; + } } + return myExecutor; } public BUNDLE transaction(RequestDetails theRequestDetails, BUNDLE theRequest, boolean theNestedMode) { @@ -354,7 +359,7 @@ public abstract class BaseTransactionProcessor { for (int i=0; i Date: Tue, 31 Aug 2021 12:58:00 -0400 Subject: [PATCH 12/12] Chnge privacy --- .../java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index 91fb57f8621..08e6f003a7c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -175,7 +175,8 @@ public abstract class BaseTransactionProcessor { public void start() { ourLog.trace("Starting transaction processor"); } - public TaskExecutor getTaskExecutor() { + + private TaskExecutor getTaskExecutor() { if (myExecutor == null) { if (myDaoConfig.getBundleBatchPoolSize() > 1) { ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();