Velocity
  1. Velocity
  2. VELOCITY-656

Wrap ref evaluate exception so vm location is revealed

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6, 1.6.1, 1.7
    • Fix Version/s: 1.7
    • Component/s: None
    • Labels:
      None

      Description

      The following VTL calls the toString() method on the object in $foo:

      #if($foo)#end

      If the toString() method throws an exception, the exception is thrown all the way out of velocity without any indication of where the vtl reference occurred. This patch wraps the exception with a VelocityException with a message that indicates the location of the offending reference.

      However, I wonder why toString() is called at all... the code tests for toString() == null and returns false if so, and true otherwise in the above example vtl. Why is this test necessary, and why isn't just a non-null object enough for testing?

      1. 656.patch
        0.9 kB
        Byron Foster

        Activity

        Hide
        Nathan Bubna added a comment -

        It's common Java practice that toString() should never return null or throw an exception. It is pretty rare that any software is prepared to handle either possibility. Velocity, however, has always been fine with returning null in toString() and never complained about such behavior. The Alternator class from VelocityTools even relies on this to alternate over sets including null values. I personally have used this often in my own development. The change to #if to test for toString() == null is to ensure that #if works with this paradigm.

        I'm not inclined to rebuild 1.6.1 again for this, and i'm hesitant about it in general. To me, an exception during a toString() call is a big no-no on the user's part. The stack trace they get should point them to the toString() call, which i think ought to be the thing they focus on fixing. The template/line/col of the guilty VTL reference might be helpful to log, but their problem is in Java, not their totally valid VTL. Wrapping the root exception in a VelocityException feels like it draws attention to the wrong code location.

        Show
        Nathan Bubna added a comment - It's common Java practice that toString() should never return null or throw an exception. It is pretty rare that any software is prepared to handle either possibility. Velocity, however, has always been fine with returning null in toString() and never complained about such behavior. The Alternator class from VelocityTools even relies on this to alternate over sets including null values. I personally have used this often in my own development. The change to #if to test for toString() == null is to ensure that #if works with this paradigm. I'm not inclined to rebuild 1.6.1 again for this, and i'm hesitant about it in general. To me, an exception during a toString() call is a big no-no on the user's part. The stack trace they get should point them to the toString() call, which i think ought to be the thing they focus on fixing. The template/line/col of the guilty VTL reference might be helpful to log, but their problem is in Java, not their totally valid VTL. Wrapping the root exception in a VelocityException feels like it draws attention to the wrong code location.
        Hide
        Byron Foster added a comment -

        This issue came up when referring to a Hibernate object wrapped with a proxy object which is a standard pattern that Hibernate uses for working with POJOs. If any methods are called on the proxy outside of the originating Hibernate session, then Hibernate throws the dreaded LazyIntializationException, this includes the toString() method. The stack trace in this case is deep within Hibernate.

        I guess I have two points, the first is that I think that Velocity should always report accurately where in a template a problem has occurred. The second, as far as I know, currently any error encountered while processing a template generates some sort of velocity exception, and the location of the problem within the template file is reported in the exception message. This patch was attempting to be consistent with that trend.

        My only concern with calling toString() is that on some object this may be non-trivial, and may be surprising behavior from #if($foo). But, it's probably not a big deal.

        Let me know if you think another approach would be better, and I'll provide a new patch. I don't think this has a high priority which is why I marked it as an improvement, so I don't think it's necessary for 1.6.1.

        Show
        Byron Foster added a comment - This issue came up when referring to a Hibernate object wrapped with a proxy object which is a standard pattern that Hibernate uses for working with POJOs. If any methods are called on the proxy outside of the originating Hibernate session, then Hibernate throws the dreaded LazyIntializationException, this includes the toString() method. The stack trace in this case is deep within Hibernate. I guess I have two points, the first is that I think that Velocity should always report accurately where in a template a problem has occurred. The second, as far as I know, currently any error encountered while processing a template generates some sort of velocity exception, and the location of the problem within the template file is reported in the exception message. This patch was attempting to be consistent with that trend. My only concern with calling toString() is that on some object this may be non-trivial, and may be surprising behavior from #if($foo). But, it's probably not a big deal. Let me know if you think another approach would be better, and I'll provide a new patch. I don't think this has a high priority which is why I marked it as an improvement, so I don't think it's necessary for 1.6.1.
        Hide
        Nathan Bubna added a comment -

        If it won't bother you to leave this out of 1.6.1, that's great, i'm rather weary of rebuilding it.
        Dropping Hibernate objects in the context is always scary, but i can't deny having done it myself.
        I still think it's terribly wrong that toString() ever throws an exception.
        My feeling was that logging this was better than wrapping it, but it's more of a -0 feeling than a -1 feeling.
        Let's leave this for 1.7.
        And i'll probably just let you commit it once we get your account set up (hopefully not too much longer).

        Show
        Nathan Bubna added a comment - If it won't bother you to leave this out of 1.6.1, that's great, i'm rather weary of rebuilding it. Dropping Hibernate objects in the context is always scary, but i can't deny having done it myself. I still think it's terribly wrong that toString() ever throws an exception. My feeling was that logging this was better than wrapping it, but it's more of a -0 feeling than a -1 feeling. Let's leave this for 1.7. And i'll probably just let you commit it once we get your account set up (hopefully not too much longer).
        Hide
        Claude Brisson added a comment -

        You might just drop Hibernate itself...

        Show
        Claude Brisson added a comment - You might just drop Hibernate itself...
        Hide
        Byron Foster added a comment -

        Setting aside Hibernate's suspicious claims to using POJO's, I think that actually my preferred solution is to remove the call to toString, but it sounds like that will meet resistance

        Show
        Byron Foster added a comment - Setting aside Hibernate's suspicious claims to using POJO's, I think that actually my preferred solution is to remove the call to toString, but it sounds like that will meet resistance
        Hide
        Nathan Bubna added a comment -

        Yeah, i think we should allow/expect toString() might return null, at least for 1.x. For 2.0, i've been musing about watching for a getAsString() when going to render/eval/whatever something. Then we can again expect toString() to never return null, and ask those objects who want special treatment to implement getAsString()

        Show
        Nathan Bubna added a comment - Yeah, i think we should allow/expect toString() might return null, at least for 1.x. For 2.0, i've been musing about watching for a getAsString() when going to render/eval/whatever something. Then we can again expect toString() to never return null, and ask those objects who want special treatment to implement getAsString()
        Hide
        Byron Foster added a comment -

        How about an interface much like TemplateNumber.java (which may be what you are referring to)

        interface TemplateEvaluate

        { public boolean evaluateBoolean() }

        evaluate would do an instanceOf and then test for evaluateBoolean instead of calling toString()?

        Classes like Alternator could implement this interface.

        Show
        Byron Foster added a comment - How about an interface much like TemplateNumber.java (which may be what you are referring to) interface TemplateEvaluate { public boolean evaluateBoolean() } evaluate would do an instanceOf and then test for evaluateBoolean instead of calling toString()? Classes like Alternator could implement this interface.
        Hide
        Nathan Bubna added a comment -

        No, no new interfaces. I'm not even much a fan of TemplateNumber. I prefer reflection and convention to avoid hard dependencies on Velocity APIs. Version compatibility is much easier on both sides when you test for the presence of the feature directly, rather than rely on interfaces. We're moving into 2.0 discussion again, but really, i would prefer to watch for methods like getAsNumber(), getAsBoolean(), and getAsString() for such things.

        In the 1.x frame, we must still allow/expect toString() to return null. Since we're looking at a 1.6.2 release, you might as well commit your try/catch exception wrapping for this into the trunk and merge it to the 1.6.x branch. My main reason for marking this as a 1.7 fix was that i didn't think it worth rebuilding 1.6.1 for this, and i didn't yet envision a 1.6.2.

        Show
        Nathan Bubna added a comment - No, no new interfaces. I'm not even much a fan of TemplateNumber. I prefer reflection and convention to avoid hard dependencies on Velocity APIs. Version compatibility is much easier on both sides when you test for the presence of the feature directly, rather than rely on interfaces. We're moving into 2.0 discussion again, but really, i would prefer to watch for methods like getAsNumber(), getAsBoolean(), and getAsString() for such things. In the 1.x frame, we must still allow/expect toString() to return null. Since we're looking at a 1.6.2 release, you might as well commit your try/catch exception wrapping for this into the trunk and merge it to the 1.6.x branch. My main reason for marking this as a 1.7 fix was that i didn't think it worth rebuilding 1.6.1 for this, and i didn't yet envision a 1.6.2.
        Hide
        Byron Foster added a comment -

        That's very Pythonic of you . Ok committed to trunk and 1.6.2

        Show
        Byron Foster added a comment - That's very Pythonic of you . Ok committed to trunk and 1.6.2
        Hide
        Nathan Bubna added a comment -

        Thanks!

        Show
        Nathan Bubna added a comment - Thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development