Remove obsolete NonblockingServletHolder (#3527)

Refs jetty/jetty.project#5496
Refs #3526

(cherry picked from commit eef4527)

dropwizard-core/src/main/java/io/dropwizard/server/AbstractServerFactory.java

@@ -18,7 +18,6 @@
 import io.dropwizard.jersey.validation.HibernateValidationFeature;
 import io.dropwizard.jetty.GzipHandlerFactory;
 import io.dropwizard.jetty.MutableServletContextHandler;
-import io.dropwizard.jetty.NonblockingServletHolder;
 import io.dropwizard.jetty.ServerPushFilterFactory;
 import io.dropwizard.lifecycle.setup.LifecycleEnvironment;
 import io.dropwizard.request.logging.LogbackAccessRequestLogFactory;
@@ -33,6 +32,7 @@
 import org.eclipse.jetty.server.handler.ErrorHandler;
 import org.eclipse.jetty.server.handler.RequestLogHandler;
 import org.eclipse.jetty.server.handler.StatisticsHandler;
+import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jetty.setuid.RLimit;
 import org.eclipse.jetty.setuid.SetUIDListener;
 import org.eclipse.jetty.util.BlockingArrayQueue;
@@ -506,7 +506,7 @@ protected Handler createAdminServlet(Server server,
         handler.setServer(server);
         handler.getServletContext().setAttribute(MetricsServlet.METRICS_REGISTRY, metrics);
         handler.getServletContext().setAttribute(HealthCheckServlet.HEALTH_CHECK_REGISTRY, healthChecks);
-        handler.addServlet(new NonblockingServletHolder(new AdminServlet()), "/*");
+        handler.addServlet(AdminServlet.class, "/*");
         handler.addFilter(AllowedMethodsFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST))
                 .setInitParameter(AllowedMethodsFilter.ALLOWED_METHODS_PARAM, Joiner.on(',').join(allowedMethods));
         return handler;
@@ -543,7 +543,7 @@ protected Handler createAppServlet(Server server,
             if (registerDefaultExceptionMappers == null || registerDefaultExceptionMappers) {
                 jersey.register(new ExceptionMapperBinder(detailedJsonProcessingExceptionMapper));
             }
-            handler.addServlet(new NonblockingServletHolder(jerseyContainer), jersey.getUrlPattern());
+            handler.addServlet(new ServletHolder("jersey", jerseyContainer), jersey.getUrlPattern());
         }
         final InstrumentedHandler instrumented = new InstrumentedHandler(metricRegistry);
         instrumented.setServer(server);

dropwizard-jetty/src/main/java/io/dropwizard/jetty/NonblockingServletHolder.java

@@ -12,7 +12,12 @@
 /**
  * A {@link ServletHolder} subclass which removes the synchronization around servlet initialization
  * by requiring a pre-initialized servlet holder.
+ *
+ * @deprecated If necessary, use {@link ServletHolder} or {@link org.eclipse.jetty.servlet.FilterHolder} directly.
+ * This class will be removed in Dropwizard 2.1.0.
  */
+@SuppressWarnings("unused")
+@Deprecated
 public class NonblockingServletHolder extends ServletHolder {
     private final Servlet servlet;
 

dropwizard-jetty/src/main/java/io/dropwizard/jetty/setup/ServletEnvironment.java

@@ -1,10 +1,10 @@
 package io.dropwizard.jetty.setup;
 
 import io.dropwizard.jetty.MutableServletContextHandler;
-import io.dropwizard.jetty.NonblockingServletHolder;
 import org.eclipse.jetty.security.SecurityHandler;
 import org.eclipse.jetty.server.session.SessionHandler;
 import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlet.ServletHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jetty.util.resource.Resource;
 import org.eclipse.jetty.util.resource.ResourceCollection;
@@ -43,9 +43,9 @@ public ServletEnvironment(MutableServletContextHandler handler) {
      *         configuration
      */
     public ServletRegistration.Dynamic addServlet(String name, Servlet servlet) {
-        final ServletHolder holder = new NonblockingServletHolder(requireNonNull(servlet));
-        holder.setName(name);
-        handler.getServletHandler().addServlet(holder);
+        final ServletHolder holder = new ServletHolder(name, servlet);
+        final ServletHandler servletHandler = handler.getServletHandler();
+        servletHandler.addServlet(holder);
 
         final ServletRegistration.Dynamic registration = holder.getRegistration();
         checkDuplicateRegistration(name, servlets, "servlet");
@@ -61,9 +61,9 @@ public ServletRegistration.Dynamic addServlet(String name, Servlet servlet) {
      * @return a {@link javax.servlet.ServletRegistration.Dynamic} instance allowing for further configuration
      */
     public ServletRegistration.Dynamic addServlet(String name, Class<? extends Servlet> klass) {
-        final ServletHolder holder = new ServletHolder(requireNonNull(klass));
-        holder.setName(name);
-        handler.getServletHandler().addServlet(holder);
+        final ServletHolder holder = new ServletHolder(name, klass);
+        final ServletHandler servletHandler = handler.getServletHandler();
+        servletHandler.addServlet(holder);
 
         final ServletRegistration.Dynamic registration = holder.getRegistration();
         checkDuplicateRegistration(name, servlets, "servlet");

dropwizard-jetty/src/test/java/io/dropwizard/jetty/NonblockingServletHolderTest.java

@@ -15,6 +15,7 @@
 
 public class NonblockingServletHolderTest {
     private final Servlet servlet = mock(Servlet.class);
+    @SuppressWarnings("deprecation")
     private final NonblockingServletHolder holder = new NonblockingServletHolder(servlet);
     private final Request baseRequest = mock(Request.class);
     private final ServletRequest request = mock(ServletRequest.class);

dropwizard-jetty/src/test/java/io/dropwizard/jetty/setup/ServletEnvironmentTest.java

@@ -1,136 +1,132 @@
 package io.dropwizard.jetty.setup;
 
 import io.dropwizard.jetty.MutableServletContextHandler;
-import org.eclipse.jetty.http.MimeTypes;
+import org.eclipse.jetty.security.ConstraintSecurityHandler;
 import org.eclipse.jetty.security.SecurityHandler;
+import org.eclipse.jetty.server.DebugListener;
 import org.eclipse.jetty.server.session.SessionHandler;
 import org.eclipse.jetty.servlet.FilterHolder;
 import org.eclipse.jetty.servlet.ServletHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
+import org.eclipse.jetty.servlets.ConcatServlet;
 import org.eclipse.jetty.servlets.WelcomeFilter;
 import org.eclipse.jetty.util.resource.Resource;
 import org.eclipse.jetty.util.resource.ResourceCollection;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
-import org.mockito.ArgumentCaptor;
 
 import javax.servlet.Filter;
 import javax.servlet.FilterRegistration;
-import javax.servlet.GenericServlet;
 import javax.servlet.Servlet;
 import javax.servlet.ServletContextListener;
 import javax.servlet.ServletRegistration;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
 
 public class ServletEnvironmentTest {
-    private final ServletHandler servletHandler = mock(ServletHandler.class);
-    private final MutableServletContextHandler handler = mock(MutableServletContextHandler.class);
-    private final ServletEnvironment environment = new ServletEnvironment(handler);
+
+    private MutableServletContextHandler handler;
+    private ServletHandler servletHandler;
+    private ServletEnvironment environment;
 
     @Rule
     public TemporaryFolder tempDir = new TemporaryFolder();
 
     @Before
-    public void setUp() throws Exception {
-        when(handler.getServletHandler()).thenReturn(servletHandler);
+    public void setUp() {
+        handler = new MutableServletContextHandler();
+        servletHandler = handler.getServletHandler();
+        environment = new ServletEnvironment(handler);
     }
 
     @Test
     public void addsServletInstances() throws Exception {
-        final Servlet servlet = mock(Servlet.class);
-
+        final Servlet servlet = new ConcatServlet();
         final ServletRegistration.Dynamic builder = environment.addServlet("servlet", servlet);
-        assertThat(builder)
-                .isNotNull();
-
-        final ArgumentCaptor<ServletHolder> holder = ArgumentCaptor.forClass(ServletHolder.class);
-        verify(servletHandler).addServlet(holder.capture());
+        assertThat(builder).isNotNull();
 
-        assertThat(holder.getValue().getName())
-                .isEqualTo("servlet");
+        try {
+            servletHandler.start();
 
-        assertThat(holder.getValue().getServlet())
-                .isEqualTo(servlet);
+            final ServletHolder servletHolder = servletHandler.getServlet("servlet");
+            assertThat(servletHolder.getServlet()).isEqualTo(servlet);
+        } finally {
+            servletHandler.stop();
+        }
     }
 
     @Test
     public void addsServletClasses() throws Exception {
-        final ServletRegistration.Dynamic builder = environment.addServlet("servlet", GenericServlet.class);
-        assertThat(builder)
-                .isNotNull();
-
-        final ArgumentCaptor<ServletHolder> holder = ArgumentCaptor.forClass(ServletHolder.class);
-        verify(servletHandler).addServlet(holder.capture());
-
-        assertThat(holder.getValue().getName())
-                .isEqualTo("servlet");
-
-        // this is ugly, but comparing classes sucks with these type bounds
-        assertThat(holder.getValue().getHeldClass().equals(GenericServlet.class))
-                .isTrue();
+        final Class<ConcatServlet> servletClass = ConcatServlet.class;
+        final ServletRegistration.Dynamic builder = environment.addServlet("servlet", servletClass);
+        assertThat(builder).isNotNull();
+
+        try {
+            servletHandler.start();
+
+            final ServletHolder servletHolder = servletHandler.getServlet("servlet");
+            assertThat(servletHolder.getServlet()).isExactlyInstanceOf(servletClass);
+        } finally {
+            servletHandler.stop();
+        }
     }
 
     @Test
     public void addsFilterInstances() throws Exception {
         final Filter filter = new WelcomeFilter();
 
         final FilterRegistration.Dynamic builder = environment.addFilter("filter", filter);
-        assertThat(builder)
-                .isNotNull();
-
-        final ArgumentCaptor<FilterHolder> holder = ArgumentCaptor.forClass(FilterHolder.class);
-        verify(servletHandler).addFilter(holder.capture());
+        assertThat(builder).isNotNull();
 
-        assertThat(holder.getValue().getName())
-                .isEqualTo("filter");
+        try {
+            servletHandler.start();
 
-        assertThat(holder.getValue()).hasFieldOrPropertyWithValue("_instance", filter);
+            final FilterHolder filterHolder = servletHandler.getFilter("filter");
+            assertThat(filterHolder.getFilter()).isEqualTo(filter);
+        } finally {
+            servletHandler.stop();
+        }
     }
 
     @Test
     public void addsFilterClasses() throws Exception {
-        final FilterRegistration.Dynamic builder = environment.addFilter("filter", WelcomeFilter.class);
-        assertThat(builder)
-                .isNotNull();
-
-        final ArgumentCaptor<FilterHolder> holder = ArgumentCaptor.forClass(FilterHolder.class);
-        verify(servletHandler).addFilter(holder.capture());
-
-        assertThat(holder.getValue().getName())
-                .isEqualTo("filter");
-
-        // this is ugly, but comparing classes sucks with these type bounds
-        assertThat(holder.getValue().getHeldClass().equals(WelcomeFilter.class))
-                .isTrue();
+        final Class<WelcomeFilter> filterClass = WelcomeFilter.class;
+        final FilterRegistration.Dynamic builder = environment.addFilter("filter", filterClass);
+        assertThat(builder).isNotNull();
+
+        try {
+            servletHandler.start();
+
+            final FilterHolder filterHolder = servletHandler.getFilter("filter");
+            assertThat(filterHolder.getFilter()).isExactlyInstanceOf(filterClass);
+        } finally {
+            servletHandler.stop();
+        }
     }
 
     @Test
-    public void addsServletListeners() throws Exception {
-        final ServletContextListener listener = mock(ServletContextListener.class);
+    public void addsServletListeners() {
+        final ServletContextListener listener = new DebugListener();
         environment.addServletListeners(listener);
 
-        verify(handler).addEventListener(listener);
+        assertThat(handler.getEventListeners()).contains(listener);
     }
 
     @Test
-    public void addsProtectedTargets() throws Exception {
+    public void addsProtectedTargets() {
         environment.setProtectedTargets("/woo");
 
-        verify(handler).setProtectedTargets(new String[]{"/woo"});
+        assertThat(handler.getProtectedTargets()).contains("/woo");
     }
 
     @Test
     public void setsBaseResource() throws Exception {
         final Resource testResource = Resource.newResource(tempDir.newFolder());
         environment.setBaseResource(testResource);
 
-        verify(handler).setBaseResource(testResource);
+        assertThat(handler.getBaseResource()).isEqualTo(testResource);
     }
 
     @Test
@@ -141,22 +137,15 @@ public void setsBaseResourceList() throws Exception {
         final Resource[] testResources = new Resource[]{wooResource, fooResource};
         environment.setBaseResource(testResources);
 
-        ArgumentCaptor<Resource> captor = ArgumentCaptor.forClass(Resource.class);
-        verify(handler).setBaseResource(captor.capture());
-
-        Resource actualResource = captor.getValue();
-        assertThat(actualResource).isInstanceOf(ResourceCollection.class);
-
-        ResourceCollection actualResourceCollection = (ResourceCollection) actualResource;
-        assertThat(actualResourceCollection.getResources()).contains(wooResource, fooResource);
-
+        assertThat(handler.getBaseResource()).isExactlyInstanceOf(ResourceCollection.class);
+        assertThat(((ResourceCollection) handler.getBaseResource()).getResources()).contains(wooResource, fooResource);
     }
 
     @Test
     public void setsResourceBase() throws Exception {
         environment.setResourceBase("/woo");
 
-        verify(handler).setResourceBase("/woo");
+        assertThat(handler.getResourceBase()).isEqualTo(handler.newResource("/woo").toString());
     }
 
     @Test
@@ -167,53 +156,41 @@ public void setsBaseResourceStringList() throws Exception {
         final String[] testResources = new String[]{wooResource, fooResource};
         environment.setBaseResource(testResources);
 
-        ArgumentCaptor<Resource> captor = ArgumentCaptor.forClass(Resource.class);
-        verify(handler).setBaseResource(captor.capture());
-
-        Resource actualResource = captor.getValue();
-        assertThat(actualResource).isInstanceOf(ResourceCollection.class);
-
-        ResourceCollection actualResourceCollection = (ResourceCollection) actualResource;
-        assertThat(actualResourceCollection.getResources()).contains(Resource.newResource(wooResource),
-            Resource.newResource(fooResource));
-
+        assertThat(handler.getBaseResource()).isExactlyInstanceOf(ResourceCollection.class);
+        assertThat(((ResourceCollection) handler.getBaseResource()).getResources())
+            .contains(Resource.newResource(wooResource), Resource.newResource(fooResource));
     }
 
     @Test
-    public void setsInitParams() throws Exception {
+    public void setsInitParams() {
         environment.setInitParameter("a", "b");
 
-        verify(handler).setInitParameter("a", "b");
+        assertThat(handler.getInitParameter("a")).isEqualTo("b");
     }
 
     @Test
-    public void setsSessionHandlers() throws Exception {
-        final SessionHandler sessionHandler = mock(SessionHandler.class);
-
+    public void setsSessionHandlers() {
+        final SessionHandler sessionHandler = new SessionHandler();
         environment.setSessionHandler(sessionHandler);
 
-        verify(handler).setSessionHandler(sessionHandler);
-        verify(handler).setSessionsEnabled(true);
+        assertThat(handler.getSessionHandler()).isEqualTo(sessionHandler);
+        assertThat(handler.isSessionsEnabled()).isTrue();
     }
 
 
     @Test
-    public void setsSecurityHandlers() throws Exception {
-        final SecurityHandler securityHandler = mock(SecurityHandler.class);
-
+    public void setsSecurityHandlers() {
+        final SecurityHandler securityHandler = new ConstraintSecurityHandler();
         environment.setSecurityHandler(securityHandler);
 
-        verify(handler).setSecurityHandler(securityHandler);
-        verify(handler).setSecurityEnabled(true);
+        assertThat(handler.getSecurityHandler()).isEqualTo(securityHandler);
+        assertThat(handler.isSecurityEnabled()).isTrue();
     }
 
     @Test
     public void addsMimeMapping() {
-        final MimeTypes mimeTypes = mock(MimeTypes.class);
-        when(handler.getMimeTypes()).thenReturn(mimeTypes);
-
-        environment.addMimeMapping("example/foo", "foo");
+        environment.addMimeMapping("foo", "example/foo");
 
-        verify(mimeTypes).addMimeMapping("example/foo", "foo");
+        assertThat(handler.getMimeTypes().getMimeMap()).containsEntry("foo", "example/foo");
     }
 }