Skip to content

Commit

Permalink
[MSHADE-366] "Access denied" during 'minimizeJar' (#161)
Browse files Browse the repository at this point in the history 
* [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>
  • Loading branch information
@gnodet @JanMosigItemis @kriegaex
3 people authored Oct 20, 2022
1 parent e342059 commit 41bd72f
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 70 deletions.
121 changes: 74 additions & 47 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 @@ -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;
}
}
}
Expand All @@ -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 );
Expand Down
86 changes: 63 additions & 23 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 @@ -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);
}

/**
Expand All @@ -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 );

Expand All @@ -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();
Expand All @@ -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 );

Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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 + "' (" ) );
}

}

0 comments on commit 41bd72f

Please sign in to comment.