Work on #164 - Improve error messages when an invalid or missing content

type header is detected for creat/update requests

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/client/GenericClient.java

@@ -581,7 +581,7 @@ public class GenericClient extends BaseClient implements IGenericClient {
 		}
 
 		protected IResource parseResourceBody(String theResourceBody) {
-			EncodingEnum encoding = determineRawEncoding(theResourceBody);
+			EncodingEnum encoding = MethodUtil.detectEncodingNoDefault(theResourceBody);
 			if (encoding == null) {
 				throw new InvalidRequestException("FHIR client can't determine resource encoding");
 			}
@@ -598,24 +598,6 @@ public class GenericClient extends BaseClient implements IGenericClient {
 
 	}
 	
-	/**
-	 * Returns null if encoding can't be determined
-	 */
-	private static EncodingEnum determineRawEncoding(String theResourceBody) {
-		EncodingEnum encoding = null;
-		for (int i = 0; i < theResourceBody.length() && encoding == null; i++) {
-			switch (theResourceBody.charAt(i)) {
-			case '<':
-				encoding = EncodingEnum.XML;
-				break;
-			case '{':
-				encoding = EncodingEnum.JSON;
-				break;
-			}
-		}
-		return encoding;
-	}
-	
 	private final class BundleResponseHandler implements IClientResponseHandler<Bundle> {
 
 		private Class<? extends IBaseResource> myType;
@@ -1579,7 +1561,7 @@ public class GenericClient extends BaseClient implements IGenericClient {
 
 		public TransactionExecutable(String theBundle) {
 			myRawBundle = theBundle;
-			myRawBundleEncoding = determineRawEncoding(myRawBundle);
+			myRawBundleEncoding = MethodUtil.detectEncodingNoDefault(myRawBundle);
 			if (myRawBundleEncoding == null) {
 				throw new IllegalArgumentException("Can not determine encoding of raw resource body");
 			}
@@ -1603,7 +1585,7 @@ public class GenericClient extends BaseClient implements IGenericClient {
 				 * If the user has explicitly requested a given encoding, we may need to reencode the raw string
 				 */
 				if (getParamEncoding() != null) {
-					if (determineRawEncoding(myRawBundle) != getParamEncoding()) {
+					if (MethodUtil.detectEncodingNoDefault(myRawBundle) != getParamEncoding()) {
 						IResource parsed = parseResourceBody(myRawBundle);
 						myRawBundle = getParamEncoding().newParser(getFhirContext()).encodeResourceToString(parsed);
 					}

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

@@ -167,6 +167,25 @@ public abstract class BaseMethodBinding<T> implements IClientResponseHandler<T>
 	 */
 	public abstract String getResourceName();
 
+	/**
+	 * Returns the value of {@link #getResourceOperationType()} or {@link #getSystemOperationType()} or {@link #getOtherOperationType()}
+	 */
+	public String getResourceOrSystemOperationType() {
+		Enum<?> retVal = getResourceOperationType();
+		if (retVal != null) {
+			return retVal.name();
+		}
+		retVal = getSystemOperationType();
+		if (retVal != null) {
+			return retVal.name();
+		}
+		retVal = getOtherOperationType();
+		if (retVal != null) {
+			return retVal.name();
+		}
+		return null;
+	}
+
 	public abstract RestfulOperationTypeEnum getResourceOperationType();
 
 	public abstract RestfulOperationSystemEnum getSystemOperationType();

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

@@ -20,9 +20,12 @@ package ca.uhn.fhir.rest.method;
  * #L%
  */
 
+import static org.apache.commons.lang3.StringUtils.isBlank;
+
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.Reader;
+import java.io.StringReader;
 import java.io.Writer;
 import java.lang.reflect.Method;
 import java.util.Enumeration;
@@ -32,6 +35,7 @@ import java.util.Set;
 
 import javax.servlet.http.HttpServletResponse;
 
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.hl7.fhir.instance.model.IBaseResource;
 
@@ -52,6 +56,7 @@ import ca.uhn.fhir.rest.server.RestfulServer;
 import ca.uhn.fhir.rest.server.RestfulServerUtils;
 import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException;
 import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
+import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor;
 
 abstract class BaseOutcomeReturningMethodBinding extends BaseMethodBinding<MethodOutcome> {
@@ -64,7 +69,8 @@ abstract class BaseOutcomeReturningMethodBinding extends BaseMethodBinding<Metho
 
 		if (!theMethod.getReturnType().equals(MethodOutcome.class)) {
 			if (!allowVoidReturnType()) {
-				throw new ConfigurationException("Method " + theMethod.getName() + " in type " + theMethod.getDeclaringClass().getCanonicalName() + " is a @" + theMethodAnnotation.getSimpleName() + " method but it does not return " + MethodOutcome.class);
+				throw new ConfigurationException("Method " + theMethod.getName() + " in type " + theMethod.getDeclaringClass().getCanonicalName() + " is a @" + theMethodAnnotation.getSimpleName()
+						+ " method but it does not return " + MethodOutcome.class);
 			} else if (theMethod.getReturnType() == void.class) {
 				myReturnVoid = true;
 			}
@@ -100,8 +106,7 @@ abstract class BaseOutcomeReturningMethodBinding extends BaseMethodBinding<Metho
 	protected abstract BaseHttpClientInvocation createClientInvocation(Object[] theArgs, IResource resource);
 
 	/**
-	 * For servers, this method will match only incoming requests that match the given operation, or which have no
+	 * For servers, this method will match only incoming requests that match the given operation, or which have no operation in the URL if this method returns null.
-	 * operation in the URL if this method returns null.
 	 */
 	protected abstract String getMatchingOperation();
 
@@ -125,7 +130,8 @@ abstract class BaseOutcomeReturningMethodBinding extends BaseMethodBinding<Metho
 	}
 
 	@Override
-	public MethodOutcome invokeClient(String theResponseMimeType, Reader theResponseReader, int theResponseStatusCode, Map<String, List<String>> theHeaders) throws IOException, BaseServerResponseException {
+	public MethodOutcome invokeClient(String theResponseMimeType, Reader theResponseReader, int theResponseStatusCode, Map<String, List<String>> theHeaders) throws IOException,
+			BaseServerResponseException {
 		switch (theResponseStatusCode) {
 		case Constants.STATUS_HTTP_200_OK:
 		case Constants.STATUS_HTTP_201_CREATED:
@@ -195,7 +201,8 @@ abstract class BaseOutcomeReturningMethodBinding extends BaseMethodBinding<Metho
 		switch (getResourceOperationType()) {
 		case CREATE:
 			if (response == null) {
-				throw new InternalErrorException("Method " + getMethod().getName() + " in type " + getMethod().getDeclaringClass().getCanonicalName() + " returned null, which is not allowed for create operation");
+				throw new InternalErrorException("Method " + getMethod().getName() + " in type " + getMethod().getDeclaringClass().getCanonicalName()
+						+ " returned null, which is not allowed for create operation");
 			}
 			if (response.getCreated() == null || Boolean.TRUE.equals(response.getCreated())) {
 				servletResponse.setStatus(Constants.STATUS_HTTP_201_CREATED);
@@ -269,9 +276,39 @@ abstract class BaseOutcomeReturningMethodBinding extends BaseMethodBinding<Metho
 	 * @throws IOException
 	 */
 	protected IResource parseIncomingServerResource(Request theRequest) throws IOException {
-		EncodingEnum encoding = RestfulServerUtils.determineRequestEncoding(theRequest);
+
+		Reader requestReader;
+		EncodingEnum encoding = RestfulServerUtils.determineRequestEncodingNoDefault(theRequest);
+		if (encoding == null) {
+			String ctValue = theRequest.getServletRequest().getHeader(Constants.HEADER_CONTENT_TYPE);
+			if (ctValue != null) {
+				if (ctValue.startsWith("application/x-www-form-urlencoded")) {
+					String msg = getContext().getLocalizer().getMessage(BaseOutcomeReturningMethodBinding.class, "invalidContentTypeInRequest", ctValue, getResourceOrSystemOperationType());
+					throw new InvalidRequestException(msg);
+				}
+			}
+			if (isBlank(ctValue)) {
+				/*
+				 * If the client didn't send a content type, try to guess
+				 */
+				requestReader = theRequest.getServletRequest().getReader();
+				String body = IOUtils.toString(requestReader);
+				encoding = MethodUtil.detectEncodingNoDefault(body);
+				if (encoding == null) {
+					String msg = getContext().getLocalizer().getMessage(BaseOutcomeReturningMethodBinding.class, "noContentTypeInRequest", getResourceOrSystemOperationType());
+					throw new InvalidRequestException(msg);
+				} else {
+					requestReader = new StringReader(body);
+				}
+			} else {
+				String msg = getContext().getLocalizer().getMessage(BaseOutcomeReturningMethodBinding.class, "invalidContentTypeInRequest", ctValue, getResourceOrSystemOperationType());
+				throw new InvalidRequestException(msg);
+			}
+		} else {
+			requestReader = theRequest.getServletRequest().getReader();
+		}
+
 		IParser parser = encoding.newParser(getContext());
-		BufferedReader requestReader = theRequest.getServletRequest().getReader();
 
 		Class<? extends IBaseResource> wantedResourceType = requestContainsResourceType();
 		IResource retVal;
@@ -296,14 +333,14 @@ abstract class BaseOutcomeReturningMethodBinding extends BaseMethodBinding<Metho
 	}
 
 	/**
-	 * Subclasses may override to provide a specific resource type that this method wants
+	 * Subclasses may override to provide a specific resource type that this method wants as a parameter
-	 * as a parameter
 	 */
 	protected Class<? extends IBaseResource> requestContainsResourceType() {
 		return null;
 	}
 
-	protected void streamOperationOutcome(BaseServerResponseException theE, RestfulServer theServer, EncodingEnum theEncodingNotNull, HttpServletResponse theResponse, Request theRequest) throws IOException {
+	protected void streamOperationOutcome(BaseServerResponseException theE, RestfulServer theServer, EncodingEnum theEncodingNotNull, HttpServletResponse theResponse, Request theRequest)
+			throws IOException {
 		theResponse.setStatus(theE.getStatusCode());
 
 		theServer.addHeadersToResponse(theResponse);

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

@@ -248,15 +248,26 @@ public class MethodUtil {
 	}
 
 	public static EncodingEnum detectEncoding(String theBody) {
-		for (int i = 0; i < theBody.length(); i++) {
+		EncodingEnum retVal = detectEncodingNoDefault(theBody);
+		if (retVal == null) {
+		retVal = EncodingEnum.XML;
+		}
+		return retVal;
+	}
+
+	public static EncodingEnum detectEncodingNoDefault(String theBody) {
+		EncodingEnum retVal = null;
+		for (int i = 0; i < theBody.length() && retVal == null; i++) {
 			switch (theBody.charAt(i)) {
 				case '<':
-					return EncodingEnum.XML;
+					retVal = EncodingEnum.XML;
+					break;
 				case '{':
-					return EncodingEnum.JSON;
+					retVal = EncodingEnum.JSON;
+					break;
 			}
 		}
-		return EncodingEnum.XML;
+		return retVal;
 	}
 
 	public static void extractDescription(SearchParameter theParameter, Annotation[] theAnnotations) {

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

@@ -195,9 +195,18 @@ public class RestfulServerUtils {
 	}
 
 	public static EncodingEnum determineRequestEncoding(Request theReq) {
+		EncodingEnum retVal = determineRequestEncodingNoDefault(theReq);
+		if (retVal != null) {
+			return retVal;
+		}
+		return EncodingEnum.XML;
+	}
+
+	public static EncodingEnum determineRequestEncodingNoDefault(Request theReq) {
+		EncodingEnum retVal = null;
 		Enumeration<String> acceptValues = theReq.getServletRequest().getHeaders(Constants.HEADER_CONTENT_TYPE);
 		if (acceptValues != null) {
-			while (acceptValues.hasMoreElements()) {
+			while (acceptValues.hasMoreElements() && retVal == null) {
 				String nextAcceptHeaderValue = acceptValues.nextElement();
 				if (nextAcceptHeaderValue != null && isNotBlank(nextAcceptHeaderValue)) {
 					for (String nextPart : nextAcceptHeaderValue.split(",")) {
@@ -209,16 +218,16 @@ public class RestfulServerUtils {
 							nextPart = nextPart.substring(0, scIdx);
 						}
 						nextPart = nextPart.trim();
-						EncodingEnum retVal = Constants.FORMAT_VAL_TO_ENCODING.get(nextPart);
+						retVal = Constants.FORMAT_VAL_TO_ENCODING.get(nextPart);
 						if (retVal != null) {
+							break;
+						}
+					}
+				}
+			}
+		}
 		return retVal;
 	}
-					}
-				}
-			}
-		}
-		return EncodingEnum.XML;
-	}
 
 	public static String createPagingLink(Set<Include> theIncludes, String theServerBase, String theSearchId, int theOffset, int theCount, EncodingEnum theResponseEncoding, boolean thePrettyPrint) {
 		try {

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

@@ -12,6 +12,9 @@ ca.uhn.fhir.rest.client.GenericClient.cannotDetermineResourceTypeFromUri=Unable
 ca.uhn.fhir.rest.client.RestfulClientFactory.failedToRetrieveConformance=Failed to retrieve the server's metadata statement during client initialization. URL used was: {0}
 ca.uhn.fhir.rest.client.RestfulClientFactory.wrongVersionInConformance=The server at base URL "{0}" returned a conformance statement indicating that it supports FHIR version "{1}" which corresponds to {2}, but this client is configured to use {3} (via the FhirContext). 
 
+ca.uhn.fhir.rest.method.BaseOutcomeReturningMethodBinding.noContentTypeInRequest=No Content-Type header was provided in the request. This is required for "{0}" operation
+ca.uhn.fhir.rest.method.BaseOutcomeReturningMethodBinding.invalidContentTypeInRequest=Incorrect Content-Type header value of "{0}" was provided in the request. A FHIR Content-Type is required for "{1}" operation
+
 ca.uhn.fhir.rest.method.OperationMethodBinding.methodNotSupported=HTTP Method {0} is not allowed for this operation. Allowed method(s): {1}
 ca.uhn.fhir.rest.method.OperationParamBinder.urlParamNotPrimitive=Can not invoke operation {0} using HTTP GET because parameter {1} is not a primitive datatype
 ca.uhn.fhir.rest.method.IncludeParameter.invalidIncludeNameInRequest=Invalid {2} parameter value: "{0}". Valid values are: {1}

hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/SearchTest.java

@@ -9,12 +9,14 @@ import java.util.List;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.commons.io.IOUtils;
+import org.apache.http.HttpEntity;
 import org.apache.http.HttpResponse;
 import org.apache.http.NameValuePair;
 import org.apache.http.client.entity.UrlEncodedFormEntity;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
+import org.apache.http.entity.ByteArrayEntity;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
@@ -38,10 +40,13 @@ import ca.uhn.fhir.model.dstu.resource.Observation;
 import ca.uhn.fhir.model.dstu.resource.Patient;
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.narrative.DefaultThymeleafNarrativeGenerator;
+import ca.uhn.fhir.rest.annotation.Create;
 import ca.uhn.fhir.rest.annotation.IdParam;
 import ca.uhn.fhir.rest.annotation.OptionalParam;
 import ca.uhn.fhir.rest.annotation.RequiredParam;
+import ca.uhn.fhir.rest.annotation.ResourceParam;
 import ca.uhn.fhir.rest.annotation.Search;
+import ca.uhn.fhir.rest.api.MethodOutcome;
 import ca.uhn.fhir.rest.client.IGenericClient;
 import ca.uhn.fhir.rest.param.ReferenceParam;
 import ca.uhn.fhir.rest.param.StringOrListParam;
@@ -233,6 +238,75 @@ public class SearchTest {
 		assertEquals("IDAAA (identifier123)", bundle.getEntries().get(0).getTitle().getValue());
 	}
 
+	/**
+	 * See #164
+	 */
+	@Test
+	public void testSearchByPostWithParamsInBodyAndUrl() throws Exception {
+		HttpPost filePost = new HttpPost("http://localhost:" + ourPort + "/Patient/_search?name=Central");
+
+		// add parameters to the post method
+		List<NameValuePair> parameters = new ArrayList<NameValuePair>();
+		parameters.add(new BasicNameValuePair("_id", "aaa"));
+
+		UrlEncodedFormEntity sendentity = new UrlEncodedFormEntity(parameters, "UTF-8");
+		filePost.setEntity(sendentity);
+
+		HttpResponse status = ourClient.execute(filePost);
+		String responseContent = IOUtils.toString(status.getEntity().getContent());
+		IOUtils.closeQuietly(status.getEntity().getContent());
+		ourLog.info(responseContent);
+		assertEquals(200, status.getStatusLine().getStatusCode());
+		
+		Bundle bundle = ourCtx.newXmlParser().parseBundle(responseContent);
+		assertEquals(1, bundle.getEntries().size());
+
+		Patient p = bundle.getResources(Patient.class).get(0);
+		assertEquals("idaaa", p.getName().get(0).getFamilyAsSingleString());
+		assertEquals("nameCentral", p.getName().get(1).getFamilyAsSingleString());
+
+	}
+
+	/**
+	 * See #164
+	 */
+	@Test
+	public void testSearchByPostWithInvalidPostUrl() throws Exception {
+		HttpPost filePost = new HttpPost("http://localhost:" + ourPort + "/Patient?name=Central"); // should end with _search
+
+		// add parameters to the post method
+		List<NameValuePair> parameters = new ArrayList<NameValuePair>();
+		parameters.add(new BasicNameValuePair("_id", "aaa"));
+
+		UrlEncodedFormEntity sendentity = new UrlEncodedFormEntity(parameters, "UTF-8");
+		filePost.setEntity(sendentity);
+
+		HttpResponse status = ourClient.execute(filePost);
+		String responseContent = IOUtils.toString(status.getEntity().getContent());
+		IOUtils.closeQuietly(status.getEntity().getContent());
+		ourLog.info(responseContent);
+		assertEquals(400, status.getStatusLine().getStatusCode());
+		assertThat(responseContent, containsString("<details value=\"Incorrect Content-Type header value of &quot;application/x-www-form-urlencoded; charset=UTF-8&quot; was provided in the request. This is required for &quot;CREATE&quot; operation\"/>"));
+	}
+
+	/**
+	 * See #164
+	 */
+	@Test
+	public void testSearchByPostWithMissingContentType() throws Exception {
+		HttpPost filePost = new HttpPost("http://localhost:" + ourPort + "/Patient?name=Central"); // should end with _search
+
+		HttpEntity sendentity = new ByteArrayEntity(new byte[] {1,2,3,4} );
+		filePost.setEntity(sendentity);
+
+		HttpResponse status = ourClient.execute(filePost);
+		String responseContent = IOUtils.toString(status.getEntity().getContent());
+		IOUtils.closeQuietly(status.getEntity().getContent());
+		ourLog.info(responseContent);
+		assertEquals(400, status.getStatusLine().getStatusCode());
+		assertThat(responseContent, containsString("<details value=\"No Content-Type header was provided in the request. This is required for &quot;CREATE&quot; operation\"/>"));
+	}
+
 	@Test
 	public void testSearchCompartment() throws Exception {
 		HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/123/fooCompartment");
@@ -354,6 +428,14 @@ public class SearchTest {
 	 */
 	public static class DummyPatientResourceProvider implements IResourceProvider {
 
+		/**
+		 * Only needed for #164
+		 */
+		@Create
+		public MethodOutcome create(@ResourceParam Patient thePatient) {
+			throw new IllegalArgumentException();
+		}
+		
 		@Search(compartmentName = "fooCompartment")
 		public List<Patient> compartment(@IdParam IdDt theId) {
 			ArrayList<Patient> retVal = new ArrayList<Patient>();
@@ -383,7 +465,7 @@ public class SearchTest {
 
 
 		@Search
-		public List<Patient> findPatient(@RequiredParam(name = "_id") StringParam theParam) {
+		public List<Patient> findPatient(@RequiredParam(name = "_id") StringParam theParam, @OptionalParam(name="name") StringParam theName) {
 			ArrayList<Patient> retVal = new ArrayList<Patient>();
 
 			Patient patient = new Patient();
@@ -391,6 +473,7 @@ public class SearchTest {
 			patient.addIdentifier("system", "identifier123");
 			if (theParam != null) {
 				patient.addName().addFamily("id" + theParam.getValue());
+				patient.addName().addFamily("name" + theName.getValue());
 			}
 			retVal.add(patient);
 			return retVal;

src/changes/changes.xml

@@ -183,6 +183,10 @@
 				default bean name expected in other parts of the Spring framework. 
 				Thanks to Mohammad Jafari for the suggestion!
 			</action>
+			<action type="add" issue="164">
+				Improve error message when a user tries to perform a create/update with an invalid
+				or missing Content-Type header.
+			</action>
 		</release>
 		<release version="0.9" date="2015-Mar-14">
 			<action type="add">