diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SearchMethodBinding.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SearchMethodBinding.java index 8f852762b09..066a7f751a7 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SearchMethodBinding.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SearchMethodBinding.java @@ -19,15 +19,6 @@ package ca.uhn.fhir.rest.server.method; * limitations under the License. * #L% */ -import static org.apache.commons.lang3.StringUtils.isBlank; -import static org.apache.commons.lang3.StringUtils.isNotBlank; - -import java.lang.reflect.Method; -import java.util.*; - -import org.apache.commons.lang3.StringUtils; -import org.hl7.fhir.instance.model.api.IAnyResource; -import org.hl7.fhir.instance.model.api.IBaseResource; import ca.uhn.fhir.context.ConfigurationException; import ca.uhn.fhir.context.FhirContext; @@ -44,27 +35,38 @@ import ca.uhn.fhir.rest.param.ParameterUtil; import ca.uhn.fhir.rest.param.QualifierDetails; import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import org.apache.commons.lang3.StringUtils; +import org.hl7.fhir.instance.model.api.IAnyResource; +import org.hl7.fhir.instance.model.api.IBaseResource; import javax.annotation.Nonnull; +import java.lang.reflect.Method; +import java.util.*; + +import static org.apache.commons.lang3.StringUtils.isBlank; +import static org.apache.commons.lang3.StringUtils.isNotBlank; public class SearchMethodBinding extends BaseResourceReturningMethodBinding { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SearchMethodBinding.class); private static final Set SPECIAL_SEARCH_PARAMS; - private String myCompartmentName; - private String myDescription; - private Integer myIdParamIndex; - private String myQueryName; - private boolean myAllowUnknownParams; - private final String myResourceProviderResourceName; static { HashSet specialSearchParams = new HashSet<>(); specialSearchParams.add(IAnyResource.SP_RES_ID); specialSearchParams.add(IAnyResource.SP_RES_LANGUAGE); + specialSearchParams.add(Constants.PARAM_INCLUDE); + specialSearchParams.add(Constants.PARAM_REVINCLUDE); SPECIAL_SEARCH_PARAMS = Collections.unmodifiableSet(specialSearchParams); } + private final String myResourceProviderResourceName; + private String myCompartmentName; + private String myDescription; + private Integer myIdParamIndex; + private String myQueryName; + private boolean myAllowUnknownParams; + public SearchMethodBinding(Class theReturnResourceType, Class theResourceProviderResourceType, Method theMethod, FhirContext theContext, Object theProvider) { super(theReturnResourceType, theMethod, theContext, theProvider); Search search = theMethod.getAnnotation(Search.class); @@ -90,11 +92,11 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding { throw new ConfigurationException(msg); } - if (theResourceProviderResourceType != null) { - this.myResourceProviderResourceName = theContext.getResourceDefinition(theResourceProviderResourceType).getName(); - } else { - this.myResourceProviderResourceName = null; - } + if (theResourceProviderResourceType != null) { + this.myResourceProviderResourceName = theContext.getResourceDefinition(theResourceProviderResourceType).getName(); + } else { + this.myResourceProviderResourceName = null; + } } @@ -106,8 +108,8 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding { return myQueryName; } - public String getResourceProviderResourceName() { - return myResourceProviderResourceName; + public String getResourceProviderResourceName() { + return myResourceProviderResourceName; } @Nonnull @@ -123,12 +125,12 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding { @Override public ReturnTypeEnum getReturnType() { - return ReturnTypeEnum.BUNDLE; + return ReturnTypeEnum.BUNDLE; } @Override public boolean incomingServerRequestMatchesMethod(RequestDetails theRequest) { - + if (theRequest.getId() != null && myIdParamIndex == null) { ourLog.trace("Method {} doesn't match because ID is not null: {}", getMethod(), theRequest.getId()); return false; @@ -155,27 +157,26 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding { Set unqualifiedNames = theRequest.getUnqualifiedToQualifiedNames().keySet(); Set qualifiedParamNames = theRequest.getParameters().keySet(); - for (int i = 0; i < this.getParameters().size(); i++) { - if (!(getParameters().get(i) instanceof BaseQueryParameter)) { + for (IParameter nextParameter : getParameters()) { + if (!(nextParameter instanceof BaseQueryParameter)) { continue; } - BaseQueryParameter temp = (BaseQueryParameter) getParameters().get(i); - String name = temp.getName(); - if (temp.isRequired()) { + BaseQueryParameter nextQueryParameter = (BaseQueryParameter) nextParameter; + String name = nextQueryParameter.getName(); + if (nextQueryParameter.isRequired()) { if (qualifiedParamNames.contains(name)) { QualifierDetails qualifiers = extractQualifiersFromParameterName(name); - if (qualifiers.passes(temp.getQualifierWhitelist(), temp.getQualifierBlacklist())) { + if (qualifiers.passes(nextQueryParameter.getQualifierWhitelist(), nextQueryParameter.getQualifierBlacklist())) { methodParamsTemp.add(name); } } if (unqualifiedNames.contains(name)) { List qualifiedNames = theRequest.getUnqualifiedToQualifiedNames().get(name); - qualifiedNames = processWhitelistAndBlacklist(qualifiedNames, temp.getQualifierWhitelist(), temp.getQualifierBlacklist()); + qualifiedNames = processWhitelistAndBlacklist(qualifiedNames, nextQueryParameter.getQualifierWhitelist(), nextQueryParameter.getQualifierBlacklist()); methodParamsTemp.addAll(qualifiedNames); } - if (!qualifiedParamNames.contains(name) && !unqualifiedNames.contains(name)) - { + if (!qualifiedParamNames.contains(name) && !unqualifiedNames.contains(name)) { ourLog.trace("Method {} doesn't match param '{}' is not present", getMethod().getName(), name); return false; } @@ -183,16 +184,16 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding { } else { if (qualifiedParamNames.contains(name)) { QualifierDetails qualifiers = extractQualifiersFromParameterName(name); - if (qualifiers.passes(temp.getQualifierWhitelist(), temp.getQualifierBlacklist())) { + if (qualifiers.passes(nextQueryParameter.getQualifierWhitelist(), nextQueryParameter.getQualifierBlacklist())) { methodParamsTemp.add(name); } - } + } if (unqualifiedNames.contains(name)) { List qualifiedNames = theRequest.getUnqualifiedToQualifiedNames().get(name); - qualifiedNames = processWhitelistAndBlacklist(qualifiedNames, temp.getQualifierWhitelist(), temp.getQualifierBlacklist()); + qualifiedNames = processWhitelistAndBlacklist(qualifiedNames, nextQueryParameter.getQualifierWhitelist(), nextQueryParameter.getQualifierBlacklist()); methodParamsTemp.addAll(qualifiedNames); } - if (!qualifiedParamNames.contains(name)) { + if (!qualifiedParamNames.contains(name)) { methodParamsTemp.add(name); } } @@ -271,6 +272,7 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding { public String toString() { return getMethod().toString(); } + public static QualifierDetails extractQualifiersFromParameterName(String theParamName) { QualifierDetails retVal = new QualifierDetails(); if (theParamName == null || theParamName.length() == 0) { diff --git a/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/SearchMethodBindingTest.java b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/SearchMethodBindingTest.java new file mode 100644 index 00000000000..11bc0a42426 --- /dev/null +++ b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/SearchMethodBindingTest.java @@ -0,0 +1,111 @@ +package ca.uhn.fhir.rest.server.method; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.context.RuntimeResourceDefinition; +import ca.uhn.fhir.rest.annotation.OptionalParam; +import ca.uhn.fhir.rest.annotation.RequiredParam; +import ca.uhn.fhir.rest.annotation.Search; +import ca.uhn.fhir.rest.api.RequestTypeEnum; +import ca.uhn.fhir.rest.api.server.RequestDetails; +import com.google.common.collect.ImmutableMap; +import org.hamcrest.Matchers; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.util.Map; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class SearchMethodBindingTest { + + private static final TestResourceProvider TEST_RESOURCE_PROVIDER = new TestResourceProvider(); + + private FhirContext fhirContext; + + @Before + public void setUp() { + fhirContext = mock(FhirContext.class); + RuntimeResourceDefinition definition = mock(RuntimeResourceDefinition.class); + when(definition.isBundle()).thenReturn(false); + when(fhirContext.getResourceDefinition(any(Class.class))).thenReturn(definition); + } + + @Test // fails + public void methodShouldNotMatchWhenUnderscoreQueryParameter() throws NoSuchMethodException { + Assert.assertThat(getBinding("param", String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "_include", new String[]{"test"}))), + Matchers.is(false)); + Assert.assertThat(getBinding("paramAndTest", String.class, String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "_include", new String[]{"test"}))), + Matchers.is(false)); + Assert.assertThat(getBinding("paramAndUnderscoreTest", String.class, String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "_include", new String[]{"test"}))), + Matchers.is(false)); + } + + @Test + public void methodShouldNotMatchWhenExtraQueryParameter() throws NoSuchMethodException { + Assert.assertThat(getBinding("param", String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "extra", new String[]{"test"}))), + Matchers.is(false)); + Assert.assertThat(getBinding("paramAndTest", String.class, String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "extra", new String[]{"test"}))), + Matchers.is(false)); + Assert.assertThat(getBinding("paramAndUnderscoreTest", String.class, String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "extra", new String[]{"test"}))), + Matchers.is(false)); + } + + @Test + public void methodMatchesOwnParams() throws NoSuchMethodException { + Assert.assertThat(getBinding("param", String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}))), + Matchers.is(true)); + Assert.assertThat(getBinding("paramAndTest", String.class, String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "test", new String[]{"test"}))), + Matchers.is(true)); + Assert.assertThat(getBinding("paramAndUnderscoreTest", String.class, String.class).incomingServerRequestMatchesMethod( + mockSearchRequest(ImmutableMap.of("param", new String[]{"value"}, "_test", new String[]{"test"}))), + Matchers.is(true)); + } + + private SearchMethodBinding getBinding(String name, Class... parameters) throws NoSuchMethodException { + return new SearchMethodBinding(IBaseResource.class, + IBaseResource.class, + TestResourceProvider.class.getMethod(name, parameters), + fhirContext, + TEST_RESOURCE_PROVIDER); + } + + private RequestDetails mockSearchRequest(Map params) { + RequestDetails requestDetails = mock(RequestDetails.class); + when(requestDetails.getOperation()).thenReturn("_search"); + when(requestDetails.getRequestType()).thenReturn(RequestTypeEnum.GET); + when(requestDetails.getParameters()).thenReturn(params); + return requestDetails; + } + + private static class TestResourceProvider { + + @Search + public IBaseResource param(@RequiredParam(name = "param") String param) { + return null; + } + + @Search + public IBaseResource paramAndTest(@RequiredParam(name = "param") String param, @OptionalParam(name = "test") String test) { + return null; + } + + @Search + public IBaseResource paramAndUnderscoreTest(@RequiredParam(name = "param") String param, @OptionalParam(name = "_test") String test) { + return null; + } + + } + +} diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/ServerMethodSelectionR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/ServerMethodSelectionR4Test.java new file mode 100644 index 00000000000..14fa05826ef --- /dev/null +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/ServerMethodSelectionR4Test.java @@ -0,0 +1,190 @@ +package ca.uhn.fhir.rest.server; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.context.api.BundleInclusionRule; +import ca.uhn.fhir.model.api.Include; +import ca.uhn.fhir.rest.annotation.IncludeParam; +import ca.uhn.fhir.rest.annotation.OptionalParam; +import ca.uhn.fhir.rest.annotation.Search; +import ca.uhn.fhir.rest.api.EncodingEnum; +import ca.uhn.fhir.rest.client.api.IGenericClient; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.test.utilities.JettyUtil; +import com.google.common.collect.Lists; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.servlet.ServletHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.Bundle; +import org.hl7.fhir.r4.model.Patient; +import org.hl7.fhir.r4.model.StringType; +import org.junit.After; +import org.junit.Test; + +import java.util.List; +import java.util.Set; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.*; + +public class ServerMethodSelectionR4Test { + + + private FhirContext myCtx = FhirContext.forR4(); + private Server myServer; + private IGenericClient myClient; + + @After + public void after() throws Exception { + JettyUtil.closeServer(myServer); + } + + /** + * Server method with no _include + * Client request with _include + *

+ * See #1421 + */ + @Test + public void testRejectIncludeIfNotProvided() throws Exception { + + class MyProvider extends MyBaseProvider { + @Search + public List search(@OptionalParam(name = "name") StringType theName) { + return Lists.newArrayList(new Patient().setActive(true).setId("Patient/123")); + } + } + MyProvider provider = new MyProvider(); + + startServer(provider); + + try { + myClient + .search() + .forResource(Patient.class) + .where(Patient.NAME.matches().value("foo")) + .include(Patient.INCLUDE_ORGANIZATION) + .execute(); + fail(); + } catch (InvalidRequestException e) { + assertThat(e.getMessage(), containsString("this server does not know how to handle GET operation[Patient] with parameters [[_include, name]]")); + } + } + + /** + * Server method with no _include + * Client request with _include + *

+ * See #1421 + */ + @Test + public void testAllowIncludeIfProvided() throws Exception { + + class MyProvider extends MyBaseProvider { + @Search + public List search(@OptionalParam(name = "name") StringType theName, @IncludeParam Set theIncludes) { + return Lists.newArrayList(new Patient().setActive(true).setId("Patient/123")); + } + } + MyProvider provider = new MyProvider(); + + startServer(provider); + + Bundle results = myClient + .search() + .forResource(Patient.class) + .where(Patient.NAME.matches().value("foo")) + .include(Patient.INCLUDE_ORGANIZATION) + .returnBundle(Bundle.class) + .execute(); + assertEquals(1, results.getEntry().size()); + } + + /** + * Server method with no _revinclude + * Client request with _revinclude + *

+ * See #1421 + */ + @Test + public void testRejectRevIncludeIfNotProvided() throws Exception { + + class MyProvider extends MyBaseProvider { + @Search + public List search(@OptionalParam(name = "name") StringType theName) { + return Lists.newArrayList(new Patient().setActive(true).setId("Patient/123")); + } + } + MyProvider provider = new MyProvider(); + + startServer(provider); + + try { + myClient + .search() + .forResource(Patient.class) + .where(Patient.NAME.matches().value("foo")) + .revInclude(Patient.INCLUDE_ORGANIZATION) + .execute(); + fail(); + } catch (InvalidRequestException e) { + assertThat(e.getMessage(), containsString("this server does not know how to handle GET operation[Patient] with parameters [[_revinclude, name]]")); + } + } + + /** + * Server method with no _revInclude + * Client request with _revInclude + *

+ * See #1421 + */ + @Test + public void testAllowRevIncludeIfProvided() throws Exception { + + class MyProvider extends MyBaseProvider { + @Search + public List search(@OptionalParam(name = "name") StringType theName, @IncludeParam(reverse = true) Set theRevIncludes) { + return Lists.newArrayList(new Patient().setActive(true).setId("Patient/123")); + } + } + MyProvider provider = new MyProvider(); + + startServer(provider); + + Bundle results = myClient + .search() + .forResource(Patient.class) + .where(Patient.NAME.matches().value("foo")) + .revInclude(Patient.INCLUDE_ORGANIZATION) + .returnBundle(Bundle.class) + .execute(); + assertEquals(1, results.getEntry().size()); + } + + private void startServer(Object theProvider) throws Exception { + RestfulServer servlet = new RestfulServer(myCtx); + servlet.registerProvider(theProvider); + ServletHandler proxyHandler = new ServletHandler(); + servlet.setDefaultResponseEncoding(EncodingEnum.XML); + servlet.setBundleInclusionRule(BundleInclusionRule.BASED_ON_RESOURCE_PRESENCE); + ServletHolder servletHolder = new ServletHolder(servlet); + proxyHandler.addServletWithMapping(servletHolder, "/*"); + + myServer = new Server(0); + myServer.setHandler(proxyHandler); + JettyUtil.startServer(myServer); + int port = JettyUtil.getPortForStartedServer(myServer); + + myClient = myCtx.newRestfulGenericClient("http://localhost:" + port); + } + + + public static class MyBaseProvider implements IResourceProvider { + + @Override + public Class getResourceType() { + return Patient.class; + } + } + +} diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 293b3260a9a..0e1bb4b885a 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -212,6 +212,11 @@ with the new request id, resulting in an ever growing source.meta value. E.g. after the first update, it looks like "#9f0a901387128111#5f37835ee38a89e2" when it should only be "#5f37835ee38a89e2". This has been corrected. + + The Plain Server method selector was incorrectly allowing client requests with _include statements to be + handled by method implementations that did not have any @IncludeParam]]> defined. This + is now corrected. Thanks to Tuomo Ala-Vannesluoma for reporting and providing a test case! +