Jd 20241113 fix ref integrity with client assigned ids

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6476-fix-ref-integrity-with-client-assigned-ids.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+jira: SMILE-8941
+issue: 6476
+title: "Previously, when Enforce Referential Integrity on Write was enabled, it was possible to create a resource with 
+a reference to another resource by using its internal PID instead of its forced ID. This has now been fixed."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceTableDao.java

@@ -136,15 +136,15 @@ Slice<Long> findIdsOfResourcesWithinUpdatedRangeOrderedFromOldest(
 	 * is an object array, where the order matters (the array represents columns returned by the query). Be careful if you change this query in any way.
 	 */
 	@Query(
-			"SELECT t.myResourceType, t.myId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid)")
+			"SELECT t.myResourceType, t.myId, t.myFhirId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid)")
 	Collection<Object[]> findLookupFieldsByResourcePid(@Param("pid") List<Long> thePids);
 
 	/**
 	 * This method returns a Collection where each row is an element in the collection. Each element in the collection
 	 * is an object array, where the order matters (the array represents columns returned by the query). Be careful if you change this query in any way.
 	 */
 	@Query(
-			"SELECT t.myResourceType, t.myId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid) AND t.myPartitionIdValue IN :partition_id")
+			"SELECT t.myResourceType, t.myId, t.myFhirId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid) AND t.myPartitionIdValue IN :partition_id")
 	Collection<Object[]> findLookupFieldsByResourcePidInPartitionIds(
 			@Param("pid") List<Long> thePids, @Param("partition_id") Collection<Integer> thePartitionId);
 
@@ -153,7 +153,7 @@ Collection<Object[]> findLookupFieldsByResourcePidInPartitionIds(
 	 * is an object array, where the order matters (the array represents columns returned by the query). Be careful if you change this query in any way.
 	 */
 	@Query(
-			"SELECT t.myResourceType, t.myId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid) AND (t.myPartitionIdValue IS NULL OR t.myPartitionIdValue IN :partition_id)")
+			"SELECT t.myResourceType, t.myId, t.myFhirId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid) AND (t.myPartitionIdValue IS NULL OR t.myPartitionIdValue IN :partition_id)")
 	Collection<Object[]> findLookupFieldsByResourcePidInPartitionIdsOrNullPartition(
 			@Param("pid") List<Long> thePids, @Param("partition_id") Collection<Integer> thePartitionId);
 
@@ -162,7 +162,7 @@ Collection<Object[]> findLookupFieldsByResourcePidInPartitionIdsOrNullPartition(
 	 * is an object array, where the order matters (the array represents columns returned by the query). Be careful if you change this query in any way.
 	 */
 	@Query(
-			"SELECT t.myResourceType, t.myId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid) AND t.myPartitionIdValue IS NULL")
+			"SELECT t.myResourceType, t.myId, t.myFhirId, t.myDeleted, t.myPartitionIdValue, t.myPartitionDateValue FROM ResourceTable t WHERE t.myId IN (:pid) AND t.myPartitionIdValue IS NULL")
 	Collection<Object[]> findLookupFieldsByResourcePidInPartitionNull(@Param("pid") List<Long> thePids);
 
 	@Query("SELECT t.myVersion FROM ResourceTable t WHERE t.myId = :pid")

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java

@@ -146,6 +146,8 @@ public IResourceLookup<JpaPid> resolveResourceIdentity(
 	 * Given a forced ID, convert it to its Long value. Since you are allowed to use string IDs for resources, we need to
 	 * convert those to the underlying Long values that are stored, for lookup and comparison purposes.
 	 * Optionally filters out deleted resources.
+	 * Note that when given a PID for a resource created with a forced ID (client-assigned ID), this method will throw
+	 * @ResourceNotFoundException
 	 *
 	 * @throws ResourceNotFoundException If the ID can not be found
 	 */
@@ -509,19 +511,19 @@ private ListMultimap<String, String> organizeIdsByResourceType(Collection<IIdTyp
 	}
 
 	private Map<String, List<IResourceLookup<JpaPid>>> translateForcedIdToPids(
-			@Nonnull RequestPartitionId theRequestPartitionId, Collection<IIdType> theId, boolean theExcludeDeleted) {
-		theId.forEach(id -> Validate.isTrue(id.hasIdPart()));
+			@Nonnull RequestPartitionId theRequestPartitionId, Collection<IIdType> theIds, boolean theExcludeDeleted) {
+		theIds.forEach(id -> Validate.isTrue(id.hasIdPart()));
 
-		if (theId.isEmpty()) {
+		if (theIds.isEmpty()) {
 			return new HashMap<>();
 		}
 
 		Map<String, List<IResourceLookup<JpaPid>>> retVal = new HashMap<>();
 		RequestPartitionId requestPartitionId = replaceDefault(theRequestPartitionId);
 
 		if (myStorageSettings.getResourceClientIdStrategy() != JpaStorageSettings.ClientIdStrategyEnum.ANY) {
-			List<Long> pids = theId.stream()
-					.filter(t -> isValidPid(t))
+			List<Long> pids = theIds.stream()
+					.filter(IdHelperService::isValidPid)
 					.map(IIdType::getIdPartAsLong)
 					.collect(Collectors.toList());
 			if (!pids.isEmpty()) {
@@ -530,7 +532,7 @@ private Map<String, List<IResourceLookup<JpaPid>>> translateForcedIdToPids(
 		}
 
 		// returns a map of resourcetype->id
-		ListMultimap<String, String> typeToIds = organizeIdsByResourceType(theId);
+		ListMultimap<String, String> typeToIds = organizeIdsByResourceType(theIds);
 		for (Map.Entry<String, Collection<String>> nextEntry : typeToIds.asMap().entrySet()) {
 			String nextResourceType = nextEntry.getKey();
 			Collection<String> nextIds = nextEntry.getValue();
@@ -583,6 +585,7 @@ private Map<String, List<IResourceLookup<JpaPid>>> translateForcedIdToPids(
 					JpaResourceLookup lookup = new JpaResourceLookup(
 							resourceType,
 							resourcePid,
+							forcedId,
 							deletedAt,
 							PartitionablePartitionId.with(partitionId, partitionDate));
 					retVal.computeIfAbsent(forcedId, id -> new ArrayList<>()).add(lookup);
@@ -646,14 +649,19 @@ private void resolvePids(
 					.map(t -> new JpaResourceLookup(
 							(String) t[0],
 							(Long) t[1],
-							(Date) t[2],
-							PartitionablePartitionId.with((Integer) t[3], (LocalDate) t[4])))
+							(String) t[2],
+							(Date) t[3],
+							PartitionablePartitionId.with((Integer) t[4], (LocalDate) t[5])))
 					.forEach(t -> {
 						String id = t.getPersistentId().toString();
-						if (!theTargets.containsKey(id)) {
-							theTargets.put(id, new ArrayList<>());
+						Long pid = t.getPersistentId().getId();
+						String fhirId = t.getFhirId();
+						if (!isResolvingPidOfResourceWithForcedId(thePidsToResolve, pid, fhirId)) {
+							if (!theTargets.containsKey(id)) {
+								theTargets.put(id, new ArrayList<>());
+							}
+							theTargets.get(id).add(t);
 						}
-						theTargets.get(id).add(t);
 						if (!myStorageSettings.isDeleteEnabled()) {
 							String nextKey = t.getPersistentId().toString();
 							myMemoryCacheService.putAfterCommit(
@@ -663,6 +671,14 @@ private void resolvePids(
 		}
 	}
 
+	/**
+	 * @return true if we were given the PID of a resource that has a forced ID
+	 */
+	private boolean isResolvingPidOfResourceWithForcedId(
+			List<Long> thePidsToResolve, Long theFoundPid, String theFhirId) {
+		return thePidsToResolve.contains(theFoundPid) && !theFhirId.equals(String.valueOf(theFoundPid));
+	}
+
 	@Override
 	public PersistentIdToForcedIdMap<JpaPid> translatePidsToForcedIds(Set<JpaPid> theResourceIds) {
 		assert myDontCheckActiveTransactionForUnitTest || TransactionSynchronizationManager.isSynchronizationActive();
@@ -725,7 +741,11 @@ public void addResolvedPidToForcedId(
 
 		if (!myStorageSettings.isDeleteEnabled()) {
 			JpaResourceLookup lookup = new JpaResourceLookup(
-					theResourceType, theJpaPid.getId(), theDeletedAt, theJpaPid.getPartitionablePartitionId());
+					theResourceType,
+					theJpaPid.getId(),
+					theForcedId,
+					theDeletedAt,
+					theJpaPid.getPartitionablePartitionId());
 			String nextKey = theJpaPid.toString();
 			myMemoryCacheService.putAfterCommit(MemoryCacheService.CacheEnum.RESOURCE_LOOKUP, nextKey, lookup);
 		}

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/model/cross/JpaResourceLookup.java

@@ -30,14 +30,17 @@ public class JpaResourceLookup implements IResourceLookup<JpaPid> {
 	private final Long myResourcePid;
 	private final Date myDeletedAt;
 	private final PartitionablePartitionId myPartitionablePartitionId;
+	private final String myFhirId;
 
 	public JpaResourceLookup(
 			String theResourceType,
 			Long theResourcePid,
+			String theFhirId,
 			Date theDeletedAt,
 			PartitionablePartitionId thePartitionablePartitionId) {
 		myResourceType = theResourceType;
 		myResourcePid = theResourcePid;
+		myFhirId = theFhirId;
 		myDeletedAt = theDeletedAt;
 		myPartitionablePartitionId = thePartitionablePartitionId;
 	}
@@ -59,4 +62,8 @@ public JpaPid getPersistentId() {
 
 		return jpaPid;
 	}
+
+	public String getFhirId() {
+		return myFhirId;
+	}
 }

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java

@@ -7,7 +7,11 @@
 import ca.uhn.fhir.jpa.model.cross.IResourceLookup;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.util.MemoryCacheService;
+import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
 import org.hl7.fhir.r4.model.Patient;
+
+import static org.junit.jupiter.api.Assertions.fail;
+
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -121,7 +125,7 @@ public void resolveResourcePersistentIds_withForcedIdsAndDeleteEnabled_returnsMa
 	}
 
 	@Test
-	public void resolveResourcePersistenIds_withForcedIdAndDeleteDisabled_returnsMap() {
+	public void resolveResourcePersistentIds_withForcedIdAndDeleteDisabled_returnsMap() {
 		RequestPartitionId partitionId = RequestPartitionId.allPartitions();
 		String resourceType = Patient.class.getSimpleName();
 		List<String> patientIdsToResolve = new ArrayList<>();
@@ -149,8 +153,7 @@ public void resolveResourcePersistenIds_withForcedIdAndDeleteDisabled_returnsMap
 		for (String id : patientIdsToResolve) {
 			assertThat(map).containsKey(id);
 		}
-		assertThat(map).containsEntry("RED", red);
-		assertThat(map).containsEntry("BLUE", blue);
+		assertThat(map).containsExactlyInAnyOrderEntriesOf(Map.of("RED", red, "BLUE", blue));
 	}
 
 	@Test
@@ -177,6 +180,50 @@ public void testResolveResourceIdentity_defaultFunctionality(){
 		assertEquals(forcedIdView[3], result.getDeleted());
 	}
 
+	@Test
+	public void testResolveResourceResourceIdentity_withPersistentIdOfResourceWithForcedIdAndDefaultClientIdStrategy_returnsNotFound(){
+		RequestPartitionId partitionId = RequestPartitionId.fromPartitionIdAndName(1, "partition");
+		String resourceType = "Patient";
+
+		Object[] forcedIdView = new Object[6];
+		forcedIdView[0] = "Patient";
+		forcedIdView[1] = 1L;
+		forcedIdView[2] = "AAA";
+		forcedIdView[3] = null;
+		forcedIdView[4] = null;
+		forcedIdView[5] = null;
+
+		Collection<Object[]> testForcedIdViews = new ArrayList<>();
+		testForcedIdViews.add(forcedIdView);
+		when(myResourceTableDao.findLookupFieldsByResourcePidInPartitionIds(any(), any())).thenReturn(testForcedIdViews);
+
+		try {
+			// Search by the PID of the resource that has a client assigned FHIR Id
+			myHelperService.resolveResourceIdentity(partitionId, resourceType, "1");
+			fail();
+		} catch(ResourceNotFoundException e) {
+			assertThat(e.getMessage()).isEqualTo("HAPI-2001: Resource Patient/1 is not known");
+		}
+	}
+
+	@Test
+	public void testResolveResourceResourceIdentity_withPersistentIdOfResourceWithForcedIdAndClientIdStrategyAny_returnsNotFound(){
+		when(myStorageSettings.getResourceClientIdStrategy()).thenReturn(JpaStorageSettings.ClientIdStrategyEnum.ANY);
+		RequestPartitionId partitionId = RequestPartitionId.fromPartitionIdAndName(1, "partition");
+		String resourceType = "Patient";
+
+		Collection<Object[]> testForcedIdViews = new ArrayList<>();
+		when(myResourceTableDao.findAndResolveByForcedIdWithNoTypeInPartition(any(), any(), any(), anyBoolean())).thenReturn(testForcedIdViews);
+
+		try {
+			// Search by the PID of the resource that has a client assigned FHIR Id
+			myHelperService.resolveResourceIdentity(partitionId, resourceType, "1");
+			fail();
+		} catch(ResourceNotFoundException e) {
+			assertThat(e.getMessage()).isEqualTo("HAPI-2001: Resource Patient/1 is not known");
+		}
+	}
+
 	@Test
 	public void testResolveResourcePersistentIds_mapDefaultFunctionality(){
 		RequestPartitionId partitionId = RequestPartitionId.fromPartitionIdAndName(1, "partition");

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ReferentialIntegrityTest.java

@@ -2,10 +2,14 @@
 
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
+import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 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 static org.assertj.core.api.Assertions.assertThat;
+
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.Observation;
@@ -16,6 +20,10 @@
 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.EnumSource;
+
+import static org.junit.jupiter.params.provider.EnumSource.Mode.EXCLUDE;
+
 import org.junit.jupiter.params.provider.MethodSource;
 
 import java.util.stream.Stream;
@@ -190,6 +198,29 @@ public void testRefIntegrityOnWrite_withReferenceIdOfDeletedResource(boolean the
 		}
 	}
 
+	@ParameterizedTest
+	@EnumSource(value = JpaStorageSettings.ClientIdStrategyEnum.class, mode = EXCLUDE, names = {"NOT_ALLOWED"})
+	public void testReferentialIntegrityOnWrite_withReferenceByPidForClientAssignedIdResource(JpaStorageSettings.ClientIdStrategyEnum theClientIdStrategy) {
+		myStorageSettings.setResourceClientIdStrategy(theClientIdStrategy);
+		myStorageSettings.setEnforceReferentialIntegrityOnWrite(true);
+
+		Organization o = new Organization();
+		o.setName("FOO");
+		o.setId("O1");
+		DaoMethodOutcome outcome = myOrganizationDao.update(o);
+		Long organizationPid = (Long) outcome.getEntity().getPersistentId().getId();
+
+		Patient p = new Patient();
+		p.setManagingOrganization(new Reference("Organization/" + organizationPid));
+
+		try {
+			myPatientDao.create(p);
+			fail();
+		} catch (InvalidRequestException e) {
+			assertThat(e.getMessage()).contains("Resource Organization/" + organizationPid + " not found, specified in path: Patient.managingOrganization");
+		}
+	}
+
 	@Test
 	public void testDeleteFail() {
 		Organization o = new Organization();
@@ -229,5 +260,4 @@ public void testDeleteAllow() {
 
 	}
 
-
 }