Return-Path: 
 <dev-return-149576-apmail-tomcat-dev-archive=tomcat.apache.org@tomcat.apache.org>
X-Original-To: apmail-tomcat-dev-archive@www.apache.org
Delivered-To: apmail-tomcat-dev-archive@www.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id 579BAFB06
	for <apmail-tomcat-dev-archive@www.apache.org>;
 Fri, 25 Apr 2014 19:50:13 +0000 (UTC)
Received: (qmail 78179 invoked by uid 500); 25 Apr 2014 19:50:08 -0000
Delivered-To: apmail-tomcat-dev-archive@tomcat.apache.org
Received: (qmail 78110 invoked by uid 500); 25 Apr 2014 19:50:08 -0000
Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@tomcat.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@tomcat.apache.org>
List-Post: <mailto:dev@tomcat.apache.org>
List-Id: <dev.tomcat.apache.org>
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Delivered-To: mailing list dev@tomcat.apache.org
Received: (qmail 78101 invoked by uid 99); 25 Apr 2014 19:50:08 -0000
Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136)
    by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 25 Apr 2014 19:50:08 +0000
X-ASF-Spam-Status: No, hits=-2000.0 required=5.0
	tests=ALL_TRUSTED
X-Spam-Check-By: apache.org
Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4)
    by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 25 Apr 2014 19:50:06 +0000
Received: from eris.apache.org (localhost [127.0.0.1])
	by eris.apache.org (Postfix) with ESMTP id CE44B23888E4
	for <dev@tomcat.apache.org>; Fri, 25 Apr 2014 19:49:46 +0000 (UTC)
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
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 -0000
To: dev@tomcat.apache.org
From: markt@apache.org
X-Mailer: svnmailer-1.0.9
Message-Id: <20140425194946.CE44B23888E4@eris.apache.org>
X-Virus-Checked: Checked by ClamAV on apache.org

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