Velocity
  1. Velocity
  2. VELOCITY-742

Easily Add/Remove/Replace Directives Without Cracking Open Velocity Jar

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.6.2
    • Fix Version/s: 1.7
    • Component/s: Engine
    • Labels:
      None

      Description

      There are two use cases that I have for this issue:

      1) We need to turn off #include in order to parse .html files that include SSI directives: <!-#include virtual=""-> (we always use #parse in our world, so no loss for #include going away)

      2) We need to supply our own version of #parse that contains the development hooks that I described in a different issue.

      Right now, the only crisp way to do is is to crack open the velocity .jar and make these changes, which is very risky, because future developers may upgrade to new versions of Velocity and not realize that there are customizations and/or not understand how to remake them....not to mention in a shared-classpath environment, we don't always have control over which copy of the Velocity .jar we're using.

      Ideally, there would be config options like this:

      directive_remove ("#parse", "#include")
      directive_add ("#parse", "com.qwest.velocity.directives.Parse")

      Thanks!

      Tim

      1. velocity-742.patch
        10 kB
        Jarkko Viinamäki

        Activity

        Tim White created issue -
        Hide
        Jarkko Viinamäki added a comment -

        OK. This patch now provides two new public methods in VelocityEngine and Velocity classes:

        removeDirective(String name)
        loadDirective(String className)

        Note that all parser instances use the same set of directives so it's not possible to disable e.g. #include directive for certain page rendering but still keep it active for other possibly concurrent page renderings. Note also that you need to first call init on Velocity or VelocityEngine before you start removing directives (otherwise the autoinit "overwrites" the removal operation). See the Velocity742TestCase.

        Note! After applying the patch, you need to run "ant parser test" in order to update the generated Parser.java and other related classes.

        I also noticed that there was a hidden bug in the code: RuntimeInstance keeps a map of directives in variable called runtimeDirectives. When a new parser was created that variable was passed to the Parser but the parser made a HashMap copy of the runtimeDirectives (this was one of the speed improvements since it slows things down if many parsers hammer the same synchronized map).

        Therefore when addDirective, removeDirective etc. were called in RuntimeInstance, they had no effect since all the created parsers still had a copy of the old runtimeDirectives map.

        I fixed it so that Parser instances do not have a copy of the directives map at all. Instead, they redirect e.g. getDirective and isDirective calls to RuntimeInstance which uses unsynchronized HashMap for the lookups. When the runtimeDirectives map is modified (new directive is added or an old one is deleted), a new HashMap is created as a copy and the reference is updated so that subsequent calls to RuntimeInstance.getDirective use the newly created map (Copy-on-Write pattern I guess).

        Nathan & others, please take a look at this when you have time and check if I did something really silly.

        Merry Christmas!

        Show
        Jarkko Viinamäki added a comment - OK. This patch now provides two new public methods in VelocityEngine and Velocity classes: removeDirective(String name) loadDirective(String className) Note that all parser instances use the same set of directives so it's not possible to disable e.g. #include directive for certain page rendering but still keep it active for other possibly concurrent page renderings. Note also that you need to first call init on Velocity or VelocityEngine before you start removing directives (otherwise the autoinit "overwrites" the removal operation). See the Velocity742TestCase. Note! After applying the patch, you need to run "ant parser test" in order to update the generated Parser.java and other related classes. – I also noticed that there was a hidden bug in the code: RuntimeInstance keeps a map of directives in variable called runtimeDirectives. When a new parser was created that variable was passed to the Parser but the parser made a HashMap copy of the runtimeDirectives (this was one of the speed improvements since it slows things down if many parsers hammer the same synchronized map). Therefore when addDirective, removeDirective etc. were called in RuntimeInstance, they had no effect since all the created parsers still had a copy of the old runtimeDirectives map. I fixed it so that Parser instances do not have a copy of the directives map at all. Instead, they redirect e.g. getDirective and isDirective calls to RuntimeInstance which uses unsynchronized HashMap for the lookups. When the runtimeDirectives map is modified (new directive is added or an old one is deleted), a new HashMap is created as a copy and the reference is updated so that subsequent calls to RuntimeInstance.getDirective use the newly created map (Copy-on-Write pattern I guess). Nathan & others, please take a look at this when you have time and check if I did something really silly. Merry Christmas!
        Jarkko Viinamäki made changes -
        Field Original Value New Value
        Attachment velocity-742.patch [ 12428804 ]
        Hide
        Nathan Bubna added a comment -

        It all seems reasonable to me. I imagine there's potential for some unpredictability in multi-thread situations where one thread removes/loads a directive while a parser in another thread is getting a directive. But i think that's acceptable. Ideally people would make very light use of the remove/load API, and then only when setting things up.

        I'm going to go ahead and commit it.

        Show
        Nathan Bubna added a comment - It all seems reasonable to me. I imagine there's potential for some unpredictability in multi-thread situations where one thread removes/loads a directive while a parser in another thread is getting a directive. But i think that's acceptable. Ideally people would make very light use of the remove/load API, and then only when setting things up. I'm going to go ahead and commit it.
        Hide
        Nathan Bubna added a comment -

        Ok, it's in the trunk. I'll try and port today's changes to 2.x also.

        Show
        Nathan Bubna added a comment - Ok, it's in the trunk. I'll try and port today's changes to 2.x also.
        Nathan Bubna made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 1.7 [ 12313453 ]
        Resolution Fixed [ 1 ]
        Mark Thomas made changes -
        Workflow jira [ 12481389 ] Default workflow, editable Closed status [ 12551650 ]
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12551650 ] jira [ 12552528 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Tim White
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development