Velocity
  1. Velocity
  2. VELOCITY-618

Strict property and method references

    Details

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

      Description

      The given patch against trunk adds a new option 'runtime.references.strict'. When set to true, invalid property references will throw a InvalidMethodException. For example $foo.bar will throw an exception if the object contained in $foo has no such property as bar. Any kind of reference to bar will cause an exception including:

      #if(#foo.bar)
      #set($foo.bar = "junk")
      #set($foo.getBar())
      etc...

      1. docs.patch
        4 kB
        Byron Foster
      2. MacroAndVarEscape.patch
        7 kB
        Byron Foster
      3. strictPropertyAndVariable_3.patch
        20 kB
        Byron Foster

        Activity

        Hide
        Byron Foster added a comment -

        docs.patch provides documentation

        Show
        Byron Foster added a comment - docs.patch provides documentation
        Hide
        Byron Foster added a comment -

        I think it's all in, including the macro changes. All get the docs done and attach, and I think that will be it.

        Show
        Byron Foster added a comment - I think it's all in, including the macro changes. All get the docs done and attach, and I think that will be it.
        Hide
        Nathan Bubna added a comment -

        Yeah, i know very well how bad it is. But it's been this way a long time and this upcoming release is still just a minor version one. And yeah, i know about the \$alt problem. This is why i essentially never use \ for escaping. It long ago became second nature to just use $

        {esc.d}

        alt (since i pretty much always have VelocityTools available). Anyway, we can take the escaping discussion over to VELOCITY-623.

        I'll try and get the rest of this strict mode stuff in shortly (sans the escaping part).

        Show
        Nathan Bubna added a comment - Yeah, i know very well how bad it is. But it's been this way a long time and this upcoming release is still just a minor version one. And yeah, i know about the \$alt problem. This is why i essentially never use \ for escaping. It long ago became second nature to just use $ {esc.d} alt (since i pretty much always have VelocityTools available). Anyway, we can take the escaping discussion over to VELOCITY-623 . I'll try and get the rest of this strict mode stuff in shortly (sans the escaping part).
        Hide
        Byron Foster added a comment -

        It's actually worse then this. The way it stands, Velocity always tries to evaluate a reference even if it is escaped. In fact, Velocity's escape behavior is dependent on evaluating the reference. So, even \$abc will throw an exception in strict mode. Any dollar sign in the template that can be interpreted as a reference will need to be escaped with $

        {D}. yuck... However, I'll take what I can get VELOCITY-623 is a separate patch for adding this escape functionality.

        One a side note, this escape behavior in any case seems in general to borderlines on a bug. For example, the Alternator velocity tool has a side effect, as pointed out, when calling toString(). It will certainly be surprising to the developer that \$alt will change the value of $alt, and there is no way to prevent this unless you use ${D}

        alt.

        Show
        Byron Foster added a comment - It's actually worse then this. The way it stands, Velocity always tries to evaluate a reference even if it is escaped. In fact, Velocity's escape behavior is dependent on evaluating the reference. So, even \$abc will throw an exception in strict mode. Any dollar sign in the template that can be interpreted as a reference will need to be escaped with $ {D}. yuck... However, I'll take what I can get VELOCITY-623 is a separate patch for adding this escape functionality. One a side note, this escape behavior in any case seems in general to borderlines on a bug. For example, the Alternator velocity tool has a side effect, as pointed out, when calling toString(). It will certainly be surprising to the developer that \$alt will change the value of $alt, and there is no way to prevent this unless you use ${D} alt.
        Hide
        Nathan Bubna added a comment -

        I wouldn't bet against you on developers' first reaction. But i'll have to surprise you and cry foul over the inconsistency. I just don't feel we need to go around adding special cases to an escaping system that is already confusing. Really, i can appreciate your concern, but it feels wrong to me to have a "strict reference mode" property change escaping rules for references. I'm stretched enough by having "strict reference mode" be strict about undefined macros as well.

        If you really want to pursue configurability of escaping behavior, that's probably best left for a different issue/patch/property.

        I really don't think abc$

        {D}

        xyz is all that onerous. i use that type of escaping regularly.

        Show
        Nathan Bubna added a comment - I wouldn't bet against you on developers' first reaction. But i'll have to surprise you and cry foul over the inconsistency. I just don't feel we need to go around adding special cases to an escaping system that is already confusing. Really, i can appreciate your concern, but it feels wrong to me to have a "strict reference mode" property change escaping rules for references. I'm stretched enough by having "strict reference mode" be strict about undefined macros as well. If you really want to pursue configurability of escaping behavior, that's probably best left for a different issue/patch/property. I really don't think abc$ {D} xyz is all that onerous. i use that type of escaping regularly.
        Hide
        Byron Foster added a comment -

        The problem that comes up in strict mode is if you have a situation in a template where something looks like a variable such as 'abc$xyz'. Since this wouldn't be a defined variable, an exception would be thrown. I think the developer needs an easy way to escape these character sequences like 'abc\$xyz, and I think it would be a shame to burden strict mode with something like 'abc$

        {DOL}

        xyz'

        While this is a change in behavior, I don't think it is very dramatic, and it does act just like non strict mode for defined variables. in strict mode:

        $abc = <exception>
        \$abc = $abc
        $abc = <exception>
        \\\$abc = \$abc

        I bet money that when Velocity throws an exception on an unintended variable pattern, the first thing a developer will try is to throw a back slash in front of it. I would be surprised if someone cried fowl over it not being consistent with their understanding of the Velocity escaping rules. I have used Velocity on and off for years, and when this issue came up I still had to go to the docs to understand what the behavior is.. and still Anyway.. that's my case, let me know if you'd like to see a change to this.

        Show
        Byron Foster added a comment - The problem that comes up in strict mode is if you have a situation in a template where something looks like a variable such as 'abc$xyz'. Since this wouldn't be a defined variable, an exception would be thrown. I think the developer needs an easy way to escape these character sequences like 'abc\$xyz, and I think it would be a shame to burden strict mode with something like 'abc$ {DOL} xyz' While this is a change in behavior, I don't think it is very dramatic, and it does act just like non strict mode for defined variables. in strict mode: $abc = <exception> \$abc = $abc $abc = <exception> \\\$abc = \$abc I bet money that when Velocity throws an exception on an unintended variable pattern, the first thing a developer will try is to throw a back slash in front of it. I would be surprised if someone cried fowl over it not being consistent with their understanding of the Velocity escaping rules. I have used Velocity on and off for years, and when this issue came up I still had to go to the docs to understand what the behavior is.. and still Anyway.. that's my case, let me know if you'd like to see a change to this.
        Hide
        Nathan Bubna added a comment -

        Thanks. the RuntimeMacro change looks good. I'm skeptical of the reference escaping changes you added though, since it changes the way escaping works.

        Our confusing-yet-long-standing "escaping" support is and always has been for escaping valid references. "Valid references" are defined as those which are syntactically correct and have a defined value (in practice, this has meant "a non-null value"). So, you can only legitimately escape things that are syntactically correct and non-null. In my understanding, this "strict reference mode" should only change the way Velocity handles invalid references, not the definition of valid. So, whether the reference is escaped or not, should not change whether or not we throw an exception in strict mode.

        Show
        Nathan Bubna added a comment - Thanks. the RuntimeMacro change looks good. I'm skeptical of the reference escaping changes you added though, since it changes the way escaping works. Our confusing-yet-long-standing "escaping" support is and always has been for escaping valid references. "Valid references" are defined as those which are syntactically correct and have a defined value (in practice, this has meant "a non-null value"). So, you can only legitimately escape things that are syntactically correct and non-null. In my understanding, this "strict reference mode" should only change the way Velocity handles invalid references, not the definition of valid. So, whether the reference is escaped or not, should not change whether or not we throw an exception in strict mode.
        Hide
        Byron Foster added a comment -

        MacroAndVarEscape.patch is in addition to patch 3 which adds the above functionality described for patch 4, but against trunk with 3 already applied.

        Show
        Byron Foster added a comment - MacroAndVarEscape.patch is in addition to patch 3 which adds the above functionality described for patch 4, but against trunk with 3 already applied.
        Hide
        Nathan Bubna added a comment -

        Could you run that last patch against the current head. It'll be easier for me to apply the additional changes if i don't have to sort out the old ones.

        Show
        Nathan Bubna added a comment - Could you run that last patch against the current head. It'll be easier for me to apply the additional changes if i don't have to sort out the old ones.
        Hide
        Byron Foster added a comment -

        Patch 4 adds macro references strict so that an undefined macro will throw an exception.

        Also, fixed escaping of variables in strict mode, added unit tests.

        Sorry, if this messed you up Nathan!

        Show
        Byron Foster added a comment - Patch 4 adds macro references strict so that an undefined macro will throw an exception. Also, fixed escaping of variables in strict mode, added unit tests. Sorry, if this messed you up Nathan!
        Hide
        Nathan Bubna added a comment -

        Byron, this looks great. The only change i'm making before checking it out is protecting uberInfo in ASTReference and adding a getTemplateName() method for ProxyVMContext to use. The test case is especially helpful.

        I'll get this checked in today, but i'll leave the bug unresolved for now so we don't forget the documentation part.

        Show
        Nathan Bubna added a comment - Byron, this looks great. The only change i'm making before checking it out is protecting uberInfo in ASTReference and adding a getTemplateName() method for ProxyVMContext to use. The test case is especially helpful. I'll get this checked in today, but i'll leave the bug unresolved for now so we don't forget the documentation part.
        Hide
        Byron Foster added a comment -

        Patch 3 contains the changes discusses about the #if statement. Mainly, if a variable is referenced alone within an #if expression then the variable is evaluated as if strict mode is false. So the following examples will work (Won't throw exceptions):

        #if($bogus)
        #if($bogus && $bogus.foo) // Short circuit, and bogus has foo
        #if($val == "junk" || $bogus) // Works if $val is defined
        #if($bogus1 || $bogus2)

        The following will throw an exception
        #if($bogus == "junk")
        #if($bogus.foo)
        #if($bogus > 4)

        So, I have mixed feeling about this solution.. but I think I can live with it. This is a special case, but the special case sort of stands out, and may actually be somewhat intuitive... If someone wants to actually test if a variable exists then they can use #if($foo != $NULL). and I like the ability to do #If($foo && $foo.bar).

        Unit tests are included.

        If this looks good I'll submit a patch for docs.

        Show
        Byron Foster added a comment - Patch 3 contains the changes discusses about the #if statement. Mainly, if a variable is referenced alone within an #if expression then the variable is evaluated as if strict mode is false. So the following examples will work (Won't throw exceptions): #if($bogus) #if($bogus && $bogus.foo) // Short circuit, and bogus has foo #if($val == "junk" || $bogus) // Works if $val is defined #if($bogus1 || $bogus2) The following will throw an exception #if($bogus == "junk") #if($bogus.foo) #if($bogus > 4) So, I have mixed feeling about this solution.. but I think I can live with it. This is a special case, but the special case sort of stands out, and may actually be somewhat intuitive... If someone wants to actually test if a variable exists then they can use #if($foo != $NULL). and I like the ability to do #If($foo && $foo.bar). Unit tests are included. If this looks good I'll submit a patch for docs.
        Hide
        Byron Foster added a comment -

        Yes, I'll add unit tests and documentation for this feature on inclusion.

        Show
        Byron Foster added a comment - Yes, I'll add unit tests and documentation for this feature on inclusion.
        Hide
        Byron Foster added a comment -

        If VELOCITY-619 is not accepted then a possible change to this patch is to allow undefined variables in the one case of:

        #if ($foo)

        As discussed in VELOCITY-619, creating a special case sometimes leads to behavior confusion, but it may work out well in this case.

        Show
        Byron Foster added a comment - If VELOCITY-619 is not accepted then a possible change to this patch is to allow undefined variables in the one case of: #if ($foo) As discussed in VELOCITY-619 , creating a special case sometimes leads to behavior confusion, but it may work out well in this case.
        Hide
        Byron Foster added a comment - - edited

        Nathan, The reason uberInfo is public is so this member can be accessed from ProxyVMContext to create a meaningful exception:

        throw new MethodInvocationException("Parameter '" + ref.getRootString() +"' not defined", null, key, ref.uberInfo.getTemplateName(), ref.getLine(), ref.getColumn());

        Otherwise, I can't report the template name of the error.

        Show
        Byron Foster added a comment - - edited Nathan, The reason uberInfo is public is so this member can be accessed from ProxyVMContext to create a meaningful exception: throw new MethodInvocationException("Parameter '" + ref.getRootString() +"' not defined", null, key, ref.uberInfo.getTemplateName(), ref.getLine(), ref.getColumn()); Otherwise, I can't report the template name of the error.
        Hide
        Byron Foster added a comment - - edited

        Removed modification of ASTNENode.java and ASTEQNode.java so that the behavior of #if( $foo == "bar") is not changed. These changes are not appropriate for this patch, and should be separated out into a different patch, to be discussed independently.

        Edit: yea you're right. These fixes exist already, and strict mode will fall out without changes.

        Show
        Byron Foster added a comment - - edited Removed modification of ASTNENode.java and ASTEQNode.java so that the behavior of #if( $foo == "bar") is not changed. These changes are not appropriate for this patch, and should be separated out into a different patch, to be discussed independently. Edit: yea you're right. These fixes exist already, and strict mode will fall out without changes.
        Hide
        Nathan Bubna added a comment -

        at first glance, i see two problems:

        • I recently fixed the "issue of #if statements being ignored because one of the sides of the comparison is null, or non-existent". This doesn't require a strict mode being enabled to allow it. So, the changes to ASTNENode and ASTEQNode are extraneous.
        • i don't see any reason to make uberInfo a public field in ASTReference

        Otherwise, it looks good. I can see people using this too, and i don't really have any problem with allowing null values to be kept/set in the context as long as it still passes the test suite. Speaking of which, would you be willing to create a test case for these changes/features? It would make it a lot easier for me to get these changes in and would help ensure no one breaks them in the future.

        Show
        Nathan Bubna added a comment - at first glance, i see two problems: I recently fixed the "issue of #if statements being ignored because one of the sides of the comparison is null, or non-existent". This doesn't require a strict mode being enabled to allow it. So, the changes to ASTNENode and ASTEQNode are extraneous. i don't see any reason to make uberInfo a public field in ASTReference Otherwise, it looks good. I can see people using this too, and i don't really have any problem with allowing null values to be kept/set in the context as long as it still passes the test suite. Speaking of which, would you be willing to create a test case for these changes/features? It would make it a lot easier for me to get these changes in and would help ensure no one breaks them in the future.
        Hide
        Byron Foster added a comment -

        strictPropertAndVariable.patch includes the changes of patch strictPropertyRef.patch and also enforces that variables must exist otherwise an exception is thrown. This is turned on using the same runtime.references.strict option. Internally variables may now be set with a NULL value, as in #(set = $null), and not just simply removed.

        This patch also fixes the issue of #if statements being ignored because one of the sides of the comparison is null, or non-existent. In the latter case bad references to variables or properties will throw an exception. In the former case nulls can be properly compared.

        Show
        Byron Foster added a comment - strictPropertAndVariable.patch includes the changes of patch strictPropertyRef.patch and also enforces that variables must exist otherwise an exception is thrown. This is turned on using the same runtime.references.strict option. Internally variables may now be set with a NULL value, as in #(set = $null), and not just simply removed. This patch also fixes the issue of #if statements being ignored because one of the sides of the comparison is null, or non-existent. In the latter case bad references to variables or properties will throw an exception. In the former case nulls can be properly compared.
        Hide
        Byron Foster added a comment -

        Patch for enforcing strict method and property references

        Show
        Byron Foster added a comment - Patch for enforcing strict method and property references

          People

          • Assignee:
            Unassigned
            Reporter:
            Byron Foster
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development