Return HAPI-2498 if a REST request contains an :identifier modifier (#5696)

* Failing tests and logging.

* Spotless.

* Fix error message for bad modifiers.  Add more tests.

* Spotless.

* Fix conditional logic for detecting modifiers to check for colon (:).

* Finalize tests.  Fix animal sniffer error.  Remove logging code.  Finalize changelog.

* Try again to fix animal sniffer error.

* Null handling on request.

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java

@@ -25,6 +25,7 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -232,6 +233,8 @@ public class Constants {
 	public static final String PARAMQUALIFIER_NICKNAME = ":nickname";
 	public static final String PARAMQUALIFIER_TOKEN_OF_TYPE = ":of-type";
 	public static final String PARAMQUALIFIER_TOKEN_NOT = ":not";
+	public static final String PARAMQUALIFIER_TOKEN_IDENTIFIER = ":identifier";
+
 	public static final int STATUS_HTTP_200_OK = 200;
 	public static final int STATUS_HTTP_201_CREATED = 201;
 	public static final int STATUS_HTTP_204_NO_CONTENT = 204;
@@ -314,6 +317,17 @@ public class Constants {
 	public static final String PARAMQUALIFIER_TOKEN_NOT_IN = ":not-in";
 	public static final String PARAMQUALIFIER_TOKEN_ABOVE = ":above";
 	public static final String PARAMQUALIFIER_TOKEN_BELOW = ":below";
+
+	public static final List<String> VALID_MODIFIERS = Collections.unmodifiableList(Arrays.asList(
+			PARAMQUALIFIER_STRING_CONTAINS,
+			PARAMQUALIFIER_STRING_EXACT,
+			PARAMQUALIFIER_TOKEN_IN,
+			PARAM_INCLUDE_QUALIFIER_ITERATE,
+			PARAMQUALIFIER_MISSING,
+			PARAMQUALIFIER_TOKEN_NOT_IN,
+			PARAMQUALIFIER_TOKEN_OF_TYPE,
+			PARAM_INCLUDE_QUALIFIER_RECURSE,
+			PARAMQUALIFIER_TOKEN_TEXT));
 	/**
 	 * The number of characters in a UUID (36)
 	 */

hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties

@@ -211,3 +211,5 @@ ca.uhn.fhir.jpa.provider.DiffProvider.cantDiffDifferentTypes=Unable to diff two
 
 ca.uhn.fhir.jpa.interceptor.validation.RuleRequireProfileDeclaration.noMatchingProfile=Resource of type "{0}" does not declare conformance to profile from: {1} 
 ca.uhn.fhir.jpa.interceptor.validation.RuleRequireProfileDeclaration.illegalProfile=Resource of type "{0}" must not declare conformance to profile: {1}
+
+ca.uhn.fhir.jpa.search.SearchCoordinatorSvcImpl.invalidUseOfSearchIdentifier=Unsupported search modifier(s): "{0}" for resource type "{1}". Valid search modifiers are: {2}

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5701-search-by-identifier-inconsistent-results.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 5701
+title: "Previously, invoking search URLs containing ':identifier' would result in a HAPI-1250 error complaining about
+        an invalid resource type.
+        This has been fixed by returning a clearer error message for this specific condition: HAPI-2498."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilder.java

@@ -49,7 +49,6 @@ import ca.uhn.fhir.jpa.util.QueryParameterUtils;
 import ca.uhn.fhir.model.api.IQueryParameterType;
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.parser.DataFormatException;
-import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.RestSearchParameterTypeEnum;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.param.ReferenceParam;
@@ -60,6 +59,7 @@ import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
 import ca.uhn.fhir.rest.server.util.CompositeInterceptorBroadcaster;
 import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Lists;
 import com.healthmarketscience.sqlbuilder.BinaryCondition;
 import com.healthmarketscience.sqlbuilder.ComboCondition;
@@ -84,16 +84,20 @@ import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.ListIterator;
+import java.util.Optional;
 import java.util.Set;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 import static ca.uhn.fhir.jpa.search.builder.QueryStack.SearchForIdsParams.with;
+import static ca.uhn.fhir.rest.api.Constants.*;
 import static org.apache.commons.lang3.StringUtils.isBlank;
 import static org.apache.commons.lang3.StringUtils.trim;
 
 public class ResourceLinkPredicateBuilder extends BaseJoiningPredicateBuilder implements ICanMakeMissingParamPredicate {
 
 	private static final Logger ourLog = LoggerFactory.getLogger(ResourceLinkPredicateBuilder.class);
+	private static final Pattern MODIFIER_REPLACE_PATTERN = Pattern.compile(".*:");
 	private final DbColumn myColumnSrcType;
 	private final DbColumn myColumnSrcPath;
 	private final DbColumn myColumnTargetResourceId;
@@ -205,6 +209,7 @@ public class ResourceLinkPredicateBuilder extends BaseJoiningPredicateBuilder im
 							targetQualifiedUrls.add(dt.getValue());
 						}
 					} else {
+						validateModifierUse(theRequest, theResourceType, ref);
 						validateResourceTypeInReferenceParam(ref.getResourceType());
 						targetIds.add(dt);
 					}
@@ -258,6 +263,41 @@ public class ResourceLinkPredicateBuilder extends BaseJoiningPredicateBuilder im
 		}
 	}
 
+	private void validateModifierUse(RequestDetails theRequest, String theResourceType, ReferenceParam theRef) {
+		try {
+			final String resourceTypeFromRef = theRef.getResourceType();
+			if (StringUtils.isEmpty(resourceTypeFromRef)) {
+				return;
+			}
+			// TODO: LD: unless we do this, ResourceProviderR4Test#testSearchWithSlashes will fail due to its
+			// derived-from: syntax
+			getFhirContext().getResourceDefinition(resourceTypeFromRef);
+		} catch (DataFormatException e) {
+			final List<String> nonMatching = Optional.ofNullable(theRequest)
+					.map(RequestDetails::getParameters)
+					.map(params -> params.keySet().stream()
+							.filter(mod -> mod.contains(":"))
+							.map(MODIFIER_REPLACE_PATTERN::matcher)
+							.map(pattern -> pattern.replaceAll(":"))
+							.filter(mod -> !VALID_MODIFIERS.contains(mod))
+							.distinct()
+							.collect(Collectors.toUnmodifiableList()))
+					.orElse(Collections.emptyList());
+
+			if (!nonMatching.isEmpty()) {
+				final String msg = getFhirContext()
+						.getLocalizer()
+						.getMessageSanitized(
+								SearchCoordinatorSvcImpl.class,
+								"invalidUseOfSearchIdentifier",
+								nonMatching,
+								theResourceType,
+								VALID_MODIFIERS);
+				throw new InvalidRequestException(Msg.code(2498) + msg);
+			}
+		}
+	}
+
 	private void validateResourceTypeInReferenceParam(final String theResourceType) {
 		if (StringUtils.isEmpty(theResourceType)) {
 			return;
@@ -369,7 +409,7 @@ public class ResourceLinkPredicateBuilder extends BaseJoiningPredicateBuilder im
 		/*
 		 * Handle chain on _type
 		 */
-		if (Constants.PARAM_TYPE.equals(theReferenceParam.getChain())) {
+		if (PARAM_TYPE.equals(theReferenceParam.getChain())) {
 
 			List<String> pathsToMatch = createResourceLinkPaths(theResourceName, theParamName, theQualifiers);
 			Condition typeCondition = createPredicateSourcePaths(pathsToMatch);
@@ -775,4 +815,14 @@ public class ResourceLinkPredicateBuilder extends BaseJoiningPredicateBuilder im
 
 		return combineWithRequestPartitionIdPredicate(theParams.getRequestPartitionId(), unaryCondition);
 	}
+
+	@VisibleForTesting
+	void setSearchParamRegistryForUnitTest(ISearchParamRegistry theSearchParamRegistry) {
+		mySearchParamRegistry = theSearchParamRegistry;
+	}
+
+	@VisibleForTesting
+	void setIdHelperServiceForUnitTest(IIdHelperService theIdHelperService) {
+		myIdHelperService = theIdHelperService;
+	}
 }

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java

@@ -26,13 +26,11 @@ import ca.uhn.fhir.model.primitive.InstantDt;
 import ca.uhn.fhir.model.primitive.UriDt;
 import ca.uhn.fhir.parser.IParser;
 import ca.uhn.fhir.parser.StrictErrorHandler;
-import ca.uhn.fhir.rest.api.CacheControlDirective;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.MethodOutcome;
 import ca.uhn.fhir.rest.api.PreferReturnEnum;
 import ca.uhn.fhir.rest.api.SearchTotalModeEnum;
 import ca.uhn.fhir.rest.api.SummaryEnum;
-import ca.uhn.fhir.rest.api.server.IRestfulServer;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.client.apache.ResourceEntity;
 import ca.uhn.fhir.rest.client.api.IClientInterceptor;
@@ -45,7 +43,6 @@ import ca.uhn.fhir.rest.gclient.NumberClientParam;
 import ca.uhn.fhir.rest.gclient.StringClientParam;
 import ca.uhn.fhir.rest.param.DateRangeParam;
 import ca.uhn.fhir.rest.param.ParamPrefixEnum;
-import ca.uhn.fhir.rest.server.IPagingProvider;
 import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException;
@@ -58,6 +55,7 @@ import ca.uhn.fhir.util.TestUtil;
 import ca.uhn.fhir.util.UrlUtil;
 import com.google.common.base.Charsets;
 import com.google.common.collect.Lists;
+import jakarta.annotation.Nonnull;
 import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -116,7 +114,9 @@ import org.hl7.fhir.r4.model.Enumerations;
 import org.hl7.fhir.r4.model.Enumerations.AdministrativeGender;
 import org.hl7.fhir.r4.model.Extension;
 import org.hl7.fhir.r4.model.Group;
+import org.hl7.fhir.r4.model.HumanName;
 import org.hl7.fhir.r4.model.IdType;
+import org.hl7.fhir.r4.model.Identifier;
 import org.hl7.fhir.r4.model.ImagingStudy;
 import org.hl7.fhir.r4.model.InstantType;
 import org.hl7.fhir.r4.model.Location;
@@ -167,14 +167,12 @@ import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.CsvSource;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.junit.jupiter.params.provider.ValueSource;
-import org.mockito.Spy;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.test.util.AopTestUtils;
 import org.springframework.transaction.TransactionStatus;
 import org.springframework.transaction.support.TransactionCallbackWithoutResult;
 import org.springframework.transaction.support.TransactionTemplate;
 
-import jakarta.annotation.Nonnull;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
@@ -191,7 +189,6 @@ import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -227,8 +224,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
 @SuppressWarnings("Duplicates")
@@ -7423,4 +7418,49 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test {
 			);
 		}
 	}
+	@Nested
+	class SearchWithIdentifiers {
+		private static final String SYSTEM = "http://acme.org/fhir/identifier/mrn";
+		private static final String VALUE = "123456";
+		private IIdType myPatientId;
+
+		@BeforeEach
+		void beforeEach() {
+			final Patient patient = new Patient();
+			patient.addIdentifier().setValue(VALUE).setSystem(SYSTEM);
+			patient.addName().setUse(HumanName.NameUse.OFFICIAL).setFamily("Abbott").addGiven("Elias").addPrefix("Mr.");
+			patient.setGender(Enumerations.AdministrativeGender.MALE);
+
+			myPatientId = myClient.create().resource(patient).execute().getResource().getIdElement();
+
+			final Observation observation = new Observation();
+			observation.setStatus(Observation.ObservationStatus.FINAL);
+			observation.setCode(new CodeableConcept().addCoding(new Coding().setSystem("http://loinc.org").setCode("15074-8").setDisplay("Glucose [Moles/volume] in Blood")));
+			observation.setSubject(new Reference(myPatientId.toUnqualifiedVersionless()).setIdentifier(new Identifier().setSystem(SYSTEM).setValue(VALUE)));
+
+			myClient.create().resource(observation).execute().getResource().getIdElement();
+		}
+
+		@Test
+		void searchWithIdentifierToIdentifier() {
+			testAndAssertFailureFor("Observation?subject:identifier=http://acme.org/fhir/identifier/mrn|123456");
+		}
+
+		@Test
+		void searchWithIdentifierToId() {
+			testAndAssertFailureFor(String.format("Observation?subject:identifier=%s", myPatientId.getIdPart()));
+		}
+
+		private void testAndAssertFailureFor(String theUrl) {
+			try {
+				myClient.search()
+					.byUrl(theUrl)
+					.returnBundle(Bundle.class)
+					.execute();
+				fail();
+			} catch (InvalidRequestException exception) {
+				assertEquals("HTTP 400 Bad Request: HAPI-2498: Unsupported search modifier(s): \"[:identifier]\" for resource type \"Observation\". Valid search modifiers are: [:contains, :exact, :in, :iterate, :missing, :not-in, :of-type, :recurse, :text]", exception.getMessage());
+			}
+		}
+	}
 }

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilderTest.java

@@ -1,6 +1,14 @@
 package ca.uhn.fhir.jpa.search.builder.predicate;
 
+import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.interceptor.model.RequestPartitionId;
+import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
 import ca.uhn.fhir.jpa.search.builder.sql.SearchQueryBuilder;
+import ca.uhn.fhir.model.api.IQueryParameterType;
+import ca.uhn.fhir.model.primitive.IdDt;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
+import ca.uhn.fhir.rest.param.ReferenceParam;
+import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
 import com.healthmarketscience.sqlbuilder.BinaryCondition;
 import com.healthmarketscience.sqlbuilder.Condition;
 import com.healthmarketscience.sqlbuilder.InCondition;
@@ -15,10 +23,14 @@ import org.mockito.Mockito;
 import org.mockito.junit.jupiter.MockitoExtension;
 
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.UUID;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
 import static org.mockito.ArgumentMatchers.anyCollection;
 import static org.mockito.Mockito.when;
 
@@ -26,11 +38,18 @@ import static org.mockito.Mockito.when;
 public class ResourceLinkPredicateBuilderTest {
 
     private static final String PLACEHOLDER_BASE = UUID.randomUUID().toString();
+
 	private ResourceLinkPredicateBuilder myResourceLinkPredicateBuilder;
 
     @Mock
     private SearchQueryBuilder mySearchQueryBuilder;
 
+	@Mock
+	private ISearchParamRegistry mySearchParamRegistry;
+
+	@Mock
+	private IIdHelperService myIdHelperService;
+
 	@BeforeEach
 	public void init() {
 		DbSpec spec = new DbSpec();
@@ -38,6 +57,8 @@ public class ResourceLinkPredicateBuilderTest {
 		DbTable table = new DbTable(schema, "table");
 		when(mySearchQueryBuilder.addTable(Mockito.anyString())).thenReturn(table);
         myResourceLinkPredicateBuilder = new ResourceLinkPredicateBuilder(null, mySearchQueryBuilder, false);
+		myResourceLinkPredicateBuilder.setSearchParamRegistryForUnitTest(mySearchParamRegistry);
+		myResourceLinkPredicateBuilder.setIdHelperServiceForUnitTest(myIdHelperService);
 	}
 
 	@Test
@@ -59,4 +80,25 @@ public class ResourceLinkPredicateBuilderTest {
         Condition condition = myResourceLinkPredicateBuilder.createEverythingPredicate("Patient", new ArrayList<>(), new Long[0]);
         assertEquals(BinaryCondition.class, condition.getClass());
 	}
+
+	@Test
+	void validateInvalidModifiers() {
+		when(mySearchQueryBuilder.getFhirContext()).thenReturn(FhirContext.forR4Cached());
+		final ReferenceParam referenceParam = new ReferenceParam(new IdDt(":identifier", "123"));
+		final List<IQueryParameterType> referenceOrParamList = List.of(referenceParam);
+		final SystemRequestDetails requestDetails = new SystemRequestDetails();
+		final Map<String, String[]> params = new LinkedHashMap<>();
+		params.put("subject:identifier", new String[]{"1"});
+		params.put("subject:x", new String[]{"2"});
+		params.put("subject:y", new String[]{"3"});
+		params.put("patient", new String[]{"4"});
+		requestDetails.setParameters(params);
+
+		try {
+			myResourceLinkPredicateBuilder.createPredicate(requestDetails, "Observation", "", Collections.emptyList(), referenceOrParamList, null, RequestPartitionId.allPartitions());
+			fail();
+		} catch (Exception exception) {
+			assertEquals("HAPI-2498: Unsupported search modifier(s): \"[:identifier, :x, :y]\" for resource type \"Observation\". Valid search modifiers are: [:contains, :exact, :in, :iterate, :missing, :not-in, :of-type, :recurse, :text]", exception.getMessage());
+		}
+	}
 }