extra comments and tests based on review

Author: rfscholte

src/main/java/org/apache/maven/plugins/dependency/fromDependencies/AbstractDependencyFilterMojo.java

@@ -239,7 +239,7 @@ public abstract class AbstractDependencyFilterMojo extends AbstractDependencyMoj
 
     /**
      * By default, this goal uses the project itself as the root of the dependency tree.
-     * With graphRoots, you can select a subtree of dependencies.
+     * With graphRoots, you can select a subtree of dependencies based on groupId and artifactId.
      * After that, the general include/exclude filters can be applied.
      *
      * @since 3.10.0
@@ -504,8 +504,12 @@ private Set<Artifact> resolve(Set<org.eclipse.aether.artifact.Artifact> artifact
 
     private Set<Artifact> collectArtifacts(MavenProject project) throws DependencyResolutionException {
         if (graphRoots == null || graphRoots.isEmpty()) {
+            // artifact have already been resolved here due to
+            // @Mojo(requiresDependencyResolution = ResolutionScope.TEST) on final Mojo
             return project.getArtifacts();
         } else {
+            // MavenProject doesn't provide access to the graph of dependencies(only the direct dependencies)
+            // Hence we need to re-resolve artifacts, but only for the matching graphnodes
             List<DependencyMatcher> filterMatchers =
                     graphRoots.stream().map(GraphRootMatcher::new).collect(Collectors.toList());
 

src/main/java/org/apache/maven/plugins/dependency/fromDependencies/GraphRoot.java

@@ -18,17 +18,12 @@
  */
 package org.apache.maven.plugins.dependency.fromDependencies;
 
-/**
- * All values are static, no expression, so matcher can use equals
- */
 public class GraphRoot {
 
     private String groupId;
 
     private String artifactId;
 
-    private String type;
-
     public String getGroupId() {
         return groupId;
     }
@@ -44,12 +39,4 @@ public String getArtifactId() {
     public void setArtifactId(String artifactId) {
         this.artifactId = artifactId;
     }
-
-    public String getType() {
-        return type;
-    }
-
-    public void setType(String type) {
-        this.type = type;
-    }
 }

src/main/java/org/apache/maven/plugins/dependency/fromDependencies/GraphRootMatcher.java

@@ -18,24 +18,21 @@
  */
 package org.apache.maven.plugins.dependency.fromDependencies;
 
+import java.util.Objects;
+
 import org.apache.maven.model.Dependency;
 
 class GraphRootMatcher implements DependencyMatcher {
 
     private final GraphRoot graphRoot;
 
     GraphRootMatcher(GraphRoot graphRoot) {
-        this.graphRoot = graphRoot;
+        this.graphRoot = Objects.requireNonNull(graphRoot);
     }
 
     @Override
     public boolean matches(Dependency dependency) {
-        return matches(graphRoot.getGroupId(), dependency.getGroupId())
-                && matches(graphRoot.getArtifactId(), dependency.getArtifactId())
-                && matches(graphRoot.getType(), dependency.getType());
-    }
-
-    private static boolean matches(String filterValue, String nodeValue) {
-        return filterValue == null || filterValue.equals(nodeValue);
+        return Objects.equals(graphRoot.getGroupId(), dependency.getGroupId())
+                && Objects.equals(graphRoot.getArtifactId(), dependency.getArtifactId());
     }
 }

src/main/java/org/apache/maven/plugins/dependency/fromDependencies/OrDependencyMatcher.java

@@ -28,7 +28,14 @@ class OrDependencyMatcher implements DependencyMatcher {
     private Collection<DependencyMatcher> matchers;
 
     OrDependencyMatcher(Collection<DependencyMatcher> matchers) {
-        this.matchers = new ArrayList<>(matchers);
+        this.matchers = new ArrayList<>(validate(matchers));
+    }
+
+    private Collection<DependencyMatcher> validate(Collection<DependencyMatcher> matchers) {
+        if (matchers == null || matchers.isEmpty()) {
+            throw new IllegalArgumentException("There must be at least 1 dependencyMatcher");
+        }
+        return matchers;
     }
 
     @Override

src/test/java/org/apache/maven/plugins/dependency/fromDependencies/GraphRootMatcherTest.java

@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.plugins.dependency.fromDependencies;
+
+import java.util.stream.Stream;
+
+import org.apache.maven.model.Dependency;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.NullSource;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class GraphRootMatcherTest {
+
+    @ParameterizedTest
+    @NullSource
+    void illegalArgument(GraphRoot root) {
+        assertThatThrownBy(() -> new GraphRootMatcher(root)).isInstanceOf(NullPointerException.class);
+    }
+
+    @ParameterizedTest
+    @MethodSource
+    void match(GraphRoot graphRoot, Dependency dependency, boolean result) {
+        assertThat(new GraphRootMatcher(graphRoot).matches(dependency)).isEqualTo(result);
+    }
+
+    static Stream<Arguments> match() {
+        return Stream.of(
+                Arguments.arguments(newGraphRoot("g", "a"), newDependency("g", "a"), true),
+                Arguments.arguments(newGraphRoot("x", "a"), newDependency("g", "a"), false),
+                Arguments.arguments(newGraphRoot("g", "x"), newDependency("g", "a"), false),
+                Arguments.arguments(newGraphRoot("g", "a"), newDependency("x", "a"), false),
+                Arguments.arguments(newGraphRoot("g", "a"), newDependency("g", "x"), false));
+    }
+
+    private static GraphRoot newGraphRoot(String groupId, String artifactId) {
+        GraphRoot graphRoot = new GraphRoot();
+        graphRoot.setGroupId(groupId);
+        graphRoot.setArtifactId(artifactId);
+        return graphRoot;
+    }
+
+    private static Dependency newDependency(String groupId, String artifactId) {
+        Dependency dependency = new Dependency();
+        dependency.setGroupId(groupId);
+        dependency.setArtifactId(artifactId);
+        return dependency;
+    }
+}

src/test/java/org/apache/maven/plugins/dependency/fromDependencies/OrDependencyMatcherTest.java

@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.plugins.dependency.fromDependencies;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.NullAndEmptySource;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class OrDependencyMatcherTest {
+
+    @ParameterizedTest
+    @NullAndEmptySource
+    void illegalArgument(Collection<DependencyMatcher> source) {
+        assertThatThrownBy(() -> new OrDependencyMatcher(source))
+                .hasMessage("There must be at least 1 dependencyMatcher");
+    }
+
+    @ParameterizedTest
+    @CsvSource({
+        "true, true",
+        "false, false",
+    })
+    void singleMatcher(boolean input, boolean result) {
+        Collection<DependencyMatcher> matchers = Arrays.asList(d -> input);
+        assertThat(new OrDependencyMatcher(matchers).matches(null)).isEqualTo(result);
+    }
+
+    @ParameterizedTest
+    @CsvSource({
+        "true,  true,  true",
+        "true,  false, true",
+        "false, true,  true",
+        "false, false, false",
+    })
+    void doubleMatcher(boolean input1, boolean input2, boolean result) {
+        Collection<DependencyMatcher> matchers = Arrays.asList(d -> input1, d -> input2);
+        assertThat(new OrDependencyMatcher(matchers).matches(null)).isEqualTo(result);
+    }
+}