tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1590120 - in /tomcat/trunk: java/javax/el/Util.java java/org/apache/el/util/ReflectionUtil.java test/javax/el/TestUtil.java webapps/docs/changelog.xml
Date Fri, 25 Apr 2014 19:49:46 GMT
Author: markt
Date: Fri Apr 25 19:49:46 2014
New Revision: 1590120

URL: http://svn.apache.org/r1590120
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56425
 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.

Modified:
    tomcat/trunk/java/javax/el/Util.java
    tomcat/trunk/java/org/apache/el/util/ReflectionUtil.java
    tomcat/trunk/test/javax/el/TestUtil.java
    tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/javax/el/Util.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/Util.java?rev=1590120&r1=1590119&r2=1590120&view=diff
==============================================================================
--- tomcat/trunk/java/javax/el/Util.java (original)
+++ tomcat/trunk/java/javax/el/Util.java Fri Apr 25 19:49:46 2014
@@ -228,7 +228,7 @@ class Util {
     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) {
@@ -255,6 +255,8 @@ class Util {
 
             // 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
@@ -263,12 +265,16 @@ class Util {
                 } 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;
                                 }
@@ -278,12 +284,16 @@ class Util {
                         // 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;
                         }
@@ -300,26 +310,26 @@ class Util {
                 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);
@@ -700,4 +710,56 @@ class Util {
             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;
+                    }
+                }
+            }
+        }
+    }
 }

Modified: tomcat/trunk/java/org/apache/el/util/ReflectionUtil.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/util/ReflectionUtil.java?rev=1590120&r1=1590119&r2=1590120&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/el/util/ReflectionUtil.java (original)
+++ tomcat/trunk/java/org/apache/el/util/ReflectionUtil.java Fri Apr 25 19:49:46 2014
@@ -138,7 +138,7 @@ public class ReflectionUtil {
         }
 
         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)) {
@@ -163,6 +163,8 @@ public class ReflectionUtil {
 
             // 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
@@ -171,12 +173,16 @@ public class ReflectionUtil {
                 } 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;
                                 }
@@ -186,12 +192,16 @@ public class ReflectionUtil {
                         // 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;
                         }
@@ -208,26 +218,26 @@ public class ReflectionUtil {
                 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);
@@ -430,4 +440,57 @@ public class ReflectionUtil {
         }
         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;
+                    }
+                }
+            }
+        }
+    }
+
 }

Modified: tomcat/trunk/test/javax/el/TestUtil.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/javax/el/TestUtil.java?rev=1590120&r1=1590119&r2=1590120&view=diff
==============================================================================
--- tomcat/trunk/test/javax/el/TestUtil.java (original)
+++ tomcat/trunk/test/javax/el/TestUtil.java Fri Apr 25 19:49:46 2014
@@ -38,4 +38,12 @@ public class TestUtil {
         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(\"-\",\"_\")"));
+    }
 }

Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1590120&r1=1590119&r2=1590120&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Fri Apr 25 19:49:46 2014
@@ -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">



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message