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

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 =
+		IBase request = addEntryAndReturnRequest(
-				addEntryAndReturnRequest(theResource, theResource.getIdElement().getValue());
+				theResource,
+				theFullUrl != null ? theFullUrl : theResource.getIdElement().getValue());
 
 		String resourceType = myContext.getResourceType(theResource);
 

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."

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;

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;

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<Task> taskDao = getTaskDao();
+			taskDao.create(myTask1, new SystemRequestDetails());
+
+			final List<Task> 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<Bundle.BundleEntryComponent> responseEntries = sendBundleAndGetResponse(bundleBuilder.getBundle());
+
+			assertEquals(1, responseEntries.size());
+
+			final Bundle.BundleEntryComponent bundleEntry = responseEntries.get(0);
+
+			assertEquals("200 OK", bundleEntry.getResponse().getStatus());
+
+			final List<Task> 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<Bundle.BundleEntryComponent> 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<Task> 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<Reference> task2BasedOn = taskPostBundle2.getBasedOn();
+			assertEquals(1, task2BasedOn.size());
+			final Reference task2BasedOnReference = task2BasedOn.get(0);
+			assertEquals(taskPostBundle1.getIdElement().toUnqualifiedVersionless().asStringValue(), task2BasedOnReference.getReference());
+		}
+	}
+
+	private List<Bundle.BundleEntryComponent> sendBundleAndGetResponse(IBaseBundle theRequestBundle) {
+		assertTrue(theRequestBundle instanceof Bundle);
+
+		return mySystemDao.transaction(new SystemRequestDetails(), (Bundle)theRequestBundle).getEntry();
+	}
+
+	private List<Task> searchAllTasks() {
+		return unsafeCast(getTaskDao().search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()).getAllResources());
+	}
+
+	private IFhirResourceDao<Task> getTaskDao() {
+		return unsafeCast(myDaoRegistry.getResourceDao("Task"));
+	}
+
+	@SuppressWarnings("unchecked")
+	private static <T> T unsafeCast(Object theObject) {
+		return (T)theObject;
+	}
 }

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);
+	}
 }

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 = 0;
-			int startIdx = matchUrl.indexOf('?');
 			while (startIdx != -1) {
 
 				int endIdx = matchUrl.indexOf('&', startIdx + 1);