Issue #10661 Allow jetty api to override servlets and mappings from w…

…ebdefault
jetty · Oct 4, 2023 · 8c50e0b · 8c50e0b
1 parent ff25dd8
commit 8c50e0b
Show file tree

Hide file tree

Showing 4 changed files with 187 additions and 22 deletions.
diff --git a/...-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessor.java b/...-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessor.java
@@ -213,9 +213,18 @@ public void visitServlet(WebAppContext context, Descriptor descriptor, XmlParser
             _servletHolderMap.put(name, holder);
             _servletHolders.add(holder);
         }
+        else
+        {
+            //A servlet of the same name already exists. If it came from the jetty api
+            //and we're parsing webdefaults, then we will stop reading the descriptor to allow
+            //the api to define the default
+            if (Source.Origin.EMBEDDED == holder.getSource().getOrigin() && descriptor instanceof DefaultsDescriptor)
+                return;
+        }
 
         // init params
         Iterator<?> iParamsIter = node.iterator("init-param");
+
         while (iParamsIter.hasNext())
         {
             XmlParser.Node paramNode = (XmlParser.Node)iParamsIter.next();
@@ -1054,34 +1063,41 @@ public ServletMapping addServletMapping(String servletName, XmlParser.Node node,
                     {
                         //The same path has been mapped multiple times, either to a different servlet or the same servlet.
                         //If its a different servlet, this is only valid to do if the old mapping was from a default descriptor.
-                        if (p.equals(ps) && (sm.isFromDefaultDescriptor() || servletName.equals(sm.getServletName())))
+                        if (p.equals(ps))
                         {
-                            if (sm.isFromDefaultDescriptor())
-                            {
-                                if (LOG.isDebugEnabled())
-                                    LOG.debug("{} in mapping {} from defaults descriptor is overridden by {}", ps, sm, servletName);
-                            }
-                            else
-                                LOG.warn("Duplicate mapping from {} to {}", p, servletName);
-
-                            //remove ps from the path specs on the existing mapping
-                            //if the mapping now has no pathspecs, remove it
-                            String[] updatedPaths = ArrayUtil.removeFromArray(sm.getPathSpecs(), ps);
-
-                            if (updatedPaths == null || updatedPaths.length == 0)
+                            if (!servletName.equals(sm.getServletName()))
                             {
-                                if (LOG.isDebugEnabled())
-                                    LOG.debug("Removed empty mapping {}", sm);
-                                listItor.remove();
+                                if (!sm.isFromDefaultDescriptor())
+                                    throw new IllegalStateException("Duplicate mapping of " + p + ": " + servletName + "," + sm.getServletName());
+                                else
+                                {
+                                    if (LOG.isDebugEnabled())
+                                        LOG.debug("{} in mapping {} from defaults descriptor is overridden by {}", ps, sm, servletName);
+
+                                    //remove ps from the path specs on the existing mapping
+                                    //if the mapping now has no pathspecs, remove it
+                                    String[] updatedPaths = ArrayUtil.removeFromArray(sm.getPathSpecs(), ps);
+
+                                    if (updatedPaths == null || updatedPaths.length == 0)
+                                    {
+                                        if (LOG.isDebugEnabled())
+                                            LOG.debug("Removed empty mapping {}", sm);
+                                        listItor.remove();
+                                    }
+                                    else
+                                    {
+                                        sm.setPathSpecs(updatedPaths);
+                                        if (LOG.isDebugEnabled())
+                                            LOG.debug("Removed path {} from mapping {}", p, sm);
+                                    }
+                                    found = true;
+                                    break;
+                                }
                             }
                             else
                             {
-                                sm.setPathSpecs(updatedPaths);
-                                if (LOG.isDebugEnabled())
-                                    LOG.debug("Removed path {} from mapping {}", p, sm);
+                                LOG.warn("Duplicate mapping from {} to {}", p, servletName);
                             }
-                            found = true;
-                            break;
                         }
                     }
                 }

diff --git a/...0-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessorTest.java b/...0-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessorTest.java
@@ -14,11 +14,15 @@
 package org.eclipse.jetty.ee10.webapp;
 
 import java.io.File;
+import java.lang.reflect.InvocationTargetException;
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 
+import org.eclipse.jetty.ee10.servlet.DefaultServlet;
+import org.eclipse.jetty.ee10.servlet.ServletHolder;
+import org.eclipse.jetty.logging.StacklessLogging;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
@@ -33,6 +37,8 @@
 import static org.hamcrest.Matchers.equalToIgnoringCase;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 @ExtendWith(WorkDirExtension.class)
 public class StandardDescriptorProcessorTest
@@ -53,6 +59,104 @@ public void afterEach() throws Exception
         _server.stop();
     }
 
+    @Test
+    public void testJettyApiDefaults(WorkDir workDir) throws Exception
+    {
+        //Test that the DefaultServlet named "default" defined by jetty api is not redefined by webdefault-ee10.xml
+        Path docroot = workDir.getEmptyPathDir();
+        WebAppContext wac = new WebAppContext();
+        wac.setServer(_server);
+        wac.setBaseResourceAsPath(docroot);
+        ServletHolder defaultServlet = new ServletHolder(DefaultServlet.class);
+        defaultServlet.setName("default");
+        defaultServlet.setInitParameter("acceptRanges", "false");
+        defaultServlet.setInitParameter("dirAllowed", "false");
+        defaultServlet.setInitParameter("welcomeServlets", "true");
+        defaultServlet.setInitParameter("redirectWelcome", "true");
+        defaultServlet.setInitParameter("maxCacheSize", "10");
+        defaultServlet.setInitParameter("maxCachedFileSize", "1");
+        defaultServlet.setInitParameter("maxCacheFiles", "10");
+        defaultServlet.setInitParameter("etags", "true");
+        defaultServlet.setInitParameter("useFileMappedBuffer", "false");
+        defaultServlet.setInitOrder(2);
+        defaultServlet.setRunAsRole("foo");
+        wac.getServletHandler().addServletWithMapping(defaultServlet, "/");
+        wac.start();
+
+        ServletHolder[] holders = wac.getServletHandler().getServlets();
+        ServletHolder holder = null;
+        for (ServletHolder h:holders)
+        {
+            if ("default".equals(h.getName()))
+            {
+                assertEquals(null, holder);
+                holder = h;
+            }
+        }
+        assertNotNull(holder);
+        assertEquals("false", holder.getInitParameter("acceptRanges"));
+        assertEquals("false", holder.getInitParameter("dirAllowed"));
+        assertEquals("true", holder.getInitParameter("welcomeServlets"));
+        assertEquals("true", holder.getInitParameter("redirectWelcome"));
+        assertEquals("10", holder.getInitParameter("maxCacheSize"));
+        assertEquals("1", holder.getInitParameter("maxCachedFileSize"));
+        assertEquals("10", holder.getInitParameter("maxCacheFiles"));
+        assertEquals("true", holder.getInitParameter("etags"));
+        assertEquals("false", holder.getInitParameter("useFileMappedBuffer"));
+        assertEquals(2, holder.getInitOrder());
+        assertEquals("foo", holder.getRunAsRole());
+    }
+
+    @Test
+    public void testDuplicateServletMappingsFromJettyApi(WorkDir workDir) throws Exception
+    {
+        Path docroot = workDir.getEmptyPathDir();
+        WebAppContext wac = new WebAppContext();
+        wac.setServer(_server);
+        wac.setBaseResourceAsPath(docroot);
+        wac.setThrowUnavailableOnStartupException(true);
+        //add a mapping that will conflict with the webdefault-ee10.xml mapping
+        //for the default servlet
+        ServletHolder defaultServlet = new ServletHolder(DefaultServlet.class);
+        defaultServlet.setName("noname");
+        wac.getServletHandler().addServletWithMapping(defaultServlet, "/");
+        try (StacklessLogging ignored = new StacklessLogging(WebAppContext.class))
+        {
+            assertThrows(InvocationTargetException.class, () -> wac.start());
+        }
+    }
+
+    @Test
+    public void testDuplicateServletMappingsFromDescriptors(WorkDir workDir) throws Exception
+    {
+        //Test that the DefaultServlet mapping from webdefault-ee10.xml can be overridden in web.xml
+        Path docroot = workDir.getEmptyPathDir();
+        File webXml = MavenTestingUtils.getTestResourceFile("web-redefine-mapping.xml");
+        WebAppContext wac = new WebAppContext();
+        wac.setServer(_server);
+        wac.setBaseResourceAsPath(docroot);
+        wac.setDescriptor(webXml.toURI().toURL().toString());
+        wac.start();
+        assertEquals("other", wac.getServletHandler().getServletMapping("/").getServletName());
+    }
+
+    @Test
+    public void testBadDuplicateServletMappingsFromDescriptors(WorkDir workDir) throws Exception
+    {
+        //Test that the same mapping cannot be redefined to a different servlet in the same (non-default) descriptor
+        Path docroot = workDir.getEmptyPathDir();
+        File webXml = MavenTestingUtils.getTestResourceFile("web-redefine-mapping-fail.xml");
+        WebAppContext wac = new WebAppContext();
+        wac.setServer(_server);
+        wac.setBaseResourceAsPath(docroot);
+        wac.setDescriptor(webXml.toURI().toURL().toString());
+        wac.setThrowUnavailableOnStartupException(true);
+        try (StacklessLogging ignored = new StacklessLogging(WebAppContext.class))
+        {
+            assertThrows(InvocationTargetException.class, () -> wac.start());
+        }
+    }
+
     @Test
     public void testVisitSessionConfig(WorkDir workDir) throws Exception
     {

diff --git a/jetty-ee10/jetty-ee10-webapp/src/test/resources/web-redefine-mapping-fail.xml b/jetty-ee10/jetty-ee10-webapp/src/test/resources/web-redefine-mapping-fail.xml
@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<web-app xmlns="https://jakarta.ee/xml/ns/jakartaee"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="https://jakarta.ee/xml/ns/jakartaee https://jakarta.ee/xml/ns/jakartaee/web-app_6_0.xsd"
+	version="6.0">
+
+	<display-name>Bad Redefine Default Servlet Mapping WebApp</display-name>
+  <servlet>
+    <servlet-name>first</servlet-name>
+    <servlet-class>org.acme.webapp.GetResourceServlet</servlet-class>
+  </servlet>
+
+  <servlet-mapping>
+    <servlet-name>first</servlet-name>
+    <url-pattern>/x</url-pattern>
+  </servlet-mapping>
+
+  <servlet>
+    <servlet-name>second</servlet-name>
+    <servlet-class>org.acme.webapp.GetResourceServlet</servlet-class>
+  </servlet>
+
+  <servlet-mapping>
+    <servlet-name>second</servlet-name>
+    <url-pattern>/x</url-pattern>
+  </servlet-mapping>
+</web-app>
diff --git a/jetty-ee10/jetty-ee10-webapp/src/test/resources/web-redefine-mapping.xml b/jetty-ee10/jetty-ee10-webapp/src/test/resources/web-redefine-mapping.xml
@@ -0,0 +1,18 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<web-app xmlns="https://jakarta.ee/xml/ns/jakartaee"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="https://jakarta.ee/xml/ns/jakartaee https://jakarta.ee/xml/ns/jakartaee/web-app_6_0.xsd"
+	version="6.0">
+
+	<display-name>Redefine Default Servlet Mapping WebApp</display-name>
+  <servlet>
+    <servlet-name>other</servlet-name>
+    <servlet-class>org.acme.webapp.GetResourceServlet</servlet-class>
+  </servlet>
+
+  <servlet-mapping>
+    <servlet-name>other</servlet-name>
+    <url-pattern>/</url-pattern>
+  </servlet-mapping>
+
+</web-app>