diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java index 5d43ea2052d..0c962c63340 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/Constants.java +++ b/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 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) */ diff --git a/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties b/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties index 849dc2b6bac..fae7bf1c491 100644 --- a/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties +++ b/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} diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5701-search-by-identifier-inconsistent-results.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5701-search-by-identifier-inconsistent-results.yaml new file mode 100644 index 00000000000..0ec9cea92af --- /dev/null +++ b/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." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilder.java index 0ee46dacf07..3848158b1d8 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilder.java +++ b/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 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 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; + } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java index 6c28d39348a..59f5519c007 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java +++ b/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()); + } + } + } } diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilderTest.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilderTest.java index 36766739cab..9da4f0b4cf2 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilderTest.java +++ b/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 referenceOrParamList = List.of(referenceParam); + final SystemRequestDetails requestDetails = new SystemRequestDetails(); + final Map 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()); + } + } }