Skip to content

Commit

Permalink
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56425
Browse files Browse the repository at this point in the history 
 Improve method matching for EL expressions. When looking for matching methods, an exact match between parameter types is preferred followed by an assignable match followed by a coercible match.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1590120 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@markt-asf
markt-asf committed Apr 25, 2014
1 parent 8881717 commit cf76211
Show file tree
Hide file tree
Showing 4 changed files with 165 additions and 26 deletions.
88 changes: 75 additions & 13 deletions java/javax/el/Util.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ static Method findMethod(Class<?> clazz, String methodName,
private static Wrapper findWrapper(Class<?> clazz, List<Wrapper> wrappers,
String name, Class<?>[] paramTypes, Object[] paramValues) {

Map<Wrapper,Integer> candidates = new HashMap<>();
Map<Wrapper,MatchResult> candidates = new HashMap<>();

int paramCount;
if (paramTypes == null) {
Expand All @@ -255,6 +255,8 @@ private static Wrapper findWrapper(Class<?> clazz, List<Wrapper> wrappers,

// Check the parameters match
int exactMatch = 0;
int assignableMatch = 0;
int coercibleMatch = 0;
boolean noMatch = false;
for (int i = 0; i < mParamCount; i++) {
// Can't be null
Expand All @@ -263,12 +265,16 @@ private static Wrapper findWrapper(Class<?> clazz, List<Wrapper> wrappers,
} else if (i == (mParamCount - 1) && w.isVarArgs()) {
Class<?> varType = mParamTypes[i].getComponentType();
for (int j = i; j < paramCount; j++) {
if (!isAssignableFrom(paramTypes[j], varType)) {
if (isAssignableFrom(paramTypes[j], varType)) {
assignableMatch++;
} else {
if (paramValues == null) {
noMatch = true;
break;
} else {
if (!isCoercibleFrom(paramValues[j], varType)) {
if (isCoercibleFrom(paramValues[j], varType)) {
coercibleMatch++;
} else {
noMatch = true;
break;
}
Expand All @@ -278,12 +284,16 @@ private static Wrapper findWrapper(Class<?> clazz, List<Wrapper> wrappers,
// lead to a varArgs method matching when the result
// should be ambiguous
}
} else if (!isAssignableFrom(paramTypes[i], mParamTypes[i])) {
} else if (isAssignableFrom(paramTypes[i], mParamTypes[i])) {
assignableMatch++;
} else {
if (paramValues == null) {
noMatch = true;
break;
} else {
if (!isCoercibleFrom(paramValues[i], mParamTypes[i])) {
if (isCoercibleFrom(paramValues[i], mParamTypes[i])) {
coercibleMatch++;
} else {
noMatch = true;
break;
}
Expand All @@ -300,26 +310,26 @@ private static Wrapper findWrapper(Class<?> clazz, List<Wrapper> wrappers,
return w;
}

candidates.put(w, Integer.valueOf(exactMatch));
candidates.put(w, new MatchResult(exactMatch, assignableMatch, coercibleMatch));
}

// Look for the method that has the highest number of parameters where
// the type matches exactly
int bestMatch = 0;
MatchResult bestMatch = new MatchResult(0, 0, 0);
Wrapper match = null;
boolean multiple = false;
for (Map.Entry<Wrapper, Integer> entry : candidates.entrySet()) {
if (entry.getValue().intValue() > bestMatch ||
match == null) {
bestMatch = entry.getValue().intValue();
for (Map.Entry<Wrapper, MatchResult> entry : candidates.entrySet()) {
int cmp = entry.getValue().compareTo(bestMatch);
if (cmp > 0 || match == null) {
bestMatch = entry.getValue();
match = entry.getKey();
multiple = false;
} else if (entry.getValue().intValue() == bestMatch) {
} else if (cmp == 0) {
multiple = true;
}
}
if (multiple) {
if (bestMatch == paramCount - 1) {
if (bestMatch.getExact() == paramCount - 1) {
// Only one parameter is not an exact match - try using the
// super class
match = resolveAmbiguousWrapper(candidates.keySet(), paramTypes);
Expand Down Expand Up @@ -700,4 +710,56 @@ public boolean isVarArgs() {
return c.isVarArgs();
}
}

/*
* This class duplicates code in org.apache.el.util.ReflectionUtil. When
* making changes keep the code in sync.
*/
private static class MatchResult implements Comparable<MatchResult> {

private final int exact;
private final int assignable;
private final int coercible;

public MatchResult(int exact, int assignable, int coercible) {
this.exact = exact;
this.assignable = assignable;
this.coercible = coercible;
}

public int getExact() {
return exact;
}

public int getAssignable() {
return assignable;
}

public int getCoercible() {
return coercible;
}

@Override
public int compareTo(MatchResult o) {
if (this.getExact() < o.getExact()) {
return -1;
} else if (this.getExact() > o.getExact()) {
return 1;
} else {
if (this.getAssignable() < o.getAssignable()) {
return -1;
} else if (this.getAssignable() > o.getAssignable()) {
return 1;
} else {
if (this.getCoercible() < o.getCoercible()) {
return -1;
} else if (this.getCoercible() > o.getCoercible()) {
return 1;
} else {
return 0;
}
}
}
}
}
}
89 changes: 76 additions & 13 deletions java/org/apache/el/util/ReflectionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public static Method getMethod(Object base, Object property,
}

Method[] methods = base.getClass().getMethods();
Map<Method,Integer> candidates = new HashMap<>();
Map<Method,MatchResult> candidates = new HashMap<>();

for (Method m : methods) {
if (!m.getName().equals(methodName)) {
Expand All @@ -163,6 +163,8 @@ public static Method getMethod(Object base, Object property,

// Check the parameters match
int exactMatch = 0;
int assignableMatch = 0;
int coercibleMatch = 0;
boolean noMatch = false;
for (int i = 0; i < mParamCount; i++) {
// Can't be null
Expand All @@ -171,12 +173,16 @@ public static Method getMethod(Object base, Object property,
} else if (i == (mParamCount - 1) && m.isVarArgs()) {
Class<?> varType = mParamTypes[i].getComponentType();
for (int j = i; j < paramCount; j++) {
if (!isAssignableFrom(paramTypes[j], varType)) {
if (isAssignableFrom(paramTypes[j], varType)) {
assignableMatch++;
} else {
if (paramValues == null) {
noMatch = true;
break;
} else {
if (!isCoercibleFrom(paramValues[j], varType)) {
if (isCoercibleFrom(paramValues[j], varType)) {
coercibleMatch++;
} else {
noMatch = true;
break;
}
Expand All @@ -186,12 +192,16 @@ public static Method getMethod(Object base, Object property,
// lead to a varArgs method matching when the result
// should be ambiguous
}
} else if (!isAssignableFrom(paramTypes[i], mParamTypes[i])) {
} else if (isAssignableFrom(paramTypes[i], mParamTypes[i])) {
assignableMatch++;
} else {
if (paramValues == null) {
noMatch = true;
break;
} else {
if (!isCoercibleFrom(paramValues[i], mParamTypes[i])) {
if (isCoercibleFrom(paramValues[i], mParamTypes[i])) {
coercibleMatch++;
} else {
noMatch = true;
break;
}
Expand All @@ -208,26 +218,26 @@ public static Method getMethod(Object base, Object property,
return getMethod(base.getClass(), m);
}

candidates.put(m, Integer.valueOf(exactMatch));
candidates.put(m, new MatchResult(exactMatch, assignableMatch, coercibleMatch));
}

// Look for the method that has the highest number of parameters where
// the type matches exactly
int bestMatch = 0;
MatchResult bestMatch = new MatchResult(0, 0, 0);
Method match = null;
boolean multiple = false;
for (Map.Entry<Method, Integer> entry : candidates.entrySet()) {
if (entry.getValue().intValue() > bestMatch ||
match == null) {
bestMatch = entry.getValue().intValue();
for (Map.Entry<Method, MatchResult> entry : candidates.entrySet()) {
int cmp = entry.getValue().compareTo(bestMatch);
if (cmp > 0 || match == null) {
bestMatch = entry.getValue();
match = entry.getKey();
multiple = false;
} else if (entry.getValue().intValue() == bestMatch) {
} else if (cmp == 0) {
multiple = true;
}
}
if (multiple) {
if (bestMatch == paramCount - 1) {
if (bestMatch.getExact() == paramCount - 1) {
// Only one parameter is not an exact match - try using the
// super class
match = resolveAmbiguousMethod(candidates.keySet(), paramTypes);
Expand Down Expand Up @@ -430,4 +440,57 @@ private static final String paramString(Class<?>[] types) {
}
return null;
}

/*
* This class duplicates code in javax.el.Util. When making changes keep
* the code in sync.
*/
private static class MatchResult implements Comparable<MatchResult> {

private final int exact;
private final int assignable;
private final int coercible;

public MatchResult(int exact, int assignable, int coercible) {
this.exact = exact;
this.assignable = assignable;
this.coercible = coercible;
}

public int getExact() {
return exact;
}

public int getAssignable() {
return assignable;
}

public int getCoercible() {
return coercible;
}

@Override
public int compareTo(MatchResult o) {
if (this.getExact() < o.getExact()) {
return -1;
} else if (this.getExact() > o.getExact()) {
return 1;
} else {
if (this.getAssignable() < o.getAssignable()) {
return -1;
} else if (this.getAssignable() > o.getAssignable()) {
return 1;
} else {
if (this.getCoercible() < o.getCoercible()) {
return -1;
} else if (this.getCoercible() > o.getCoercible()) {
return 1;
} else {
return 0;
}
}
}
}
}

}
8 changes: 8 additions & 0 deletions test/javax/el/TestUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,12 @@ public void test02() {
Date result = (Date) processor.eval("Date(86400)");
Assert.assertEquals(86400, result.getTime());
}


@Test
public void testBug56425() {
ELProcessor processor = new ELProcessor();
processor.defineBean("string", "a-b-c-d");
Assert.assertEquals("a_b_c_d", processor.eval("string.replace(\"-\",\"_\")"));
}
}
6 changes: 6 additions & 0 deletions webapps/docs/changelog.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,12 @@
<bug>56334</bug>: Fix a regression in the handling of back-slash
escaping introduced by the fix for <bug>55735</bug>. (markt)
</fix>
<fix>
<bug>56425</bug>: Improve method matching for EL expressions. When
looking for matching methods, an exact match between parameter types is
preferred followed by an assignable match followed by a coercible match.
(markt)
</fix>
</changelog>
</subsection>
<subsection name="Cluster">
Expand Down

0 comments on commit cf76211

Please sign in to comment.