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.

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

Issue #10084 - Directory results from getResourcePaths(String) should include trailing slash #10085

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,29 @@ public static String getFileName(String path)
return "";
int idx = path.lastIndexOf('/');
if (idx >= 0)
{
if (idx == path.length() - 1)
{
// we found the trailing slash
// eg: file:/path/to/dir/
// we want to return the "dir" segment here
int previousSlash = path.lastIndexOf('/', idx - 1);
if (previousSlash >= 0)
{
// we have a previous slash
// so return the segment without the trailing slash
return path.substring(previousSlash + 1, idx);
}
else
{
// we have no previous slash
// this input string is something like "foo/"
// so return the segment without trailing slash
return path.substring(0, idx);
}
}
return path.substring(idx + 1);
}
return path;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ public long length()
*
* <p>This is the last segment of the path.</p>
*
* @return the filename of the resource, or "" if there are no path segments (eg: path of "/"), or null if not backed by a Path
* @return the filename of the resource, or "" if there are no path segments (eg: path of "/"), or null if resource has no path.
* @see Path#getFileName()
*/
public abstract String getFileName();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,14 @@ public static Stream<Arguments> fileNameSource()
{
return Stream.of(
Arguments.of(null, ""),
Arguments.of(URI.create("foo:bar"), "bar"),
Arguments.of(URI.create("file:/"), ""),
Arguments.of(URI.create("file:///"), ""),
Arguments.of(URI.create("file:zed/"), ""),
Arguments.of(URI.create("file:zed/"), "zed"),
Arguments.of(URI.create("file:///path/to/test.txt"), "test.txt"),
Arguments.of(URI.create("file:///path/to/dir/"), ""),
Arguments.of(URI.create("file:///path/to/dir/"), "dir"),
Arguments.of(URI.create("jar:file:///home/user/libs/jetty-server-12.jar!/org/eclipse/jetty/server/jetty-dir.css"), "jetty-dir.css"),
Arguments.of(URI.create("http://eclipse.org/jetty/"), ""),
Arguments.of(URI.create("http://eclipse.org/jetty/"), "jetty"),
Arguments.of(URI.create("http://eclipse.org/jetty/index.html"), "index.html"),
Arguments.of(URI.create("http://eclipse.org/jetty/docs.html?query=val#anchor"), "docs.html")
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.HashMap;
import java.util.Map;

import org.eclipse.jetty.toolchain.test.FS;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
Expand Down Expand Up @@ -353,6 +354,25 @@ public void testNullCharEndingFilename(WorkDir workDir) throws Exception
}
}

@Test
public void testGetFileName(WorkDir workDir) throws IOException
{
Path tmpPath = workDir.getEmptyPathDir();
Path dir = tmpPath.resolve("foo-dir");
FS.ensureDirExists(dir);
Path file = dir.resolve("bar.txt");
Files.writeString(file, "This is bar.txt", StandardCharsets.UTF_8);

Resource baseResource = new PathResource(tmpPath);
assertThat(baseResource.getFileName(), is(tmpPath.getFileName().toString()));

Resource dirResource = baseResource.resolve("foo-dir");
assertThat(dirResource.getFileName(), is(dir.getFileName().toString()));

Resource fileResource = dirResource.resolve("bar.txt");
assertThat(fileResource.getFileName(), is(file.getFileName().toString()));
}

@Test
public void testSymlink(WorkDir workDir) throws Exception
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,20 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

import org.eclipse.jetty.toolchain.test.FS;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.URIUtil;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

import static org.awaitility.Awaitility.await;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.endsWith;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
Expand All @@ -44,6 +49,7 @@
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

@ExtendWith(WorkDirExtension.class)
public class UrlResourceFactoryTest
{
@Test
Expand Down Expand Up @@ -77,7 +83,7 @@ public void testHttps() throws IOException
assertThat(blogs.lastModified().toEpochMilli(), not(Instant.EPOCH));
assertThat(blogs.length(), not(-1));
assertTrue(blogs.isDirectory());
assertThat(blogs.getFileName(), is(""));
assertThat(blogs.getFileName(), is("blog"));

Resource favicon = resource.resolve("favicon.ico");
assertThat(favicon, notNullValue());
Expand All @@ -93,6 +99,27 @@ public void testHttps() throws IOException
}
}

@Test
public void testGetFileName(WorkDir workDir) throws IOException
{
Path tmpPath = workDir.getEmptyPathDir();
Path dir = tmpPath.resolve("foo-dir");
FS.ensureDirExists(dir);
Path file = dir.resolve("bar.txt");
Files.writeString(file, "This is bar.txt", StandardCharsets.UTF_8);

URLResourceFactory urlResourceFactory = new URLResourceFactory();

Resource baseResource = urlResourceFactory.newResource(tmpPath);
assertThat(baseResource.getFileName(), endsWith(""));

Resource dirResource = baseResource.resolve("foo-dir/");
assertThat(dirResource.getFileName(), endsWith("foo-dir"));

Resource fileResource = dirResource.resolve("bar.txt");
assertThat(fileResource.getFileName(), endsWith("bar.txt"));
}

@Test
public void testInputStreamCleanedUp() throws Exception
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.net.URI;
import java.net.URL;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -810,12 +811,15 @@ public Set<String> getResourcePaths(String path)
Resource resource = getResource(path);

if (!path.endsWith("/"))
path = path + "/";
path = path + '/';

HashSet<String> set = new HashSet<>();
for (Resource item: resource.list())
{
set.add(path + item.getFileName());
String entry = path + item.getFileName();
if (item.isDirectory())
entry = entry + '/';
set.add(entry);
Comment on lines +819 to +822
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to resolve this within the resource classes? Previously the resource classes would have the trailing '/' on directories so it would be everywhere. Doing it just here as a post process risks not covering all usages.

Copy link
Contributor Author

@joakime joakime Jul 8, 2023

Choose a reason for hiding this comment

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

The Resource object returns a List<Resource> so I think perhaps the place would be Resource.getFileName() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes getFileName, toString and anything else that returns the path as a string. Ideally even the URI should have the trailing slash. But
I don't think Path retains it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out fixing getFileName() breaks a lot of test cases.
See latest Jenkins build on this PR.
I'll noodle through them on monday.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to check if any of these texts existed in jetty 11 and see if they were changed to not expect the trailing slash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Jetty 11 Resource.getName() does not return the trailing slash on directories.

However, the Jetty 11 Resource.list() returns a String array, and that array has the trailing slash.
The Resource.list() method in Jetty 11 is used ...

  • ContextHandler.getResourcePaths(String) which returns a Set<String> on the Servlet API
  • Resource.getListHTML - used to iterate a directory, and build a new set of Resources, that eventually are presented (in HTML)
  • MetaInfConfiguration - iterates over WEB-INF/lib/*.jar files, and builds a new set of Resources that eventually wind up in the WebAppClassLoader
  • WebAppClassLoader - used in addJars(Resource) to iterate a directory and build a new set of Resources that are added to the WebAppClassLoader
  • Runner.Classpath.addJars(Resource) - same a WebAppClassLoader.addJars(Resource)

Turns out that in Jetty 12 we skip the String array step (eg: the Resource -> String array -> List of Resource -> iterate/process flow).
In Jetty 11, we don't rely on the results of Resource.getName() for anything critical, it seems to be used only for things like debugging, exception messages, keys in the annotation parser, etc.

}
return set;
}
Expand Down Expand Up @@ -2768,7 +2772,12 @@ else if (path.charAt(0) != '/')
{
Path resourcePath = r.getPath();
if (resourcePath != null)
return resourcePath.normalize().toString();
{
String realPath = resourcePath.normalize().toString();
if (Files.isDirectory(resourcePath))
realPath = realPath + "/";
return realPath;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.List;
import java.util.function.Consumer;
Expand Down Expand Up @@ -46,6 +45,7 @@
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.equalTo;

public class ServletRequestListenerTest
Expand Down Expand Up @@ -316,7 +316,7 @@ private void assertEventsEmpty()

private void assertEvents(String... events)
{
assertThat(_events, equalTo(Arrays.asList(events)));
assertThat(_events, contains(events));
_events.clear();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Arrays;
import java.util.Comparator;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -512,8 +513,8 @@ public void testGetResourceFromCollection() throws Exception
WebAppContext context = new WebAppContext();
context.setContextPath("/");
context.setBaseResource(ResourceFactory.combine(
ResourceFactory.root().newResource(MavenTestingUtils.getTestResourcePath("wars/layer0/")),
ResourceFactory.root().newResource(MavenTestingUtils.getTestResourcePath("wars/layer1/"))));
context.getResourceFactory().newResource(MavenTestingUtils.getTestResourcePath("wars/layer0/")),
context.getResourceFactory().newResource(MavenTestingUtils.getTestResourcePath("wars/layer1/"))));
server.setHandler(context);
server.start();

Expand All @@ -530,13 +531,39 @@ public void testGetResourcePathsFromCollection() throws Exception
WebAppContext context = new WebAppContext();
context.setContextPath("/");
context.setBaseResource(ResourceFactory.combine(
ResourceFactory.root().newResource(MavenTestingUtils.getTestResourcePath("wars/layer0/")),
ResourceFactory.root().newResource(MavenTestingUtils.getTestResourcePath("wars/layer1/"))));
context.getResourceFactory().newResource(MavenTestingUtils.getTestResourcePath("wars/layer0/")),
context.getResourceFactory().newResource(MavenTestingUtils.getTestResourcePath("wars/layer1/"))));
server.setHandler(context);
server.start();

ServletContext servletContext = context.getServletContext();
assertThat(servletContext.getResourcePaths("/WEB-INF"), containsInAnyOrder("/WEB-INF/zero.xml", "/WEB-INF/one.xml"));
assertThat(servletContext.getResourcePaths("/WEB-INF/"), containsInAnyOrder("/WEB-INF/zero.xml", "/WEB-INF/one.xml"));
}

@Test
public void testGetResourcePathsWithDirsFromCollection() throws Exception
{
Server server = newServer();

WebAppContext context = new WebAppContext();
context.setContextPath("/");
context.setBaseResource(ResourceFactory.combine(
context.getResourceFactory().newResource(MavenTestingUtils.getTestResourcePath("wars/layer0/")),
context.getResourceFactory().newResource(MavenTestingUtils.getTestResourcePath("wars/layer1/")),
context.getResourceFactory().newResource(MavenTestingUtils.getTestResourcePath("wars/with_dirs/"))
));
server.setHandler(context);
server.start();

ServletContext servletContext = context.getServletContext();
Set<String> results = servletContext.getResourcePaths("/WEB-INF/");
String[] expected = {
"/WEB-INF/zero.xml",
"/WEB-INF/one.xml",
"/WEB-INF/bar/",
"/WEB-INF/foo/"
};
assertThat(results, containsInAnyOrder(expected));
}

@Test
Expand All @@ -552,18 +579,21 @@ public void testGetResourcePaths() throws Exception

ServletContext servletContext = context.getServletContext();

List<String> resourcePaths = List.copyOf(servletContext.getResourcePaths("/"));
Set<String> resourcePaths = servletContext.getResourcePaths("/");
String[] expected = {
"/WEB-INF/",
"/nested-reserved-!#\\\\$%&()*+,:=?@[]-meta-inf-resource.txt",
};
assertThat(resourcePaths.size(), is(2));
assertThat(resourcePaths.get(0), is("/WEB-INF"));
assertThat(resourcePaths.get(1), is("/nested-reserved-!#\\\\$%&()*+,:=?@[]-meta-inf-resource.txt"));
assertThat(resourcePaths, containsInAnyOrder(expected));

String realPath = servletContext.getRealPath("/");
assertThat(realPath, notNullValue());
assertThat(servletContext.getRealPath(resourcePaths.get(0)), endsWith("/WEB-INF"));
assertThat(servletContext.getRealPath("/WEB-INF/"), endsWith("/WEB-INF/"));
// TODO the following assertion fails because of a bug in the JDK (see JDK-8311079 and MountedPathResourceTest.testJarFileResourceAccessBackSlash())
//assertThat(servletContext.getRealPath(resourcePaths.get(1)), endsWith("/nested-reserved-!#\\\\$%&()*+,:=?@[]-meta-inf-resource.txt"));

assertThat(servletContext.getResource("/WEB-INF"), notNullValue());
assertThat(servletContext.getResource("/WEB-INF/"), notNullValue());
// TODO the following assertion fails because of a bug in the JDK (see JDK-8311079 and MountedPathResourceTest.testJarFileResourceAccessBackSlash())
//assertThat(servletContext.getResource("/nested-reserved-!#\\\\$%&()*+,:=?@[]-meta-inf-resource.txt"), notNullValue());

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This is the "main.txt" in the /bar/ directory for war "with_dirs"
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This is the "alt.txt" in the /foo/ directory for war "with_dirs"
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -1568,16 +1569,16 @@ public Set<String> getResourcePaths(String path)
Resource resource = getResource(path);

if (!path.endsWith("/"))
path = path + "/";
path = path + '/';

HashSet<String> set = new HashSet<>();

for (Resource r: resource)
for (Resource item: resource.list())
{
for (Resource item: r.list())
{
set.add(path + item.getFileName());
}
String entry = path + item.getFileName();
if (item.isDirectory())
entry = entry + '/';
set.add(entry);
}
return set;
}
Expand Down Expand Up @@ -1907,9 +1908,24 @@ else if (path.charAt(0) != '/')
Resource resource = ContextHandler.this.getResource(path);
if (resource != null)
{
Path resourcePath = resource.getPath();
if (resourcePath != null)
return resourcePath.toAbsolutePath().normalize().toString();
for (Resource r : resource)
{
// return first
if (Resources.exists(r))
{
Path resourcePath = r.getPath();
if (resourcePath != null)
{
String realPath = resourcePath.normalize().toString();
if (Files.isDirectory(resourcePath))
realPath = realPath + "/";
return realPath;
}
}
}

// A Resource was returned, but did not exist
return null;
}
}
catch (Exception e)
Expand Down
6 changes: 6 additions & 0 deletions jetty-ee9/jetty-ee9-webapp/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@
<directory>src/test/webapp</directory>
<targetPath>webapp</targetPath>
</testResource>
<testResource>
<directory>src/test/webapp-with-resources</directory>
<targetPath>webapp-with-resources</targetPath>
</testResource>
<testResource>
<directory>src/test/webapp-alt-jsp</directory>
<targetPath>webapp-alt-jsp</targetPath>
Expand All @@ -50,6 +54,8 @@
<configuration>
<argLine>
@{argLine} ${jetty.surefire.argLine}
--add-exports org.eclipse.jetty.ee9.webapp/org.acme.webapp=org.eclipse.jetty.ee9.servlet
--add-exports org.eclipse.jetty.ee9.webapp/org.acme.webapp=org.eclipse.jetty.ee9.nested
</argLine>
<useManifestOnlyJar>false</useManifestOnlyJar>
<additionalClasspathElements>
Expand Down
Loading