Skip to content

Fixed issue where a 200 response is returned when server throws exception #272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fixed issue where a 200 response is returned when server throws exception #272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5da368e
Added handling of all exceptions in LambdaContainerHandler.proxyStrea…
Jul 30, 2019
0896370
Adding InternalServerError to exception handler, modified the JerseyS…
Jul 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...container-core/src/main/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -31,7 +32,8 @@
/**
* Default implementation of the <code>ExceptionHandler</code> 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
Expand Down Expand Up @@ -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));
Expand Down
43 changes: 42 additions & 1 deletion ...ainer-core/src/test/java/com/amazonaws/serverless/proxy/AwsProxyExceptionHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 2 additions & 8 deletions ...rsey/src/main/java/com/amazonaws/serverless/proxy/jersey/JerseyServletResponseWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering what will happen to the latch in this case. The behavior seems correct as far as the outcome (what we return to the client). I'll add a TODO list item to verify that we don't leak anything in this case. I'll take care of it before the next release

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tested that latch.await() is not called during unit tests but this might not be the case every time. Looking closer at the code it could cause a leak. I took a look at Jersey's ApplicationHandler.java they use a future and call .completeExceptionally(). Maybe the code should be refactored to use futures instead of a latch. Also I wonder how framework failures are handled by the other consumers of LambdaContainerHandler.java.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created issue #276 to track this for the 2.0 release of the framework.

}


Expand Down
4 changes: 4 additions & 0 deletions ...tainer-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/EchoJerseyResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
47 changes: 36 additions & 11 deletions ...tainer-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyAwsProxyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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<AwsProxyRequest, AwsProxyResponse> handler = JerseyLambdaContainerHandler.getAwsProxyHandler(app);

private static JerseyLambdaContainerHandler<AwsProxyRequest, AwsProxyResponse> handlerWithoutRegisteredDependencies
= JerseyLambdaContainerHandler.getAwsProxyHandler(appWithoutRegisteredDependencies);

private static Context lambdaContext = new MockLambdaContext();

private boolean isAlb;
Expand All @@ -76,15 +90,14 @@ public JerseyAwsProxyTest(boolean alb) {
public static Collection<Object> 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")
Expand Down Expand Up @@ -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")
Expand Down
6 changes: 6 additions & 0 deletions ...ontainer-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyDependency.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.amazonaws.serverless.proxy.jersey;

// This class is used to test HK2 dependency injection.
public class JerseyDependency {

}
2 changes: 1 addition & 1 deletion ...ainer-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyInjectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
*/
public class JerseyInjectionTest {

// Test ressource binder
// Test resource binder
private static class ResourceBinder extends AbstractBinder {

@Override
Expand Down
6 changes: 1 addition & 5 deletions ...r-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/JerseyParamEncodingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<AwsProxyRequest, AwsProxyResponse> handler = JerseyLambdaContainerHandler.getAwsProxyHandler(app);
Expand Down
12 changes: 12 additions & 0 deletions ...-container-jersey/src/test/java/com/amazonaws/serverless/proxy/jersey/ResourceBinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}