diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_6_0/7217-non-partitionable-resource-is-searched-on-non-default-partition.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_6_0/7217-non-partitionable-resource-is-searched-on-non-default-partition.yaml new file mode 100644 index 000000000000..e1338c1e9385 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_6_0/7217-non-partitionable-resource-is-searched-on-non-default-partition.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 7217 +title: "Previously, searches for non-partitionable resources (e.g. a Library) could be executed against a partition that +is not the default partition. This has been corrected." diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BasePartitioningR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BasePartitioningR4Test.java index 0cd2b14d6b62..2ea3676b03ca 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BasePartitioningR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BasePartitioningR4Test.java @@ -153,7 +153,6 @@ public void afterPurgeDatabase() { protected void createUniqueComboSp() { addNextTargetPartitionForCreateWithIdDefaultPartition(); - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation SearchParameter sp = new SearchParameter(); sp.setId("SearchParameter/patient-gender"); sp.setType(Enumerations.SearchParamType.TOKEN); @@ -164,7 +163,6 @@ protected void createUniqueComboSp() { mySearchParameterDao.update(sp, mySrd); addNextTargetPartitionForCreateWithIdDefaultPartition(); - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation sp = new SearchParameter(); sp.setId("SearchParameter/patient-family"); sp.setType(Enumerations.SearchParamType.STRING); @@ -198,7 +196,6 @@ protected void createUniqueComboSp() { protected void createNonUniqueComboSp() { addNextTargetPartitionForCreateWithIdDefaultPartition(); - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation SearchParameter sp = new SearchParameter(); sp.setId("SearchParameter/patient-family"); sp.setType(Enumerations.SearchParamType.STRING); @@ -209,7 +206,6 @@ protected void createNonUniqueComboSp() { mySearchParameterDao.update(sp, mySrd); addNextTargetPartitionForCreateWithIdDefaultPartition(); - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation sp = new SearchParameter(); sp.setId("SearchParameter/patient-managingorg"); sp.setType(Enumerations.SearchParamType.REFERENCE); @@ -346,6 +342,11 @@ protected void addNextTargetPartitionForUpdate(int thePartitionId) { addNextTargetPartitionForUpdate(fromPartitionId(thePartitionId)); } + protected RequestPartitionId withPartitionNames(String... thePartitionNames){ + Validate.notNull(thePartitionNames); + Validate.isTrue(thePartitionNames.length > 0); + return fromPartitionNames(thePartitionNames); + } protected void addNextTargetPartitionsForRead(Integer... thePartitionId) { Validate.notNull(thePartitionId); @@ -395,22 +396,9 @@ protected ICreationArgument withCreatePartition(Integer thePartitionId) { }; } - protected ICreationArgument withReadWritePartitions(Integer thePartitionId) { - return t -> { - if (thePartitionId != null) { - addNextTargetPartitionsForRead(thePartitionId); - addNextTargetPartitionForCreate(thePartitionId, null); - } else { - addNextTargetPartitionForReadDefaultPartition(); - addNextTargetPartitionForCreateDefaultPartition(); - } - }; - } - @Interceptor public static class MyReadWriteInterceptor extends MyWriteInterceptor { - private final List myReadRequestPartitionIds = new ArrayList<>(); public void addNextIterceptorReadResult(RequestPartitionId theRequestPartitionId) { diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningNonNullDefaultPartitionR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningNonNullDefaultPartitionR4Test.java index dd2264b9701a..152ebc0158d5 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningNonNullDefaultPartitionR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningNonNullDefaultPartitionR4Test.java @@ -243,9 +243,7 @@ private Encounter createEncounter(IParser jsonParser) { @Test public void testCreateAndSearch_NonPartitionable() { addNextTargetPartitionForCreateDefaultPartition(); - // we need two read partition accesses for when the creation of the SP triggers a reindex of Patient - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation - addNextTargetPartitionForReadDefaultPartition(); // and one for the reindex job + SearchParameter sp = new SearchParameter(); sp.addBase("Patient"); sp.setStatus(Enumerations.PublicationStatus.ACTIVE); @@ -261,12 +259,10 @@ public void testCreateAndSearch_NonPartitionable() { }); // Search on Token - addNextTargetPartitionForReadDefaultPartition(); List outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous().add("code", new TokenParam("extpatorg")), mySrd)); assertThat(outcome).containsExactly("SearchParameter/" + id); // Search on All Resources - addNextTargetPartitionForReadDefaultPartition(); outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous(), mySrd)); assertThat(outcome).containsExactly("SearchParameter/" + id); @@ -275,8 +271,6 @@ public void testCreateAndSearch_NonPartitionable() { @Test public void testCreateAndSearch_NonPartitionable_ForcedId() { addNextTargetPartitionForCreateWithIdDefaultPartition(); - // we need two read partition accesses for when the creation of the SP triggers a reindex of Patient - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation addNextTargetPartitionForReadDefaultPartition(); // and one for the reindex job SearchParameter sp = new SearchParameter(); sp.setId("SearchParameter/A"); @@ -294,12 +288,10 @@ public void testCreateAndSearch_NonPartitionable_ForcedId() { }); // Search on Token - addNextTargetPartitionForReadDefaultPartition(); List outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous().add("code", new TokenParam("extpatorg")), mySrd)); assertThat(outcome).containsExactly("SearchParameter/A"); // Search on All Resources - addNextTargetPartitionForReadDefaultPartition(); outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous(), mySrd)); assertThat(outcome).containsExactly("SearchParameter/A"); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java index dacd11ad69c1..45bfb4e4419b 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java @@ -137,9 +137,8 @@ public void afterEach() { @Test public void testCreateSearchParameter_DefaultPartition() { + addNextInterceptorCreateResult(defaultPartition()); - addNextTargetPartitionForCreateDefaultPartition(); - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation SearchParameter sp = new SearchParameter(); sp.addBase("Patient"); sp.setStatus(Enumerations.PublicationStatus.ACTIVE); @@ -314,8 +313,7 @@ public void testCreate_SamePartitionReference_DefaultPartition_ByForcedId() { @Test public void testCreateSearchParameter_DefaultPartitionWithDate() { - addNextTargetPartitionForCreateDefaultPartition(myPartitionDate); - addNextTargetPartitionForReadDefaultPartition(); // one for search param validation + addNextInterceptorCreateResult(fromPartitionId(null, myPartitionDate)); SearchParameter sp = new SearchParameter(); sp.addBase("Patient"); @@ -339,7 +337,7 @@ public void testCreateSearchParameter_DefaultPartitionWithDate() { @Test public void testCreateSearchParameter_NonDefaultPartition() { - addNextTargetPartitionForCreate(myPartitionId, myPartitionDate); + addNextInterceptorCreateResult(fromPartitionId(myPartitionId, myPartitionDate)); SearchParameter sp = new SearchParameter(); sp.addBase("Patient"); @@ -963,7 +961,7 @@ public void testRead_PidId_MultiplePartitionNames() { { myCaptureQueriesListener.clear(); assertNoRemainingPartitionIds(); - myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, PARTITION_2)); + addNextInterceptorReadResult(withPartitionNames(PARTITION_1, PARTITION_2)); IdType gotId1 = myPatientDao.read(patientId1, mySrd).getIdElement().toUnqualifiedVersionless(); assertEquals(patientId1, gotId1); @@ -977,7 +975,8 @@ public void testRead_PidId_MultiplePartitionNames() { // Two partitions including default - Found { myCaptureQueriesListener.clear(); - myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, JpaConstants.DEFAULT_PARTITION_NAME)); + addNextInterceptorReadResult(withPartitionNames(PARTITION_1, JpaConstants.DEFAULT_PARTITION_NAME)); + IdType gotId1; try { gotId1 = myPatientDao.read(patientIdNull, mySrd).getIdElement().toUnqualifiedVersionless(); @@ -996,7 +995,7 @@ public void testRead_PidId_MultiplePartitionNames() { // Two partitions - Not Found { - myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, PARTITION_2)); + addNextInterceptorReadResult(withPartitionNames(PARTITION_1, PARTITION_2)); try { myPatientDao.read(patientId3, mySrd); fail(); @@ -1004,7 +1003,7 @@ public void testRead_PidId_MultiplePartitionNames() { // good } - myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, PARTITION_2)); + addNextInterceptorReadResult(withPartitionNames(PARTITION_1, PARTITION_2)); try { myPatientDao.read(patientIdNull, mySrd); fail(); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/partition/RequestPartitionHelperSvcTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/partition/RequestPartitionHelperSvcTest.java index 910c35c9e21e..e73845b982f3 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/partition/RequestPartitionHelperSvcTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/partition/RequestPartitionHelperSvcTest.java @@ -5,8 +5,10 @@ import ca.uhn.fhir.jpa.entity.PartitionEntity; import ca.uhn.fhir.jpa.model.config.PartitionSettings; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; +import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import org.hl7.fhir.r4.model.IdType; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.AfterEach; @@ -40,6 +42,8 @@ class RequestPartitionHelperSvcTest extends BaseJpaR4Test { static final int UNKNOWN_PARTITION_ID = 1_000_000; static final String UNKNOWN_PARTITION_NAME = "UNKNOWN"; + static final Integer DEFAULT_PARTITION_ID = null; + @Autowired IPartitionDao myPartitionDao; @Autowired @@ -79,6 +83,34 @@ public void testDetermineReadPartitionForSystemRequest_withPartitionIdOnly_retur assertEquals(PARTITION_NAME_1, result.getFirstPartitionNameOrNull()); } + + @Test + public void testDetermineReadPartitionForServletRequest_whenResourceIsNonPartitionable() { + PartitionEntity partition1 = createPartition1(); + ServletRequestDetails servletRequestDetails = new ServletRequestDetails(); + servletRequestDetails.setTenantId(partition1.getName()); + + testDetermineReadPartitionRequest_whenResourceIsNonPartitionable_returnsDefaultPartition(servletRequestDetails); + } + + @Test + public void testDetermineReadPartitionForSystemRequest_whenResourceIsNonPartitionable() { + PartitionEntity partitionEntity = createPartition1(); + SystemRequestDetails srd = new SystemRequestDetails(); + srd.setTenantId(partitionEntity.getName()); + srd.setRequestPartitionId(RequestPartitionId.fromPartitionId(partitionEntity.getId())); + + testDetermineReadPartitionRequest_whenResourceIsNonPartitionable_returnsDefaultPartition(srd); + } + + public void testDetermineReadPartitionRequest_whenResourceIsNonPartitionable_returnsDefaultPartition(RequestDetails theDetails) { + // execute + RequestPartitionId result = mySvc.determineReadPartitionForRequestForSearchType(theDetails, "Library"); + + // verify + assertThat(result.hasDefaultPartitionId(DEFAULT_PARTITION_ID)).isTrue(); + } + @Test public void testDetermineCreatePartitionForRequest_withPartitionIdOnly_returnsCorrectPartition() { // setup diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java index b7255fee2982..7dfa9d4f52ac 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java @@ -18,7 +18,6 @@ import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.hapi.rest.server.helper.BatchHelperR4; import org.hl7.fhir.r4.model.Bundle; -import org.hl7.fhir.r4.model.IdType; import org.hl7.fhir.r4.model.Observation; import org.hl7.fhir.r4.model.Parameters; import org.hl7.fhir.r4.model.StringType; diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/partition/BaseRequestPartitionHelperSvc.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/partition/BaseRequestPartitionHelperSvc.java index 9f7b6f895bc6..f0315008a9ee 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/partition/BaseRequestPartitionHelperSvc.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/partition/BaseRequestPartitionHelperSvc.java @@ -120,20 +120,19 @@ public RequestPartitionId determineReadPartitionForRequest( boolean nonPartitionableResource = isResourceNonPartitionable(resourceType); RequestPartitionId requestPartitionId = null; - // Handle system requests - if (requestDetails instanceof SystemRequestDetails - && systemRequestHasExplicitPartition((SystemRequestDetails) requestDetails) - && !nonPartitionableResource) { - requestPartitionId = getSystemRequestPartitionId((SystemRequestDetails) requestDetails, false); - logSystemRequestDetailsResolution((SystemRequestDetails) requestDetails); - - } else if ((requestDetails instanceof SystemRequestDetails) && nonPartitionableResource) { + + // FIXME: EHP -> this block is the 'good' block but test for it are failing. see the next fixme. + if (nonPartitionableResource) { requestPartitionId = myPartitionSettings.getDefaultRequestPartitionId(); - logSystemRequestDetailsResolution((SystemRequestDetails) requestDetails); + logRequestDetailsResolution(requestDetails); logNonPartitionableType(resourceType); + } else if (requestDetails instanceof SystemRequestDetails systemRequestDetails + && systemRequestHasExplicitPartition(systemRequestDetails)) { + // !nonPartitionableResource + requestPartitionId = getSystemRequestPartitionId(systemRequestDetails, false); + logRequestDetailsResolution(systemRequestDetails); + } else { - // TODO mb: why is this path different than create? - // Here, a non-partitionable resource is still delivered to the pointcuts. IInterceptorBroadcaster compositeBroadcaster = CompositeInterceptorBroadcaster.newCompositeBroadcaster(myInterceptorBroadcaster, requestDetails); if (compositeBroadcaster.hasHooks(Pointcut.STORAGE_PARTITION_IDENTIFY_ANY)) { @@ -143,6 +142,29 @@ && systemRequestHasExplicitPartition((SystemRequestDetails) requestDetails) } } + // FIXME: EHP -> the next block from the if to the end of the else should be removed. it is there only to be added and + // commented out in troubleshooting efforts +// if (requestDetails instanceof SystemRequestDetails +// && systemRequestHasExplicitPartition((SystemRequestDetails) requestDetails) +// && !nonPartitionableResource) { +// requestPartitionId = getSystemRequestPartitionId((SystemRequestDetails) requestDetails, false); +// logRequestDetailsResolution((SystemRequestDetails) requestDetails); +// +// } else if ((requestDetails instanceof SystemRequestDetails) && nonPartitionableResource) { +// requestPartitionId = myPartitionSettings.getDefaultRequestPartitionId(); +// logRequestDetailsResolution((SystemRequestDetails) requestDetails); +// logNonPartitionableType(resourceType); +// } else { +// // TODO mb: why is this path different than create? +// // Here, a non-partitionable resource is still delivered to the pointcuts. +// IInterceptorBroadcaster compositeBroadcaster = +// CompositeInterceptorBroadcaster.newCompositeBroadcaster(myInterceptorBroadcaster, requestDetails); +// if (compositeBroadcaster.hasHooks(Pointcut.STORAGE_PARTITION_IDENTIFY_ANY)) { +// requestPartitionId = callAnyPointcut(compositeBroadcaster, requestDetails); +// } else if (compositeBroadcaster.hasHooks(Pointcut.STORAGE_PARTITION_IDENTIFY_READ)) { +// requestPartitionId = callReadPointcut(compositeBroadcaster, requestDetails, theDetails); +// } +// } validateRequestPartitionNotNull( requestPartitionId, Pointcut.STORAGE_PARTITION_IDENTIFY_ANY, Pointcut.STORAGE_PARTITION_IDENTIFY_READ); @@ -213,10 +235,10 @@ public RequestPartitionId determineGenericPartitionForRequest(RequestDetails the return RequestPartitionId.allPartitions(); } - if (theRequestDetails instanceof SystemRequestDetails - && systemRequestHasExplicitPartition((SystemRequestDetails) theRequestDetails)) { - requestPartitionId = getSystemRequestPartitionId((SystemRequestDetails) theRequestDetails); - logSystemRequestDetailsResolution((SystemRequestDetails) theRequestDetails); + if (theRequestDetails instanceof SystemRequestDetails systemRequestDetails + && systemRequestHasExplicitPartition(systemRequestDetails)) { + requestPartitionId = getSystemRequestPartitionId(systemRequestDetails); + logRequestDetailsResolution(systemRequestDetails); } else { IInterceptorBroadcaster compositeBroadcaster = CompositeInterceptorBroadcaster.newCompositeBroadcaster( myInterceptorBroadcaster, theRequestDetails); @@ -303,12 +325,12 @@ public RequestPartitionId determineCreatePartitionForRequest( } RequestPartitionId requestPartitionId = null; - if (theRequest instanceof SystemRequestDetails - && systemRequestHasExplicitPartition((SystemRequestDetails) theRequest)) { - requestPartitionId = - getSystemRequestPartitionId((SystemRequestDetails) theRequest, nonPartitionableResource); - logSystemRequestDetailsResolution((SystemRequestDetails) theRequest); + if (theRequest instanceof SystemRequestDetails systemRequestDetails + && systemRequestHasExplicitPartition(systemRequestDetails)) { + requestPartitionId = getSystemRequestPartitionId(systemRequestDetails, nonPartitionableResource); + + logRequestDetailsResolution(systemRequestDetails); } else { IInterceptorBroadcaster compositeBroadcaster = CompositeInterceptorBroadcaster.newCompositeBroadcaster(myInterceptorBroadcaster, requestDetails); @@ -492,10 +514,19 @@ private static void logTroubleshootingResult( theResult); } - private void logSystemRequestDetailsResolution(SystemRequestDetails theRequest) { - ourLog.trace( - "Partitioning: request is a SystemRequestDetails, with RequestPartitionId={}.", - theRequest.getRequestPartitionId()); + private void logRequestDetailsResolution(RequestDetails theRequest) { + String logString = "Partitioning: request is a {}, with tenantId {}"; + + String requestClassName = theRequest.getClass().getSimpleName(); + String tenantId = theRequest.getTenantId(); + RequestPartitionId requestPartitionId = null; + + if (theRequest instanceof SystemRequestDetails systemRequestDetails) { + requestPartitionId = systemRequestDetails.getRequestPartitionId(); + logString = logString + ", with request partitionId {}"; + } + + ourLog.trace(logString, requestClassName, tenantId, requestPartitionId); } private static void logSubstitutingDefaultSystemRequestDetails() {