Skip to content

Commit

Permalink
[MSHADE-366] Refactor fix by @JanMosigItemis from apache#83
Browse files Browse the repository at this point in the history 
- Simplify Jan's solution from apache#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.
  • Loading branch information
@kriegaex
kriegaex committed Jul 21, 2021
1 parent 0cd2be6 commit fe5e81a
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 77 deletions.
124 changes: 68 additions & 56 deletions src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
Expand All @@ -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 );
Expand Down
37 changes: 16 additions & 21 deletions src/test/java/org/apache/maven/plugins/shade/filter/MinijarFilterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
{

Expand Down Expand Up @@ -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 + "'."
);
}

}

0 comments on commit fe5e81a

Please sign in to comment.