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

Author: kriegaex

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<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() );
-                                }
-                            }
-                        }
-                        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<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( 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 );

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

@@ -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 <a href="https://issues.apache.org/jira/browse/MSHADE-366">MSHADE-366</a>
      */
     @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 + "'."
+        );
     }
+
 }