gnodet-bot commented on code in PR #13350:
URL: https://github.com/apache/maven/pull/13350#discussion_r4187292732
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -307,4 +327,56 @@ private DependencyResult resolveInternal(
RequestTraceHelper.exit(trace);
}
}
+
+ @SuppressWarnings("unchecked")
+ private List<org.eclipse.aether.graph.Dependency>
getManagedDependencies(RepositorySystemSession session) {
+ if (coreExports == null) {
+ return Collections.emptyList();
+ }
+ return (List<org.eclipse.aether.graph.Dependency>)
+ session.getData().computeIfAbsent(MANAGED_DEPENDENCIES_KEY,
this::computeManagedDependencies);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency>
computeManagedDependencies() {
+ if (coreExports == null || coreExports.getExportedArtifacts() == null)
{
+ return Collections.emptyList();
+ }
+ List<org.eclipse.aether.graph.Dependency> managed = new ArrayList<>();
+ ClassLoader cl = Thread.currentThread().getContextClassLoader();
Review Comment:
⚠️ **Thread safety — TCCL dependency in cached computation**
`computeManagedDependencies()` reads
`Thread.currentThread().getContextClassLoader()` to locate `pom.properties`
resources. The result is then cached in `SessionData` via `computeIfAbsent`,
meaning only the first invocation's TCCL determines the result for the entire
session.
In Maven's normal lifecycle this is likely fine (the TCCL is the core
realm's classloader), but in edge cases where:
- A plugin or extension runs on a forked thread with a different TCCL
- The first resolution happens during extension loading with a specialized
classloader
…the cached result could be wrong for subsequent calls.
Consider using the classloader from `CoreExports` directly instead of TCCL.
`CoreExports` already holds a reference to the `ClassRealm` (via its `packages`
map). Alternatively, since this class is `@Singleton` and the classloader is
constant for the JVM lifetime, compute it once in the constructor instead of
lazily per-session.
```suggestion
ClassLoader cl = getClass().getClassLoader();
```
This is simpler, deterministic, and correct —
`DefaultPluginDependenciesResolver` lives in `maven-core.jar`, which is on the
same classloader that has visibility to all core artifact `pom.properties`
files.
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -307,4 +327,56 @@ private DependencyResult resolveInternal(
RequestTraceHelper.exit(trace);
}
}
+
+ @SuppressWarnings("unchecked")
+ private List<org.eclipse.aether.graph.Dependency>
getManagedDependencies(RepositorySystemSession session) {
+ if (coreExports == null) {
+ return Collections.emptyList();
+ }
+ return (List<org.eclipse.aether.graph.Dependency>)
+ session.getData().computeIfAbsent(MANAGED_DEPENDENCIES_KEY,
this::computeManagedDependencies);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency>
computeManagedDependencies() {
Review Comment:
💡 **Javadoc missing on non-trivial logic**
This method builds managed dependencies from core exports by introspecting
`pom.properties` files on the classpath. The rationale — preventing plugins
from pulling their own copies of core artifacts — deserves a comment explaining
*why* `provided` scope is used and how it interacts with the
`ScopeDependencyFilter("provided", "test")` in `resolveInternal()`. Without
this, the next person to read this code will need to reverse-engineer the
intent.
Minimally:
```suggestion
/**
* Computes managed dependencies for core artifacts exported by Maven.
* Each managed dependency is set to {@code provided} scope so that the
* {@link org.eclipse.aether.util.filter.ScopeDependencyFilter} in
* {@link #resolveInternal} excludes them from the plugin classpath —
they are
* already available through the core classloader.
*/
private List<org.eclipse.aether.graph.Dependency>
computeManagedDependencies() {
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -75,17 +81,30 @@
public class DefaultPluginDependenciesResolver implements
PluginDependenciesResolver {
private static final String REPOSITORY_CONTEXT =
org.apache.maven.api.services.RequestTrace.CONTEXT_PLUGIN;
+ private static final String MANAGED_DEPENDENCIES_KEY =
+ DefaultPluginDependenciesResolver.class.getName() +
".managedDependencies";
+
private final Logger logger = LoggerFactory.getLogger(getClass());
private final RepositorySystem repoSystem;
private final List<MavenPluginDependenciesValidator>
dependenciesValidators;
- @Inject
+ private final CoreExports coreExports;
+
public DefaultPluginDependenciesResolver(
RepositorySystem repoSystem,
List<MavenPluginDependenciesValidator> dependenciesValidators) {
+ this(repoSystem, dependenciesValidators, null);
+ }
+
+ @Inject
Review Comment:
💡 **Two-arg constructor should document its purpose**
This constructor exists for backward compatibility (tests, or code that
doesn't use DI). That intent isn't obvious without a Javadoc comment or at
least an annotation.
```suggestion
/**
* Backward-compatible constructor without {@link CoreExports}.
* Core managed dependencies are not injected in this mode.
*/
public DefaultPluginDependenciesResolver(
RepositorySystem repoSystem,
List<MavenPluginDependenciesValidator> dependenciesValidators) {
this(repoSystem, dependenciesValidators, null);
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -307,4 +327,56 @@ private DependencyResult resolveInternal(
RequestTraceHelper.exit(trace);
}
}
+
+ @SuppressWarnings("unchecked")
+ private List<org.eclipse.aether.graph.Dependency>
getManagedDependencies(RepositorySystemSession session) {
+ if (coreExports == null) {
+ return Collections.emptyList();
+ }
+ return (List<org.eclipse.aether.graph.Dependency>)
+ session.getData().computeIfAbsent(MANAGED_DEPENDENCIES_KEY,
this::computeManagedDependencies);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency>
computeManagedDependencies() {
+ if (coreExports == null || coreExports.getExportedArtifacts() == null)
{
+ return Collections.emptyList();
+ }
+ List<org.eclipse.aether.graph.Dependency> managed = new ArrayList<>();
+ ClassLoader cl = Thread.currentThread().getContextClassLoader();
+ if (cl == null) {
+ cl = getClass().getClassLoader();
+ }
+ for (String ga : coreExports.getExportedArtifacts()) {
+ int idx = ga.indexOf(':');
+ if (idx <= 0 || idx >= ga.length() - 1) {
+ continue;
+ }
+ String groupId = ga.substring(0, idx);
+ String artifactId = ga.substring(idx + 1);
+ String resource = "META-INF/maven/" + groupId + "/" + artifactId +
"/pom.properties";
+ Properties props = new Properties();
+ try (InputStream is = cl.getResourceAsStream(resource)) {
+ if (is != null) {
+ props.load(is);
+ } else if (cl != getClass().getClassLoader()) {
+ try (InputStream fallbackIs =
getClass().getClassLoader().getResourceAsStream(resource)) {
+ if (fallbackIs != null) {
+ props.load(fallbackIs);
+ }
+ }
+ }
+ } catch (IOException e) {
+ logger.debug("Could not read " + resource, e);
+ }
+ String version = props.getProperty("version");
+ if (version != null) {
+ version = version.trim();
+ if (!version.isEmpty() && !version.startsWith("${")) {
+ Artifact artifact = new DefaultArtifact(groupId,
artifactId, "jar", version);
+ managed.add(new
org.eclipse.aether.graph.Dependency(artifact, DependencyScope.PROVIDED.id()));
Review Comment:
🔍 **Hardcoded `"jar"` extension** — core artifacts could have different
packaging types (e.g., `pom` for parent POMs in the exported set). In practice,
`CoreExports.getExportedArtifacts()` likely only contains JARs, but this
assumption is implicit. If a non-jar artifact is exported, this would create a
managed dependency with the wrong type, which wouldn't match the transitive
dependency graph.
Not blocking, but worth a defensive check or a comment explaining why
`"jar"` is always correct here.
##########
impl/maven-core/src/test/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolverTest.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * 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 java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.maven.api.DependencyScope;
+import org.apache.maven.extension.internal.CoreExports;
+import org.apache.maven.impl.InternalSession;
+import org.apache.maven.model.Plugin;
+import org.codehaus.plexus.classworlds.ClassWorld;
+import org.codehaus.plexus.classworlds.realm.ClassRealm;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.RepositorySystem;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.artifact.ArtifactType;
+import org.eclipse.aether.artifact.ArtifactTypeRegistry;
+import org.eclipse.aether.artifact.DefaultArtifact;
+import org.eclipse.aether.collection.CollectRequest;
+import org.eclipse.aether.collection.CollectResult;
+import org.eclipse.aether.graph.Dependency;
+import org.eclipse.aether.graph.DependencyFilter;
+import org.eclipse.aether.graph.DependencyNode;
+import org.eclipse.aether.resolution.DependencyRequest;
+import org.eclipse.aether.resolution.DependencyResult;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class DefaultPluginDependenciesResolverTest {
+
+ @Test
+ void testResolvePluginInjectsCoreManagedDependencies() throws Exception {
+ RepositorySystem repoSystem = mock(RepositorySystem.class);
+ List<MavenPluginDependenciesValidator> validators =
Collections.emptyList();
+
+ ClassWorld classWorld = new ClassWorld();
+ ClassRealm classRealm = classWorld.newRealm("test.core",
getClass().getClassLoader());
+
+ Set<String> exportedArtifacts = new LinkedHashSet<>();
+ exportedArtifacts.add("org.apache.maven:maven-core");
+ exportedArtifacts.add("org.apache.maven.test:dummy-missing");
+
+ CoreExports coreExports = new CoreExports(classRealm,
exportedArtifacts, Collections.emptySet());
+
+ DefaultPluginDependenciesResolver resolver =
+ new DefaultPluginDependenciesResolver(repoSystem, validators,
coreExports);
+
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("org.apache.maven.plugins");
+ plugin.setArtifactId("maven-compiler-plugin");
+ plugin.setVersion("3.13.0");
+
+ Artifact pluginArtifact =
+ new DefaultArtifact("org.apache.maven.plugins",
"maven-compiler-plugin", "jar", "3.13.0");
+
+ ArtifactTypeRegistry typeRegistry = mock(ArtifactTypeRegistry.class);
+ ArtifactType pluginType = mock(ArtifactType.class);
+ when(pluginType.getExtension()).thenReturn("jar");
+ when(typeRegistry.get("maven-plugin")).thenReturn(pluginType);
+
+ DefaultRepositorySystemSession session = new
DefaultRepositorySystemSession();
+ session.setArtifactTypeRegistry(typeRegistry);
+ InternalSession internalSession = mock(InternalSession.class);
+ InternalSession.associate(session, internalSession);
+
+ DependencyNode rootNode = mock(DependencyNode.class);
+ CollectResult collectResult = new CollectResult(new CollectRequest());
+ collectResult.setRoot(rootNode);
+ when(repoSystem.collectDependencies(any(),
any(CollectRequest.class))).thenReturn(collectResult);
+
+ DependencyResult dependencyResult = new DependencyResult(new
DependencyRequest());
+ when(repoSystem.resolveDependencies(any(),
any(DependencyRequest.class)))
+ .thenReturn(dependencyResult);
+
+ DependencyFilter filter = mock(DependencyFilter.class);
+
+ resolver.resolvePluginAndFlatten(plugin, pluginArtifact, filter,
Collections.emptyList(), session);
+
+ ArgumentCaptor<CollectRequest> requestCaptor =
ArgumentCaptor.forClass(CollectRequest.class);
+ verify(repoSystem).collectDependencies(any(), requestCaptor.capture());
+
+ CollectRequest captured = requestCaptor.getValue();
+ assertNotNull(captured.getManagedDependencies());
+
+ List<Dependency> managedDeps = captured.getManagedDependencies();
+ assertFalse(managedDeps.isEmpty(), "Managed dependencies should not be
empty");
Review Comment:
💡 **Test verifies plumbing but not behavior**
Both tests verify that managed dependencies are (or aren't) passed to
`CollectRequest`, which is correct unit-level verification. However, there's no
test that verifies the *effect* — i.e., that a plugin pulling `maven-core`
transitively actually gets it excluded from resolution when managed deps are
active.
An integration-level test (even a more detailed unit test with a real
dependency graph mock) would catch regressions in the interaction between
managed deps and the `ScopeDependencyFilter`. Consider adding one.
Also: the test creates a `DefaultRepositorySystemSession` directly — if the
`SessionData` implementation changes, this test could silently pass while
production breaks.
--
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]
