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 - MetaInfConfiguration Overhaul #10816

Closed
wants to merge 12 commits into from
Closed
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
//
// ========================================================================
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.util;

import java.util.Collection;
import java.util.Collections;
import java.util.Map;
import java.util.Set;

/**
* A Map Implementation that never has entries.
*
* @param <K> the key type
* @param <V> the value type
*/
public class NullMap<K, V> implements Map<K, V>
joakime marked this conversation as resolved.
Show resolved Hide resolved
{
@Override
public int size()
{
// always empty
return 0;
}

@Override
public boolean isEmpty()
{
// always empty
return true;
}

@Override
public boolean containsKey(Object key)
{
// always false
return false;
}

@Override
public boolean containsValue(Object value)
{
// always false
return false;
}

@Override
public V get(Object key)
{
// always null
return null;
}

@Override
public V put(K key, V value)
{
// no-op
return null;
}

@Override
public V remove(Object key)
{
return null;
}

@Override
public void putAll(Map<? extends K, ? extends V> m)
{
// no-op
}

@Override
public void clear()
{
// no-op
}

@Override
public Set<K> keySet()
{
return Collections.emptySet();
}

@Override
public Collection<V> values()
{
return Collections.emptySet();
}

@Override
public Set<Entry<K, V>> entrySet()
{
return Collections.emptySet();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import java.io.File;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URL;
import java.net.URLClassLoader;
Expand Down Expand Up @@ -1841,6 +1842,28 @@ public static URI toURI(String resource)
: Paths.get(resource).toUri();
}

/**
* <p>Convert a URI to a URL without checked exceptions</p>
*
* @param uri the URI to convert from
* @return The {@link URL} of the URI
* @throws RuntimeException if unable to convert the URI to URL
* @see URI#toURL()
*/
public static URL toURL(URI uri)
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 motivation for changing the checked exception into a runtime exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it can be used in a stream

{
Objects.requireNonNull(uri);

try
{
return uri.toURL();
}
catch (MalformedURLException e)
{
throw new RuntimeException(e);
}
}

/**
* <p>
* Unwrap a URI to expose its container path reference.
Expand Down
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment below.

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()))
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 understand this change. We're checking here if the resource that contains the SCI is the webInfJar that we're looking at. Makes no sense to check containment.

nonExcludedInitializers.add(entry.getKey());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,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
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 understand the TODO comment. FileID just checks if the path ends in ".jar", so this should be fine?

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 resource being passed in is never a raw path to a jar file in this method.
Even if the underlying resource was a JAR, by the time it reaches here the Resource a mounted JAR already, meaning r.getPath() is just a directory inside the JAR.

This entire method parse(handlers , r) is basically a no-op. nothing happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean that after these changes `parse(handlers, r) is a no-op, because it certainly isn't now.

{
parseJar(handlers, r);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,15 @@ public Class<? extends Configuration> replaces()
}

/**
* Get the jars to examine from the files from which we have
* Get the webapp paths (jars &amp; dirs) to examine from the files from which we have
* synthesized the classpath. Note that the classpath is not
* set at this point, so we cannot get them from the classpath.
*
* @param context the web app context
* @return the list of jars found
* @return the list of webapp resources found
*/
@Override
protected List<Resource> findJars(WebAppContext context)
throws Exception
protected List<Resource> getWebAppPaths(WebAppContext context) throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that getWebAppPaths is any better than findJars. The method returns Resource not Path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the name can be changed, but its basically taking a WebAppContext and returning a List of Resource objects that all point to directories (either in the raw, or mounted)

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmmm, not sure that characterisation is correct - I'm not at all convinced that we should be changing file:/some/where/something.jar into jar:file:/some/where/something.jar!/.

{
List<Resource> list = new ArrayList<>();
MavenWebAppContext jwac = (MavenWebAppContext)context;
Expand All @@ -77,7 +76,7 @@ protected List<Resource> findJars(WebAppContext context)
});
}

List<Resource> superList = super.findJars(context);
List<Resource> superList = super.getWebAppPaths(context);
if (superList != null)
list.addAll(superList);
return list;
Expand All @@ -87,7 +86,7 @@ protected List<Resource> findJars(WebAppContext context)
* Add in the classes dirs from test/classes and target/classes
*/
@Override
protected List<Resource> findClassDirs(WebAppContext context) throws Exception
protected List<Resource> findClassesDirs(WebAppContext context) throws Exception
{
List<Resource> list = new ArrayList<>();

Expand All @@ -113,7 +112,7 @@ protected List<Resource> findClassDirs(WebAppContext context) throws Exception
});
}

List<Resource> classesDirs = super.findClassDirs(context);
List<Resource> classesDirs = super.findClassesDirs(context);
if (classesDirs != null)
list.addAll(classesDirs);
return list;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.eclipse.jetty.ee10.webapp.WebAppClassLoader;
import org.eclipse.jetty.ee10.webapp.WebAppContext;
import org.eclipse.jetty.ee10.webapp.WebInfConfiguration;
import org.eclipse.jetty.util.URIUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -59,7 +60,28 @@ public void configure(WebAppContext context) throws Exception
LOG.debug("Setting up classpath ...");
for (URI uri : jwac.getClassPathUris())
{
loader.addClassPath(uri.toASCIIString());
// Not all Resource types supported by Jetty can be supported by WebAppClassLoader
Copy link
Contributor

Choose a reason for hiding this comment

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

Motivation for this change? There haven't been any reported issues with the jetty maven plugin AFAIK.

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 builds a URLClassLoader, which is limited in ability to only working with URLs.
No point putting everything and the kitchen sink supported by Jetty into this.
We are encountering more and more java FileSystem support in Maven, this moves the error from being relatively useless URL errors from URLClassLoader when it cannot open the instance, to reporting that it's unsupported by the Maven Plugin.

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 fact, this change was directly motivated by folks using jar:file://// urls here.
Something that the URLClassLoader doesn't support.

String scheme = uri.getScheme();
if (scheme == null || scheme.equals("file"))
{
// no scheme? or "file" scheme, assume it is just a path.
loader.addClassPath(uri.getPath());
continue;
}

if (scheme.equals("jar"))
{
URI container = URIUtil.unwrapContainer(uri);
if (container.getScheme().equals("file"))
{
// Just add a reference to the
loader.addClassPath(container.getPath());
continue;
}
}
Comment on lines +72 to +81
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this converting things like "jar:file:/usr/jetty/lib/somedependency.jar!/" back to just "/usr/jetty/lib/somedependency.jar"?

Are we to assume that we will never get anything like: "jar:file:/usr/jetty/lib/somedependency.jar!/WEB-INF/classes" or "jar:file:/usr/jetty/lib/somedependency.jar!/WEB-INF/lib/some.jar" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@janbartel Can you give us a list of all the different uris that this method might see and what it is meant to do with each of them? i.e. what is "the brief" of this method?


// Anything else is a warning
LOG.warn("Skipping unsupported URI on ClassPath: {}", uri);
}
}

Expand Down
Loading