issue-2901 some refactoring

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

@@ -384,7 +384,8 @@ public abstract class BaseTransactionProcessor {
 		myHapiTransactionService = theHapiTransactionService;
 	}
 
-	private IBaseBundle processTransaction(final RequestDetails theRequestDetails, final IBaseBundle theRequest, final String theActionName, boolean theNestedMode) {
+	private IBaseBundle processTransaction(final RequestDetails theRequestDetails, final IBaseBundle theRequest,
+														final String theActionName, boolean theNestedMode) {
 		validateDependencies();
 
 		String transactionType = myVersionAdapter.getBundleType(theRequest);
@@ -440,10 +441,11 @@ public abstract class BaseTransactionProcessor {
 		List<IBase> getEntries = new ArrayList<>();
 		final IdentityHashMap<IBase, Integer> originalRequestOrder = new IdentityHashMap<>();
 		for (int i = 0; i < requestEntries.size(); i++) {
-			originalRequestOrder.put(requestEntries.get(i), i);
+			IBase requestEntry = requestEntries.get(i);
+			originalRequestOrder.put(requestEntry, i);
 			myVersionAdapter.addEntry(response);
-			if (myVersionAdapter.getEntryRequestVerb(myContext, requestEntries.get(i)).equals("GET")) {
+			if (myVersionAdapter.getEntryRequestVerb(myContext, requestEntry).equals("GET")) {
-				getEntries.add(requestEntries.get(i));
+				getEntries.add(requestEntry);
 			}
 		}
 
@@ -462,16 +464,43 @@ public abstract class BaseTransactionProcessor {
 		}
 		entries.sort(new TransactionSorter(placeholderIds));
 
-		doTransactionWriteOperations(theRequestDetails, theActionName, transactionDetails, transactionStopWatch, response, originalRequestOrder, entries);
+		// perform all writes
+		doTransactionWriteOperations(theRequestDetails, theActionName,
+			transactionDetails, transactionStopWatch,
+			response, originalRequestOrder, entries);
+
+		// perform all gets
+		// (we do these last so that the gets happen on the final state of the DB;
+		// see above note)
+		doTransactionReadOperations(theRequestDetails, response,
+			getEntries, originalRequestOrder,
+			transactionStopWatch, theNestedMode);
+
+		// Interceptor broadcast: JPA_PERFTRACE_INFO
+		if (CompositeInterceptorBroadcaster.hasHooks(Pointcut.JPA_PERFTRACE_INFO, myInterceptorBroadcaster, theRequestDetails)) {
+			String taskDurations = transactionStopWatch.formatTaskDurations();
+			StorageProcessingMessage message = new StorageProcessingMessage();
+			message.setMessage("Transaction timing:\n" + taskDurations);
+			HookParams params = new HookParams()
+				.add(RequestDetails.class, theRequestDetails)
+				.addIfMatchesType(ServletRequestDetails.class, theRequestDetails)
+				.add(StorageProcessingMessage.class, message);
+			CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequestDetails, Pointcut.JPA_PERFTRACE_INFO, params);
+		}
+
+		return response;
+	}
+
+	private void doTransactionReadOperations(final RequestDetails theRequestDetails, IBaseBundle theResponse,
+														  List<IBase> theGetEntries, IdentityHashMap<IBase, Integer> theOriginalRequestOrder,
+														  StopWatch theTransactionStopWatch, boolean theNestedMode) {
+		if (theGetEntries.size() > 0) {
+			theTransactionStopWatch.startTask("Process " + theGetEntries.size() + " GET entries");
 
 			/*
 			 * Loop through the request and process any entries of type GET
 			 */
-		if (getEntries.size() > 0) {
+			for (IBase nextReqEntry : theGetEntries) {
-			transactionStopWatch.startTask("Process " + getEntries.size() + " GET entries");
-		}
-		for (IBase nextReqEntry : getEntries) {
-
 				if (theNestedMode) {
 					throw new InvalidRequestException("Can not invoke read operation on nested transaction");
 				}
@@ -481,8 +510,8 @@ public abstract class BaseTransactionProcessor {
 				}
 
 				ServletRequestDetails srd = (ServletRequestDetails) theRequestDetails;
-			Integer originalOrder = originalRequestOrder.get(nextReqEntry);
+				Integer originalOrder = theOriginalRequestOrder.get(nextReqEntry);
-			IBase nextRespEntry = (IBase) myVersionAdapter.getEntries(response).get(originalOrder);
+				IBase nextRespEntry = (IBase) myVersionAdapter.getEntries(theResponse).get(originalOrder);
 
 				ArrayListMultimap<String, String> paramValues = ArrayListMultimap.create();
 
@@ -509,7 +538,6 @@ public abstract class BaseTransactionProcessor {
 
 				Validate.isTrue(method instanceof BaseResourceReturningMethodBinding, "Unable to handle GET {}", url);
 				try {
-
 					BaseResourceReturningMethodBinding methodBinding = (BaseResourceReturningMethodBinding) method;
 					requestDetails.setRestOperationType(methodBinding.getRestOperationType());
 
@@ -526,23 +554,9 @@ public abstract class BaseTransactionProcessor {
 					myVersionAdapter.setResponseStatus(nextRespEntry, toStatusString(e.getStatusCode()));
 					populateEntryWithOperationOutcome(e, nextRespEntry);
 				}
-
 			}
-		transactionStopWatch.endCurrentTask();
+			theTransactionStopWatch.endCurrentTask();
-
-		// Interceptor broadcast: JPA_PERFTRACE_INFO
-		if (CompositeInterceptorBroadcaster.hasHooks(Pointcut.JPA_PERFTRACE_INFO, myInterceptorBroadcaster, theRequestDetails)) {
-			String taskDurations = transactionStopWatch.formatTaskDurations();
-			StorageProcessingMessage message = new StorageProcessingMessage();
-			message.setMessage("Transaction timing:\n" + taskDurations);
-			HookParams params = new HookParams()
-				.add(RequestDetails.class, theRequestDetails)
-				.addIfMatchesType(ServletRequestDetails.class, theRequestDetails)
-				.add(StorageProcessingMessage.class, message);
-			CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequestDetails, Pointcut.JPA_PERFTRACE_INFO, params);
 		}
-
-		return response;
 	}
 
 	/**
@@ -554,7 +568,10 @@ public abstract class BaseTransactionProcessor {
 	 * heavy load with lots of concurrent transactions using all available
 	 * database connections.
 	 */
-	private void doTransactionWriteOperations(RequestDetails theRequestDetails, String theActionName, TransactionDetails theTransactionDetails, StopWatch theTransactionStopWatch, IBaseBundle theResponse, IdentityHashMap<IBase, Integer> theOriginalRequestOrder, List<IBase> theEntries) {
+	private void doTransactionWriteOperations(RequestDetails theRequestDetails, String theActionName,
+															TransactionDetails theTransactionDetails, StopWatch theTransactionStopWatch,
+															IBaseBundle theResponse, IdentityHashMap<IBase, Integer> theOriginalRequestOrder,
+															List<IBase> theEntries) {
 		TransactionWriteOperationsDetails writeOperationsDetails = null;
 		if (CompositeInterceptorBroadcaster.hasHooks(Pointcut.STORAGE_TRANSACTION_WRITE_OPERATIONS_PRE, myInterceptorBroadcaster, theRequestDetails) ||
 			CompositeInterceptorBroadcaster.hasHooks(Pointcut.STORAGE_TRANSACTION_WRITE_OPERATIONS_POST, myInterceptorBroadcaster, theRequestDetails)) {
@@ -583,14 +600,18 @@ public abstract class BaseTransactionProcessor {
 				.add(TransactionDetails.class, theTransactionDetails)
 				.add(TransactionWriteOperationsDetails.class, writeOperationsDetails);
 			CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequestDetails, Pointcut.STORAGE_TRANSACTION_WRITE_OPERATIONS_PRE, params);
-
 		}
 
 		TransactionCallback<Map<IBase, IIdType>> txCallback = status -> {
 			final Set<IIdType> allIds = new LinkedHashSet<>();
 			final Map<IIdType, IIdType> idSubstitutions = new HashMap<>();
 			final Map<IIdType, DaoMethodOutcome> idToPersistedOutcome = new HashMap<>();
-			Map<IBase, IIdType> retVal = doTransactionWriteOperations(theRequestDetails, theActionName, theTransactionDetails, allIds, idSubstitutions, idToPersistedOutcome, theResponse, theOriginalRequestOrder, theEntries, theTransactionStopWatch);
+
+			Map<IBase, IIdType> retVal = doTransactionWriteOperations(theRequestDetails, theActionName,
+				theTransactionDetails, allIds,
+				idSubstitutions, idToPersistedOutcome,
+				theResponse, theOriginalRequestOrder,
+				theEntries, theTransactionStopWatch);
 
 			theTransactionStopWatch.startTask("Commit writes to database");
 			return retVal;
@@ -721,7 +742,7 @@ public abstract class BaseTransactionProcessor {
 	}
 
 	/**
-	 * Retrieves teh next resource id (IIdType) from the base resource and next request entry.
+	 * Retrieves the next resource id (IIdType) from the base resource and next request entry.
 	 * @param theBaseResource - base resource
 	 * @param theNextReqEntry - next request entry
 	 * @param theAllIds - set of all IIdType values
@@ -1161,19 +1182,11 @@ public abstract class BaseTransactionProcessor {
 					nextOutcome, nextResource,
 					referencesToAutoVersion); // this is empty
 			} else {
+				// we have autoversioned things to defer until later
 				if (deferredIndexesForAutoVersioning == null) {
 					deferredIndexesForAutoVersioning = new IdentityHashMap<>();
 				}
 				deferredIndexesForAutoVersioning.put(nextOutcome, referencesToAutoVersion);
-
-				// TODO - add the references to the
-				// idsToPersistedOutcomes
-//				for (IBaseReference autoVersion: referencesToAutoVersion) {
-//					IBaseResource resource = myVersionAdapter.getResource(autoVersion);
-//					IFhirResourceDao dao = getDaoOrThrowException(resource.getClass());
-//
-//				}
-//				theIdToPersistedOutcome.put()
 			}
 		}
 
@@ -1183,6 +1196,15 @@ public abstract class BaseTransactionProcessor {
 				DaoMethodOutcome nextOutcome = nextEntry.getKey();
 				Set<IBaseReference> referencesToAutoVersion = nextEntry.getValue();
 				IBaseResource nextResource = nextOutcome.getResource();
+
+				//TODO - should we add the autoversioned resources to our idtoPersistedoutcomes here?
+//				for (IBaseReference autoVersionRef : referencesToAutoVersion) {
+//					IBaseResource baseResource = myVersionAdapter.getResource(autoVersionRef);
+//					IFhirResourceDao dao = getDaoOrThrowException(baseResource.getClass());
+//
+//					theIdToPersistedOutcome.put(baseResource.getIdElement(), );
+//				}
+
 				resolveReferencesThenSaveAndIndexResource(theRequest, theTransactionDetails,
 					theIdSubstitutions, theIdToPersistedOutcome,
 					entriesToProcess, nonUpdatedEntities,

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

@@ -1,83 +0,0 @@
-package ca.uhn.fhir.jpa.dao.r4;
-
-import ca.uhn.fhir.context.FhirContext;
-import ca.uhn.fhir.context.ParserOptions;
-import ca.uhn.fhir.jpa.api.config.DaoConfig;
-import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
-import ca.uhn.fhir.jpa.model.entity.ModelConfig;
-import ca.uhn.fhir.jpa.partition.SystemRequestDetails;
-import ca.uhn.fhir.jpa.provider.r4.ResourceProviderR4Test;
-import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
-import ca.uhn.fhir.rest.api.server.IBundleProvider;
-import ca.uhn.fhir.rest.param.TokenParam;
-import ca.uhn.fhir.util.BundleBuilder;
-import org.hl7.fhir.instance.model.api.IBaseResource;
-import org.hl7.fhir.instance.model.api.IIdType;
-import org.hl7.fhir.r4.model.Bundle;
-import org.hl7.fhir.r4.model.Condition;
-import org.hl7.fhir.r4.model.Encounter;
-import org.hl7.fhir.r4.model.ExplanationOfBenefit;
-import org.hl7.fhir.r4.model.IdType;
-import org.hl7.fhir.r4.model.Observation;
-import org.hl7.fhir.r4.model.Organization;
-import org.hl7.fhir.r4.model.Patient;
-import org.hl7.fhir.r4.model.Reference;
-import org.hl7.fhir.r4.model.Task;
-import org.junit.jupiter.api.AfterEach;
-import org.junit.jupiter.api.DisplayName;
-import org.junit.jupiter.api.Test;
-
-import java.io.InputStreamReader;
-import java.util.Arrays;
-import java.util.Date;
-import java.util.HashSet;
-import java.util.List;
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.function.Supplier;
-import java.util.stream.Collectors;
-
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.matchesPattern;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
-public class AAAATests extends BaseJpaR4Test {
-	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoR4VersionedReferenceTest.class);
-
-	@AfterEach
-	public void afterEach() {
-		myFhirCtx.getParserOptions().setStripVersionsFromReferences(true);
-		myFhirCtx.getParserOptions().getDontStripVersionsFromReferencesAtPaths().clear();
-		myDaoConfig.setDeleteEnabled(new DaoConfig().isDeleteEnabled());
-		myModelConfig.setRespectVersionsForSearchIncludes(new ModelConfig().isRespectVersionsForSearchIncludes());
-		myModelConfig.setAutoVersionReferenceAtPaths(new ModelConfig().getAutoVersionReferenceAtPaths());
-	}
-
-
-	@Test
-	@DisplayName("GH-2901 Test no NPE is thrown on autoversioned references")
-	public void testNoNpeMinimal() {
-		myDaoConfig.setAutoCreatePlaceholderReferenceTargets(false);
-		myModelConfig.setAutoVersionReferenceAtPaths("Observation.subject");
-
-//		ParserOptions options = new ParserOptions();
-//		options.setDontStripVersionsFromReferencesAtPaths("Observation.subject");
-//		myFhirCtx.setParserOptions(options);
-
-		Patient patient = new Patient();
-		patient.setId("Patient/RED");
-		myPatientDao.update(patient);
-
-		Observation obs = new Observation();
-		obs.setId("Observation/DEF");
-		obs.setSubject(new Reference("Patient/RED"));
-		BundleBuilder builder = new BundleBuilder(myFhirCtx);
-		builder.addTransactionUpdateEntry(obs);
-
-		mySystemDao.transaction(new SystemRequestDetails(), (Bundle) builder.getBundle());
-	}
-
-
-}

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

@@ -303,7 +303,6 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
 		myFhirCtx.getParserOptions().setStripVersionsFromReferences(false);
 		myModelConfig.setAutoVersionReferenceAtPaths("Observation.subject");
 
-
 		BundleBuilder builder = new BundleBuilder(myFhirCtx);
 
 		Patient patient = new Patient();
@@ -334,7 +333,6 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
 		observation = myObservationDao.read(observationId);
 		assertEquals(patientId.getValue(), observation.getSubject().getReference());
 		assertEquals(encounterId.toVersionless().getValue(), observation.getEncounter().getReference());
-
 	}
 
 	@Test
@@ -397,7 +395,6 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
 		assertEquals(patientId.getValue(), observation.getSubject().getReference());
 		assertEquals("2", observation.getSubject().getReferenceElement().getVersionIdPart());
 		assertEquals(encounterId.toVersionless().getValue(), observation.getEncounter().getReference());
-
 	}
 
 
@@ -417,7 +414,6 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
 			// Update patient to make a second version
 			patient.setActive(false);
 			myPatientDao.update(patient);
-
 		}
 
 		BundleBuilder builder = new BundleBuilder(myFhirCtx);
@@ -466,7 +462,6 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
 			// Update patient to make a second version
 			patient.setActive(false);
 			myPatientDao.update(patient);
-
 		}
 
 		BundleBuilder builder = new BundleBuilder(myFhirCtx);
@@ -499,10 +494,8 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
 		// Read back and verify that reference is now versioned
 		observation = myObservationDao.read(observationId);
 		assertEquals(patientId.getValue(), observation.getSubject().getReference());
-
 	}
 
-
 	@Test
 	public void testSearchAndIncludeVersionedReference_Asynchronous() {
 		myFhirCtx.getParserOptions().setStripVersionsFromReferences(false);