From ec1b8fe7ee194c0a94aceeb59933537b974efc48 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 31 Jan 2024 17:20:38 -0500 Subject: [PATCH] Fix conditional creates without leading '?' (#5646) * First commit with failing test. * More tests and logs. * More logs * Try new solution for BaseTransactionProcessor.performIdSubstitutionsInMatchUrl(). * Simplify solution. Add more tests. * Changelog. * javadoc --- .../java/ca/uhn/fhir/util/BundleBuilder.java | 16 ++- ...itional-create-reference-query-string.yaml | 6 + .../jpa/dao/ResourceHistoryCalculator.java | 19 +++ .../fhir/jpa/dao/ResourceHistoryState.java | 19 +++ .../dao/r4/FhirResourceDaoR4CreateTest.java | 121 ++++++++++++++++++ .../test/BaseTransactionProcessorTest.java | 17 +++ .../jpa/dao/BaseTransactionProcessor.java | 3 +- 7 files changed, 197 insertions(+), 4 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5651-bundle-conditional-create-reference-query-string.yaml diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/BundleBuilder.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/BundleBuilder.java index 842fd855508..c5f25cb3744 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/BundleBuilder.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/BundleBuilder.java @@ -251,10 +251,22 @@ public class BundleBuilder { * @param theResource The resource to create */ public CreateBuilder addTransactionCreateEntry(IBaseResource theResource) { + return addTransactionCreateEntry(theResource, null); + } + + /** + * Adds an entry containing an create (POST) request. + * Also sets the Bundle.type value to "transaction" if it is not already set. + * + * @param theResource The resource to create + * @param theFullUrl The fullUrl to attach to the entry. If null, will default to the resource ID. + */ + public CreateBuilder addTransactionCreateEntry(IBaseResource theResource, @Nullable String theFullUrl) { setBundleField("type", "transaction"); - IBase request = - addEntryAndReturnRequest(theResource, theResource.getIdElement().getValue()); + IBase request = addEntryAndReturnRequest( + theResource, + theFullUrl != null ? theFullUrl : theResource.getIdElement().getValue()); String resourceType = myContext.getResourceType(theResource); diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5651-bundle-conditional-create-reference-query-string.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5651-bundle-conditional-create-reference-query-string.yaml new file mode 100644 index 00000000000..df8d742600e --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5651-bundle-conditional-create-reference-query-string.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 5651 +jira: SMILE-7855 +title: "Previously, conditional creates would fail with HAPI-0929 errors if there was no preceding '?'. + This has been fixed." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryCalculator.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryCalculator.java index fafa3c4ca29..1114af1a887 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryCalculator.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryCalculator.java @@ -1,3 +1,22 @@ +/*- + * #%L + * HAPI FHIR JPA Server + * %% + * Copyright (C) 2014 - 2024 Smile CDR, Inc. + * %% + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * #L% + */ package ca.uhn.fhir.jpa.dao; import ca.uhn.fhir.context.FhirContext; diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryState.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryState.java index 6da5f9ab594..bdf5cdfa8ee 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryState.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/ResourceHistoryState.java @@ -1,3 +1,22 @@ +/*- + * #%L + * HAPI FHIR JPA Server + * %% + * Copyright (C) 2014 - 2024 Smile CDR, Inc. + * %% + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * #L% + */ package ca.uhn.fhir.jpa.dao; import ca.uhn.fhir.jpa.model.entity.ResourceEncodingEnum; diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4CreateTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4CreateTest.java index db30329cbad..a4747add0e2 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4CreateTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4CreateTest.java @@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.dao.r4; import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; +import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao; import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome; import ca.uhn.fhir.jpa.model.entity.NormalizedQuantitySearchLevel; import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable; @@ -29,6 +30,7 @@ import ca.uhn.fhir.util.ClasspathUtil; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.time.DateUtils; import org.exparity.hamcrest.date.DateMatchers; +import org.hl7.fhir.instance.model.api.IBaseBundle; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.Bundle; @@ -39,6 +41,7 @@ import org.hl7.fhir.r4.model.DecimalType; import org.hl7.fhir.r4.model.Encounter; import org.hl7.fhir.r4.model.Enumerations; import org.hl7.fhir.r4.model.IdType; +import org.hl7.fhir.r4.model.Identifier; import org.hl7.fhir.r4.model.InstantType; import org.hl7.fhir.r4.model.Observation; import org.hl7.fhir.r4.model.Organization; @@ -48,8 +51,12 @@ import org.hl7.fhir.r4.model.Reference; import org.hl7.fhir.r4.model.SampledData; import org.hl7.fhir.r4.model.SearchParameter; import org.hl7.fhir.r4.model.StructureDefinition; +import org.hl7.fhir.r4.model.Task; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.data.domain.PageRequest; @@ -60,6 +67,7 @@ import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -77,6 +85,7 @@ import static org.hamcrest.Matchers.matchesPattern; import static org.hamcrest.Matchers.notNullValue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -1224,4 +1233,116 @@ public class FhirResourceDaoR4CreateTest extends BaseJpaR4Test { assertEquals(1, ids.size()); } + @Nested + class ConditionalCreates { + private static final String SYSTEM = "http://tempuri.org"; + private static final String VALUE_1 = "1"; + private static final String VALUE_2 = "2"; + + private final Task myTask1 = new Task() + .setStatus(Task.TaskStatus.DRAFT) + .setIntent(Task.TaskIntent.UNKNOWN) + .addIdentifier(new Identifier() + .setSystem(SYSTEM) + .setValue(VALUE_1)); + + private final Task myTask2 = new Task() + .setStatus(Task.TaskStatus.DRAFT) + .setIntent(Task.TaskIntent.UNKNOWN) + .addIdentifier(new Identifier() + .setSystem(SYSTEM) + .setValue(VALUE_2)) + .addBasedOn(new Reference().setReference("urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea")); + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testConditionalCreateDependsOnPOSTedResource(boolean theHasQuestionMark) { + final IFhirResourceDao taskDao = getTaskDao(); + taskDao.create(myTask1, new SystemRequestDetails()); + + final List allTasksPreBundle = searchAllTasks(); + assertEquals(1, allTasksPreBundle.size()); + final Task taskPreBundle = allTasksPreBundle.get(0); + assertEquals(VALUE_1, taskPreBundle.getIdentifier().get(0).getValue()); + assertEquals(SYSTEM, taskPreBundle.getIdentifier().get(0).getSystem()); + + final BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext); + + final String entryConditionalTemplate = "%sidentifier=http://tempuri.org|1"; + final String matchUrl = String.format(entryConditionalTemplate, theHasQuestionMark ? "?" : ""); + + bundleBuilder.addTransactionCreateEntry(myTask2) + .conditional(matchUrl); + + final List responseEntries = sendBundleAndGetResponse(bundleBuilder.getBundle()); + + assertEquals(1, responseEntries.size()); + + final Bundle.BundleEntryComponent bundleEntry = responseEntries.get(0); + + assertEquals("200 OK", bundleEntry.getResponse().getStatus()); + + final List allTasksPostBundle = searchAllTasks(); + assertEquals(1, allTasksPostBundle.size()); + final Task taskPostBundle = allTasksPostBundle.get(0); + assertEquals(VALUE_1, taskPostBundle.getIdentifier().get(0).getValue()); + assertEquals(SYSTEM, taskPostBundle.getIdentifier().get(0).getSystem()); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testConditionalCreateDependsOnFirstEntryExisting(boolean theHasQuestionMark) { + final BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext); + + bundleBuilder.addTransactionCreateEntry(myTask1, "urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea") + .conditional("identifier=http://tempuri.org|1"); + + final String secondEntryConditionalTemplate = "%sidentifier=http://tempuri.org|2&based-on=urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea"; + final String secondMatchUrl = String.format(secondEntryConditionalTemplate, theHasQuestionMark ? "?" : ""); + + bundleBuilder.addTransactionCreateEntry(myTask2) + .conditional(secondMatchUrl); + + final IBaseBundle requestBundle = bundleBuilder.getBundle(); + assertTrue(requestBundle instanceof Bundle); + + final List responseEntries = sendBundleAndGetResponse(requestBundle); + + assertEquals(2, responseEntries.size()); + assertEquals(Set.of("201 Created"), responseEntries.stream().map(Bundle.BundleEntryComponent::getResponse).map(Bundle.BundleEntryResponseComponent::getStatus).collect(Collectors.toUnmodifiableSet())); + + final List allTasksPostBundle = searchAllTasks(); + assertEquals(2, allTasksPostBundle.size()); + final Task taskPostBundle1 = allTasksPostBundle.get(0); + assertEquals(VALUE_1, taskPostBundle1.getIdentifier().get(0).getValue()); + assertEquals(SYSTEM, taskPostBundle1.getIdentifier().get(0).getSystem()); + final Task taskPostBundle2 = allTasksPostBundle.get(1); + assertEquals(VALUE_2, taskPostBundle2.getIdentifier().get(0).getValue()); + assertEquals(SYSTEM, taskPostBundle2.getIdentifier().get(0).getSystem()); + + final List task2BasedOn = taskPostBundle2.getBasedOn(); + assertEquals(1, task2BasedOn.size()); + final Reference task2BasedOnReference = task2BasedOn.get(0); + assertEquals(taskPostBundle1.getIdElement().toUnqualifiedVersionless().asStringValue(), task2BasedOnReference.getReference()); + } + } + + private List sendBundleAndGetResponse(IBaseBundle theRequestBundle) { + assertTrue(theRequestBundle instanceof Bundle); + + return mySystemDao.transaction(new SystemRequestDetails(), (Bundle)theRequestBundle).getEntry(); + } + + private List searchAllTasks() { + return unsafeCast(getTaskDao().search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()).getAllResources()); + } + + private IFhirResourceDao getTaskDao() { + return unsafeCast(myDaoRegistry.getResourceDao("Task")); + } + + @SuppressWarnings("unchecked") + private static T unsafeCast(Object theObject) { + return (T)theObject; + } } diff --git a/hapi-fhir-storage-test-utilities/src/test/java/ca/uhn/fhir/storage/test/BaseTransactionProcessorTest.java b/hapi-fhir-storage-test-utilities/src/test/java/ca/uhn/fhir/storage/test/BaseTransactionProcessorTest.java index f8e70e6b624..d19088baaf9 100644 --- a/hapi-fhir-storage-test-utilities/src/test/java/ca/uhn/fhir/storage/test/BaseTransactionProcessorTest.java +++ b/hapi-fhir-storage-test-utilities/src/test/java/ca/uhn/fhir/storage/test/BaseTransactionProcessorTest.java @@ -110,4 +110,21 @@ public class BaseTransactionProcessorTest { assertTrue(matchResult, "Failed to find a Regex match using Url '" + matchUrl + "'"); } + @Test + void identifierSubstitutionNoQuestionMark() { + final IdSubstitutionMap idSubstitutions = new IdSubstitutionMap(); + idSubstitutions.put(new IdType("Task/urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea"), new IdType("Task/1/history/1")); + idSubstitutions.put(new IdType("urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea"), new IdType("Task/1/_history/1")); + final String outcome = BaseTransactionProcessor.performIdSubstitutionsInMatchUrl(idSubstitutions, "identifier=http://tempuri.org|2&based-on=urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea"); + assertEquals("identifier=http://tempuri.org|2&based-on=Task/1", outcome); + } + + @Test + void identifierSubstitutionYesQuestionMar() { + final IdSubstitutionMap idSubstitutions = new IdSubstitutionMap(); + idSubstitutions.put(new IdType("Task/urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea"), new IdType("Task/1/history/1")); + idSubstitutions.put(new IdType("urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea"), new IdType("Task/1/_history/1")); + final String outcome = BaseTransactionProcessor.performIdSubstitutionsInMatchUrl(idSubstitutions, "?identifier=http://tempuri.org|2&based-on=urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea"); + assertEquals("?identifier=http://tempuri.org|2&based-on=Task/1", outcome); + } } diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index 05f912d0150..4c4c153eec8 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -2253,8 +2253,7 @@ public abstract class BaseTransactionProcessor { public static String performIdSubstitutionsInMatchUrl(IdSubstitutionMap theIdSubstitutions, String theMatchUrl) { String matchUrl = theMatchUrl; if (isNotBlank(matchUrl) && !theIdSubstitutions.isEmpty()) { - - int startIdx = matchUrl.indexOf('?'); + int startIdx = 0; while (startIdx != -1) { int endIdx = matchUrl.indexOf('&', startIdx + 1);