Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Match URL cache should take partition into account #6768

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/util/BundleBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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."
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,11 @@ protected IDeleteExpungeJobSubmitter getDeleteExpungeJobSubmitter() {
return myDeleteExpungeJobSubmitter;
}

@Override
protected IRequestPartitionHelperSvc getRequestPartitionHelperService() {
return myRequestPartitionHelperService;
}

/**
* @deprecated Use {@link #create(T, RequestDetails)} instead
*/
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package ca.uhn.fhir.jpa.batch2;

import static ca.uhn.fhir.jpa.util.ConcurrencyTestUtil.executeFutures;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import ca.uhn.fhir.batch2.api.IJobCoordinator;
import ca.uhn.fhir.batch2.model.JobInstance;
Expand Down Expand Up @@ -78,7 +78,7 @@ void afterEach() {
}

@Test
public void testGroupBulkExportNotInGroup_DoesNotShowUp() throws InterruptedException, ExecutionException {
public void testGroupBulkExportNotInGroup_DoesNotShowUp() {
duAbuseTest(100);
}

Expand All @@ -93,7 +93,7 @@ public void testGroupBulkExportNotInGroup_DoesNotShowUp() throws InterruptedExce
*/
@Test
@Disabled("for manual debugging")
public void testNonStopAbuseBatch2BulkExportStressTest() throws InterruptedException, ExecutionException {
public void testNonStopAbuseBatch2BulkExportStressTest() {
duAbuseTest(Integer.MAX_VALUE);
}

Expand Down Expand Up @@ -190,35 +190,6 @@ private void duAbuseTest(int taskExecutions) {
ourLog.info("Finished task execution");
}

private void executeFutures(CompletionService<Boolean> theCompletionService, int theTotal) {
List<String> errors = new ArrayList<>();
int count = 0;

while (count + errors.size() < theTotal) {
try {
Future<Boolean> future = theCompletionService.take();
boolean r = future.get();
assertTrue(r);
count++;
} catch (Exception ex) {
// we will run all the threads to completion, even if we have errors;
// this is so we don't have background threads kicking around with
// partial changes.
// we either do this, or shutdown the completion service in an
// "inelegant" manner, dropping all threads (which we aren't doing)
ourLog.error("Failed after checking " + count + " futures");
String[] frames = ExceptionUtils.getRootCauseStackTrace(ex);
errors.add(ex + "\n" + String.join("\n ", frames));
}
}

if (!errors.isEmpty()) {
fail(String.format("Failed to execute futures. Found %d errors :\n", errors.size())
+ String.join(", ", errors));
}
}


private void verifyBulkExportResults(String theInstanceId, List<String> theContainedList, List<String> theExcludedList) {
// Iterate over the files
JobInstance jobInfo = myJobCoordinator.getInstance(theInstanceId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ public void testAlreadyExisting_WithChanges(boolean partitionEnabled) {
if (partitionEnabled) {
myPartitionSettings.setPartitioningEnabled(true);
myPartitionSettings.setIncludePartitionInSearchHashes(true);
addCreatePartition(1);
addCreatePartition(1);
addNextTargetPartitionNTimesForCreate(1, 2);
}
Patient patient = new Patient();
patient.setId("A");
Expand Down Expand Up @@ -132,8 +131,8 @@ public void testAlreadyExisting_WithChanges(boolean partitionEnabled) {
myMemoryCacheService.invalidateAllCaches();
myCaptureQueriesListener.clear();
if (partitionEnabled) {
addReadPartition(1);
addReadPartition(1);
addNextTargetPartitionsForRead(1);
addNextTargetPartitionsForRead(1);
mySvc.storeResources(resources, myRequestPartitionId);
} else {
mySvc.storeResources(resources, null);
Expand Down
Loading
Loading