Merge pull request #2880 from hapifhir/weird-contained-issue

Contained Issue

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2868-bug-in-conditional-create.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 2868
+jira: SMILE-1153
+title: "Fixed a bug in transaction bundle processing, specifically for bundles which contained both a conditional create, and a resource which relied on this conditional create as a reference.
+This would cause the referring resource to generate a contained resource instead of appropriately referencing the existing patient."

hapi-fhir-docs/src/test/java/ca/uhn/hapi/fhir/docs/ChangelogFilesTest.java

@@ -69,6 +69,9 @@ public class ChangelogFilesTest {
 			// this one is optional
 			fieldNames.remove("backport");
 
+			// this one is optional
+			fieldNames.remove("jira");
+
 			assertThat("Invalid element in " + next + ": " + fieldNames, fieldNames, empty());
 
 			if (haveIssue) {

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

@@ -94,6 +94,7 @@ import com.google.common.collect.Sets;
 import com.google.common.hash.HashFunction;
 import com.google.common.hash.Hashing;
 import org.apache.commons.lang3.NotImplementedException;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Validate;
 import org.hl7.fhir.instance.model.api.IAnyResource;
 import org.hl7.fhir.instance.model.api.IBase;
@@ -1162,10 +1163,8 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 					validateResourceForStorage((T) theResource, entity);
 				}
 			}
-			String resourceType = myContext.getResourceType(theResource);
+			if (!StringUtils.isBlank(entity.getResourceType())) {
-			if (isNotBlank(entity.getResourceType()) && !entity.getResourceType().equals(resourceType)) {
+				validateIncomingResourceTypeMatchesExisting(theResource, entity);
-				throw new UnprocessableEntityException(
-					"Existing resource ID[" + entity.getIdDt().toUnqualifiedVersionless() + "] is of type[" + entity.getResourceType() + "] - Cannot update with [" + resourceType + "]");
 			}
 		}
 
@@ -1206,6 +1205,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 			if (thePerformIndexing || ((ResourceTable) theEntity).getVersion() == 1) {
 
 				newParams = new ResourceIndexedSearchParams();
+
 				mySearchParamWithInlineReferencesExtractor.populateFromResource(newParams, theTransactionDetails, entity, theResource, existingParams, theRequest, thePerformIndexing);
 
 				changed = populateResourceIntoEntity(theTransactionDetails, theRequest, theResource, entity, true);
@@ -1415,6 +1415,14 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 		return entity;
 	}
 
+	private void validateIncomingResourceTypeMatchesExisting(IBaseResource theResource, ResourceTable entity) {
+		String resourceType = myContext.getResourceType(theResource);
+		if (!resourceType.equals(entity.getResourceType())) {
+			throw new UnprocessableEntityException(
+				"Existing resource ID[" + entity.getIdDt().toUnqualifiedVersionless() + "] is of type[" + entity.getResourceType() + "] - Cannot update with [" + resourceType + "]");
+		}
+	}
+
 	@Override
 	public ResourceTable updateInternal(RequestDetails theRequestDetails, T theResource, boolean thePerformIndexing, boolean theForceUpdateVersion,
 													IBasePersistedResource theEntity, IIdType theResourceId, IBaseResource theOldResource, TransactionDetails theTransactionDetails) {

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

@@ -89,6 +89,7 @@ import org.hl7.fhir.instance.model.api.IBaseReference;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.instance.model.api.IPrimitiveType;
+import org.hl7.fhir.r4.model.Task;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -790,6 +791,7 @@ public abstract class BaseTransactionProcessor {
 						String matchUrl = myVersionAdapter.getEntryRequestIfNoneExist(nextReqEntry);
 						matchUrl = performIdSubstitutionsInMatchUrl(theIdSubstitutions, matchUrl);
 						outcome = resourceDao.create(res, matchUrl, false, theTransactionDetails, theRequest);
+					 	res.setId(outcome.getId());
 						if (nextResourceId != null) {
 							handleTransactionCreateOrUpdateOutcome(theIdSubstitutions, theIdToPersistedOutcome, nextResourceId, outcome, nextRespEntry, resourceType, res, theRequest);
 						}
@@ -1010,13 +1012,9 @@ public abstract class BaseTransactionProcessor {
 
 			for (IIdType next : theAllIds) {
 				IIdType replacement = theIdSubstitutions.get(next);
-				if (replacement == null) {
+				if (replacement != null && !replacement.equals(next)) {
-					continue;
+					ourLog.debug("Placeholder resource ID \"{}\" was replaced with permanent ID \"{}\"", next, replacement);
 				}
-				if (replacement.equals(next)) {
-					continue;
-				}
-				ourLog.debug("Placeholder resource ID \"{}\" was replaced with permanent ID \"{}\"", next, replacement);
 			}
 
 			ListMultimap<Pointcut, HookParams> deferredBroadcastEvents = theTransactionDetails.endAcceptingDeferredInterceptorBroadcasts();
@@ -1099,7 +1097,6 @@ public abstract class BaseTransactionProcessor {
 				}
 				deferredIndexesForAutoVersioning.put(nextOutcome, referencesToAutoVersion);
 			}
-
 		}
 
 		// If we have any resources we'll be auto-versioning, index these next

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

@@ -46,6 +46,7 @@ import org.hl7.fhir.instance.model.api.IBase;
 import org.hl7.fhir.instance.model.api.IBaseBundle;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
+import org.jetbrains.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -61,10 +62,12 @@ import javax.persistence.criteria.Predicate;
 import javax.persistence.criteria.Root;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
@@ -218,18 +221,9 @@ public class TransactionProcessor extends BaseTransactionProcessor {
 						IQueryParameterType param = andList.get(0).get(0);
 
 						if (param instanceof TokenParam) {
-							TokenParam tokenParam = (TokenParam) param;
+							Predicate hashPredicate = buildHashPredicateFromTokenParam((TokenParam)param, requestPartitionId, cb, from, next);
-							Predicate hashPredicate = null;
-							if (isNotBlank(tokenParam.getValue()) && isNotBlank(tokenParam.getSystem())) {
-								next.myHashSystemAndValue = ResourceIndexedSearchParamToken.calculateHashSystemAndValue(myPartitionSettings, requestPartitionId, next.myResourceDefinition.getName(), next.myMatchUrlSearchMap.keySet().iterator().next(), tokenParam.getSystem(), tokenParam.getValue());
-								hashPredicate = cb.equal(from.get("myHashSystemAndValue").as(Long.class), next.myHashSystemAndValue);
-							} else if (isNotBlank(tokenParam.getValue())) {
-								next.myHashValue = ResourceIndexedSearchParamToken.calculateHashValue(myPartitionSettings, requestPartitionId, next.myResourceDefinition.getName(), next.myMatchUrlSearchMap.keySet().iterator().next(), tokenParam.getValue());
-								hashPredicate = cb.equal(from.get("myHashValue").as(Long.class), next.myHashValue);
-							}
 
 							if (hashPredicate != null) {
-
 								if (myPartitionSettings.isPartitioningEnabled() && !myPartitionSettings.isIncludePartitionInSearchHashes()) {
 									if (requestPartitionId.isDefaultPartition()) {
 										Predicate partitionIdCriteria = cb.isNull(from.get("myPartitionIdValue").as(Integer.class));
@@ -250,35 +244,26 @@ public class TransactionProcessor extends BaseTransactionProcessor {
 				if (orPredicates.size() > 1) {
 					cq.where(cb.or(orPredicates.toArray(EMPTY_PREDICATE_ARRAY)));
 
+					Map<Long, MatchUrlToResolve> hashToSearchMap = buildHashToSearchMap(searchParameterMapsToResolve);
+
 					TypedQuery<ResourceIndexedSearchParamToken> query = myEntityManager.createQuery(cq);
 					List<ResourceIndexedSearchParamToken> results = query.getResultList();
+
 					for (ResourceIndexedSearchParamToken nextResult : results) {
-
+						Optional<MatchUrlToResolve> matchedSearch = Optional.ofNullable(hashToSearchMap.get(nextResult.getHashSystemAndValue()));
-						for (MatchUrlToResolve nextSearchParameterMap : searchParameterMapsToResolve) {
+						if (!matchedSearch.isPresent()) {
-							if (nextSearchParameterMap.myHashSystemAndValue != null && nextSearchParameterMap.myHashSystemAndValue.equals(nextResult.getHashSystemAndValue())) {
+							matchedSearch =  Optional.ofNullable(hashToSearchMap.get(nextResult.getHashValue()));
-								idsToPreFetch.add(nextResult.getResourcePid());
-								myMatchResourceUrlService.matchUrlResolved(theTransactionDetails, nextSearchParameterMap.myResourceDefinition.getName(), nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid()));
-								theTransactionDetails.addResolvedMatchUrl(nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid()));
-								nextSearchParameterMap.myResolved = true;
-							}
-							if (nextSearchParameterMap.myHashValue != null && nextSearchParameterMap.myHashValue.equals(nextResult.getHashValue())) {
-								idsToPreFetch.add(nextResult.getResourcePid());
-								myMatchResourceUrlService.matchUrlResolved(theTransactionDetails, nextSearchParameterMap.myResourceDefinition.getName(), nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid()));
-								theTransactionDetails.addResolvedMatchUrl(nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid()));
-								nextSearchParameterMap.myResolved = true;
-							}
-
 						}
-
+						matchedSearch.ifPresent(matchUrlToResolve -> setSearchToResolvedAndPrefetchFoundResourcePid(theTransactionDetails, idsToPreFetch, nextResult, matchUrlToResolve));
 					}
-
+					//For each SP Map which did not return a result, tag it as not found.
-					for (MatchUrlToResolve nextSearchParameterMap : searchParameterMapsToResolve) {
+					searchParameterMapsToResolve.stream()
 						// No matches
-						if (!nextSearchParameterMap.myResolved) {
+						.filter(match -> !match.myResolved)
-							theTransactionDetails.addResolvedMatchUrl(nextSearchParameterMap.myRequestUrl, TransactionDetails.NOT_FOUND);
+						.forEach(match -> {
-						}
+							ourLog.warn("Was unable to match url {} from database", match.myRequestUrl);
-					}
+							theTransactionDetails.addResolvedMatchUrl(match.myRequestUrl, TransactionDetails.NOT_FOUND);
-
+						});
 				}
 			}
 
@@ -320,6 +305,45 @@ public class TransactionProcessor extends BaseTransactionProcessor {
 		return super.doTransactionWriteOperations(theRequest, theActionName, theTransactionDetails, theAllIds, theIdSubstitutions, theIdToPersistedOutcome, theResponse, theOriginalRequestOrder, theEntries, theTransactionStopWatch);
 	}
 
+	/**
+	 * Given a token parameter, build the query predicate based on its hash. Uses system and value if both are available, otherwise just value.
+	 * If neither are available, it returns null.
+	 */
+	@Nullable
+	private Predicate buildHashPredicateFromTokenParam(TokenParam theTokenParam, RequestPartitionId theRequestPartitionId, CriteriaBuilder cb, Root<ResourceIndexedSearchParamToken> from, MatchUrlToResolve theMatchUrl) {
+		Predicate hashPredicate = null;
+		if (isNotBlank(theTokenParam.getValue()) && isNotBlank(theTokenParam.getSystem())) {
+			theMatchUrl.myHashSystemAndValue = ResourceIndexedSearchParamToken.calculateHashSystemAndValue(myPartitionSettings, theRequestPartitionId, theMatchUrl.myResourceDefinition.getName(), theMatchUrl.myMatchUrlSearchMap.keySet().iterator().next(), theTokenParam.getSystem(), theTokenParam.getValue());
+			hashPredicate = cb.equal(from.get("myHashSystemAndValue").as(Long.class), theMatchUrl.myHashSystemAndValue);
+		} else if (isNotBlank(theTokenParam.getValue())) {
+			theMatchUrl.myHashValue = ResourceIndexedSearchParamToken.calculateHashValue(myPartitionSettings, theRequestPartitionId, theMatchUrl.myResourceDefinition.getName(), theMatchUrl.myMatchUrlSearchMap.keySet().iterator().next(), theTokenParam.getValue());
+			hashPredicate = cb.equal(from.get("myHashValue").as(Long.class), theMatchUrl.myHashValue);
+		}
+		return hashPredicate;
+	}
+
+	private Map<Long, MatchUrlToResolve> buildHashToSearchMap(List<MatchUrlToResolve> searchParameterMapsToResolve) {
+		Map<Long, MatchUrlToResolve> hashToSearch = new HashMap<>();
+		//Build a lookup map so we don't have to iterate over the searches repeatedly.
+		for (MatchUrlToResolve nextSearchParameterMap : searchParameterMapsToResolve) {
+			if (nextSearchParameterMap.myHashSystemAndValue != null) {
+				hashToSearch.put(nextSearchParameterMap.myHashSystemAndValue,  nextSearchParameterMap);
+			}
+			if (nextSearchParameterMap.myHashValue!= null) {
+				hashToSearch.put(nextSearchParameterMap.myHashValue,  nextSearchParameterMap);
+			}
+		}
+		return hashToSearch;
+	}
+
+	private void setSearchToResolvedAndPrefetchFoundResourcePid(TransactionDetails theTransactionDetails, List<Long> idsToPreFetch, ResourceIndexedSearchParamToken nextResult, MatchUrlToResolve nextSearchParameterMap) {
+		ourLog.warn("Matched url {} from database", nextSearchParameterMap.myRequestUrl);
+		idsToPreFetch.add(nextResult.getResourcePid());
+		myMatchResourceUrlService.matchUrlResolved(theTransactionDetails, nextSearchParameterMap.myResourceDefinition.getName(), nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid()));
+		theTransactionDetails.addResolvedMatchUrl(nextSearchParameterMap.myRequestUrl, new ResourcePersistentId(nextResult.getResourcePid()));
+		nextSearchParameterMap.setResolved(true);
+	}
+
 	private List<ResourceTable> preFetchIndexes(List<Long> ids, String typeDesc, String fieldName) {
 		TypedQuery<ResourceTable> query = myEntityManager.createQuery("FROM ResourceTable r LEFT JOIN FETCH r." + fieldName + " WHERE r.myId IN ( :IDS )", ResourceTable.class);
 		query.setParameter("IDS", ids);
@@ -379,5 +403,8 @@ public class TransactionProcessor extends BaseTransactionProcessor {
 			myMatchUrlSearchMap = theMatchUrlSearchMap;
 			myResourceDefinition = theResourceDefinition;
 		}
+		public void setResolved(boolean theResolved) {
+			myResolved = theResolved;
+		}
 	}
 }

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

@@ -1,6 +1,7 @@
 package ca.uhn.fhir.jpa.dao.r4;
 
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
+import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao;
 import ca.uhn.fhir.jpa.model.entity.NormalizedQuantitySearchLevel;
 import ca.uhn.fhir.jpa.model.entity.ResourceEncodingEnum;
@@ -63,6 +64,7 @@ import org.hl7.fhir.r4.model.Practitioner;
 import org.hl7.fhir.r4.model.Quantity;
 import org.hl7.fhir.r4.model.Reference;
 import org.hl7.fhir.r4.model.Resource;
+import org.hl7.fhir.r4.model.Task;
 import org.hl7.fhir.r4.model.ValueSet;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -1002,6 +1004,23 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
 	}
 
 
+	@Test
+	public void testTransactionNoContainedRedux() throws IOException {
+		//Pre-create the patient, which will cause the ifNoneExist to prevent a new creation during bundle transaction
+		Patient patient = loadResourceFromClasspath(Patient.class, "/r4/preexisting-patient.json");
+		myPatientDao.create(patient);
+
+		//Post the Bundle containing a conditional POST with an identical patient from the above resource.
+		Bundle request = loadResourceFromClasspath(Bundle.class, "/r4/transaction-no-contained-2.json");
+
+		Bundle outcome = mySystemDao.transaction(mySrd, request);
+
+		IdType taskId = new IdType(outcome.getEntry().get(0).getResponse().getLocation());
+		Task task = myTaskDao.read(taskId, mySrd);
+
+		assertThat(task.getBasedOn().get(0).getReference(), matchesPattern("Patient/[0-9]+"));
+	}
+
 	@Test
 	public void testTransactionNoContained() throws IOException {
 

hapi-fhir-jpaserver-base/src/test/resources/r4/preexisting-patient.json

@@ -0,0 +1,13 @@
+{
+	"resourceType": "Patient",
+	"identifier": [
+		{
+			"system": "https://example.org/fhir/memberidentifier",
+			"value": "12345670"
+		},
+		{
+			"system": "https://example.org/fhir/memberuniqueidentifier",
+			"value": "12345670TT"
+		}
+	]
+}

hapi-fhir-jpaserver-base/src/test/resources/r4/transaction-no-contained-2.json

@@ -0,0 +1,49 @@
+{
+	"resourceType": "Bundle",
+	"type": "transaction",
+	"entry": [
+		{
+			"fullUrl": "urn:uuid:1f3b9e25-fd45-4342-a82b-7ca5755923bb",
+			"resource": {
+				"resourceType": "Task",
+				"language": "en-US",
+				"identifier": [
+					{
+						"system": "https://example.org/fhir/taskidentifier",
+						"value": "101019"
+					}
+				],
+				"basedOn": [
+					{
+						"reference": "urn:uuid:47c6d106-3441-41c0-8a2c-054ad9897ced"
+					}
+				]
+			},
+			"request": {
+				"method": "PUT",
+				"url": "/Task?identifier\u003dhttps%3A%2F%2Fexample.org%2Ffhir%2Ftaskidentifier|101019"
+			}
+		},
+		{
+			"fullUrl": "urn:uuid:47c6d106-3441-41c0-8a2c-054ad9897ced",
+			"resource": {
+				"resourceType": "Patient",
+				"identifier": [
+					{
+						"system": "https://example.org/fhir/memberidentifier",
+						"value": "12345670"
+					},
+					{
+						"system": "https://example.org/fhir/memberuniqueidentifier",
+						"value": "12345670TT"
+					}
+				]
+			},
+			"request": {
+				"method": "POST",
+				"url": "/Patient",
+				"ifNoneExist": "Patient?identifier\u003dhttps%3A%2F%2Fexample.org%2Ffhir%2Fmemberuniqueidentifier|12345670TT"
+			}
+		}
+	]
+}

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/BaseSearchParamExtractor.java

@@ -78,6 +78,7 @@ import java.util.Collections;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeSet;
@@ -964,6 +965,18 @@ public abstract class BaseSearchParamExtractor implements ISearchParamExtractor
 		return retVal;
 	}
 
+
+	/**
+	 * Helper function to determine if a set of SPs for a resource uses a resolve as part of its fhir path.
+	 */
+	private boolean anySearchParameterUsesResolve(Collection<RuntimeSearchParam> searchParams, RestSearchParameterTypeEnum theSearchParamType) {
+		return searchParams.stream()
+			.filter(param -> param.getParamType() != theSearchParamType)
+			.map(RuntimeSearchParam::getPath)
+			.filter(Objects::nonNull)
+			.anyMatch(path-> path.contains("resolve"));
+	}
+
 	/**
 	 * HAPI FHIR Reference objects (e.g. {@link org.hl7.fhir.r4.model.Reference}) can hold references either by text
 	 * (e.g. "#3") or by resource (e.g. "new Reference(patientInstance)"). The FHIRPath evaluator only understands the
@@ -974,17 +987,12 @@ public abstract class BaseSearchParamExtractor implements ISearchParamExtractor
 	 * if we think there's actually a chance
 	 */
 	private void cleanUpContainedResourceReferences(IBaseResource theResource, RestSearchParameterTypeEnum theSearchParamType, Collection<RuntimeSearchParam> searchParams) {
-		boolean havePathWithResolveExpression = myModelConfig.isIndexOnContainedResources();
+		boolean havePathWithResolveExpression =
-		for (RuntimeSearchParam nextSpDef : searchParams) {
+			myModelConfig.isIndexOnContainedResources()
-			if (nextSpDef.getParamType() != theSearchParamType) {
+			|| anySearchParameterUsesResolve(searchParams, theSearchParamType);
-				continue;
+
-			}
-			if (defaultString(nextSpDef.getPath()).contains("resolve")) {
-				havePathWithResolveExpression = true;
-				break;
-			}
-		}
 		if (havePathWithResolveExpression) {
+			//TODO GGG/JA: At this point, if the Task.basedOn.reference.resource does _not_ have an ID, we will attempt to contain it internally. Wild
 			myContext.newTerser().containResources(theResource, FhirTerser.OptionsEnum.MODIFY_RESOURCE, FhirTerser.OptionsEnum.STORE_AND_REUSE_RESULTS);
 		}
 	}