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

Referential integrity validation is broken in transactions #1241

Closed
tuomoa opened this issue Mar 15, 2019 · 4 comments

Comments

@tuomoa
Copy link

commented Mar 15, 2019

Hello,

There appears to be a problem with the referential integrity validation when committing transaction bundles. I've tested the behaviour with version 3.6.0 as well as in the master branch.

If there is some resource A referencing to resource B, resource B cannot be deleted because it generates the reference error. However if resource B is deleted in a transaction Bundle, it will succeed.

I wrote two simple tests to FhirSystemDaoDstu3Test which are testing the correct behavior. The first test is not passing, as the TransactionProcessor is not doing validation correctly.

@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();
	}
}

I think the problem is in TransactionProcessor.java:748:

deleteConflicts.removeIf(next ->
	deletedResources.contains(next.getTargetId().toUnqualifiedVersionless().getValue()));

For example when deleting the Observation in the first test, DeleteConflict source is the DiagnosticReport and target is the Observation being deleted. It should only delete the conflict from the list if the source of the reference is removed as well, and therefore I think it should be:

deleteConflicts.removeIf(next ->
	deletedResources.contains(next.getSourceId().toUnqualifiedVersionless().getValue()));

However when I tried this, there are some tests which starts to fail:

[ERROR] testDeleteWithHas(ca.uhn.fhir.jpa.dao.dstu3.FhirSystemDaoDstu3Test)  Time elapsed: 0.123 s  <<< ERROR!
ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException: Unable to delete Observation/318 because at least one resource has a reference to this resource. First reference found was resource Observation/318 in path DiagnosticReport.result
	at ca.uhn.fhir.jpa.dao.dstu3.FhirSystemDaoDstu3Test.testDeleteWithHas(FhirSystemDaoDstu3Test.java:399)

[ERROR] testTransactionOruBundle(ca.uhn.fhir.jpa.dao.dstu3.FhirSystemDaoDstu3Test)  Time elapsed: 0.384 s  <<< ERROR!
ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException: Unable to delete Observation/449 because at least one resource has a reference to this resource. First reference found was resource Observation/449 in path DiagnosticReport.result
	at ca.uhn.fhir.jpa.dao.dstu3.FhirSystemDaoDstu3Test.testTransactionOruBundle(FhirSystemDaoDstu3Test.java:1916)

[ERROR] testTransactionOrdering(ca.uhn.fhir.jpa.dao.dstu3.FhirSystemDaoDstu3Test)  Time elapsed: 0.154 s  <<< ERROR!
ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException: Unable to delete Patient/463 because at least one resource has a reference to this resource. First reference found was resource Patient/463 in path Observation.subject
	at ca.uhn.fhir.jpa.dao.dstu3.FhirSystemDaoDstu3Test.testTransactionOrdering(FhirSystemDaoDstu3Test.java:1851)

It looks like there is also some other issue with the referential integrity validation in the transaction context. The testDeleteWithHas test tries to remove the reference and the referenced object in one transaction, but it fails because in the deleteConflicts list there is the DiagnosticReport->Observation reference even though DiagnosticReport is being updated to NOT contain the reference.

In the testTransactionOrdering test there is Patient and Observation with reference to the Patient created. Then the Patient is tried to be deleted. On the first transaction Patient deletion fails because it didn't exist yet. But in the second transaction it should fail, because Patient should not be allowed to be deleted if there are Observation resources that are not deleted.

The testTransactionOruBundle appears to have similar kind of problem with the testTransactionOrdering. It does the same transaction twice, and in the second run it tries to delete the Observation resources which are referenced from the DiagnosticReport. However it shouldn't be allowed if the DiagnosticReport is not deleted as well.

@jamesagnew

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

This is so interesting. @tuomoa I'd say your fix is correct.

You're also right that this breaks other tests, but it's because they are testing something that seemed like it was working, but actually wasn't.

Say DiagnosticReport/1 refers to Observation/2

  • You can’t delete Observation/1 by itself
  • You can delete Observation/1 if you also delete DiagnosticReport/2
  • But I guess you should also be able to delete Observation/1 if you remove the reference in DiagnosticReport/2 within the same transaction

That last thing doesn't actually work, but it seemed like it did because of this bug. Trying to fix now.

@jamesagnew

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

Ok, after pondering this for ages... I've come to the conclusion that you shouldn't be able to delete Observation/1 and remove the reference to it from DiagnosticReport/2 at the same time. I think this because of the transaction order of operations. From the FHIR spec: http://hl7.org/fhir/http.html#trules

    Process any DELETE interactions
    Process any POST interactions
    Process any PUT or PATCH interactions
    Process any GET or HEAD interactions
    Resolve any conditional references
@jamesagnew

This comment has been minimized.

Copy link
Owner

commented Jun 4, 2019

Ok, one more update. I did find a way to make this work, so I decided to allow the delete after all. Fix coming up.

@jamesagnew

This comment has been minimized.

Copy link
Owner

commented Jun 5, 2019

Fixed in #1243

@jamesagnew jamesagnew closed this Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.