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

        Malcolm Edgar created issue -
        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.
        Will Glass-Husain made changes -
        Field Original Value New Value
        Fix Version/s 1.5 [ 12310253 ]
        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?
        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 -

        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
        Will Glass-Husain made changes -
        Attachment macroargumenterror.patch [ 12324340 ]
        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
        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
        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
        Henning Schmiedehausen made changes -
        Component/s Engine [ 12311337 ]
        Will Glass-Husain made changes -
        Assignee Will Glass-Husain [ wglass ]
        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 -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        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.
        Henning Schmiedehausen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Mark Thomas made changes -
        Workflow jira [ 12351535 ] Default workflow, editable Closed status [ 12551279 ]
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12551279 ] jira [ 12552186 ]

          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