175 Velocity path delimiter bug

elda-lda/src/main/java/com/epimorphics/lda/Version.java

@@ -17,5 +17,5 @@
  *
  */
 public interface Version {
-    public final String string = "3.0.3";
+    public final String string = "3.0.4-SNAPSHOT";
 }

elda-lda/src/main/java/com/epimorphics/lda/renderers/velocity/VelocityRendering.java

@@ -39,6 +39,7 @@
 
 import java.io.*;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Properties;
 
@@ -62,7 +63,7 @@ public class VelocityRendering
     /**
      * The default place we look for Velocity files
      */
-    public static final String DEFAULT_VELOCITY_ROOT_PATH = "/velocity/";
+    public static final String DEFAULT_VELOCITY_ROOT_PATH = "velocity/";
 
     /**
      * The configuration parameter which sets an alternative location for Velocity templates etc
@@ -205,7 +206,7 @@ protected void render(OutputStream os) {
      * @return A new Velocity engine
      */
     public VelocityEngine createVelocityEngine() {
-        List<String> velocityPath = expandVelocityPath(bindings);
+        List<String> velocityPath = expandVelocityPath(bindings, System.getenv(VELOCITY_PATH_ENV_VAR));
 
         Properties p = getProperties(velocityPath);
         VelocityEngine ve = new VelocityEngine();
@@ -232,51 +233,54 @@ public VelocityEngine createVelocityEngine() {
      *
      * @return An array of expanded file paths or other URLs where we will search for Velocity assets
      */
-    protected List<String> expandVelocityPath(Bindings b) {
+    public static List<String> expandVelocityPath(Bindings b, String envRoot) {
         List<String> roots = new ArrayList<>();
-        String userRootPath = b.getAsString(VELOCITY_PATH_CONFIG_PARAM, null);
-
-        String rootPath =
-                (userRootPath == null ? "" : userRootPath + ",")
-                        + etcPath()
-                        + webappPath()
-                        + defaultVelocityRoot();
-
-        for (String pathEntry : StringUtils.split(rootPath, ",")) {
-            pathEntry = StringUtils.trim(pathEntry);
-            String pathURL = b.pathAsURL(pathEntry).toString();
-            roots.add(pathURL + (pathURL.endsWith("/") ? "" : "/"));
+        String velocityPath = b.getAsString(VELOCITY_PATH_CONFIG_PARAM, null);
+
+        List<String> rawRoots = new ArrayList<>();
+        if (velocityPath != null) {
+            Collections.addAll(rawRoots, StringUtils.split(velocityPath, ","));
+        }
+        rawRoots.add(etcPath(b));
+        rawRoots.addAll(defaultVelocityRoots(envRoot));
+
+        for (String rawRoot : rawRoots) {
+            rawRoot = StringUtils.trim(rawRoot);
+            if (!rawRoot.isEmpty()) {
+                String pathURL = b.pathAsURL(rawRoot).toString();
+                roots.add(pathURL + (pathURL.endsWith("/") ? "" : "/"));
+            }
         }
-        log.debug("rootPath '{}'", rootPath);
+        log.debug("rootPath '{}'", String.join(",", rawRoots));
         log.debug("complete expanded path '{}'", roots);
         return roots;
     }
 
-    private String webappPath() {
+    private static String webappPath() {
         return "_error_pages/";
     }
 
-    private String etcPath() {
+    private static String etcPath(Bindings bindings) {
         String context = bindings
                 .getAsString("_rootPath", "NO_ROOTPATH")
                 .replaceAll("/([^/]*)/.*", "$1");
         String appPath = "/etc/elda/conf.d/REPLACE/_error_pages".replace("REPLACE", context);
         List<File> files = new Glob().filesMatching(appPath);
-        return (files.size() == 0 ? "" : files.get(0)) + ",";
+        return (files.isEmpty() ? "" : files.getFirst().toString());
     }
 
     /**
      * @return The default Velocity root directory, which may set by an environment variable
      */
-    protected String defaultVelocityRoot() {
-        String envRoot = null;
-        try {
-            envRoot = System.getenv(VELOCITY_PATH_ENV_VAR);
-        } catch (SecurityException ignore) {
-            // not allowed to read the environment, no biggie
+    protected static List<String> defaultVelocityRoots(String envRoot) {
+        List<String> roots = new ArrayList<>();
+        if (envRoot == null) {
+            roots.add(webappPath() + DEFAULT_VELOCITY_ROOT_PATH);
+        } else {
+            Collections.addAll(roots, envRoot.split(","));
         }
 
-        return (envRoot == null) ? DEFAULT_VELOCITY_ROOT_PATH : envRoot;
+        return roots;
     }
 
     /**

elda-lda/src/main/java/com/epimorphics/lda/restlets/RouterRestlet.java

@@ -595,7 +595,7 @@ public static URI makeRequestURI(UriInfo ui, Boolean enableForwardHeaders, Strin
 
     private static final String STARTS_WITH_SCHEME = "^" + MATCHES_SCHEME + ".*";
 
-    private static URLforResource pathAsURLFactory(final ServletContext servCon) {
+    public static URLforResource pathAsURLFactory(final ServletContext servCon) {
         return new URLforResource() {
             @Override
             public URL asResourceURL(String ePath) {

elda-lda/src/test/java/com/epimorphics/lda/renderers/velocity/VelocityRenderingTest.java

@@ -0,0 +1,99 @@
+package com.epimorphics.lda.renderers.velocity;
+
+import com.epimorphics.lda.bindings.Bindings;
+import org.junit.Test;
+
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.List;
+
+import static org.junit.Assert.*;
+
+public class VelocityRenderingTest {
+
+    @Test
+    public void expandVelocityPath_default() {
+        List<String> result = new VelocityPathScenario().run();
+        List<String> expected = List.of("file:_error_pages/velocity/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withVelocityPath_singleValue() {
+        List<String> result = new VelocityPathScenario().withVelocityPath("elda/test").run();
+        List<String> expected = List.of("file:elda/test/", "file:_error_pages/velocity/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withVelocityPath_multipleValues() {
+        List<String> result = new VelocityPathScenario().withVelocityPath("elda/test,/var/velocity").run();
+        List<String> expected = List.of("file:elda/test/", "file:/var/velocity/", "file:_error_pages/velocity/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withVelocityPath_multipleValues_withWhitespace() {
+        List<String> result = new VelocityPathScenario().withVelocityPath("elda/test , /var/velocity, elda/void ").run();
+        List<String> expected = List.of("file:elda/test/", "file:/var/velocity/", "file:elda/void/", "file:_error_pages/velocity/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withVelocityPath_multipleValues_withEmpty() {
+        List<String> result = new VelocityPathScenario().withVelocityPath(", elda/test,,/var/velocity, ,elda/void").run();
+        List<String> expected = List.of("file:elda/test/", "file:/var/velocity/", "file:elda/void/", "file:_error_pages/velocity/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withEnvVar_singleValue() {
+        List<String> result = new VelocityPathScenario().withEnvVar("elda/test").run();
+        List<String> expected = List.of("file:elda/test/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withEnvVar_multipleValues() {
+        List<String> result = new VelocityPathScenario().withEnvVar("elda/test,/var/velocity").run();
+        List<String> expected = List.of("file:elda/test/", "file:/var/velocity/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withEnvVar_multipleValues_withWhitespace() {
+        List<String> result = new VelocityPathScenario().withEnvVar("elda/test , /var/velocity, elda/void ").run();
+        List<String> expected = List.of("file:elda/test/", "file:/var/velocity/", "file:elda/void/");
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void expandVelocityPath_withEnvVar_multipleValues_withEmpty() {
+        List<String> result = new VelocityPathScenario().withEnvVar(", elda/test,,/var/velocity, ,elda/void").run();
+        List<String> expected = List.of("file:elda/test/", "file:/var/velocity/", "file:elda/void/");
+        assertEquals(expected, result);
+    }
+
+    private class VelocityPathScenario {
+        private Bindings b = new Bindings(u -> {
+            try {
+                return new URL("file:" + u);
+            } catch (MalformedURLException e) {
+                throw new RuntimeException(e);
+            }
+        });
+        private String envRoot;
+
+        public VelocityPathScenario withVelocityPath(String value) {
+            b.put("_velocityPath", value); return this;
+        }
+
+        public VelocityPathScenario withEnvVar(String value) {
+            envRoot = value; return this;
+        }
+
+        public List<String> run() {
+            return VelocityRendering.expandVelocityPath(b, envRoot);
+        }
+    }
+}