Velocity
  1. Velocity
  2. VELOCITY-254

in #foreach: if nulls are encountered last value is returned instead of null

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: None
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: PC

      Description

      I put an object having a method that returns an array of Integers in the context
      with contents
      [100,100,NULL,NULL,...]
      Then looping over the contents using
      #foreach ($elt in $Object.List) $!elt #end

      ALL entries become 100 with output:
      100 100 100 100 (...)
      instead of the expected
      100 100

      When converting all null objects to say new Integer(0) in the
      getList() body before returning the list, output becomes
      100 100 0 0 0 0 0

      Apparently null values are not treated as first citizens,

      Cheers,

      Indra

        Activity

        Hide
        Gernot Hüller added a comment -

        looks like another duplicate of bug #20999

        Show
        Gernot Hüller added a comment - looks like another duplicate of bug #20999
        Hide
        Shinobu Kawai added a comment -

        Workaround #1 : Use IteratorTool.
        Assuming that IteratorTool is in the Context as $mill,

        #set (#list = $mill.wrap($Object.List))
        #foreach ($elt in $list)
        $!list.more()
        #end

        This will work so long as you only use the element once per loop.

        Workaround #2 : Use NullTool as well.
        Assuming that IteratorTool is in the Context as $mill,
        NullTool is in the Context as $null
        and the Context itself in the Context as $ctx,

        #set (#list = $mill.wrap($Object.List))
        #foreach ($elt in $list)
        $null.set($ctx, "elt", list.more())
        $!elt
        $!elt again
        #end

        This will work all the time.

        http://jakarta.apache.org/velocity/tools/javadoc/org/apache/velocity/tools/gener
        ic/IteratorTool.html
        http://wiki.apache.org/jakarta-velocity/NullTool

        Show
        Shinobu Kawai added a comment - Workaround #1 : Use IteratorTool. Assuming that IteratorTool is in the Context as $mill, #set (#list = $mill.wrap($Object.List)) #foreach ($elt in $list) $!list.more() #end This will work so long as you only use the element once per loop. Workaround #2 : Use NullTool as well. Assuming that IteratorTool is in the Context as $mill, NullTool is in the Context as $null and the Context itself in the Context as $ctx, #set (#list = $mill.wrap($Object.List)) #foreach ($elt in $list) $null.set($ctx, "elt", list.more()) $!elt $!elt again #end This will work all the time. http://jakarta.apache.org/velocity/tools/javadoc/org/apache/velocity/tools/gener ic/IteratorTool.html http://wiki.apache.org/jakarta-velocity/NullTool
        Hide
        Shinobu Kawai added a comment -

        Workaround #3 : Use NullTool.
        Assuming that NullTool is in the Context as $null
        and the Context itself in the Context as $ctx,

        #foreach ($elt in $Object.List)
        $!elt
        $!elt again
        $null.setNull($ctx, "elt")
        #end

        This is a lot simpler than #2.

        Show
        Shinobu Kawai added a comment - Workaround #3 : Use NullTool. Assuming that NullTool is in the Context as $null and the Context itself in the Context as $ctx, #foreach ($elt in $Object.List) $!elt $!elt again $null.setNull($ctx, "elt") #end This is a lot simpler than #2.
        Hide
        Shinobu Kawai added a comment -

        Created an attachment (id=13665)
        Patch to allow null in #foreach

        Add
        directive.foreach.null.allowed=true
        in your velocity.properties to make it work.

        Show
        Shinobu Kawai added a comment - Created an attachment (id=13665) Patch to allow null in #foreach Add directive.foreach.null.allowed=true in your velocity.properties to make it work.
        Hide
        cawreck@gmx.net added a comment -

        The historical source of why velocity does not handle nulls:
        Originally vel started off with the posibility of using a simple Hashtable
        as the context - which does not allow nulls.

        a) If we where allowed to set null into the context - no problem.

        b) We could also wrap the user context into a chained one with only
        the #foreach var - which does allow setting it to null.
        -> This could raise some BC problem, since after the #foreach loop
        the original context may be restored, but the application wants
        to see the last value we had in the loop.

        c) When setting null into the map value is not allowed, removing the key
        could yield the desired result.
        -> but beware... if the context was a chained context, the parent
        key could suddendly show - ugh.
        -> also beware that the parent context could be read-only and thus
        may not be altered...

        d) The best solution is to have a pre-made key=null context at hand in the
        foreach directive instance to overlay it as a chained context every
        time a null is encountered. Wihtin the #foreach all non-null calls
        should be delegated to the user context; setting the #foreach var
        to non-null should also disable the key=null chained context.

        Hope this helps to understand the whole problem.

        I hope the coding of solution d) is easy as it could be made as an inner
        class to the #foreach directive and could be lazily instantiated upon
        the first use.

        The solution d) is not possible for a #set directive, since it does not
        have a body and thus it is not possible to overlay a chained context.
        So the pittfalls of c) come to show...

        Show
        cawreck@gmx.net added a comment - The historical source of why velocity does not handle nulls: Originally vel started off with the posibility of using a simple Hashtable as the context - which does not allow nulls. a) If we where allowed to set null into the context - no problem. b) We could also wrap the user context into a chained one with only the #foreach var - which does allow setting it to null. -> This could raise some BC problem, since after the #foreach loop the original context may be restored, but the application wants to see the last value we had in the loop. c) When setting null into the map value is not allowed, removing the key could yield the desired result. -> but beware... if the context was a chained context, the parent key could suddendly show - ugh. -> also beware that the parent context could be read-only and thus may not be altered... d) The best solution is to have a pre-made key=null context at hand in the foreach directive instance to overlay it as a chained context every time a null is encountered. Wihtin the #foreach all non-null calls should be delegated to the user context; setting the #foreach var to non-null should also disable the key=null chained context. Hope this helps to understand the whole problem. I hope the coding of solution d) is easy as it could be made as an inner class to the #foreach directive and could be lazily instantiated upon the first use. The solution d) is not possible for a #set directive, since it does not have a body and thus it is not possible to overlay a chained context. So the pittfalls of c) come to show...
        Hide
        cawreck@gmx.net added a comment -

        Created an attachment (id=13794)
        Failsafe and BC patch to handle null in #foreach

        Patch against vel 1.4-rc1 (sorry, no SVN yet installed...)

        Show
        cawreck@gmx.net added a comment - Created an attachment (id=13794) Failsafe and BC patch to handle null in #foreach Patch against vel 1.4-rc1 (sorry, no SVN yet installed...)
        Hide
        Claude Brisson added a comment -

        > This [the chained context implementation choice] could raise some BC problem,
        > since after the #foreach loop
        > the original context may be restored, but the application wants
        > to see the last value we had in the loop.

        very, very few BC problems : It's by far not worth the complexity introduced to
        keep BC for the 0.00001 % of the templates relaying on this behaviour.

        Plus, restraining the scope of the loop variable to the foreach block looks like
        quite natural.

        So IMO a simple chained context (solution b) is enough.

        Just a thought...

        Show
        Claude Brisson added a comment - > This [the chained context implementation choice] could raise some BC problem, > since after the #foreach loop > the original context may be restored, but the application wants > to see the last value we had in the loop. very, very few BC problems : It's by far not worth the complexity introduced to keep BC for the 0.00001 % of the templates relaying on this behaviour. Plus, restraining the scope of the loop variable to the foreach block looks like quite natural. So IMO a simple chained context (solution b) is enough. Just a thought...
        Hide
        Will Glass-Husain added a comment -

        Thanks for the detailed explanation of the alternative approaches. To sum
        up then, $foreach puts the iterating value into the current context. Since
        we can't assume that the current context will allow a value of null, this
        approach is not safe.

        The negative part of this idea is that the page will not be able to access the
        final value of the $foreach variable after the loop is ended. Maybe we can do
        a poll, see if any users depend on this.

        Show
        Will Glass-Husain added a comment - Thanks for the detailed explanation of the alternative approaches. To sum up then, $foreach puts the iterating value into the current context. Since we can't assume that the current context will allow a value of null, this approach is not safe. The negative part of this idea is that the page will not be able to access the final value of the $foreach variable after the loop is ended. Maybe we can do a poll, see if any users depend on this.
        Hide
        Will Glass-Husain added a comment -

        Committed patch. An elegant solution to this issue. (note: did not compile
        as included). Created unit tests to verify behavior. Appears to be fully
        backwards compatible. revision #125251

        Show
        Will Glass-Husain added a comment - Committed patch. An elegant solution to this issue. (note: did not compile as included). Created unit tests to verify behavior. Appears to be fully backwards compatible. revision #125251
        Hide
        Henning Schmiedehausen added a comment -

        Close resolved issues for Release 1.5

        Show
        Henning Schmiedehausen added a comment - Close resolved issues for Release 1.5

          People

          • Assignee:
            Unassigned
            Reporter:
            Indra Polak
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development