Author: markt Date: Wed Oct 13 11:10:30 2010 New Revision: 1022068 URL: http://svn.apache.org/viewvc?rev=1022068&view=rev Log: 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
Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationFilterChain.java tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java tomcat/trunk/test/org/apache/catalina/core/TestStandardContext.java tomcat/trunk/test/org/apache/catalina/startup/TestContextConfigAnnotation.java tomcat/trunk/test/webapp-3.0/WEB-INF/web.xml tomcat/trunk/webapps/docs/changelog.xml Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationFilterChain.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ApplicationFilterChain.java?rev=1022068&r1=1022067&r2=1022068&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/core/ApplicationFilterChain.java (original) +++ tomcat/trunk/java/org/apache/catalina/core/ApplicationFilterChain.java Wed Oct 13 11:10:30 2010 @@ -525,6 +525,11 @@ final class ApplicationFilterChain imple */ 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]; Modified: tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java?rev=1022068&r1=1022067&r2=1022068&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java (original) +++ tomcat/trunk/java/org/apache/catalina/deploy/WebXml.java Wed Oct 13 11:10:30 2010 @@ -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); } Modified: tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java?rev=1022068&r1=1022067&r2=1022068&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java (original) +++ tomcat/trunk/java/org/apache/catalina/startup/ContextConfig.java Wed Oct 13 11:10:30 2010 @@ -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())) { Modified: tomcat/trunk/test/org/apache/catalina/core/TestStandardContext.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/core/TestStandardContext.java?rev=1022068&r1=1022067&r2=1022068&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/catalina/core/TestStandardContext.java (original) +++ tomcat/trunk/test/org/apache/catalina/core/TestStandardContext.java Wed Oct 13 11:10:30 2010 @@ -126,23 +126,43 @@ public class TestStandardContext extends 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 } } + 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 @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { - // NOOP + resp.setContentType("text/plain"); + resp.getWriter().print("Servlet"); } } Modified: tomcat/trunk/test/org/apache/catalina/startup/TestContextConfigAnnotation.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/startup/TestContextConfigAnnotation.java?rev=1022068&r1=1022067&r2=1022068&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/catalina/startup/TestContextConfigAnnotation.java (original) +++ tomcat/trunk/test/org/apache/catalina/startup/TestContextConfigAnnotation.java Wed Oct 13 11:10:30 2010 @@ -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 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 Modified: tomcat/trunk/test/webapp-3.0/WEB-INF/web.xml URL: http://svn.apache.org/viewvc/tomcat/trunk/test/webapp-3.0/WEB-INF/web.xml?rev=1022068&r1=1022067&r2=1022068&view=diff ============================================================================== --- tomcat/trunk/test/webapp-3.0/WEB-INF/web.xml (original) +++ tomcat/trunk/test/webapp-3.0/WEB-INF/web.xml Wed Oct 13 11:10:30 2010 @@ -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> Modified: tomcat/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1022068&r1=1022067&r2=1022068&view=diff ============================================================================== --- tomcat/trunk/webapps/docs/changelog.xml (original) +++ tomcat/trunk/webapps/docs/changelog.xml Wed Oct 13 11:10:30 2010 @@ -45,7 +45,8 @@ </fix> <fix> <bug>49922</bug>: Don'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 --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org