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: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • 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.
        Hide
        Claude Brisson added a comment -

        I'm in the process of reviewing some old issues... this one was a good candidate for 2.x.
        Here's what I did, it should correspond to what emerged in the comments:

        $invalid => triggers invalid ref
        $nullvalue => silent
        $valid.nullProp => silent
        $valid.nullMethod => silent
        $valid.invalid => triggers invalid ref

        That is, null is now always considered a legitimate final value.

        For #if / #elseif :

        #if($invalid) => silent
        #if($valid.invalid) => silent
        #if($invalid.prop) => triggers invalid ref
        #if($valid.invalid.prop) => triggers invalid ref

        Tests that trigger the invalid reference can all be rewritten so that they don't:

        #if($invalid && $invalid.prop) => silent
        #if($valid && $valid.invalid && $valid.invalid.prop) => silent

        Thanks to Renato Steiner for his test case patch.

        Show
        Claude Brisson added a comment - I'm in the process of reviewing some old issues... this one was a good candidate for 2.x. Here's what I did, it should correspond to what emerged in the comments: $invalid => triggers invalid ref $nullvalue => silent $valid.nullProp => silent $valid.nullMethod => silent $valid.invalid => triggers invalid ref That is, null is now always considered a legitimate final value. For #if / #elseif : #if($invalid) => silent #if($valid.invalid) => silent #if($invalid.prop) => triggers invalid ref #if($valid.invalid.prop) => triggers invalid ref Tests that trigger the invalid reference can all be rewritten so that they don't: #if($invalid && $invalid.prop) => silent #if($valid && $valid.invalid && $valid.invalid.prop) => silent Thanks to Renato Steiner for his test case patch.
        Hide
        Sergiu Dumitriu added a comment -

        What about the $!silent notation? Will all of them be silently ignored?

        Show
        Sergiu Dumitriu added a comment - What about the $!silent notation? Will all of them be silently ignored?
        Hide
        Nathan Bubna added a comment -

        Good question. My instinct is to say yes, silent is silent and give that control to the template instead of letting the configuration override it. But i can see arguments either way.

        Show
        Nathan Bubna added a comment - Good question. My instinct is to say yes, silent is silent and give that control to the template instead of letting the configuration override it. But i can see arguments either way.
        Hide
        Claude Brisson added a comment -

        I agree, the quiet references should never trigger an invalid reference event.

        Show
        Claude Brisson added a comment - I agree, the quiet references should never trigger an invalid reference event.
        Hide
        Claude Brisson added a comment -

        Commited. Now quiet references never trigger any invalid reference event.

        By the way, I also checked what happened in strict mode. I noticed that $!invalid will throw an exception. Is it intended?

        Show
        Claude Brisson added a comment - Commited. Now quiet references never trigger any invalid reference event. By the way, I also checked what happened in strict mode. I noticed that $!invalid will throw an exception. Is it intended?
        Hide
        Nathan Bubna added a comment -

        Yes, that one i'm sure about. Strict mode should act more like strict typing and never be generous about invalid refs, only null ones.

        Show
        Nathan Bubna added a comment - Yes, that one i'm sure about. Strict mode should act more like strict typing and never be generous about invalid refs, only null ones.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development