gnodet-bot commented on code in PR #13378:
URL: https://github.com/apache/maven/pull/13378#discussion_r4208018698
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -163,6 +163,7 @@ public DependencyResolutionResult
resolve(DependencyResolutionRequest request)
try {
collect.setTrace(RequestTrace.newChild(trace, depRequest));
node = repoSystem.collectDependencies(session, collect).getRoot();
+ warnAboutObsoleteExclusions(session, collect);
Review Comment:
⚠️ **Behavioral regression:** `warnAboutObsoleteExclusions` is called inside
the `try` block before `result.setDependencyGraph(node)`. If the diagnostic
resolution throws `DependencyCollectionException`, the `catch` block overwrites
the result with the diagnostic exception's root — silently discarding the real
(successful) dependency graph.
This must be moved **after** `result.setDependencyGraph(node)` and its
exception must be caught independently, never allowed to propagate.
```suggestion
node = repoSystem.collectDependencies(session,
collect).getRoot();
result.setDependencyGraph(node);
warnAboutObsoleteExclusions(session, collect);
```
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -216,4 +217,55 @@ private void process(DefaultDependencyResolutionResult
result, Collection<Artifa
}
}
}
+
+ private void warnAboutObsoleteExclusions(RepositorySystemSession session,
CollectRequest collect)
+ throws DependencyCollectionException {
Review Comment:
🔴 **`throws DependencyCollectionException` must be removed.** This is an
advisory diagnostic — it must never propagate a checked exception. If the
diagnostic resolution fails, log at DEBUG and continue. Propagating here makes
the caller responsible for handling failures in what is supposed to be a
best-effort warning.
```suggestion
private void warnAboutObsoleteExclusions(RepositorySystemSession
session, CollectRequest collect) {
```
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -216,4 +217,55 @@ private void process(DefaultDependencyResolutionResult
result, Collection<Artifa
}
}
}
+
+ private void warnAboutObsoleteExclusions(RepositorySystemSession session,
CollectRequest collect)
+ throws DependencyCollectionException {
+
+ if (!logger.isWarnEnabled()) {
+ return;
+ }
+
+ for (org.eclipse.aether.graph.Dependency dependency :
collect.getDependencies()) {
+ if (dependency.getExclusions() == null ||
dependency.getExclusions().isEmpty()) {
+ continue;
+ }
+
+ CollectRequest diagnosticCollect = new CollectRequest();
+ diagnosticCollect.setRequestContext(collect.getRequestContext());
+ diagnosticCollect.setRepositories(collect.getRepositories());
+
diagnosticCollect.setManagedDependencies(collect.getManagedDependencies());
+ diagnosticCollect.addDependency(dependency.setExclusions(null));
+
+ DependencyNode diagnosticRoot =
+ repoSystem.collectDependencies(session,
diagnosticCollect).getRoot();
+
+ checkObsoleteExclusions(dependency, diagnosticRoot);
Review Comment:
🔴 **Compilation failure:** `checkObsoleteExclusions(dependency,
diagnosticRoot)` is called here but this method is never defined in the file.
The defined method is `containsExcludedDependency(DependencyNode, Exclusion)`.
The call site and method name got out of sync — the PR cannot compile as
submitted.
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -216,4 +217,55 @@ private void process(DefaultDependencyResolutionResult
result, Collection<Artifa
}
}
}
+
+ private void warnAboutObsoleteExclusions(RepositorySystemSession session,
CollectRequest collect)
+ throws DependencyCollectionException {
+
+ if (!logger.isWarnEnabled()) {
+ return;
+ }
+
+ for (org.eclipse.aether.graph.Dependency dependency :
collect.getDependencies()) {
+ if (dependency.getExclusions() == null ||
dependency.getExclusions().isEmpty()) {
+ continue;
+ }
+
+ CollectRequest diagnosticCollect = new CollectRequest();
+ diagnosticCollect.setRequestContext(collect.getRequestContext());
+ diagnosticCollect.setRepositories(collect.getRepositories());
+
diagnosticCollect.setManagedDependencies(collect.getManagedDependencies());
+ diagnosticCollect.addDependency(dependency.setExclusions(null));
Review Comment:
💡 Prefer `Collections.emptyList()` over `null` for `setExclusions`. While
current Aether handles null, passing an empty list is clearer in intent and
safer across versions.
```suggestion
diagnosticCollect.addDependency(dependency.setExclusions(Collections.emptyList()));
```
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng12428ObsoleteExclusionTest.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.it;
+
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+
+
+/**
+ * This is a test set for <a
href="https://issues.apache.org/jira/browse/MNG-12428";>MNG-12428</a>.
+ */
+public class MavenITmng12428ObsoleteExclusionTest extends
AbstractMavenIntegrationTestCase {
+
+ /**
+ * Test that Maven warns when a dependency exclusion does not exclude
+ * anything from the dependency subtree.
+ *
+ * @throws Exception in case of failure
+ */
+ @Test
+ void testObsoleteExclusionWarning() throws Exception {
+ Path testDir = extractResources("mng-12428");
+
+ Verifier verifier = newVerifier(testDir);
+ verifier.setAutoclean(false);
+ verifier.deleteDirectory("target");
+ verifier.deleteArtifacts("org.apache.maven.its.mng12428");
+ verifier.filterFile("settings-template.xml", "settings.xml");
+ verifier.addCliArgument("-s");
+ verifier.addCliArgument("settings.xml");
+ verifier.addCliArgument("test");
+ verifier.execute();
+
+ verifier.verifyErrorFreeLog();
+ verifier.verifyTextInLog(
+ "exclusion of org.apache.maven.its.mng12428:unused "
+ + "for org.apache.maven.its.mng12428:c");
+
+ verifier.verifyTextNotInLog(
+ "exclusion of org.apache.maven.its.mng12428:b "
+ + "for org.apache.maven.its.mng12428:d");
Review Comment:
🔵 **Mixed indentation:** line 57 uses a tab character (`\t`) followed by
spaces, while the rest of the file uses spaces only. Please fix to use
consistent 4-space indentation.
Also: `verifyTextNotInLog` checks for artifact `d` (`mng12428:d`), but
artifact `d` does not appear in the test POM's dependency list — only `c` is
declared. If `d` was never going to be resolved, this assertion is vacuously
true and tests nothing. Please verify the test resources include artifact `d`
as a transitive dependency of `c`, or update the assertion to test something
meaningful.
```suggestion
verifier.verifyTextNotInLog(
"exclusion of org.apache.maven.its.mng12428:b "
+ "for org.apache.maven.its.mng12428:d");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
