honeymoose
/
hapi-fhir
mirror of https://github.com/hapifhir/hapi-fhir.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

reported issue with proposed fix that breaks tests (#1243) 

* reported issue with proposed fix that breaks tests

* Fix spelling

* Merge my changes into this

* Correctly check referential integrity on deletes

* Add one more test
This commit is contained in:
Ken Stevens 2019-06-04 20:40:20 -04:00 committed by James Agnew
parent f6439cb270
commit 5752a6e2b3
5 changed files with 314 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.dao;
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
*
* http://www.apache.org/licenses/LICENSE-2.0
*
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@ -508,6 +508,7 @@ public class TransactionProcessor<BUNDLE extends IBaseBundle, BUNDLEENTRY> {
Map<BUNDLEENTRY, ResourceTable> entriesToProcess = new IdentityHashMap<>();
Set<ResourceTable> nonUpdatedEntities = new HashSet<>();
Set<ResourceTable> updatedEntities = new HashSet<>();
List<IBaseResource> updatedResources = new ArrayList<>();
Map<String, Class<? extends IBaseResource>> conditionalRequestUrls = new HashMap<>();
/*
@ -722,6 +723,9 @@ public class TransactionProcessor<BUNDLE extends IBaseBundle, BUNDLEENTRY> {
if (outcome.getCreated() == Boolean.FALSE
|| (outcome.getCreated() == Boolean.TRUE && outcome.getId().getVersionIdPartAsLong() > 1)) {
updatedEntities.add(outcome.getEntity());
if (outcome.getResource() != null) {
updatedResources.add(outcome.getResource());
}
}
handleTransactionCreateOrUpdateOutcome(theIdSubstitutions, theIdToPersistedOutcome, nextResourceId, outcome, nextRespEntry, resourceType, res, theRequestDetails);
@ -744,9 +748,41 @@ public class TransactionProcessor<BUNDLE extends IBaseBundle, BUNDLEENTRY> {
* was also deleted as a part of this transaction, which is why we check this now at the
* end.
*/
for (Iterator<DeleteConflict> iter = deleteConflicts.iterator(); iter.hasNext(); ) {
DeleteConflict nextDeleteConflict = iter.next();
deleteConflicts.removeIf(next ->
deletedResources.contains(next.getTargetId().toUnqualifiedVersionless().getValue()));
/*
* If we have a conflict, it means we can't delete Resource/A because
* Resource/B has a reference to it. We'll ignore that conflict though
* if it turns out we're also deleting Resource/B in this transaction.
*/
if (deletedResources.contains(nextDeleteConflict.getSourceId().toUnqualifiedVersionless().getValue())) {
iter.remove();
continue;
}
/*
* And then, this is kind of a last ditch check. It's also ok to delete
* Resource/A if Resource/B isn't being deleted, but it is being UPDATED
* in this transaction, and the updated version of it has no references
* to Resource/A any more.
*/
String sourceId = nextDeleteConflict.getSourceId().toUnqualifiedVersionless().getValue();
String targetId = nextDeleteConflict.getTargetId().toUnqualifiedVersionless().getValue();
Optional<IBaseResource> updatedSource = updatedResources
.stream()
.filter(t -> sourceId.equals(t.getIdElement().toUnqualifiedVersionless().getValue()))
.findFirst();
if (updatedSource.isPresent()) {
List<ResourceReferenceInfo> referencesInSource = myContext.newTerser().getAllResourceReferences(updatedSource.get());
boolean sourceStillReferencesTarget = referencesInSource
.stream()
.anyMatch(t-> targetId.equals(t.getResourceReference().getReferenceElement().toUnqualifiedVersionless().getValue()));
if (!sourceStillReferencesTarget) {
iter.remove();
}
}
}
myDao.validateDeleteConflictsEmptyOrThrowException(deleteConflicts);
/*

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

@ -3,18 +3,16 @@ package ca.uhn.fhir.jpa.dao.r4;
import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao;
import ca.uhn.fhir.jpa.dao.DaoConfig;
import ca.uhn.fhir.jpa.model.entity.*;
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import ca.uhn.fhir.jpa.provider.SystemProviderDstu2Test;
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import ca.uhn.fhir.model.api.ResourceMetadataKeyEnum;
import ca.uhn.fhir.model.primitive.IdDt;
import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
import ca.uhn.fhir.rest.api.server.IBundleProvider;
import ca.uhn.fhir.rest.param.ReferenceParam;
import ca.uhn.fhir.rest.param.StringParam;
import ca.uhn.fhir.rest.param.TokenParam;
import ca.uhn.fhir.rest.server.exceptions.*;
import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor.ActionRequestDetails;
import ca.uhn.fhir.util.TestUtil;
import org.apache.commons.io.IOUtils;
import org.hamcrest.Matchers;
@ -25,7 +23,6 @@ import org.hl7.fhir.r4.model.Bundle.*;
import org.hl7.fhir.r4.model.Observation.ObservationStatus;
import org.hl7.fhir.r4.model.OperationOutcome.IssueSeverity;
import org.junit.*;
import org.mockito.ArgumentCaptor;
import org.springframework.transaction.TransactionDefinition;
import org.springframework.transaction.TransactionStatus;
import org.springframework.transaction.support.TransactionCallback;
@ -42,9 +39,6 @@ import java.util.stream.Collectors;
import static org.hamcrest.Matchers.*;
import static org.junit.Assert.*;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
@ -415,44 +409,6 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
ourLog.info(myFhirCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(mo));
}
@Test
public void testDeleteWithHas() {
Observation obs1 = new Observation();
obs1.setStatus(ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
Observation obs2 = new Observation();
obs2.setStatus(ObservationStatus.FINAL);
IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
DiagnosticReport rpt = new DiagnosticReport();
rpt.addIdentifier().setSystem("foo").setValue("IDENTIFIER");
rpt.addResult(new Reference(obs2id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
rpt = new DiagnosticReport();
rpt.addIdentifier().setSystem("foo").setValue("IDENTIFIER");
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(HTTPVerb.DELETE).setUrl("Observation?_has:DiagnosticReport:result:identifier=foo|IDENTIFIER");
b.addEntry().setResource(rpt).getRequest().setMethod(HTTPVerb.PUT).setUrl("DiagnosticReport?identifier=foo|IDENTIFIER");
mySystemDao.transaction(mySrd, b);
myObservationDao.read(obs1id);
try {
myObservationDao.read(obs2id);
fail();
} catch (ResourceGoneException e) {
// good
}
rpt = myDiagnosticReportDao.read(rptId);
assertThat(rpt.getResult(), empty());
}
@Test
public void testMultipleUpdatesWithNoChangesDoesNotResultInAnUpdateForTransaction() {
Bundle bundle;
@ -2429,6 +2385,66 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
}
@Test
public void testDeleteInTransactionShouldFailWhenReferencesExist() {
final Observation obs1 = new Observation();
obs1.setStatus(ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
final Observation obs2 = new Observation();
obs2.setStatus(ObservationStatus.FINAL);
IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
final DiagnosticReport rpt = new DiagnosticReport();
rpt.addResult(new Reference(obs2id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
myDiagnosticReportDao.read(rptId);
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(HTTPVerb.DELETE).setUrl(obs2id.getValue());
try {
mySystemDao.transaction(mySrd, b);
fail();
} catch (ResourceVersionConflictException e) {
// good, transaction should not succeed because DiagnosticReport has a reference to the obs2
}
}
@Test
public void testDeleteInTransactionShouldSucceedWhenReferencesAreAlsoRemoved() {
final Observation obs1 = new Observation();
obs1.setStatus(ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
final Observation obs2 = new Observation();
obs2.setStatus(ObservationStatus.FINAL);
IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
final DiagnosticReport rpt = new DiagnosticReport();
rpt.addResult(new Reference(obs2id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
myDiagnosticReportDao.read(rptId);
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(HTTPVerb.DELETE).setUrl(obs2id.getValue());
b.addEntry().getRequest().setMethod(HTTPVerb.DELETE).setUrl(rptId.getValue());
try {
// transaction should succeed because the DiagnosticReport which references obs2 is also deleted
mySystemDao.transaction(mySrd, b);
} catch (ResourceVersionConflictException e) {
fail();
}
}
private Bundle createTransactionBundleForTestTransactionWithRefsToConditionalCreate() {
Bundle b = new Bundle();
b.setType(BundleType.TRANSACTION);

207
hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/TransactionDeleteR4Test.java Normal file
View File

@ -0,0 +1,207 @@
package ca.uhn.fhir.jpa.dao.r4;
import ca.uhn.fhir.jpa.config.TestR4Config;
import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException;
import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
import org.hl7.fhir.instance.model.api.IIdType;
import org.hl7.fhir.r4.model.Bundle;
import org.hl7.fhir.r4.model.DiagnosticReport;
import org.hl7.fhir.r4.model.Observation;
import org.hl7.fhir.r4.model.Reference;
import org.junit.After;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.matchesPattern;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = {TestR4Config.class})
public class TransactionDeleteR4Test extends BaseJpaR4SystemTest {
@After
public void after() {
myDaoConfig.setEnforceReferentialIntegrityOnDelete(true);
}
@Test
public void testDeleteInTransactionShouldFailWhenReferencesExist() {
final Observation obs1 = new Observation();
obs1.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
final Observation obs2 = new Observation();
obs2.setStatus(Observation.ObservationStatus.FINAL);
org.hl7.fhir.instance.model.api.IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
final DiagnosticReport rpt = new DiagnosticReport();
rpt.addResult(new Reference(obs2id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
myDiagnosticReportDao.read(rptId);
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl(obs2id.getValue());
try {
mySystemDao.transaction(mySrd, b);
fail();
} catch (ResourceVersionConflictException e) {
// good, transaction should not succeed because DiagnosticReport has a reference to the obs2
}
}
@Test
public void testDeleteInTransactionShouldSucceedWhenReferencesAreAlsoRemoved() {
final Observation obs1 = new Observation();
obs1.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
final Observation obs2 = new Observation();
obs2.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
final DiagnosticReport rpt = new DiagnosticReport();
rpt.addResult(new Reference(obs2id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
myDiagnosticReportDao.read(rptId);
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl(rptId.getValue());
b.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl(obs2id.getValue());
try {
// transaction should succeed because the DiagnosticReport which references obs2 is also deleted
mySystemDao.transaction(mySrd, b);
} catch (ResourceVersionConflictException e) {
fail();
}
}
@Test
public void testDeleteWithHas_SourceModifiedToNoLongerIncludeReference() {
Observation obs1 = new Observation();
obs1.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
Observation obs2 = new Observation();
obs2.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
DiagnosticReport rpt = new DiagnosticReport();
rpt.addIdentifier().setSystem("foo").setValue("IDENTIFIER");
rpt.addResult(new Reference(obs2id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
rpt = new DiagnosticReport();
rpt.addIdentifier().setSystem("foo").setValue("IDENTIFIER");
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl("Observation?_has:DiagnosticReport:result:identifier=foo|IDENTIFIER");
b.addEntry().setResource(rpt).getRequest().setMethod(Bundle.HTTPVerb.PUT).setUrl("DiagnosticReport?identifier=foo|IDENTIFIER");
mySystemDao.transaction(mySrd, b);
myObservationDao.read(obs1id);
try {
myObservationDao.read(obs2id);
fail();
} catch (ResourceGoneException e) {
// good
}
rpt = myDiagnosticReportDao.read(rptId);
assertThat(rpt.getResult(), empty());
}
@Test
public void testDeleteWithId_SourceModifiedToNoLongerIncludeReference() {
Observation obs1 = new Observation();
obs1.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
Observation obs2 = new Observation();
obs2.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
DiagnosticReport rpt = new DiagnosticReport();
rpt.addResult(new Reference(obs1id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
rpt = new DiagnosticReport();
rpt.addResult(new Reference(obs2id));
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl(obs1id.getValue());
b.addEntry().setResource(rpt).getRequest().setMethod(Bundle.HTTPVerb.PUT).setUrl(rptId.getValue());
mySystemDao.transaction(mySrd, b);
myObservationDao.read(obs2id);
myDiagnosticReportDao.read(rptId);
try {
myObservationDao.read(obs1id);
fail();
} catch (ResourceGoneException e) {
// good
}
}
@Test
public void testDeleteWithHas_SourceModifiedToStillIncludeReference() {
Observation obs1 = new Observation();
obs1.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
Observation obs2 = new Observation();
obs2.setStatus(Observation.ObservationStatus.FINAL);
IIdType obs2id = myObservationDao.create(obs2).getId().toUnqualifiedVersionless();
DiagnosticReport rpt = new DiagnosticReport();
rpt.addIdentifier().setSystem("foo").setValue("IDENTIFIER");
rpt.addResult(new Reference(obs2id));
IIdType rptId = myDiagnosticReportDao.create(rpt).getId().toUnqualifiedVersionless();
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
rpt = new DiagnosticReport();
rpt.addIdentifier().setSystem("foo").setValue("IDENTIFIER");
rpt.addResult(new Reference(obs2id));
Bundle b = new Bundle();
b.addEntry().getRequest().setMethod(Bundle.HTTPVerb.DELETE).setUrl("Observation?_has:DiagnosticReport:result:identifier=foo|IDENTIFIER");
b.addEntry().setResource(rpt).getRequest().setMethod(Bundle.HTTPVerb.PUT).setUrl("DiagnosticReport?identifier=foo|IDENTIFIER");
try {
mySystemDao.transaction(mySrd, b);
fail();
} catch (ResourceVersionConflictException e ) {
assertThat(e.getMessage(), matchesPattern("Unable to delete Observation/[0-9]+ because at least one resource has a reference to this resource. First reference found was resource DiagnosticReport/[0-9]+ in path DiagnosticReport.result"));
}
myObservationDao.read(obs1id);
myObservationDao.read(obs2id);
myDiagnosticReportDao.read(rptId);
}
}

2
hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/tenant/ITenantIdentificationStrategy.java
View File

@ -37,7 +37,7 @@ public interface ITenantIdentificationStrategy {
/**
* Implementations may use this method to tweak the server base URL
* if neccesary based on the tenant ID
* if necessary based on the tenant ID
*/
String massageServerBaseUrl(String theFhirServerBase, RequestDetails theRequestDetails);
}

5
src/changes/changes.xml
View File

@ -20,6 +20,11 @@
The HAPI FHIR CLI import-csv-to-conceptmap command was not accounting for byte order marks in
CSV files (e.g. some Excel CSV files). This has been fixed.
</action>
<action type="fix" issue="1241">
A bug was fixed where deleting resources within a transaction did not always correctly
enforce referential integrity even if referential integrity was enabled. Thanks to
Tuomo Ala-Vannesluoma for reporting!
</action>
</release>
<release version="3.8.0" date="2019-05-30" description="Hippo">
<action type="fix">