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 #11092 - Allow MetaInfConfiguration parsing of java.class.path to support globs #12287

Merged
merged 4 commits into from
Sep 26, 2024
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 @@ -447,21 +447,40 @@ default Resource newJarFileResource(URI uri)
}

/**
* Split a string of references, that may be split with '{@code ,}', or '{@code ;}', or '{@code |}' into URIs.
* Split a string of references, that may be split with '{@code ,}', or '{@code ;}', or '{@code |}' into a List of {@link Resource}.
* <p>
* Each part of the input string could be path references (unix or windows style), or string URI references.
* Each part of the input string could be path references (unix or windows style), string URI references, or even glob references (eg: {@code /path/to/libs/*}).
* </p>
* <p>
* If the result of processing the input segment is a java archive, then its resulting URI will be a mountable URI as {@code jar:file:...!/}
* </p>
*
* @param str the input string of references
* @return list of resources
*/
default List<Resource> split(String str)
{
return split(str, ",;|");
}

/**
* Split a string of references by provided delims into a List of {@link Resource}.
* <p>
* Each part of the input string could be path references (unix or windows style), string URI references, or even glob references (eg: {@code /path/to/libs/*}).
* Note: that if you use the {@code :} character in your delims, then URI references will be impossible.
* </p>
* <p>
* If the result of processing the input segment is a java archive, then its resulting URI will be a mountable URI as {@code jar:file:...!/}
* </p>
*
* @param str the input string of references
* @return list of resources
*/
default List<Resource> split(String str, String delims)
{
List<Resource> list = new ArrayList<>();

StringTokenizer tokenizer = new StringTokenizer(str, ",;|");
StringTokenizer tokenizer = new StringTokenizer(str, delims);
while (tokenizer.hasMoreTokens())
{
String reference = tokenizer.nextToken();
Expand All @@ -475,7 +494,6 @@ default List<Resource> split(String str)
{
List<Resource> expanded = dir.list();
expanded.sort(ResourceCollators.byName(true));
// TODO it is unclear why non archive files are not expanded into the list
expanded.stream().filter(r -> FileID.isLibArchive(r.getName())).forEach(list::add);
}
}
Expand All @@ -487,11 +505,12 @@ default List<Resource> split(String str)
}
catch (Exception e)
{
LOG.warn("Invalid Resource Reference: " + reference);
LOG.warn("Invalid Resource Reference: {}", reference);
throw e;
}
}

// Perform Archive file mounting (if needed)
for (ListIterator<Resource> i = list.listIterator(); i.hasNext(); )
{
Resource resource = i.next();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ public void testSplitOnSemicolon()
}

@Test
public void testSplitOnPipeWithGlob() throws IOException
public void testSplitOnPathSeparatorWithGlob() throws IOException
{
try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add another term to the glob string that is just a single jar file, to mimic a classpath that has both glob and individual jar files on 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.

This test kind has that already, 2 non globs, and 1 glob.
Also the existing test testSplitOnPipeWithGlob() has the same.
Do you want the non-glob to be specifically a jar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followup, do you want the extra non-globs to be before or after the glob?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really mind the position of the jar, just so long as the test is a combination of dirs, a dir with glob and at least one explicit jar.

{
Expand All @@ -418,9 +418,54 @@ public void testSplitOnPipeWithGlob() throws IOException
FS.ensureDirExists(bar);
Files.copy(MavenPaths.findTestResourceFile("jar-file-resource.jar"), bar.resolve("lib-foo.jar"));
Files.copy(MavenPaths.findTestResourceFile("jar-file-resource.jar"), bar.resolve("lib-zed.zip"));
Path exampleJar = base.resolve("example.jar");
Files.copy(MavenPaths.findTestResourceFile("example.jar"), exampleJar);

// This represents a classpath with a glob
String config = String.join(File.pathSeparator, List.of(
dir.toString(), foo.toString(), bar + File.separator + "*", exampleJar.toString()
));

// Split using commas
List<URI> uris = resourceFactory.split(config, File.pathSeparator).stream().map(Resource::getURI).toList();

URI[] expected = new URI[]{
dir.toUri(),
foo.toUri(),
// Should see the two archives as `jar:file:` URI entries
URIUtil.toJarFileUri(bar.resolve("lib-foo.jar").toUri()),
URIUtil.toJarFileUri(bar.resolve("lib-zed.zip").toUri()),
URIUtil.toJarFileUri(exampleJar.toUri())
};

assertThat(uris, contains(expected));
}
}

@ParameterizedTest
@ValueSource(strings = {";", "|", ","})
public void testSplitOnDelimWithGlob(String delimChar) throws IOException
{
try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
// TIP: don't allow raw delim to show up in base dir, otherwise the string split later will be wrong.
Path base = MavenPaths.targetTestDir("testSplitOnPipeWithGlob_%02x".formatted((byte)delimChar.charAt(0)));
FS.ensureEmpty(base);
Path dir = base.resolve("dir");
FS.ensureDirExists(dir);
Path foo = dir.resolve("foo");
FS.ensureDirExists(foo);
Path bar = dir.resolve("bar");
FS.ensureDirExists(bar);
Files.copy(MavenPaths.findTestResourceFile("jar-file-resource.jar"), bar.resolve("lib-foo.jar"));
Files.copy(MavenPaths.findTestResourceFile("jar-file-resource.jar"), bar.resolve("lib-zed.zip"));
Path exampleJar = base.resolve("example.jar");
Files.copy(MavenPaths.findTestResourceFile("example.jar"), exampleJar);

// This represents the user-space raw configuration with a glob
String config = String.format("%s;%s;%s%s*", dir, foo, bar, File.separator);
String config = String.join(delimChar, List.of(
dir.toString(), foo.toString(), bar + File.separator + "*", exampleJar.toString()
));

// Split using commas
List<URI> uris = resourceFactory.split(config).stream().map(Resource::getURI).toList();
Expand All @@ -430,7 +475,8 @@ public void testSplitOnPipeWithGlob() throws IOException
foo.toUri(),
// Should see the two archives as `jar:file:` URI entries
URIUtil.toJarFileUri(bar.resolve("lib-foo.jar").toUri()),
URIUtil.toJarFileUri(bar.resolve("lib-zed.zip").toUri())
URIUtil.toJarFileUri(bar.resolve("lib-zed.zip").toUri()),
URIUtil.toJarFileUri(exampleJar.toUri())
};

assertThat(uris, contains(expected));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,10 @@ public void findAndFilterContainerPaths(final WebAppContext context) throws Exce
String classPath = System.getProperty("java.class.path");
if (classPath != null)
{
Stream.of(classPath.split(File.pathSeparator))
.map(resourceFactory::newResource)
resourceFactory.split(classPath, File.pathSeparator)
.stream()
.filter(Objects::nonNull)
.filter(r -> uriPatternPredicate.test(r.getURI()))
.filter(r -> uriPatternPredicate.test(URIUtil.unwrapContainer(r.getURI())))
.forEach(addContainerResource);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,7 @@ public void testGetContainerPathsWithModuleSystem() throws Exception
.stream()
.sorted(ResourceCollators.byName(true))
.map(Resource::getURI)
.map(URIUtil::unwrapContainer)
.map(URI::toASCIIString)
.toList();
// we "correct" the bad file URLs that come from the ClassLoader
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,10 @@ public void findAndFilterContainerPaths(final WebAppContext context) throws Exce
String classPath = System.getProperty("java.class.path");
if (classPath != null)
{
Stream.of(classPath.split(File.pathSeparator))
.map(resourceFactory::newResource)
.filter(r -> uriPatternPredicate.test(r.getURI()))
resourceFactory.split(classPath, File.pathSeparator)
Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, technically we shouldn't fix ee8 and ee9 because they should be bug-for-bug compatible with jetty 10/11. But ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Jetty 10/11 we ignore globs here.
The raw string is created a File and then if it doesn't exist, it's ignored.
The thing is, a File with a * glob always produces a File instance that doesn't exist (or can be opened).
But a Path cannot even be created with a * glob, it throws an Exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

understood.

.stream()
.filter(Objects::nonNull)
.filter(r -> uriPatternPredicate.test(URIUtil.unwrapContainer(r.getURI())))
.forEach(addContainerResource);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,11 @@

import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.util.resource.FileSystemPool;
import org.eclipse.jetty.util.URIUtil;
import org.eclipse.jetty.util.component.LifeCycle;
import org.eclipse.jetty.util.resource.Resource;
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.empty;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -153,13 +150,14 @@ public void testFindAndFilterContainerPathsJDK9() throws Exception
assertEquals(2, containerResources.size());
for (Resource r : containerResources)
{
String s = r.toString();
String s = URIUtil.unwrapContainer(r.getURI()).toASCIIString();
assertTrue(s.endsWith("foo-bar-janb.jar") || s.contains("servlet-api"));
}
}
finally
{
config.postConfigure(context);
LifeCycle.stop(context.getResourceFactory());
}
}
}
Loading