diff options
-rw-r--r-- | README.md | 3 | ||||
-rw-r--r-- | pom.xml | 2 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/DirectiveNode.java | 37 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/EvaluationContext.java | 16 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/EvaluationException.java | 2 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/Macro.java | 8 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/MethodFinder.java | 172 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/Node.java | 1 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/Parser.java | 122 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/ReferenceNode.java | 176 | ||||
-rw-r--r-- | src/main/java/com/google/escapevelocity/Template.java | 13 | ||||
-rw-r--r-- | src/test/java/com/google/escapevelocity/MethodFinderTest.java | 92 | ||||
-rw-r--r-- | src/test/java/com/google/escapevelocity/ReferenceNodeTest.java | 18 | ||||
-rw-r--r-- | src/test/java/com/google/escapevelocity/TemplateTest.java | 169 |
14 files changed, 600 insertions, 231 deletions
@@ -14,9 +14,6 @@ the exact same string that Velocity produces. If not, that is a bug. EscapeVelocity has no facilities for HTML escaping and it is not appropriate for producing HTML output that might include portions of untrusted input. -<!-- MOE:begin_strip --> -[TOC] -<!-- MOE:end_strip --> ## Motivation @@ -74,7 +74,7 @@ <dependency> <groupId>com.google.truth</groupId> <artifactId>truth</artifactId> - <version>0.36</version> + <version>0.44</version> <scope>test</scope> </dependency> </dependencies> diff --git a/src/main/java/com/google/escapevelocity/DirectiveNode.java b/src/main/java/com/google/escapevelocity/DirectiveNode.java index 2d4decc..fd0cd22 100644 --- a/src/main/java/com/google/escapevelocity/DirectiveNode.java +++ b/src/main/java/com/google/escapevelocity/DirectiveNode.java @@ -122,7 +122,7 @@ abstract class DirectiveNode extends Node { } Runnable undo = context.setVar(var, null); StringBuilder sb = new StringBuilder(); - Iterator<?> it = iterable.iterator(); + CountingIterator it = new CountingIterator(iterable.iterator()); Runnable undoForEach = context.setVar("foreach", new ForEachVar(it)); while (it.hasNext()) { context.setVar(var, it.next()); @@ -133,21 +133,50 @@ abstract class DirectiveNode extends Node { return sb.toString(); } + private static class CountingIterator implements Iterator<Object> { + private final Iterator<?> iterator; + private int index = -1; + + CountingIterator(Iterator<?> iterator) { + this.iterator = iterator; + } + + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public Object next() { + Object next = iterator.next(); + index++; + return next; + } + + int index() { + return index; + } + } + /** * This class is the type of the variable {@code $foreach} that is defined within * {@code #foreach} loops. Its {@link #getHasNext()} method means that we can write - * {@code #if ($foreach.hasNext)}. + * {@code #if ($foreach.hasNext)} and likewise for {@link #getIndex()}. */ private static class ForEachVar { - private final Iterator<?> iterator; + private final CountingIterator iterator; - ForEachVar(Iterator<?> iterator) { + ForEachVar(CountingIterator iterator) { this.iterator = iterator; } public boolean getHasNext() { return iterator.hasNext(); } + + public int getIndex() { + return iterator.index(); + } } } diff --git a/src/main/java/com/google/escapevelocity/EvaluationContext.java b/src/main/java/com/google/escapevelocity/EvaluationContext.java index d2f2914..d40b717 100644 --- a/src/main/java/com/google/escapevelocity/EvaluationContext.java +++ b/src/main/java/com/google/escapevelocity/EvaluationContext.java @@ -15,6 +15,8 @@ */ package com.google.escapevelocity; +import com.google.common.collect.ImmutableSet; +import java.lang.reflect.Method; import java.util.Map; import java.util.TreeMap; @@ -40,11 +42,16 @@ interface EvaluationContext { */ Runnable setVar(final String var, Object value); + /** See {@link MethodFinder#publicMethodsWithName}. */ + ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name); + class PlainEvaluationContext implements EvaluationContext { private final Map<String, Object> vars; + private final MethodFinder methodFinder; - PlainEvaluationContext(Map<String, ?> vars) { - this.vars = new TreeMap<String, Object>(vars); + PlainEvaluationContext(Map<String, ?> vars, MethodFinder methodFinder) { + this.vars = new TreeMap<>(vars); + this.methodFinder = methodFinder; } @Override @@ -69,5 +76,10 @@ interface EvaluationContext { vars.put(var, value); return undo; } + + @Override + public ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name) { + return methodFinder.publicMethodsWithName(startClass, name); + } } } diff --git a/src/main/java/com/google/escapevelocity/EvaluationException.java b/src/main/java/com/google/escapevelocity/EvaluationException.java index 2457d0a..a1f25a4 100644 --- a/src/main/java/com/google/escapevelocity/EvaluationException.java +++ b/src/main/java/com/google/escapevelocity/EvaluationException.java @@ -29,6 +29,6 @@ public class EvaluationException extends RuntimeException { } EvaluationException(String message, Throwable cause) { - super(cause); + super(message, cause); } } diff --git a/src/main/java/com/google/escapevelocity/Macro.java b/src/main/java/com/google/escapevelocity/Macro.java index b7a547f..afa7bf0 100644 --- a/src/main/java/com/google/escapevelocity/Macro.java +++ b/src/main/java/com/google/escapevelocity/Macro.java @@ -17,11 +17,12 @@ package com.google.escapevelocity; import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import java.lang.reflect.Method; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; - /** * A macro definition. Macros appear in templates using the syntax {@code #macro (m $x $y) ... #end} * and each one produces an instance of this class. Evaluating a macro involves setting the @@ -131,5 +132,10 @@ class Macro { }; } } + + @Override + public ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name) { + return originalEvaluationContext.publicMethodsWithName(startClass, name); + } } } diff --git a/src/main/java/com/google/escapevelocity/MethodFinder.java b/src/main/java/com/google/escapevelocity/MethodFinder.java new file mode 100644 index 0000000..f8f91f5 --- /dev/null +++ b/src/main/java/com/google/escapevelocity/MethodFinder.java @@ -0,0 +1,172 @@ +/* + * Copyright (C) 2019 Google, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.escapevelocity; + +import static com.google.common.reflect.Reflection.getPackageName; +import static java.util.stream.Collectors.toSet; + +import com.google.common.collect.HashBasedTable; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Table; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.Objects; +import java.util.Set; + +/** + * Finds public methods in a class. For each one, it determines the public class or interface in + * which it is declared. This avoids a problem with reflection, where we get an exception if we call + * a {@code Method} in a non-public class, even if the {@code Method} is public and if there is a + * public ancestor class or interface that declares it. We need to use the {@code Method} from the + * public ancestor. + * + * <p>Because looking for these methods is relatively expensive, an instance of this class will keep + * a cache of methods it previously discovered. + */ +class MethodFinder { + + /** + * For a given class and name, returns all public methods of that name in the class, as previously + * determined by {@link #publicMethodsWithName}. The set of methods for a given class and name is + * saved the first time it is searched for, and returned directly thereafter. It may be empty. + * + * <p>Currently we add the entry for any given (class, name) pair on demand. An alternative would + * be to add all the methods for a given class at once. With the current scheme, we may end up + * calling {@link Class#getMethods()} several times for the same class, if methods of the + * different names are called at different times. With an all-at-once scheme, we might end up + * computing and storing information about a bunch of methods that will never be called. Because + * the profiling that led to the creation of this class revealed that {@link #visibleMethods} in + * particular is quite expensive, it's probably best to avoid calling it unnecessarily. + */ + private final Table<Class<?>, String, ImmutableSet<Method>> methodCache = HashBasedTable.create(); + + /** + * Returns the set of public methods with the given name in the given class. Here, "public + * methods" means public methods in public classes or interfaces. If {@code startClass} is not + * itself public, its methods are effectively not public either, but inherited methods may still + * appear in the returned set, with the {@code Method} objects belonging to public ancestors. More + * than one ancestor may define an appropriate method, but it doesn't matter because invoking any + * of those {@code Method} objects will have the same effect. + */ + synchronized ImmutableSet<Method> publicMethodsWithName(Class<?> startClass, String name) { + ImmutableSet<Method> cachedMethods = methodCache.get(startClass, name); + if (cachedMethods == null) { + cachedMethods = uncachedPublicMethodsWithName(startClass, name); + methodCache.put(startClass, name, cachedMethods); + } + return cachedMethods; + } + + private ImmutableSet<Method> uncachedPublicMethodsWithName(Class<?> startClass, String name) { + // Class.getMethods() only returns public methods, so no need to filter explicitly for public. + Set<Method> methods = + Arrays.stream(startClass.getMethods()) + .filter(m -> m.getName().equals(name)) + .collect(toSet()); + if (!classIsPublic(startClass)) { + methods = + methods.stream() + .map(m -> visibleMethod(m, startClass)) + .filter(Objects::nonNull) + .collect(toSet()); + // It would be a bit simpler to use ImmutableSet.toImmutableSet() here, but there've been + // problems in the past with versions of Guava that don't have that method. + } + return ImmutableSet.copyOf(methods); + } + + private static final String THIS_PACKAGE = getPackageName(Node.class) + "."; + + /** + * Returns a Method with the same name and parameter types as the given one, but that is in a + * public class or interface. This might be the given method, or it might be a method in a + * superclass or superinterface. + * + * @return a public method in a public class or interface, or null if none was found. + */ + static Method visibleMethod(Method method, Class<?> in) { + if (in == null) { + return null; + } + Method methodInClass; + try { + methodInClass = in.getMethod(method.getName(), method.getParameterTypes()); + } catch (NoSuchMethodException e) { + return null; + } + if (classIsPublic(in) || in.getName().startsWith(THIS_PACKAGE)) { + // The second disjunct is a hack to allow us to use the public methods of $foreach without + // having to make the ForEachVar class public. We can invoke those methods from the same + // package since ForEachVar is package-protected. + return methodInClass; + } + Method methodInSuperclass = visibleMethod(method, in.getSuperclass()); + if (methodInSuperclass != null) { + return methodInSuperclass; + } + for (Class<?> superinterface : in.getInterfaces()) { + Method methodInSuperinterface = visibleMethod(method, superinterface); + if (methodInSuperinterface != null) { + return methodInSuperinterface; + } + } + return null; + } + + /** + * Returns whether the given class is public as seen from this class. Prior to Java 9, a class was + * either public or not public. But with the introduction of modules in Java 9, a class can be + * marked public and yet not be visible, if it is not exported from the module it appears in. So, + * on Java 9, we perform an additional check on class {@code c}, which is effectively {@code + * c.getModule().isExported(c.getPackageName())}. We use reflection so that the code can compile + * on earlier Java versions. + */ + private static boolean classIsPublic(Class<?> c) { + return Modifier.isPublic(c.getModifiers()) && classIsExported(c); + } + + private static boolean classIsExported(Class<?> c) { + if (CLASS_GET_MODULE_METHOD == null) { + return true; // There are no modules, so all classes are exported. + } + try { + String pkg = getPackageName(c); + Object module = CLASS_GET_MODULE_METHOD.invoke(c); + return (Boolean) MODULE_IS_EXPORTED_METHOD.invoke(module, pkg); + } catch (Exception e) { + return false; + } + } + + private static final Method CLASS_GET_MODULE_METHOD; + private static final Method MODULE_IS_EXPORTED_METHOD; + + static { + Method classGetModuleMethod; + Method moduleIsExportedMethod; + try { + classGetModuleMethod = Class.class.getMethod("getModule"); + Class<?> moduleClass = classGetModuleMethod.getReturnType(); + moduleIsExportedMethod = moduleClass.getMethod("isExported", String.class); + } catch (Exception e) { + classGetModuleMethod = null; + moduleIsExportedMethod = null; + } + CLASS_GET_MODULE_METHOD = classGetModuleMethod; + MODULE_IS_EXPORTED_METHOD = moduleIsExportedMethod; + } +} diff --git a/src/main/java/com/google/escapevelocity/Node.java b/src/main/java/com/google/escapevelocity/Node.java index 485f8a5..a017afa 100644 --- a/src/main/java/com/google/escapevelocity/Node.java +++ b/src/main/java/com/google/escapevelocity/Node.java @@ -64,7 +64,6 @@ abstract class Node { return new Cons(resourceName, lineNumber, ImmutableList.<Node>of()); } - /** * Create a new parse tree node that is the concatenation of the given ones. Evaluating the * new node produces the same string as evaluating each of the given nodes and concatenating the diff --git a/src/main/java/com/google/escapevelocity/Parser.java b/src/main/java/com/google/escapevelocity/Parser.java index 17c7fba..4416c48 100644 --- a/src/main/java/com/google/escapevelocity/Parser.java +++ b/src/main/java/com/google/escapevelocity/Parser.java @@ -15,6 +15,13 @@ */ package com.google.escapevelocity; +import com.google.common.base.CharMatcher; +import com.google.common.base.Verify; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.Iterables; +import com.google.common.primitives.Chars; +import com.google.common.primitives.Ints; import com.google.escapevelocity.DirectiveNode.SetNode; import com.google.escapevelocity.ExpressionNode.BinaryExpressionNode; import com.google.escapevelocity.ExpressionNode.NotExpressionNode; @@ -31,13 +38,6 @@ import com.google.escapevelocity.TokenNode.ForEachTokenNode; import com.google.escapevelocity.TokenNode.IfTokenNode; import com.google.escapevelocity.TokenNode.MacroDefinitionTokenNode; import com.google.escapevelocity.TokenNode.NestedTokenNode; -import com.google.common.base.CharMatcher; -import com.google.common.base.Verify; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableListMultimap; -import com.google.common.collect.Iterables; -import com.google.common.primitives.Chars; -import com.google.common.primitives.Ints; import java.io.IOException; import java.io.LineNumberReader; import java.io.Reader; @@ -419,10 +419,11 @@ class Parser { private Node parseParse() throws IOException { expect('('); skipSpace(); - if (c != '"') { + if (c != '"' && c != '\'') { throw parseException("#parse only supported with string literal argument"); } - String nestedResourceName = readStringLiteral(); + ExpressionNode nestedResourceNameExpression = parseStringLiteral(c, false); + String nestedResourceName = nestedResourceNameExpression.evaluate(null).toString(); expect(')'); try (Reader nestedReader = resourceOpener.openResource(nestedResourceName)) { Parser nestedParser = new Parser(nestedReader, nestedResourceName, resourceOpener); @@ -438,7 +439,7 @@ class Parser { * $<id> <macro-parameter-list> * }</pre> * - * <p>Macro parameters are not separated by commas, though method-reference parameters are. + * <p>Macro parameters are optionally separated by commas. */ private Node parseMacroDefinition() throws IOException { expect('('); @@ -451,6 +452,10 @@ class Parser { next(); break; } + if (c == ',') { + next(); + skipSpace(); + } if (c != '$') { throw parseException("Macro parameters should look like $name"); } @@ -518,15 +523,12 @@ class Parser { int startLine = lineNumber(); int lastC = '\0'; next(); - while (!(lastC == '*' && c == '#')) { - if (c == EOF) { - throw new ParseException( - "Unterminated #* - did not see matching *#", resourceName, startLine); - } + // Consistently with Velocity, we do not make it an error if a #* comment is not closed. + while (!(lastC == '*' && c == '#') && c != EOF) { lastC = c; next(); } - next(); + next(); // this may read EOF twice, which works return new CommentTokenNode(resourceName, startLine); } @@ -889,7 +891,6 @@ class Parser { } } - /** * Parses an expression containing only literals or references. * <pre>{@code @@ -905,7 +906,9 @@ class Parser { next(); node = parseRequiredReference(); } else if (c == '"') { - node = parseStringLiteral(); + node = parseStringLiteral(c, true); + } else if (c == '\'') { + node = parseStringLiteral(c, false); } else if (c == '-') { // Velocity does not have a negation operator. If we see '-' it must be the start of a // negative integer literal. @@ -922,30 +925,73 @@ class Parser { return node; } - private ExpressionNode parseStringLiteral() throws IOException { - return new ConstantExpressionNode(resourceName, lineNumber(), readStringLiteral()); - } - - private String readStringLiteral() throws IOException { - assert c == '"'; - StringBuilder sb = new StringBuilder(); + /** + * Parses a string literal, which may contain references to be expanded. Examples are + * {@code "foo"} or {@code "foo${bar}baz"}. + * <pre>{@code + * <string-literal> -> <double-quote-literal> | <single-quote-literal> + * <double-quote-literal> -> " <double-quote-string-contents> " + * <double-quote-string-contents> -> <empty> | + * <reference> <double-quote-string-contents> | + * <character-other-than-"> <double-quote-string-contents> + * <single-quote-literal> -> ' <single-quote-string-contents> ' + * <single-quote-string-contents> -> <empty> | + * <character-other-than-'> <single-quote-string-contents> + * }</pre> + */ + private ExpressionNode parseStringLiteral(int quote, boolean allowReferences) + throws IOException { + assert c == quote; next(); - while (c != '"') { - if (c == '\n' || c == EOF) { - throw parseException("Unterminated string constant"); - } - if (c == '$' || c == '\\') { - // In real Velocity, you can have a $ reference expanded inside a "" string literal. - // There are also '' string literals where that is not so. We haven't needed that yet - // so it's not supported. - throw parseException( - "Escapes or references in string constants are not currently supported"); + ImmutableList.Builder<Node> nodes = ImmutableList.builder(); + StringBuilder sb = new StringBuilder(); + while (c != quote) { + switch (c) { + case '\n': + case EOF: + throw parseException("Unterminated string constant"); + case '\\': + throw parseException( + "Escapes in string constants are not currently supported"); + case '$': + if (allowReferences) { + if (sb.length() > 0) { + nodes.add(new ConstantExpressionNode(resourceName, lineNumber(), sb.toString())); + sb.setLength(0); + } + next(); + nodes.add(parseReference()); + break; + } + // fall through + default: + sb.appendCodePoint(c); + next(); } - sb.appendCodePoint(c); - next(); } next(); - return sb.toString(); + if (sb.length() > 0) { + nodes.add(new ConstantExpressionNode(resourceName, lineNumber(), sb.toString())); + } + return new StringLiteralNode(resourceName, lineNumber(), nodes.build()); + } + + private static class StringLiteralNode extends ExpressionNode { + private final ImmutableList<Node> nodes; + + StringLiteralNode(String resourceName, int lineNumber, ImmutableList<Node> nodes) { + super(resourceName, lineNumber); + this.nodes = nodes; + } + + @Override + Object evaluate(EvaluationContext context) { + StringBuilder sb = new StringBuilder(); + for (Node node : nodes) { + sb.append(node.evaluate(context)); + } + return sb.toString(); + } } private ExpressionNode parseIntLiteral(String prefix) throws IOException { diff --git a/src/main/java/com/google/escapevelocity/ReferenceNode.java b/src/main/java/com/google/escapevelocity/ReferenceNode.java index 4b43f77..622388f 100644 --- a/src/main/java/com/google/escapevelocity/ReferenceNode.java +++ b/src/main/java/com/google/escapevelocity/ReferenceNode.java @@ -15,16 +15,18 @@ */ package com.google.escapevelocity; +import static java.util.stream.Collectors.toList; + import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.primitives.Primitives; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; -import java.lang.reflect.Modifier; -import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Optional; /** * A node in the parse tree that is a reference. A reference is anything beginning with {@code $}, @@ -89,21 +91,27 @@ abstract class ReferenceNode extends ExpressionNode { if (lhsValue == null) { throw evaluationException("Cannot get member " + id + " of null value"); } + // If this is a Map, then Velocity looks up the property in the map. + if (lhsValue instanceof Map<?, ?>) { + Map<?, ?> map = (Map<?, ?>) lhsValue; + return map.get(id); + } // Velocity specifies that, given a reference .foo, it will first look for getfoo() and then // for getFoo(), and likewise given .Foo it will look for getFoo() and then getfoo(). for (String prefix : PREFIXES) { for (boolean changeCase : CHANGE_CASE) { String baseId = changeCase ? changeInitialCase(id) : id; String methodName = prefix + baseId; - Method method; - try { - method = lhsValue.getClass().getMethod(methodName); + Optional<Method> maybeMethod = + context.publicMethodsWithName(lhsValue.getClass(), methodName).stream() + .filter(m -> m.getParameterTypes().length == 0) + .findFirst(); + if (maybeMethod.isPresent()) { + Method method = maybeMethod.get(); if (!prefix.equals("is") || method.getReturnType().equals(boolean.class)) { // Don't consider methods that happen to be called isFoo() but don't return boolean. return invokeMethod(method, lhsValue, ImmutableList.of()); } - } catch (NoSuchMethodException e) { - // Continue with next possibility } } } @@ -206,25 +214,35 @@ abstract class ReferenceNode extends ExpressionNode { if (lhsValue == null) { throw evaluationException("Cannot invoke method " + id + " on null value"); } - List<Object> argValues = new ArrayList<>(); - for (ExpressionNode arg : args) { - argValues.add(arg.evaluate(context)); - } - List<Method> methodsWithName = new ArrayList<>(); - for (Method method : lhsValue.getClass().getMethods()) { - if (method.getName().equals(id) && !method.isSynthetic()) { - methodsWithName.add(method); + try { + return evaluate(context, lhsValue, lhsValue.getClass()); + } catch (EvaluationException e) { + // If this is a Class, try invoking a static method of the class it refers to. + // This is what Apache Velocity does. If the method exists as both an instance method of + // Class and a static method of the referenced class, then it is the instance method of + // Class that wins, again consistent with Velocity. + if (lhsValue instanceof Class<?>) { + return evaluate(context, null, (Class<?>) lhsValue); } + throw e; } - if (methodsWithName.isEmpty()) { - throw evaluationException("No method " + id + " in " + lhsValue.getClass().getName()); + } + + private Object evaluate(EvaluationContext context, Object lhsValue, Class<?> targetClass) { + List<Object> argValues = args.stream() + .map(arg -> arg.evaluate(context)) + .collect(toList()); + ImmutableSet<Method> publicMethodsWithName = context.publicMethodsWithName(targetClass, id); + if (publicMethodsWithName.isEmpty()) { + throw evaluationException("No method " + id + " in " + targetClass.getName()); } - List<Method> compatibleMethods = new ArrayList<>(); - for (Method method : methodsWithName) { - // TODO(emcmanus): support varargs, if it's useful - if (compatibleArgs(method.getParameterTypes(), argValues)) { - compatibleMethods.add(method); - } + List<Method> compatibleMethods = publicMethodsWithName.stream() + .filter(method -> compatibleArgs(method.getParameterTypes(), argValues)) + .collect(toList()); + // TODO(emcmanus): support varargs, if it's useful + if (compatibleMethods.size() > 1) { + compatibleMethods = + compatibleMethods.stream().filter(method -> !method.isSynthetic()).collect(toList()); } switch (compatibleMethods.size()) { case 0: @@ -253,7 +271,7 @@ abstract class ReferenceNode extends ExpressionNode { Object argValue = argValues.get(i); if (paramType.isPrimitive()) { return primitiveIsCompatible(paramType, argValue); - } else if (!paramType.isInstance(argValue)) { + } else if (argValue != null && !paramType.isInstance(argValue)) { return false; } } @@ -267,7 +285,7 @@ abstract class ReferenceNode extends ExpressionNode { return primitiveTypeIsAssignmentCompatible(primitive, Primitives.unwrap(value.getClass())); } - private static final ImmutableList<Class<?>> NUMERICAL_PRIMITIVES = ImmutableList.<Class<?>>of( + private static final ImmutableList<Class<?>> NUMERICAL_PRIMITIVES = ImmutableList.of( byte.class, short.class, int.class, long.class, float.class, double.class); private static final int INDEX_OF_INT = NUMERICAL_PRIMITIVES.indexOf(int.class); @@ -300,21 +318,9 @@ abstract class ReferenceNode extends ExpressionNode { } /** - * Invoke the given method on the given target with the given arguments. The method is expected - * to be public, but the class it is in might not be. In that case we will search up the - * hierarchy for an ancestor that is public and has the same method, and use that to invoke the - * method. Otherwise we would get an {@link IllegalAccessException}. More than one ancestor might - * define the method, but it doesn't matter which one we invoke since ultimately the code that - * will run will be the same. + * Invoke the given method on the given target with the given arguments. */ Object invokeMethod(Method method, Object target, List<Object> argValues) { - if (!classIsPublic(target.getClass())) { - method = visibleMethod(method, target.getClass()); - if (method == null) { - throw evaluationException( - "Method is not visible in class " + target.getClass().getName() + ": " + method); - } - } try { return method.invoke(target, argValues.toArray()); } catch (InvocationTargetException e) { @@ -323,98 +329,4 @@ abstract class ReferenceNode extends ExpressionNode { throw evaluationException(e); } } - - private static String packageNameOf(Class<?> c) { - String name = c.getName(); - int lastDot = name.lastIndexOf('.'); - if (lastDot > 0) { - return name.substring(0, lastDot); - } else { - return ""; - } - } - - private static final String THIS_PACKAGE = packageNameOf(Node.class) + "."; - - /** - * Returns a Method with the same name and parameter types as the given one, but that is in a - * public class or interface. This might be the given method, or it might be a method in a - * superclass or superinterface. - * - * @return a public method in a public class or interface, or null if none was found. - */ - static Method visibleMethod(Method method, Class<?> in) { - if (in == null) { - return null; - } - Method methodInClass; - try { - methodInClass = in.getMethod(method.getName(), method.getParameterTypes()); - } catch (NoSuchMethodException e) { - return null; - } - if (classIsPublic(in) || in.getName().startsWith(THIS_PACKAGE)) { - // The second disjunct is a hack to allow us to use the methods of $foreach without having - // to make the ForEachVar class public. We can invoke those methods from here since they - // are in the same package. - return methodInClass; - } - Method methodSuper = visibleMethod(method, in.getSuperclass()); - if (methodSuper != null) { - return methodSuper; - } - for (Class<?> intf : in.getInterfaces()) { - Method methodIntf = visibleMethod(method, intf); - if (methodIntf != null) { - return methodIntf; - } - } - return null; - } - - /** - * Returns whether the given class is public as seen from this class. Prior to Java 9, a class - * was either public or not public. But with the introduction of modules in Java 9, a class can - * be marked public and yet not be visible, if it is not exported from the module it appears in. - * So, on Java 9, we perform an additional check on class {@code c}, which is effectively - * {@code c.getModule().isExported(c.getPackageName())}. We use reflection so that the code can - * compile on earlier Java versions. - */ - private static boolean classIsPublic(Class<?> c) { - if (!Modifier.isPublic(c.getModifiers())) { - return false; - } - if (CLASS_GET_MODULE_METHOD != null) { - return classIsExported(c); - } - return true; - } - - private static boolean classIsExported(Class<?> c) { - try { - String pkg = packageNameOf(c); - Object module = CLASS_GET_MODULE_METHOD.invoke(c); - return (Boolean) MODULE_IS_EXPORTED_METHOD.invoke(module, pkg); - } catch (Exception e) { - return false; - } - } - - private static final Method CLASS_GET_MODULE_METHOD; - private static final Method MODULE_IS_EXPORTED_METHOD; - - static { - Method classGetModuleMethod; - Method moduleIsExportedMethod; - try { - classGetModuleMethod = Class.class.getMethod("getModule"); - Class<?> moduleClass = classGetModuleMethod.getReturnType(); - moduleIsExportedMethod = moduleClass.getMethod("isExported", String.class); - } catch (Exception e) { - classGetModuleMethod = null; - moduleIsExportedMethod = null; - } - CLASS_GET_MODULE_METHOD = classGetModuleMethod; - MODULE_IS_EXPORTED_METHOD = moduleIsExportedMethod; - } } diff --git a/src/main/java/com/google/escapevelocity/Template.java b/src/main/java/com/google/escapevelocity/Template.java index 0f4de8f..6bc75c2 100644 --- a/src/main/java/com/google/escapevelocity/Template.java +++ b/src/main/java/com/google/escapevelocity/Template.java @@ -31,6 +31,17 @@ import java.util.Map; // TODO(emcmanus): spell out exactly what Velocity features are unsupported. public class Template { private final Node root; + + /** + * Caches {@link Method} objects for public methods accessed through references. The first time + * we evaluate {@code $var.property} or {@code $var.method(...)} for a {@code $var} of a given + * class and for a given property or method signature, we'll store the resultant {@link Method} + * object. Every subsequent time we'll reuse that {@link Method}. The method lookup is quite slow + * so caching is useful. The main downside is that we may potentially hold on to {@link Method} + * objects that will never be used with this {@link Template} again. But in practice templates + * tend to be used repeatedly with the same classes. + */ + private final MethodFinder methodFinder = new MethodFinder(); /** * Used to resolve references to resources in the template, through {@code #parse} directives. @@ -116,7 +127,7 @@ public class Template { * @return the string result of evaluating the template. */ public String evaluate(Map<String, ?> vars) { - EvaluationContext evaluationContext = new PlainEvaluationContext(vars); + EvaluationContext evaluationContext = new PlainEvaluationContext(vars, methodFinder); return String.valueOf(root.evaluate(evaluationContext)); } } diff --git a/src/test/java/com/google/escapevelocity/MethodFinderTest.java b/src/test/java/com/google/escapevelocity/MethodFinderTest.java new file mode 100644 index 0000000..66b8948 --- /dev/null +++ b/src/test/java/com/google/escapevelocity/MethodFinderTest.java @@ -0,0 +1,92 @@ +/* + * Copyright (C) 2019 Google, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.escapevelocity; + +import static com.google.common.truth.Truth.assertThat; +import static java.util.stream.Collectors.toSet; + +import com.google.common.collect.ImmutableMap; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class MethodFinderTest { + @Test + public void visibleMethodFromClass() throws Exception { + Map<String, String> map = Collections.singletonMap("foo", "bar"); + Class<?> mapClass = map.getClass(); + assertThat(Modifier.isPublic(mapClass.getModifiers())).isFalse(); + + Method size = mapClass.getMethod("size"); + Method visibleSize = MethodFinder.visibleMethod(size, mapClass); + assertThat(visibleSize.getDeclaringClass().isInterface()).isFalse(); + assertThat(visibleSize.invoke(map)).isEqualTo(1); + } + + @Test + public void visibleMethodFromInterface() throws Exception { + Map<String, String> map = ImmutableMap.of("foo", "bar"); + Map.Entry<String, String> entry = map.entrySet().iterator().next(); + Class<?> entryClass = entry.getClass(); + assertThat(Modifier.isPublic(entryClass.getModifiers())).isFalse(); + + Method getValue = entryClass.getMethod("getValue"); + Method visibleGetValue = MethodFinder.visibleMethod(getValue, entryClass); + assertThat(visibleGetValue.getDeclaringClass().isInterface()).isTrue(); + assertThat(visibleGetValue.invoke(entry)).isEqualTo("bar"); + } + + @Test + public void publicMethodsWithName() { + List<String> list = Collections.singletonList("foo"); + Class<?> listClass = list.getClass(); + assertThat(Modifier.isPublic(listClass.getModifiers())).isFalse(); + + MethodFinder methodFinder = new MethodFinder(); + Set<Method> methods = methodFinder.publicMethodsWithName(listClass, "remove"); + // This should find at least remove(int) and remove(Object). + assertThat(methods.size()).isAtLeast(2); + assertThat(methods.stream().map(Method::getName).collect(toSet())).containsExactly("remove"); + assertThat(methods.stream().allMatch(MethodFinderTest::isPublic)).isTrue(); + + // We should cache the result, meaning we get back the same result if we ask a second time. + Set<Method> methods2 = methodFinder.publicMethodsWithName(listClass, "remove"); + assertThat(methods2).isSameInstanceAs(methods); + } + + @Test + public void publicMethodsWithName_Nonexistent() { + List<String> list = Collections.singletonList("foo"); + Class<?> listClass = list.getClass(); + assertThat(Modifier.isPublic(listClass.getModifiers())).isFalse(); + + MethodFinder methodFinder = new MethodFinder(); + Set<Method> methods = methodFinder.publicMethodsWithName(listClass, "nonexistentMethod"); + assertThat(methods).isEmpty(); + } + + private static boolean isPublic(Method method) { + return Modifier.isPublic(method.getModifiers()) + && Modifier.isPublic(method.getDeclaringClass().getModifiers()); + } +} diff --git a/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java b/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java index 3e784f6..b1759bd 100644 --- a/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java +++ b/src/test/java/com/google/escapevelocity/ReferenceNodeTest.java @@ -17,15 +17,11 @@ package com.google.escapevelocity; import static com.google.common.truth.Truth.assertThat; -import com.google.escapevelocity.ReferenceNode.MethodReferenceNode; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.primitives.Primitives; import com.google.common.truth.Expect; -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; -import java.util.Collections; -import java.util.Map; +import com.google.escapevelocity.ReferenceNode.MethodReferenceNode; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -85,22 +81,12 @@ public class ReferenceNodeTest { MethodReferenceNode.primitiveTypeIsAssignmentCompatible(to, from); expect .withMessage(from + " assignable to " + to) - .that(expected).isEqualTo(actual); + .that(actual).isEqualTo(expected); } } } @Test - public void testVisibleMethod() throws Exception { - Map<String, String> map = Collections.singletonMap("foo", "bar"); - Class<?> mapClass = map.getClass(); - assertThat(Modifier.isPublic(mapClass.getModifiers())).isFalse(); - Method size = map.getClass().getMethod("size"); - Method visibleSize = ReferenceNode.visibleMethod(size, mapClass); - assertThat(visibleSize.invoke(map)).isEqualTo(1); - } - - @Test public void testCompatibleArgs() { assertThat(MethodReferenceNode.compatibleArgs( new Class<?>[]{int.class}, ImmutableList.of((Object) 5))).isTrue(); diff --git a/src/test/java/com/google/escapevelocity/TemplateTest.java b/src/test/java/com/google/escapevelocity/TemplateTest.java index 04bad8e..0503125 100644 --- a/src/test/java/com/google/escapevelocity/TemplateTest.java +++ b/src/test/java/com/google/escapevelocity/TemplateTest.java @@ -16,10 +16,12 @@ package com.google.escapevelocity; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSetMultimap; import com.google.common.truth.Expect; import java.io.ByteArrayInputStream; import java.io.FileNotFoundException; @@ -27,6 +29,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.StringReader; import java.io.StringWriter; +import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.HashMap; @@ -36,6 +39,7 @@ import java.util.function.Supplier; import org.apache.commons.collections.ExtendedProperties; import org.apache.velocity.VelocityContext; import org.apache.velocity.exception.ResourceNotFoundException; +import org.apache.velocity.exception.VelocityException; import org.apache.velocity.runtime.RuntimeConstants; import org.apache.velocity.runtime.RuntimeInstance; import org.apache.velocity.runtime.log.NullLogChute; @@ -45,7 +49,6 @@ import org.apache.velocity.runtime.resource.loader.ResourceLoader; import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.junit.rules.TestName; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -57,7 +60,6 @@ import org.junit.runners.JUnit4; public class TemplateTest { @Rule public TestName testName = new TestName(); @Rule public Expect expect = Expect.create(); - @Rule public ExpectedException thrown = ExpectedException.none(); private RuntimeInstance velocityRuntimeInstance; @@ -128,6 +130,31 @@ public class TemplateTest { return writer.toString(); } + private void expectParseException( + String template, + String expectedMessageSubstring) { + Exception velocityException = null; + try { + SimpleNode parsedTemplate = + velocityRuntimeInstance.parse(new StringReader(template), testName.getMethodName()); + VelocityContext velocityContext = new VelocityContext(new TreeMap<>()); + velocityRuntimeInstance.render( + velocityContext, new StringWriter(), parsedTemplate.getTemplateName(), parsedTemplate); + fail("Velocity did not throw an exception for this template"); + } catch (org.apache.velocity.runtime.parser.ParseException | VelocityException expected) { + velocityException = expected; + } + try { + Template.parseFrom(new StringReader(template)); + fail("Velocity generated an exception, but EscapeVelocity did not: " + velocityException); + } catch (IOException e) { + throw new UncheckedIOException(e); + } catch (ParseException expected) { + assertWithMessage("Got expected exception, but message did not match") + .that(expected).hasMessageThat().contains(expectedMessageSubstring); + } + } + @Test public void empty() { compare(""); @@ -210,13 +237,6 @@ public class TemplateTest { compare("$foo.!", ImmutableMap.of("foo", false)); } - /* TODO(emcmanus): make this work. - @Test - public void substituteNotPropertyId() { - compare("$foo.!", ImmutableMap.of("foo", false)); - } - */ - @Test public void substituteNestedProperty() { compare("\n$t.name.empty\n", ImmutableMap.of("t", Thread.currentThread())); @@ -228,23 +248,64 @@ public class TemplateTest { } @Test + public void substituteMethodNoArgsSyntheticOverride() { + compare("<$c.isEmpty()>", ImmutableMap.of("c", ImmutableSetMultimap.of())); + } + + @Test public void substituteMethodOneArg() { compare("<$list.get(0)>", ImmutableMap.of("list", ImmutableList.of("foo"))); } @Test + public void substituteMethodOneNullArg() { + // This should evaluate map.containsKey(map.get("absent")), which is map.containsKey(null). + compare("<$map.containsKey($map.get(\"absent\"))>", ImmutableMap.of("map", ImmutableMap.of())); + } + + @Test public void substituteMethodTwoArgs() { compare("\n$s.indexOf(\"bar\", 2)\n", ImmutableMap.of("s", "barbarbar")); } @Test - public void substituteMethodNoSynthetic() { + public void substituteMethodSyntheticOverloads() { // If we aren't careful, we'll see both the inherited `Set<K> keySet()` from Map // and the overridden `ImmutableSet<K> keySet()` in ImmutableMap. compare("$map.keySet()", ImmutableMap.of("map", ImmutableMap.of("foo", "bar"))); } @Test + public void substituteStaticMethod() { + compare("$Integer.toHexString(23)", ImmutableMap.of("Integer", Integer.class)); + } + + @Test + public void substituteStaticMethodAsInstanceMethod() { + compare("$i.toHexString(23)", ImmutableMap.of("i", 0)); + } + + @Test + public void substituteClassMethod() { + // This is Class.getName(). + compare("$Integer.getName()", ImmutableMap.of("Integer", Integer.class)); + } + + /** See {@link #substituteClassOrInstanceMethod}. */ + public static class GetName { + public static String getName() { + return "Noddy"; + } + } + + @Test + public void substituteClassOrInstanceMethod() { + // If the method exists as both an instance method on Class and a static method on the named + // class, it's the instance method that wins, so this is still Class.getName(). + compare("$GetName.getName()", ImmutableMap.of("GetName", GetName.class)); + } + + @Test public void substituteIndexNoBraces() { compare("<$map[\"x\"]>", ImmutableMap.of("map", ImmutableMap.of("x", "y"))); } @@ -254,6 +315,14 @@ public class TemplateTest { compare("<${map[\"x\"]}>", ImmutableMap.of("map", ImmutableMap.of("x", "y"))); } + // Velocity allows you to write $map.foo instead of $map["foo"]. + @Test + public void substituteMapProperty() { + compare("$map.foo", ImmutableMap.of("map", ImmutableMap.of("foo", "bar"))); + // $map.empty is always equivalent to $map["empty"], never Map.isEmpty(). + compare("$map.empty", ImmutableMap.of("map", ImmutableMap.of("empty", "foo"))); + } + @Test public void substituteIndexThenProperty() { compare("<$map[2].name>", ImmutableMap.of("map", ImmutableMap.of(2, getClass()))); @@ -291,6 +360,29 @@ public class TemplateTest { } @Test + public void substituteInString() { + String template = + "#foreach ($a in $list)" + + "#set ($s = \"THING_${foreach.index}\")" + + "$s,$s;" + + "#end"; + compare(template, ImmutableMap.of("list", ImmutableList.of(1, 2, 3))); + compare("#set ($s = \"$x\") <$s>", ImmutableMap.of("x", "fred")); + compare("#set ($s = \"==$x$y\") <$s>", ImmutableMap.of("x", "fred", "y", "jim")); + compare("#set ($s = \"$x$y==\") <$s>", ImmutableMap.of("x", "fred", "y", "jim")); + } + + @Test + public void stringOperationsOnSubstitution() { + compare("#set ($s = \"a${b}c\") $s.length()", ImmutableMap.of("b", 23)); + } + + @Test + public void singleQuoteNoSubstitution() { + compare("#set ($s = 'a${b}c') x${s}y", ImmutableMap.of("b", 23)); + } + + @Test public void simpleSet() { compare("$x#set ($x = 17)#set ($y = 23) ($x, $y)", ImmutableMap.of("x", 1)); } @@ -506,6 +598,18 @@ public class TemplateTest { } @Test + public void forEachIndex() { + String template = + "#foreach ($x in $list)" + + "[$foreach.index]" + + "#foreach ($y in $list)" + + "($foreach.index)==$x.$y==" + + "#end" + + "#end"; + compare(template, ImmutableMap.of("list", ImmutableList.of("blim", "blam", "blum"))); + } + + @Test public void setSpacing() { // The spacing in the output from #set is eccentric. compare("x#set ($x = 0)x"); @@ -550,6 +654,18 @@ public class TemplateTest { compare(template, ImmutableMap.of("x", "tiddly")); } + @Test + public void macroWithCommaSeparatedArgs() { + String template = + "$x\n" + + "#macro (m, $x, $y)\n" + + " #if ($x < $y) less #else greater #end\n" + + "#end\n" + + "#m(17 23) #m(23 17) #m(17 17)\n" + + "$x"; + compare(template, ImmutableMap.of("x", "tiddly")); + } + /** * Tests defining a macro inside a conditional. This proves that macros are not evaluated in the * main control flow, but rather are extracted at parse time. It also tests what happens if there @@ -706,45 +822,36 @@ public class TemplateTest { } @Test - public void badBraceReference() throws IOException { + public void badBraceReference() { String template = "line 1\nline 2\nbar${foo.!}baz"; - thrown.expect(ParseException.class); - thrown.expectMessage("Expected }, on line 3, at text starting: .!}baz"); - Template.parseFrom(new StringReader(template)); + expectParseException(template, "Expected }, on line 3, at text starting: .!}baz"); } @Test - public void undefinedMacro() throws IOException { + public void undefinedMacro() { String template = "#oops()"; - thrown.expect(ParseException.class); - thrown.expectMessage("#oops is neither a standard directive nor a macro that has been defined"); - Template.parseFrom(new StringReader(template)); + expectParseException( + template, + "#oops is neither a standard directive nor a macro that has been defined"); } @Test - public void macroArgumentMismatch() throws IOException { + public void macroArgumentMismatch() { String template = "#macro (twoArgs $a $b) $a $b #end\n" + "#twoArgs(23)\n"; - thrown.expect(ParseException.class); - thrown.expectMessage("Wrong number of arguments to #twoArgs: expected 2, got 1"); - Template.parseFrom(new StringReader(template)); + expectParseException(template, "Wrong number of arguments to #twoArgs: expected 2, got 1"); } @Test - public void unclosedBlockQuote() throws IOException { + public void unclosedBlockQuote() { String template = "foo\nbar #[[\nblah\nblah"; - thrown.expect(ParseException.class); - thrown.expectMessage("Unterminated #[[ - did not see matching ]]#, on line 2"); - Template.parseFrom(new StringReader(template)); + expectParseException(template, "Unterminated #[[ - did not see matching ]]#, on line 2"); } @Test - public void unclosedBlockComment() throws IOException { - String template = "foo\nbar #*\nblah\nblah"; - thrown.expect(ParseException.class); - thrown.expectMessage("Unterminated #* - did not see matching *#, on line 2"); - Template.parseFrom(new StringReader(template)); + public void unclosedBlockComment() { + compare("foo\nbar #*\nblah\nblah"); } /** |