From 143474225e4b801d8c353512d727daf0470a977c Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 18 May 2021 09:58:58 -0400 Subject: [PATCH 1/5] Add test. Add fix --- .../jpa/dao/expunge/DeleteExpungeService.java | 17 +++++++-- .../fhir/jpa/dao/expunge/PartitionRunner.java | 2 + .../dao/expunge/DeleteExpungeServiceTest.java | 37 +++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java index bf780638986..064318421fd 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java @@ -29,6 +29,7 @@ import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome; import ca.uhn.fhir.jpa.dao.BaseHapiFhirResourceDao; import ca.uhn.fhir.jpa.dao.data.IResourceLinkDao; import ca.uhn.fhir.jpa.dao.data.IResourceTableDao; +import ca.uhn.fhir.jpa.dao.index.IdHelperService; import ca.uhn.fhir.jpa.model.entity.ResourceLink; import ca.uhn.fhir.jpa.util.JpaInterceptorBroadcaster; import ca.uhn.fhir.rest.api.server.RequestDetails; @@ -43,6 +44,8 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.domain.Slice; import org.springframework.stereotype.Service; import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.TransactionTemplate; import javax.persistence.EntityManager; @@ -74,6 +77,8 @@ public class DeleteExpungeService { private IInterceptorBroadcaster myInterceptorBroadcaster; @Autowired private DaoConfig myDaoConfig; + @Autowired + private IdHelperService myIdHelper; public DeleteMethodOutcome expungeByResourcePids(String theUrl, String theResourceName, Slice thePids, RequestDetails theRequest) { StopWatch w = new StopWatch(); @@ -132,13 +137,19 @@ public class DeleteExpungeService { } ResourceLink firstConflict = conflictResourceLinks.get(0); - String sourceResourceId = firstConflict.getSourceResource().getIdDt().toVersionless().getValue(); - String targetResourceId = firstConflict.getTargetResource().getIdDt().toVersionless().getValue(); + + //NB-GGG: We previously instantiated these ID values from firstConflict.getSourceResource().getIdDt(), but in a situation where we + //actually had to run delete conflict checks in multiple partitions, the executor service starts its own sessions on a per thread basis, and by the time + //we arrive here, those sessions are closed. So instead, we resolve them from PIDs, which are eagerly loaded. + String sourceResourceId = myIdHelper.resourceIdFromPidOrThrowException(firstConflict.getSourceResourcePid()).toVersionless().getValue(); + String targetResourceId = myIdHelper.resourceIdFromPidOrThrowException(firstConflict.getSourceResourcePid()).toVersionless().getValue(); + throw new InvalidRequestException("DELETE with _expunge=true failed. Unable to delete " + targetResourceId + " because " + sourceResourceId + " refers to it via the path " + firstConflict.getSourcePath()); } - private void findResourceLinksWithTargetPidIn(List theAllTargetPids, List theSomeTargetPids, List theConflictResourceLinks) { + @Transactional(propagation = Propagation.NESTED) + public void findResourceLinksWithTargetPidIn(List theAllTargetPids, List theSomeTargetPids, List theConflictResourceLinks) { // We only need to find one conflict, so if we found one already in an earlier partition run, we can skip the rest of the searches if (theConflictResourceLinks.isEmpty()) { List conflictResourceLinks = myResourceLinkDao.findWithTargetPidIn(theSomeTargetPids).stream() diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/PartitionRunner.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/PartitionRunner.java index 9432d052e7d..ecd0058010f 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/PartitionRunner.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/PartitionRunner.java @@ -30,6 +30,8 @@ import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.domain.Slice; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; import java.util.ArrayList; import java.util.List; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeServiceTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeServiceTest.java index 45503106d66..f619f49b2c6 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeServiceTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeServiceTest.java @@ -4,10 +4,15 @@ import ca.uhn.fhir.jpa.api.config.DaoConfig; import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome; import ca.uhn.fhir.jpa.dao.r4.BaseJpaR4Test; import ca.uhn.fhir.jpa.model.util.JpaConstants; +import ca.uhn.fhir.jpa.partition.SystemRequestDetails; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.util.BundleBuilder; import org.hl7.fhir.instance.model.api.IIdType; +import org.hl7.fhir.r4.model.Bundle; +import org.hl7.fhir.r4.model.Claim; +import org.hl7.fhir.r4.model.Encounter; import org.hl7.fhir.r4.model.Organization; import org.hl7.fhir.r4.model.Patient; import org.hl7.fhir.r4.model.Reference; @@ -18,7 +23,9 @@ import org.springframework.beans.factory.annotation.Autowired; import java.util.List; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.fail; import static org.hamcrest.MatcherAssert.assertThat; @@ -65,6 +72,36 @@ class DeleteExpungeServiceTest extends BaseJpaR4Test { assertEquals(e.getMessage(), "DELETE with _expunge=true failed. Unable to delete " + organizationId.toVersionless() + " because " + patientId.toVersionless() + " refers to it via the path Patient.managingOrganization"); } } + + + @Test + public void testDeleteWithExpungeFailsIfConflictsAreGeneratedByMultiplePartitions() { + //See https://github.com/hapifhir/hapi-fhir/issues/2661 + + //Given + BundleBuilder builder = new BundleBuilder(myFhirCtx); + for (int i = 0; i < 20; i++) { + Organization o = new Organization(); + o.setId("Organization/O-" + i); + Patient p = new Patient(); + p.setId("Patient/P-" + i); + p.setManagingOrganization(new Reference(o.getId())); + builder.addTransactionUpdateEntry(o); + builder.addTransactionUpdateEntry(p); + } + mySystemDao.transaction(new SystemRequestDetails(), (Bundle) builder.getBundle()); + + //When + myDaoConfig.setExpungeBatchSize(10); + try { + myOrganizationDao.deleteByUrl("Organization?" + JpaConstants.PARAM_DELETE_EXPUNGE + "=true", mySrd); + fail(); + } catch (InvalidRequestException e) { + //Then + assertThat(e.getMessage(), is(containsString("DELETE with _expunge=true failed. Unable to delete "))); + } + } + @Test public void testDeleteExpungeRespectsSynchronousSize() { //Given From 22a1a05cfbeaccaffb0b44c060a27fd1e64070fa Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 18 May 2021 10:05:59 -0400 Subject: [PATCH 2/5] Add changelog --- .../2661-lazyload-fail-during-delete-with-expunge.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2661-lazyload-fail-during-delete-with-expunge.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2661-lazyload-fail-during-delete-with-expunge.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2661-lazyload-fail-during-delete-with-expunge.yaml new file mode 100644 index 00000000000..394bd935d8f --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2661-lazyload-fail-during-delete-with-expunge.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 2661 +title: "Fixed a bug where delete with expunge would throw a LazyInitializationException if there were delete conflicts, and + the expunge batch size was smaller than the available list of resources to delete." + From 5b6f76e2dcc108585c38f17da8a9968e44467c0d Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 18 May 2021 11:05:56 -0400 Subject: [PATCH 3/5] Address review comments --- .../ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java index 064318421fd..f647627a20a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java @@ -28,7 +28,6 @@ import ca.uhn.fhir.jpa.api.config.DaoConfig; import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome; import ca.uhn.fhir.jpa.dao.BaseHapiFhirResourceDao; import ca.uhn.fhir.jpa.dao.data.IResourceLinkDao; -import ca.uhn.fhir.jpa.dao.data.IResourceTableDao; import ca.uhn.fhir.jpa.dao.index.IdHelperService; import ca.uhn.fhir.jpa.model.entity.ResourceLink; import ca.uhn.fhir.jpa.util.JpaInterceptorBroadcaster; @@ -44,8 +43,6 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.domain.Slice; import org.springframework.stereotype.Service; import org.springframework.transaction.PlatformTransactionManager; -import org.springframework.transaction.annotation.Propagation; -import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.TransactionTemplate; import javax.persistence.EntityManager; @@ -148,7 +145,6 @@ public class DeleteExpungeService { targetResourceId + " because " + sourceResourceId + " refers to it via the path " + firstConflict.getSourcePath()); } - @Transactional(propagation = Propagation.NESTED) public void findResourceLinksWithTargetPidIn(List theAllTargetPids, List theSomeTargetPids, List theConflictResourceLinks) { // We only need to find one conflict, so if we found one already in an earlier partition run, we can skip the rest of the searches if (theConflictResourceLinks.isEmpty()) { From 9d039e095b6718a53b9194152647fe530e51ae7f Mon Sep 17 00:00:00 2001 From: Tadgh Date: Tue, 18 May 2021 11:12:59 -0400 Subject: [PATCH 4/5] Fix copy paste error. shame. --- .../java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java index f647627a20a..31995f6fd81 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/DeleteExpungeService.java @@ -139,7 +139,7 @@ public class DeleteExpungeService { //actually had to run delete conflict checks in multiple partitions, the executor service starts its own sessions on a per thread basis, and by the time //we arrive here, those sessions are closed. So instead, we resolve them from PIDs, which are eagerly loaded. String sourceResourceId = myIdHelper.resourceIdFromPidOrThrowException(firstConflict.getSourceResourcePid()).toVersionless().getValue(); - String targetResourceId = myIdHelper.resourceIdFromPidOrThrowException(firstConflict.getSourceResourcePid()).toVersionless().getValue(); + String targetResourceId = myIdHelper.resourceIdFromPidOrThrowException(firstConflict.getTargetResourcePid()).toVersionless().getValue(); throw new InvalidRequestException("DELETE with _expunge=true failed. Unable to delete " + targetResourceId + " because " + sourceResourceId + " refers to it via the path " + firstConflict.getSourcePath()); From 6d06fd8b1905b09af22b202e4170056c0c276585 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Tue, 18 May 2021 12:42:42 -0400 Subject: [PATCH 5/5] fix cql synchronous search param --- .../uhn/fhir/cql/common/retrieve/JpaFhirRetrieveProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-cql/src/main/java/ca/uhn/fhir/cql/common/retrieve/JpaFhirRetrieveProvider.java b/hapi-fhir-jpaserver-cql/src/main/java/ca/uhn/fhir/cql/common/retrieve/JpaFhirRetrieveProvider.java index 0cd17acdaa7..26edb1d39cb 100644 --- a/hapi-fhir-jpaserver-cql/src/main/java/ca/uhn/fhir/cql/common/retrieve/JpaFhirRetrieveProvider.java +++ b/hapi-fhir-jpaserver-cql/src/main/java/ca/uhn/fhir/cql/common/retrieve/JpaFhirRetrieveProvider.java @@ -72,7 +72,7 @@ public class JpaFhirRetrieveProvider extends SearchParamFhirRetrieveProvider { protected Collection executeQuery(String dataType, SearchParameterMap map) { // TODO: Once HAPI breaks this out from the server dependencies // we can include it on its own. - ca.uhn.fhir.jpa.searchparam.SearchParameterMap hapiMap = new ca.uhn.fhir.jpa.searchparam.SearchParameterMap(); + ca.uhn.fhir.jpa.searchparam.SearchParameterMap hapiMap = ca.uhn.fhir.jpa.searchparam.SearchParameterMap.newSynchronous(); try { Method[] methods = hapiMap.getClass().getDeclaredMethods();