spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.6k stars 40.56k forks source link

DispatcherServlet#setThrowExceptionIfNoHandlerFound doesn't work for REST application #3980

Closed igilham closed 8 years ago

igilham commented 9 years ago

This issue occurs in Spring Boot 1.1.12, 1.2.0, and 1.2.5. There are a few related answers on StackOverflow but none of them seem to work.

I'm trying to override the error handling behaviour to return a json-encoded custom POJO to the user when exceptions are thrown by the controllers in my application. This works for most exceptions but I can't get it to work with for 404s where there is no handler matching the path of the request.

Overriding the DispatcherServlet and setting setThrowExceptionIfNoHandlerFound to true does not result in the appropriate exception handler being called.

Here is my DispatcherServlet Bean declaration:

@Configuration
public class WebAppContext extends WebMvcConfigurerAdapter {

    @Bean(name = DispatcherServletAutoConfiguration.DEFAULT_DISPATCHER_SERVLET_BEAN_NAME)
    public DispatcherServlet dispatcherServlet() {
        DispatcherServlet ds = new DispatcherServlet();
        ds.setDetectAllHandlerAdapters(true);
        ds.setDetectAllHandlerExceptionResolvers(true);
        ds.setDetectAllHandlerMappings(true);
        ds.setDetectAllViewResolvers(true);
        ds.setThrowExceptionIfNoHandlerFound(true);
        return ds;
    }

// ... more Bean definitions
}

And here's part of the global Exception handler:

@ControllerAdvice(annotations = RestController.class)
public class GlobalExceptionHandler extends ResponseEntityExceptionHandler {

    // TODO: figure out how to configure the DispatcherServlet's throwExceptionIfNoHandlerFound property so that this can work
    @Override
    protected ResponseEntity<Object> handleNoHandlerFoundException(NoHandlerFoundException ex, HttpHeaders headers, HttpStatus status, WebRequest request) {
        return new ResponseEntity<>(new CustomErrorType(HttpStatus.NOT_FOUND.value(), "Not Found"), HttpStatus.NOT_FOUND);
    }

// ... @ExceptionHandler methods etc.
}

I have also tried using the BeanPostProcessor interface to customise the DispatcherServlet as noted in a comment on an answer to this question, but that doesn't work either.

The documentation for how to handle global exceptions and customise the DispatcherServlet is incredibly obtuse. Am I missing something or is there a bug in there somewhere?

philwebb commented 9 years ago

We've clearly not done a great job at documenting this or making it particularly easy to use setThrowExceptionIfNoHandlerFound. I'll raise some new issues to address those points.

For your specific case I think that you can implement your own ErrorController to handle returning JSON. Looks at BasicErrorController for some inspiration. I tried adding this to one of the sample application and it appears to achieve what you need (assuming I've understood the issue correctly):

@Controller
@RequestMapping("${error.path:/error}")
public class CustomErrorController implements ErrorController {

    @Value("${error.path:/error}")
    private String errorPath;

    @Override
    public String getErrorPath() {
        return this.errorPath;
    }

    @RequestMapping
    @ResponseBody
    public ResponseEntity<Object> error(HttpServletRequest request) {
        HttpStatus status = getStatus(request);
        return new ResponseEntity<Object>(new CustomErrorType(status.value(), status.name()), status);
    }

    private HttpStatus getStatus(HttpServletRequest request) {
        Integer statusCode = (Integer) request.getAttribute("javax.servlet.error.status_code");
        if (statusCode == null) {
            return HttpStatus.INTERNAL_SERVER_ERROR;
        }
        return HttpStatus.valueOf(statusCode);
    }

}
$ curl -v -H "Accept: application/json" http://localhost:8080/missing
*   Trying ::1...
* Connected to localhost (::1) port 8080 (#0)
> GET /missing HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.43.0
> Accept: application/json
> 
< HTTP/1.1 404 Not Found
< Server: Apache-Coyote/1.1
< Content-Type: application/json;charset=UTF-8
< Transfer-Encoding: chunked
< Date: Mon, 21 Sep 2015 18:37:01 GMT
< 
* Connection #0 to host localhost left intact
{"code":404,"message":"NOT_FOUND"}
philwebb commented 9 years ago

@igilham I've raised #3998, #3999 and #4000 to address some concrete things that we can do to improve this. Let me know if the solution above works for you and I'll close this one.

igilham commented 9 years ago

@philwebb I originally started with an ErrorController implementation and Spring Boot 1.1.12.

When I changed the Spring Boot dependency to 1.2.x, the behaviour changed in ways not documented in the migration notes, so I had to scour the web for a better alternative. Using @ControllerAdvice and custom @ExceptionHandlers seemed to be the most idiomatic way to handle application-defined exceptions. Extending ResponseEntityExceptionHandler should have allowed me to handle common framework-defined exceptions, like a 404 occurring due to there being no matching handler for a request.

Unfortunately, the mechanisms made available to customise the DispatcherServlet do not appear to work. As noted previously, there appears to be no way to call setThrowExceptionIfNoHandlerFound and expect it to actually do anything, as the framework ignores or overrules that decision.

Can you confirm if the ErrorController is the preferred and supported way to do this in the current version of the framework?

Can you advise me how to prevent the framework from calling sendError, instead letting the ErrorController handle an exception?

philwebb commented 9 years ago

When I changed the Spring Boot dependency to 1.2.x, the behaviour changed in ways not documented in the migration notes, so I had to scour the web for a better alternative.

I'm sorry about that, whatever the issue was we should have at least documented it.

Using @ControllerAdvice and custom @ExceptionHandlers seemed to be the most idiomatic way to handle application-defined exceptions.

I agree, that does seem to be the best long-term solution. @rstoyanchev from the MVC team could probably confirm this.

Unfortunately, the mechanisms made available to customise the DispatcherServlet do not appear to work.

I'll double check this when looking into #4000 and make sure we add a test case or sample. I've not had time to dig deeper into this yet.

Can you advise me how to prevent the framework from calling sendError, instead letting the ErrorController handle an exception?

I will, as soon as I look into #4000 and check the MVC implementation details. My aim with suggesting that you use a custom ErrorController was to buy us a little time to look into this so that we can provide the best long-term solution for 1.3.

rstoyanchev commented 9 years ago

It sounds like when #4000 is resolved, it should be possible to use ResponseEntityExceptionHandler annotated with @ControllerAdvice consistently. If you want to implement your own REST API error handling it's indeed the most idiomatic Spring MVC way.

Nothing wrong with relying on the Boot provided ErrorController mechanism either for its default JSON responses or as a place to handle any other unhandled exceptions (e.g. from view rendering).

igilham commented 9 years ago

Thanks for your help @philwebb, @rstoyanchev.

For what it's worth, the undocumented behaviour changes I mentioned largely relate to Spring Security filtering and @ResponseStatus handling.

Using a custom GenericFilterBean in Boot 1.2 changes the accessibility of the Actuator endpoints, making it more difficult to fine-tune access control to business requirements.

The @ResponseStatus handling now exposes the reason field whereas it previously used the Exception's message property. This makes the mechanism less flexible overall, making it more difficult to provide context to the user via a simple Error Controller. This is the reason I started looking into the @ControllerAdvice and @ExceptionHandler approach.

jasperblues commented 8 years ago

:+1: @ControllerAdvice stopped working recently. Just now hand-rolled error controller using example above. Mostly worked, but I need to dig the exception up in order to fill in some details. Will use BasicErrorController as example.

jasperblues commented 8 years ago

It turns out the exception is gone from the request - only the high level details are there. The workaround listed above doesn't help us - we need the exception so that we can map some information there onto our response object.

wilkinsona commented 8 years ago

@jasperblues Can you provide a small sample that illustrates your problem?

jasperblues commented 8 years ago

Sure. The following works fine:

@Controller
@RequestMapping("@{error.path:/error}")
class VamprErrorController @Autowired constructor(errorAttributes: ErrorAttributes): ErrorController
{

    @Value("@{error.path:/error}")
    lateinit var mapping: String

    val errorAttributes: ErrorAttributes

init
    {
        this.errorAttributes = errorAttributes
    }

    @RequestMapping
    @ResponseBody
    fun error(request: HttpServletRequest): ResponseEntity<ErrorResponse>
    {
        val status = getStatus(request)
        val attributes = getErrorAttributes(request)

        val header = ResponseHeader(ResponseCode.VAMPServiceError, attributes["message"] as String?)
        val errorResponse = ErrorResponse(header)
        return ResponseEntity(errorResponse, status)
    }

    private fun getErrorAttributes(request: HttpServletRequest): Map<String, Any>
    {
        val requestAttributes = ServletRequestAttributes(request)
        return this.errorAttributes.getErrorAttributes(requestAttributes, true)
    }

//etc utility methods

But in the example above, where we have ResponseCode.VAMPServiceError, in fact I want to ask the exception what that should be. So that's been set up as follows:

public enum class ResponseCode
{
    VAMPServiceError,
    VAMPUnauthorized,
    VAMPContentNotFound
}

interface ResponseCodeReporter
{
    fun responseCode() : ResponseCode
}

/**
 * By default, throwable maps to VAMPServiceError. Specific exception types may return alternative a ResponseCode that
 * is appropriate to the type.
 */
public fun Throwable.responseCode() : ResponseCode
{
    return ResponseCode.VAMPServiceError
}

And specific exception types can have their own response code.

When it came to wiring it up, the following does not work, because the exception is no longer in the HttpServletRequest by the time it reaches the error controller:


    @RequestMapping
    @ResponseBody
    fun error(request: HttpServletRequest): ResponseEntity<ErrorResponse>
    {
        val status = getStatus(request)
        val requestAttributes = ServletRequestAttributes(request)
        val exception = errorAttributes.getError(requestAttributes) //<----- Gone. This is null

        val header = ResponseHeader(exception.responseCode(), exception.message)
        val errorResponse = ErrorResponse(header)
        return ResponseEntity(errorResponse, status)
    }

If you set a break-point on private void addErrorDetails(Map<String, Object> errorAttributes in BasicErrorAttributes, you'll see that the Throwable is not around to extract error details from, instead we have the fields message, status, etc.

Like @igilham our API contract includes a custom payload for errors. I need to map the exception onto an application field.

wilkinsona commented 8 years ago

Thanks, @jasperblues.

because the exception is no longer in the HttpServletRequest by the time it reaches the error controller

What exception are you referring to? It works fine for me if I have a handler method that throws an exception.

Like @igilham our API contract includes a custom payload for errors. I need to map the exception onto an application field.

@igilham was having trouble with setThrowExceptionIfNoHandlerFound. It's not clear from the above if you're struggling with the same thing or something else.

In short, you haven't really given me enough to go on. What you have given me is also written in Kotlin(?) which makes running it rather tricky or means I need to convert it to Java and may inadvertently end up testing something that's subtly different.

Can you please provide a complete sample that reproduces the specific problem that you're seeing? A project on GitHub that I can clone and run, along with details of what request to make to trigger the problem would be ideal.

jasperblues commented 8 years ago

hello @wilkinsona,

Can you please provide a complete sample that reproduces the specific problem that you're seeing?

Sure. I'll provide answers to your questions below first. Perhaps there's something that I'm doing obviously wrong - I'm new to Spring Boot. If something's still not clear, I'll set up a sample project with a failing test and accompanying documentation for you.

What you have given me is also written in Kotlin(?)

It was indeed. I think Kotlin is a fantastic pairing for Spring Boot! It provides a fashionable, modernized syntax along with better support for functional programming, where needed, without eschewing the existing Java eco-system.

Additionally, we're a mobile and rich-media studio (check out our Spring inspired https://github.com/appsquickly/Typhoon for iOS) and we see the following additional benefits with Kotlin:

. . . but back to the problem at hand:

Do you need me to attach the sample project in Java or is it enough to have a gradle.build and failing test that would allow you to observe the problem first-hand?

was having trouble with setThrowExceptionIfNoHandlerFound. It's not clear from the above if you're struggling with the same thing or something else.

:blush: Ah, sorry, that may be the case. My issue is:

{
  "response": {
    "code": "VAMPForbidden",
    "message": "Description of error",
  }
}

In Summary:

jasperblues commented 8 years ago

It sounds like when #4000 is resolved, it should be possible to use ResponseEntityExceptionHandler annotated with @ControllerAdvice consistently. If you want to implement your own REST API error handling it's indeed the most idiomatic Spring MVC way.

This would be ideal for me, and was working before.

Proceed with sample project?

jasperblues commented 8 years ago

@wilkinsona After studying the docs for Spring Boot (rtfm before reading github issues) it turns out that I'm simply asking why my @ControllerAdvice exception handler is not firing.

Question here: http://stackoverflow.com/questions/34366964/spring-boot-restore-controlleradvice-aka-remove-basicerrorcontroller

You were right, my question was not related to this ticket.

Edit: Solved: http://stackoverflow.com/a/34367982/404201

xolvo commented 8 years ago

Hello, everyone!

This issue is still true even for Spring Boot 1.3.2

When DispatcherServlet#setThrowExceptionIfNoHandlerFound set to TRUE framework MUST throw exception but it doesn't. Instead it routes errors to BasicErrorController where exceptions are lost. On the other hand, @EnableWebMvc fixes it all but Boot's auto configuration is lost in this case, which is undesirable anyway.

snicoll commented 8 years ago

@xolvo this issue is still open...

wilkinsona commented 8 years ago

@xolvo What have you done in addition to setting throwExceptionIfNoHandlerFound to true? Have you following the advice above on using a ResponseEntityExceptionHandler, for example? That approach is described in the documentation now.

Given the changes in #3998, #3999, and #4000 I think there's a good chance that this issue can now be closed.

xolvo commented 8 years ago

@wilkinsona I tried every possible solution from this issue or stackoverflow. I even created Spring Boot project from scratch with only web component and try ResponseEntityExceptionHandler with @ControllerAdvice on it but it's not working. Of course I also added spring.mvc.throw-exception-if-no-handler-found=true to application.properties

wilkinsona commented 8 years ago

@xolvo If you are using Spring Boot's default configuration for handling static resources then the resource handler will be handling the request (it's ordered last and mapped to /** which means that it picks up any requests that haven't been handled by any other handler in the application) so the dispatcher servlet doesn't get a chance to throw an exception.

You may want to consider disabling the static resource handling entirely if you're building a RESTful API:

spring.resources.add-mappings=false
xolvo commented 8 years ago

Awesome! With this option disabled another option spring.mvc.throw-exception-if-no-handler-found=true works as expected now. Thanks.

wilkinsona commented 8 years ago

@xolvo I'm pleased to hear it's working as expected. Thanks for letting us know.

checklist commented 8 years ago

Joining a little late... but I do need to use static resource mapping and still need the exception to be thrown in case there was NO static resource. My project is web and not REST.

Thanks!

wilkinsona commented 8 years ago

@checklist This issue is closed, and we believe the problem's been solved. If you're looking for some guidance please ask on Stack Overflow. If you believe you've found a bug please open a new issue with a detailed description and, ideally, a sample that reproduces the problem.

bclozel commented 7 years ago

See #7653