Detect invalid read method

hapi-fhir-base/src/changes/changes.xml

@@ -103,6 +103,10 @@
 			<action type="fix">
 				Fix performance issue in date/time datatypes where pattern matchers were not static
 			</action>
+			<action type="fix">
+				Server now gives a more helpful error message if a @Read method has a search parameter (which is invalid, but
+				previously lead to a very unhelpful error message). Thanks to Tahura Chaudhry of UHN for reporting!
+			</action>
 		</release>
 		<release version="0.5" date="2014-Jul-30">
 			<action type="add">

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/BaseMethodBinding.java

@@ -95,6 +95,11 @@ public abstract class BaseMethodBinding<T> implements IClientResponseHandler<T>
 		myParameters = MethodUtil.getResourceParameters(theMethod);
 	}
 
+	
+	public List<Class<?>> getAllowableParamAnnotations() {
+		return null;
+	}
+	
 	public Set<String> getIncludes() {
 		Set<String> retVal = new TreeSet<String>();
 		for (IParameter next : myParameters) {

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/ReadMethodBinding.java

@@ -23,6 +23,9 @@ package ca.uhn.fhir.rest.method;
 import java.io.IOException;
 import java.io.InputStream;
 import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -39,6 +42,7 @@ import ca.uhn.fhir.model.dstu.resource.Binary;
 import ca.uhn.fhir.model.dstu.valueset.RestfulOperationSystemEnum;
 import ca.uhn.fhir.model.dstu.valueset.RestfulOperationTypeEnum;
 import ca.uhn.fhir.model.primitive.IdDt;
+import ca.uhn.fhir.rest.annotation.IdParam;
 import ca.uhn.fhir.rest.annotation.Read;
 import ca.uhn.fhir.rest.method.SearchMethodBinding.RequestType;
 import ca.uhn.fhir.rest.server.Constants;
@@ -52,9 +56,9 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding implem
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ReadMethodBinding.class);
 
 	private Integer myIdIndex;
-	private Integer myVersionIdIndex;
 	private boolean mySupportsVersion;
-
+	private Integer myVersionIdIndex;
+	
 	public ReadMethodBinding(Class<? extends IResource> theAnnotatedResourceType, Method theMethod, FhirContext theContext, Object theProvider) {
 		super(theAnnotatedResourceType, theMethod, theContext, theProvider);
 
@@ -63,7 +67,7 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding implem
 		Integer idIndex = MethodUtil.findIdParameterIndex(theMethod);
 		Integer versionIdIndex = MethodUtil.findVersionIdParameterIndex(theMethod);
 
-		mySupportsVersion = theMethod.getAnnotation(Read.class).version();		
+		mySupportsVersion = theMethod.getAnnotation(Read.class).version();
 		myIdIndex = idIndex;
 		myVersionIdIndex = versionIdIndex;
 
@@ -77,8 +81,26 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding implem
 
 	}
 
-	public boolean isVread() {
+	@Override
-		return mySupportsVersion || myVersionIdIndex != null;
+	public List<Class<?>> getAllowableParamAnnotations() {
+		ArrayList<Class<?>> retVal = new ArrayList<Class<?>>();
+		retVal.add(IdParam.class);
+		return retVal;
+	}
+
+	@Override
+	public RestfulOperationTypeEnum getResourceOperationType() {
+		return isVread() ? RestfulOperationTypeEnum.VREAD : RestfulOperationTypeEnum.READ;
+	}
+
+	@Override
+	public ReturnTypeEnum getReturnType() {
+		return ReturnTypeEnum.RESOURCE;
+	}
+
+	@Override
+	public RestfulOperationSystemEnum getSystemOperationType() {
+		return null;
 	}
 
 	@Override
@@ -107,7 +129,7 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding implem
 			if (mySupportsVersion == false && myVersionIdIndex == null) {
 				return false;
 			}
-			if (theRequest.getId().hasVersionIdPart()==false) {
+			if (theRequest.getId().hasVersionIdPart() == false) {
 				return false;
 			}
 		} else if (!StringUtils.isBlank(theRequest.getOperation())) {
@@ -116,23 +138,6 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding implem
 		return true;
 	}
 
-	@Override
-	public ReturnTypeEnum getReturnType() {
-		return ReturnTypeEnum.RESOURCE;
-	}
-
-	@Override
-	public IBundleProvider invokeServer(RequestDetails theRequest, Object[] theMethodParams) throws InvalidRequestException, InternalErrorException {
-		theMethodParams[myIdIndex] = theRequest.getId();
-		if (myVersionIdIndex != null) {
-			theMethodParams[myVersionIdIndex] = new IdDt(theRequest.getId().getVersionIdPart());
-		}
-
-		Object response = invokeServerMethod(theMethodParams);
-
-		return toResourceList(response);
-	}
-
 	@Override
 	public HttpGetClientInvocation invokeClient(Object[] theArgs) {
 		HttpGetClientInvocation retVal;
@@ -154,35 +159,9 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding implem
 		return retVal;
 	}
 
-	public static HttpGetClientInvocation createVReadInvocation(IdDt theId, IdDt vid, String resourceName) {
-		return new HttpGetClientInvocation(resourceName, theId.getIdPart(), Constants.URL_TOKEN_HISTORY, vid.getIdPart());
-	}
-
-	public static HttpGetClientInvocation createReadInvocation(IdDt theId, String resourceName) {
-		if (theId.hasVersionIdPart()) {
-			return new HttpGetClientInvocation(resourceName, theId.getIdPart(), Constants.URL_TOKEN_HISTORY, theId.getVersionIdPart());
-		} else {
-			return new HttpGetClientInvocation(resourceName, theId.getIdPart());
-		}
-	}
-
 	@Override
-	public RestfulOperationTypeEnum getResourceOperationType() {
+	public Object invokeClient(String theResponseMimeType, InputStream theResponseReader, int theResponseStatusCode, Map<String, List<String>> theHeaders) throws IOException,
-		return isVread() ? RestfulOperationTypeEnum.VREAD : RestfulOperationTypeEnum.READ;
+			BaseServerResponseException {
-	}
-
-	@Override
-	public RestfulOperationSystemEnum getSystemOperationType() {
-		return null;
-	}
-
-	@Override
-	public boolean isBinary() {
-		return "Binary".equals(getResourceName());
-	}
-
-	@Override
-	public Object invokeClient(String theResponseMimeType, InputStream theResponseReader, int theResponseStatusCode, Map<String, List<String>> theHeaders) throws IOException, BaseServerResponseException {
 		byte[] contents = IOUtils.toByteArray(theResponseReader);
 		Binary resource = new Binary(theResponseMimeType, contents);
 
@@ -200,4 +179,37 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding implem
 		throw new IllegalStateException("" + getMethodReturnType()); // should not happen
 	}
 
+	@Override
+	public IBundleProvider invokeServer(RequestDetails theRequest, Object[] theMethodParams) throws InvalidRequestException, InternalErrorException {
+		theMethodParams[myIdIndex] = theRequest.getId();
+		if (myVersionIdIndex != null) {
+			theMethodParams[myVersionIdIndex] = new IdDt(theRequest.getId().getVersionIdPart());
+		}
+
+		Object response = invokeServerMethod(theMethodParams);
+
+		return toResourceList(response);
+	}
+
+	@Override
+	public boolean isBinary() {
+		return "Binary".equals(getResourceName());
+	}
+
+	public boolean isVread() {
+		return mySupportsVersion || myVersionIdIndex != null;
+	}
+
+	public static HttpGetClientInvocation createReadInvocation(IdDt theId, String resourceName) {
+		if (theId.hasVersionIdPart()) {
+			return new HttpGetClientInvocation(resourceName, theId.getIdPart(), Constants.URL_TOKEN_HISTORY, theId.getVersionIdPart());
+		} else {
+			return new HttpGetClientInvocation(resourceName, theId.getIdPart());
+		}
+	}
+
+	public static HttpGetClientInvocation createVReadInvocation(IdDt theId, IdDt vid, String resourceName) {
+		return new HttpGetClientInvocation(resourceName, theId.getIdPart(), Constants.URL_TOKEN_HISTORY, vid.getIdPart());
+	}
+
 }

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java

@@ -26,6 +26,7 @@ import java.io.IOException;
 import java.io.OutputStreamWriter;
 import java.io.UnsupportedEncodingException;
 import java.io.Writer;
+import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.net.URLEncoder;
@@ -52,6 +53,7 @@ import javax.servlet.http.HttpServletResponse;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Validate;
 import org.apache.commons.lang3.exception.ExceptionUtils;
+import org.eclipse.jetty.security.MappedLoginService.Anonymous;
 
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.context.RuntimeResourceDefinition;
@@ -68,6 +70,7 @@ import ca.uhn.fhir.model.dstu.valueset.IssueSeverityEnum;
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.model.primitive.InstantDt;
 import ca.uhn.fhir.parser.IParser;
+import ca.uhn.fhir.rest.annotation.IdParam;
 import ca.uhn.fhir.rest.method.BaseMethodBinding;
 import ca.uhn.fhir.rest.method.ConformanceMethodBinding;
 import ca.uhn.fhir.rest.method.Request;
@@ -524,6 +527,21 @@ public class RestfulServer extends HttpServlet {
 						}
 					}
 
+					List<Class<?>> allowableParams = foundMethodBinding.getAllowableParamAnnotations();
+					if (allowableParams != null) {
+						for (Annotation[] nextParamAnnotations : m.getParameterAnnotations()) {
+							for (Annotation annotation : nextParamAnnotations) {
+								Package pack = annotation.annotationType().getPackage();
+								if (pack.equals(IdParam.class.getPackage())) {
+									if (!allowableParams.contains(annotation)) {
+										throw new ConfigurationException("Method[" + m.toString() + "] is not allowed to have a parameter annotated with "+ annotation);
+									}
+								}
+							}
+						}
+					}
+					
+					
 					resourceBinding.addMethod(foundMethodBinding);
 					ourLog.debug(" * Method: {}#{} is a handler", theProvider.getClass(), m.getName());
 				}

hapi-fhir-base/src/site/resources/svg/hapi-fhir-logging-complete.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="614px" height="153px" version="1.1"><defs/><g transform="translate(0.5,0.5)"><path d="M 17 1 L 121 1 L 121 61 L 17 61 L 17 49 L 1 49 L 1 37 L 17 37 L 17 25 L 1 25 L 1 13 L 17 13 Z" fill="#ffffff" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 17 13 L 33 13 L 33 25 L 17 25 M 17 37 L 33 37 L 33 49 L 17 49" fill="none" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><g fill="#000000" font-family="Helvetica" font-size="12px"><text x="39" y="35">hapi-fhir</text></g><rect x="321" y="1" width="110" height="60" rx="9" ry="9" fill="#ffffff" stroke="#000000" pointer-events="none"/><g transform="translate(323,24)"><switch><foreignObject pointer-events="all" width="106" height="15" requiredFeatures="http://www.w3.org/TR/SVG11/feature#Extensibility"><div xmlns="http://www.w3.org/1999/xhtml" style="display: inline-block; font-size: 12px; font-family: Helvetica; color: rgb(0, 0, 0); line-height: 1.26; vertical-align: top; width: 106px; white-space: normal; text-align: center;">slf4j-api</div></foreignObject><text x="53" y="14" fill="#000000" text-anchor="middle" font-size="12px" font-family="Helvetica">[Not supported by viewer]</text></switch></g><path d="M 121 31 L 315 31" fill="none" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 320 31 L 313 35 L 315 31 L 313 28 Z" fill="#000000" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 67 91 L 171 91 L 171 151 L 67 151 L 67 139 L 51 139 L 51 127 L 67 127 L 67 115 L 51 115 L 51 103 L 67 103 Z" fill="#ffffff" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 67 103 L 83 103 L 83 115 L 67 115 M 67 127 L 83 127 L 83 139 L 67 139" fill="none" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><g fill="#000000" font-family="Helvetica" font-size="12px"><text x="89" y="118">commons-</text><text x="89" y="132">httpclient</text></g><rect x="221" y="91" width="110" height="60" rx="9" ry="9" fill="#ffffff" stroke="#000000" pointer-events="none"/><g transform="translate(223,114)"><switch><foreignObject pointer-events="all" width="106" height="15" requiredFeatures="http://www.w3.org/TR/SVG11/feature#Extensibility"><div xmlns="http://www.w3.org/1999/xhtml" style="display: inline-block; font-size: 12px; font-family: Helvetica; color: rgb(0, 0, 0); line-height: 1.26; vertical-align: top; width: 106px; white-space: normal; text-align: center;">jcl-over-slf4j</div></foreignObject><text x="53" y="14" fill="#000000" text-anchor="middle" font-size="12px" font-family="Helvetica">[Not supported by viewer]</text></switch></g><path d="M 91 61 L 107 86" fill="none" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 110 90 L 104 86 L 107 86 L 109 82 Z" fill="#000000" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 171 121 L 215 121" fill="none" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 220 121 L 213 125 L 215 121 L 213 118 Z" fill="#000000" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 309 91 L 338 65" fill="none" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 342 62 L 339 69 L 338 65 L 334 64 Z" fill="#000000" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><path d="M 441 36 L 441 26 L 461 26 L 461 16 L 491 31 L 461 46 L 461 36 Z" fill="#ffffff" stroke="#000000" stroke-miterlimit="10" pointer-events="none"/><rect x="501" y="11" width="80" height="40" fill="none" stroke="none" pointer-events="none"/><g fill="#000000" font-family="Helvetica" font-size="12px"><text x="503" y="14">Underlying </text><text x="503" y="28">Logging</text><text x="503" y="42">Framework</text><text x="503" y="56">(logback, log4j, etc.)</text></g></g></svg>

hapi-fhir-base/src/site/xdoc/doc_logging.xml

@@ -23,7 +23,9 @@
 				Unfortunately HAPI is not immune to this issue.
 			</p>
 
-			<subsection name="SLF4j">
+			<subsection name="Configuring HAPI's Logging - SLF4j">
+				
+				<img src="svg/hapi-fhir-logging.svg"  width="723" height="273" alt="Logging arch diagram" align="right"/>
 			
 				<p>
 					HAPI uses 
@@ -62,11 +64,13 @@
 						</ul> 
 					</li>
 				</ul>
-				
+								
 			</subsection>
 
 			<subsection name="Commons-Logging">
 				
+				<img src="svg/hapi-fhir-logging-complete.svg" width="614" height="153" alt="Logging arch diagram" align="right"/>
+			
 				<p>
 					Note that HAPI's client uses Apache HttpComponents Client internally, and that 
 					library uses Apache Commons Logging as a logging facade. The recommended approach to
@@ -75,6 +79,21 @@
 					but will redirect its logging statements to the same target as SLF4j has been
 					configured to.   
 				</p>
+				
+				<p>
+					The diagram at the right shows the chain of command for logging under this scheme.
+				</p>
+				
+				<p>
+					Note that some popular libraries (e.g. Spring Framework) also use commons-logging
+					for logging. As such they may include a commons-logging JAR automatically as 
+					a transitive dependency in Maven. If you are using jcl-over-slf4j and it isn't
+					working correctly, it is often worth checking the list of JARs included in your
+					application to see whether commons-logging has also been added. It can then be specifically
+					excluded in Maven. 
+				</p>
+			
+				<br clear="all"/>
 			
 			</subsection>
 

hapi-fhir-base/src/test/java/ca/uhn/fhir/context/DummyPatientWithExtensions.java

@@ -20,6 +20,7 @@ import ca.uhn.fhir.util.ElementUtil;
 @ResourceDef(name="Patient")
 public class DummyPatientWithExtensions extends Patient {
 
+	
 	/**
 	 * Each extension is defined in a field. Any valid HAPI Data Type
 	 * can be used for the field type. Note that the [name=""] attribute

hapi-fhir-base/src/test/java/ca/uhn/fhir/rest/server/ServerInvalidDefinitionTest.java

@@ -1,6 +1,7 @@
 package ca.uhn.fhir.rest.server;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
 
 import javax.servlet.ServletException;
 
@@ -13,6 +14,8 @@ import ca.uhn.fhir.model.dstu.resource.Patient;
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.rest.annotation.IdParam;
 import ca.uhn.fhir.rest.annotation.Read;
+import ca.uhn.fhir.rest.annotation.RequiredParam;
+import ca.uhn.fhir.rest.param.StringParam;
 
 public class ServerInvalidDefinitionTest {
 
@@ -42,7 +45,20 @@ public class ServerInvalidDefinitionTest {
 			assertThat(e.getCause().toString(), StringContains.containsString("public"));
 		}
 	}
-	
+
+	@Test
+	public void testReadMethodWithSearchParameters() {
+		RestfulServer srv = new RestfulServer();
+		srv.setResourceProviders(new ReadMethodWithSearchParamProvider());
+		
+		try {
+			srv.init();
+			fail();
+		} catch (ServletException e) {
+			assertThat(e.getCause().toString(), StringContains.containsString("ConfigurationException"));
+		}
+	}
+
 	/**
 	 * Normal, should initialize properly
 	 */
@@ -69,6 +85,22 @@ public class ServerInvalidDefinitionTest {
 		
 	}
 	
+	public static class ReadMethodWithSearchParamProvider implements IResourceProvider
+	{
+
+		@Override
+		public Class<? extends IResource> getResourceType() {
+			return Patient.class;
+		}
+		
+		@SuppressWarnings("unused")
+		@Read
+		public Patient read(@IdParam IdDt theId, @RequiredParam(name="aaa") StringParam theParam) {
+			return null;
+		}
+		
+	}
+	
 	public static class NonInstantiableTypeForResourceProvider implements IResourceProvider
 	{
 

hapi-fhir-jpaserver-uhnfhirtest/.settings/org.eclipse.wst.common.component

@@ -12,7 +12,7 @@
         <dependent-module archiveName="hapi-fhir-base-0.6-SNAPSHOT.jar" deploy-path="/WEB-INF/lib" handle="module:/resource/hapi-fhir-base/hapi-fhir-base">
             <dependency-type>uses</dependency-type>
         </dependent-module>
-        <dependent-module deploy-path="/" handle="module:/overlay/var/M2_REPO/ca/uhn/hapi/fhir/hapi-fhir-testpage-overlay/0.6-SNAPSHOT/hapi-fhir-testpage-overlay-0.6-SNAPSHOT.war?unpackFolder=target/m2e-wtp/overlays&amp;includes=**/**&amp;excludes=META-INF/MANIFEST.MF">
+        <dependent-module deploy-path="/" handle="module:/overlay/prj/hapi-fhir-testpage-overlay?includes=**/**&amp;excludes=META-INF/MANIFEST.MF">
             <dependency-type>consumes</dependency-type>
         </dependent-module>
         <dependent-module deploy-path="/" handle="module:/overlay/slf/?includes=**/**&amp;excludes=META-INF/MANIFEST.MF">

restful-server-example/.settings/org.eclipse.wst.common.component

@@ -6,7 +6,7 @@
         <dependent-module archiveName="hapi-fhir-base-0.6-SNAPSHOT.jar" deploy-path="/WEB-INF/lib" handle="module:/resource/hapi-fhir-base/hapi-fhir-base">
             <dependency-type>uses</dependency-type>
         </dependent-module>
-        <dependent-module deploy-path="/" handle="module:/overlay/var/M2_REPO/ca/uhn/hapi/fhir/hapi-fhir-testpage-overlay/0.6-SNAPSHOT/hapi-fhir-testpage-overlay-0.6-SNAPSHOT.war?unpackFolder=target/m2e-wtp/overlays&amp;includes=**/**&amp;excludes=META-INF/MANIFEST.MF">
+        <dependent-module deploy-path="/" handle="module:/overlay/prj/hapi-fhir-testpage-overlay?includes=**/**&amp;excludes=META-INF/MANIFEST.MF">
             <dependency-type>consumes</dependency-type>
         </dependent-module>
         <dependent-module deploy-path="/" handle="module:/overlay/slf/?includes=**/**&amp;excludes=META-INF/MANIFEST.MF">