Velocity
  1. Velocity
  2. VELOCITY-681

[regression] Changes on the macro parameters are not persisted outside the macro call

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6.1
    • Fix Version/s: 1.6.2
    • Component/s: Engine
    • Labels:
      None

      Description

      The fix for VELOCITY-615 was too radical, since it completely disables #setting new values to the formal arguments. A minimalistic example that used to work up to 1.6 (but not with 1.6.1) is:

      
      

      #macro(myMacro $result)
      #set($result = 'some value')
      #end
      #myMacro($x)
      $x

      {/noformat}

      which prints $x (as an undefined variable).

      1. VELOCITY-681-trunk.patch
        16 kB
        Sergiu Dumitriu
      2. VELOCITY-681-1.6.patch
        15 kB
        Sergiu Dumitriu

        Issue Links

          Activity

          Show
          Sergiu Dumitriu added a comment - See http://wiki.apache.org/velocity/MacroEvaluationStrategy
          Hide
          Nathan Bubna added a comment -

          Ok, this is fixed in 1.6.2, but will not be fixed in the trunk (per discussion).

          Show
          Nathan Bubna added a comment - Ok, this is fixed in 1.6.2, but will not be fixed in the trunk (per discussion).
          Hide
          Byron Foster added a comment -

          I looked at this some more, and my vote is to release 1.6.2 as it is. This is obscure behavior even though we know at least one person who is taking advantage of it. However, I would argue that there are better ways to do this. LIke I said, Sergiu has a good point about the behavior change, but fixing it is a real pain with little upside. I'm pretty confident that I could find some other obscure error as a result of fixing this.

          Show
          Byron Foster added a comment - I looked at this some more, and my vote is to release 1.6.2 as it is. This is obscure behavior even though we know at least one person who is taking advantage of it. However, I would argue that there are better ways to do this. LIke I said, Sergiu has a good point about the behavior change, but fixing it is a real pain with little upside. I'm pretty confident that I could find some other obscure error as a result of fixing this.
          Hide
          Claude Brisson added a comment - - edited

          Ok, I try to summarize.

          Prior to 1.6.1, the behaviour of the ProxyVM was ok. That is:

          #macro(populate $foo)#set($foo='hello')#end
          #populate($bar)

          would affect $bar and not $foo (or nothing at all, depending on the localscope setting) as 1.6.1 is doing.

          I'm not using such settings from inside macros myself, but I'm pretty sure users have, so I think like Byron that we should take this issue seriously and resolve it before pushing 1.6.2 out.

          I took a look at commit 724498, and I don't understand why the previous put method was so drastically reduced. I think we should fix the fix rather than applying Sergiu's patch (nothing personal, Sergiu!).

          Show
          Claude Brisson added a comment - - edited Ok, I try to summarize. Prior to 1.6.1, the behaviour of the ProxyVM was ok. That is: #macro(populate $foo)#set($foo='hello')#end #populate($bar) would affect $bar and not $foo (or nothing at all, depending on the localscope setting) as 1.6.1 is doing. I'm not using such settings from inside macros myself, but I'm pretty sure users have, so I think like Byron that we should take this issue seriously and resolve it before pushing 1.6.2 out. I took a look at commit 724498, and I don't understand why the previous put method was so drastically reduced. I think we should fix the fix rather than applying Sergiu's patch (nothing personal, Sergiu!).
          Hide
          Jarkko Viinamäki added a comment -

          Unfortunately I don't have any time until Saturday to take a closer look at this but I still don't understand what those ".literal."-calls are doing in ProxyVMContext.

          Question to the committer: do you fully understand what this patch does, why it does it so and how "render literal if null" functionality works?

          My answer to that question at the moment is "no" (because I haven't had time to investigate) and therefore I would not blindly commit this.

          Show
          Jarkko Viinamäki added a comment - Unfortunately I don't have any time until Saturday to take a closer look at this but I still don't understand what those ".literal."-calls are doing in ProxyVMContext. Question to the committer: do you fully understand what this patch does, why it does it so and how "render literal if null" functionality works? My answer to that question at the moment is "no" (because I haven't had time to investigate) and therefore I would not blindly commit this.
          Hide
          Nathan Bubna added a comment -

          If Jarkko (who knows the .literal stuff best) gives this patch an "all clear" soon, then i'd say we should get it in before releasing 1.6.2. If, however, the 72 hours is up and Claude has the votes to do the release and we still have heard an "all clear" on this, then i think we should just let the release proceed and "take our lumps" for the behavior change in a bugfix release. Sergiu and any other users who depended on the 1.5/1.6 behavior here will have to adapt eventually for 1.7 anyway and can patch their personal 1.6.2 version if desparate in the meantime.

          So, i think, if the latest release files look good when i check them out, i'll vote like this:

          (jarkko gives all clear on 681 patch) ? -1 : +1

          Show
          Nathan Bubna added a comment - If Jarkko (who knows the .literal stuff best) gives this patch an "all clear" soon, then i'd say we should get it in before releasing 1.6.2. If, however, the 72 hours is up and Claude has the votes to do the release and we still have heard an "all clear" on this, then i think we should just let the release proceed and "take our lumps" for the behavior change in a bugfix release. Sergiu and any other users who depended on the 1.5/1.6 behavior here will have to adapt eventually for 1.7 anyway and can patch their personal 1.6.2 version if desparate in the meantime. So, i think, if the latest release files look good when i check them out, i'll vote like this: (jarkko gives all clear on 681 patch) ? -1 : +1
          Hide
          Byron Foster added a comment -

          I applied this patch locally to 1.6.x and took a look at, it does pass all test cases. However, I can not comfortably determine if there will not be side effects that lead to a 1.6.3 Unfortunately, the changes that introduced this issue resulted from VELOCITY-615, which was a clear bug, so we can't simply roll it back.

          The choice: Commit this patch, and risk undesirable side effects. Don't commit this patch and suffer our lumps for a behavioral change in a bug fix release (Even though I would add this is the correct behavior).

          I'm leaning toward taking our lumps.

          Show
          Byron Foster added a comment - I applied this patch locally to 1.6.x and took a look at, it does pass all test cases. However, I can not comfortably determine if there will not be side effects that lead to a 1.6.3 Unfortunately, the changes that introduced this issue resulted from VELOCITY-615 , which was a clear bug, so we can't simply roll it back. The choice: Commit this patch, and risk undesirable side effects. Don't commit this patch and suffer our lumps for a behavioral change in a bug fix release (Even though I would add this is the correct behavior). I'm leaning toward taking our lumps.
          Hide
          Sergiu Dumitriu added a comment -

          After reading a bit more, I think that velocimacros are supposed to work as pass-by-name, and pass-by-name should allow the usecase described above. So, even if the behavior is not intuitive for some, it is correct for pass-by-name.

          Show
          Sergiu Dumitriu added a comment - After reading a bit more, I think that velocimacros are supposed to work as pass-by-name, and pass-by-name should allow the usecase described above. So, even if the behavior is not intuitive for some, it is correct for pass-by-name.
          Hide
          Sergiu Dumitriu added a comment -

          Agreed, this is not the expected behavior, but still we had some macros that depended on it. And given that this behavior has just recently been changed (and in a minor release), it should be changed back.

          The .literal.$varName was used so that assigning a value to a read-only proxy should still work (VELOCITY-615). I don't know exactly what is it supposed to do, but since it was already used as a local alias, and the kind of change VELOCITY-615 does is a local change, it seemed to be the right thing to modify. Creating a local variable meant that the proxy will be completely hidden after the first change, and changing the proxy is not possible at all.

          Show
          Sergiu Dumitriu added a comment - Agreed, this is not the expected behavior, but still we had some macros that depended on it. And given that this behavior has just recently been changed (and in a minor release), it should be changed back. The .literal.$varName was used so that assigning a value to a read-only proxy should still work ( VELOCITY-615 ). I don't know exactly what is it supposed to do, but since it was already used as a local alias, and the kind of change VELOCITY-615 does is a local change, it seemed to be the right thing to modify. Creating a local variable meant that the proxy will be completely hidden after the first change, and changing the proxy is not possible at all.
          Hide
          Jarkko Viinamäki added a comment -

          Please don't commit this yet.

          I need to review this carefully since there is some funny stuff happing with .literal.$varName that should not be at all in ProxyVMContext. It's possible that purpose of those keys have been misunderstood.

          Show
          Jarkko Viinamäki added a comment - Please don't commit this yet. I need to review this carefully since there is some funny stuff happing with .literal.$varName that should not be at all in ProxyVMContext. It's possible that purpose of those keys have been misunderstood.
          Hide
          Nathan Bubna added a comment -

          Thanks for testing this out, Jarkko. Ok, if it worked in 1.5 and 1.6 but not in 1.6.1, then i'm fine with this being added to the 1.6.x branch. In a bugfix release, we should try to keep changes in behavior minimal. I think though, that this is strange, surprising and uncommonly used enough that we should leave it out of the trunk. We can just put a notice in the changelog that as of 1.7, this behavior is no longer valid. Even after thinking about it for a night, i'm convinced the current trunk treatment of #setting macro args is superior.

          Show
          Nathan Bubna added a comment - Thanks for testing this out, Jarkko. Ok, if it worked in 1.5 and 1.6 but not in 1.6.1, then i'm fine with this being added to the 1.6.x branch. In a bugfix release, we should try to keep changes in behavior minimal. I think though, that this is strange, surprising and uncommonly used enough that we should leave it out of the trunk. We can just put a notice in the changelog that as of 1.7, this behavior is no longer valid. Even after thinking about it for a night, i'm convinced the current trunk treatment of #setting macro args is superior.
          Hide
          Byron Foster added a comment -

          Sergiu is correct in that this is a pretty heavy change for a bug fix release. And, technically this is correct behavior for pass by name, especially given the roots of the the macro directive which originated as a simple parameter substitution, then evaluation.

          However, i think the behavior is waked for all the points that Nathan brought up. We could apply this patch to 1.6.x. But I would be slow to add it to trunk.

          Show
          Byron Foster added a comment - Sergiu is correct in that this is a pretty heavy change for a bug fix release. And, technically this is correct behavior for pass by name, especially given the roots of the the macro directive which originated as a simple parameter substitution, then evaluation. However, i think the behavior is waked for all the points that Nathan brought up. We could apply this patch to 1.6.x. But I would be slow to add it to trunk.
          Hide
          Jarkko Viinamäki added a comment -

          I tested this and Sergiu's test case passes with 1.5 and 1.6 but not with 1.6.1. Nathan has valid points though. Maybe we could introduce (yet another) configuration file option for this so that both 1.6.1 type functionality and the old one would be possible?

          I didn't understand why there were these special .literal.$varName checks in ProxyVMContext in Sergiu's patch. There should be no need to check for those since there are needed only for "render literal if null" functionality and the only classes that should know about that special key structure are VelocimacroProxy and ASTReference.

          Show
          Jarkko Viinamäki added a comment - I tested this and Sergiu's test case passes with 1.5 and 1.6 but not with 1.6.1. Nathan has valid points though. Maybe we could introduce (yet another) configuration file option for this so that both 1.6.1 type functionality and the old one would be possible? I didn't understand why there were these special .literal.$varName checks in ProxyVMContext in Sergiu's patch. There should be no need to check for those since there are needed only for "render literal if null" functionality and the only classes that should know about that special key structure are VelocimacroProxy and ASTReference.
          Hide
          Nathan Bubna added a comment -

          Your claim that 1.6.1 "completely disables #setting new values to the formal arguments" is...er...rather overstated. The mere #setting of things was broken in 1.6 but promptly fixed in 1.6.1. It's only this proxying of reference #setting that fails, and i was unaware that it ever used to work (though i take your word for it). This isn't something i ever used, much less relied upon.

          That said, i guess i don't think passing macro args by name necessarily implies such complete proxying that it would proxy #set calls on those args. I consider that behavior to be rather surprising and over-complicated. I think few users would expect that #setting $result would set a value for $x and not even set a value for $result when velocimacro.context.localscope = false.

          If we (return) down the path of supporting this, what happens when a user forgets that #myMacro will try reassign the value of anything he passes to it, and does this:

          #myMacro( $foo.getBar() )

          or does:

          #myMacro( $x )
          $result

          because the user knows $result was #set and that velocimacro.context.localscope = false. Or for that matter, users might simply be surprised to find that setting $result changed their previous value for $x, when they go to use it again. Such problems will become even more of an issue if the user is using a macro library that they did not write, as they will then have to be exceedingly careful that they don't pass references to macros unless they're sure they don't have further use for that reference, for who knows what the macro could do to their reference, even if velocimacro.context.localscope is true. Even the new #local directive proposal would not prevent such overriding, given the way you implemented your patch. There would be no way to protect a reference you passed to a macro except to do a wasted #set before it:

          #set( $disposableX = $x )
          #myMacro( $disposableX )

          Honestly, i think i'd rather limit the proxying to the passing of args, not to the re-#setting of them. Then users don't have to worry about overriding the references being proxied when writing or using macros. The way it is is simpler to use and implement. Of course, on the other hand, breaking backwards compatibility is never ideal. Anyone else have thoughts?

          Show
          Nathan Bubna added a comment - Your claim that 1.6.1 "completely disables #setting new values to the formal arguments" is...er...rather overstated. The mere #setting of things was broken in 1.6 but promptly fixed in 1.6.1. It's only this proxying of reference #setting that fails, and i was unaware that it ever used to work (though i take your word for it). This isn't something i ever used, much less relied upon. That said, i guess i don't think passing macro args by name necessarily implies such complete proxying that it would proxy #set calls on those args. I consider that behavior to be rather surprising and over-complicated. I think few users would expect that #setting $result would set a value for $x and not even set a value for $result when velocimacro.context.localscope = false. If we (return) down the path of supporting this, what happens when a user forgets that #myMacro will try reassign the value of anything he passes to it, and does this: #myMacro( $foo.getBar() ) or does: #myMacro( $x ) $result because the user knows $result was #set and that velocimacro.context.localscope = false. Or for that matter, users might simply be surprised to find that setting $result changed their previous value for $x, when they go to use it again. Such problems will become even more of an issue if the user is using a macro library that they did not write, as they will then have to be exceedingly careful that they don't pass references to macros unless they're sure they don't have further use for that reference, for who knows what the macro could do to their reference, even if velocimacro.context.localscope is true. Even the new #local directive proposal would not prevent such overriding, given the way you implemented your patch. There would be no way to protect a reference you passed to a macro except to do a wasted #set before it: #set( $disposableX = $x ) #myMacro( $disposableX ) Honestly, i think i'd rather limit the proxying to the passing of args, not to the re-#setting of them. Then users don't have to worry about overriding the references being proxied when writing or using macros. The way it is is simpler to use and implement. Of course, on the other hand, breaking backwards compatibility is never ideal. Anyone else have thoughts?
          Hide
          Sergiu Dumitriu added a comment -

          Provided a fix, which doesn't break any of the existing tests.

          The behavior of org.apache.velocity.context.ProxyVMContext#put was changed to match the following behavior:

          • If this is a valid proxy to a variable, set the value of the proxied variable; otherwise:
          • If a local scope is enforced, set a local variable; otherwise:
          • If this is a local variable, set a local variable; otherwise:
          • If this is a proxy to a literal, set a local literal variable, and a global variable; otherwise:
          • Set a global variable (where global means the parent context)

          Also, #get was changed to take into account local literal variables.

          Local literal variable means a context parameter with the key ".literal.$varName", which was already used in #addVMProxyArg.

          Show
          Sergiu Dumitriu added a comment - Provided a fix, which doesn't break any of the existing tests. The behavior of org.apache.velocity.context.ProxyVMContext#put was changed to match the following behavior: If this is a valid proxy to a variable, set the value of the proxied variable; otherwise: If a local scope is enforced, set a local variable; otherwise: If this is a local variable, set a local variable; otherwise: If this is a proxy to a literal, set a local literal variable, and a global variable; otherwise: Set a global variable (where global means the parent context) Also, #get was changed to take into account local literal variables. Local literal variable means a context parameter with the key ".literal.$varName", which was already used in #addVMProxyArg.

            People

            • Assignee:
              Unassigned
              Reporter:
              Sergiu Dumitriu
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development