Fix #2022 - Invalidate caches on expunge (#2029)

* Fix #2022 - Invalidate caches on expunge

* Rename changelog file

hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/FhirServerConfig.java

@@ -33,6 +33,7 @@ import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
+import org.springframework.context.annotation.Primary;
 import org.springframework.orm.jpa.JpaTransactionManager;
 import org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean;
 import org.springframework.transaction.annotation.EnableTransactionManagement;
@@ -106,8 +107,9 @@ public class FhirServerConfig extends BaseJavaConfigDstu2 {
 		return retVal;
 	}
 
+	@Primary
 	@Bean
-	public JpaTransactionManager transactionManager(EntityManagerFactory entityManagerFactory) {
+	public JpaTransactionManager hapiTransactionManager(EntityManagerFactory entityManagerFactory) {
 		JpaTransactionManager retVal = new JpaTransactionManager();
 		retVal.setEntityManagerFactory(entityManagerFactory);
 		return retVal;

hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/FhirServerConfigDstu3.java

@@ -20,6 +20,8 @@ package ca.uhn.fhir.jpa.demo;
  * #L%
  */
 
+import ca.uhn.fhir.jpa.binstore.DatabaseBlobBinaryStorageSvcImpl;
+import ca.uhn.fhir.jpa.binstore.IBinaryStorageSvc;
 import ca.uhn.fhir.jpa.config.BaseJavaConfigDstu3;
 import ca.uhn.fhir.jpa.util.SubscriptionsRequireManualActivationInterceptorDstu3;
 import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor;
@@ -31,6 +33,7 @@ import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
+import org.springframework.context.annotation.Primary;
 import org.springframework.orm.jpa.JpaTransactionManager;
 import org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean;
 import org.springframework.transaction.annotation.EnableTransactionManagement;
@@ -97,10 +100,12 @@ public class FhirServerConfigDstu3 extends BaseJavaConfigDstu3 {
 		return retVal;
 	}
 
+	@Primary
 	@Bean
-	public JpaTransactionManager transactionManager(EntityManagerFactory entityManagerFactory) {
+	public JpaTransactionManager hapiTransactionManager(EntityManagerFactory entityManagerFactory) {
 		JpaTransactionManager retVal = new JpaTransactionManager();
 		retVal.setEntityManagerFactory(entityManagerFactory);
 		return retVal;
 	}
+
 }

hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/JpaServerDemo.java

@@ -26,6 +26,8 @@ import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.IFhirSystemDao;
+import ca.uhn.fhir.jpa.binstore.BinaryAccessProvider;
+import ca.uhn.fhir.jpa.binstore.BinaryStorageInterceptor;
 import ca.uhn.fhir.jpa.config.BaseConfig;
 import ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor;
 import ca.uhn.fhir.jpa.provider.JpaConformanceProviderDstu2;
@@ -180,6 +182,8 @@ public class JpaServerDemo extends RestfulServer {
 
 		getInterceptorService().registerInterceptor(new ResponseHighlighterInterceptor());
 
+		registerProvider(myAppCtx.getBean(BinaryAccessProvider.class));
+
 	}
 
 }

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_1_0/2022-invalidate-caches-on-expunge.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 2022
+title: When performing a resource $expunge in the JPA server, in-memory caches caused issues if a
+   forced ID was reused quickly enough (as can be the case in some testing scenarios). Thanks to
+   GitHub user @janvdpol for reporting!"

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java

@@ -46,6 +46,7 @@ import ca.uhn.fhir.jpa.model.entity.ForcedId;
 import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.util.JpaInterceptorBroadcaster;
+import ca.uhn.fhir.jpa.util.MemoryCacheService;
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
@@ -60,7 +61,11 @@ import org.springframework.data.domain.Pageable;
 import org.springframework.data.domain.Slice;
 import org.springframework.data.domain.SliceImpl;
 import org.springframework.stereotype.Service;
+import org.springframework.transaction.TransactionManager;
 import org.springframework.transaction.annotation.Transactional;
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationAdapter;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
 
 import java.util.Collections;
 import java.util.List;
@@ -108,6 +113,8 @@ public class ResourceExpungeService implements IResourceExpungeService {
 	private ISearchParamPresentDao mySearchParamPresentDao;
 	@Autowired
 	private DaoConfig myDaoConfig;
+	@Autowired
+	private MemoryCacheService myMemoryCacheService;
 
 	@Override
 	@Transactional
@@ -158,6 +165,20 @@ public class ResourceExpungeService implements IResourceExpungeService {
 				return;
 			}
 		}
+
+		/*
+		 * Once this transaction is committed, we will invalidate all memory caches
+		 * in order to avoid any caches having references to things that no longer
+		 * exist. This is a pretty brute-force way of addressing this, and could probably
+		 * be optimized, but expunge is hopefully not frequently called on busy servers
+		 * so it shouldn't be too big a deal.
+		 */
+		TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization(){
+			@Override
+			public void afterCommit() {
+				myMemoryCacheService.invalidateAllCaches();
+			}
+		});
 	}
 
 	private void expungeHistoricalVersion(RequestDetails theRequestDetails, Long theNextVersionId, AtomicInteger theRemainingCount) {

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java

@@ -100,6 +100,8 @@ public class IdHelperService {
 	private IInterceptorBroadcaster myInterceptorBroadcaster;
 	@Autowired
 	private FhirContext myFhirCtx;
+	@Autowired
+	private MemoryCacheService myMemoryCacheService;
 
 	public void delete(ForcedId forcedId) {
 		myForcedIdDao.deleteByPid(forcedId.getId());
@@ -123,9 +125,6 @@ public class IdHelperService {
 		return matches.iterator().next();
 	}
 
-	@Autowired
-	private MemoryCacheService myMemoryCacheService;
-
 	/**
 	 * Given a resource type and ID, determines the internal persistent ID for the resource.
 	 *
@@ -389,7 +388,7 @@ public class IdHelperService {
 			lookup
 				.stream()
 				.map(t -> new ResourceLookup((String) t[0], (Long) t[1], (Date) t[2]))
-				.forEach(t->{
+				.forEach(t -> {
 					theTarget.add(t);
 					if (!myDaoConfig.isDeleteEnabled()) {
 						String nextKey = Long.toString(t.getResourceId());
@@ -432,19 +431,6 @@ public class IdHelperService {
 		return retVal;
 	}
 
-	public static boolean isValidPid(IIdType theId) {
-		if (theId == null) {
-			return false;
-		}
-
-		String idPart = theId.getIdPart();
-		return isValidPid(idPart);
-	}
-
-	public static boolean isValidPid(String theIdPart) {
-		return StringUtils.isNumeric(theIdPart);
-	}
-
 	@Nullable
 	public Long getPidOrNull(IBaseResource theResource) {
 		IAnyResource anyResource = (IAnyResource) theResource;
@@ -481,11 +467,24 @@ public class IdHelperService {
 		return theIds.stream().collect(Collectors.toMap(this::getPidOrThrowException, Function.identity()));
 	}
 
-    public IIdType resourceIdFromPidOrThrowException(Long thePid) {
+	public IIdType resourceIdFromPidOrThrowException(Long thePid) {
-		 Optional<ResourceTable> optionalResource = myResourceTableDao.findById(thePid);
+		Optional<ResourceTable> optionalResource = myResourceTableDao.findById(thePid);
-		 if (!optionalResource.isPresent()) {
+		if (!optionalResource.isPresent()) {
-		 	throw new ResourceNotFoundException("Requested resource not found");
+			throw new ResourceNotFoundException("Requested resource not found");
-		 }
+		}
-		 return optionalResource.get().getIdDt().toVersionless();
+		return optionalResource.get().getIdDt().toVersionless();
-    }
+	}
+
+	public static boolean isValidPid(IIdType theId) {
+		if (theId == null) {
+			return false;
+		}
+
+		String idPart = theId.getIdPart();
+		return isValidPid(idPart);
+	}
+
+	public static boolean isValidPid(String theIdPart) {
+		return StringUtils.isNumeric(theIdPart);
+	}
 }

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/BinaryAccessProviderR4Test.java

@@ -413,6 +413,8 @@ public class BinaryAccessProviderR4Test extends BaseResourceProviderR4Test {
 
 	}
 
+
+
 	/**
 	 * Stores a binary large enough that it should live in binary storage
 	 */
@@ -469,6 +471,77 @@ public class BinaryAccessProviderR4Test extends BaseResourceProviderR4Test {
 
 	}
 
+
+	@Test
+	public void testWriteLargeBinaryToDocumentReference() throws IOException {
+		byte[] bytes = new byte[134696];
+		for (int i = 0; i < bytes.length; i++) {
+			bytes[i] = (byte) (((float)Byte.MAX_VALUE) * Math.random());
+		}
+
+		DocumentReference dr = new DocumentReference();
+		dr.addContent().getAttachment()
+			.setContentType("application/pdf")
+			.setSize(12345)
+			.setTitle("hello")
+			.setCreationElement(new DateTimeType("2002"));
+		IIdType id = myClient.create().resource(dr).execute().getId().toUnqualifiedVersionless();
+
+		IAnonymousInterceptor interceptor = mock(IAnonymousInterceptor.class);
+		myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.STORAGE_PRESHOW_RESOURCES, interceptor);
+		myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED, interceptor);
+		myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED, interceptor);
+
+		// Write using the operation
+
+		String path = ourServerBase +
+			"/DocumentReference/" + id.getIdPart() + "/" +
+			JpaConstants.OPERATION_BINARY_ACCESS_WRITE +
+			"?path=DocumentReference.content.attachment";
+		HttpPost post = new HttpPost(path);
+		post.setEntity(new ByteArrayEntity(bytes, ContentType.IMAGE_JPEG));
+		post.addHeader("Accept", "application/fhir+json; _pretty=true");
+		String attachmentId;
+		try (CloseableHttpResponse resp = ourHttpClient.execute(post)) {
+			assertEquals(200, resp.getStatusLine().getStatusCode());
+			assertThat(resp.getEntity().getContentType().getValue(), containsString("application/fhir+json"));
+
+			String response = IOUtils.toString(resp.getEntity().getContent(), Constants.CHARSET_UTF8);
+			ourLog.info("Response: {}", response);
+
+			DocumentReference target = myFhirCtx.newJsonParser().parseResource(DocumentReference.class, response);
+
+			assertEquals(null, target.getContentFirstRep().getAttachment().getData());
+			assertEquals("2", target.getMeta().getVersionId());
+			attachmentId = target.getContentFirstRep().getAttachment().getDataElement().getExtensionString(HapiExtensions.EXT_EXTERNALIZED_BINARY_ID);
+			assertThat(attachmentId, matchesPattern("[a-zA-Z0-9]{100}"));
+
+		}
+
+		verify(interceptor, timeout(5000).times(1)).invoke(eq(Pointcut.STORAGE_PRESHOW_RESOURCES), any());
+		verify(interceptor, timeout(5000).times(1)).invoke(eq(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED), any());
+		verifyNoMoreInteractions(interceptor);
+
+		// Read it back using the operation
+
+		path = ourServerBase +
+			"/DocumentReference/" + id.getIdPart() + "/" +
+			JpaConstants.OPERATION_BINARY_ACCESS_READ +
+			"?path=DocumentReference.content.attachment";
+		HttpGet get = new HttpGet(path);
+		try (CloseableHttpResponse resp = ourHttpClient.execute(get)) {
+
+			assertEquals(200, resp.getStatusLine().getStatusCode());
+			assertEquals("image/jpeg", resp.getEntity().getContentType().getValue());
+			assertEquals(bytes.length, resp.getEntity().getContentLength());
+
+			byte[] actualBytes = IOUtils.toByteArray(resp.getEntity().getContent());
+			assertArrayEquals(bytes, actualBytes);
+		}
+
+	}
+
+
 	private IIdType createDocumentReference(boolean theSetData) {
 		DocumentReference documentReference = new DocumentReference();
 		Attachment attachment = documentReference

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ExpungeR4Test.java

@@ -8,11 +8,12 @@ import ca.uhn.fhir.jpa.dao.data.ISearchResultDao;
 import ca.uhn.fhir.jpa.search.PersistedJpaSearchFirstPageBundleProvider;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
+import ca.uhn.fhir.rest.param.ReferenceParam;
+import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
 import ca.uhn.fhir.util.HapiExtensions;
-import ca.uhn.fhir.util.TestUtil;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.BooleanType;
@@ -23,7 +24,6 @@ import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.Patient;
 import org.hl7.fhir.r4.model.SearchParameter;
 import org.junit.jupiter.api.AfterEach;
-import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.slf4j.Logger;
@@ -36,7 +36,9 @@ import static org.awaitility.Awaitility.await;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.not;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 public class ExpungeR4Test extends BaseResourceProviderR4Test {
 
@@ -410,16 +412,16 @@ public class ExpungeR4Test extends BaseResourceProviderR4Test {
 	public void testExpungeEverythingWhereResourceInSearchResults() {
 		createStandardPatients();
 
-		await().until(()-> runInTransaction(() -> mySearchEntityDao.count() == 0));
+		await().until(() -> runInTransaction(() -> mySearchEntityDao.count() == 0));
-		await().until(()-> runInTransaction(() -> mySearchResultDao.count() == 0));
+		await().until(() -> runInTransaction(() -> mySearchResultDao.count() == 0));
 
 		PersistedJpaSearchFirstPageBundleProvider search = (PersistedJpaSearchFirstPageBundleProvider) myPatientDao.search(new SearchParameterMap());
 		assertEquals(PersistedJpaSearchFirstPageBundleProvider.class, search.getClass());
 		assertEquals(2, search.size().intValue());
 		assertEquals(2, search.getResources(0, 2).size());
 
-		await().until(()-> runInTransaction(() -> mySearchEntityDao.count() == 1));
+		await().until(() -> runInTransaction(() -> mySearchEntityDao.count() == 1));
-		await().until(()-> runInTransaction(() -> mySearchResultDao.count() == 2));
+		await().until(() -> runInTransaction(() -> mySearchResultDao.count() == 2));
 
 		mySystemDao.expunge(new ExpungeOptions()
 			.setExpungeEverything(true), null);
@@ -465,4 +467,87 @@ public class ExpungeR4Test extends BaseResourceProviderR4Test {
 	}
 
 
+	@Test
+	public void testExpungeForcedIdAndThenReuseIt() {
+		// Create with forced ID, and an Observation that links to it
+		Patient p = new Patient();
+		p.setId("TEST");
+		p.setActive(true);
+		p.addName().setFamily("FOO");
+		myPatientDao.update(p);
+
+		Observation obs = new Observation();
+		obs.setId("OBS");
+		obs.getSubject().setReference("Patient/TEST");
+		myObservationDao.update(obs);
+
+		// Make sure read works
+		p = myPatientDao.read(new IdType("Patient/TEST"));
+		assertTrue(p.getActive());
+
+		// Make sure search by ID works
+		IBundleProvider outcome = myPatientDao.search(SearchParameterMap.newSynchronous("_id", new TokenParam("Patient/TEST")));
+		p = (Patient) outcome.getResources(0, 1).get(0);
+		assertTrue(p.getActive());
+
+		// Make sure search by Reference works
+		outcome = myObservationDao.search(SearchParameterMap.newSynchronous(Observation.SP_SUBJECT, new ReferenceParam("Patient/TEST")));
+		obs = (Observation) outcome.getResources(0, 1).get(0);
+		assertEquals("OBS", obs.getIdElement().getIdPart());
+
+		// Delete and expunge
+		myObservationDao.delete(new IdType("Observation/OBS"));
+		myPatientDao.delete(new IdType("Patient/TEST"));
+		myPatientDao.expunge(new ExpungeOptions()
+			.setExpungeDeletedResources(true)
+			.setExpungeOldVersions(true), null);
+		myObservationDao.expunge(new ExpungeOptions()
+			.setExpungeDeletedResources(true)
+			.setExpungeOldVersions(true), null);
+		runInTransaction(() -> assertThat(myResourceTableDao.findAll(), empty()));
+		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), empty()));
+		runInTransaction(() -> assertThat(myForcedIdDao.findAll(), empty()));
+
+		// Create again with the same forced ID
+		p = new Patient();
+		p.setId("TEST");
+		p.setActive(true);
+		p.addName().setFamily("FOO");
+		myPatientDao.update(p);
+
+		obs = new Observation();
+		obs.setId("OBS");
+		obs.getSubject().setReference("Patient/TEST");
+		myObservationDao.update(obs);
+
+		// Make sure read works
+		p = myPatientDao.read(new IdType("Patient/TEST"));
+		assertTrue(p.getActive());
+
+		// Make sure search works
+		outcome = myPatientDao.search(SearchParameterMap.newSynchronous("_id", new TokenParam("Patient/TEST")));
+		p = (Patient) outcome.getResources(0, 1).get(0);
+		assertTrue(p.getActive());
+
+		// Make sure search by Reference works
+		outcome = myObservationDao.search(SearchParameterMap.newSynchronous(Observation.SP_SUBJECT, new ReferenceParam("Patient/TEST")));
+		obs = (Observation) outcome.getResources(0, 1).get(0);
+		assertEquals("OBS", obs.getIdElement().getIdPart());
+
+		// Delete and expunge
+		myObservationDao.delete(new IdType("Observation/OBS"));
+		myPatientDao.delete(new IdType("Patient/TEST"));
+		myPatientDao.expunge(new ExpungeOptions()
+			.setExpungeDeletedResources(true)
+			.setExpungeOldVersions(true), null);
+		myObservationDao.expunge(new ExpungeOptions()
+			.setExpungeDeletedResources(true)
+			.setExpungeOldVersions(true), null);
+		runInTransaction(() -> assertThat(myResourceTableDao.findAll(), empty()));
+		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), empty()));
+		runInTransaction(() -> assertThat(myForcedIdDao.findAll(), empty()));
+
+	}
+
+
 }