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

Cleanup webappconfiguration #3703

Merged
merged 7 commits into from
Jun 5, 2019
Merged

Cleanup webappconfiguration #3703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff67b37
Cleanup webappconfiguration
gregw May 28, 2019
646fa4f
formatting fix
gregw May 28, 2019
89291bb
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
gregw May 28, 2019
cf9a1f9
cleanup examples and OSGi
gregw May 29, 2019
71e66ad
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
gregw Jun 3, 2019
52c332a
disable classloader test until 9.4.19 is released with #3726 is fixed
gregw Jun 3, 2019
4062cde
cleanup from review
gregw Jun 4, 2019
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
4 changes: 2 additions & 2 deletions examples/async-rest/async-rest-webapp/src/main/webapp/WEB-INF/jetty-web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ org.eclipse.jetty.servlet.WebApplicationContext object
-->

<Configure class="org.eclipse.jetty.webapp.WebAppContext">
<Get class="org.eclipse.jetty.util.log.Log" name="rootLogger">
<Call name="warn"><Arg>async-rest webapp is deployed. DO NOT USE IN PRODUCTION!</Arg></Call>
<Get name="servletContext">
<Call name="log"><Arg>The async-rest webapp is deployed. DO NOT USE IN PRODUCTION!</Arg><Arg></Arg></Call>
gregw marked this conversation as resolved.
Show resolved Hide resolved
</Get>
</Configure>
2 changes: 2 additions & 0 deletions ...etty-osgi/src/test/java/org/eclipse/jetty/osgi/test/TestJettyOSGiBootWithAnnotations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ public static Option[] configure()
"com.sun.org.apache.xpath.internal.jaxp", "com.sun.org.apache.xpath.internal.objects"));

options.addAll(TestOSGiUtil.coreJettyDependencies());
// TODO uncomment and update the following once 9.4.19 is released with a fix for #3726
// options.add(mavenBundle().groupId("org.eclipse.jetty").artifactId("jetty-util").version("9.4.19.v????????").noStart());
options.add(mavenBundle().groupId("org.eclipse.jetty").artifactId("jetty-alpn-java-client").versionAsInProject().start());
options.add(mavenBundle().groupId("org.eclipse.jetty").artifactId("jetty-alpn-client").versionAsInProject().start());
options.add(systemProperty("org.ops4j.pax.logging.DefaultServiceLog.level").value(LOG_LEVEL));
Expand Down
2 changes: 1 addition & 1 deletion jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ boolean appliesTo(int type)
if (_holder==null)
return false;
if (_dispatches==0)
return type==REQUEST || type==ASYNC && holder.isAsyncSupported();
return type==REQUEST || type==ASYNC && holder.isAsyncSupported();
return (_dispatches&type)!=0;
}

Expand Down
12 changes: 4 additions & 8 deletions jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,10 @@ public WebAppConfiguration()
addDependencies(WebXmlConfiguration.class, MetaInfConfiguration.class, WebInfConfiguration.class);
addDependents(JettyWebXmlConfiguration.class);
protectAndExpose(
"org.eclipse.jetty.util.log.",
"org.eclipse.jetty.server.session.SessionData",
"org.eclipse.jetty.servlet.StatisticsServlet",
"org.eclipse.jetty.servlet.StatisticsServlet",
"org.eclipse.jetty.servlet.DefaultServlet",
"org.eclipse.jetty.servlet.NoJspServlet",
"org.eclipse.jetty.continuation.");
expose( // TODO Evaluate why these are not protectAndExpose?
"org.eclipse.jetty.servlet.listener.",
"org.eclipse.jetty.alpn.");
"org.eclipse.jetty.servlet.NoJspServlet"
);
expose("org.eclipse.jetty.servlet.listener.");
Copy link
Contributor

Choose a reason for hiding this comment

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

All these are classes/packages are in org.eclipse.jetty.servlet.
Would it not make more sense to have a Configuration class in the jetty-servlet module rather than in jetty-webapp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. servlet is usable without webapps. But configuration really about configuring webapps - specifically by a combination of convention and the configurations we push into it from our modules. If we wanted to move Configuration down to servlet, then we'd have to move the WebAppClassLoader (or at least an abstraction of it) down as well... and then we start having no difference between servlet and webapp.

tl;dr; any direct users of jetty-servlet configure by using API directly and probably don't have a different classloader (maybe not even a context).

}
}
4 changes: 2 additions & 2 deletions tests/test-webapps/test-jaas-webapp/src/main/webapp/WEB-INF/jetty-web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<!DOCTYPE Configure PUBLIC "-//Mort Bay Consulting//DTD Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">

<Configure class="org.eclipse.jetty.webapp.WebAppContext">
<Get class="org.eclipse.jetty.util.log.Log" name="rootLogger">
<Call name="warn"><Arg>test-jaas webapp is deployed. DO NOT USE IN PRODUCTION!</Arg></Call>
<Get name="servletContext">
<Call name="log"><Arg>The test-jaas webapp is deployed. DO NOT USE IN PRODUCTION!</Arg><Arg></Arg></Call>
</Get>
</Configure>
1 change: 1 addition & 0 deletions tests/test-webapps/test-jetty-webapp/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-servlets</artifactId>
<version>${project.version}</version>
<scope>provided</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why provided? jetty-servlets is supposed to be portable across containers (it's not, but I think that's the idea), so it should really be in WEB-INF/lib.

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 don't think they are portable. More over, the jetty-servlets module exposes them on the webapp classpath. Putting them in webapp just brings in all our io, http and util classes and exposes all the classloader problems that entails.
In fact, some of the classes (eg PushCacheFilter) only work if loaded from the container. I don't think we are in the business of providing general utility servlets for any container.

</dependency>
<dependency>
<groupId>org.eclipse.jetty.toolchain</groupId>
Expand Down
5 changes: 3 additions & 2 deletions tests/test-webapps/test-jetty-webapp/src/main/webapp/WEB-INF/jetty-web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ org.eclipse.jetty.servlet.WebApplicationContext object
</Arg>
</Call>
</Get>
<Get class="org.eclipse.jetty.util.log.Log" name="rootLogger">
<Call name="warn"><Arg>test webapp is deployed. DO NOT USE IN PRODUCTION!</Arg></Call>

<Get name="servletContext">
<Call name="log"><Arg>The test-jetty webapp is deployed. DO NOT USE IN PRODUCTION!</Arg><Arg></Arg></Call>
</Get>
</Configure>
4 changes: 2 additions & 2 deletions tests/test-webapps/test-jndi-webapp/src/main/webapp/WEB-INF/jetty-web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<!DOCTYPE Configure PUBLIC "-//Mort Bay Consulting//DTD Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">

<Configure class="org.eclipse.jetty.webapp.WebAppContext">
<Get class="org.eclipse.jetty.util.log.Log" name="rootLogger">
<Call name="warn"><Arg>test-jndi webapp is deployed. DO NOT USE IN PRODUCTION!</Arg></Call>
<Get name="servletContext">
<Call name="log"><Arg>The test-jndi webapp is deployed. DO NOT USE IN PRODUCTION!</Arg><Arg></Arg></Call>
</Get>
</Configure>
3 changes: 3 additions & 0 deletions tests/test-webapps/test-proxy-webapp/src/main/webapp/WEB-INF/jetty-web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,8 @@
<!DOCTYPE Configure PUBLIC "-//Mort Bay Consulting//DTD Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">

<Configure class="org.eclipse.jetty.webapp.WebAppContext">
<Get name="servletContext">
<Call name="log"><Arg>The test-proxy webapp is deployed. DO NOT USE IN PRODUCTION!</Arg><Arg></Arg></Call>
</Get>
<Set name="contextPath">/proxy</Set>
</Configure>
48 changes: 26 additions & 22 deletions tests/test-webapps/test-servlet-spec/test-spec-webapp/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
<instructions>
<Bundle-SymbolicName>org.eclipse.jetty.tests.test-spec-webapp</Bundle-SymbolicName>
<Bundle-Description>Test Webapp for Servlet 3.1 Features</Bundle-Description>
<!-- TODO Add 'org.eclipse.jetty.util;version="[9.4.19,9.4.20)",' below, once 9.4.19 is released with a fix for #3726 -->
<Import-Package>
javax.transaction*;version="[1.1,1.3)",
javax.servlet*;version="[2.6,4.1)",
Expand Down Expand Up @@ -199,12 +200,6 @@
<artifactId>jakarta.transaction-api</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-server</artifactId>
<version>${project.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.toolchain</groupId>
<artifactId>jetty-servlet-api</artifactId>
Expand All @@ -215,21 +210,30 @@
<artifactId>jakarta.annotation-api</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.tests</groupId>
<artifactId>test-web-fragment</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.tests</groupId>
<artifactId>test-container-initializer</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
<!-- deliberately old version to test classloading -->
<version>9.3.0.RC0</version>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.tests</groupId>
<artifactId>test-web-fragment</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.tests</groupId>
<artifactId>test-container-initializer</artifactId>
<version>${project.version}</version>
</dependency>

<!-- deliberately old version to test classloading -->
<!-- TODO uncomment and update the following once 9.4.19 is released with a fix for #3726
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
<version>9.4.19.vXXXXXXXX</version>
<exclusions>
<exclusion>
<groupId>javax.servlet</groupId>
<artifactId>javax.servlet-api</artifactId>
</exclusion>
</exclusions>
</dependency>
-->
</dependencies>
</project>
5 changes: 3 additions & 2 deletions ...ps/test-servlet-spec/test-spec-webapp/src/main/java/com/acme/test/ClassLoaderServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.eclipse.jetty.util.IO;

@WebServlet(urlPatterns="/classloader")
public class ClassLoaderServlet extends HttpServlet
{
Expand All @@ -48,6 +46,8 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
writer.println("<body>");
writer.println("<h1>ClassLoader Isolation Test</h1>");

// TODO uncomment the following once 9.4.19 is released with a fix for #3726
/*
Class<?> webappIO = IO.class;
URI webappURI = getLocationOfClass(webappIO);
String webappVersion = webappIO.getPackage().getImplementationVersion();
Expand All @@ -65,6 +65,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
writer.println("<br/><b>URI Result: <span class=\"fail\">FAIL</span></b></p>");
else
writer.println("<br/><b>URI Result: <span class=\"pass\">PASS</span></b></p>");
*/

writer.println("</body>");
writer.println("</html>");
Expand Down
25 changes: 19 additions & 6 deletions ...webapps/test-servlet-spec/test-spec-webapp/src/main/java/com/acme/test/MultiPartTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
package com.acme.test;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.util.Collection;

import javax.servlet.ServletConfig;
Expand All @@ -30,7 +32,6 @@
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.Part;

import org.eclipse.jetty.util.IO;
/**
* MultiPartTest
*
Expand Down Expand Up @@ -58,7 +59,6 @@ public void init(ServletConfig config) throws ServletException
@Override
public void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{

try
{
response.setContentType("text/html");
Expand All @@ -78,7 +78,7 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) thr
if (p.getContentType() == null || p.getContentType().startsWith("text/plain"))
{
out.println("<p>");
IO.copy(p.getInputStream(),out);
copy(p.getInputStream(),out);
out.println("</p>");
}
}
Expand Down Expand Up @@ -116,8 +116,21 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro
throw new ServletException(e);
}
}




// TODO remove inline once 9.4.19 is released with a fix for #3726
public static void copy(InputStream in,
OutputStream out)
throws IOException
{
int bufferSize = 8192;
byte buffer[] = new byte[bufferSize];

while (true)
{
int len=in.read(buffer,0,bufferSize);
if (len<0 )
break;
out.write(buffer,0,len);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason to inline IO.copy()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because you wouldn't let me do a release of a fixed jetty-util jar :)
Once we have a fixed 9.4 jetty-util release, we can re-instate the tests that use it and then we can remove this inline (I told you it was a pain!)
I don't want to bring in jetty-10 util just for this 1 usage.

}
7 changes: 6 additions & 1 deletion tests/test-webapps/test-servlet-spec/test-spec-webapp/src/main/webapp/WEB-INF/jetty-web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
<!DOCTYPE Configure PUBLIC "-//Mort Bay Consulting//DTD Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">

<Configure class="org.eclipse.jetty.webapp.WebAppContext">
<Get name="servletContext">
<Call name="log"><Arg>The test-spec webapp is deployed. DO NOT USE IN PRODUCTION!</Arg><Arg></Arg></Call>
</Get>
<!--TODO uncomment once 9.4.19 is released with a fix for #3726
<Get class="org.eclipse.jetty.util.log.Log" name="rootLogger">
<Call name="warn"><Arg>test-spec webapp is deployed. DO NOT USE IN PRODUCTION!</Arg></Call>
<Call name="info"><Arg>WEB-INF/lib/jetty-util.jar logging used!</Arg></Call>
gregw marked this conversation as resolved.
Show resolved Hide resolved
</Get>
-->
</Configure>