From: markt <markt@13f79535-47bb-0310-9956-ffa450edef68>
Date: Wed, 13 Oct 2010 11:10:30 +0000 (+0000)
Subject: Better fix for https://issues.apache.org/bugzilla/show_bug.cgi?id=49922
X-Git-Url: https://git.internetallee.de/?a=commitdiff_plain;h=0d2c540f3cbe39b419d7743848d8cc85e3b64b59;p=tomcat7.0

Better fix for https://issues.apache.org/bugzilla/show_bug.cgi?id=49922
Revert my approach and go with patch suggested by heyoulin.
Extend test cases

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1022068 13f79535-47bb-0310-9956-ffa450edef68
---

diff --git a/java/org/apache/catalina/core/ApplicationFilterChain.java b/java/org/apache/catalina/core/ApplicationFilterChain.java
index 847c2b66a..395c2a590 100644
--- a/java/org/apache/catalina/core/ApplicationFilterChain.java
+++ b/java/org/apache/catalina/core/ApplicationFilterChain.java
@@ -525,6 +525,11 @@ final class ApplicationFilterChain implements FilterChain, CometFilterChain {
      */
     void addFilter(ApplicationFilterConfig filterConfig) {
 
+        // Prevent the same filter being added multiple times
+        for(ApplicationFilterConfig filter:filters)
+            if(filter==filterConfig)
+                return;
+
         if (n == filters.length) {
             ApplicationFilterConfig[] newFilters =
                 new ApplicationFilterConfig[n + INCREMENT];
diff --git a/java/org/apache/catalina/deploy/WebXml.java b/java/org/apache/catalina/deploy/WebXml.java
index a0b79fa1c..230e9fc80 100644
--- a/java/org/apache/catalina/deploy/WebXml.java
+++ b/java/org/apache/catalina/deploy/WebXml.java
@@ -278,37 +278,13 @@ public class WebXml {
     public Map<String,FilterDef> getFilters() { return filters; }
     
     // filter-mapping
-    private Map<String,FilterMap> filterMaps =
-        new LinkedHashMap<String,FilterMap>();
+    private Set<FilterMap> filterMaps = new LinkedHashSet<FilterMap>();
+    private Set<String> filterMappingNames = new HashSet<String>();
     public void addFilterMapping(FilterMap filterMap) {
-        FilterMap fm = filterMaps.get(filterMap.getFilterName());
-        if (fm == null) {
-            filterMaps.put(filterMap.getFilterName(), filterMap);
-        } else {
-            for (String dispatcher : filterMap.getDispatcherNames()) {
-                fm.setDispatcher(dispatcher);
-            }
-            if (!fm.getMatchAllServletNames()) {
-                if (filterMap.getMatchAllServletNames()) {
-                    fm.addServletName("*");
-                } else {
-                    for (String servletName : filterMap.getServletNames()) {
-                        fm.addServletName(servletName);
-                    }
-                }
-            }
-            if (!fm.getMatchAllUrlPatterns()) {
-                if (filterMap.getMatchAllUrlPatterns()) {
-                    fm.addURLPattern("*");
-                } else {
-                    for (String urlPattern : filterMap.getURLPatterns()) {
-                        fm.addURLPattern(urlPattern);
-                    }
-                }
-            }
-        }
+        filterMaps.add(filterMap);
+        filterMappingNames.add(filterMap.getFilterName());
     }
-    public Map<String,FilterMap> getFilterMappings() { return filterMaps; }
+    public Set<FilterMap> getFilterMappings() { return filterMaps; }
     
     // listener
     // TODO: description (multiple with language) is ignored
@@ -651,7 +627,7 @@ public class WebXml {
         }
         sb.append('\n');
 
-        for (FilterMap filterMap : filterMaps.values()) {
+        for (FilterMap filterMap : filterMaps) {
             sb.append("  <filter-mapping>\n");
             appendElement(sb, INDENT4, "filter-name",
                     filterMap.getFilterName());
@@ -1200,7 +1176,7 @@ public class WebXml {
             }
             context.addFilterDef(filter);
         }
-        for (FilterMap filterMap : filterMaps.values()) {
+        for (FilterMap filterMap : filterMaps) {
             context.addFilterMap(filterMap);
         }
         for (JspPropertyGroup jspPropertyGroup : jspPropertyGroups) {
@@ -1442,16 +1418,17 @@ public class WebXml {
         // main web.xml override those in fragments and those in fragments
         // override mappings in annotations
         for (WebXml fragment : fragments) {
-            Iterator<String> iterFilterMaps =
-                fragment.getFilterMappings().keySet().iterator();
+            Iterator<FilterMap> iterFilterMaps =
+                fragment.getFilterMappings().iterator();
             while (iterFilterMaps.hasNext()) {
-                if (filterMaps.containsKey(iterFilterMaps.next())) {
+                FilterMap filterMap = iterFilterMaps.next();
+                if (filterMappingNames.contains(filterMap.getFilterName())) {
                     iterFilterMaps.remove();
                 }
             }
         }
         for (WebXml fragment : fragments) {
-            for (FilterMap filterMap : fragment.getFilterMappings().values()) {
+            for (FilterMap filterMap : fragment.getFilterMappings()) {
                 // Additive
                 addFilterMapping(filterMap);
             }
diff --git a/java/org/apache/catalina/startup/ContextConfig.java b/java/org/apache/catalina/startup/ContextConfig.java
index 1900fa338..a04b97a16 100644
--- a/java/org/apache/catalina/startup/ContextConfig.java
+++ b/java/org/apache/catalina/startup/ContextConfig.java
@@ -33,7 +33,6 @@ import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -2210,7 +2209,7 @@ public class ContextConfig
             fragment.addFilterMapping(filterMap);
         }
         if (urlPatternsSet || dispatchTypesSet) {
-            Collection<FilterMap> fmap = fragment.getFilterMappings().values();
+            Set<FilterMap> fmap = fragment.getFilterMappings();
             FilterMap descMap = null;
             for (FilterMap map : fmap) {
                 if (filterName.equals(map.getFilterName())) {
diff --git a/test/org/apache/catalina/core/TestStandardContext.java b/test/org/apache/catalina/core/TestStandardContext.java
index 6ea70a148..3c12da721 100644
--- a/test/org/apache/catalina/core/TestStandardContext.java
+++ b/test/org/apache/catalina/core/TestStandardContext.java
@@ -126,23 +126,43 @@ public class TestStandardContext extends TomcatBaseTest {
         tomcat.addWebapp("", root.getAbsolutePath());
         
         tomcat.start();
+        ByteChunk result;
 
-        // Check path mapping works
-        ByteChunk result = getUrl("http://localhost:" + getPort() +
-        "/bug49922/foo");
-        // Filter should only have been called once
-        assertEquals("Filter", result.toString());
+        // Check filter and servlet aren't called
+        result = getUrl("http://localhost:" + getPort() +
+                "/bug49922/foo");
+        assertNull(result.toString());
 
         // Check extension mapping works
+        result = getUrl("http://localhost:" + getPort() + "/foo.do");
+        assertEquals("FilterServlet", result.toString());
+
+        // Check path mapping works
+        result = getUrl("http://localhost:" + getPort() + "/bug49922/servlet");
+        assertEquals("FilterServlet", result.toString());
+
+        // Check servlet name mapping works
+        result = getUrl("http://localhost:" + getPort() + "/foo.od");
+        assertEquals("FilterServlet", result.toString());
+
+        // Check filter is only called once
+        result = getUrl("http://localhost:" + getPort() +
+                "/bug49922/servlet/foo.do");
+        assertEquals("FilterServlet", result.toString());
         result = getUrl("http://localhost:" + getPort() +
-        "/foo.do");
-        // Filter should only have been called once
-        assertEquals("Filter", result.toString());
+                "/bug49922/servlet/foo.od");
+        assertEquals("FilterServlet", result.toString());
 
+        // Check dispatcher mapping
+        result = getUrl("http://localhost:" + getPort() +
+                "/bug49922/target");
+        assertEquals("Target", result.toString());
         result = getUrl("http://localhost:" + getPort() +
-                "/bug49922/index.do");
-        // Filter should only have been called once
-        assertEquals("Filter", result.toString());
+                "/bug49922/forward");
+        assertEquals("FilterTarget", result.toString());
+        result = getUrl("http://localhost:" + getPort() +
+                "/bug49922/include");
+        assertEquals("IncludeFilterTarget", result.toString());
     }
 
     
@@ -167,6 +187,45 @@ public class TestStandardContext extends TomcatBaseTest {
         }
     }
     
+    public static final class Bug49922ForwardServlet extends HttpServlet {
+
+        private static final long serialVersionUID = 1L;
+
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+                throws ServletException, IOException {
+            req.getRequestDispatcher("/bug49922/target").forward(req, resp);
+        }
+        
+    }
+
+    public static final class Bug49922IncludeServlet extends HttpServlet {
+
+        private static final long serialVersionUID = 1L;
+
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+                throws ServletException, IOException {
+            resp.setContentType("text/plain");
+            resp.getWriter().print("Include");
+            req.getRequestDispatcher("/bug49922/target").include(req, resp);
+        }
+        
+    }
+
+    public static final class Bug49922TargetServlet extends HttpServlet {
+
+        private static final long serialVersionUID = 1L;
+
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+                throws ServletException, IOException {
+            resp.setContentType("text/plain");
+            resp.getWriter().print("Target");
+        }
+        
+    }
+
     public static final class Bug49922Servlet extends HttpServlet {
 
         private static final long serialVersionUID = 1L;
@@ -174,7 +233,8 @@ public class TestStandardContext extends TomcatBaseTest {
         @Override
         protected void doGet(HttpServletRequest req, HttpServletResponse resp)
                 throws ServletException, IOException {
-            // NOOP
+            resp.setContentType("text/plain");
+            resp.getWriter().print("Servlet");
         }
         
     }
diff --git a/test/org/apache/catalina/startup/TestContextConfigAnnotation.java b/test/org/apache/catalina/startup/TestContextConfigAnnotation.java
index bfc53df64..cabf5e77e 100644
--- a/test/org/apache/catalina/startup/TestContextConfigAnnotation.java
+++ b/test/org/apache/catalina/startup/TestContextConfigAnnotation.java
@@ -18,7 +18,7 @@ package org.apache.catalina.startup;
 
 import java.io.File;
 import java.net.URL;
-import java.util.Collection;
+import java.util.Set;
 
 import javax.servlet.DispatcherType;
 
@@ -201,8 +201,7 @@ public class TestContextConfigAnnotation extends TestCase {
         assertNotNull(fdef);
         assertEquals(filterDef,fdef);
         assertEquals("tomcat",fdef.getParameterMap().get("message"));
-        Collection<FilterMap> filterMappings =
-            webxml.getFilterMappings().values();
+        Set<FilterMap> filterMappings = webxml.getFilterMappings();
         assertTrue(filterMappings.contains(filterMap));
         // annotation mapping not added s. Servlet Spec 3.0 (Nov 2009)
         // 8.2.3.3.vi page 81
diff --git a/test/webapp-3.0/WEB-INF/web.xml b/test/webapp-3.0/WEB-INF/web.xml
index 9d975fd8b..71ae7d9af 100644
--- a/test/webapp-3.0/WEB-INF/web.xml
+++ b/test/webapp-3.0/WEB-INF/web.xml
@@ -37,13 +37,47 @@
   </filter>
   <filter-mapping>
     <filter-name>Bug49922</filter-name>
-    <url-pattern>/bug49922/*</url-pattern>
+    <url-pattern>/bug49922/servlet/*</url-pattern>
+    <url-pattern>*.do</url-pattern>
+    <servlet-name>Bug49922</servlet-name>
   </filter-mapping>
   <filter-mapping>
     <filter-name>Bug49922</filter-name>
-    <url-pattern>*.do</url-pattern>
+    <dispatcher>FORWARD</dispatcher>
+    <dispatcher>INCLUDE</dispatcher>
+    <servlet-name>Bug49922Target</servlet-name>
   </filter-mapping>
   <servlet>
+    <servlet-name>Bug49922Forward</servlet-name>
+    <servlet-class>
+      org.apache.catalina.core.TestStandardContext$Bug49922ForwardServlet
+    </servlet-class>
+  </servlet>
+  <servlet-mapping>
+    <servlet-name>Bug49922Forward</servlet-name>
+    <url-pattern>/bug49922/forward</url-pattern>
+  </servlet-mapping>
+  <servlet>
+    <servlet-name>Bug49922Include</servlet-name>
+    <servlet-class>
+      org.apache.catalina.core.TestStandardContext$Bug49922IncludeServlet
+    </servlet-class>
+  </servlet>
+  <servlet-mapping>
+    <servlet-name>Bug49922Include</servlet-name>
+    <url-pattern>/bug49922/include</url-pattern>
+  </servlet-mapping>
+  <servlet>
+    <servlet-name>Bug49922Target</servlet-name>
+    <servlet-class>
+      org.apache.catalina.core.TestStandardContext$Bug49922TargetServlet
+    </servlet-class>
+  </servlet>
+  <servlet-mapping>
+    <servlet-name>Bug49922Target</servlet-name>
+    <url-pattern>/bug49922/target</url-pattern>
+  </servlet-mapping>
+  <servlet>
     <servlet-name>Bug49922</servlet-name>
     <servlet-class>
       org.apache.catalina.core.TestStandardContext$Bug49922Servlet
@@ -51,12 +85,16 @@
   </servlet>
   <servlet-mapping>
     <servlet-name>Bug49922</servlet-name>
-    <url-pattern>/bug49922/*</url-pattern>
+    <url-pattern>/bug49922/servlet</url-pattern>
   </servlet-mapping>
   <servlet-mapping>
     <servlet-name>Bug49922</servlet-name>
     <url-pattern>*.do</url-pattern>
   </servlet-mapping>
+  <servlet-mapping>
+    <servlet-name>Bug49922</servlet-name>
+    <url-pattern>*.od</url-pattern>
+  </servlet-mapping>
   
   <jsp-config>
     <jsp-property-group>
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index bccc98cff..e67cd2306 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -45,7 +45,8 @@
       </fix>
       <fix>
         <bug>49922</bug>: Don&apos;t add filter twice to filter chain if the
-        filter matches more than one URL pattern and/or Servlet name. (markt)
+        filter matches more than one URL pattern and/or Servlet name. Patch
+        provided by heyoulin. (markt)
       </fix>
       <fix>
         <bug>49937</bug>: Use an InstanceManager when creating an AsyncListener