Commons Jelly
  1. Commons Jelly
  2. JELLY-116

[PATCH] InvokeTag and InvokeStaticTag should export exception

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0-beta-4
    • Fix Version/s: 1.0-beta-4
    • Component/s: None
    • Labels:
      None

      Description

      If the method invoked throws a InvocationTargetException, invoke and invokeStatic should allow the caller to obtain the original exception.

      So, I'm providing a patch (including test cases) that export that exception if the exceptionVar paramater is set (and in this case, it does not throw a JellyTagException). If that parameter is not set, it assumes the old behaviour (i.e., throws a JellyTagException).

      1. JELLY-116.patch
        10 kB
        Felipe Leme
      2. JELLY-116-missing.patch
        3 kB
        Felipe Leme
      3. JELLY-116-after-closed.patch
        4 kB
        Felipe Leme

        Activity

        Hide
        Felipe Leme added a comment -

        Here is the patch and a snippet of the maven test:

        [junit] Running org.apache.commons.jelly.core.TestInvokeStaticTag
        [junit] Tests run: 5, Failures: 0, Errors: 0, Time elapsed: 3.896 sec
        [junit] Running org.apache.commons.jelly.core.TestInvokeTag
        [junit] Tests run: 6, Failures: 0, Errors: 0, Time elapsed: 3.146 sec

        Note that I'm not sure if the tags.html has been updated correctly - I couldn't run maven site:generate

        Show
        Felipe Leme added a comment - Here is the patch and a snippet of the maven test: [junit] Running org.apache.commons.jelly.core.TestInvokeStaticTag [junit] Tests run: 5, Failures: 0, Errors: 0, Time elapsed: 3.896 sec [junit] Running org.apache.commons.jelly.core.TestInvokeTag [junit] Tests run: 6, Failures: 0, Errors: 0, Time elapsed: 3.146 sec Note that I'm not sure if the tags.html has been updated correctly - I couldn't run maven site:generate
        Hide
        dion gillard added a comment -

        Test cases fail after applying the patch.

        I'll investigate

        Show
        dion gillard added a comment - Test cases fail after applying the patch. I'll investigate
        Hide
        dion gillard added a comment -

        The patch was missing changes to testInvokeStaticTag.jelly and others

        Show
        dion gillard added a comment - The patch was missing changes to testInvokeStaticTag.jelly and others
        Hide
        Felipe Leme added a comment -

        Hmm, let me create a new patch then...

        Show
        Felipe Leme added a comment - Hmm, let me create a new patch then...
        Hide
        Felipe Leme added a comment -

        Here is the new patch.

        Sorry for the missing files - I didn't do a recursive patch because of JELLY-115 (i.e., my cvs repository has 2 issues patched)

        Show
        Felipe Leme added a comment - Here is the new patch. Sorry for the missing files - I didn't do a recursive patch because of JELLY-115 (i.e., my cvs repository has 2 issues patched)
        Hide
        dion gillard added a comment -

        Will wait for the new patch before trying again.

        Also, when you throw the JellyException, can you please use the constructor that takes a string and a Throwable so the original exception details are preserved?

        Thanks.

        Show
        dion gillard added a comment - Will wait for the new patch before trying again. Also, when you throw the JellyException, can you please use the constructor that takes a string and a Throwable so the original exception details are preserved? Thanks.
        Hide
        dion gillard added a comment -

        Fixed with minor changes

        Show
        dion gillard added a comment - Fixed with minor changes
        Hide
        Felipe Leme added a comment -

        Here is the change you've requested - preserving the original exception.

        Show
        Felipe Leme added a comment - Here is the change you've requested - preserving the original exception.
        Hide
        dion gillard added a comment -

        This patch appears to be a JDK 1.4 specific one.

        It uses Throwable.getCause() which was introduced in jdk 1.4

        I'm commenting those lines out for now.

        Show
        dion gillard added a comment - This patch appears to be a JDK 1.4 specific one. It uses Throwable.getCause() which was introduced in jdk 1.4 I'm commenting those lines out for now.
        Hide
        Felipe Leme added a comment -

        Sorry, my bad, I used the wrong casts.

        Yes, Throwable.getCause() is JDK 1.4, but I'm using InvocationTarget.getTargetMessage() instead:

        http://java.sun.com/j2se/1.3/docs/api/java/lang/reflect/InvocationTargetException.html

        I mean, there is also a call to jellyException.getCause(), but JellyException defines it indenpendently on the JDK version.

        Anyway, I've just ran maven on it using JDK 1.3.1, and it worked fine. It's just a matter of modifying the test cases, changing the following line:

        from:
        Exception jellyException = (Exception) getJellyContext().getVariable("jellyException");

        to:
        JellyException jellyException = (JellyException) getJellyContext().getVariable("jellyException");

        And also adding a

        import org.apache.commons.jelly.JellyException;

        (Sorry, I'm not with CVS set up right now to send you a patch)

        Show
        Felipe Leme added a comment - Sorry, my bad, I used the wrong casts. Yes, Throwable.getCause() is JDK 1.4, but I'm using InvocationTarget.getTargetMessage() instead: http://java.sun.com/j2se/1.3/docs/api/java/lang/reflect/InvocationTargetException.html I mean, there is also a call to jellyException.getCause(), but JellyException defines it indenpendently on the JDK version. Anyway, I've just ran maven on it using JDK 1.3.1, and it worked fine. It's just a matter of modifying the test cases, changing the following line: from: Exception jellyException = (Exception) getJellyContext().getVariable("jellyException"); to: JellyException jellyException = (JellyException) getJellyContext().getVariable("jellyException"); And also adding a import org.apache.commons.jelly.JellyException; (Sorry, I'm not with CVS set up right now to send you a patch)
        Hide
        dion gillard added a comment -

        Fixing now

        Show
        dion gillard added a comment - Fixing now

          People

          • Assignee:
            dion gillard
            Reporter:
            Felipe Leme
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development