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

Improve Javadoc on configuring exception resolvers via WebMvcConfigurer [SPR-15324] #19887

Closed
spring-issuemaster opened this issue Mar 7, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Mar 7, 2017

Belozorov Volodymyr opened SPR-15324 and commented

Good time of the day, dear Spring team,

After extending WebMvcConfigurerAdapter and configuring HttpMessageConverters as well as HandlerExceptionResolvers, I have the following (relevant) configuration for a servlet context

@Configuration
@EnableWebMvc
@ComponentScan({"ua.belozorov.xxx"})
public class WebConfig extends WebMvcConfigurerAdapter {
...
    @Override
    public void configureMessageConverters(List<HttpMessageConverter<?>> converters) {
        converters.add(new MappingJackson2HttpMessageConverter(objectMapper()));
    }

...
    @Override
    public void configureHandlerExceptionResolvers(List<HandlerExceptionResolver> exceptionResolvers) {
        exceptionResolvers.add(exceptionHandlerExceptionResolver());
        exceptionResolvers.add(appHandlerExceptionResolver());
    }

    @Bean
    public ExceptionHandlerExceptionResolver exceptionHandlerExceptionResolver() {
        ExceptionHandlerExceptionResolver resolver = new ExceptionHandlerExceptionResolver();
        resolver.setOrder(1);
        return resolver;
    }

    @Bean
    public AppHandlerExceptionResolver appHandlerExceptionResolver() {
        AppHandlerExceptionResolver resolver = new AppHandlerExceptionResolver(objectMapper());
        resolver.setOrder(2);
        return resolver;
    }

As a result of such configuration, ExceptionHandlerExceptionResolver is initialized with a default list of HttpMessageConverters (in its constructor) which doesn't get overriden later with a configured list of HttpMessageConverters.

As I understood, this is due to the following code:

...
@Bean
	public HandlerExceptionResolver handlerExceptionResolver() {
		List<HandlerExceptionResolver> exceptionResolvers = new ArrayList<HandlerExceptionResolver>();
		configureHandlerExceptionResolvers(exceptionResolvers);
		if (exceptionResolvers.isEmpty()) {
			addDefaultHandlerExceptionResolvers(exceptionResolvers);
		}

...
	protected final void addDefaultHandlerExceptionResolvers(List<HandlerExceptionResolver> exceptionResolvers) {
		...
		exceptionHandlerResolver.setMessageConverters(getMessageConverters());

That is, only if one does not provide his/her own list of HttpMessageConverters (first method), the created ExceptionHandlerExceptionResolver is configured with the supplied HttpMessageConverters (second method)

This behavior seems a little counterintuitive to me.
Is it possible to fix by moving the converters initialization to the ExceptionHandlerExceptionResolver#afterPropertiesSet() method?


Affects: 4.3.4

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2017

Rossen Stoyanchev commented

It's basically the application providing the exception resolvers (fully initialized) or Spring MVC (with a default set of converters). Any alternative would require explicit indication if you want the default converters or not.

What are you actually trying to achieve? If you simply want to register a custom resolver such as your AppHandlerExceptionResolver you can do that via extendHandlerExceptionResolvers in addition to the default ones either before or after them. Or is there a reason for registering your own ExceptionHandlerExceptionResolver instance?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2017

Belozorov Volodymyr commented

In my particular case I want all exceptions to be handled by @ControllerAdvice bean. But, as far as I am aware, ExceptionHandlerExceptionResolver does not handle exceptions that are thrown outside of my application (when a handler method is null). For example, HttpMessageNotReadableException. For that case I created AppHandlerExceptionResolver which extends DefaultHandlerExceptionResolver and overrides some of its exception handling methods.

Your suggestions certainly allows me to achieve what I want. Using #configureHandlerExceptionResolvers has its roots in the simple fact that a couple days ago I didn't understand all the intricacies of Spring MVC exception handling, so I made a list of HandlerExceptionResolvers and its order deterministic by configuring them myself :)

But I think what I want to achieve is irrelevant to this case. As I see the issue:

  1. #configureMessageConverters provides an application-wide final list of message converters
  2. so my natural expectation is that every component which relies on message converters will have this exact list at its disposal
    • unless I explicitly configured a certain component with its own list

I just discovered this behavior while doing my things and in my personal opinion it might be not what one generally expects.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2017

Rossen Stoyanchev commented

Okay I think we can probably improve the Javadoc a bit to clarify and cross-reference these related methods.

Also in case you didn't come across it, consider ResponseEntityExceptionHandler as a base class vs DefaultHandlerExceptionResolver especially if you want a response body with error details.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2017

Rossen Stoyanchev commented

I've updated the title from "ExceptionHandlerExceptionResolver is not configured with HttpMessageConverters supplied by overriden WebMvcConfigurerAdapter#configureMessageConverters()".

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2017

Belozorov Volodymyr commented

Ok, thank you for your tip, I'll keep that in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.