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 #10164 - improve MetaInfConfiguration use of Resource objects #10171

Closed
wants to merge 16 commits into from
Closed
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 @@ -692,7 +692,7 @@ protected Resource getJarFor(WebAppContext context, ServletContainerInitializer
}

/**
* Check to see if the ServletContainerIntializer loaded via the ServiceLoader came
* Check to see if the ServletContainerInitializer loaded via the ServiceLoader came
* from a jar that is excluded by the fragment ordering. See ServletSpec 3.0 p.85.
*
* @param context the context for the jars
Expand Down Expand Up @@ -754,7 +754,7 @@ public boolean isFromExcludedJar(WebAppContext context, ServletContainerInitiali
boolean included = false;
for (Resource r : orderedJars)
{
included = r.equals(sciResource);
included = r.isContainedIn(sciResource);
if (included)
break;
}
Expand Down Expand Up @@ -968,7 +968,7 @@ else if (entry.getValue() == null) //can't work out provenance of SCI, so can't
{
for (Map.Entry<ServletContainerInitializer, Resource> entry : sciResourceMap.entrySet())
{
if (webInfJar.equals(entry.getValue()))
if (webInfJar.isContainedIn(entry.getValue()))
nonExcludedInitializers.add(entry.getKey());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ public void parse(final Set<? extends Handler> handlers, Resource r) throws Exce
if (!r.exists())
return;

if (FileID.isJavaArchive(r.getPath()))
if (FileID.isJavaArchive(r.getPath())) // TODO: this is now always false, as all Resource objects are directories
{
parseJar(handlers, r);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,28 +285,33 @@ private List<Resource> getBundleAsResource(ResourceFactory resourceFactory, Bund
File file = BundleFileLocatorHelperFactory.getFactory().getHelper().getBundleInstallLocation(bundle);
if (file.isDirectory())
{
// Add directory bundle as a directory
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't add the directory in which the bundles are stored as a Resource, we just want the individual bundles.

resources.add(resourceFactory.newResource(file.toPath()));

for (File f : file.listFiles())
{
if (FileID.isJavaArchive(f.getName()) && f.isFile())
{
resources.add(resourceFactory.newResource(f.toPath()));
// add *.jar as jar files
resources.add(resourceFactory.newJarFileResource(f.toPath().toUri()));
}
else if (f.isDirectory() && f.getName().equals("lib"))
{
for (File f2 : file.listFiles())
for (File libFile : file.listFiles())
{
if (FileID.isJavaArchive(f2.getName()) && f2.isFile())
if (FileID.isJavaArchive(libFile.getName()) && libFile.isFile())
{
resources.add(resourceFactory.newResource(f.toPath()));
// add lib/*.jar as jar files
resources.add(resourceFactory.newJarFileResource(f.toPath().toUri()));
}
}
}
}
resources.add(resourceFactory.newResource(file.toPath())); //TODO really???
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I doubt we need the directory that is the bundle installation directory as a Resource.

}
else
{
resources.add(resourceFactory.newResource(file.toPath()));
// Treat bundle as jar file that needs to be opened (so that resources within it can be found)
resources.add(resourceFactory.newJarFileResource(file.toPath().toUri()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we explicitly having to use JarFileResource?

}

return resources;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -137,7 +136,7 @@ public void findAndFilterContainerPaths(final WebAppContext context) throws Exce
UriPatternPredicate uriPatternPredicate = new UriPatternPredicate(pattern, false);
Consumer<URI> addContainerResource = (uri) ->
{
Resource resource = resourceFactory.newResource(uri);
Resource resource = newDirectoryResource(context, uri);
if (Resources.missing(resource))
{
if (LOG.isDebugEnabled())
Expand Down Expand Up @@ -180,20 +179,22 @@ public void findAndFilterContainerPaths(final WebAppContext context) throws Exce
.toList();
for (Path path: matchingBasePaths)
{
if (LOG.isDebugEnabled())
LOG.debug("Found jdk.module.path entry: {}", path);
if (Files.isDirectory(path))
{
try (Stream<Path> listing = Files.list(path))
{
for (Path listEntry: listing.toList())
{
Resource resource = resourceFactory.newResource(listEntry);
Resource resource = newDirectoryResource(context, listEntry);
context.getMetaData().addContainerResource(resource);
}
}
}
else
{
Resource resource = resourceFactory.newResource(path);
Resource resource = newDirectoryResource(context, path);
context.getMetaData().addContainerResource(resource);
}
}
Expand Down Expand Up @@ -297,20 +298,6 @@ protected void scanJars(WebAppContext context) throws Exception
scanJars(context, context.getMetaData().getWebInfResources(false), false, scanTypes);
}

/**
* For backwards compatibility. This method will always scan for all types of data.
*
* @param context the context for the scan
* @param jars the jars to scan
* @param useCaches if true, the scanned info is cached
* @throws Exception if unable to scan the jars
*/
public void scanJars(final WebAppContext context, Collection<Resource> jars, boolean useCaches)
throws Exception
{
scanJars(context, jars, useCaches, __allScanTypes);
}

/**
* Look into the jars to discover info in META-INF. If useCaches == true, then we will
* cache the info discovered indexed by the jar in which it was discovered: this speeds
Expand Down Expand Up @@ -398,17 +385,8 @@ else if (LOG.isDebugEnabled())
//not using caches or not in the cache so check for the resources dir
if (LOG.isDebugEnabled())
LOG.debug("{} META-INF/resources checked", target);
if (target.isDirectory())
{
//TODO think how to handle an unpacked jar file (eg for osgi)
resourcesDir = target.resolve("/META-INF/resources");
}
else
{
// Resource represents a packed jar
URI uri = target.getURI();
resourcesDir = resourceFactory.newResource(URIUtil.uriJarPrefix(uri, "!/META-INF/resources"));
}

resourcesDir = target.resolve("META-INF/resources");
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires target to be a directory, this should be asserted and javadoc'ed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You still need the if (target.isDirectory()) don't you?


if (Resources.isReadableDirectory(resourcesDir) && (cache != null))
{
Expand Down Expand Up @@ -472,15 +450,8 @@ else if (LOG.isDebugEnabled())
//not using caches or not in the cache so check for the web-fragment.xml
if (LOG.isDebugEnabled())
LOG.debug("{} META-INF/web-fragment.xml checked", jar);
if (jar.isDirectory())
{
webFrag = resourceFactory.newResource(jar.getPath().resolve("META-INF/web-fragment.xml"));
}
else
{
URI uri = jar.getURI();
webFrag = resourceFactory.newResource(URIUtil.uriJarPrefix(uri, "!/META-INF/web-fragment.xml"));
}

webFrag = jar.resolve("META-INF/web-fragment.xml");
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, don't you need to test first if it is a dir or a jar?


if (Resources.isReadable(webFrag) && (cache != null))
{
Expand Down Expand Up @@ -531,34 +502,29 @@ public void scanForTlds(WebAppContext context, Resource jar, ConcurrentHashMap<R
if (tmp.isEmpty())
{
if (LOG.isDebugEnabled())
LOG.debug("{} cached as containing no tlds", jar);
LOG.debug("{} cached as containing no TLDs", jar);
return;
}
else
{
tlds = tmp;
if (LOG.isDebugEnabled())
LOG.debug("{} tlds found in cache ", jar);
LOG.debug("{} found {} TLDs in cache: [{}]", jar, tlds.size(), tlds.stream().map(URL::toExternalForm).sorted().collect(Collectors.joining("\n ")));
}
}
else
{
//not using caches or not in the cache so find all tlds
tlds = new HashSet<>();
if (jar.isDirectory())
{
tlds.addAll(getTlds(jar.getPath()));
}
else
{
URI uri = jar.getURI();
tlds.addAll(getTlds(context, uri));
}
tlds.addAll(getTlds(jar));

if (LOG.isDebugEnabled())
LOG.debug("TLDs scan of {} - found {} : [{}]", jar, tlds.size(), tlds.stream().map(URL::toExternalForm).sorted().collect(Collectors.joining("\n ")));

if (cache != null)
{
if (LOG.isDebugEnabled())
LOG.debug("{} tld cache updated", jar);
LOG.debug("{} TLD cache updated with {} entries", jar, tlds.size());
Collection<URL> old = cache.putIfAbsent(jar, tlds);
if (old != null)
tlds = old;
Expand All @@ -576,7 +542,7 @@ public void scanForTlds(WebAppContext context, Resource jar, ConcurrentHashMap<R
}
metaInfTlds.addAll(tlds);
if (LOG.isDebugEnabled())
LOG.debug("tlds added to context");
LOG.debug("TLDs added to context: [{}]", metaInfTlds.stream().map(URL::toExternalForm).sorted().collect(Collectors.joining("\n ")));
}

@Override
Expand All @@ -592,55 +558,39 @@ public void postConfigure(WebAppContext context) throws Exception
/**
* Find all .tld files in all subdirs of the given dir.
*
* @param dir the directory to scan
* @param res the directory to scan
* @return the list of tlds found
* @throws IOException if unable to scan the directory
*/
private Collection<URL> getTlds(Path dir) throws IOException
private Collection<URL> getTlds(Resource res) throws IOException
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto EE9: you changed the logic, so we should check if it's valid, and if so, change the javadoc accordingly.

{
if (dir == null || !Files.isDirectory(dir))
if (!Resources.isReadableDirectory(res))
return Collections.emptySet();

Set<URL> tlds = new HashSet<>();
Resource metaInfDir = res.resolve("META-INF");
if (!Resources.isReadableDirectory(metaInfDir))
return Collections.emptySet();

try (Stream<Path> entries = Files.walk(dir)
.filter(Files::isRegularFile)
.filter(FileID::isTld))
Set<URI> tldURIs = new HashSet<>();
for (Resource entry: metaInfDir.list())
{
Iterator<Path> iter = entries.iterator();
while (iter.hasNext())
URI uri = entry.getURI();
if (FileID.isExtension(entry.getFileName(), "tld"))
{
Path entry = iter.next();
tlds.add(entry.toUri().toURL());
if (Resources.isReadableFile(entry))
tldURIs.add(uri);
else
LOG.warn("TLD not a readable: {}", uri);
}
}
return tlds;
}

/**
* Find all .tld files in the given jar.
*
* @param uri the uri to jar file
* @return the collection of tlds as url references
* @throws IOException if unable to scan the jar file
*/
private Collection<URL> getTlds(WebAppContext context, URI uri) throws IOException
{
HashSet<URL> tlds = new HashSet<>();
Resource r = ResourceFactory.of(context).newResource(URIUtil.uriJarPrefix(uri, "!/"));
try (Stream<Path> stream = Files.walk(r.getPath()))
{
Iterator<Path> it = stream
.filter(Files::isRegularFile)
.filter(FileID::isTld)
.iterator();
while (it.hasNext())
{
Path entry = it.next();
tlds.add(entry.toUri().toURL());
}
}
return tlds;
// URL hashCode() and equals() calls on java.net.URL objects and calls that add URL objects to maps and sets.
// URL's equals() and hashCode() methods can perform a DNS lookup to resolve the host name.
// This is why we collect the Set as a URI, then convert to List to satisfy the collection of unique URLs
List<URL> tldURLs = new ArrayList<>();
for (URI uri: tldURIs)
tldURLs.add(uri.toURL()); // done this way to allow toURL to throw if need be
return tldURLs;
}

protected List<Resource> findClassDirs(WebAppContext context)
Expand Down Expand Up @@ -699,6 +649,7 @@ protected List<Resource> findWebInfLibJars(WebAppContext context)
{
return webInfLib.list().stream()
.filter((lib) -> FileID.isLibArchive(lib.getFileName()))
.map(r -> toDirectoryResource(context, r))
.sorted(ResourceCollators.byName(true))
.collect(Collectors.toList());
}
Expand All @@ -722,6 +673,7 @@ protected List<Resource> findExtraClasspathJars(WebAppContext context)
return context.getExtraClasspath()
.stream()
.filter(this::isFileSupported)
.map(r -> toDirectoryResource(context, r))
.collect(Collectors.toList());
}

Expand Down Expand Up @@ -767,6 +719,35 @@ protected List<Resource> findExtraClasspathDirs(WebAppContext context)
.collect(Collectors.toList());
}

private Resource newDirectoryResource(WebAppContext context, Path path)
{
if (path == null)
return null;
return newDirectoryResource(context, path.toUri());
}

private Resource newDirectoryResource(WebAppContext context, URI uri)
{
if (FileID.isJavaArchive(uri) &&
!"jar".equals(uri.getScheme()))
{
return context.getResourceFactory().newJarFileResource(uri);
}

return context.getResourceFactory().newResource(uri);
}

private Resource toDirectoryResource(WebAppContext context, Resource resource)
{
if (Resources.isReadable(resource) &&
FileID.isJavaArchive(resource.getURI()) &&
!"jar".equals(resource.getURI().getScheme()))
{
return context.getResourceFactory().newJarFileResource(resource.getURI());
Copy link
Contributor

Choose a reason for hiding this comment

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

You're only using the context to get the ResourceFactory, so you should pass the ResourceFactory as a parameter instead of the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to do this? Why can't we just trust that the Resource will be the right type of resource, and that it will open the jar when and if it needs to? Why does the user of the Resource suddenly have to be aware of the internal implementation of Resource and JarFileResource?

}
return resource;
}

private boolean isFileSupported(Resource resource)
{
return FileID.isLibArchive(resource.getURI());
Expand Down
Loading