spring-projects / spring-boot

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

AbstractErrorWebExceptionHandler is unfriendly to extend/use #12414

Closed Clockwork-Muse closed 6 years ago

Clockwork-Muse commented 6 years ago

The spring feature documentation has the following example for extending AbstractErrorWebExceptionHandler:

To change the error handling behavior, you can implement ErrorWebExceptionHandler and register a bean definition of that type. Because a WebExceptionHandler is quite low-level, Spring Boot also provides a convenient AbstractErrorWebExceptionHandler to let you handle errors in a WebFlux functional way, as shown in the following example:


public class CustomErrorWebExceptionHandler extends AbstractErrorWebExceptionHandler {
// Define constructor here

@Override
protected RouterFunction<ServerResponse> getRoutingFunction(ErrorAttributes errorAttributes) {

    return RouterFunctions
            .route(aPredicate, aHandler)
            .andRoute(anotherPredicate, anotherHandler);
}

}


... see that `// Define constructor here`?  Yeah.  It has to _start_ like this (all properties are required):
```Java
public ExtendingClass(ErrorAttributes errorAttributes, ResourceProperties resourceProperties, ApplicationContext applicationContext) {
    super(errorAttributes, resourceProperties, applicationContext);
}

Annoyingly, ErrorAttributes is for some reason not provided when using @WebFluxTest, meaning you have to mock it.

...and when those beans are provided, there's a hidden dependency - it has an afterPropertiesSet override, which is looking for a List<HttpMessageWriter<?>>, which is normally provided via ServerCodecConfigurer.


Can we at least get this documented better? The constructor, at minimum, needs to look like this:

public ExtendingClass(ErrorAttributes errorAttributes, ResourceProperties resourceProperties,
         ApplicationContext applicationContext, ServerCodecConfigurer configurer) {
    super(errorAttributes, resourceProperties, applicationContext);
    this.setMessageWriters(codec.getWriters());
}

... but I'd really prefer a friendlier way to instantiate this thing. And should ErrorAttributes be added as a dependency for @WebFluxTest?

bclozel commented 6 years ago

Putting aside the improvements on the ErrorWebExceptionHandler (I'm taking a look at this).

I don't understand why you're trying to test the error handling support in a @WebFluxTest; this is designed for integration tests on your application web layer, mocking the actual HTTP layer and not starting a web server.

For example, you can't hit error handling pages when using @WebMvcTest nor @WebFluxTest. So we're quite consistent here. This is why the ErrorAttributes bean is not available in that test slice.

Now could you tell me a bit more about the use case you're trying to solve by implementing your own error handler? I've been thinking about possible improvements here but I'd like to know more about the motivations to create your own.

Thanks!

Clockwork-Muse commented 6 years ago

The error handler is because we have a specific error-message JSON object we're supposed to use for RPC/API returns (not RFC7807 unfortunately). If there's a faster/better way, I'd take that instead.

Currently I'm using a wired component in each of my handler functions, which I'm guessing is going to be the recommended way, I was just hoping to centralize the error messaging. Something like this:

public Mono<ServerResponse> someFunction(final ServerRequest request) {
    return request.bodyToMono(Pojo.class)
            .flatMap(found -> {
                URI location = URI.create("/" + "location");
                return ServerResponse.created(location).body(fromObject(found));
            })
            // I'd like to remove this line, since it's everywhere
            .onErrorResume(wiredErrorHandler::handle);
}

...which also plays nice with testing via WebTestClient. Which is the only way I have to test this, since there's no way to get the body from ServerResponse that I've found (so, I can't use Reactor's StepVerifier).

bclozel commented 6 years ago

Have you considered exposing your own ErrorAttributes bean to replace the error content without overriding the whole error handling feature?

Clockwork-Muse commented 6 years ago

Hrm, that is a possibility, I'll have to play with it to make sure it plays nicely with the custom object I need to return.

bclozel commented 6 years ago

One more note about testing .onErrorResume(wiredErrorHandler::handle);.

There are several levels of error handling:

  1. local to the controller handler
  2. spanning multiple controllers with @ControllerAdvice
  3. the Spring Boot error handling

1) and 2) belong to your web application code, so it should be possible to test those through @WebFluxTest. 3) on the other hand is closer to web framework infrastructure, so you should test it independently of your web application endpoints. Pretty much like we're testing our own error handling support in Boot.

Clockwork-Muse commented 6 years ago

Yeah, essentially what I'd like is some equivalent of @ControllerAdvice for functional endpoints, to try and eliminate some boilerplate from my functions. Given the functional nature of Mono et al, though, maybe I should just stick with manually mapping it (what I really would have preferred is to be able to return an arbitrary object as an error, not just an exception - or rather, Mono.error(ServerResponse errorResponse), really).

bclozel commented 6 years ago

Well, you can't really have both; functional endpoints are about having full control+being more explicit over the routing and the response. That being said, a HandlerFilterFunction is somehow the @ControllerAdvice equivalent in WebFlux.fn (see reference doc).

So a HandlerFilterFunction, or overriding the ErrorAttributes bean should help you here. I don't think redefining WebExceptionHandler is wise at that point.

Shilpathakur02 commented 5 years ago

Why can't the logError method of AbstractErrorWebExceptionHandler is overridable ? I just want to given some more information in logs.