Click
  1. Click
  2. CLK-606

Remove Click core's dependency on Velocity

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.0, 2.1.0 RC1
    • Fix Version/s: 2.2.0
    • Component/s: core
    • Labels:
      None
    • Environment:
      click-nodeps-2.1.0-RC1-incubating.jar , click-extras-2.1.0-RC1-incubating.jar, ognl-2.6.9.jar, freemarket2.3.16.jar

      Description

      I use freemarker and dependencies in separate jars.
      I haven't 'velocity' in my classpath.
      So I found bug: common click core depends on Velocity.

      Namely:
      org.apache.click.ClickServlet.java
      org.apache.click.util.ErrorReport.java

      They both depend on
      org.apache.velocity.exception.ParseErrorException (search:
      instanceof ParseErrorException)
      and require Velocity be present in classpath even if freemarker are used.

      My workaround:
      I made fake public class ParseErrorException extends Exception {}.

      But you can make generic solution, for example:

      TemplateService
      + boolean isParseErrorException (Exception e)
      + Map<String, Object> describeParseErrorException (Exception e)

      or your own ClickTemplateException to wrap low level velocity/freemarker exceptions.

        Activity

        Hide
        Bob Schellink added a comment -

        everything looks good

        Show
        Bob Schellink added a comment - everything looks good
        Hide
        Malcolm Edgar added a comment -

        Good question, I am not sure as I can see arguments for both. I suppose the suggested change takes you to the error quicker, so its probably a good change.

        Show
        Malcolm Edgar added a comment - Good question, I am not sure as I can see arguments for both. I suppose the suggested change takes you to the error quicker, so its probably a good change.
        Hide
        Bob Schellink added a comment -

        Thanks Malcolm, the latest checkin looks good. I'm wondering if we should only print the cause and not the template exception trace. By changing TemplateException to this:

        public class TemplateException {
        ...

        @Override
        public void printStackTrace (java.io.PrintStream s) {
        synchronized(s) {
        if(getCause() == this)

        { super.printStackTrace(s); } else { getCause().printStackTrace(s); }
        }
        }

        @Override
        public void printStackTrace (java.io.PrintWriter s) {
        synchronized(s) {
        if(getCause() == this) { super.printStackTrace(s); }

        else

        { getCause().printStackTrace(s); }

        }
        }

        @Override
        public Throwable fillInStackTrace () {
        if(getCause() == this)

        { return super.fillInStackTrace (); }

        return this;
        }
        }

        The stackTrace goes from this:

        [Click] [error] handleException: org.apache.click.service.TemplateException: java.lang.NullPointerException
        at org.apache.click.service.VelocityTemplateService.internalRenderTemplate(VelocityTemplateService.java:621)
        at org.apache.click.service.VelocityTemplateService.renderTemplate(VelocityTemplateService.java:331)
        at org.apache.click.ClickServlet.renderTemplate(ClickServlet.java:842)
        at org.apache.click.ClickServlet.performRender(ClickServlet.java:800)
        at org.apache.click.ClickServlet.processPage(ClickServlet.java:566)
        at org.apache.click.ClickServlet.handleRequest(ClickServlet.java:374)
        ...
        Caused by: java.lang.NullPointerException
        at org.apache.click.examples.page.general.ExceptionPage$BrokenRenderer.toString(ExceptionPage.java:101)
        at org.apache.velocity.runtime.parser.node.ASTReference.render(ASTReference.java:393)
        at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:336)
        at org.apache.velocity.runtime.directive.Parse.render(Parse.java:260)

        to this:

        [Click] [error] handleException: java.lang.NullPointerException
        at org.apache.click.examples.page.general.ExceptionPage$BrokenRenderer.toString(ExceptionPage.java:101)
        at org.apache.velocity.runtime.parser.node.ASTReference.render(ASTReference.java:393)
        at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:336)
        at org.apache.velocity.runtime.directive.Parse.render(Parse.java:260)

        Show
        Bob Schellink added a comment - Thanks Malcolm, the latest checkin looks good. I'm wondering if we should only print the cause and not the template exception trace. By changing TemplateException to this: public class TemplateException { ... @Override public void printStackTrace (java.io.PrintStream s) { synchronized(s) { if(getCause() == this) { super.printStackTrace(s); } else { getCause().printStackTrace(s); } } } @Override public void printStackTrace (java.io.PrintWriter s) { synchronized(s) { if(getCause() == this) { super.printStackTrace(s); } else { getCause().printStackTrace(s); } } } @Override public Throwable fillInStackTrace () { if(getCause() == this) { return super.fillInStackTrace (); } return this; } } The stackTrace goes from this: [Click] [error] handleException: org.apache.click.service.TemplateException: java.lang.NullPointerException at org.apache.click.service.VelocityTemplateService.internalRenderTemplate(VelocityTemplateService.java:621) at org.apache.click.service.VelocityTemplateService.renderTemplate(VelocityTemplateService.java:331) at org.apache.click.ClickServlet.renderTemplate(ClickServlet.java:842) at org.apache.click.ClickServlet.performRender(ClickServlet.java:800) at org.apache.click.ClickServlet.processPage(ClickServlet.java:566) at org.apache.click.ClickServlet.handleRequest(ClickServlet.java:374) ... Caused by: java.lang.NullPointerException at org.apache.click.examples.page.general.ExceptionPage$BrokenRenderer.toString(ExceptionPage.java:101) at org.apache.velocity.runtime.parser.node.ASTReference.render(ASTReference.java:393) at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:336) at org.apache.velocity.runtime.directive.Parse.render(Parse.java:260) to this: [Click] [error] handleException: java.lang.NullPointerException at org.apache.click.examples.page.general.ExceptionPage$BrokenRenderer.toString(ExceptionPage.java:101) at org.apache.velocity.runtime.parser.node.ASTReference.render(ASTReference.java:393) at org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java:336) at org.apache.velocity.runtime.directive.Parse.render(Parse.java:260)
        Hide
        Bob Schellink added a comment -

        One issue I've stubled upon is that TemplateExceptions are not logged to the LogService. In the previous implementation, logging was skipped only for Velocity's ParseException. As TemplateExcpetion encapsulate all Exception types, we should log this information.

        Show
        Bob Schellink added a comment - One issue I've stubled upon is that TemplateExceptions are not logged to the LogService. In the previous implementation, logging was skipped only for Velocity's ParseException. As TemplateExcpetion encapsulate all Exception types, we should log this information.
        Hide
        Malcolm Edgar added a comment -

        Fixed template exception handling in ErrorReport and cleaned up checkstyle issues

        Show
        Malcolm Edgar added a comment - Fixed template exception handling in ErrorReport and cleaned up checkstyle issues
        Hide
        Malcolm Edgar added a comment -

        ErrorReport handling needs further work. TemplateException is current not executing the correct code path when wrapping an exception, e.g. NPE

        Show
        Malcolm Edgar added a comment - ErrorReport handling needs further work. TemplateException is current not executing the correct code path when wrapping an exception, e.g. NPE
        Hide
        Malcolm Edgar added a comment -

        This change has been checked in, as well as fixes to Click Example demonstration.

        Show
        Malcolm Edgar added a comment - This change has been checked in, as well as fixes to Click Example demonstration.
        Hide
        Andrew Fink added a comment -

        2.1.0 affected too

        Show
        Andrew Fink added a comment - 2.1.0 affected too
        Hide
        Andrew Fink added a comment -

        >I think too that this is not a but but a feature.

        Nо. This is bug, mistake and hole in test coverage

        Actually, Click's Core doesn't depend on Velocity.
        This one class used absolutely by incident.

        It should be wrapped in Click own exception class (as Malcolm said).

        Show
        Andrew Fink added a comment - >I think too that this is not a but but a feature. Nо. This is bug, mistake and hole in test coverage Actually, Click's Core doesn't depend on Velocity. This one class used absolutely by incident. It should be wrapped in Click own exception class (as Malcolm said).
        Hide
        Malcolm Edgar added a comment -

        I agree we should probably work to remove this direct Velocity dependency from ClickServlet and ErrorReport.

        I think we need a TemplateException which can be used to wrap the Velocity ParseErrorException and provide the equivalent properties: templateName, lineNumber, columnNumber

        Anyone have a patch?

        Show
        Malcolm Edgar added a comment - I agree we should probably work to remove this direct Velocity dependency from ClickServlet and ErrorReport. I think we need a TemplateException which can be used to wrap the Velocity ParseErrorException and provide the equivalent properties: templateName, lineNumber, columnNumber Anyone have a patch?
        Hide
        Joseph Schmidt added a comment -

        I think too that this is not a but but a feature.

        From what I understood, Click supports several template languages:
        Velocity, JSP, FreeMarker, and it's possible to add new ones too quite simply.

        Click internally however and the official components too, use Velocity.
        The fact that Click supports FreeMarker or JSP too (or your own template) means that users
        can write their own pages with that - not that Click Core should be implemented in those too.

        E.g. JSP is present in Click since very very long ago (to allow the easy migration of Struts and JSP applications too), but still, no component is duplicated with JSP too.

        The Velocity JAR is quite small (and also included in click.jar), so there's no reason not to have it, and why the architecture of click should be made even more abstract and complicated.

        Show
        Joseph Schmidt added a comment - I think too that this is not a but but a feature. From what I understood, Click supports several template languages: Velocity, JSP, FreeMarker, and it's possible to add new ones too quite simply. Click internally however and the official components too, use Velocity. The fact that Click supports FreeMarker or JSP too (or your own template) means that users can write their own pages with that - not that Click Core should be implemented in those too. E.g. JSP is present in Click since very very long ago (to allow the easy migration of Struts and JSP applications too), but still, no component is duplicated with JSP too. The Velocity JAR is quite small (and also included in click.jar), so there's no reason not to have it, and why the architecture of click should be made even more abstract and complicated.

          People

          • Assignee:
            Malcolm Edgar
            Reporter:
            Andrew Fink
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development