fix: Classes should not be loaded from the plugin

[Artur-]

Related-to #19009

[sonarcloud]

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[github-actions]

Test Results

1 142 files  ± 0  1 142 suites  ±0   1h 22m 17s ⏱️ - 5m 10s
7 456 tests ± 0  7 406 ✅ ± 0  50 💤 ±0  0 ❌ ±0 
7 748 runs   - 78  7 688 ✅  - 78  60 💤 ±0  0 ❌ ±0 

Results for commit 4ba9429. ± Comparison against base commit b2993c2.

♻️ This comment has been updated with latest results.

[Artur-]

Does @mcollovati, @platosha or somebody else know why this should not be done? The current implementation finds Flow classes the plugin was built with and not from the project. No tests here fail, does Hilla rely on this somehow?

[mcollovati]

The only reason I can see against null parent is that the provided URLs might not cover all possible location for parent classes, so a specific class cannot be loaded because the classloader cannot find one ancestor.

Looking at the flow maven plugin code, it looks like we provide all runtime dependencies (compile + runtime scopes) but we filter out provided and system to only get portlet and servlet-api.
However, according to the documentation, the plugin class loader should not contain the project dependencies, so the execution may fail anyway for dependencies excluded from URL. Unless the same dependencies are defined as plugin dependencies.

I wonder if we should provide to ReflectionsClassFinder also a class loader, and create that one based on all MavenProject dependencies.
Or temporarily change the context class loader during mojo execution.

[mcollovati]

The Surefire plugin for in-process execution uses a custom class loader, temporarily set as the current thread context class loader. The classloader is built on project dependencies ( getProject().getArtifacts()).

        @Override
        public RunResult invoke(Object forkTestSet) throws ReporterException, InvocationTargetException {
            ClassLoader current = swapClassLoader(testsClassLoader);
            try {
                Method invoke = getMethod(providerInOtherClassLoader.getClass(), "invoke", INVOKE_PARAMETERS);
                Object result = invokeMethodWithArray2(providerInOtherClassLoader, invoke, forkTestSet);
                return (RunResult) surefireReflector.convertIfRunResult(result);
            } finally {
                if (System.getSecurityManager() == null) {
                    Thread.currentThread().setContextClassLoader(current);
                }
            }
        }

        private ClassLoader swapClassLoader(ClassLoader newClassLoader) {
            ClassLoader current = Thread.currentThread().getContextClassLoader();
            Thread.currentThread().setContextClassLoader(newClassLoader);
            return current;
        }

[Artur-]

and presumably the built test classloader does not use the plugin classloader as its parent?

[mcollovati]

If I got it correctly, it uses the platform classloader as parent.

https://github.com/apache/maven-surefire/blob/4281a9bdd41a5cc4f17f4ed101ee9e8a1a089aa5/surefire-booter/src/main/java/org/apache/maven/surefire/booter/Classpath.java#L120-L136

https://github.com/apache/maven-surefire/blob/master/surefire-booter/src/main/java/org/apache/maven/surefire/booter/IsolatedClassLoader.java

[Artur-]

Added the platform classloader as parent..

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[Artur-]

Should we merge this for 24.6 or try to figure out if it breaks something or is there some better approach?

[sonarcloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud