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 #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

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 31, 2024

  • Improve null handling in FileID.
  • Improve AnnotationParser.parse() method to better utilize Resource object, don't rely on Path existing for things that don't require Path to function.

+ Better utilize Resource object, don't rely
  on Path existing for things that don't
  require Path.
@joakime joakime added the Bug For general bugs on Jetty side label Oct 31, 2024
@joakime joakime self-assigned this Oct 31, 2024
@joakime
Copy link
Contributor Author

joakime commented Oct 31, 2024

@janbartel this is WIP at the moment, as we haven't gotten details of the OPs setup to replicate the scenario in a test case.

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

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()))
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

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

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.

@joakime joakime changed the title Issue #12453 - AnnotationParser NPE protection Issue #12453 - Allow AnnotationParser to work with Resources types that do support Path Nov 1, 2024
@joakime joakime changed the title Issue #12453 - Allow AnnotationParser to work with Resources types that do support Path Issue #12453 - Allow AnnotationParser to work with Resources types that do not support Path Nov 1, 2024
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.

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

Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

The other way to fix this is to simply make FileID support arguments of type Resource, preserving the same behaviour that AnnotationParser uses in its method calls with Path parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Guard against NullPointerException in AnnotationParser
2 participants