From 811a2376935e573d280a589fcfd2e0877b0ce138 Mon Sep 17 00:00:00 2001 From: Martin Desruisseaux Date: Sun, 18 May 2025 18:41:49 +0200 Subject: [PATCH 1/3] Make package-private the DefaultDependencyResolverResult constructor having a PathModularizationCache argument, and add a public constructor without the cache argument for code in other packages that need to instantiate. The public constructor may be temporary, until we decide in the future where to store session-wide cache. --- .../internal/impl/DefaultProjectBuilder.java | 3 +- .../maven/impl/DefaultDependencyResolver.java | 46 +++++++++++++++---- .../impl/DefaultDependencyResolverResult.java | 24 +++++++++- 3 files changed, 60 insertions(+), 13 deletions(-) diff --git a/impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java b/impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java index b51241642aae..62ae3b9db97d 100644 --- a/impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java +++ b/impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java @@ -189,8 +189,7 @@ public Severity getSeverity() { public Optional getDependencyResolverResult() { return Optional.ofNullable(res.getDependencyResolutionResult()) .map(r -> new DefaultDependencyResolverResult( - // TODO: this should not be null - null, null, r.getCollectionErrors(), session.getNode(r.getDependencyGraph()), 0)); + null, r.getCollectionErrors(), session.getNode(r.getDependencyGraph()), 0)); } }; } catch (ProjectBuildingException e) { diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java index 7bab2446c6f0..b5cb6ba10e55 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java @@ -67,6 +67,21 @@ @Named @Singleton public class DefaultDependencyResolver implements DependencyResolver { + /** + * Cache of information about the modules contained in a path element. + * This cache is created when first needed. It may be never created. + * + *

TODO: This field should not be in this class, because the cache should be global to the session. + * This field exists here only temporarily, until we clarified where to store session-wide caches.

+ * + * @see moduleCache() + */ + private PathModularizationCache moduleCache; + + /** + * Creates an initially empty resolver. + */ + public DefaultDependencyResolver() {} @Nonnull @Override @@ -126,7 +141,11 @@ public DependencyResolverResult collect(@Nonnull DependencyResolverRequest reque final CollectResult result = session.getRepositorySystem().collectDependencies(systemSession, collectRequest); return new DefaultDependencyResolverResult( - null, null, result.getExceptions(), session.getNode(result.getRoot(), request.getVerbose()), 0); + null, + moduleCache(), + result.getExceptions(), + session.getNode(result.getRoot(), request.getVerbose()), + 0); } catch (DependencyCollectionException e) { throw new DependencyResolverException("Unable to collect dependencies", e); } @@ -191,18 +210,14 @@ public DependencyResolverResult resolve(DependencyResolverRequest request) .map(Artifact::toCoordinates) .collect(Collectors.toList()); Predicate filter = request.getPathTypeFilter(); + DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult( + null, moduleCache(), collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size()); if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) { - DefaultDependencyResolverResult flattenResult = new DefaultDependencyResolverResult( - null, null, collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size()); for (Node node : nodes) { - flattenResult.addNode(node); + resolverResult.addNode(node); } - result = flattenResult; + result = resolverResult; } else { - PathModularizationCache cache = - new PathModularizationCache(); // TODO: should be project-wide cache. - DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult( - null, cache, collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size()); ArtifactResolverResult artifactResolverResult = session.getService(ArtifactResolver.class).resolve(session, coordinates, repositories); for (Node node : nodes) { @@ -226,6 +241,19 @@ public DependencyResolverResult resolve(DependencyResolverRequest request) } } + /** + * {@return the cache of information about the modules contained in a path element}. + * + *

TODO: This method should not be in this class, because the cache should be global to the session. + * This method exists here only temporarily, until we clarified where to store session-wide caches.

+ */ + private PathModularizationCache moduleCache() { + if (moduleCache == null) { + moduleCache = new PathModularizationCache(); + } + return moduleCache; + } + private static DependencyResolverException cannotReadModuleInfo(final Path path, final IOException cause) { return new DependencyResolverException("Cannot read module information of " + path, cause); } diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java index fa7ace6e0d05..0f7d12d5e469 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java @@ -27,6 +27,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Predicate; @@ -56,6 +57,7 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult * The corresponding request. */ private final DependencyResolverRequest request; + /** * The exceptions that occurred while building the dependency graph. */ @@ -97,6 +99,24 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult */ private final PathModularizationCache cache; + /** + * Creates an initially empty result with a temporary cache. + * Callers should add path elements by calls to {@link #addDependency(Node, Dependency, Predicate, Path)}. + * + *

WARNING: this constructor may be removed in a future Maven release. + * The reason is because {@code DefaultDependencyResolverResult} needs a cache, which should + * preferably by session-wide. But we have not yet clarified how such caches should be managed.

+ * + * @param request the corresponding request + * @param exceptions the exceptions that occurred while building the dependency graph + * @param root the root node of the dependency graph + * @param count estimated number of dependencies + */ + public DefaultDependencyResolverResult( + DependencyResolverRequest request, List exceptions, Node root, int count) { + this(request, new PathModularizationCache(), exceptions, root, count); + } + /** * Creates an initially empty result. Callers should add path elements by calls * to {@link #addDependency(Node, Dependency, Predicate, Path)}. @@ -107,14 +127,14 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult * @param root the root node of the dependency graph * @param count estimated number of dependencies */ - public DefaultDependencyResolverResult( + DefaultDependencyResolverResult( DependencyResolverRequest request, PathModularizationCache cache, List exceptions, Node root, int count) { this.request = request; - this.cache = cache; + this.cache = Objects.requireNonNull(cache); this.exceptions = exceptions; this.root = root; nodes = new ArrayList<>(count); From f878a62a089cdfcd32835cc5897f46124617a145 Mon Sep 17 00:00:00 2001 From: Martin Desruisseaux Date: Sun, 18 May 2025 20:20:49 +0200 Subject: [PATCH 2/3] Spelling and formatting fixes from comments on the pull request. --- .../org/apache/maven/impl/DefaultDependencyResolver.java | 7 ++++--- .../apache/maven/impl/DefaultDependencyResolverResult.java | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java index b5cb6ba10e55..a4dd77f33e68 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java @@ -67,14 +67,15 @@ @Named @Singleton public class DefaultDependencyResolver implements DependencyResolver { + /** * Cache of information about the modules contained in a path element. * This cache is created when first needed. It may be never created. * *

TODO: This field should not be in this class, because the cache should be global to the session. - * This field exists here only temporarily, until we clarified where to store session-wide caches.

+ * This field exists here only temporarily, until clarified where to store session-wide caches.

* - * @see moduleCache() + * @see #moduleCache() */ private PathModularizationCache moduleCache; @@ -245,7 +246,7 @@ public DependencyResolverResult resolve(DependencyResolverRequest request) * {@return the cache of information about the modules contained in a path element}. * *

TODO: This method should not be in this class, because the cache should be global to the session. - * This method exists here only temporarily, until we clarified where to store session-wide caches.

+ * This method exists here only temporarily, until clarified where to store session-wide caches.

*/ private PathModularizationCache moduleCache() { if (moduleCache == null) { diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java index 0f7d12d5e469..11743f1f22e0 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java @@ -105,7 +105,7 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult * *

WARNING: this constructor may be removed in a future Maven release. * The reason is because {@code DefaultDependencyResolverResult} needs a cache, which should - * preferably by session-wide. But we have not yet clarified how such caches should be managed.

+ * preferably be session-wide. How to manage such caches has not yet been clarified.

* * @param request the corresponding request * @param exceptions the exceptions that occurred while building the dependency graph From 8e773d54ec55b63a5786c3d7c5951d232fd38b6b Mon Sep 17 00:00:00 2001 From: Martin Desruisseaux Date: Tue, 20 May 2025 00:52:23 +0200 Subject: [PATCH 3/3] Changes based on comments in the pull request: - PathModularizationCache is always required, no need for lazy instantiation. - DefaultDependencyResolverResult returns immutable views of collections. --- .../maven/impl/DefaultDependencyResolver.java | 34 ++++++------------- .../impl/DefaultDependencyResolverResult.java | 10 +++--- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java index a4dd77f33e68..67d291328b1b 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java @@ -70,19 +70,19 @@ public class DefaultDependencyResolver implements DependencyResolver { /** * Cache of information about the modules contained in a path element. - * This cache is created when first needed. It may be never created. * *

TODO: This field should not be in this class, because the cache should be global to the session. * This field exists here only temporarily, until clarified where to store session-wide caches.

- * - * @see #moduleCache() */ - private PathModularizationCache moduleCache; + private final PathModularizationCache moduleCache; /** * Creates an initially empty resolver. */ - public DefaultDependencyResolver() {} + public DefaultDependencyResolver() { + // TODO: the cache should not be instantiated here, but should rather be session-wide. + moduleCache = new PathModularizationCache(); + } @Nonnull @Override @@ -143,7 +143,7 @@ public DependencyResolverResult collect(@Nonnull DependencyResolverRequest reque session.getRepositorySystem().collectDependencies(systemSession, collectRequest); return new DefaultDependencyResolverResult( null, - moduleCache(), + moduleCache, result.getExceptions(), session.getNode(result.getRoot(), request.getVerbose()), 0); @@ -191,8 +191,8 @@ public DependencyResolverResult resolve(DependencyResolverRequest request) InternalSession session = InternalSession.from(nonNull(request, "request").getSession()); RequestTraceHelper.ResolverTrace trace = RequestTraceHelper.enter(session, request); + DependencyResolverResult result; try { - DependencyResolverResult result; DependencyResolverResult collectorResult = collect(request); List repositories = request.getRepositories() != null ? request.getRepositories() @@ -212,12 +212,11 @@ public DependencyResolverResult resolve(DependencyResolverRequest request) .collect(Collectors.toList()); Predicate filter = request.getPathTypeFilter(); DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult( - null, moduleCache(), collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size()); + null, moduleCache, collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size()); if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) { for (Node node : nodes) { resolverResult.addNode(node); } - result = resolverResult; } else { ArtifactResolverResult artifactResolverResult = session.getService(ArtifactResolver.class).resolve(session, coordinates, repositories); @@ -233,26 +232,13 @@ public DependencyResolverResult resolve(DependencyResolverRequest request) throw cannotReadModuleInfo(path, e); } } - result = resolverResult; } + result = resolverResult; } - return result; } finally { RequestTraceHelper.exit(trace); } - } - - /** - * {@return the cache of information about the modules contained in a path element}. - * - *

TODO: This method should not be in this class, because the cache should be global to the session. - * This method exists here only temporarily, until clarified where to store session-wide caches.

- */ - private PathModularizationCache moduleCache() { - if (moduleCache == null) { - moduleCache = new PathModularizationCache(); - } - return moduleCache; + return result; } private static DependencyResolverException cannotReadModuleInfo(final Path path, final IOException cause) { diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java index 11743f1f22e0..4b67c9ee32c6 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java @@ -370,7 +370,7 @@ public DependencyResolverRequest getRequest() { @Override public List getExceptions() { - return exceptions; + return Collections.unmodifiableList(exceptions); } @Override @@ -380,22 +380,22 @@ public Node getRoot() { @Override public List getNodes() { - return nodes; + return Collections.unmodifiableList(nodes); } @Override public List getPaths() { - return paths; + return Collections.unmodifiableList(paths); } @Override public Map> getDispatchedPaths() { - return dispatchedPaths; + return Collections.unmodifiableMap(dispatchedPaths); } @Override public Map getDependencies() { - return dependencies; + return Collections.unmodifiableMap(dependencies); } @Override