From f7d43102173568f4272325ad71e334279b91db17 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 14 Aug 2019 08:51:42 +0100 Subject: [PATCH] [SUREFIRE-1679] Prevent classpath caching from causing pollution Previously, classpath caching was performed statically. This resulted in the classpath cached by one project for a particular provider being used by a subsequent project. As a result any customizations to the classpath, such as removing duplicate artifacts, would leak out and pollute the classpath used by subsequent projects. This commit prevents the pollution by making the classpath cache instance-scoped so that the cache is only used by a single mojo and, therefore, a single project. --- .../plugin/surefire/AbstractSurefireMojo.java | 41 +++++++++++-- .../maven/plugin/surefire/ClasspathCache.java | 1 + .../surefire/AbstractSurefireMojoTest.java | 61 +++++++++++++++++++ 3 files changed, 97 insertions(+), 6 deletions(-) diff --git a/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java b/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java index 2f179e850d..e2c69b6d51 100644 --- a/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java +++ b/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java @@ -159,6 +159,8 @@ public abstract class AbstractSurefireMojo private final ProviderDetector providerDetector = new ProviderDetector(); + private final ClasspathCache classpathCache = new ClasspathCache(); + /** * Note: use the legacy system property disableXmlReport set to {@code true} to disable the report. */ @@ -1787,10 +1789,10 @@ private StartupConfiguration newStartupConfigWithClasspath( { Classpath testClasspath = testClasspathWrapper.toClasspath(); - Classpath providerClasspath = ClasspathCache.getCachedClassPath( providerName ); + Classpath providerClasspath = classpathCache.getCachedClassPath( providerName ); if ( providerClasspath == null ) { - providerClasspath = ClasspathCache.setCachedClasspath( providerName, providerArtifacts ); + providerClasspath = classpathCache.setCachedClasspath( providerName, providerArtifacts ); } getConsoleLogger().debug( testClasspath.getLogMessage( "test classpath:" ) ); @@ -1868,10 +1870,10 @@ private StartupConfiguration newStartupConfigWithModularPath( { Classpath testClasspath = testClasspathWrapper.toClasspath(); - Classpath providerClasspath = ClasspathCache.getCachedClassPath( providerName ); + Classpath providerClasspath = classpathCache.getCachedClassPath( providerName ); if ( providerClasspath == null ) { - providerClasspath = ClasspathCache.setCachedClasspath( providerName, providerArtifacts ); + providerClasspath = classpathCache.setCachedClasspath( providerName, providerArtifacts ); } ResolvePathsRequest req = ResolvePathsRequest.ofStrings( testClasspath.getClassPath() ) @@ -2629,7 +2631,7 @@ private void showMap( Map map, String setting ) private Classpath getArtifactClasspath( Artifact surefireArtifact ) { - Classpath existing = ClasspathCache.getCachedClassPath( surefireArtifact.getArtifactId() ); + Classpath existing = classpathCache.getCachedClassPath( surefireArtifact.getArtifactId() ); if ( existing == null ) { List items = new ArrayList<>(); @@ -2643,7 +2645,7 @@ private Classpath getArtifactClasspath( Artifact surefireArtifact ) items.add( artifact.getFile().getAbsolutePath() ); } existing = new Classpath( items ); - ClasspathCache.setCachedClasspath( surefireArtifact.getArtifactId(), existing ); + classpathCache.setCachedClasspath( surefireArtifact.getArtifactId(), existing ); } return existing; } @@ -3839,4 +3841,31 @@ else if ( forkMode.equals( FORK_NEVER ) || forkMode.equals( FORK_ONCE ) throw new IllegalArgumentException( "Fork mode " + forkMode + " is not a legal value" ); } } + + private static final class ClasspathCache + { + private final Map classpaths = new HashMap<>( 4 ); + + private Classpath getCachedClassPath( @Nonnull String artifactId ) + { + return classpaths.get( artifactId ); + } + + private void setCachedClasspath( @Nonnull String key, @Nonnull Classpath classpath ) + { + classpaths.put( key, classpath ); + } + + private Classpath setCachedClasspath( @Nonnull String key, @Nonnull Set artifacts ) + { + Collection files = new ArrayList<>(); + for ( Artifact artifact : artifacts ) + { + files.add( artifact.getFile().getAbsolutePath() ); + } + Classpath classpath = new Classpath( files ); + setCachedClasspath( key, classpath ); + return classpath; + } + } } diff --git a/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ClasspathCache.java b/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ClasspathCache.java index 7ba7b5475f..6dbaf29f92 100644 --- a/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ClasspathCache.java +++ b/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/ClasspathCache.java @@ -32,6 +32,7 @@ /** * @author Kristian Rosenvold */ +@Deprecated public class ClasspathCache { private static final ConcurrentHashMap CLASSPATHS = diff --git a/maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java b/maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java index cec6760461..bd77143e75 100644 --- a/maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java +++ b/maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java @@ -335,6 +335,67 @@ public void shouldHaveStartupConfigForNonModularClasspath() .isEqualTo( "org.asf.Provider" ); } + @Test + public void providerClasspathCachingIsNotSharedAcrossMojoInstances() throws Exception { + ProviderInfo providerInfo = mock( ProviderInfo.class ); + when( providerInfo.getProviderName() ).thenReturn( "test-provider" ); + Artifact provider = new DefaultArtifact( "com.example", "provider", createFromVersion( "1" ), "runtime", "jar", "", handler ); + provider.setFile( mockFile( "original-test-provider.jar" ) ); + HashSet providerClasspath = new HashSet( asList( provider ) ); + when( providerInfo.getProviderClasspath()).thenReturn( providerClasspath ); + + StartupConfiguration startupConfiguration = startupConfigurationForProvider(providerInfo); + assertThat( startupConfiguration.getClasspathConfiguration().getProviderClasspath().getClassPath() ) + .containsExactly( "original-test-provider.jar" ); + + provider.setFile( mockFile( "modified-test-provider.jar" ) ); + startupConfiguration = startupConfigurationForProvider(providerInfo); + assertThat( startupConfiguration.getClasspathConfiguration().getProviderClasspath().getClassPath() ) + .containsExactly( "modified-test-provider.jar" ); + } + + private StartupConfiguration startupConfigurationForProvider(ProviderInfo providerInfo) throws Exception { + AbstractSurefireMojo mojo = spy( new Mojo() ); + + Logger logger = mock( Logger.class ); + when( logger.isDebugEnabled() ).thenReturn( true ); + doNothing().when( logger ).debug( anyString() ); + when( mojo.getConsoleLogger() ).thenReturn( new PluginConsoleLogger( logger ) ); + + File classesDir = mockFile( "classes" ); + File testClassesDir = mockFile( "test-classes" ); + TestClassPath testClassPath = new TestClassPath( new ArrayList(), classesDir, testClassesDir, new String[0] ); + + Artifact common = new DefaultArtifact( "org.apache.maven.surefire", "maven-surefire-common", + createFromVersion( "1" ), "runtime", "jar", "", handler ); + common.setFile( mockFile( "maven-surefire-common.jar" ) ); + + Artifact ext = new DefaultArtifact( "org.apache.maven.surefire", "surefire-extensions-api", + createFromVersion( "1" ), "runtime", "jar", "", handler ); + ext.setFile( mockFile( "surefire-extensions-api.jar" ) ); + + Artifact api = new DefaultArtifact( "org.apache.maven.surefire", "surefire-api", + createFromVersion( "1" ), "runtime", "jar", "", handler ); + api.setFile( mockFile( "surefire-api.jar" ) ); + + Artifact loggerApi = new DefaultArtifact( "org.apache.maven.surefire", "surefire-logger-api", + createFromVersion( "1" ), "runtime", "jar", "", handler ); + loggerApi.setFile( mockFile( "surefire-logger-api.jar" ) ); + + Map providerArtifactsMap = new HashMap<>(); + providerArtifactsMap.put( "org.apache.maven.surefire:maven-surefire-common", common ); + providerArtifactsMap.put( "org.apache.maven.surefire:surefire-extensions-api", ext ); + providerArtifactsMap.put( "org.apache.maven.surefire:surefire-api", api ); + providerArtifactsMap.put( "org.apache.maven.surefire:surefire-logger-api", loggerApi ); + + when( mojo.getPluginArtifactMap() ).thenReturn( providerArtifactsMap ); + + doReturn( 1 ).when( mojo, "getEffectiveForkCount" ); + + StartupConfiguration startupConfiguration = invokeMethod( mojo, "createStartupConfiguration", providerInfo, false, null, null, null, testClassPath); + return startupConfiguration; + } + @Test public void shouldExistTmpDirectory() throws IOException {