483620 Servlet annotation mapping to "/" should override webdefault.xml mapping

jetty-annotations/src/main/java/org/eclipse/jetty/annotations/WebServletAnnotation.java

@@ -19,6 +19,9 @@
 package org.eclipse.jetty.annotations;
 
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 
 import javax.servlet.Servlet;
 import javax.servlet.annotation.WebInitParam;
@@ -28,6 +31,7 @@ import javax.servlet.http.HttpServlet;
 import org.eclipse.jetty.servlet.Holder;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jetty.servlet.ServletMapping;
+import org.eclipse.jetty.util.ArrayUtil;
 import org.eclipse.jetty.util.LazyList;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
@@ -104,10 +108,11 @@ public class WebServletAnnotation extends DiscoveredAnnotation
         String servletName = (annotation.name().equals("")?clazz.getName():annotation.name());
 
         MetaData metaData = _context.getMetaData();
+        ServletMapping mapping = null; //the new mapping
 
         //Find out if a <servlet> already exists with this name
         ServletHolder[] holders = _context.getServletHandler().getServlets();
-        boolean isNew = true;
+
         ServletHolder holder = null;
         if (holders != null)
         {
@@ -116,13 +121,13 @@ public class WebServletAnnotation extends DiscoveredAnnotation
                 if (h.getName() != null && servletName.equals(h.getName()))
                 {
                     holder = h;
-                    isNew = false;
                     break;
                 }
             }
         }
 
-        if (isNew)
+        //handle creation/completion of a servlet 
+        if (holder == null)
         {
             //No servlet of this name has already been defined, either by a descriptor
             //or another annotation (which would be impossible).
@@ -147,11 +152,11 @@ public class WebServletAnnotation extends DiscoveredAnnotation
             }
 
             _context.getServletHandler().addServlet(holder);
-            ServletMapping mapping = new ServletMapping();
+            
+            
+            mapping = new ServletMapping();
             mapping.setServletName(holder.getName());
             mapping.setPathSpecs( LazyList.toStringArray(urlPatternList));
-            _context.getServletHandler().addServletMapping(mapping);
-            metaData.setOrigin(servletName+".servlet.mappings",annotation,clazz);
         }
         else
         {
@@ -174,55 +179,103 @@ public class WebServletAnnotation extends DiscoveredAnnotation
                     metaData.setOrigin(servletName+".servlet.init-param."+ip.name(),ip,clazz);
                 }
             }
+            
 
             //check the url-patterns
             //ServletSpec 3.0 p81 If a servlet already has url mappings from a
-            //webxml or fragment descriptor the annotation is ignored. However, we want to be able to
-            //replace mappings that were given in webdefault.xml
-            boolean mappingsExist = false;
-            boolean anyNonDefaults = false;
-            ServletMapping[] allMappings = _context.getServletHandler().getServletMappings();
-            if (allMappings != null)
+            //webxml or fragment descriptor the annotation is ignored. 
+            //However, we want to be able to replace mappings that were given in webdefault.xml         
+            List<ServletMapping> existingMappings = getServletMappingsForServlet(servletName);
+            
+            //if any mappings for this servlet already set by a descriptor that is not webdefault.xml forget 
+            //about processing these url mappings        
+            if (existingMappings.isEmpty() || !containsNonDefaultMappings(existingMappings))
             {
-                for (ServletMapping m:allMappings)
-                {
-                    if (m.getServletName() != null && servletName.equals(m.getServletName()))
-                    {
-                        mappingsExist = true;
-                        if (!m.isDefault())
-                        {
-                            anyNonDefaults = true;
-                            break;
-                        }
-                    }
-                }
-            }
-
-            if (anyNonDefaults)
-                return;  //if any mappings already set by a descriptor that is not webdefault.xml, we're done
-
-            boolean clash = false;
-            if (mappingsExist)
-            {
-                for (String p:urlPatternList)
-                {
-                    ServletMapping m = _context.getServletHandler().getServletMapping(p);
-                    if (m != null && !m.isDefault())
-                    {
-                        //trying to override a servlet-mapping that was added not by webdefault.xml
-                        clash = true;
-                        break;
-                    }
-                }
-            }
-
-            if (!mappingsExist || !clash)
-            {
-                ServletMapping m = new ServletMapping();
-                m.setServletName(servletName);
-                m.setPathSpecs(LazyList.toStringArray(urlPatternList));
-                _context.getServletHandler().addServletMapping(m);
+                mapping = new ServletMapping();
+                mapping.setServletName(servletName);
+                mapping.setPathSpecs(LazyList.toStringArray(urlPatternList));
             }
         }
+
+        
+        //We also want to be able to replace mappings that were defined in webdefault.xml
+        //that were for a different servlet eg a mapping in webdefault.xml for / to the jetty
+        //default servlet should be able to be replaced by an annotation for / to a different
+        //servlet
+        if (mapping != null)
+        {
+            //url mapping was permitted by annotation processing rules
+
+            //take a copy of the existing servlet mappings that we can iterate over and remove from. This is
+            //because the ServletHandler interface does not support removal of individual mappings.
+            List<ServletMapping> allMappings = ArrayUtil.asMutableList(_context.getServletHandler().getServletMappings());
+            
+            //for each of the urls in the annotation, check if a mapping to same/different servlet exists
+            //  if mapping exists and is from a default descriptor, it can be replaced. NOTE: we do not
+            //  guard against duplicate path mapping here: that is the job of the ServletHandler
+            for (String p:urlPatternList)
+            {
+                ServletMapping existingMapping = _context.getServletHandler().getServletMapping(p);
+                if (existingMapping != null && existingMapping.isDefault())
+                {
+                    String[] updatedPaths = ArrayUtil.removeFromArray(existingMapping.getPathSpecs(), p);
+                    //if we removed the last path from a servletmapping, delete the servletmapping
+                    if (updatedPaths == null || updatedPaths.length == 0)
+                    { 
+                        boolean success = allMappings.remove(existingMapping);
+                        if (LOG.isDebugEnabled()) LOG.debug("Removed empty mapping {} from defaults descriptor success:{}",existingMapping, success);
+                    }
+                    else 
+                    {
+                        existingMapping.setPathSpecs(updatedPaths);
+                        if (LOG.isDebugEnabled()) LOG.debug("Removed path {} from mapping {} from defaults descriptor ", p,existingMapping);
+                    }
+                }
+                _context.getMetaData().setOrigin(servletName+".servlet.mapping."+p, annotation, clazz);
+            }
+            allMappings.add(mapping);
+            _context.getServletHandler().setServletMappings(allMappings.toArray(new ServletMapping[allMappings.size()]));
+        }
+    }
+
+
+    
+    
+    /**
+     * @param name
+     * @return
+     */
+    private List<ServletMapping>  getServletMappingsForServlet (String name)
+    {
+        ServletMapping[] allMappings = _context.getServletHandler().getServletMappings();
+        if (allMappings == null)
+            return Collections.emptyList();
+
+        List<ServletMapping> mappings = new ArrayList<ServletMapping>();
+        for (ServletMapping m:allMappings)
+        {
+            if (m.getServletName() != null && name.equals(m.getServletName()))
+            {
+                mappings.add(m);
+            }
+        }
+        return mappings;
+    }
+    
+    
+    /**
+     * @param mappings
+     * @return
+     */
+    private boolean containsNonDefaultMappings (List<ServletMapping> mappings)
+    {
+        if (mappings == null)
+            return false;
+        for (ServletMapping m:mappings)
+        {
+            if (!m.isDefault())
+                return true;
+        }
+        return false;
     }
 }

jetty-annotations/src/test/java/org/eclipse/jetty/annotations/ServletD.java

@@ -0,0 +1,31 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2015 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.annotations;
+
+import javax.servlet.annotation.WebInitParam;
+import javax.servlet.annotation.WebServlet;
+import javax.servlet.http.HttpServlet;
+
+
+
+@WebServlet(urlPatterns = { "/", "/bah/*" }, name="DServlet", initParams={@WebInitParam(name="x", value="y")}, loadOnStartup=1, asyncSupported=false)
+public class ServletD extends HttpServlet
+{
+
+}

jetty-annotations/src/test/java/org/eclipse/jetty/annotations/TestServletAnnotations.java

@@ -107,7 +107,196 @@ public class TestServletAnnotations
         assertNotNull(paths);
         assertEquals(2, paths.length);
     }
+    
+    
+    @Test
+    public void testWebServletAnnotationOverrideDefault () throws Exception
+    {
+        //if the existing servlet mapping TO A DIFFERENT SERVLET IS from a default descriptor we
+        //DO allow the annotation to replace the mapping.
+        
+        WebAppContext wac = new WebAppContext();
+        ServletHolder defaultServlet = new ServletHolder();
+        defaultServlet.setClassName("org.eclipse.jetty.servlet.DefaultServlet");
+        defaultServlet.setName("default");      
+        wac.getServletHandler().addServlet(defaultServlet);
+        
+        ServletMapping m = new ServletMapping();
+        m.setPathSpec("/");
+        m.setServletName("default");
+        m.setDefault(true);  //this mapping will be from a default descriptor
+        wac.getServletHandler().addServletMapping(m);
+        
+        WebServletAnnotation annotation = new WebServletAnnotation(wac, "org.eclipse.jetty.annotations.ServletD", null);
+        annotation.apply();
+        
+        //test that as the original servlet mapping had only 1 pathspec, then the whole
+        //servlet mapping should be deleted as that pathspec will be remapped to the DServlet
+        ServletMapping[] resultMappings = wac.getServletHandler().getServletMappings();
+        assertNotNull(resultMappings);    
+        assertEquals(1, resultMappings.length);
+        assertEquals(2, resultMappings[0].getPathSpecs().length);
+        resultMappings[0].getServletName().equals("DServlet");
+        for (String s:resultMappings[0].getPathSpecs())
+        {
+            assertTrue (s.equals("/") || s.equals("/bah/*"));
+        }
+    }
+    
+    
+    
+    @Test
+    public void testWebServletAnnotationReplaceDefault () throws Exception
+    {
+        //if the existing servlet mapping TO A DIFFERENT SERVLET IS from a default descriptor we
+        //DO allow the annotation to replace the mapping.    
+        WebAppContext wac = new WebAppContext();
+        ServletHolder defaultServlet = new ServletHolder();
+        defaultServlet.setClassName("org.eclipse.jetty.servlet.DefaultServlet");
+        defaultServlet.setName("default");      
+        wac.getServletHandler().addServlet(defaultServlet);
+        
+        ServletMapping m = new ServletMapping();
+        m.setPathSpec("/");
+        m.setServletName("default");
+        m.setDefault(true);  //this mapping will be from a default descriptor
+        wac.getServletHandler().addServletMapping(m);
+        
+        ServletMapping m2 = new ServletMapping();
+        m2.setPathSpec("/other");
+        m2.setServletName("default");
+        m2.setDefault(true);  //this mapping will be from a default descriptor
+        wac.getServletHandler().addServletMapping(m2);
+        
+        WebServletAnnotation annotation = new WebServletAnnotation(wac, "org.eclipse.jetty.annotations.ServletD", null);
+        annotation.apply();
+        
+        //test that only the mapping for "/" was removed from the mappings to the default servlet
+        ServletMapping[] resultMappings = wac.getServletHandler().getServletMappings();
+        assertNotNull(resultMappings);    
+        assertEquals(2, resultMappings.length);
+        for (ServletMapping r:resultMappings)
+        {
+           if (r.getServletName().equals("default"))
+           {
+               assertEquals(1,r.getPathSpecs().length);
+               assertEquals("/other", r.getPathSpecs()[0]);
+           }
+           else if (r.getServletName().equals("DServlet"))
+           {
+               assertEquals(2,r.getPathSpecs().length);
+               for (String p:r.getPathSpecs())
+               {
+                   if (!p.equals("/") && !p.equals("/bah/*"))
+                       fail("Unexpected path");
+               }
+           }
+           else
+               fail("Unexpected servlet mapping");
+        }
+        
+    }
+    
+    
+    @Test
+    public void testWebServletAnnotationNotOverride () throws Exception
+    {
+        //if the existing servlet mapping TO A DIFFERENT SERVLET IS NOT from a default descriptor we
+        //DO NOT allow the annotation to replace the mapping
+        WebAppContext wac = new WebAppContext();
+        ServletHolder servlet = new ServletHolder();
+        servlet.setClassName("org.eclipse.jetty.servlet.FooServlet");
+        servlet.setName("foo");
+        wac.getServletHandler().addServlet(servlet);
+        ServletMapping m = new ServletMapping();
+        m.setPathSpec("/");
+        m.setServletName("foo");
+        wac.getServletHandler().addServletMapping(m);
 
+        WebServletAnnotation annotation = new WebServletAnnotation(wac, "org.eclipse.jetty.annotations.ServletD", null);
+        annotation.apply();
+
+        ServletMapping[] resultMappings = wac.getServletHandler().getServletMappings();
+        assertEquals(2, resultMappings.length);
+        for (ServletMapping r:resultMappings)
+        {
+            if (r.getServletName().equals("DServlet"))
+            {
+                assertEquals(2, r.getPathSpecs().length);
+            }
+            else if (r.getServletName().equals("foo"))
+            {
+                assertEquals(1, r.getPathSpecs().length);
+            }
+            else
+                fail("Unexpected servlet name");
+        }
+    }
+    
+    @Test
+    public void testWebServletAnnotationIgnore () throws Exception
+    {
+        //an existing servlet OF THE SAME NAME has even 1 non-default mapping we can't use
+        //any of the url mappings in the annotation
+        WebAppContext wac = new WebAppContext();
+        ServletHolder servlet = new ServletHolder();
+        servlet.setClassName("org.eclipse.jetty.servlet.OtherDServlet");
+        servlet.setName("DServlet");
+        wac.getServletHandler().addServlet(servlet);
+        
+        ServletMapping m = new ServletMapping();
+        m.setPathSpec("/default");
+        m.setDefault(true);
+        m.setServletName("DServlet");
+        wac.getServletHandler().addServletMapping(m);
+        
+        ServletMapping m2 = new ServletMapping();
+        m2.setPathSpec("/other");
+        m2.setServletName("DServlet");
+        wac.getServletHandler().addServletMapping(m2);
+        
+        WebServletAnnotation annotation = new WebServletAnnotation(wac, "org.eclipse.jetty.annotations.ServletD", null);
+        annotation.apply();
+
+        ServletMapping[] resultMappings = wac.getServletHandler().getServletMappings();
+        assertEquals(2, resultMappings.length);
+        
+        for (ServletMapping r:resultMappings)
+        {
+            assertEquals(1, r.getPathSpecs().length);
+            if (!r.getPathSpecs()[0].equals("/default") && !r.getPathSpecs()[0].equals("/other"))
+                fail("Unexpected path in mapping");
+        }
+        
+    }
+    
+    @Test
+    public void testWebServletAnnotationNoMappings () throws Exception
+    {
+        //an existing servlet OF THE SAME NAME has no mappings, therefore all mappings in the annotation
+        //should be accepted
+        WebAppContext wac = new WebAppContext();
+        ServletHolder servlet = new ServletHolder();
+        servlet.setName("foo");
+        wac.getServletHandler().addServlet(servlet);
+
+
+        WebServletAnnotation annotation = new WebServletAnnotation(wac, "org.eclipse.jetty.annotations.ServletD", null);
+        annotation.apply();
+
+        ServletMapping[] resultMappings = wac.getServletHandler().getServletMappings();
+        assertEquals(1, resultMappings.length);
+        assertEquals(2, resultMappings[0].getPathSpecs().length);
+        for (String s:resultMappings[0].getPathSpecs())
+        {
+            if (!s.equals("/") && !s.equals("/bah/*"))
+                fail("Unexpected path mapping");
+        }
+    }
+    
+    
+
+    @Test
     public void testDeclareRoles ()
     throws Exception
     {