Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-9193

ExceptionHandlerExceptionResolver: premature null value check

    Details

    • Type: Refactoring
    • Status: Closed
    • Priority: Major
    • Resolution: Complete
    • Affects Version/s: 3.1 GA
    • Fix Version/s: 3.2 M1
    • Component/s: Web
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      This is doResolveHandlerMethodException method from ExceptionHandlerExceptionResolver class:

      @Override
      protected ModelAndView doResolveHandlerMethodException(HttpServletRequest request, 
                                                             HttpServletResponse response,
                                                             HandlerMethod handlerMethod, 
                                                             Exception exception) {
          if (handlerMethod == null) {
              return null;
          }
      	
          ServletInvocableHandlerMethod exceptionHandlerMethod = getExceptionHandlerMethod(handlerMethod, exception);
          if (exceptionHandlerMethod == null) {
              return null;
          }
       
          exceptionHandlerMethod.setHandlerMethodArgumentResolvers(this.argumentResolvers);
          exceptionHandlerMethod.setHandlerMethodReturnValueHandlers(this.returnValueHandlers);
       
          ServletWebRequest webRequest = new ServletWebRequest(request, response);
          ModelAndViewContainer mavContainer = new ModelAndViewContainer();
       
          try {
              if (logger.isDebugEnabled()) {
                  logger.debug("Invoking @ExceptionHandler method: " + exceptionHandlerMethod);
              }
              exceptionHandlerMethod.invokeAndHandle(webRequest, mavContainer, exception);
          }
          catch (Exception invocationEx) {
              logger.error("Failed to invoke @ExceptionHandler method: " + exceptionHandlerMethod, invocationEx);
              return null;
          }
      		
          if (mavContainer.isRequestHandled()) {
              return new ModelAndView();
          }
          else {
              ModelAndView mav = new ModelAndView().addAllObjects(mavContainer.getModel());
              mav.setViewName(mavContainer.getViewName());
              if (!mavContainer.isViewReference()) {
                  mav.setView((View) mavContainer.getView());
              }
              return mav;				
          }
      }

      What bothers me is the first if condition:

          if (handlerMethod == null) {
              return null;
          }

      The handlerMethod variable is then used in the protected getExceptionHandlerMethod(handlerMethod, exception) method only so I believe the null check should be inside the getExceptionHandlerMethod method.

      We developed an ExceptionHandlerExceptionResolver extension which just overrides the getExceptionHandlerMethod(handlerMethod, exception. This method works even for null handlerMethod values (which happens when there is no suitable handler method available), unfortunatelly the processing flow never reaches it because of the 'premature' null check. Moving the null check to the more appropriate place would help us a lot.

        Issue Links

          Activity

          Hide
          rstoya05-aop Rossen Stoyanchev added a comment -

          Good point.

          Show
          rstoya05-aop Rossen Stoyanchev added a comment - Good point.

            People

            • Assignee:
              rstoya05-aop Rossen Stoyanchev
              Reporter:
              lfty Jan Dudek
              Last updater:
              Trevor Marshall
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                5 years, 37 weeks, 6 days ago