From 8434f96e97630b6a3ef369360b3465ea97e3d2b2 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 11 Mar 2015 17:18:42 -0400 Subject: [PATCH] Fix #111 - Don't return stack traces in server responses y default --- .../main/java/example/ServletExamples.java | 25 +++ .../uhn/fhir/rest/server/RestfulServer.java | 60 +----- .../ExceptionHandlingInterceptor.java | 119 ++++++++++++ .../uhn/fhir/rest/server/ExceptionTest.java | 116 +++++++----- .../ExceptionHandlingInterceptorTest.java | 175 +++++++++--------- .../ExceptionInterceptorMethodTest.java | 165 +++++++++++++++++ src/changes/changes.xml | 6 + src/site/xdoc/doc_rest_server_interceptor.xml | 31 +++- 8 files changed, 511 insertions(+), 186 deletions(-) create mode 100644 hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptor.java create mode 100644 hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionInterceptorMethodTest.java diff --git a/examples/src/main/java/example/ServletExamples.java b/examples/src/main/java/example/ServletExamples.java index 2c30ff66a6a..e96c2d69b6d 100644 --- a/examples/src/main/java/example/ServletExamples.java +++ b/examples/src/main/java/example/ServletExamples.java @@ -4,6 +4,8 @@ import javax.servlet.ServletException; import javax.servlet.annotation.WebServlet; import ca.uhn.fhir.rest.server.RestfulServer; +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; +import ca.uhn.fhir.rest.server.interceptor.ExceptionHandlingInterceptor; import ca.uhn.fhir.rest.server.interceptor.LoggingInterceptor; @SuppressWarnings("serial") @@ -34,4 +36,27 @@ public class ServletExamples { } // END SNIPPET: loggingInterceptor + + + // START SNIPPET: exceptionInterceptor + @WebServlet(urlPatterns = { "/fhir/*" }, displayName = "FHIR Server") + public class RestfulServerWithExceptionHandling extends RestfulServer { + + @Override + protected void initialize() throws ServletException { + + // ... define your resource providers here ... + + // Now register the logging interceptor + ExceptionHandlingInterceptor interceptor = new ExceptionHandlingInterceptor(); + registerInterceptor(interceptor); + + // Return the stack trace to the client for the following exception types + interceptor.setReturnStackTracesForExceptionTypes(InternalErrorException.class, NullPointerException.class); + + } + + } + // END SNIPPET: exceptionInterceptor + } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java index f4a8155da40..420a206c2a3 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java @@ -62,6 +62,7 @@ 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.exceptions.NotModifiedException; +import ca.uhn.fhir.rest.server.interceptor.ExceptionHandlingInterceptor; import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor; import ca.uhn.fhir.util.ReflectionUtil; import ca.uhn.fhir.util.UrlUtil; @@ -715,61 +716,8 @@ public class RestfulServer extends HttpServlet { } } - BaseOperationOutcome oo = null; - int statusCode = Constants.STATUS_HTTP_500_INTERNAL_ERROR; - - if (e instanceof BaseServerResponseException) { - oo = ((BaseServerResponseException) e).getOperationOutcome(); - statusCode = ((BaseServerResponseException) e).getStatusCode(); - } - - /* - * Generate an OperationOutcome to return, unless the exception throw by the resource provider had one - */ - if (oo == null) { - try { - oo = (BaseOperationOutcome) myFhirContext.getResourceDefinition("OperationOutcome").getImplementingClass().newInstance(); - } catch (Exception e1) { - ourLog.error("Failed to instantiate OperationOutcome resource instance", e1); - throw new ServletException("Failed to instantiate OperationOutcome resource instance", e1); - } - - BaseIssue issue = oo.addIssue(); - issue.getSeverityElement().setValue("error"); - if (e instanceof InternalErrorException) { - ourLog.error("Failure during REST processing", e); - issue.getDetailsElement().setValue(e.toString() + "\n\n" + ExceptionUtils.getStackTrace(e)); - } else if (e instanceof BaseServerResponseException) { - ourLog.warn("Failure during REST processing: {}", e); - BaseServerResponseException baseServerResponseException = (BaseServerResponseException) e; - statusCode = baseServerResponseException.getStatusCode(); - issue.getDetailsElement().setValue(e.getMessage()); - if (baseServerResponseException.getAdditionalMessages() != null) { - for (String next : baseServerResponseException.getAdditionalMessages()) { - BaseIssue issue2 = oo.addIssue(); - issue2.getSeverityElement().setValue("error"); - issue2.setDetails(next); - } - } - } else { - ourLog.error("Failure during REST processing: " + e.toString(), e); - issue.getDetailsElement().setValue(e.toString() + "\n\n" + ExceptionUtils.getStackTrace(e)); - statusCode = Constants.STATUS_HTTP_500_INTERNAL_ERROR; - } - } else { - ourLog.error("Unknown error during processing", e); - } - - RestfulServerUtils.streamResponseAsResource(this, theResponse, oo, RestfulServerUtils.determineResponseEncodingNoDefault(theRequest), true, requestIsBrowser, NarrativeModeEnum.NORMAL, - statusCode, false, fhirServerBase); - - theResponse.setStatus(statusCode); - addHeadersToResponse(theResponse); - theResponse.setContentType("text/plain"); - theResponse.setCharacterEncoding("UTF-8"); - theResponse.getWriter().append(e.getMessage()); - theResponse.getWriter().close(); - + new ExceptionHandlingInterceptor().handleException(requestDetails, e, theRequest, theResponse); + } } @@ -897,7 +845,7 @@ public class RestfulServer extends HttpServlet { myInterceptors.add(theInterceptor); } - private boolean requestIsBrowser(HttpServletRequest theRequest) { + public static boolean requestIsBrowser(HttpServletRequest theRequest) { String userAgent = theRequest.getHeader("User-Agent"); return userAgent != null && userAgent.contains("Mozilla"); } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptor.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptor.java new file mode 100644 index 00000000000..46ff9a7a76d --- /dev/null +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptor.java @@ -0,0 +1,119 @@ +package ca.uhn.fhir.rest.server.interceptor; + +import java.io.IOException; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.apache.commons.lang3.exception.ExceptionUtils; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.model.base.resource.BaseOperationOutcome; +import ca.uhn.fhir.model.base.resource.BaseOperationOutcome.BaseIssue; +import ca.uhn.fhir.rest.method.Request; +import ca.uhn.fhir.rest.method.RequestDetails; +import ca.uhn.fhir.rest.server.Constants; +import ca.uhn.fhir.rest.server.RestfulServer; +import ca.uhn.fhir.rest.server.RestfulServer.NarrativeModeEnum; +import ca.uhn.fhir.rest.server.RestfulServerUtils; +import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException; +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; + +public class ExceptionHandlingInterceptor extends InterceptorAdapter { + + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ExceptionHandlingInterceptor.class); + private Class[] myReturnStackTracesForExceptionTypes; + + /** + * If any server methods throw an exception which extends any of the given exception types, the exception + * stack trace will be returned to the user. This can be useful for helping to diagnose issues, but may + * not be desirable for production situations. + * + * @param theExceptionTypes The exception types for which to return the stack trace to the user. + * @return Returns an instance of this interceptor, to allow for easy method chaining. + */ + public ExceptionHandlingInterceptor setReturnStackTracesForExceptionTypes(Class... theExceptionTypes) { + myReturnStackTracesForExceptionTypes = theExceptionTypes; + return this; + } + + @Override + public boolean handleException(RequestDetails theRequestDetails, Throwable theException, HttpServletRequest theRequest, HttpServletResponse theResponse) throws ServletException, IOException { + + BaseOperationOutcome oo = null; + int statusCode = Constants.STATUS_HTTP_500_INTERNAL_ERROR; + + FhirContext ctx = theRequestDetails.getServer().getFhirContext(); + + if (theException instanceof BaseServerResponseException) { + oo = ((BaseServerResponseException) theException).getOperationOutcome(); + statusCode = ((BaseServerResponseException) theException).getStatusCode(); + } + + /* + * Generate an OperationOutcome to return, unless the exception throw by the resource provider had one + */ + if (oo == null) { + try { + oo = (BaseOperationOutcome) ctx.getResourceDefinition("OperationOutcome").getImplementingClass().newInstance(); + } catch (Exception e1) { + ourLog.error("Failed to instantiate OperationOutcome resource instance", e1); + throw new ServletException("Failed to instantiate OperationOutcome resource instance", e1); + } + + BaseIssue issue = oo.addIssue(); + issue.getSeverityElement().setValue("error"); + if (theException instanceof InternalErrorException) { + ourLog.error("Failure during REST processing", theException); + populateDetails(theException, issue); + } else if (theException instanceof BaseServerResponseException) { + ourLog.warn("Failure during REST processing: {}", theException); + BaseServerResponseException baseServerResponseException = (BaseServerResponseException) theException; + statusCode = baseServerResponseException.getStatusCode(); + populateDetails(theException, issue); + if (baseServerResponseException.getAdditionalMessages() != null) { + for (String next : baseServerResponseException.getAdditionalMessages()) { + BaseIssue issue2 = oo.addIssue(); + issue2.getSeverityElement().setValue("error"); + issue2.setDetails(next); + } + } + } else { + ourLog.error("Failure during REST processing: " + theException.toString(), theException); + populateDetails(theException, issue); + statusCode = Constants.STATUS_HTTP_500_INTERNAL_ERROR; + } + } else { + ourLog.error("Unknown error during processing", theException); + } + + boolean requestIsBrowser = RestfulServer.requestIsBrowser(theRequest); + String fhirServerBase = ((Request) theRequestDetails).getFhirServerBase(); + RestfulServerUtils.streamResponseAsResource(theRequestDetails.getServer(), theResponse, oo, RestfulServerUtils.determineResponseEncodingNoDefault(theRequest), true, requestIsBrowser, + NarrativeModeEnum.NORMAL, statusCode, false, fhirServerBase); + + theResponse.setStatus(statusCode); + theRequestDetails.getServer().addHeadersToResponse(theResponse); + theResponse.setContentType("text/plain"); + theResponse.setCharacterEncoding("UTF-8"); + theResponse.getWriter().append(theException.getMessage()); + theResponse.getWriter().close(); + + return false; + } + + private void populateDetails(Throwable theException, BaseIssue issue) { + if (myReturnStackTracesForExceptionTypes != null) { + for (Class next : myReturnStackTracesForExceptionTypes) { + if (next.isAssignableFrom(theException.getClass())) { + issue.getDetailsElement().setValue(theException.getMessage() + "\n\n" + ExceptionUtils.getStackTrace(theException)); + return; + } + } + } + + issue.getDetailsElement().setValue(theException.getMessage()); + } + +} diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/ExceptionTest.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/ExceptionTest.java index 44c9e5ca8cb..66ebd6150ee 100644 --- a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/ExceptionTest.java +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/ExceptionTest.java @@ -1,5 +1,6 @@ package ca.uhn.fhir.rest.server; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.*; import java.util.List; @@ -41,35 +42,24 @@ import ca.uhn.fhir.util.PortUtil; */ public class ExceptionTest { + private static final String OPERATION_OUTCOME_DETAILS = "OperationOutcomeDetails"; private static CloseableHttpClient ourClient; + private static Class ourExceptionType; private static boolean ourGenerateOperationOutcome; private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ExceptionTest.class); + private static int ourPort; + private static Server ourServer; private static RestfulServer servlet; - + @Before public void before() { ourGenerateOperationOutcome = false; ourExceptionType=null; } - @Test - public void testThrowUnprocessableEntityWithMultipleMessages() throws Exception { - { - HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwUnprocessableEntityWithMultipleMessages=aaa"); - HttpResponse status = ourClient.execute(httpGet); - String responseContent = IOUtils.toString(status.getEntity().getContent()); - IOUtils.closeQuietly(status.getEntity().getContent()); - ourLog.info(responseContent); - assertEquals(422, status.getStatusLine().getStatusCode()); - OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newXmlParser().parseResource(responseContent); - assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("message1")); - assertEquals(3, oo.getIssue().size()); - } - } - @Test public void testInternalError() throws Exception { { @@ -80,10 +70,39 @@ public class ExceptionTest { ourLog.info(responseContent); assertEquals(500, status.getStatusLine().getStatusCode()); OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newXmlParser().parseResource(responseContent); - assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("InternalErrorException: Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), not(StringContains.containsString("InternalErrorException"))); } } + @Test + public void testInternalErrorFormatted() throws Exception { + { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwInternalError=aaa&_format=true"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(500, status.getStatusLine().getStatusCode()); + OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newXmlParser().parseResource(responseContent); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), not(StringContains.containsString("InternalErrorException"))); + } + } + + @Test + public void testInternalErrorJson() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwInternalError=aaa&_format=json"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(500, status.getStatusLine().getStatusCode()); + OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newJsonParser().parseResource(responseContent); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), not(StringContains.containsString("InternalErrorException"))); + } + @Test public void testResourceReturning() throws Exception { // No OO @@ -115,31 +134,36 @@ public class ExceptionTest { } @Test - public void testInternalErrorFormatted() throws Exception { + public void testThrowUnprocessableEntityWithMultipleMessages() throws Exception { { - HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwInternalError=aaa&_format=true"); + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwUnprocessableEntityWithMultipleMessages=aaa"); HttpResponse status = ourClient.execute(httpGet); String responseContent = IOUtils.toString(status.getEntity().getContent()); IOUtils.closeQuietly(status.getEntity().getContent()); ourLog.info(responseContent); - assertEquals(500, status.getStatusLine().getStatusCode()); + assertEquals(422, status.getStatusLine().getStatusCode()); OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newXmlParser().parseResource(responseContent); - assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("InternalErrorException: Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("message1")); + assertEquals(3, oo.getIssue().size()); } } @Test - public void testInternalErrorJson() throws Exception { - HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwInternalError=aaa&_format=json"); - HttpResponse status = ourClient.execute(httpGet); - String responseContent = IOUtils.toString(status.getEntity().getContent()); - IOUtils.closeQuietly(status.getEntity().getContent()); - ourLog.info(responseContent); - assertEquals(500, status.getStatusLine().getStatusCode()); - OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newJsonParser().parseResource(responseContent); - assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("InternalErrorException: Exception Text")); + public void testUnprocessableEntityFormatted() throws Exception { + { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwUnprocessableEntity=aaa&_format=true"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(UnprocessableEntityException.STATUS_CODE, status.getStatusLine().getStatusCode()); + OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newXmlParser().parseResource(responseContent); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), not(StringContains.containsString("UnprocessableEntityException"))); + } } - + + @AfterClass public static void afterClass() throws Exception { ourServer.stop(); @@ -166,27 +190,12 @@ public class ExceptionTest { ourClient = builder.build(); } - - - private static Class ourExceptionType; - - private static final String OPERATION_OUTCOME_DETAILS = "OperationOutcomeDetails"; /** * Created by dsotnikov on 2/25/2014. */ public static class DummyPatientResourceProvider implements IResourceProvider { - @Search - public List throwInternalError(@RequiredParam(name = "throwInternalError") StringParam theParam) { - throw new InternalErrorException("Exception Text"); - } - - @Search - public List throwUnprocessableEntityWithMultipleMessages(@RequiredParam(name = "throwUnprocessableEntityWithMultipleMessages") StringParam theParam) { - throw new UnprocessableEntityException("message1", "message2", "message3"); - } - @Override public Class getResourceType() { return Patient.class; @@ -208,6 +217,21 @@ public class ExceptionTest { } + @Search + public List throwInternalError(@RequiredParam(name = "throwInternalError") StringParam theParam) { + throw new InternalErrorException("Exception Text"); + } + + @Search() + public List throwUnprocessableEntity(@RequiredParam(name = "throwUnprocessableEntity") StringParam theParam) { + throw new UnprocessableEntityException("Exception Text"); + } + + @Search + public List throwUnprocessableEntityWithMultipleMessages(@RequiredParam(name = "throwUnprocessableEntityWithMultipleMessages") StringParam theParam) { + throw new UnprocessableEntityException("message1", "message2", "message3"); + } + } } diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptorTest.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptorTest.java index 5f40c786aa3..497f74982f4 100644 --- a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptorTest.java +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionHandlingInterceptorTest.java @@ -1,16 +1,13 @@ package ca.uhn.fhir.rest.server.interceptor; -import static org.junit.Assert.*; -import static org.mockito.Mockito.*; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; -import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.concurrent.TimeUnit; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; +import junit.framework.AssertionFailedError; import org.apache.commons.io.IOUtils; import org.apache.http.HttpResponse; @@ -26,96 +23,87 @@ import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; -import org.mockito.ArgumentCaptor; -import org.mockito.invocation.InvocationOnMock; -import org.mockito.stubbing.Answer; -import org.slf4j.Logger; -import ca.uhn.fhir.model.dstu.composite.HumanNameDt; -import ca.uhn.fhir.model.dstu.composite.IdentifierDt; +import ca.uhn.fhir.model.api.IResource; +import ca.uhn.fhir.model.dstu.resource.OperationOutcome; import ca.uhn.fhir.model.dstu.resource.Patient; -import ca.uhn.fhir.model.dstu.valueset.IdentifierUseEnum; import ca.uhn.fhir.model.primitive.IdDt; -import ca.uhn.fhir.model.primitive.UriDt; 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.method.RequestDetails; +import ca.uhn.fhir.rest.param.StringParam; +import ca.uhn.fhir.rest.server.ExceptionTest; import ca.uhn.fhir.rest.server.IResourceProvider; import ca.uhn.fhir.rest.server.RestfulServer; +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; +import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; import ca.uhn.fhir.util.PortUtil; public class ExceptionHandlingInterceptorTest { + private static final String OPERATION_OUTCOME_DETAILS = "OperationOutcomeDetails"; private static CloseableHttpClient ourClient; + private static Class ourExceptionType; + private static boolean ourGenerateOperationOutcome; + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ExceptionTest.class); + private static int ourPort; + private static Server ourServer; + private static RestfulServer servlet; - private IServerInterceptor myInterceptor; - private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ExceptionHandlingInterceptorTest.class); - - @Test - public void testThrowUnprocessableEntityException() throws Exception { - - when(myInterceptor.incomingRequestPreProcessed(any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); - when(myInterceptor.incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); - when(myInterceptor.handleException(any(RequestDetails.class), any(Throwable.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); - - HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_query=throwUnprocessableEntityException"); - HttpResponse status = ourClient.execute(httpGet); - ourLog.info(IOUtils.toString(status.getEntity().getContent())); - assertEquals(422, status.getStatusLine().getStatusCode()); - IOUtils.closeQuietly(status.getEntity().getContent()); - - ArgumentCaptor captor = ArgumentCaptor.forClass(Throwable.class); - verify(myInterceptor, times(1)).handleException(any(RequestDetails.class), captor.capture(), any(HttpServletRequest.class), any(HttpServletResponse.class)); - - assertEquals(UnprocessableEntityException.class, captor.getValue().getClass()); - } + private static ExceptionHandlingInterceptor myInterceptor; - - @Test - public void testThrowUnprocessableEntityExceptionAndOverrideResponse() throws Exception { - - when(myInterceptor.incomingRequestPreProcessed(any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); - when(myInterceptor.incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); - - when(myInterceptor.handleException(any(RequestDetails.class), any(Throwable.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenAnswer(new Answer() { - @Override - public Boolean answer(InvocationOnMock theInvocation) throws Throwable { - HttpServletResponse resp = (HttpServletResponse) theInvocation.getArguments()[3]; - resp.setStatus(405); - resp.setContentType("text/plain"); - resp.getWriter().write("HELP IM A BUG"); - resp.getWriter().close(); - return false; - } - }); - - HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_query=throwUnprocessableEntityException"); - HttpResponse status = ourClient.execute(httpGet); - String responseContent = IOUtils.toString(status.getEntity().getContent()); - ourLog.info(responseContent); - assertEquals(405, status.getStatusLine().getStatusCode()); - IOUtils.closeQuietly(status.getEntity().getContent()); - - assertEquals("HELP IM A BUG", responseContent); + @Before + public void before() { + ourGenerateOperationOutcome = false; + ourExceptionType=null; + myInterceptor.setReturnStackTracesForExceptionTypes(Throwable.class); } + @Test + public void testInternalError() throws Exception { + myInterceptor.setReturnStackTracesForExceptionTypes(Throwable.class); + { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwInternalError=aaa"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(500, status.getStatusLine().getStatusCode()); + OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newXmlParser().parseResource(responseContent); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), (StringContains.containsString("InternalErrorException: Exception Text"))); + } + } + + @Test + public void testInternalErrorFormatted() throws Exception { + myInterceptor.setReturnStackTracesForExceptionTypes(Throwable.class); + { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?throwInternalError=aaa&_format=true"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(500, status.getStatusLine().getStatusCode()); + OperationOutcome oo = (OperationOutcome) servlet.getFhirContext().newXmlParser().parseResource(responseContent); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), StringContains.containsString("Exception Text")); + assertThat(oo.getIssueFirstRep().getDetails().getValue(), (StringContains.containsString("InternalErrorException: Exception Text"))); + } + } + + + + @AfterClass public static void afterClass() throws Exception { ourServer.stop(); } - @Before - public void before() { - myInterceptor = mock(IServerInterceptor.class); - servlet.setInterceptors(Collections.singletonList(myInterceptor)); - } - @BeforeClass public static void beforeClass() throws Exception { ourPort = PortUtil.findFreePort(); @@ -136,30 +124,51 @@ public class ExceptionHandlingInterceptorTest { builder.setConnectionManager(connectionManager); ourClient = builder.build(); + myInterceptor = new ExceptionHandlingInterceptor(); + servlet.registerInterceptor(myInterceptor); } - /** * Created by dsotnikov on 2/25/2014. */ public static class DummyPatientResourceProvider implements IResourceProvider { - /** - * Retrieve the resource by its identifier - * - * @param theId - * The resource identity - * @return The resource - */ - @Search(queryName = "throwUnprocessableEntityException") - public List throwUnprocessableEntityException() { - throw new UnprocessableEntityException("Unprocessable!"); - } @Override - public Class getResourceType() { + public Class getResourceType() { return Patient.class; } + @Read + public Patient read(@IdParam IdDt theId) { + OperationOutcome oo = null; + if (ourGenerateOperationOutcome) { + oo = new OperationOutcome(); + oo.addIssue().setDetails(OPERATION_OUTCOME_DETAILS); + } + + if (ourExceptionType == ResourceNotFoundException.class) { + throw new ResourceNotFoundException(theId, oo); + }else { + throw new AssertionFailedError("Unknown exception type: " + ourExceptionType); + } + + } + + @Search + public List throwInternalError(@RequiredParam(name = "throwInternalError") StringParam theParam) { + throw new InternalErrorException("Exception Text"); + } + + @Search() + public List throwUnprocessableEntity(@RequiredParam(name = "throwUnprocessableEntity") StringParam theParam) { + throw new UnprocessableEntityException("Exception Text"); + } + + @Search + public List throwUnprocessableEntityWithMultipleMessages(@RequiredParam(name = "throwUnprocessableEntityWithMultipleMessages") StringParam theParam) { + throw new UnprocessableEntityException("message1", "message2", "message3"); + } + } } diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionInterceptorMethodTest.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionInterceptorMethodTest.java new file mode 100644 index 00000000000..f8b62273190 --- /dev/null +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/interceptor/ExceptionInterceptorMethodTest.java @@ -0,0 +1,165 @@ +package ca.uhn.fhir.rest.server.interceptor; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.*; + +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.apache.commons.io.IOUtils; +import org.apache.http.HttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.servlet.ServletHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.hamcrest.core.StringContains; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.slf4j.Logger; + +import ca.uhn.fhir.model.dstu.composite.HumanNameDt; +import ca.uhn.fhir.model.dstu.composite.IdentifierDt; +import ca.uhn.fhir.model.dstu.resource.Patient; +import ca.uhn.fhir.model.dstu.valueset.IdentifierUseEnum; +import ca.uhn.fhir.model.primitive.IdDt; +import ca.uhn.fhir.model.primitive.UriDt; +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.method.RequestDetails; +import ca.uhn.fhir.rest.server.IResourceProvider; +import ca.uhn.fhir.rest.server.RestfulServer; +import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import ca.uhn.fhir.util.PortUtil; + +public class ExceptionInterceptorMethodTest { + + private static CloseableHttpClient ourClient; + private static int ourPort; + private static Server ourServer; + private static RestfulServer servlet; + private IServerInterceptor myInterceptor; + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ExceptionInterceptorMethodTest.class); + + @Test + public void testThrowUnprocessableEntityException() throws Exception { + + when(myInterceptor.incomingRequestPreProcessed(any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); + when(myInterceptor.incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); + when(myInterceptor.handleException(any(RequestDetails.class), any(Throwable.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); + + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_query=throwUnprocessableEntityException"); + HttpResponse status = ourClient.execute(httpGet); + ourLog.info(IOUtils.toString(status.getEntity().getContent())); + assertEquals(422, status.getStatusLine().getStatusCode()); + IOUtils.closeQuietly(status.getEntity().getContent()); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Throwable.class); + verify(myInterceptor, times(1)).handleException(any(RequestDetails.class), captor.capture(), any(HttpServletRequest.class), any(HttpServletResponse.class)); + + assertEquals(UnprocessableEntityException.class, captor.getValue().getClass()); + } + + + @Test + public void testThrowUnprocessableEntityExceptionAndOverrideResponse() throws Exception { + + when(myInterceptor.incomingRequestPreProcessed(any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); + when(myInterceptor.incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true); + + when(myInterceptor.handleException(any(RequestDetails.class), any(Throwable.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenAnswer(new Answer() { + @Override + public Boolean answer(InvocationOnMock theInvocation) throws Throwable { + HttpServletResponse resp = (HttpServletResponse) theInvocation.getArguments()[3]; + resp.setStatus(405); + resp.setContentType("text/plain"); + resp.getWriter().write("HELP IM A BUG"); + resp.getWriter().close(); + return false; + } + }); + + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_query=throwUnprocessableEntityException"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + ourLog.info(responseContent); + assertEquals(405, status.getStatusLine().getStatusCode()); + IOUtils.closeQuietly(status.getEntity().getContent()); + + assertEquals("HELP IM A BUG", responseContent); + + } + + @AfterClass + public static void afterClass() throws Exception { + ourServer.stop(); + } + + @Before + public void before() { + myInterceptor = mock(IServerInterceptor.class); + servlet.setInterceptors(Collections.singletonList(myInterceptor)); + } + + @BeforeClass + public static void beforeClass() throws Exception { + ourPort = PortUtil.findFreePort(); + ourServer = new Server(ourPort); + + DummyPatientResourceProvider patientProvider = new DummyPatientResourceProvider(); + + ServletHandler proxyHandler = new ServletHandler(); + servlet = new RestfulServer(); + servlet.setResourceProviders(patientProvider); + ServletHolder servletHolder = new ServletHolder(servlet); + proxyHandler.addServletWithMapping(servletHolder, "/*"); + ourServer.setHandler(proxyHandler); + ourServer.start(); + + PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(5000, TimeUnit.MILLISECONDS); + HttpClientBuilder builder = HttpClientBuilder.create(); + builder.setConnectionManager(connectionManager); + ourClient = builder.build(); + + } + + /** + * Created by dsotnikov on 2/25/2014. + */ + public static class DummyPatientResourceProvider implements IResourceProvider { + + /** + * Retrieve the resource by its identifier + * + * @param theId + * The resource identity + * @return The resource + */ + @Search(queryName = "throwUnprocessableEntityException") + public List throwUnprocessableEntityException() { + throw new UnprocessableEntityException("Unprocessable!"); + } + + @Override + public Class getResourceType() { + return Patient.class; + } + + } + +} diff --git a/src/changes/changes.xml b/src/changes/changes.xml index f83a98e2bf1..976bbf58632 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -158,6 +158,12 @@ Sorting is now supported in the Web Testing UI (previously a button existed for sorting, but it didn't do anything) + + Server will no longer include stack traces in the OperationOutcome returned to the client + when an exception is thrown. A new interceptor called ExceptionHandlingInterceptor has been + created which adds this functionality back if it is needed (e.g. for DEV setups). See the + server interceptor documentation for more information. Thanks to Andy Huang for the suggestion! + diff --git a/src/site/xdoc/doc_rest_server_interceptor.xml b/src/site/xdoc/doc_rest_server_interceptor.xml index 877f706db3b..d5899792a6e 100644 --- a/src/site/xdoc/doc_rest_server_interceptor.xml +++ b/src/site/xdoc/doc_rest_server_interceptor.xml @@ -150,8 +150,37 @@ + + + +

+ The + ExceptionHandlingInterceptor + (code) + can be used to customize what is returned to the client and what is logged when the server throws an + exception for any reason (including routine things like UnprocessableEntityExceptions thrown as a matter of + normal processing in a create method, but also including unexpected NullPointerExceptions thrown by client code). +

+ +

+ The following example shows how to register an exception handling interceptor within + a FHIR RESTful server. +

+ + + + + +

+ This interceptor will then produce output similar to the following: +

+ + + + - +