Merge 82075fa3ee into 061390d76b

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6478-transaction-bundle-updates-with-multiple-conditional-urls.yaml

@@ -0,0 +1,8 @@
+---
+type: perf
+issue: 6478
+jira: SMILE-8955
+title: "Transactions with multiple saved search urls will have the saved search urls
+        deleted in a batch, instead of 1 at a time.
+        This is a minor performance update.
+"

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

@@ -2351,13 +2351,13 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 			RequestDetails theRequest,
 			TransactionDetails theTransactionDetails,
 			RequestPartitionId theRequestPartitionId) {
-
+		DaoMethodOutcome outcome = null;
 		preProcessResourceForStorage(theResource);
 		preProcessResourceForStorage(theResource, theRequest, theTransactionDetails, thePerformIndexing);
 
 		ResourceTable entity = null;
 
-		IIdType resourceId;
+		IIdType resourceId = null;
 		RestOperationTypeEnum update = RestOperationTypeEnum.UPDATE;
 		if (isNotBlank(theMatchUrl)) {
 			// Validate that the supplied resource matches the conditional.
@@ -2399,7 +2399,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 					theResource.setId(UUID.randomUUID().toString());
 					theResource.setUserData(JpaConstants.RESOURCE_ID_SERVER_ASSIGNED, Boolean.TRUE);
 				}
-				DaoMethodOutcome outcome = doCreateForPostOrPut(
+				outcome = doCreateForPostOrPut(
 						theRequest,
 						theResource,
 						theMatchUrl,
@@ -2414,8 +2414,6 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 					myMatchResourceUrlService.matchUrlResolved(
 							theTransactionDetails, getResourceName(), theMatchUrl, (JpaPid) outcome.getPersistentId());
 				}
-
-				return outcome;
 			}
 		} else {
 			/*
@@ -2445,7 +2443,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 			}
 
 			if (create) {
-				return doCreateForPostOrPut(
+				outcome = doCreateForPostOrPut(
 						theRequest,
 						theResource,
 						null,
@@ -2458,7 +2456,8 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		}
 
 		// Start
-		return doUpdateForUpdateOrPatch(
+		if (outcome == null) {
+			outcome = doUpdateForUpdateOrPatch(
 					theRequest,
 					resourceId,
 					theMatchUrl,
@@ -2470,6 +2469,24 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 					theTransactionDetails);
 		}
 
+		postUpdateTransaction(theTransactionDetails);
+
+		return outcome;
+	}
+
+	@SuppressWarnings("rawtypes")
+	protected void postUpdateTransaction(TransactionDetails theTransactionDetails) {
+		// Transactions will delete these at the end of the entire transaction
+		if (!theTransactionDetails.isFhirTransaction()) {
+			Set<IResourcePersistentId> resourceIds = theTransactionDetails.getUpdatedResourceIds();
+			if (resourceIds != null && !resourceIds.isEmpty()) {
+				List<Long> ids = resourceIds.stream().map(r -> (Long) r.getId()).collect(Collectors.toList());
+
+				myResourceSearchUrlSvc.deleteByResIds(ids);
+			}
+		}
+	}
+
 	@Override
 	protected DaoMethodOutcome doUpdateForUpdateOrPatch(
 			RequestDetails theRequest,
@@ -2489,9 +2506,10 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		 */
 		ResourceTable entity = (ResourceTable) theEntity;
 		if (entity.isSearchUrlPresent()) {
-			myResourceSearchUrlSvc.deleteByResId(
+			JpaPid persistentId = JpaPid.fromId(entity.getResourceId());
-					(Long) theEntity.getPersistentId().getId());
+			theTransactionDetails.addUpdatedResourceId(persistentId);
-			entity.setSearchUrlPresent(false);
+
+			entity.setSearchUrlPresent(false); // it will be removed at the end
 		}
 
 		return super.doUpdateForUpdateOrPatch(

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

@@ -33,10 +33,12 @@ import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
 import ca.uhn.fhir.jpa.model.entity.StorageSettings;
 import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc;
+import ca.uhn.fhir.jpa.search.ResourceSearchUrlSvc;
 import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.model.api.IQueryParameterType;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
+import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId;
 import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.util.FhirTerser;
@@ -114,6 +116,9 @@ public class TransactionProcessor extends BaseTransactionProcessor {
 	@Autowired
 	private MatchUrlService myMatchUrlService;
 
+	@Autowired
+	private ResourceSearchUrlSvc myResourceSearchUrlSvc;
+
 	@Autowired
 	private IRequestPartitionHelperSvc myRequestPartitionSvc;
 
@@ -224,6 +229,16 @@ public class TransactionProcessor extends BaseTransactionProcessor {
 		systemDao.preFetchResources(JpaPid.fromLongList(idsToPreFetch), true);
 	}
 
+	@SuppressWarnings("rawtypes")
+	protected void postTransactionProcess(TransactionDetails theTransactionDetails) {
+		Set<IResourcePersistentId> resourceIds = theTransactionDetails.getUpdatedResourceIds();
+		if (resourceIds != null && !resourceIds.isEmpty()) {
+			List<Long> ids = resourceIds.stream().map(r -> (Long) r.getId()).collect(Collectors.toList());
+
+			myResourceSearchUrlSvc.deleteByResIds(ids);
+		}
+	}
+
 	private void preFetchResourcesById(
 			TransactionDetails theTransactionDetails,
 			List<IBase> theEntries,

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceSearchUrlDao.java

@@ -26,6 +26,7 @@ import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
 
 import java.util.Date;
+import java.util.List;
 
 public interface IResourceSearchUrlDao extends JpaRepository<ResourceSearchUrlEntity, Long>, IHapiFhirJpaRepository {
 
@@ -36,4 +37,8 @@ public interface IResourceSearchUrlDao extends JpaRepository<ResourceSearchUrlEn
 	@Modifying
 	@Query("DELETE FROM ResourceSearchUrlEntity s WHERE (s.myResourcePid = :resID)")
 	int deleteByResId(@Param("resID") long resId);
+
+	@Modifying
+	@Query("DELETE FROM ResourceSearchUrlEntity s WHERE s.myResourcePid IN (:resIDS)")
+	int deleteByResIds(@Param("resIDS") List<Long> theIds);
 }

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/ResourceSearchUrlSvc.java

@@ -34,6 +34,7 @@ import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
 
 import java.util.Date;
+import java.util.List;
 
 /**
  * This service ensures uniqueness of resources during create or create-on-update
@@ -84,6 +85,10 @@ public class ResourceSearchUrlSvc {
 		myResourceSearchUrlDao.deleteByResId(theResId);
 	}
 
+	public void deleteByResIds(List<Long> theResIds) {
+		myResourceSearchUrlDao.deleteByResIds(theResIds);
+	}
+
 	/**
 	 *  We store a record of match urls with res_id so a db constraint can catch simultaneous creates that slip through.
 	 */

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java

@@ -556,7 +556,6 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 		assertEquals(1L, id.getVersionIdPartAsLong());
 
 		// Update 1 - Should delete search URL
-
 		p.setActive(true);
 		myCaptureQueriesListener.clear();
 		id = myPatientDao.update(p, "Patient?identifier=http://foo|123", mySrd).getId();
@@ -564,13 +563,11 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 		assertEquals(2L, id.getVersionIdPartAsLong());
 
 		// Update 2 - Should not try to delete search URL
-
 		p.setActive(false);
 		myCaptureQueriesListener.clear();
 		id = myPatientDao.update(p, "Patient?identifier=http://foo|123", mySrd).getId();
 		assertEquals(0, myCaptureQueriesListener.countDeleteQueries());
 		assertEquals(3L, id.getVersionIdPartAsLong());
-
 	}
 
 	/**
@@ -2500,7 +2497,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 		assertEquals(4, myCaptureQueriesListener.countInsertQueries());
 		myCaptureQueriesListener.logUpdateQueries();
 		assertEquals(8, myCaptureQueriesListener.countUpdateQueries());
-		assertEquals(4, myCaptureQueriesListener.countDeleteQueries());
+		assertEquals(1, myCaptureQueriesListener.countDeleteQueries());
 
 		/*
 		 * Third time with mass ingestion mode enabled
@@ -3699,7 +3696,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 		myCaptureQueriesListener.logInsertQueries();
 		assertEquals(4, myCaptureQueriesListener.countInsertQueriesForCurrentThread());
 		assertEquals(7, myCaptureQueriesListener.countUpdateQueriesForCurrentThread());
-		assertEquals(2, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
+		assertEquals(1, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
 
 		ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome));
 		IdType patientId = new IdType(outcome.getEntry().get(1).getResponse().getLocation());
@@ -3782,7 +3779,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 		assertEquals(6, myCaptureQueriesListener.countSelectQueriesForCurrentThread());
 		assertEquals(2, myCaptureQueriesListener.countInsertQueriesForCurrentThread());
 		assertEquals(6, myCaptureQueriesListener.countUpdateQueriesForCurrentThread());
-		assertEquals(2, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
+		assertEquals(1, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
 
 	}
 

hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirSystemDaoTransactionR5Test.java

@@ -162,7 +162,7 @@ public class FhirSystemDaoTransactionR5Test extends BaseJpaR5Test {
 		assertEquals(theMatchUrlCacheEnabled ? 3 : 4, myCaptureQueriesListener.countSelectQueriesForCurrentThread());
 		assertEquals(0, myCaptureQueriesListener.countInsertQueriesForCurrentThread());
 		assertEquals(4, myCaptureQueriesListener.countUpdateQueriesForCurrentThread());
-		assertEquals(4, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
+		assertEquals(1, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
 		assertEquals(1, myCaptureQueriesListener.countCommits());
 		assertEquals(0, myCaptureQueriesListener.countRollbacks());
 

hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/provider/r5/ServerR5Test.java

@@ -1,31 +1,45 @@
 package ca.uhn.fhir.jpa.provider.r5;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNull;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
+import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
+import ca.uhn.fhir.jpa.util.SqlQuery;
 import ca.uhn.fhir.rest.api.EncodingEnum;
+import ca.uhn.fhir.rest.api.server.IBundleProvider;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
 import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException;
 import ca.uhn.fhir.util.ExtensionConstants;
 import org.apache.commons.io.IOUtils;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
+import org.hl7.fhir.r5.model.Bundle;
 import org.hl7.fhir.r5.model.CapabilityStatement;
 import org.hl7.fhir.r5.model.CapabilityStatement.CapabilityStatementRestResourceComponent;
 import org.hl7.fhir.r5.model.CapabilityStatement.CapabilityStatementRestResourceSearchParamComponent;
+import org.hl7.fhir.r5.model.CodeableConcept;
+import org.hl7.fhir.r5.model.Coding;
+import org.hl7.fhir.r5.model.DateTimeType;
+import org.hl7.fhir.r5.model.Enumerations;
 import org.hl7.fhir.r5.model.Extension;
+import org.hl7.fhir.r5.model.Identifier;
+import org.hl7.fhir.r5.model.Observation;
 import org.hl7.fhir.r5.model.Patient;
+import org.hl7.fhir.r5.model.Quantity;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
 
@@ -145,6 +159,99 @@ public class ServerR5Test extends BaseResourceProviderR5Test {
 
 	}
 
+	@Test
+	public void updateOrCreate_batchesSearchUrlDelete() {
+		int numOfObs = 10;
+		int txSize = 5;
+		List<SqlQuery> deleteQueries;
+		myCaptureQueriesListener.clear();
 
+		String prefix = "p1";
+
+		// initial insert
+		doUpdateOrCreate(prefix, numOfObs, txSize);
+
+		// verify we have created the observations (sanity check)
+		@SuppressWarnings("unchecked")
+		IFhirResourceDao<Observation> obsDao = myDaoRegistry.getResourceDao("Observation");
+		IBundleProvider result = obsDao.search(new SearchParameterMap().setLoadSynchronous(true), new SystemRequestDetails());
+		assertFalse(result.isEmpty());
+
+		// creates create the initial search urls, so we expect no deletes
+		deleteQueries = myCaptureQueriesListener.getDeleteQueries();
+		assertTrue(deleteQueries.isEmpty());
+		myCaptureQueriesListener.clear();
+
+		// update
+		doUpdateOrCreate(prefix, numOfObs, txSize);
+
+		// the searchURLs should be deleted now, so we expect some deletes
+		// specifically, as many deletes as there were "transaction bundles" to process
+		deleteQueries = myCaptureQueriesListener.getDeleteQueries();
+		assertFalse(deleteQueries.isEmpty());
+		assertEquals(numOfObs / txSize, deleteQueries.size());
+		myCaptureQueriesListener.clear();
+	}
+
+	private void doUpdateOrCreate(String prefix, int numOfObs, int txSize) {
+		for (int i = 0; i < numOfObs / txSize; i++) {
+			Bundle bundle = new Bundle();
+			bundle.setType(Bundle.BundleType.TRANSACTION);
+			List<Bundle.BundleEntryComponent> bundleEntryComponents = createObservations(prefix, 10000 + i * txSize, txSize);
+			bundle.setEntry(bundleEntryComponents);
+			mySystemDao.transaction(new SystemRequestDetails(),
+				bundle);
+		}
+	}
+
+	private List<Bundle.BundleEntryComponent> createObservations(String prefix, int observationId, int num) {
+		List<Bundle.BundleEntryComponent> list = new ArrayList<>();
+		for (int i = 0; i < num; i++) {
+			Bundle.BundleEntryComponent bundleEntryComponent = new Bundle.BundleEntryComponent();
+			Observation obs = new Observation();
+			List<Identifier> identifierList = new ArrayList<>();
+			identifierList.add(new Identifier().setValue(prefix + observationId + i));
+			obs.setIdentifier(identifierList);
+			obs.setStatus(Enumerations.ObservationStatus.FINAL);
+			Coding code = new Coding("http://loinc.org", "85354-9", "Blood pressure");
+			obs.setCode(new CodeableConcept().addCoding(code));
+			obs.setEffective(createDateTime("2020-01-01T12:00:00-05:00"));
+			Coding code1 = new Coding("http://loinc.org", "8480-6", "Systolic blood pressure");
+			List<Observation.ObservationComponentComponent> obsccList = new ArrayList<>();
+			Observation.ObservationComponentComponent obsvC = new Observation.ObservationComponentComponent();
+			CodeableConcept cc = new CodeableConcept().addCoding(code1);
+			obsvC.setValue(cc);
+			Quantity quantity = new Quantity();
+			quantity.setUnit("mmHg");
+			quantity.setValue(170);
+			quantity.setSystem("http://unitsofmeasure.org");
+			quantity.setCode("mm[Hg]");
+			obsvC.setValue(quantity);
+			obsccList.add(obsvC);
+
+			Observation.ObservationComponentComponent obsvC1 = new Observation.ObservationComponentComponent();
+			CodeableConcept cc1 = new CodeableConcept(new Coding("http://loinc.org", "8462-4", "Diastolic blood pressure"));
+			Quantity quantity1 = new Quantity();
+			quantity1.setValue(110);
+			quantity1.setUnit("mmHg");
+			quantity1.setSystem("http://unitsofmeasure.org");
+			quantity1.setCode("mm[Hg]");
+			obsvC1.setCode(cc1);
+			obsvC1.setValue(quantity1);
+			obsccList.add(obsvC1);
+
+			bundleEntryComponent.setResource(obs);
+			Bundle.BundleEntryRequestComponent bundleEntryRequestComponent = new Bundle.BundleEntryRequestComponent();
+			bundleEntryRequestComponent.setMethod(Bundle.HTTPVerb.PUT);
+			bundleEntryRequestComponent.setUrl("Observation?identifier=" + prefix + observationId + i);
+			bundleEntryComponent.setRequest(bundleEntryRequestComponent);
+			list.add(bundleEntryComponent);
 
 		}
+		return list;
+	}
+
+	private DateTimeType createDateTime(String theDateString) {
+		return new DateTimeType(theDateString);
+	}
+}

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/storage/TransactionDetails.java

@@ -66,6 +66,7 @@ public class TransactionDetails {
 	private Map<String, IResourcePersistentId> myResolvedMatchUrls = Collections.emptyMap();
 	private Map<String, Supplier<IBaseResource>> myResolvedResources = Collections.emptyMap();
 	private Set<IResourcePersistentId> myDeletedResourceIds = Collections.emptySet();
+	private Set<IResourcePersistentId> myUpdatedResourceIds = Collections.emptySet();
 	private Map<String, Object> myUserData;
 	private ListMultimap<Pointcut, HookParams> myDeferredInterceptorBroadcasts;
 	private EnumSet<Pointcut> myDeferredInterceptorBroadcastPointcuts;
@@ -118,9 +119,40 @@ public class TransactionDetails {
 		}
 	}
 
+	/**
+	 * @since 7.6.0
+	 */
+	@SuppressWarnings("rawtypes")
+	public void addUpdatedResourceId(@Nonnull IResourcePersistentId theResourceId) {
+		Validate.notNull(theResourceId, "theResourceId must not be null");
+		if (myUpdatedResourceIds.isEmpty()) {
+			myUpdatedResourceIds = new HashSet<>();
+		}
+		myUpdatedResourceIds.add(theResourceId);
+	}
+
+	/**
+	 * @since 7.6.0
+	 */
+	@SuppressWarnings("rawtypes")
+	public void addUpdatedResourceIds(Collection<? extends IResourcePersistentId> theResourceIds) {
+		for (IResourcePersistentId id : theResourceIds) {
+			addUpdatedResourceId(id);
+		}
+	}
+
+	/**
+	 * @since 7.6.0
+	 */
+	@SuppressWarnings("rawtypes")
+	public Set<IResourcePersistentId> getUpdatedResourceIds() {
+		return myUpdatedResourceIds;
+	}
+
 	/**
 	 * @since 6.8.0
 	 */
+	@SuppressWarnings("rawtypes")
 	public void addDeletedResourceId(@Nonnull IResourcePersistentId theResourceId) {
 		Validate.notNull(theResourceId, "theResourceId must not be null");
 		if (myDeletedResourceIds.isEmpty()) {
@@ -132,6 +164,7 @@ public class TransactionDetails {
 	/**
 	 * @since 6.8.0
 	 */
+	@SuppressWarnings("rawtypes")
 	public void addDeletedResourceIds(Collection<? extends IResourcePersistentId> theResourceIds) {
 		for (IResourcePersistentId<?> next : theResourceIds) {
 			addDeletedResourceId(next);
@@ -141,6 +174,7 @@ public class TransactionDetails {
 	/**
 	 * @since 6.8.0
 	 */
+	@SuppressWarnings("rawtypes")
 	@Nonnull
 	public Set<IResourcePersistentId> getDeletedResourceIds() {
 		return Collections.unmodifiableSet(myDeletedResourceIds);

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java

@@ -1408,6 +1408,8 @@ public abstract class BaseTransactionProcessor {
 				theTransactionStopWatch.endCurrentTask();
 			}
 
+			postTransactionProcess(theTransactionDetails);
+
 			/*
 			 * Make sure that there are no conflicts from deletions. E.g. we can't delete something
 			 * if something else has a reference to it.. Unless the thing that has a reference to it
@@ -1517,6 +1519,13 @@ public abstract class BaseTransactionProcessor {
 		// nothing
 	}
 
+	/**
+	 * Implement to handle post transaction processing
+	 */
+	protected void postTransactionProcess(TransactionDetails theTransactionDetails) {
+		// nothing
+	}
+
 	/**
 	 * Check for if a resource id should be matched in a conditional update
 	 * If the FHIR version is older than R4, it follows the old specifications and does not match