fix: Classes should not be loaded from the plugin

[Artur-]

Related-to https://github.com/vaadin/flow/issues/19009

[sonarqubecloud[bot]]

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[bot]]

Test Results

1 028 files   -  61  1 028 suites   - 61   1h 9m 0s :stopwatch: - 12m 6s 6 470 tests  - 442  6 419 :white_check_mark:  - 444  48 :zzz:  - 1  1 :x: +1  2 :fire: +2  6 827 runs   - 419  6 764 :white_check_mark:  - 421  60 :zzz:  - 1  1 :x: +1  2 :fire: +2 

For more details on these failures and errors, see this check.

Results for commit cdd42fc5. ± Comparison against base commit ba28266f.

This pull request removes 442 tests.

com.vaadin.base.devserver.AbstractDevServerRunnerTest ‑ shouldPassEncodedUrlToDevServer
com.vaadin.base.devserver.AbstractDevServerRunnerTest ‑ updateServerStartupEnvironment_preferIpv4_LocalhostIpAddressAddedToProcessEnvironment
com.vaadin.base.devserver.AbstractDevServerRunnerTest ‑ updateServerStartupEnvironment_preferIpv6_LocalhostIpAddressAddedToProcessEnvironment
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_devMode_contextHasNoReloadInstance_instanceIsCreated
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_devMode_contextHasReloadInstance_instanceIsReturned
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_liveReloadDisabled_instanceIsCreated
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_productionMode_nullIsReturned
com.vaadin.base.devserver.DebugWindowConnectionTest ‑ backwardsCompatibilityClassExists
com.vaadin.base.devserver.DebugWindowConnectionTest ‑ getBackend_HotSwapVaadinIntegrationClassLoaded_returnsHOTSWAP_AGENT
com.vaadin.base.devserver.DebugWindowConnectionTest ‑ getBackend_JRebelClassEventListenerClassLoaded_returnsJREBEL
…

:recycle: 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