From 148cbf119b3a90d129850ff19fc39ae3fdddc6c0 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Sat, 4 Feb 2023 18:40:39 -0500 Subject: [PATCH] Clean up references in IPS documents (#4509) * Clean up references in IPS documents * Add changelog --- .../changelog/6_6_0/4509-fix-refs-in-ips.yaml | 6 +++ .../ips/generator/IpsGeneratorSvcImpl.java | 39 +++++++++------ .../jpa/ips/generator/IpsGenerationTest.java | 30 +++++++++-- .../dao/r5/FhirResourceDaoR5ValueSetTest.java | 50 +++++++++++++++++++ .../ResponseHighlighterInterceptor.java | 2 +- 5 files changed, 108 insertions(+), 19 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4509-fix-refs-in-ips.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4509-fix-refs-in-ips.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4509-fix-refs-in-ips.yaml new file mode 100644 index 00000000000..cb5a2892fa8 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4509-fix-refs-in-ips.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 4509 +title: "References to the patient/subject in IPS documents generated using the $summary + operation were not replaced with the bundle-local UUID assigned to the patient. Also, + some dangling references were left in the generated bundle. This has been corrected." diff --git a/hapi-fhir-jpaserver-ips/src/main/java/ca/uhn/fhir/jpa/ips/generator/IpsGeneratorSvcImpl.java b/hapi-fhir-jpaserver-ips/src/main/java/ca/uhn/fhir/jpa/ips/generator/IpsGeneratorSvcImpl.java index b3d51b4b023..62d573b55ea 100644 --- a/hapi-fhir-jpaserver-ips/src/main/java/ca/uhn/fhir/jpa/ips/generator/IpsGeneratorSvcImpl.java +++ b/hapi-fhir-jpaserver-ips/src/main/java/ca/uhn/fhir/jpa/ips/generator/IpsGeneratorSvcImpl.java @@ -114,16 +114,18 @@ public class IpsGeneratorSvcImpl implements IIpsGeneratorSvc { } private IBaseBundle generateIpsForPatient(RequestDetails theRequestDetails, IBaseResource thePatient) { - IIdType originalSubjectId = myFhirContext.getVersion().newIdType().setValue(thePatient.getIdElement().getValue()); + IIdType originalSubjectId = myFhirContext.getVersion().newIdType().setValue(thePatient.getIdElement().getValue()).toUnqualifiedVersionless(); massageResourceId(null, thePatient); IpsContext context = new IpsContext(thePatient, originalSubjectId); + ResourceInclusionCollection globalResourcesToInclude = new ResourceInclusionCollection(); + globalResourcesToInclude.addResourceIfNotAlreadyPresent(thePatient, originalSubjectId.getValue()); + IBaseResource author = myGenerationStrategy.createAuthor(); massageResourceId(context, author); CompositionBuilder compositionBuilder = createComposition(thePatient, context, author); - - ResourceInclusionCollection globalResourcesToInclude = determineInclusions(theRequestDetails, originalSubjectId, context, compositionBuilder); + determineInclusions(theRequestDetails, originalSubjectId, context, compositionBuilder, globalResourcesToInclude); IBaseResource composition = compositionBuilder.getComposition(); @@ -131,10 +133,10 @@ public class IpsGeneratorSvcImpl implements IIpsGeneratorSvc { CustomThymeleafNarrativeGenerator generator = newNarrativeGenerator(globalResourcesToInclude); generator.populateResourceNarrative(myFhirContext, composition); - return createCompositionDocument(thePatient, author, composition, globalResourcesToInclude); + return createCompositionDocument(author, composition, globalResourcesToInclude); } - private IBaseBundle createCompositionDocument(IBaseResource thePatient, IBaseResource author, IBaseResource composition, ResourceInclusionCollection theResourcesToInclude) { + private IBaseBundle createCompositionDocument(IBaseResource author, IBaseResource composition, ResourceInclusionCollection theResourcesToInclude) { BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext); bundleBuilder.setType(Bundle.BundleType.DOCUMENT.toCode()); bundleBuilder.setIdentifier("urn:ietf:rfc:4122", UUID.randomUUID().toString()); @@ -143,9 +145,6 @@ public class IpsGeneratorSvcImpl implements IIpsGeneratorSvc { // Add composition to document bundleBuilder.addDocumentEntry(composition); - // Add subject to document - bundleBuilder.addDocumentEntry(thePatient); - // Add inclusion candidates for (IBaseResource next : theResourcesToInclude.getResources()) { bundleBuilder.addDocumentEntry(next); @@ -158,13 +157,12 @@ public class IpsGeneratorSvcImpl implements IIpsGeneratorSvc { } @Nonnull - private ResourceInclusionCollection determineInclusions(RequestDetails theRequestDetails, IIdType originalSubjectId, IpsContext context, CompositionBuilder theCompositionBuilder) { - ResourceInclusionCollection globalResourcesToInclude = new ResourceInclusionCollection(); + private ResourceInclusionCollection determineInclusions(RequestDetails theRequestDetails, IIdType originalSubjectId, IpsContext context, CompositionBuilder theCompositionBuilder, ResourceInclusionCollection theGlobalResourcesToInclude) { SectionRegistry sectionRegistry = myGenerationStrategy.getSectionRegistry(); for (SectionRegistry.Section nextSection : sectionRegistry.getSections()) { - determineInclusionsForSection(theRequestDetails, originalSubjectId, context, theCompositionBuilder, globalResourcesToInclude, nextSection); + determineInclusionsForSection(theRequestDetails, originalSubjectId, context, theCompositionBuilder, theGlobalResourcesToInclude, nextSection); } - return globalResourcesToInclude; + return theGlobalResourcesToInclude; } private void determineInclusionsForSection(RequestDetails theRequestDetails, IIdType theOriginalSubjectId, IpsContext theIpsContext, CompositionBuilder theCompositionBuilder, ResourceInclusionCollection theGlobalResourcesToInclude, SectionRegistry.Section theSection) { @@ -241,8 +239,15 @@ public class IpsGeneratorSvcImpl implements IIpsGeneratorSvc { if (isNotBlank(existingReference)) { existingReference = new IdType(existingReference).toUnqualifiedVersionless().getValue(); String replacement = theGlobalResourcesToInclude.getIdSubstitution(existingReference); - if (isNotBlank(replacement) && !replacement.equals(existingReference)) { - nextReference.getResourceReference().setReference(replacement); + if (isNotBlank(replacement)) { + if (!replacement.equals(existingReference)) { + nextReference.getResourceReference().setReference(replacement); + } + } else if (theGlobalResourcesToInclude.getResourceById(existingReference) == null) { + // If this reference doesn't point to something we have actually + // included in the bundle, clear the reference. + nextReference.getResourceReference().setReference(null); + nextReference.getResourceReference().setResource(null); } } } @@ -541,7 +546,11 @@ public class IpsGeneratorSvcImpl implements IIpsGeneratorSvc { } public IBaseResource getResourceById(IIdType theReference) { - return myIdToResource.get(theReference.toUnqualifiedVersionless().getValue()); + return getResourceById(theReference.toUnqualifiedVersionless().getValue()); + } + + public IBaseResource getResourceById(String theReference) { + return myIdToResource.get(theReference); } @Nullable diff --git a/hapi-fhir-jpaserver-ips/src/test/java/ca/uhn/fhir/jpa/ips/generator/IpsGenerationTest.java b/hapi-fhir-jpaserver-ips/src/test/java/ca/uhn/fhir/jpa/ips/generator/IpsGenerationTest.java index 5de2d5517fd..eb140fa1881 100644 --- a/hapi-fhir-jpaserver-ips/src/test/java/ca/uhn/fhir/jpa/ips/generator/IpsGenerationTest.java +++ b/hapi-fhir-jpaserver-ips/src/test/java/ca/uhn/fhir/jpa/ips/generator/IpsGenerationTest.java @@ -1,5 +1,6 @@ package ca.uhn.fhir.jpa.ips.generator; +import ca.uhn.fhir.batch2.jobs.models.BatchResourceId; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.api.dao.DaoRegistry; import ca.uhn.fhir.jpa.ips.api.IIpsGenerationStrategy; @@ -8,8 +9,11 @@ import ca.uhn.fhir.jpa.ips.strategy.DefaultIpsGenerationStrategy; import ca.uhn.fhir.jpa.model.util.JpaConstants; import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test; import ca.uhn.fhir.util.ClasspathUtil; +import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.r4.model.Bundle; +import org.hl7.fhir.r4.model.MedicationStatement; import org.hl7.fhir.r4.model.Parameters; +import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -18,7 +22,10 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.test.context.ContextConfiguration; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.matchesPattern; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; @ContextConfiguration(classes = {IpsGenerationTest.IpsConfig.class}) public class IpsGenerationTest extends BaseResourceProviderR4Test { @@ -28,12 +35,12 @@ public class IpsGenerationTest extends BaseResourceProviderR4Test { @BeforeEach public void beforeEach() { - myServer.withServer(t->t.registerProvider(myIpsOperationProvider)); + myServer.withServer(t -> t.registerProvider(myIpsOperationProvider)); } @AfterEach public void afterEach() { - myServer.withServer(t->t.unregisterProvider(myIpsOperationProvider)); + myServer.withServer(t -> t.unregisterProvider(myIpsOperationProvider)); } @@ -55,10 +62,27 @@ public class IpsGenerationTest extends BaseResourceProviderR4Test { .withNoParameters(Parameters.class) .returnResourceType(Bundle.class) .execute(); - ourLog.info("Output: {}", myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(output)); + // Verify + assertEquals(37, output.getEntry().size()); + String patientId = findFirstEntryResource(output, Patient.class).getId(); + assertThat(patientId, matchesPattern("urn:uuid:.*")); + MedicationStatement medicationStatement = findFirstEntryResource(output, MedicationStatement.class); + assertEquals(patientId, medicationStatement.getSubject().getReference()); + assertNull(medicationStatement.getInformationSource().getReference()); + } + + @SuppressWarnings("unchecked") + private T findFirstEntryResource(Bundle theBundle, Class theType) { + return (T) theBundle + .getEntry() + .stream() + .filter(t -> theType.isAssignableFrom(t.getResource().getClass())) + .findFirst() + .orElseThrow() + .getResource(); } diff --git a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirResourceDaoR5ValueSetTest.java b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirResourceDaoR5ValueSetTest.java index 9fb6b1d0a0a..77d4b524afb 100644 --- a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirResourceDaoR5ValueSetTest.java +++ b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirResourceDaoR5ValueSetTest.java @@ -7,11 +7,14 @@ import ca.uhn.fhir.context.support.ValidationSupportContext; import ca.uhn.fhir.context.support.ValueSetExpansionOptions; import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.jpa.api.config.DaoConfig; +import ca.uhn.fhir.jpa.api.model.ExpungeOptions; +import ca.uhn.fhir.jpa.model.entity.ResourceTable; import ca.uhn.fhir.jpa.term.api.ITermDeferredStorageSvc; import ca.uhn.fhir.jpa.term.custom.CustomTerminologySet; import ca.uhn.fhir.jpa.util.ValueSetTestUtil; import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; import org.hl7.fhir.common.hapi.validation.support.CachingValidationSupport; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.instance.model.api.IPrimitiveType; @@ -19,6 +22,7 @@ import org.hl7.fhir.r5.model.CodeSystem; import org.hl7.fhir.r5.model.CodeType; import org.hl7.fhir.r5.model.CodeableConcept; import org.hl7.fhir.r5.model.Coding; +import org.hl7.fhir.r5.model.Enumerations; import org.hl7.fhir.r5.model.IdType; import org.hl7.fhir.r5.model.StringType; import org.hl7.fhir.r5.model.UriType; @@ -30,6 +34,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.transaction.annotation.Transactional; import java.io.IOException; +import java.util.Optional; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; @@ -38,6 +43,7 @@ import static org.hamcrest.Matchers.stringContainsInOrder; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -56,6 +62,7 @@ public class FhirResourceDaoR5ValueSetTest extends BaseJpaR5Test { public void after() { myDaoConfig.setPreExpandValueSets(new DaoConfig().isPreExpandValueSets()); myDaoConfig.setMaximumExpansionSize(new DaoConfig().getMaximumExpansionSize()); + myDaoConfig.setExpungeEnabled(new DaoConfig().isExpungeEnabled()); } @@ -248,6 +255,49 @@ public class FhirResourceDaoR5ValueSetTest extends BaseJpaR5Test { } + + /** + * See #4305 + */ + @Test + public void testDeleteExpungePreExpandedValueSet() { + myDaoConfig.setExpungeEnabled(true); + + // Create valueset + ValueSet vs = myValidationSupport.fetchResource(ValueSet.class, "http://hl7.org/fhir/ValueSet/address-use"); + assertNotNull(vs); + IIdType id = myValueSetDao.create(vs).getId().toUnqualifiedVersionless(); + + // Update valueset + vs.setName("Hello"); + assertEquals("2", myValueSetDao.update(vs, mySrd).getId().getVersionIdPart()); + runInTransaction(()->{ + Optional resource = myResourceTableDao.findById(id.getIdPartAsLong()); + assertTrue(resource.isPresent()); + }); + + // Precalculate + myTerminologyDeferredStorageSvc.saveAllDeferred(); + myTermSvc.preExpandDeferredValueSetsToTerminologyTables(); + logAllValueSets(); + + // Delete + myValueSetDao.delete(id, mySrd); + + // Verify it's deleted + assertThrows(ResourceGoneException.class, ()-> myValueSetDao.read(id, mySrd)); + + // Expunge + myValueSetDao.expunge(id, new ExpungeOptions().setExpungeDeletedResources(true).setExpungeOldVersions(true), mySrd); + + // Verify expunged + runInTransaction(()->{ + Optional resource = myResourceTableDao.findById(id.getIdPartAsLong()); + assertFalse(resource.isPresent()); + }); + } + + @Test public void testExpandByValueSet_ExceedsMaxSize() { // Add a bunch of codes diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java index ea10c3f73e8..1fcd7d7858d 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java @@ -814,7 +814,7 @@ public class ResponseHighlighterInterceptor { writeLength(theServletResponse, outputBuffer.length()); theServletResponse.getWriter().append(" total including HTML)"); - theServletResponse.getWriter().append(" in estimated "); + theServletResponse.getWriter().append(" in approximately "); theServletResponse.getWriter().append(writeSw.toString()); theServletResponse.getWriter().append("");