Add proper JBoss VFS support

[classgraph]

Add proper JBoss VFS support

[lukehutch]

Reflections works with JBoss too. Once proper JBoss support is in place, ping @Faliorn to test (see #43).

[vipcxj]

Reflections get the correct path elements. However these pathes are not real. So FastClasspathScanner lose them in next step.

[lukehutch]

Hi @vipcxj , thanks for the info, can you please elaborate a bit more?

[lukehutch]

Note to self -- see also, for JBoss 6 VFS support:

https://github.com/StripesFramework/stripes/blob/master/stripes/src/main/java/net/sourceforge/stripes/vfs/JBoss6VFS.java

See also the comment here: #90 (comment)

[lukehutch]

See also Reflections' VFS support:

https://github.com/ronmamo/reflections/blob/master/src/main/java/org/reflections/vfs/Vfs.java

[DavyDeDurpel]

I'm a bit confused. I see that there is already jboss-vfs support in the latest version of this library but it seems to be only searching for classes in the ejb jar containing the scanner logic. Is this correct? I would expect the scanner to look for classes inside all the jars defined in the root of the ear.
Is that something that you still plan to do?

In the mean time I will create a custom loader based on your JBossClassLoaderHandler and this example on how to find all the (ejb) jars inside the ear:

https://developer.jboss.org/thread/170388

[DavyDeDurpel]

It seems the order in which you load the handlers is not 'hacker' friendly ;-)
It's for instance currently not possible to provide a custom JBossClassLoaderHandler as it always picks the one included in the library.

Wouldn't it be a better default to put the manually added ClassLoaderHandlers as first in the list?

[lukehutch]

@DavyDeDurpel good point on the loading order, I should fix that. I am currently traveling, so for now, are you able to simply hack on the existing ClassLoaderHandler? It would be better to fix the existing code anyway.

Correct, the current JBoss VFS support right now is minimal and hacky. It would be great if you could please fix that. I don't know almost anything about JBoss, so would appreciate your help. Please see all the links in the other comments above to combine all of the other approaches into one.

I do know that JBoss classloading has undergone at least a couple of major revisions, and you want to make sure you handle as many recent major versions as possible. (The current code handles two different internal classloader versions, if I remember right.) One change that should be implemented is to call the actual VFS API(s), rather than probing the fields for values.

Also, I don't want to add any external dependencies, so you'll need to make use of the ReflectionUtils class to call the API using reflection.

[lukehutch]

@DavyDeDurpel I just pushed a change that calls the user-provided ClassLoaderHandlers before the defaults.

[DavyDeDurpel]

I tested your fix and it works. I was also able to get my version working. It now scans all jars inside the ear.
But my change is absolutely not production ready because:

• I only tested it on JBoss EAP 6.4
• I made the assumption that the scanner logic is executed from a jar in the root of the ear
• I'm not sure if there are any side effects

I think I already found a nicer solution (at least for EAP6/JBoss7) but I'll have to do some changes in ReflectionUtils because I need to call a method with 2 arguments which is currently not possible with ReflectionUtils.

I'll keep you posted on my progress... ( most of my progress will be realized after working hours ;-) )

[lukehutch]

It might be worth it to just add your code to the existing code, to catch a few more cases -- JBoss is complex, I'm sure we're not done yet, and the kitchen sink approach may not be a bad thing.

PS you don't have to use ReflectionUtils, it's just there to simplify life. Our you can add a method to it if you want.

[DavyDeDurpel]

I finally didn't have to change ReflectionUtils. My new solution seems already a lot better than the old one. The old one just scanned the directory of the ear for jar files.
The new solution uses the ServiceModuleLoader to find all child modules. This system should be more secure as the scanner won't be seeing bundles for which it doesn't have access rights.

This weekend I will create a test ear with 2 ejb jars, 1 war containing itself an ejb jar. The scanner should be able to detected those 4. I will also try to find the time to already test it on JBoss EAP 6.4, JBoss EAP 7.x, Wildfly 8, 9 and 10.

[lukehutch]

Awesome, thanks so much for your work on this!

[DavyDeDurpel]

The code isn't working (yet) for class files inside the WAR and (EJB) jars inside the WAR :-(
It will be a hard nut to crack this one...

[lukehutch]

FCS has the ability to decompress jars inside jars, as long as the URLs have the ! separator in them. Is the issue finding where the classpath of the inner jars is specified, or something?

[DavyDeDurpel]

I think I found out why it won't work on JBoss without having to rewrite ClasspathFinder.
In JBoss all J2EE modules have their own class loader. So each (EJB) jar has a class loader, the WAR has it's own classloader and also the EAR. The problem with ClasspathFinder is that it only takes the class loader of the jar in which the scanner logic is contained. If the jar is located inside the EAR, then it seems that it's classloader is also able to load classes from other jars from the root of the EAR. (I'm not 100% sure about that but that seemed to be the behavior after my small hack on JBossClassLoaderHandler).

However loading classes from inside the WAR always results in ClassNotFoundExeception (which is to be expected as the WAR has it's own classloader).

After debugging I realized that my hack is too late in the process. Before the JBossClassLoaderHandler is called some logic is executed to search for class loaders. And for each of these the JBossClassLoaderHandler gets called.

In ClasspathFinder starting from line 153 you build a collection of classloaders starting from the classloader of the current jar (or WAR if the scanner logic is inside WEB-INF/classes).

At this point on JBoss you have a ModuleClassLoader. This is the one you need to find all other classloaders used inside the ear. This is the logic for finding all other classloaders (the JBoss way):

if ("org.jboss.modules.ModuleClassLoader".equals(c.getName())) { Object module = ReflectionUtils.invokeMethod(classloader, "getModule"); Object serviceLoader = ReflectionUtils.invokeMethod(module, "getCallerModuleLoader"); @SuppressWarnings("unchecked") Map<Object, Object> moduleMap = (Map<Object, Object>) ReflectionUtils.getFieldVal(serviceLoader, "moduleMap"); for (Object key : moduleMap.keySet()) { Object futureModule = moduleMap.get(key); Object realModule = ReflectionUtils.invokeMethod(futureModule, "getModule"); // these are the class loaders you need Object moduleLoader = ReflectionUtils.invokeMethod(realModule, "getClassLoader"); } }

Sorry for the format of the code but I always have trouble getting it right on github.
Anyway, the above code should be executed before the handlers are called.
I guess you could add a second method to the handlers in which you pass a classloader and it returns a new list of classloaders. You could call that handler method when building the collection of classloaders.

Do you understand the issue? I think it will be a small change with only impact on ClasspathFinder and all the handlers.
Do you want me to test it out and make a pull request in case I got it working?

[lukehutch]

This makes sense, I think. However, as long as you can find all the JBoss classloaders given a reference to just the current context classloader, FCS doesn't need to know about the other ones -- you can do all the rest with reflection, as you indicate. You can just recursively fetch each classloader, then get its sub-classloaders, and get the URLs from each as you recurse down the jar tree. I don't think FCS needs to be modified for that to work? but you will need to set up a recursive function in the ClassLoaderHandler to recursively fetch the nested classloaders.

[DavyDeDurpel]

The ClassLoaderHandler only calls classpathFinder.addClasspathElement(path, log) to add class paths from inside the jars, WAR, EAR... A Handler is not able to add the necessary ClassLoader to load the classes in these paths.
But after the ClassLoaderHandlers have been processed, somewhere (I can't remember the exact location and I already closed my Eclipse) a Class.forName(...) is done to load the classes that were found. The problem is that at that point only the ClassLoaders that were available in ClasspathFinder are used and this is what's causing the ClassNotFoundExeception.

The only real solution I see is that the Handlers are in some way able to help build the list of ClassLoaders.

[lukehutch]

@DavyDeDurpel I just pushed a change that allows ClassLoaders to be specified when classpath elements are added. This breaks the ABI for ClasspathFinder#addClasspathElement(), but you're right that this is needed. Hopefully this does what you need; please let me know.

[lukehutch]

@DavyDeDurpel I released the patch (and the ABI change) in v2.2.0. Please let me know if this is sufficient for what you need.

[DavyDeDurpel]

I tested out the ABI change and it solves the ClassNotFoundException.
We're not there yet as for 2 of my test classes I do not get the callback. I will try to find out this weekend (or beginning next week as I have a busy weekend) where and why it goes wrong.

What I already know for sure is that the annotations on these classes are well detected so the JBossClassLoaderHandler does it's job.

[lukehutch]

@DavyDeDurpel great, thanks for testing. Calling .verbose() before .scan() may help you with debugging that last issue. I improved verbose logging a bit in a recent release.

[lukehutch]

@DavyDeDurpel feel free to post logs here, if you need help debugging the last issue. I'm going to push out a new release soon, and would love to include your ClassLoaderHandler fixes.

[DavyDeDurpel]

Sorry, I've been busy renewing the pavement at home so I haven't had the time yet to investigate the issue(s). I already tried debugging your code but it's not easy to do it.

This is one of the errors:

FastClasspathScanner ---- Class be.war.test.TestWarClass is at incorrect relative path WEB-INF/classes/be/war/test/TestWarClass.class -- ignoring

So it seems that somewhere a 'WEB-INF/classes' is added to the qualified name and not stripped of.
Does that ring a bell to you?

[lukehutch]

@DavyDeDurpel that's actually very interesting -- it could imply that there is a custom classloader that expects classes at a fixed path prefix of WEB-INF/classes, for example. When you have time (no rush, take care of your paving first), could you please attach a minimal repro case to this bug report?

[DavyDeDurpel]

I found the cause by writing this reply! I noticed that I had put the annotations on the 2 undetected classes on the constructor of these classes. Apparently the scanner does not do a callback for annotations on the constructor. Is that normal behavior or might it be a bug? (I'm wondering if anyone would even put annotations on a constructor?) Anyway I moved the annotations to normal methods and now the callbacks are done for all 3 of my test classes. This also means that the WEB-INF/classes error logged in verbose mode does not impact the scanning result.

Anyway, I have uploaded my test ear so you can play around yourself.

test-scanner-ear.ear.zip

This is a very simple ear. It contains 1 WAR and 2 EJB jars. One of the EJB jars is located directly under the EAR and the other one is located inside the WEB-INF/lib folder of the WAR.
This is what is currently supported for EJB's.

I use the scanner with callbacks for the @deprecated annotation. Normally I should get a callback for 3 classes/methods but I only get 1 for the moment (like told above, the reason is that the others have the annotation on the constructor).

test-ejb-ear/be.test.ear.TestScanner.java contains the scan logic.

The classes that should (at least) be detected are:

test-scanner-ear/test-ejb-ear/be.test.ear.TestEar -> method
test-scanner-ear/test-scanner-war/be.war.test.TestWarClass -> method
test-scanner-ear/test-scanner-war/WEB-INF/lib/test-ejb-war/be.test.TestWarEjb -> class

The paving itself was finished yesterday evening. Today I only need to install the garden furniture and have a glass of cold Belgian beer to celebrate the new pavement :-)

This evening I will start testing my changes on other JBoss/Wildfly versions.

[DavyDeDurpel]

I have also added the sources in the ear. So you have my version of JBossClassLoaderHandler.java
It's located in test-scanner-ear/test-ejb-ear

[DavyDeDurpel]

I finished testing my example ear on these 5 variants:

JBoss EAP 6.4.Final (JDK 8)
Jboss 7.1.Final (JDK 7)
Wildfly 8.2.1.Final (JDK 8)
Wildfly 9.0.2.Final (JDK 8)
Wildfly 10.1.0.Final (JDK 8)

All of them produce the same result so I think the changes to JBossClassLoaderHandler can be merged. Do you want me to make a pull request or are you going to do the merge yourself?

[lukehutch]

@DavyDeDurpel Thanks for testing! Please send a PR over. Really appreciate your work on this.

I'm looking at the constructor annotation issue now. It's possible annotations are not retained the same way for constructors or something.

[lukehutch]

@DavyDeDurpel I found the problem -- Class#getDeclaredMethods() returns Method[], containing only non-constructor methods. Constructors are obtained by calling Class#getDeclaredConstructors(), returning Constructor<?>[]. However, Constructor<?> cannot be cast to Method, so this breaks the FCS API! Constructors and methods cannot be handled by the same MethodAnnotationMatchProcessor. This pretty much sucks... I'm not sure of the best way to handle this yet.

[lukehutch]

I guess the right thing to do is modify the FCS API to return Executable, which is the supertype of Method and Constructor...

[lukehutch]

@DavyDeDurpel This should be fixed in 2.4.0 (just released). I also pulled in your code changes, so there's no need to send me a PR. Thanks for all your work on this!

[lukecapizano]

I know you have probably already made a decision, but this sounds like a good case for a ConstructorAnnotationMatchProcessor. I haven't looked at your code much, so I don't know what your proposed solution entails, but since a Construtor is not a Method, it makes sense that the MethodAnnotationMatchProcessor wouldn't match on constructors.

[lukehutch]

@lukecapizano Constructor and Method share a superclass, Executable, so I opted to change MethodAnnotationMatchProcessor to take a parameter of type Executable, and bumped the API version number. I don't think it is common knowledge that constructors are treated entirely differently than regular methods by the reflection API (at least I never knew that before!). Is it reasonable to handle both method types using a single MatchProcessor type, do you think?

[lukehutch]

Sorry, I just realized my comment contradicts what you wrote -- I guess the question comes down to (1) convenience and (2) common expectation -- would people be more or less inconvenienced or surprised by having two different MatchProcessor types? I think having only one is cleaner, and more in line with common expectations of common treatment of methods and constructors (at least both @DaveDeDurpel and I had assumed both methods and constructors should be treated the same way, from the point of view of annotation processing), but I'm willing to be convinced otherwise.

[lukecapizano]

My personal expectation was that it would not match on constructors, but maybe that is because I have some experience with reflection and already knew about the difference between Method and Constructor. I didn't know that they share a superclass, though!

Even still, I agree with you (and by extension @DavyDeDurpel). Having the MethodAnnotationMatchProcessor being able to detect annotations on both constructors and methods is intuitive enough for, and probably expected by, the majority of developers and is likely the best way forward.

I was just throwing some ideas out there in the event you had to decide between making a breaking change, and looking for an alternative solution. It sounds like this change doesn't affect the public api at all, in which case I suppose I am just making noise over here. :)

[lukehutch]

@lukecapizano no, it was a good suggestion, and I did in fact have to break the API to raise the parameter type from Method to Executable, hence the version bump to 2.4.x, although this API change shouldn't affect too many users. Having to test the subtype of Executable and cast the result is not ideal, but I think there was no ideal option here.

[DavyDeDurpel]

@lukecapizano might have a good point. It's true that now the name of the method call (.matchClassesWithMethodAnnotation(...) and the callback (MethodAnnotationMatchProcessor) both still refer to 'method' while now it's also used for 'constructor' matching.

I think we need to look at it from the perspective from a typical user. This library will probably only be used by experienced users. Most likely by library builders as it really shines in that domain. So I guess that the current implementation might confuse the more experienced Java Developer.

Keeping only 1 callback would somewhat imply that ...WithMethodAnnotation... should become ...WithExecutableAnnotation... and MethodAnnotationMatchProcessor should become ExecutableAnnotationMatchProcessor.
But this will probably confuse people more as I don't expect a lot of experienced Java Developers to have ever heard of Executable. I also learned about it only very recently ;-)

So I think that the solution proposed by @lukecapizano might be the cleanest of all possible solutions.

[lukehutch]

@DavyDeDurpel I went through the same thoughts in my head about the naming! I have to believe though that a larger percentage of users will be surprised that a constructor is "not a method" (not just "is a special type of method") than would be surprised that constructors are not treated differently. Executable is extremely obscure (I didn't know about it either until I looked up the supertype of the two classes). Going for the principle of least surprise, I think having a single MatchProcessor (and requiring people to look up the JavaDoc to understand what the heck an Executable is) is the simplest option. Otherwise I'm sure others in future will wonder why method annotations don't work on constructors. I can change it back though if there's a strong consensus that the latest change (in 2.4.0) wasn't the right one.

[DavyDeDurpel]

Any which solution is fine for me. I'm already very happy that it just works :-)

You could even put everything together and by so reduce the number of methods on FastClassPathScanner ;-)

fastClassPathScanner.matchWithAnnotation(MyAnnotation.class, new AnnotationMatchProcessor() {
@OverRide
public void processClassMatch(Class<?> matchingClass) { }

@OverRide
public void processMethodMatch(Method matchingMethod) { }

@OverRide
public void processConstructorMatch(Constructor matchingContructor) { }

@OverRide
public void processFieldMatch(Field matchingField) { }
});

[lukehutch]

I am looking at ways to dramatically simplify the API in the next major version, yes. But I do want to try to have only one method per interface, so that lambdas can be used.

[lukehutch]

Davy -- I'm still interested in this warning message you saw in the logs: FastClasspathScanner ---- Class be.war.test.TestWarClass is at incorrect relative path WEB-INF/classes/be/war/test/TestWarClass.class -- ignoring Is this still present in the latest version of FCS, with your ClassLoaderHandler? Or did your latest changes fix this? (What this says is that the classpath element WEB-INF/classes within the jar was not being obtained from the ClassLoader.)

…

On Tue, Jun 27, 2017 at 1:50 AM, Davy De Durpel ***@***.***> wrote: Sorry, I've been busy renewing the pavement at home so I haven't had the time yet to investigate the issue(s). I already tried debugging your code but it's not easy to do it. This is one of the errors: FastClasspathScanner ---- Class be.war.test.TestWarClass is at incorrect relative path WEB-INF/classes/be/war/test/TestWarClass.class -- ignoring So it seems that somewhere a 'WEB-INF/classes' is added to the qualified name and not stripped of. Does that ring a bell to you? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#37 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxhKfGVf7Jb4akpvNEFH6Grjs3J2dHgks5sIMJTgaJpZM4G57Xl> .

[DavyDeDurpel]

The error is still there but there's no impact.
This class is still detected and annotations on it are found.

Below the error the following is logged:

---- Found class be.war.test.TestWarClass
------- Superclass: java.lang.Object
------- Method annotations: java.lang.Deprecated

[lukehutch]

@DavyDeDurpel I see. That means the classloader provides both the root of the classpath element and WEB-INF/classes as classpath elements.

This brings up an important point though -- the entire class hierarchy is scanned twice in this case (once at the correct root, and once from two directories above that). Probably I should stop recursively scanning one classpath element at any point where the root of another classpath element is found. I'll open a bug for that.

[lukehutch]

As a heads-up, there is much improved support for JBoss' ModuleClassLoader in the latest release (4.6.6) of ClassGraph (formerly FastClasspathScanner).