Struts 2
  1. Struts 2
  2. WW-3559

PrepareInterceptor is eating exceptions that occur in prepare{methodName} methods

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.2.1.1
    • Fix Version/s: 2.2.3
    • Component/s: Core Interceptors
    • Labels:
      None
    • Environment:

      All

    • Flags:
      Patch, Important

      Description

      The PrepareInterceptor is eating any exceptions that occur when calling prepare

      {MethodName}

      methods.
      It is catching the exceptions and simply logging a warning then continuing.
      It should be re-throwing these exception so that normal exception handling login (i.e. the Exception Interceptor) is performed.
      Exceptions that occur in the prepare() method are (correctly) not caught but those in the prepare{MethodName) methods are being caught and ignored.

      I have a patch that corrects this bug.

      1. WW-3559-test.patch
        2 kB
        Maurizio Cucchiara
      2. PrepareInterceptor_patch.txt
        3 kB
        Greg Lindholm

        Activity

        Hide
        Greg Lindholm added a comment -

        Patch to fix this bug

        Show
        Greg Lindholm added a comment - Patch to fix this bug
        Hide
        Maurizio Cucchiara added a comment -

        I'm sorry but your patch changes the default behavior.
        If for some reason the PrefixMethodInvocationUtil.invokePrefixMethod method raises an InvocationTargetException, the prepare method of called action will no longer be called (code comments: just in case there's an exception while doing reflection, we still want prepare() to be able to get called).

        There is also a lost of informations, at this time every intercepted exception gives a self explanatory warning.

        Show
        Maurizio Cucchiara added a comment - I'm sorry but your patch changes the default behavior. If for some reason the PrefixMethodInvocationUtil.invokePrefixMethod method raises an InvocationTargetException, the prepare method of called action will no longer be called (code comments: just in case there's an exception while doing reflection, we still want prepare() to be able to get called). There is also a lost of informations, at this time every intercepted exception gives a self explanatory warning.
        Hide
        Maurizio Cucchiara added a comment -

        Furthermore, AFAIK, java raises this exceptions when the prepareXXX or prepareDoXXX method are:
        1. inaccessible according to access control (IllegalAccessException)
        2. if the number of actual and formal parameters differ; (IllegalArgumentException)
        3. NullPointerException if the action target is null
        ... and so on

        Each of them makes me think of a configuration error in (the way the user declares prepare[Do]XXX method).

        Show
        Maurizio Cucchiara added a comment - Furthermore, AFAIK, java raises this exceptions when the prepareXXX or prepareDoXXX method are: 1. inaccessible according to access control (IllegalAccessException) 2. if the number of actual and formal parameters differ; (IllegalArgumentException) 3. NullPointerException if the action target is null ... and so on Each of them makes me think of a configuration error in (the way the user declares prepare [Do] XXX method).
        Hide
        Greg Lindholm added a comment -

        Let me provide a little more information and see if the helps with a sample use condition:

        public class MyAction implements Preparable {

        public String doUpdate() {
        ...
        if (/* bad condition */)

        { throw new Exception1(); }

        ...
        }

        public void prepareDoUpdate() {
        if ( /* bad condition */)

        { throw new Exception2(); }

        ...
        }

        public void prepare() {
        if ( /* bad condition */)

        { throw new Exception3(); }

        ...
        }

        /* other doXXX() action methods not shown */
        }

        Given the above code sample Exception1 and Exception3 that occur in the action method and in the prepare() method will not be caught and will travel up the stack until the Exception interceptor which will handle them. However, Exception2 thrown from prepareDoUpdate() will be caught by the current PrepareInterceptor and ignored. This is incorrect behavior, all exception in user code should follow the same logic and go up the stack so they can be handled by the Exception interceptor.

        Exceptions that get throw in reflection invoked methods get wrapped in InvocationTargetException which the current PrepareInterceptor is catching and ignoring. The patch I submitted would catch the InvocationTargetException and unwrap Exception2 and re-throw it so that it can then be handle like all other exceptions. I have been using this patch in production code for some time now it it works correctly.

        There are now 2 questions:
        1) is there actually a bug in PrepareInterceptor?
        2) does the patch fix the bug (and generally improve the behavior)?

        I hope you can see that the current PrepareInterceptor behavior is incorrect and that there really is a bug here?

        To answer your specific comments:
        Comment #1:

        • If an exception is throw in prepare {MethodName} I don't believe you want to then try to call prepare(), the correct action is to allow normal exception processing logic to proceed. This way the exception will be handle by the Exception interceptor as is the documented behavior.

          - I'm sorry I don't know what you mean by "every intercepted exception gives a self explanatory warning". My experience with this is my exceptions where being caught and ignored.

          Comment#2:
          These exception all relate to a incorrect configuration. Granted they are all possible but if these exceptions are throw they will be handled by the Exception Interceptor. Any configuration errors that are detected they should be logger (with Error level) but you should not be interfering with normal exception processing that is in the user's code.
          Allowing normal exception handling to occur will work for both configuration errors and for exceptions thrown in the users code.

          Currently exception handling works correctly in action and prepare() methods but not in prepare{MethodName}

          methods.

        I believe my patch corrects this problem.

        Show
        Greg Lindholm added a comment - Let me provide a little more information and see if the helps with a sample use condition: public class MyAction implements Preparable { public String doUpdate() { ... if (/* bad condition */) { throw new Exception1(); } ... } public void prepareDoUpdate() { if ( /* bad condition */) { throw new Exception2(); } ... } public void prepare() { if ( /* bad condition */) { throw new Exception3(); } ... } /* other doXXX() action methods not shown */ } Given the above code sample Exception1 and Exception3 that occur in the action method and in the prepare() method will not be caught and will travel up the stack until the Exception interceptor which will handle them. However, Exception2 thrown from prepareDoUpdate() will be caught by the current PrepareInterceptor and ignored. This is incorrect behavior, all exception in user code should follow the same logic and go up the stack so they can be handled by the Exception interceptor. Exceptions that get throw in reflection invoked methods get wrapped in InvocationTargetException which the current PrepareInterceptor is catching and ignoring. The patch I submitted would catch the InvocationTargetException and unwrap Exception2 and re-throw it so that it can then be handle like all other exceptions. I have been using this patch in production code for some time now it it works correctly. There are now 2 questions: 1) is there actually a bug in PrepareInterceptor? 2) does the patch fix the bug (and generally improve the behavior)? I hope you can see that the current PrepareInterceptor behavior is incorrect and that there really is a bug here? To answer your specific comments: Comment #1: If an exception is throw in prepare {MethodName} I don't believe you want to then try to call prepare(), the correct action is to allow normal exception processing logic to proceed. This way the exception will be handle by the Exception interceptor as is the documented behavior. - I'm sorry I don't know what you mean by "every intercepted exception gives a self explanatory warning". My experience with this is my exceptions where being caught and ignored. Comment#2: These exception all relate to a incorrect configuration. Granted they are all possible but if these exceptions are throw they will be handled by the Exception Interceptor. Any configuration errors that are detected they should be logger (with Error level) but you should not be interfering with normal exception processing that is in the user's code. Allowing normal exception handling to occur will work for both configuration errors and for exceptions thrown in the users code. Currently exception handling works correctly in action and prepare() methods but not in prepare{MethodName} methods. I believe my patch corrects this problem.
        Hide
        Maurizio Cucchiara added a comment -

        Sorry If I didn't get what you said.
        I totally agree with you. I'm attaching a patch which contains a test that summarizes the new, right behaviour.
        I'm going to apply both of them, any thoughts?

        Show
        Maurizio Cucchiara added a comment - Sorry If I didn't get what you said. I totally agree with you. I'm attaching a patch which contains a test that summarizes the new, right behaviour. I'm going to apply both of them, any thoughts?
        Hide
        Greg Lindholm added a comment -

        Looks good. Thanks.

        Show
        Greg Lindholm added a comment - Looks good. Thanks.
        Hide
        Maurizio Cucchiara added a comment -

        Patch applied, thanks

        Show
        Maurizio Cucchiara added a comment - Patch applied, thanks

          People

          • Assignee:
            Maurizio Cucchiara
            Reporter:
            Greg Lindholm
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development