Fix Match URL cache should take partition into account

hapi-fhir-base/src/main/java/ca/uhn/fhir/util/BundleBuilder.java

@@ -27,6 +27,7 @@
 import ca.uhn.fhir.model.primitive.IdDt;
 import jakarta.annotation.Nonnull;
 import jakarta.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Validate;
 import org.hl7.fhir.instance.model.api.IBase;
 import org.hl7.fhir.instance.model.api.IBaseBackboneElement;
@@ -194,13 +195,26 @@ public PatchBuilder addTransactionFhirPatchEntry(IBaseParameters thePatch) {
 	 * @param theResource The resource to update
 	 */
 	public UpdateBuilder addTransactionUpdateEntry(IBaseResource theResource) {
+		return addTransactionUpdateEntry(theResource, null);
+	}
+
+	/**
+	 * Adds an entry containing an update (PUT) request.
+	 * Also sets the Bundle.type value to "transaction" if it is not already set.
+	 *
+	 * @param theResource The resource to update
+	 * @param theRequestUrl The url to attach to the Bundle.entry.request.url. If null, will default to the resource ID.
+	 */
+	public UpdateBuilder addTransactionUpdateEntry(IBaseResource theResource, String theRequestUrl) {
 		Validate.notNull(theResource, "theResource must not be null");
 
 		IIdType id = getIdTypeForUpdate(theResource);
 
-		String requestUrl = id.toUnqualifiedVersionless().getValue();
 		String fullUrl = id.getValue();
 		String verb = "PUT";
+		String requestUrl = StringUtils.isBlank(theRequestUrl)
+				? id.toUnqualifiedVersionless().getValue()
+				: theRequestUrl;
 
 		IPrimitiveType<?> url = addAndPopulateTransactionBundleEntryRequest(theResource, fullUrl, requestUrl, verb);
 
@@ -215,10 +229,7 @@ private IPrimitiveType<?> addAndPopulateTransactionBundleEntryRequest(
 		IBase request = addEntryAndReturnRequest(theResource, theFullUrl);
 
 		// Bundle.entry.request.url
-		IPrimitiveType<?> url =
-				(IPrimitiveType<?>) myContext.getElementDefinition("uri").newInstance();
-		url.setValueAsString(theRequestUrl);
-		myEntryRequestUrlChild.getMutator().setValue(request, url);
+		IPrimitiveType<?> url = addRequestUrl(request, theRequestUrl);
 
 		// Bundle.entry.request.method
 		addRequestMethod(request, theHttpVerb);
@@ -416,11 +427,12 @@ private void addFullUrl(IBase theEntry, String theFullUrl) {
 		myEntryFullUrlChild.getMutator().setValue(theEntry, fullUrl);
 	}
 
-	private void addRequestUrl(IBase request, String theRequestUrl) {
+	private IPrimitiveType<?> addRequestUrl(IBase request, String theRequestUrl) {
 		IPrimitiveType<?> url =
 				(IPrimitiveType<?>) myContext.getElementDefinition("uri").newInstance();
 		url.setValueAsString(theRequestUrl);
 		myEntryRequestUrlChild.getMutator().setValue(request, url);
+		return url;
 	}
 
 	private void addRequestMethod(IBase theRequest, String theMethod) {

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_2_0/6767-fix-match-url-cache-and-partitions.yaml

@@ -0,0 +1,7 @@
+---
+type: fix
+issue: 6767
+jira: SMILE-7942
+title: "Previously, when both Match URL cache and Partitioning mode were enabled, the Match URL cache would sometimes 
+return the incorrect resource while resolving a conditional URL that matches to a resource that exists in multiple 
+partitions. This has now been fixed."

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

@@ -302,6 +302,11 @@ protected IDeleteExpungeJobSubmitter getDeleteExpungeJobSubmitter() {
 		return myDeleteExpungeJobSubmitter;
 	}
 
+	@Override
+	protected IRequestPartitionHelperSvc getRequestPartitionHelperService() {
+		return myRequestPartitionHelperService;
+	}
+
 	/**
 	 * @deprecated Use {@link #create(T, RequestDetails)} instead
 	 */
@@ -425,7 +430,7 @@ private DaoMethodOutcome doCreateForPostOrPut(
 
 		if (isNotBlank(theMatchUrl) && theProcessMatchUrl) {
 			Set<JpaPid> match = myMatchResourceUrlService.processMatchUrl(
-					theMatchUrl, myResourceType, theTransactionDetails, theRequest);
+					theMatchUrl, myResourceType, theTransactionDetails, theRequest, theRequestPartitionId);
 			ourLog.trace("Resolving match URL {} found: {}", theMatchUrl, match);
 			if (match.size() > 1) {
 				String msg = getContext()
@@ -2393,7 +2398,7 @@ private DaoMethodOutcome doUpdate(
 		if (isNotBlank(theMatchUrl)) {
 			// Validate that the supplied resource matches the conditional.
 			Set<JpaPid> match = myMatchResourceUrlService.processMatchUrl(
-					theMatchUrl, myResourceType, theTransactionDetails, theRequest, theResource);
+					theMatchUrl, myResourceType, theTransactionDetails, theRequest, theResource, theRequestPartitionId);
 			if (match.size() > 1) {
 				String msg = getContext()
 						.getLocalizer()

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

@@ -358,10 +358,21 @@ private void preFetchConditionalUrls(
 					resourceType = myFhirContext.getResourceType(resource);
 				}
 				if (("PUT".equals(verb) || "PATCH".equals(verb)) && requestUrl != null && requestUrl.contains("?")) {
-					preFetchConditionalUrl(resourceType, requestUrl, true, idsToPreFetch, searchParameterMapsToResolve);
+					preFetchConditionalUrl(
+							resourceType,
+							requestUrl,
+							true,
+							idsToPreFetch,
+							searchParameterMapsToResolve,
+							theRequestPartitionId);
 				} else if ("POST".equals(verb) && requestIfNoneExist != null && requestIfNoneExist.contains("?")) {
 					preFetchConditionalUrl(
-							resourceType, requestIfNoneExist, false, idsToPreFetch, searchParameterMapsToResolve);
+							resourceType,
+							requestIfNoneExist,
+							false,
+							idsToPreFetch,
+							searchParameterMapsToResolve,
+							theRequestPartitionId);
 				}
 
 				if (myStorageSettings.isAllowInlineMatchUrlReferences()) {
@@ -374,7 +385,12 @@ private void preFetchConditionalUrls(
 						String refResourceType = determineResourceTypeInResourceUrl(myFhirContext, referenceUrl);
 						if (refResourceType != null) {
 							preFetchConditionalUrl(
-									refResourceType, referenceUrl, false, idsToPreFetch, searchParameterMapsToResolve);
+									refResourceType,
+									referenceUrl,
+									false,
+									idsToPreFetch,
+									searchParameterMapsToResolve,
+									theRequestPartitionId);
 						}
 					}
 				}
@@ -546,14 +562,17 @@ private void preFetchSearchParameterMapsToken(
 	 * @param theShouldPreFetchResourceBody         Should we also fetch the actual resource body, or just figure out the PID associated with it. See the method javadoc above for some context.
 	 * @param theOutputIdsToPreFetch                This will be populated with any resource PIDs that need to be pre-fetched
 	 * @param theOutputSearchParameterMapsToResolve This will be populated with any {@link SearchParameterMap} instances corresponding to match URLs we need to resolve
+	 * @param thePartitionId						The partition ID of the associated resource (can be null)
 	 */
 	private void preFetchConditionalUrl(
 			String theResourceType,
 			String theRequestUrl,
 			boolean theShouldPreFetchResourceBody,
 			List<Long> theOutputIdsToPreFetch,
-			List<MatchUrlToResolve> theOutputSearchParameterMapsToResolve) {
-		JpaPid cachedId = myMatchResourceUrlService.processMatchUrlUsingCacheOnly(theResourceType, theRequestUrl);
+			List<MatchUrlToResolve> theOutputSearchParameterMapsToResolve,
+			RequestPartitionId thePartitionId) {
+		JpaPid cachedId =
+				myMatchResourceUrlService.processMatchUrlUsingCacheOnly(theResourceType, theRequestUrl, thePartitionId);
 		if (cachedId != null) {
 			if (theShouldPreFetchResourceBody) {
 				theOutputIdsToPreFetch.add(cachedId.getId());

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/TransactionConcurrencySemaphoreInterceptor.java

@@ -180,7 +180,7 @@ public void post(TransactionDetails theTransactionDetails) {
 	}
 
 	/**
-	 * Clear all semaphors from the list. This is really mostly intended for testing scenarios.
+	 * Clear all semaphores from the list. This is really mostly intended for testing scenarios.
 	 */
 	public void clearSemaphores() {
 		mySemaphoreCache.invalidateAll();

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

@@ -0,0 +1,123 @@
+package ca.uhn.fhir.jpa.dao;
+
+import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.interceptor.model.RequestPartitionId;
+import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
+import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
+import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
+import ca.uhn.fhir.jpa.model.dao.JpaPid;
+import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
+import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
+import ca.uhn.fhir.jpa.util.MemoryCacheService;
+
+import ca.uhn.fhir.rest.api.server.RequestDetails;
+import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
+
+import ca.uhn.fhir.rest.param.DateRangeParam;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.hl7.fhir.r4.model.Patient;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import static org.mockito.ArgumentMatchers.any;
+
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+
+import static org.mockito.Mockito.when;
+
+import org.mockito.Spy;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class MatchResourceUrlServiceTest {
+	@Spy
+	private JpaStorageSettings myStorageSettings = new JpaStorageSettings();
+	@Spy
+	private MemoryCacheService myMemoryCacheService = new MemoryCacheService(myStorageSettings);
+
+	@Mock
+	private TransactionDetails myTransactionDetails;
+
+	@Mock
+	private RequestDetails myRequestDetails;
+
+	@Mock
+	private DaoRegistry myDaoRegistry;
+
+	@Mock
+	private IFhirResourceDao myFhirResourceDao;
+
+	@Mock
+	private FhirContext myCtx = FhirContext.forR4();
+
+	@Mock
+	private MatchUrlService myMatchUrlSvc;
+
+	@InjectMocks
+	private MatchResourceUrlService<JpaPid> myMatchResourceUrlSvc = new MatchResourceUrlService();
+
+	@BeforeEach
+	public void beforeEach() {
+		myMemoryCacheService.invalidateAllCaches();
+	}
+
+	@Test
+	void testProcessMatchUrlUsingCacheOnly_shouldNotReturnPidsFromWrongPartition() {
+		myStorageSettings.setMatchUrlCacheEnabled(true);
+
+		String matchUrl = "Patient?identifier=test|123";
+		final int partitionId = 1;
+		JpaPid cachedPid = JpaPid.fromId(1L);
+		cachedPid.setPartitionId(partitionId);
+
+		myMatchResourceUrlSvc.matchUrlResolved(myTransactionDetails, "Patient", matchUrl, cachedPid);
+
+		JpaPid pid = myMatchResourceUrlSvc.processMatchUrlUsingCacheOnly("Patient", matchUrl, RequestPartitionId.fromPartitionId(1));
+		assertNotNull(pid);
+		assertThat(pid.getPartitionId()).isEqualTo(partitionId);
+		assertThat(pid.getId()).isEqualTo(1L);
+
+		pid = myMatchResourceUrlSvc.processMatchUrlUsingCacheOnly("Patient", matchUrl, RequestPartitionId.allPartitions());
+		assertNotNull(pid);
+		assertThat(pid.getPartitionId()).isEqualTo(partitionId);
+
+		pid = myMatchResourceUrlSvc.processMatchUrlUsingCacheOnly("Patient", matchUrl, RequestPartitionId.fromPartitionId(2));
+		assertNull(pid);
+
+		pid = myMatchResourceUrlSvc.processMatchUrlUsingCacheOnly("Patient", matchUrl, RequestPartitionId.fromPartitionId(null));
+		assertNull(pid);
+	}
+
+	@Test
+	void testProcessMatchUrl_storesFoundMatchInCache() {
+		myStorageSettings.setMatchUrlCacheEnabled(true);
+
+		String matchUrl = "Patient?identifier=test|123";
+		final int partitionId = 1;
+		JpaPid cachedPid = JpaPid.fromId(1L);
+		cachedPid.setPartitionId(partitionId);
+
+		SearchParameterMap sp = new SearchParameterMap();
+		sp.setLastUpdated(new DateRangeParam().setLowerBound("2024").setUpperBound("2025"));
+
+		when(myDaoRegistry.getResourceDao(Patient.class)).thenReturn(myFhirResourceDao);
+		when(myFhirResourceDao.searchForIds(any(), any(), any())).thenReturn(List.of(cachedPid));
+		when(myMatchUrlSvc.translateMatchUrl(any(), any())).thenReturn(sp);
+
+		myMatchResourceUrlSvc.processMatchUrl(matchUrl, Patient.class, myTransactionDetails, myRequestDetails, RequestPartitionId.fromPartitionId(1));
+
+		JpaPid pid = myMatchResourceUrlSvc.processMatchUrlUsingCacheOnly("Patient", matchUrl, RequestPartitionId.fromPartitionId(1));
+		assertNotNull(pid);
+		assertThat(pid.getPartitionId()).isEqualTo(partitionId);
+		assertThat(pid.getId()).isEqualTo(1L);
+	}
+}

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/imprt2/ConsumeFilesStepR4Test.java

@@ -102,8 +102,7 @@ public void testAlreadyExisting_WithChanges(boolean partitionEnabled) {
 		if (partitionEnabled) {
 			myPartitionSettings.setPartitioningEnabled(true);
 			myPartitionSettings.setIncludePartitionInSearchHashes(true);
-			addCreatePartition(1);
-			addCreatePartition(1);
+			addCreatePartitionNTimes(1, 2);
 		}
 		Patient patient = new Patient();
 		patient.setId("A");

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

@@ -61,17 +61,20 @@ public abstract class BasePartitioningR4Test extends BaseJpaR4SystemTest {
 	public void after() {
 		assertNoRemainingPartitionIds();
 
-		myPartitionSettings.setIncludePartitionInSearchHashes(new PartitionSettings().isIncludePartitionInSearchHashes());
-		myPartitionSettings.setPartitioningEnabled(new PartitionSettings().isPartitioningEnabled());
-		myPartitionSettings.setAllowReferencesAcrossPartitions(new PartitionSettings().getAllowReferencesAcrossPartitions());
-		myPartitionSettings.setDefaultPartitionId(new PartitionSettings().getDefaultPartitionId());
+		PartitionSettings defaultPartitionSettings = new PartitionSettings();
+		JpaStorageSettings defaultStorageSettings = new JpaStorageSettings();
+
+		myPartitionSettings.setIncludePartitionInSearchHashes(defaultPartitionSettings.isIncludePartitionInSearchHashes());
+		myPartitionSettings.setPartitioningEnabled(defaultPartitionSettings.isPartitioningEnabled());
+		myPartitionSettings.setAllowReferencesAcrossPartitions(defaultPartitionSettings.getAllowReferencesAcrossPartitions());
+		myPartitionSettings.setDefaultPartitionId(defaultPartitionSettings.getDefaultPartitionId());
 
 		mySrdInterceptorService.unregisterInterceptorsIf(t -> t instanceof MyReadWriteInterceptor);
 
-		myStorageSettings.setIndexMissingFields(new JpaStorageSettings().getIndexMissingFields());
-		myStorageSettings.setAutoCreatePlaceholderReferenceTargets(new JpaStorageSettings().isAutoCreatePlaceholderReferenceTargets());
-		myStorageSettings.setMassIngestionMode(new JpaStorageSettings().isMassIngestionMode());
-		myStorageSettings.setMatchUrlCacheEnabled(new JpaStorageSettings().getMatchUrlCache());
+		myStorageSettings.setIndexMissingFields(defaultStorageSettings.getIndexMissingFields());
+		myStorageSettings.setAutoCreatePlaceholderReferenceTargets(defaultStorageSettings.isAutoCreatePlaceholderReferenceTargets());
+		myStorageSettings.setMassIngestionMode(defaultStorageSettings.isMassIngestionMode());
+		myStorageSettings.setMatchUrlCacheEnabled(defaultStorageSettings.isMatchUrlCacheEnabled());
 
 		if (myRegisteredSearchParamValidatingInterceptor) {
 			myInterceptorRegistry.unregisterInterceptor(mySearchParamValidatingInterceptor);
@@ -238,6 +241,12 @@ protected void dropForcedIdUniqueConstraint() {
 		myHaveDroppedForcedIdUniqueConstraint = true;
 	}
 
+	protected void addCreatePartitionNTimes(Integer thePartitionId, Integer theNumberOfTimes) {
+		for (int i = 0; i < theNumberOfTimes; i++) {
+			addCreatePartition(thePartitionId, null);
+		}
+	}
+
 	protected void addCreatePartition(Integer thePartitionId) {
 		addCreatePartition(thePartitionId, null);
 	}

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

@@ -1,5 +1,16 @@
 package ca.uhn.fhir.jpa.dao.r4;
 
+import ca.uhn.fhir.util.BundleBuilder;
+
+import org.hl7.fhir.r4.model.Bundle;
+import org.hl7.fhir.r4.model.CodeableConcept;
+import org.hl7.fhir.r4.model.HumanName;
+import org.hl7.fhir.r4.model.Identifier;
+import org.hl7.fhir.r4.model.OperationOutcome;
+import org.hl7.fhir.r4.model.Patient;
+
+import org.hl7.fhir.r4.model.StringType;
+
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import ca.uhn.fhir.jpa.api.dao.PatientEverythingParameters;
@@ -155,4 +166,38 @@ public void testSearch_MultiplePartitions_UseCache() {
 
 	}
 
+	@Test
+	public void testConditionalCreate_withMultiplePartitionsAndMatchUrlCache_NoCachePartitionConflicts() {
+		myStorageSettings.setMatchUrlCacheEnabled(true);
+		myPartitionSettings.setConditionalCreateDuplicateIdentifiersEnabled(true);
+
+		Bundle responseBundle1 = mySystemDao.transaction(mySrd, createPatientWithConditionalUrlOnPartition(1));
+		assertResourceCreated(responseBundle1);
+
+		Bundle responseBundle2 = mySystemDao.transaction(mySrd, createPatientWithConditionalUrlOnPartition(2));
+		assertResourceCreated(responseBundle2);
+	}
+
+	private Bundle createPatientWithConditionalUrlOnPartition(Integer thePartitionId) {
+		BundleBuilder bb = new BundleBuilder(myFhirContext);
+
+		Patient p = new Patient();
+		p.setIdentifier(List.of(new Identifier().setSystem("foo").setValue("bar")));
+		p.setActive(true);
+		p.setName(List.of(new HumanName().setFamily("ABC").setGiven(List.of(new StringType("DEF")))));
+		bb.addTransactionUpdateEntry(p, "Patient?identifier=foo|bar");
+		addCreatePartitionNTimes(thePartitionId, 3);
+
+		return (Bundle) bb.getBundle();
+	}
+
+	private static void assertResourceCreated(Bundle responseBundle1) {
+		assertThat(responseBundle1.getEntry()).hasSize(1);
+		Bundle.BundleEntryResponseComponent response1 = responseBundle1.getEntryFirstRep().getResponse();
+		assertThat(response1.getStatus()).isEqualTo("201 Created");
+		OperationOutcome oo1 = (OperationOutcome) response1.getOutcome();
+		assertThat(oo1.getIssue()).hasSize(1);
+		CodeableConcept details1 = oo1.getIssueFirstRep().getDetails();
+		assertThat(details1.getCodingFirstRep().getCode()).isEqualTo("SUCCESSFUL_UPDATE_NO_CONDITIONAL_MATCH");
+	}
 }