From fddea8db92b12f76a98c9c9ac167076e59dc6560 Mon Sep 17 00:00:00 2001 From: TynerGjs <132295567+TynerGjs@users.noreply.github.com> Date: Thu, 8 Jun 2023 12:38:24 -0400 Subject: [PATCH] Resolve 4952 PUT requests for resources that don't exist get created in sequential format rather than abiding by the ResourceServerIdStrategy (#4953) * Test implemented, issue resolved * fixed a logical bug in the previous change * modified comment * added tests for Dstu2&3 resources * Adding afterEach constructions to reset spring context beans. * resolved comments --------- Co-authored-by: Ken Stevens Co-authored-by: peartree --- ...iding-by-the-resourceserveridstrategy.yaml | 5 ++++ .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 5 ++++ .../dstu2/FhirResourceDaoDstu2UpdateTest.java | 29 +++++++++++++++++++ .../dstu3/FhirResourceDaoDstu3UpdateTest.java | 26 +++++++++++++++++ .../dao/r4/FhirResourceDaoR4UpdateTest.java | 20 ++++++++++++- 5 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4952-put-requests-for-resources-that-dont-exist-get-created-in-sequential-format-rather-than-abiding-by-the-resourceserveridstrategy.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4952-put-requests-for-resources-that-dont-exist-get-created-in-sequential-format-rather-than-abiding-by-the-resourceserveridstrategy.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4952-put-requests-for-resources-that-dont-exist-get-created-in-sequential-format-rather-than-abiding-by-the-resourceserveridstrategy.yaml new file mode 100644 index 00000000000..29c8ba9e9b0 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4952-put-requests-for-resources-that-dont-exist-get-created-in-sequential-format-rather-than-abiding-by-the-resourceserveridstrategy.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 4952 +title: "Previously, the resource server Id strategy was not honoured when doing a create with a conditional update +operation on a resource where the ID was not assigned. This is now fixed." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java index 4a2cbd39981..2f3db26a966 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java @@ -1894,6 +1894,11 @@ public abstract class BaseHapiFhirResourceDao extends B } } } else { + // assign UUID if no id provided in the request (numeric id mode is handled in doCreateForPostOrPut) + if (!theResource.getIdElement().hasIdPart() && getStorageSettings().getResourceServerIdStrategy() == JpaStorageSettings.IdStrategyEnum.UUID) { + theResource.setId(UUID.randomUUID().toString()); + theResource.setUserData(JpaConstants.RESOURCE_ID_SERVER_ASSIGNED, Boolean.TRUE); + } DaoMethodOutcome outcome = doCreateForPostOrPut(theRequest, resource, theMatchUrl, false, thePerformIndexing, theRequestPartitionId, update, theTransactionDetails); // Pre-cache the match URL diff --git a/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2UpdateTest.java b/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2UpdateTest.java index a779870745a..21ae34a0335 100644 --- a/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2UpdateTest.java +++ b/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2UpdateTest.java @@ -1,6 +1,7 @@ package ca.uhn.fhir.jpa.dao.dstu2; import ca.uhn.fhir.i18n.Msg; +import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; import ca.uhn.fhir.jpa.model.dao.JpaPid; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.model.api.ResourceMetadataKeyEnum; @@ -17,6 +18,7 @@ import ca.uhn.fhir.rest.api.MethodOutcome; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; import org.hl7.fhir.instance.model.api.IIdType; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import java.io.IOException; @@ -24,6 +26,7 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.UUID; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; @@ -39,6 +42,11 @@ public class FhirResourceDaoDstu2UpdateTest extends BaseJpaDstu2Test { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoDstu2UpdateTest.class); + @AfterEach + public void afterEach(){ + myStorageSettings.setResourceServerIdStrategy(JpaStorageSettings.IdStrategyEnum.SEQUENTIAL_NUMERIC); + } + @Test public void testUpdateByUrl() { String methodName = "testUpdateByUrl"; @@ -350,4 +358,25 @@ public class FhirResourceDaoDstu2UpdateTest extends BaseJpaDstu2Test { } + + @Test + void testCreateWithConditionalUpdate_withUuidAsServerResourceStrategyAndNoIdProvided_uuidAssignedAsResourceId() { + // setup + myStorageSettings.setResourceServerIdStrategy(JpaStorageSettings.IdStrategyEnum.UUID); + Patient p = new Patient(); + p.addIdentifier().setSystem("http://my-lab-system").setValue("123"); + p.addName().addFamily("FAM"); + String theMatchUrl = "Patient?identifier=http://my-lab-system|123"; + + // execute + String result = myPatientDao.update(p, theMatchUrl, mySrd).getId().getIdPart(); + + // verify + try { + UUID.fromString(result); + } catch (IllegalArgumentException exception){ + fail("Result id is not a UUID. Instead, it was: " + result); + } + } + } diff --git a/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3UpdateTest.java b/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3UpdateTest.java index 8ab5d48b2e8..0d7bbf88437 100644 --- a/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3UpdateTest.java +++ b/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3UpdateTest.java @@ -29,6 +29,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.TimeZone; +import java.util.UUID; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; @@ -43,6 +44,11 @@ import static org.junit.jupiter.api.Assertions.fail; public class FhirResourceDaoDstu3UpdateTest extends BaseJpaDstu3Test { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoDstu3UpdateTest.class); + @AfterEach + public void afterEach(){ + myStorageSettings.setResourceServerIdStrategy(JpaStorageSettings.IdStrategyEnum.SEQUENTIAL_NUMERIC); + } + @Test public void testReCreateMatchResource() { @@ -827,4 +833,24 @@ public class FhirResourceDaoDstu3UpdateTest extends BaseJpaDstu3Test { } + @Test + void testCreateWithConditionalUpdate_withUuidAsServerResourceStrategyAndNoIdProvided_uuidAssignedAsResourceId() { + // setup + myStorageSettings.setResourceServerIdStrategy(JpaStorageSettings.IdStrategyEnum.UUID); + Patient p = new Patient(); + p.addIdentifier().setSystem("http://my-lab-system").setValue("123"); + p.addName().setFamily("FAM"); + String theMatchUrl = "Patient?identifier=http://my-lab-system|123"; + + // execute + String result = myPatientDao.update(p, theMatchUrl, mySrd).getId().getIdPart(); + + // verify + try { + UUID.fromString(result); + } catch (IllegalArgumentException exception){ + fail("Result id is not a UUID. Instead, it was: " + result); + } + } + } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java index 07552eeb139..3dcd8508609 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java @@ -52,7 +52,6 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.matchesPattern; -import static org.hamcrest.Matchers.not; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -71,6 +70,7 @@ public class FhirResourceDaoR4UpdateTest extends BaseJpaR4Test { myStorageSettings.setIndexMissingFields(new JpaStorageSettings().getIndexMissingFields()); myStorageSettings.setResourceServerIdStrategy(new JpaStorageSettings().getResourceServerIdStrategy()); myStorageSettings.setResourceClientIdStrategy(new JpaStorageSettings().getResourceClientIdStrategy()); + myStorageSettings.setResourceServerIdStrategy(JpaStorageSettings.IdStrategyEnum.SEQUENTIAL_NUMERIC); } @@ -1152,6 +1152,24 @@ public class FhirResourceDaoR4UpdateTest extends BaseJpaR4Test { } + @Test + void testCreateWithConditionalUpdate_withUuidAsServerResourceStrategyAndNoIdProvided_uuidAssignedAsResourceId() { + // setup + myStorageSettings.setResourceServerIdStrategy(JpaStorageSettings.IdStrategyEnum.UUID); + Patient p = new Patient(); + p.addIdentifier().setSystem("http://my-lab-system").setValue("123"); + p.addName().setFamily("FAM"); + String theMatchUrl = "Patient?identifier=http://my-lab-system|123"; + // execute + String result = myPatientDao.update(p, theMatchUrl, mySrd).getId().getIdPart(); + + // verify + try { + UUID.fromString(result); + } catch (IllegalArgumentException exception){ + fail("Result id is not a UUID. Instead, it was: " + result); + } + } }