Velocity
  1. Velocity
  2. VELOCITY-654

Fix correct template name reporting, enhance error logging information

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.1
    • Fix Version/s: 1.6.1, 1.7
    • Component/s: Engine
    • Labels:
      None

      Description

      Fix template name reporting for #include and #parse if an exception occurs. Error reporting in Velocity tends to use context.getTemplateName() which is intended for scoping information, and does not always provide the template name containing the node or directive that generates an error. This adds a templateName field to the Directive object and assigns it on creation, if a template name is available.

      Also added template and location info when logging errors thrown from #parse. This compliments the pseudo-stack trace that is already logged to error for macros. So, now a complete trace is logged to error of the macro and template layers with template name and location. Yea!

      1. 654_2.patch
        9 kB
        Byron Foster
      2. 654.patch
        9 kB
        Byron Foster
      3. patch.txt
        10 kB
        Nathan Bubna

        Activity

        Hide
        Nathan Bubna added a comment -

        Looks good, but why did you change #parse and #include to catch RuntimeExceptions (and MIEs) and wrap them in VelocityException instead of just passing them through? That's more than just a logging change.

        Show
        Nathan Bubna added a comment - Looks good, but why did you change #parse and #include to catch RuntimeExceptions (and MIEs) and wrap them in VelocityException instead of just passing them through? That's more than just a logging change.
        Hide
        Byron Foster added a comment -

        Yea, you're right, that's more aggressive then my intent, I thought I was being clever, but I wasn't . Patch 2 should right my wrongs.

        I removed the catch(Exception e) in Parse.java because I believe it is unreachable.

        Show
        Byron Foster added a comment - Yea, you're right, that's more aggressive then my intent, I thought I was being clever, but I wasn't . Patch 2 should right my wrongs. I removed the catch(Exception e) in Parse.java because I believe it is unreachable.
        Hide
        Nathan Bubna added a comment -

        Ok, but now it's not catching & wrapping non-checked Exceptions... since your goal appears to be logging alone, i'll just alter your patch to leave everything but logging alone. which will make me feel better about pushing this to the 1.6.x branch, unless you can explain why we should make the other changes.

        Show
        Nathan Bubna added a comment - Ok, but now it's not catching & wrapping non-checked Exceptions... since your goal appears to be logging alone, i'll just alter your patch to leave everything but logging alone. which will make me feel better about pushing this to the 1.6.x branch, unless you can explain why we should make the other changes.
        Hide
        Nathan Bubna added a comment -

        How does this look to you Byron? It only removes the catch(MIE) since MIE is now a RuntimeException, and it logs all caught exceptions, whether wrapped or just re-thrown. I think it should cover things, though i'd like to hear some confirmation from you.

        Show
        Nathan Bubna added a comment - How does this look to you Byron? It only removes the catch(MIE) since MIE is now a RuntimeException, and it logs all caught exceptions, whether wrapped or just re-thrown. I think it should cover things, though i'd like to hear some confirmation from you.
        Hide
        Nathan Bubna added a comment -

        Ah. Well, let's leave the catch(Exception) for now, just in case. So, is there a reason you didn't log an error for all the caught exceptions? Would some be redundant?

        Show
        Nathan Bubna added a comment - Ah. Well, let's leave the catch(Exception) for now, just in case. So, is there a reason you didn't log an error for all the caught exceptions? Would some be redundant?
        Hide
        Byron Foster added a comment -

        Yes, patch.txt looks fine! I'm good with it.

        I believe that 654_2.patch and patch.txt are functionally exactly equivalent. However, you're right for a 1.6.1 addition it is safer to go with changes only consistent with intent of the patch. I can do a small cleanup later.

        Show
        Byron Foster added a comment - Yes, patch.txt looks fine! I'm good with it. I believe that 654_2.patch and patch.txt are functionally exactly equivalent. However, you're right for a 1.6.1 addition it is safer to go with changes only consistent with intent of the patch. I can do a small cleanup later.
        Hide
        Byron Foster added a comment -

        You: "So, is there a reason you didn't log an error for all the caught exceptions? Would some be redundant"

        To make sure we are talking about the same thing, So in patch 654_2 we have the following piece of code:

        try
        {
        if (!blockinput)

        { context.pushCurrentTemplateName(arg); ((SimpleNode) t.getData()).render( context, writer ); }

        }
        catch(RuntimeException e)

        { /** * Log #parse errors so the user can track which file called which. */ rsvc.getLog().error("Exception rendering #parse(" + arg + ") at " + Log.formatFileString(this)); throw e; }

        finally

        { if (!blockinput) context.popCurrentTemplateName(); }

        So all caught exceptions should log an error, If I'm understanding your question.

        Show
        Byron Foster added a comment - You: "So, is there a reason you didn't log an error for all the caught exceptions? Would some be redundant" To make sure we are talking about the same thing, So in patch 654_2 we have the following piece of code: try { if (!blockinput) { context.pushCurrentTemplateName(arg); ((SimpleNode) t.getData()).render( context, writer ); } } catch(RuntimeException e) { /** * Log #parse errors so the user can track which file called which. */ rsvc.getLog().error("Exception rendering #parse(" + arg + ") at " + Log.formatFileString(this)); throw e; } finally { if (!blockinput) context.popCurrentTemplateName(); } So all caught exceptions should log an error, If I'm understanding your question.
        Hide
        Nathan Bubna added a comment -

        Compare what i checked in to your current code. I think you'll see the extra error logs i added.

        Show
        Nathan Bubna added a comment - Compare what i checked in to your current code. I think you'll see the extra error logs i added.
        Hide
        Nathan Bubna added a comment -

        I went ahead and checked it in, so we can call this fixed. If it turns out some of those error logs were redundant let me know and i'll drop them.

        Show
        Nathan Bubna added a comment - I went ahead and checked it in, so we can call this fixed. If it turns out some of those error logs were redundant let me know and i'll drop them.
        Hide
        Byron Foster added a comment -

        I see what you did. I'm not sure if it will be redundant or not, no big deal though.

        Show
        Byron Foster added a comment - I see what you did. I'm not sure if it will be redundant or not, no big deal though.

          People

          • Assignee:
            Unassigned
            Reporter:
            Byron Foster
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development