From fe5e81a7db020e25e8f22b940be6001c6fd1031e Mon Sep 17 00:00:00 2001 From: Alexander Kriegisch Date: Sat, 3 Jul 2021 12:11:06 +0700 Subject: [PATCH] [MSHADE-366] Refactor fix by @JanMosigItemis from #83 - Simplify Jan's solution from #83 in order to use 'continue' instead of nested 'if-else'. - Factor out two helper methods from 'removeServices', because that method was way too big to still be readable. - DRY-refactor Jan's new test cases into one checking two conditions. --- .../plugins/shade/filter/MinijarFilter.java | 124 ++++++++++-------- .../shade/filter/MinijarFilterTest.java | 37 +++--- 2 files changed, 84 insertions(+), 77 deletions(-) diff --git a/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java b/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java index 48f8f189..23286788 100644 --- a/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java +++ b/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java @@ -35,8 +35,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; -import java.nio.file.Files; -import java.nio.file.Paths; import java.util.Collections; import java.util.Enumeration; import java.util.HashSet; @@ -133,63 +131,14 @@ private void removeServices( final MavenProject project, final Clazzpath cp ) { for ( final String fileName : project.getRuntimeClasspathElements() ) { - if ( !Files.isDirectory( Paths.get( fileName ) ) ) + if ( new File( fileName ).isDirectory() ) { - try ( final JarFile jar = new JarFile( fileName ) ) - { - for ( final Enumeration entries = jar.entries(); entries.hasMoreElements(); ) - { - final JarEntry jarEntry = entries.nextElement(); - if ( jarEntry.isDirectory() || !jarEntry.getName().startsWith( "META-INF/services/" ) ) - { - continue; - } - - final String serviceClassName = - jarEntry.getName().substring( "META-INF/services/".length() ); - final boolean isNeededClass = neededClasses.contains( cp.getClazz( serviceClassName ) ); - if ( !isNeededClass ) - { - continue; - } - - try ( final BufferedReader bufferedReader = - new BufferedReader( new InputStreamReader( jar.getInputStream( jarEntry ), UTF_8 ) ) ) - { - for ( String line = bufferedReader.readLine(); line != null; - line = bufferedReader.readLine() ) - { - final String className = line.split( "#", 2 )[0].trim(); - if ( className.isEmpty() ) - { - continue; - } - - final Clazz clazz = cp.getClazz( className ); - if ( clazz == null || !removable.contains( clazz ) ) - { - continue; - } - - log.debug( className + " was not removed because it is a service" ); - removeClass( clazz ); - repeatScan = true; // check whether the found classes use services in turn - } - } - catch ( final IOException e ) - { - log.warn( e.getMessage() ); - } - } - } - catch ( final IOException e ) - { - log.warn( e.getMessage() ); - } + log.debug( "Not a JAR file candidate. Ignoring classpath element '" + fileName + "'." ); + continue; } - else + if ( removeServicesFromJar( cp, neededClasses, fileName ) ) { - log.debug( "Not a JAR file candidate. Ignoring classpath element '" + fileName + "'." ); + repeatScan = true; } } } @@ -201,6 +150,69 @@ private void removeServices( final MavenProject project, final Clazzpath cp ) while ( repeatScan ); } + private boolean removeServicesFromJar( Clazzpath cp, Set neededClasses, String fileName ) + { + boolean repeatScan = false; + try ( final JarFile jar = new JarFile( fileName ) ) + { + for ( final Enumeration entries = jar.entries(); entries.hasMoreElements(); ) + { + final JarEntry jarEntry = entries.nextElement(); + if ( jarEntry.isDirectory() || !jarEntry.getName().startsWith( "META-INF/services/" ) ) + { + continue; + } + + final String serviceClassName = jarEntry.getName().substring( "META-INF/services/".length() ); + final boolean isNeededClass = neededClasses.contains( cp.getClazz( serviceClassName ) ); + if ( !isNeededClass ) + { + continue; + } + + try ( final BufferedReader configFileReader = new BufferedReader( + new InputStreamReader( jar.getInputStream( jarEntry ), UTF_8 ) ) ) + { + // check whether the found classes use services in turn + repeatScan = scanServiceProviderConfigFile( cp, configFileReader ); + } + catch ( final IOException e ) + { + log.warn( e.getMessage() ); + } + } + } + catch ( final IOException e ) + { + log.warn( e.getMessage() ); + } + return repeatScan; + } + + private boolean scanServiceProviderConfigFile( Clazzpath cp, BufferedReader configFileReader ) throws IOException + { + boolean serviceClassFound = false; + for ( String line = configFileReader.readLine(); line != null; line = configFileReader.readLine() ) + { + final String className = line.split( "#", 2 )[0].trim(); + if ( className.isEmpty() ) + { + continue; + } + + final Clazz clazz = cp.getClazz( className ); + if ( clazz == null || !removable.contains( clazz ) ) + { + continue; + } + + log.debug( className + " was not removed because it is a service" ); + removeClass( clazz ); + serviceClassFound = true; + } + return serviceClassFound; + } + private void removeClass( final Clazz clazz ) { removable.remove( clazz ); diff --git a/src/test/java/org/apache/maven/plugins/shade/filter/MinijarFilterTest.java b/src/test/java/org/apache/maven/plugins/shade/filter/MinijarFilterTest.java index 58e14d26..874c93e7 100644 --- a/src/test/java/org/apache/maven/plugins/shade/filter/MinijarFilterTest.java +++ b/src/test/java/org/apache/maven/plugins/shade/filter/MinijarFilterTest.java @@ -28,6 +28,12 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.io.File; +import java.io.IOException; +import java.util.Arrays; +import java.util.Set; +import java.util.TreeSet; + import org.apache.maven.artifact.Artifact; import org.apache.maven.artifact.DefaultArtifact; import org.apache.maven.artifact.DependencyResolutionRequiredException; @@ -39,12 +45,6 @@ import org.junit.rules.TemporaryFolder; import org.mockito.ArgumentCaptor; -import java.io.File; -import java.io.IOException; -import java.util.Arrays; -import java.util.Set; -import java.util.TreeSet; - public class MinijarFilterTest { @@ -166,27 +166,22 @@ public void finishedShouldProduceMessageForClassesTotalZero() } /** - * Check that the algorithm that removes services does not consider directories comming from the - * classpath as jar file candidates. - * - * @see https://issues.apache.org/jira/browse/MSHADE-366 + * Verify that directories are ignored when scanning the classpath for JARs containing services, + * but warnings are logged instead + * + * @see MSHADE-366 */ @Test - public void remove_services_ignores_directories() throws Exception { - MavenProject mockedProject = mockProject(emptyFile, tempFolder.getRoot().getAbsolutePath()); - - new MinijarFilter(mockedProject, log); - - verify(log, never()).warn(logCaptor.capture()); - } - - @Test - public void remove_services_logs_ignored_items() throws Exception { + public void removeServicesShouldIgnoreDirectories() throws Exception { String classPathElementToIgnore = tempFolder.getRoot().getAbsolutePath(); MavenProject mockedProject = mockProject(emptyFile, classPathElementToIgnore); new MinijarFilter(mockedProject, log); - verify(log, times(1)).debug("Not a JAR file candidate. Ignoring classpath element '" + classPathElementToIgnore + "'."); + verify(log, never()).warn(logCaptor.capture()); + verify(log, times(1)).debug( + "Not a JAR file candidate. Ignoring classpath element '" + classPathElementToIgnore + "'." + ); } + }