Skip to content

Commit

Permalink
Issue #4064 - MinimalServlets test and ServletHolder fix
Browse files Browse the repository at this point in the history 
+ Also made ContextHandler warning message about features
  that are unimplemented (and you should use ServletContextHandler)
  more clear. (this helped with diagnosing where the bug was
  in ServletHolder)

Signed-off-by: Joakim Erdfelt <[email protected]>
  • Loading branch information
@joakime
joakime committed Sep 6, 2019
1 parent 829ccca commit 7618eae
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 36 deletions.
25 changes: 15 additions & 10 deletions examples/embedded/src/main/java/org/eclipse/jetty/embedded/MinimalServlets.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.eclipse.jetty.embedded;

import java.io.IOException;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand All @@ -29,13 +28,13 @@

public class MinimalServlets
{
public static void main(String[] args) throws Exception

public static Server createServer(int port)
{
// Create a basic jetty server object that will listen on port 8080.
// Note that if you set this to port 0 then a randomly available port
// will be assigned that you can either look in the logs for the port,
// or programmatically obtain it for use in test cases.
Server server = new Server(8080);
Server server = new Server(port);

// The ServletHandler is a dead simple way to create a context handler
// that is backed by an instance of a Servlet.
Expand All @@ -51,13 +50,19 @@ public static void main(String[] args) throws Exception
// through a web.xml @WebServlet annotation, or anything similar.
handler.addServletWithMapping(HelloServlet.class, "/*");

return server;
}

public static void main(String[] args) throws Exception
{
// Create a basic jetty server object that will listen on port 8080.
Server server = createServer(8080);

// Start things up!
server.start();

// The use of server.join() the will make the current thread join and
// wait until the server is done executing.
// See
// http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#join()
// wait until the server thread is done executing.
server.join();
}

Expand All @@ -66,11 +71,11 @@ public static class HelloServlet extends HttpServlet
{
@Override
protected void doGet(HttpServletRequest request,
HttpServletResponse response) throws ServletException,
IOException
HttpServletResponse response) throws IOException
{
response.setContentType("text/html");
response.setStatus(HttpServletResponse.SC_OK);
response.setContentType("text/html");
response.setCharacterEncoding("utf-8");
response.getWriter().println("<h1>Hello from HelloServlet</h1>");
}
}
Expand Down
64 changes: 64 additions & 0 deletions examples/embedded/src/test/java/org/eclipse/jetty/embedded/MinimalServletsTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
//
// ========================================================================
// Copyright (c) 1995-2019 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.embedded;

import java.io.IOException;
import java.net.HttpURLConnection;
import java.net.URI;

import org.eclipse.jetty.server.Server;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;

public class MinimalServletsTest
{
private Server server;

@BeforeEach
public void startServer() throws Exception
{
server = MinimalServlets.createServer(0);
server.start();
}

@AfterEach
public void stopServer() throws Exception
{
server.stop();
}

@Test
public void testGetHello() throws IOException
{
URI uri = server.getURI().resolve("/hello");
HttpURLConnection http = (HttpURLConnection)uri.toURL().openConnection();
assertThat("HTTP Response Status", http.getResponseCode(), is(HttpURLConnection.HTTP_OK));

// HttpUtil.dumpResponseHeaders(http);

// test response content
String responseBody = HttpUtil.getResponseBody(http);
assertThat("Response Content", responseBody, containsString("Hello"));
}
}
46 changes: 23 additions & 23 deletions jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
public static final int DEFAULT_LISTENER_TYPE_INDEX = 1;
public static final int EXTENDED_LISTENER_TYPE_INDEX = 0;

private static final String __unimplmented = "Unimplemented - use org.eclipse.jetty.servlet.ServletContextHandler";
private static final String UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER = "Unimplemented {} - use org.eclipse.jetty.servlet.ServletContextHandler";

private static final Logger LOG = Log.getLogger(ContextHandler.class);

Expand Down Expand Up @@ -2479,7 +2479,7 @@ public ClassLoader getClassLoader()
@Override
public JspConfigDescriptor getJspConfigDescriptor()
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getJspConfigDescriptor()");
return null;
}

Expand Down Expand Up @@ -2673,130 +2673,130 @@ public boolean setInitParameter(String name, String value)
@Override
public Dynamic addFilter(String filterName, Class<? extends Filter> filterClass)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addFilter(String, Class)");
return null;
}

@Override
public Dynamic addFilter(String filterName, Filter filter)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addFilter(String, Filter)");
return null;
}

@Override
public Dynamic addFilter(String filterName, String className)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addFilter(String, String)");
return null;
}

@Override
public javax.servlet.ServletRegistration.Dynamic addServlet(String servletName, Class<? extends Servlet> servletClass)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addServlet(String, Class)");
return null;
}

@Override
public javax.servlet.ServletRegistration.Dynamic addServlet(String servletName, Servlet servlet)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addServlet(String, Servlet)");
return null;
}

@Override
public javax.servlet.ServletRegistration.Dynamic addServlet(String servletName, String className)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addServlet(String, String)");
return null;
}

@Override
public <T extends Filter> T createFilter(Class<T> c) throws ServletException
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "createFilter(Class)");
return null;
}

@Override
public <T extends Servlet> T createServlet(Class<T> c) throws ServletException
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "createServlet(Class)");
return null;
}

@Override
public Set<SessionTrackingMode> getDefaultSessionTrackingModes()
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getDefaultSessionTrackingModes()");
return null;
}

@Override
public Set<SessionTrackingMode> getEffectiveSessionTrackingModes()
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getEffectiveSessionTrackingModes()");
return null;
}

@Override
public FilterRegistration getFilterRegistration(String filterName)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getFilterRegistration(String)");
return null;
}

@Override
public Map<String, ? extends FilterRegistration> getFilterRegistrations()
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getFilterRegistrations()");
return null;
}

@Override
public ServletRegistration getServletRegistration(String servletName)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getServletRegistration(String)");
return null;
}

@Override
public Map<String, ? extends ServletRegistration> getServletRegistrations()
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getServletRegistrations()");
return null;
}

@Override
public SessionCookieConfig getSessionCookieConfig()
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getSessionCookieConfig()");
return null;
}

@Override
public void setSessionTrackingModes(Set<SessionTrackingMode> sessionTrackingModes)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "setSessionTrackingModes(Set<SessionTrackingMode>)");
}

@Override
public void addListener(String className)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addListener(String)");
}

@Override
public <T extends EventListener> void addListener(T t)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addListener(T)");
}

@Override
public void addListener(Class<? extends EventListener> listenerClass)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addListener(Class)");
}

@Override
Expand Down Expand Up @@ -2843,14 +2843,14 @@ public void setEffectiveMinorVersion(int v)
@Override
public JspConfigDescriptor getJspConfigDescriptor()
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "getJspConfigDescriptor()");
return null;
}

@Override
public void declareRoles(String... roleNames)
{
LOG.warn(__unimplmented);
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "declareRoles(String...)");
}

@Override
Expand Down
7 changes: 4 additions & 3 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1265,9 +1265,10 @@ protected Servlet newInstance() throws ServletException, IllegalAccessException,
try
{
ServletContext ctx = getServletHandler().getServletContext();
if (ctx == null)
return getHeldClass().getDeclaredConstructor().newInstance();
return ctx.createServlet(getHeldClass());
if (ctx instanceof ServletContextHandler.Context)

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Sep 7, 2019

Contributor

the createServlet method is on ServletContext, so we don't need to check for ServletContextHandler.Context

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Sep 10, 2019

Author Contributor

When the embedded example (MinimalServlets) runs, the ctx is a ContextHandler$StaticContext which doesn't have the createServlet method implemented.
This is the reason that Issue #4064 occurs.

We can't just test for null here, we need to know if it's an implementation of ServletContext that actually has a createServlet method implemented. Which in our case is at least ServletContextHandler.Context or better (like WebAppContext.Context)

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Sep 10, 2019

Contributor

@joakime I think the fix should be to improve StaticContext so that it can actually create and register Servlets and Filters. It should probably be moved to ServletHandler as well.

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Sep 10, 2019

Contributor

Hmmm maybe revert to instanceof check in 9.4 and we can clean this up better in 10,

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Sep 10, 2019

Author Contributor

The code we have in this commit is just a selective revert of the commit that broke this feature in 9.4.20 - 819c379#diff-0d419a5cace2d3e164cd481d96f24734
return ctx.createServlet(getHeldClass());
return getHeldClass().getDeclaredConstructor().newInstance();

}
catch (ServletException ex)
{
Expand Down

0 comments on commit 7618eae

Please sign in to comment.