@ExceptionHandler should match cause as well (e.g. for exception thrown from argument formatter) [SPR-14291]

[spring-projects-issues]

Juan Carlos García del Canto opened SPR-14291 and commented

The following problem is related with this StackOverFlow question.

I have a controller that contains a show method to display info about the provided entity.

@Controller
@RequestMapping("/owners")
public class OwnersController {
   
   @RequestMapping(value = "/{owner}", method = RequestMethod.GET,
    	      produces = MediaType.TEXT_HTML_VALUE)
    public String show(@PathVariable Owner owner, Model model) {
        // Return view
        return "owners/show";
    }
}

To invoke this operation I use http://localhost:8080/owners/1 URL. As you could see, I provide Owner identifier 1.

To be able to convert identifier 1 to a valid Owner element is necessary to define an Spring Formatter and register it on addFormatters method from WebMvcConfigurerAdapter.

I have the following OwnerFormatter:

public class OwnerFormatter implements Formatter<Owner> {
  private final OwnerService ownerService;
  private final ConversionService conversionService;
    public OwnerFormatter(OwnerService ownerService,
      ConversionService conversionService) {
    this.ownerService = ownerService;
    this.conversionService = conversionService;
  }
  @Override
  public Owner parse(String text, Locale locale) throws ParseException {
    if (text == null || !StringUtils.hasText(text)) {
      return null;
    }
    Long id = conversionService.convert(text, Long.class);
    Owner owner = ownerService.findOne(id);
    if(owner == null){
        throw new EntityResultNotFoundException();
    }
    return owner;
  }
  @Override
  public String print(Owner owner, Locale locale) {
    return owner == null ? null : owner.getName();
  }
}

As you could see, I've use findOne method to obtain Owner with id 1. But, what happens if this method returns null because there aren't any Owner with id 1?

To prevent this, I throw a custom Exception called EntityResultNotFoundException. This exception contains the following code:

public class EntityResultNotFoundException extends RuntimeException {
         public EntityResultNotFoundException() {
                  super("ERROR: Entity not found");
         }
}

I want to configure project to be able to return errors/404.html when this exception throws, so, following Spring documentation, I have two options:

Option a)

Read StackOverFlow question, this is not related with this issue.

Option b)

Include a new @Bean SimpleMappingExceptionResolver inside a @Configuration class to map exceptions with the view identifier.

@Configuration
public class WebMvcConfiguration extends WebMvcConfigurerAdapter {
  [...]
  @Bean
  public SimpleMappingExceptionResolver simpleMappingExceptionResolver() {
      SimpleMappingExceptionResolver resolver =
            new SimpleMappingExceptionResolver();
      Properties mappings = new Properties();
      mappings.setProperty("EntityResultNotFoundException", "errores/404");
      resolver.setExceptionMappings(mappings);
      return resolver;
  }
}

However, the implementation above doesn't works with exceptions throwed on Spring Formatters.

I've been debugging Spring code and I've found two things that maybe could be an interesting improvement to Spring Framework.

First of all, DispatcherServlet is loading all registered HandlerExceptionResolver from initStrategies and initHandlerExceptionResolvers methods. This method is taken all HandlerExceptionResolvers in the correct order, but then, uses the following code to order them again:

AnnotationAwareOrderComparator.sort(this.handlerExceptionResolvers);

The problem is that this method delegates on the findOrder method that tries to obtain order from the HandlerExceptionResolver that is instance of Ordered. As you could see, I've not defined order on my registered @Bean, so when tries to obtain order from my declared bean SimpleMappingExceptionResolver is using LOWEST_PRECEDENCE. This causes that Spring uses DefaultHandlerExceptionResolver because is the first one that returns a result.

So, to solve this I've added order value to my declared bean with the following code.

@Bean
public SimpleMappingExceptionResolver simpleMappingExceptionResolver() {
    SimpleMappingExceptionResolver resolver =
          new SimpleMappingExceptionResolver();
    Properties mappings = new Properties();
    mappings.setProperty("EntityResultNotFoundException", "errores/404");
    resolver.setOrder(-1);
    resolver.setExceptionMappings(mappings);
    return resolver;
}  

Now, when AnnotationAwareOrderComparator sorts all registered HandlerExceptionResolver the _SimpleMappingExceptionResolver_is the first one and it will be used as resolver.

Anyway, is not working yet. I've continue debugging and I've saw that now is using doResolveException from SimpleMappingExceptionResolver to resolve the exception, so it's ok. However, the method findMatchingViewName that tries to obtain the mapped view returns null.

The problem is that findMatchingViewName is trying to check if the received exception match with some exception defined on the exceptionMappings of the SimpleMappingExceptionResolver, but it's only checking the super classes inside getDepth method. Should be necessary to check the cause exception.

I've applied the following workaround to continue working (just extend SimpleMappingExceptionResolver and implements findMatchingViewName method to try to find matching view again with cause exception if depth is not valid)

public class CauseAdviceSimpleMappingExceptionResolver extends SimpleMappingExceptionResolver{
         /**
          * Find a matching view name in the given exception mappings.
          * @param exceptionMappings mappings between exception class names and error view names
          * @param ex the exception that got thrown during handler execution
          * @return the view name, or {@code null} if none found
          * @see #setExceptionMappings
          */
         @Override
         protected String findMatchingViewName(Properties exceptionMappings, Exception ex) {
                  String viewName = null;
                  String dominantMapping = null;
                  int deepest = Integer.MAX_VALUE;
                  for (Enumeration<?> names = exceptionMappings.propertyNames(); names.hasMoreElements();) {
                           String exceptionMapping = (String) names.nextElement();
                           int depth = getDepth(exceptionMapping, ex);
                           if (depth >= 0 && (depth < deepest || (depth == deepest &&
                                                 dominantMapping != null && exceptionMapping.length() > dominantMapping.length()))) {
                                    deepest = depth;
                                    dominantMapping = exceptionMapping;
                                    viewName = exceptionMappings.getProperty(exceptionMapping);
                           }else if(ex.getCause() instanceof Exception){
                                    return findMatchingViewName(exceptionMappings, (Exception) ex.getCause() );
                           }
                  }
                  if (viewName != null && logger.isDebugEnabled()) {
                           logger.debug("Resolving to view '" + viewName + "' for exception of type [" + ex.getClass().getName() +
                                                 "], based on exception mapping [" + dominantMapping + "]");
                  }
                  return viewName;
         }
         }

I think that this implementation is really interesting because also use the cause exception class instead of use only the superclasses exceptions.

I'm going to create a new Pull-Request on Spring Framework github including this improvement.

With these 2 changes (order and extending SimpleMappingExceptionResolver) I'm able to catch an exception thrown from Spring Formatter and return a custom view.

What do you think about it?

---

Affects: 4.2.5

Reference URL: http://stackoverflow.com/questions/37322182/exceptionhandler-doesnt-catch-exceptions-being-thrown-from-spring-formatters

Issue Links:

• Allow SimpleMappingExceptionResolver to navigate exception "cause" hierarchy [SPR-5053] #9728 Allow SimpleMappingExceptionResolver to navigate exception "cause" hierarchy
• @ExceptionHandler cannot handler java.lang.Error despite the annotation accept ? extends Throwable [SPR-11106] #15732 @ExceptionHandler cannot handler java.lang.Error despite the annotation accept ? extends Throwable
• Exception swallowed in ResponseEntityExceptionHandler [SPR-16743] #21284 Exception swallowed in ResponseEntityExceptionHandler
• AsyncRestTemplate changes in SPR-13413 made its harder to detect Client/Server errors [SPR-14353] #18925 AsyncRestTemplate changes in SPR-13413 made its harder to detect Client/Server errors
• @MessageExceptionHandler should match cause as well (analogous to @ExceptionHandler) [SPR-14424] #18995 @MessageExceptionHandler should match cause as well (analogous to @ExceptionHandler)

Referenced from: commits 981c894

1 votes, 4 watchers

[spring-projects-issues]

Juan Carlos García del Canto commented

I've just sent a pull-request that fixes the problem:

#1062

[spring-projects-issues]

Rossen Stoyanchev commented

Such a change in SimpleMappingExceptionResolver was discussed previously (see #9728). It is easy to do but there are some complications with potentially confusing behavior, as a result of which it wasn't done.

However back to you options above and the first option that you abandoned. The answer on SO is completely inaccurate. An @ControllerAdvice for once is perfectly capable of dealing with any controller-related exceptions. This is what it was created for. It is also wrong in stating that AOP is involved, simply not the case. I've created a repro project where use of @ControllerAdvice works just fine.

I'm not sure why it didn't work for you, perhaps the @ControllerAdvice wasn't picked up by a component-scan declaration and thus wasn't a bean? In any case it should work.

[spring-projects-issues]

Juan Carlos García del Canto commented

Hi Rossen, thanks for your comment. I've just tried your example on github and it's working because you are checking exception using getRootCause() method. However, @ExceptionHandler is not catching the EntityResultNotFoundException exception:

@ControllerAdvice
public class ExceptionControllerAdvice {

	// THIS IS NOT WORKING. Seems like @ControllerAdvice is not
	// checking root cause exception
	/*@ExceptionHandler(EntityResultNotFoundException.class)
	public String handle(MethodArgumentTypeMismatchException ex) {
		return "errors/404";
	}*/
	
	// We should check it manually using getRootCause() method.
	@ExceptionHandler
	public String handle(MethodArgumentTypeMismatchException ex) {
		if(ex.getRootCause() instanceof EntityResultNotFoundException){
			return "errors/404";
		}
		return "errors/500";
	}
	
}

This could be another related issue or improvement? See full example here

On the other hand, about SimpleMappingExceptionResolver improvement... will be necessary that I apply my workaround explained above everytime I want to throw a custom exception from Spring Formatters?

I've uploaded some samples to gitHub repository about these related errors.

[spring-projects-issues]

Juergen Hoeller commented

I'm repurposing this issue for @ExceptionHandler, as per the comments. Following up on #15732 which deals with the handling of nested Error causes, this is actually quite straightforward to add to our algorithm there.