Velocity
  1. Velocity
  2. VELOCITY-435

ParseErrorException not thrown with #macro parse error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Windows XP, JDK 1.4.2_09

      Description

      I have just been reviewing the new error handlingin Velocity 1.5-dev.
      One change I have observed it that an invalid macro call, passing 2
      arguments instead of one will log an error message:

      [Velocity] [error] VM #writeForm: error : too many arguments to macro.
      Wanted 1 got 2
      [Velocity] [error] VM error writeForm. Null AST

      However it will not throw an ParseErrorException like it used to in
      1.5-dev. Please see the example below for the earlier behaviour:

      http://www.sunvolt.com/click-examples/exception.htm?actionLink=brokenBorderLink#

      I prefer earlier approach, as the error is explicit. The new approach
      logs an error message, but beyond that you would not have known that
      an error occured. The #writeForm() call is not even rendered, as is
      done with an invalid object reference.

      regards Malcolm Edgar

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        244d 22h 31m 1 Will Glass-Husain 05/Nov/06 06:03
        Resolved Resolved Closed Closed
        122d 18h 1m 1 Henning Schmiedehausen 08/Mar/07 00:04
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12551279 ] jira [ 12552186 ]
        Mark Thomas made changes -
        Workflow jira [ 12351535 ] Default workflow, editable Closed status [ 12551279 ]
        Henning Schmiedehausen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        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.
        Will Glass-Husain made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Hide
        Will Glass-Husain added a comment -

        Fix applied. Need to set property "velocimacro.arguments.strict" to true for the exception to be thrown. Thanks for the idea, Malcom!

        Show
        Will Glass-Husain added a comment - Fix applied. Need to set property "velocimacro.arguments.strict" to true for the exception to be thrown. Thanks for the idea, Malcom!
        Will Glass-Husain made changes -
        Assignee Will Glass-Husain [ wglass ]
        Henning Schmiedehausen made changes -
        Component/s Engine [ 12311337 ]
        Hide
        Will Glass-Husain added a comment -

        Hi Malcolm,

        Actually this is intentional. We now allow comments or spaces are parameter separators in macros.

        WILL

        Show
        Will Glass-Husain added a comment - Hi Malcolm, Actually this is intentional. We now allow comments or spaces are parameter separators in macros. WILL
        Hide
        Malcolm Edgar added a comment -

        The strict mode parameter is an excellent idea.

        regards Malcolm

        Show
        Malcolm Edgar added a comment - The strict mode parameter is an excellent idea. regards Malcolm
        Hide
        Malcolm Edgar added a comment -

        Hi Will,

        I have run up your test case against Velocity 1.4.

        I am afraid my error description was actually mis leading, the cause of the error was not a second argument:

        #foo('test1' 'test2')

        But adding a comma between the arguments. This causes a ParseErrorException in 1.4 and previous versions of 1.5-dev, but not in the latest 1.5-dev code.

        #foo('test1,' 'test2')

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Hi Will, I have run up your test case against Velocity 1.4. I am afraid my error description was actually mis leading, the cause of the error was not a second argument: #foo('test1' 'test2') But adding a comma between the arguments. This causes a ParseErrorException in 1.4 and previous versions of 1.5-dev, but not in the latest 1.5-dev code. #foo('test1,' 'test2') regards Malcolm Edgar
        Will Glass-Husain made changes -
        Attachment macroargumenterror.patch [ 12324340 ]
        Hide
        Will Glass-Husain added a comment -

        Attached file - throws an argument for invalid macro argument count

        Show
        Will Glass-Husain added a comment - Attached file - throws an argument for invalid macro argument count
        Hide
        Will Glass-Husain added a comment -

        Just to clarify my findings, these errors cause exceptions to be thrown

        • errors in macro body
        • syntax errors in macro arguments, e.g.
          #foo(abc)

        But calling macros with an invalid number of arguments does not thrown an exception, only logs an error
        #foo ("abc" "def")

        when #foo expects 1 argument

        The behavior appears to be the same in 1.4 and the latest source control head.

        Show
        Will Glass-Husain added a comment - Just to clarify my findings, these errors cause exceptions to be thrown errors in macro body syntax errors in macro arguments, e.g. #foo(abc) But calling macros with an invalid number of arguments does not thrown an exception, only logs an error #foo ("abc" "def") when #foo expects 1 argument The behavior appears to be the same in 1.4 and the latest source control head.
        Hide
        Will Glass-Husain added a comment -

        Hi,

        Spent some time working on this, even coded a solution. When I ran "ant test" though one of the Anakia tests failed due to an invalid macro call.

        The problem is that there are likely thousands of Velocity templates out there with a macro argument problem. If we put this error check in 1.5, then all those templates will suddenly fail.

        I do think that throwing an error here would be very appropriate. Maybe we need to add a "strict mode" property that will throw errors for invalid macro calls and invalid references.

        Comments?

        Show
        Will Glass-Husain added a comment - Hi, Spent some time working on this, even coded a solution. When I ran "ant test" though one of the Anakia tests failed due to an invalid macro call. The problem is that there are likely thousands of Velocity templates out there with a macro argument problem. If we put this error check in 1.5, then all those templates will suddenly fail. I do think that throwing an error here would be very appropriate. Maybe we need to add a "strict mode" property that will throw errors for invalid macro calls and invalid references. Comments?
        Will Glass-Husain made changes -
        Field Original Value New Value
        Fix Version/s 1.5 [ 12310253 ]
        Hide
        Will Glass-Husain added a comment -

        I agree. Let's make this work as Malcolm describes.

        Show
        Will Glass-Husain added a comment - I agree. Let's make this work as Malcolm describes.
        Malcolm Edgar created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development