Enforce limit on the number of resources resolved by a deleteByUrl call. (#5689)
* Enforce limit on the number of resources resolved by a deleteByUrl call. * Add changelog and TODO comment. * Code review suggestion.
This commit is contained in:
Luke deGruchy
GitHub
parent
43f1e4b2dd
commit
a7eceda5d7
|
|
@ -89,6 +89,7 @@ ca.uhn.fhir.jpa.dao.BaseHapiFhirDao.invalidMatchUrlInvalidResourceType=Invalid m
|
|||
ca.uhn.fhir.jpa.dao.BaseStorageDao.invalidMatchUrlNoMatches=Invalid match URL "{0}" - No resources match this search
|
||||
ca.uhn.fhir.jpa.dao.BaseStorageDao.inlineMatchNotSupported=Inline match URLs are not supported on this server. Cannot process reference: "{0}"
|
||||
ca.uhn.fhir.jpa.dao.BaseStorageDao.transactionOperationWithMultipleMatchFailure=Failed to {0} resource with match URL "{1}" because this search matched {2} resources
|
||||
ca.uhn.fhir.jpa.dao.BaseStorageDao.deleteByUrlThresholdExceeded=Failed to DELETE resources with match URL "{0}" because the resolved number of resources: {1} exceeds the threshold of {2}
|
||||
ca.uhn.fhir.jpa.dao.BaseStorageDao.transactionOperationWithIdNotMatchFailure=Failed to {0} resource with match URL "{1}" because the matching resource does not match the provided ID
|
||||
ca.uhn.fhir.jpa.dao.BaseHapiFhirDao.transactionOperationFailedNoId=Failed to {0} resource in transaction because no ID was provided
|
||||
ca.uhn.fhir.jpa.dao.BaseHapiFhirDao.transactionOperationFailedUnknownId=Failed to {0} resource in transaction because no resource could be found with ID {1}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,7 @@
|
|||
---
|
||||
type: fix
|
||||
issue: 5690
|
||||
title: "Previously, a DELETE on a specific URL search string would always attempt to delete no matter the number of
|
||||
resolved resources.
|
||||
This has been fixed by adding a storage setting to enforce a threshold for resolved resources, above which
|
||||
the DELETE operation will fail to execute with HAPI-2496."
|
||||
|
|
@ -852,7 +852,10 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
|
|||
return deleteExpunge(theUrl, theRequest);
|
||||
}
|
||||
|
||||
return myTransactionService.execute(theRequest, transactionDetails, tx -> {
|
||||
return myTransactionService
|
||||
.withRequest(theRequest)
|
||||
.withTransactionDetails(transactionDetails)
|
||||
.execute(tx -> {
|
||||
DeleteConflictList deleteConflicts = new DeleteConflictList();
|
||||
DeleteMethodOutcome outcome = deleteByUrl(theUrl, deleteConflicts, theRequest, transactionDetails);
|
||||
DeleteConflictUtil.validateDeleteConflictsEmptyOrThrowException(getContext(), deleteConflicts);
|
||||
|
|
@ -872,10 +875,10 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
|
|||
@Nonnull TransactionDetails theTransactionDetails) {
|
||||
validateDeleteEnabled();
|
||||
|
||||
return myTransactionService.execute(
|
||||
theRequestDetails,
|
||||
theTransactionDetails,
|
||||
tx -> doDeleteByUrl(theUrl, deleteConflicts, theTransactionDetails, theRequestDetails));
|
||||
return myTransactionService
|
||||
.withRequest(theRequestDetails)
|
||||
.withTransactionDetails(theTransactionDetails)
|
||||
.execute(tx -> doDeleteByUrl(theUrl, deleteConflicts, theTransactionDetails, theRequestDetails));
|
||||
}
|
||||
|
||||
@Nonnull
|
||||
|
|
@ -902,6 +905,19 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
|
|||
theUrl,
|
||||
resourceIds.size()));
|
||||
}
|
||||
// TODO: LD: There is a still a bug on slow deletes: https://github.com/hapifhir/hapi-fhir/issues/5675
|
||||
final long threshold = getStorageSettings().getRestDeleteByUrlResourceIdThreshold();
|
||||
if (resourceIds.size() > threshold) {
|
||||
throw new PreconditionFailedException(Msg.code(2496)
|
||||
+ getContext()
|
||||
.getLocalizer()
|
||||
.getMessageSanitized(
|
||||
BaseStorageDao.class,
|
||||
"deleteByUrlThresholdExceeded",
|
||||
theUrl,
|
||||
resourceIds.size(),
|
||||
threshold));
|
||||
}
|
||||
}
|
||||
|
||||
return deletePidList(theUrl, resourceIds, deleteConflicts, theRequestDetails, theTransactionDetails);
|
||||
|
|
|
|||
|
|
@ -6,55 +6,76 @@ import ca.uhn.fhir.batch2.jobs.parameters.UrlPartitioner;
|
|||
import ca.uhn.fhir.batch2.jobs.reindex.ReindexJobParameters;
|
||||
import ca.uhn.fhir.batch2.model.JobInstanceStartRequest;
|
||||
import ca.uhn.fhir.context.FhirContext;
|
||||
import ca.uhn.fhir.context.RuntimeResourceDefinition;
|
||||
import ca.uhn.fhir.i18n.Msg;
|
||||
import ca.uhn.fhir.interceptor.model.RequestPartitionId;
|
||||
import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
|
||||
import ca.uhn.fhir.jpa.api.dao.IFhirSystemDao;
|
||||
import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
|
||||
import ca.uhn.fhir.jpa.api.model.DeleteConflictList;
|
||||
import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome;
|
||||
import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
|
||||
import ca.uhn.fhir.jpa.dao.tx.HapiTransactionService;
|
||||
import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService;
|
||||
import ca.uhn.fhir.jpa.delete.DeleteConflictService;
|
||||
import ca.uhn.fhir.jpa.model.dao.JpaPid;
|
||||
import ca.uhn.fhir.jpa.model.entity.ForcedId;
|
||||
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
|
||||
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.ResourceSearch;
|
||||
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
|
||||
import ca.uhn.fhir.jpa.svc.MockHapiTransactionService;
|
||||
import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
|
||||
import ca.uhn.fhir.rest.api.server.RequestDetails;
|
||||
import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
|
||||
import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
|
||||
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
|
||||
import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException;
|
||||
import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
|
||||
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
|
||||
import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
|
||||
import com.google.common.collect.Lists;
|
||||
import jakarta.persistence.EntityManager;
|
||||
import org.hl7.fhir.instance.model.api.IIdType;
|
||||
import org.hl7.fhir.r4.model.IdType;
|
||||
import org.hl7.fhir.r4.model.Patient;
|
||||
import org.junit.jupiter.api.Assertions;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Nested;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.extension.ExtendWith;
|
||||
import org.junit.jupiter.params.ParameterizedTest;
|
||||
import org.junit.jupiter.params.provider.Arguments;
|
||||
import org.junit.jupiter.params.provider.MethodSource;
|
||||
import org.mockito.ArgumentCaptor;
|
||||
import org.mockito.ArgumentMatchers;
|
||||
import org.mockito.Captor;
|
||||
import org.mockito.InjectMocks;
|
||||
import org.mockito.Mock;
|
||||
import org.mockito.Mockito;
|
||||
import org.mockito.junit.jupiter.MockitoExtension;
|
||||
import org.mockito.stubbing.Answer;
|
||||
import org.springframework.context.ApplicationContext;
|
||||
import org.springframework.transaction.TransactionStatus;
|
||||
import org.springframework.transaction.support.TransactionCallback;
|
||||
|
||||
import jakarta.persistence.EntityManager;
|
||||
import java.util.Collections;
|
||||
import java.util.List;
|
||||
import java.util.Set;
|
||||
import java.util.stream.Collectors;
|
||||
import java.util.stream.Stream;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
||||
import static org.junit.jupiter.api.Assertions.fail;
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.ArgumentMatchers.anyLong;
|
||||
import static org.mockito.ArgumentMatchers.isNotNull;
|
||||
import static org.mockito.Mockito.doReturn;
|
||||
import static org.mockito.Mockito.lenient;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.spy;
|
||||
import static org.mockito.Mockito.verify;
|
||||
import static org.mockito.Mockito.when;
|
||||
|
||||
|
|
@ -76,6 +97,9 @@ class BaseHapiFhirResourceDaoTest {
|
|||
@Mock
|
||||
private IJobCoordinator myJobCoordinator;
|
||||
|
||||
@Mock
|
||||
private IJpaStorageResourceParser myJpaStorageResourceParser;
|
||||
|
||||
@Mock
|
||||
private UrlPartitioner myUrlPartitioner;
|
||||
|
||||
|
|
@ -91,6 +115,23 @@ class BaseHapiFhirResourceDaoTest {
|
|||
@Mock
|
||||
private ISearchBuilder<JpaPid> myISearchBuilder;
|
||||
|
||||
@Mock
|
||||
private MatchUrlService myMatchUrlService;
|
||||
@Mock
|
||||
private MatchResourceUrlService<JpaPid> myMatchResourceUrlService;
|
||||
|
||||
@Mock
|
||||
private HapiTransactionService myTransactionService;
|
||||
|
||||
@Mock
|
||||
private DeleteConflictService myDeleteConflictService;
|
||||
|
||||
@Mock
|
||||
private IFhirSystemDao<?, ?> mySystemDao;
|
||||
|
||||
@Mock
|
||||
private ResourceSearchUrlSvc myResourceSearchUrlSvc;
|
||||
|
||||
@Captor
|
||||
private ArgumentCaptor<SearchParameterMap> mySearchParameterMapCaptor;
|
||||
|
||||
|
|
@ -100,6 +141,8 @@ class BaseHapiFhirResourceDaoTest {
|
|||
@InjectMocks
|
||||
private TestResourceDao mySvc;
|
||||
|
||||
private TestResourceDao mySpiedSvc;
|
||||
|
||||
@BeforeEach
|
||||
public void init() {
|
||||
// set our context
|
||||
|
|
@ -108,6 +151,7 @@ class BaseHapiFhirResourceDaoTest {
|
|||
// the individual tests will have to start
|
||||
// by calling setup themselves
|
||||
mySvc.setContext(myFhirContext);
|
||||
mySpiedSvc = spy(mySvc);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -198,7 +242,7 @@ class BaseHapiFhirResourceDaoTest {
|
|||
)).thenReturn(jpaPid);
|
||||
when(myEntityManager.find(
|
||||
any(Class.class),
|
||||
Mockito.anyLong()
|
||||
anyLong()
|
||||
)).thenReturn(entity);
|
||||
// we don't stub myConfig.getResourceClientIdStrategy()
|
||||
// because even a null return isn't ANY...
|
||||
|
|
@ -288,6 +332,97 @@ class BaseHapiFhirResourceDaoTest {
|
|||
);
|
||||
}
|
||||
|
||||
@Nested
|
||||
class DeleteThresholds {
|
||||
private static final String URL = "Patient?_lastUpdated=gt2024-01-01";
|
||||
private static final RequestDetails REQUEST = new SystemRequestDetails();
|
||||
private static final DeleteMethodOutcome EXPECTED_DELETE_OUTCOME = new DeleteMethodOutcome();
|
||||
|
||||
@BeforeEach
|
||||
void beforeEach() {
|
||||
when(myStorageSettings.isDeleteEnabled()).thenReturn(true);
|
||||
when(myMatchUrlService.getResourceSearch(URL))
|
||||
.thenReturn(new ResourceSearch(mock(RuntimeResourceDefinition.class), SearchParameterMap.newSynchronous(), RequestPartitionId.allPartitions()));
|
||||
|
||||
// mocks for transaction handling:
|
||||
final IHapiTransactionService.IExecutionBuilder mockExecutionBuilder = mock(IHapiTransactionService.IExecutionBuilder.class);
|
||||
when(mockExecutionBuilder.withTransactionDetails(any(TransactionDetails.class))).thenReturn(mockExecutionBuilder);
|
||||
when(myTransactionService.withRequest(REQUEST)).thenReturn(mockExecutionBuilder);
|
||||
final Answer<DeleteMethodOutcome> answer = theInvocationOnMock -> {
|
||||
final TransactionCallback<DeleteMethodOutcome> arg = theInvocationOnMock.getArgument(0);
|
||||
return arg.doInTransaction(mock(TransactionStatus.class));
|
||||
};
|
||||
when(mockExecutionBuilder.execute(ArgumentMatchers.<TransactionCallback<DeleteMethodOutcome>>any()))
|
||||
.thenAnswer(answer);
|
||||
}
|
||||
|
||||
@ParameterizedTest
|
||||
@MethodSource("thresholdsAndResourceIds_Pass")
|
||||
void deleteByUrlConsiderThresholdUnder_Pass(long theThreshold, Set<Long> theResourceIds) {
|
||||
if (theResourceIds.size() > 1) {
|
||||
when(myStorageSettings.isAllowMultipleDelete()).thenReturn(true);
|
||||
when(myStorageSettings.getRestDeleteByUrlResourceIdThreshold()).thenReturn(theThreshold);
|
||||
}
|
||||
|
||||
doReturn(EXPECTED_DELETE_OUTCOME).when(mySpiedSvc).deletePidList(any(), any(), any(), any(), any());
|
||||
|
||||
handleExpectedResourceIds(theResourceIds);
|
||||
|
||||
final DeleteMethodOutcome deleteMethodOutcome = mySpiedSvc.deleteByUrl(URL, REQUEST);
|
||||
assertEquals(EXPECTED_DELETE_OUTCOME, deleteMethodOutcome);
|
||||
}
|
||||
|
||||
@ParameterizedTest
|
||||
@MethodSource("thresholdsAndResourceIds_Fail")
|
||||
void deleteByUrlConsiderThreshold_Over_Fail(long theThreshold, Set<Long> theResourceIds) {
|
||||
when(myStorageSettings.isAllowMultipleDelete()).thenReturn(true);
|
||||
when(myStorageSettings.getRestDeleteByUrlResourceIdThreshold()).thenReturn(theThreshold);
|
||||
|
||||
final Set<JpaPid> expectedResourceIds = handleExpectedResourceIds(theResourceIds);
|
||||
|
||||
try {
|
||||
mySpiedSvc.deleteByUrl(URL, REQUEST);
|
||||
fail();
|
||||
} catch (PreconditionFailedException exception) {
|
||||
assertEquals(String.format("HAPI-2496: Failed to DELETE resources with match URL \"Patient?_lastUpdated=gt2024-01-01\" because the resolved number of resources: %s exceeds the threshold of %s", expectedResourceIds.size(), theThreshold), exception.getMessage());
|
||||
}
|
||||
}
|
||||
|
||||
private Set<JpaPid> handleExpectedResourceIds(Set<Long> theResourceIds) {
|
||||
final Set<JpaPid> expectedResourceIds = theResourceIds.stream().map(JpaPid::fromId).collect(Collectors.toUnmodifiableSet());
|
||||
when(myMatchResourceUrlService.search(any(), any(), any(), any())).thenReturn(expectedResourceIds);
|
||||
return expectedResourceIds;
|
||||
}
|
||||
|
||||
static Stream<Arguments> thresholdsAndResourceIds_Pass() {
|
||||
return Stream.of(
|
||||
Arguments.of(0, Collections.emptySet()),
|
||||
Arguments.of(1, Collections.emptySet()),
|
||||
Arguments.of(2, Collections.emptySet()),
|
||||
Arguments.of(3, Collections.emptySet()),
|
||||
Arguments.of(4, Collections.emptySet()),
|
||||
Arguments.of(5, Collections.emptySet()),
|
||||
Arguments.of(1, Set.of(1L)),
|
||||
Arguments.of(2, Set.of(1L)),
|
||||
Arguments.of(3, Set.of(1L)),
|
||||
Arguments.of(4, Set.of(1L)),
|
||||
Arguments.of(5, Set.of(1L)),
|
||||
Arguments.of(4, Set.of(1L,2L,3L)),
|
||||
Arguments.of(5, Set.of(1L,2L,3L))
|
||||
);
|
||||
}
|
||||
|
||||
static Stream<Arguments> thresholdsAndResourceIds_Fail() {
|
||||
return Stream.of(
|
||||
Arguments.of(0, Set.of(1L,2L)),
|
||||
Arguments.of(1, Set.of(1L,2L)),
|
||||
Arguments.of(0, Set.of(1L,2L,3L)),
|
||||
Arguments.of(1, Set.of(1L,2L,3L)),
|
||||
Arguments.of(2, Set.of(1L,2L,3L))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
static class TestResourceDao extends BaseHapiFhirResourceDao<Patient> {
|
||||
|
||||
@Override
|
||||
|
|
|
|||
|
|
@ -110,6 +110,7 @@ public class JpaStorageSettings extends StorageSettings {
|
|||
private static final Integer DEFAULT_INTERNAL_SYNCHRONOUS_SEARCH_SIZE = 10000;
|
||||
|
||||
private static final boolean DEFAULT_PREVENT_INVALIDATING_CONDITIONAL_MATCH_CRITERIA = false;
|
||||
private static final long DEFAULT_REST_DELETE_BY_URL_RESOURCE_ID_THRESHOLD = 10000;
|
||||
|
||||
/**
|
||||
* Do not change default of {@code 0}!
|
||||
|
|
@ -344,6 +345,13 @@ public class JpaStorageSettings extends StorageSettings {
|
|||
private boolean myPreventInvalidatingConditionalMatchCriteria =
|
||||
DEFAULT_PREVENT_INVALIDATING_CONDITIONAL_MATCH_CRITERIA;
|
||||
|
||||
/**
|
||||
* This setting helps to enforce a threshold in number of resolved resources for DELETE by URL REST calls
|
||||
*
|
||||
* @since 7.2.0
|
||||
*/
|
||||
private long myRestDeleteByUrlResourceIdThreshold = DEFAULT_REST_DELETE_BY_URL_RESOURCE_ID_THRESHOLD;
|
||||
|
||||
/**
|
||||
* Constructor
|
||||
*/
|
||||
|
|
@ -2427,6 +2435,14 @@ public class JpaStorageSettings extends StorageSettings {
|
|||
return myPreventInvalidatingConditionalMatchCriteria;
|
||||
}
|
||||
|
||||
public long getRestDeleteByUrlResourceIdThreshold() {
|
||||
return myRestDeleteByUrlResourceIdThreshold;
|
||||
}
|
||||
|
||||
public void setRestDeleteByUrlResourceIdThreshold(long theRestDeleteByUrlResourceIdThreshold) {
|
||||
myRestDeleteByUrlResourceIdThreshold = theRestDeleteByUrlResourceIdThreshold;
|
||||
}
|
||||
|
||||
public enum StoreMetaSourceInformationEnum {
|
||||
NONE(false, false),
|
||||
SOURCE_URI(true, false),
|
||||
|
|
|
|||
Loading…
Reference in New Issue