Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #4433 - Implement ServletContext.addJspFile #4463

Merged
merged 4 commits into from
Jan 20, 2020
Merged

Issue #4433 - Implement ServletContext.addJspFile #4463

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60833a9
Issue #4433 Implement ServletContext.addJspFile
janbartel Jan 8, 2020
8fc82bf
Issue #4433 remove todo
janbartel Jan 8, 2020
dbeb486
Issue #4433 Add extra tests
janbartel Jan 16, 2020
0040f55
Issue #4433 Add demo base test for addJsp
janbartel Jan 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2695,7 +2695,6 @@ public javax.servlet.ServletRegistration.Dynamic addServlet(String servletName,
@Override
public ServletRegistration.Dynamic addJspFile(String servletName, String jspFile)
{
// TODO new in 4.0
LOG.warn(UNIMPLEMENTED_USE_SERVLET_CONTEXT_HANDLER, "addJspFile(String, String)");
return null;
}
Expand Down
28 changes: 28 additions & 0 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import javax.servlet.ServletContextListener;
import javax.servlet.ServletException;
import javax.servlet.ServletRegistration;
import javax.servlet.ServletRegistration.Dynamic;
import javax.servlet.ServletSecurityElement;
import javax.servlet.SessionCookieConfig;
import javax.servlet.SessionTrackingMode;
Expand Down Expand Up @@ -1256,6 +1257,33 @@ public ServletRegistration.Dynamic addServlet(String servletName, Servlet servle
else
return null; //existing completed registration for servlet name
}

@Override
public ServletRegistration.Dynamic addJspFile(String servletName, String jspFile)
{
checkDynamic(servletName);

final ServletHandler handler = ServletContextHandler.this.getServletHandler();
ServletHolder holder = handler.getServlet(servletName);
if (holder == null)
{
//new servlet
holder = handler.newServletHolder(Source.JAVAX_API);
holder.setName(servletName);
holder.setForcedPath(jspFile);
handler.addServlet(holder);
return dynamicHolderAdded(holder);
}

//complete a partial registration
if (holder.getClassName() == null && holder.getHeldClass() == null && holder.getForcedPath() == null)
{
holder.setForcedPath(jspFile);
return holder.getRegistration();
}
else
return null; //existing completed registration for servlet name
}

@Override
public boolean setInitParameter(String name, String value)
Expand Down
117 changes: 117 additions & 0 deletions jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletContextHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import javax.servlet.http.HttpSessionIdListener;
import javax.servlet.http.HttpSessionListener;

import org.eclipse.jetty.http.pathmap.MappedResource;
import org.eclipse.jetty.security.ConstraintSecurityHandler;
import org.eclipse.jetty.security.RoleInfo;
import org.eclipse.jetty.security.SecurityHandler;
Expand Down Expand Up @@ -884,6 +885,117 @@ public void onStartup(Set<Class<?>> c, ServletContext ctx) throws ServletExcepti
assertThat("Response", response, containsString("Hello World"));
}

@Test
public void testAddJspFile() throws Exception
{
ContextHandlerCollection contexts = new ContextHandlerCollection();
_server.setHandler(contexts);

ServletContextHandler root = new ServletContextHandler(contexts, "/");
ServletHolder jspServlet = new ServletHolder();
jspServlet.setName("jsp");
jspServlet.setHeldClass(FakeJspServlet.class);
root.addServlet(jspServlet, "*.jsp");
class JSPAddingSCI implements ServletContainerInitializer
{
@Override
public void onStartup(Set<Class<?>> c, ServletContext ctx) throws ServletException
{
try
{
ServletRegistration rego = ctx.addJspFile("some.jsp", "/path/to/some.jsp");
rego.addMapping("/somejsp/*");
}
catch (Exception e)
{
fail(e);
}
}
}

root.addBean(new MySCIStarter(root.getServletContext(), new JSPAddingSCI()), true);
_server.start();
MappedResource<ServletHolder> mappedServlet = root.getServletHandler().getMappedServlet("/somejsp/xxx");
assertNotNull(mappedServlet.getResource());
assertEquals("some.jsp", mappedServlet.getResource().getName());
}

@Test
public void testAddJspFileWithExistingRegistration() throws Exception
{
ContextHandlerCollection contexts = new ContextHandlerCollection();
_server.setHandler(contexts);

ServletContextHandler root = new ServletContextHandler(contexts, "/");
ServletHolder jspServlet = new ServletHolder();
jspServlet.setName("jsp");
jspServlet.setHeldClass(FakeJspServlet.class);
root.addServlet(jspServlet, "*.jsp");
//add a full registration so that the addJspFile will fail
ServletHolder barServlet = new ServletHolder();
barServlet.setName("some.jsp");
barServlet.setHeldClass(HelloServlet.class);
root.addServlet(barServlet, "/bar/*");
class JSPAddingSCI implements ServletContainerInitializer
{
@Override
public void onStartup(Set<Class<?>> c, ServletContext ctx) throws ServletException
{
try
{
ServletRegistration rego = ctx.addJspFile("some.jsp", "/path/to/some.jsp");
assertNull(rego);
}
catch (Exception e)
{
fail(e);
}
}
}

root.addBean(new MySCIStarter(root.getServletContext(), new JSPAddingSCI()), true);
_server.start();
}

@Test
public void testAddJspFileWithPartialRegistration() throws Exception
{
ContextHandlerCollection contexts = new ContextHandlerCollection();
_server.setHandler(contexts);

ServletContextHandler root = new ServletContextHandler(contexts, "/");
ServletHolder jspServlet = new ServletHolder();
jspServlet.setName("jsp");
jspServlet.setHeldClass(FakeJspServlet.class);
root.addServlet(jspServlet, "*.jsp");
//add a preliminary registration so that the addJspFile will complete it
ServletHolder barServlet = new ServletHolder();
barServlet.setName("some.jsp");
root.addServlet(barServlet, "/bar/*");
class JSPAddingSCI implements ServletContainerInitializer
{
@Override
public void onStartup(Set<Class<?>> c, ServletContext ctx) throws ServletException
{
try
{
ServletRegistration rego = ctx.addJspFile("some.jsp", "/path/to/some.jsp");
assertNotNull(rego);
}
catch (Exception e)
{
fail(e);
}
}
}

root.addBean(new MySCIStarter(root.getServletContext(), new JSPAddingSCI()), true);
_server.start();
MappedResource<ServletHolder> mappedServlet = root.getServletHandler().getMappedServlet("/bar/xxx");
assertNotNull(mappedServlet.getResource());
assertEquals("some.jsp", mappedServlet.getResource().getName());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other tests in this class have code that actually make a HTTP request to test the functionality.
This new test only adds the dynamic JSP but does not test that making a HTTP request to it works.
Is that intended?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should we not have 3 tests for this functionality? No mapping, partial mapping, existing mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't add a test for a jsp file because jetty-servlet cannot depend on apache-jsp module. I've added an assertion that tests that the mapped jsp servlet is the one matched by the ServletHandler for the given path, that's as much as we can do.

I also added tests for an existing full mapping and an existing preliminary mapping.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a jspFile via this API in the test-spec webapp as an integration test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was already part of the initial commit: the test-spec-webapp calls addJsp via a ServletContainerInitiaizer. See https://github.com/eclipse/jetty.project/blob/jetty-10.0.x-4433-addJspFile/tests/test-webapps/test-servlet-spec/test-container-initializer/src/main/java/com/acme/initializer/FooInitializer.java#L108 and https://github.com/eclipse/jetty.project/blob/jetty-10.0.x-4433-addJspFile/tests/test-webapps/test-servlet-spec/test-spec-webapp/src/main/java/com/acme/test/AnnotationTest.java#L255

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! can we then check that the test-distribution will see any fail for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


@Test
public void testAddServletAfterStart() throws Exception
{
Expand Down Expand Up @@ -1391,6 +1503,11 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se
}
}
}

public static class FakeJspServlet extends HttpServlet
{

}

public static class ServletAddingServlet extends HttpServlet
{
Expand Down
5 changes: 5 additions & 0 deletions ...et-spec/test-container-initializer/src/main/java/com/acme/initializer/FooInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,10 @@ public void onStartup(Set<Class<?>> classes, ServletContext context)
ServletRegistration.Dynamic reg = context.addServlet("AnnotationTest", "com.acme.AnnotationTest");
context.setAttribute("com.acme.AnnotationTest.complete", (reg == null));
context.addListener(new FooListener());

//test adding jsp file dynamically
ServletRegistration.Dynamic jspFile = context.addJspFile("dynamic.jsp", "/dynamic.jsp");
context.setAttribute("com.acme.jsp.file", (jspFile != null));
jspFile.addMapping("/dynamicjsp/*");
}
}
6 changes: 5 additions & 1 deletion ...ebapps/test-servlet-spec/test-spec-webapp/src/main/java/com/acme/test/AnnotationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,11 @@ else if (!classNames.containsAll(__HandlesTypes))
out.println("<h2>ServletContextListener Registration Prevented from ServletContextListener</h2>");
Boolean webListenerPrevention = (Boolean)config.getServletContext().getAttribute("com.acme.AnnotationTest.sclFromSclRegoTest");
out.println("<p><b>Result: " + (webListenerPrevention.booleanValue() ? "<span class=\"pass\">PASS" : "<span class=\"fail\">FAIL") + "</span></b></p>");


out.println("<h2>Add Jsp File Registration</h2>");
complete = (Boolean)config.getServletContext().getAttribute("com.acme.jsp.file");
out.println("<p><b>Result: " + (complete.booleanValue() ? "<span class=\"pass\">PASS" : "<span class=\"fail\">FAIL") + "</span></b></p>");

out.println("<h2>ServletContextListener In web.xml Injected</h2>");
Boolean listenerInject = (Boolean)config.getServletContext().getAttribute("com.acme.AnnotationTest.sclInjectTest");
out.println("<p><b>Result: " + (listenerInject.booleanValue() ? "<span class=\"pass\">PASS" : "<span class=\"fail\">FAIL") + "</span></b></p>");
Expand Down
16 changes: 16 additions & 0 deletions tests/test-webapps/test-servlet-spec/test-spec-webapp/src/main/webapp/dynamic.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<%@ page contentType="text/html; charset=UTF-8" %>
<%@ page import="java.util.*"%>
<%@ page import="javax.servlet.*" %>
<%@ page import="javax.servlet.http.*" %>
<html>
<head>
<title>Dynamic</title>
</head>

<body>

<h1>Programmatically Added Jsp File</h1>
Success.
</body>

</html>
4 changes: 4 additions & 0 deletions tests/test-webapps/test-servlet-spec/test-spec-webapp/src/main/webapp/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ <h3>Test Servlet 2.5/3.1 Annotations, Fragments and Initializers</h3>
<button type="submit">Test</button>
</form>

<h3>Test Dynamically Added Jsp File</h3>
<p>Click the link to test accessing a programmatically added jsp file</p>
<a href="dynamicjsp/x">Dynamically added jsp </a>

<h3>Test Static Content from Fragment </h3>
<p>Click the link to test accessing static content from a fragment's META-INF/resources</p>
<a href="fragmentA/index.html">Static resource from a fragment </a>
Expand Down