From fe9cf2027111283f4def95a5d51b8a1e18714a68 Mon Sep 17 00:00:00 2001 From: jdar Date: Wed, 13 Nov 2024 10:25:14 -0800 Subject: [PATCH 1/2] fix ref integrity when using pid on resource with forced id --- .../fhir/jpa/dao/data/IResourceTableDao.java | 8 +-- .../fhir/jpa/dao/index/IdHelperService.java | 44 ++++++++++----- .../jpa/model/cross/JpaResourceLookup.java | 7 +++ .../jpa/dao/index/IdHelperServiceTest.java | 53 +++++++++++++++++-- ...ResourceDaoR4ReferentialIntegrityTest.java | 32 ++++++++++- 5 files changed, 124 insertions(+), 20 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceTableDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceTableDao.java index a081668cdaeb..5f37f70bf3ba 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceTableDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceTableDao.java @@ -136,7 +136,7 @@ Slice 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 findLookupFieldsByResourcePid(@Param("pid") List thePids); /** @@ -144,7 +144,7 @@ Slice 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) 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 findLookupFieldsByResourcePidInPartitionIds( @Param("pid") List thePids, @Param("partition_id") Collection thePartitionId); @@ -153,7 +153,7 @@ Collection 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 findLookupFieldsByResourcePidInPartitionIdsOrNullPartition( @Param("pid") List thePids, @Param("partition_id") Collection thePartitionId); @@ -162,7 +162,7 @@ Collection 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 findLookupFieldsByResourcePidInPartitionNull(@Param("pid") List thePids); @Query("SELECT t.myVersion FROM ResourceTable t WHERE t.myId = :pid") diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index 9e81e87f598c..6dd7e9a2fb7c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -146,6 +146,8 @@ public IResourceLookup 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,10 +511,10 @@ private ListMultimap organizeIdsByResourceType(Collection>> translateForcedIdToPids( - @Nonnull RequestPartitionId theRequestPartitionId, Collection theId, boolean theExcludeDeleted) { - theId.forEach(id -> Validate.isTrue(id.hasIdPart())); + @Nonnull RequestPartitionId theRequestPartitionId, Collection theIds, boolean theExcludeDeleted) { + theIds.forEach(id -> Validate.isTrue(id.hasIdPart())); - if (theId.isEmpty()) { + if (theIds.isEmpty()) { return new HashMap<>(); } @@ -520,8 +522,8 @@ private Map>> translateForcedIdToPids( RequestPartitionId requestPartitionId = replaceDefault(theRequestPartitionId); if (myStorageSettings.getResourceClientIdStrategy() != JpaStorageSettings.ClientIdStrategyEnum.ANY) { - List pids = theId.stream() - .filter(t -> isValidPid(t)) + List pids = theIds.stream() + .filter(IdHelperService::isValidPid) .map(IIdType::getIdPartAsLong) .collect(Collectors.toList()); if (!pids.isEmpty()) { @@ -530,7 +532,7 @@ private Map>> translateForcedIdToPids( } // returns a map of resourcetype->id - ListMultimap typeToIds = organizeIdsByResourceType(theId); + ListMultimap typeToIds = organizeIdsByResourceType(theIds); for (Map.Entry> nextEntry : typeToIds.asMap().entrySet()) { String nextResourceType = nextEntry.getKey(); Collection nextIds = nextEntry.getValue(); @@ -583,6 +585,7 @@ private Map>> 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 thePidsToResolve, Long theFoundPid, String theFhirId) { + return thePidsToResolve.contains(theFoundPid) && !theFhirId.equals(String.valueOf(theFoundPid)); + } + @Override public PersistentIdToForcedIdMap translatePidsToForcedIds(Set 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); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/model/cross/JpaResourceLookup.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/model/cross/JpaResourceLookup.java index 891617aa007b..82ae36eb40b8 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/model/cross/JpaResourceLookup.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/model/cross/JpaResourceLookup.java @@ -30,14 +30,17 @@ public class JpaResourceLookup implements IResourceLookup { 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; + } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java index fa0870d4e67a..7466fe346208 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java +++ b/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 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 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 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"); 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 8020f0556ea2..07a93ceb6505 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 @@ -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() { } - } From 76a5e3c9e2b1a33dd720818d582a2f77846b11be Mon Sep 17 00:00:00 2001 From: jdar Date: Tue, 19 Nov 2024 11:05:49 -0800 Subject: [PATCH 2/2] changelog --- .../6476-fix-ref-integrity-with-client-assigned-ids.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6476-fix-ref-integrity-with-client-assigned-ids.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6476-fix-ref-integrity-with-client-assigned-ids.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6476-fix-ref-integrity-with-client-assigned-ids.yaml new file mode 100644 index 000000000000..8fee49c1f482 --- /dev/null +++ b/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."