From 5da368ef082a78edd21702b096c9926f3be335b3 Mon Sep 17 00:00:00 2001 From: Mikael Quist Date: Tue, 30 Jul 2019 16:22:37 -0700 Subject: [PATCH 1/2] Added handling of all exceptions in LambdaContainerHandler.proxyStream() to return an error rather than a 200. --- .../serverless/proxy/internal/LambdaContainerHandler.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java index 0e61495a0..9af153f56 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java @@ -215,6 +215,9 @@ public void proxyStream(InputStream input, OutputStream output, Context context) } catch (JsonMappingException e) { log.error("Error while mapping object to RequestType class", e); getObjectMapper().writeValue(output, exceptionHandler.handle(e)); + } catch (Exception e) { + log.error("Error while proxying request.", e); + getObjectMapper().writeValue(output, exceptionHandler.handle(e)); } finally { output.flush(); output.close(); From 089637083e74e626a777399356bdf27543a5378c Mon Sep 17 00:00:00 2001 From: Mikael Quist Date: Wed, 31 Jul 2019 12:16:59 -0700 Subject: [PATCH 2/2] Adding InternalServerError to exception handler, modified the JerseyServletResponseWriter.failure() to throw if an error occured rather than return ultimately returning a 200 --- .../proxy/AwsProxyExceptionHandler.java | 6 ++- .../internal/LambdaContainerHandler.java | 3 -- .../proxy/AwsProxyExceptionHandlerTest.java | 43 ++++++++++++++++- .../jersey/JerseyServletResponseWriter.java | 10 +--- .../proxy/jersey/EchoJerseyResource.java | 4 ++ .../proxy/jersey/JerseyAwsProxyTest.java | 47 ++++++++++++++----- .../proxy/jersey/JerseyDependency.java | 6 +++ .../proxy/jersey/JerseyInjectionTest.java | 2 +- .../proxy/jersey/JerseyParamEncodingTest.java | 6 +-- .../proxy/jersey/ResourceBinder.java | 12 +++++ 10 files changed, 108 insertions(+), 31 deletions(-) create mode 100644 aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyDependency.java create mode 100644 aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/ResourceBinder.java diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandler.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandler.java index d2b96d8e2..cf2aae3e7 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandler.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandler.java @@ -22,6 +22,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.ws.rs.InternalServerErrorException; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; @@ -31,7 +32,8 @@ /** * Default implementation of the ExceptionHandler object that returns AwsProxyResponse objects. * - * Returns application/json messages with a status code of 500 when the RequestReader failed to read the incoming event. + * Returns application/json messages with a status code of 500 when the RequestReader failed to read the incoming event + * or if InternalServerErrorException is thrown. * For all other exceptions returns a 502. Responses are populated with a JSON object containing a message property. * * @see ExceptionHandler @@ -76,7 +78,7 @@ public AwsProxyResponse handle(Throwable ex) { // adding a print stack trace in case we have no appender or we are running inside SAM local, where need the // output to go to the stderr. ex.printStackTrace(); - if (ex instanceof InvalidRequestEventException) { + if (ex instanceof InvalidRequestEventException || ex instanceof InternalServerErrorException) { return new AwsProxyResponse(500, headers, getErrorJson(INTERNAL_SERVER_ERROR)); } else { return new AwsProxyResponse(502, headers, getErrorJson(GATEWAY_TIMEOUT_ERROR)); diff --git a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java index 9af153f56..0e61495a0 100644 --- a/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java +++ b/aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/LambdaContainerHandler.java @@ -215,9 +215,6 @@ public void proxyStream(InputStream input, OutputStream output, Context context) } catch (JsonMappingException e) { log.error("Error while mapping object to RequestType class", e); getObjectMapper().writeValue(output, exceptionHandler.handle(e)); - } catch (Exception e) { - log.error("Error while proxying request.", e); - getObjectMapper().writeValue(output, exceptionHandler.handle(e)); } finally { output.flush(); output.close(); diff --git a/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandlerTest.java b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandlerTest.java index b5ed6f77a..cf86c57a6 100644 --- a/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandlerTest.java +++ b/aws-serverless-java-container-core/src/test/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandlerTest.java @@ -3,7 +3,6 @@ import com.amazonaws.serverless.exceptions.InvalidRequestEventException; import com.amazonaws.serverless.exceptions.InvalidResponseObjectException; -import com.amazonaws.serverless.proxy.AwsProxyExceptionHandler; import com.amazonaws.serverless.proxy.model.AwsProxyResponse; import com.amazonaws.serverless.proxy.model.ErrorModel; import com.fasterxml.jackson.core.JsonProcessingException; @@ -16,18 +15,22 @@ import org.junit.Before; import org.junit.Test; +import org.mockito.Mockito; +import javax.ws.rs.InternalServerErrorException; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; import java.io.*; public class AwsProxyExceptionHandlerTest { + private static final String INTERNAL_SERVER_ERROR_MESSAGE = "Internal server error"; private static final String INVALID_REQUEST_MESSAGE = "Invalid request error"; private static final String INVALID_RESPONSE_MESSAGE = "Invalid response error"; private AwsProxyExceptionHandler exceptionHandler; private ObjectMapper objectMapper; + @Before public void setUp() { exceptionHandler = new AwsProxyExceptionHandler(); @@ -88,6 +91,44 @@ public void typedHandle_InvalidResponseObjectException_jsonContentTypeHeader() { assertEquals(MediaType.APPLICATION_JSON, resp.getMultiValueHeaders().getFirst(HttpHeaders.CONTENT_TYPE)); } + @Test + public void typedHandle_InternalServerErrorException_500State() { + // Needed to mock InternalServerErrorException because it leverages RuntimeDelegate to set an internal + // response object. + InternalServerErrorException mockInternalServerErrorException = Mockito.mock(InternalServerErrorException.class); + Mockito.when(mockInternalServerErrorException.getMessage()).thenReturn(INTERNAL_SERVER_ERROR_MESSAGE); + + AwsProxyResponse resp = exceptionHandler.handle(mockInternalServerErrorException); + + assertNotNull(resp); + assertEquals(500, resp.getStatusCode()); + } + + @Test + public void typedHandle_InternalServerErrorException_responseString() + throws JsonProcessingException { + InternalServerErrorException mockInternalServerErrorException = Mockito.mock(InternalServerErrorException.class); + Mockito.when(mockInternalServerErrorException.getMessage()).thenReturn(INTERNAL_SERVER_ERROR_MESSAGE); + + AwsProxyResponse resp = exceptionHandler.handle(mockInternalServerErrorException); + + assertNotNull(resp); + String body = objectMapper.writeValueAsString(new ErrorModel(AwsProxyExceptionHandler.INTERNAL_SERVER_ERROR)); + assertEquals(body, resp.getBody()); + } + + @Test + public void typedHandle_InternalServerErrorException_jsonContentTypeHeader() { + InternalServerErrorException mockInternalServerErrorException = Mockito.mock(InternalServerErrorException.class); + Mockito.when(mockInternalServerErrorException.getMessage()).thenReturn(INTERNAL_SERVER_ERROR_MESSAGE); + + AwsProxyResponse resp = exceptionHandler.handle(mockInternalServerErrorException); + + assertNotNull(resp); + assertTrue(resp.getMultiValueHeaders().containsKey(HttpHeaders.CONTENT_TYPE)); + assertEquals(MediaType.APPLICATION_JSON, resp.getMultiValueHeaders().getFirst(HttpHeaders.CONTENT_TYPE)); + } + @Test public void typedHandle_NullPointerException_responseObject() throws JsonProcessingException { diff --git a/aws-serverless-java-container-jersey/src/main/java/com/amazonaws/serverless/proxy/jersey/JerseyServletResponseWriter.java b/aws-serverless-java-container-jersey/src/main/java/com/amazonaws/serverless/proxy/jersey/JerseyServletResponseWriter.java index c2cba7319..938c6f9ff 100644 --- a/aws-serverless-java-container-jersey/src/main/java/com/amazonaws/serverless/proxy/jersey/JerseyServletResponseWriter.java +++ b/aws-serverless-java-container-jersey/src/main/java/com/amazonaws/serverless/proxy/jersey/JerseyServletResponseWriter.java @@ -122,14 +122,8 @@ public void commit() { public void failure(Throwable throwable) { - try { - log.error("failure", throwable); - jerseyLatch.countDown(); - servletResponse.flushBuffer(); - } catch (IOException e) { - log.error("Could not fail response", e); - throw new InternalServerErrorException(e); - } + log.error("failure", throwable); + throw new InternalServerErrorException("Jersey failed to process request", throwable); } diff --git a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/EchoJerseyResource.java b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/EchoJerseyResource.java index da67dcf36..ec0f4b2ac 100644 --- a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/EchoJerseyResource.java +++ b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/EchoJerseyResource.java @@ -24,6 +24,7 @@ import org.glassfish.jersey.media.multipart.FormDataMultiPart; import org.glassfish.jersey.media.multipart.FormDataParam; +import javax.inject.Inject; import javax.servlet.ServletContext; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -59,6 +60,9 @@ public class EchoJerseyResource { @Context SecurityContext securityCtx; + @Inject + JerseyDependency jerseyDependency; + @Path("/decoded-param") @GET @Produces(MediaType.APPLICATION_JSON) public SingleValueModel echoDecodedParam(@QueryParam("param") String param) { diff --git a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyAwsProxyTest.java b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyAwsProxyTest.java index c54de6c5a..e791ae810 100644 --- a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyAwsProxyTest.java +++ b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyAwsProxyTest.java @@ -14,16 +14,15 @@ import com.amazonaws.serverless.proxy.internal.LambdaContainerHandler; -import com.amazonaws.serverless.proxy.jersey.providers.ServletRequestFilter; -import com.amazonaws.serverless.proxy.model.AwsProxyRequest; -import com.amazonaws.serverless.proxy.model.AwsProxyResponse; import com.amazonaws.serverless.proxy.internal.servlet.AwsServletContext; -import com.amazonaws.serverless.proxy.jersey.model.MapResponseModel; -import com.amazonaws.serverless.proxy.jersey.model.SingleValueModel; import com.amazonaws.serverless.proxy.internal.testutils.AwsProxyRequestBuilder; import com.amazonaws.serverless.proxy.internal.testutils.MockLambdaContext; +import com.amazonaws.serverless.proxy.jersey.model.MapResponseModel; +import com.amazonaws.serverless.proxy.jersey.model.SingleValueModel; +import com.amazonaws.serverless.proxy.jersey.providers.ServletRequestFilter; +import com.amazonaws.serverless.proxy.model.AwsProxyRequest; +import com.amazonaws.serverless.proxy.model.AwsProxyResponse; import com.amazonaws.services.lambda.runtime.Context; - import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.commons.codec.binary.Base64; @@ -37,13 +36,15 @@ import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; - import java.io.IOException; import java.util.Arrays; import java.util.Collection; import java.util.UUID; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * Unit test class for the Jersey AWS_PROXY default implementation @@ -57,13 +58,26 @@ public class JerseyAwsProxyTest { private static ObjectMapper objectMapper = new ObjectMapper(); + private static ResourceConfig app = new ResourceConfig().packages("com.amazonaws.serverless.proxy.jersey") + .register(LoggingFeature.class) + .register(ServletRequestFilter.class) + .register(MultiPartFeature.class) + .register(new ResourceBinder()) + .property(LoggingFeature.LOGGING_FEATURE_VERBOSITY_SERVER, LoggingFeature.Verbosity.PAYLOAD_ANY); + + private static ResourceConfig appWithoutRegisteredDependencies = new ResourceConfig() + .packages("com.amazonaws.serverless.proxy.jersey") .register(LoggingFeature.class) .register(ServletRequestFilter.class) .register(MultiPartFeature.class) .property(LoggingFeature.LOGGING_FEATURE_VERBOSITY_SERVER, LoggingFeature.Verbosity.PAYLOAD_ANY); + private static JerseyLambdaContainerHandler handler = JerseyLambdaContainerHandler.getAwsProxyHandler(app); + private static JerseyLambdaContainerHandler handlerWithoutRegisteredDependencies + = JerseyLambdaContainerHandler.getAwsProxyHandler(appWithoutRegisteredDependencies); + private static Context lambdaContext = new MockLambdaContext(); private boolean isAlb; @@ -76,15 +90,14 @@ public JerseyAwsProxyTest(boolean alb) { public static Collection data() { return Arrays.asList(new Object[] { false, true }); } - + private AwsProxyRequestBuilder getRequestBuilder(String path, String method) { AwsProxyRequestBuilder builder = new AwsProxyRequestBuilder(path, method); if (isAlb) builder.alb(); - + return builder; } - @Test public void alb_basicRequest_expectSuccess() { AwsProxyRequest request = getRequestBuilder("/echo/headers", "GET") @@ -130,6 +143,18 @@ public void headers_servletRequest_echo() { validateMapResponseModel(output); } + @Test + public void headers_servletRequest_failedDependencyInjection_expectInternalServerError() { + AwsProxyRequest request = getRequestBuilder("/echo/servlet-headers", "GET") + .json() + .header(CUSTOM_HEADER_KEY, CUSTOM_HEADER_VALUE) + .build(); + + AwsProxyResponse output = handlerWithoutRegisteredDependencies.proxy(request, lambdaContext); + assertEquals("application/json", output.getMultiValueHeaders().getFirst("Content-Type")); + assertEquals(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), output.getStatusCode()); + } + @Test public void context_servletResponse_setCustomHeader() { AwsProxyRequest request = getRequestBuilder("/echo/servlet-response", "GET") diff --git a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyDependency.java b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyDependency.java new file mode 100644 index 000000000..0633b96b4 --- /dev/null +++ b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyDependency.java @@ -0,0 +1,6 @@ +package com.amazonaws.serverless.proxy.jersey; + +// This class is used to test HK2 dependency injection. +public class JerseyDependency { + +} diff --git a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyInjectionTest.java b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyInjectionTest.java index 0af2785ee..e8e86270b 100644 --- a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyInjectionTest.java +++ b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyInjectionTest.java @@ -30,7 +30,7 @@ */ public class JerseyInjectionTest { - // Test ressource binder + // Test resource binder private static class ResourceBinder extends AbstractBinder { @Override diff --git a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyParamEncodingTest.java b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyParamEncodingTest.java index e3eb92de6..06edbdaab 100644 --- a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyParamEncodingTest.java +++ b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyParamEncodingTest.java @@ -11,7 +11,6 @@ import com.amazonaws.services.lambda.runtime.Context; import com.fasterxml.jackson.databind.ObjectMapper; -import org.glassfish.jersey.logging.LoggingFeature; import org.glassfish.jersey.media.multipart.MultiPartFeature; import org.glassfish.jersey.server.ResourceConfig; import org.junit.Ignore; @@ -26,10 +25,6 @@ import java.net.URLEncoder; import java.util.Arrays; import java.util.Collection; -import java.util.logging.ConsoleHandler; -import java.util.logging.Level; -import java.util.logging.Logger; -import java.util.logging.SimpleFormatter; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -69,6 +64,7 @@ public class JerseyParamEncodingTest { private static ObjectMapper objectMapper = new ObjectMapper(); private static ResourceConfig app = new ResourceConfig().packages("com.amazonaws.serverless.proxy.jersey") .register(MultiPartFeature.class) + .register(new ResourceBinder()) .property("jersey.config.server.tracing.type", "ALL") .property("jersey.config.server.tracing.threshold", "VERBOSE"); private static JerseyLambdaContainerHandler handler = JerseyLambdaContainerHandler.getAwsProxyHandler(app); diff --git a/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/ResourceBinder.java b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/ResourceBinder.java new file mode 100644 index 000000000..e98ea805a --- /dev/null +++ b/aws-serverless-java-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/ResourceBinder.java @@ -0,0 +1,12 @@ +package com.amazonaws.serverless.proxy.jersey; + +import org.glassfish.jersey.internal.inject.AbstractBinder; + +import javax.inject.Singleton; + +public class ResourceBinder extends AbstractBinder { + @Override + protected void configure() { + bind(new JerseyDependency()).to(JerseyDependency.class).in(Singleton.class); + } +}