Close #14 - HAPI will now not ignore _id parameter when choosing a

backing method

hapi-fhir-base/src/main/java/ca/uhn/fhir/context/FhirContext.java

@@ -30,9 +30,9 @@ import java.util.Map;
 import org.apache.commons.lang3.Validate;
 import org.apache.commons.lang3.text.WordUtils;
 
+import ca.uhn.fhir.i18n.HapiLocalizer;
 import ca.uhn.fhir.model.api.IElement;
 import ca.uhn.fhir.model.api.IResource;
-import ca.uhn.fhir.model.dstu.resource.Patient;
 import ca.uhn.fhir.model.view.ViewGenerator;
 import ca.uhn.fhir.narrative.INarrativeGenerator;
 import ca.uhn.fhir.parser.DataFormatException;
@@ -48,33 +48,30 @@ import ca.uhn.fhir.util.FhirTerser;
 import ca.uhn.fhir.validation.FhirValidator;
 
 /**
- * The FHIR context is the central starting point for the use of the HAPI FHIR API. It should be created once, and then
+ * The FHIR context is the central starting point for the use of the HAPI FHIR API. It should be created once, and then used as a factory for various other types of objects (parsers, clients, etc.).
- * used as a factory for various other types of objects (parsers, clients, etc.).
  * 
  * <p>
  * Important usage notes:
  * <ul>
  * <li>Thread safety: <b>This class is thread safe</b> and may be shared between multiple processing threads.</li>
  * <li>
- * Performance: <b>This class is expensive</b> to create, as it scans every resource class it needs to parse or encode
+ * Performance: <b>This class is expensive</b> to create, as it scans every resource class it needs to parse or encode to build up an internal model of those classes. For that reason, you should try
- * to build up an internal model of those classes. For that reason, you should try to create one FhirContext instance
+ * to create one FhirContext instance which remains for the life of your application and reuse that instance. Note that it will not cause problems to create multiple instances (ie. resources
- * which remains for the life of your application and reuse that instance. Note that it will not cause problems to
+ * originating from one FhirContext may be passed to parsers originating from another) but you will incur a performance penalty if a new FhirContext is created for every message you parse/encode.</li>
- * create multiple instances (ie. resources originating from one FhirContext may be passed to parsers originating from
- * another) but you will incur a performance penalty if a new FhirContext is created for every message you parse/encode.
- * </li>
  * </ul>
  * </p>
  */
 public class FhirContext {
 
+	private static final List<Class<? extends IResource>> EMPTY_LIST = Collections.emptyList();
 	private volatile Map<Class<? extends IElement>, BaseRuntimeElementDefinition<?>> myClassToElementDefinition = Collections.emptyMap();
 	private volatile Map<String, RuntimeResourceDefinition> myIdToResourceDefinition = Collections.emptyMap();
+	private HapiLocalizer myLocalizer = new HapiLocalizer();
 	private volatile Map<String, RuntimeResourceDefinition> myNameToElementDefinition = Collections.emptyMap();
+	private Map<String, String> myNameToResourceType;
 	private volatile INarrativeGenerator myNarrativeGenerator;
 	private volatile IRestfulClientFactory myRestfulClientFactory;
 	private volatile RuntimeChildUndeclaredExtensionDefinition myRuntimeChildUndeclaredExtensionDefinition;
-	private Map<String, String> myNameToResourceType;
-	private static final List<Class<? extends IResource>> EMPTY_LIST = Collections.emptyList();
 
 	/**
 	 * Default constructor. In most cases this is the right constructor to use.
@@ -96,20 +93,33 @@ public class FhirContext {
 	}
 
 	/**
-	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed
+	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed for extending the core library.
-	 * for extending the core library.
 	 */
 	public BaseRuntimeElementDefinition<?> getElementDefinition(Class<? extends IElement> theElementType) {
 		return myClassToElementDefinition.get(theElementType);
 	}
 
+	/** For unit tests only */
+	int getElementDefinitionCount() {
+		return myClassToElementDefinition.size();
+	}
+
+	/**
+	 * This feature is not yet in its final state and should be considered an internal part of HAPI for now - use with caution
+	 */
+	public HapiLocalizer getLocalizer() {
+		if (myLocalizer == null) {
+			myLocalizer = new HapiLocalizer();
+		}
+		return myLocalizer;
+	}
+
 	public INarrativeGenerator getNarrativeGenerator() {
 		return myNarrativeGenerator;
 	}
 
 	/**
-	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed
+	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed for extending the core library.
-	 * for extending the core library.
 	 */
 	public RuntimeResourceDefinition getResourceDefinition(Class<? extends IResource> theResourceType) {
 		RuntimeResourceDefinition retVal = (RuntimeResourceDefinition) myClassToElementDefinition.get(theResourceType);
@@ -120,24 +130,21 @@ public class FhirContext {
 	}
 
 	/**
-	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed
+	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed for extending the core library.
-	 * for extending the core library.
 	 */
 	public RuntimeResourceDefinition getResourceDefinition(IResource theResource) {
 		return getResourceDefinition(theResource.getClass());
 	}
 
 	/**
-	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed
+	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed for extending the core library.
-	 * for extending the core library.
 	 */
 	@SuppressWarnings("unchecked")
 	public RuntimeResourceDefinition getResourceDefinition(String theResourceName) {
 		String resourceName = theResourceName;
 
 		/*
-		 * TODO: this is a bit of a hack, really we should have a translation table based on a property file or
+		 * TODO: this is a bit of a hack, really we should have a translation table based on a property file or something so that we can detect names like diagnosticreport
-		 * something so that we can detect names like diagnosticreport
 		 */
 		if (Character.isLowerCase(resourceName.charAt(0))) {
 			resourceName = WordUtils.capitalize(resourceName);
@@ -166,16 +173,14 @@ public class FhirContext {
 	}
 
 	/**
-	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed
+	 * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed for extending the core library.
-	 * for extending the core library.
 	 */
 	public RuntimeResourceDefinition getResourceDefinitionById(String theId) {
 		return myIdToResourceDefinition.get(theId);
 	}
 
 	/**
-	 * Returns the scanned runtime models. This is an advanced feature which is generally only needed for extending the
+	 * Returns the scanned runtime models. This is an advanced feature which is generally only needed for extending the core library.
-	 * core library.
 	 */
 	public Collection<RuntimeResourceDefinition> getResourceDefinitions() {
 		return myIdToResourceDefinition.values();
@@ -196,8 +201,7 @@ public class FhirContext {
 	 * Create and return a new JSON parser.
 	 * 
 	 * <p>
-	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every message being processed
+	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every message being processed without incurring any performance penalty
-	 * without incurring any performance penalty
 	 * </p>
 	 */
 	public IParser newJsonParser() {
@@ -205,16 +209,12 @@ public class FhirContext {
 	}
 
 	/**
-	 * Instantiates a new client instance. This method requires an interface which is defined specifically for your use
+	 * Instantiates a new client instance. This method requires an interface which is defined specifically for your use cases to contain methods for each of the RESTful operations you wish to
-	 * cases to contain methods for each of the RESTful operations you wish to implement (e.g. "read ImagingStudy",
+	 * implement (e.g. "read ImagingStudy", "search Patient by identifier", etc.). This interface must extend {@link IRestfulClient} (or commonly its sub-interface {@link IBasicClient}). See the <a
-	 * "search Patient by identifier", etc.). This interface must extend {@link IRestfulClient} (or commonly its
+	 * href="http://hl7api.sourceforge.net/hapi-fhir/doc_rest_client.html">RESTful Client</a> documentation for more information on how to define this interface.
-	 * sub-interface {@link IBasicClient}). See the <a
-	 * href="http://hl7api.sourceforge.net/hapi-fhir/doc_rest_client.html">RESTful Client</a> documentation for more
-	 * information on how to define this interface.
 	 * 
 	 * <p>
-	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every operation invocation
+	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every operation invocation without incurring any performance penalty
-	 * without incurring any performance penalty
 	 * </p>
 	 * 
 	 * @param theClientType
@@ -230,13 +230,11 @@ public class FhirContext {
 	}
 
 	/**
-	 * Instantiates a new generic client. A generic client is able to perform any of the FHIR RESTful operations against
+	 * Instantiates a new generic client. A generic client is able to perform any of the FHIR RESTful operations against a compliant server, but does not have methods defining the specific
-	 * a compliant server, but does not have methods defining the specific functionality required (as is the case with
+	 * functionality required (as is the case with {@link #newRestfulClient(Class, String) non-generic clients}).
-	 * {@link #newRestfulClient(Class, String) non-generic clients}).
 	 * 
 	 * <p>
-	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every operation invocation
+	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every operation invocation without incurring any performance penalty
-	 * without incurring any performance penalty
 	 * </p>
 	 * 
 	 * @param theServerBase
@@ -263,18 +261,13 @@ public class FhirContext {
 	 * Create and return a new XML parser.
 	 * 
 	 * <p>
-	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every message being processed
+	 * Performance Note: <b>This method is cheap</b> to call, and may be called once for every message being processed without incurring any performance penalty
-	 * without incurring any performance penalty
 	 * </p>
 	 */
 	public IParser newXmlParser() {
 		return new XmlParser(this);
 	}
 
-	public void setNarrativeGenerator(INarrativeGenerator theNarrativeGenerator) {
-		myNarrativeGenerator = theNarrativeGenerator;
-	}
-
 	private RuntimeResourceDefinition scanResourceType(Class<? extends IResource> theResourceType) {
 		ArrayList<Class<? extends IResource>> resourceTypes = new ArrayList<Class<? extends IResource>>();
 		resourceTypes.add(theResourceType);
@@ -305,13 +298,19 @@ public class FhirContext {
 		myIdToResourceDefinition = idToElementDefinition;
 
 		myNameToResourceType = scanner.getNameToResourceType();
-		
+
 		return classToElementDefinition;
 	}
 
-	/** For unit tests only */
+	/**
-	int getElementDefinitionCount() {
+	 * This feature is not yet in its final state and should be considered an internal part of HAPI for now - use with caution
-		return myClassToElementDefinition.size();
+	 */
+	public void setLocalizer(HapiLocalizer theMessages) {
+		myLocalizer = theMessages;
+	}
+
+	public void setNarrativeGenerator(INarrativeGenerator theNarrativeGenerator) {
+		myNarrativeGenerator = theNarrativeGenerator;
 	}
 
 	private static Collection<Class<? extends IResource>> toCollection(Class<? extends IResource> theResourceType) {

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

@@ -32,6 +32,7 @@ import java.util.Set;
 
 import org.apache.commons.lang3.StringUtils;
 
+import ca.uhn.fhir.context.ConfigurationException;
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.model.api.IResource;
 import ca.uhn.fhir.model.api.annotation.Description;
@@ -48,10 +49,17 @@ import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
  */
 public class SearchMethodBinding extends BaseResourceReturningMethodBinding {
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SearchMethodBinding.class);
+	private static final Set<String> SPECIAL_PARAM_NAMES;
+
+	static {
+		HashSet<String> specialParamNames = new HashSet<String>();
+		specialParamNames.add("_id");
+		specialParamNames.add("_language");
+		SPECIAL_PARAM_NAMES = Collections.unmodifiableSet(specialParamNames);
+	}
 
 	private Class<? extends IResource> myDeclaredResourceType;
 	private String myQueryName;
-
 	private String myDescription;
 
 	@SuppressWarnings("unchecked")
@@ -69,6 +77,20 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding {
 			}
 		}
 
+		for (IParameter next : getParameters()) {
+			if (!(next instanceof SearchParameter)) {
+				continue;
+			}
+			
+			SearchParameter sp = (SearchParameter)next;
+			if (sp.getName().startsWith("_")) {
+				if (ALLOWED_PARAMS.contains(sp.getName())) {
+					String msg = getContext().getLocalizer().getMessage(getClass().getName() + ".invalidSpecialParamName", theMethod.getName(), theMethod.getDeclaringClass().getSimpleName(), sp.getName());
+					throw new ConfigurationException(msg); 
+				}
+			}
+		}
+		
 	}
 
 	public String getDescription() {
@@ -216,7 +238,9 @@ public class SearchMethodBinding extends BaseResourceReturningMethodBinding {
 		Set<String> keySet = theRequest.getParameters().keySet();
 		for (String next : keySet) {
 			if (next.startsWith("_")) {
-				continue;
+				if (!SPECIAL_PARAM_NAMES.contains(next)) {
+					continue;
+				}
 			}
 			if (!methodParamsTemp.contains(next)) {
 				return false;

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

@@ -0,0 +1,2 @@
+
+ca.uhn.fhir.rest.method.SearchMethodBinding.invalidSpecialParamName=Method [{0}] in provider [{1}] contains search parameter annotated to use name [{2}] - This name is reserved according to the FHIR specification and can not be used as a search parameter name. 

hapi-fhir-base/src/test/java/ca/uhn/fhir/i18n/HapiLocalizer.java

@@ -0,0 +1,45 @@
+package ca.uhn.fhir.i18n;
+
+import java.text.MessageFormat;
+import java.util.Map;
+import java.util.ResourceBundle;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * This feature is not yet in its final state and should be considered an internal part of HAPI for now - use with caution
+ */
+public class HapiLocalizer {
+
+	private ResourceBundle myBundle;
+	private final Map<String, MessageFormat> myKeyToMessageFormat = new ConcurrentHashMap<String, MessageFormat>();
+
+	public HapiLocalizer() {
+		myBundle = ResourceBundle.getBundle(HapiLocalizer.class.getPackage().getName() + ".hapi-messages");
+	}
+
+	public String getMessage(String theKey, Object... theParameters) {
+		if (theParameters != null && theParameters.length > 0) {
+			MessageFormat format = myKeyToMessageFormat.get(theKey);
+			if (format != null) {
+				return format.format(theParameters).toString();
+			}
+			
+			String formatString = myBundle.getString(theKey);
+			if (formatString== null) {
+				formatString = "!MESSAGE!";
+			}
+			
+			format = new MessageFormat(formatString);
+			myKeyToMessageFormat.put(theKey, format);
+			return format.format(theParameters).toString();
+		} else {
+			String retVal = myBundle.getString(theKey);
+			if (retVal == null) {
+				retVal = "!MESSAGE!";
+			}
+			return retVal;
+		}
+	}
+	
+	
+}

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

@@ -42,7 +42,8 @@ public class MethodPriorityTest {
 	private static Server ourServer;
 	private static RestfulServer ourServlet;
 
-	public void testOmitEmptyOptionalParam() throws Exception {
+	@Test
+	public void testDelegateTo_idMethod() throws Exception {
 		ourServlet.setResourceProviders(new DummyObservationResourceProvider());
 		ourServer.start();
 

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

@@ -3,6 +3,8 @@ package ca.uhn.fhir.rest.server;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 
+import java.util.List;
+
 import javax.servlet.ServletException;
 
 import org.hamcrest.core.StringContains;
@@ -15,6 +17,7 @@ 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.annotation.Search;
 import ca.uhn.fhir.rest.param.StringParam;
 
 public class ServerInvalidDefinitionTest {
@@ -46,6 +49,20 @@ public class ServerInvalidDefinitionTest {
 		}
 	}
 
+	@Test
+	public void testInvalidSpecialNameResourceProvider() {
+		RestfulServer srv = new RestfulServer();
+		srv.setResourceProviders(new InvalidSpecialParameterNameResourceProvider());
+		
+		try {
+			srv.init();
+			fail();
+		} catch (ServletException e) {
+			assertThat(e.getCause().toString(), StringContains.containsString("ConfigurationException"));
+			assertThat(e.getCause().toString(), StringContains.containsString("_pretty"));
+		}
+	}
+	
 	@Test
 	public void testReadMethodWithSearchParameters() {
 		RestfulServer srv = new RestfulServer();
@@ -117,6 +134,24 @@ public class ServerInvalidDefinitionTest {
 		
 	}
 	
+	
+	public static class InvalidSpecialParameterNameResourceProvider implements IResourceProvider
+	{
+
+		@Override
+		public Class<? extends IResource> getResourceType() {
+			return Patient.class;
+		}
+
+		@SuppressWarnings("unused")
+		@Search
+		public List<Patient> search(@RequiredParam(name="_pretty") StringParam theParam) {
+			return null;
+		}
+		
+	}
+	
+	
 	public static class InstantiableTypeForResourceProvider implements IResourceProvider
 	{