Velocity
  1. Velocity
  2. VELOCITY-537

Multi-line comments causing ParseException in macros in Velocity engine 1.5

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.6
    • Component/s: Engine
    • Labels:
      None

      Description

      Moving from velocity engine 1.4 to 1.5, one of my macros is now causing a ParseException:

      #macro( makeLink $filepath )#*

      ##if ($request.serverName.equalsIgnoreCase("www.liveserver.com") || $request.serverName.equalsIgnoreCase("liveserver.com"))#

      ##set($mirrorDomain = "livemirror.com")#

      ##elseif($request.serverName.equalsIgnoreCase("stagingserver.com"))#

      ##set($mirrorDomain = "stagingmirror.com")#

      ##elseif($request.serverName.equalsIgnoreCase("devserver.com") || $request.serverName.equalsIgnoreCase("localhost"))#

      ##set($mirrorDomain = "devmirror.com")#

      ##else#

      ##set($mirrorDomain = "liveserver.com")#

      ##end#

      *#http://$

      {mirrorDomain}

      $

      {filepath}

      #*

      *##end

      This macro uses the mutli-line comment hack to gobble whitespace (so that none appears in the link text, whilst preserving some kind of readability.

      This works fine in 1.4, but throws a ParseException in 1.5 (stacktrace below)

      – stacktrace –

      2007-04-03 16:05:38,712 - VelocimacroManager.parseTree() : exception makeLink
      org.apache.velocity.runtime.parser.ParseException: Lexical error: org.apache.velocity.runtime.parser.TokenMgrError: Lexical error at line 1, column 535. Encountered: <EOF> after : ""
      at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:124)
      at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1042)
      at org.apache.velocity.runtime.directive.VelocimacroProxy.parseTree(VelocimacroProxy.java:342)
      at org.apache.velocity.runtime.directive.VelocimacroProxy.setupMacro(VelocimacroProxy.java:322)
      at org.apache.velocity.runtime.directive.VelocimacroProxy.init(VelocimacroProxy.java:309)
      at org.apache.velocity.runtime.parser.node.ASTDirective.init(ASTDirective.java:134)
      at org.apache.velocity.runtime.parser.node.SimpleNode.init(SimpleNode.java:285)
      at org.apache.velocity.Template.initDocument(Template.java:199)
      at org.apache.velocity.Template.process(Template.java:121)
      at org.apache.velocity.runtime.resource.ResourceManagerImpl.loadResource(ResourceManagerImpl.java:415)
      at org.apache.velocity.runtime.resource.ResourceManagerImpl.getResource(ResourceManagerImpl.java:335)
      at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1102)
      at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1077)
      at org.apache.velocity.app.VelocityEngine.getTemplate(VelocityEngine.java:528)
      at org.apache.velocity.tools.view.servlet.VelocityViewServlet.getTemplate(VelocityViewServlet.java:667)
      at org.apache.velocity.tools.view.servlet.VelocityViewServlet.handleRequest(VelocityViewServlet.java:601)
      at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doRequest(VelocityViewServlet.java:541)
      at org.apache.velocity.tools.view.servlet.VelocityViewServlet.doGet(VelocityViewServlet.java:507)
      at javax.servlet.http.HttpServlet.service(HttpServlet.java:689)
      at javax.servlet.http.HttpServlet.service(HttpServlet.java:802)
      at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:252)
      at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:173)
      at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:213)
      at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:178)
      at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:126)
      at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:105)
      at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:107)
      at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:148)
      at org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:869)
      at org.apache.coyote.http11.Http11BaseProtocol$Http11ConnectionHandler.processConnection(Http11BaseProtocol.java:664)
      at org.apache.tomcat.util.net.PoolTcpEndpoint.processSocket(PoolTcpEndpoint.java:527)
      at org.apache.tomcat.util.net.LeaderFollowerWorkerThread.runIt(LeaderFollowerWorkerThread.java:80)
      at org.apache.tomcat.util.threads.ThreadPool$ControlRunnable.run(ThreadPool.java:684)
      at java.lang.Thread.run(Thread.java:595)

      1. velocity-537.zip
        4 kB
        Marnix van Bochove

        Issue Links

          Activity

          Hide
          Nathan Bubna added a comment -

          The exception does not occur until you try to use the macro, and it can be reproduced with a macro as simple as:

          #macro( test537a )test#*
          *##end

          Change the macro to any of these:

          #macro( test537b )test
          #**##end

          #macro( test537c )
          test#**##end

          #macro( test537d )#**#test#end

          #macro( test537e )
          #**#test#end

          #macro( test537f )#*
          *#test#end

          And the error no longer occurs. However, #test537f() does not produce the expected output:

          #test537f()

          produces:

          *#test

          Which is baffling, to say the least!

          Something's very rotten here. The worst part is that i could not find any simple change to Christopher's macro that would make it work as expected. Instead i got increasingly bizarre results. Did anyone who uses the common whitespace removal/management hacks test their code against 1.5? It's not looking that way to me...

          Show
          Nathan Bubna added a comment - The exception does not occur until you try to use the macro, and it can be reproduced with a macro as simple as: #macro( test537a )test#* *##end Change the macro to any of these: #macro( test537b )test #**##end #macro( test537c ) test#**##end #macro( test537d )#**#test#end #macro( test537e ) #**#test#end #macro( test537f )#* *#test#end And the error no longer occurs. However, #test537f() does not produce the expected output: #test537f() produces: *#test Which is baffling, to say the least! Something's very rotten here. The worst part is that i could not find any simple change to Christopher's macro that would make it work as expected. Instead i got increasingly bizarre results. Did anyone who uses the common whitespace removal/management hacks test their code against 1.5? It's not looking that way to me...
          Hide
          Ben Speakmon added a comment -

          Is this still a problem? I haven't seen this.

          Show
          Ben Speakmon added a comment - Is this still a problem? I haven't seen this.
          Hide
          Peter Locke added a comment -

          Definitely still a problem. I've encountered this in an attempted upgrade.

          Show
          Peter Locke added a comment - Definitely still a problem. I've encountered this in an attempted upgrade.
          Hide
          Christopher Owen added a comment -

          At Atlassian we're looking to upgrade Confluence to use Velocity 1.5 and I've just run into this problem as well.

          The funny thing is, if you add an argument to the macro definition it then parses fine. I'd love to dig into the Velocity parser but I'm not that familiar with the tools used to generate it. We might just have to work around it.

          Show
          Christopher Owen added a comment - At Atlassian we're looking to upgrade Confluence to use Velocity 1.5 and I've just run into this problem as well. The funny thing is, if you add an argument to the macro definition it then parses fine. I'd love to dig into the Velocity parser but I'm not that familiar with the tools used to generate it. We might just have to work around it.
          Hide
          Will Glass-Husain added a comment -

          I did a little work on this earlier in the year. Something tricky is going on.

          Let me take another stab at this – I have a little downtime during the holidays. Kind of bizarre issue. The "adding an argument fixes the issue" is a good clue.

          Show
          Will Glass-Husain added a comment - I did a little work on this earlier in the year. Something tricky is going on. Let me take another stab at this – I have a little downtime during the holidays. Kind of bizarre issue. The "adding an argument fixes the issue" is a good clue.
          Hide
          Christopher Owen added a comment -

          Actually it looks like I'm not 100% correct. I've found cases where adding an argument doesn't work.

          Show
          Christopher Owen added a comment - Actually it looks like I'm not 100% correct. I've found cases where adding an argument doesn't work.
          Hide
          Marnix van Bochove added a comment -

          I found a solution to this problem (and the underlying issue as mentioned in issue VELOCITY-580:

          The problem is when method getASTAsStringArray(Node rootNode) of class Macro (package org.apache.velocity.runtime.directive) is called to produce a List of Strings, for some reason a string "#" appears. This string is coming from a ASTComment node which has a token whose image is "#". I don't know if ASTComment is parsed invalid or the image of the token is wrong, but when changing method NodeUtils.tokenLiteral( Token t ) (in package org.apache.velocity.runtime.parser.node) to this:

          public static String tokenLiteral( Token t )
          {
          String result;
          // Issue: VELOCITY-580
          // Marnix 2008-02-20: Look at king of token and return "" when it's a multiline comment
          if (t.kind == ParserConstants.MULTI_LINE_COMMENT)

          { result = ""; }

          else

          { result = specialText( t ) + t.image; }

          return result;
          }

          the problem of "*#" in macro output will disappear.

          Another solution could be, to change the image of the first token inside a ASTComment Node.

          Show
          Marnix van Bochove added a comment - I found a solution to this problem (and the underlying issue as mentioned in issue VELOCITY-580 : The problem is when method getASTAsStringArray(Node rootNode) of class Macro (package org.apache.velocity.runtime.directive) is called to produce a List of Strings, for some reason a string " #" appears. This string is coming from a ASTComment node which has a token whose image is " #". I don't know if ASTComment is parsed invalid or the image of the token is wrong, but when changing method NodeUtils.tokenLiteral( Token t ) (in package org.apache.velocity.runtime.parser.node) to this: public static String tokenLiteral( Token t ) { String result; // Issue: VELOCITY-580 // Marnix 2008-02-20: Look at king of token and return "" when it's a multiline comment if (t.kind == ParserConstants.MULTI_LINE_COMMENT) { result = ""; } else { result = specialText( t ) + t.image; } return result; } the problem of "*#" in macro output will disappear. Another solution could be, to change the image of the first token inside a ASTComment Node.
          Hide
          Christopher Schultz added a comment -

          Marnix,

          I'm no expert in how Velocity parses and executes templates, but this seems like the kind of thing that should be fixed in the parser itself, not an AST walker. I'm not sure how the team feels, but I'm not sure there's a reason to do anything with comments but completely ignore them during the building of the AST.

          From the stack trace, though, it looks like this is actually /during/ parsing, rather than evaluation.

          That would indicate to me that the problem /is/ with the parser (probably the grammar). Is the method indicated in the previous comment automatically generated by javacc/antlr/jlex+cup or whatever compiler compiler we use? If so, the opportunities for fixing this bug in an auto-generated method are quite limited.

          Show
          Christopher Schultz added a comment - Marnix, I'm no expert in how Velocity parses and executes templates, but this seems like the kind of thing that should be fixed in the parser itself, not an AST walker. I'm not sure how the team feels, but I'm not sure there's a reason to do anything with comments but completely ignore them during the building of the AST. From the stack trace, though, it looks like this is actually /during/ parsing, rather than evaluation. That would indicate to me that the problem /is/ with the parser (probably the grammar). Is the method indicated in the previous comment automatically generated by javacc/antlr/jlex+cup or whatever compiler compiler we use? If so, the opportunities for fixing this bug in an auto-generated method are quite limited.
          Hide
          Marnix van Bochove added a comment -

          Christopher,

          I think the ASTComment node should not be in the list, so I agree it's probably a problem with the parser. I don't know how the tokenLiteral method is generated or not.

          Show
          Marnix van Bochove added a comment - Christopher, I think the ASTComment node should not be in the list, so I agree it's probably a problem with the parser. I don't know how the tokenLiteral method is generated or not.
          Hide
          Nathan Bubna added a comment -

          NodeUtils.tokenLiteral() is not a generated class/method. But it does seem odd to have the fix here when the problem seems to be rooted from elsewhere. Still, if it fixes it and no one steps up to dig for the root issue, then i'd support including this fix. Better a workaround than nothing! Marnix, do you think you could whip up a test case to go with this patch (well, it's sort of a patch ?

          Show
          Nathan Bubna added a comment - NodeUtils.tokenLiteral() is not a generated class/method. But it does seem odd to have the fix here when the problem seems to be rooted from elsewhere. Still, if it fixes it and no one steps up to dig for the root issue, then i'd support including this fix. Better a workaround than nothing! Marnix, do you think you could whip up a test case to go with this patch (well, it's sort of a patch ?
          Hide
          Marnix van Bochove added a comment -

          I used this template to produce the problem:

          #macro(someMacro )
          #*
          *#
          #end
          #someMacro()

          I use velocity to produce a application which consists of 385 generated java files. With the patch the resulting formatted java code produced by Velocity 4 is the same as produced by Velocity 5 (with patch). What's the best way to proof this patch for the project? I could make a unit test which fails without the patch and succeeds with the patch.

          Show
          Marnix van Bochove added a comment - I used this template to produce the problem: #macro(someMacro ) #* *# #end #someMacro() I use velocity to produce a application which consists of 385 generated java files. With the patch the resulting formatted java code produced by Velocity 4 is the same as produced by Velocity 5 (with patch). What's the best way to proof this patch for the project? I could make a unit test which fails without the patch and succeeds with the patch.
          Hide
          Nathan Bubna added a comment -

          A unit test would be great!

          Show
          Nathan Bubna added a comment - A unit test would be great!
          Hide
          Marnix van Bochove added a comment -

          I created a unit test. Extract the attached zip file to the root folder of velocity. Run ant to verify the build fails due to "Test org.apache.velocity.test.issues.Velocity537TestCase".

          Apply this patch to file src\java\org\apache\velocity\runtime\parser\node\NodeUtils.java :

          public static String tokenLiteral( Token t )
          {
          String result;
          // Issue: VELOCITY-580
          // Marnix 2008-02-20: Look at kind of token and return "" when it's a multiline comment
          if (t.kind == ParserConstants.MULTI_LINE_COMMENT)

          { result = ""; }

          else

          { result = specialText( t ) + t.image; }

          return result;
          }

          Run ant clean test and check if test succeeds.

          Show
          Marnix van Bochove added a comment - I created a unit test. Extract the attached zip file to the root folder of velocity. Run ant to verify the build fails due to "Test org.apache.velocity.test.issues.Velocity537TestCase". Apply this patch to file src\java\org\apache\velocity\runtime\parser\node\NodeUtils.java : public static String tokenLiteral( Token t ) { String result; // Issue: VELOCITY-580 // Marnix 2008-02-20: Look at kind of token and return "" when it's a multiline comment if (t.kind == ParserConstants.MULTI_LINE_COMMENT) { result = ""; } else { result = specialText( t ) + t.image; } return result; } Run ant clean test and check if test succeeds.
          Hide
          Nathan Bubna added a comment -

          Ah, so this is really a test for VELOCITY-580. I'm not seeing any parse exceptions, just an extra *# in the output. I suppose that makes sense since NodeUtils doesn't have to do with parsing . I see your original patch comment refers to 580, not 537; i clearly wasn't paying sufficient attention earlier. I'll rename the test case to match. Anyway, it's great to have either dealt with, even if it's just a workaround. I'll probably hold off on committing the code for now, in hopes someone addresses the root cause and this won't be necessary. Thanks, this certainly resolves 580 nicely!

          Show
          Nathan Bubna added a comment - Ah, so this is really a test for VELOCITY-580 . I'm not seeing any parse exceptions, just an extra *# in the output. I suppose that makes sense since NodeUtils doesn't have to do with parsing . I see your original patch comment refers to 580, not 537; i clearly wasn't paying sufficient attention earlier. I'll rename the test case to match. Anyway, it's great to have either dealt with, even if it's just a workaround. I'll probably hold off on committing the code for now, in hopes someone addresses the root cause and this won't be necessary. Thanks, this certainly resolves 580 nicely!
          Hide
          Rafal Krzewski added a comment -

          I agree that the issue needs to be fixed in the parser itself, because the change to NodeUtils is just masking out one symptom of the problem.
          The parser, or rather the lexer is seriously going haywire. Depending on the number and configuration of the comments ParseExceptions occur, or comment boundaries get mismatched resulting in cropped literal text and ommitted logic. I'll try to provide more testcases showcasing the problems. Too bad that JavaCC is an absolute b*tch to debug. It would be most helpful if the folks that developed this particular parser could take a look at this...

          Show
          Rafal Krzewski added a comment - I agree that the issue needs to be fixed in the parser itself, because the change to NodeUtils is just masking out one symptom of the problem. The parser, or rather the lexer is seriously going haywire. Depending on the number and configuration of the comments ParseExceptions occur, or comment boundaries get mismatched resulting in cropped literal text and ommitted logic. I'll try to provide more testcases showcasing the problems. Too bad that JavaCC is an absolute b*tch to debug. It would be most helpful if the folks that developed this particular parser could take a look at this...
          Hide
          Nathan Bubna added a comment -

          agreed. i've tried to throw a little time into investigating this myself here and there, but i'm out of my depth, as i have no experience with JavaCC at this point in my life. the time it would take me to figure this out is probably an order of magnitude higher than it is for some others of you who have played with the parser already.

          this is a really unfortunate regression in 1.5 and is really the biggest thing holding me back from advocating for a 1.6 release. Anyone more parser savvy got a few cycles to spare on this? Henning? Geir? Will? Supun? Christoph? (guessing at names of people likely to be more apt for this problem)

          Show
          Nathan Bubna added a comment - agreed. i've tried to throw a little time into investigating this myself here and there, but i'm out of my depth, as i have no experience with JavaCC at this point in my life. the time it would take me to figure this out is probably an order of magnitude higher than it is for some others of you who have played with the parser already. this is a really unfortunate regression in 1.5 and is really the biggest thing holding me back from advocating for a 1.6 release. Anyone more parser savvy got a few cycles to spare on this? Henning? Geir? Will? Supun? Christoph? (guessing at names of people likely to be more apt for this problem)
          Hide
          Nathan Bubna added a comment -

          Found some time today to investigate this a bit further. The lexical error can be replicated with a template as simple as:

          #macro( test )#*
          *#end
          #test()

          If you add a quiet null reference at the end, the error disappears:

          #macro( test )#*
          *#$!null#end
          #test()

          but then VELOCITY-580 manifests and the output is

          *#

          So, if i get no further on this, we can at least hack-patch VELOCITY-580 with Marnix's NodeUtils' patch and propose the insertion of $!null at the end as a workaround for this. That won't make me happy, but it's better than the "sorry, no options to fix this" we currently have. And perhaps this smaller test case can help more parser-savvy folks figure out the real problem. I will start looking now, but i can't spend long on this and know little about JavaCC. Help would be great...

          Show
          Nathan Bubna added a comment - Found some time today to investigate this a bit further. The lexical error can be replicated with a template as simple as: #macro( test )#* *#end #test() If you add a quiet null reference at the end, the error disappears: #macro( test )#* *#$!null#end #test() but then VELOCITY-580 manifests and the output is *# So, if i get no further on this, we can at least hack-patch VELOCITY-580 with Marnix's NodeUtils' patch and propose the insertion of $!null at the end as a workaround for this. That won't make me happy, but it's better than the "sorry, no options to fix this" we currently have. And perhaps this smaller test case can help more parser-savvy folks figure out the real problem. I will start looking now, but i can't spend long on this and know little about JavaCC. Help would be great...
          Hide
          Nathan Bubna added a comment -

          Actually, it's better than i thought; Marnix's patch for NodeUtils fixes both VELOCITY-580 and VELOCITY-537. No *# in the output and no lexical errors in my unit tests for either issue.

          I realize that it is not the ideal fix, but a true fix it appears to be. Thoughts, anyone?

          Show
          Nathan Bubna added a comment - Actually, it's better than i thought; Marnix's patch for NodeUtils fixes both VELOCITY-580 and VELOCITY-537 . No *# in the output and no lexical errors in my unit tests for either issue. I realize that it is not the ideal fix, but a true fix it appears to be. Thoughts, anyone?
          Hide
          Nathan Bubna added a comment -

          To aid others in investigation of this (e.g. Will), i've checked in the unit tests for VELOCITY-580 and VELOCITY-537. Also, here is the relevant stack from the TokenMgrError that is at the heart of VELOCITY-537 (and is wrapped by the ParseException shown in the original description):

          org.apache.velocity.runtime.parser.TokenMgrError: Lexical error at line 1, column 3. Encountered: <EOF> after : ""
          at org.apache.velocity.runtime.parser.ParserTokenManager.getNextToken(ParserTokenManager.java:4093)
          at org.apache.velocity.runtime.parser.Parser.jj_ntk(Parser.java:3343)
          at org.apache.velocity.runtime.parser.Parser.process(Parser.java:275)
          at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105)
          at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078)
          at org.apache.velocity.runtime.directive.VelocimacroProxy.parseTree(VelocimacroProxy.java:355)
          at org.apache.velocity.runtime.directive.VelocimacroProxy.setupMacro(VelocimacroProxy.java:335)
          at org.apache.velocity.runtime.directive.VelocimacroProxy.init(VelocimacroProxy.java:322)
          at org.apache.velocity.runtime.directive.RuntimeMacro.render(RuntimeMacro.java:218)
          at org.apache.velocity.runtime.parser.node.ASTDirective.render(ASTDirective.java:178)
          at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:317)
          at org.apache.velocity.Template.merge(Template.java:324)
          at org.apache.velocity.Template.merge(Template.java:232)
          at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:96)

          I also traced the callstack of the relevant call to NodeUtils.tokenLiteral() (where Marnix's patch is applied) to try and figure out why his patch worked. The stack is:

          at org.apache.velocity.runtime.parser.node.NodeUtils.tokenLiteral(NodeUtils.java:159)
          at org.apache.velocity.runtime.directive.Macro.getASTAsStringArray(Macro.java:313)
          at org.apache.velocity.runtime.directive.Macro.processAndRegister(Macro.java:188)
          at org.apache.velocity.runtime.parser.Parser.Directive(Parser.java:902)
          at org.apache.velocity.runtime.parser.Parser.Statement(Parser.java:365)
          at org.apache.velocity.runtime.parser.Parser.process(Parser.java:303)
          at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105)
          at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078)
          at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1008)
          at org.apache.velocity.Template.process(Template.java:121)
          at org.apache.velocity.runtime.resource.ResourceManagerImpl.loadResource(ResourceManagerImpl.java:434)
          at org.apache.velocity.runtime.resource.ResourceManagerImpl.getResource(ResourceManagerImpl.java:350)
          at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1342)
          at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1317)
          at org.apache.velocity.runtime.RuntimeSingleton.getTemplate(RuntimeSingleton.java:303)
          at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:88)

          I'm not sure anymore how much this is a JavaCC grammar/parser problem. The lexical error is only thrown when you try to use the macro in question. It appears that the content is parsed a second time. I wonder if it is being mangled in between...

          Show
          Nathan Bubna added a comment - To aid others in investigation of this (e.g. Will), i've checked in the unit tests for VELOCITY-580 and VELOCITY-537 . Also, here is the relevant stack from the TokenMgrError that is at the heart of VELOCITY-537 (and is wrapped by the ParseException shown in the original description): org.apache.velocity.runtime.parser.TokenMgrError: Lexical error at line 1, column 3. Encountered: <EOF> after : "" at org.apache.velocity.runtime.parser.ParserTokenManager.getNextToken(ParserTokenManager.java:4093) at org.apache.velocity.runtime.parser.Parser.jj_ntk(Parser.java:3343) at org.apache.velocity.runtime.parser.Parser.process(Parser.java:275) at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105) at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078) at org.apache.velocity.runtime.directive.VelocimacroProxy.parseTree(VelocimacroProxy.java:355) at org.apache.velocity.runtime.directive.VelocimacroProxy.setupMacro(VelocimacroProxy.java:335) at org.apache.velocity.runtime.directive.VelocimacroProxy.init(VelocimacroProxy.java:322) at org.apache.velocity.runtime.directive.RuntimeMacro.render(RuntimeMacro.java:218) at org.apache.velocity.runtime.parser.node.ASTDirective.render(ASTDirective.java:178) at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:317) at org.apache.velocity.Template.merge(Template.java:324) at org.apache.velocity.Template.merge(Template.java:232) at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:96) I also traced the callstack of the relevant call to NodeUtils.tokenLiteral() (where Marnix's patch is applied) to try and figure out why his patch worked. The stack is: at org.apache.velocity.runtime.parser.node.NodeUtils.tokenLiteral(NodeUtils.java:159) at org.apache.velocity.runtime.directive.Macro.getASTAsStringArray(Macro.java:313) at org.apache.velocity.runtime.directive.Macro.processAndRegister(Macro.java:188) at org.apache.velocity.runtime.parser.Parser.Directive(Parser.java:902) at org.apache.velocity.runtime.parser.Parser.Statement(Parser.java:365) at org.apache.velocity.runtime.parser.Parser.process(Parser.java:303) at org.apache.velocity.runtime.parser.Parser.parse(Parser.java:105) at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1078) at org.apache.velocity.runtime.RuntimeInstance.parse(RuntimeInstance.java:1008) at org.apache.velocity.Template.process(Template.java:121) at org.apache.velocity.runtime.resource.ResourceManagerImpl.loadResource(ResourceManagerImpl.java:434) at org.apache.velocity.runtime.resource.ResourceManagerImpl.getResource(ResourceManagerImpl.java:350) at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1342) at org.apache.velocity.runtime.RuntimeInstance.getTemplate(RuntimeInstance.java:1317) at org.apache.velocity.runtime.RuntimeSingleton.getTemplate(RuntimeSingleton.java:303) at org.apache.velocity.test.issues.Velocity537TestCase.executeTest(Velocity537TestCase.java:88) I'm not sure anymore how much this is a JavaCC grammar/parser problem. The lexical error is only thrown when you try to use the macro in question. It appears that the content is parsed a second time. I wonder if it is being mangled in between...
          Hide
          Will Glass-Husain added a comment -

          Played with different examples. The problem seems to be that the first #* is being skipped by the parser. (also shown in Nathan's examples). All marnix's patch does is skip the # which is generating an error due to the missing #.

          I haven't tried it, but I'm guessing that

          #macro( test537f )#*
          abc*#test#end

          will incorrectly show "abc" with Marnix's patch. So I don't think this is the way to go.

          Let me look at this further again later today.

          Show
          Will Glass-Husain added a comment - Played with different examples. The problem seems to be that the first #* is being skipped by the parser. (also shown in Nathan's examples). All marnix's patch does is skip the # which is generating an error due to the missing # . I haven't tried it, but I'm guessing that #macro( test537f )#* abc*#test#end will incorrectly show "abc" with Marnix's patch. So I don't think this is the way to go. Let me look at this further again later today.
          Hide
          Nathan Bubna added a comment -

          Actually, i believe it's that the parser is errantly leaving in the *# token when it directly precedes the #end

          The start token and bodies of the multi-line comments never get to tokenLiteral() or getASTAsStringArray() in my testing. Changing the tests for 537 and 580 to have text in the comments does not make any difference, either with or without Marnix's patch. Really, i've tried most variations i can think of. Marnix's patch does seem to fix them all. Though ideally, the parser should eat the *# token along with the rest of the comment.

          Show
          Nathan Bubna added a comment - Actually, i believe it's that the parser is errantly leaving in the *# token when it directly precedes the #end The start token and bodies of the multi-line comments never get to tokenLiteral() or getASTAsStringArray() in my testing. Changing the tests for 537 and 580 to have text in the comments does not make any difference, either with or without Marnix's patch. Really, i've tried most variations i can think of. Marnix's patch does seem to fix them all. Though ideally, the parser should eat the *# token along with the rest of the comment.
          Hide
          Will Glass-Husain added a comment -

          FYI - I've got a fix to the parser for this issue. Almost. Just trying to make the unit tests pass.

          I spent about 6 hours working through this (surprisingly complex!). If we want to keep track of comments with the parser, then Marnix's patch is probably the best solution. But I think we should take the comments out in the tokenizer and only track the actual legit VTL. I think I have that, just need to make sure all edge cases are covered.

          Show
          Will Glass-Husain added a comment - FYI - I've got a fix to the parser for this issue. Almost. Just trying to make the unit tests pass. I spent about 6 hours working through this (surprisingly complex!). If we want to keep track of comments with the parser, then Marnix's patch is probably the best solution. But I think we should take the comments out in the tokenizer and only track the actual legit VTL. I think I have that, just need to make sure all edge cases are covered.
          Hide
          Will Glass-Husain added a comment -

          Hi - I spent another few hours on this. Tried to remove comments in the lexer. Turns out I re-introduced hitting a number of the comment-related bugs in the earlier versions of Velocity. (Hooray for regression tests). Specifically, if you change the comment tokens to SKIP instead of TOKEN, the MORE for a $ reference never ends. This means that the test

          $## comment
          something

          which should produce "$something" actually outputs "something". instead.

          So I gave up on that approach.

          Taking a step back, here's why this is all happening. When a comment is encountered a ASTComment node is included in the parse tree. (instead of just being skipped at the token level like you'd expect). When a page is rendered, comments do not render. So far, so good. But when a comment is included in a macro, the nodes of the parse tree are strung back together into text so that the actual string body of the macro is stored. The problem is that the full comment is not stored, just the ending "#". Consequently, any multiline comment in a macro results in an extra "#" being included in the macro. This may cause an error (e.g. the original VELOCITY-537) or just display "*#" literally.

          There's two approaches to solving this. Marnix's patch simply ignores the ASTComment token when creating the body for the macro. This makes the most sense to me. We could also save the complete comments and include in the macro. But since it's eventually ignored anyway this is pointless.

          Bottom line-- I've applied Marnix's patch and like it. (Good work). All unit tests pass, including the breaking ones by Nathan (thanks!) and one more I added.

          WILL

          Show
          Will Glass-Husain added a comment - Hi - I spent another few hours on this. Tried to remove comments in the lexer. Turns out I re-introduced hitting a number of the comment-related bugs in the earlier versions of Velocity. (Hooray for regression tests). Specifically, if you change the comment tokens to SKIP instead of TOKEN, the MORE for a $ reference never ends. This means that the test $## comment something which should produce "$something" actually outputs "something". instead. So I gave up on that approach. Taking a step back, here's why this is all happening. When a comment is encountered a ASTComment node is included in the parse tree. (instead of just being skipped at the token level like you'd expect). When a page is rendered, comments do not render. So far, so good. But when a comment is included in a macro, the nodes of the parse tree are strung back together into text so that the actual string body of the macro is stored. The problem is that the full comment is not stored, just the ending " #". Consequently, any multiline comment in a macro results in an extra " #" being included in the macro. This may cause an error (e.g. the original VELOCITY-537 ) or just display "*#" literally. There's two approaches to solving this. Marnix's patch simply ignores the ASTComment token when creating the body for the macro. This makes the most sense to me. We could also save the complete comments and include in the macro. But since it's eventually ignored anyway this is pointless. Bottom line-- I've applied Marnix's patch and like it. (Good work). All unit tests pass, including the breaking ones by Nathan (thanks!) and one more I added. WILL
          Hide
          Will Glass-Husain added a comment -

          applied patch

          Show
          Will Glass-Husain added a comment - applied patch
          Hide
          Nathan Bubna added a comment -

          Thanks, Will! And thanks again Marnix! It's great to have this fixed.

          Show
          Nathan Bubna added a comment - Thanks, Will! And thanks again Marnix! It's great to have this fixed.

            People

            • Assignee:
              Will Glass-Husain
              Reporter:
              Christopher Townson
            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development