[MSHADE-366] "Access denied" during 'minimizeJar' (#161)

* [MSHADE-366] - "Access denied" during 'minimizeJar'

Now ignoring directories when scanning the classpath for services.

* [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.

* Another attempt to clarify the problem

- do not ignore directories, print a warning as before
- ignore the project's build output directory which is always returned by getRuntimeClassPathElements()

* Fix the test to work on all platforms, irrespective of the actual exception sent by the JDK

Co-authored-by: Jan Mosig <jan.mosig@itemis.de>
Co-authored-by: Alexander Kriegisch <Alexander@Kriegisch.name>

Author: 3 people

src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java

@@ -129,58 +129,22 @@ private void removeServices( final MavenProject project, final Clazzpath cp )
             neededClasses.removeAll( removable );
             try
             {
+                // getRuntimeClasspathElements returns a list of
+                //  - the build output directory
+                //  - all the paths to the dependencies' jars
+                // We thereby need to ignore the build directory because we don't want
+                // to remove anything from it, as it's the starting point of the
+                // minification process.
                 for ( final String fileName : project.getRuntimeClasspathElements() )
                 {
-                    try ( final JarFile jar = new JarFile( fileName ) )
+                    // Ignore the build directory from this project
+                    if ( fileName.equals( project.getBuild().getOutputDirectory() ) )
                     {
-                        for ( final Enumeration<JarEntry> 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() );
-                            }
-                        }
+                        continue;
                     }
-                    catch ( final IOException e )
+                    if ( removeServicesFromJar( cp, neededClasses, fileName ) )
                     {
-                        log.warn( e.getMessage() );
+                        repeatScan = true;
                     }
                 }
             }
@@ -192,6 +156,69 @@ private void removeServices( final MavenProject project, final Clazzpath cp )
         while ( repeatScan );
     }
 
+    private boolean removeServicesFromJar( Clazzpath cp, Set<Clazz> neededClasses, String fileName )
+    {
+        boolean repeatScan = false;
+        try ( final JarFile jar = new JarFile( fileName ) )
+        {
+            for ( final Enumeration<JarEntry> 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( "Not a JAR file candidate. Ignoring classpath element '" + fileName + "' (" + e + ")." );
+        }
+        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 );

src/test/java/org/apache/maven/plugins/shade/filter/MinijarFilterTest.java

@@ -19,40 +19,60 @@
  * under the License.
  */
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeFalse;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import java.io.File;
+import java.io.FileOutputStream;
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.jar.JarOutputStream;
 
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.artifact.DefaultArtifact;
+import org.apache.maven.artifact.DependencyResolutionRequiredException;
+import org.apache.maven.model.Build;
 import org.apache.maven.plugin.logging.Log;
 import org.apache.maven.project.MavenProject;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 import org.mockito.ArgumentCaptor;
 
 public class MinijarFilterTest
 {
 
+    @Rule
+    public TemporaryFolder tempFolder = TemporaryFolder.builder().assureDeletion().build();
+
+    private File outputDirectory;
     private File emptyFile;
+    private File jarFile;
+    private Log log;
+    private ArgumentCaptor<CharSequence> logCaptor;
 
     @Before
     public void init()
         throws IOException
     {
-        TemporaryFolder tempFolder = new TemporaryFolder();
-        tempFolder.create();
+        this.outputDirectory = tempFolder.newFolder();
         this.emptyFile = tempFolder.newFile();
-
+        this.jarFile = tempFolder.newFile();
+        new JarOutputStream( new FileOutputStream( this.jarFile ) ).close();
+        this.log = mock(Log.class);
+        logCaptor = ArgumentCaptor.forClass(CharSequence.class);
     }
 
     /**
@@ -64,11 +84,7 @@ public void testWithMockProject()
     {
         assumeFalse( "Expected to run under JDK8+", System.getProperty("java.version").startsWith("1.7") );
 
-        ArgumentCaptor<CharSequence> logCaptor = ArgumentCaptor.forClass( CharSequence.class );
-
-        MavenProject mavenProject = mockProject( emptyFile );
-
-        Log log = mock( Log.class );
+        MavenProject mavenProject = mockProject( outputDirectory, emptyFile );
 
         MinijarFilter mf = new MinijarFilter( mavenProject, log );
 
@@ -84,14 +100,10 @@ public void testWithMockProject()
     public void testWithPomProject()
         throws IOException
     {
-        ArgumentCaptor<CharSequence> logCaptor = ArgumentCaptor.forClass( CharSequence.class );
-
         // project with pom packaging and no artifact.
-        MavenProject mavenProject = mockProject( null );
+        MavenProject mavenProject = mockProject( outputDirectory, null );
         mavenProject.setPackaging( "pom" );
 
-        Log log = mock( Log.class );
-
         MinijarFilter mf = new MinijarFilter( mavenProject, log );
 
         mf.finished();
@@ -105,7 +117,7 @@ public void testWithPomProject()
 
     }
 
-    private MavenProject mockProject( File file )
+    private MavenProject mockProject( File outputDirectory, File file, String... classPathElements )
     {
         MavenProject mavenProject = mock( MavenProject.class );
 
@@ -129,17 +141,29 @@ private MavenProject mockProject( File file )
 
         when( mavenProject.getArtifact().getFile() ).thenReturn( file );
 
-        return mavenProject;
+        Build build = new Build();
+        build.setOutputDirectory( outputDirectory.toString() );
+
+        List<String> classpath = new ArrayList<>();
+        classpath.add( outputDirectory.toString() );
+        if ( file != null )
+        {
+            classpath.add(file.toString());
+        }
+        classpath.addAll( Arrays.asList( classPathElements ) );
+        when( mavenProject.getBuild() ).thenReturn( build );
+        try {
+            when(mavenProject.getRuntimeClasspathElements()).thenReturn(classpath);
+        } catch (DependencyResolutionRequiredException e) {
+            fail("Encountered unexpected exception: " + e.getClass().getSimpleName() + ": " + e.getMessage());
+        }
 
+        return mavenProject;
     }
 
     @Test
     public void finsishedShouldProduceMessageForClassesTotalNonZero()
     {
-        ArgumentCaptor<CharSequence> logCaptor = ArgumentCaptor.forClass( CharSequence.class );
-
-        Log log = mock( Log.class );
-
         MinijarFilter m = new MinijarFilter( 1, 50, log );
 
         m.finished();
@@ -153,10 +177,6 @@ public void finsishedShouldProduceMessageForClassesTotalNonZero()
     @Test
     public void finishedShouldProduceMessageForClassesTotalZero()
     {
-        ArgumentCaptor<CharSequence> logCaptor = ArgumentCaptor.forClass( CharSequence.class );
-
-        Log log = mock( Log.class );
-
         MinijarFilter m = new MinijarFilter( 0, 0, log );
 
         m.finished();
@@ -166,4 +186,24 @@ public void finishedShouldProduceMessageForClassesTotalZero()
         assertEquals( "Minimized 0 -> 0", logCaptor.getValue() );
 
     }
+
+    /**
+     * Verify that directories are ignored when scanning the classpath for JARs containing services,
+     * but warnings are logged instead
+     *
+     * @see <a href="https://issues.apache.org/jira/browse/MSHADE-366">MSHADE-366</a>
+     */
+    @Test
+    public void removeServicesShouldIgnoreDirectories() throws Exception {
+        String classPathElementToIgnore = tempFolder.newFolder().getAbsolutePath();
+        MavenProject mockedProject = mockProject( outputDirectory, jarFile, classPathElementToIgnore );
+
+        new MinijarFilter(mockedProject, log);
+
+        verify( log, times( 1 ) ).warn( logCaptor.capture() );
+
+        assertThat( logCaptor.getValue().toString(), startsWith(
+                "Not a JAR file candidate. Ignoring classpath element '" + classPathElementToIgnore + "' (" ) );
+    }
+
 }