[MNG-7789] Dependency validation rules used wrong data (#1115)

Refactored and now dropped the unused pluginDescriptor/dependencies validation. Now we have "plugin dependency" (using POM) components to perform dependency validation.

Also, two "inlined" validation from plugin dependencies resolver factored out to components, along with 3 existing moved to newly added (they were wrongly validating descriptor dependencies while intent was to validate "real" dependencies) validators.

---

https://issues.apache.org/jira/browse/MNG-7789

maven-core/src/main/java/org/apache/maven/plugin/internal/AbstractMavenPluginDependenciesValidator.java

@@ -18,9 +18,10 @@
  */
 package org.apache.maven.plugin.internal;
 
-import org.apache.maven.execution.MavenSession;
 import org.apache.maven.plugin.PluginValidationManager;
-import org.apache.maven.plugin.descriptor.MojoDescriptor;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.resolution.ArtifactDescriptorResult;
 
 import static java.util.Objects.requireNonNull;
 
@@ -38,12 +39,17 @@ abstract class AbstractMavenPluginDependenciesValidator implements MavenPluginDe
     }
 
     @Override
-    public void validate(MavenSession mavenSession, MojoDescriptor mojoDescriptor) {
-        if (mojoDescriptor.getPluginDescriptor() != null
-                && mojoDescriptor.getPluginDescriptor().getDependencies() != null) {
-            doValidate(mavenSession, mojoDescriptor);
+    public void validate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult) {
+        if (artifactDescriptorResult.getDependencies() != null) {
+            doValidate(session, pluginArtifact, artifactDescriptorResult);
         }
     }
 
-    protected abstract void doValidate(MavenSession mavenSession, MojoDescriptor mojoDescriptor);
+    protected abstract void doValidate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult);
 }

maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java

@@ -166,9 +166,6 @@ public class DefaultMavenPluginManager implements MavenPluginManager {
     @Requirement
     private List<MavenPluginConfigurationValidator> configurationValidators;
 
-    @Requirement
-    private List<MavenPluginDependenciesValidator> dependenciesValidators;
-
     @Requirement
     private PluginValidationManager pluginValidationManager;
 
@@ -556,10 +553,6 @@ public class DefaultMavenPluginManager implements MavenPluginManager {
                         "Mojo implements `Contextualizable` interface from Plexus Container, which is EOL.");
             }
 
-            for (MavenPluginDependenciesValidator validator : dependenciesValidators) {
-                validator.validate(session, mojoDescriptor);
-            }
-
             Xpp3Dom dom = mojoExecution.getConfiguration();
 
             PlexusConfiguration pomConfiguration;

maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java

@@ -23,8 +23,6 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Collectors;
 
 import org.apache.maven.RepositoryUtils;
 import org.apache.maven.model.Dependency;
@@ -84,6 +82,9 @@ public class DefaultPluginDependenciesResolver implements PluginDependenciesReso
     @Requirement
     private PluginValidationManager pluginValidationManager;
 
+    @Requirement
+    private List<MavenPluginDependenciesValidator> dependenciesValidators;
+
     private Artifact toArtifact(Plugin plugin, RepositorySystemSession session) {
         return new DefaultArtifact(
                 plugin.getGroupId(),
@@ -109,34 +110,8 @@ public class DefaultPluginDependenciesResolver implements PluginDependenciesReso
             request.setTrace(trace);
             ArtifactDescriptorResult result = repoSystem.readArtifactDescriptor(pluginSession, request);
 
-            if (result.getDependencies() != null) {
-                for (org.eclipse.aether.graph.Dependency dependency : result.getDependencies()) {
-                    if ("org.apache.maven".equals(dependency.getArtifact().getGroupId())
-                            && "maven-compat".equals(dependency.getArtifact().getArtifactId())
-                            && !JavaScopes.TEST.equals(dependency.getScope())) {
-                        pluginValidationManager.reportPluginValidationIssue(
-                                session,
-                                pluginArtifact,
-                                "Plugin depends on the deprecated Maven 2.x compatibility layer, which may not be supported in Maven 4.x");
-                    }
-                }
-
-                Set<String> mavenArtifacts = result.getDependencies().stream()
-                        .filter(d -> !JavaScopes.PROVIDED.equals(d.getScope()) && !JavaScopes.TEST.equals(d.getScope()))
-                        .map(org.eclipse.aether.graph.Dependency::getArtifact)
-                        .filter(a -> "org.apache.maven".equals(a.getGroupId()))
-                        .filter(a -> !MavenPluginDependenciesValidator.EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA.contains(
-                                a.getGroupId() + ":" + a.getArtifactId()))
-                        .filter(a -> a.getVersion().startsWith("3."))
-                        .map(a -> a.getGroupId() + ":" + a.getArtifactId() + ":" + a.getVersion())
-                        .collect(Collectors.toSet());
-
-                if (!mavenArtifacts.isEmpty()) {
-                    pluginValidationManager.reportPluginValidationIssue(
-                            session,
-                            pluginArtifact,
-                            "Plugin should declare these Maven artifacts in `provided` scope: " + mavenArtifacts);
-                }
+            for (MavenPluginDependenciesValidator dependenciesValidator : dependenciesValidators) {
+                dependenciesValidator.validate(session, pluginArtifact, result);
             }
 
             pluginArtifact = result.getArtifact();

maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginValidationManager.java

@@ -22,11 +22,7 @@ import javax.inject.Named;
 import javax.inject.Singleton;
 
 import java.io.File;
-import java.util.Arrays;
-import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
-import java.util.Locale;
-import java.util.Map;
+import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.maven.eventspy.AbstractEventSpy;
@@ -46,6 +42,13 @@ import org.slf4j.LoggerFactory;
 @Singleton
 @Named
 public final class DefaultPluginValidationManager extends AbstractEventSpy implements PluginValidationManager {
+    /**
+     * The collection of "G:A" combinations that do NOT belong to Maven Core, hence, should be excluded from
+     * "expected in provided scope" type of checks.
+     */
+    static final Collection<String> EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA =
+            Collections.unmodifiableCollection(Arrays.asList(
+                    "org.apache.maven:maven-archiver", "org.apache.maven:maven-jxr", "org.apache.maven:plexus-utils"));
 
     private static final String ISSUES_KEY = DefaultPluginValidationManager.class.getName() + ".issues";
 

maven-core/src/main/java/org/apache/maven/plugin/internal/Maven2DependenciesValidator.java

@@ -25,10 +25,11 @@ import javax.inject.Singleton;
 import java.util.Set;
 import java.util.stream.Collectors;
 
-import org.apache.maven.execution.MavenSession;
 import org.apache.maven.plugin.PluginValidationManager;
-import org.apache.maven.plugin.descriptor.MojoDescriptor;
-import org.codehaus.plexus.component.repository.ComponentDependency;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.graph.Dependency;
+import org.eclipse.aether.resolution.ArtifactDescriptorResult;
 
 /**
  * Detects Maven2 plugins.
@@ -45,19 +46,22 @@ class Maven2DependenciesValidator extends AbstractMavenPluginDependenciesValidat
     }
 
     @Override
-    protected void doValidate(MavenSession mavenSession, MojoDescriptor mojoDescriptor) {
-        Set<String> maven2Versions = mojoDescriptor.getPluginDescriptor().getDependencies().stream()
+    protected void doValidate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult) {
+        Set<String> maven2Versions = artifactDescriptorResult.getDependencies().stream()
+                .map(Dependency::getArtifact)
                 .filter(d -> "org.apache.maven".equals(d.getGroupId()))
-                .filter(d -> !EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA.contains(d.getGroupId() + ":" + d.getArtifactId()))
-                .map(ComponentDependency::getVersion)
+                .filter(d -> !DefaultPluginValidationManager.EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA.contains(
+                        d.getGroupId() + ":" + d.getArtifactId()))
+                .map(Artifact::getVersion)
                 .filter(v -> v.startsWith("2."))
                 .collect(Collectors.toSet());
 
         if (!maven2Versions.isEmpty()) {
             pluginValidationManager.reportPluginValidationIssue(
-                    mavenSession,
-                    mojoDescriptor,
-                    "Plugin is a Maven 2.x plugin, which will be not supported in Maven 4.x");
+                    session, pluginArtifact, "Plugin is a Maven 2.x plugin, which will be not supported in Maven 4.x");
         }
     }
 }

maven-core/src/main/java/org/apache/maven/plugin/internal/Maven3CompatDependenciesValidator.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.plugin.internal;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.plugin.PluginValidationManager;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.resolution.ArtifactDescriptorResult;
+import org.eclipse.aether.util.artifact.JavaScopes;
+
+/**
+ * Detects Maven3 plugins using maven-compat Maven2 compatibility layer.
+ *
+ * @since 3.9.3
+ */
+@Singleton
+@Named
+class Maven3CompatDependenciesValidator extends AbstractMavenPluginDependenciesValidator {
+
+    @Inject
+    Maven3CompatDependenciesValidator(PluginValidationManager pluginValidationManager) {
+        super(pluginValidationManager);
+    }
+
+    @Override
+    protected void doValidate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult) {
+        for (org.eclipse.aether.graph.Dependency dependency : artifactDescriptorResult.getDependencies()) {
+            if ("org.apache.maven".equals(dependency.getArtifact().getGroupId())
+                    && "maven-compat".equals(dependency.getArtifact().getArtifactId())
+                    && !JavaScopes.TEST.equals(dependency.getScope())) {
+                pluginValidationManager.reportPluginValidationIssue(
+                        session,
+                        pluginArtifact,
+                        "Plugin depends on the deprecated Maven 2.x compatibility layer, which will be not supported in Maven 4.x");
+            }
+        }
+    }
+}

maven-core/src/main/java/org/apache/maven/plugin/internal/MavenMixedDependenciesValidator.java

@@ -25,10 +25,11 @@ import javax.inject.Singleton;
 import java.util.Set;
 import java.util.stream.Collectors;
 
-import org.apache.maven.execution.MavenSession;
 import org.apache.maven.plugin.PluginValidationManager;
-import org.apache.maven.plugin.descriptor.MojoDescriptor;
-import org.codehaus.plexus.component.repository.ComponentDependency;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.graph.Dependency;
+import org.eclipse.aether.resolution.ArtifactDescriptorResult;
 
 /**
  * Detects mixed Maven versions in plugins.
@@ -45,16 +46,21 @@ class MavenMixedDependenciesValidator extends AbstractMavenPluginDependenciesVal
     }
 
     @Override
-    protected void doValidate(MavenSession mavenSession, MojoDescriptor mojoDescriptor) {
-        Set<String> mavenVersions = mojoDescriptor.getPluginDescriptor().getDependencies().stream()
+    protected void doValidate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult) {
+        Set<String> mavenVersions = artifactDescriptorResult.getDependencies().stream()
+                .map(Dependency::getArtifact)
                 .filter(d -> "org.apache.maven".equals(d.getGroupId()))
-                .filter(d -> !EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA.contains(d.getGroupId() + ":" + d.getArtifactId()))
-                .map(ComponentDependency::getVersion)
+                .filter(d -> !DefaultPluginValidationManager.EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA.contains(
+                        d.getGroupId() + ":" + d.getArtifactId()))
+                .map(Artifact::getVersion)
                 .collect(Collectors.toSet());
 
         if (mavenVersions.size() > 1) {
             pluginValidationManager.reportPluginValidationIssue(
-                    mavenSession, mojoDescriptor, "Plugin mixes multiple Maven versions: " + mavenVersions);
+                    session, pluginArtifact, "Plugin mixes multiple Maven versions: " + mavenVersions);
         }
     }
 }

maven-core/src/main/java/org/apache/maven/plugin/internal/MavenPluginDependenciesValidator.java

@@ -18,31 +18,21 @@
  */
 package org.apache.maven.plugin.internal;
 
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-
-import org.apache.maven.execution.MavenSession;
-import org.apache.maven.plugin.descriptor.MojoDescriptor;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.resolution.ArtifactDescriptorResult;
 
 /**
  * Service responsible for validating plugin dependencies.
  *
- * @since 3.9.2
+ * @since 3.9.3
  */
 interface MavenPluginDependenciesValidator {
-
-    /**
-     * The collection of "G:A" combinations that do NOT belong to Maven Core, hence, should be excluded from
-     * "expected in provided scope" type of checks.
-     *
-     * @since 3.9.3
-     */
-    Collection<String> EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA = Collections.unmodifiableCollection(Arrays.asList(
-            "org.apache.maven:maven-archiver", "org.apache.maven:maven-jxr", "org.apache.maven:plexus-utils"));
-
     /**
      * Checks mojo dependency issues.
      */
-    void validate(MavenSession mavenSession, MojoDescriptor mojoDescriptor);
+    void validate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult);
 }

maven-core/src/main/java/org/apache/maven/plugin/internal/MavenPluginDescriptorDependenciesValidator.java

@@ -1,72 +0,0 @@
-/*
- * 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.plugin.internal;
-
-import javax.inject.Inject;
-import javax.inject.Named;
-import javax.inject.Singleton;
-
-import java.util.Set;
-import java.util.stream.Collectors;
-
-import org.apache.maven.execution.MavenSession;
-import org.apache.maven.plugin.PluginValidationManager;
-import org.apache.maven.plugin.descriptor.MojoDescriptor;
-
-/**
- * Detects presence of unwanted Maven3 artifacts in plugin descriptor, possibly caused by multitude of reasons, among
- * them is "wrong scope" dependency declaration as well.
- * <p>
- * Historically, this class was named as "MavenScopeDependenciesValidator" due original intent to check "wrong Maven
- * Artifact scopes". Since then, it turned out that the values validated (the plugin descriptor dependencies, that is
- * produced at plugin build time by maven-plugin-plugin) may be off (for example due maven-plugin-plugin bug), and
- * is potentially not inline with "reality" (actual plugin dependencies).
- * <p>
- * The original intent related check is moved to
- * {@link DefaultPluginDependenciesResolver#resolve(org.apache.maven.model.Plugin, java.util.List, org.eclipse.aether.RepositorySystemSession)}
- * method instead.
- *
- * @since 3.9.3
- */
-@Singleton
-@Named
-class MavenPluginDescriptorDependenciesValidator extends AbstractMavenPluginDependenciesValidator {
-
-    @Inject
-    MavenPluginDescriptorDependenciesValidator(PluginValidationManager pluginValidationManager) {
-        super(pluginValidationManager);
-    }
-
-    @Override
-    protected void doValidate(MavenSession mavenSession, MojoDescriptor mojoDescriptor) {
-        Set<String> mavenArtifacts = mojoDescriptor.getPluginDescriptor().getDependencies().stream()
-                .filter(d -> "org.apache.maven".equals(d.getGroupId()))
-                .filter(d -> !EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA.contains(d.getGroupId() + ":" + d.getArtifactId()))
-                .filter(d -> d.getVersion().startsWith("3."))
-                .map(d -> d.getGroupId() + ":" + d.getArtifactId() + ":" + d.getVersion())
-                .collect(Collectors.toSet());
-
-        if (!mavenArtifacts.isEmpty()) {
-            pluginValidationManager.reportPluginValidationIssue(
-                    mavenSession,
-                    mojoDescriptor,
-                    "Plugin descriptor should not contain these Maven artifacts: " + mavenArtifacts);
-        }
-    }
-}

maven-core/src/main/java/org/apache/maven/plugin/internal/MavenScopeDependenciesValidator.java

@@ -0,0 +1,71 @@
+/*
+ * 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.plugin.internal;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.maven.plugin.PluginValidationManager;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.resolution.ArtifactDescriptorResult;
+import org.eclipse.aether.util.artifact.JavaScopes;
+
+/**
+ * Detects Maven3 dependencies scope.
+ *
+ * @since 3.9.3
+ */
+@Singleton
+@Named
+class MavenScopeDependenciesValidator extends AbstractMavenPluginDependenciesValidator {
+
+    @Inject
+    MavenScopeDependenciesValidator(PluginValidationManager pluginValidationManager) {
+        super(pluginValidationManager);
+    }
+
+    @Override
+    protected void doValidate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult) {
+        Set<String> mavenArtifacts = artifactDescriptorResult.getDependencies().stream()
+                .filter(d -> !JavaScopes.PROVIDED.equals(d.getScope()) && !JavaScopes.TEST.equals(d.getScope()))
+                .map(org.eclipse.aether.graph.Dependency::getArtifact)
+                .filter(a -> "org.apache.maven".equals(a.getGroupId()))
+                .filter(a -> !DefaultPluginValidationManager.EXPECTED_PROVIDED_SCOPE_EXCLUSIONS_GA.contains(
+                        a.getGroupId() + ":" + a.getArtifactId()))
+                .filter(a -> a.getVersion().startsWith("3."))
+                .map(a -> a.getGroupId() + ":" + a.getArtifactId() + ":" + a.getVersion())
+                .collect(Collectors.toSet());
+
+        if (!mavenArtifacts.isEmpty()) {
+            pluginValidationManager.reportPluginValidationIssue(
+                    session,
+                    pluginArtifact,
+                    "Plugin should declare Maven artifacts in `provided` scope. If the plugin already declares them in `provided` scope, update the maven-plugin-plugin to latest version. Artifacts found with wrong scope: "
+                            + mavenArtifacts);
+        }
+    }
+}

maven-core/src/main/java/org/apache/maven/plugin/internal/PlexusContainerDefaultDependenciesValidator.java

@@ -22,9 +22,10 @@ import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
-import org.apache.maven.execution.MavenSession;
 import org.apache.maven.plugin.PluginValidationManager;
-import org.apache.maven.plugin.descriptor.MojoDescriptor;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.resolution.ArtifactDescriptorResult;
 
 /**
  * Detects Plexus Container Default in plugins.
@@ -40,14 +41,17 @@ class PlexusContainerDefaultDependenciesValidator extends AbstractMavenPluginDep
         super(pluginValidationManager);
     }
 
-    protected void doValidate(MavenSession mavenSession, MojoDescriptor mojoDescriptor) {
-        boolean pcdPresent = mojoDescriptor.getPluginDescriptor().getDependencies().stream()
-                .filter(d -> "org.codehaus.plexus".equals(d.getGroupId()))
-                .anyMatch(d -> "plexus-container-default".equals(d.getArtifactId()));
+    protected void doValidate(
+            RepositorySystemSession session,
+            Artifact pluginArtifact,
+            ArtifactDescriptorResult artifactDescriptorResult) {
+        boolean pcdPresent = artifactDescriptorResult.getDependencies().stream()
+                .filter(d -> "org.codehaus.plexus".equals(d.getArtifact().getGroupId()))
+                .anyMatch(d -> "plexus-container-default".equals(d.getArtifact().getArtifactId()));
 
         if (pcdPresent) {
             pluginValidationManager.reportPluginValidationIssue(
-                    mavenSession, mojoDescriptor, "Plugin depends on plexus-container-default, which is EOL");
+                    session, pluginArtifact, "Plugin depends on plexus-container-default, which is EOL");
         }
     }
 }