3283 delete will not return a 404 if resource is not found (#3284)

* 3283 delete will not return a 404 if resource is not found

* minor tweaks

* checking if tests fail due to asynchronicity

* 3283 added integration test

* cleanup

* review fixes

* changed id names

* cleanup

Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-MacBook-Pro.local>
This commit is contained in:
TipzCM 2022-01-18 13:07:26 -05:00 committed by GitHub
parent 9f7c454c6b
commit f0a9b33671
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 191 additions and 17 deletions

View File

@ -76,6 +76,7 @@ public class HeaderPassthroughOptionTest {
@Test
public void oneHeader() throws Exception {
writeConceptAndHierarchyFiles();
String[] args = new String[] {
"-v", FHIR_VERSION,
"-m", "SNAPSHOT",
@ -101,6 +102,7 @@ public class HeaderPassthroughOptionTest {
@Test
public void twoHeadersSameKey() throws Exception {
writeConceptAndHierarchyFiles();
final String headerValue2 = "test header value-2";
String[] args = new String[] {
@ -131,6 +133,7 @@ public class HeaderPassthroughOptionTest {
@Test
public void twoHeadersDifferentKeys() throws Exception {
writeConceptAndHierarchyFiles();
final String headerKey2 = "test-header-key-2";
final String headerValue2 = "test header value-2";

View File

@ -0,0 +1,4 @@
---
type: fix
issue: 3283
title: "Calling delete on a non-existent resource should not return a 404 (not found)."

View File

@ -461,32 +461,62 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
});
}
/**
* Creates a base method outcome for a delete request for the provided ID.
*
* Additional information may be set on the outcome.
*
* @param theId - the id of the object being deleted. Eg: Patient/123
*/
private DaoMethodOutcome createMethodOutcomeForDelete(String theId) {
DaoMethodOutcome outcome = new DaoMethodOutcome();
IIdType id = getContext().getVersion().newIdType();
id.setValue(theId);
outcome.setId(id);
IBaseOperationOutcome oo = OperationOutcomeUtil.newInstance(getContext());
String message = getContext().getLocalizer().getMessage(BaseStorageDao.class, "successfulDeletes", 1, 0);
String severity = "information";
String code = "informational";
OperationOutcomeUtil.addIssue(getContext(), oo, severity, message, null, code);
outcome.setOperationOutcome(oo);
return outcome;
}
@Override
public DaoMethodOutcome delete(IIdType theId, DeleteConflictList theDeleteConflicts, RequestDetails theRequestDetails, @Nonnull TransactionDetails theTransactionDetails) {
public DaoMethodOutcome delete(IIdType theId,
DeleteConflictList theDeleteConflicts,
RequestDetails theRequestDetails,
@Nonnull TransactionDetails theTransactionDetails) {
validateIdPresentForDelete(theId);
validateDeleteEnabled();
final ResourceTable entity = readEntityLatestVersion(theId, theRequestDetails, theTransactionDetails);
final ResourceTable entity;
try {
entity = readEntityLatestVersion(theId, theRequestDetails, theTransactionDetails);
} catch (ResourceNotFoundException ex) {
// we don't want to throw 404s.
// if not found, return an outcome anyways.
// Because no object actually existed, we'll
// just set the id and nothing else
DaoMethodOutcome outcome = createMethodOutcomeForDelete(theId.getValue());
return outcome;
}
if (theId.hasVersionIdPart() && Long.parseLong(theId.getVersionIdPart()) != entity.getVersion()) {
throw new ResourceVersionConflictException("Trying to delete " + theId + " but this is not the current version");
}
// Don't delete again if it's already deleted
if (entity.getDeleted() != null) {
DaoMethodOutcome outcome = new DaoMethodOutcome().setPersistentId(new ResourcePersistentId(entity.getResourceId()));
DaoMethodOutcome outcome = createMethodOutcomeForDelete(entity.getIdDt().getValue());
// used to exist, so we'll set the persistent id
outcome.setPersistentId(new ResourcePersistentId(entity.getResourceId()));
outcome.setEntity(entity);
IIdType id = getContext().getVersion().newIdType();
id.setValue(entity.getIdDt().getValue());
outcome.setId(id);
IBaseOperationOutcome oo = OperationOutcomeUtil.newInstance(getContext());
String message = getContext().getLocalizer().getMessage(BaseStorageDao.class, "successfulDeletes", 1, 0);
String severity = "information";
String code = "informational";
OperationOutcomeUtil.addIssue(getContext(), oo, severity, message, null, code);
outcome.setOperationOutcome(oo);
return outcome;
}

View File

@ -1,20 +1,78 @@
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.DaoConfig;
import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
import ca.uhn.fhir.jpa.api.model.DeleteConflictList;
import ca.uhn.fhir.jpa.dao.index.IdHelperService;
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.partition.SystemRequestDetails;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
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.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;
import javax.persistence.EntityManager;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;
@ExtendWith(MockitoExtension.class)
class BaseHapiFhirResourceDaoTest {
TestResourceDao mySvc = new TestResourceDao();
@Mock
private IRequestPartitionHelperSvc myRequestPartitionHelperSvc;
@Mock
private IdHelperService myIdHelperService;
@Mock
private EntityManager myEntityManager;
@Mock
private DaoConfig myConfig;
// we won't inject this
private FhirContext myFhirContext = FhirContext.forR4Cached();
@InjectMocks
private TestResourceDao mySvc;
@BeforeEach
public void init() {
// set our context
// NB: if other tests need to
// have access to resourcetype/name
// the individual tests will have to start
// by calling setup themselves
mySvc.setContext(myFhirContext);
}
/**
* To be called for tests that require additional
* setup
* @param clazz
*/
private void setup(Class clazz) {
mySvc.setResourceType(clazz);
mySvc.postConstruct();
}
@Test
public void validateResourceIdCreation_asSystem() {
@ -62,6 +120,50 @@ class BaseHapiFhirResourceDaoTest {
// no exception is thrown
}
@Test
public void delete_nonExistentEntity_doesNotThrow404() {
// initialize our class
setup(Patient.class);
// setup
IIdType id = new IdType("Patient/123"); // id part is only numbers
DeleteConflictList deleteConflicts = new DeleteConflictList();
RequestDetails requestDetails = new SystemRequestDetails();
TransactionDetails transactionDetails = new TransactionDetails();
RequestPartitionId partitionId = Mockito.mock(RequestPartitionId.class);
ResourcePersistentId resourcePersistentId = new ResourcePersistentId("Patient", 1l);
ResourceTable entity = new ResourceTable();
entity.setForcedId(new ForcedId());
// mock
Mockito.when(myRequestPartitionHelperSvc.determineReadPartitionForRequestForRead(
Mockito.any(RequestDetails.class),
Mockito.anyString(),
Mockito.any(IIdType.class)
)).thenReturn(partitionId);
Mockito.when(myIdHelperService.resolveResourcePersistentIds(
Mockito.any(RequestPartitionId.class),
Mockito.anyString(),
Mockito.anyString()
)).thenReturn(resourcePersistentId);
Mockito.when(myEntityManager.find(
Mockito.any(Class.class),
Mockito.anyString()
)).thenReturn(entity);
// we don't stub myConfig.getResourceClientIdStrategy()
// because even a null return isn't ANY...
// if this changes, though, we will have to stub it.
// but for now, Mockito will complain, so we'll leave it out
// test
DaoMethodOutcome outcome = mySvc.delete(id, deleteConflicts, requestDetails, transactionDetails);
// verify
Assertions.assertNotNull(outcome);
Assertions.assertEquals(id.getValue(), outcome.getId().getValue());
}
static class TestResourceDao extends BaseHapiFhirResourceDao<Patient> {
private final DaoConfig myDaoConfig = new DaoConfig();

View File

@ -135,6 +135,7 @@ import org.hl7.fhir.r4.model.UnsignedIntType;
import org.hl7.fhir.r4.model.ValueSet;
import org.hl7.fhir.utilities.xhtml.XhtmlNode;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
@ -218,9 +219,9 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test {
mySearchCoordinatorSvcRaw.setSyncSizeForUnitTests(SearchCoordinatorSvcImpl.DEFAULT_SYNC_SIZE);
mySearchCoordinatorSvcRaw.setNeverUseLocalSearchForUnitTests(false);
mySearchCoordinatorSvcRaw.cancelAllActiveSearches();
myDaoConfig.getModelConfig().setNormalizedQuantitySearchLevel(NormalizedQuantitySearchLevel.NORMALIZED_QUANTITY_SEARCH_NOT_SUPPORTED);
myDaoConfig.getModelConfig().setNormalizedQuantitySearchLevel(NormalizedQuantitySearchLevel.NORMALIZED_QUANTITY_SEARCH_NOT_SUPPORTED);
myClient.unregisterInterceptor(myCapturingInterceptor);
myClient.unregisterInterceptor(myCapturingInterceptor);
}
@BeforeEach
@ -6104,6 +6105,40 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test {
}
@Test
public void testDeleteNonExistentResourceReturns200() throws IOException {
HttpDelete delete = new HttpDelete(ourServerBase + "/Patient/someIdHereThatIsUnique");
try (CloseableHttpResponse response = ourHttpClient.execute(delete)) {
Assertions.assertEquals(200, response.getStatusLine().getStatusCode());
}
}
@Test
public void testDeleteSameResourceTwice() throws IOException {
String id = "mySecondUniqueId";
Patient p = new Patient();
p.setId(id);
String encoded = myFhirCtx.newJsonParser().encodeResourceToString(p);
HttpPut put = new HttpPut(ourServerBase + "/Patient/" + id);
put.setEntity(new StringEntity(encoded, ContentType.create("application/fhir+json", "UTF-8")));
try (CloseableHttpResponse response = ourHttpClient.execute(put)) {
assertEquals(201, response.getStatusLine().getStatusCode());
}
HttpDelete delete = new HttpDelete(ourServerBase + "/Patient/" + id);
for (int i = 0; i < 2; i++) {
// multiple deletes of the same resource
// should always succeed
try (CloseableHttpResponse response = ourHttpClient.execute(delete)) {
Assertions.assertEquals(200, response.getStatusLine().getStatusCode());
}
}
}
@Test
public void testUpdateWithClientSuppliedIdWhichDoesntExist() {
Patient p1 = new Patient();