[MDEP-645] Created new mojo GetClassesMojo which lists all class dependencies for a specified artifact. by jonvolfson · Pull Request #57 · apache/maven-dependency-plugin

jonvolfson · 2020-05-26T15:12:28Z

…r a specified artifact

khmarbaise · 2020-05-26T15:26:00Z

First please create an appropriate JIRA issue for that and describe the use case for this.

jonvolfson · 2020-05-26T15:37:36Z

Updated PR title. This is a fix for MDEP-645

ghost · 2020-05-26T15:33:59Z

+     *
+     * @since 2.7
+     */
+    @Parameter( property = "mdep.skip", defaultValue = "false" )


I think you can remove all of the 'skip' processing; it's not terribly useful for a mojo intended for CLI.

@bimargulies-google I'd keep it since you never know how it is used and maven does not control that

ghost · 2020-05-26T15:35:17Z

+    public ProjectBuildingRequest buildBuildingRequest()
+            throws MojoExecutionException, MojoFailureException
+    {
+        if ( coordinate.getArtifactId() == null && artifact == null )


You don't use coordinate below. Are you missing an else clause? Is that the code that would run if someone used -DgroupId ... instead of -Dartifact?

Fixed. Removed the coordinate.getArtifactId() part. This was recycled code from the GetMojo with a similar use case.

ghost · 2020-05-26T15:37:15Z

+
+                for ( ArtifactResult result : artifacts )
+                {
+                    printClassesFromArtifactResult( result );


Print the file name of the jar before printing its contents if you are printing more than one jar file's worth?

ghost · 2020-05-26T15:37:46Z

+        setVariableValueToObject( mojo, "transitive", Boolean.FALSE );
+
+        mojo.execute();
+        return;


No need for return.

ghost · 2020-05-26T15:37:57Z

+        setVariableValueToObject( mojo, "transitive", Boolean.TRUE );
+
+        mojo.execute();
+        return;


No need for return.

ghost · 2020-05-26T15:41:47Z

+        return;
+    }
+
+    public void testGetClassesTransitive()


Um, this does not seem to test anything except that it does not explode.

When listing classes for something like org.apache.maven:maven-model:2.0.9, setting -Dtransitive=true lists more classes than the false alternative. It's possible that the artifact used in the test has little/no transitive dependencies which means the results would be similar/the same.

ghost · 2020-05-26T15:42:50Z

+
+            // remove .class from the end and change format to use periods instead of forward slashes
+            name = name.substring( 0, name.length() - 6 ).replace( '/', '.' );
+            getLog().info( name );


I don't think that using the log is the best delivery mechanism, but I could be confused. @elharo what do you think? Also, I have no idea how to test for log output, you'd need to mock the log somehow.

+1, can be neat to be able to inject the list of classes in a maven project properties to reuse it downstream (exec-maven-plugin is my immediate thought but i'm sure there are more use cases). A dump in a file can be useful too.

side note: what about multi release jars (META-INF/versions)? should it be filtered?

I think logging is how we usually do this. The log can be injected or tested.

…recycled code for building the ProjectBuildRequest

elharo · 2020-05-26T16:11:45Z

+
+
+/**
+ * Retrieves and lists all class dependencies for the specified artifact from the specified remote repositories.


class dependencies for --> "classes contained in" as these are not dependencies

elharo · 2020-05-26T16:12:16Z

+ * Retrieves and lists all class dependencies for the specified artifact from the specified remote repositories.
+ */
+@Mojo( name = "get-classes", requiresProject = false, threadSafe = true )
+public class GetClassesMojo


Perhaps ListClassesMojo?

elharo · 2020-05-26T16:12:32Z

+    private DefaultDependableCoordinate coordinate = new DefaultDependableCoordinate();
+
+    /**
+     * The groupId of the artifact to download. Ignored if {@link #artifact} is used.


elharo · 2020-05-26T16:12:47Z

+    private String groupId;
+
+    /**
+     * The artifactId of the artifact to download. Ignored if {@link #artifact} is used.


artifact ID

elharo · 2020-05-26T16:13:09Z

+     * The packaging of the artifact to download. Ignored if {@link #artifact} is used.
+     */
+    @Parameter( property = "packaging", defaultValue = "jar" )
+    private String packaging = "jar";


will this work for anything that's not a jar?

It will work for a .war or any other 'renamed jar'. But the substitution of dots and slashes should perhaps be removed for non-.jar.

elharo · 2020-05-26T16:21:54Z

+    }
+
+    /**
+     * @param artifact The artifact


doc comments begin with lower case, per Oracle guidelines

More clarification? This is taken from the GetMojo.

elharo · 2020-05-26T16:22:06Z

+    }
+
+    /**
+     * @param artifactId The artifactId.


artifact ID

elharo · 2020-05-26T16:22:58Z

+import org.apache.maven.plugin.testing.stubs.MavenProjectStub;
+import org.apache.maven.settings.Server;
+import org.apache.maven.settings.Settings;
+import org.sonatype.aether.impl.internal.SimpleLocalRepositoryManager;


Tricky but can we replace this with org.eclipse equivalents? Probably not a 1:1 replacement.

Do you have an example to point at here?

Not off the top of my head but try calling lookup(LocalRepositoryManager.class) in the setUp method and see if that returns an object that works.

elharo · 2020-05-26T16:23:12Z

+public class TestGetClassesMojo
+        extends AbstractDependencyMojoTestCase
+{
+    GetClassesMojo mojo;


private is not allowed for this test class

elharo · 2020-05-26T16:23:29Z

+        super.setUp( "markers", false );
+
+        File testPom = new File( getBasedir(), "target/test-classes/unit/get-test/plugin-config.xml" );
+        assert testPom.exists();


NEVER use assert

oh, whoops, sorry I missed this.

…ansitive default value to false, changed ListClassesMojo methods to private if able, replaced assert in test case.

elharo · 2020-05-26T18:47:02Z

+    private String packaging = "jar";
+
+    /**
+     * Repositories in the format id::[layout]::url or just URLs, separated by comma. ie.


avoid Latin. ie. --> That is,

elharo · 2020-05-26T18:47:58Z

+        return dependencyResolver;
+    }
+
+    protected ArtifactResolver getArtifactResolver()


do you need this method?

fixed, removed this method and other getters that weren't needed

elharo · 2020-05-26T18:49:24Z

+        {
+            if ( isTransitive() )
+            {
+                Iterable<ArtifactResult> artifacts = getDependencyResolver()


you can access fields like dependencyResolver directly. There's no need to use a getter method in the same class.

elharo · 2020-05-26T18:49:50Z

+    public void execute() throws MojoExecutionException, MojoFailureException
+    {
+        ProjectBuildingRequest buildingRequest = buildBuildingRequest();
+        DefaultDependableCoordinate coordinate = getCoordinate();


don't shadow the field here

elharo · 2020-05-26T18:50:44Z

+        }
+        catch ( ArtifactResolverException | DependencyResolverException | IOException e )
+        {
+            throw new MojoExecutionException( "Couldn't download artifact: " + e.getMessage(), e );


exception message should contain coordinates of artifact it was trying to download

elharo · 2020-05-26T18:51:19Z

+        jarFile.close();
+    }
+
+    private ProjectBuildingRequest buildBuildingRequest()


makeBuildingRequest?

elharo · 2020-05-26T19:00:10Z

+            }
+
+            // remove .class from the end and change format to use periods instead of forward slashes
+            name = name.substring( 0, name.length() - 6 ).replace( '/', '.' );


try to avoid reusing local variables. Here you have two things: an entry name and a class name, so two different variables make this clearer

elharo · 2020-05-26T19:02:24Z

+    }
+
+    /**
+     * @param artifact The artifact.


https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#tag

…eract with the mojo, fixed comments to follow oracle javadoc guidelines, changes method names to be more descriptive of their purpose, fixed shadowing variable names and renamed variables to be more descriptive

elharo · 2020-05-27T17:14:36Z

+
+    /**
+     * Skip plugin execution completely.
+     *


nit: remove blank line

elharo · 2020-05-27T17:16:02Z

+        }
+        catch ( ArtifactResolverException | DependencyResolverException | IOException e )
+        {
+            throw new MojoExecutionException( "Couldn't download artifact " + artifact + ": " + e.getMessage(), e );


This message might not be quite accurate when you're downloading all transitive dependencies. That is, it could be one of the transitive deps we can't download instead of the original.

I believe the original message was correct before I changed it to include artifact. If a transitive dependency wasn't able to be resolved, e.getMessage() should have included the correct one.

elharo · 2020-05-27T17:17:10Z

+            String className = entryName.substring( 0, entryName.length() - 6 ).replace( '/', '.' );
+            getLog().info( className );
+        }
+        jarFile.close();


use try-with-resources to guarantee that this is closed when an exception is thrown

elharo · 2020-05-27T17:20:07Z

+    }
+
+    /**
+     * @return {@link #skip}


I don't think you need these getter methods, and probably not the setters

elharo · 2020-05-27T17:20:21Z

+public class TestGetClassesMojo
+        extends AbstractDependencyMojoTestCase
+{
+    ListClassesMojo mojo;


make field private

…test class a private field, included printing logic to use a try-with-resource statement to guarantee JarFile closure, reverted thrown error message when resolving dependencies to original to be less confusing.

…ndencies for a specified artifact. (apache#57) * Created new mojo GetClassesMojo which lists all class dependencies for a specified artifact * adding test file and fixing small function name * Updating test functions names to be self-describing * Fixing styling issues preventing build completion * Removed return statements from test cases, removed unneeded piece of recycled code for building the ProjectBuildRequest * Changed GetClassesMojo to ListClassesMojo, fixed comments, changed transitive default value to false, changed ListClassesMojo methods to private if able, replaced assert in test case. * Changed modifying methods to private since outside classes do not interact with the mojo, fixed comments to follow oracle javadoc guidelines, changes method names to be more descriptive of their purpose, fixed shadowing variable names and renamed variables to be more descriptive * Removed unneccessary setter and getter methods, made the mojo in the test class a private field, included printing logic to use a try-with-resource statement to guarantee JarFile closure, reverted thrown error message when resolving dependencies to original to be less confusing.

jira-importer · 2025-06-18T08:01:11Z

Resolve #1075

jonvolfson added 3 commits May 26, 2020 11:05

Created new mojo GetClassesMojo which lists all class dependencies fo…

ad9a74a

…r a specified artifact

adding test file and fixing small function name

3cf44ac

Updating test functions names to be self-describing

1ae2324

Fixing styling issues preventing build completion

71000a7

jonvolfson changed the title ~~Created new mojo GetClassesMojo which lists all class dependencies for a specified artifact.~~ [MDEP-645] Created new mojo GetClassesMojo which lists all class dependencies for a specified artifact. May 26, 2020

ghost reviewed May 26, 2020

View reviewed changes

Removed return statements from test cases, removed unneeded piece of …

45cc373

…recycled code for building the ProjectBuildRequest

elharo reviewed May 26, 2020

View reviewed changes

Changed GetClassesMojo to ListClassesMojo, fixed comments, changed tr…

8eae254

…ansitive default value to false, changed ListClassesMojo methods to private if able, replaced assert in test case.

elharo reviewed May 26, 2020

View reviewed changes

Changed modifying methods to private since outside classes do not int…

44330b9

…eract with the mojo, fixed comments to follow oracle javadoc guidelines, changes method names to be more descriptive of their purpose, fixed shadowing variable names and renamed variables to be more descriptive

elharo reviewed May 27, 2020

View reviewed changes

Removed unneccessary setter and getter methods, made the mojo in the …

448432a

…test class a private field, included printing logic to use a try-with-resource statement to guarantee JarFile closure, reverted thrown error message when resolving dependencies to original to be less confusing.

elharo merged commit e86cbe2 into apache:master Jun 17, 2020



		/**
		* Retrieves and lists all class dependencies for the specified artifact from the specified remote repositories.

Conversation

jonvolfson commented May 26, 2020

Uh oh!

khmarbaise commented May 26, 2020

Uh oh!

jonvolfson commented May 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment