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

Jump to bottom

Issue #12453 - Allow AnnotationParser to work with Resources types that do not support Path #12454

Draft
wants to merge 2 commits into
base: jetty-12.0.x
Choose a base branch
from
Draft

Issue #12453 - Allow AnnotationParser to work with Resources types that do not support Path #12454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1905e6
Issue #12453 - AnnotationParser NPE protection
joakime Oct 31, 2024
31606ab
Address change review comments
joakime Nov 1, 2024
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
33 changes: 31 additions & 2 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FileID.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Locale;
import java.util.Objects;
import java.util.regex.Pattern;

/**
Expand All @@ -38,6 +39,7 @@ public class FileID
*/
public static String getBasename(Path path)
{
Objects.requireNonNull(path);
Path filename = path.getFileName();
if (filename == null)
return "";
Expand Down Expand Up @@ -366,11 +368,27 @@ public static boolean isLibArchive(URI uri)
*/
public static boolean isClassFile(Path path)
{
if (path == null)
return false;

Path fileNamePath = path.getFileName();
if (fileNamePath == null)
return false;

String filename = fileNamePath.toString();

return isClassFile(fileNamePath.toString());
}

/**
* Predicate to test for class files
*
* @param filename the filename to test
* @return true if the filename ends with {@code .class}
*/
public static boolean isClassFile(String filename)
{
if (StringUtil.isBlank(filename))
return false;

// has to end in ".class"
if (!StringUtil.asciiEndsWithIgnoreCase(filename, ".class"))
return false;
Expand Down Expand Up @@ -404,6 +422,8 @@ public static boolean isClassFile(Path path)
*/
public static boolean isHidden(Path path)
{
Objects.requireNonNull(path);

int count = path.getNameCount();
for (int i = 0; i < count; i++)
{
Expand Down Expand Up @@ -443,6 +463,9 @@ public static boolean isHidden(Path path)
*/
public static boolean isHidden(Path base, Path path)
{
Objects.requireNonNull(base);
Objects.requireNonNull(path);

// Work with the path in relative form, from the base onwards to the path
return isHidden(base.relativize(path));
}
Expand Down Expand Up @@ -492,6 +515,9 @@ public static boolean isJavaArchive(String filename)
*/
public static boolean isMetaInfVersions(Path path)
{
if (path == null)
return false;

if (path.getNameCount() < 3)
return false;

Expand Down Expand Up @@ -531,6 +557,9 @@ public static boolean isNotMetaInfVersions(Path path)
*/
public static boolean isModuleInfoClass(Path path)
{
if (path == null)
return false;

Path filenameSegment = path.getFileName();
if (filenameSegment == null)
return false;
Expand Down
108 changes: 84 additions & 24 deletions ...y-ee10-annotations/src/main/java/org/eclipse/jetty/ee10/annotations/AnnotationParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,18 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

import org.eclipse.jetty.io.IOResources;
import org.eclipse.jetty.util.ExceptionUtil;
import org.eclipse.jetty.util.FileID;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.resource.Resource;
import org.eclipse.jetty.util.resource.ResourceFactory;
import org.eclipse.jetty.util.resource.Resources;
import org.objectweb.asm.AnnotationVisitor;
import org.objectweb.asm.ClassReader;
import org.objectweb.asm.ClassVisitor;
Expand Down Expand Up @@ -550,21 +553,23 @@ public void parse(final Set<? extends Handler> handlers, Resource r) throws Exce
if (!r.exists())
return;

if (FileID.isJavaArchive(r.getPath()))
{
parseJar(handlers, r);
return;
}

if (r.isDirectory())
{
parseDir(handlers, r);
return;
}

if (FileID.isClassFile(r.getPath()))
else
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

@joakime joakime Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See previous

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to change the established ordering: the change you've made just replaces r.getPath() with r.getFileName()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's far more subtle then that.

Path has knowledge of things like .isFile and .isRegularFile and .isReadable.
String (from r.getFileName) does not, it's just a String.

The original test FileID.isClassFile(Path) doesn't only check that the last Path segment text filename is a *.class, but also checks if the provided Path is a file, not allowing directories with the name *.class to pass as true.

So, for example, the following change would need to be made at a minimum to maintain backward compatibility to the original code.

- if (FileID.isClassFile(r.getPath()))
+ if (Resoures.isReadableFile(r) && FileID.isClassFile(r.getFileName()))

That would need to occur in at least two places.
Once you see that code, you'll quickly understand the order change.

{
parseClass(handlers, null, r.getPath());
if (FileID.isJavaArchive(r.getFileName()))
{
parseJar(handlers, r);
return;
}

if (FileID.isClassFile(r.getFileName()))
{
parseClass(handlers, null, r);
}
}

if (LOG.isDebugEnabled())
Expand All @@ -580,15 +585,23 @@ public void parse(final Set<? extends Handler> handlers, Resource r) throws Exce
*/
protected void parseDir(Set<? extends Handler> handlers, Resource dirResource) throws Exception
{
Objects.requireNonNull(handlers);
Objects.requireNonNull(dirResource);

if (LOG.isDebugEnabled())
LOG.debug("Scanning dir {}", dirResource);

assert dirResource.isDirectory();
if (!Resources.isDirectory(dirResource))
return;

ExceptionUtil.MultiException multiException = new ExceptionUtil.MultiException();

for (Resource candidate : dirResource.getAllResources())
{
// Skip ones that don't exist
if (!Resources.exists(candidate))
continue;

// Skip directories
if (candidate.isDirectory())
continue;
Expand All @@ -606,7 +619,7 @@ protected void parseDir(Set<? extends Handler> handlers, Resource dirResource) t

try
{
parseClass(handlers, dirResource, candidate.getPath());
parseClass(handlers, dirResource, candidate);
}
catch (Exception ex)
{
Expand All @@ -626,15 +639,14 @@ protected void parseDir(Set<? extends Handler> handlers, Resource dirResource) t
*/
protected void parseJar(Set<? extends Handler> handlers, Resource jarResource) throws Exception
{
if (jarResource == null)
return;

/* if (!FileID.isJavaArchive(jarResource.getPath()))
return;*/
Objects.requireNonNull(jarResource);

if (LOG.isDebugEnabled())
LOG.debug("Scanning jar {}", jarResource);

if (!Resources.isReadableFile(jarResource))
return;

try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
Resource insideJarResource = resourceFactory.newJarFileResource(jarResource.getURI());
Expand All @@ -649,27 +661,75 @@ protected void parseJar(Set<? extends Handler> handlers, Resource jarResource) t
* @param containingResource the dir or jar that the class is contained within, can be null if not known
* @param classFile the class file to parse
* @throws IOException if unable to parse
* @deprecated use {@link #parseClass(Set, Resource, Resource)} instead (which uses {@link Resource} instead of {@link Path})
*/
@Deprecated(since = "12.0.16", forRemoval = true)
protected void parseClass(Set<? extends Handler> handlers, Resource containingResource, Path classFile) throws IOException
{
if (LOG.isDebugEnabled())
LOG.debug("Parse class from {}", classFile.toUri());
Objects.requireNonNull(classFile);

if (!(Files.isRegularFile(classFile) && Files.isReadable(classFile)))
return;

try (InputStream inputStream = Files.newInputStream(classFile))
joakime marked this conversation as resolved.
Show resolved Hide resolved
{
parseClass(handlers, containingResource, classFile.toUri(), inputStream);
}
}

/**
* Use ASM on a class
*
* @param handlers the handlers to look for classes in
* @param containingResource the dir or jar that the class is contained within, can be null if not known
* @param classFile the class file to parse
* @throws IOException if unable to parse
*/
protected void parseClass(Set<? extends Handler> handlers, Resource containingResource, Resource classFile) throws IOException
{
Objects.requireNonNull(classFile);

if (!Resources.isReadableFile(classFile))
return;

try (InputStream inputStream = IOResources.asInputStream(classFile))
joakime marked this conversation as resolved.
Show resolved Hide resolved
{
parseClass(handlers, containingResource, classFile.getURI(), inputStream);
}
}

URI location = classFile.toUri();
/**
* Use ASM on a class
*
* @param handlers the handlers to look for classes in
* @param containingResource the dir or jar that the class is contained within, can be null if not known
* @param classFileRef the URI reference to the classfile location
* @param inputStream the class file contents to parse
* @throws IOException if unable to parse
*/
private void parseClass(Set<? extends Handler> handlers, Resource containingResource, URI classFileRef, InputStream inputStream) throws IOException
{
Objects.requireNonNull(handlers);
Objects.requireNonNull(containingResource);
Objects.requireNonNull(classFileRef);
Objects.requireNonNull(inputStream);

try (InputStream in = Files.newInputStream(classFile))
if (LOG.isDebugEnabled())
joakime marked this conversation as resolved.
Show resolved Hide resolved
LOG.debug("Parse class from {}", classFileRef);

try
{
ClassReader reader = new ClassReader(in);
ClassReader reader = new ClassReader(inputStream);
reader.accept(new MyClassVisitor(handlers, containingResource, _asmVersion), ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES);

String classname = normalize(reader.getClassName());
URI existing = _parsedClassNames.putIfAbsent(classname, location);
URI existing = _parsedClassNames.putIfAbsent(classname, classFileRef);
if (existing != null)
LOG.warn("{} scanned from multiple locations: {}, {}", classname, existing, location);
LOG.warn("{} scanned from multiple locations: {}, {}", classname, existing, classFileRef);
}
catch (IllegalArgumentException | IOException e)
{
throw new IOException("Unable to parse class: " + classFile.toUri(), e);
throw new IOException("Unable to parse class: " + classFileRef, e);
}
}

Expand Down
26 changes: 14 additions & 12 deletions ...e10-osgi-boot/src/main/java/org/eclipse/jetty/ee10/osgi/annotations/AnnotationParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ protected Bundle getBundle(Resource resource)
public void parse(Set<? extends Handler> handlers, Bundle bundle)
throws Exception
{

Resource bundleResource = _bundleToResource.get(bundle);
if (bundleResource == null)
return;
Expand All @@ -109,25 +108,28 @@ public void parse(final Set<? extends Handler> handlers, Resource r) throws Exce
if (!r.exists())
return;

if (FileID.isJavaArchive(r.getPath()))
{
parseJar(handlers, r);
return;
}

if (r.isDirectory())
{
parseDir(handlers, r);
return;
}

if (FileID.isClassFile(r.getPath()))
else
{
parseClass(handlers, null, r.getPath());
if (FileID.isJavaArchive(r.getFileName()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change in order?

Copy link
Contributor Author

@joakime joakime Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To make the conditions easier.

Keeping the order it would have been ...

        if (!r.isDirectory() && FileID.isJavaArchive(r.getFileName()))
        {
            parseJar(handlers, r);
            return;
        }

        if (r.isDirectory())
        {
            parseDir(handlers, r);
            return;
        }

        if (!r.isDirectory() && FileID.isClassFile(r.getFileName()))
        {
            parseClass(handlers, null, r.getPath());
        }

Which reads really weird.
So, it was changed to be anything directory related do this, anything else (a file), do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the point of this PR is just to add null protection, so I don't see how a change in the order of the way we consider what to do with a given resource is related. I don't like changing the well-established order of things unless there is an associated bug.

Copy link
Contributor Author

@joakime joakime Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The NPE protection is just part of the change.
The change also moves away from Path to Resource, so that the example of URLResource has an opportunity to work.
That change means we also change this section.
The new parseClass() that uses Resource is another example of this change.
Let me change the title of this PR, the description still has the correct details.

{
parseJar(handlers, r);
return;
}

if (FileID.isClassFile(r.getFileName()))
{
parseClass(handlers, null, r);
return;
}
}

//Not already parsed, it could be a file that actually is compressed but does not have
//.jar/.zip etc extension, such as equinox urls, so try to parse it
// Not already parsed, it could be a file that actually is compressed but does not have
// .jar/.zip etc extension, such as equinox urls, so try to parse it
try
{
parseJar(handlers, r);
Expand Down
Loading
Loading