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

jetty-ee10/jetty-ee10-tests/jetty-ee10-test-cdi/src/test/java/org/eclipse/jetty/ee10/cdi/tests/EmbeddedWeldTest.java

@@ -207,14 +207,14 @@ public void testWebappContext() throws Exception
         webapp.getServerClassMatcher().add("-" + pkg + ".");
         webapp.getSystemClassMatcher().add(pkg + ".");
 
-        webapp.addServlet(GreetingsServlet.class, "/");
+        webapp.addServlet(GreetingsServlet.class, "/greet");
         webapp.addFilter(MyFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
         webapp.getServletHandler().addListener(new ListenerHolder(MyContextListener.class));
 
         server.start();
 
         LocalConnector connector = server.getBean(LocalConnector.class);
-        String response = connector.getResponse("GET / HTTP/1.0\r\n\r\n");
+        String response = connector.getResponse("GET /greet HTTP/1.0\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 200 OK"));
         assertThat(response, containsString("Hello GreetingsServlet filtered by Weld BeanManager "));
         assertThat(response, containsString("Beans from Weld BeanManager "));
@@ -245,12 +245,12 @@ public void testWebappContextDiscovered() throws Exception
 
         webapp.getServletHandler().addListener(new ListenerHolder(MyContextListener.class));
         webapp.addFilter(MyFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
-        webapp.addServlet(GreetingsServlet.class, "/");
+        webapp.addServlet(GreetingsServlet.class, "/greet");
 
         server.start();
 
         LocalConnector connector = server.getBean(LocalConnector.class);
-        String response = connector.getResponse("GET / HTTP/1.0\r\n\r\n");
+        String response = connector.getResponse("GET /greet HTTP/1.0\r\n\r\n");
         assertThat(response, containsString("HTTP/1.1 200 OK"));
         assertThat(response, containsString("Hello GreetingsServlet filtered by Weld BeanManager "));
         assertThat(response, containsString("Beans from Weld BeanManager "));

jetty-ee10/jetty-ee10-webapp/pom.xml

@@ -87,7 +87,7 @@
         <artifactId>maven-surefire-plugin</artifactId>
         <configuration>
           <argLine>@{argLine} ${jetty.surefire.argLine}
-            --add-exports org.eclipse.jetty.ee10.webapp/org.acme.webapp=org.eclipse.jetty.ee10.servlet</argLine>
+            --add-exports org.eclipse.jetty.ee10.webapp/org.acme.webapp=org.eclipse.jetty.ee10.servlet --add-reads org.eclipse.jetty.ee10.webapp=org.eclipse.jetty.logging</argLine>
           <useManifestOnlyJar>false</useManifestOnlyJar>
           <additionalClasspathElements>
             <additionalClasspathElement>${basedir}/src/test/resources/mods/foo-bar-janb.jar</additionalClasspathElement>

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessor.java

@@ -23,6 +23,7 @@
 import java.util.ListIterator;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
@@ -213,9 +214,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 visiting this Servlet to allow
+            //the api to define the defaults rather than webdefaults
+            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();
@@ -1028,70 +1038,102 @@ public void addWelcomeFiles(WebAppContext context, XmlParser.Node node, Descript
         }
     }
 
+    /**
+     * Resolve any duplicate servlet path mappings.
+     * A path can be (re)mapped iff:
+     * <ul>
+     *     <li>it is not already mapped</li>
+     *     <li>it is already mapped by webdefault.xml</li>
+     *     <li>it is already mapped by the embedded api, and the descriptor is not webdefault.xml</li>
+     * </ul>
+     * The effect of the above conditions is to produce the following precedence hierarchy:
+     * <ol>
+     *     <li>jetty-override.xml (OverrideDescriptor)</li>
+     *     <li>web.xml (WebDescriptor)</li>
+     *     <li>web-fragment.xml (FragmentDescriptor)</li>
+     *     <li>embedded api</li>
+     *     <li>webdefault.xml (DefaultsDescriptor)</li>
+     * </ol>
+     * @param servletName the servlet target of the mapping
+     * @param path the path to check
+     * @param context the context of the mapping
+     * @param descriptor the descriptor currently being parsed
+     * @return true if the path can be mapped, false otherwise
+     * @throws IllegalStateException if the duplicate mappings are illegal
+     */
+    private boolean resolveAnyDuplicateServletPathMapping(String servletName, String path, WebAppContext context, Descriptor descriptor)
+    {
+        Objects.requireNonNull(servletName);
+        Objects.requireNonNull(path);
+        Objects.requireNonNull(context);
+        Objects.requireNonNull(descriptor);
+
+        //Find any duplicate mapping
+        ServletMapping existing = null;
+        loop: for (ServletMapping existingMapping: _servletMappings)
+        {
+            for (String pathSpec : existingMapping.getPathSpecs())
+            {
+                if (Objects.equals(pathSpec, path))
+                {
+                    existing = existingMapping;
+                    break loop;
+                }
+            }
+        }
+
+        //If there is no duplicate we can add it
+        if (existing == null)
+            return true;
+
+        //If it is the same name we can ignore it
+        if (Objects.equals(existing.getServletName(), servletName))
+            return false;
+
+        //A defaults descriptor cannot override embedded.
+        if (existing.getSource().getOrigin() == Source.Origin.EMBEDDED && descriptor instanceof DefaultsDescriptor)
+            return false;
+
+        //If the descriptor of the original mapping and the new mapping are the same, that is an error
+        if (existing.getSource().getOrigin() == Source.Origin.DESCRIPTOR && existing.getSource().getResource().equals(descriptor.getResource()))
+            throw new IllegalStateException("Duplicate mappings for " + path);
+
+        //Remove existing duplicate mapping and tidy up by removing the originMapping if it now has no paths
+        if (LOG.isDebugEnabled())
+            LOG.debug("Removed path {} from mapping {}", path, existing);
+        existing.setPathSpecs(ArrayUtil.removeFromArray(existing.getPathSpecs(), path));
+        if (existing.getPathSpecs() == null || existing.getPathSpecs().length == 0)
+            _servletMappings.remove(existing);
+
+        return true;
+    }
+
     public ServletMapping addServletMapping(String servletName, XmlParser.Node node, WebAppContext context, Descriptor descriptor)
     {
-        ServletMapping mapping = new ServletMapping(new Source(Source.Origin.DESCRIPTOR, descriptor.getResource()));
-        mapping.setServletName(servletName);
-        mapping.setFromDefaultDescriptor(descriptor instanceof DefaultsDescriptor);
-        context.getMetaData().setOrigin(servletName + ".servlet.mapping." + Long.toHexString(mapping.hashCode()), descriptor);
-
         List<String> paths = new ArrayList<String>();
         Iterator<XmlParser.Node> iter = node.iterator("url-pattern");
+        //For each url pattern, check if that has already been mapped or not
         while (iter.hasNext())
         {
-            String p = iter.next().toString(false, true);
-            p = ServletPathSpec.normalize(p);
+            String path = iter.next().toString(false, true);
+            path = ServletPathSpec.normalize(path);
 
             //check if there is already a mapping for this path
-            ListIterator<ServletMapping> listItor = _servletMappings.listIterator();
-            boolean found = false;
-            while (listItor.hasNext() && !found)
+            if (resolveAnyDuplicateServletPathMapping(servletName, path, context, descriptor))
             {
-                ServletMapping sm = listItor.next();
-                if (sm.getPathSpecs() != null)
-                {
-                    for (String ps : sm.getPathSpecs())
-                    {
-                        //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 (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 (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;
-                        }
-                    }
-                }
+                paths.add(path);
+                context.getMetaData().setOrigin(servletName + ".servlet.mapping.url" + path, descriptor);
             }
-
-            paths.add(p);
-            context.getMetaData().setOrigin(servletName + ".servlet.mapping.url" + p, descriptor);
         }
 
-        mapping.setPathSpecs((String[])paths.toArray(new String[paths.size()]));
+        if (paths.isEmpty())
+            return null;  //no paths were added, skip adding a ServletMapping
+
+        ServletMapping mapping = new ServletMapping(new Source(Source.Origin.DESCRIPTOR, descriptor.getResource()));
+        mapping.setServletName(servletName);
+        mapping.setFromDefaultDescriptor(descriptor instanceof DefaultsDescriptor);
+        context.getMetaData().setOrigin(servletName + ".servlet.mapping." + Long.toHexString(mapping.hashCode()), descriptor);
+        mapping.setPathSpecs(paths.toArray(new String[0]));
         if (LOG.isDebugEnabled())
             LOG.debug("Added mapping {} ", mapping);
         _servletMappings.add(mapping);

jetty-ee10/jetty-ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessorTest.java

@@ -14,11 +14,16 @@
 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.ee10.servlet.ServletMapping;
+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;
@@ -31,8 +36,12 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.equalToIgnoringCase;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
 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 +62,126 @@ 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()))
+            {
+                assertThat(holder, nullValue());
+                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
+    {
+        //Test that an embedded mapping overrides one from webdefault-ee10.xml
+        Path docroot = workDir.getEmptyPathDir();
+        WebAppContext wac = new WebAppContext();
+        wac.setServer(_server);
+        wac.setBaseResourceAsPath(docroot);
+        wac.setThrowUnavailableOnStartupException(true);
+        ServletHolder defaultServlet = new ServletHolder(DefaultServlet.class);
+        defaultServlet.setName("noname");
+        wac.getServletHandler().addServletWithMapping(defaultServlet, "/");
+        wac.start();
+
+        ServletMapping[] mappings = wac.getServletHandler().getServletMappings();
+        ServletMapping mapping = null;
+        for (ServletMapping m : mappings)
+        {
+            assertThat(m.getServletName(), not(equals("default")));
+            if (m.containsPathSpec("/"))
+            {
+                assertThat(mapping, nullValue());
+                mapping = m;
+            }
+        }
+        assertNotNull(mapping);
+        assertEquals("noname", mapping.getServletName());
+    }
+
+    @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();
+        ServletMapping[] mappings = wac.getServletHandler().getServletMappings();
+        ServletMapping mapping = null;
+        for (ServletMapping m : mappings)
+        {
+            assertThat(m.getServletName(), not(equals("default")));
+            if (m.containsPathSpec("/"))
+            {
+                assertThat(mapping, nullValue());
+                mapping = m;
+            }
+        }
+        assertNotNull(mapping);
+        assertEquals("other", mapping.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
     {
@@ -118,7 +247,6 @@ public void testVisitSessionConfig(WorkDir workDir) throws Exception
 
         //test the attributes on SessionHandler do NOT contain the name
         Map<String, String> sessionAttributes = wac.getSessionHandler().getSessionCookieAttributes();
-        sessionAttributes.keySet().forEach(System.err::println);
         assertThat(sessionAttributes.keySet(),
             containsInAnyOrder(Arrays.asList(
                 equalToIgnoringCase("comment"),