Skip to content
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.

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

Handling ResponseStatusException Results in 502 #342

Open
szaluk opened this issue May 4, 2020 · 8 comments
Open

Handling ResponseStatusException Results in 502 #342

szaluk opened this issue May 4, 2020 · 8 comments
Assignees

Comments

@szaluk
Copy link

szaluk commented May 4, 2020

  • Framework version: 1.5 / 5.2.6-RELEASE
  • Implementations: Spring

Scenario

I am throwing a ResponseStatusException to handle 404's, 500's, etc but no matter when I throw one I am getting the following response with a 502 from the call:

{
    "message": "Gateway timeout"
}

This is an example of how I am creating the ResponseStatusException:

            if(queryResults.getSize() == 0) {
                throw new ResponseStatusException(HttpStatus.NOT_FOUND, String.format("Unable To Find User for Email Address %s", email));
            } else {
              // Do something else
            }

I am seeing the following in my logs so I know the exception is being thrown properly:

2020-05-04 21:20:04 f1ab9c4e-75ee-457f-b222-dedc69daf8f5 ERROR LambdaContainerHandler - Error while handling request
org.springframework.web.util.NestedServletException: Request processing failed; nested exception is org.springframework.web.server.ResponseStatusException: 404 NOT_FOUND "Unable To Find User for Email Address invalid@email.com"

It doesn't seem to handle the ResponseStatusException being thrown. I added the following to my application config class:

    /*
     * optimization - avoids creating default exception resolvers; not required as the serverless container handles
     * all exceptions
     *
     * By default, an ExceptionHandlerExceptionResolver is created which creates many dependent object, including
     * an expensive ObjectMapper instance.
     *
     * To enable custom @ControllerAdvice classes remove this bean.
     */
    @Bean
    public HandlerExceptionResolver handlerExceptionResolver() {
        return new HandlerExceptionResolver() {

            @Override
            public ModelAndView resolveException(HttpServletRequest request, HttpServletResponse response, Object handler, Exception ex) {
                return null;
            }
        };
    }

Shouldn't the container handle the ResponseStatusException automatically?

Thanks,
Steve

Expected behavior

The proper response code and response returned

Actual behavior

Always retuning a 502 and Bad Gateway reponse

@dtelaroli
Copy link

+1

@sapessi sapessi added this to the Release 1.5.1 milestone Jul 10, 2020
@sapessi sapessi self-assigned this Jul 10, 2020
@sapessi
Copy link
Collaborator

sapessi commented Jul 10, 2020

I attempted to replicate this in the SpringBoot 2 unit tests and it seems to work fine, the test below passes. Not sure what could be missing. Any hints?

Controller

@GetMapping(value = "/ex/customstatus")
public String throw404Exception() {
    throw new ResponseStatusException(HttpStatus.NOT_FOUND, EX_MESSAGE);
}

Test method

@Test
public void throw404Exception_callEndpoint_expect404StatusCode() {
    LambdaContainerHandler.getContainerConfig().setDefaultContentCharset(ContainerConfig.DEFAULT_CONTENT_CHARSET);
    AwsProxyRequestBuilder req = new AwsProxyRequestBuilder("/ex/customstatus", "GET");
    AwsProxyResponse resp = handler.handleRequest(req, lambdaContext);
    assertNotNull(resp);
    assertEquals(404, resp.getStatusCode());
}

@szaluk
Copy link
Author

szaluk commented Jul 10, 2020

It does work fine with Spring Boot. It's not working with Spring MVC. Sorry for not being clearer in my original post.

@sapessi
Copy link
Collaborator

sapessi commented Jul 10, 2020

Thanks @szaluk. I just added the same test to the Spring echo tests app (using Spring version 5.2.5.RELEASE) and it also works fine. Wonder if there's anything different in the app config. Let me know if you see anything in the code.

@sapessi
Copy link
Collaborator

sapessi commented Jul 11, 2020

Let me know if you can see what the difference is between my app's configuration and yours @szaluk, @dtelaroli

@sapessi
Copy link
Collaborator

sapessi commented Jul 15, 2020

Removing this from 1.5.1 release since I still cannot replicate and the patch release needs to go out.

@sapessi sapessi removed this from the Release 1.5.1 milestone Jul 15, 2020
sapessi added a commit that referenced this issue Jul 15, 2020
* fix: Adding pathParameters to v2 proxy event as reported in #358.

* fix: Address JSON content type issue reported in #352 and #344

* fix: Fixed bug caught by integration tests for #352

* fix: Fix struts tests for the changes made for #352

* test: Attempting to replicate the issue reported in #342

* test: Reverting exception test in Spring package since it's only available in Spring5, not Spring4

* fix: Sigh, forgot to remove the import for the class that doesn't exist from the previous commit

* fix: Addresses bug reported in query string parsing (#363) for HTTP API support where we have a query string key, followed by a value declarator (=), but then no value

* chore: Update GitHub issue and PR templates

* fix: Fixed issue reported by SpotBugs with the exception logging of the HTTP API query string parsing
jogep pushed a commit to jogep/aws-serverless-java-container that referenced this issue Dec 8, 2020
* fix: Adding pathParameters to v2 proxy event as reported in aws#358.

* fix: Address JSON content type issue reported in aws#352 and aws#344

* fix: Fixed bug caught by integration tests for aws#352

* fix: Fix struts tests for the changes made for aws#352

* test: Attempting to replicate the issue reported in aws#342

* test: Reverting exception test in Spring package since it's only available in Spring5, not Spring4

* fix: Sigh, forgot to remove the import for the class that doesn't exist from the previous commit

* fix: Addresses bug reported in query string parsing (aws#363) for HTTP API support where we have a query string key, followed by a value declarator (=), but then no value

* chore: Update GitHub issue and PR templates

* fix: Fixed issue reported by SpotBugs with the exception logging of the HTTP API query string parsing
@mrgatto
Copy link

mrgatto commented Sep 9, 2021

This really doesn´t work as posted by @szaluk. AwsProxyExceptionHandler handle exceptions with fixed error messages no matter what exception you throw.

Note that this is only true with the recommended approach of not using the Spring default ExceptionHandlerExceptionResolver.

I wonder how to set my own custom AwsProxyExceptionHandler to overcome this.

Other options are using the @ControllerAdvice with the default ExceptionHandlerExceptionResolver or creating a simpler implementation of HandlerExceptionResolver like in the example below:

public class CustomExceptionResolver implements HandlerExceptionResolver {

	private static Logger log = LoggerFactory.getLogger(CustomExceptionResolver.class);

	@Override
	public ModelAndView resolveException(HttpServletRequest request, HttpServletResponse response, Object handler,
			Exception ex) {

		ErrorModel errorModel = new ErrorModel("Unknown error");
		int status = HttpStatus.INTERNAL_SERVER_ERROR.value();

		if (ex instanceof ResponseStatusException) {
			ResponseStatusException responseException = (ResponseStatusException) ex;
			status = responseException.getRawStatusCode();
			errorModel.setMessage(responseException.getReason());
		}

		/* resolve other exceptions... */
		
		String body = null;
		try {
			body = LambdaContainerHandler.getObjectMapper().writeValueAsString(errorModel);
		} catch (JsonProcessingException e) {
			log.error("Could not produce error JSON", e);
			body = "{ \"message\": \"" + errorModel.getMessage() + "\" }";
		}

		try {
			response.setStatus(status);
			response.setContentType(ContentType.APPLICATION_JSON.getMimeType());
			response.setCharacterEncoding(StandardCharsets.UTF_8.name());
			response.getWriter().write(body);
		} catch (IOException e) {
			log.error("Could not write client http body", e);
		}

		return new ModelAndView();
	}
}

Framework version: 1.6 / Srping 5.3.9

@deki
Copy link
Collaborator

deki commented Dec 29, 2021

SpringProxyHandlerBuilder and SpringBootProxyHandlerBuilder allow to provide an ExceptionHandler using the exceptionHandler() method. By default AwsProxyExceptionHandler will be used.

We are open for suggestions and will also review PRs that optimize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants