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 25m 54s ⏱️ + 1m 1s
7 464 tests ± 0  7 414 ✅ + 1  50 💤 ±0  0 ❌ ±0 
7 771 runs   - 62  7 713 ✅  - 59  58 💤  - 2  0 ❌ ±0 

Results for commit 83c5cf5. ± Comparison against base commit 1e3308d.

♻️ 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

[mcollovati]

Building a hybrid application with this change results in an error

[ERROR] Failed to execute goal com.vaadin:vaadin-maven-plugin:24.6-SNAPSHOT:prepare-frontend (default) on project spring-petclinic-vaadin-flow: Could not execute prepare-frontend goal.: Error occured during goal execution: class com.vaadin.hilla.internal.EndpointGeneratorTaskFactoryImpl cannot be cast to class com.vaadin.flow.server.frontend.EndpointGeneratorTaskFactory (com.vaadin.hilla.internal.EndpointGeneratorTaskFactoryImpl is in unnamed module of loader java.net.URLClassLoader @2b20caed; com.vaadin.flow.server.frontend.EndpointGeneratorTaskFactory is in unnamed module of loader org.codehaus.plexus.classworlds.realm.ClassRealm @78d92eef)

Caused by: java.lang.ClassCastException: class com.vaadin.hilla.internal.EndpointGeneratorTaskFactoryImpl cannot be cast to class com.vaadin.flow.server.frontend.EndpointGeneratorTaskFactory (com.vaadin.hilla.internal.EndpointGeneratorTaskFactoryImpl is in unnamed module of loader java.net.URLClassLoader @307eb95f; com.vaadin.flow.server.frontend.EndpointGeneratorTaskFactory is in unnamed module of loader org.codehaus.plexus.classworlds.realm.ClassRealm @226d5af0)
    at com.vaadin.flow.server.frontend.NodeTasks.addEndpointServicesTasks (NodeTasks.java:321)
    at com.vaadin.flow.server.frontend.NodeTasks.<init> (NodeTasks.java:236)
    at com.vaadin.flow.plugin.base.BuildFrontendUtil.prepareFrontend (BuildFrontendUtil.java:174)
    at com.vaadin.flow.plugin.maven.PrepareFrontendMojo.execute (PrepareFrontendMojo.java:64)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)

[mcollovati]

I think the missing piece here is to make the Flow mojos set the ClassFinder as thread context class loader.
At least, this seems to work for Flow projects: for example, if the project uses Vaadin 24.5.0 but sets flow-maven-plugin to version 24.6-SNAPSHOT, the resources are then correctly collected from the 24.5 flow-server jar.

I wonder if the previous error can be fixed by applying the same pattern to the Hilla configure mojo, that seems to be executed before prepare-fronted, and probably is loading the EndpointGeneratorTaskFactory into a shared classloader.

[mcollovati]

Isolating the execution so that flow-server classes are not loaded into the plugin class loader seems to be more complicated than expected.

[mcollovati]

Changing the thread context class loader during mojo execution does not work. flow-server classes are still loaded into the plugin class loader, causing a ClassCastException when Hilla or Copilot classes are loaded.

[mcollovati]

Also, making flow-server as provided dependency seems not to work. In addition, we might end up in situations where the plugin might fail or misbehave because of incompatibilities between the flow-server version expected by the plugin and the one used by the end-user project.

[mcollovati]

Superseded by #20465