From 12765ac397030493929cbeec6d1ddd2d0472d642 Mon Sep 17 00:00:00 2001
From: jdar8 <69840459+jdar8@users.noreply.github.com>
Date: Fri, 6 Sep 2024 10:16:49 -0700
Subject: [PATCH] Disabling enforce referential integrity on write still checks
references (#6219)
* fix
* tests, changelog, spotless
* document tests better
* address review comments
* reset settings so other tests don't fail
* unused import
---------
Co-authored-by: jdar <justin.dar@smiledigitalhealth.com>
---
...y-on-write-disabled-still-checks-refs.yaml | 6 +
...ResourceDaoR4ReferentialIntegrityTest.java | 168 +++++++++++++++---
.../dao/index/DaoResourceLinkResolver.java | 57 ++++--
3 files changed, 194 insertions(+), 37 deletions(-)
create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6218-fix-ref-integrity-on-write-disabled-still-checks-refs.yaml
diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6218-fix-ref-integrity-on-write-disabled-still-checks-refs.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6218-fix-ref-integrity-on-write-disabled-still-checks-refs.yaml
new file mode 100644
index 00000000000..b6469d5c8b5
--- /dev/null
+++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6218-fix-ref-integrity-on-write-disabled-still-checks-refs.yaml
@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 6218
+jira: SMILE-8370
+title: "Previously, when disabling the 'Enforce Referential Integrity on Write' setting, referential integrity was still
+partially enforced when posting a resource with an invalid reference. This has now been fixed."
diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ReferentialIntegrityTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ReferentialIntegrityTest.java
index f75fce4b44c..8020f0556ea 100644
--- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ReferentialIntegrityTest.java
+++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ReferentialIntegrityTest.java
@@ -1,19 +1,26 @@
package ca.uhn.fhir.jpa.dao.r4;
-import static org.junit.jupiter.api.Assertions.assertEquals;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
+import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
import org.hl7.fhir.instance.model.api.IIdType;
+import org.hl7.fhir.r4.model.IdType;
+import org.hl7.fhir.r4.model.Observation;
import org.hl7.fhir.r4.model.Organization;
import org.hl7.fhir.r4.model.Patient;
import org.hl7.fhir.r4.model.Reference;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
-import static org.assertj.core.api.Assertions.assertThat;
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;
public class FhirResourceDaoR4ReferentialIntegrityTest extends BaseJpaR4Test {
@@ -23,51 +30,168 @@ public class FhirResourceDaoR4ReferentialIntegrityTest extends BaseJpaR4Test {
myStorageSettings.setEnforceReferentialIntegrityOnWrite(new JpaStorageSettings().isEnforceReferentialIntegrityOnWrite());
myStorageSettings.setEnforceReferentialIntegrityOnDelete(new JpaStorageSettings().isEnforceReferentialIntegrityOnDelete());
myStorageSettings.setEnforceReferenceTargetTypes(new JpaStorageSettings().isEnforceReferenceTargetTypes());
+ myStorageSettings.setResourceClientIdStrategy(new JpaStorageSettings().getResourceClientIdStrategy());
+ }
+
+ @ParameterizedTest
+ @MethodSource("paramsProvider_withResourceType")
+ public void referentialIntegrityOnWriteSetting_unknownIds_fullScopeTest(boolean theIsEnforceRefIntegrityEnabled,
+ JpaStorageSettings.ClientIdStrategyEnum theClientIdStrategy,
+ String theReferenceId) {
+ // Given
+ myStorageSettings.setEnforceReferentialIntegrityOnWrite(theIsEnforceRefIntegrityEnabled);
+ myStorageSettings.setEnforceReferenceTargetTypes(theIsEnforceRefIntegrityEnabled);
+ myStorageSettings.setResourceClientIdStrategy(theClientIdStrategy);
+
+ Patient p = new Patient();
+ p.setManagingOrganization(new Reference(theReferenceId));
+
+ if (theIsEnforceRefIntegrityEnabled) {
+ try {
+ // When
+ myPatientDao.create(p);
+ fail();
+ } catch (InvalidRequestException e) {
+ // Then
+ assertEquals(Msg.code(1094) + "Resource " + theReferenceId + " not found, specified in path: Patient.managingOrganization", e.getMessage());
+ }
+ } else {
+ // Disabled ref integrity on write case: all POSTs should succeed
+ // When
+ IIdType id = myPatientDao.create(p).getId().toUnqualifiedVersionless();
+ // Then
+ p = myPatientDao.read(id);
+ assertEquals(theReferenceId, p.getManagingOrganization().getReference());
+ }
+ }
+
+ private static Stream<Arguments> paramsProvider_withResourceType() {
+ // theIsEnforceRefIntegrityEnabled, theClientIdStrategy, theReferenceId
+ // note: client ID is tested since resolving the resource reference is different depending on the strategy
+ return Stream.of(
+ Arguments.of(true, JpaStorageSettings.ClientIdStrategyEnum.ALPHANUMERIC, "Organization/123"),
+ Arguments.of(true, JpaStorageSettings.ClientIdStrategyEnum.ALPHANUMERIC, "Organization/A123"),
+ Arguments.of(true, JpaStorageSettings.ClientIdStrategyEnum.ANY, "Organization/123"),
+ Arguments.of(true, JpaStorageSettings.ClientIdStrategyEnum.ANY, "Organization/A123"),
+ Arguments.of(false, JpaStorageSettings.ClientIdStrategyEnum.ALPHANUMERIC, "Organization/123"),
+ Arguments.of(false, JpaStorageSettings.ClientIdStrategyEnum.ALPHANUMERIC, "Organization/A123"),
+ Arguments.of(false, JpaStorageSettings.ClientIdStrategyEnum.ANY, "Organization/123"),
+ Arguments.of(false, JpaStorageSettings.ClientIdStrategyEnum.ANY, "Organization/A123")
+ );
}
@Test
- public void testCreateUnknownReferenceFail() throws Exception {
+ public void testRefIntegrityOnWrite_withReferenceIdOfAnotherResourceType() {
+ // Given
+ myStorageSettings.setResourceClientIdStrategy(JpaStorageSettings.ClientIdStrategyEnum.ALPHANUMERIC);
+ // Create Observation with some ID...
+ Observation obs = new Observation();
+ IIdType obsId = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless();
+
+ // And reference a non-existing Organization resource with the same ID as the Observation
Patient p = new Patient();
- p.setManagingOrganization(new Reference("Organization/AAA"));
+ p.setManagingOrganization(new Reference(new IdType("Organization", obsId.getIdPart())));
+
try {
+ // When
myPatientDao.create(p);
fail();
- } catch (InvalidRequestException e) {
- assertEquals(Msg.code(1094) + "Resource Organization/AAA not found, specified in path: Patient.managingOrganization", e.getMessage());
+ } catch (UnprocessableEntityException e) {
+ // Then: identify that it is the wrong resource type, since ref integrity is enabled
+ assertEquals(Msg.code(1095) + "Resource contains reference to unknown resource ID Organization/" + obsId.getIdPart(), e.getMessage());
}
+ // Given: now disable referential integrity on write
+ myStorageSettings.setEnforceReferentialIntegrityOnWrite(false);
+ // When
+ IIdType id = myPatientDao.create(p).getId().toUnqualifiedVersionless();
+ // Then: this should now succeed
+ p = myPatientDao.read(id);
+ assertEquals("Organization/" + obsId.getIdPart(), p.getManagingOrganization().getReference());
}
- @Test
- public void testCreateUnknownReferenceAllowed() {
- myStorageSettings.setEnforceReferentialIntegrityOnWrite(false);
+ @ParameterizedTest
+ @MethodSource("paramsProvider_noResourceType")
+ public void testRefIntegrityOnWrite_withValidReferenceId_shouldAlwaysSucceed(boolean theIsEnforceRefIntegrityEnabled,
+ JpaStorageSettings.ClientIdStrategyEnum theClientIdStrategy,
+ String theReferenceId) {
+ // Given
+ myStorageSettings.setEnforceReferentialIntegrityOnWrite(theIsEnforceRefIntegrityEnabled);
+ myStorageSettings.setEnforceReferenceTargetTypes(theIsEnforceRefIntegrityEnabled);
+ myStorageSettings.setResourceClientIdStrategy(theClientIdStrategy);
+
+ Organization obs = new Organization();
+ obs.setId(theReferenceId);
+ myOrganizationDao.update(obs, mySrd);
+ String qualifiedReferenceId = "Organization/" + theReferenceId;
Patient p = new Patient();
- p.setManagingOrganization(new Reference("Organization/AAA"));
+ p.setManagingOrganization(new Reference(qualifiedReferenceId));
+
+ // When
IIdType id = myPatientDao.create(p).getId().toUnqualifiedVersionless();
+ // Then
p = myPatientDao.read(id);
- assertEquals("Organization/AAA", p.getManagingOrganization().getReference());
-
+ assertEquals(qualifiedReferenceId, p.getManagingOrganization().getReference());
}
- @Test
- public void testCreateUnknownReferenceAllowed_NumericId() {
- myStorageSettings.setEnforceReferentialIntegrityOnWrite(false);
- myStorageSettings.setEnforceReferenceTargetTypes(false);
+ private static Stream<Arguments> paramsProvider_noResourceType() {
+ // theIsEnforceRefIntegrityEnabled, theClientIdStrategy, theReferenceId
+ // note: client ID is tested since resolving the resource reference is different depending on the strategy
+ return Stream.of(
+ Arguments.of(true, JpaStorageSettings.ClientIdStrategyEnum.ALPHANUMERIC, "A123"),
+ Arguments.of(true, JpaStorageSettings.ClientIdStrategyEnum.ANY, "123"),
+ Arguments.of(true, JpaStorageSettings.ClientIdStrategyEnum.ANY, "A123"),
+ Arguments.of(false, JpaStorageSettings.ClientIdStrategyEnum.ALPHANUMERIC, "A123"),
+ Arguments.of(false, JpaStorageSettings.ClientIdStrategyEnum.ANY, "123"),
+ Arguments.of(false, JpaStorageSettings.ClientIdStrategyEnum.ANY, "A123")
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("paramsProvider_noResourceType")
+ public void testRefIntegrityOnWrite_withReferenceIdOfDeletedResource(boolean theIsEnforceRefIntegrityEnabled,
+ JpaStorageSettings.ClientIdStrategyEnum theClientIdStrategy,
+ String theReferenceId) {
+ // Given
+ myStorageSettings.setEnforceReferentialIntegrityOnWrite(theIsEnforceRefIntegrityEnabled);
+ myStorageSettings.setEnforceReferenceTargetTypes(theIsEnforceRefIntegrityEnabled);
+ myStorageSettings.setResourceClientIdStrategy(theClientIdStrategy);
+
+ Organization obs = new Organization();
+ obs.setId(theReferenceId);
+ IIdType obsId = myOrganizationDao.update(obs, mySrd).getId();
+ String qualifiedReferenceId = "Organization/" + theReferenceId;
+
+ myOrganizationDao.delete(obsId, mySrd);
Patient p = new Patient();
- p.setManagingOrganization(new Reference("Organization/123"));
- IIdType id = myPatientDao.create(p).getId().toUnqualifiedVersionless();
+ p.setManagingOrganization(new Reference(qualifiedReferenceId));
- p = myPatientDao.read(id);
- assertEquals("Organization/123", p.getManagingOrganization().getReference());
+ if (theIsEnforceRefIntegrityEnabled) {
+ try {
+ // When
+ myPatientDao.create(p);
+ fail();
+ } catch (InvalidRequestException e) {
+ // Then
+ assertEquals(Msg.code(1096) + "Resource " + qualifiedReferenceId + " is deleted, specified in path: Patient.managingOrganization", e.getMessage());
+ }
+ } else {
+ // Disabled ref integrity on write case: all POSTs should succeed
+ // When
+ IIdType id = myPatientDao.create(p).getId().toUnqualifiedVersionless();
+ // Then
+ p = myPatientDao.read(id);
+ assertEquals(qualifiedReferenceId, p.getManagingOrganization().getReference());
+ }
}
@Test
- public void testDeleteFail() throws Exception {
+ public void testDeleteFail() {
Organization o = new Organization();
o.setName("FOO");
IIdType oid = myOrganizationDao.create(o).getId().toUnqualifiedVersionless();
@@ -89,7 +213,7 @@ public class FhirResourceDaoR4ReferentialIntegrityTest extends BaseJpaR4Test {
}
@Test
- public void testDeleteAllow() throws Exception {
+ public void testDeleteAllow() {
myStorageSettings.setEnforceReferentialIntegrityOnDelete(false);
Organization o = new Organization();
diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java
index e76ed2801bb..9434e126936 100644
--- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java
+++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java
@@ -134,7 +134,7 @@ public class DaoResourceLinkResolver<T extends IResourcePersistentId> implements
type, targetReference, idPart, theRequest, theTransactionDetails);
if (!createdTableOpt.isPresent()) {
- if (myStorageSettings.isEnforceReferentialIntegrityOnWrite() == false) {
+ if (!myStorageSettings.isEnforceReferentialIntegrityOnWrite()) {
return null;
}
@@ -150,20 +150,8 @@ public class DaoResourceLinkResolver<T extends IResourcePersistentId> implements
"Resolved resource of type {} as PID: {}",
resolvedResource.getResourceType(),
resolvedResource.getPersistentId());
- if (!resourceType.equals(resolvedResource.getResourceType())) {
- ourLog.error(
- "Resource with PID {} was of type {} and wanted {}",
- resolvedResource.getPersistentId(),
- resourceType,
- resolvedResource.getResourceType());
- throw new UnprocessableEntityException(Msg.code(1095)
- + "Resource contains reference to unknown resource ID " + targetResourceId.getValue());
- }
-
- if (resolvedResource.getDeleted() != null) {
- String resName = resolvedResource.getResourceType();
- throw new InvalidRequestException(Msg.code(1096) + "Resource " + resName + "/" + idPart
- + " is deleted, specified in path: " + sourcePath);
+ if (!validateResolvedResourceOrThrow(resourceType, resolvedResource, targetResourceId, idPart, sourcePath)) {
+ return null;
}
if (persistentId == null) {
@@ -182,6 +170,45 @@ public class DaoResourceLinkResolver<T extends IResourcePersistentId> implements
return resolvedResource;
}
+ /**
+ * Validates the resolved resource.
+ * If 'Enforce Referential Integrity on Write' is enabled:
+ * Throws <code>UnprocessableEntityException</code> when resource types do not match
+ * Throws <code>InvalidRequestException</code> when the resolved resource was deleted
+ * <p>
+ * Otherwise, return false when resource types do not match or resource was deleted
+ * and return true if the resolved resource is valid.
+ */
+ private boolean validateResolvedResourceOrThrow(
+ String resourceType,
+ IResourceLookup resolvedResource,
+ IIdType targetResourceId,
+ String idPart,
+ String sourcePath) {
+ if (!resourceType.equals(resolvedResource.getResourceType())) {
+ ourLog.error(
+ "Resource with PID {} was of type {} and wanted {}",
+ resolvedResource.getPersistentId(),
+ resourceType,
+ resolvedResource.getResourceType());
+ if (!myStorageSettings.isEnforceReferentialIntegrityOnWrite()) {
+ return false;
+ }
+ throw new UnprocessableEntityException(Msg.code(1095)
+ + "Resource contains reference to unknown resource ID " + targetResourceId.getValue());
+ }
+
+ if (resolvedResource.getDeleted() != null) {
+ if (!myStorageSettings.isEnforceReferentialIntegrityOnWrite()) {
+ return false;
+ }
+ String resName = resolvedResource.getResourceType();
+ throw new InvalidRequestException(Msg.code(1096) + "Resource " + resName + "/" + idPart
+ + " is deleted, specified in path: " + sourcePath);
+ }
+ return true;
+ }
+
@Nullable
@Override
public IBaseResource loadTargetResource(