Skip to content

[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
811a237
Make package-private the DefaultDependencyResolverResult constructor …
desruisseaux May 18, 2025
f878a62
Spelling and formatting fixes from comments on the pull request.
desruisseaux May 18, 2025
8e773d5
Changes based on comments in the pull request:
desruisseaux May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ public Severity getSeverity() {
public Optional<DependencyResolverResult> 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) {
Expand Down
39 changes: 27 additions & 12 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,22 @@
@Singleton
public class DefaultDependencyResolver implements DependencyResolver {

/**
* Cache of information about the modules contained in a path element.
*
* <p><b>TODO:</b> 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.</p>
*/
private final PathModularizationCache moduleCache;

/**
* Creates an initially empty resolver.
*/
public DefaultDependencyResolver() {
// TODO: the cache should not be instantiated here, but should rather be session-wide.
moduleCache = new PathModularizationCache();
}

@Nonnull
@Override
public DependencyResolverResult collect(@Nonnull DependencyResolverRequest request)
Expand Down Expand Up @@ -126,7 +142,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);
}
Expand Down Expand Up @@ -171,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<RemoteRepository> repositories = request.getRepositories() != null
? request.getRepositories()
Expand All @@ -191,18 +211,13 @@ public DependencyResolverResult resolve(DependencyResolverRequest request)
.map(Artifact::toCoordinates)
.collect(Collectors.toList());
Predicate<PathType> 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this scope was right. this field concern lives only here. Even could dedicate whole block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change was for removing code duplication. The exact same code was inside the two branches of the if … else statement, with only different variable names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes see.

Common part can be extracted from 'if'

its same obtain the warning:

image 
    @Override
    public DependencyResolverResult resolve(DependencyResolverRequest request)
            throws DependencyResolverException, ArtifactResolverException {
        InternalSession session =
                InternalSession.from(nonNull(request, "request").getSession());
        try {
            DependencyResolverResult collectorResult = collect(request);
            DependencyResolverResult result = collectorResult;
            List<RemoteRepository> repositories = request.getRepositories() != null
                    ? request.getRepositories()
                    : request.getProject().isPresent()
                    ? session.getService(ProjectManager.class)
                    .getRemoteProjectRepositories(
                            request.getProject().get())
                    : session.getRemoteRepositories();
            if (request.getRequestType()!= DependencyResolverRequest.RequestType.COLLECT) {
                List<Node> nodes = flatten(session, collectorResult.getRoot(), request.getPathScope());
                DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult(
                        null, moduleCache(), collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size());
                if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) {
                    for (Node node : nodes) {
                        resolverResult.addNode(node);
                    }
                } else {
                    for (Node node : nodes) {
                        Path path = (node.getArtifact() != null)
                                ? session.getService(ArtifactResolver.class).resolve(session, nodes.stream()
                                        .map(Node::getDependency)
                                        .filter(Objects::nonNull)
                                        .map(Artifact::toCoordinates)
                                        .collect(Collectors.toList()), repositories)
                                .getResult(node.getArtifact().toCoordinates())
                                .getPath()
                                : null;
                        try {
                            resolverResult.addDependency(node, node.getDependency(), request.getPathTypeFilter(), path);
                        } catch (IOException e) {
                            throw cannotReadModuleInfo(path, e);
                        }
                    }
                }
                result = resolverResult;
            }
            return result;
        } finally {
            RequestTraceHelper.exit(RequestTraceHelper.enter(session, request));
        }
    }

is the finally an issue inlining exit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the finally an issue inlining exit ?

Judging by the method names, it would probably not work. RequestTraceHelper.enter probably needs to be invoked before to start the execution of the code inside the try block. Inlining would cause enter to be invoked too late.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx.

null, null, collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size());
for (Node node : nodes) {
flattenResult.addNode(node);
resolverResult.addNode(node);
}
result = flattenResult;
} 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) {
Expand All @@ -217,13 +232,13 @@ public DependencyResolverResult resolve(DependencyResolverRequest request)
throw cannotReadModuleInfo(path, e);
}
}
result = resolverResult;
}
result = resolverResult;
}
return result;
} finally {
RequestTraceHelper.exit(trace);
}
return result;
}

private static DependencyResolverException cannotReadModuleInfo(final Path path, final IOException cause) {
Expand Down
34 changes: 27 additions & 7 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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)}.
*
* <p><b>WARNING: this constructor may be removed in a future Maven release.</b>
* The reason is because {@code DefaultDependencyResolverResult} needs a cache, which should
* preferably be session-wide. How to manage such caches has not yet been clarified.</p>
*
* @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<Exception> exceptions, Node root, int count) {
this(request, new PathModularizationCache(), exceptions, root, count);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this gives dedication and option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this(request, new PathModularizationCache(), exceptions, root, count);
this(request, moduleCache(), exceptions, root, count);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moduleCache() method is defined in another class. We cannot access that method from here.

}

/**
* Creates an initially empty result. Callers should add path elements by calls
* to {@link #addDependency(Node, Dependency, Predicate, Path)}.
Expand All @@ -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<Exception> exceptions,
Node root,
int count) {
this.request = request;
this.cache = cache;
this.cache = Objects.requireNonNull(cache);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or here

Suggested change
this.cache = Objects.requireNonNull(cache);
this.cache = Objects.requireNonNull(moduleCache());

one of the suggestions is right. as moduleCache() is SSOT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moduleCache() method is defined in another class. We cannot access that method from here.

However, this is indeed the kind of code that we would have after we clarified how to manage session-wide caches. The moduleCache() method would be in the class managing those caches. We are just not there yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, session wide informations can be stored in Session.getData().

this.exceptions = exceptions;
this.root = root;
nodes = new ArrayList<>(count);
Expand Down Expand Up @@ -350,7 +370,7 @@ public DependencyResolverRequest getRequest() {

@Override
public List<Exception> getExceptions() {
return exceptions;
return Collections.unmodifiableList(exceptions);
}

@Override
Expand All @@ -360,22 +380,22 @@ public Node getRoot() {

@Override
public List<Node> getNodes() {
return nodes;
return Collections.unmodifiableList(nodes);
}

@Override
public List<Path> getPaths() {
return paths;
return Collections.unmodifiableList(paths);
}

@Override
public Map<PathType, List<Path>> getDispatchedPaths() {
return dispatchedPaths;
return Collections.unmodifiableMap(dispatchedPaths);
}

@Override
public Map<Dependency, Path> getDependencies() {
return dependencies;
return Collections.unmodifiableMap(dependencies);
}

@Override
Expand Down