Velocity
  1. Velocity
  2. VELOCITY-553

Posibility to configure ReportInvalidReferences to don't report report variables,properties and method which exist, but only have null value

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.5
    • Fix Version/s: 2.x
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      any

      Description

      ReportInvalidReferences has very big imperfection, it report by default all variables, properties and method which has null value.
      This may cause many problems for developer.

      I for example need only validate template without any data, only check which contain right variables, properties or method (which exist), it's value is not important for me.

      I tried use my own ReferenceInsertionEventHandler for replace null value with "" (empty String) but Velocity call InvalidReference handler before ReferenceInsertionEventHandler.

      I suggest configuration options for this (repor or doesn't report null value)

        Activity

        Hide
        Will Glass-Husain added a comment -

        Good comment. Rather than a configuration option, we should just change the behavior.

        It's an experimental feature, seems ok to me alter.

        Targeting for 1.6.

        Show
        Will Glass-Husain added a comment - Good comment. Rather than a configuration option, we should just change the behavior. It's an experimental feature, seems ok to me alter. Targeting for 1.6.
        Hide
        Will Glass-Husain added a comment -

        Hi,

        Looking at this issue. There's a couple of cases here, any comment?

        $something.abc where there is no $something in the context
        --> should call the event handler

        $something.abc where this is no property getAbc() or isAbc()
        --> should call the event handler

        $something.abc where $something.getAbc() is null
        --> should not call the event handler

        But how about
        $something.abc.def where $something.getAbc() is null
        --> I'd argue this should call the event handler

        In other words, a null response doesn't trigger this, but calling another method on top of null does.

        Seem reasonable?

        Show
        Will Glass-Husain added a comment - Hi, Looking at this issue. There's a couple of cases here, any comment? $something.abc where there is no $something in the context --> should call the event handler $something.abc where this is no property getAbc() or isAbc() --> should call the event handler $something.abc where $something.getAbc() is null --> should not call the event handler But how about $something.abc.def where $something.getAbc() is null --> I'd argue this should call the event handler In other words, a null response doesn't trigger this, but calling another method on top of null does. Seem reasonable?
        Hide
        Nathan Bubna added a comment -

        Yes, that makes perfect sense to me.

        Show
        Nathan Bubna added a comment - Yes, that makes perfect sense to me.
        Hide
        Byron Foster added a comment -

        Really, strict mode VELOCITY-618 provides the functionality that I think the creator of this issue seeks. I think several of us have gone down the road of wanting strict type behavior and have looked into this to find that it doesn't distinguish between and invalid reference, and a reference that returns null .

        However, I think Will's comments above are exactly right for the desired behavior, and it parallels the same cases in which strict mode throws exceptions. ReportInvalidReference could be implemented as Will has outlined by simply making the call at every point that strict mode throws an exception.

        Further, you could replace all instances where strict mode throws exceptions with a default implementation of ReportInvalidReference that throws an exception. Even though you would have to augment the parameters passed so that you could provide the same level of informative error messages. For 2.0 this would make this type of handling nicely plugable. I would make strict the default, and drop the option.. then people could use an "anything goes" ReportInvalidReference if they wanted older behavior

        Show
        Byron Foster added a comment - Really, strict mode VELOCITY-618 provides the functionality that I think the creator of this issue seeks. I think several of us have gone down the road of wanting strict type behavior and have looked into this to find that it doesn't distinguish between and invalid reference, and a reference that returns null . However, I think Will's comments above are exactly right for the desired behavior, and it parallels the same cases in which strict mode throws exceptions. ReportInvalidReference could be implemented as Will has outlined by simply making the call at every point that strict mode throws an exception. Further, you could replace all instances where strict mode throws exceptions with a default implementation of ReportInvalidReference that throws an exception. Even though you would have to augment the parameters passed so that you could provide the same level of informative error messages. For 2.0 this would make this type of handling nicely plugable. I would make strict the default, and drop the option.. then people could use an "anything goes" ReportInvalidReference if they wanted older behavior
        Hide
        Byron Foster added a comment -

        One more comment, the reason why ReportInvalidReference works the way it does is that before strict mode was added there was no distinction in Velocity between a null value and a non-exiting key. An attempt to place a key into the context with a null value would be ignored. strict mode changed that so now there is a distinction.

        Show
        Byron Foster added a comment - One more comment, the reason why ReportInvalidReference works the way it does is that before strict mode was added there was no distinction in Velocity between a null value and a non-exiting key. An attempt to place a key into the context with a null value would be ignored. strict mode changed that so now there is a distinction.
        Hide
        Trejkaz added a comment -

        What I find surprising about this is that it results in errors when checking for existence of the object.

        e.g., this works if you don't override the handler:

        – --

        class A {
        public String getValue()

        { return null; }

        }

        – --

        context.put("record", new A());

        – --

        #if( $record.value )
        $record. value
        #end

        – --

        But if you override the handler you will get an error from the #if line. So even if you wanted to detect null values before you reference them, the normal way of doing it in the template doesn't work either because you get an error before you can.

        Show
        Trejkaz added a comment - What I find surprising about this is that it results in errors when checking for existence of the object. e.g., this works if you don't override the handler: – -- class A { public String getValue() { return null; } } – -- context.put("record", new A()); – -- #if( $record.value ) $record. value #end – -- But if you override the handler you will get an error from the #if line. So even if you wanted to detect null values before you reference them, the normal way of doing it in the template doesn't work either because you get an error before you can.
        Hide
        Renato Steiner added a comment - - edited

        I have noticed that Velocity makes a difference between accessing a property vs. accessing the property via a property accessor method:
        Note: $util.isNull is a simple utility method in one of the classes I unfortunately cannot attach. But you can simply create a utility method in one of your own classes which returns true if the params value is null.

        => The code below results in the invocation of InvalidReferenceEventHandler::invalidGetMethod(Context, String, Object, String, Info).

        #set ($objRef = $util.getTestObject())
        #if ($util.isNull($objRef.nullValueAttribute))
        #debugMsg("@@util.isNull(objRef.nullValueAttribute) => isNull==true")
        #else
        #debugMsg("@@objRef.nullValueAttribute=$objRef.nullValueAttribute")
        #end

        ---------

        => The code below works correctly:

        #set ($objRef = $util.getTestObject())
        #if ($util.isNull($objRef.getNullValueAttribute()))
        #debugMsg("@@util.isNull(objRef.getNullValueAttribute()) => isNull==true")
        #else
        #debugMsg("@@objRef.nullValueAttribute=$objRef.nullValueAttribute")
        #end

        ---------

        Note: TestObject is declared within InvalidEventHandlerTestCase.java.

        I have created and attached a new unit test which can be used to verify the desired behavior.
        If the patch is applied, the InvalidEventHandlerTestCase.java test case should run without errors.
        If you uncomment the documented code segments, errors will appear until the bug is fixed.

        Notes:
        Single testcase execution (requires the patch: velocity-661-v1.0.patch -> http://issues.apache.org/jira/browse/VELOCITY-661)
        $ ant -Dtestcase=org.apache.velocity.test.InvalidEventHandlerTestCase test

        Related:
        http://issues.apache.org/jira/browse/VELOCITY-618
        https://issues.apache.org/jira/browse/VELOCITY-423
        http://wiki.apache.org/velocity/VelocityNullSupport

        Show
        Renato Steiner added a comment - - edited I have noticed that Velocity makes a difference between accessing a property vs. accessing the property via a property accessor method: Note: $util.isNull is a simple utility method in one of the classes I unfortunately cannot attach. But you can simply create a utility method in one of your own classes which returns true if the params value is null. => The code below results in the invocation of InvalidReferenceEventHandler::invalidGetMethod(Context, String, Object, String, Info). #set ($objRef = $util.getTestObject()) #if ($util.isNull($objRef.nullValueAttribute)) #debugMsg("@@util.isNull(objRef.nullValueAttribute) => isNull==true") #else #debugMsg("@@objRef.nullValueAttribute=$objRef.nullValueAttribute") #end --------- => The code below works correctly: #set ($objRef = $util.getTestObject()) #if ($util.isNull($objRef.getNullValueAttribute())) #debugMsg("@@util.isNull(objRef.getNullValueAttribute()) => isNull==true") #else #debugMsg("@@objRef.nullValueAttribute=$objRef.nullValueAttribute") #end --------- Note: TestObject is declared within InvalidEventHandlerTestCase.java. I have created and attached a new unit test which can be used to verify the desired behavior. If the patch is applied, the InvalidEventHandlerTestCase.java test case should run without errors. If you uncomment the documented code segments, errors will appear until the bug is fixed. Notes: Single testcase execution (requires the patch: velocity-661-v1.0.patch -> http://issues.apache.org/jira/browse/VELOCITY-661 ) $ ant -Dtestcase=org.apache.velocity.test.InvalidEventHandlerTestCase test Related: http://issues.apache.org/jira/browse/VELOCITY-618 https://issues.apache.org/jira/browse/VELOCITY-423 http://wiki.apache.org/velocity/VelocityNullSupport
        Hide
        Renato Steiner added a comment - - edited

        Attached a modified Unit Test (InvalidEventHandlerTestCase.java.patch) in order to reproduce the behavior of velocity when a method returns null.

        Show
        Renato Steiner added a comment - - edited Attached a modified Unit Test (InvalidEventHandlerTestCase.java.patch) in order to reproduce the behavior of velocity when a method returns null.
        Hide
        Nathan Bubna added a comment -

        Where are we here? It sounds like Byron had ideas for fixing/using this in 2.0, but no one has stepped up to resolve this for 1.7 (much less 1.6). Since we have strict mode now, is this even worth fixing in 1.7? I'd like to push out a 1.7 final soon. Unless someone steps up soon, this won't happen in 1.7.

        Show
        Nathan Bubna added a comment - Where are we here? It sounds like Byron had ideas for fixing/using this in 2.0, but no one has stepped up to resolve this for 1.7 (much less 1.6). Since we have strict mode now, is this even worth fixing in 1.7? I'd like to push out a 1.7 final soon. Unless someone steps up soon, this won't happen in 1.7.
        Hide
        Trejkaz added a comment -

        Still occurs in 1.7. Incidentally, strict mode also exhibits the same behaviour, throwing "does not contain property" errors when it gets nulls.

        Show
        Trejkaz added a comment - Still occurs in 1.7. Incidentally, strict mode also exhibits the same behaviour, throwing "does not contain property" errors when it gets nulls.

          People

          • Assignee:
            Unassigned
            Reporter:
            Tomáš Procházka
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development