Velocity
  1. Velocity
  2. VELOCITY-284

MethodInvocationException is handled inconsistently

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4, 1.5
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: Other

      Description

      It appears that the exception handling is different depending on whether the
      method is invoked as a passed parameter to a velocimacro or just in a template.

      Ex.

      #doTextLink( $

      {myObj.exception})

      ${myObj.exception}

      The first #doTextLink macro will throw an exception, but it will be logged and
      ignored. However, the second invocation will throw the exception properly.

      I have a patch that should fix this behavior. The exception just needs to be
      rethrown up the call stack. A small signature change to a couple of methods.

      Thanks,

      Mike Rettig

        Activity

        Hide
        Henning Schmiedehausen added a comment -

        Close all resolved issues for Engine 1.5 release.

        Show
        Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.
        Hide
        Will Glass-Husain added a comment -

        rsolved again. r473363

        Show
        Will Glass-Husain added a comment - rsolved again. r473363
        Hide
        Henning Schmiedehausen added a comment -

        As we already did the scary thing (rebasing VelocityException), we might as well look into catching the exception. Will, can you take a look at that?

        Show
        Henning Schmiedehausen added a comment - As we already did the scary thing (rebasing VelocityException), we might as well look into catching the exception. Will, can you take a look at that?
        Hide
        Nathan Bubna added a comment -

        That's a good question. I can't think of any likely problems offhand, but it does give me a sense of foreboding.

        Show
        Nathan Bubna added a comment - That's a good question. I can't think of any likely problems offhand, but it does give me a sense of foreboding.
        Hide
        Will Glass-Husain added a comment -

        Question

        – are there any good backwards-compatibility arguments against making VelocityException based on a RuntimeException?

        Show
        Will Glass-Husain added a comment - Question – are there any good backwards-compatibility arguments against making VelocityException based on a RuntimeException?
        Hide
        Henning Schmiedehausen added a comment -

        There are a number of options on this for 1.6:

        • We could do the scary thing. Rebase VelocityException from Exception to RuntimeException. Then catch MethodInvocationException explicitly on
          top of the chain.
        • We could do the less scary thing of creating VelocityRuntimeException similar to VelocityException and rebase just MethodInvocationException.
        • We could create a new RuntimeMethodInvocationException (don't).

        Mike: Do you have a unit test handy for that change?

        Show
        Henning Schmiedehausen added a comment - There are a number of options on this for 1.6: We could do the scary thing. Rebase VelocityException from Exception to RuntimeException. Then catch MethodInvocationException explicitly on top of the chain. We could do the less scary thing of creating VelocityRuntimeException similar to VelocityException and rebase just MethodInvocationException. We could create a new RuntimeMethodInvocationException (don't). Mike: Do you have a unit test handy for that change?
        Hide
        Will Glass-Husain added a comment -

        Reversed patch. Revision #151591.

        Show
        Will Glass-Husain added a comment - Reversed patch. Revision #151591.
        Hide
        Will Glass-Husain added a comment -

        Reopened the bug. Can't apply this patch in current version - it changes the
        Context interface which breaks compatibility. Reversed the patch.

        We can leave this open as a reminder to consider for v2.0. Or maybe there's
        an alternative way of handling this.

        Show
        Will Glass-Husain added a comment - Reopened the bug. Can't apply this patch in current version - it changes the Context interface which breaks compatibility. Reversed the patch. We can leave this open as a reminder to consider for v2.0. Or maybe there's an alternative way of handling this.
        Hide
        Will Glass-Husain added a comment -

        Thanks for the fix. Patch applied. Revision #151478

        Show
        Will Glass-Husain added a comment - Thanks for the fix. Patch applied. Revision #151478
        Hide
        Mike Rettig added a comment -

        Sorry, I don't have time to create a new diff. However, the code change is not
        difficult. The only change is to rethrow an exception instead of swallowing it.

        Here is where to start. Change "return null" to "throw mie". Then change the
        method signatures to add the throws clause.

        RCS file:
        /home/cvspublic/jakarta-velocity/src/java/org/apache/velocity/runtime/directive/VMProxyArg.java,v
        retrieving revision 1.17
        diff -r1.17 VMProxyArg.java
        242,243c242
        < public Object getObject( InternalContextAdapter context )
        < {

        > public Object getObject( InternalContextAdapter context ) throws
        MethodInvocationException {
        343,344c342
        <
        < return null;

        > throw mie;

        Show
        Mike Rettig added a comment - Sorry, I don't have time to create a new diff. However, the code change is not difficult. The only change is to rethrow an exception instead of swallowing it. Here is where to start. Change "return null" to "throw mie". Then change the method signatures to add the throws clause. RCS file: /home/cvspublic/jakarta-velocity/src/java/org/apache/velocity/runtime/directive/VMProxyArg.java,v retrieving revision 1.17 diff -r1.17 VMProxyArg.java 242,243c242 < public Object getObject( InternalContextAdapter context ) < { — > public Object getObject( InternalContextAdapter context ) throws MethodInvocationException { 343,344c342 < < return null; — > throw mie;
        Hide
        Will Glass-Husain added a comment -

        Mike,

        Thanks for posting this. Looks like a useful patch. Would you have time to
        redo this patch using unified format (-u option)? If you do that I'll commit
        this.

        You might find this handy.
        http://wiki.apache.org/jakarta-velocity/GettingYourPatchCommitted

        Thanks.

        Show
        Will Glass-Husain added a comment - Mike, Thanks for posting this. Looks like a useful patch. Would you have time to redo this patch using unified format (-u option)? If you do that I'll commit this. You might find this handy. http://wiki.apache.org/jakarta-velocity/GettingYourPatchCommitted Thanks.
        Hide
        Shinobu Kawai added a comment -
            • Bug 30219 has been marked as a duplicate of this bug. ***
        Show
        Shinobu Kawai added a comment - Bug 30219 has been marked as a duplicate of this bug. ***
        Hide
        Mike Rettig added a comment -

        Created an attachment (id=12177)
        Patch file created against cvs HEAD to rethrow exception.

        Show
        Mike Rettig added a comment - Created an attachment (id=12177) Patch file created against cvs HEAD to rethrow exception.

          People

          • Assignee:
            Will Glass-Husain
            Reporter:
            Mike Rettig
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development