Merge branch 'tuomoa-searchmethodbinding_test'

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<String> SPECIAL_SEARCH_PARAMS;
-	private String myCompartmentName;
-	private String myDescription;
-	private Integer myIdParamIndex;
-	private String myQueryName;
-	private boolean myAllowUnknownParams;
-  private final String myResourceProviderResourceName;
 
 	static {
 		HashSet<String> 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<? extends IBaseResource> theReturnResourceType, Class<? extends IBaseResource> theResourceProviderResourceType, Method theMethod, FhirContext theContext, Object theProvider) {
 		super(theReturnResourceType, theMethod, theContext, theProvider);
 		Search search = theMethod.getAnnotation(Search.class);
@@ -155,27 +157,26 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding {
 
 		Set<String> unqualifiedNames = theRequest.getUnqualifiedToQualifiedNames().keySet();
 		Set<String> qualifiedParamNames = theRequest.getParameters().keySet();
-		for (int i = 0; i < this.getParameters().size(); i++) {
+		for (IParameter nextParameter : getParameters()) {
-			if (!(getParameters().get(i) instanceof BaseQueryParameter)) {
+			if (!(nextParameter instanceof BaseQueryParameter)) {
 				continue;
 			}
-			BaseQueryParameter temp = (BaseQueryParameter) getParameters().get(i);
+			BaseQueryParameter nextQueryParameter = (BaseQueryParameter) nextParameter;
-			String name = temp.getName();
+			String name = nextQueryParameter.getName();
-			if (temp.isRequired()) {
+			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<String> 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,13 +184,13 @@ 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<String> 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)) {
@@ -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) {

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<String, String[]> 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;
+        }
+
+    }
+
+}

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
+	 * <p>
+	 * See #1421
+	 */
+	@Test
+	public void testRejectIncludeIfNotProvided() throws Exception {
+
+		class MyProvider extends MyBaseProvider {
+			@Search
+			public List<IBaseResource> 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
+	 * <p>
+	 * See #1421
+	 */
+	@Test
+	public void testAllowIncludeIfProvided() throws Exception {
+
+		class MyProvider extends MyBaseProvider {
+			@Search
+			public List<IBaseResource> search(@OptionalParam(name = "name") StringType theName, @IncludeParam Set<Include> 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
+	 * <p>
+	 * See #1421
+	 */
+	@Test
+	public void testRejectRevIncludeIfNotProvided() throws Exception {
+
+		class MyProvider extends MyBaseProvider {
+			@Search
+			public List<IBaseResource> 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
+	 * <p>
+	 * See #1421
+	 */
+	@Test
+	public void testAllowRevIncludeIfProvided() throws Exception {
+
+		class MyProvider extends MyBaseProvider {
+			@Search
+			public List<IBaseResource> search(@OptionalParam(name = "name") StringType theName, @IncludeParam(reverse = true) Set<Include> 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<? extends IBaseResource> getResourceType() {
+			return Patient.class;
+		}
+	}
+
+}

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.
 			</action>
+			<action type="fix" issue="1421">
+				The Plain Server method selector was incorrectly allowing client requests with _include statements to be
+				handled by method implementations that did not have any <![CDATA[<code>@IncludeParam</code>]]> defined. This
+				is now corrected. Thanks to Tuomo Ala-Vannesluoma for reporting and providing a test case!
+			</action>
 		</release>
 		<release version="4.0.3" date="2019-09-03" description="Igloo (Point Release)">
 			<action type="fix">