[MNG-7854] Warn if imported dep is ignored (#1370)

The artificial (or "bolted on") scope "import" behaves wildly differently that rest of Maven, causes surprise to our users. For start, we should emit "actionable" warnings about these, ultimate goal is to align this behaviour with "maven way" of working ("closest occurence wins" vs current weird "direct occurrence then first occurence wins" strategy).

Warnings are only created when the dependency is not directly managed. The warning also suggests to add a direct managed dependency to get rid of it, so it's now helpful and actionnable.

---

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

maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelProblem.java

@@ -136,16 +136,16 @@ public Exception getException() {
 
     @Override
     public String getMessage() {
-        String msg;
+        String msg = null;
 
-        if (message != null && message.length() > 0) {
+        if (message != null && !message.isEmpty()) {
             msg = message;
-        } else {
+        } else if (exception != null) {
             msg = exception.getMessage();
+        }
 
-            if (msg == null) {
-                msg = "";
-            }
+        if (msg == null) {
+            msg = "";
         }
 
         return msg;

maven-model-builder/src/main/java/org/apache/maven/model/composition/DefaultDependencyManagementImporter.java

@@ -21,15 +21,16 @@
 import javax.inject.Named;
 import javax.inject.Singleton;
 
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 import org.apache.maven.api.model.Dependency;
 import org.apache.maven.api.model.DependencyManagement;
+import org.apache.maven.api.model.Exclusion;
 import org.apache.maven.api.model.Model;
 import org.apache.maven.model.building.ModelBuildingRequest;
+import org.apache.maven.model.building.ModelProblem;
 import org.apache.maven.model.building.ModelProblemCollector;
+import org.apache.maven.model.building.ModelProblemCollectorRequest;
 
 /**
  * Handles the import of dependency management from other models into the target model.
@@ -58,15 +59,81 @@ public Model importManagement(
                 depMgmt = DependencyManagement.newInstance();
             }
 
+            Set<String> directDependencies = new HashSet<>(dependencies.keySet());
+
             for (DependencyManagement source : sources) {
                 for (Dependency dependency : source.getDependencies()) {
                     String key = dependency.getManagementKey();
-                    dependencies.putIfAbsent(key, dependency);
+                    Dependency present = dependencies.putIfAbsent(key, dependency);
+                    if (present != null && !equals(dependency, present) && !directDependencies.contains(key)) {
+                        // TODO: https://issues.apache.org/jira/browse/MNG-8004
+                        problems.add(new ModelProblemCollectorRequest(
+                                        ModelProblem.Severity.WARNING, ModelProblem.Version.V40)
+                                .setMessage("Ignored POM import for: " + toString(dependency) + " as already imported "
+                                        + toString(present) + ".  Add a the conflicting managed dependency directly "
+                                        + "to the dependencyManagement section of the POM."));
+                    }
                 }
             }
 
             return target.withDependencyManagement(depMgmt.withDependencies(dependencies.values()));
         }
         return target;
     }
+
+    private String toString(Dependency dependency) {
+        StringBuilder stringBuilder = new StringBuilder();
+        stringBuilder
+                .append(dependency.getGroupId())
+                .append(":")
+                .append(dependency.getArtifactId())
+                .append(":")
+                .append(dependency.getType());
+        if (dependency.getClassifier() != null && !dependency.getClassifier().isEmpty()) {
+            stringBuilder.append(":").append(dependency.getClassifier());
+        }
+        stringBuilder
+                .append(":")
+                .append(dependency.getVersion())
+                .append("@")
+                .append(dependency.getScope() == null ? "compile" : dependency.getScope());
+        if (dependency.isOptional()) {
+            stringBuilder.append("[optional]");
+        }
+        if (!dependency.getExclusions().isEmpty()) {
+            stringBuilder.append("[").append(dependency.getExclusions().size()).append(" exclusions]");
+        }
+        return stringBuilder.toString();
+    }
+
+    private boolean equals(Dependency d1, Dependency d2) {
+        return Objects.equals(d1.getGroupId(), d2.getGroupId())
+                && Objects.equals(d1.getArtifactId(), d2.getArtifactId())
+                && Objects.equals(d1.getVersion(), d2.getVersion())
+                && Objects.equals(d1.getType(), d2.getType())
+                && Objects.equals(d1.getClassifier(), d2.getClassifier())
+                && Objects.equals(d1.getScope(), d2.getScope())
+                && Objects.equals(d1.getSystemPath(), d2.getSystemPath())
+                && Objects.equals(d1.getOptional(), d2.getOptional())
+                && equals(d1.getExclusions(), d2.getExclusions());
+    }
+
+    private boolean equals(Collection<Exclusion> ce1, Collection<Exclusion> ce2) {
+        if (ce1.size() == ce2.size()) {
+            Iterator<Exclusion> i1 = ce1.iterator();
+            Iterator<Exclusion> i2 = ce2.iterator();
+            while (i1.hasNext() && i2.hasNext()) {
+                if (!equals(i1.next(), i2.next())) {
+                    return false;
+                }
+            }
+            return !i1.hasNext() && !i2.hasNext();
+        }
+        return false;
+    }
+
+    private boolean equals(Exclusion e1, Exclusion e2) {
+        return Objects.equals(e1.getGroupId(), e2.getGroupId())
+                && Objects.equals(e1.getArtifactId(), e2.getArtifactId());
+    }
 }

maven-model-builder/src/test/java/org/apache/maven/model/building/DefaultModelBuilderTest.java

@@ -30,8 +30,7 @@
 import org.apache.maven.model.resolution.UnresolvableModelException;
 import org.junit.jupiter.api.Test;
 
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.*;
 
 /**
  */
@@ -163,4 +162,229 @@ void testBuildRawModel() throws Exception {
                 false);
         assertNotNull(res.get());
     }
+
+    /**
+     * This test has
+     *   - a dependency directly managed to 0.2
+     *   - then a BOM import which manages that same dep to 0.1
+     * This <i>currently</i> results in 0.2 and a no warning
+     */
+    @Test
+    void testManagedDependencyBeforeImport() throws Exception {
+        ModelBuilder builder = new DefaultModelBuilderFactory().newInstance();
+        assertNotNull(builder);
+
+        DefaultModelBuildingRequest request = new DefaultModelBuildingRequest();
+        request.setModelSource(new FileModelSource(new File(
+                getClass().getResource("/poms/depmgmt/root-dep-first.xml").getFile())));
+        request.setModelResolver(new BaseModelResolver() {
+            public ModelSource resolveModel(org.apache.maven.model.Dependency dependency)
+                    throws UnresolvableModelException {
+                switch (dependency.getManagementKey()) {
+                    case "test:import:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/import.xml")
+                                .getFile()));
+                    default:
+                        throw new UnresolvableModelException(
+                                "Cannot resolve",
+                                dependency.getGroupId(),
+                                dependency.getArtifactId(),
+                                dependency.getVersion());
+                }
+            }
+        });
+
+        ModelBuildingResult result = builder.build(request);
+        Dependency dep = result.getEffectiveModel().getDelegate().getDependencyManagement().getDependencies().stream()
+                .filter(d -> "test:mydep:jar".equals(d.getManagementKey()))
+                .findFirst()
+                .get();
+        assertEquals("0.2", dep.getVersion());
+        assertEquals(0, result.getProblems().size());
+    }
+
+    /**
+     * This test has
+     *   - a BOM import which manages the dep to 0.1
+     *   - then a directly managed dependency to 0.2
+     * This <i>currently</i> results in 0.2 and no warning
+     */
+    @Test
+    void testManagedDependencyAfterImport() throws Exception {
+        ModelBuilder builder = new DefaultModelBuilderFactory().newInstance();
+        assertNotNull(builder);
+
+        DefaultModelBuildingRequest request = new DefaultModelBuildingRequest();
+        request.setModelSource(new FileModelSource(new File(
+                getClass().getResource("/poms/depmgmt/root-dep-last.xml").getFile())));
+        request.setModelResolver(new BaseModelResolver() {
+            public ModelSource resolveModel(org.apache.maven.model.Dependency dependency)
+                    throws UnresolvableModelException {
+                switch (dependency.getManagementKey()) {
+                    case "test:import:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/import.xml")
+                                .getFile()));
+                    default:
+                        throw new UnresolvableModelException(
+                                "Cannot resolve",
+                                dependency.getGroupId(),
+                                dependency.getArtifactId(),
+                                dependency.getVersion());
+                }
+            }
+        });
+
+        ModelBuildingResult result = builder.build(request);
+        Dependency dep = result.getEffectiveModel().getDelegate().getDependencyManagement().getDependencies().stream()
+                .filter(d -> "test:mydep:jar".equals(d.getManagementKey()))
+                .findFirst()
+                .get();
+        assertEquals("0.2", dep.getVersion());
+        assertEquals(0, result.getProblems().size());
+    }
+
+    /**
+     * This test has
+     *   - a BOM import which manages dep to 0.3
+     *   - another BOM import which manages the dep to 0.1
+     * This <i>currently</i> results in 0.3 (first wins) and a warning
+     */
+    @Test
+    void testManagedDependencyTwoImports() throws Exception {
+        ModelBuilder builder = new DefaultModelBuilderFactory().newInstance();
+        assertNotNull(builder);
+
+        DefaultModelBuildingRequest request = new DefaultModelBuildingRequest();
+        request.setModelSource(new FileModelSource(new File(
+                getClass().getResource("/poms/depmgmt/root-two-imports.xml").getFile())));
+        request.setModelResolver(new BaseModelResolver() {
+            public ModelSource resolveModel(org.apache.maven.model.Dependency dependency)
+                    throws UnresolvableModelException {
+                switch (dependency.getManagementKey()) {
+                    case "test:import:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/import.xml")
+                                .getFile()));
+                    case "test:other:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/other-import.xml")
+                                .getFile()));
+                    default:
+                        throw new UnresolvableModelException(
+                                "Cannot resolve",
+                                dependency.getGroupId(),
+                                dependency.getArtifactId(),
+                                dependency.getVersion());
+                }
+            }
+        });
+
+        ModelBuildingResult result = builder.build(request);
+        Dependency dep = result.getEffectiveModel().getDelegate().getDependencyManagement().getDependencies().stream()
+                .filter(d -> "test:mydep:jar".equals(d.getManagementKey()))
+                .findFirst()
+                .get();
+        assertEquals("0.3", dep.getVersion());
+        assertEquals(1, result.getProblems().size());
+        ModelProblem problem = result.getProblems().get(0);
+        assertTrue(problem.toString().contains("Ignored POM import"));
+    }
+
+    /**
+     * This test has
+     *   - a BOM import which itself imports the junit BOM which manages the dep to 0.1
+     *   - a BOM import to the junit BOM which manages the dep to 0.2
+     * This <i>currently</i> results in 0.1 (first wins, unexpected as this is not the closest) and a warning
+     */
+    @Test
+    void testManagedDependencyDistance() throws Exception {
+        ModelBuilder builder = new DefaultModelBuilderFactory().newInstance();
+        assertNotNull(builder);
+
+        DefaultModelBuildingRequest request = new DefaultModelBuildingRequest();
+        request.setLocationTracking(true);
+        request.setModelSource(new FileModelSource(new File(
+                getClass().getResource("/poms/depmgmt/root-distance.xml").getFile())));
+        request.setModelResolver(new BaseModelResolver() {
+            public ModelSource resolveModel(org.apache.maven.model.Dependency dependency)
+                    throws UnresolvableModelException {
+                switch (dependency.getManagementKey()) {
+                    case "org.junit:bom:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/junit-" + dependency.getVersion() + ".xml")
+                                .getFile()));
+                    case "test:other:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/distant-import.xml")
+                                .getFile()));
+                    default:
+                        throw new UnresolvableModelException(
+                                "Cannot resolve",
+                                dependency.getGroupId(),
+                                dependency.getArtifactId(),
+                                dependency.getVersion());
+                }
+            }
+        });
+
+        ModelBuildingResult result = builder.build(request);
+        Dependency dep = result.getEffectiveModel().getDelegate().getDependencyManagement().getDependencies().stream()
+                .filter(d -> "org.junit:junit:jar".equals(d.getManagementKey()))
+                .findFirst()
+                .get();
+        assertEquals("0.1", dep.getVersion());
+        assertEquals(1, result.getProblems().size());
+        ModelProblem problem = result.getProblems().get(0);
+        assertTrue(problem.toString().contains("Ignored POM import"));
+    }
+
+    /**
+     * This test has
+     *   - a BOM import which itself imports the junit BOM which manages the dep to 0.1
+     *   - a BOM import to the junit BOM which manages the dep to 0.2
+     *   - a direct managed dependency to the dep at 0.3 (as suggested by the warning)
+     * This results in 0.3 (explicit managed wins) and no warning
+     */
+    @Test
+    void testManagedDependencyDistanceWithExplicit() throws Exception {
+        ModelBuilder builder = new DefaultModelBuilderFactory().newInstance();
+        assertNotNull(builder);
+
+        DefaultModelBuildingRequest request = new DefaultModelBuildingRequest();
+        request.setLocationTracking(true);
+        request.setModelSource(new FileModelSource(new File(getClass()
+                .getResource("/poms/depmgmt/root-distance-explicit.xml")
+                .getFile())));
+        request.setModelResolver(new BaseModelResolver() {
+            public ModelSource resolveModel(org.apache.maven.model.Dependency dependency)
+                    throws UnresolvableModelException {
+                switch (dependency.getManagementKey()) {
+                    case "org.junit:bom:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/junit-" + dependency.getVersion() + ".xml")
+                                .getFile()));
+                    case "test:other:pom":
+                        return new FileModelSource(new File(getClass()
+                                .getResource("/poms/depmgmt/distant-import.xml")
+                                .getFile()));
+                    default:
+                        throw new UnresolvableModelException(
+                                "Cannot resolve",
+                                dependency.getGroupId(),
+                                dependency.getArtifactId(),
+                                dependency.getVersion());
+                }
+            }
+        });
+
+        ModelBuildingResult result = builder.build(request);
+        Dependency dep = result.getEffectiveModel().getDelegate().getDependencyManagement().getDependencies().stream()
+                .filter(d -> "org.junit:junit:jar".equals(d.getManagementKey()))
+                .findFirst()
+                .get();
+        assertEquals("0.3", dep.getVersion());
+        assertEquals(0, result.getProblems().size());
+    }
 }

maven-model-builder/src/test/resources/poms/depmgmt/distant-import.xml

@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.1.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.1.0 https://maven.apache.org/xsd/maven-4.1.0.xsd">
+    <modelVersion>4.1.0</modelVersion>
+
+    <groupId>test</groupId>
+    <artifactId>other</artifactId>
+    <version>0.1-SNAPSHOT</version>
+
+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>org.junit</groupId>
+                <artifactId>bom</artifactId>
+                <version>0.1</version>
+                <type>pom</type>
+                <scope>import</scope>
+            </dependency>
+        </dependencies>
+    </dependencyManagement>
+
+</project>