gnodet-bot commented on code in PR #13378:
URL: https://github.com/apache/maven/pull/13378#discussion_r4209566568
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -216,4 +219,79 @@ private void process(DefaultDependencyResolutionResult
result, Collection<Artifa
}
}
}
+
+ private void warnAboutObsoleteExclusions(RepositorySystemSession session,
CollectRequest collect) {
+
+ if (!logger.isWarnEnabled()) {
+ return;
+ }
+
+ for (org.eclipse.aether.graph.Dependency dependency :
collect.getDependencies()) {
Review Comment:
⚠️ **`collect.getDependencies()` may include inherited/non-actionable
exclusions.**
`collect.getDependencies()` returns all dependencies from the effective
model, which includes dependencies inherited from parent POMs and imported
BOMs. When a warning fires for such an exclusion, the user has no way to act on
it — the exclusion doesn't even appear in their own POM. This would violate
Maven's own warning policy that only actionable warnings should be emitted.
Consider restricting the check to `project.getDependencies()` (the project's
own declared dependencies), or filtering `collect.getDependencies()` to only
include dependencies that are directly declared in the current project's POM
(not inherited).
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -216,4 +219,79 @@ private void process(DefaultDependencyResolutionResult
result, Collection<Artifa
}
}
}
+
+ private void warnAboutObsoleteExclusions(RepositorySystemSession session,
CollectRequest collect) {
+
+ 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(Collections.emptyList()));
+
+ try {
+ DependencyNode diagnosticRoot = repoSystem
+ .collectDependencies(session, diagnosticCollect)
+ .getRoot();
Review Comment:
⚠️ **O(N) resolver round-trips — one `collectDependencies` call per
exclusion-bearing dependency.**
For a project with N dependencies that each have exclusions, this triggers N
full resolver invocations at the end of every build. `collectDependencies` is
expensive: it resolves remote metadata, traverses the full transitive graph,
and applies version management. This can significantly slow down large
multi-module builds.
Consider a single-pass approach: run one diagnostic `collectDependencies`
without any exclusions at all (or with all exclusions stripped), then check all
exclusions against that single resolved tree. This reduces the cost from O(N)
to O(1) resolver round-trips.
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -216,4 +219,79 @@ private void process(DefaultDependencyResolutionResult
result, Collection<Artifa
}
}
}
+
+ private void warnAboutObsoleteExclusions(RepositorySystemSession session,
CollectRequest collect) {
+
+ 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(Collections.emptyList()));
+
+ try {
+ DependencyNode diagnosticRoot = repoSystem
+ .collectDependencies(session, diagnosticCollect)
+ .getRoot();
+
+ checkObsoleteExclusions(dependency, diagnosticRoot);
+ } catch (DependencyCollectionException e) {
+ logger.debug("Could not perform diagnostic resolution for
obsolete-exclusion check", e);
+ }
+ }
+ }
+
+ private void checkObsoleteExclusions(
+ org.eclipse.aether.graph.Dependency dependency, DependencyNode
diagnosticRoot) {
+
+ for (org.eclipse.aether.graph.Exclusion exclusion :
dependency.getExclusions()) {
+ boolean found = containsExcludedDependency(diagnosticRoot,
exclusion);
+
+ if (!found) {
+ logger.warn("exclusion of "
+ + exclusion.getGroupId()
+ + ":"
+ + exclusion.getArtifactId()
+ + " for "
+ + dependency.getArtifact().getGroupId()
+ + ":"
+ + dependency.getArtifact().getArtifactId()
+ + " is obsolete - there is no dependency on this
exclusion");
Review Comment:
💡 **Prefer SLF4J parameterized logging over string concatenation.**
Even with the `isWarnEnabled()` guard at line 225, using parameterized
logging is the idiomatic SLF4J convention and future-proofs against guard
removal:
```suggestion
logger.warn("exclusion of {}:{} for {}:{} is obsolete -
there is no dependency on this exclusion",
exclusion.getGroupId(), exclusion.getArtifactId(),
dependency.getArtifact().getGroupId(),
dependency.getArtifact().getArtifactId());
```
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -216,4 +219,79 @@ private void process(DefaultDependencyResolutionResult
result, Collection<Artifa
}
}
}
+
+ private void warnAboutObsoleteExclusions(RepositorySystemSession session,
CollectRequest collect) {
+
+ 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(Collections.emptyList()));
+
+ try {
+ DependencyNode diagnosticRoot = repoSystem
+ .collectDependencies(session, diagnosticCollect)
+ .getRoot();
+
+ checkObsoleteExclusions(dependency, diagnosticRoot);
+ } catch (DependencyCollectionException e) {
+ logger.debug("Could not perform diagnostic resolution for
obsolete-exclusion check", e);
+ }
+ }
+ }
+
+ private void checkObsoleteExclusions(
Review Comment:
💡 **No unit tests for the core logic methods.**
`warnAboutObsoleteExclusions`, `checkObsoleteExclusions`, and
`containsExcludedDependency` contain non-trivial logic (wildcard matching,
recursive tree traversal) but are tested only via the integration test. Unit
tests would be faster, more targeted, and would cover edge cases (wildcard `*`,
empty tree, multi-level nesting) without the full IT setup overhead.
##########
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 and inconsistent continuation indentation.**
Line 57 uses a tab character (`\t`) followed by spaces, while the rest of
the file uses spaces only. Additionally, the string continuation on lines 58-59
uses a different indentation level than the analogous block at lines 53-55.
```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]
