Merge pull request #3 from google/sync-4-30-2019
Moe Sync
diff --git a/README.md b/README.md
index 0f24bc5..e716ecf 100644
--- a/README.md
+++ b/README.md
@@ -14,9 +14,6 @@
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
diff --git a/pom.xml b/pom.xml
index 8f6da5d..076165b 100644
--- a/pom.xml
+++ b/pom.xml
@@ -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 @@
}
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 @@
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 @@
*/
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 @@
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 @@
}
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 @@
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 @@
};
}
}
+
+ @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 @@
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.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 @@
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 @@
* $<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 @@
next();
break;
}
+ if (c == ',') {
+ next();
+ skipSpace();
+ }
if (c != '$') {
throw parseException("Macro parameters should look like $name");
}
@@ -518,15 +523,12 @@
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 @@
}
}
-
/**
* Parses an expression containing only literals or references.
* <pre>{@code
@@ -905,7 +906,9 @@
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 @@
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");
+ 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();
}
- 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");
- }
- 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 @@
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 @@
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 @@
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 @@
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 @@
}
/**
- * 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 @@
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 @@
// 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 @@
* @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 @@
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 @@
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.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 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.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 @@
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 @@
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 @@
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 @@
}
@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 @@
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 @@
}
@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 @@
}
@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 @@
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 @@
}
@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");
}
/**
