From 9a58e2e56e1d3b9b614c58b14f988dfb8dbe8cc9 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 27 Aug 2014 11:34:13 -0400 Subject: [PATCH] Close #14 - HAPI will now not ignore _id parameter when choosing a backing method --- .../java/ca/uhn/fhir/context/FhirContext.java | 99 +++++++++---------- .../fhir/rest/method/SearchMethodBinding.java | 28 +++++- .../ca/uhn/fhir/i18n/hapi-messages.properties | 2 + .../java/ca/uhn/fhir/i18n/HapiLocalizer.java | 45 +++++++++ .../fhir/rest/server/MethodPriorityTest.java | 3 +- .../server/ServerInvalidDefinitionTest.java | 35 +++++++ 6 files changed, 159 insertions(+), 53 deletions(-) create mode 100644 hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties create mode 100644 hapi-fhir-base/src/test/java/ca/uhn/fhir/i18n/HapiLocalizer.java diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/FhirContext.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/FhirContext.java index 21bfa2e0798..c2ee302495f 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/FhirContext.java +++ b/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 - * used as a factory for various other types of objects (parsers, clients, etc.). + * 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.). * *

* Important usage notes: *

*

*/ public class FhirContext { + private static final List> EMPTY_LIST = Collections.emptyList(); private volatile Map, BaseRuntimeElementDefinition> myClassToElementDefinition = Collections.emptyMap(); private volatile Map myIdToResourceDefinition = Collections.emptyMap(); + private HapiLocalizer myLocalizer = new HapiLocalizer(); private volatile Map myNameToElementDefinition = Collections.emptyMap(); + private Map myNameToResourceType; private volatile INarrativeGenerator myNarrativeGenerator; private volatile IRestfulClientFactory myRestfulClientFactory; private volatile RuntimeChildUndeclaredExtensionDefinition myRuntimeChildUndeclaredExtensionDefinition; - private Map myNameToResourceType; - private static final List> 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 - * for extending the core library. + * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed for extending the core library. */ public BaseRuntimeElementDefinition getElementDefinition(Class 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 - * for extending the core library. + * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed for extending the core library. */ public RuntimeResourceDefinition getResourceDefinition(Class 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 - * for extending the core library. + * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed 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 - * for extending the core library. + * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed 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 - * something so that we can detect names like diagnosticreport + * 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 */ 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 - * for extending the core library. + * Returns the scanned runtime model for the given type. This is an advanced feature which is generally only needed 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 - * core library. + * Returns the scanned runtime models. This is an advanced feature which is generally only needed for extending the core library. */ public Collection getResourceDefinitions() { return myIdToResourceDefinition.values(); @@ -196,8 +201,7 @@ public class FhirContext { * Create and return a new JSON parser. * *

- * Performance Note: This method is cheap to call, and may be called once for every message being processed - * without incurring any performance penalty + * Performance Note: This method is cheap to call, and may be called once for every message being processed without incurring any performance penalty *

*/ 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 - * cases to contain methods for each of the RESTful operations you wish to 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 RESTful Client documentation for more - * information on how to define this interface. + * 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 + * 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 RESTful Client documentation for more information on how to define this interface. * *

- * Performance Note: This method is cheap to call, and may be called once for every operation invocation - * without incurring any performance penalty + * Performance Note: This method is cheap to call, and may be called once for every operation invocation without incurring any performance penalty *

* * @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 - * a compliant server, but does not have methods defining the specific functionality required (as is the case with - * {@link #newRestfulClient(Class, String) non-generic clients}). + * 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 + * functionality required (as is the case with {@link #newRestfulClient(Class, String) non-generic clients}). * *

- * Performance Note: This method is cheap to call, and may be called once for every operation invocation - * without incurring any performance penalty + * Performance Note: This method is cheap to call, and may be called once for every operation invocation without incurring any performance penalty *

* * @param theServerBase @@ -263,18 +261,13 @@ public class FhirContext { * Create and return a new XML parser. * *

- * Performance Note: This method is cheap to call, and may be called once for every message being processed - * without incurring any performance penalty + * Performance Note: This method is cheap to call, and may be called once for every message being processed without incurring any performance penalty *

*/ public IParser newXmlParser() { return new XmlParser(this); } - public void setNarrativeGenerator(INarrativeGenerator theNarrativeGenerator) { - myNarrativeGenerator = theNarrativeGenerator; - } - private RuntimeResourceDefinition scanResourceType(Class theResourceType) { ArrayList> resourceTypes = new ArrayList>(); resourceTypes.add(theResourceType); @@ -305,13 +298,19 @@ public class FhirContext { myIdToResourceDefinition = idToElementDefinition; myNameToResourceType = scanner.getNameToResourceType(); - + return classToElementDefinition; } - /** 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 void setLocalizer(HapiLocalizer theMessages) { + myLocalizer = theMessages; + } + + public void setNarrativeGenerator(INarrativeGenerator theNarrativeGenerator) { + myNarrativeGenerator = theNarrativeGenerator; } private static Collection> toCollection(Class theResourceType) { diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/SearchMethodBinding.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/SearchMethodBinding.java index 1cedba458c6..f85f3d72709 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/SearchMethodBinding.java +++ b/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 SPECIAL_PARAM_NAMES; + + static { + HashSet specialParamNames = new HashSet(); + specialParamNames.add("_id"); + specialParamNames.add("_language"); + SPECIAL_PARAM_NAMES = Collections.unmodifiableSet(specialParamNames); + } private Class 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 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; 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 new file mode 100644 index 00000000000..65fbfb543fb --- /dev/null +++ b/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. \ No newline at end of file diff --git a/hapi-fhir-base/src/test/java/ca/uhn/fhir/i18n/HapiLocalizer.java b/hapi-fhir-base/src/test/java/ca/uhn/fhir/i18n/HapiLocalizer.java new file mode 100644 index 00000000000..311cfbd7a4f --- /dev/null +++ b/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 myKeyToMessageFormat = new ConcurrentHashMap(); + + 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; + } + } + + +} diff --git a/hapi-fhir-base/src/test/java/ca/uhn/fhir/rest/server/MethodPriorityTest.java b/hapi-fhir-base/src/test/java/ca/uhn/fhir/rest/server/MethodPriorityTest.java index f46c4dfaef1..fba54b4dd1e 100644 --- a/hapi-fhir-base/src/test/java/ca/uhn/fhir/rest/server/MethodPriorityTest.java +++ b/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(); diff --git a/hapi-fhir-base/src/test/java/ca/uhn/fhir/rest/server/ServerInvalidDefinitionTest.java b/hapi-fhir-base/src/test/java/ca/uhn/fhir/rest/server/ServerInvalidDefinitionTest.java index cdc7e9a3803..6de73851f92 100644 --- a/hapi-fhir-base/src/test/java/ca/uhn/fhir/rest/server/ServerInvalidDefinitionTest.java +++ b/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 getResourceType() { + return Patient.class; + } + + @SuppressWarnings("unused") + @Search + public List search(@RequiredParam(name="_pretty") StringParam theParam) { + return null; + } + + } + + public static class InstantiableTypeForResourceProvider implements IResourceProvider {