Velocity
  1. Velocity
  2. VELOCITY-504

Call to evaluate inside of macro fails with 1.5 beta2

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.5 beta2
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Windows XP, Sun J2SE 1.5.0_09

      Description

      When upgrading from 1.5 beta 1 to beta2, I ran into a problem with one of our macros that makes a call to Velocity.evaluate (via a tool) within a foreach loop. With beta 1, everything worked fine. But we get a "cannot be resolved" error with beta 2. The same code works fine outside of a macro with either version.

      I've distilled the issue we're having down to a simple test case and have attached example code and a build file showing the problem. Just do an "ant beta1" or "ant beta2" to see how the results differ between beta1 and beta2.

      1. evaltooltest.patch
        10 kB
        Will Glass-Husain
      2. macrotest.zip
        1.86 MB
        Reggie Riser

        Issue Links

          Activity

          Reggie Riser created issue -
          Reggie Riser made changes -
          Field Original Value New Value
          Attachment macrotest.zip [ 12345970 ]
          Hide
          Will Glass-Husain added a comment -

          thanks so much for reporting this. we'll look into it asap.

          Show
          Will Glass-Husain added a comment - thanks so much for reporting this. we'll look into it asap.
          Henning Schmiedehausen made changes -
          Assignee Henning Schmiedehausen [ henning ]
          Hide
          Henning Schmiedehausen added a comment -

          Without any looking at your example or the source code, I'd bet that this is related to VELOCITY-285 and my fix for it.

          Could you please try whether changing velocimacro.context.localscope (default is false, so probably setting it to true) changes the behaviour for you?

          I might be able to look into this over the weekend.

          Show
          Henning Schmiedehausen added a comment - Without any looking at your example or the source code, I'd bet that this is related to VELOCITY-285 and my fix for it. Could you please try whether changing velocimacro.context.localscope (default is false, so probably setting it to true) changes the behaviour for you? I might be able to look into this over the weekend.
          Hide
          Reggie Riser added a comment -

          I tried setting "velocimacro.context.localscope = true" in velocity.properties in my attached example, but that made no difference in the test using beta 2. It broke the previously working test with beta 1.

          Thanks for taking a look at this. I'd really like to upgrade to beta 2, but this is a blocker for us until we find a workaround or fix.

          Show
          Reggie Riser added a comment - I tried setting "velocimacro.context.localscope = true" in velocity.properties in my attached example, but that made no difference in the test using beta 2. It broke the previously working test with beta 1. Thanks for taking a look at this. I'd really like to upgrade to beta 2, but this is a blocker for us until we find a workaround or fix.
          Hide
          Reggie Riser added a comment -

          After doing a little tracing, it appears that the foreach counter and the loop variable are showing up in the context for Test 3 in my example when using beta 1, but not appearing in the context in beta 2. It looks like it may have something to do with local scope as ForEach.java now puts the counter and loop variable in the local context with beta 2. The call to Velocity.evaluate does not appear to be seeing this local context, thus causing the "cannot be resolved" error.

          I'm not that familiar with the Velocity source, so I'd certainly appreciate any help with diagnosing the problem.

          Show
          Reggie Riser added a comment - After doing a little tracing, it appears that the foreach counter and the loop variable are showing up in the context for Test 3 in my example when using beta 1, but not appearing in the context in beta 2. It looks like it may have something to do with local scope as ForEach.java now puts the counter and loop variable in the local context with beta 2. The call to Velocity.evaluate does not appear to be seeing this local context, thus causing the "cannot be resolved" error. I'm not that familiar with the Velocity source, so I'd certainly appreciate any help with diagnosing the problem.
          Hide
          Will Glass-Husain added a comment -

          Thanks – will try to dig into this.

          Show
          Will Glass-Husain added a comment - Thanks – will try to dig into this.
          Will Glass-Husain made changes -
          Fix Version/s 1.5 [ 12310253 ]
          Hide
          Henning Schmiedehausen added a comment -

          This is on the very top of my list of things to do after the Velocity Site is deployed. Unfortunately I got distracted a bit with some creative Maven hacking. Don't worry, I will take care of this; Will if you want to dig into this, please start by reviewing the patch for VELOCITY-285 as mentioned above.

          Show
          Henning Schmiedehausen added a comment - This is on the very top of my list of things to do after the Velocity Site is deployed. Unfortunately I got distracted a bit with some creative Maven hacking. Don't worry, I will take care of this; Will if you want to dig into this, please start by reviewing the patch for VELOCITY-285 as mentioned above.
          Hide
          Will Glass-Husain added a comment -

          Ok, figured this out. I made an even simpler example; I can post it if others want to look at it.

          This example is in direct conflict to the bug fix in VELOCITY-285. In that issue we made it so that loop references are local to a macro. This was done because of problems with recursive macros-- with a macro and a loop the loop references needed to be kept distinct.

          The problem here is that the evaluated text passed to $tool.eval() (called within the macro) refers to $name, which is the loop reference. Since $tool.eval is accessing the global context, and $name is within the macro local context it cannot be referenced by $tool.eval().

          A workaround for this situation is to use #set($name2 = $name) and then have the evaluated text refer to $name2. This works fine, since $name2 is set within the global context by default (unless velocimacro.context.localscope = true).

          Bottom line, the bug fix for VELOCITY-285 is not backwards compatible. But, this is such a rare case (calling an evaluation tool and referencing the loop variable) AND there's a workaround I'm inclined to leave this as is. (with a note in the README file?)

          Comments? Particularly from Henning.

          Show
          Will Glass-Husain added a comment - Ok, figured this out. I made an even simpler example; I can post it if others want to look at it. This example is in direct conflict to the bug fix in VELOCITY-285 . In that issue we made it so that loop references are local to a macro. This was done because of problems with recursive macros-- with a macro and a loop the loop references needed to be kept distinct. The problem here is that the evaluated text passed to $tool.eval() (called within the macro) refers to $name, which is the loop reference. Since $tool.eval is accessing the global context, and $name is within the macro local context it cannot be referenced by $tool.eval(). A workaround for this situation is to use #set($name2 = $name) and then have the evaluated text refer to $name2. This works fine, since $name2 is set within the global context by default (unless velocimacro.context.localscope = true). Bottom line, the bug fix for VELOCITY-285 is not backwards compatible. But, this is such a rare case (calling an evaluation tool and referencing the loop variable) AND there's a workaround I'm inclined to leave this as is. (with a note in the README file?) Comments? Particularly from Henning.
          Will Glass-Husain made changes -
          Attachment evaltooltest.patch [ 12348158 ]
          Hide
          Will Glass-Husain added a comment -

          attached Junit test case (slightly simpler illustration of issue): evaltooltest.patch

          Show
          Will Glass-Husain added a comment - attached Junit test case (slightly simpler illustration of issue): evaltooltest.patch
          Hide
          Nathan Bubna added a comment -

          So, in order to trigger the issue, you have to be using a tool that calls evaluate() within a for loop within a macro, where the string being evaluated contains a reference to the loop variable?

          If i understand this right, then the fix for VELOCITY-285 should definitely win out over this problem. However, it should also be asked whether there is any way to give a tool access to the local context here? Could the global context be somehow notified that a local macro or for-loop context is in effect?

          I'm not aware of a way to do this offhand, and it doesn't sound simple to me. But i'm not fully versed in the relevant code here either...

          Show
          Nathan Bubna added a comment - So, in order to trigger the issue, you have to be using a tool that calls evaluate() within a for loop within a macro, where the string being evaluated contains a reference to the loop variable? If i understand this right, then the fix for VELOCITY-285 should definitely win out over this problem. However, it should also be asked whether there is any way to give a tool access to the local context here? Could the global context be somehow notified that a local macro or for-loop context is in effect? I'm not aware of a way to do this offhand, and it doesn't sound simple to me. But i'm not fully versed in the relevant code here either...
          Hide
          Will Glass-Husain added a comment -

          in response to your first question-- yes that is correct.

          Here's a crazy idea. Instead of using a tool to call a method, use a custom directive. This would have access to the local context.

          Better yet-- include it in the core as #evaluate()

          (why not? it's always asked for).

          WILL

          Show
          Will Glass-Husain added a comment - in response to your first question-- yes that is correct. Here's a crazy idea. Instead of using a tool to call a method, use a custom directive. This would have access to the local context. Better yet-- include it in the core as #evaluate() (why not? it's always asked for). WILL
          Hide
          Reggie Riser added a comment -

          FYI, the workaround that Will suggested worked fine for us. That said, I'd love to see an included #evaluate directive that has access to the local context.

          Show
          Reggie Riser added a comment - FYI, the workaround that Will suggested worked fine for us. That said, I'd love to see an included #evaluate directive that has access to the local context.
          Hide
          Nathan Bubna added a comment -

          what?! but that's CRAZY!

          ok, seriously, someone needs to create that directive. we should at least include that in the org.apache.velocity.runtime.directive package as a corollary to VelocityEngine.

          Then, when we have something to evaluate (pun intended), we can debate whether or not to include it in the default directive.properties or leave it as something that must be enabled by users.

          Show
          Nathan Bubna added a comment - what?! but that's CRAZY! ok, seriously, someone needs to create that directive. we should at least include that in the org.apache.velocity.runtime.directive package as a corollary to VelocityEngine. Then, when we have something to evaluate (pun intended), we can debate whether or not to include it in the default directive.properties or leave it as something that must be enabled by users.
          Hide
          Will Glass-Husain added a comment -

          ok, I'll make a separate issue for this.

          What did you mean "corollary to VelocityEngine"?

          Show
          Will Glass-Husain added a comment - ok, I'll make a separate issue for this. What did you mean "corollary to VelocityEngine"?
          Hide
          Nathan Bubna added a comment -

          oops. i didn't finish that. it should've read "as a corollary to VelocityEngine.evaluate()"

          in other words:

          #evaluate( ... ) is to VTL

          as

          engine.evaluate(...) is to Java

          Show
          Nathan Bubna added a comment - oops. i didn't finish that. it should've read "as a corollary to VelocityEngine.evaluate()" in other words: #evaluate( ... ) is to VTL as engine.evaluate(...) is to Java
          Will Glass-Husain made changes -
          Link This issue is related to VELOCITY-509 [ VELOCITY-509 ]
          Hide
          Will Glass-Husain added a comment -

          Henning – you have previously assigned this issue to yourself.

          I was going to mark this "wontfix" but i think you should have the final word since it is technically your issue. Please resolve if you are satisfied here.

          Show
          Will Glass-Husain added a comment - Henning – you have previously assigned this issue to yourself. I was going to mark this "wontfix" but i think you should have the final word since it is technically your issue. Please resolve if you are satisfied here.
          Hide
          Henning Schmiedehausen added a comment -

          Well, there is not much that I can add. Your analysis is spot on and basically VELOCITY-285 and this report show two sides of the same medal. I like the idea of #evaluate(), however I don't think that it will (or should) make the boat for 1.5. Maybe this is a good reason to have an 1.6 really soon.

          I think the current behaviour (having the loop variable as a local) is consistent with most other programming languages (especially Java... ), so I'd say "won't fix" is the right resolution.

          Thanks for digging into this.

          Show
          Henning Schmiedehausen added a comment - Well, there is not much that I can add. Your analysis is spot on and basically VELOCITY-285 and this report show two sides of the same medal. I like the idea of #evaluate(), however I don't think that it will (or should) make the boat for 1.5. Maybe this is a good reason to have an 1.6 really soon. I think the current behaviour (having the loop variable as a local) is consistent with most other programming languages (especially Java... ), so I'd say "won't fix" is the right resolution. Thanks for digging into this.
          Henning Schmiedehausen made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Won't Fix [ 2 ]
          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 ]
          Hide
          Will Glass-Husain added a comment -

          note that VELOCITY-509 has been implemented with new directive #evaluate(). (for Velocity 1.6)

          Show
          Will Glass-Husain added a comment - note that VELOCITY-509 has been implemented with new directive #evaluate(). (for Velocity 1.6)
          Mark Thomas made changes -
          Workflow jira [ 12390965 ] Default workflow, editable Closed status [ 12551482 ]
          Mark Thomas made changes -
          Workflow Default workflow, editable Closed status [ 12551482 ] jira [ 12552152 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          35d 23h 57m 1 Henning Schmiedehausen 03/Jan/07 22:29
          Resolved Resolved Closed Closed
          63d 1h 34m 1 Henning Schmiedehausen 08/Mar/07 00:04

            People

            • Assignee:
              Henning Schmiedehausen
              Reporter:
              Reggie Riser
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development